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@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.

> 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
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 ...