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@xxxxxxxxxx>:
> 2017-03-20 05:25-0700, Wanpeng Li:
>> From: Wanpeng Li <wanpeng.li@xxxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> Cc: Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx>
>> Signed-off-by: Wanpeng Li <wanpeng.li@xxxxxxxxxxx>
>> ---
>>  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