[PATCH] x86/apic: Use byte array apic_version[], not int array. Saves up to 96k

[PATCH] x86/apic: Use byte array apic_version[], not int array. Saves up to 96k

[Denys Vlasenko]

This array is [MAX_LOCAL_APIC], and MAX_LOCAL_APIC can easily be up to 32k.

This patch changes apic_version[] array elements from int to u8 -
APIC version values as of year 2016 are no larger than 0x1f on all known CPUs.
Version field in the APIC register is 8 bit wide - not likely
to overflow byte range in foreseeable future.

The "ver" argument of generic_processor_info(id,ver), which goes into apic_version[id],
is also changed from int to u8: make it obvious that assignment can't overflow.

generic_processor_info() has four callsites, none of them can put an out-of-range value
into this argument.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: Ingo Molnar <mingo@kernel.org>
CC: Andy Lutomirski <luto@amacapital.net>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Borislav Petkov <bp@alien8.de>
CC: Brian Gerst <brgerst@gmail.com>
CC: x86@kernel.org
CC: linux-kernel@vger.kernel.org
---
 arch/x86/include/asm/mpspec.h | 4 ++--
 arch/x86/kernel/acpi/boot.c   | 2 +-
 arch/x86/kernel/apic/apic.c   | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/mpspec.h b/arch/x86/include/asm/mpspec.h
index b07233b..a0bc349 100644
--- a/arch/x86/include/asm/mpspec.h
+++ b/arch/x86/include/asm/mpspec.h
@@ -6,7 +6,7 @@
 #include <asm/x86_init.h>
 #include <asm/apicdef.h>
 
-extern int apic_version[];
+extern u8 apic_version[];
 extern int pic_mode;
 
 #ifdef CONFIG_X86_32
@@ -85,7 +85,7 @@ static inline void early_reserve_e820_mpc_new(void) { }
 #define default_get_smp_config x86_init_uint_noop
 #endif
 
-int generic_processor_info(int apicid, int version);
+int generic_processor_info(int apicid, u8 version);
 
 #define PHYSID_ARRAY_SIZE	BITS_TO_LONGS(MAX_LOCAL_APIC)
 
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 90d84c3..fde236f 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -168,7 +168,7 @@ static int __init acpi_parse_madt(struct acpi_table_header *table)
  */
 static int acpi_register_lapic(int id, u32 acpiid, u8 enabled)
 {
-	unsigned int ver = 0;
+	u8 ver = 0;
 	int cpu;
 
 	if (id >= MAX_LOCAL_APIC) {
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 50c95af..d5cc7c6 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1837,7 +1837,7 @@ void __init register_lapic_address(unsigned long address)
 	}
 }
 
