LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/5] KVM: x86: Clean up redundant macro definitions
@ 2021-08-09  9:34 Like Xu
  2021-08-09  9:34 ` [PATCH 1/5] KVM: x86: Clean up redundant mod_64(x, y) macro definition Like Xu
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: Like Xu @ 2021-08-09  9:34 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, Ingo Molnar, kvm, linux-kernel

In KVM/x86 code, there are macros with the same name that are
defined and used separately in the evolving code, and if the scope
of the code review is only on iterations of the patch set, it can be
difficult to spot these fragmented macros being defined repeatedly.

IMO, it's necessary to clean this up to improve the consistency and
readability of the code, and it also helps to avoid software defects
caused by inconsistencies in the scope of influence of macros.

Like Xu (5):
  KVM: x86: Clean up redundant mod_64(x, y) macro definition
  KVM: x86: Clean up redundant CC macro definition
  KVM: x86: Clean up redundant ROL16(val, n) macro definition
  KVM: x86: Clean up redundant __ex(x) macro definition
  KVM: x86: Clean up redundant pr_fmt(fmt) macro definition for svm

 arch/x86/include/asm/kvm_host.h | 2 ++
 arch/x86/kvm/i8254.c            | 6 ------
 arch/x86/kvm/lapic.c            | 6 ------
 arch/x86/kvm/svm/avic.c         | 2 --
 arch/x86/kvm/svm/nested.c       | 4 ----
 arch/x86/kvm/svm/sev.c          | 2 --
 arch/x86/kvm/svm/svm.c          | 4 ----
 arch/x86/kvm/svm/svm.h          | 3 +++
 arch/x86/kvm/vmx/evmcs.c        | 1 -
 arch/x86/kvm/vmx/evmcs.h        | 4 ----
 arch/x86/kvm/vmx/nested.c       | 2 --
 arch/x86/kvm/vmx/vmcs.h         | 2 ++
 arch/x86/kvm/vmx/vmcs12.c       | 1 -
 arch/x86/kvm/vmx/vmcs12.h       | 4 ----
 arch/x86/kvm/vmx/vmx_ops.h      | 2 --
 arch/x86/kvm/x86.h              | 8 ++++++++
 16 files changed, 15 insertions(+), 38 deletions(-)

-- 
2.32.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/5] KVM: x86: Clean up redundant mod_64(x, y) macro definition
  2021-08-09  9:34 [PATCH 0/5] KVM: x86: Clean up redundant macro definitions Like Xu
@ 2021-08-09  9:34 ` Like Xu
  2021-08-09  9:34 ` [PATCH 2/5] KVM: x86: Clean up redundant CC " Like Xu
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Like Xu @ 2021-08-09  9:34 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, Ingo Molnar, kvm, linux-kernel

From: Like Xu <likexu@tencent.com>

The mod_64(x, y) macro is only defined and used in the kvm/x86 context.
It's safe to move the definition of mod_64(x, y) from x86/{i8254lapic}.c
to the generic x86.h without any intended functional change.

Signed-off-by: Like Xu <likexu@tencent.com>
---
 arch/x86/kvm/i8254.c | 6 ------
 arch/x86/kvm/lapic.c | 6 ------
 arch/x86/kvm/x86.h   | 6 ++++++
 3 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index 5a69cce4d72d..81d2ba064dc3 100644
--- a/arch/x86/kvm/i8254.c
+++ b/arch/x86/kvm/i8254.c
@@ -40,12 +40,6 @@
 #include "i8254.h"
 #include "x86.h"
 
-#ifndef CONFIG_X86_64
-#define mod_64(x, y) ((x) - (y) * div64_u64(x, y))
-#else
-#define mod_64(x, y) ((x) % (y))
-#endif
-
 #define RW_STATE_LSB 1
 #define RW_STATE_MSB 2
 #define RW_STATE_WORD0 3
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 76fb00921203..6b3d8feac1d0 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -42,12 +42,6 @@
 #include "cpuid.h"
 #include "hyperv.h"
 
-#ifndef CONFIG_X86_64
-#define mod_64(x, y) ((x) - (y) * div64_u64(x, y))
-#else
-#define mod_64(x, y) ((x) % (y))
-#endif
-
 #define PRId64 "d"
 #define PRIx64 "llx"
 #define PRIu64 "u"
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 44ae10312740..6aac4a901b65 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -8,6 +8,12 @@
 #include "kvm_cache_regs.h"
 #include "kvm_emulate.h"
 
+#ifndef CONFIG_X86_64
+#define mod_64(x, y) ((x) - (y) * div64_u64(x, y))
+#else
+#define mod_64(x, y) ((x) % (y))
+#endif
+
 static __always_inline void kvm_guest_enter_irqoff(void)
 {
 	/*
-- 
2.32.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 2/5] KVM: x86: Clean up redundant CC macro definition
  2021-08-09  9:34 [PATCH 0/5] KVM: x86: Clean up redundant macro definitions Like Xu
  2021-08-09  9:34 ` [PATCH 1/5] KVM: x86: Clean up redundant mod_64(x, y) macro definition Like Xu
