diff mbox series

KVM: x86: Do not expose the host value of CPUID.8000001EH

Message ID 20221022082643.1725875-1-pbonzini@redhat.com
State New
Headers
Series KVM: x86: Do not expose the host value of CPUID.8000001EH |

Commit Message

Paolo Bonzini Oct. 22, 2022, 8:26 a.m. UTC 
  Several fields of CPUID.8000001EH (ExtendedApicId in EAX[31:0],
CoreId in EBX[7:0], NodeId in ECX[7:0]) vary on each processor,
and it is simply impossible to fit the right values in the
KVM_GET_SUPPORTED_CPUID API, in such a way that they can be
passed to KVM_SET_CPUID2.

The most likely way to avoid confusion in the guest is to zero
out all the values.  Userspace will most likely override it
anyway if it want to present a specific topology to the guest.

This patch essentially reverts commit 382409b4c43e ("kvm: x86: Include
CPUID leaf 0x8000001e in kvm's supported CPUID").

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/cpuid.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Xiaoyao Li Oct. 24, 2022, 1:06 p.m. UTC | #1 
On 10/22/2022 4:26 PM, Paolo Bonzini wrote:
> Several fields of CPUID.8000001EH (ExtendedApicId in EAX[31:0],
> CoreId in EBX[7:0], NodeId in ECX[7:0]) vary on each processor,
> and it is simply impossible to fit the right values in the
> KVM_GET_SUPPORTED_CPUID API, in such a way that they can be
> passed to KVM_SET_CPUID2.
> 
> The most likely way to avoid confusion in the guest is to zero
> out all the values.  Userspace will most likely override it
> anyway if it want to present a specific topology to the guest.
> 
> This patch essentially reverts commit 382409b4c43e ("kvm: x86: Include
> CPUID leaf 0x8000001e in kvm's supported CPUID").

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   arch/x86/kvm/cpuid.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index a0292ba650df..380b71600a9e 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -1193,6 +1193,9 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>   		entry->ebx = entry->ecx = entry->edx = 0;
>   		break;
>   	case 0x8000001e:
> +		/* Different on each processor, just hide it.  */
> +		entry->eax = entry->ebx = entry->ecx = 0;
> +		entry->edx = 0;
>   		break;
>   	case 0x8000001F:
>   		if (!kvm_cpu_cap_has(X86_FEATURE_SEV)) {
Sean Christopherson Oct. 25, 2022, 4:46 p.m. UTC | #2 
On Sat, Oct 22, 2022, Paolo Bonzini wrote:
> Several fields of CPUID.8000001EH (ExtendedApicId in EAX[31:0],
> CoreId in EBX[7:0], NodeId in ECX[7:0]) vary on each processor,
> and it is simply impossible to fit the right values in the
> KVM_GET_SUPPORTED_CPUID API, in such a way that they can be
> passed to KVM_SET_CPUID2.

The same is true for 0xb and 0x1f, why delete 0x8000001e but keep those? I agree
that KVM_GET_SUPPORTED_CPUID can't get this right, but KVM can at least be
consistent with itself.

> The most likely way to avoid confusion in the guest is to zero
> out all the values.  Userspace will most likely override it
> anyway if it want to present a specific topology to the guest.
> 
> This patch essentially reverts commit 382409b4c43e ("kvm: x86: Include
> CPUID leaf 0x8000001e in kvm's supported CPUID").

Why not do a full revert?

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/cpuid.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index a0292ba650df..380b71600a9e 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -1193,6 +1193,9 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>  		entry->ebx = entry->ecx = entry->edx = 0;
>  		break;
>  	case 0x8000001e:
> +		/* Different on each processor, just hide it.  */
> +		entry->eax = entry->ebx = entry->ecx = 0;
> +		entry->edx = 0;

Putting EDX in a separate line is rather weird.
Paolo Bonzini Oct. 25, 2022, 9:09 p.m. UTC | #3 
On 10/25/22 18:46, Sean Christopherson wrote:
> On Sat, Oct 22, 2022, Paolo Bonzini wrote:
>> Several fields of CPUID.8000001EH (ExtendedApicId in EAX[31:0],
>> CoreId in EBX[7:0], NodeId in ECX[7:0]) vary on each processor,
>> and it is simply impossible to fit the right values in the
>> KVM_GET_SUPPORTED_CPUID API, in such a way that they can be
>> passed to KVM_SET_CPUID2.
> 
> The same is true for 0xb and 0x1f, why delete 0x8000001e but keep those? I agree
> that KVM_GET_SUPPORTED_CPUID can't get this right, but KVM can at least be
> consistent with itself.

0xb and 0x1f are already special cased because EDX is set to the X2APIC 
id.  KVM knows how to do that unlike the NodeId and CoreId.

It would indeed be more consistent with 0xb and 0x1f if KVM set EAX to 
the X2APIC id automatically; on the other hand the value of EAX for 
0x8000001eh would not be consistent with EBX and ECX, which I think is 
worse.

>> The most likely way to avoid confusion in the guest is to zero
>> out all the values.  Userspace will most likely override it
>> anyway if it want to present a specific topology to the guest.
>>
>> This patch essentially reverts commit 382409b4c43e ("kvm: x86: Include
>> CPUID leaf 0x8000001e in kvm's supported CPUID").
> 
> Why not do a full revert?

To document the reason why the leaf is hidden; after all it was gotten 
wrong once.

>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   arch/x86/kvm/cpuid.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index a0292ba650df..380b71600a9e 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -1193,6 +1193,9 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>>   		entry->ebx = entry->ecx = entry->edx = 0;
>>   		break;
>>   	case 0x8000001e:
>> +		/* Different on each processor, just hide it.  */
>> +		entry->eax = entry->ebx = entry->ecx = 0;
>> +		entry->edx = 0;
> 
> Putting EDX in a separate line is rather weird.

It is, but entry->edx is not different on each processor (it is not 
defined at all, and so it should be zeroed).

Paolo
Sean Christopherson Oct. 25, 2022, 9:25 p.m. UTC | #4 
On Tue, Oct 25, 2022, Paolo Bonzini wrote:
> On 10/25/22 18:46, Sean Christopherson wrote:
> > On Sat, Oct 22, 2022, Paolo Bonzini wrote:
> > > Several fields of CPUID.8000001EH (ExtendedApicId in EAX[31:0],
> > > CoreId in EBX[7:0], NodeId in ECX[7:0]) vary on each processor,
> > > and it is simply impossible to fit the right values in the
> > > KVM_GET_SUPPORTED_CPUID API, in such a way that they can be
> > > passed to KVM_SET_CPUID2.
> > 
> > The same is true for 0xb and 0x1f, why delete 0x8000001e but keep those? I agree
> > that KVM_GET_SUPPORTED_CPUID can't get this right, but KVM can at least be
> > consistent with itself.
> 
> 0xb and 0x1f are already special cased because EDX is set to the X2APIC id.
> KVM knows how to do that unlike the NodeId and CoreId.

But KVM doesn't properly support 0xB/0x1F.  E.g. if usersepace regurgitates
KVM_GET_SUPPORTED_CPUID back into KVM_SET_CPUID2, all vCPUs will observe the same
x2APIC ID in EDX, and it will be a host x2APIC ID to boot.

KVM only handles the where userspace provides 0xB.1 (or 0x1F.1), the guest performs
CPUID with ECX>1, _and_ userspace doesn't provide the exact CPUID entry. 

I suppose one could argue that KVM needs to communicate to userspace that KVM
emulates the edge case behavior of CPUID 0xB and 0x1F, but I would argue that KVM
communicates that by announcing a max basic leaf >= 0xB/0x1F.
Paolo Bonzini Oct. 25, 2022, 9:31 p.m. UTC | #5 
On 10/25/22 23:25, Sean Christopherson wrote:
>> 0xb and 0x1f are already special cased because EDX is set to the X2APIC id.
>> KVM knows how to do that unlike the NodeId and CoreId.
> But KVM doesn't properly support 0xB/0x1F.  E.g. if usersepace regurgitates
> KVM_GET_SUPPORTED_CPUID back into KVM_SET_CPUID2, all vCPUs will observe the same
> x2APIC ID in EDX, and it will be a host x2APIC ID to boot.
> 
> KVM only handles the where userspace provides 0xB.1 (or 0x1F.1), the guest performs
> CPUID with ECX>1,_and_  userspace doesn't provide the exact CPUID entry.

Ah, you're right - I confused it with the "undefined leaves" behavior here:

         } else {
                 *eax = *ebx = *ecx = *edx = 0;
                 /*
                  * When leaf 0BH or 1FH is defined, CL is pass-through
                  * and EDX is always the x2APIC ID, even for undefined
                  * subleaves. Index 1 will exist iff the leaf is
                  * implemented, so we pass through CL iff leaf 1
                  * exists. EDX can be copied from any existing index.
                  */
                 if (function == 0xb || function == 0x1f) {
                         entry = kvm_find_cpuid_entry(vcpu, function, 1);
                         if (entry) {
                                 *ecx = index & 0xff;
                                 *edx = entry->edx;
                         }
                 }
         }

but KVM in principle could set EDX to the right value for 0xB and 0x1F, 
the x2APIC ID is available for the kernel LAPIC case.  0x8000001e cannot 
be fixed up the same way.

> I suppose one could argue that KVM needs to communicate to userspace that KVM
> emulates the edge case behavior of CPUID 0xB and 0x1F, but I would argue that KVM
> communicates that by announcing a max basic leaf >= 0xB/0x1F.

I agree (or we could fix it up automagically if so inclined).  Either 
way it should be documented at the end of api.rst.

Paolo
diff mbox series

Patch

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index a0292ba650df..380b71600a9e 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -1193,6 +1193,9 @@  static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
 		entry->ebx = entry->ecx = entry->edx = 0;
 		break;
 	case 0x8000001e:
+		/* Different on each processor, just hide it.  */
+		entry->eax = entry->ebx = entry->ecx = 0;
+		entry->edx = 0;
 		break;
 	case 0x8000001F:
 		if (!kvm_cpu_cap_has(X86_FEATURE_SEV)) {