[PATCH] kvm: x86: Make traced and returned value of kvm_cpuid consistent again

[PATCH] kvm: x86: Make traced and returned value of kvm_cpuid consistent again

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

After 43561123ab37, found is not set correctly in case of leaves 0BH,
1FH, or anything out-of-range. This is currently harmless for the return
value because the only caller evaluating it passes leaf 0x80000008.

However, the trace entry is now misleading due to this inaccuracy. It is
furthermore misleading because it reports the effective function, not
the originally passed one. Fix that as well.

Fixes: 43561123ab37 ("kvm: x86: Improve emulation of CPUID leaves 0BH and 1FH")
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/x86/kvm/cpuid.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index b1c469446b07..79a738f313f8 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -1000,13 +1000,12 @@ static bool cpuid_function_in_range(struct kvm_vcpu *vcpu, u32 function)
 bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
 	       u32 *ecx, u32 *edx, bool check_limit)
 {
-	u32 function = *eax, index = *ecx;
+	u32 orig_function = *eax, function = *eax, index = *ecx;
 	struct kvm_cpuid_entry2 *entry;
 	struct kvm_cpuid_entry2 *max;
 	bool found;

 	entry = kvm_find_cpuid_entry(vcpu, function, index);
-	found = entry;
 	/*
 	 * Intel CPUID semantics treats any query for an out-of-range
 	 * leaf as if the highest basic leaf (i.e. CPUID.0H:EAX) were
@@ -1049,7 +1048,8 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
 			}
 		}
 	}
-	trace_kvm_cpuid(function, *eax, *ebx, *ecx, *edx, found);
+	found = entry;
+	trace_kvm_cpuid(orig_function, *eax, *ebx, *ecx, *edx, found);
 	return found;
 }
 EXPORT_SYMBOL_GPL(kvm_cpuid);
--
2.16.4

Re: [PATCH] kvm: x86: Make traced and returned value of kvm_cpuid consistent again

[Xiaoyao Li]

On 3/1/2020 6:47 PM, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> After 43561123ab37, found is not set correctly in case of leaves 0BH,
> 1FH, or anything out-of-range. This is currently harmless for the return
> value because the only caller evaluating it passes leaf 0x80000008.

Nice catch!

> However, the trace entry is now misleading due to this inaccuracy. It is
> furthermore misleading because it reports the effective function, not
> the originally passed one. Fix that as well.

BTW, the trace lacks subleaf(ECX) info, it's meaning for the the leaf 
does have a subleaf, maybe we'd better add it?

> Fixes: 43561123ab37 ("kvm: x86: Improve emulation of CPUID leaves 0BH and 1FH")
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>   arch/x86/kvm/cpuid.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index b1c469446b07..79a738f313f8 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -1000,13 +1000,12 @@ static bool cpuid_function_in_range(struct kvm_vcpu *vcpu, u32 function)
>   bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
>   	       u32 *ecx, u32 *edx, bool check_limit)
>   {
> -	u32 function = *eax, index = *ecx;
> +	u32 orig_function = *eax, function = *eax, index = *ecx;
>   	struct kvm_cpuid_entry2 *entry;
>   	struct kvm_cpuid_entry2 *max;
>   	bool found;
> 
>   	entry = kvm_find_cpuid_entry(vcpu, function, index);
> -	found = entry;
>   	/*
>   	 * Intel CPUID semantics treats any query for an out-of-range
>   	 * leaf as if the highest basic leaf (i.e. CPUID.0H:EAX) were
> @@ -1049,7 +1048,8 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
>   			}
>   		}
>   	}
> -	trace_kvm_cpuid(function, *eax, *ebx, *ecx, *edx, found);
> +	found = entry;
> +	trace_kvm_cpuid(orig_function, *eax, *ebx, *ecx, *edx, found);
>   	return found;
>   }
>   EXPORT_SYMBOL_GPL(kvm_cpuid);
> --
> 2.16.4
> 

Re: [PATCH] kvm: x86: Make traced and returned value of kvm_cpuid consistent again

[Jan Kiszka]

On 01.03.20 13:25, Xiaoyao Li wrote:
> On 3/1/2020 6:47 PM, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> After 43561123ab37, found is not set correctly in case of leaves 0BH,
>> 1FH, or anything out-of-range. This is currently harmless for the return
>> value because the only caller evaluating it passes leaf 0x80000008.
>
> Nice catch!
>
>> However, the trace entry is now misleading due to this inaccuracy. It is
>> furthermore misleading because it reports the effective function, not
>> the originally passed one. Fix that as well.
>
> BTW, the trace lacks subleaf(ECX) info, it's meaning for the the leaf
> does have a subleaf, maybe we'd better add it?
>

Yes, was also thinking about that. Could be done on top.

Jan

>> Fixes: 43561123ab37 ("kvm: x86: Improve emulation of CPUID leaves 0BH
>> and 1FH")
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>   arch/x86/kvm/cpuid.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index b1c469446b07..79a738f313f8 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -1000,13 +1000,12 @@ static bool cpuid_function_in_range(struct
>> kvm_vcpu *vcpu, u32 function)
>>   bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
>>              u32 *ecx, u32 *edx, bool check_limit)
>>   {
>> -    u32 function = *eax, index = *ecx;
>> +    u32 orig_function = *eax, function = *eax, index = *ecx;
>>       struct kvm_cpuid_entry2 *entry;
>>       struct kvm_cpuid_entry2 *max;
>>       bool found;
>>
>>       entry = kvm_find_cpuid_entry(vcpu, function, index);
>> -    found = entry;
>>       /*
>>        * Intel CPUID semantics treats any query for an out-of-range
>>        * leaf as if the highest basic leaf (i.e. CPUID.0H:EAX) were
>> @@ -1049,7 +1048,8 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax,
>> u32 *ebx,
>>               }
>>           }
>>       }
>> -    trace_kvm_cpuid(function, *eax, *ebx, *ecx, *edx, found);
>> +    found = entry;
>> +    trace_kvm_cpuid(orig_function, *eax, *ebx, *ecx, *edx, found);
>>       return found;
>>   }
>>   EXPORT_SYMBOL_GPL(kvm_cpuid);
>> --
>> 2.16.4
>>
>

Re: [PATCH] kvm: x86: Make traced and returned value of kvm_cpuid consistent again

[Paolo Bonzini]

On 01/03/20 11:47, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> After 43561123ab37, found is not set correctly in case of leaves 0BH,
> 1FH, or anything out-of-range. This is currently harmless for the return
> value because the only caller evaluating it passes leaf 0x80000008.
> 
> However, the trace entry is now misleading due to this inaccuracy. It is
> furthermore misleading because it reports the effective function, not
> the originally passed one. Fix that as well.
> 
> Fixes: 43561123ab37 ("kvm: x86: Improve emulation of CPUID leaves 0BH and 1FH")
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Queued, thanks.

Paolo

> ---
>  arch/x86/kvm/cpuid.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index b1c469446b07..79a738f313f8 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -1000,13 +1000,12 @@ static bool cpuid_function_in_range(struct kvm_vcpu *vcpu, u32 function)
>  bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
>  	       u32 *ecx, u32 *edx, bool check_limit)
>  {
> -	u32 function = *eax, index = *ecx;
> +	u32 orig_function = *eax, function = *eax, index = *ecx;
>  	struct kvm_cpuid_entry2 *entry;
>  	struct kvm_cpuid_entry2 *max;
>  	bool found;
> 
>  	entry = kvm_find_cpuid_entry(vcpu, function, index);
> -	found = entry;
>  	/*
>  	 * Intel CPUID semantics treats any query for an out-of-range
>  	 * leaf as if the highest basic leaf (i.e. CPUID.0H:EAX) were
> @@ -1049,7 +1048,8 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
>  			}
>  		}
>  	}
> -	trace_kvm_cpuid(function, *eax, *ebx, *ecx, *edx, found);
> +	found = entry;
> +	trace_kvm_cpuid(orig_function, *eax, *ebx, *ecx, *edx, found);
>  	return found;
>  }
>  EXPORT_SYMBOL_GPL(kvm_cpuid);
> --
> 2.16.4
> 

Re: [PATCH] kvm: x86: Make traced and returned value of kvm_cpuid consistent again

[Sean Christopherson]

On Mon, Mar 02, 2020 at 05:11:57PM +0100, Paolo Bonzini wrote:
> Queued, thanks.

Too fast, too fast!

On Sun, Mar 01, 2020 at 11:47:20AM +0100, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> After 43561123ab37, found is not set correctly in case of leaves 0BH,
> 1FH, or anything out-of-range.

No, found is set correctly, kvm_cpuid() should return true if and only if
an exact match for the requested function is found, and that's the original
tracing behavior of "found" (pre-43561123ab37).

> This is currently harmless for the return value because the only caller
> evaluating it passes leaf 0x80000008.

No, it's 100% correct.  Well, technically it's irrelevant because the only
caller, check_cr_write(), passes %false for check_limit, i.e. found will be
true if and only if entry 0x80000008 exists.  But, in a purely hypothetical
scenario where the emulator passed check_limit=%true, the intent of "found"
is to report that the exact leaf was found, not if some random entry was
found.

> However, the trace entry is now misleading due to this inaccuracy. It is
> furthermore misleading because it reports the effective function, not
> the originally passed one. Fix that as well.
>
> Fixes: 43561123ab37 ("kvm: x86: Improve emulation of CPUID leaves 0BH and 1FH")
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  arch/x86/kvm/cpuid.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index b1c469446b07..79a738f313f8 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -1000,13 +1000,12 @@ static bool cpuid_function_in_range(struct kvm_vcpu *vcpu, u32 function)
>  bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
>              u32 *ecx, u32 *edx, bool check_limit)
>  {
> -     u32 function = *eax, index = *ecx;
> +     u32 orig_function = *eax, function = *eax, index = *ecx;
>       struct kvm_cpuid_entry2 *entry;
>       struct kvm_cpuid_entry2 *max;

Rather than add another variable, this can be cleaned up to remove "max".
cpuid_function_in_range() also has a bug.  I've got patches, in the process
of whipping up a unit test.

Re: [PATCH] kvm: x86: Make traced and returned value of kvm_cpuid consistent again

[Jan Kiszka]

On 02.03.20 17:38, Sean Christopherson wrote:
> On Mon, Mar 02, 2020 at 05:11:57PM +0100, Paolo Bonzini wrote:
>> Queued, thanks.
> 
> Too fast, too fast!
> 
> On Sun, Mar 01, 2020 at 11:47:20AM +0100, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> After 43561123ab37, found is not set correctly in case of leaves 0BH,
>> 1FH, or anything out-of-range.
> 
> No, found is set correctly, kvm_cpuid() should return true if and only if
> an exact match for the requested function is found, and that's the original
> tracing behavior of "found" (pre-43561123ab37).
> 
>> This is currently harmless for the return value because the only caller
>> evaluating it passes leaf 0x80000008.
> 
> No, it's 100% correct.  Well, technically it's irrelevant because the only
> caller, check_cr_write(), passes %false for check_limit, i.e. found will be
> true if and only if entry 0x80000008 exists.  But, in a purely hypothetical
> scenario where the emulator passed check_limit=%true, the intent of "found"
> is to report that the exact leaf was found, not if some random entry was
> found.

Nicely non-intuitive semantics. Should definitely be documented.

And then it's questionable to me what value tracing such a return code 
has. At the bare minimum, "found" should be renamed to something like 
"exact_match".

> 
>> However, the trace entry is now misleading due to this inaccuracy. It is
>> furthermore misleading because it reports the effective function, not
>> the originally passed one. Fix that as well.
>>
>> Fixes: 43561123ab37 ("kvm: x86: Improve emulation of CPUID leaves 0BH and 1FH")
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>   arch/x86/kvm/cpuid.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index b1c469446b07..79a738f313f8 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -1000,13 +1000,12 @@ static bool cpuid_function_in_range(struct kvm_vcpu *vcpu, u32 function)
>>   bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
>>               u32 *ecx, u32 *edx, bool check_limit)
>>   {
>> -     u32 function = *eax, index = *ecx;
>> +     u32 orig_function = *eax, function = *eax, index = *ecx;
>>        struct kvm_cpuid_entry2 *entry;
>>        struct kvm_cpuid_entry2 *max;
> 
> Rather than add another variable, this can be cleaned up to remove "max".
> cpuid_function_in_range() also has a bug.  I've got patches, in the process
> of whipping up a unit test.
> 

Fine with me.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH] kvm: x86: Make traced and returned value of kvm_cpuid consistent again

[Sean Christopherson]

On Mon, Mar 02, 2020 at 05:46:01PM +0100, Jan Kiszka wrote:
> On 02.03.20 17:38, Sean Christopherson wrote:
> >On Mon, Mar 02, 2020 at 05:11:57PM +0100, Paolo Bonzini wrote:
> >>Queued, thanks.
> >
> >Too fast, too fast!
> >
> >On Sun, Mar 01, 2020 at 11:47:20AM +0100, Jan Kiszka wrote:
> >>From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >>After 43561123ab37, found is not set correctly in case of leaves 0BH,
> >>1FH, or anything out-of-range.
> >
> >No, found is set correctly, kvm_cpuid() should return true if and only if
> >an exact match for the requested function is found, and that's the original
> >tracing behavior of "found" (pre-43561123ab37).
> >
> >>This is currently harmless for the return value because the only caller
> >>evaluating it passes leaf 0x80000008.
> >
> >No, it's 100% correct.  Well, technically it's irrelevant because the only
> >caller, check_cr_write(), passes %false for check_limit, i.e. found will be
> >true if and only if entry 0x80000008 exists.  But, in a purely hypothetical
> >scenario where the emulator passed check_limit=%true, the intent of "found"
> >is to report that the exact leaf was found, not if some random entry was
> >found.
> 
> Nicely non-intuitive semantics. Should definitely be documented.
> 
> And then it's questionable to me what value tracing such a return code has.

There's value in knowing the the output came from the actual requested leaf
as opposed to the max basic leaf, e.g. if the guest is seeing weird CPUID
output in the guest, knowing whether it was explicitly configured by the
userspace VMM versus coming from KVM's emulation of Intel's wonderful CPUID
behavior.

> At the bare minimum, "found" should be renamed to something like
> "exact_match".

I can do something along these lines.  kvm_cpuid() really doesn't need to
be returning a value, i.e. the emulator shouldn't be manually calculating
maxphyaddr anyways.