Re: [PATCH 3/3] x86: decouple pat and mtrr handling

[Juergen Gross <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 3734 bytes --]

On 19.07.22 17:15, Borislav Petkov wrote:
> On Fri, Jul 15, 2022 at 04:25:49PM +0200, Juergen Gross wrote:
>> Today PAT is usable only with MTRR being active, with some nasty tweaks
>> to make PAT usable when running as Xen PV guest, which doesn't support
>> MTRR.
>>
>> The reason for this coupling is, that both, PAT MSR changes and MTRR
>> changes, require a similar sequence and so full PAT support was added
>> using the already available MTRR handling.
>>
>> Xen PV PAT handling can work without MTRR, as it just needs to consume
>> the PAT MSR setting done by the hypervisor without the ability and need
>> to change it. This in turn has resulted in a convoluted initialization
>> sequence and wrong decisions regarding cache mode availability due to
>> misguiding PAT availability flags.
>>
>> Fix all of that by allowing to use PAT without MTRR and by adding an
>> environment dependent PAT init function.
> 
> Aha, there's the explanation I was looking for.
> 
>> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
>> index 0a1bd14f7966..3edfb779dab5 100644
>> --- a/arch/x86/kernel/cpu/common.c
>> +++ b/arch/x86/kernel/cpu/common.c
>> @@ -2408,8 +2408,8 @@ void __init cache_bp_init(void)
>>   {
>>   	if (IS_ENABLED(CONFIG_MTRR))
>>   		mtrr_bp_init();
>> -	else
>> -		pat_disable("PAT support disabled because CONFIG_MTRR is disabled in the kernel.");
>> +
>> +	pat_cpu_init();
>>   }
>>   
>>   void cache_ap_init(void)
>> @@ -2417,7 +2417,8 @@ void cache_ap_init(void)
>>   	if (cache_aps_delayed_init)
>>   		return;
>>   
>> -	mtrr_ap_init();
>> +	if (!mtrr_ap_init())
>> +		pat_ap_init_nomtrr();
>>   }
> 
> So I'm reading this as: if it couldn't init AP's MTRRs, init its PAT.
> 
> But currently, the code sets the MTRRs for the delayed case or when the
> CPU is not online by doing ->set_all and in there it sets first MTRRs
> and then PAT.
> 
> I think the code above should simply try the two things, one after the
> other, independently from one another.
> 
> And I see you've added another stomp machine call for PAT only.
> 
> Now, what I think the design of all this should be, is:
> 
> you have a bunch of things you need to do at each point:
> 
> * cache_ap_init
> 
> * cache_aps_init
> 
> * ...
> 
> Now, in each those, you look at whether PAT or MTRR is supported and you
> do only those which are supported.
> 
> Also, the rendezvous handler should do:
> 
> 	if MTRR:
> 		do MTRR specific stuff
> 
> 	if PAT:
> 		do PAT specific stuff
> 
> This way you have clean definitions of what needs to happen when and you
> also do *only* the things that the platform supports, by keeping the
> proper order of operations - I believe MTRRs first and then PAT.
> 
> This way we'll get rid of that crazy maze of who calls what and when.
> 
> But first we need to define those points where stuff needs to happen and
> then for each point define what stuff needs to happen.
> 
> How does that sound?

This asks for some more cleanup in the MTRR code:

mtrr_if->set_all() is the relevant callback, and it will only ever be called
for the generic case (use_intel() == true), so I think we want to:

- remove the cyrix specific set_all() function
- split the set_all() callback case from mtrr_rendezvous_handler() into a
   dedicated rendezvous handler
- remove the set_all() member from struct mtrr_ops and directly call
   generic_set_all() from the new rendezvous handler
- optional: rename use_intel() to use_generic(), or even introduce just
   a static bool for that purpose

Then the new rendezvous handler can be modified as you suggested.

Are you okay with that route?


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]