[PATCH v3 00/15] KVM: x86: Add "governed" X86_FEATURE framework

[PATCH v3 00/15] KVM: x86: Add "governed" X86_FEATURE framework

[Sean Christopherson]

Third and hopefully final version of the framework to manage and cache
KVM-governed features, i.e. CPUID based features that require explicit
KVM enabling and/or need to be queried semi-frequently by KVM.

This version is just the governed features patches, as I kept the TSC
scaling patches in kvm-x86/misc but blasted away the goverend features
with a forced push.

My plan is again to apply this quite quickly so that it can be used as
a base for other series.

v3:
 - "Drop" TSC scaling patches (already applied).
 - Remove dead xsave_enabled code (snuck into v2 by a bad conflict
   resolution). [Zeng]
 - Make kvm_is_governed_feature() return a bool. [Zeng]
 - Collect a review. [Yuan]

v2:
 - https://lore.kernel.org/all/20230729011608.1065019-1-seanjc@google.com
 - Add patches to clean up TSC scaling.
 - Add a comment explaining the virtual VMLOAD/VMLAVE vs. SYSENTER on
   Intel madness.
 - Use a governed feature for X86_FEATURE_VMX.
 - Incorporate KVM capabilities into the main check-and-set helper. [Chao]

v1: https://lore.kernel.org/all/20230217231022.816138-1-seanjc@google.com

Sean Christopherson (15):
  KVM: x86: Add a framework for enabling KVM-governed x86 features
  KVM: x86/mmu: Use KVM-governed feature framework to track "GBPAGES
    enabled"
  KVM: VMX: Recompute "XSAVES enabled" only after CPUID update
  KVM: VMX: Check KVM CPU caps, not just VMX MSR support, for XSAVE
    enabling
  KVM: VMX: Rename XSAVES control to follow KVM's preferred "ENABLE_XYZ"
  KVM: x86: Use KVM-governed feature framework to track "XSAVES enabled"
  KVM: nVMX: Use KVM-governed feature framework to track "nested VMX
    enabled"
  KVM: nSVM: Use KVM-governed feature framework to track "NRIPS enabled"
  KVM: nSVM: Use KVM-governed feature framework to track "TSC scaling
    enabled"
  KVM: nSVM: Use KVM-governed feature framework to track "vVM{SAVE,LOAD}
    enabled"
  KVM: nSVM: Use KVM-governed feature framework to track "LBRv enabled"
  KVM: nSVM: Use KVM-governed feature framework to track "Pause Filter
    enabled"
  KVM: nSVM: Use KVM-governed feature framework to track "vGIF enabled"
  KVM: nSVM: Use KVM-governed feature framework to track "vNMI enabled"
  KVM: x86: Disallow guest CPUID lookups when IRQs are disabled

 arch/x86/include/asm/kvm_host.h  | 20 ++++++++-
 arch/x86/include/asm/vmx.h       |  2 +-
 arch/x86/kvm/cpuid.c             | 34 ++++++++++++++++
 arch/x86/kvm/cpuid.h             | 46 +++++++++++++++++++++
 arch/x86/kvm/governed_features.h | 21 ++++++++++
 arch/x86/kvm/mmu/mmu.c           | 20 ++-------
 arch/x86/kvm/svm/nested.c        | 46 ++++++++++++---------
 arch/x86/kvm/svm/svm.c           | 59 +++++++++++++++------------
 arch/x86/kvm/svm/svm.h           | 16 ++------
 arch/x86/kvm/vmx/capabilities.h  |  2 +-
 arch/x86/kvm/vmx/hyperv.c        |  2 +-
 arch/x86/kvm/vmx/nested.c        | 13 +++---
 arch/x86/kvm/vmx/nested.h        |  2 +-
 arch/x86/kvm/vmx/vmx.c           | 69 ++++++++++++++------------------
 arch/x86/kvm/vmx/vmx.h           |  3 +-
 arch/x86/kvm/x86.c               |  4 +-
 16 files changed, 232 insertions(+), 127 deletions(-)
 create mode 100644 arch/x86/kvm/governed_features.h


base-commit: aaf44a3a699309c77537d0abf49411f9dc7dc523
-- 
2.41.0.694.ge786442a9b-goog

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

[Sean Christopherson]

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

[Huang, Kai]

>  
> +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

[Sean Christopherson]

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

[Huang, Kai]

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>

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

[Yuan Yao]

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

[Binbin Wu]

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

[PATCH v3 02/15] KVM: x86/mmu: Use KVM-governed feature framework to track "GBPAGES enabled"

[Sean Christopherson]

Use the governed feature framework to track whether or not the guest can
use 1GiB pages, and drop the one-off helper that wraps the surprisingly
non-trivial logic surrounding 1GiB page usage in the guest.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/cpuid.c             | 17 +++++++++++++++++
 arch/x86/kvm/governed_features.h |  2 ++
 arch/x86/kvm/mmu/mmu.c           | 20 +++-----------------
 3 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 4ba43ae008cb..67e9f79fe059 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -312,11 +312,28 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
 	struct kvm_cpuid_entry2 *best;
+	bool allow_gbpages;
 
 	BUILD_BUG_ON(KVM_NR_GOVERNED_FEATURES > KVM_MAX_NR_GOVERNED_FEATURES);
 	bitmap_zero(vcpu->arch.governed_features.enabled,
 		    KVM_MAX_NR_GOVERNED_FEATURES);
 
+	/*
+	 * If TDP is enabled, let the guest use GBPAGES if they're supported in
+	 * hardware.  The hardware page walker doesn't let KVM disable GBPAGES,
+	 * i.e. won't treat them as reserved, and KVM doesn't redo the GVA->GPA
+	 * walk for performance and complexity reasons.  Not to mention KVM
+	 * _can't_ solve the problem because GVA->GPA walks aren't visible to
+	 * KVM once a TDP translation is installed.  Mimic hardware behavior so
+	 * that KVM's is at least consistent, i.e. doesn't randomly inject #PF.
+	 * If TDP is disabled, honor *only* guest CPUID as KVM has full control
+	 * and can install smaller shadow pages if the host lacks 1GiB support.
+	 */
+	allow_gbpages = tdp_enabled ? boot_cpu_has(X86_FEATURE_GBPAGES) :
+				      guest_cpuid_has(vcpu, X86_FEATURE_GBPAGES);
+	if (allow_gbpages)
+		kvm_governed_feature_set(vcpu, X86_FEATURE_GBPAGES);
+
 	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/governed_features.h b/arch/x86/kvm/governed_features.h
index 40ce8e6608cd..b29c15d5e038 100644
--- a/arch/x86/kvm/governed_features.h
+++ b/arch/x86/kvm/governed_features.h
@@ -5,5 +5,7 @@ BUILD_BUG()
 
 #define KVM_GOVERNED_X86_FEATURE(x) KVM_GOVERNED_FEATURE(X86_FEATURE_##x)
 
+KVM_GOVERNED_X86_FEATURE(GBPAGES)
+
 #undef KVM_GOVERNED_X86_FEATURE
 #undef KVM_GOVERNED_FEATURE
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 5bdda75bfd10..9e4cd8b4a202 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -4779,28 +4779,13 @@ static void __reset_rsvds_bits_mask(struct rsvd_bits_validate *rsvd_check,
 	}
 }
 
-static bool guest_can_use_gbpages(struct kvm_vcpu *vcpu)
-{
-	/*
-	 * If TDP is enabled, let the guest use GBPAGES if they're supported in
-	 * hardware.  The hardware page walker doesn't let KVM disable GBPAGES,
-	 * i.e. won't treat them as reserved, and KVM doesn't redo the GVA->GPA
-	 * walk for performance and complexity reasons.  Not to mention KVM
-	 * _can't_ solve the problem because GVA->GPA walks aren't visible to
-	 * KVM once a TDP translation is installed.  Mimic hardware behavior so
-	 * that KVM's is at least consistent, i.e. doesn't randomly inject #PF.
-	 */
-	return tdp_enabled ? boot_cpu_has(X86_FEATURE_GBPAGES) :
-			     guest_cpuid_has(vcpu, X86_FEATURE_GBPAGES);
-}
-
 static void reset_guest_rsvds_bits_mask(struct kvm_vcpu *vcpu,
 					struct kvm_mmu *context)
 {
 	__reset_rsvds_bits_mask(&context->guest_rsvd_check,
 				vcpu->arch.reserved_gpa_bits,
 				context->cpu_role.base.level, is_efer_nx(context),
-				guest_can_use_gbpages(vcpu),
+				guest_can_use(vcpu, X86_FEATURE_GBPAGES),
 				is_cr4_pse(context),
 				guest_cpuid_is_amd_or_hygon(vcpu));
 }
@@ -4877,7 +4862,8 @@ static void reset_shadow_zero_bits_mask(struct kvm_vcpu *vcpu,
 	__reset_rsvds_bits_mask(shadow_zero_check, reserved_hpa_bits(),
 				context->root_role.level,
 				context->root_role.efer_nx,
-				guest_can_use_gbpages(vcpu), is_pse, is_amd);
+				guest_can_use(vcpu, X86_FEATURE_GBPAGES),
+				is_pse, is_amd);
 
 	if (!shadow_me_mask)
 		return;
-- 
2.41.0.694.ge786442a9b-goog

Re: [PATCH v3 02/15] KVM: x86/mmu: Use KVM-governed feature framework to track "GBPAGES enabled"

[Yuan Yao]

On Tue, Aug 15, 2023 at 01:36:40PM -0700, Sean Christopherson wrote:
> Use the governed feature framework to track whether or not the guest can
> use 1GiB pages, and drop the one-off helper that wraps the surprisingly
> non-trivial logic surrounding 1GiB page usage in the guest.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/cpuid.c             | 17 +++++++++++++++++
>  arch/x86/kvm/governed_features.h |  2 ++
>  arch/x86/kvm/mmu/mmu.c           | 20 +++-----------------
>  3 files changed, 22 insertions(+), 17 deletions(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 4ba43ae008cb..67e9f79fe059 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -312,11 +312,28 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_lapic *apic = vcpu->arch.apic;
>  	struct kvm_cpuid_entry2 *best;
> +	bool allow_gbpages;
>
>  	BUILD_BUG_ON(KVM_NR_GOVERNED_FEATURES > KVM_MAX_NR_GOVERNED_FEATURES);
>  	bitmap_zero(vcpu->arch.governed_features.enabled,
>  		    KVM_MAX_NR_GOVERNED_FEATURES);
>
> +	/*
> +	 * If TDP is enabled, let the guest use GBPAGES if they're supported in
> +	 * hardware.  The hardware page walker doesn't let KVM disable GBPAGES,
> +	 * i.e. won't treat them as reserved, and KVM doesn't redo the GVA->GPA
> +	 * walk for performance and complexity reasons.  Not to mention KVM
> +	 * _can't_ solve the problem because GVA->GPA walks aren't visible to
> +	 * KVM once a TDP translation is installed.  Mimic hardware behavior so
> +	 * that KVM's is at least consistent, i.e. doesn't randomly inject #PF.
> +	 * If TDP is disabled, honor *only* guest CPUID as KVM has full control
> +	 * and can install smaller shadow pages if the host lacks 1GiB support.
> +	 */
> +	allow_gbpages = tdp_enabled ? boot_cpu_has(X86_FEATURE_GBPAGES) :
> +				      guest_cpuid_has(vcpu, X86_FEATURE_GBPAGES);

tdp_enabled only changes at kvm_configure_mmu() when hardware setup for
VMX and SVM, so:

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

> +	if (allow_gbpages)
> +		kvm_governed_feature_set(vcpu, X86_FEATURE_GBPAGES);
> +
>  	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/governed_features.h b/arch/x86/kvm/governed_features.h
> index 40ce8e6608cd..b29c15d5e038 100644
> --- a/arch/x86/kvm/governed_features.h
> +++ b/arch/x86/kvm/governed_features.h
> @@ -5,5 +5,7 @@ BUILD_BUG()
>
>  #define KVM_GOVERNED_X86_FEATURE(x) KVM_GOVERNED_FEATURE(X86_FEATURE_##x)
>
> +KVM_GOVERNED_X86_FEATURE(GBPAGES)
> +
>  #undef KVM_GOVERNED_X86_FEATURE
>  #undef KVM_GOVERNED_FEATURE
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 5bdda75bfd10..9e4cd8b4a202 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -4779,28 +4779,13 @@ static void __reset_rsvds_bits_mask(struct rsvd_bits_validate *rsvd_check,
>  	}
>  }
>
> -static bool guest_can_use_gbpages(struct kvm_vcpu *vcpu)
> -{
> -	/*
> -	 * If TDP is enabled, let the guest use GBPAGES if they're supported in
> -	 * hardware.  The hardware page walker doesn't let KVM disable GBPAGES,
> -	 * i.e. won't treat them as reserved, and KVM doesn't redo the GVA->GPA
> -	 * walk for performance and complexity reasons.  Not to mention KVM
> -	 * _can't_ solve the problem because GVA->GPA walks aren't visible to
> -	 * KVM once a TDP translation is installed.  Mimic hardware behavior so
> -	 * that KVM's is at least consistent, i.e. doesn't randomly inject #PF.
> -	 */
> -	return tdp_enabled ? boot_cpu_has(X86_FEATURE_GBPAGES) :
> -			     guest_cpuid_has(vcpu, X86_FEATURE_GBPAGES);
> -}
> -
>  static void reset_guest_rsvds_bits_mask(struct kvm_vcpu *vcpu,
>  					struct kvm_mmu *context)
>  {
>  	__reset_rsvds_bits_mask(&context->guest_rsvd_check,
>  				vcpu->arch.reserved_gpa_bits,
>  				context->cpu_role.base.level, is_efer_nx(context),
> -				guest_can_use_gbpages(vcpu),
> +				guest_can_use(vcpu, X86_FEATURE_GBPAGES),
>  				is_cr4_pse(context),
>  				guest_cpuid_is_amd_or_hygon(vcpu));
>  }
> @@ -4877,7 +4862,8 @@ static void reset_shadow_zero_bits_mask(struct kvm_vcpu *vcpu,
>  	__reset_rsvds_bits_mask(shadow_zero_check, reserved_hpa_bits(),
>  				context->root_role.level,
>  				context->root_role.efer_nx,
> -				guest_can_use_gbpages(vcpu), is_pse, is_amd);
> +				guest_can_use(vcpu, X86_FEATURE_GBPAGES),
> +				is_pse, is_amd);
>
>  	if (!shadow_me_mask)
>  		return;
> --
> 2.41.0.694.ge786442a9b-goog
>

[PATCH v3 03/15] KVM: VMX: Recompute "XSAVES enabled" only after CPUID update

[Sean Christopherson]

Recompute whether or not XSAVES is enabled for the guest only if the
guest's CPUID model changes instead of redoing the computation every time
KVM generates vmcs01's secondary execution controls.  The boot_cpu_has()
and cpu_has_vmx_xsaves() checks should never change after KVM is loaded,
and if they do the kernel/KVM is hosed.

Opportunistically add a comment explaining _why_ XSAVES is effectively
exposed to the guest if and only if XSAVE is also exposed to the guest.

Practically speaking, no functional change intended (KVM will do fewer
computations, but should still see the same xsaves_enabled value whenever
KVM looks at it).

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/vmx.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 434bf524e712..1bf85bd53416 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4612,19 +4612,10 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx *vmx)
 	if (!enable_pml || !atomic_read(&vcpu->kvm->nr_memslots_dirty_logging))
 		exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
 
