On Mon, 2024-07-08 at 17:10 -0700, Sean Christopherson wrote:
> On Thu, Jul 04, 2024, Maxim Levitsky wrote:
> > On Fri, 2024-05-17 at 10:39 -0700, Sean Christopherson wrote:
> > > @@ -421,6 +423,7 @@ void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> > > */
> > > for (i = 0; i < NR_KVM_CPU_CAPS; i++) {
> > > const struct cpuid_reg cpuid = reverse_cpuid[i];
> > > + struct kvm_cpuid_entry2 emulated;
> > >
> > > if (!cpuid.function)
> > > continue;
> > > @@ -429,7 +432,16 @@ void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> > > if (!entry)
> > > continue;
> > >
> > > - vcpu->arch.cpu_caps[i] = cpuid_get_reg_unsafe(entry, cpuid.reg);
> > > + cpuid_func_emulated(&emulated, cpuid.function);
> > > +
> > > + /*
> > > + * A vCPU has a feature if it's supported by KVM and is enabled
> > > + * in guest CPUID. Note, this includes features that are
> > > + * supported by KVM but aren't advertised to userspace!
> > > + */
> > > + vcpu->arch.cpu_caps[i] = kvm_cpu_caps[i] | kvm_vmm_cpu_caps[i] |
> > > + cpuid_get_reg_unsafe(&emulated, cpuid.reg);
> > > + vcpu->arch.cpu_caps[i] &= cpuid_get_reg_unsafe(entry, cpuid.reg);
> >
> > Hi,
> >
> > I have an idea. What if we get rid of kvm_vmm_cpu_caps, and instead advertise the
> > MWAIT in KVM_GET_EMULATED_CPUID?
> >
> > MWAIT is sort of emulated as NOP after all, plus features in KVM_GET_EMULATED_CPUID are
> > sort of 'emulated inefficiently' and you can say that NOP is an inefficient emulation
> > of MWAIT sort of.
>
> Heh, sort of indeed. I really don't want to advertise MWAIT to userspace in any
> capacity beyond KVM_CAP_X86_DISABLE_EXITS, because advertising MWAIT to VMs when
> MONITOR/MWAIT exiting is enabled is actively harmful, to both host and guest.
Assuming that the only purpose of the KVM_GET_EMULATED_CPUID is to allow the guest
to use a feature if it really insists, there should be no harm, but yes, I understand
your concert here.
>
> KVM also doesn't emulate them on #UD, unlike MOVBE, which would make the API even
> more confusing than it already is.
This is even bigger justification for not doing this.
>
> > It just feels to me that kvm_vmm_cpu_caps, is somewhat an overkill, and its name is
> > somewhat confusing.
>
> Yeah, I don't love it either, but trying to handle MWAIT as a one-off was even
> uglier. One option would be to piggyback cpuid_func_emulated(), but add a param
> to have it fill MWAIT only for KVM's internal purposes. That'd essentially be
> the same as a one-off in kvm_vcpu_after_set_cpuid(), but less ugly.
>
> I'd say it comes down to whether or not we expect to have more features that KVM
> "supports", but doesn't advertise to userspace. If we do, then I think adding
> VMM_F() is the way to go. If we expect MWAIT to be the only feature that gets
> this treatment, then I'm ok if we bastardize cpuid_func_emulated().
>
> And I think/hope that MWAIT will be a one-off. Emulating it as a nop was a
> mistake and has since been quirked, and I like to think we (eventually) learn
> from our mistakes.
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 0e64a6332052..dbc3f6ce9203 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -448,7 +448,7 @@ void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> if (!entry)
> continue;
>
> - cpuid_func_emulated(&emulated, cpuid.function);
> + cpuid_func_emulated(&emulated, cpuid.function, false);
>
> /*
> * A vCPU has a feature if it's supported by KVM and is enabled
> @@ -1034,7 +1034,8 @@ static struct kvm_cpuid_entry2 *do_host_cpuid(struct kvm_cpuid_array *array,
> return entry;
> }
>
> -static int cpuid_func_emulated(struct kvm_cpuid_entry2 *entry, u32 func)
> +static int cpuid_func_emulated(struct kvm_cpuid_entry2 *entry, u32 func,
> + bool only_advertised)
I'll say, lets call this boolean, 'include_partially_emulated',
(basically features that kvm emulates but only partially,
and thus doesn't advertise, aka mwait)
and then it doesn't look that bad, assuming that comes with a comment.
> {
> memset(entry, 0, sizeof(*entry));
>
> @@ -1048,6 +1049,9 @@ static int cpuid_func_emulated(struct kvm_cpuid_entry2 *entry, u32 func)
> return 1;
> case 1:
> entry->ecx = F(MOVBE);
> + /* comment goes here. */
> + if (!only_advertised)
And here
if(include_partially_emulated) ...
It sort of even self-documents nature of mwait emulation.
> + entry->ecx |= F(MWAIT);
> return 1;
> case 7:
> entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
> @@ -1065,7 +1069,7 @@ static int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 func)
> if (array->nent >= array->maxnent)
> return -E2BIG;
>
> - array->nent += cpuid_func_emulated(&array->entries[array->nent], func);
> + array->nent += cpuid_func_emulated(&array->entries[array->nent], func, true);
> return 0;
> }
>
Best regards,
Maxim Levitsky
