Re: [Xen-devel] [PATCH v3] x86: do not enable global pages when virtualized on AMD hardware

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

On Mon, Dec 09, 2019 at 03:21:28PM +0100, Jan Beulich wrote:
> On 09.12.2019 11:20, Roger Pau Monné wrote:
> > On Wed, Dec 04, 2019 at 06:05:11PM +0100, Jan Beulich wrote:
> >> On 04.12.2019 17:18, Roger Pau Monné wrote:
> >>> On Wed, Dec 04, 2019 at 05:11:42PM +0100, Jan Beulich wrote:
> >>>> On 04.12.2019 16:12, Roger Pau Monne wrote:
> >>>>> @@ -130,7 +143,7 @@ unsigned long pv_make_cr4(const struct vcpu *v)
> >>>>>       */
> >>>>>      if ( d->arch.pv.pcid )
> >>>>>          cr4 |= X86_CR4_PCIDE;
> >>>>> -    else if ( !d->arch.pv.xpti )
> >>>>> +    else if ( !d->arch.pv.xpti && opt_global_pages )
> >>>>>          cr4 |= X86_CR4_PGE;
> >>>>
> >>>> I'm sorry for noticing this only now, but what about XEN_MINIMAL_CR4,
> >>>> which includes X86_CR4_PGE?
> >>>
> >>> I've tried removing PGE from XEN_MINIMAL_CR4 but it made no noticeable
> >>> performance difference, so I left it as-is.
> >>
> >> My concern isn't about performance, but correctness. I admit I
> >> forgot for a moment that we now always write CR4 (unless the
> >> cached value matches the intended new one). Yet
> >> mmu_cr4_features (starting out as XEN_MINIMAL_CR4) is still of
> >> concern.
> >>
> >> I think this at least requires extending the description to
> >> discuss the correctness.
> > 
> > Would you be fine with adding the following at the end of the commit
> > message.
> > 
> > "Note that XEN_MINIMAL_CR4 is not modified, and thus global pages are
> > left enabled for the hypervisor. This is not an issue because the code
> > to switch the control registers (cr3 and cr4) already takes into
> > account such situation and performs the necessary flushes. The same
> > already happens when using XPTI or PCIDE, as the guest cr4 doesn't
> > have global pages enabled in that case either."
> 
> Yes, this is good for XEN_MINIMAL_CR4. But I think mmu_cr4_features
> needs discussing (or at least mentioning, if the same arguments
> apply) as well.

The same applies to mmu_cr4_features, it's fine for the hypervisor to
use a different set of cr4 features (especially PGE) than PV guests:
this is already the case when using XPTI or PCIDE when Xen cr4 will
have PGE and guests cr4 won't, and switch_cr3_cr4 DTRT.

So I would s/XEN_MINIMAL_CR4/XEN_MINIMAL_CR4 and mmu_cr4_features/ in
the above proposed paragraph.

Thanks, Roger.

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