Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b7fa785b-cea3-3e05-c101-d6c7bd101ef3@suse.com>
Date:   Wed, 6 Jul 2022 08:17:41 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH] x86/PAT: have pat_enabled() properly reflect state when
 running on e.g. Xen
Content-Language: en-US
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>
References: <9385fa60-fa5d-f559-a137-6608408f88b0@suse.com>
 <YsRTAGI2PhfZ5V7M@zn.tnic> <016d281b-7e40-f1bd-66ee-c19c3cc56efe@suse.com>
 <YsRjX/U1XN8rq+8u@zn.tnic>
From:   Jan Beulich <jbeulich@suse.com>
In-Reply-To: <YsRjX/U1XN8rq+8u@zn.tnic>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UHFrVkVkUkd0SFZJR3AvMHZyb0t4NU5pQ2EwQ0hoU28vckIwakpvNVpXMzBj?=
 =?utf-8?B?Q0J5cVZLTWZWelhXS25XZnBEZ1JWZTk4Z2VOR0RwdmVNSzNITGdUS0xLQW9q?=
 =?utf-8?B?eForSTdic0xpbi93TGdVTG1rWDhpWkJ1TncvY0dsRDhHbVo3VHlQaXN2dG5k?=
 =?utf-8?B?MWtVeG1VcFhwanFJaDE4aGE3TlpKUlJrcUprRCsrR1IxbEQ2cDVGQVZ6c3dv?=
 =?utf-8?B?TWtFb2R3aVArQ2k0MnF1MCtUc1NXOUZKekc3WWNJQXplcGtjMEZieTBlQ0FU?=
 =?utf-8?B?QVdzY0REUlBkMFhkL1I1eXFZemg0bzlOSlFXZFMrVzY3NVdubzUvNmk4VlJ1?=
 =?utf-8?B?TTVRM05qT1IzUGNYRkE3ZlZwWm55MXoxTTJaR3VWL0hZVzdtVDdZaThrMllz?=
 =?utf-8?B?Vmk3cjFyRlhUeXkwd1M4TVBCR3JVNSsrUHZRY1RwOVhqTUxYM0hkcHdDZGJp?=
 =?utf-8?B?UHZxYzV1aXAyZTdzVkNIdjVnSHE3MFJ6blQwYXhTRkxYZENaWEt0d3RvSXYx?=
 =?utf-8?B?Nk9vU0NaTzJnVkpkcjdQdnVuL2dCdmNVL2t4M0VjR3pNR2lGRXo2S3ZGVThP?=
 =?utf-8?B?OWMyNVpEK05xUUZTWktTUWd5WDdxUm94TXljYmlsR1hxUG9vVXM1SDRlVHJz?=
 =?utf-8?B?Witqbi9aanUrM3Q1aU5QMWQ4cXlnTHFxM2NOSmFNSm5WNG56aEpuN2hjMWFH?=
 =?utf-8?B?RjljU2ZuQlZQTWFIOGtJbnJ2UkhpOHlZVVlCMStZUWlFOXFCR2tqNGgvOTcr?=
 =?utf-8?B?ZXdGMFZDRXBxWHVjaXpkcENXeHl5ZU9KcSt5WU10RElNZ25ZOTBraU9KaHcw?=
 =?utf-8?B?ekR3SWhZekJMUkt2Um5JSFV3UStVdkgvdmttOE5hMTF3aXR4aWE3clZGTlBo?=
 =?utf-8?B?d3pCS092M2IxQUtmS243QzJSeHE5NTNkTFd3Zm9EUTdkaXlCRnpHUFFKbDV2?=
 =?utf-8?B?bVpmQ2w0Zzcza1JTNUplcUFVVlZ3dXUrMmg4dkhPRVJud2VydmFGT0xNZnJq?=
 =?utf-8?B?NDlueldmSmFkSTBVazJ2RjZVRlVKTVFPMGJjek5XM1crcWJMZDdOS0hnVUNQ?=
 =?utf-8?B?OVlpa2ltV1lZbjR0WUE3NGtJd1ZENWJqWWcrOVhnSGl5YldiYWFwVzNNSDE1?=
 =?utf-8?B?SmhXS1ZTTXdmclFrcFFJd1dDVWo0V0MzK29vSnZaaVBEWmFnVjFJcXlMeEFB?=
 =?utf-8?B?T21hbHFNQjhiRHdJWjJHSjhhajBXb1A3UUh0NFBVRU1iUmlFR1hkZGtoSjJs?=
 =?utf-8?B?NGtmSlFiRW84cnVyWnA0a0FWQUQzRVgreU5pNndDeXlRcEtFME55eXErVnVH?=
 =?utf-8?B?Q0toV01QWDJJNE5OMVJHcG4zR1JQRzBtL1k1SDlTL3JXc1ZmUHFmcVZPU01w?=
 =?utf-8?B?cU8yalBRYldERm5kTkx6bTAvMmRoVnFVSW91Y1dNWVpqR1BFSzJsUDdJc0pG?=
 =?utf-8?B?TnA0b1BaN0JGSjVQZUhQWXBrYWFIdjgzeTlNbXpPOHNJTTd0eGpTMUhsUGZY?=
 =?utf-8?B?QkEwekZCMWR1SkF0SmIyc3k2ZGd3a0pubmMwSFZsNnpIZFE3RWdUL1lUUllu?=
 =?utf-8?B?Ulg0ZE9UeHIxQURDTXU5a0dzYU45akRtOFNFUnFRdnhOYVFZRWJYYjZ4ZEdw?=
 =?utf-8?B?RjRpTFMwNXcyMDVxcXhoaS9MSWx3UmFqWDBoempzbForRk9hSnNMcUEzdlJz?=
 =?utf-8?B?eG56cFFRaGxYTVRJQlJSWFd5RDFLSnVvd3habzcwR29IcHVCcnVNS0dlQXcv?=
 =?utf-8?B?Lyt2SXl1cmtXdWVtWHl1U2hCVE9nTXBmcFNNQWdLTXBHR0Y5czF4UCt1TWlX?=
 =?utf-8?B?R0UyMEJOc2xzVVdLYkFjbFJ6eDByaUxsV1d3NUtzT2s3N0Yrbk9MRUdhcXRU?=
 =?utf-8?B?QXYra2NGeGRPM1RQbEJUNU9valV4Mm95VVhhcTBxS1A4UGdNV3MvU1N6RFJY?=
 =?utf-8?B?RDJ3SlVnZHlFdUJvRUc4eVFIZGcyL2tmQVJ1QS9jV2VWWkljVk1JUkVkbjhZ?=
 =?utf-8?B?VGpjS05ONmJKQ214ejdFbWFDM21MM0NRMnByM21XYXpGTVBzSVM1czNIQVdM?=
 =?utf-8?B?RjVXY29BOTJScGZqazN5S2RCNFFEWDFOcU8vRSs1OWFGZEZ1UE5xY2FHNm9m?=
 =?utf-8?Q?lmrr6Xqsc9douVJkvE4aMfkfB?=
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1a15c52d-1ced-4263-0bc7-08da5f173c96
X-MS-Exchange-CrossTenant-AuthSource: AM6PR04MB6551.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Jul 2022 06:17:43.3105
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: I0afLh/yH6v1DmY6Z8RlKepQkH80TrWasyUwR57tFxxhD+TyRk9ChdNnnWC9ZI12bWbgJ1TRMhFwfDpWo96nKg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB3PR0402MB3787
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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