[PATCH] x86/MTRR: avoid several indirect calls

[PATCH] x86/MTRR: avoid several indirect calls

[Jan Beulich]

The use of (supposedly) vendor-specific hooks is a relic from the days
when Xen was still possible to build as 32-bit binary. There's no
expectation that a new need for such an abstraction would arise. Convert
mttr_if to a mere boolean and all prior calls through it to direct ones,
thus allowing to eliminate 6 ENDBR from .text.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Strictly speaking mtrr_if could go apparently away as well, for
effectively mirroring cpu_has_mtrr now (once mtrr_bp_init() has run).
Replacing the variable uses would be further code churn, though.

--- a/xen/arch/x86/cpu/mtrr/generic.c
+++ b/xen/arch/x86/cpu/mtrr/generic.c
@@ -287,7 +287,7 @@ static void set_fixed_range(int msr, boo
 	}
 }
 
-int cf_check generic_get_free_region(
+int mtrr_get_free_region(
     unsigned long base, unsigned long size, int replace_reg)
 /*  [SUMMARY] Get a free MTRR.
     <base> The starting (base) address of the region.
@@ -303,14 +303,14 @@ int cf_check generic_get_free_region(
 	if (replace_reg >= 0 && replace_reg < max)
 		return replace_reg;
 	for (i = 0; i < max; ++i) {
-		mtrr_if->get(i, &lbase, &lsize, &ltype);
+		mtrr_get(i, &lbase, &lsize, &ltype);
 		if (lsize == 0)
 			return i;
 	}
 	return -ENOSPC;
 }
 
-static void cf_check generic_get_mtrr(
+void mtrr_get(
     unsigned int reg, unsigned long *base, unsigned long *size, mtrr_type *type)
 {
 	uint64_t _mask, _base;
@@ -500,7 +500,7 @@ static void post_set(bool pge)
 	spin_unlock(&set_atomicity_lock);
 }
 
-static void cf_check generic_set_all(void)
+void mtrr_set_all(void)
 {
 	unsigned long mask, count;
 	unsigned long flags;
@@ -523,7 +523,7 @@ static void cf_check generic_set_all(voi
 	}
 }
 
-static void cf_check generic_set_mtrr(
+void mtrr_set(
     unsigned int reg, unsigned long base, unsigned long size, mtrr_type type)
 /*  [SUMMARY] Set variable MTRR register on the local CPU.
     <reg> The register to set.
@@ -567,7 +567,7 @@ static void cf_check generic_set_mtrr(
 	local_irq_restore(flags);
 }
 
-int cf_check generic_validate_add_page(
+int mtrr_validate_add_page(
     unsigned long base, unsigned long size, unsigned int type)
 {
 	unsigned long lbase, last;
@@ -586,21 +586,9 @@ int cf_check generic_validate_add_page(
 }
 
 
-static int cf_check generic_have_wrcomb(void)
+bool mtrr_have_wrcomb(void)
 {
 	unsigned long config;
 	rdmsrl(MSR_MTRRcap, config);
 	return (config & (1ULL << 10));
 }
-
-/* generic structure...
- */
-const struct mtrr_ops generic_mtrr_ops = {
-	.use_intel_if      = true,
-	.set_all	   = generic_set_all,
-	.get               = generic_get_mtrr,
-	.get_free_region   = generic_get_free_region,
-	.set               = generic_set_mtrr,
-	.validate_add_page = generic_validate_add_page,
-	.have_wrcomb       = generic_have_wrcomb,
-};
--- a/xen/arch/x86/cpu/mtrr/main.c
+++ b/xen/arch/x86/cpu/mtrr/main.c
@@ -57,7 +57,7 @@ static DEFINE_MUTEX(mtrr_mutex);
 u64 __read_mostly size_or_mask;
 u64 __read_mostly size_and_mask;
 
-const struct mtrr_ops *__read_mostly mtrr_if = NULL;
+static bool __ro_after_init mtrr_if;
 
 static void set_mtrr(unsigned int reg, unsigned long base,
 		     unsigned long size, mtrr_type type);
@@ -78,23 +78,12 @@ static const char *mtrr_attrib_to_str(in
 	return (x <= 6) ? mtrr_strings[x] : "?";
 }
 
-/*  Returns non-zero if we have the write-combining memory type  */
-static int have_wrcomb(void)
-{
-	return (mtrr_if->have_wrcomb ? mtrr_if->have_wrcomb() : 0);
-}
-
 /*  This function returns the number of variable MTRRs  */
 static void __init set_num_var_ranges(void)
 {
-	unsigned long config = 0;
+	unsigned long config;
 
-	if (use_intel()) {
-		rdmsrl(MSR_MTRRcap, config);
-	} else if (is_cpu(AMD))
-		config = 2;
-	else if (is_cpu(CENTAUR))
-		config = 8;
+	rdmsrl(MSR_MTRRcap, config);
 	num_var_ranges = MASK_EXTR(config, MTRRcap_VCNT);
 }
 
@@ -149,10 +138,10 @@ static void cf_check ipi_handler(void *i
 	if (data->smp_reg == ~0U) /* update all mtrr registers */
 		/* At the cpu hot-add time this will reinitialize mtrr 
  		 * registres on the existing cpus. It is ok.  */
-		mtrr_if->set_all();
+		mtrr_set_all();
 	else /* single mtrr register update */
-		mtrr_if->set(data->smp_reg, data->smp_base, 
-			     data->smp_size, data->smp_type);
+		mtrr_set(data->smp_reg, data->smp_base,
+			 data->smp_size, data->smp_type);
 
 	atomic_dec(&data->count);
 	while(atomic_read(&data->gate))
@@ -198,10 +187,9 @@ static inline int types_compatible(mtrr_
  * of CPUs. As each CPU disables interrupts, it'll decrement it once. We wait
  * until it hits 0 and proceed. We set the data.gate flag and reset data.count.
  * Meanwhile, they are waiting for that flag to be set. Once it's set, each 
- * CPU goes through the transition of updating MTRRs. The CPU vendors may each do it 
- * differently, so we call mtrr_if->set() callback and let them take care of it.
- * When they're done, they again decrement data->count and wait for data.gate to 
- * be reset. 
+ * CPU goes through the transition of updating MTRRs.
+ * When mtrr_set() is done, they again decrement data->count and wait for
+ * data.gate to be reset.
  * When we finish, we wait for data.count to hit 0 and toggle the data.gate flag.
  * Everyone then enables interrupts and we all continue on.
  *
@@ -251,9 +239,9 @@ static void set_mtrr(unsigned int reg, u
 	if (reg == ~0U)  /* update all mtrr registers */
 		/* at boot or resume time, this will reinitialize the mtrrs on 
 		 * the bp. It is ok. */
-		mtrr_if->set_all();
+		mtrr_set_all();
 	else /* update the single mtrr register */
-		mtrr_if->set(reg,base,size,type);
+		mtrr_set(reg, base, size, type);
 
 	/* wait for the others */
 	while (atomic_read(&data.count))
@@ -319,7 +307,7 @@ int mtrr_add_page(unsigned long base, un
 	if (!mtrr_if)
 		return -ENXIO;
 		
-	if ((error = mtrr_if->validate_add_page(base,size,type)))
+	if ((error = mtrr_validate_add_page(base, size, type)))
 		return error;
 
 	if (type >= MTRR_NUM_TYPES) {
@@ -328,7 +316,7 @@ int mtrr_add_page(unsigned long base, un
 	}
 
 	/*  If the type is WC, check that this processor supports it  */
-	if ((type == X86_MT_WC) && !have_wrcomb()) {
+	if ((type == X86_MT_WC) && mtrr_have_wrcomb()) {
 		printk(KERN_WARNING
 		       "mtrr: your processor doesn't support write-combining\n");
 		return -EOPNOTSUPP;
@@ -350,7 +338,7 @@ int mtrr_add_page(unsigned long base, un
 	/*  Search for existing MTRR  */
 	mutex_lock(&mtrr_mutex);
 	for (i = 0; i < num_var_ranges; ++i) {
-		mtrr_if->get(i, &lbase, &lsize, &ltype);
+		mtrr_get(i, &lbase, &lsize, &ltype);
 		if (!lsize || base > lbase + lsize - 1 || base + size - 1 < lbase)
 			continue;
 		/*  At this point we know there is some kind of overlap/enclosure  */
@@ -385,7 +373,7 @@ int mtrr_add_page(unsigned long base, un
 		goto out;
 	}
 	/*  Search for an empty MTRR  */
-	i = mtrr_if->get_free_region(base, size, replace);
+	i = mtrr_get_free_region(base, size, replace);
 	if (i >= 0) {
 		set_mtrr(i, base, size, type);
 		if (likely(replace < 0))
@@ -494,7 +482,7 @@ int mtrr_del_page(int reg, unsigned long
 	if (reg < 0) {
 		/*  Search for existing MTRR  */
 		for (i = 0; i < max; ++i) {
-			mtrr_if->get(i, &lbase, &lsize, &ltype);
+			mtrr_get(i, &lbase, &lsize, &ltype);
 			if (lbase == base && lsize == size) {
 				reg = i;
 				break;
@@ -510,7 +498,7 @@ int mtrr_del_page(int reg, unsigned long
 		printk(KERN_WARNING "mtrr: register: %d too big\n", reg);
 		goto out;
 	}
-	mtrr_if->get(reg, &lbase, &lsize, &ltype);
+	mtrr_get(reg, &lbase, &lsize, &ltype);
 	if (lsize < 1) {
 		printk(KERN_WARNING "mtrr: MTRR %d not used\n", reg);
 		goto out;
@@ -568,7 +556,7 @@ struct mtrr_value {
 void __init mtrr_bp_init(void)
 {
 	if (cpu_has_mtrr) {
-		mtrr_if = &generic_mtrr_ops;
+		mtrr_if = true;
 		size_or_mask = ~((1ULL << (paddr_bits - PAGE_SHIFT)) - 1);
 		size_and_mask = ~size_or_mask & 0xfffff00000ULL;
 	}
@@ -576,14 +564,13 @@ void __init mtrr_bp_init(void)
 	if (mtrr_if) {
 		set_num_var_ranges();
 		init_table();
-		if (use_intel())
-			get_mtrr_state();
+		get_mtrr_state();
 	}
 }
 
 void mtrr_ap_init(void)
 {
-	if (!mtrr_if || !use_intel() || hold_mtrr_updates_on_aps)
+	if (!mtrr_if || hold_mtrr_updates_on_aps)
 		return;
 	/*
 	 * Ideally we should hold mtrr_mutex here to avoid mtrr entries changed,
@@ -612,32 +599,25 @@ void mtrr_save_state(void)
 
 void mtrr_aps_sync_begin(void)
 {
-	if (!use_intel())
-		return;
 	hold_mtrr_updates_on_aps = 1;
 }
 
 void mtrr_aps_sync_end(void)
 {
-	if (!use_intel())
-		return;
 	set_mtrr(~0U, 0, 0, 0);
 	hold_mtrr_updates_on_aps = 0;
 }
 
 void mtrr_bp_restore(void)
 {
-	if (!use_intel())
-		return;
-	mtrr_if->set_all();
+	mtrr_set_all();
 }
 
 static int __init cf_check mtrr_init_finialize(void)
 {
 	if (!mtrr_if)
 		return 0;
-	if (use_intel())
-		mtrr_state_warn();
+	mtrr_state_warn();
 	return 0;
 }
 __initcall(mtrr_init_finialize);
--- a/xen/arch/x86/cpu/mtrr/mtrr.h
+++ b/xen/arch/x86/cpu/mtrr/mtrr.h
@@ -6,40 +6,21 @@
 #define MTRR_CHANGE_MASK_VARIABLE  0x02
 #define MTRR_CHANGE_MASK_DEFTYPE   0x04
 
-
-struct mtrr_ops {
-	u32	vendor;
-	bool	use_intel_if;
-//	void	(*init)(void);
-	void	(*set)(unsigned int reg, unsigned long base,
-		       unsigned long size, mtrr_type type);
-	void	(*set_all)(void);
-
-	void	(*get)(unsigned int reg, unsigned long *base,
-		       unsigned long *size, mtrr_type * type);
-	int	(*get_free_region)(unsigned long base, unsigned long size,
-				   int replace_reg);
-	int	(*validate_add_page)(unsigned long base, unsigned long size,
-				     unsigned int type);
-	int	(*have_wrcomb)(void);
-};
-
-int cf_check generic_get_free_region(
+void mtrr_get(
+    unsigned int reg, unsigned long *base, unsigned long *size,
+    mtrr_type *type);
+void mtrr_set(
+    unsigned int reg, unsigned long base, unsigned long size, mtrr_type type);
+void mtrr_set_all(void);
+int mtrr_get_free_region(
     unsigned long base, unsigned long size, int replace_reg);
-int cf_check generic_validate_add_page(
+int mtrr_validate_add_page(
     unsigned long base, unsigned long size, unsigned int type);
-
-extern const struct mtrr_ops generic_mtrr_ops;
+bool mtrr_have_wrcomb(void);
 
 void get_mtrr_state(void);
 
-extern void set_mtrr_ops(const struct mtrr_ops *);
-
 extern u64 size_or_mask, size_and_mask;
-extern const struct mtrr_ops *mtrr_if;
-
-#define is_cpu(vnd)	(mtrr_if && mtrr_if->vendor == X86_VENDOR_##vnd)
-#define use_intel()	(mtrr_if && mtrr_if->use_intel_if)
 
 extern unsigned int num_var_ranges;
 
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -295,7 +295,7 @@ ret_t do_platform_op(
         ret = -EINVAL;
         if ( op->u.read_memtype.reg < num_var_ranges )
         {
-            mtrr_if->get(op->u.read_memtype.reg, &mfn, &nr_mfns, &type);
+            mtrr_get(op->u.read_memtype.reg, &mfn, &nr_mfns, &type);
             op->u.read_memtype.mfn     = mfn;
             op->u.read_memtype.nr_mfns = nr_mfns;
             op->u.read_memtype.type    = type;

Re: [PATCH] x86/MTRR: avoid several indirect calls

[Andrew Cooper]

On 17/01/2024 9:32 am, Jan Beulich wrote:
> The use of (supposedly) vendor-specific hooks is a relic from the days
> when Xen was still possible to build as 32-bit binary. There's no
> expectation that a new need for such an abstraction would arise. Convert
> mttr_if to a mere boolean and all prior calls through it to direct ones,
> thus allowing to eliminate 6 ENDBR from .text.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
> Strictly speaking mtrr_if could go apparently away as well, for
> effectively mirroring cpu_has_mtrr now (once mtrr_bp_init() has run).
> Replacing the variable uses would be further code churn, though.

There's loads more cleanup I'd like to do, but that doesn't invalidate
this as an improvement.

Bloat-o-meter reports:

    add/remove: 7/8 grow/shrink: 1/14 up/down: 1354/-1803 (-449)

for this change, which definitely welcome.  That said ...

> --- a/xen/arch/x86/cpu/mtrr/generic.c
> +++ b/xen/arch/x86/cpu/mtrr/generic.c
> @@ -586,21 +586,9 @@ int cf_check generic_validate_add_page(
>  }
>  
>  
> -static int cf_check generic_have_wrcomb(void)
> +bool mtrr_have_wrcomb(void)
>  {
>  	unsigned long config;
>  	rdmsrl(MSR_MTRRcap, config);
>  	return (config & (1ULL << 10));
>  }

... I do have half a mind to fix this as a followon.  We really don't
want to be re-reading the caps MSR for every call to mtrr_add_page().

~Andrew

Re: [PATCH] x86/MTRR: avoid several indirect calls

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 636 bytes --]

On Wed, Jan 17, 2024 at 10:32:53AM +0100, Jan Beulich wrote:
> --- a/xen/arch/x86/cpu/mtrr/main.c
> +++ b/xen/arch/x86/cpu/mtrr/main.c
> @@ -328,7 +316,7 @@ int mtrr_add_page(unsigned long base, un
>  	}
>  
>  	/*  If the type is WC, check that this processor supports it  */
> -	if ((type == X86_MT_WC) && !have_wrcomb()) {
> +	if ((type == X86_MT_WC) && mtrr_have_wrcomb()) {

Is reversing the condition intentional here? 

>  		printk(KERN_WARNING
>  		       "mtrr: your processor doesn't support write-combining\n");
>  		return -EOPNOTSUPP;

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] x86/MTRR: avoid several indirect calls

[Jan Beulich]

On 23.04.2024 03:17, Marek Marczykowski-Górecki wrote:
> On Wed, Jan 17, 2024 at 10:32:53AM +0100, Jan Beulich wrote:
>> --- a/xen/arch/x86/cpu/mtrr/main.c
>> +++ b/xen/arch/x86/cpu/mtrr/main.c
>> @@ -328,7 +316,7 @@ int mtrr_add_page(unsigned long base, un
>>  	}
>>  
>>  	/*  If the type is WC, check that this processor supports it  */
>> -	if ((type == X86_MT_WC) && !have_wrcomb()) {
>> +	if ((type == X86_MT_WC) && mtrr_have_wrcomb()) {
> 
> Is reversing the condition intentional here? 

Clearly not. Patch sent.

Jan