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

[Julien Grall <julien@xxxxxxx>]

Hi Henry,

On 28/02/2024 01:58, 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. Drop the in-code comment which seems to be outdated.
>
> Signed-off-by: Henry Wang <Henry.Wang@xxxxxxx>
> Signed-off-by: Henry Wang <xin.wang2@xxxxxxx>
> ---
> v2:
> - Do not do the multithread check.
> ---
>   xen/arch/arm/smpboot.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index a84e706d77..d9ebd55d4a 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -66,7 +66,6 @@ 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... */
:). I guess at the time we didn't realize that MT was supported on Arm. 
It is at least part of the spec, but as Michal pointed out it doesn't 
look like a lot of processors supports it.
>   /* 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 */
> @@ -89,6 +88,10 @@ static int setup_cpu_sibling_map(int cpu)
>       cpumask_set_cpu(cpu, per_cpu(cpu_sibling_mask, cpu));
>       cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu));
>   
> +    /* Currently we assume there is no multithread. */
I am not very familiar with the scheduling in Xen. Do you know what's 
the consequence of not properly supporting MT? One thing I can think of 
is security (I know there were plenty of security issues with SMT).
Depending on the answer, I would consider to print a warning and maybe 
add it in SUPPORT.MD in a separate patch.
Also, looking at the v1 discussion, it sounds like even cpu_sibling_mask 
would not be correct. So I think it would make sense to move the comment 
on top of setup_cpu_sibling_map.
> +    cpumask_or(per_cpu(cpu_core_mask, cpu),
> +               per_cpu(cpu_core_mask, cpu), &cpu_possible_map);
AFIACT, per_cpu(cpu_core_mask, ...) will now be equal to 
cpu_possible_map. In that case, wouldn't it be better to simply copy the 
cpumask?
This would also allow to drop cpumask_set_cpu(..., cpu_core_mask) above.

Cheers,

--
Julien Grall