Re: [Xen-devel] [PATCH 3/3] x86/vmx: Better description of CR4 settings outside of paged mode

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

On Tue, Oct 03, 2017 at 02:12:22PM +0000, Andrew Cooper wrote:
> On 03/10/17 15:04, Roger Pau Monné wrote:
> > On Fri, Sep 29, 2017 at 06:31:03PM +0000, Andrew Cooper wrote:
> >> This rearanges the logic to avoid the double !hvm_paging_enabled(v) check, 
> >> but
> >> is otherwise identical.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >> ---
> >> CC: Jan Beulich <JBeulich@xxxxxxxx>
> >> CC: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> >> CC: Kevin Tian <kevin.tian@xxxxxxxxx>
> >> ---
> >>  xen/arch/x86/hvm/vmx/vmx.c | 37 ++++++++++++++++++++++++++++---------
> >>  1 file changed, 28 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> >> index 5b943d4..5b9b074 100644
> >> --- a/xen/arch/x86/hvm/vmx/vmx.c
> >> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> >> @@ -1642,21 +1642,40 @@ static void vmx_update_guest_cr(struct vcpu *v, 
> >> unsigned int cr)
> >>          v->arch.hvm_vcpu.hw_cr[4] |= v->arch.hvm_vcpu.guest_cr[4];
> >>          if ( v->arch.hvm_vmx.vmx_realmode )
> >>              v->arch.hvm_vcpu.hw_cr[4] |= X86_CR4_VME;
> >> -        if ( paging_mode_hap(v->domain) && !hvm_paging_enabled(v) )
> >> -        {
> >> -            v->arch.hvm_vcpu.hw_cr[4] |= X86_CR4_PSE;
> >> -            v->arch.hvm_vcpu.hw_cr[4] &= ~X86_CR4_PAE;
> >> -        }
> >> +
> >>          if ( !hvm_paging_enabled(v) )
> >>          {
> >>              /*
> >> -             * SMEP/SMAP is disabled if CPU is in non-paging mode in 
> >> hardware.
> >> -             * However Xen always uses paging mode to emulate guest 
> >> non-paging
> >> -             * mode. To emulate this behavior, SMEP/SMAP needs to be 
> >> manually
> >> -             * disabled when guest VCPU is in non-paging mode.
> >> +             * When the guest thinks paging is disabled, Xen may need to 
> >> hide
> >> +             * the effects of running with CR0.PG actually enabled.  
> >> There are
> >> +             * two subtly complicated cases.
> >> +             */
> >> +
> >> +            if ( paging_mode_hap(v->domain) )
> >> +            {
> >> +                /*
> >> +                 * On hardware lacking the Unrestricted Guest feature (or 
> >> with
> >> +                 * it disabled in the VMCS), we may not enter the guest 
> >> with
> > Shouldn't this be paging_mode_hap && vmx_unrestricted_guest?
> 
> ITYM paging_mode_hap && !vmx_unrestricted_guest
> 
> >
> > From the code below I think it's harmless what we do with CR4 if
> > CR0.PG is disabled, but in any case it would be good to mention it in
> > the comment IMHO.
> 
> Indeed it is harmless, which is why I didn't include the extra
> conditional.  Including it would invalidate my statement of "otherwise
> identical".

Right.

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel