Re: [PATCH] x86/PAT: have pat_enabled() properly reflect state when running on e.g. Xen

From: Jan Beulich <jbeulich@suse.com>
To: Borislav Petkov <bp@alien8.de>
Cc: Andrew Lutomirski <luto@kernel.org>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
	Peter Zijlstra <peterz@infradead.org>,
	lkml <linux-kernel@vger.kernel.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH] x86/PAT: have pat_enabled() properly reflect state when running on e.g. Xen
Date: Wed, 6 Jul 2022 08:17:41 +0200	[thread overview]
Message-ID: <b7fa785b-cea3-3e05-c101-d6c7bd101ef3@suse.com> (raw)
In-Reply-To: <YsRjX/U1XN8rq+8u@zn.tnic>

On 05.07.2022 18:14, Borislav Petkov wrote:
> On Tue, Jul 05, 2022 at 05:56:36PM +0200, Jan Beulich wrote:
>> Re-using pat_disabled like you do in your suggestion below won't
>> work, because mtrr_bp_init() calls pat_disable() when MTRRs
>> appear to be disabled (from the kernel's view). The goal is to
>> honor "nopat" without honoring any other calls to pat_disable().
> 
> Actually, the current goal is to adjust Xen dom0 because:
> 
> 1. it uses the PAT code
> 
> 2. but then it does something special and hides the MTRRs
> 
> which is not something real hardware does.
> 
> So this one-off thing should be prominent, visible and not get in the
> way.
> 
> As to mtrr_bp_init(), can you use X86_FEATURE_XENPV there to detect this
> special case when the kernel is running as dom0 and set stuff there
> accordingly so that it doesn't disable PAT?

Sure, but that alone won't help. There's a beneficial side effect
of running through pat_disable(): That way pat_init() will bail
right away. Without that I'd need to further special case things
there (as under Xen/PV PAT must not be written, only read) and I'd
also need to set pat_bp_enabled and pat_bp_initialized somewhere.
I could of course check X86_FEATURE_XENPV in all the necessary
places, but I was quite certain _that_ wouldn't be liked (nor
would I be convinced this is the right thing to do - see bottom).

> Then you don't have to touch pat_disabled() either but intergrate the
> Xen variant properly...
> 
>> I can probably fiddle with pat_enabled() instead of with
>> init_cache_modes(), but when making the change I had the feeling
>> this might be less liked (as looking more hacky, at least to me).
> 
> Why would that be more hacky?

My view on it, as said. I did actually make several attempts, until
reaching what I then submitted. All earlier ones were quite a bit
more intrusive (see above for an outline).

> I'd much rather check upfront what the kernel is running on and act
> accordingly instead of hooking into random functions and then years
> later wonder why was it done in the first place.

Thank you for putting it that kindly. It was a pretty conscious
decision where to make the changes, after - as said - quite a bit
of trying other variants. History with Xen-specific changes has
taught me to try to keep them as uninvasive and generic as possible.
The more things smelled like Xen-only, the less they were liked.

>> But besides the "where" the other question is: Do you really want
>> me to limit this to Xen/PV, rather than - as I have it now -
>> extending it to any hypervisor, which may behave in similar ways?
> 
> Well, do you know of some other HV which hides MTRRs from the guest?
> 
> I haven't heard of any...

Any decent hypervisor will allow overriding CPUID, so in principle
I'd expect any to permit disabling MTRR to leave a guest to use
the (more modern and less cumbersome) PAT alone.

Jan