On Fri, 2023-11-10 at 15:55 -0800, Sean Christopherson wrote:
> Initialize a vCPU's capabilities based on the guest CPUID provided by
> userspace instead of simply zeroing the entire array. This will allow
> using cpu_caps to query *all* CPUID-based guest capabilities, i.e. will
> allow converting all usage of guest_cpuid_has() to guest_cpu_cap_has().
>
> Zeroing the array was the logical choice when using cpu_caps was opt-in,
> e.g. "unsupported" was generally a safer default, and the whole point of
> governed features is that KVM would need to check host and guest support,
> i.e. making everything unsupported by default didn't require more code.
>
> But requiring KVM to manually "enable" every CPUID-based feature in
> cpu_caps would require an absurd amount of boilerplate code.
>
> Follow existing CPUID/kvm_cpu_caps nomenclature where possible, e.g. for
> the change() and clear() APIs. Replace check_and_set() with restrict() to
> try and capture that KVM is restricting userspace's desired guest feature
> set based on KVM's capabilities.
>
> This is intended to be gigantic nop, i.e. should not have any impact on
> guest or KVM functionality.
>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/x86/kvm/cpuid.c | 43 +++++++++++++++++++++++++++++++++++++++---
> arch/x86/kvm/cpuid.h | 25 +++++++++++++++++++++---
> arch/x86/kvm/svm/svm.c | 24 +++++++++++------------
> arch/x86/kvm/vmx/vmx.c | 6 ++++--
> 4 files changed, 78 insertions(+), 20 deletions(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 4bf3c2d4dc7c..5cf3d697ecb3 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -321,13 +321,51 @@ static bool kvm_cpuid_has_hyperv(struct kvm_cpuid_entry2 *entries, int nent)
> return entry && entry->eax == HYPERV_CPUID_SIGNATURE_EAX;
> }
>
> +/*
> + * This isn't truly "unsafe", but all callers except kvm_cpu_after_set_cpuid()
> + * should use __cpuid_entry_get_reg(), which provides compile-time validation
> + * of the input.
> + */
> +static u32 cpuid_get_reg_unsafe(struct kvm_cpuid_entry2 *entry, u32 reg)
> +{
> + switch (reg) {
> + case CPUID_EAX:
> + return entry->eax;
> + case CPUID_EBX:
> + return entry->ebx;
> + case CPUID_ECX:
> + return entry->ecx;
> + case CPUID_EDX:
> + return entry->edx;
> + default:
> + WARN_ON_ONCE(1);
> + return 0;
> + }
> +}
> +
> 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;
> + int i;
>
> - memset(vcpu->arch.cpu_caps, 0, sizeof(vcpu->arch.cpu_caps));
> + BUILD_BUG_ON(ARRAY_SIZE(reverse_cpuid) != NR_KVM_CPU_CAPS);
> +
> + /*
> + * Reset guest capabilities to userspace's guest CPUID definition, i.e.
> + * honor userspace's definition for features that don't require KVM or
> + * hardware management/support (or that KVM simply doesn't care about).
> + */
> + for (i = 0; i < NR_KVM_CPU_CAPS; i++) {
> + const struct cpuid_reg cpuid = reverse_cpuid[i];
> +
> + best = kvm_find_cpuid_entry_index(vcpu, cpuid.function, cpuid.index);
> + if (best)
> + vcpu->arch.cpu_caps[i] = cpuid_get_reg_unsafe(best, cpuid.reg);
Why not just use __cpuid_entry_get_reg?
cpuid.reg comes from read/only 'reverse_cpuid' anyway, and in fact
it seems that all callers of __cpuid_entry_get_reg, take the reg value from
x86_feature_cpuid() which also takes it from 'reverse_cpuid'.
So if the compiler is smart enough to not complain in these cases, I don't see why this case
is different.
Also why not to initialize guest_caps = host_caps & userspace_cpuid?
If this was the default we won't need any guest_cpu_cap_restrict and such,
instead it will just work.
Special code will only be needed in few more complex cases, like forced exposed
of a feature to a guest due to a virtualization hole.
> + else
> + vcpu->arch.cpu_caps[i] = 0;
> + }
>
> /*
> * If TDP is enabled, let the guest use GBPAGES if they're supported in
> @@ -342,8 +380,7 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> */
> allow_gbpages = tdp_enabled ? boot_cpu_has(X86_FEATURE_GBPAGES) :
> guest_cpuid_has(vcpu, X86_FEATURE_GBPAGES);
> - if (allow_gbpages)
> - guest_cpu_cap_set(vcpu, X86_FEATURE_GBPAGES);
> + guest_cpu_cap_change(vcpu, X86_FEATURE_GBPAGES, allow_gbpages);
IMHO the original code was more readable, now I need to look up the 'guest_cpu_cap_change()'
to understand what is going on.
>
> best = kvm_find_cpuid_entry(vcpu, 1);
> if (best && apic) {
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index 9f18c4395b71..1707ef10b269 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -263,11 +263,30 @@ static __always_inline void guest_cpu_cap_set(struct kvm_vcpu *vcpu,
> vcpu->arch.cpu_caps[x86_leaf] |= __feature_bit(x86_feature);
> }
>
> -static __always_inline void guest_cpu_cap_check_and_set(struct kvm_vcpu *vcpu,
> - unsigned int x86_feature)
> +static __always_inline void guest_cpu_cap_clear(struct kvm_vcpu *vcpu,
> + unsigned int x86_feature)
> {
> - if (kvm_cpu_cap_has(x86_feature) && guest_cpuid_has(vcpu, x86_feature))
> + unsigned int x86_leaf = __feature_leaf(x86_feature);
> +
> + reverse_cpuid_check(x86_leaf);
> + vcpu->arch.cpu_caps[x86_leaf] &= ~__feature_bit(x86_feature);
> +}
> +
> +static __always_inline void guest_cpu_cap_change(struct kvm_vcpu *vcpu,
> + unsigned int x86_feature,
> + bool guest_has_cap)
> +{
> + if (guest_has_cap)
> guest_cpu_cap_set(vcpu, x86_feature);
> + else
> + guest_cpu_cap_clear(vcpu, x86_feature);
> +}
Let's not have this function, it's just not worth it IMHO.
> +
> +static __always_inline void guest_cpu_cap_restrict(struct kvm_vcpu *vcpu,
> + unsigned int x86_feature)
> +{
> + if (!kvm_cpu_cap_has(x86_feature))
> + guest_cpu_cap_clear(vcpu, x86_feature);
> }
The purpose of this function is also very hard to decipher.
If we initialize guest_caps = host_caps & guest_cpuid then we won't
need this function.
>
> static __always_inline bool guest_cpu_cap_has(struct kvm_vcpu *vcpu,
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 8a99a73b6ee5..5827328e30f1 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4315,14 +4315,14 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> * 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))
> - guest_cpu_cap_set(vcpu, X86_FEATURE_XSAVES);
> + guest_cpu_cap_change(vcpu, X86_FEATURE_XSAVES,
> + boot_cpu_has(X86_FEATURE_XSAVE) &&
> + boot_cpu_has(X86_FEATURE_XSAVES) &&
> + guest_cpuid_has(vcpu, X86_FEATURE_XSAVE));
In theory this change does change behavior, now the X86_FEATURE_XSAVE will
be set iff the condition is true, but before it was set *if* the condition was true.
>
> - guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_NRIPS);
> - guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_TSCRATEMSR);
> - guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_LBRV);
> + guest_cpu_cap_restrict(vcpu, X86_FEATURE_NRIPS);
> + guest_cpu_cap_restrict(vcpu, X86_FEATURE_TSCRATEMSR);
> + guest_cpu_cap_restrict(vcpu, X86_FEATURE_LBRV);
One of the main reasons I don't like governed features is this manual list.
I want to reach the point that one won't need to add anything manually, unless there
is a good reason to do so, and there are only a few exceptions when the guest cap is set,
while the host's isn't.
>
> /*
> * Intercept VMLOAD if the vCPU mode is Intel in order to emulate that
> @@ -4330,12 +4330,12 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> * SVM on Intel is bonkers and extremely unlikely to work).
> */
> if (!guest_cpuid_is_intel(vcpu))
> - guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_V_VMSAVE_VMLOAD);
> + guest_cpu_cap_restrict(vcpu, X86_FEATURE_V_VMSAVE_VMLOAD);
>
> - guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_PAUSEFILTER);
> - guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_PFTHRESHOLD);
> - guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_VGIF);
> - guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_VNMI);
> + guest_cpu_cap_restrict(vcpu, X86_FEATURE_PAUSEFILTER);
> + guest_cpu_cap_restrict(vcpu, X86_FEATURE_PFTHRESHOLD);
> + guest_cpu_cap_restrict(vcpu, X86_FEATURE_VGIF);
> + guest_cpu_cap_restrict(vcpu, X86_FEATURE_VNMI);
>
> svm_recalc_instruction_intercepts(vcpu, svm);
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 6328f0d47c64..5a056ad1ae55 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7757,9 +7757,11 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> */
> if (boot_cpu_has(X86_FEATURE_XSAVE) &&
> guest_cpuid_has(vcpu, X86_FEATURE_XSAVE))
> - guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_XSAVES);
> + guest_cpu_cap_restrict(vcpu, X86_FEATURE_XSAVES);
> + else
> + guest_cpu_cap_clear(vcpu, X86_FEATURE_XSAVES);
>
> - guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_VMX);
> + guest_cpu_cap_restrict(vcpu, X86_FEATURE_VMX);
>
> vmx_setup_uret_msrs(vmx);
>
Best regards,
Maxim Levitsky
On Sun, Nov 19, 2023, Maxim Levitsky wrote:
> On Fri, 2023-11-10 at 15:55 -0800, Sean Christopherson wrote:
> > +/*
> > + * This isn't truly "unsafe", but all callers except kvm_cpu_after_set_cpuid()
> > + * should use __cpuid_entry_get_reg(), which provides compile-time validation
> > + * of the input.
> > + */
> > +static u32 cpuid_get_reg_unsafe(struct kvm_cpuid_entry2 *entry, u32 reg)
> > +{
> > + switch (reg) {
> > + case CPUID_EAX:
> > + return entry->eax;
> > + case CPUID_EBX:
> > + return entry->ebx;
> > + case CPUID_ECX:
> > + return entry->ecx;
> > + case CPUID_EDX:
> > + return entry->edx;
> > + default:
> > + WARN_ON_ONCE(1);
> > + return 0;
> > + }
> > +}
...
> > 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;
> > + int i;
> >
> > - memset(vcpu->arch.cpu_caps, 0, sizeof(vcpu->arch.cpu_caps));
> > + BUILD_BUG_ON(ARRAY_SIZE(reverse_cpuid) != NR_KVM_CPU_CAPS);
> > +
> > + /*
> > + * Reset guest capabilities to userspace's guest CPUID definition, i.e.
> > + * honor userspace's definition for features that don't require KVM or
> > + * hardware management/support (or that KVM simply doesn't care about).
> > + */
> > + for (i = 0; i < NR_KVM_CPU_CAPS; i++) {
> > + const struct cpuid_reg cpuid = reverse_cpuid[i];
> > +
> > + best = kvm_find_cpuid_entry_index(vcpu, cpuid.function, cpuid.index);
> > + if (best)
> > + vcpu->arch.cpu_caps[i] = cpuid_get_reg_unsafe(best, cpuid.reg);
>
> Why not just use __cpuid_entry_get_reg?
>
> cpuid.reg comes from read/only 'reverse_cpuid' anyway, and in fact
> it seems that all callers of __cpuid_entry_get_reg, take the reg value from
> x86_feature_cpuid() which also takes it from 'reverse_cpuid'.
>
> So if the compiler is smart enough to not complain in these cases, I don't
> see why this case is different.
It's because the input isn't a compile-time constant, and so the BUILD_BUG() in
the default path will fire. All of the compile-time assertions in reverse_cpuid.h
rely on the feature being a constant value, which allows the compiler to optimize
away the dead paths, i.e. turn __cpuid_entry_get_reg()'s switch statement into
simple pointer arithmetic and thus omit the BUILD_BUG() code.
> Also why not to initialize guest_caps = host_caps & userspace_cpuid?
>
> If this was the default we won't need any guest_cpu_cap_restrict and such,
> instead it will just work.
Hrm, I definitely like the idea. Unfortunately, unless we do an audit of all
~120 uses of guest_cpuid_has(), restricting those based on kvm_cpu_caps might
break userspace.
Aside from purging the governed feature nomenclature, the main goal of this series
provide a way to do fast lookups of all known guest CPUID bits without needing to
opt-in on a feature-by-feature basis, including for features that are fully
controlled by userspace.
It's definitely doable, but I'm not all that confident that the end result would
be a net positive, e.g. I believe we would need to special case things like the
feature bits that gate MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD. MOVBE and RDPID
are other features that come to mind, where KVM emulates the feature in software
but it won't be set in kvm_cpu_caps.
Oof, and MONITOR and MWAIT too, as KVM deliberately doesn't advertise those to
userspace.
So yeah, I'm not opposed to trying that route at some point, but I really don't
want to do that in this series as the risk of subtly breaking something is super
high.
> Special code will only be needed in few more complex cases, like forced exposed
> of a feature to a guest due to a virtualization hole.
>
>
> > + else
> > + vcpu->arch.cpu_caps[i] = 0;
> > + }
> >
> > /*
> > * If TDP is enabled, let the guest use GBPAGES if they're supported in
> > @@ -342,8 +380,7 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> > */
> > allow_gbpages = tdp_enabled ? boot_cpu_has(X86_FEATURE_GBPAGES) :
> > guest_cpuid_has(vcpu, X86_FEATURE_GBPAGES);
> > - if (allow_gbpages)
> > - guest_cpu_cap_set(vcpu, X86_FEATURE_GBPAGES);
> > + guest_cpu_cap_change(vcpu, X86_FEATURE_GBPAGES, allow_gbpages);
>
> IMHO the original code was more readable, now I need to look up the
> 'guest_cpu_cap_change()' to understand what is going on.
The change is "necessary". The issue is that with the caps 0-initialied, the
!allow_gbpages could simply do nothing. Now, KVM needs to explicitly clear the
flag, i.e. would need to do:
if (allow_gbpages)
guest_cpu_cap_set(vcpu, X86_FEATURE_GBPAGES);
else
guest_cpu_cap_clear(vcpu, X86_FEATURE_GBPAGES);
I don't much love the name either, but it pairs with cpuid_entry_change() and I
want to keep the kvm_cpu_cap, cpuid_entry, and guest_cpu_cap APIs in sync as far
as the APIs go. The only reason kvm_cpu_cap_change() doesn't exist is because
there aren't any flows that need to toggle a bit.
> > static __always_inline bool guest_cpu_cap_has(struct kvm_vcpu *vcpu,
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index 8a99a73b6ee5..5827328e30f1 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -4315,14 +4315,14 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> > * 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))
> > - guest_cpu_cap_set(vcpu, X86_FEATURE_XSAVES);
> > + guest_cpu_cap_change(vcpu, X86_FEATURE_XSAVES,
> > + boot_cpu_has(X86_FEATURE_XSAVE) &&
> > + boot_cpu_has(X86_FEATURE_XSAVES) &&
> > + guest_cpuid_has(vcpu, X86_FEATURE_XSAVE));
>
> In theory this change does change behavior, now the X86_FEATURE_XSAVE will
> be set iff the condition is true, but before it was set *if* the condition was true.
No, before it was set if and only if the condition was true, because in that case
caps were 0-initialized, i.e. this was/is the only way for XSAVE to be set.
> > - guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_NRIPS);
> > - guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_TSCRATEMSR);
> > - guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_LBRV);
> > + guest_cpu_cap_restrict(vcpu, X86_FEATURE_NRIPS);
> > + guest_cpu_cap_restrict(vcpu, X86_FEATURE_TSCRATEMSR);
> > + guest_cpu_cap_restrict(vcpu, X86_FEATURE_LBRV);
>
> One of the main reasons I don't like governed features is this manual list.
To be fair, the manual lists predate the governed features.
> I want to reach the point that one won't need to add anything manually,
> unless there is a good reason to do so, and there are only a few exceptions
> when the guest cap is set, while the host's isn't.
Yeah, agreed.
On Thu, 2023-11-30 at 17:51 -0800, Sean Christopherson wrote:
> On Sun, Nov 19, 2023, Maxim Levitsky wrote:
> > On Fri, 2023-11-10 at 15:55 -0800, Sean Christopherson wrote:
> > > +/*
> > > + * This isn't truly "unsafe", but all callers except kvm_cpu_after_set_cpuid()
> > > + * should use __cpuid_entry_get_reg(), which provides compile-time validation
> > > + * of the input.
> > > + */
> > > +static u32 cpuid_get_reg_unsafe(struct kvm_cpuid_entry2 *entry, u32 reg)
> > > +{
> > > + switch (reg) {
> > > + case CPUID_EAX:
> > > + return entry->eax;
> > > + case CPUID_EBX:
> > > + return entry->ebx;
> > > + case CPUID_ECX:
> > > + return entry->ecx;
> > > + case CPUID_EDX:
> > > + return entry->edx;
> > > + default:
> > > + WARN_ON_ONCE(1);
> > > + return 0;
> > > + }
> > > +}
>
> ...
>
> > > 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;
> > > + int i;
> > >
> > > - memset(vcpu->arch.cpu_caps, 0, sizeof(vcpu->arch.cpu_caps));
> > > + BUILD_BUG_ON(ARRAY_SIZE(reverse_cpuid) != NR_KVM_CPU_CAPS);
> > > +
> > > + /*
> > > + * Reset guest capabilities to userspace's guest CPUID definition, i.e.
> > > + * honor userspace's definition for features that don't require KVM or
> > > + * hardware management/support (or that KVM simply doesn't care about).
> > > + */
> > > + for (i = 0; i < NR_KVM_CPU_CAPS; i++) {
> > > + const struct cpuid_reg cpuid = reverse_cpuid[i];
> > > +
> > > + best = kvm_find_cpuid_entry_index(vcpu, cpuid.function, cpuid.index);
> > > + if (best)
> > > + vcpu->arch.cpu_caps[i] = cpuid_get_reg_unsafe(best, cpuid.reg);
> >
> > Why not just use __cpuid_entry_get_reg?
> >
> > cpuid.reg comes from read/only 'reverse_cpuid' anyway, and in fact
> > it seems that all callers of __cpuid_entry_get_reg, take the reg value from
> > x86_feature_cpuid() which also takes it from 'reverse_cpuid'.
> >
> > So if the compiler is smart enough to not complain in these cases, I don't
> > see why this case is different.
>
> It's because the input isn't a compile-time constant, and so the BUILD_BUG() in
> the default path will fire.
> All of the compile-time assertions in reverse_cpuid.h
> rely on the feature being a constant value, which allows the compiler to optimize
> away the dead paths, i.e. turn __cpuid_entry_get_reg()'s switch statement into
> simple pointer arithmetic and thus omit the BUILD_BUG() code.
In the above code, assuming that the compiler really treats the reverse_cpuid as const
(if that assumption is not true, then all uses of __cpuid_entry_get_reg are also not compile
time constant either),
then the 'reg' value depends only on 'i', and therefore for each iteration of the loop,
the compiler does know the compile time value of the 'reg',
and so it can easily prove that 'default' case in __cpuid_entry_get_reg can't be reached,
and thus eliminate that BUILD_BUG().
>
> > Also why not to initialize guest_caps = host_caps & userspace_cpuid?
> >
> > If this was the default we won't need any guest_cpu_cap_restrict and such,
> > instead it will just work.
>
> Hrm, I definitely like the idea. Unfortunately, unless we do an audit of all
> ~120 uses of guest_cpuid_has(), restricting those based on kvm_cpu_caps might
> break userspace.
120 uses is not that bad, IMHO it is worth it - we won't need to deal with that
in the future.
How about a compromise - you change the patches such as it will be possible to remove
these cases one by one, and also all new cases will be fully automatic?
>
> Aside from purging the governed feature nomenclature, the main goal of this series
> provide a way to do fast lookups of all known guest CPUID bits without needing to
> opt-in on a feature-by-feature basis, including for features that are fully
> controlled by userspace.
I'll note that, this makes sense.
>
> It's definitely doable, but I'm not all that confident that the end result would
> be a net positive, e.g. I believe we would need to special case things like the
> feature bits that gate MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD. MOVBE and RDPID
> are other features that come to mind, where KVM emulates the feature in software
> but it won't be set in kvm_cpu_caps.
>
> Oof, and MONITOR and MWAIT too, as KVM deliberately doesn't advertise those to
> userspace.
>
> So yeah, I'm not opposed to trying that route at some point, but I really don't
> want to do that in this series as the risk of subtly breaking something is super
> high.
>
> > Special code will only be needed in few more complex cases, like forced exposed
> > of a feature to a guest due to a virtualization hole.
> >
> >
> > > + else
> > > + vcpu->arch.cpu_caps[i] = 0;
> > > + }
> > >
> > > /*
> > > * If TDP is enabled, let the guest use GBPAGES if they're supported in
> > > @@ -342,8 +380,7 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> > > */
> > > allow_gbpages = tdp_enabled ? boot_cpu_has(X86_FEATURE_GBPAGES) :
> > > guest_cpuid_has(vcpu, X86_FEATURE_GBPAGES);
> > > - if (allow_gbpages)
> > > - guest_cpu_cap_set(vcpu, X86_FEATURE_GBPAGES);
> > > + guest_cpu_cap_change(vcpu, X86_FEATURE_GBPAGES, allow_gbpages);
> >
> > IMHO the original code was more readable, now I need to look up the
> > 'guest_cpu_cap_change()' to understand what is going on.
>
> The change is "necessary". The issue is that with the caps 0-initialied, the
> !allow_gbpages could simply do nothing. Now, KVM needs to explicitly clear the
> flag, i.e. would need to do:
>
> if (allow_gbpages)
> guest_cpu_cap_set(vcpu, X86_FEATURE_GBPAGES);
> else
> guest_cpu_cap_clear(vcpu, X86_FEATURE_GBPAGES);
I understand now but I am complaining more about the fact that I like the
explicit longer version better than calling guest_cpu_cap_change because it's not obvious
what guest_cpu_cap_change really does. I am not going to fight over this though,
just saying.
>
> I don't much love the name either, but it pairs with cpuid_entry_change() and I
> want to keep the kvm_cpu_cap, cpuid_entry, and guest_cpu_cap APIs in sync as far
> as the APIs go. The only reason kvm_cpu_cap_change() doesn't exist is because
> there aren't any flows that need to toggle a bit.
>
> > > static __always_inline bool guest_cpu_cap_has(struct kvm_vcpu *vcpu,
> > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > > index 8a99a73b6ee5..5827328e30f1 100644
> > > --- a/arch/x86/kvm/svm/svm.c
> > > +++ b/arch/x86/kvm/svm/svm.c
> > > @@ -4315,14 +4315,14 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> > > * 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))
> > > - guest_cpu_cap_set(vcpu, X86_FEATURE_XSAVES);
> > > + guest_cpu_cap_change(vcpu, X86_FEATURE_XSAVES,
> > > + boot_cpu_has(X86_FEATURE_XSAVE) &&
> > > + boot_cpu_has(X86_FEATURE_XSAVES) &&
> > > + guest_cpuid_has(vcpu, X86_FEATURE_XSAVE));
> >
> > In theory this change does change behavior, now the X86_FEATURE_XSAVE will
> > be set iff the condition is true, but before it was set *if* the condition was true.
>
> No, before it was set if and only if the condition was true, because in that case
> caps were 0-initialized, i.e. this was/is the only way for XSAVE to be set.
>
> > > - guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_NRIPS);
> > > - guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_TSCRATEMSR);
> > > - guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_LBRV);
> > > + guest_cpu_cap_restrict(vcpu, X86_FEATURE_NRIPS);
> > > + guest_cpu_cap_restrict(vcpu, X86_FEATURE_TSCRATEMSR);
> > > + guest_cpu_cap_restrict(vcpu, X86_FEATURE_LBRV);
> >
> > One of the main reasons I don't like governed features is this manual list.
>
> To be fair, the manual lists predate the governed features.
100% agree, however the point of governed features was to simplify this list,
the point of this patch set is to simplify these lists and yet they still remain,
more or less untouched, and we will still need to maintain them.
Again I do think that governed features and/or this patchset are better than
the mess that was there before, but a part of me wants to fully get rid of this mess instead
of just making it a bit more beautiful.
>
> > I want to reach the point that one won't need to add anything manually,
> > unless there is a good reason to do so, and there are only a few exceptions
> > when the guest cap is set, while the host's isn't.
>
> Yeah, agreed.
I am glad that we are on the same page here.
Best regards,
Maxim Levitsky
>
On Thu, Dec 21, 2023, Maxim Levitsky wrote:
> On Thu, 2023-11-30 at 17:51 -0800, Sean Christopherson wrote:
> > On Sun, Nov 19, 2023, Maxim Levitsky wrote:
> > > On Fri, 2023-11-10 at 15:55 -0800, Sean Christopherson wrote:
> > > > +/*
> > > > + * This isn't truly "unsafe", but all callers except kvm_cpu_after_set_cpuid()
> > > > + * should use __cpuid_entry_get_reg(), which provides compile-time validation
> > > > + * of the input.
> > > > + */
> > > > +static u32 cpuid_get_reg_unsafe(struct kvm_cpuid_entry2 *entry, u32 reg)
> > > > +{
> > > > + switch (reg) {
> > > > + case CPUID_EAX:
> > > > + return entry->eax;
> > > > + case CPUID_EBX:
> > > > + return entry->ebx;
> > > > + case CPUID_ECX:
> > > > + return entry->ecx;
> > > > + case CPUID_EDX:
> > > > + return entry->edx;
> > > > + default:
> > > > + WARN_ON_ONCE(1);
> > > > + return 0;
> > > > + }
> > > > +}
> >
> > ...
> >
> > > > 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;
> > > > + int i;
> > > >
> > > > - memset(vcpu->arch.cpu_caps, 0, sizeof(vcpu->arch.cpu_caps));
> > > > + BUILD_BUG_ON(ARRAY_SIZE(reverse_cpuid) != NR_KVM_CPU_CAPS);
> > > > +
> > > > + /*
> > > > + * Reset guest capabilities to userspace's guest CPUID definition, i.e.
> > > > + * honor userspace's definition for features that don't require KVM or
> > > > + * hardware management/support (or that KVM simply doesn't care about).
> > > > + */
> > > > + for (i = 0; i < NR_KVM_CPU_CAPS; i++) {
> > > > + const struct cpuid_reg cpuid = reverse_cpuid[i];
> > > > +
> > > > + best = kvm_find_cpuid_entry_index(vcpu, cpuid.function, cpuid.index);
> > > > + if (best)
> > > > + vcpu->arch.cpu_caps[i] = cpuid_get_reg_unsafe(best, cpuid.reg);
> > >
> > > Why not just use __cpuid_entry_get_reg?
> > >
> > > cpuid.reg comes from read/only 'reverse_cpuid' anyway, and in fact
> > > it seems that all callers of __cpuid_entry_get_reg, take the reg value from
> > > x86_feature_cpuid() which also takes it from 'reverse_cpuid'.
> > >
> > > So if the compiler is smart enough to not complain in these cases, I don't
> > > see why this case is different.
> >
> > It's because the input isn't a compile-time constant, and so the BUILD_BUG() in
> > the default path will fire.
> > All of the compile-time assertions in reverse_cpuid.h
> > rely on the feature being a constant value, which allows the compiler to optimize
> > away the dead paths, i.e. turn __cpuid_entry_get_reg()'s switch statement into
> > simple pointer arithmetic and thus omit the BUILD_BUG() code.
>
> In the above code, assuming that the compiler really treats the reverse_cpuid
> as const (if that assumption is not true, then all uses of __cpuid_entry_get_reg
> are also not compile time constant either),
It's not so much the compiler treating something as const as it is the compiler
generating code that resolves the relevant inputs to compile-time constants.
> then the 'reg' value depends only on 'i', and therefore for each iteration of
> the loop, the compiler does know the compile time value of the 'reg', and so
> it can easily prove that 'default' case in __cpuid_entry_get_reg can't be
> reached, and thus eliminate that BUILD_BUG().
A compiler _could_ know, but as above what truly matters is what code the compiler
actually generates. E.g. all helpers are tagged __always_inline to prevent the
compiler from uninlining the functions (thanks, KASAN), at which point the code
of the non-inline function is no longer dealing with compile-time constants and
so the BUILD_BUG_ON() doesn't get eliminated.
For the loop, while all values are indeed constant, the compiler may choose to
generate a loop instead of unrolling everything. A sufficiently clever compiler
could still detect that nothing in the loop can hit the "default" case, but in
practice neither clang nor gcc does that level of optimization, at least not yet.
> > > Also why not to initialize guest_caps = host_caps & userspace_cpuid?
> > >
> > > If this was the default we won't need any guest_cpu_cap_restrict and such,
> > > instead it will just work.
> >
> > Hrm, I definitely like the idea. Unfortunately, unless we do an audit of all
> > ~120 uses of guest_cpuid_has(), restricting those based on kvm_cpu_caps might
> > break userspace.
>
> 120 uses is not that bad, IMHO it is worth it - we won't need to deal with that
> in the future.
>
> How about a compromise - you change the patches such as it will be possible
> to remove these cases one by one, and also all new cases will be fully
> automatic?
Hrm, I'm not necessarily opposed to that, but I don't think we should go partway
unless we are 100% confident that changing the default to "use guest CPUID ANDed
with KVM capabilities" is the best end state, *and* that someone will actually
have the bandwidth to do the work soon-ish so that KVM isn't in a half-baked
state for months on end. Even then, my preference would definitely be to switch
everything in one go.
And automatically handling new features would only be feasible for entirely new
leafs. E.g. X86_FEATURE_RDPID is buried in CPUID.0x7.0x0.ECX, so to automatically
handle new features KVM would need to set the default guest_caps for all bits
*except* RDPID, at which point we're again building a set of features that need
to opt-out.
> > To be fair, the manual lists predate the governed features.
>
> 100% agree, however the point of governed features was to simplify this list,
> the point of this patch set is to simplify these lists and yet they still remain,
> more or less untouched, and we will still need to maintain them.
>
> Again I do think that governed features and/or this patchset are better than
> the mess that was there before, but a part of me wants to fully get rid of
> this mess instead of just making it a bit more beautiful.
Oh, I would love to get rid of the mess too, I _completely_ getting rid of the
mess isn't realistic. There are guaranteed to be exceptions to the rule, whether
the rule is "use guest CPUID by default" or "use guest CPUID constrained by KVM
capabilities by default".
I.e. there will always be some amount of manual messiness, the question is which
default behavior would yield the smallest mess. My gut agrees with you, that
defaulting to "guest & KVM" would yield the fewest exceptions. But as above,
I think we're better off doing the switch as an all-or-nothing things (where "all"
means within a single series, not within a single patch).
On Thu, Jan 04, 2024, Sean Christopherson wrote:
> On Thu, Dec 21, 2023, Maxim Levitsky wrote:
> > On Thu, 2023-11-30 at 17:51 -0800, Sean Christopherson wrote:
> > > On Sun, Nov 19, 2023, Maxim Levitsky wrote:
> > > > On Fri, 2023-11-10 at 15:55 -0800, Sean Christopherson wrote:
> > > > Also why not to initialize guest_caps = host_caps & userspace_cpuid?
> > > >
> > > > If this was the default we won't need any guest_cpu_cap_restrict and such,
> > > > instead it will just work.
> > >
> > > Hrm, I definitely like the idea. Unfortunately, unless we do an audit of all
> > > ~120 uses of guest_cpuid_has(), restricting those based on kvm_cpu_caps might
> > > break userspace.
> >
> > 120 uses is not that bad, IMHO it is worth it - we won't need to deal with that
> > in the future.
> >
> > How about a compromise - you change the patches such as it will be possible
> > to remove these cases one by one, and also all new cases will be fully
> > automatic?
>
> Hrm, I'm not necessarily opposed to that, but I don't think we should go partway
> unless we are 100% confident that changing the default to "use guest CPUID ANDed
> with KVM capabilities" is the best end state, *and* that someone will actually
> have the bandwidth to do the work soon-ish so that KVM isn't in a half-baked
> state for months on end. Even then, my preference would definitely be to switch
> everything in one go.
>
> And automatically handling new features would only be feasible for entirely new
> leafs. E.g. X86_FEATURE_RDPID is buried in CPUID.0x7.0x0.ECX, so to automatically
> handle new features KVM would need to set the default guest_caps for all bits
> *except* RDPID, at which point we're again building a set of features that need
> to opt-out.
>
> > > To be fair, the manual lists predate the governed features.
> >
> > 100% agree, however the point of governed features was to simplify this list,
> > the point of this patch set is to simplify these lists and yet they still remain,
> > more or less untouched, and we will still need to maintain them.
> >
> > Again I do think that governed features and/or this patchset are better than
> > the mess that was there before, but a part of me wants to fully get rid of
> > this mess instead of just making it a bit more beautiful.
>
> Oh, I would love to get rid of the mess too, I _completely_ getting rid of the
> mess isn't realistic. There are guaranteed to be exceptions to the rule, whether
> the rule is "use guest CPUID by default" or "use guest CPUID constrained by KVM
> capabilities by default".
>
> I.e. there will always be some amount of manual messiness, the question is which
> default behavior would yield the smallest mess. My gut agrees with you, that
> defaulting to "guest & KVM" would yield the fewest exceptions. But as above,
> I think we're better off doing the switch as an all-or-nothing things (where "all"
> means within a single series, not within a single patch).
Ok, the idea of having vcpu->arch.cpu_caps default to a KVM & GUEST is growing
on me. There's a lurking bug in KVM that in some ways is due to lack of a per-vCPU,
KVM-enforced set of a features. The bug is relatively benign (VMX passes through
CR4.FSGSBASE when it's not supported in the host), and easy to fix (incorporate
KVM-reserved CR4 bits into vcpu->arch.cr4_guest_rsvd_bits), but it really is
something that just shouldn't happen. E.g. KVM's handling of EFER has a similar
lurking problem where __kvm_valid_efer() is unsafe to use without also consulting
efer_reserved_bits.
And after digging a bit more, I think I'm just being overly paranoid. I'm fairly
certain the only exceptions are literally the few that I've called out (RDPID,
MOVBE, and MWAIT (which is only a problem because of a stupid quirk)). I don't
yet have a firm plan on how to deal with the exceptions in a clean way, e.g. I'd
like to somehow have the "opt-out" code share the set of emulated features with
__do_cpuid_func_emulated(). One thought would be to add kvm_emulated_cpu_caps,
which would be *comically* wasteful, but might be worth the 90 bytes.
For v2, what if I post this more or less as-is, with a "convert to KVM & GUEST"
patch thrown in at the end as an RFC? I want to do a lot more testing (and staring)
before committing to the conversion, and sadly I don't have anywhere near enough
cycles to do that right now.