Re: [PATCH] xen/arm: smpboot: Allocate the CPU sibling/core maps while preparing the CPU

[Julien Grall <julien@xxxxxxx>]

On 14/06/2022 12:02, Michal Orzel wrote:
> Hi Julien,
Hi Michal,

> On 14.06.2022 11:41, Julien Grall wrote:
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > Commit 5047cd1d5dea "xen/common: Use enhanced ASSERT_ALLOC_CONTEXT in
> > xmalloc()" extended the checks in _xmalloc() to catch any use of the
> > helpers from context with interrupts disabled.
> >
> > Unfortunately, the rule is not followed when allocating the CPU
> > sibling/core maps.
> >
> > (XEN) Xen call trace:
> > (XEN)    [<00238a5c>] _xmalloc+0xfc/0x314 (PC)
> > (XEN)    [<00000000>] 00000000 (LR)
> > (XEN)    [<00238c8c>] _xzalloc+0x18/0x4c
> > (XEN)    [<00288cb4>] smpboot.c#setup_cpu_sibling_map+0x38/0x138
> > (XEN)    [<00289024>] start_secondary+0x1b4/0x270
> > (XEN)    [<40010170>] 40010170
> > (XEN)
> > (XEN)
> > (XEN) ****************************************
> > (XEN) Panic on CPU 2:
> > (XEN) Assertion '!in_irq() && (local_irq_is_enabled() || num_online_cpus() <= 
> > 1)' failed at common/xmalloc_tlsf.c:601
> > (XEN) ****************************************
> >
> > This is happening because zalloc_cpumask_var() may allocate memory
> > if NR_CPUS is > 2 * sizeof(unsigned long).
> >
> > Avoid the problem by allocate the per-CPU IRQs while preparing the
> > CPU.
> Shouldn't this be "by allocating the CPU sibling/core maps while ..."
> to reflect the commit title and to distinguish between this change and the IRQ 
> one?
Yes. I will update it.

[...]

> >   static void remove_cpu_sibling_map(int cpu)
> > @@ -292,9 +294,14 @@ smp_get_max_cpus (void)
> >   void __init
> >   smp_prepare_cpus(void)
> >   {
> > +    int rc;
> Here you are leaving rc uninitialized (which is ok) but ...
>
> > +
> >       cpumask_copy(&cpu_present_map, &cpu_possible_map);
> >   
> > -    setup_cpu_sibling_map(0);
> > +    rc = setup_cpu_sibling_map(0);
> > +    if ( rc )
> > +        panic("Unable to allocate CPU sibling/core maps\n");
> > +
> >   }
> >   
> >   /* Boot the current CPU */
> > @@ -361,8 +368,6 @@ void start_secondary(void)
> >   
> >       set_current(idle_vcpu[cpuid]);
> >   
> > -    setup_cpu_sibling_map(cpuid);
> > -
> >       /* Run local notifiers */
> >       notify_cpu_starting(cpuid);
> >       /*
> > @@ -530,9 +535,19 @@ static int cpu_smpboot_callback(struct notifier_block *nfb,
> >                                   void *hcpu)
> >   {
> >       unsigned int cpu = (unsigned long)hcpu;
> > +    unsigned int rc = 0;
> ... here you are setting rc to 0 even though it will be reassigned.
> Furthermore, if rc is used only in case of CPU_UP_PREPARE, why not moving the 
> definition there?
Because I forgot to replace "return NOTIFY_DONE;" with :

return !rc ? NOTIFY_DONE : notifier_from_errno(rc);

In this case, we would need to initialize rc to 0 so it doesn't get used 
initialized.
Cheers,

--
Julien Grall