Re: [PATCH v3 07/22] x86/mtrr: expose functions for pausing caching

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.05.2025 15:17, Sergii Dmytruk wrote:
> This allows the functionality to be reused by other units that need to
> update MTRRs.
> 
> This also gets rid of a static variable.
> 
> Signed-off-by: Sergii Dmytruk <sergii.dmytruk@xxxxxxxxx>

This may want to be split.

> --- a/xen/arch/x86/cpu/mtrr/generic.c
> +++ b/xen/arch/x86/cpu/mtrr/generic.c
> @@ -396,9 +396,7 @@ static bool set_mtrr_var_ranges(unsigned int index, 
> struct mtrr_var_range *vr)
>       return changed;
>  }
>  
> -static uint64_t deftype;
> -
> -static unsigned long set_mtrr_state(void)
> +static unsigned long set_mtrr_state(uint64_t *deftype)
>  /*  [SUMMARY] Set the MTRR state for this CPU.
>      <state> The MTRR state information to read.
>      <ctxt> Some relevant CPU context.
> @@ -416,14 +414,12 @@ static unsigned long set_mtrr_state(void)
>       if (mtrr_state.have_fixed && set_fixed_ranges(mtrr_state.fixed_ranges))
>               change_mask |= MTRR_CHANGE_MASK_FIXED;
>  
> -     /*  Set_mtrr_restore restores the old value of MTRRdefType,
> -        so to set it we fiddle with the saved value  */
> -     if ((deftype & 0xff) != mtrr_state.def_type
> -         || MASK_EXTR(deftype, MTRRdefType_E) != mtrr_state.enabled
> -         || MASK_EXTR(deftype, MTRRdefType_FE) != mtrr_state.fixed_enabled) {
> -             deftype = (deftype & ~0xcff) | mtrr_state.def_type |
> -                       MASK_INSR(mtrr_state.enabled, MTRRdefType_E) |
> -                       MASK_INSR(mtrr_state.fixed_enabled, MTRRdefType_FE);
> +     if ((*deftype & 0xff) != mtrr_state.def_type
> +         || MASK_EXTR(*deftype, MTRRdefType_E) != mtrr_state.enabled
> +         || MASK_EXTR(*deftype, MTRRdefType_FE) != mtrr_state.fixed_enabled) 
> {
> +             *deftype = (*deftype & ~0xcff) | mtrr_state.def_type |
> +                        MASK_INSR(mtrr_state.enabled, MTRRdefType_E) |
> +                        MASK_INSR(mtrr_state.fixed_enabled, MTRRdefType_FE);
>               change_mask |= MTRR_CHANGE_MASK_DEFTYPE;
>       }

This (together with the caller side adjustment) looks like it could be a 
separate
change.

> @@ -440,9 +436,10 @@ static DEFINE_SPINLOCK(set_atomicity_lock);
>   * has been called.
>   */
>  
> -static bool prepare_set(void)
> +struct mtrr_pausing_state mtrr_pause_caching(void)

These becoming non-static without being called from anywhere else isn't going to
be liked by Misra. Hence the part of static -> extern may need to be deferred
until the new user(s) appear(s).

Furthermore this returning of a struct by value isn't very nice, and looks to be
easy to avoid here.

Jan