| Message ID | dd33df29-2c17-2dc8-cb8f-56686cd583ad@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kvm: x86: Make traced and returned value of kvm_cpuid consistent again | expand |
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 >
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 >> >
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 >
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.
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
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.
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);