[PATCH v3] xen/arm: Set correct per-cpu cpu_core_mask

Henry Wang posted 1 patch 1 year, 10 months ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20240321035706.165253-1-xin.wang2@amd.com
xen/arch/arm/smpboot.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
[PATCH v3] xen/arm: Set correct per-cpu cpu_core_mask
Posted by Henry Wang 1 year, 10 months ago
In the common sysctl command XEN_SYSCTL_physinfo, the value of
cores_per_socket is calculated based on the cpu_core_mask of CPU0.
Currently on Arm this is a fixed value 1 (can be checked via xl info),
which is not correct. This is because during the Arm CPU online
process at boot time, setup_cpu_sibling_map() only sets the per-cpu
cpu_core_mask for itself.

cores_per_socket refers to the number of cores that belong to the same
socket (NUMA node). Currently Xen on Arm does not support physical
CPU hotplug and NUMA, also we assume there is no multithread. Therefore
cores_per_socket means all possible CPUs detected from the device
tree. Setting the per-cpu cpu_core_mask in setup_cpu_sibling_map()
accordingly. Modify the in-code comment which seems to be outdated. Add
a warning to users if Xen is running on processors with multithread
support.

Signed-off-by: Henry Wang <Henry.Wang@arm.com>
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
v3:
- Use cpumask_copy() to set cpu_core_mask and drop the unnecessary
  cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu)).
- In-code comment adjustments.
- Add a warning for multithread.
v2:
- Do not do the multithread check.
---
 xen/arch/arm/smpboot.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index a84e706d77..b6268be27a 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -66,7 +66,11 @@ static bool cpu_is_dead;
 
 /* ID of the PCPU we're running on */
 DEFINE_PER_CPU(unsigned int, cpu_id);
-/* XXX these seem awfully x86ish... */
+/*
+ * Although multithread is part of the Arm spec, there are not many
+ * processors support multithread and current Xen on Arm assumes there
+ * is no multithread.
+ */
 /* representing HT siblings of each logical CPU */
 DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_sibling_mask);
 /* representing HT and core siblings of each logical CPU */
@@ -85,9 +89,13 @@ static int setup_cpu_sibling_map(int cpu)
          !zalloc_cpumask_var(&per_cpu(cpu_core_mask, cpu)) )
         return -ENOMEM;
 
-    /* A CPU is a sibling with itself and is always on its own core. */
+    /*
+     * Currently we assume there is no multithread and NUMA, so
+     * a CPU is a sibling with itself, and the all possible CPUs
+     * are supposed to belong to the same socket (NUMA node).
+     */
     cpumask_set_cpu(cpu, per_cpu(cpu_sibling_mask, cpu));
-    cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu));
+    cpumask_copy(per_cpu(cpu_core_mask, cpu), &cpu_possible_map);
 
     return 0;
 }
@@ -277,6 +285,10 @@ void __init smp_init_cpus(void)
         warning_add("WARNING: HMP COMPUTING HAS BEEN ENABLED.\n"
                     "It has implications on the security and stability of the system,\n"
                     "unless the cpu affinity of all domains is specified.\n");
+
+    if ( system_cpuinfo.mpidr.mt == 1 )
+        warning_add("WARNING: MULTITHREADING HAS BEEN DETECTED ON THE PROCESSOR.\n"
+                    "It might impact the security of the system.\n");
 }
 
 unsigned int __init smp_get_max_cpus(void)
-- 
2.34.1
Re: [PATCH v3] xen/arm: Set correct per-cpu cpu_core_mask
Posted by Henry Wang 1 year, 8 months ago
Hi All,

Gentle ping since it has been a couple of months, any comments on this 
updated patch? Thanks!

Kind regards,
Henry

