Re: [xen-unstable-smoke test] 169781: regressions - FAIL

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 28/04/2022 01:47, Stefano Stabellini wrote:
> On Thu, 28 Apr 2022, Julien Grall wrote:
> > Hi Stefano,
> >
> > On Thu, 28 Apr 2022, 00:02 Stefano Stabellini, <sstabellini@xxxxxxxxxx> wrote
> >        It seems to me that it is acceptable to allocate memory with interrupt
> >        disabled during __init. I cannot see any drawbacks with it. I think we
> >        should change the ASSERT to only trigger after __init: system_state ==
> >        SYS_STATE_active.
> >
> >        What do you think?
> >
> >
> > This would solve the immediate problem but not the long term one (i.e cpu 
> > hotplug).
> >
> > So I think it would be better to properly fix it right away.
> Yeah, you are right about cpu hotplug. I think both statements are true:
>
> - it is true that this is supposed to work with cpu hotplug and these
>    functions might be directly affected by cpu hotplug (by a CPU coming
>    online later on)
>
> - it is also true that it might not make sense to ASSERT at __init time
>    if IRQs are disabled. There might be other places, not affected by cpu
>    hotplug, where we do memory allocation at __init time with IRQ
>    disabled. It might still be a good idea to add the system_state ==
>    SYS_STATE_active check in the ASSERT, not to solve this specific
>    problem but to avoid other issues.
AFAIU, it is not safe on x86 to do TLB flush with interrupts disabled 
*and* multiple CPUs running. So we can't generically relax the check.
Looking at the OSSTest results, both Arm32 and Arm64 without GICv3 ITS 
tests have passed. So it seems unnecessary to me to preemptively relax 
the check just for Arm.
> In regard to gicv3_lpi_allocate_pendtable, I haven't thought about the
> implications of cpu hotplug for LPIs and GICv3 before. Do you envision
> that in a CPU hotplug scenario gicv3_lpi_init_rdist would be called when
> the extra CPU comes online?
It is already called per-CPU. See gicv3_secondary_cpu_init() -> 
gicv3_cpu_init() -> gicv3_populate_rdist().
> Today gicv3_lpi_init_rdist is called based on the number of
> rdist_regions without checking if the CPU is online or offline (I think ?)
The re-distributors are not banked and therefore accessible by everyone. 
However, in Xen case, each pCPU will only touch its own re-distributor 
(well aside TYPER to figure out the ID).
The loop in gicv3_populate_rdist() will walk throught all the
re-distributor to find which one corresponds to the current pCPU. Once 
we found it, we will call gicv3_lpi_init_rdist() to fully initialize the 
re-distributor.
I don't think we want to populate the memory for each re-distributor in 
advance.
Cheers,


--
Julien Grall