linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH] x86/acpi: Prevent x2apic id -1 from being accounted
@ 2018-04-08 11:38 Li RongQing
  2018-04-09  5:37 ` Dou Liyang
  0 siblings, 1 reply; 5+ messages in thread
From: Li RongQing @ 2018-04-08 11:38 UTC (permalink / raw)
  To: linux-kernel, tglx, mingo, hpa, jgross, douly.fnst, x86, peterz

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

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 <lirongqing@baidu.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Dou Liyang <douly.fnst@cn.fujitsu.com>
---
 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");
-	else
-		acpi_register_lapic(apic_id, processor->uid, enabled);
+		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;
 }
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [RFC PATCH] x86/acpi: Prevent x2apic id -1 from being accounted
  2018-04-08 11:38 [RFC PATCH] x86/acpi: Prevent x2apic id -1 from being accounted Li RongQing
@ 2018-04-09  5:37 ` Dou Liyang
       [not found]   ` <2AD939572F25A448A3AE3CAEA61328C23758FDF3@BC-MAIL-M28.internal.baidu.com>
  0 siblings, 1 reply; 5+ messages in thread
From: Dou Liyang @ 2018-04-09  5:37 UTC (permalink / raw)
  To: Li RongQing, linux-kernel, tglx, mingo, hpa, jgross, x86, peterz

Hi RongQing,

Is there an local x2apic whose ID is 0xFFFFFFFF in your machine?

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

   - the ->apic_id_valid points to x2apic_apic_id_valid(), which always
     return _ture_ , not default_apic_id_valid().

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 <lirongqing@baidu.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Dou Liyang <douly.fnst@cn.fujitsu.com>
> ---
>   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");
> -	else
> -		acpi_register_lapic(apic_id, processor->uid, enabled);
> +		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;
>   }
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: 答复: [RFC PATCH] x86/acpi: Prevent x2apic id -1 from being accounted
       [not found]   ` <2AD939572F25A448A3AE3CAEA61328C23758FDF3@BC-MAIL-M28.internal.baidu.com>
@ 2018-04-09  8:46     ` Dou Liyang
  2018-04-09  9:33       ` 答复: " Li,Rongqing
  0 siblings, 1 reply; 5+ messages in thread
From: Dou Liyang @ 2018-04-09  8:46 UTC (permalink / raw)
  To: Li,Rongqing, linux-kernel, tglx, mingo, hpa, jgross, x86, peterz

RongQing,

At 04/09/2018 02:38 PM, Li,Rongqing wrote:
> 
> 
>> -----邮件原件-----
>> 发件人: Dou Liyang [mailto:douly.fnst@cn.fujitsu.com]
>> 发送时间: 2018年4月9日 13:38
>> 收件人: Li,Rongqing <lirongqing@baidu.com>; linux-kernel@vger.kernel.org;
>> tglx@linutronix.de; mingo@redhat.com; hpa@zytor.com; jgross@suse.com;
>> x86@kernel.org; peterz@infradead.org
>> 主题: 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 <lirongqing@baidu.com>
>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> Cc: Dou Liyang <douly.fnst@cn.fujitsu.com>
>>> ---
>>>    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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* 答复: 答复: [RFC PATCH] x86/acpi: Prevent x2apic id -1 from being accounted
  2018-04-09  8:46     ` 答复: " Dou Liyang
@ 2018-04-09  9:33       ` Li,Rongqing
  2018-04-09 12:16         ` Dou Liyang
  0 siblings, 1 reply; 5+ messages in thread
From: Li,Rongqing @ 2018-04-09  9:33 UTC (permalink / raw)
  To: Dou Liyang, linux-kernel, tglx, mingo, hpa, jgross, x86, peterz



> -----邮件原件-----
> 发件人: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] 代表 Dou Liyang
> 发送时间: 2018年4月9日 16:47
> 收件人: Li,Rongqing <lirongqing@baidu.com>; linux-kernel@vger.kernel.org;
> tglx@linutronix.de; mingo@redhat.com; hpa@zytor.com; jgross@suse.com;
> x86@kernel.org; peterz@infradead.org
> 主题: 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:douly.fnst@cn.fujitsu.com]
> >> 发送时间: 2018年4月9日 13:38
> >> 收件人: Li,Rongqing <lirongqing@baidu.com>;
> >> linux-kernel@vger.kernel.org; tglx@linutronix.de; mingo@redhat.com;
> >> hpa@zytor.com; jgross@suse.com; x86@kernel.org;
> peterz@infradead.org
> >> 主题: 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 <lirongqing@baidu.com>
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 <lirongqing@baidu.com>
    Suggested-by:: Dou Liyang <douly.fnst@cn.fujitsu.com>
    Cc: Peter Zijlstra <peterz@infradead.org>
    Cc: Thomas Gleixner <tglx@linutronix.de>

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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: 答复: 答复: [RFC PATCH] x86/acpi: Prevent x2apic id -1 from being accounted
  2018-04-09  9:33       ` 答复: " Li,Rongqing
@ 2018-04-09 12:16         ` Dou Liyang
  0 siblings, 0 replies; 5+ messages in thread
From: Dou Liyang @ 2018-04-09 12:16 UTC (permalink / raw)
  To: Li,Rongqing, linux-kernel, tglx, mingo, hpa, jgross, x86, peterz

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 <lirongqing@baidu.com>
> 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 <lirongqing@baidu.com>
>      Suggested-by:: Dou Liyang <douly.fnst@cn.fujitsu.com>
        ^-----------^---- no need to add this, it is my pleasure


Thanks,

	dou

>      Cc: Peter Zijlstra <peterz@infradead.org>
>      Cc: Thomas Gleixner <tglx@linutronix.de>
> 
> 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;
>   }
> 
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-04-09 12:16 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-08 11:38 [RFC PATCH] x86/acpi: Prevent x2apic id -1 from being accounted Li RongQing
2018-04-09  5:37 ` Dou Liyang
     [not found]   ` <2AD939572F25A448A3AE3CAEA61328C23758FDF3@BC-MAIL-M28.internal.baidu.com>
2018-04-09  8:46     ` 答复: " Dou Liyang
2018-04-09  9:33       ` 答复: " Li,Rongqing
2018-04-09 12:16         ` Dou Liyang

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