diff mbox series

[v5,09/15] x86/mtrr: allocate mtrr_value array dynamically

Message ID 20230401063652.23522-10-jgross@suse.com
State New
Headers
Series x86/mtrr: fix handling with PAT but without MTRR |

Commit Message

Juergen Gross April 1, 2023, 6:36 a.m. UTC 
  The mtrr_value[] array is a static variable, which is used only in a
few configurations. Consuming 6kB is ridiculous for this case,
especially as the array doesn't need to be that large and it can easily
be allocated dynamically.

The "few configurations" are all 32-bit ones, so put the code inside a
CONFIG_X86_32 #ifdef.

Signed-off-by: Juergen Gross <jgross@suse.com>
Tested-by: Michael Kelley <mikelley@microsoft.com>
---
V5:
- check for allocation failure (Kai Huang, Boris Petkov)
- add #ifdef
---
 arch/x86/kernel/cpu/mtrr/mtrr.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

Comments

Borislav Petkov April 12, 2023, 9:11 p.m. UTC | #1 
On Sat, Apr 01, 2023 at 08:36:46AM +0200, Juergen Gross wrote:
> +#ifdef CONFIG_X86_32

TBH, I'm not really crazy about adding more ifdeffery.

Wondering if adding a 32-bit only build object:

obj-$(CONFIG_X86_32) += amd.o cyrix.o centaur.o legacy.o

to arch/x86/kernel/cpu/mtrr/Makefile and moving all that gunk over
there, out of the way, would be even cleaner...

> +	mtrr_value = kcalloc(num_var_ranges, sizeof(*mtrr_value), GFP_KERNEL);
> +
>  	/*
>  	 * The CPU has no MTRR and seems to not support SMP. They have
>  	 * specific drivers, we use a tricky method to support
> -	 * suspend/resume for them.
> +	 * suspend/resume for them. In case above allocation failed we can't
> +	 * support suspend/resume (handled in mtrr_save()).

Oh well.
Juergen Gross April 13, 2023, 10:07 a.m. UTC | #2 
On 12.04.23 23:11, Borislav Petkov wrote:
> On Sat, Apr 01, 2023 at 08:36:46AM +0200, Juergen Gross wrote:
>> +#ifdef CONFIG_X86_32
> 
> TBH, I'm not really crazy about adding more ifdeffery.
> 
> Wondering if adding a 32-bit only build object:
> 
> obj-$(CONFIG_X86_32) += amd.o cyrix.o centaur.o legacy.o
> 
> to arch/x86/kernel/cpu/mtrr/Makefile and moving all that gunk over
> there, out of the way, would be even cleaner...

Yes, that should be rather simple.

Will add a patch to do that.


Juergen
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
index 4fa3d0f94f39..76f5b5e1128b 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.c
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
@@ -560,8 +560,10 @@  int arch_phys_wc_index(int handle)
 }
 EXPORT_SYMBOL_GPL(arch_phys_wc_index);
 
-/* The suspend/resume methods are only for CPU without MTRR. CPU using generic
- * MTRR driver doesn't require this
+#ifdef CONFIG_X86_32
+/*
+ * The suspend/resume methods are only for CPUs without MTRR. CPUs using generic
+ * MTRR driver don't require this.
  */
 struct mtrr_value {
 	mtrr_type	ltype;
@@ -569,12 +571,15 @@  struct mtrr_value {
 	unsigned long	lsize;
 };
 
-static struct mtrr_value mtrr_value[MTRR_MAX_VAR_RANGES];
+static struct mtrr_value *mtrr_value;
 
 static int mtrr_save(void)
 {
 	int i;
 
+	if (!mtrr_value)
+		return -ENOMEM;
+
 	for (i = 0; i < num_var_ranges; i++) {
 		mtrr_if->get(i, &mtrr_value[i].lbase,
 				&mtrr_value[i].lsize,
@@ -596,12 +601,11 @@  static void mtrr_restore(void)
 	}
 }
 
-
-
 static struct syscore_ops mtrr_syscore_ops = {
 	.suspend	= mtrr_save,
 	.resume		= mtrr_restore,
 };
+#endif /* CONFIG_X86_32 */
 
 int __initdata changed_by_mtrr_cleanup;
 
@@ -730,15 +734,20 @@  static int __init mtrr_init_finialize(void)
 		return 0;
 	}
 
+#ifdef CONFIG_X86_32
+	mtrr_value = kcalloc(num_var_ranges, sizeof(*mtrr_value), GFP_KERNEL);
+
 	/*
 	 * The CPU has no MTRR and seems to not support SMP. They have
 	 * specific drivers, we use a tricky method to support
-	 * suspend/resume for them.
+	 * suspend/resume for them. In case above allocation failed we can't
+	 * support suspend/resume (handled in mtrr_save()).
 	 *
 	 * TBD: is there any system with such CPU which supports
 	 * suspend/resume? If no, we should remove the code.
 	 */
 	register_syscore_ops(&mtrr_syscore_ops);
+#endif
 
 	return 0;
 }