* [PATCH] x86/apic: Use byte array apic_version[], not int array. Saves up to 96k
@ 2016-09-09 8:32 Denys Vlasenko
2016-09-11 9:31 ` Borislav Petkov
0 siblings, 1 reply; 4+ messages in thread
From: Denys Vlasenko @ 2016-09-09 8:32 UTC (permalink / raw)
To: Ingo Molnar
Cc: Denys Vlasenko, Andy Lutomirski, H. Peter Anvin, Borislav Petkov,
Brian Gerst, x86, linux-kernel
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] x86/apic: Use byte array apic_version[], not int array. Saves up to 96k
2016-09-09 8:32 [PATCH] x86/apic: Use byte array apic_version[], not int array. Saves up to 96k Denys Vlasenko
@ 2016-09-11 9:31 ` Borislav Petkov
2016-09-13 15:33 ` Thomas Gleixner
0 siblings, 1 reply; 4+ messages in thread
From: Borislav Petkov @ 2016-09-11 9:31 UTC (permalink / raw)
To: Denys Vlasenko
Cc: Ingo Molnar, Andy Lutomirski, H. Peter Anvin, Brian Gerst, x86,
linux-kernel, Mike Travis
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.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] x86/apic: Use byte array apic_version[], not int array. Saves up to 96k
2016-09-11 9:31 ` Borislav Petkov
@ 2016-09-13 15:33 ` Thomas Gleixner
2016-09-13 18:07 ` Denys Vlasenko
0 siblings, 1 reply; 4+ messages in thread
From: Thomas Gleixner @ 2016-09-13 15:33 UTC (permalink / raw)
To: Borislav Petkov
Cc: Denys Vlasenko, Ingo Molnar, Andy Lutomirski, H. Peter Anvin,
Brian Gerst, x86, linux-kernel, Mike Travis
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] x86/apic: Use byte array apic_version[], not int array. Saves up to 96k
2016-09-13 15:33 ` Thomas Gleixner
@ 2016-09-13 18:07 ` Denys Vlasenko
0 siblings, 0 replies; 4+ messages in thread
From: Denys Vlasenko @ 2016-09-13 18:07 UTC (permalink / raw)
To: Thomas Gleixner, Borislav Petkov
Cc: Ingo Molnar, Andy Lutomirski, H. Peter Anvin, Brian Gerst, x86,
linux-kernel, Mike Travis
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-09-13 18:07 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-09 8:32 [PATCH] x86/apic: Use byte array apic_version[], not int array. Saves up to 96k Denys Vlasenko
2016-09-11 9:31 ` Borislav Petkov
2016-09-13 15:33 ` Thomas Gleixner
2016-09-13 18:07 ` Denys Vlasenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).