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

[Like Xu]

On 2019/4/24 22:32, Sean Christopherson wrote:
> Now that I understand how min() works...
>
> On Mon, Apr 22, 2019 at 02:40:34PM +0800, Like Xu wrote:
> > Expose Intel V2 Extended Topology Enumeration Leaf to guest only when
> > host system has multiple software-visible die within each package.
> >
> > Signed-off-by: Like Xu <like.xu@xxxxxxxxxxxxxxx>
> > ---
> >   arch/x86/kvm/cpuid.c | 13 +++++++++++++
> >   1 file changed, 13 insertions(+)
> >
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index fd39516..9fc14f2 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -65,6 +65,16 @@ u64 kvm_supported_xcr0(void)
> >   	return xcr0;
> >   }
> >   
> > +/* We need to check if the host cpu has multi-chip packaging technology. */
> > +static bool kvm_supported_intel_mcp(void)
> > +{
> > +	u32 eax, ignored;
> > +
> > +	cpuid_count(0x1f, 0, &eax, &ignored, &ignored, &ignored);
>
> This is broken because of how CPUID works for unsupported input leafs:
>
>    If a value entered for CPUID.EAX is higher than the maximum input value
>    for basic or extended function for that processor then the data for the
>    highest basic information leaf is returned.
>
> For example, my system with a max basic leaf of 0x16 returns 0x00000e74
> for CPUID.1F.EAX.

You're right and the cpuid.1f.eax check is unreliable after I checked a 
few machines.

> > +
> > +	return boot_cpu_data.x86_vendor == X86_VENDOR_INTEL && (eax != 0);
>
> Checking 'eax != 0' is broken as it will be '0' when SMT is disabled.  ecx
> is the obvious choice since bits 15:8 are guaranteed to be non-zero when
> the leaf is valid.

I agree with this and ecx[15:8] makes sense.

> I think we can skip the vendor check.  AFAIK, CPUID.1F isn't used by AMD,
> and since AMD and Intel try to maintain a semblance of CPUID compatibility
> it seems more likely that AMD/Hygon would implement CPUID.1F as-is rather
> than repurpose it to mean something else entirely.

If it's true, let's skip the vendor check.

// I have to mention that AMD already has MCP CPUs.

> > +}
> > +
> >   #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;
>
> If we put everything together, I think the code can be reduced to:
>
> 		/* comment about multi-chip leaf... */
> 		if (entry->eax >= 0x1f && cpuid_ecx(0x1f))
> 			entry->eax = 0x1f;
> 		else
> 			entry->eax = min(entry->eax,
> 					 (u32)(f_intel_pt ? 0x14 : 0xd));

Based on:

	ECX Bits 07 - 00: Level number. Same value in ECX input.
	Bits 15 - 08: Level type.
	Bits 31 - 16: Reserved.

how about using an increasing order:

	entry->eax = min(entry->eax, (u32)(f_intel_pt ? 0x14 : 0xd));

	// ... more checks when eax is between 0x14 and 0x1f if any

	/* Check if the host cpu has multi-chip packaging technology.*/
	if (((cpuid_ecx(0x1f) >> 8) & 0xff) != 0)
		entry->eax = 0x1f;

	// ... more checks when eax greater than 0x1f if any

are we OK with it?

> >   		break;
> >   	case 1:
> >   		entry->edx &= kvm_cpuid_1_edx_x86_features;
> > @@ -544,6 +555,8 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
> >   		entry->edx = edx.full;
> >   		break;
> >   	}
> > +	/* function 0x1f has additional index. */
> > +	case 0x1f:
> >   	/* function 0xb has additional index. */
> >   	case 0xb: {
> >   		int i, level_type;
> > --
> > 1.8.3.1