Re: [PATCH] KVM: x86: Add Intel CPUID.1F cpuid emulation support

[Like Xu]

On 2019/4/24 1:44, Sean Christopherson wrote:
> On Tue, Apr 23, 2019 at 11:23:59AM +0800, Like Xu wrote:
> > On 2019/4/23 2:35, Sean Christopherson wrote:
> > > >   #define F(x) bit(X86_FEATURE_##x)
> > > >   int kvm_update_cpuid(struct kvm_vcpu *vcpu)
> > > > @@ -426,6 +436,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
> > > >   	switch (function) {
> > > >   	case 0:
> > > >   		entry->eax = min(entry->eax, (u32)(f_intel_pt ? 0x14 : 0xd));
> > > > +		entry->eax = kvm_supported_intel_mcp() ? 0x1f : entry->eax;
> > >
> > > This all seems unnecessary.  And by 'all', I mean the existing Intel PT
> > > and XSAVE leaf checks, as well as the new mcp check.  entry->eax comes
> > > directly from hardware, and unless I missed something, PT and XSAVE are
> > > only exposed to the guest when they're supported in hardware.  In other
> > > words, KVM will never need to adjust entry->eax to expose PT or XSAVE.
> >
> > We call this function for both case KVM_GET_SUPPORTED_CPUID and
> > KVM_GET_EMULATED_CPUID although kvm user could reconfig them via
> > KVM_SET_CPUID* path.
>
> Not that it matters, but __do_cpuid_ent() is only used for the non-emulated
> case, KVM_GET_EMULATED_CPUID goes to __do_cpuid_ent_emulated().

It's true and I have to mention we have two scenarios to get vCPUID:

1. For kvm_dev, we have KVM_GET_EMULATED_CPUID for 
kvm_dev_ioctl_get_cpuid; (we're talking about this)

2. For kvm_vcpu,we have KVM_GET_CPUID2 for kvm_vcpu_ioctl_get_cpuid2;

>   
> > > The original min() check was added by commit 0771671749b5 ("KVM: Enhance
> > > guest cpuid management"), which doesn't provide any explicit information
> > > on why KVM does min() in the first place.
> >
> > Exposing cpuid.0.eax in a blind way (with host hardware support)
> > is not a good practice for guest migration and improves compatibility
> > requirements.
>
> Right, but isn't the f_intel_pt check for example completely irrelevant?
> f_intel_pt is true if and only if hardware supports PT, i.e. CPUID.0.EAX
> and thus entry->eax will already be >=0x14.

The f_intel_pt check is not only about hardware supports check but also 
module_param (pt_mode) supports check.

So the case is the host does have PT support which means (host 
CPUID.0.EAX already be >=0x14 for Intel CPUs) but kvm doesn't want 
advertise it and thus the min() operation is needed.

> I don't fully understand whether or not KVM needs to raise the minimum to
> 0xb regardless of h/w XSAVE support, but it's likely irrelevant in the end.
>
> Anyways, back to 0x1f, kvm_supported_intel_mcp() returns true if and only
> if hardware's CPUID.0.EAX >= 0x1f, 

According to latest SDM, the max hardware CPUID.0.EAX is 0x1f and BIOS 
would expose 0x1f only for multi-chip packaging CPUs (at least for now).

> i.e. adjusting entry->eax is always a
> nop.  So if KVM wants to advertise leaf 0x1f only when it's supported in
> hardware then adjusting entry->eax is unnecessary, and if KVM wants to
> unconditionally advertise 0x1f then adjusting entry->eax should also be
> done unconditionally.

It we have no check on kvm_supported_intel_mcp() in legacy code,
CPUID.0.EAX would be min() and thus less than 0x1f which means the 
cpuid.1f info is not exposed.

I know your point is to avoid min() totally (I thought so at the time) 
and I have pointed out it's necessary for kvm features setting.

If KVM wants to unconditionally advertise 0x1f (in EMULATED way),
kvm needs cover other side effects and this patch only advertises 0x1f
when hardware has it.

It's very common that guest wants to set 0x1f regardless of h/w support
and this is another story.

> > > Given that the original code
> > > was "entry->eax = min(entry->eax, (u32)0xb);", my *guess* is that the
> > > idea was to always report "Extended Topology Enumeration Leaf" as
> > > supported so that userspace can enumerate the VM's topology to the guest
> > > even when hardware itself doesn't do so.
> >
> > If the host cpu mode is too antiquated to support 0xb, it wouldn't report
> > 0xb for sure. The host cpuid.0.eax has been over 0xb for a long time and
> > reached 0x1f in the latest SDM.
> >
> > AFAICT, the original code keeps minimum cpuid.0.eax out of features guest
> > just used or at least it claimed to use.
> >
> > > Assuming we want to allow userspace to use "V2 Extended Topology
> > > Enumeration Leaf" regardless of hardware support, then this can simply be:
> > >
> > >    entry->eax = min(entry->eax, (u32)0x1f);
> > >
> > > Or am I completely missing something?