On 3/21/2024 11:57 AM, Henry Wang wrote:
> In the common sysctl command XEN_SYSCTL_physinfo, the value of
> cores_per_socket is calculated based on the cpu_core_mask of CPU0.
> Currently on Arm this is a fixed value 1 (can be checked via xl info),
> which is not correct. This is because during the Arm CPU online
> process at boot time, setup_cpu_sibling_map() only sets the per-cpu
> cpu_core_mask for itself.
>
> cores_per_socket refers to the number of cores that belong to the same
> socket (NUMA node). Currently Xen on Arm does not support physical
> CPU hotplug and NUMA, also we assume there is no multithread. Therefore
> cores_per_socket means all possible CPUs detected from the device
> tree. Setting the per-cpu cpu_core_mask in setup_cpu_sibling_map()
> accordingly. Modify the in-code comment which seems to be outdated. Add
> a warning to users if Xen is running on processors with multithread
> support.
>
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
> Signed-off-by: Henry Wang <xin.wang2@amd.com>
> ---
> v3:
> - Use cpumask_copy() to set cpu_core_mask and drop the unnecessary
>    cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu)).
> - In-code comment adjustments.
> - Add a warning for multithread.
> v2:
> - Do not do the multithread check.
> ---
>   xen/arch/arm/smpboot.c | 18 +++++++++++++++---
>   1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index a84e706d77..b6268be27a 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -66,7 +66,11 @@ static bool cpu_is_dead;
>   
>   /* ID of the PCPU we're running on */
>   DEFINE_PER_CPU(unsigned int, cpu_id);
> -/* XXX these seem awfully x86ish... */
> +/*
> + * Although multithread is part of the Arm spec, there are not many
> + * processors support multithread and current Xen on Arm assumes there
> + * is no multithread.
> + */
>   /* representing HT siblings of each logical CPU */
>   DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_sibling_mask);
>   /* representing HT and core siblings of each logical CPU */
> @@ -85,9 +89,13 @@ static int setup_cpu_sibling_map(int cpu)
>            !zalloc_cpumask_var(&per_cpu(cpu_core_mask, cpu)) )
>           return -ENOMEM;
>   
> -    /* A CPU is a sibling with itself and is always on its own core. */
> +    /*
> +     * Currently we assume there is no multithread and NUMA, so
> +     * a CPU is a sibling with itself, and the all possible CPUs
> +     * are supposed to belong to the same socket (NUMA node).
> +     */
>       cpumask_set_cpu(cpu, per_cpu(cpu_sibling_mask, cpu));
> -    cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu));
> +    cpumask_copy(per_cpu(cpu_core_mask, cpu), &cpu_possible_map);
>   
>       return 0;
>   }
> @@ -277,6 +285,10 @@ void __init smp_init_cpus(void)
>           warning_add("WARNING: HMP COMPUTING HAS BEEN ENABLED.\n"
>                       "It has implications on the security and stability of the system,\n"
>                       "unless the cpu affinity of all domains is specified.\n");
> +
> +    if ( system_cpuinfo.mpidr.mt == 1 )
> +        warning_add("WARNING: MULTITHREADING HAS BEEN DETECTED ON THE PROCESSOR.\n"
> +                    "It might impact the security of the system.\n");
>   }
>   
>   unsigned int __init smp_get_max_cpus(void)
Re: [PATCH v3] xen/arm: Set correct per-cpu cpu_core_mask
Posted by Michal Orzel 1 year, 8 months ago
Hi Henry.

On 20/05/2024 04:57, Henry Wang wrote:
> Hi All,
> 
> Gentle ping since it has been a couple of months, any comments on this 
> updated patch? Thanks!
Sorry for the late reply.

> 
> Kind regards,
> Henry
> 
> On 3/21/2024 11:57 AM, Henry Wang wrote:
>> In the common sysctl command XEN_SYSCTL_physinfo, the value of
>> cores_per_socket is calculated based on the cpu_core_mask of CPU0.
>> Currently on Arm this is a fixed value 1 (can be checked via xl info),
>> which is not correct. This is because during the Arm CPU online
>> process at boot time, setup_cpu_sibling_map() only sets the per-cpu
>> cpu_core_mask for itself.
>>
>> cores_per_socket refers to the number of cores that belong to the same
>> socket (NUMA node). Currently Xen on Arm does not support physical
>> CPU hotplug and NUMA, also we assume there is no multithread. Therefore
>> cores_per_socket means all possible CPUs detected from the device
>> tree. Setting the per-cpu cpu_core_mask in setup_cpu_sibling_map()
>> accordingly. Modify the in-code comment which seems to be outdated. Add
>> a warning to users if Xen is running on processors with multithread
>> support.
>>
>> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
>> Signed-off-by: Henry Wang <xin.wang2@amd.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

