Re: [Xen-devel] [PATCH] vvmx: fixes after CR4 trapping optimizations

[Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>]

On Thu, 2018-03-01 at 16:19 +0000, Roger Pau Monne wrote:
> Commit 406817 doesn't update nested VMX code in order to take into
> account L1 CR4 host mask when nested guest (L2) writes to CR4, and
> thus the mask written to CR4_GUEST_HOST_MASK is likely not as
> restrictive as it should be.
> 
> Also the VVMCS GUEST_CR4 value should be updated to match the
> underlying value when syncing the VVMCS state.
> 
> Fixes: 406817 ("vmx/hap: optimize CR4 trapping")
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Cc: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> I've manually tested and AFAICT this fixes the osstest failure
> detected in 120076 ("test-amd64-amd64-qemuu-nested-intel").
> ---
>  xen/arch/x86/hvm/vmx/vmx.c  |  4 ++++
>  xen/arch/x86/hvm/vmx/vvmx.c | 13 ++++++++++++-
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 5cee364b0c..18d8ce2303 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1617,6 +1617,10 @@ static void vmx_update_guest_cr(struct vcpu *v, 
> unsigned int cr,
>                  v->arch.hvm_vmx.cr4_host_mask |=
>                  
> ~v->domain->arch.monitor.write_ctrlreg_mask[VM_EVENT_X86_CR4];
>  
> +            if ( nestedhvm_vcpu_in_guestmode(v) )
> +                /* Add the nested host mask to get the more restrictive one. 
> */
> +                v->arch.hvm_vmx.cr4_host_mask |= get_vvmcs(v,
> +                                                           
> CR4_GUEST_HOST_MASK);
>          }
>          __vmwrite(CR4_GUEST_HOST_MASK, v->arch.hvm_vmx.cr4_host_mask);
>  
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index 8176736e8f..2baf707eec 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1101,7 +1101,8 @@ static void load_shadow_guest_state(struct vcpu *v)
>                       (get_vvmcs(v, CR4_READ_SHADOW) & cr_gh_mask);
>      __vmwrite(CR4_READ_SHADOW, cr_read_shadow);
>      /* Add the nested host mask to the one set by vmx_update_guest_cr. */
> -    __vmwrite(CR4_GUEST_HOST_MASK, cr_gh_mask | 
> v->arch.hvm_vmx.cr4_host_mask);
> +    v->arch.hvm_vmx.cr4_host_mask |= cr_gh_mask;
> +    __vmwrite(CR4_GUEST_HOST_MASK, v->arch.hvm_vmx.cr4_host_mask);
>  
>      /* TODO: CR3 target control */
>  }
> @@ -1232,6 +1233,16 @@ static void sync_vvmcs_guest_state(struct vcpu *v, 
> struct cpu_user_regs *regs)
>      /* CR3 sync if exec doesn't want cr3 load exiting: i.e. nested EPT */
>      if ( !(__n2_exec_control(v) & CPU_BASED_CR3_LOAD_EXITING) )
>          shadow_to_vvmcs(v, GUEST_CR3);
> +
> +    if ( v->arch.hvm_vmx.cr4_host_mask != ~0UL )
> +    {
> +       /* Only need to update nested GUEST_CR4 if not all bits are trapped. 
> */
> +        unsigned long nested_cr4_mask = get_vvmcs(v, CR4_GUEST_HOST_MASK);
> +
> +        set_vvmcs(v, GUEST_CR4,
> +                  (get_vvmcs(v, GUEST_CR4) & nested_cr4_mask) |
> +                  (v->arch.hvm_vcpu.guest_cr[4] & ~nested_cr4_mask));

Why reading the old GUEST_CR4 is needed here? Can the new value be set
directly from guest_cr[4]?

> +    }
>  }
>  
>  static void sync_vvmcs_ro(struct vcpu *v)
-- 
Thanks,
Sergey
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel