Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] KVM: x86: Add Intel CPUID.1F cpuid emulation support
To:     Xiaoyao Li <xiaoyao.li@intel.com>,
        Sean Christopherson <sean.j.christopherson@intel.com>
Cc:     kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Len Brown <lenb@kernel.org>, linux-kernel@vger.kernel.org
References: <1555915234-2536-1-git-send-email-like.xu@linux.intel.com>
 <20190424143238.GB18442@linux.intel.com>
 <bcc3a8ea-1fd0-124c-c472-7c096e0f8014@linux.intel.com>
 <c5d7dd8a51c606f38314ef4c7bb0702caeecbe7e.camel@intel.com>
From:   Like Xu <like.xu@linux.intel.com>
Organization: Intel OTC
Message-ID: <30857e40-05b6-0f4c-d07c-919de08c90ac@linux.intel.com>
Date:   Thu, 25 Apr 2019 14:02:39 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.6.1
MIME-Version: 1.0
In-Reply-To: <c5d7dd8a51c606f38314ef4c7bb0702caeecbe7e.camel@intel.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 2019/4/25 12:18, Xiaoyao Li wrote:
> On Thu, 2019-04-25 at 10:58 +0800, Like Xu wrote:
>> 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;
> 
> As Sean pointed out, you cannot rely on the output of cpuid.1f to indicate the
> existence of leaf 1f. If maximum basic leaf supported is smaller than 1f, the
> data returned by cpuid_ecx(0x1f) is the actual highest basic information leaf of
> the hardware.

I don't think so.

> So using "entry->eax >= 0x1f" from cpuid.0H is and only is the right way to
> check the existence of leaf 1f.
> 
> We can simply use (cpuid_ecx(0x1f) & 0x0000ff00) to avoid the unnecessory
> shifting operation.

I borrowed this "unnecessory" shifting operation from host 
check_extended_topology_leaf() and we may do better on this.

> Besides, the problem of simply using cpuid_exc(0x1f) in Sean's codes is that we
> cannot assmue the reserved bits 31:16 of ECX is always 0 for the future
> generation.

It's true cause the statement in public spec is not "Reserved = 0" but 
"Bits 31 - 16: Reserved".

> 
> In my opinion, Sean's codes is OK and much simple and clear.

If the host cpuid.0.eax is greater than 0x1f but actually it doesn't
have multi-chip packaging technology and we may want to expose 
entry->eax to some value smaller than 0x1f but greater than 0x14, much 
effort needs to apply on Sean's code.

My improvement is good to overwrite cpuid.0.eax in future usage
from the perspective of kvm feature setting not just from value check.

> All need to do is using (cpuid_ecx(0x1f) & 0x0000ff00) to verify the leaf.1f is
> valid.
> 
> Thanks,
> -Xiaoyao
>> 	// ... 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
>>>>
>>
>>
> 
>