>> ---
>> v3:
>> - Use cpumask_copy() to set cpu_core_mask and drop the unnecessary
>>    cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu)).
>> - In-code comment adjustments.
>> - Add a warning for multithread.
>> v2:
>> - Do not do the multithread check.
>> ---
>>   xen/arch/arm/smpboot.c | 18 +++++++++++++++---
>>   1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>> index a84e706d77..b6268be27a 100644
>> --- a/xen/arch/arm/smpboot.c
>> +++ b/xen/arch/arm/smpboot.c
>> @@ -66,7 +66,11 @@ static bool cpu_is_dead;
>>   
>>   /* ID of the PCPU we're running on */
>>   DEFINE_PER_CPU(unsigned int, cpu_id);
>> -/* XXX these seem awfully x86ish... */
>> +/*
>> + * Although multithread is part of the Arm spec, there are not many
>> + * processors support multithread and current Xen on Arm assumes there
NIT: s/support/supporting

>> + * is no multithread.
>> + */
>>   /* representing HT siblings of each logical CPU */
>>   DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_sibling_mask);
>>   /* representing HT and core siblings of each logical CPU */
>> @@ -85,9 +89,13 @@ static int setup_cpu_sibling_map(int cpu)
>>            !zalloc_cpumask_var(&per_cpu(cpu_core_mask, cpu)) )
>>           return -ENOMEM;
>>   
>> -    /* A CPU is a sibling with itself and is always on its own core. */
>> +    /*
>> +     * Currently we assume there is no multithread and NUMA, so
>> +     * a CPU is a sibling with itself, and the all possible CPUs
>> +     * are supposed to belong to the same socket (NUMA node).
>> +     */
>>       cpumask_set_cpu(cpu, per_cpu(cpu_sibling_mask, cpu));
>> -    cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu));
>> +    cpumask_copy(per_cpu(cpu_core_mask, cpu), &cpu_possible_map);
>>   
>>       return 0;
>>   }
>> @@ -277,6 +285,10 @@ void __init smp_init_cpus(void)
>>           warning_add("WARNING: HMP COMPUTING HAS BEEN ENABLED.\n"
>>                       "It has implications on the security and stability of the system,\n"
>>                       "unless the cpu affinity of all domains is specified.\n");
>> +
>> +    if ( system_cpuinfo.mpidr.mt == 1 )
>> +        warning_add("WARNING: MULTITHREADING HAS BEEN DETECTED ON THE PROCESSOR.\n"
>> +                    "It might impact the security of the system.\n");
>>   }
>>   
>>   unsigned int __init smp_get_max_cpus(void)
> 

~Michal
Re: [PATCH v3] xen/arm: Set correct per-cpu cpu_core_mask
Posted by Henry Wang 1 year, 8 months ago
Hi Michal,

On 5/21/2024 3:47 PM, Michal Orzel wrote:
> Hi Henry.
>
> On 3/21/2024 11:57 AM, Henry Wang wrote:
>>> In the common sysctl command XEN_SYSCTL_physinfo, the value of
>>> cores_per_socket is calculated based on the cpu_core_mask of CPU0.
>>> Currently on Arm this is a fixed value 1 (can be checked via xl info),
>>> which is not correct. This is because during the Arm CPU online
>>> process at boot time, setup_cpu_sibling_map() only sets the per-cpu
>>> cpu_core_mask for itself.
>>>
>>> cores_per_socket refers to the number of cores that belong to the same
>>> socket (NUMA node). Currently Xen on Arm does not support physical
>>> CPU hotplug and NUMA, also we assume there is no multithread. Therefore
>>> cores_per_socket means all possible CPUs detected from the device
>>> tree. Setting the per-cpu cpu_core_mask in setup_cpu_sibling_map()
>>> accordingly. Modify the in-code comment which seems to be outdated. Add
>>> a warning to users if Xen is running on processors with multithread
>>> support.
>>>
>>> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
>>> Signed-off-by: Henry Wang <xin.wang2@amd.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

Thanks.

