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

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

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

Thanks, Roger.

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