@ 2021-08-09  9:34 ` Like Xu
  2021-08-10 17:47   ` Paolo Bonzini
  2021-08-09  9:34 ` [PATCH 3/5] KVM: x86: Clean up redundant ROL16(val, n) " Like Xu
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 11+ messages in thread
From: Like Xu @ 2021-08-09  9:34 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, Ingo Molnar, kvm, linux-kernel

From: Like Xu <likexu@tencent.com>

With the exception of drivers/dma/pl330.c, the CC macro is defined and used
in {svm, vmx}/nested.c, and the KVM_NESTED_VMENTER_CONSISTENCY_CHECK
macro it refers to is defined in x86.h, so it's safe to move it into x86.h
without intended functional changes.

Signed-off-by: Like Xu <likexu@tencent.com>
---
 arch/x86/kvm/svm/nested.c | 2 --
 arch/x86/kvm/vmx/nested.c | 2 --
 arch/x86/kvm/x86.h        | 2 ++
 3 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 5e13357da21e..57c288ba6ef0 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -29,8 +29,6 @@
 #include "lapic.h"
 #include "svm.h"
 
-#define CC KVM_NESTED_VMENTER_CONSISTENCY_CHECK
-
 static void nested_svm_inject_npf_exit(struct kvm_vcpu *vcpu,
 				       struct x86_exception *fault)
 {
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 0d0dd6580cfd..404db7c854d2 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -22,8 +22,6 @@ module_param_named(enable_shadow_vmcs, enable_shadow_vmcs, bool, S_IRUGO);
 static bool __read_mostly nested_early_check = 0;
 module_param(nested_early_check, bool, S_IRUGO);
 
-#define CC KVM_NESTED_VMENTER_CONSISTENCY_CHECK
-
 /*
  * Hyper-V requires all of these, so mark them as supported even though
  * they are just treated the same as all-context.
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 6aac4a901b65..b8a024b0f91c 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -67,6 +67,8 @@ static __always_inline void kvm_guest_exit_irqoff(void)
 	failed;								\
 })
 
+#define CC KVM_NESTED_VMENTER_CONSISTENCY_CHECK
+
 #define KVM_DEFAULT_PLE_GAP		128
 #define KVM_VMX_DEFAULT_PLE_WINDOW	4096
 #define KVM_DEFAULT_PLE_WINDOW_GROW	2
-- 
2.32.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 3/5] KVM: x86: Clean up redundant ROL16(val, n) macro definition
  2021-08-09  9:34 [PATCH 0/5] KVM: x86: Clean up redundant macro definitions Like Xu
  2021-08-09  9:34 ` [PATCH 1/5] KVM: x86: Clean up redundant mod_64(x, y) macro definition Like Xu
  2021-08-09  9:34 ` [PATCH 2/5] KVM: x86: Clean up redundant CC " Like Xu
@ 2021-08-09  9:34 ` Like Xu
  2021-08-10 17:49   ` Paolo Bonzini
  2021-08-09  9:34 ` [PATCH 4/5] KVM: x86: Clean up redundant __ex(x) " Like Xu
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 11+ messages in thread
From: Like Xu @ 2021-08-09  9:34 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, Ingo Molnar, kvm, linux-kernel

From: Like Xu <likexu@tencent.com>

The ROL16(val, n) macro is repeatedly defined in several vmcs-related
files, and it has never been used outside the KVM context.

Let's move it to vmcs.h without any intended functional changes.

Signed-off-by: Like Xu <likexu@tencent.com>
---
 arch/x86/kvm/vmx/evmcs.c  | 1 -
 arch/x86/kvm/vmx/evmcs.h  | 4 ----
 arch/x86/kvm/vmx/vmcs.h   | 2 ++
 arch/x86/kvm/vmx/vmcs12.c | 1 -
 arch/x86/kvm/vmx/vmcs12.h | 4 ----
 5 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c
index 896b2a50b4aa..0dab1b7b529f 100644
--- a/arch/x86/kvm/vmx/evmcs.c
+++ b/arch/x86/kvm/vmx/evmcs.c
@@ -14,7 +14,6 @@ DEFINE_STATIC_KEY_FALSE(enable_evmcs);
 
 #if IS_ENABLED(CONFIG_HYPERV)
 
-#define ROL16(val, n) ((u16)(((u16)(val) << (n)) | ((u16)(val) >> (16 - (n)))))
 #define EVMCS1_OFFSET(x) offsetof(struct hv_enlightened_vmcs, x)
 #define EVMCS1_FIELD(number, name, clean_field)[ROL16(number, 6)] = \
 		{EVMCS1_OFFSET(name), clean_field}
diff --git a/arch/x86/kvm/vmx/evmcs.h b/arch/x86/kvm/vmx/evmcs.h
index 2ec9b46f0d0c..152ab0aa82cf 100644
--- a/arch/x86/kvm/vmx/evmcs.h
+++ b/arch/x86/kvm/vmx/evmcs.h
@@ -73,8 +73,6 @@ struct evmcs_field {
 extern const struct evmcs_field vmcs_field_to_evmcs_1[];
 extern const unsigned int nr_evmcs_1_fields;
 
-#define ROL16(val, n) ((u16)(((u16)(val) << (n)) | ((u16)(val) >> (16 - (n)))))
-
 static __always_inline int get_evmcs_offset(unsigned long field,
 					    u16 *clean_field)
 {
@@ -95,8 +93,6 @@ static __always_inline int get_evmcs_offset(unsigned long field,
 	return evmcs_field->offset;
 }
 
-#undef ROL16
-
 static inline void evmcs_write64(unsigned long field, u64 value)
 {
 	u16 clean_field;
diff --git a/arch/x86/kvm/vmx/vmcs.h b/arch/x86/kvm/vmx/vmcs.h
index 4b9957e2bf5b..6e5de2e2b0da 100644
--- a/arch/x86/kvm/vmx/vmcs.h
+++ b/arch/x86/kvm/vmx/vmcs.h
@@ -11,6 +11,8 @@
 
 #include "capabilities.h"
 
+#define ROL16(val, n) ((u16)(((u16)(val) << (n)) | ((u16)(val) >> (16 - (n)))))
+
 struct vmcs_hdr {
 	u32 revision_id:31;
 	u32 shadow_vmcs:1;
diff --git a/arch/x86/kvm/vmx/vmcs12.c b/arch/x86/kvm/vmx/vmcs12.c
index d9f5d7c56ae3..cab6ba7a5005 100644
--- a/arch/x86/kvm/vmx/vmcs12.c
+++ b/arch/x86/kvm/vmx/vmcs12.c
@@ -2,7 +2,6 @@
 
 #include "vmcs12.h"
 
-#define ROL16(val, n) ((u16)(((u16)(val) << (n)) | ((u16)(val) >> (16 - (n)))))
 #define VMCS12_OFFSET(x) offsetof(struct vmcs12, x)
 #define FIELD(number, name)	[ROL16(number, 6)] = VMCS12_OFFSET(name)
 #define FIELD64(number, name)						\
diff --git a/arch/x86/kvm/vmx/vmcs12.h b/arch/x86/kvm/vmx/vmcs12.h
index 5e0e1b39f495..2a45f026ee11 100644
--- a/arch/x86/kvm/vmx/vmcs12.h
+++ b/arch/x86/kvm/vmx/vmcs12.h
@@ -364,8 +364,6 @@ static inline void vmx_check_vmcs12_offsets(void)
 extern const unsigned short vmcs_field_to_offset_table[];
 extern const unsigned int nr_vmcs12_fields;
 
-#define ROL16(val, n) ((u16)(((u16)(val) << (n)) | ((u16)(val) >> (16 - (n)))))
-
 static inline short vmcs_field_to_offset(unsigned long field)
 {
 	unsigned short offset;
@@ -385,8 +383,6 @@ static inline short vmcs_field_to_offset(unsigned long field)
 	return offset;
 }
 
-#undef ROL16
-
 static inline u64 vmcs12_read_any(struct vmcs12 *vmcs12, unsigned long field,
 				  u16 offset)
 {
-- 
2.32.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 4/5] KVM: x86: Clean up redundant __ex(x) macro definition
  2021-08-09  9:34 [PATCH 0/5] KVM: x86: Clean up redundant macro definitions Like Xu
                   ` (2 preceding siblings ...)
  2021-08-09  9:34 ` [PATCH 3/5] KVM: x86: Clean up redundant ROL16(val, n) " Like Xu
@ 2021-08-09  9:34 ` Like Xu
  2021-08-09  9:34 ` [PATCH 5/5] KVM: x86: Clean up redundant pr_fmt(fmt) macro definition for svm Like Xu
  2021-08-09 15:08 ` [PATCH 0/5] KVM: x86: Clean up redundant macro definitions Sean Christopherson
  5 siblings, 0 replies; 11+ messages in thread
From: Like Xu @ 2021-08-09  9:34 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, Ingo Molnar, kvm, linux-kernel

From: Like Xu <likexu@tencent.com>

The __ex(x) macro is repeatedly defined in sev.c, svm.c and vmx_ops.h,
and it has never been used outside the KVM/x86 context.

Let's move it to kvm_host.h without any intended functional changes.

Signed-off-by: Like Xu <likexu@tencent.com>
---
 arch/x86/include/asm/kvm_host.h | 2 ++
 arch/x86/kvm/svm/sev.c          | 2 --
 arch/x86/kvm/svm/svm.c          | 2 --
 arch/x86/kvm/vmx/vmx_ops.h      | 2 --
 4 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 4c567b05edad..7480d31463bc 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1823,6 +1823,8 @@ asmlinkage void kvm_spurious_fault(void);
 	"668: \n\t"							\
 	_ASM_EXTABLE(666b, 667b)
 
+#define __ex(x) __kvm_handle_fault_on_reboot(x)
+
 #define KVM_ARCH_WANT_MMU_NOTIFIER
 
 int kvm_cpu_has_injectable_intr(struct kvm_vcpu *v);
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 9f1585f40c85..19cdb73aa623 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -28,8 +28,6 @@
 #include "cpuid.h"
 #include "trace.h"
 
-#define __ex(x) __kvm_handle_fault_on_reboot(x)
-
 #ifndef CONFIG_KVM_AMD_SEV
 /*
  * When this config is not defined, SEV feature is not supported and APIs in
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 9d72b1df426e..2b6632d4c76f 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -46,8 +46,6 @@
 #include "kvm_onhyperv.h"
 #include "svm_onhyperv.h"
 
-#define __ex(x) __kvm_handle_fault_on_reboot(x)
-
 MODULE_AUTHOR("Qumranet");
 MODULE_LICENSE("GPL");
 
diff --git a/arch/x86/kvm/vmx/vmx_ops.h b/arch/x86/kvm/vmx/vmx_ops.h
index 164b64f65a8f..c0d74b994b56 100644
--- a/arch/x86/kvm/vmx/vmx_ops.h
+++ b/arch/x86/kvm/vmx/vmx_ops.h
@@ -10,8 +10,6 @@
 #include "evmcs.h"
 #include "vmcs.h"
 
-#define __ex(x) __kvm_handle_fault_on_reboot(x)
-
 asmlinkage void vmread_error(unsigned long field, bool fault);
 __attribute__((regparm(0))) void vmread_error_trampoline(unsigned long field,
 							 bool fault);
-- 
2.32.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 5/5] KVM: x86: Clean up redundant pr_fmt(fmt) macro definition for svm
  2021-08-09  9:34 [PATCH 0/5] KVM: x86: Clean up redundant macro definitions Like Xu
                   ` (3 preceding siblings ...)
  2021-08-09  9:34 ` [PATCH 4/5] KVM: x86: Clean up redundant __ex(x) " Like Xu
@ 2021-08-09  9:34 ` Like Xu
  2021-08-10 17:48   ` Paolo Bonzini
  2021-08-09 15:08 ` [PATCH 0/5] KVM: x86: Clean up redundant macro definitions Sean Christopherson
  5 siblings, 1 reply; 11+ messages in thread
From: Like Xu @ 2021-08-09  9:34 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, Ingo Molnar, kvm, linux-kernel

From: Like Xu <likexu@tencent.com>

The svm specific pr_fmt(fmt) macro is repeatedly defined in svm code
and the new one has never been used outside the svm context.

Let's move it to svm.h without any intended functional changes.

Signed-off-by: Like Xu <likexu@tencent.com>
---
 arch/x86/kvm/svm/avic.c   | 2 --
 arch/x86/kvm/svm/nested.c | 2 --
 arch/x86/kvm/svm/svm.c    | 2 --
 arch/x86/kvm/svm/svm.h    | 3 +++
 4 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index a8ad78a2faa1..8b055f9ad9fe 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -12,8 +12,6 @@
  *   Avi Kivity   <avi@qumranet.com>
  */
 
-#define pr_fmt(fmt) "SVM: " fmt
-
 #include <linux/kvm_types.h>
 #include <linux/hashtable.h>
 #include <linux/amd-iommu.h>
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 57c288ba6ef0..3080776a55a5 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -12,8 +12,6 @@
  *   Avi Kivity   <avi@qumranet.com>
  */
 
-#define pr_fmt(fmt) "SVM: " fmt
-
 #include <linux/kvm_types.h>
 #include <linux/kvm_host.h>
 #include <linux/kernel.h>
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 2b6632d4c76f..4a3f8ef56daa 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1,5 +1,3 @@
-#define pr_fmt(fmt) "SVM: " fmt
-
 #include <linux/kvm_host.h>
 
 #include "irq.h"
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index bd0fe94c2920..76d5fe3f00dc 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -15,6 +15,9 @@
 #ifndef __SVM_SVM_H
 #define __SVM_SVM_H
 
+#undef pr_fmt
+#define pr_fmt(fmt) "SVM: " fmt
+
 #include <linux/kvm_types.h>
 #include <linux/kvm_host.h>
 #include <linux/bits.h>
-- 
2.32.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/5] KVM: x86: Clean up redundant macro definitions
  2021-08-09  9:34 [PATCH 0/5] KVM: x86: Clean up redundant macro definitions Like Xu
                   ` (4 preceding siblings ...)
  2021-08-09  9:34 ` [PATCH 5/5] KVM: x86: Clean up redundant pr_fmt(fmt) macro definition for svm Like Xu
@ 2021-08-09 15:08 ` Sean Christopherson
  5 siblings, 0 replies; 11+ messages in thread
From: Sean Christopherson @ 2021-08-09 15:08 UTC (permalink / raw)
  To: Like Xu
  Cc: Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, Ingo Molnar, kvm, linux-kernel

On Mon, Aug 09, 2021, Like Xu wrote:
> In KVM/x86 code, there are macros with the same name that are
> defined and used separately in the evolving code, and if the scope
> of the code review is only on iterations of the patch set, it can be
> difficult to spot these fragmented macros being defined repeatedly.
> 
> IMO, it's necessary to clean this up to improve the consistency and

Please use more specific shortlogs.  "Clean up" is too ambiguous, e.g. I would
expect it to mean "clean up the macro itself".

> readability of the code, and it also helps to avoid software defects
> caused by inconsistencies in the scope of influence of macros.
> 
> Like Xu (5):

>   KVM: x86: Clean up redundant mod_64(x, y) macro definition

Feels like we should find a home outside of KVM for mod_64(), it's a full generic
and straightforward macro.

>   KVM: x86: Clean up redundant CC macro definition

CC() should be kept where it is.  It should never be used outside of nested code
and is far too generic of a name to be exposed to the world at large.  It was
deliberately defined only in the nested.c files.

>   KVM: x86: Clean up redundant ROL16(val, n) macro definition

Moving ROL16 seems ok, though it scares me a bit that someone went through the
trouble of #undef'ing the macros.

>   KVM: x86: Clean up redundant __ex(x) macro definition

__ex() and __kvm_handle_fault_on_reboot() were supposed to have been removed.
The last two patches [8][9] of this series[*] got lost; I'll repost them as they
no longer apply cleanly.

>   KVM: x86: Clean up redundant pr_fmt(fmt) macro definition for svm

NAK, this breaks sev.c's formatting.  Arguably, nested.c and avic.c should use
more specific print messages, though that gets tricky with nested code as it's
not wholly contained in nested.c.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/5] KVM: x86: Clean up redundant CC macro definition
  2021-08-09  9:34 ` [PATCH 2/5] KVM: x86: Clean up redundant CC " Like Xu
@ 2021-08-10 17:47   ` Paolo Bonzini
  0 siblings, 0 replies; 11+ messages in thread
From: Paolo Bonzini @ 2021-08-10 17:47 UTC (permalink / raw)
  To: Like Xu
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, Ingo Molnar, kvm, linux-kernel

On 09/08/21 11:34, Like Xu wrote:
> From: Like Xu <likexu@tencent.com>
> 
> With the exception of drivers/dma/pl330.c, the CC macro is defined and used
> in {svm, vmx}/nested.c, and the KVM_NESTED_VMENTER_CONSISTENCY_CHECK
> macro it refers to is defined in x86.h, so it's safe to move it into x86.h
> without intended functional changes.
> 
> Signed-off-by: Like Xu <likexu@tencent.com>

This one is just a shortcut that should not available outside nested.c, 
so I am not applying it.

Paolo

> ---
>   arch/x86/kvm/svm/nested.c | 2 --
>   arch/x86/kvm/vmx/nested.c | 2 --
>   arch/x86/kvm/x86.h        | 2 ++
>   3 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 5e13357da21e..57c288ba6ef0 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -29,8 +29,6 @@
>   #include "lapic.h"
>   #include "svm.h"
>   
> -#define CC KVM_NESTED_VMENTER_CONSISTENCY_CHECK
> -
>   static void nested_svm_inject_npf_exit(struct kvm_vcpu *vcpu,
>   				       struct x86_exception *fault)
>   {
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 0d0dd6580cfd..404db7c854d2 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -22,8 +22,6 @@ module_param_named(enable_shadow_vmcs, enable_shadow_vmcs, bool, S_IRUGO);
>   static bool __read_mostly nested_early_check = 0;
>   module_param(nested_early_check, bool, S_IRUGO);
>   
> -#define CC KVM_NESTED_VMENTER_CONSISTENCY_CHECK
> -
>   /*
>    * Hyper-V requires all of these, so mark them as supported even though
>    * they are just treated the same as all-context.
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index 6aac4a901b65..b8a024b0f91c 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -67,6 +67,8 @@ static __always_inline void kvm_guest_exit_irqoff(void)
>   	failed;								\
>   })
>   
> +#define CC KVM_NESTED_VMENTER_CONSISTENCY_CHECK
> +
>   #define KVM_DEFAULT_PLE_GAP		128
>   #define KVM_VMX_DEFAULT_PLE_WINDOW	4096
>   #define KVM_DEFAULT_PLE_WINDOW_GROW	2
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 5/5] KVM: x86: Clean up redundant pr_fmt(fmt) macro definition for svm
  2021-08-09  9:34 ` [PATCH 5/5] KVM: x86: Clean up redundant pr_fmt(fmt) macro definition for svm Like Xu
@ 2021-08-10 17:48   ` Paolo Bonzini
  2021-08-12 13:00     ` Like Xu
  0 siblings, 1 reply; 11+ messages in thread
From: Paolo Bonzini @ 2021-08-10 17:48 UTC (permalink / raw)
  To: Like Xu
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, Ingo Molnar, kvm, linux-kernel

On 09/08/21 11:34, Like Xu wrote:
> +#undef pr_fmt
> +#define pr_fmt(fmt) "SVM: " fmt
> +
>   #include <linux/kvm_types.h>

Why do you need the #undef?

Paolo


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/5] KVM: x86: Clean up redundant ROL16(val, n) macro definition
  2021-08-09  9:34 ` [PATCH 3/5] KVM: x86: Clean up redundant ROL16(val, n) " Like Xu
@ 2021-08-10 17:49   ` Paolo Bonzini
  0 siblings, 0 replies; 11+ messages in thread
From: Paolo Bonzini @ 2021-08-10 17:49 UTC (permalink / raw)
  To: Like Xu
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, Ingo Molnar, kvm, linux-kernel

On 09/08/21 11:34, Like Xu wrote:
> From: Like Xu <likexu@tencent.com>
> 
> The ROL16(val, n) macro is repeatedly defined in several vmcs-related
> files, and it has never been used outside the KVM context.
> 
> Let's move it to vmcs.h without any intended functional changes.
> 
> Signed-off-by: Like Xu <likexu@tencent.com>
> ---
>   arch/x86/kvm/vmx/evmcs.c  | 1 -
>   arch/x86/kvm/vmx/evmcs.h  | 4 ----
>   arch/x86/kvm/vmx/vmcs.h   | 2 ++
>   arch/x86/kvm/vmx/vmcs12.c | 1 -
>   arch/x86/kvm/vmx/vmcs12.h | 4 ----
>   5 files changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c
> index 896b2a50b4aa..0dab1b7b529f 100644
> --- a/arch/x86/kvm/vmx/evmcs.c
> +++ b/arch/x86/kvm/vmx/evmcs.c
> @@ -14,7 +14,6 @@ DEFINE_STATIC_KEY_FALSE(enable_evmcs);
>   
>   #if IS_ENABLED(CONFIG_HYPERV)
>   
> -#define ROL16(val, n) ((u16)(((u16)(val) << (n)) | ((u16)(val) >> (16 - (n)))))
>   #define EVMCS1_OFFSET(x) offsetof(struct hv_enlightened_vmcs, x)
>   #define EVMCS1_FIELD(number, name, clean_field)[ROL16(number, 6)] = \
>   		{EVMCS1_OFFSET(name), clean_field}
> diff --git a/arch/x86/kvm/vmx/evmcs.h b/arch/x86/kvm/vmx/evmcs.h
> index 2ec9b46f0d0c..152ab0aa82cf 100644
> --- a/arch/x86/kvm/vmx/evmcs.h
> +++ b/arch/x86/kvm/vmx/evmcs.h
> @@ -73,8 +73,6 @@ struct evmcs_field {
>   extern const struct evmcs_field vmcs_field_to_evmcs_1[];
>   extern const unsigned int nr_evmcs_1_fields;
>   
> -#define ROL16(val, n) ((u16)(((u16)(val) << (n)) | ((u16)(val) >> (16 - (n)))))
> -
>   static __always_inline int get_evmcs_offset(unsigned long field,
>   					    u16 *clean_field)
>   {
> @@ -95,8 +93,6 @@ static __always_inline int get_evmcs_offset(unsigned long field,
>   	return evmcs_field->offset;
>   }
>   
> -#undef ROL16
> -
>   static inline void evmcs_write64(unsigned long field, u64 value)
>   {
>   	u16 clean_field;
> diff --git a/arch/x86/kvm/vmx/vmcs.h b/arch/x86/kvm/vmx/vmcs.h
> index 4b9957e2bf5b..6e5de2e2b0da 100644
> --- a/arch/x86/kvm/vmx/vmcs.h
> +++ b/arch/x86/kvm/vmx/vmcs.h
> @@ -11,6 +11,8 @@
>   
>   #include "capabilities.h"
>   
> +#define ROL16(val, n) ((u16)(((u16)(val) << (n)) | ((u16)(val) >> (16 - (n)))))
> +
>   struct vmcs_hdr {
>   	u32 revision_id:31;
>   	u32 shadow_vmcs:1;
> diff --git a/arch/x86/kvm/vmx/vmcs12.c b/arch/x86/kvm/vmx/vmcs12.c
> index d9f5d7c56ae3..cab6ba7a5005 100644
> --- a/arch/x86/kvm/vmx/vmcs12.c
> +++ b/arch/x86/kvm/vmx/vmcs12.c
> @@ -2,7 +2,6 @@
>   
>   #include "vmcs12.h"
>   
> -#define ROL16(val, n) ((u16)(((u16)(val) << (n)) | ((u16)(val) >> (16 - (n)))))
>   #define VMCS12_OFFSET(x) offsetof(struct vmcs12, x)
>   #define FIELD(number, name)	[ROL16(number, 6)] = VMCS12_OFFSET(name)
>   #define FIELD64(number, name)						\
> diff --git a/arch/x86/kvm/vmx/vmcs12.h b/arch/x86/kvm/vmx/vmcs12.h
> index 5e0e1b39f495..2a45f026ee11 100644
> --- a/arch/x86/kvm/vmx/vmcs12.h
> +++ b/arch/x86/kvm/vmx/vmcs12.h
> @@ -364,8 +364,6 @@ static inline void vmx_check_vmcs12_offsets(void)
>   extern const unsigned short vmcs_field_to_offset_table[];
>   extern const unsigned int nr_vmcs12_fields;
>   
> -#define ROL16(val, n) ((u16)(((u16)(val) << (n)) | ((u16)(val) >> (16 - (n)))))
> -
>   static inline short vmcs_field_to_offset(unsigned long field)
>   {
>   	unsigned short offset;
> @@ -385,8 +383,6 @@ static inline short vmcs_field_to_offset(unsigned long field)
>   	return offset;
>   }
>   
> -#undef ROL16
> -
>   static inline u64 vmcs12_read_any(struct vmcs12 *vmcs12, unsigned long field,
>   				  u16 offset)
>   {
> 

Queued, thanks.

Paolo


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 5/5] KVM: x86: Clean up redundant pr_fmt(fmt) macro definition for svm
  2021-08-10 17:48   ` Paolo Bonzini
@ 2021-08-12 13:00     ` Like Xu
  0 siblings, 0 replies; 11+ messages in thread
From: Like Xu @ 2021-08-12 13:00 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, Ingo Molnar, kvm, linux-kernel

On 11/8/2021 1:48 am, Paolo Bonzini wrote:
> On 09/08/21 11:34, Like Xu wrote:
>> +#undef pr_fmt
>> +#define pr_fmt(fmt) "SVM: " fmt
>> +
>>   #include <linux/kvm_types.h>
> 
> Why do you need the #undef?
> 
> Paolo
> 
>
I've seen most of the redefinition code for 'pr_fmt' like this,
for example:

- 3bfaf95cb1fe81872df884956c704469e68a5bee
- d157aa0fb241646e8818f699653ed983e6581b11

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-08-12 13:00 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-09  9:34 [PATCH 0/5] KVM: x86: Clean up redundant macro definitions Like Xu
2021-08-09  9:34 ` [PATCH 1/5] KVM: x86: Clean up redundant mod_64(x, y) macro definition Like Xu
2021-08-09  9:34 ` [PATCH 2/5] KVM: x86: Clean up redundant CC " Like Xu
2021-08-10 17:47   ` Paolo Bonzini
2021-08-09  9:34 ` [PATCH 3/5] KVM: x86: Clean up redundant ROL16(val, n) " Like Xu
2021-08-10 17:49   ` Paolo Bonzini
2021-08-09  9:34 ` [PATCH 4/5] KVM: x86: Clean up redundant __ex(x) " Like Xu
2021-08-09  9:34 ` [PATCH 5/5] KVM: x86: Clean up redundant pr_fmt(fmt) macro definition for svm Like Xu
2021-08-10 17:48   ` Paolo Bonzini
2021-08-12 13:00     ` Like Xu
2021-08-09 15:08 ` [PATCH 0/5] KVM: x86: Clean up redundant macro definitions Sean Christopherson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).