RongQing,
At 04/09/2018 02:38 PM, Li,Rongqing wrote:
>
>
>> -----邮件原件-----
>> 发件人: Dou Liyang [mailto:[email protected]]
>> 发送时间: 2018年4月9日 13:38
>> 收件人: Li,Rongqing <[email protected]>; [email protected];
>> [email protected]; [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]
>> 主题: Re: [RFC PATCH] x86/acpi: Prevent x2apic id -1 from being accounted
>>
>> Hi RongQing,
>>
>> Is there an local x2apic whose ID is in your machine?
>>
>
> I think no
[...]
> [ 0.000000] ACPI: X2APIC (apic_id[0xffffffff] uid[0x00] disabled)
> [ 0.000000] ACPI: X2APIC (apic_id[0xffffffff] uid[0x01] disabled)
> [ 0.000000] ACPI: X2APIC (apic_id[0xffffffff] uid[0x02] disabled)
Ah, sure enough!
[...]
>
>
>> At 04/08/2018 07:38 PM, Li RongQing wrote:
>>> local_apic_id of acpi_madt_local_x2apic is u32, it is converted to int
>>> when checked by default_apic_id_valid() and return true if it is
>>> larger than 0x7fffffff, this is wrong
>>>
>>
>> For x2apic enabled systems,
>>
>> - the byte length of X2APIC ID is 4, and it can be larger than
>> 0x7fffffff in theory
>>
>
> Yes
>
>> - the ->apic_id_valid points to x2apic_apic_id_valid(), which always
>> return _ture_ , not default_apic_id_valid().
>>
>
> To this machine, the apic is apic_flat
I see, I am sorry the title and changelog make me misunderstand.
Here, actually, we prevent all the X2APIC Id from being parsed in
non-x2apic mode, not just 0xFFFFFFFF. because the values of x2APIC ID
must be 255 and greater in ACPI MADT.
>
> -RongQing
>
>> Thanks,
>> dou
>>
>>> and if local_apic_id is invalid, we should prevent it from being
>>> accounted > This fixes a bug that Purley platform displays too many
>>> possible cpu
>>>
>>> Signed-off-by: Li RongQing <[email protected]>
>>> Cc: Peter Zijlstra <[email protected]>
>>> Cc: Thomas Gleixner <[email protected]>
>>> Cc: Dou Liyang <[email protected]>
>>> ---
>>> arch/x86/include/asm/apic.h | 4 ++--
>>> arch/x86/kernel/acpi/boot.c | 10 ++++++----
>>> arch/x86/kernel/apic/apic_common.c | 2 +-
>>> arch/x86/kernel/apic/apic_numachip.c | 2 +-
>>> arch/x86/kernel/apic/x2apic.h | 2 +-
>>> arch/x86/kernel/apic/x2apic_phys.c | 2 +-
>>> arch/x86/kernel/apic/x2apic_uv_x.c | 2 +-
>>> arch/x86/xen/apic.c | 2 +-
>>> 8 files changed, 14 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
>>> index 40a3d3642f3a..08acd954f00e 100644
>>> --- a/arch/x86/include/asm/apic.h
>>> +++ b/arch/x86/include/asm/apic.h
>>> @@ -313,7 +313,7 @@ struct apic {
>>> /* Probe, setup and smpboot functions */
>>> int (*probe)(void);
>>> int (*acpi_madt_oem_check)(char *oem_id, char
>> *oem_table_id);
>>> - int (*apic_id_valid)(int apicid);
>>> + int (*apic_id_valid)(u32 apicid);
>>> int (*apic_id_registered)(void);
>>>
>>> bool (*check_apicid_used)(physid_mask_t *map, int apicid);
>>> @@ -486,7 +486,7 @@ static inline unsigned int read_apic_id(void)
>>> return apic->get_apic_id(reg);
>>> }
>>>
>>> -extern int default_apic_id_valid(int apicid);
>>> +extern int default_apic_id_valid(u32 apicid);
>>> extern int default_acpi_madt_oem_check(char *, char *);
>>> extern void default_setup_apic_routing(void);
>>>
>>> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
>>> index 7a37d9357bc4..7412564dc2a7 100644
>>> --- a/arch/x86/kernel/acpi/boot.c
>>> +++ b/arch/x86/kernel/acpi/boot.c
>>> @@ -200,7 +200,7 @@ acpi_parse_x2apic(struct acpi_subtable_header
>> *header, const unsigned long end)
>>> {
>>> struct acpi_madt_local_x2apic *processor = NULL;
>>> #ifdef CONFIG_X86_X2APIC
>>> - int apic_id;
>>> + u32 apic_id;
>>> u8 enabled;
>>> #endif
>>>
>>> @@ -222,10 +222,12 @@ acpi_parse_x2apic(struct acpi_subtable_header
>> *header, const unsigned long end)
>>> * to not preallocating memory for all NR_CPUS
>>> * when we use CPU hotplug.
>>> */
>>> - if (!apic->apic_id_valid(apic_id) && enabled)
>>> + if (!apic->apic_id_valid(apic_id)) {
>>> printk(KERN_WARNING PREFIX "x2apic entry ignored\n");
Due to the disable APIC entries may not exist physically, please just
printk the warning if the APIC ID is enabled.
if (!apic->apic_id_valid(apic_id)) {
if(enabled)
printk...
return 0;
}
Thanks,
dou
> -----邮件原件-----
> 发件人: [email protected] [mailto:linux-kernel-
> [email protected]] 代表 Dou Liyang
> 发送时间: 2018年4月9日 16:47
> 收件人: Li,Rongqing <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> 主题: Re: 答复: [RFC PATCH] x86/acpi: Prevent x2apic id -1 from being
> accounted
>
> RongQing,
>
> At 04/09/2018 02:38 PM, Li,Rongqing wrote:
> >
> >
> >> -----邮件原件-----
> >> 发件人: Dou Liyang [mailto:[email protected]]
> >> 发送时间: 2018年4月9日 13:38
> >> 收件人: Li,Rongqing <[email protected]>;
> >> [email protected]; [email protected]; [email protected];
> >> [email protected]; [email protected]; [email protected];
> [email protected]
> >> 主题: Re: [RFC PATCH] x86/acpi: Prevent x2apic id -1 from being
> >> accounted
> >>
> >> Hi RongQing,
> >>
> >> Is there an local x2apic whose ID is in your machine?
> >>
> >
> > I think no
>
> [...]
>
> > [ 0.000000] ACPI: X2APIC (apic_id[0xffffffff] uid[0x00] disabled)
> > [ 0.000000] ACPI: X2APIC (apic_id[0xffffffff] uid[0x01] disabled)
> > [ 0.000000] ACPI: X2APIC (apic_id[0xffffffff] uid[0x02] disabled)
>
> Ah, sure enough!
>
> [...]
> >
> >
> >> At 04/08/2018 07:38 PM, Li RongQing wrote:
> >>> local_apic_id of acpi_madt_local_x2apic is u32, it is converted to
> >>> int when checked by default_apic_id_valid() and return true if it is
> >>> larger than 0x7fffffff, this is wrong
> >>>
> >>
> >> For x2apic enabled systems,
> >>
> >> - the byte length of X2APIC ID is 4, and it can be larger than
> >> 0x7fffffff in theory
> >>
> >
> > Yes
> >
> >> - the ->apic_id_valid points to x2apic_apic_id_valid(), which always
> >> return _ture_ , not default_apic_id_valid().
> >>
> >
> > To this machine, the apic is apic_flat
>
> I see, I am sorry the title and changelog make me misunderstand.
>
> Here, actually, we prevent all the X2APIC Id from being parsed in
> non-x2apic mode, not just 0xFFFFFFFF. because the values of x2APIC ID
> must be 255 and greater in ACPI MADT.
>
Thanks. How about the below modification:
commit 96ba42cf87ce0e62d500004c01bfa9a9479b2e87
Author: Li RongQing <[email protected]>
Date: Sun Apr 8 18:54:10 2018 +0800
x86/acpi: Prevent all the X2APIC Id from being parsed in non-x2apic mode
the values of x2APIC ID is greater than 0xff in ACPI MADT, if acpi
is apic_flat, default_apic_id_valid() is called to check id which
is converted from u32 to int, will return true if id is larger than
0x7fffffff, this is wrong
and if local_apic_id is invalid, we should prevent it from being
accounted
This fixes a bug that Purley platform displays too many possible cpu
Signed-off-by: Li RongQing <[email protected]>
Suggested-by:: Dou Liyang <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 40a3d3642f3a..08acd954f00e 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -313,7 +313,7 @@ struct apic {
/* Probe, setup and smpboot functions */
int (*probe)(void);
int (*acpi_madt_oem_check)(char *oem_id, char *oem_table_id);
- int (*apic_id_valid)(int apicid);
+ int (*apic_id_valid)(u32 apicid);
int (*apic_id_registered)(void);
bool (*check_apicid_used)(physid_mask_t *map, int apicid);
@@ -486,7 +486,7 @@ static inline unsigned int read_apic_id(void)
return apic->get_apic_id(reg);
}
-extern int default_apic_id_valid(int apicid);
+extern int default_apic_id_valid(u32 apicid);
extern int default_acpi_madt_oem_check(char *, char *);
extern void default_setup_apic_routing(void);
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 7a37d9357bc4..4ba949de1ca9 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -200,7 +200,7 @@ acpi_parse_x2apic(struct acpi_subtable_header *header, const unsigned long end)
{
struct acpi_madt_local_x2apic *processor = NULL;
#ifdef CONFIG_X86_X2APIC
- int apic_id;
+ u32 apic_id;
u8 enabled;
#endif
@@ -222,10 +222,13 @@ acpi_parse_x2apic(struct acpi_subtable_header *header, const unsigned long end)
* to not preallocating memory for all NR_CPUS
* when we use CPU hotplug.
*/
- if (!apic->apic_id_valid(apic_id) && enabled)
- printk(KERN_WARNING PREFIX "x2apic entry ignored\n");
- else
- acpi_register_lapic(apic_id, processor->uid, enabled);
+ if (!apic->apic_id_valid(apic_id)) {
+ if (enabled)
+ printk(KERN_WARNING PREFIX "x2apic entry ignored\n");
+ return 0;
+ }
+
+ acpi_register_lapic(apic_id, processor->uid, enabled);
#else
printk(KERN_WARNING PREFIX "x2apic entry ignored\n");
#endif
diff --git a/arch/x86/kernel/apic/apic_common.c b/arch/x86/kernel/apic/apic_common.c
index a360801779ae..02b4839478b1 100644
--- a/arch/x86/kernel/apic/apic_common.c
+++ b/arch/x86/kernel/apic/apic_common.c
@@ -40,7 +40,7 @@ int default_check_phys_apicid_present(int phys_apicid)
return physid_isset(phys_apicid, phys_cpu_present_map);
}
-int default_apic_id_valid(int apicid)
+int default_apic_id_valid(u32 apicid)
{
return (apicid < 255);
}
diff --git a/arch/x86/kernel/apic/apic_numachip.c b/arch/x86/kernel/apic/apic_numachip.c
index 134e04506ab4..78778b54f904 100644
--- a/arch/x86/kernel/apic/apic_numachip.c
+++ b/arch/x86/kernel/apic/apic_numachip.c
@@ -56,7 +56,7 @@ static u32 numachip2_set_apic_id(unsigned int id)
return id << 24;
}
-static int numachip_apic_id_valid(int apicid)
+static int numachip_apic_id_valid(u32 apicid)
{
/* Trust what bootloader passes in MADT */
return 1;
diff --git a/arch/x86/kernel/apic/x2apic.h b/arch/x86/kernel/apic/x2apic.h
index b107de381cb5..a49b3604027f 100644
--- a/arch/x86/kernel/apic/x2apic.h
+++ b/arch/x86/kernel/apic/x2apic.h
@@ -1,6 +1,6 @@
/* Common bits for X2APIC cluster/physical modes. */
-int x2apic_apic_id_valid(int apicid);
+int x2apic_apic_id_valid(u32 apicid);
int x2apic_apic_id_registered(void);
void __x2apic_send_IPI_dest(unsigned int apicid, int vector, unsigned int dest);
unsigned int x2apic_get_apic_id(unsigned long id);
diff --git a/arch/x86/kernel/apic/x2apic_phys.c b/arch/x86/kernel/apic/x2apic_phys.c
index e2829bf40e4a..b5cf9e7b3830 100644
--- a/arch/x86/kernel/apic/x2apic_phys.c
+++ b/arch/x86/kernel/apic/x2apic_phys.c
@@ -101,7 +101,7 @@ static int x2apic_phys_probe(void)
}
/* Common x2apic functions, also used by x2apic_cluster */
-int x2apic_apic_id_valid(int apicid)
+int x2apic_apic_id_valid(u32 apicid)
{
return 1;
}
diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
index f11910b44638..efaf2d4f9c3c 100644
--- a/arch/x86/kernel/apic/x2apic_uv_x.c
+++ b/arch/x86/kernel/apic/x2apic_uv_x.c
@@ -557,7 +557,7 @@ static void uv_send_IPI_all(int vector)
uv_send_IPI_mask(cpu_online_mask, vector);
}
-static int uv_apic_id_valid(int apicid)
+static int uv_apic_id_valid(u32 apicid)
{
return 1;
}
diff --git a/arch/x86/xen/apic.c b/arch/x86/xen/apic.c
index 2163888497d3..5e53bfbe5823 100644
--- a/arch/x86/xen/apic.c
+++ b/arch/x86/xen/apic.c
@@ -112,7 +112,7 @@ static int xen_madt_oem_check(char *oem_id, char *oem_table_id)
return xen_pv_domain();
}
-static int xen_id_always_valid(int apicid)
+static int xen_id_always_valid(u32 apicid)
{
return 1;
}
Hi Rongqing,
At 04/09/2018 05:33 PM, Li,Rongqing wrote:
[...]
> Thanks. How about the below modification:
>
It is good to me. you need start a new v2 thread for this. so
that maintainers can easy to get it.
> commit 96ba42cf87ce0e62d500004c01bfa9a9479b2e87
> Author: Li RongQing <[email protected]>
> Date: Sun Apr 8 18:54:10 2018 +0800
>
> x86/acpi: Prevent all the X2APIC Id from being parsed in non-x2apic mode
>
> the values of x2APIC ID is greater than 0xff in ACPI MADT, if acpi
> is apic_flat, default_apic_id_valid() is called to check id which
> is converted from u32 to int, will return true if id is larger than
> 0x7fffffff, this is wrong
>
> and if local_apic_id is invalid, we should prevent it from being
> accounted
>
> This fixes a bug that Purley platform displays too many possible cpu
>
> Signed-off-by: Li RongQing <[email protected]>
> Suggested-by:: Dou Liyang <[email protected]>
^-----------^---- no need to add this, it is my pleasure
Thanks,
dou
> Cc: Peter Zijlstra <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
>
> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> index 40a3d3642f3a..08acd954f00e 100644
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -313,7 +313,7 @@ struct apic {
> /* Probe, setup and smpboot functions */
> int (*probe)(void);
> int (*acpi_madt_oem_check)(char *oem_id, char *oem_table_id);
> - int (*apic_id_valid)(int apicid);
> + int (*apic_id_valid)(u32 apicid);
> int (*apic_id_registered)(void);
>
> bool (*check_apicid_used)(physid_mask_t *map, int apicid);
> @@ -486,7 +486,7 @@ static inline unsigned int read_apic_id(void)
> return apic->get_apic_id(reg);
> }
>
> -extern int default_apic_id_valid(int apicid);
> +extern int default_apic_id_valid(u32 apicid);
> extern int default_acpi_madt_oem_check(char *, char *);
> extern void default_setup_apic_routing(void);
>
> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> index 7a37d9357bc4..4ba949de1ca9 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -200,7 +200,7 @@ acpi_parse_x2apic(struct acpi_subtable_header *header, const unsigned long end)
> {
> struct acpi_madt_local_x2apic *processor = NULL;
> #ifdef CONFIG_X86_X2APIC
> - int apic_id;
> + u32 apic_id;
> u8 enabled;
> #endif
>
> @@ -222,10 +222,13 @@ acpi_parse_x2apic(struct acpi_subtable_header *header, const unsigned long end)
> * to not preallocating memory for all NR_CPUS
> * when we use CPU hotplug.
> */
> - if (!apic->apic_id_valid(apic_id) && enabled)
> - printk(KERN_WARNING PREFIX "x2apic entry ignored\n");
> - else
> - acpi_register_lapic(apic_id, processor->uid, enabled);
> + if (!apic->apic_id_valid(apic_id)) {
> + if (enabled)
> + printk(KERN_WARNING PREFIX "x2apic entry ignored\n");
> + return 0;
> + }
> +
> + acpi_register_lapic(apic_id, processor->uid, enabled);
> #else
> printk(KERN_WARNING PREFIX "x2apic entry ignored\n");
> #endif
> diff --git a/arch/x86/kernel/apic/apic_common.c b/arch/x86/kernel/apic/apic_common.c
> index a360801779ae..02b4839478b1 100644
> --- a/arch/x86/kernel/apic/apic_common.c
> +++ b/arch/x86/kernel/apic/apic_common.c
> @@ -40,7 +40,7 @@ int default_check_phys_apicid_present(int phys_apicid)
> return physid_isset(phys_apicid, phys_cpu_present_map);
> }
>
> -int default_apic_id_valid(int apicid)
> +int default_apic_id_valid(u32 apicid)
> {
> return (apicid < 255);
> }
> diff --git a/arch/x86/kernel/apic/apic_numachip.c b/arch/x86/kernel/apic/apic_numachip.c
> index 134e04506ab4..78778b54f904 100644
> --- a/arch/x86/kernel/apic/apic_numachip.c
> +++ b/arch/x86/kernel/apic/apic_numachip.c
> @@ -56,7 +56,7 @@ static u32 numachip2_set_apic_id(unsigned int id)
> return id << 24;
> }
>
> -static int numachip_apic_id_valid(int apicid)
> +static int numachip_apic_id_valid(u32 apicid)
> {
> /* Trust what bootloader passes in MADT */
> return 1;
> diff --git a/arch/x86/kernel/apic/x2apic.h b/arch/x86/kernel/apic/x2apic.h
> index b107de381cb5..a49b3604027f 100644
> --- a/arch/x86/kernel/apic/x2apic.h
> +++ b/arch/x86/kernel/apic/x2apic.h
> @@ -1,6 +1,6 @@
> /* Common bits for X2APIC cluster/physical modes. */
>
> -int x2apic_apic_id_valid(int apicid);
> +int x2apic_apic_id_valid(u32 apicid);
> int x2apic_apic_id_registered(void);
> void __x2apic_send_IPI_dest(unsigned int apicid, int vector, unsigned int dest);
> unsigned int x2apic_get_apic_id(unsigned long id);
> diff --git a/arch/x86/kernel/apic/x2apic_phys.c b/arch/x86/kernel/apic/x2apic_phys.c
> index e2829bf40e4a..b5cf9e7b3830 100644
> --- a/arch/x86/kernel/apic/x2apic_phys.c
> +++ b/arch/x86/kernel/apic/x2apic_phys.c
> @@ -101,7 +101,7 @@ static int x2apic_phys_probe(void)
> }
>
> /* Common x2apic functions, also used by x2apic_cluster */
> -int x2apic_apic_id_valid(int apicid)
> +int x2apic_apic_id_valid(u32 apicid)
> {
> return 1;
> }
> diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
> index f11910b44638..efaf2d4f9c3c 100644
> --- a/arch/x86/kernel/apic/x2apic_uv_x.c
> +++ b/arch/x86/kernel/apic/x2apic_uv_x.c
> @@ -557,7 +557,7 @@ static void uv_send_IPI_all(int vector)
> uv_send_IPI_mask(cpu_online_mask, vector);
> }
>
> -static int uv_apic_id_valid(int apicid)
> +static int uv_apic_id_valid(u32 apicid)
> {
> return 1;
> }
> diff --git a/arch/x86/xen/apic.c b/arch/x86/xen/apic.c
> index 2163888497d3..5e53bfbe5823 100644
> --- a/arch/x86/xen/apic.c
> +++ b/arch/x86/xen/apic.c
> @@ -112,7 +112,7 @@ static int xen_madt_oem_check(char *oem_id, char *oem_table_id)
> return xen_pv_domain();
> }
>
> -static int xen_id_always_valid(int apicid)
> +static int xen_id_always_valid(u32 apicid)
> {
> return 1;
> }
>
>