Message ID | 20230217231022.816138-2-seanjc@google.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | KVM: x86: Add "governed" X86_FEATURE framework | expand |
Sean Christopherson <seanjc@google.com> writes: > Introduce yet another X86_FEATURE flag framework to manage and cache KVM > governed features (for lack of a better term). "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] Nit: unneeded ']' > SVM features" track whether or not a flag is exposed to L1. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/include/asm/kvm_host.h | 11 +++++++ > arch/x86/kvm/cpuid.c | 2 ++ > arch/x86/kvm/cpuid.h | 51 ++++++++++++++++++++++++++++++++ > arch/x86/kvm/governed_features.h | 9 ++++++ > 4 files changed, 73 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 792a6037047a..cd660de02f7b 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -835,6 +835,17 @@ struct kvm_vcpu_arch { > struct kvm_cpuid_entry2 *cpuid_entries; > struct kvm_hypervisor_cpuid kvm_cpuid; > > + /* > + * 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 { > + u32 enabled; > + } governed_features; > + > u64 reserved_gpa_bits; > int maxphyaddr; > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 8f8edeaf8177..013fdc27fc8f 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -335,6 +335,8 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) > struct kvm_lapic *apic = vcpu->arch.apic; > struct kvm_cpuid_entry2 *best; > > + vcpu->arch.governed_features.enabled = 0; > + > 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..f61a2106ba90 100644 > --- a/arch/x86/kvm/cpuid.h > +++ b/arch/x86/kvm/cpuid.h > @@ -232,4 +232,55 @@ 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 int kvm_is_governed_feature(unsigned int x86_feature) > +{ > + return kvm_governed_feature_index(x86_feature) >= 0; > +} > + > +static __always_inline u32 kvm_governed_feature_bit(unsigned int x86_feature) > +{ > + int index = kvm_governed_feature_index(x86_feature); > + > + BUILD_BUG_ON(index < 0); > + return BIT(index); > +} > + > +static __always_inline void kvm_governed_feature_set(struct kvm_vcpu *vcpu, > + unsigned int x86_feature) > +{ > + BUILD_BUG_ON(KVM_NR_GOVERNED_FEATURES > > + sizeof(vcpu->arch.governed_features.enabled) * BITS_PER_BYTE); > + > + vcpu->arch.governed_features.enabled |= kvm_governed_feature_bit(x86_feature); > +} > + > +static __always_inline void kvm_governed_feature_check_and_set(struct kvm_vcpu *vcpu, > + unsigned int x86_feature) > +{ > + if (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) > +{ > + return vcpu->arch.governed_features.enabled & kvm_governed_feature_bit(x86_feature); > +} > + > #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
On 2/18/2023 7:10 AM, Sean Christopherson wrote: > Introduce yet another X86_FEATURE flag framework to manage and cache KVM > governed features (for lack of a better term). "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] spare ] > SVM features" track whether or not a flag is exposed to L1. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/include/asm/kvm_host.h | 11 +++++++ > arch/x86/kvm/cpuid.c | 2 ++ > arch/x86/kvm/cpuid.h | 51 ++++++++++++++++++++++++++++++++ > arch/x86/kvm/governed_features.h | 9 ++++++ > 4 files changed, 73 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 792a6037047a..cd660de02f7b 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -835,6 +835,17 @@ struct kvm_vcpu_arch { > struct kvm_cpuid_entry2 *cpuid_entries; > struct kvm_hypervisor_cpuid kvm_cpuid; > > + /* > + * 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 { > + u32 enabled; Although there are some guidances/preconditions of using the framework, is it possible that u32 will be ran out quickly after people starts to use the framework? Of course, I noticed there is build bug check on the length, it should be OK to increase the length when needed. > + } governed_features; > + > u64 reserved_gpa_bits; > int maxphyaddr; > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 8f8edeaf8177..013fdc27fc8f 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -335,6 +335,8 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) > struct kvm_lapic *apic = vcpu->arch.apic; > struct kvm_cpuid_entry2 *best; > > + vcpu->arch.governed_features.enabled = 0; > + > 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..f61a2106ba90 100644 > --- a/arch/x86/kvm/cpuid.h > +++ b/arch/x86/kvm/cpuid.h > @@ -232,4 +232,55 @@ 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 int kvm_is_governed_feature(unsigned int x86_feature) Is it better to use bool instead of int? > +{ > + return kvm_governed_feature_index(x86_feature) >= 0; > +} > + > +static __always_inline u32 kvm_governed_feature_bit(unsigned int x86_feature) > +{ > + int index = kvm_governed_feature_index(x86_feature); > + > + BUILD_BUG_ON(index < 0); > + return BIT(index); > +} > + > +static __always_inline void kvm_governed_feature_set(struct kvm_vcpu *vcpu, > + unsigned int x86_feature) > +{ > + BUILD_BUG_ON(KVM_NR_GOVERNED_FEATURES > > + sizeof(vcpu->arch.governed_features.enabled) * BITS_PER_BYTE); > + > + vcpu->arch.governed_features.enabled |= kvm_governed_feature_bit(x86_feature); > +} > + > +static __always_inline void kvm_governed_feature_check_and_set(struct kvm_vcpu *vcpu, > + unsigned int x86_feature) > +{ > + if (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) > +{ > + return vcpu->arch.governed_features.enabled & kvm_governed_feature_bit(x86_feature); > +} > + > #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
On Thu, Jun 29, 2023, Binbin Wu wrote: > > > On 2/18/2023 7:10 AM, Sean Christopherson wrote: > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > index 792a6037047a..cd660de02f7b 100644 > > --- a/arch/x86/include/asm/kvm_host.h > > +++ b/arch/x86/include/asm/kvm_host.h > > @@ -835,6 +835,17 @@ struct kvm_vcpu_arch { > > struct kvm_cpuid_entry2 *cpuid_entries; > > struct kvm_hypervisor_cpuid kvm_cpuid; > > + /* > > + * 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 { > > + u32 enabled; > Although there are some guidances/preconditions of using the framework, > is it possible that u32 will be ran out quickly after people starts to use > the framework? It's definitely possible. And there's no reason to limit this to a u32, I really have no idea why I did that. Ah, it's because "struct kvm_vcpu_arch" is defined in arch/x86/include/asm/kvm_host.h, and I didn't want to expose governed_features.h in arch/x86/include/asm. Hrm, that's really annoying. Aha! A better workaround for that conudrum would be to define an explicit "max" and use that, with a FIXME to call out that this really should use KVM_NR_GOVERNED_FEATURES directly. I have aspirations of moving kvm_host.h to arch/<arch>/kvm, at which point this can be cleaned up by declaring "enum kvm_governed_features" in kvm_host.h (though it'll likely be named something like kvm_arch.h at that point). /* * 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; > Of course, I noticed there is build� bug check on the length, it should be > OK to increase the length when needed. > > +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 int kvm_is_governed_feature(unsigned int x86_feature) > Is it better to use bool instead of int? Yes, this definitely should return a bool. Thanks!
On Fri, Feb 17, 2023 at 03:10:11PM -0800, Sean Christopherson wrote: >+static __always_inline void kvm_governed_feature_set(struct kvm_vcpu *vcpu, >+ unsigned int x86_feature) >+{ >+ BUILD_BUG_ON(KVM_NR_GOVERNED_FEATURES > >+ sizeof(vcpu->arch.governed_features.enabled) * BITS_PER_BYTE); >+ >+ vcpu->arch.governed_features.enabled |= kvm_governed_feature_bit(x86_feature); >+} >+ >+static __always_inline void kvm_governed_feature_check_and_set(struct kvm_vcpu *vcpu, >+ unsigned int x86_feature) >+{ >+ if (guest_cpuid_has(vcpu, x86_feature)) Most callers in this series are conditional on either boot_cpu_has() or some local variables. Can we convert them to kvm_cpu_cap_has() and incorporate them within this function? i.e., if (kvm_cpu_cap_has(x86_feature) && guest_cpuid_has(vcpu, x86_feature)) The benefits of doing so are 1. callers needn't repeat if (kvm_cpu_cap_has(x86_feature)) kvm_governed_feature_check_and_set(x86_feature) 2. this fits the idea better that guests can use a governed feature only if host supports it _and_ QEMU exposes it to the guest. >+ kvm_governed_feature_set(vcpu, x86_feature); >+} >+
On Fri, Jun 30, 2023, Chao Gao wrote: > On Fri, Feb 17, 2023 at 03:10:11PM -0800, Sean Christopherson wrote: > >+static __always_inline void kvm_governed_feature_set(struct kvm_vcpu *vcpu, > >+ unsigned int x86_feature) > >+{ > >+ BUILD_BUG_ON(KVM_NR_GOVERNED_FEATURES > > >+ sizeof(vcpu->arch.governed_features.enabled) * BITS_PER_BYTE); > >+ > >+ vcpu->arch.governed_features.enabled |= kvm_governed_feature_bit(x86_feature); > >+} > >+ > >+static __always_inline void kvm_governed_feature_check_and_set(struct kvm_vcpu *vcpu, > >+ unsigned int x86_feature) > >+{ > >+ if (guest_cpuid_has(vcpu, x86_feature)) > > Most callers in this series are conditional on either boot_cpu_has() or some > local variables. Can we convert them to kvm_cpu_cap_has() and incorporate them > within this function? i.e., > > if (kvm_cpu_cap_has(x86_feature) && guest_cpuid_has(vcpu, x86_feature)) Hmm, I was going to say "no", as most callers don't check kvm_cpu_cap_has() verbatim, but it doesn't have to be that way. The majority of SVM features factor in module params, but KVM should set the kvm_cpu capability if and only if a feature is supported in hardware *and* enabled by its module param. And arguably that's kinda sorta a bug fix, because this if (lbrv) kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_LBRV); technically should be if (lbrv && nested) kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_LBRV); Heh, and it's kinda sorta a bug fix for XSAVES on VMX, because this if (cpu_has_vmx_xsaves() && boot_cpu_has(X86_FEATURE_XSAVE) && guest_cpuid_has(vcpu, X86_FEATURE_XSAVE)) kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_XSAVES); should technically be if (kvm_cpu_cap_has(X86_FEATURE_XSAVES) && boot_cpu_has(X86_FEATURE_XSAVE) && guest_cpuid_has(vcpu, X86_FEATURE_XSAVE)) kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_XSAVES); > The benefits of doing so are > 1. callers needn't repeat > > if (kvm_cpu_cap_has(x86_feature)) > kvm_governed_feature_check_and_set(x86_feature) > > 2. this fits the idea better that guests can use a governed feature only if host > supports it _and_ QEMU exposes it to the guest. Agreed, especially since we'll still have kvm_governed_feature_set() for the extra special cases. Thanks for the input!
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 792a6037047a..cd660de02f7b 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -835,6 +835,17 @@ struct kvm_vcpu_arch { struct kvm_cpuid_entry2 *cpuid_entries; struct kvm_hypervisor_cpuid kvm_cpuid; + /* + * 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 { + u32 enabled; + } governed_features; + u64 reserved_gpa_bits; int maxphyaddr; diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 8f8edeaf8177..013fdc27fc8f 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -335,6 +335,8 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) struct kvm_lapic *apic = vcpu->arch.apic; struct kvm_cpuid_entry2 *best; + vcpu->arch.governed_features.enabled = 0; + 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..f61a2106ba90 100644 --- a/arch/x86/kvm/cpuid.h +++ b/arch/x86/kvm/cpuid.h @@ -232,4 +232,55 @@ 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 int kvm_is_governed_feature(unsigned int x86_feature) +{ + return kvm_governed_feature_index(x86_feature) >= 0; +} + +static __always_inline u32 kvm_governed_feature_bit(unsigned int x86_feature) +{ + int index = kvm_governed_feature_index(x86_feature); + + BUILD_BUG_ON(index < 0); + return BIT(index); +} + +static __always_inline void kvm_governed_feature_set(struct kvm_vcpu *vcpu, + unsigned int x86_feature) +{ + BUILD_BUG_ON(KVM_NR_GOVERNED_FEATURES > + sizeof(vcpu->arch.governed_features.enabled) * BITS_PER_BYTE); + + vcpu->arch.governed_features.enabled |= kvm_governed_feature_bit(x86_feature); +} + +static __always_inline void kvm_governed_feature_check_and_set(struct kvm_vcpu *vcpu, + unsigned int x86_feature) +{ + if (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) +{ + return vcpu->arch.governed_features.enabled & kvm_governed_feature_bit(x86_feature); +} + #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
Introduce yet another X86_FEATURE flag framework to manage and cache KVM governed features (for lack of a better term). "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. Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/include/asm/kvm_host.h | 11 +++++++ arch/x86/kvm/cpuid.c | 2 ++ arch/x86/kvm/cpuid.h | 51 ++++++++++++++++++++++++++++++++ arch/x86/kvm/governed_features.h | 9 ++++++ 4 files changed, 73 insertions(+) create mode 100644 arch/x86/kvm/governed_features.h