Re: [PATCH v4 01/23] x86/mtrr: get rid of a static variable on pause/restore

Teddy Astie <[email protected]> Thu, 20 Aug 2026 14:34:06 +0200
Newsgroups	org.xenproject.lists.xen-devel
Message-ID	<1787229248.8631fc262581453bbf619ec5b2062170.1a01f2a1b2d000c4f3@vates.tech>
Le 02/08/2026 à 15:14, Sergii Dmytruk a écrit :
> In addition to keeping the state in one place instead of on stack and in
> a static variable, this enables exposing this functionality to other
> units in the future.
> 
> Signed-off-by: Sergii Dmytruk <[email protected]>
> ---
> 
> Notes:
>      v4: was called "x86/mtrr: expose functions for pausing caching"
>      v4: no longer makes anything public, just updates implementation
>      v4: the state structure is now an output parameter instead of a return value
>      v4: switches from rdmsrl() to rdmsr() on one line that's updated anyway
> 
>   xen/arch/x86/cpu/mtrr/generic.c | 55 +++++++++++++++++----------------
>   1 file changed, 28 insertions(+), 27 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/mtrr/generic.c b/xen/arch/x86/cpu/mtrr/generic.c
> index 23c279eb9a..86eb0f405b 100644
> --- a/xen/arch/x86/cpu/mtrr/generic.c
> +++ b/xen/arch/x86/cpu/mtrr/generic.c
> @@ -14,6 +14,11 @@
>   #include <asm/cpufeature.h>
>   #include "mtrr.h"
>   
> +struct mtrr_pausing_state {
> +	bool pge;
> +	uint64_t def_type;
> +};
> +
>   static const struct fixed_range_block {
>   	uint32_t base_msr;   /* start address of an MTRR block */
>   	unsigned int ranges; /* number of MTRRs in this block  */
> @@ -395,9 +400,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.
> @@ -415,14 +418,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;
>   	}
>   
> @@ -439,7 +440,7 @@ static DEFINE_SPINLOCK(set_atomicity_lock);
>    * has been called.
>    */
>   
> -static bool prepare_set(void)
> +static void mtrr_pause_caching(struct mtrr_pausing_state *state)
>   {
>   	unsigned long cr4;
>   
> @@ -461,7 +462,9 @@ static bool prepare_set(void)
>   	alternative("wbinvd", "", X86_FEATURE_XEN_SELFSNOOP);
>   
>   	cr4 = read_cr4();
> -	if (cr4 & X86_CR4_PGE)
> +	state->pge = cr4 & X86_CR4_PGE;
> +
> +	if (state->pge)
>   		write_cr4(cr4 & ~X86_CR4_PGE);
>   	else if (use_invpcid)
>   		invpcid_flush_all();
> @@ -469,27 +472,25 @@ static bool prepare_set(void)
>   		write_cr3(read_cr3());
>   
>   	/*  Save MTRR state */
> -	rdmsrl(MSR_MTRRdefType, deftype);
> +	state->def_type = rdmsr(MSR_MTRRdefType);
>   
>   	/*  Disable MTRRs, and set the default type to uncached  */
> -	mtrr_wrmsr(MSR_MTRRdefType, deftype & ~0xcff);
> +	mtrr_wrmsr(MSR_MTRRdefType, state->def_type & ~0xcff);
>   
>   	/* Again, only flush caches if we have to. */
>   	alternative("wbinvd", "", X86_FEATURE_XEN_SELFSNOOP);
> -
> -	return cr4 & X86_CR4_PGE;
>   }
>   
> -static void post_set(bool pge)
> +static void mtrr_resume_caching(struct mtrr_pausing_state state)

It would be preferable to have const on the struct mtrr_pausing_state 
state to potentially improve codegen, as I guess the intent here is to 
pass a read-only object.

>   {
>   	/* Intel (P6) standard MTRRs */
> -	mtrr_wrmsr(MSR_MTRRdefType, deftype);
> +	mtrr_wrmsr(MSR_MTRRdefType, state.def_type);
>   
>   	/*  Enable caches  */
>   	write_cr0(read_cr0() & ~X86_CR0_CD);
>   
>   	/*  Reenable CR4.PGE (also flushes the TLB) */
> -	if (pge)
> +	if (state.pge)
>   		write_cr4(read_cr4() | X86_CR4_PGE);
>   	else if (use_invpcid)
>   		invpcid_flush_all();
> @@ -503,15 +504,15 @@ void mtrr_set_all(void)
>   {
>   	unsigned long mask, count;
>   	unsigned long flags;
> -	bool pge;
> +	struct mtrr_pausing_state pausing_state;
>   
>   	local_irq_save(flags);
> -	pge = prepare_set();
> +	mtrr_pause_caching(&pausing_state);
>   
>   	/* Actually set the state */
> -	mask = set_mtrr_state();
> +	mask = set_mtrr_state(&pausing_state.def_type);
>   
> -	post_set(pge);
> +	mtrr_resume_caching(pausing_state);
>   	local_irq_restore(flags);
>   
>   	/*  Use the atomic bitops to update the global mask  */
> @@ -536,12 +537,12 @@ void mtrr_set(
>   {
>   	unsigned long flags;
>   	struct mtrr_var_range *vr;
> -	bool pge;
> +	struct mtrr_pausing_state pausing_state;
>   
>   	vr = &mtrr_state.var_ranges[reg];
>   
>   	local_irq_save(flags);
> -	pge = prepare_set();
> +	mtrr_pause_caching(&pausing_state);
>   
>   	if (size == 0) {
>   		/* The invalid bit is kept in the mask, so we simply clear the
> @@ -562,7 +563,7 @@ void mtrr_set(
>   		mtrr_wrmsr(MSR_IA32_MTRR_PHYSMASK(reg), vr->mask);
>   	}
>   
> -	post_set(pge);
> +	mtrr_resume_caching(pausing_state);
>   	local_irq_restore(flags);
>   }
>   

With that change,

Reviewed-by: Teddy Astie <[email protected]>
OpenPGP_signature.asc (application/pgp-signature, 665 B)