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

2024-04-23 Thread 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




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

2024-04-22 Thread Marek Marczykowski-Górecki
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


signature.asc
Description: PGP signature


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

2024-01-17 Thread 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 

Reviewed-by: Andrew Cooper 

> ---
> 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



[PATCH] x86/MTRR: avoid several indirect calls

2024-01-17 Thread 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 
---
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.
  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, , , );
+   mtrr_get(i, , , );
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(_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.
  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(>count);
while(atomic_read(>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.
  *