mtrr_set_all() has quite a bit of overhead, which is entirely useless
when set_mtrr_state() really does nothing. Furthermore, with
mtrr_state.def_type never initialized from hardware, post_set()'s
unconditional writing of the MSR means would leave us running in UC
mode after the sync.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/cpu/mtrr/main.c
+++ b/xen/arch/x86/cpu/mtrr/main.c
@@ -605,13 +605,15 @@ void mtrr_aps_sync_begin(void)
 
 void mtrr_aps_sync_end(void)
 {
-	set_mtrr(~0U, 0, 0, 0);
+	if (mtrr_if)
+		set_mtrr(~0U, 0, 0, 0);
 	hold_mtrr_updates_on_aps = 0;
 }
 
 void mtrr_bp_restore(void)
 {
-	mtrr_set_all();
+	if (mtrr_if)
+		mtrr_set_all();
 }
 
 static int __init cf_check mtrr_init_finialize(void)

On Thu, Mar 27, 2025 at 10:54:23AM +0100, Jan Beulich wrote:
> mtrr_set_all() has quite a bit of overhead, which is entirely useless
> when set_mtrr_state() really does nothing. Furthermore, with
> mtrr_state.def_type never initialized from hardware, post_set()'s
> unconditional writing of the MSR means would leave us running in UC
> mode after the sync.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Conditional on reaching consensus on whether the mtrr_bp_restore()
needs re-adding to the resume path.  Otherwise the code needs to be
removed.

Thanks, Roger.

On Thu, Mar 27, 2025 at 10:54:23AM +0100, Jan Beulich wrote:
> mtrr_set_all() has quite a bit of overhead, which is entirely useless
> when set_mtrr_state() really does nothing. Furthermore, with
> mtrr_state.def_type never initialized from hardware, post_set()'s
> unconditional writing of the MSR means would leave us running in UC
> mode after the sync.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/cpu/mtrr/main.c
> +++ b/xen/arch/x86/cpu/mtrr/main.c
> @@ -605,13 +605,15 @@ void mtrr_aps_sync_begin(void)
>  
>  void mtrr_aps_sync_end(void)
>  {
> -	set_mtrr(~0U, 0, 0, 0);
> +	if (mtrr_if)
> +		set_mtrr(~0U, 0, 0, 0);
>  	hold_mtrr_updates_on_aps = 0;
>  }
>  
>  void mtrr_bp_restore(void)

Maybe I'm blind, but I cannot find any caller to mtrr_bp_restore()?
Am I missing something obvious?

Thanks, Roger.

On 27.03.2025 11:53, Roger Pau Monné wrote:
> On Thu, Mar 27, 2025 at 10:54:23AM +0100, Jan Beulich wrote:
>> mtrr_set_all() has quite a bit of overhead, which is entirely useless
>> when set_mtrr_state() really does nothing. Furthermore, with
>> mtrr_state.def_type never initialized from hardware, post_set()'s
>> unconditional writing of the MSR means would leave us running in UC
>> mode after the sync.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/cpu/mtrr/main.c
>> +++ b/xen/arch/x86/cpu/mtrr/main.c
>> @@ -605,13 +605,15 @@ void mtrr_aps_sync_begin(void)
>>  
>>  void mtrr_aps_sync_end(void)
>>  {
>> -	set_mtrr(~0U, 0, 0, 0);
>> +	if (mtrr_if)
>> +		set_mtrr(~0U, 0, 0, 0);
>>  	hold_mtrr_updates_on_aps = 0;
>>  }
>>  
>>  void mtrr_bp_restore(void)
> 
> Maybe I'm blind, but I cannot find any caller to mtrr_bp_restore()?
> Am I missing something obvious?

You don't. It was lost in 4304ff420e51 ("x86/S3: Drop
{save,restore}_rest_processor_state() completely"), with there being no
indication in the description that this was actually intentional. Looks like
another S3 regression we need to fix. Unless you, Andrew, have an explanation
for this.

Jan

On 27/03/2025 11:03 am, Jan Beulich wrote:
> On 27.03.2025 11:53, Roger Pau Monné wrote:
>> On Thu, Mar 27, 2025 at 10:54:23AM +0100, Jan Beulich wrote:
>>> mtrr_set_all() has quite a bit of overhead, which is entirely useless
>>> when set_mtrr_state() really does nothing. Furthermore, with
>>> mtrr_state.def_type never initialized from hardware, post_set()'s
>>> unconditional writing of the MSR means would leave us running in UC
>>> mode after the sync.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/xen/arch/x86/cpu/mtrr/main.c
>>> +++ b/xen/arch/x86/cpu/mtrr/main.c
>>> @@ -605,13 +605,15 @@ void mtrr_aps_sync_begin(void)
>>>  
>>>  void mtrr_aps_sync_end(void)
>>>  {
>>> -	set_mtrr(~0U, 0, 0, 0);
>>> +	if (mtrr_if)
>>> +		set_mtrr(~0U, 0, 0, 0);
>>>  	hold_mtrr_updates_on_aps = 0;
>>>  }
>>>  
>>>  void mtrr_bp_restore(void)
>> Maybe I'm blind, but I cannot find any caller to mtrr_bp_restore()?
>> Am I missing something obvious?
> You don't. It was lost in 4304ff420e51 ("x86/S3: Drop
> {save,restore}_rest_processor_state() completely"), with there being no
> indication in the description that this was actually intentional. Looks like
> another S3 regression we need to fix. Unless you, Andrew, have an explanation
> for this.

Hmm, I don't think I intended to make a change without discussing it.

However, I think I'd concluded that it was redundant with the
mtrr_aps_sync_end() call.

~Andrew