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

[Henry Wang <xin.wang2@xxxxxxx>]

Hi Julien,

On 3/14/2024 9:27 PM, Julien Grall wrote:
> Hi Henry,
>
> On 28/02/2024 01:58, Henry Wang wrote:
> > 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.
Yep. Do you think changing the content of this line to something like 
"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" makes sense to you?
> >   /* 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).
Unfortunately me neither, so adding George to this thread as I think he 
can bring us some insights on this topic from the scheduler perspective.
> Depending on the answer, I would consider to print a warning and maybe 
> add it in SUPPORT.MD in a separate patch.
To be honest, as discussed in v1. I think I am quite tempted to add an 
ASSERT(system_cpuinfo.mpidr.mt == 0) to make sure we catch the 
multithread support stuff earlier. I don't really know what will happen 
if running current Xen on top of a multithread-implemented processor, 
probably it will be fine, but probably some strange behavior will happen 
after the boot time which I think will be difficult to debug...
> 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.
Sounds good. I will move it in v3.

> > +    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?
You mean cpumask_copy(per_cpu(cpu_core_mask, cpu), &cpu_possible_map)? 
Good question...I forgot if there was a reason behind this back to the 
time I wrote this patch (it is likely just me being silly). But yes 
definitely I can do this way in v3.
> This would also allow to drop cpumask_set_cpu(..., cpu_core_mask) above.
Good point. I will drop in v3.

Kind regards,
Henry

> Cheers,