[PATCH] KVM: nVMX: Fix L2 guest crash when VPID is disabled on L0

[PATCH] KVM: nVMX: Fix L2 guest crash when VPID is disabled on L0

[Wanpeng Li]

From: Wanpeng Li <wanpeng.li@hotmail.com>

L2 guest crash during boot if VPID is enabled on L1 and disabled on L0. This 
also can be catched by kvm-unit-tests/vmx.flat when VPID is disabled on L0.

KVM: entry failed, hardware error 0x7
EAX=00000000 EBX=00000000 ECX=00000000 EDX=000306c3
ESI=00000000 EDI=00000000 EBP=00000000 ESP=00000000
EIP=0000fff0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
ES =0000 00000000 0000ffff 00009300
CS =f000 ffff0000 0000ffff 00009b00
SS =0000 00000000 0000ffff 00009300
DS =0000 00000000 0000ffff 00009300
FS =0000 00000000 0000ffff 00009300
GS =0000 00000000 0000ffff 00009300
LDT=0000 00000000 0000ffff 00008200
TR =0000 00000000 0000ffff 00008b00
GDT=     00000000 0000ffff
IDT=     00000000 0000ffff
CR0=60000010 CR2=00000000 CR3=00000000 CR4=00000000
DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000 
DR6=00000000ffff0ff0 DR7=0000000000000400
EFER=0000000000000000

The enable_vpid sysfs perm parameter miss the 0 prefix, so the enable_vpid 
is failed to be disabled though vmcs_config bit is not set. This patch fixes 
it by utilizing S_IRUGO which includes 0 prefix instead.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
 arch/x86/kvm/vmx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 98e82ee..a7e4880 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -66,7 +66,7 @@ static const struct x86_cpu_id vmx_cpu_id[] = {
 MODULE_DEVICE_TABLE(x86cpu, vmx_cpu_id);
 
 static bool __read_mostly enable_vpid = 1;
-module_param_named(vpid, enable_vpid, bool, 0444);
+module_param_named(vpid, enable_vpid, bool, S_IRUGO);
 
 static bool __read_mostly flexpriority_enabled = 1;
 module_param_named(flexpriority, flexpriority_enabled, bool, S_IRUGO);
-- 
2.7.4

Re: [PATCH] KVM: nVMX: Fix L2 guest crash when VPID is disabled on L0

[Radim Krčmář]

2017-03-20 05:25-0700, Wanpeng Li:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
> 
> L2 guest crash during boot if VPID is enabled on L1 and disabled on L0. This 
> also can be catched by kvm-unit-tests/vmx.flat when VPID is disabled on L0.
> 
> KVM: entry failed, hardware error 0x7
> EAX=00000000 EBX=00000000 ECX=00000000 EDX=000306c3
> ESI=00000000 EDI=00000000 EBP=00000000 ESP=00000000
> EIP=0000fff0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
> ES =0000 00000000 0000ffff 00009300
> CS =f000 ffff0000 0000ffff 00009b00
> SS =0000 00000000 0000ffff 00009300
> DS =0000 00000000 0000ffff 00009300
> FS =0000 00000000 0000ffff 00009300
> GS =0000 00000000 0000ffff 00009300
> LDT=0000 00000000 0000ffff 00008200
> TR =0000 00000000 0000ffff 00008b00
> GDT=     00000000 0000ffff
> IDT=     00000000 0000ffff
> CR0=60000010 CR2=00000000 CR3=00000000 CR4=00000000
> DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000 
> DR6=00000000ffff0ff0 DR7=0000000000000400
> EFER=0000000000000000

I couldn't reproduce the hardware error.  Unit test just got #UD and
exited, which seems to be because KVM doesn't add the exec controls in
vmcs02.

> The enable_vpid sysfs perm parameter miss the 0 prefix, so the enable_vpid 
> is failed to be disabled though vmcs_config bit is not set. This patch fixes 
> it by utilizing S_IRUGO which includes 0 prefix instead.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
>  arch/x86/kvm/vmx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 98e82ee..a7e4880 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -66,7 +66,7 @@ static const struct x86_cpu_id vmx_cpu_id[] = {
>  MODULE_DEVICE_TABLE(x86cpu, vmx_cpu_id);
>  
>  static bool __read_mostly enable_vpid = 1;
> -module_param_named(vpid, enable_vpid, bool, 0444);
> +module_param_named(vpid, enable_vpid, bool, S_IRUGO);

Aren't "0444" and "(00400 | 00040 | 00004)" the same number?

I'm not convinced this patch fixes anything and I think we have two
options when approaching VPID:
 1) hide and forbid VPIDs in L1 if they are disabled on L0
 2) expose VPID hardware to L1 regardless of L0 settings

