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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Mar 02, 2018 at 02:29:54PM +0000, Sergey Dyasli wrote:
> 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]?

Yes, I think so. The nested GUEST_CR4 value is the value the L1
hypervisor thinks is written to the hardware CR4 (while the L2 guest
is running) and guest_cr[4] contains the value of the CR4 register as
seen by the L1 hypervisor.

There's no way CR4 L1 tapped bits can leak into guest_cr[4] because
cr4_host_mask is always equally or more restrictive than the nested
CR4_GUEST_HOST_MASK. Let me send a new version.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel