[v3] KVM: x86: Add "governed" X86_FEATURE framework

[PATCH v3 01/15] KVM: x86: Add a framework for enabling KVM-governed x86 features

Posted by Sean Christopherson 2 years ago

Introduce yet another X86_FEATURE flag framework to manage and cache KVM
governed features (for lack of a better name).  "Governed" in this case
means that KVM has some level of involvement and/or vested interest in
whether or not an X86_FEATURE can be used by the guest.  The intent of the
framework is twofold: to simplify caching of guest CPUID flags that KVM
needs to frequently query, and to add clarity to such caching, e.g. it
isn't immediately obvious that SVM's bundle of flags for "optional nested
SVM features" track whether or not a flag is exposed to L1.

Begrudgingly define KVM_MAX_NR_GOVERNED_FEATURES for the size of the
bitmap to avoid exposing governed_features.h in arch/x86/include/asm/, but
add a FIXME to call out that it can and should be cleaned up once
"struct kvm_vcpu_arch" is no longer expose to the kernel at large.

Cc: Zeng Guang <guang.zeng@intel.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/kvm_host.h  | 19 +++++++++++++
 arch/x86/kvm/cpuid.c             |  4 +++
 arch/x86/kvm/cpuid.h             | 46 ++++++++++++++++++++++++++++++++
 arch/x86/kvm/governed_features.h |  9 +++++++
 4 files changed, 78 insertions(+)
 create mode 100644 arch/x86/kvm/governed_features.h

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 19d64f019240..60d430b4650f 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -831,6 +831,25 @@ struct kvm_vcpu_arch {
 	struct kvm_cpuid_entry2 *cpuid_entries;
 	struct kvm_hypervisor_cpuid kvm_cpuid;
 
+	/*
+	 * FIXME: Drop this macro and use KVM_NR_GOVERNED_FEATURES directly
+	 * when "struct kvm_vcpu_arch" is no longer defined in an
+	 * arch/x86/include/asm header.  The max is mostly arbitrary, i.e.
+	 * can be increased as necessary.
+	 */
+#define KVM_MAX_NR_GOVERNED_FEATURES BITS_PER_LONG
+
+	/*
+	 * Track whether or not the guest is allowed to use features that are
+	 * governed by KVM, where "governed" means KVM needs to manage state
+	 * and/or explicitly enable the feature in hardware.  Typically, but
+	 * not always, governed features can be used by the guest if and only
+	 * if both KVM and userspace want to expose the feature to the guest.
+	 */
+	struct {
+		DECLARE_BITMAP(enabled, KVM_MAX_NR_GOVERNED_FEATURES);
+	} governed_features;
+
 	u64 reserved_gpa_bits;
 	int maxphyaddr;
 
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 5a88affb2e1a..4ba43ae008cb 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -313,6 +313,10 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 	struct kvm_lapic *apic = vcpu->arch.apic;
 	struct kvm_cpuid_entry2 *best;
 
+	BUILD_BUG_ON(KVM_NR_GOVERNED_FEATURES > KVM_MAX_NR_GOVERNED_FEATURES);
+	bitmap_zero(vcpu->arch.governed_features.enabled,
+		    KVM_MAX_NR_GOVERNED_FEATURES);
+
 	best = kvm_find_cpuid_entry(vcpu, 1);
 	if (best && apic) {
 		if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index b1658c0de847..284fa4704553 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -232,4 +232,50 @@ static __always_inline bool guest_pv_has(struct kvm_vcpu *vcpu,
 	return vcpu->arch.pv_cpuid.features & (1u << kvm_feature);
 }
 
+enum kvm_governed_features {
+#define KVM_GOVERNED_FEATURE(x) KVM_GOVERNED_##x,
+#include "governed_features.h"
+	KVM_NR_GOVERNED_FEATURES
+};
+
+static __always_inline int kvm_governed_feature_index(unsigned int x86_feature)
+{
+	switch (x86_feature) {
+#define KVM_GOVERNED_FEATURE(x) case x: return KVM_GOVERNED_##x;
+#include "governed_features.h"
+	default:
+		return -1;
+	}
+}
+
+static __always_inline bool kvm_is_governed_feature(unsigned int x86_feature)
+{
+	return kvm_governed_feature_index(x86_feature) >= 0;
+}
+
+static __always_inline void kvm_governed_feature_set(struct kvm_vcpu *vcpu,
+						     unsigned int x86_feature)
+{
+	BUILD_BUG_ON(!kvm_is_governed_feature(x86_feature));
+
+	__set_bit(kvm_governed_feature_index(x86_feature),
+		  vcpu->arch.governed_features.enabled);
+}
+
+static __always_inline void kvm_governed_feature_check_and_set(struct kvm_vcpu *vcpu,
+							       unsigned int x86_feature)
+{
+	if (kvm_cpu_cap_has(x86_feature) && guest_cpuid_has(vcpu, x86_feature))
+		kvm_governed_feature_set(vcpu, x86_feature);
+}
+
+static __always_inline bool guest_can_use(struct kvm_vcpu *vcpu,
+					  unsigned int x86_feature)
+{
+	BUILD_BUG_ON(!kvm_is_governed_feature(x86_feature));
+
+	return test_bit(kvm_governed_feature_index(x86_feature),
+			vcpu->arch.governed_features.enabled);
+}
+
 #endif
diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h
new file mode 100644
index 000000000000..40ce8e6608cd
--- /dev/null
+++ b/arch/x86/kvm/governed_features.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#if !defined(KVM_GOVERNED_FEATURE) || defined(KVM_GOVERNED_X86_FEATURE)
+BUILD_BUG()
+#endif
+
+#define KVM_GOVERNED_X86_FEATURE(x) KVM_GOVERNED_FEATURE(X86_FEATURE_##x)
+
+#undef KVM_GOVERNED_X86_FEATURE
+#undef KVM_GOVERNED_FEATURE
-- 
2.41.0.694.ge786442a9b-goog

Re: [PATCH v3 01/15] KVM: x86: Add a framework for enabling KVM-governed x86 features

Posted by Binbin Wu 2 years ago

Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>


On 8/16/2023 4:36 AM, Sean Christopherson wrote:
> Introduce yet another X86_FEATURE flag framework to manage and cache KVM
> governed features (for lack of a better name).  "Governed" in this case
> means that KVM has some level of involvement and/or vested interest in
> whether or not an X86_FEATURE can be used by the guest.  The intent of the
> framework is twofold: to simplify caching of guest CPUID flags that KVM
> needs to frequently query, and to add clarity to such caching, e.g. it
> isn't immediately obvious that SVM's bundle of flags for "optional nested
> SVM features" track whether or not a flag is exposed to L1.
>
> Begrudgingly define KVM_MAX_NR_GOVERNED_FEATURES for the size of the
> bitmap to avoid exposing governed_features.h in arch/x86/include/asm/, but
> add a FIXME to call out that it can and should be cleaned up once
> "struct kvm_vcpu_arch" is no longer expose to the kernel at large.
>
> Cc: Zeng Guang <guang.zeng@intel.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/include/asm/kvm_host.h  | 19 +++++++++++++
>   arch/x86/kvm/cpuid.c             |  4 +++
>   arch/x86/kvm/cpuid.h             | 46 ++++++++++++++++++++++++++++++++
>   arch/x86/kvm/governed_features.h |  9 +++++++
>   4 files changed, 78 insertions(+)
>   create mode 100644 arch/x86/kvm/governed_features.h
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 19d64f019240..60d430b4650f 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -831,6 +831,25 @@ struct kvm_vcpu_arch {
>   	struct kvm_cpuid_entry2 *cpuid_entries;
>   	struct kvm_hypervisor_cpuid kvm_cpuid;
>   
> +	/*
> +	 * FIXME: Drop this macro and use KVM_NR_GOVERNED_FEATURES directly
> +	 * when "struct kvm_vcpu_arch" is no longer defined in an
> +	 * arch/x86/include/asm header.  The max is mostly arbitrary, i.e.
> +	 * can be increased as necessary.
> +	 */
> +#define KVM_MAX_NR_GOVERNED_FEATURES BITS_PER_LONG
> +
> +	/*
> +	 * Track whether or not the guest is allowed to use features that are
> +	 * governed by KVM, where "governed" means KVM needs to manage state
> +	 * and/or explicitly enable the feature in hardware.  Typically, but
> +	 * not always, governed features can be used by the guest if and only
> +	 * if both KVM and userspace want to expose the feature to the guest.
> +	 */
> +	struct {
> +		DECLARE_BITMAP(enabled, KVM_MAX_NR_GOVERNED_FEATURES);
> +	} governed_features;
> +
>   	u64 reserved_gpa_bits;
>   	int maxphyaddr;
>   
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 5a88affb2e1a..4ba43ae008cb 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -313,6 +313,10 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>   	struct kvm_lapic *apic = vcpu->arch.apic;
>   	struct kvm_cpuid_entry2 *best;
>   
> +	BUILD_BUG_ON(KVM_NR_GOVERNED_FEATURES > KVM_MAX_NR_GOVERNED_FEATURES);
> +	bitmap_zero(vcpu->arch.governed_features.enabled,
> +		    KVM_MAX_NR_GOVERNED_FEATURES);
> +
>   	best = kvm_find_cpuid_entry(vcpu, 1);
>   	if (best && apic) {
>   		if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index b1658c0de847..284fa4704553 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -232,4 +232,50 @@ static __always_inline bool guest_pv_has(struct kvm_vcpu *vcpu,
>   	return vcpu->arch.pv_cpuid.features & (1u << kvm_feature);
>   }
>   
> +enum kvm_governed_features {
> +#define KVM_GOVERNED_FEATURE(x) KVM_GOVERNED_##x,
> +#include "governed_features.h"
> +	KVM_NR_GOVERNED_FEATURES
> +};
> +
> +static __always_inline int kvm_governed_feature_index(unsigned int x86_feature)
> +{
> +	switch (x86_feature) {
> +#define KVM_GOVERNED_FEATURE(x) case x: return KVM_GOVERNED_##x;
> +#include "governed_features.h"
> +	default:
> +		return -1;
> +	}
> +}
> +
> +static __always_inline bool kvm_is_governed_feature(unsigned int x86_feature)
> +{
> +	return kvm_governed_feature_index(x86_feature) >= 0;
> +}
> +
> +static __always_inline void kvm_governed_feature_set(struct kvm_vcpu *vcpu,
> +						     unsigned int x86_feature)
> +{
> +	BUILD_BUG_ON(!kvm_is_governed_feature(x86_feature));
> +
> +	__set_bit(kvm_governed_feature_index(x86_feature),
> +		  vcpu->arch.governed_features.enabled);
> +}
> +
> +static __always_inline void kvm_governed_feature_check_and_set(struct kvm_vcpu *vcpu,
> +							       unsigned int x86_feature)
> +{
> +	if (kvm_cpu_cap_has(x86_feature) && guest_cpuid_has(vcpu, x86_feature))
> +		kvm_governed_feature_set(vcpu, x86_feature);
> +}
> +
> +static __always_inline bool guest_can_use(struct kvm_vcpu *vcpu,
> +					  unsigned int x86_feature)
> +{
> +	BUILD_BUG_ON(!kvm_is_governed_feature(x86_feature));
> +
> +	return test_bit(kvm_governed_feature_index(x86_feature),
> +			vcpu->arch.governed_features.enabled);
> +}
> +
>   #endif
> diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h
> new file mode 100644
> index 000000000000..40ce8e6608cd
> --- /dev/null
> +++ b/arch/x86/kvm/governed_features.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#if !defined(KVM_GOVERNED_FEATURE) || defined(KVM_GOVERNED_X86_FEATURE)
> +BUILD_BUG()
> +#endif
> +
> +#define KVM_GOVERNED_X86_FEATURE(x) KVM_GOVERNED_FEATURE(X86_FEATURE_##x)
> +
> +#undef KVM_GOVERNED_X86_FEATURE
> +#undef KVM_GOVERNED_FEATURE

Re: [PATCH v3 01/15] KVM: x86: Add a framework for enabling KVM-governed x86 features

Posted by Yuan Yao 2 years ago

On Tue, Aug 15, 2023 at 01:36:39PM -0700, Sean Christopherson wrote:
> Introduce yet another X86_FEATURE flag framework to manage and cache KVM
> governed features (for lack of a better name).  "Governed" in this case
> means that KVM has some level of involvement and/or vested interest in
> whether or not an X86_FEATURE can be used by the guest.  The intent of the
> framework is twofold: to simplify caching of guest CPUID flags that KVM
> needs to frequently query, and to add clarity to such caching, e.g. it
> isn't immediately obvious that SVM's bundle of flags for "optional nested
> SVM features" track whether or not a flag is exposed to L1.
>
> Begrudgingly define KVM_MAX_NR_GOVERNED_FEATURES for the size of the
> bitmap to avoid exposing governed_features.h in arch/x86/include/asm/, but
> add a FIXME to call out that it can and should be cleaned up once
> "struct kvm_vcpu_arch" is no longer expose to the kernel at large.
>
> Cc: Zeng Guang <guang.zeng@intel.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Yuan Yao <yuan.yao@intel.com>

> ---
>  arch/x86/include/asm/kvm_host.h  | 19 +++++++++++++
>  arch/x86/kvm/cpuid.c             |  4 +++
>  arch/x86/kvm/cpuid.h             | 46 ++++++++++++++++++++++++++++++++
>  arch/x86/kvm/governed_features.h |  9 +++++++
>  4 files changed, 78 insertions(+)
>  create mode 100644 arch/x86/kvm/governed_features.h
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 19d64f019240..60d430b4650f 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -831,6 +831,25 @@ struct kvm_vcpu_arch {
>  	struct kvm_cpuid_entry2 *cpuid_entries;
>  	struct kvm_hypervisor_cpuid kvm_cpuid;
>
> +	/*
> +	 * FIXME: Drop this macro and use KVM_NR_GOVERNED_FEATURES directly
> +	 * when "struct kvm_vcpu_arch" is no longer defined in an
> +	 * arch/x86/include/asm header.  The max is mostly arbitrary, i.e.
> +	 * can be increased as necessary.
> +	 */
> +#define KVM_MAX_NR_GOVERNED_FEATURES BITS_PER_LONG
> +
> +	/*
> +	 * Track whether or not the guest is allowed to use features that are
> +	 * governed by KVM, where "governed" means KVM needs to manage state
> +	 * and/or explicitly enable the feature in hardware.  Typically, but
> +	 * not always, governed features can be used by the guest if and only
> +	 * if both KVM and userspace want to expose the feature to the guest.
> +	 */
> +	struct {
> +		DECLARE_BITMAP(enabled, KVM_MAX_NR_GOVERNED_FEATURES);
> +	} governed_features;
> +
>  	u64 reserved_gpa_bits;
>  	int maxphyaddr;
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 5a88affb2e1a..4ba43ae008cb 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -313,6 +313,10 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  	struct kvm_lapic *apic = vcpu->arch.apic;
>  	struct kvm_cpuid_entry2 *best;
>
> +	BUILD_BUG_ON(KVM_NR_GOVERNED_FEATURES > KVM_MAX_NR_GOVERNED_FEATURES);
> +	bitmap_zero(vcpu->arch.governed_features.enabled,
> +		    KVM_MAX_NR_GOVERNED_FEATURES);
> +
>  	best = kvm_find_cpuid_entry(vcpu, 1);
>  	if (best && apic) {
>  		if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index b1658c0de847..284fa4704553 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -232,4 +232,50 @@ static __always_inline bool guest_pv_has(struct kvm_vcpu *vcpu,
>  	return vcpu->arch.pv_cpuid.features & (1u << kvm_feature);
>  }
>
> +enum kvm_governed_features {
> +#define KVM_GOVERNED_FEATURE(x) KVM_GOVERNED_##x,
> +#include "governed_features.h"
> +	KVM_NR_GOVERNED_FEATURES
> +};
> +
> +static __always_inline int kvm_governed_feature_index(unsigned int x86_feature)
> +{
> +	switch (x86_feature) {
> +#define KVM_GOVERNED_FEATURE(x) case x: return KVM_GOVERNED_##x;
> +#include "governed_features.h"
> +	default:
> +		return -1;
> +	}
> +}
> +
> +static __always_inline bool kvm_is_governed_feature(unsigned int x86_feature)
> +{
> +	return kvm_governed_feature_index(x86_feature) >= 0;
> +}
> +
> +static __always_inline void kvm_governed_feature_set(struct kvm_vcpu *vcpu,
> +						     unsigned int x86_feature)
> +{
> +	BUILD_BUG_ON(!kvm_is_governed_feature(x86_feature));
> +
> +	__set_bit(kvm_governed_feature_index(x86_feature),
> +		  vcpu->arch.governed_features.enabled);
> +}
> +
> +static __always_inline void kvm_governed_feature_check_and_set(struct kvm_vcpu *vcpu,
> +							       unsigned int x86_feature)
> +{
> +	if (kvm_cpu_cap_has(x86_feature) && guest_cpuid_has(vcpu, x86_feature))
> +		kvm_governed_feature_set(vcpu, x86_feature);
> +}
> +
> +static __always_inline bool guest_can_use(struct kvm_vcpu *vcpu,
> +					  unsigned int x86_feature)
> +{
> +	BUILD_BUG_ON(!kvm_is_governed_feature(x86_feature));
> +
> +	return test_bit(kvm_governed_feature_index(x86_feature),
> +			vcpu->arch.governed_features.enabled);
> +}
> +
>  #endif
> diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h
> new file mode 100644
> index 000000000000..40ce8e6608cd
> --- /dev/null
> +++ b/arch/x86/kvm/governed_features.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#if !defined(KVM_GOVERNED_FEATURE) || defined(KVM_GOVERNED_X86_FEATURE)
> +BUILD_BUG()
> +#endif
> +
> +#define KVM_GOVERNED_X86_FEATURE(x) KVM_GOVERNED_FEATURE(X86_FEATURE_##x)
> +
> +#undef KVM_GOVERNED_X86_FEATURE
> +#undef KVM_GOVERNED_FEATURE
> --
> 2.41.0.694.ge786442a9b-goog
>

Re: [PATCH v3 01/15] KVM: x86: Add a framework for enabling KVM-governed x86 features

Posted by Huang, Kai 2 years ago

>  
> +enum kvm_governed_features {
> +#define KVM_GOVERNED_FEATURE(x) KVM_GOVERNED_##x,
> +#include "governed_features.h"
> +	KVM_NR_GOVERNED_FEATURES
> +};
> +
> +static __always_inline int kvm_governed_feature_index(unsigned int x86_feature)
> +{
> +	switch (x86_feature) {
> +#define KVM_GOVERNED_FEATURE(x) case x: return KVM_GOVERNED_##x;
> +#include "governed_features.h"
> +	default:
> +		return -1;
> +	}
> +}
> +
> +static __always_inline bool kvm_is_governed_feature(unsigned int x86_feature)
> +{
> +	return kvm_governed_feature_index(x86_feature) >= 0;
> +}
> +
> +static __always_inline void kvm_governed_feature_set(struct kvm_vcpu *vcpu,
> +						     unsigned int x86_feature)
> +{
> +	BUILD_BUG_ON(!kvm_is_governed_feature(x86_feature));
> +
> +	__set_bit(kvm_governed_feature_index(x86_feature),
> +		  vcpu->arch.governed_features.enabled);
> +}
> +
> +static __always_inline void kvm_governed_feature_check_and_set(struct kvm_vcpu *vcpu,
> +							       unsigned int x86_feature)
> +{
> +	if (kvm_cpu_cap_has(x86_feature) && guest_cpuid_has(vcpu, x86_feature))
> +		kvm_governed_feature_set(vcpu, x86_feature);
> +}
> +
> +static __always_inline bool guest_can_use(struct kvm_vcpu *vcpu,
> +					  unsigned int x86_feature)
> +{
> +	BUILD_BUG_ON(!kvm_is_governed_feature(x86_feature));
> +
> +	return test_bit(kvm_governed_feature_index(x86_feature),
> +			vcpu->arch.governed_features.enabled);
> +}
> +
>  #endif
> diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h
> new file mode 100644
> index 000000000000..40ce8e6608cd
> --- /dev/null
> +++ b/arch/x86/kvm/governed_features.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#if !defined(KVM_GOVERNED_FEATURE) || defined(KVM_GOVERNED_X86_FEATURE)
> +BUILD_BUG()
> +#endif
> +
> +#define KVM_GOVERNED_X86_FEATURE(x) KVM_GOVERNED_FEATURE(X86_FEATURE_##x)
> +
> +#undef KVM_GOVERNED_X86_FEATURE
> +#undef KVM_GOVERNED_FEATURE

Nit:

Do you want to move the very last

	#undef KVM_GOVERNED_FEATURE

out of "governed_features.h", but to the place(s) where the macro is defined?

Yes there will be multiple:

	#define KVM_GOVERNED_FEATURE(x)	...
	#include "governed_features.h"
	#undef KVM_GOVERNED_FEATURE

But this looks clearer to me.

Re: [PATCH v3 01/15] KVM: x86: Add a framework for enabling KVM-governed x86 features

Posted by Sean Christopherson 2 years ago

On Wed, Aug 16, 2023, Kai Huang wrote:
> > diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h
> > new file mode 100644
> > index 000000000000..40ce8e6608cd
> > --- /dev/null
> > +++ b/arch/x86/kvm/governed_features.h
> > @@ -0,0 +1,9 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#if !defined(KVM_GOVERNED_FEATURE) || defined(KVM_GOVERNED_X86_FEATURE)
> > +BUILD_BUG()
> > +#endif
> > +
> > +#define KVM_GOVERNED_X86_FEATURE(x) KVM_GOVERNED_FEATURE(X86_FEATURE_##x)
> > +
> > +#undef KVM_GOVERNED_X86_FEATURE
> > +#undef KVM_GOVERNED_FEATURE
> 
> Nit:
> 
> Do you want to move the very last
> 
> 	#undef KVM_GOVERNED_FEATURE
> 
> out of "governed_features.h", but to the place(s) where the macro is defined?
> 
> Yes there will be multiple:
> 
> 	#define KVM_GOVERNED_FEATURE(x)	...
> 	#include "governed_features.h"
> 	#undef KVM_GOVERNED_FEATURE
> 
> But this looks clearer to me.

I agree the symmetry looks better, but doing the #undef in governed_features.h
is much more robust.  E.g. having the #undef in the header makes it all but impossible
to have a bug where we forget to #undef KVM_GOVERNED_FEATURE.  Or worse, have two
bugs where we forget to #undef and then also forget to #define in a later include
and consume the stale #define.

And I also want to follow the pattern used by kvm-x86-ops.h.

Re: [PATCH v3 01/15] KVM: x86: Add a framework for enabling KVM-governed x86 features

Posted by Huang, Kai 2 years ago

On Wed, 2023-08-16 at 07:20 -0700, Sean Christopherson wrote:
> On Wed, Aug 16, 2023, Kai Huang wrote:
> > > diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h
> > > new file mode 100644
> > > index 000000000000..40ce8e6608cd
> > > --- /dev/null
> > > +++ b/arch/x86/kvm/governed_features.h
> > > @@ -0,0 +1,9 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +#if !defined(KVM_GOVERNED_FEATURE) || defined(KVM_GOVERNED_X86_FEATURE)
> > > +BUILD_BUG()
> > > +#endif
> > > +
> > > +#define KVM_GOVERNED_X86_FEATURE(x) KVM_GOVERNED_FEATURE(X86_FEATURE_##x)
> > > +
> > > +#undef KVM_GOVERNED_X86_FEATURE
> > > +#undef KVM_GOVERNED_FEATURE
> > 
> > Nit:
> > 
> > Do you want to move the very last
> > 
> > 	#undef KVM_GOVERNED_FEATURE
> > 
> > out of "governed_features.h", but to the place(s) where the macro is defined?
> > 
> > Yes there will be multiple:
> > 
> > 	#define KVM_GOVERNED_FEATURE(x)	...
> > 	#include "governed_features.h"
> > 	#undef KVM_GOVERNED_FEATURE
> > 
> > But this looks clearer to me.
> 
> I agree the symmetry looks better, but doing the #undef in governed_features.h
> is much more robust.  E.g. having the #undef in the header makes it all but impossible
> to have a bug where we forget to #undef KVM_GOVERNED_FEATURE.  Or worse, have two
> bugs where we forget to #undef and then also forget to #define in a later include
> and consume the stale #define.

Fair enough.

> 
> And I also want to follow the pattern used by kvm-x86-ops.h.

Right.  I forgot to check this.

Reviewed-by: Kai Huang <kai.huang@intel.com>