Re: [PATCH] KVM: x86: Do not expose the host value of CPUID.8000001EH

[Paolo Bonzini]

On 10/25/22 18:46, Sean Christopherson wrote:
> On Sat, Oct 22, 2022, Paolo Bonzini wrote:
> > Several fields of CPUID.8000001EH (ExtendedApicId in EAX[31:0],
> > CoreId in EBX[7:0], NodeId in ECX[7:0]) vary on each processor,
> > and it is simply impossible to fit the right values in the
> > KVM_GET_SUPPORTED_CPUID API, in such a way that they can be
> > passed to KVM_SET_CPUID2.
>
> The same is true for 0xb and 0x1f, why delete 0x8000001e but keep those? I agree
> that KVM_GET_SUPPORTED_CPUID can't get this right, but KVM can at least be
> consistent with itself.

0xb and 0x1f are already special cased because EDX is set to the X2APIC 
id.  KVM knows how to do that unlike the NodeId and CoreId.

It would indeed be more consistent with 0xb and 0x1f if KVM set EAX to 
the X2APIC id automatically; on the other hand the value of EAX for 
0x8000001eh would not be consistent with EBX and ECX, which I think is 
worse.

> > The most likely way to avoid confusion in the guest is to zero
> > out all the values.  Userspace will most likely override it
> > anyway if it want to present a specific topology to the guest.
> >
> > This patch essentially reverts commit 382409b4c43e ("kvm: x86: Include
> > CPUID leaf 0x8000001e in kvm's supported CPUID").
>
> Why not do a full revert?

To document the reason why the leaf is hidden; after all it was gotten 
wrong once.

> > Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > ---
> >   arch/x86/kvm/cpuid.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index a0292ba650df..380b71600a9e 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -1193,6 +1193,9 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
> >   		entry->ebx = entry->ecx = entry->edx = 0;
> >   		break;
> >   	case 0x8000001e:
> > +		/* Different on each processor, just hide it.  */
> > +		entry->eax = entry->ebx = entry->ecx = 0;
> > +		entry->edx = 0;
>
> Putting EDX in a separate line is rather weird.

It is, but entry->edx is not different on each processor (it is not 
defined at all, and so it should be zeroed).

Paolo