We treat other features like (1), because it simplifies implementation,
so I'd do the same for VPID ...

Re: [PATCH] KVM: nVMX: Fix L2 guest crash when VPID is disabled on L0

[Wanpeng Li]

2017-03-20 22:58 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>:
> 2017-03-20 05:25-0700, Wanpeng Li:
>> From: Wanpeng Li <wanpeng.li@hotmail.com>
>>
>> L2 guest crash during boot if VPID is enabled on L1 and disabled on L0. This
>> also can be catched by kvm-unit-tests/vmx.flat when VPID is disabled on L0.
>>
>> KVM: entry failed, hardware error 0x7
>> EAX=00000000 EBX=00000000 ECX=00000000 EDX=000306c3
>> ESI=00000000 EDI=00000000 EBP=00000000 ESP=00000000
>> EIP=0000fff0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
>> ES =0000 00000000 0000ffff 00009300
>> CS =f000 ffff0000 0000ffff 00009b00
>> SS =0000 00000000 0000ffff 00009300
>> DS =0000 00000000 0000ffff 00009300
>> FS =0000 00000000 0000ffff 00009300
>> GS =0000 00000000 0000ffff 00009300
>> LDT=0000 00000000 0000ffff 00008200
>> TR =0000 00000000 0000ffff 00008b00
>> GDT=     00000000 0000ffff
>> IDT=     00000000 0000ffff
>> CR0=60000010 CR2=00000000 CR3=00000000 CR4=00000000
>> DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000
>> DR6=00000000ffff0ff0 DR7=0000000000000400
>> EFER=0000000000000000
>
> I couldn't reproduce the hardware error.  Unit test just got #UD and
> exited, which seems to be because KVM doesn't add the exec controls in
> vmcs02.

I fix this unit test #UD in v2 1/3, and the above crash in v2 2/3.

>
>> The enable_vpid sysfs perm parameter miss the 0 prefix, so the enable_vpid
>> is failed to be disabled though vmcs_config bit is not set. This patch fixes
>> it by utilizing S_IRUGO which includes 0 prefix instead.
>>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Radim Krčmář <rkrcmar@redhat.com>
>> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
>> ---
>>  arch/x86/kvm/vmx.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 98e82ee..a7e4880 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -66,7 +66,7 @@ static const struct x86_cpu_id vmx_cpu_id[] = {
>>  MODULE_DEVICE_TABLE(x86cpu, vmx_cpu_id);
>>
>>  static bool __read_mostly enable_vpid = 1;
>> -module_param_named(vpid, enable_vpid, bool, 0444);
>> +module_param_named(vpid, enable_vpid, bool, S_IRUGO);
>
> Aren't "0444" and "(00400 | 00040 | 00004)" the same number?
>
> I'm not convinced this patch fixes anything and I think we have two

Yeah, maybe I'm too tired yesterday and more coffee is really appreciated. :)

> options when approaching VPID:
>  1) hide and forbid VPIDs in L1 if they are disabled on L0
>  2) expose VPID hardware to L1 regardless of L0 settings
>
> We treat other features like (1), because it simplifies implementation,
> so I'd do the same for VPID ...

Good point. Please review v2. :)

Regards,
Wanpeng Li