-int apic_version[MAX_LOCAL_APIC];
+u8 apic_version[MAX_LOCAL_APIC];
 
 /*
  * Local APIC interrupts
@@ -2027,7 +2027,7 @@ void disconnect_bsp_APIC(int virt_wire_setup)
 	apic_write(APIC_LVT1, value);
 }
 
-int generic_processor_info(int apicid, int version)
+int generic_processor_info(int apicid, u8 version)
 {
 	int cpu, max = nr_cpu_ids;
 	bool boot_cpu_detected = physid_isset(boot_cpu_physical_apicid,
-- 
2.9.2

Re: [PATCH] x86/apic: Use byte array apic_version[], not int array. Saves up to 96k

[Borislav Petkov]

On Fri, Sep 09, 2016 at 10:32:04AM +0200, Denys Vlasenko wrote:
> This array is [MAX_LOCAL_APIC], and MAX_LOCAL_APIC can easily be up to 32k.
> 
> This patch changes apic_version[] array elements from int to u8 -
> APIC version values as of year 2016 are no larger than 0x1f on all known CPUs.
> Version field in the APIC register is 8 bit wide - not likely
> to overflow byte range in foreseeable future.
> 
> The "ver" argument of generic_processor_info(id,ver), which goes into apic_version[id],
> is also changed from int to u8: make it obvious that assignment can't overflow.
> 
> generic_processor_info() has four callsites, none of them can put an out-of-range value
> into this argument.

Right, so I dug a bit into this and found:

http://marc.info/?l=linux-kernel&m=123230551709711

and

b2b815d80a5c ("x86: put trigger in to detect mismatched apic versions")

It is from 2009 and I don't know how relevant 16-bit APIC IDs are
anymore... I guess you probably want to run this by SGI folk first.

Otherwise I was going to propose to kill that apic_version array
altogether and cache only the version of the previous CPU and compare it
to the current one to catch mismatches...

Adding Mike to CC and leaving in the rest for reference.

> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> CC: Ingo Molnar <mingo@kernel.org>
> CC: Andy Lutomirski <luto@amacapital.net>
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: Borislav Petkov <bp@alien8.de>
> CC: Brian Gerst <brgerst@gmail.com>
> CC: x86@kernel.org
> CC: linux-kernel@vger.kernel.org
> ---
>  arch/x86/include/asm/mpspec.h | 4 ++--
>  arch/x86/kernel/acpi/boot.c   | 2 +-
>  arch/x86/kernel/apic/apic.c   | 4 ++--
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/include/asm/mpspec.h b/arch/x86/include/asm/mpspec.h
> index b07233b..a0bc349 100644
> --- a/arch/x86/include/asm/mpspec.h
> +++ b/arch/x86/include/asm/mpspec.h
> @@ -6,7 +6,7 @@
>  #include <asm/x86_init.h>
>  #include <asm/apicdef.h>
>  
> -extern int apic_version[];
> +extern u8 apic_version[];
>  extern int pic_mode;
>  
>  #ifdef CONFIG_X86_32
> @@ -85,7 +85,7 @@ static inline void early_reserve_e820_mpc_new(void) { }
>  #define default_get_smp_config x86_init_uint_noop
>  #endif
>  
> -int generic_processor_info(int apicid, int version);
> +int generic_processor_info(int apicid, u8 version);
>  
>  #define PHYSID_ARRAY_SIZE	BITS_TO_LONGS(MAX_LOCAL_APIC)
>  
> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> index 90d84c3..fde236f 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -168,7 +168,7 @@ static int __init acpi_parse_madt(struct acpi_table_header *table)
>   */
>  static int acpi_register_lapic(int id, u32 acpiid, u8 enabled)
>  {
> -	unsigned int ver = 0;
> +	u8 ver = 0;
>  	int cpu;
>  
>  	if (id >= MAX_LOCAL_APIC) {
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index 50c95af..d5cc7c6 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -1837,7 +1837,7 @@ void __init register_lapic_address(unsigned long address)
>  	}
>  }
>  
> -int apic_version[MAX_LOCAL_APIC];
> +u8 apic_version[MAX_LOCAL_APIC];
>  
>  /*
>   * Local APIC interrupts
> @@ -2027,7 +2027,7 @@ void disconnect_bsp_APIC(int virt_wire_setup)
>  	apic_write(APIC_LVT1, value);
>  }
>  
> -int generic_processor_info(int apicid, int version)
> +int generic_processor_info(int apicid, u8 version)
>  {
>  	int cpu, max = nr_cpu_ids;
>  	bool boot_cpu_detected = physid_isset(boot_cpu_physical_apicid,
> -- 

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH] x86/apic: Use byte array apic_version[], not int array. Saves up to 96k

[Thomas Gleixner]

On Sun, 11 Sep 2016, Borislav Petkov wrote:
> On Fri, Sep 09, 2016 at 10:32:04AM +0200, Denys Vlasenko wrote:
> > This array is [MAX_LOCAL_APIC], and MAX_LOCAL_APIC can easily be up to 32k.
> > 
> > This patch changes apic_version[] array elements from int to u8 -
> > APIC version values as of year 2016 are no larger than 0x1f on all known CPUs.
> > Version field in the APIC register is 8 bit wide - not likely
> > to overflow byte range in foreseeable future.
> > 
> > The "ver" argument of generic_processor_info(id,ver), which goes into apic_version[id],
> > is also changed from int to u8: make it obvious that assignment can't overflow.
> > 
> > generic_processor_info() has four callsites, none of them can put an out-of-range value
> > into this argument.
> 
> Right, so I dug a bit into this and found:
> 
> http://marc.info/?l=linux-kernel&m=123230551709711
> 
> and
> 
> b2b815d80a5c ("x86: put trigger in to detect mismatched apic versions")
> 
> It is from 2009 and I don't know how relevant 16-bit APIC IDs are
> anymore... I guess you probably want to run this by SGI folk first.
> 
> Otherwise I was going to propose to kill that apic_version array
> altogether and cache only the version of the previous CPU and compare it
> to the current one to catch mismatches...

Yeah, the idea was back then to eliminate the array, but we wanted to make
sure that we don't have systems out in the wild which have different apic
versions. I really doubt that we can deal with that proper, so having a
single version entry and yelling loudly when we detect a mismatch is good
enough.

Thanks,

	tglx

Re: [PATCH] x86/apic: Use byte array apic_version[], not int array. Saves up to 96k

[Denys Vlasenko]

On 09/13/2016 05:33 PM, Thomas Gleixner wrote:
> On Sun, 11 Sep 2016, Borislav Petkov wrote:
>> On Fri, Sep 09, 2016 at 10:32:04AM +0200, Denys Vlasenko wrote:
>>> This array is [MAX_LOCAL_APIC], and MAX_LOCAL_APIC can easily be up to 32k.
>>>
>>> This patch changes apic_version[] array elements from int to u8 -
>>> APIC version values as of year 2016 are no larger than 0x1f on all known CPUs.
>>> Version field in the APIC register is 8 bit wide - not likely
>>> to overflow byte range in foreseeable future.
>>>
>>> The "ver" argument of generic_processor_info(id,ver), which goes into apic_version[id],
>>> is also changed from int to u8: make it obvious that assignment can't overflow.
>>>
>>> generic_processor_info() has four callsites, none of them can put an out-of-range value
>>> into this argument.
>>
>> Right, so I dug a bit into this and found:
>>
>> http://marc.info/?l=linux-kernel&m=123230551709711
>>
>> and
>>
>> b2b815d80a5c ("x86: put trigger in to detect mismatched apic versions")
>>
>> It is from 2009 and I don't know how relevant 16-bit APIC IDs are
>> anymore... I guess you probably want to run this by SGI folk first.
>>
>> Otherwise I was going to propose to kill that apic_version array
>> altogether and cache only the version of the previous CPU and compare it
>> to the current one to catch mismatches...
>
> Yeah, the idea was back then to eliminate the array, but we wanted to make
> sure that we don't have systems out in the wild which have different apic
> versions. I really doubt that we can deal with that proper, so having a
> single version entry and yelling loudly when we detect a mismatch is good
> enough.

Makes sense. I'll send a patch