Re: [PATCH 1/6] KVM: x86: Fix tracing of CPUID.function when function is out-of-range

From: Xiaoyao Li <xiaoyao.li@intel.com>
To: Sean Christopherson <sean.j.christopherson@intel.com>,
	Paolo Bonzini <pbonzini@redhat.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Jim Mattson <jmattson@google.com>, Joerg Roedel <joro@8bytes.org>,
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	Jan Kiszka <jan.kiszka@siemens.com>
Subject: Re: [PATCH 1/6] KVM: x86: Fix tracing of CPUID.function when function is out-of-range
Date: Tue, 3 Mar 2020 10:50:03 +0800	[thread overview]
Message-ID: <6b41fc5c-f7f4-b20d-cfb5-95bf13cc7534@intel.com> (raw)
In-Reply-To: <20200302195736.24777-2-sean.j.christopherson@intel.com>

On 3/3/2020 3:57 AM, Sean Christopherson wrote:
> Rework kvm_cpuid() to query entry->function when adjusting the output
> values so that the original function (in the aptly named "function") is
> preserved for tracing.  This fixes a bug where trace_kvm_cpuid() will
> trace the max function for a range instead of the requested function if
> the requested function is out-of-range and an entry for the max function
> exists.
> 
> Fixes: 43561123ab37 ("kvm: x86: Improve emulation of CPUID leaves 0BH and 1FH")
> Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
> Cc: Jim Mattson <jmattson@google.com>
> Cc: Xiaoyao Li <xiaoyao.li@intel.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>   arch/x86/kvm/cpuid.c | 15 +++++++--------
>   1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index b1c469446b07..6be012937eba 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -997,12 +997,12 @@ static bool cpuid_function_in_range(struct kvm_vcpu *vcpu, u32 function)
>   	return max && function <= max->eax;
>   }
>   
> +/* Returns true if the requested leaf/function exists in guest CPUID. */
>   bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
>   	       u32 *ecx, u32 *edx, bool check_limit)
>   {
> -	u32 function = *eax, index = *ecx;
> +	const u32 function = *eax, index = *ecx;
>   	struct kvm_cpuid_entry2 *entry;
> -	struct kvm_cpuid_entry2 *max;
>   	bool found;
>   
>   	entry = kvm_find_cpuid_entry(vcpu, function, index);
> @@ -1015,18 +1015,17 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
>   	 */
>   	if (!entry && check_limit && !guest_cpuid_is_amd(vcpu) &&
>   	    !cpuid_function_in_range(vcpu, function)) {
> -		max = kvm_find_cpuid_entry(vcpu, 0, 0);
> -		if (max) {
> -			function = max->eax;
> -			entry = kvm_find_cpuid_entry(vcpu, function, index);
> -		}
> +		entry = kvm_find_cpuid_entry(vcpu, 0, 0);
> +		if (entry)
> +			entry = kvm_find_cpuid_entry(vcpu, entry->eax, index);

There is a problem.

when queried leaf is out of range on Intel CPU, it returns the maximum 
basic leaf, and any dependence on input ECX (i.e., subleaf) value in the 
basic leaf is honored. As disclaimed in SDM of CPUID instruction.

The ECX should be honored if and only the leaf has a significant index.
If the leaf doesn't has a significant index, it just ignores the EDX 
input in bare metal.

So it should be something like:

if (!entry && check_limit && !guest_cpuid_is_amd(vcpu) &&
	!cpuid_function_in_range(vcpu, function)) {
	entry = kvm_find_cpuid_entry(vcpu, 0, 0);
	if (entry) {
		entry = kvm_find_cpuid_entry(vcpu, entry->eax, 0);
		if (entry &&
		    entry->flags & KVM_CPUID_FLAG_SIGNIFCANT_INDEX ) {
			entry = kvm_find_cpuid_entry(vcpu, entry->eax,
						     index);
		}
	}
}

>   	}
>   	if (entry) {
>   		*eax = entry->eax;
>   		*ebx = entry->ebx;
>   		*ecx = entry->ecx;
>   		*edx = entry->edx;
> -		if (function == 7 && index == 0) {
> +
> +		if (entry->function == 7 && index == 0) {
>   			u64 data;
>   		        if (!__kvm_get_msr(vcpu, MSR_IA32_TSX_CTRL, &data, true) &&
>   			    (data & TSX_CTRL_CPUID_CLEAR))
> 