lkml.org 
[lkml]   [2019]   [Apr]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH] KVM: x86: Add Intel CPUID.1F cpuid emulation support
From
Date
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@linux.intel.com>
>> ---
>> 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
>>
>

\
 
 \ /
  Last update: 2019-04-25 04:58    [W:0.107 / U:1.400 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site