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

[Michal Orzel <michal.orzel@xxxxxxx>]

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@xxxxxxx>
>> Signed-off-by: Henry Wang <xin.wang2@xxxxxxx>
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

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