>>>    /* ID of the PCPU we're running on */
>>>    DEFINE_PER_CPU(unsigned int, cpu_id);
>>> -/* XXX these seem awfully x86ish... */
>>> +/*
>>> + * Although multithread is part of the Arm spec, there are not many
>>> + * processors support multithread and current Xen on Arm assumes there
> NIT: s/support/supporting

Sorry, it should have been spotted locally before sending. Anyway, I 
will correct this in v4 with your Reviewed-by tag taken. Thanks for 
pointing this out.

Kind regards,
Henry

>>> __init smp_get_max_cpus(void)
> ~Michal
Re: [PATCH v3] xen/arm: Set correct per-cpu cpu_core_mask
Posted by Michal Orzel 1 year, 8 months ago

On 21/05/2024 09:51, Henry Wang wrote:
> Hi Michal,
> 
> On 5/21/2024 3:47 PM, Michal Orzel wrote:
>> Hi Henry.
>>
>> On 3/21/2024 11:57 AM, Henry Wang wrote:
>>>> In the common sysctl command XEN_SYSCTL_physinfo, the value of
>>>> cores_per_socket is calculated based on the cpu_core_mask of CPU0.
>>>> Currently on Arm this is a fixed value 1 (can be checked via xl info),
>>>> which is not correct. This is because during the Arm CPU online
>>>> process at boot time, setup_cpu_sibling_map() only sets the per-cpu
>>>> cpu_core_mask for itself.
>>>>
>>>> cores_per_socket refers to the number of cores that belong to the same
>>>> socket (NUMA node). Currently Xen on Arm does not support physical
>>>> CPU hotplug and NUMA, also we assume there is no multithread. Therefore
>>>> cores_per_socket means all possible CPUs detected from the device
>>>> tree. Setting the per-cpu cpu_core_mask in setup_cpu_sibling_map()
>>>> accordingly. Modify the in-code comment which seems to be outdated. Add
>>>> a warning to users if Xen is running on processors with multithread
>>>> support.
>>>>
>>>> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
>>>> Signed-off-by: Henry Wang <xin.wang2@amd.com>
>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
> 
> Thanks.
> 
>>>>    /* ID of the PCPU we're running on */
>>>>    DEFINE_PER_CPU(unsigned int, cpu_id);
>>>> -/* XXX these seem awfully x86ish... */
>>>> +/*
>>>> + * Although multithread is part of the Arm spec, there are not many
>>>> + * processors support multithread and current Xen on Arm assumes there
>> NIT: s/support/supporting
> 
> Sorry, it should have been spotted locally before sending. Anyway, I 
> will correct this in v4 with your Reviewed-by tag taken. Thanks for 
> pointing this out.
I don't think there is a need to resend a patch just for fixing this typo. It can be done on commit.

~Michal
Re: [PATCH v3] xen/arm: Set correct per-cpu cpu_core_mask
Posted by Julien Grall 1 year, 8 months ago
Hi,

On 21/05/2024 08:57, Michal Orzel wrote:
> 
> 
> On 21/05/2024 09:51, Henry Wang wrote:
>> Hi Michal,
>>
>> On 5/21/2024 3:47 PM, Michal Orzel wrote:
>>> Hi Henry.
>>>
>>> On 3/21/2024 11:57 AM, Henry Wang wrote:
>>>>> In the common sysctl command XEN_SYSCTL_physinfo, the value of
>>>>> cores_per_socket is calculated based on the cpu_core_mask of CPU0.
>>>>> Currently on Arm this is a fixed value 1 (can be checked via xl info),
>>>>> which is not correct. This is because during the Arm CPU online
>>>>> process at boot time, setup_cpu_sibling_map() only sets the per-cpu
>>>>> cpu_core_mask for itself.
>>>>>
>>>>> cores_per_socket refers to the number of cores that belong to the same
>>>>> socket (NUMA node). Currently Xen on Arm does not support physical
>>>>> CPU hotplug and NUMA, also we assume there is no multithread. Therefore
>>>>> cores_per_socket means all possible CPUs detected from the device
>>>>> tree. Setting the per-cpu cpu_core_mask in setup_cpu_sibling_map()
>>>>> accordingly. Modify the in-code comment which seems to be outdated. Add
>>>>> a warning to users if Xen is running on processors with multithread
>>>>> support.
>>>>>
>>>>> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
>>>>> Signed-off-by: Henry Wang <xin.wang2@amd.com>
>>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>>
>> Thanks.
>>
>>>>>     /* ID of the PCPU we're running on */
>>>>>     DEFINE_PER_CPU(unsigned int, cpu_id);
>>>>> -/* XXX these seem awfully x86ish... */
>>>>> +/*
>>>>> + * Although multithread is part of the Arm spec, there are not many
>>>>> + * processors support multithread and current Xen on Arm assumes there
>>> NIT: s/support/supporting
>>
>> Sorry, it should have been spotted locally before sending. Anyway, I
>> will correct this in v4 with your Reviewed-by tag taken. Thanks for
>> pointing this out.
> I don't think there is a need to resend a patch just for fixing this typo. It can be done on commit.

Fixed and committed.

Cheers,

> 
> ~Michal
> 

-- 
Julien Grall