-	if (cpu_has_vmx_xsaves()) {
-		/* Exposing XSAVES only when XSAVE is exposed */
-		bool xsaves_enabled =
-			boot_cpu_has(X86_FEATURE_XSAVE) &&
-			guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
-			guest_cpuid_has(vcpu, X86_FEATURE_XSAVES);
-
-		vcpu->arch.xsaves_enabled = xsaves_enabled;
-
+	if (cpu_has_vmx_xsaves())
 		vmx_adjust_secondary_exec_control(vmx, &exec_control,
 						  SECONDARY_EXEC_XSAVES,
-						  xsaves_enabled, false);
-	}
+						  vcpu->arch.xsaves_enabled, false);
 
 	/*
 	 * RDPID is also gated by ENABLE_RDTSCP, turn on the control if either
@@ -7749,8 +7740,15 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 
-	/* xsaves_enabled is recomputed in vmx_compute_secondary_exec_control(). */
-	vcpu->arch.xsaves_enabled = false;
+	/*
+	 * XSAVES is effectively enabled if and only if XSAVE is also exposed
+	 * to the guest.  XSAVES depends on CR4.OSXSAVE, and CR4.OSXSAVE can be
+	 * set if and only if XSAVE is supported.
+	 */
+	vcpu->arch.xsaves_enabled = cpu_has_vmx_xsaves() &&
+				    boot_cpu_has(X86_FEATURE_XSAVE) &&
+				    guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
+				    guest_cpuid_has(vcpu, X86_FEATURE_XSAVES);
 
 	vmx_setup_uret_msrs(vmx);
 
-- 
2.41.0.694.ge786442a9b-goog

Re: [PATCH v3 03/15] KVM: VMX: Recompute "XSAVES enabled" only after CPUID update

[Yuan Yao]

On Tue, Aug 15, 2023 at 01:36:41PM -0700, Sean Christopherson wrote:
> Recompute whether or not XSAVES is enabled for the guest only if the
> guest's CPUID model changes instead of redoing the computation every time
> KVM generates vmcs01's secondary execution controls.  The boot_cpu_has()
> and cpu_has_vmx_xsaves() checks should never change after KVM is loaded,
> and if they do the kernel/KVM is hosed.
>
> Opportunistically add a comment explaining _why_ XSAVES is effectively
> exposed to the guest if and only if XSAVE is also exposed to the guest.
>
> Practically speaking, no functional change intended (KVM will do fewer
> computations, but should still see the same xsaves_enabled value whenever
> KVM looks at it).
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 434bf524e712..1bf85bd53416 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4612,19 +4612,10 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx *vmx)
>  	if (!enable_pml || !atomic_read(&vcpu->kvm->nr_memslots_dirty_logging))
>  		exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
>
> -	if (cpu_has_vmx_xsaves()) {
> -		/* Exposing XSAVES only when XSAVE is exposed */
> -		bool xsaves_enabled =
> -			boot_cpu_has(X86_FEATURE_XSAVE) &&
> -			guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
> -			guest_cpuid_has(vcpu, X86_FEATURE_XSAVES);
> -
> -		vcpu->arch.xsaves_enabled = xsaves_enabled;
> -
> +	if (cpu_has_vmx_xsaves())
>  		vmx_adjust_secondary_exec_control(vmx, &exec_control,
>  						  SECONDARY_EXEC_XSAVES,
> -						  xsaves_enabled, false);
> -	}
> +						  vcpu->arch.xsaves_enabled, false);

xsaves_enabled is same as before in case of init_vmcs().

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

>
>  	/*
>  	 * RDPID is also gated by ENABLE_RDTSCP, turn on the control if either
> @@ -7749,8 +7740,15 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
>
> -	/* xsaves_enabled is recomputed in vmx_compute_secondary_exec_control(). */
> -	vcpu->arch.xsaves_enabled = false;
> +	/*
> +	 * XSAVES is effectively enabled if and only if XSAVE is also exposed
> +	 * to the guest.  XSAVES depends on CR4.OSXSAVE, and CR4.OSXSAVE can be
> +	 * set if and only if XSAVE is supported.
> +	 */
> +	vcpu->arch.xsaves_enabled = cpu_has_vmx_xsaves() &&
> +				    boot_cpu_has(X86_FEATURE_XSAVE) &&
> +				    guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
> +				    guest_cpuid_has(vcpu, X86_FEATURE_XSAVES);
>
>  	vmx_setup_uret_msrs(vmx);
>
> --
> 2.41.0.694.ge786442a9b-goog
>

[PATCH v3 04/15] KVM: VMX: Check KVM CPU caps, not just VMX MSR support, for XSAVE enabling

[Sean Christopherson]

Check KVM CPU capabilities instead of raw VMX support for XSAVES when
determining whether or not XSAVER can/should be exposed to the guest.
Practically speaking, it's nonsensical/impossible for a CPU to support
"enable XSAVES" without XSAVES being supported natively.  The real
motivation for checking kvm_cpu_cap_has() is to allow using the governed
feature's standard check-and-set logic.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/vmx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 1bf85bd53416..78f292b7e2c5 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7745,7 +7745,7 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 	 * to the guest.  XSAVES depends on CR4.OSXSAVE, and CR4.OSXSAVE can be
 	 * set if and only if XSAVE is supported.
 	 */
-	vcpu->arch.xsaves_enabled = cpu_has_vmx_xsaves() &&
+	vcpu->arch.xsaves_enabled = kvm_cpu_cap_has(X86_FEATURE_XSAVES) &&
 				    boot_cpu_has(X86_FEATURE_XSAVE) &&
 				    guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
 				    guest_cpuid_has(vcpu, X86_FEATURE_XSAVES);
-- 
2.41.0.694.ge786442a9b-goog

Re: [PATCH v3 04/15] KVM: VMX: Check KVM CPU caps, not just VMX MSR support, for XSAVE enabling

[Yuan Yao]

On Tue, Aug 15, 2023 at 01:36:42PM -0700, Sean Christopherson wrote:
> Check KVM CPU capabilities instead of raw VMX support for XSAVES when
> determining whether or not XSAVER can/should be exposed to the guest.
> Practically speaking, it's nonsensical/impossible for a CPU to support
> "enable XSAVES" without XSAVES being supported natively.  The real
> motivation for checking kvm_cpu_cap_has() is to allow using the governed
> feature's standard check-and-set logic.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

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

> ---
>  arch/x86/kvm/vmx/vmx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 1bf85bd53416..78f292b7e2c5 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7745,7 +7745,7 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  	 * to the guest.  XSAVES depends on CR4.OSXSAVE, and CR4.OSXSAVE can be
>  	 * set if and only if XSAVE is supported.
>  	 */
> -	vcpu->arch.xsaves_enabled = cpu_has_vmx_xsaves() &&
> +	vcpu->arch.xsaves_enabled = kvm_cpu_cap_has(X86_FEATURE_XSAVES) &&
>  				    boot_cpu_has(X86_FEATURE_XSAVE) &&
>  				    guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
>  				    guest_cpuid_has(vcpu, X86_FEATURE_XSAVES);
> --
> 2.41.0.694.ge786442a9b-goog
>

[PATCH v3 05/15] KVM: VMX: Rename XSAVES control to follow KVM's preferred "ENABLE_XYZ"

[Sean Christopherson]

Rename the XSAVES secondary execution control to follow KVM's preferred
style so that XSAVES related logic can use common macros that depend on
KVM's preferred style.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/vmx.h      | 2 +-
 arch/x86/kvm/vmx/capabilities.h | 2 +-
 arch/x86/kvm/vmx/hyperv.c       | 2 +-
 arch/x86/kvm/vmx/nested.c       | 6 +++---
 arch/x86/kvm/vmx/nested.h       | 2 +-
 arch/x86/kvm/vmx/vmx.c          | 2 +-
 arch/x86/kvm/vmx/vmx.h          | 2 +-
 7 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
index 0d02c4aafa6f..0e73616b82f3 100644
--- a/arch/x86/include/asm/vmx.h
+++ b/arch/x86/include/asm/vmx.h
@@ -71,7 +71,7 @@
 #define SECONDARY_EXEC_RDSEED_EXITING		VMCS_CONTROL_BIT(RDSEED_EXITING)
 #define SECONDARY_EXEC_ENABLE_PML               VMCS_CONTROL_BIT(PAGE_MOD_LOGGING)
 #define SECONDARY_EXEC_PT_CONCEAL_VMX		VMCS_CONTROL_BIT(PT_CONCEAL_VMX)
-#define SECONDARY_EXEC_XSAVES			VMCS_CONTROL_BIT(XSAVES)
+#define SECONDARY_EXEC_ENABLE_XSAVES		VMCS_CONTROL_BIT(XSAVES)
 #define SECONDARY_EXEC_MODE_BASED_EPT_EXEC	VMCS_CONTROL_BIT(MODE_BASED_EPT_EXEC)
 #define SECONDARY_EXEC_PT_USE_GPA		VMCS_CONTROL_BIT(PT_USE_GPA)
 #define SECONDARY_EXEC_TSC_SCALING              VMCS_CONTROL_BIT(TSC_SCALING)
diff --git a/arch/x86/kvm/vmx/capabilities.h b/arch/x86/kvm/vmx/capabilities.h
index d0abee35d7ba..41a4533f9989 100644
--- a/arch/x86/kvm/vmx/capabilities.h
+++ b/arch/x86/kvm/vmx/capabilities.h
@@ -252,7 +252,7 @@ static inline bool cpu_has_vmx_pml(void)
 static inline bool cpu_has_vmx_xsaves(void)
 {
 	return vmcs_config.cpu_based_2nd_exec_ctrl &
-		SECONDARY_EXEC_XSAVES;
+		SECONDARY_EXEC_ENABLE_XSAVES;
 }
 
 static inline bool cpu_has_vmx_waitpkg(void)
diff --git a/arch/x86/kvm/vmx/hyperv.c b/arch/x86/kvm/vmx/hyperv.c
index 79450e1ed7cf..313b8bb5b8a7 100644
--- a/arch/x86/kvm/vmx/hyperv.c
+++ b/arch/x86/kvm/vmx/hyperv.c
@@ -78,7 +78,7 @@
 	 SECONDARY_EXEC_DESC |						\
 	 SECONDARY_EXEC_ENABLE_RDTSCP |					\
 	 SECONDARY_EXEC_ENABLE_INVPCID |				\
-	 SECONDARY_EXEC_XSAVES |					\
+	 SECONDARY_EXEC_ENABLE_XSAVES |					\
 	 SECONDARY_EXEC_RDSEED_EXITING |				\
 	 SECONDARY_EXEC_RDRAND_EXITING |				\
 	 SECONDARY_EXEC_TSC_SCALING |					\
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 516391cc0d64..22e08d30baef 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2307,7 +2307,7 @@ static void prepare_vmcs02_early(struct vcpu_vmx *vmx, struct loaded_vmcs *vmcs0
 				  SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE |
 				  SECONDARY_EXEC_ENABLE_INVPCID |
 				  SECONDARY_EXEC_ENABLE_RDTSCP |
-				  SECONDARY_EXEC_XSAVES |
+				  SECONDARY_EXEC_ENABLE_XSAVES |
 				  SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE |
 				  SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |
 				  SECONDARY_EXEC_APIC_REGISTER_VIRT |
@@ -6331,7 +6331,7 @@ static bool nested_vmx_l1_wants_exit(struct kvm_vcpu *vcpu,
 		 * If if it were, XSS would have to be checked against
 		 * the XSS exit bitmap in vmcs12.
 		 */
-		return nested_cpu_has2(vmcs12, SECONDARY_EXEC_XSAVES);
+		return nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENABLE_XSAVES);
 	case EXIT_REASON_UMWAIT:
 	case EXIT_REASON_TPAUSE:
 		return nested_cpu_has2(vmcs12,
@@ -6874,7 +6874,7 @@ static void nested_vmx_setup_secondary_ctls(u32 ept_caps,
 		SECONDARY_EXEC_ENABLE_INVPCID |
 		SECONDARY_EXEC_ENABLE_VMFUNC |
 		SECONDARY_EXEC_RDSEED_EXITING |
-		SECONDARY_EXEC_XSAVES |
+		SECONDARY_EXEC_ENABLE_XSAVES |
 		SECONDARY_EXEC_TSC_SCALING |
 		SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE;
 
diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h
index 96952263b029..b4b9d51438c6 100644
--- a/arch/x86/kvm/vmx/nested.h
+++ b/arch/x86/kvm/vmx/nested.h
@@ -168,7 +168,7 @@ static inline int nested_cpu_has_ept(struct vmcs12 *vmcs12)
 
 static inline bool nested_cpu_has_xsaves(struct vmcs12 *vmcs12)
 {
-	return nested_cpu_has2(vmcs12, SECONDARY_EXEC_XSAVES);
+	return nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENABLE_XSAVES);
 }
 
 static inline bool nested_cpu_has_pml(struct vmcs12 *vmcs12)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 78f292b7e2c5..22975cc949b7 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4614,7 +4614,7 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx *vmx)
 
 	if (cpu_has_vmx_xsaves())
 		vmx_adjust_secondary_exec_control(vmx, &exec_control,
-						  SECONDARY_EXEC_XSAVES,
+						  SECONDARY_EXEC_ENABLE_XSAVES,
 						  vcpu->arch.xsaves_enabled, false);
 
 	/*
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index 32384ba38499..cde902b44d97 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -562,7 +562,7 @@ static inline u8 vmx_get_rvi(void)
 	 SECONDARY_EXEC_APIC_REGISTER_VIRT |				\
 	 SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |				\
 	 SECONDARY_EXEC_SHADOW_VMCS |					\
-	 SECONDARY_EXEC_XSAVES |					\
+	 SECONDARY_EXEC_ENABLE_XSAVES |					\
 	 SECONDARY_EXEC_RDSEED_EXITING |				\
 	 SECONDARY_EXEC_RDRAND_EXITING |				\
 	 SECONDARY_EXEC_ENABLE_PML |					\
-- 
2.41.0.694.ge786442a9b-goog

Re: [PATCH v3 05/15] KVM: VMX: Rename XSAVES control to follow KVM's preferred "ENABLE_XYZ"

[Vitaly Kuznetsov]

Sean Christopherson <seanjc@google.com> writes:

> Rename the XSAVES secondary execution control to follow KVM's preferred
> style so that XSAVES related logic can use common macros that depend on
> KVM's preferred style.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/include/asm/vmx.h      | 2 +-
>  arch/x86/kvm/vmx/capabilities.h | 2 +-
>  arch/x86/kvm/vmx/hyperv.c       | 2 +-

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

with a minor comment

>  arch/x86/kvm/vmx/nested.c       | 6 +++---
>  arch/x86/kvm/vmx/nested.h       | 2 +-
>  arch/x86/kvm/vmx/vmx.c          | 2 +-
>  arch/x86/kvm/vmx/vmx.h          | 2 +-
>  7 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
> index 0d02c4aafa6f..0e73616b82f3 100644
> --- a/arch/x86/include/asm/vmx.h
> +++ b/arch/x86/include/asm/vmx.h
> @@ -71,7 +71,7 @@
>  #define SECONDARY_EXEC_RDSEED_EXITING		VMCS_CONTROL_BIT(RDSEED_EXITING)
>  #define SECONDARY_EXEC_ENABLE_PML               VMCS_CONTROL_BIT(PAGE_MOD_LOGGING)
>  #define SECONDARY_EXEC_PT_CONCEAL_VMX		VMCS_CONTROL_BIT(PT_CONCEAL_VMX)
> -#define SECONDARY_EXEC_XSAVES			VMCS_CONTROL_BIT(XSAVES)
> +#define SECONDARY_EXEC_ENABLE_XSAVES		VMCS_CONTROL_BIT(XSAVES)
>  #define SECONDARY_EXEC_MODE_BASED_EPT_EXEC	VMCS_CONTROL_BIT(MODE_BASED_EPT_EXEC)
>  #define SECONDARY_EXEC_PT_USE_GPA		VMCS_CONTROL_BIT(PT_USE_GPA)
>  #define SECONDARY_EXEC_TSC_SCALING              VMCS_CONTROL_BIT(TSC_SCALING)

To avoid the need to make up these names in KVM we can probably just
stick to SDM; that would make it easier to make a connection between KVM
and Intel docs if needed. E.g. SDM uses "Use TSC scaling" so this
could've been "SECONDARY_EXEC_USE_TSC_SCALING" for consistency.

Unfortunatelly, SDM itself is not very consistent in the naming,
e.g. compare "WBINVD exiting"/"RDSEED exiting" with "Enable ENCLS
exiting"/"Enable ENCLV exiting" but I guess we won't be able to do
significantly better in KVM anyways..

> diff --git a/arch/x86/kvm/vmx/capabilities.h b/arch/x86/kvm/vmx/capabilities.h
> index d0abee35d7ba..41a4533f9989 100644
> --- a/arch/x86/kvm/vmx/capabilities.h
> +++ b/arch/x86/kvm/vmx/capabilities.h
> @@ -252,7 +252,7 @@ static inline bool cpu_has_vmx_pml(void)
>  static inline bool cpu_has_vmx_xsaves(void)
>  {
>  	return vmcs_config.cpu_based_2nd_exec_ctrl &
> -		SECONDARY_EXEC_XSAVES;
> +		SECONDARY_EXEC_ENABLE_XSAVES;
>  }
>  
>  static inline bool cpu_has_vmx_waitpkg(void)
> diff --git a/arch/x86/kvm/vmx/hyperv.c b/arch/x86/kvm/vmx/hyperv.c
> index 79450e1ed7cf..313b8bb5b8a7 100644
> --- a/arch/x86/kvm/vmx/hyperv.c
> +++ b/arch/x86/kvm/vmx/hyperv.c
> @@ -78,7 +78,7 @@
>  	 SECONDARY_EXEC_DESC |						\
>  	 SECONDARY_EXEC_ENABLE_RDTSCP |					\
>  	 SECONDARY_EXEC_ENABLE_INVPCID |				\
> -	 SECONDARY_EXEC_XSAVES |					\
> +	 SECONDARY_EXEC_ENABLE_XSAVES |					\
>  	 SECONDARY_EXEC_RDSEED_EXITING |				\
>  	 SECONDARY_EXEC_RDRAND_EXITING |				\
>  	 SECONDARY_EXEC_TSC_SCALING |					\
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 516391cc0d64..22e08d30baef 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2307,7 +2307,7 @@ static void prepare_vmcs02_early(struct vcpu_vmx *vmx, struct loaded_vmcs *vmcs0
>  				  SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE |
>  				  SECONDARY_EXEC_ENABLE_INVPCID |
>  				  SECONDARY_EXEC_ENABLE_RDTSCP |
> -				  SECONDARY_EXEC_XSAVES |
> +				  SECONDARY_EXEC_ENABLE_XSAVES |
>  				  SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE |
>  				  SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |
>  				  SECONDARY_EXEC_APIC_REGISTER_VIRT |
> @@ -6331,7 +6331,7 @@ static bool nested_vmx_l1_wants_exit(struct kvm_vcpu *vcpu,
>  		 * If if it were, XSS would have to be checked against
>  		 * the XSS exit bitmap in vmcs12.
>  		 */
> -		return nested_cpu_has2(vmcs12, SECONDARY_EXEC_XSAVES);
> +		return nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENABLE_XSAVES);
>  	case EXIT_REASON_UMWAIT:
>  	case EXIT_REASON_TPAUSE:
>  		return nested_cpu_has2(vmcs12,
> @@ -6874,7 +6874,7 @@ static void nested_vmx_setup_secondary_ctls(u32 ept_caps,
>  		SECONDARY_EXEC_ENABLE_INVPCID |
>  		SECONDARY_EXEC_ENABLE_VMFUNC |
>  		SECONDARY_EXEC_RDSEED_EXITING |
> -		SECONDARY_EXEC_XSAVES |
> +		SECONDARY_EXEC_ENABLE_XSAVES |
>  		SECONDARY_EXEC_TSC_SCALING |
>  		SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE;
>  
> diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h
> index 96952263b029..b4b9d51438c6 100644
> --- a/arch/x86/kvm/vmx/nested.h
> +++ b/arch/x86/kvm/vmx/nested.h
> @@ -168,7 +168,7 @@ static inline int nested_cpu_has_ept(struct vmcs12 *vmcs12)
>  
>  static inline bool nested_cpu_has_xsaves(struct vmcs12 *vmcs12)
>  {
> -	return nested_cpu_has2(vmcs12, SECONDARY_EXEC_XSAVES);
> +	return nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENABLE_XSAVES);
>  }
>  
>  static inline bool nested_cpu_has_pml(struct vmcs12 *vmcs12)
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 78f292b7e2c5..22975cc949b7 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4614,7 +4614,7 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx *vmx)
>  
>  	if (cpu_has_vmx_xsaves())
>  		vmx_adjust_secondary_exec_control(vmx, &exec_control,
> -						  SECONDARY_EXEC_XSAVES,
> +						  SECONDARY_EXEC_ENABLE_XSAVES,
>  						  vcpu->arch.xsaves_enabled, false);
>  
>  	/*
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index 32384ba38499..cde902b44d97 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -562,7 +562,7 @@ static inline u8 vmx_get_rvi(void)
>  	 SECONDARY_EXEC_APIC_REGISTER_VIRT |				\
>  	 SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |				\
>  	 SECONDARY_EXEC_SHADOW_VMCS |					\
> -	 SECONDARY_EXEC_XSAVES |					\
> +	 SECONDARY_EXEC_ENABLE_XSAVES |					\
>  	 SECONDARY_EXEC_RDSEED_EXITING |				\
>  	 SECONDARY_EXEC_RDRAND_EXITING |				\
>  	 SECONDARY_EXEC_ENABLE_PML |					\



-- 
Vitaly

Re: [PATCH v3 05/15] KVM: VMX: Rename XSAVES control to follow KVM's preferred "ENABLE_XYZ"

[Sean Christopherson]

On Wed, Aug 16, 2023, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@google.com> writes:
> > diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
> > index 0d02c4aafa6f..0e73616b82f3 100644
> > --- a/arch/x86/include/asm/vmx.h
> > +++ b/arch/x86/include/asm/vmx.h
> > @@ -71,7 +71,7 @@
> >  #define SECONDARY_EXEC_RDSEED_EXITING		VMCS_CONTROL_BIT(RDSEED_EXITING)
> >  #define SECONDARY_EXEC_ENABLE_PML               VMCS_CONTROL_BIT(PAGE_MOD_LOGGING)
> >  #define SECONDARY_EXEC_PT_CONCEAL_VMX		VMCS_CONTROL_BIT(PT_CONCEAL_VMX)
> > -#define SECONDARY_EXEC_XSAVES			VMCS_CONTROL_BIT(XSAVES)
> > +#define SECONDARY_EXEC_ENABLE_XSAVES		VMCS_CONTROL_BIT(XSAVES)
> >  #define SECONDARY_EXEC_MODE_BASED_EPT_EXEC	VMCS_CONTROL_BIT(MODE_BASED_EPT_EXEC)
> >  #define SECONDARY_EXEC_PT_USE_GPA		VMCS_CONTROL_BIT(PT_USE_GPA)
> >  #define SECONDARY_EXEC_TSC_SCALING              VMCS_CONTROL_BIT(TSC_SCALING)
> 
> To avoid the need to make up these names in KVM we can probably just
> stick to SDM; that would make it easier to make a connection between KVM
> and Intel docs if needed. E.g. SDM uses "Use TSC scaling" so this
> could've been "SECONDARY_EXEC_USE_TSC_SCALING" for consistency.
> 
> Unfortunatelly, SDM itself is not very consistent in the naming,
> e.g. compare "WBINVD exiting"/"RDSEED exiting" with "Enable ENCLS
> exiting"/"Enable ENCLV exiting" but I guess we won't be able to do
> significantly better in KVM anyways..

Heh, I agree that we should default to whatever the SDM uses, but I disagree with
the assertion that KVM can't do better.  Every once in a while the SDM ends up with
a name that is weirdly different for no obvious reason, or aggressively abbreviated
to the point where the "name" is completely inscrutable without an absurb amount of
context (I've mostly seen this with MSR bits).

In those cases I think we should exercise common sense and deviate from the SDM,
e.g. in the outliers you pointed out, omit the "ENABLE" from the ENCLS/ENCLV flags.

[PATCH v3 06/15] KVM: x86: Use KVM-governed feature framework to track "XSAVES enabled"

[Sean Christopherson]

Use the governed feature framework to track if XSAVES is "enabled", i.e.
if XSAVES can be used by the guest.  Add a comment in the SVM code to
explain the very unintuitive logic of deliberately NOT checking if XSAVES
is enumerated in the guest CPUID model.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/kvm_host.h  |  1 -
 arch/x86/kvm/governed_features.h |  1 +
 arch/x86/kvm/svm/svm.c           | 17 ++++++++++++---
 arch/x86/kvm/vmx/vmx.c           | 36 ++++++++++++++++----------------
 arch/x86/kvm/x86.c               |  4 ++--
 5 files changed, 35 insertions(+), 24 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 60d430b4650f..9f57aa33798b 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -746,7 +746,6 @@ struct kvm_vcpu_arch {
 	u64 smi_count;
 	bool at_instruction_boundary;
 	bool tpr_access_reporting;
-	bool xsaves_enabled;
 	bool xfd_no_write_intercept;
 	u64 ia32_xss;
 	u64 microcode_version;
diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h
index b29c15d5e038..b896a64e4ac3 100644
--- a/arch/x86/kvm/governed_features.h
+++ b/arch/x86/kvm/governed_features.h
@@ -6,6 +6,7 @@ BUILD_BUG()
 #define KVM_GOVERNED_X86_FEATURE(x) KVM_GOVERNED_FEATURE(X86_FEATURE_##x)
 
 KVM_GOVERNED_X86_FEATURE(GBPAGES)
+KVM_GOVERNED_X86_FEATURE(XSAVES)
 
 #undef KVM_GOVERNED_X86_FEATURE
 #undef KVM_GOVERNED_FEATURE
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 6aaa3c7b4578..d67f6e23dcd2 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4273,9 +4273,20 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 	struct vcpu_svm *svm = to_svm(vcpu);
 	struct kvm_cpuid_entry2 *best;
 
-	vcpu->arch.xsaves_enabled = guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
-				    boot_cpu_has(X86_FEATURE_XSAVE) &&
-				    boot_cpu_has(X86_FEATURE_XSAVES);
+	/*
+	 * SVM doesn't provide a way to disable just XSAVES in the guest, KVM
+	 * can only disable all variants of by disallowing CR4.OSXSAVE from
+	 * being set.  As a result, if the host has XSAVE and XSAVES, and the
+	 * guest has XSAVE enabled, the guest can execute XSAVES without
+	 * faulting.  Treat XSAVES as enabled in this case regardless of
+	 * whether it's advertised to the guest so that KVM context switches
+	 * XSS on VM-Enter/VM-Exit.  Failure to do so would effectively give
+	 * the guest read/write access to the host's XSS.
+	 */
+	if (boot_cpu_has(X86_FEATURE_XSAVE) &&
+	    boot_cpu_has(X86_FEATURE_XSAVES) &&
+	    guest_cpuid_has(vcpu, X86_FEATURE_XSAVE))
+		kvm_governed_feature_set(vcpu, X86_FEATURE_XSAVES);
 
 	/* Update nrips enabled cache */
 	svm->nrips_enabled = kvm_cpu_cap_has(X86_FEATURE_NRIPS) &&
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 22975cc949b7..6314ca32a5cf 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4543,16 +4543,19 @@ vmx_adjust_secondary_exec_control(struct vcpu_vmx *vmx, u32 *exec_control,
  * based on a single guest CPUID bit, with a dedicated feature bit.  This also
  * verifies that the control is actually supported by KVM and hardware.
  */
-#define vmx_adjust_sec_exec_control(vmx, exec_control, name, feat_name, ctrl_name, exiting) \
-({									 \
-	bool __enabled;							 \
-									 \
-	if (cpu_has_vmx_##name()) {					 \
-		__enabled = guest_cpuid_has(&(vmx)->vcpu,		 \
-					    X86_FEATURE_##feat_name);	 \
-		vmx_adjust_secondary_exec_control(vmx, exec_control,	 \
-			SECONDARY_EXEC_##ctrl_name, __enabled, exiting); \
-	}								 \
+#define vmx_adjust_sec_exec_control(vmx, exec_control, name, feat_name, ctrl_name, exiting)	\
+({												\
+	struct kvm_vcpu *__vcpu = &(vmx)->vcpu;							\
+	bool __enabled;										\
+												\
+	if (cpu_has_vmx_##name()) {								\
+		if (kvm_is_governed_feature(X86_FEATURE_##feat_name))				\
+			__enabled = guest_can_use(__vcpu, X86_FEATURE_##feat_name);		\
+		else										\
+			__enabled = guest_cpuid_has(__vcpu, X86_FEATURE_##feat_name);		\
+		vmx_adjust_secondary_exec_control(vmx, exec_control, SECONDARY_EXEC_##ctrl_name,\
+						  __enabled, exiting);				\
+	}											\
 })
 
 /* More macro magic for ENABLE_/opt-in versus _EXITING/opt-out controls. */
@@ -4612,10 +4615,7 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx *vmx)
 	if (!enable_pml || !atomic_read(&vcpu->kvm->nr_memslots_dirty_logging))
 		exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
 
-	if (cpu_has_vmx_xsaves())
-		vmx_adjust_secondary_exec_control(vmx, &exec_control,
-						  SECONDARY_EXEC_ENABLE_XSAVES,
-						  vcpu->arch.xsaves_enabled, false);
+	vmx_adjust_sec_exec_feature(vmx, &exec_control, xsaves, XSAVES);
 
 	/*
 	 * RDPID is also gated by ENABLE_RDTSCP, turn on the control if either
@@ -4634,6 +4634,7 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx *vmx)
 						  SECONDARY_EXEC_ENABLE_RDTSCP,
 						  rdpid_or_rdtscp_enabled, false);
 	}
+
 	vmx_adjust_sec_exec_feature(vmx, &exec_control, invpcid, INVPCID);
 
 	vmx_adjust_sec_exec_exiting(vmx, &exec_control, rdrand, RDRAND);
@@ -7745,10 +7746,9 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 	 * to the guest.  XSAVES depends on CR4.OSXSAVE, and CR4.OSXSAVE can be
 	 * set if and only if XSAVE is supported.
 	 */
-	vcpu->arch.xsaves_enabled = kvm_cpu_cap_has(X86_FEATURE_XSAVES) &&
-				    boot_cpu_has(X86_FEATURE_XSAVE) &&
-				    guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
-				    guest_cpuid_has(vcpu, X86_FEATURE_XSAVES);
+	if (boot_cpu_has(X86_FEATURE_XSAVE) &&
+	    guest_cpuid_has(vcpu, X86_FEATURE_XSAVE))
+		kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_XSAVES);
 
 	vmx_setup_uret_msrs(vmx);
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index eba35d43e3fe..34945c7dba38 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1016,7 +1016,7 @@ void kvm_load_guest_xsave_state(struct kvm_vcpu *vcpu)
 		if (vcpu->arch.xcr0 != host_xcr0)
 			xsetbv(XCR_XFEATURE_ENABLED_MASK, vcpu->arch.xcr0);
 
-		if (vcpu->arch.xsaves_enabled &&
+		if (guest_can_use(vcpu, X86_FEATURE_XSAVES) &&
 		    vcpu->arch.ia32_xss != host_xss)
 			wrmsrl(MSR_IA32_XSS, vcpu->arch.ia32_xss);
 	}
@@ -1047,7 +1047,7 @@ void kvm_load_host_xsave_state(struct kvm_vcpu *vcpu)
 		if (vcpu->arch.xcr0 != host_xcr0)
 			xsetbv(XCR_XFEATURE_ENABLED_MASK, host_xcr0);
 
-		if (vcpu->arch.xsaves_enabled &&
+		if (guest_can_use(vcpu, X86_FEATURE_XSAVES) &&
 		    vcpu->arch.ia32_xss != host_xss)
 			wrmsrl(MSR_IA32_XSS, host_xss);
 	}
-- 
2.41.0.694.ge786442a9b-goog

Re: [PATCH v3 06/15] KVM: x86: Use KVM-governed feature framework to track "XSAVES enabled"

[Yuan Yao]

On Tue, Aug 15, 2023 at 01:36:44PM -0700, Sean Christopherson wrote:
> Use the governed feature framework to track if XSAVES is "enabled", i.e.
> if XSAVES can be used by the guest.  Add a comment in the SVM code to
> explain the very unintuitive logic of deliberately NOT checking if XSAVES
> is enumerated in the guest CPUID model.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/include/asm/kvm_host.h  |  1 -
>  arch/x86/kvm/governed_features.h |  1 +
>  arch/x86/kvm/svm/svm.c           | 17 ++++++++++++---
>  arch/x86/kvm/vmx/vmx.c           | 36 ++++++++++++++++----------------
>  arch/x86/kvm/x86.c               |  4 ++--
>  5 files changed, 35 insertions(+), 24 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 60d430b4650f..9f57aa33798b 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -746,7 +746,6 @@ struct kvm_vcpu_arch {
>  	u64 smi_count;
>  	bool at_instruction_boundary;
>  	bool tpr_access_reporting;
> -	bool xsaves_enabled;
>  	bool xfd_no_write_intercept;
>  	u64 ia32_xss;
>  	u64 microcode_version;
> diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h
> index b29c15d5e038..b896a64e4ac3 100644
> --- a/arch/x86/kvm/governed_features.h
> +++ b/arch/x86/kvm/governed_features.h
> @@ -6,6 +6,7 @@ BUILD_BUG()
>  #define KVM_GOVERNED_X86_FEATURE(x) KVM_GOVERNED_FEATURE(X86_FEATURE_##x)
>
>  KVM_GOVERNED_X86_FEATURE(GBPAGES)
> +KVM_GOVERNED_X86_FEATURE(XSAVES)
>
>  #undef KVM_GOVERNED_X86_FEATURE
>  #undef KVM_GOVERNED_FEATURE
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 6aaa3c7b4578..d67f6e23dcd2 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4273,9 +4273,20 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  	struct kvm_cpuid_entry2 *best;
>
> -	vcpu->arch.xsaves_enabled = guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
> -				    boot_cpu_has(X86_FEATURE_XSAVE) &&
> -				    boot_cpu_has(X86_FEATURE_XSAVES);
> +	/*
> +	 * SVM doesn't provide a way to disable just XSAVES in the guest, KVM
> +	 * can only disable all variants of by disallowing CR4.OSXSAVE from
> +	 * being set.  As a result, if the host has XSAVE and XSAVES, and the
> +	 * guest has XSAVE enabled, the guest can execute XSAVES without
> +	 * faulting.  Treat XSAVES as enabled in this case regardless of
> +	 * whether it's advertised to the guest so that KVM context switches
> +	 * XSS on VM-Enter/VM-Exit.  Failure to do so would effectively give
> +	 * the guest read/write access to the host's XSS.
> +	 */
> +	if (boot_cpu_has(X86_FEATURE_XSAVE) &&
> +	    boot_cpu_has(X86_FEATURE_XSAVES) &&
> +	    guest_cpuid_has(vcpu, X86_FEATURE_XSAVE))
> +		kvm_governed_feature_set(vcpu, X86_FEATURE_XSAVES);
>
>  	/* Update nrips enabled cache */
>  	svm->nrips_enabled = kvm_cpu_cap_has(X86_FEATURE_NRIPS) &&
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 22975cc949b7..6314ca32a5cf 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4543,16 +4543,19 @@ vmx_adjust_secondary_exec_control(struct vcpu_vmx *vmx, u32 *exec_control,
>   * based on a single guest CPUID bit, with a dedicated feature bit.  This also
>   * verifies that the control is actually supported by KVM and hardware.
>   */
> -#define vmx_adjust_sec_exec_control(vmx, exec_control, name, feat_name, ctrl_name, exiting) \
> -({									 \
> -	bool __enabled;							 \
> -									 \
> -	if (cpu_has_vmx_##name()) {					 \
> -		__enabled = guest_cpuid_has(&(vmx)->vcpu,		 \
> -					    X86_FEATURE_##feat_name);	 \
> -		vmx_adjust_secondary_exec_control(vmx, exec_control,	 \
> -			SECONDARY_EXEC_##ctrl_name, __enabled, exiting); \
> -	}								 \
> +#define vmx_adjust_sec_exec_control(vmx, exec_control, name, feat_name, ctrl_name, exiting)	\
> +({												\
> +	struct kvm_vcpu *__vcpu = &(vmx)->vcpu;							\
> +	bool __enabled;										\
> +												\
> +	if (cpu_has_vmx_##name()) {								\
> +		if (kvm_is_governed_feature(X86_FEATURE_##feat_name))				\
> +			__enabled = guest_can_use(__vcpu, X86_FEATURE_##feat_name);		\
> +		else										\
> +			__enabled = guest_cpuid_has(__vcpu, X86_FEATURE_##feat_name);		\
> +		vmx_adjust_secondary_exec_control(vmx, exec_control, SECONDARY_EXEC_##ctrl_name,\
> +						  __enabled, exiting);				\
> +	}											\
>  })
>
>  /* More macro magic for ENABLE_/opt-in versus _EXITING/opt-out controls. */
> @@ -4612,10 +4615,7 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx *vmx)
>  	if (!enable_pml || !atomic_read(&vcpu->kvm->nr_memslots_dirty_logging))
>  		exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
>
> -	if (cpu_has_vmx_xsaves())
> -		vmx_adjust_secondary_exec_control(vmx, &exec_control,
> -						  SECONDARY_EXEC_ENABLE_XSAVES,
> -						  vcpu->arch.xsaves_enabled, false);
> +	vmx_adjust_sec_exec_feature(vmx, &exec_control, xsaves, XSAVES);
>
>  	/*
>  	 * RDPID is also gated by ENABLE_RDTSCP, turn on the control if either
> @@ -4634,6 +4634,7 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx *vmx)
>  						  SECONDARY_EXEC_ENABLE_RDTSCP,
>  						  rdpid_or_rdtscp_enabled, false);
>  	}
> +
>  	vmx_adjust_sec_exec_feature(vmx, &exec_control, invpcid, INVPCID);
>
>  	vmx_adjust_sec_exec_exiting(vmx, &exec_control, rdrand, RDRAND);
> @@ -7745,10 +7746,9 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  	 * to the guest.  XSAVES depends on CR4.OSXSAVE, and CR4.OSXSAVE can be
>  	 * set if and only if XSAVE is supported.
>  	 */
> -	vcpu->arch.xsaves_enabled = kvm_cpu_cap_has(X86_FEATURE_XSAVES) &&
> -				    boot_cpu_has(X86_FEATURE_XSAVE) &&
> -				    guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
> -				    guest_cpuid_has(vcpu, X86_FEATURE_XSAVES);
> +	if (boot_cpu_has(X86_FEATURE_XSAVE) &&
> +	    guest_cpuid_has(vcpu, X86_FEATURE_XSAVE))

Should above 2 be X86_FEATURE_XSAVES ? XSAVE and XSAVES have different
cpuid definition.
Otherwise X86_FEATURE_XSAVES is allowed in governor even XSAVES
is not exposed to guest cpuid, with unnecessary context switches.

> +		kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_XSAVES);
>
>  	vmx_setup_uret_msrs(vmx);
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index eba35d43e3fe..34945c7dba38 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1016,7 +1016,7 @@ void kvm_load_guest_xsave_state(struct kvm_vcpu *vcpu)
>  		if (vcpu->arch.xcr0 != host_xcr0)
>  			xsetbv(XCR_XFEATURE_ENABLED_MASK, vcpu->arch.xcr0);
>
> -		if (vcpu->arch.xsaves_enabled &&
> +		if (guest_can_use(vcpu, X86_FEATURE_XSAVES) &&
>  		    vcpu->arch.ia32_xss != host_xss)
>  			wrmsrl(MSR_IA32_XSS, vcpu->arch.ia32_xss);
>  	}
> @@ -1047,7 +1047,7 @@ void kvm_load_host_xsave_state(struct kvm_vcpu *vcpu)
>  		if (vcpu->arch.xcr0 != host_xcr0)
>  			xsetbv(XCR_XFEATURE_ENABLED_MASK, host_xcr0);
>
> -		if (vcpu->arch.xsaves_enabled &&
> +		if (guest_can_use(vcpu, X86_FEATURE_XSAVES) &&
>  		    vcpu->arch.ia32_xss != host_xss)
>  			wrmsrl(MSR_IA32_XSS, host_xss);
>  	}
> --
> 2.41.0.694.ge786442a9b-goog
>

Re: [PATCH v3 06/15] KVM: x86: Use KVM-governed feature framework to track "XSAVES enabled"

[Yuan Yao]

On Wed, Aug 16, 2023 at 10:58:41AM +0800, Yuan Yao wrote:
> On Tue, Aug 15, 2023 at 01:36:44PM -0700, Sean Christopherson wrote:
> > Use the governed feature framework to track if XSAVES is "enabled", i.e.
> > if XSAVES can be used by the guest.  Add a comment in the SVM code to
> > explain the very unintuitive logic of deliberately NOT checking if XSAVES
> > is enumerated in the guest CPUID model.
> >
> > No functional change intended.
> >
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >  arch/x86/include/asm/kvm_host.h  |  1 -
> >  arch/x86/kvm/governed_features.h |  1 +
> >  arch/x86/kvm/svm/svm.c           | 17 ++++++++++++---
> >  arch/x86/kvm/vmx/vmx.c           | 36 ++++++++++++++++----------------
> >  arch/x86/kvm/x86.c               |  4 ++--
> >  5 files changed, 35 insertions(+), 24 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 60d430b4650f..9f57aa33798b 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -746,7 +746,6 @@ struct kvm_vcpu_arch {
> >  	u64 smi_count;
> >  	bool at_instruction_boundary;
> >  	bool tpr_access_reporting;
> > -	bool xsaves_enabled;
> >  	bool xfd_no_write_intercept;
> >  	u64 ia32_xss;
> >  	u64 microcode_version;
> > diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h
> > index b29c15d5e038..b896a64e4ac3 100644
> > --- a/arch/x86/kvm/governed_features.h
> > +++ b/arch/x86/kvm/governed_features.h
> > @@ -6,6 +6,7 @@ BUILD_BUG()
> >  #define KVM_GOVERNED_X86_FEATURE(x) KVM_GOVERNED_FEATURE(X86_FEATURE_##x)
> >
> >  KVM_GOVERNED_X86_FEATURE(GBPAGES)
> > +KVM_GOVERNED_X86_FEATURE(XSAVES)
> >
> >  #undef KVM_GOVERNED_X86_FEATURE
> >  #undef KVM_GOVERNED_FEATURE
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index 6aaa3c7b4578..d67f6e23dcd2 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -4273,9 +4273,20 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> >  	struct vcpu_svm *svm = to_svm(vcpu);
> >  	struct kvm_cpuid_entry2 *best;
> >
> > -	vcpu->arch.xsaves_enabled = guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
> > -				    boot_cpu_has(X86_FEATURE_XSAVE) &&
> > -				    boot_cpu_has(X86_FEATURE_XSAVES);
> > +	/*
> > +	 * SVM doesn't provide a way to disable just XSAVES in the guest, KVM
> > +	 * can only disable all variants of by disallowing CR4.OSXSAVE from
> > +	 * being set.  As a result, if the host has XSAVE and XSAVES, and the
> > +	 * guest has XSAVE enabled, the guest can execute XSAVES without
> > +	 * faulting.  Treat XSAVES as enabled in this case regardless of
> > +	 * whether it's advertised to the guest so that KVM context switches
> > +	 * XSS on VM-Enter/VM-Exit.  Failure to do so would effectively give
> > +	 * the guest read/write access to the host's XSS.
> > +	 */
> > +	if (boot_cpu_has(X86_FEATURE_XSAVE) &&
> > +	    boot_cpu_has(X86_FEATURE_XSAVES) &&
> > +	    guest_cpuid_has(vcpu, X86_FEATURE_XSAVE))
> > +		kvm_governed_feature_set(vcpu, X86_FEATURE_XSAVES);
> >
> >  	/* Update nrips enabled cache */
> >  	svm->nrips_enabled = kvm_cpu_cap_has(X86_FEATURE_NRIPS) &&
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 22975cc949b7..6314ca32a5cf 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -4543,16 +4543,19 @@ vmx_adjust_secondary_exec_control(struct vcpu_vmx *vmx, u32 *exec_control,
> >   * based on a single guest CPUID bit, with a dedicated feature bit.  This also
> >   * verifies that the control is actually supported by KVM and hardware.
> >   */
> > -#define vmx_adjust_sec_exec_control(vmx, exec_control, name, feat_name, ctrl_name, exiting) \
> > -({									 \
> > -	bool __enabled;							 \
> > -									 \
> > -	if (cpu_has_vmx_##name()) {					 \
> > -		__enabled = guest_cpuid_has(&(vmx)->vcpu,		 \
> > -					    X86_FEATURE_##feat_name);	 \
> > -		vmx_adjust_secondary_exec_control(vmx, exec_control,	 \
> > -			SECONDARY_EXEC_##ctrl_name, __enabled, exiting); \
> > -	}								 \
> > +#define vmx_adjust_sec_exec_control(vmx, exec_control, name, feat_name, ctrl_name, exiting)	\
> > +({												\
> > +	struct kvm_vcpu *__vcpu = &(vmx)->vcpu;							\
> > +	bool __enabled;										\
> > +												\
> > +	if (cpu_has_vmx_##name()) {								\
> > +		if (kvm_is_governed_feature(X86_FEATURE_##feat_name))				\
> > +			__enabled = guest_can_use(__vcpu, X86_FEATURE_##feat_name);		\
> > +		else										\
> > +			__enabled = guest_cpuid_has(__vcpu, X86_FEATURE_##feat_name);		\
> > +		vmx_adjust_secondary_exec_control(vmx, exec_control, SECONDARY_EXEC_##ctrl_name,\
> > +						  __enabled, exiting);				\
> > +	}											\
> >  })
> >
> >  /* More macro magic for ENABLE_/opt-in versus _EXITING/opt-out controls. */
> > @@ -4612,10 +4615,7 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx *vmx)
> >  	if (!enable_pml || !atomic_read(&vcpu->kvm->nr_memslots_dirty_logging))
> >  		exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
> >
> > -	if (cpu_has_vmx_xsaves())
> > -		vmx_adjust_secondary_exec_control(vmx, &exec_control,
> > -						  SECONDARY_EXEC_ENABLE_XSAVES,
> > -						  vcpu->arch.xsaves_enabled, false);
> > +	vmx_adjust_sec_exec_feature(vmx, &exec_control, xsaves, XSAVES);
> >
> >  	/*
> >  	 * RDPID is also gated by ENABLE_RDTSCP, turn on the control if either
> > @@ -4634,6 +4634,7 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx *vmx)
> >  						  SECONDARY_EXEC_ENABLE_RDTSCP,
> >  						  rdpid_or_rdtscp_enabled, false);
> >  	}
> > +
> >  	vmx_adjust_sec_exec_feature(vmx, &exec_control, invpcid, INVPCID);
> >
> >  	vmx_adjust_sec_exec_exiting(vmx, &exec_control, rdrand, RDRAND);
> > @@ -7745,10 +7746,9 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> >  	 * to the guest.  XSAVES depends on CR4.OSXSAVE, and CR4.OSXSAVE can be
> >  	 * set if and only if XSAVE is supported.
> >  	 */
> > -	vcpu->arch.xsaves_enabled = kvm_cpu_cap_has(X86_FEATURE_XSAVES) &&
> > -				    boot_cpu_has(X86_FEATURE_XSAVE) &&
> > -				    guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
> > -				    guest_cpuid_has(vcpu, X86_FEATURE_XSAVES);
> > +	if (boot_cpu_has(X86_FEATURE_XSAVE) &&
> > +	    guest_cpuid_has(vcpu, X86_FEATURE_XSAVE))
>
> Should above 2 be X86_FEATURE_XSAVES ? XSAVE and XSAVES have different
> cpuid definition.
> Otherwise X86_FEATURE_XSAVES is allowed in governor even XSAVES
> is not exposed to guest cpuid, with unnecessary context switches.

Oh! false alarm.
I just forgot that kvm_governed_feature_check_and_set() does checks
on kvm cpu cap and guest cpuid set, thus no problem.

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

>
> > +		kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_XSAVES);
> >
> >  	vmx_setup_uret_msrs(vmx);
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index eba35d43e3fe..34945c7dba38 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -1016,7 +1016,7 @@ void kvm_load_guest_xsave_state(struct kvm_vcpu *vcpu)
> >  		if (vcpu->arch.xcr0 != host_xcr0)
> >  			xsetbv(XCR_XFEATURE_ENABLED_MASK, vcpu->arch.xcr0);
> >
> > -		if (vcpu->arch.xsaves_enabled &&
> > +		if (guest_can_use(vcpu, X86_FEATURE_XSAVES) &&
> >  		    vcpu->arch.ia32_xss != host_xss)
> >  			wrmsrl(MSR_IA32_XSS, vcpu->arch.ia32_xss);
> >  	}
> > @@ -1047,7 +1047,7 @@ void kvm_load_host_xsave_state(struct kvm_vcpu *vcpu)
> >  		if (vcpu->arch.xcr0 != host_xcr0)
> >  			xsetbv(XCR_XFEATURE_ENABLED_MASK, host_xcr0);
> >
> > -		if (vcpu->arch.xsaves_enabled &&
> > +		if (guest_can_use(vcpu, X86_FEATURE_XSAVES) &&
> >  		    vcpu->arch.ia32_xss != host_xss)
> >  			wrmsrl(MSR_IA32_XSS, host_xss);
> >  	}
> > --
> > 2.41.0.694.ge786442a9b-goog
> >

[PATCH v3 07/15] KVM: nVMX: Use KVM-governed feature framework to track "nested VMX enabled"

[Sean Christopherson]

Track "VMX exposed to L1" via a governed feature flag instead of using a
dedicated helper to provide the same functionality.  The main goal is to
drive convergence between VMX and SVM with respect to querying features
that are controllable via module param (SVM likes to cache nested
features), avoiding the guest CPUID lookups at runtime is just a bonus
and unlikely to provide any meaningful performance benefits.

No functional change intended.

Reviewed-by: Yuan Yao <yuan.yao@intel.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/governed_features.h |  1 +
 arch/x86/kvm/vmx/nested.c        |  7 ++++---
 arch/x86/kvm/vmx/vmx.c           | 21 ++++++---------------
 arch/x86/kvm/vmx/vmx.h           |  1 -
 4 files changed, 11 insertions(+), 19 deletions(-)

diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h
index b896a64e4ac3..22446614bf49 100644
--- a/arch/x86/kvm/governed_features.h
+++ b/arch/x86/kvm/governed_features.h
@@ -7,6 +7,7 @@ BUILD_BUG()
 
 KVM_GOVERNED_X86_FEATURE(GBPAGES)
 KVM_GOVERNED_X86_FEATURE(XSAVES)
+KVM_GOVERNED_X86_FEATURE(VMX)
 
 #undef KVM_GOVERNED_X86_FEATURE
 #undef KVM_GOVERNED_FEATURE
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 22e08d30baef..c5ec0ef51ff7 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -6426,7 +6426,7 @@ static int vmx_get_nested_state(struct kvm_vcpu *vcpu,
 	vmx = to_vmx(vcpu);
 	vmcs12 = get_vmcs12(vcpu);
 
-	if (nested_vmx_allowed(vcpu) &&
+	if (guest_can_use(vcpu, X86_FEATURE_VMX) &&
 	    (vmx->nested.vmxon || vmx->nested.smm.vmxon)) {
 		kvm_state.hdr.vmx.vmxon_pa = vmx->nested.vmxon_ptr;
 		kvm_state.hdr.vmx.vmcs12_pa = vmx->nested.current_vmptr;
@@ -6567,7 +6567,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
 		if (kvm_state->flags & ~KVM_STATE_NESTED_EVMCS)
 			return -EINVAL;
 	} else {
-		if (!nested_vmx_allowed(vcpu))
+		if (!guest_can_use(vcpu, X86_FEATURE_VMX))
 			return -EINVAL;
 
 		if (!page_address_valid(vcpu, kvm_state->hdr.vmx.vmxon_pa))
@@ -6601,7 +6601,8 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
 		return -EINVAL;
 
 	if ((kvm_state->flags & KVM_STATE_NESTED_EVMCS) &&
-		(!nested_vmx_allowed(vcpu) || !vmx->nested.enlightened_vmcs_enabled))
+	    (!guest_can_use(vcpu, X86_FEATURE_VMX) ||
+	     !vmx->nested.enlightened_vmcs_enabled))
 			return -EINVAL;
 
 	vmx_leave_nested(vcpu);
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 6314ca32a5cf..caeb415eb5a3 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -1908,17 +1908,6 @@ static void vmx_write_tsc_multiplier(struct kvm_vcpu *vcpu)
 	vmcs_write64(TSC_MULTIPLIER, vcpu->arch.tsc_scaling_ratio);
 }
 
-/*
- * nested_vmx_allowed() checks whether a guest should be allowed to use VMX
- * instructions and MSRs (i.e., nested VMX). Nested VMX is disabled for
- * all guests if the "nested" module option is off, and can also be disabled
- * for a single guest by disabling its VMX cpuid bit.
- */
-bool nested_vmx_allowed(struct kvm_vcpu *vcpu)
-{
-	return nested && guest_cpuid_has(vcpu, X86_FEATURE_VMX);
-}
-
 /*
  * Userspace is allowed to set any supported IA32_FEATURE_CONTROL regardless of
  * guest CPUID.  Note, KVM allows userspace to set "VMX in SMX" to maintain
@@ -2046,7 +2035,7 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			[msr_info->index - MSR_IA32_SGXLEPUBKEYHASH0];
 		break;
 	case KVM_FIRST_EMULATED_VMX_MSR ... KVM_LAST_EMULATED_VMX_MSR:
-		if (!nested_vmx_allowed(vcpu))
+		if (!guest_can_use(vcpu, X86_FEATURE_VMX))
 			return 1;
 		if (vmx_get_vmx_msr(&vmx->nested.msrs, msr_info->index,
 				    &msr_info->data))
@@ -2354,7 +2343,7 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case KVM_FIRST_EMULATED_VMX_MSR ... KVM_LAST_EMULATED_VMX_MSR:
 		if (!msr_info->host_initiated)
 			return 1; /* they are read-only */
-		if (!nested_vmx_allowed(vcpu))
+		if (!guest_can_use(vcpu, X86_FEATURE_VMX))
 			return 1;
 		return vmx_set_vmx_msr(vcpu, msr_index, data);
 	case MSR_IA32_RTIT_CTL:
@@ -7750,13 +7739,15 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 	    guest_cpuid_has(vcpu, X86_FEATURE_XSAVE))
 		kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_XSAVES);
 
+	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_VMX);
+
 	vmx_setup_uret_msrs(vmx);
 
 	if (cpu_has_secondary_exec_ctrls())
 		vmcs_set_secondary_exec_control(vmx,
 						vmx_secondary_exec_control(vmx));
 
-	if (nested_vmx_allowed(vcpu))
+	if (guest_can_use(vcpu, X86_FEATURE_VMX))
 		vmx->msr_ia32_feature_control_valid_bits |=
 			FEAT_CTL_VMX_ENABLED_INSIDE_SMX |
 			FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX;
@@ -7765,7 +7756,7 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 			~(FEAT_CTL_VMX_ENABLED_INSIDE_SMX |
 			  FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX);
 
-	if (nested_vmx_allowed(vcpu))
+	if (guest_can_use(vcpu, X86_FEATURE_VMX))
 		nested_vmx_cr_fixed1_bits_update(vcpu);
 
 	if (boot_cpu_has(X86_FEATURE_INTEL_PT) &&
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index cde902b44d97..c2130d2c8e24 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -374,7 +374,6 @@ struct kvm_vmx {
 	u64 *pid_table;
 };
 
-bool nested_vmx_allowed(struct kvm_vcpu *vcpu);
 void vmx_vcpu_load_vmcs(struct kvm_vcpu *vcpu, int cpu,
 			struct loaded_vmcs *buddy);
 int allocate_vpid(void);
-- 
2.41.0.694.ge786442a9b-goog

Re: [PATCH v3 07/15] KVM: nVMX: Use KVM-governed feature framework to track "nested VMX enabled"

[Huang, Kai]

On Tue, 2023-08-15 at 13:36 -0700, Sean Christopherson wrote:
> Track "VMX exposed to L1" via a governed feature flag instead of using a
> dedicated helper to provide the same functionality.  The main goal is to
> drive convergence between VMX and SVM with respect to querying features
> that are controllable via module param (SVM likes to cache nested
> features), avoiding the guest CPUID lookups at runtime is just a bonus
> and unlikely to provide any meaningful performance benefits.
> 
> No functional change intended.
> 
> Reviewed-by: Yuan Yao <yuan.yao@intel.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

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

Re: [PATCH v3 07/15] KVM: nVMX: Use KVM-governed feature framework to track "nested VMX enabled"

[Huang, Kai]

On Tue, 2023-08-15 at 13:36 -0700, Sean Christopherson wrote:
> Track "VMX exposed to L1" via a governed feature flag instead of using a
> dedicated helper to provide the same functionality.  The main goal is to
> drive convergence between VMX and SVM with respect to querying features
> that are controllable via module param (SVM likes to cache nested
> features), avoiding the guest CPUID lookups at runtime is just a bonus
> and unlikely to provide any meaningful performance benefits.
> 
> No functional change intended.
> 
> 
[...]

>  
> -/*
> - * nested_vmx_allowed() checks whether a guest should be allowed to use VMX
> - * instructions and MSRs (i.e., nested VMX). Nested VMX is disabled for
> - * all guests if the "nested" module option is off, and can also be disabled
> - * for a single guest by disabling its VMX cpuid bit.
> - */
> -bool nested_vmx_allowed(struct kvm_vcpu *vcpu)
> -{
> -	return nested && guest_cpuid_has(vcpu, X86_FEATURE_VMX);
> -}
> -
> 

[...]

> @@ -7750,13 +7739,15 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  	    guest_cpuid_has(vcpu, X86_FEATURE_XSAVE))
>  		kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_XSAVES);
>  
> +	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_VMX);
> +
> 

Nit:

nested_vmx_allowed() also checks 'nested' global variable.  However
kvm_governed_feature_check_and_set() is called unconditionally.  Although IIUC
it should never actually set the VMX governed bit when 'nested=0', it's not that
obvious in _this_ patch.

Should we explicitly call this out in the changelog so git blamers can
understand this more easily in the future?

Re: [PATCH v3 07/15] KVM: nVMX: Use KVM-governed feature framework to track "nested VMX enabled"

[Sean Christopherson]

On Wed, Aug 16, 2023, Kai Huang wrote:
> On Tue, 2023-08-15 at 13:36 -0700, Sean Christopherson wrote:
> > Track "VMX exposed to L1" via a governed feature flag instead of using a
> > dedicated helper to provide the same functionality.  The main goal is to
> > drive convergence between VMX and SVM with respect to querying features
> > that are controllable via module param (SVM likes to cache nested
> > features), avoiding the guest CPUID lookups at runtime is just a bonus
> > and unlikely to provide any meaningful performance benefits.
> > 
> > No functional change intended.
> > 
> > 
> [...]
> 
> >  
> > -/*
> > - * nested_vmx_allowed() checks whether a guest should be allowed to use VMX
> > - * instructions and MSRs (i.e., nested VMX). Nested VMX is disabled for
> > - * all guests if the "nested" module option is off, and can also be disabled
> > - * for a single guest by disabling its VMX cpuid bit.
> > - */
> > -bool nested_vmx_allowed(struct kvm_vcpu *vcpu)
> > -{
> > -	return nested && guest_cpuid_has(vcpu, X86_FEATURE_VMX);
> > -}
> > -
> > 
> 
> [...]
> 
> > @@ -7750,13 +7739,15 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> >  	    guest_cpuid_has(vcpu, X86_FEATURE_XSAVE))
> >  		kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_XSAVES);
> >  
> > +	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_VMX);
> > +
> > 
> 
> Nit:
> 
> nested_vmx_allowed() also checks 'nested' global variable.  However
> kvm_governed_feature_check_and_set() is called unconditionally.  Although IIUC
> it should never actually set the VMX governed bit when 'nested=0', it's not that
> obvious in _this_ patch.

Yeah, 100% agree after re-reading this patch without context.  I had to go search
the code to remember where "nested" is checked.  :-)

> Should we explicitly call this out in the changelog so git blamers can
> understand this more easily in the future?

Yep, I'll add a blurb to point out the dependency in vmx_set_cpu_caps().

Thanks!

[PATCH v3 08/15] KVM: nSVM: Use KVM-governed feature framework to track "NRIPS enabled"

[Sean Christopherson]

Track "NRIPS exposed to L1" via a governed feature flag instead of using
a dedicated bit/flag in vcpu_svm.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/governed_features.h | 1 +
 arch/x86/kvm/svm/nested.c        | 6 +++---
 arch/x86/kvm/svm/svm.c           | 4 +---
 arch/x86/kvm/svm/svm.h           | 1 -
 4 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h
index 22446614bf49..722b66af412c 100644
--- a/arch/x86/kvm/governed_features.h
+++ b/arch/x86/kvm/governed_features.h
@@ -8,6 +8,7 @@ BUILD_BUG()
 KVM_GOVERNED_X86_FEATURE(GBPAGES)
 KVM_GOVERNED_X86_FEATURE(XSAVES)
 KVM_GOVERNED_X86_FEATURE(VMX)
+KVM_GOVERNED_X86_FEATURE(NRIPS)
 
 #undef KVM_GOVERNED_X86_FEATURE
 #undef KVM_GOVERNED_FEATURE
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 3342cc4a5189..9092f3f8dccf 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -716,7 +716,7 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
 	 * what a nrips=0 CPU would do (L1 is responsible for advancing RIP
 	 * prior to injecting the event).
 	 */
-	if (svm->nrips_enabled)
+	if (guest_can_use(vcpu, X86_FEATURE_NRIPS))
 		vmcb02->control.next_rip    = svm->nested.ctl.next_rip;
 	else if (boot_cpu_has(X86_FEATURE_NRIPS))
 		vmcb02->control.next_rip    = vmcb12_rip;
@@ -726,7 +726,7 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
 		svm->soft_int_injected = true;
 		svm->soft_int_csbase = vmcb12_csbase;
 		svm->soft_int_old_rip = vmcb12_rip;
-		if (svm->nrips_enabled)
+		if (guest_can_use(vcpu, X86_FEATURE_NRIPS))
 			svm->soft_int_next_rip = svm->nested.ctl.next_rip;
 		else
 			svm->soft_int_next_rip = vmcb12_rip;
@@ -1026,7 +1026,7 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
 	if (vmcb12->control.exit_code != SVM_EXIT_ERR)
 		nested_save_pending_event_to_vmcb12(svm, vmcb12);
 
-	if (svm->nrips_enabled)
+	if (guest_can_use(vcpu, X86_FEATURE_NRIPS))
 		vmcb12->control.next_rip  = vmcb02->control.next_rip;
 
 	vmcb12->control.int_ctl           = svm->nested.ctl.int_ctl;
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index d67f6e23dcd2..c8b97cb3138c 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4288,9 +4288,7 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 	    guest_cpuid_has(vcpu, X86_FEATURE_XSAVE))
 		kvm_governed_feature_set(vcpu, X86_FEATURE_XSAVES);
 
-	/* Update nrips enabled cache */
-	svm->nrips_enabled = kvm_cpu_cap_has(X86_FEATURE_NRIPS) &&
-			     guest_cpuid_has(vcpu, X86_FEATURE_NRIPS);
+	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_NRIPS);
 
 	svm->tsc_scaling_enabled = tsc_scaling && guest_cpuid_has(vcpu, X86_FEATURE_TSCRATEMSR);
 	svm->lbrv_enabled = lbrv && guest_cpuid_has(vcpu, X86_FEATURE_LBRV);
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 5115b35a4d31..e147f2046ffa 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -259,7 +259,6 @@ struct vcpu_svm {
 	bool soft_int_injected;
 
 	/* optional nested SVM features that are enabled for this guest  */
-	bool nrips_enabled                : 1;
 	bool tsc_scaling_enabled          : 1;
 	bool v_vmload_vmsave_enabled      : 1;
 	bool lbrv_enabled                 : 1;
-- 
2.41.0.694.ge786442a9b-goog

Re: [PATCH v3 08/15] KVM: nSVM: Use KVM-governed feature framework to track "NRIPS enabled"

[Yuan Yao]

On Tue, Aug 15, 2023 at 01:36:46PM -0700, Sean Christopherson wrote:
> Track "NRIPS exposed to L1" via a governed feature flag instead of using
> a dedicated bit/flag in vcpu_svm.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

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

> ---
>  arch/x86/kvm/governed_features.h | 1 +
>  arch/x86/kvm/svm/nested.c        | 6 +++---
>  arch/x86/kvm/svm/svm.c           | 4 +---
>  arch/x86/kvm/svm/svm.h           | 1 -
>  4 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h
> index 22446614bf49..722b66af412c 100644
> --- a/arch/x86/kvm/governed_features.h
> +++ b/arch/x86/kvm/governed_features.h
> @@ -8,6 +8,7 @@ BUILD_BUG()
>  KVM_GOVERNED_X86_FEATURE(GBPAGES)
>  KVM_GOVERNED_X86_FEATURE(XSAVES)
>  KVM_GOVERNED_X86_FEATURE(VMX)
> +KVM_GOVERNED_X86_FEATURE(NRIPS)
>
>  #undef KVM_GOVERNED_X86_FEATURE
>  #undef KVM_GOVERNED_FEATURE
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 3342cc4a5189..9092f3f8dccf 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -716,7 +716,7 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
>  	 * what a nrips=0 CPU would do (L1 is responsible for advancing RIP
>  	 * prior to injecting the event).
>  	 */
> -	if (svm->nrips_enabled)
> +	if (guest_can_use(vcpu, X86_FEATURE_NRIPS))
>  		vmcb02->control.next_rip    = svm->nested.ctl.next_rip;
>  	else if (boot_cpu_has(X86_FEATURE_NRIPS))
>  		vmcb02->control.next_rip    = vmcb12_rip;
> @@ -726,7 +726,7 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
>  		svm->soft_int_injected = true;
>  		svm->soft_int_csbase = vmcb12_csbase;
>  		svm->soft_int_old_rip = vmcb12_rip;
> -		if (svm->nrips_enabled)
> +		if (guest_can_use(vcpu, X86_FEATURE_NRIPS))
>  			svm->soft_int_next_rip = svm->nested.ctl.next_rip;
>  		else
>  			svm->soft_int_next_rip = vmcb12_rip;
> @@ -1026,7 +1026,7 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
>  	if (vmcb12->control.exit_code != SVM_EXIT_ERR)
>  		nested_save_pending_event_to_vmcb12(svm, vmcb12);
>
> -	if (svm->nrips_enabled)
> +	if (guest_can_use(vcpu, X86_FEATURE_NRIPS))
>  		vmcb12->control.next_rip  = vmcb02->control.next_rip;
>
>  	vmcb12->control.int_ctl           = svm->nested.ctl.int_ctl;
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index d67f6e23dcd2..c8b97cb3138c 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4288,9 +4288,7 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  	    guest_cpuid_has(vcpu, X86_FEATURE_XSAVE))
>  		kvm_governed_feature_set(vcpu, X86_FEATURE_XSAVES);
>
> -	/* Update nrips enabled cache */
> -	svm->nrips_enabled = kvm_cpu_cap_has(X86_FEATURE_NRIPS) &&
> -			     guest_cpuid_has(vcpu, X86_FEATURE_NRIPS);
> +	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_NRIPS);
>
>  	svm->tsc_scaling_enabled = tsc_scaling && guest_cpuid_has(vcpu, X86_FEATURE_TSCRATEMSR);
>  	svm->lbrv_enabled = lbrv && guest_cpuid_has(vcpu, X86_FEATURE_LBRV);
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 5115b35a4d31..e147f2046ffa 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -259,7 +259,6 @@ struct vcpu_svm {
>  	bool soft_int_injected;
>
>  	/* optional nested SVM features that are enabled for this guest  */
> -	bool nrips_enabled                : 1;
>  	bool tsc_scaling_enabled          : 1;
>  	bool v_vmload_vmsave_enabled      : 1;
>  	bool lbrv_enabled                 : 1;
> --
> 2.41.0.694.ge786442a9b-goog
>

[PATCH v3 09/15] KVM: nSVM: Use KVM-governed feature framework to track "TSC scaling enabled"

[Sean Christopherson]

Track "TSC scaling exposed to L1" via a governed feature flag instead of
using a dedicated bit/flag in vcpu_svm.

Note, this fixes a benign bug where KVM would mark TSC scaling as exposed
to L1 even if overall nested SVM supported is disabled, i.e. KVM would let
L1 write MSR_AMD64_TSC_RATIO even when KVM didn't advertise TSCRATEMSR
support to userspace.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/governed_features.h |  1 +
 arch/x86/kvm/svm/nested.c        |  2 +-
 arch/x86/kvm/svm/svm.c           | 10 ++++++----
 arch/x86/kvm/svm/svm.h           |  1 -
 4 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h
index 722b66af412c..32c0469cf952 100644
--- a/arch/x86/kvm/governed_features.h
+++ b/arch/x86/kvm/governed_features.h
@@ -9,6 +9,7 @@ KVM_GOVERNED_X86_FEATURE(GBPAGES)
 KVM_GOVERNED_X86_FEATURE(XSAVES)
 KVM_GOVERNED_X86_FEATURE(VMX)
 KVM_GOVERNED_X86_FEATURE(NRIPS)
+KVM_GOVERNED_X86_FEATURE(TSCRATEMSR)
 
 #undef KVM_GOVERNED_X86_FEATURE
 #undef KVM_GOVERNED_FEATURE
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 9092f3f8dccf..da65948064dc 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -695,7 +695,7 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
 
 	vmcb02->control.tsc_offset = vcpu->arch.tsc_offset;
 
-	if (svm->tsc_scaling_enabled &&
+	if (guest_can_use(vcpu, X86_FEATURE_TSCRATEMSR) &&
 	    svm->tsc_ratio_msr != kvm_caps.default_tsc_scaling_ratio)
 		nested_svm_update_tsc_ratio_msr(vcpu);
 
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index c8b97cb3138c..15c79457d8c5 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2809,7 +2809,8 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 
 	switch (msr_info->index) {
 	case MSR_AMD64_TSC_RATIO:
-		if (!msr_info->host_initiated && !svm->tsc_scaling_enabled)
+		if (!msr_info->host_initiated &&
+		    !guest_can_use(vcpu, X86_FEATURE_TSCRATEMSR))
 			return 1;
 		msr_info->data = svm->tsc_ratio_msr;
 		break;
@@ -2959,7 +2960,7 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 	switch (ecx) {
 	case MSR_AMD64_TSC_RATIO:
 
-		if (!svm->tsc_scaling_enabled) {
+		if (!guest_can_use(vcpu, X86_FEATURE_TSCRATEMSR)) {
 
 			if (!msr->host_initiated)
 				return 1;
@@ -2981,7 +2982,8 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 
 		svm->tsc_ratio_msr = data;
 
-		if (svm->tsc_scaling_enabled && is_guest_mode(vcpu))
+		if (guest_can_use(vcpu, X86_FEATURE_TSCRATEMSR) &&
+		    is_guest_mode(vcpu))
 			nested_svm_update_tsc_ratio_msr(vcpu);
 
 		break;
@@ -4289,8 +4291,8 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 		kvm_governed_feature_set(vcpu, X86_FEATURE_XSAVES);
 
 	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_NRIPS);
+	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_TSCRATEMSR);
 
-	svm->tsc_scaling_enabled = tsc_scaling && guest_cpuid_has(vcpu, X86_FEATURE_TSCRATEMSR);
 	svm->lbrv_enabled = lbrv && guest_cpuid_has(vcpu, X86_FEATURE_LBRV);
 
 	svm->v_vmload_vmsave_enabled = vls && guest_cpuid_has(vcpu, X86_FEATURE_V_VMSAVE_VMLOAD);
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index e147f2046ffa..3696f10e2887 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -259,7 +259,6 @@ struct vcpu_svm {
 	bool soft_int_injected;
 
 	/* optional nested SVM features that are enabled for this guest  */
-	bool tsc_scaling_enabled          : 1;
 	bool v_vmload_vmsave_enabled      : 1;
 	bool lbrv_enabled                 : 1;
 	bool pause_filter_enabled         : 1;
-- 
2.41.0.694.ge786442a9b-goog

Re: [PATCH v3 09/15] KVM: nSVM: Use KVM-governed feature framework to track "TSC scaling enabled"

[Yuan Yao]

On Tue, Aug 15, 2023 at 01:36:47PM -0700, Sean Christopherson wrote:
> Track "TSC scaling exposed to L1" via a governed feature flag instead of
> using a dedicated bit/flag in vcpu_svm.
>
> Note, this fixes a benign bug where KVM would mark TSC scaling as exposed
> to L1 even if overall nested SVM supported is disabled, i.e. KVM would let
> L1 write MSR_AMD64_TSC_RATIO even when KVM didn't advertise TSCRATEMSR
> support to userspace.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/governed_features.h |  1 +
>  arch/x86/kvm/svm/nested.c        |  2 +-
>  arch/x86/kvm/svm/svm.c           | 10 ++++++----
>  arch/x86/kvm/svm/svm.h           |  1 -
>  4 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h
> index 722b66af412c..32c0469cf952 100644
> --- a/arch/x86/kvm/governed_features.h
> +++ b/arch/x86/kvm/governed_features.h
> @@ -9,6 +9,7 @@ KVM_GOVERNED_X86_FEATURE(GBPAGES)
>  KVM_GOVERNED_X86_FEATURE(XSAVES)
>  KVM_GOVERNED_X86_FEATURE(VMX)
>  KVM_GOVERNED_X86_FEATURE(NRIPS)
> +KVM_GOVERNED_X86_FEATURE(TSCRATEMSR)
>
>  #undef KVM_GOVERNED_X86_FEATURE
>  #undef KVM_GOVERNED_FEATURE
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 9092f3f8dccf..da65948064dc 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -695,7 +695,7 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
>
>  	vmcb02->control.tsc_offset = vcpu->arch.tsc_offset;
>
> -	if (svm->tsc_scaling_enabled &&
> +	if (guest_can_use(vcpu, X86_FEATURE_TSCRATEMSR) &&
>  	    svm->tsc_ratio_msr != kvm_caps.default_tsc_scaling_ratio)
>  		nested_svm_update_tsc_ratio_msr(vcpu);
>
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index c8b97cb3138c..15c79457d8c5 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2809,7 +2809,8 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>
>  	switch (msr_info->index) {
>  	case MSR_AMD64_TSC_RATIO:
> -		if (!msr_info->host_initiated && !svm->tsc_scaling_enabled)
> +		if (!msr_info->host_initiated &&
> +		    !guest_can_use(vcpu, X86_FEATURE_TSCRATEMSR))
>  			return 1;
>  		msr_info->data = svm->tsc_ratio_msr;
>  		break;
> @@ -2959,7 +2960,7 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>  	switch (ecx) {
>  	case MSR_AMD64_TSC_RATIO:
>
> -		if (!svm->tsc_scaling_enabled) {
> +		if (!guest_can_use(vcpu, X86_FEATURE_TSCRATEMSR)) {
>
>  			if (!msr->host_initiated)
>  				return 1;
> @@ -2981,7 +2982,8 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>
>  		svm->tsc_ratio_msr = data;
>
> -		if (svm->tsc_scaling_enabled && is_guest_mode(vcpu))
> +		if (guest_can_use(vcpu, X86_FEATURE_TSCRATEMSR) &&
> +		    is_guest_mode(vcpu))

I prefer (is_guest_mode(vcpu) && ....), so I can skip them more quickly LOL.
but anyway depends on you :-)

>  			nested_svm_update_tsc_ratio_msr(vcpu);
>
>  		break;
> @@ -4289,8 +4291,8 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  		kvm_governed_feature_set(vcpu, X86_FEATURE_XSAVES);
>
>  	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_NRIPS);
> +	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_TSCRATEMSR);
>
> -	svm->tsc_scaling_enabled = tsc_scaling && guest_cpuid_has(vcpu, X86_FEATURE_TSCRATEMSR);

Not account "nested" is the reason of the benign bug.

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

>  	svm->lbrv_enabled = lbrv && guest_cpuid_has(vcpu, X86_FEATURE_LBRV);
>
>  	svm->v_vmload_vmsave_enabled = vls && guest_cpuid_has(vcpu, X86_FEATURE_V_VMSAVE_VMLOAD);
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index e147f2046ffa..3696f10e2887 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -259,7 +259,6 @@ struct vcpu_svm {
>  	bool soft_int_injected;
>
>  	/* optional nested SVM features that are enabled for this guest  */
> -	bool tsc_scaling_enabled          : 1;
>  	bool v_vmload_vmsave_enabled      : 1;
>  	bool lbrv_enabled                 : 1;
>  	bool pause_filter_enabled         : 1;
> --
> 2.41.0.694.ge786442a9b-goog
>

Re: [PATCH v3 09/15] KVM: nSVM: Use KVM-governed feature framework to track "TSC scaling enabled"

[Sean Christopherson]

On Wed, Aug 16, 2023, Yuan Yao wrote:
> On Tue, Aug 15, 2023 at 01:36:47PM -0700, Sean Christopherson wrote:
> > @@ -2981,7 +2982,8 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
> >
> >  		svm->tsc_ratio_msr = data;
> >
> > -		if (svm->tsc_scaling_enabled && is_guest_mode(vcpu))
> > +		if (guest_can_use(vcpu, X86_FEATURE_TSCRATEMSR) &&
> > +		    is_guest_mode(vcpu))
> 
> I prefer (is_guest_mode(vcpu) && ....), so I can skip them more quickly LOL.
> but anyway depends on you :-)

For this series, I want to do as much of a straight replacement as possible, i.e.
not change any ordering unless it's "necessary".

Re: [PATCH v3 09/15] KVM: nSVM: Use KVM-governed feature framework to track "TSC scaling enabled"

[Yuan Yao]

On Wed, Aug 16, 2023 at 06:44:42AM -0700, Sean Christopherson wrote:
> On Wed, Aug 16, 2023, Yuan Yao wrote:
> > On Tue, Aug 15, 2023 at 01:36:47PM -0700, Sean Christopherson wrote:
> > > @@ -2981,7 +2982,8 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
> > >
> > >  		svm->tsc_ratio_msr = data;
> > >
> > > -		if (svm->tsc_scaling_enabled && is_guest_mode(vcpu))
> > > +		if (guest_can_use(vcpu, X86_FEATURE_TSCRATEMSR) &&
> > > +		    is_guest_mode(vcpu))
> >
> > I prefer (is_guest_mode(vcpu) && ....), so I can skip them more quickly LOL.
> > but anyway depends on you :-)
>
> For this series, I want to do as much of a straight replacement as possible, i.e.
> not change any ordering unless it's "necessary".

Yeah, I'm fine with this.

[PATCH v3 10/15] KVM: nSVM: Use KVM-governed feature framework to track "vVM{SAVE,LOAD} enabled"

[Sean Christopherson]

Track "virtual VMSAVE/VMLOAD exposed to L1" via a governed feature flag
instead of using a dedicated bit/flag in vcpu_svm.

Opportunistically add a comment explaining why KVM disallows virtual
VMLOAD/VMSAVE when the vCPU model is Intel.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/governed_features.h |  1 +
 arch/x86/kvm/svm/nested.c        |  2 +-
 arch/x86/kvm/svm/svm.c           | 10 +++++++---
 arch/x86/kvm/svm/svm.h           |  1 -
 4 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h
index 32c0469cf952..f01a95fd0071 100644
--- a/arch/x86/kvm/governed_features.h
+++ b/arch/x86/kvm/governed_features.h
@@ -10,6 +10,7 @@ KVM_GOVERNED_X86_FEATURE(XSAVES)
 KVM_GOVERNED_X86_FEATURE(VMX)
 KVM_GOVERNED_X86_FEATURE(NRIPS)
 KVM_GOVERNED_X86_FEATURE(TSCRATEMSR)
+KVM_GOVERNED_X86_FEATURE(V_VMSAVE_VMLOAD)
 
 #undef KVM_GOVERNED_X86_FEATURE
 #undef KVM_GOVERNED_FEATURE
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index da65948064dc..24d47ebeb0e0 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -107,7 +107,7 @@ static void nested_svm_uninit_mmu_context(struct kvm_vcpu *vcpu)
 
 static bool nested_vmcb_needs_vls_intercept(struct vcpu_svm *svm)
 {
-	if (!svm->v_vmload_vmsave_enabled)
+	if (!guest_can_use(&svm->vcpu, X86_FEATURE_V_VMSAVE_VMLOAD))
 		return true;
 
 	if (!nested_npt_enabled(svm))
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 15c79457d8c5..7cecbb58c60f 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1201,8 +1201,6 @@ static inline void init_vmcb_after_set_cpuid(struct kvm_vcpu *vcpu)
 
 		set_msr_interception(vcpu, svm->msrpm, MSR_IA32_SYSENTER_EIP, 0, 0);
 		set_msr_interception(vcpu, svm->msrpm, MSR_IA32_SYSENTER_ESP, 0, 0);
-
-		svm->v_vmload_vmsave_enabled = false;
 	} else {
 		/*
 		 * If hardware supports Virtual VMLOAD VMSAVE then enable it
@@ -4295,7 +4293,13 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 
 	svm->lbrv_enabled = lbrv && guest_cpuid_has(vcpu, X86_FEATURE_LBRV);
 
-	svm->v_vmload_vmsave_enabled = vls && guest_cpuid_has(vcpu, X86_FEATURE_V_VMSAVE_VMLOAD);
+	/*
+	 * Intercept VMLOAD if the vCPU mode is Intel in order to emulate that
+	 * VMLOAD drops bits 63:32 of SYSENTER (ignoring the fact that exposing
+	 * SVM on Intel is bonkers and extremely unlikely to work).
+	 */
+	if (!guest_cpuid_is_intel(vcpu))
+		kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_V_VMSAVE_VMLOAD);
 
 	svm->pause_filter_enabled = kvm_cpu_cap_has(X86_FEATURE_PAUSEFILTER) &&
 			guest_cpuid_has(vcpu, X86_FEATURE_PAUSEFILTER);
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 3696f10e2887..b3fdaab57363 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -259,7 +259,6 @@ struct vcpu_svm {
 	bool soft_int_injected;
 
 	/* optional nested SVM features that are enabled for this guest  */
-	bool v_vmload_vmsave_enabled      : 1;
 	bool lbrv_enabled                 : 1;
 	bool pause_filter_enabled         : 1;
 	bool pause_threshold_enabled      : 1;
-- 
2.41.0.694.ge786442a9b-goog

[PATCH v3 11/15] KVM: nSVM: Use KVM-governed feature framework to track "LBRv enabled"

[Sean Christopherson]

Track "LBR virtualization exposed to L1" via a governed feature flag
instead of using a dedicated bit/flag in vcpu_svm.

Note, checking KVM's capabilities instead of the "lbrv" param means that
the code isn't strictly equivalent, as lbrv_enabled could have been set
if nested=false where as that the governed feature cannot.  But that's a
glorified nop as the feature/flag is consumed only by paths that are
gated by nSVM being enabled.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/governed_features.h |  1 +
 arch/x86/kvm/svm/nested.c        | 23 +++++++++++++----------
 arch/x86/kvm/svm/svm.c           |  5 ++---
 arch/x86/kvm/svm/svm.h           |  1 -
 4 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h
index f01a95fd0071..3a4c0e40e1e0 100644
--- a/arch/x86/kvm/governed_features.h
+++ b/arch/x86/kvm/governed_features.h
@@ -11,6 +11,7 @@ KVM_GOVERNED_X86_FEATURE(VMX)
 KVM_GOVERNED_X86_FEATURE(NRIPS)
 KVM_GOVERNED_X86_FEATURE(TSCRATEMSR)
 KVM_GOVERNED_X86_FEATURE(V_VMSAVE_VMLOAD)
+KVM_GOVERNED_X86_FEATURE(LBRV)
 
 #undef KVM_GOVERNED_X86_FEATURE
 #undef KVM_GOVERNED_FEATURE
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 24d47ebeb0e0..f50f74b1a04e 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -552,6 +552,7 @@ static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12
 	bool new_vmcb12 = false;
 	struct vmcb *vmcb01 = svm->vmcb01.ptr;
 	struct vmcb *vmcb02 = svm->nested.vmcb02.ptr;
+	struct kvm_vcpu *vcpu = &svm->vcpu;
 
 	nested_vmcb02_compute_g_pat(svm);
 
@@ -577,18 +578,18 @@ static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12
 		vmcb_mark_dirty(vmcb02, VMCB_DT);
 	}
 
-	kvm_set_rflags(&svm->vcpu, vmcb12->save.rflags | X86_EFLAGS_FIXED);
+	kvm_set_rflags(vcpu, vmcb12->save.rflags | X86_EFLAGS_FIXED);
 
-	svm_set_efer(&svm->vcpu, svm->nested.save.efer);
+	svm_set_efer(vcpu, svm->nested.save.efer);
 
-	svm_set_cr0(&svm->vcpu, svm->nested.save.cr0);
-	svm_set_cr4(&svm->vcpu, svm->nested.save.cr4);
+	svm_set_cr0(vcpu, svm->nested.save.cr0);
+	svm_set_cr4(vcpu, svm->nested.save.cr4);
 
 	svm->vcpu.arch.cr2 = vmcb12->save.cr2;
 
-	kvm_rax_write(&svm->vcpu, vmcb12->save.rax);
-	kvm_rsp_write(&svm->vcpu, vmcb12->save.rsp);
-	kvm_rip_write(&svm->vcpu, vmcb12->save.rip);
+	kvm_rax_write(vcpu, vmcb12->save.rax);
+	kvm_rsp_write(vcpu, vmcb12->save.rsp);
+	kvm_rip_write(vcpu, vmcb12->save.rip);
 
 	/* In case we don't even reach vcpu_run, the fields are not updated */
 	vmcb02->save.rax = vmcb12->save.rax;
@@ -602,7 +603,8 @@ static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12
 		vmcb_mark_dirty(vmcb02, VMCB_DR);
 	}
 
-	if (unlikely(svm->lbrv_enabled && (svm->nested.ctl.virt_ext & LBR_CTL_ENABLE_MASK))) {
+	if (unlikely(guest_can_use(vcpu, X86_FEATURE_LBRV) &&
+		     (svm->nested.ctl.virt_ext & LBR_CTL_ENABLE_MASK))) {
 		/*
 		 * Reserved bits of DEBUGCTL are ignored.  Be consistent with
 		 * svm_set_msr's definition of reserved bits.
@@ -734,7 +736,7 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
 
 	vmcb02->control.virt_ext            = vmcb01->control.virt_ext &
 					      LBR_CTL_ENABLE_MASK;
-	if (svm->lbrv_enabled)
+	if (guest_can_use(vcpu, X86_FEATURE_LBRV))
 		vmcb02->control.virt_ext  |=
 			(svm->nested.ctl.virt_ext & LBR_CTL_ENABLE_MASK);
 
@@ -1065,7 +1067,8 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
 	if (!nested_exit_on_intr(svm))
 		kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
 
-	if (unlikely(svm->lbrv_enabled && (svm->nested.ctl.virt_ext & LBR_CTL_ENABLE_MASK))) {
+	if (unlikely(guest_can_use(vcpu, X86_FEATURE_LBRV) &&
+		     (svm->nested.ctl.virt_ext & LBR_CTL_ENABLE_MASK))) {
 		svm_copy_lbrs(vmcb12, vmcb02);
 		svm_update_lbrv(vcpu);
 	} else if (unlikely(vmcb01->control.virt_ext & LBR_CTL_ENABLE_MASK)) {
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 7cecbb58c60f..de40745bc8a6 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1032,7 +1032,7 @@ void svm_update_lbrv(struct kvm_vcpu *vcpu)
 	struct vcpu_svm *svm = to_svm(vcpu);
 	bool current_enable_lbrv = svm->vmcb->control.virt_ext & LBR_CTL_ENABLE_MASK;
 	bool enable_lbrv = (svm_get_lbr_vmcb(svm)->save.dbgctl & DEBUGCTLMSR_LBR) ||
-			   (is_guest_mode(vcpu) && svm->lbrv_enabled &&
+			   (is_guest_mode(vcpu) && guest_can_use(vcpu, X86_FEATURE_LBRV) &&
 			    (svm->nested.ctl.virt_ext & LBR_CTL_ENABLE_MASK));
 
 	if (enable_lbrv == current_enable_lbrv)
@@ -4290,8 +4290,7 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 
 	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_NRIPS);
 	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_TSCRATEMSR);
-
-	svm->lbrv_enabled = lbrv && guest_cpuid_has(vcpu, X86_FEATURE_LBRV);
+	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_LBRV);
 
 	/*
 	 * Intercept VMLOAD if the vCPU mode is Intel in order to emulate that
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index b3fdaab57363..45cbbdeac3a3 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -259,7 +259,6 @@ struct vcpu_svm {
 	bool soft_int_injected;
 
 	/* optional nested SVM features that are enabled for this guest  */
-	bool lbrv_enabled                 : 1;
 	bool pause_filter_enabled         : 1;
 	bool pause_threshold_enabled      : 1;
 	bool vgif_enabled                 : 1;
-- 
2.41.0.694.ge786442a9b-goog

[PATCH v3 12/15] KVM: nSVM: Use KVM-governed feature framework to track "Pause Filter enabled"

[Sean Christopherson]

Track "Pause Filtering is exposed to L1" via governed feature flags
instead of using dedicated bits/flags in vcpu_svm.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/governed_features.h |  2 ++
 arch/x86/kvm/svm/nested.c        | 10 ++++++++--
 arch/x86/kvm/svm/svm.c           |  7 ++-----
 arch/x86/kvm/svm/svm.h           |  2 --
 4 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h
index 3a4c0e40e1e0..9afd34f30599 100644
--- a/arch/x86/kvm/governed_features.h
+++ b/arch/x86/kvm/governed_features.h
@@ -12,6 +12,8 @@ KVM_GOVERNED_X86_FEATURE(NRIPS)
 KVM_GOVERNED_X86_FEATURE(TSCRATEMSR)
 KVM_GOVERNED_X86_FEATURE(V_VMSAVE_VMLOAD)
 KVM_GOVERNED_X86_FEATURE(LBRV)
+KVM_GOVERNED_X86_FEATURE(PAUSEFILTER)
+KVM_GOVERNED_X86_FEATURE(PFTHRESHOLD)
 
 #undef KVM_GOVERNED_X86_FEATURE
 #undef KVM_GOVERNED_FEATURE
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index f50f74b1a04e..ac03b2bc5b2c 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -743,8 +743,14 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
 	if (!nested_vmcb_needs_vls_intercept(svm))
 		vmcb02->control.virt_ext |= VIRTUAL_VMLOAD_VMSAVE_ENABLE_MASK;
 
-	pause_count12 = svm->pause_filter_enabled ? svm->nested.ctl.pause_filter_count : 0;
-	pause_thresh12 = svm->pause_threshold_enabled ? svm->nested.ctl.pause_filter_thresh : 0;
+	if (guest_can_use(vcpu, X86_FEATURE_PAUSEFILTER))
+		pause_count12 = svm->nested.ctl.pause_filter_count;
+	else
+		pause_count12 = 0;
+	if (guest_can_use(vcpu, X86_FEATURE_PFTHRESHOLD))
+		pause_thresh12 = svm->nested.ctl.pause_filter_thresh;
+	else
+		pause_thresh12 = 0;
 	if (kvm_pause_in_guest(svm->vcpu.kvm)) {
 		/* use guest values since host doesn't intercept PAUSE */
 		vmcb02->control.pause_filter_count = pause_count12;
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index de40745bc8a6..9bfff65e8b7a 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4300,11 +4300,8 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 	if (!guest_cpuid_is_intel(vcpu))
 		kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_V_VMSAVE_VMLOAD);
 
-	svm->pause_filter_enabled = kvm_cpu_cap_has(X86_FEATURE_PAUSEFILTER) &&
-			guest_cpuid_has(vcpu, X86_FEATURE_PAUSEFILTER);
-
-	svm->pause_threshold_enabled = kvm_cpu_cap_has(X86_FEATURE_PFTHRESHOLD) &&
-			guest_cpuid_has(vcpu, X86_FEATURE_PFTHRESHOLD);
+	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_PAUSEFILTER);
+	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_PFTHRESHOLD);
 
 	svm->vgif_enabled = vgif && guest_cpuid_has(vcpu, X86_FEATURE_VGIF);
 
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 45cbbdeac3a3..d57a096e070a 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -259,8 +259,6 @@ struct vcpu_svm {
 	bool soft_int_injected;
 
 	/* optional nested SVM features that are enabled for this guest  */
-	bool pause_filter_enabled         : 1;
-	bool pause_threshold_enabled      : 1;
 	bool vgif_enabled                 : 1;
 	bool vnmi_enabled                 : 1;
 
-- 
2.41.0.694.ge786442a9b-goog

Re: [PATCH v3 12/15] KVM: nSVM: Use KVM-governed feature framework to track "Pause Filter enabled"

[Yuan Yao]

On Tue, Aug 15, 2023 at 01:36:50PM -0700, Sean Christopherson wrote:
> Track "Pause Filtering is exposed to L1" via governed feature flags
> instead of using dedicated bits/flags in vcpu_svm.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

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

> ---
>  arch/x86/kvm/governed_features.h |  2 ++
>  arch/x86/kvm/svm/nested.c        | 10 ++++++++--
>  arch/x86/kvm/svm/svm.c           |  7 ++-----
>  arch/x86/kvm/svm/svm.h           |  2 --
>  4 files changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h
> index 3a4c0e40e1e0..9afd34f30599 100644
> --- a/arch/x86/kvm/governed_features.h
> +++ b/arch/x86/kvm/governed_features.h
> @@ -12,6 +12,8 @@ KVM_GOVERNED_X86_FEATURE(NRIPS)
>  KVM_GOVERNED_X86_FEATURE(TSCRATEMSR)
>  KVM_GOVERNED_X86_FEATURE(V_VMSAVE_VMLOAD)
>  KVM_GOVERNED_X86_FEATURE(LBRV)
> +KVM_GOVERNED_X86_FEATURE(PAUSEFILTER)
> +KVM_GOVERNED_X86_FEATURE(PFTHRESHOLD)
>
>  #undef KVM_GOVERNED_X86_FEATURE
>  #undef KVM_GOVERNED_FEATURE
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index f50f74b1a04e..ac03b2bc5b2c 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -743,8 +743,14 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
>  	if (!nested_vmcb_needs_vls_intercept(svm))
>  		vmcb02->control.virt_ext |= VIRTUAL_VMLOAD_VMSAVE_ENABLE_MASK;
>
> -	pause_count12 = svm->pause_filter_enabled ? svm->nested.ctl.pause_filter_count : 0;
> -	pause_thresh12 = svm->pause_threshold_enabled ? svm->nested.ctl.pause_filter_thresh : 0;
> +	if (guest_can_use(vcpu, X86_FEATURE_PAUSEFILTER))
> +		pause_count12 = svm->nested.ctl.pause_filter_count;
> +	else
> +		pause_count12 = 0;
> +	if (guest_can_use(vcpu, X86_FEATURE_PFTHRESHOLD))
> +		pause_thresh12 = svm->nested.ctl.pause_filter_thresh;
> +	else
> +		pause_thresh12 = 0;
>  	if (kvm_pause_in_guest(svm->vcpu.kvm)) {
>  		/* use guest values since host doesn't intercept PAUSE */
>  		vmcb02->control.pause_filter_count = pause_count12;
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index de40745bc8a6..9bfff65e8b7a 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4300,11 +4300,8 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  	if (!guest_cpuid_is_intel(vcpu))
>  		kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_V_VMSAVE_VMLOAD);
>
> -	svm->pause_filter_enabled = kvm_cpu_cap_has(X86_FEATURE_PAUSEFILTER) &&
> -			guest_cpuid_has(vcpu, X86_FEATURE_PAUSEFILTER);
> -
> -	svm->pause_threshold_enabled = kvm_cpu_cap_has(X86_FEATURE_PFTHRESHOLD) &&
> -			guest_cpuid_has(vcpu, X86_FEATURE_PFTHRESHOLD);
> +	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_PAUSEFILTER);
> +	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_PFTHRESHOLD);
>
>  	svm->vgif_enabled = vgif && guest_cpuid_has(vcpu, X86_FEATURE_VGIF);
>
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 45cbbdeac3a3..d57a096e070a 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -259,8 +259,6 @@ struct vcpu_svm {
>  	bool soft_int_injected;
>
>  	/* optional nested SVM features that are enabled for this guest  */
> -	bool pause_filter_enabled         : 1;
> -	bool pause_threshold_enabled      : 1;
>  	bool vgif_enabled                 : 1;
>  	bool vnmi_enabled                 : 1;
>
> --
> 2.41.0.694.ge786442a9b-goog
>

[PATCH v3 13/15] KVM: nSVM: Use KVM-governed feature framework to track "vGIF enabled"

[Sean Christopherson]

Track "virtual GIF exposed to L1" via a governed feature flag instead of
using a dedicated bit/flag in vcpu_svm.

Note, checking KVM's capabilities instead of the "vgif" param means that
the code isn't strictly equivalent, as vgif_enabled could have been set
if nested=false where as that the governed feature cannot.  But that's a
glorified nop as the feature/flag is consumed only by paths that are

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/governed_features.h | 1 +
 arch/x86/kvm/svm/nested.c        | 3 ++-
 arch/x86/kvm/svm/svm.c           | 3 +--
 arch/x86/kvm/svm/svm.h           | 5 +++--
 4 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h
index 9afd34f30599..368696c2e96b 100644
--- a/arch/x86/kvm/governed_features.h
+++ b/arch/x86/kvm/governed_features.h
@@ -14,6 +14,7 @@ KVM_GOVERNED_X86_FEATURE(V_VMSAVE_VMLOAD)
 KVM_GOVERNED_X86_FEATURE(LBRV)
 KVM_GOVERNED_X86_FEATURE(PAUSEFILTER)
 KVM_GOVERNED_X86_FEATURE(PFTHRESHOLD)
+KVM_GOVERNED_X86_FEATURE(VGIF)
 
 #undef KVM_GOVERNED_X86_FEATURE
 #undef KVM_GOVERNED_FEATURE
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index ac03b2bc5b2c..dd496c9e5f91 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -660,7 +660,8 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
 	 * exit_int_info, exit_int_info_err, next_rip, insn_len, insn_bytes.
 	 */
 
-	if (svm->vgif_enabled && (svm->nested.ctl.int_ctl & V_GIF_ENABLE_MASK))
+	if (guest_can_use(vcpu, X86_FEATURE_VGIF) &&
+	    (svm->nested.ctl.int_ctl & V_GIF_ENABLE_MASK))
 		int_ctl_vmcb12_bits |= (V_GIF_MASK | V_GIF_ENABLE_MASK);
 	else
 		int_ctl_vmcb01_bits |= (V_GIF_MASK | V_GIF_ENABLE_MASK);
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 9bfff65e8b7a..9eac0ad3403e 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4302,8 +4302,7 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 
 	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_PAUSEFILTER);
 	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_PFTHRESHOLD);
-
-	svm->vgif_enabled = vgif && guest_cpuid_has(vcpu, X86_FEATURE_VGIF);
+	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_VGIF);
 
 	svm->vnmi_enabled = vnmi && guest_cpuid_has(vcpu, X86_FEATURE_VNMI);
 
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index d57a096e070a..eaddaac6bf18 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -22,6 +22,7 @@
 #include <asm/svm.h>
 #include <asm/sev-common.h>
 
+#include "cpuid.h"
 #include "kvm_cache_regs.h"
 
 #define __sme_page_pa(x) __sme_set(page_to_pfn(x) << PAGE_SHIFT)
@@ -259,7 +260,6 @@ struct vcpu_svm {
 	bool soft_int_injected;
 
 	/* optional nested SVM features that are enabled for this guest  */
-	bool vgif_enabled                 : 1;
 	bool vnmi_enabled                 : 1;
 
 	u32 ldr_reg;
@@ -443,7 +443,8 @@ static inline bool svm_is_intercept(struct vcpu_svm *svm, int bit)
 
 static inline bool nested_vgif_enabled(struct vcpu_svm *svm)
 {
-	return svm->vgif_enabled && (svm->nested.ctl.int_ctl & V_GIF_ENABLE_MASK);
+	return guest_can_use(&svm->vcpu, X86_FEATURE_VGIF) &&
+	       (svm->nested.ctl.int_ctl & V_GIF_ENABLE_MASK);
 }
 
 static inline struct vmcb *get_vgif_vmcb(struct vcpu_svm *svm)
-- 
2.41.0.694.ge786442a9b-goog

Re: [PATCH v3 13/15] KVM: nSVM: Use KVM-governed feature framework to track "vGIF enabled"

[Yuan Yao]

On Tue, Aug 15, 2023 at 01:36:51PM -0700, Sean Christopherson wrote:
> Track "virtual GIF exposed to L1" via a governed feature flag instead of
> using a dedicated bit/flag in vcpu_svm.
>
> Note, checking KVM's capabilities instead of the "vgif" param means that
> the code isn't strictly equivalent, as vgif_enabled could have been set
> if nested=false where as that the governed feature cannot.  But that's a
> glorified nop as the feature/flag is consumed only by paths that are

gated by nSVM being enabled.

>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/governed_features.h | 1 +
>  arch/x86/kvm/svm/nested.c        | 3 ++-
>  arch/x86/kvm/svm/svm.c           | 3 +--
>  arch/x86/kvm/svm/svm.h           | 5 +++--
>  4 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h
> index 9afd34f30599..368696c2e96b 100644
> --- a/arch/x86/kvm/governed_features.h
> +++ b/arch/x86/kvm/governed_features.h
> @@ -14,6 +14,7 @@ KVM_GOVERNED_X86_FEATURE(V_VMSAVE_VMLOAD)
>  KVM_GOVERNED_X86_FEATURE(LBRV)
>  KVM_GOVERNED_X86_FEATURE(PAUSEFILTER)
>  KVM_GOVERNED_X86_FEATURE(PFTHRESHOLD)
> +KVM_GOVERNED_X86_FEATURE(VGIF)
>
>  #undef KVM_GOVERNED_X86_FEATURE
>  #undef KVM_GOVERNED_FEATURE
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index ac03b2bc5b2c..dd496c9e5f91 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -660,7 +660,8 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
>  	 * exit_int_info, exit_int_info_err, next_rip, insn_len, insn_bytes.
>  	 */
>
> -	if (svm->vgif_enabled && (svm->nested.ctl.int_ctl & V_GIF_ENABLE_MASK))
> +	if (guest_can_use(vcpu, X86_FEATURE_VGIF) &&
> +	    (svm->nested.ctl.int_ctl & V_GIF_ENABLE_MASK))
>  		int_ctl_vmcb12_bits |= (V_GIF_MASK | V_GIF_ENABLE_MASK);
>  	else
>  		int_ctl_vmcb01_bits |= (V_GIF_MASK | V_GIF_ENABLE_MASK);
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 9bfff65e8b7a..9eac0ad3403e 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4302,8 +4302,7 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>
>  	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_PAUSEFILTER);
>  	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_PFTHRESHOLD);
> -
> -	svm->vgif_enabled = vgif && guest_cpuid_has(vcpu, X86_FEATURE_VGIF);
> +	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_VGIF);
>
>  	svm->vnmi_enabled = vnmi && guest_cpuid_has(vcpu, X86_FEATURE_VNMI);
>
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index d57a096e070a..eaddaac6bf18 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -22,6 +22,7 @@
>  #include <asm/svm.h>
>  #include <asm/sev-common.h>
>
> +#include "cpuid.h"
>  #include "kvm_cache_regs.h"
>
>  #define __sme_page_pa(x) __sme_set(page_to_pfn(x) << PAGE_SHIFT)
> @@ -259,7 +260,6 @@ struct vcpu_svm {
>  	bool soft_int_injected;
>
>  	/* optional nested SVM features that are enabled for this guest  */
> -	bool vgif_enabled                 : 1;
>  	bool vnmi_enabled                 : 1;
>
>  	u32 ldr_reg;
> @@ -443,7 +443,8 @@ static inline bool svm_is_intercept(struct vcpu_svm *svm, int bit)
>
>  static inline bool nested_vgif_enabled(struct vcpu_svm *svm)
>  {
> -	return svm->vgif_enabled && (svm->nested.ctl.int_ctl & V_GIF_ENABLE_MASK);
> +	return guest_can_use(&svm->vcpu, X86_FEATURE_VGIF) &&
> +	       (svm->nested.ctl.int_ctl & V_GIF_ENABLE_MASK);
>  }
>
>  static inline struct vmcb *get_vgif_vmcb(struct vcpu_svm *svm)
> --
> 2.41.0.694.ge786442a9b-goog
>

[PATCH v3 14/15] KVM: nSVM: Use KVM-governed feature framework to track "vNMI enabled"

[Sean Christopherson]

Track "virtual NMI exposed to L1" via a governed feature flag instead of
using a dedicated bit/flag in vcpu_svm.

Note, checking KVM's capabilities instead of the "vnmi" param means that
the code isn't strictly equivalent, as vnmi_enabled could have been set
if nested=false where as that the governed feature cannot.  But that's a
glorified nop as the feature/flag is consumed only by paths that are
gated by nSVM being enabled.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/governed_features.h | 1 +
 arch/x86/kvm/svm/svm.c           | 3 +--
 arch/x86/kvm/svm/svm.h           | 5 +----
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h
index 368696c2e96b..423a73395c10 100644
--- a/arch/x86/kvm/governed_features.h
+++ b/arch/x86/kvm/governed_features.h
@@ -15,6 +15,7 @@ KVM_GOVERNED_X86_FEATURE(LBRV)
 KVM_GOVERNED_X86_FEATURE(PAUSEFILTER)
 KVM_GOVERNED_X86_FEATURE(PFTHRESHOLD)
 KVM_GOVERNED_X86_FEATURE(VGIF)
+KVM_GOVERNED_X86_FEATURE(VNMI)
 
 #undef KVM_GOVERNED_X86_FEATURE
 #undef KVM_GOVERNED_FEATURE
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 9eac0ad3403e..a139c626fa8b 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4303,8 +4303,7 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_PAUSEFILTER);
 	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_PFTHRESHOLD);
 	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_VGIF);
-
-	svm->vnmi_enabled = vnmi && guest_cpuid_has(vcpu, X86_FEATURE_VNMI);
+	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_VNMI);
 
 	svm_recalc_instruction_intercepts(vcpu, svm);
 
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index eaddaac6bf18..2237230aad98 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -259,9 +259,6 @@ struct vcpu_svm {
 	unsigned long soft_int_next_rip;
 	bool soft_int_injected;
 
-	/* optional nested SVM features that are enabled for this guest  */
-	bool vnmi_enabled                 : 1;
-
 	u32 ldr_reg;
 	u32 dfr_reg;
 	struct page *avic_backing_page;
@@ -495,7 +492,7 @@ static inline bool nested_npt_enabled(struct vcpu_svm *svm)
 
 static inline bool nested_vnmi_enabled(struct vcpu_svm *svm)
 {
-	return svm->vnmi_enabled &&
+	return guest_can_use(&svm->vcpu, X86_FEATURE_VNMI) &&
 	       (svm->nested.ctl.int_ctl & V_NMI_ENABLE_MASK);
 }
 
-- 
2.41.0.694.ge786442a9b-goog

[PATCH v3 15/15] KVM: x86: Disallow guest CPUID lookups when IRQs are disabled

[Sean Christopherson]

Now that KVM has a framework for caching guest CPUID feature flags, add
a "rule" that IRQs must be enabled when doing guest CPUID lookups, and
enforce the rule via a lockdep assertion.  CPUID lookups are slow, and
within KVM, IRQs are only ever disabled in hot paths, e.g. the core run
loop, fast page fault handling, etc.  I.e. querying guest CPUID with IRQs
disabled, especially in the run loop, should be avoided.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/cpuid.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 67e9f79fe059..e961e9a05847 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -11,6 +11,7 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/kvm_host.h>
+#include "linux/lockdep.h"
 #include <linux/export.h>
 #include <linux/vmalloc.h>
 #include <linux/uaccess.h>
@@ -84,6 +85,18 @@ static inline struct kvm_cpuid_entry2 *cpuid_entry2_find(
 	struct kvm_cpuid_entry2 *e;
 	int i;
 
+	/*
+	 * KVM has a semi-arbitrary rule that querying the guest's CPUID model
+	 * with IRQs disabled is disallowed.  The CPUID model can legitimately
+	 * have over one hundred entries, i.e. the lookup is slow, and IRQs are
+	 * typically disabled in KVM only when KVM is in a performance critical
+	 * path, e.g. the core VM-Enter/VM-Exit run loop.  Nothing will break
+	 * if this rule is violated, this assertion is purely to flag potential
+	 * performance issues.  If this fires, consider moving the lookup out
+	 * of the hotpath, e.g. by caching information during CPUID updates.
+	 */
+	lockdep_assert_irqs_enabled();
+
 	for (i = 0; i < nent; i++) {
 		e = &entries[i];
 
-- 
2.41.0.694.ge786442a9b-goog

Re: [PATCH v3 00/15] KVM: x86: Add "governed" X86_FEATURE framework

[Sean Christopherson]

On Tue, 15 Aug 2023 13:36:38 -0700, Sean Christopherson wrote:
> Third and hopefully final version of the framework to manage and cache
> KVM-governed features, i.e. CPUID based features that require explicit
> KVM enabling and/or need to be queried semi-frequently by KVM.
> 
> This version is just the governed features patches, as I kept the TSC
> scaling patches in kvm-x86/misc but blasted away the goverend features
> with a forced push.
> 
> [...]

Applied to kvm-x86 misc, with a blurb added to the nVMX changelog to explain
how "nested" is factored in.

[01/15] KVM: x86: Add a framework for enabling KVM-governed x86 features
        https://github.com/kvm-x86/linux/commit/42764413d195
[02/15] KVM: x86/mmu: Use KVM-governed feature framework to track "GBPAGES enabled"
        https://github.com/kvm-x86/linux/commit/ccf31d6e6cc5
[03/15] KVM: VMX: Recompute "XSAVES enabled" only after CPUID update
        https://github.com/kvm-x86/linux/commit/1143c0b85c07
[04/15] KVM: VMX: Check KVM CPU caps, not just VMX MSR support, for XSAVE enabling
        https://github.com/kvm-x86/linux/commit/0497d2ac9b26
[05/15] KVM: VMX: Rename XSAVES control to follow KVM's preferred "ENABLE_XYZ"
        https://github.com/kvm-x86/linux/commit/662f6815786e
[06/15] KVM: x86: Use KVM-governed feature framework to track "XSAVES enabled"
        https://github.com/kvm-x86/linux/commit/fe60e8f65f79
[07/15] KVM: nVMX: Use KVM-governed feature framework to track "nested VMX enabled"
        https://github.com/kvm-x86/linux/commit/1c18efdaa314
[08/15] KVM: nSVM: Use KVM-governed feature framework to track "NRIPS enabled"
        https://github.com/kvm-x86/linux/commit/7a6a6a3bf5d8
[09/15] KVM: nSVM: Use KVM-governed feature framework to track "TSC scaling enabled"
        https://github.com/kvm-x86/linux/commit/4365a45571c7
[10/15] KVM: nSVM: Use KVM-governed feature framework to track "vVM{SAVE,LOAD} enabled"
        https://github.com/kvm-x86/linux/commit/4d2a1560ffc2
[11/15] KVM: nSVM: Use KVM-governed feature framework to track "LBRv enabled"
        https://github.com/kvm-x86/linux/commit/e183d17ac362
[12/15] KVM: nSVM: Use KVM-governed feature framework to track "Pause Filter enabled"
        https://github.com/kvm-x86/linux/commit/59d67fc1f0db
[13/15] KVM: nSVM: Use KVM-governed feature framework to track "vGIF enabled"
        https://github.com/kvm-x86/linux/commit/b89456aee78d
[14/15] KVM: nSVM: Use KVM-governed feature framework to track "vNMI enabled"
        https://github.com/kvm-x86/linux/commit/ee785c870d6f
[15/15] KVM: x86: Disallow guest CPUID lookups when IRQs are disabled
        https://github.com/kvm-x86/linux/commit/9717efbe5ba3

--
https://github.com/kvm-x86/linux/tree/next
https://github.com/kvm-x86/linux/tree/fixes