Re: [PATCH for-4.22 v2 1/4] xen/arm: gic: defer host LPI allocation until after ITS init

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Oleksandr,

Thanks for the review.

On Wed, Jun 3, 2026 at 2:19 PM Oleksandr Tyshchenko <olekstysh@xxxxxxxxx> wrote:
>
>
>
> On 5/28/26 03:25, Mykola Kvach wrote:
>
> Hello Mykola
>
> > From: Mykola Kvach <mykola_kvach@xxxxxxxx>
> >
> > gicv3_lpi_init_host_lpis() allocates host LPI state, including the
> > host LPI lookup table, CPU notifier state and the boot CPU pending table.
> > Those allocations use gicv3_its_get_memflags().
> >
> > ITS workarounds are discovered from gicv3_its_init(), so allocating host
> > LPI state from gicv3_dist_init() can happen before the memory restrictions
> > required by the ITS are known. On affected systems this can leave
> > Redistributor LPI state allocated and programmed with the default memory
> > policy.
> >
> > Move host LPI initialization after gicv3_its_init(), and only run it when
> > a host ITS was found. The old call ignored the return value. Now that the
> > call is made from gicv3_init(), check it and panic on failure because
> > Redistributor LPI initialization relies on that state being available.
> >
> > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > ---
> > Changes in v2:
> > - Replace the v1 ITS pre-initialization hook with the less invasive
> >    approach suggested during review: move the existing host LPI
> >    initialization after gicv3_its_init().
>
>
> Just for the context: The original review suggestion [1] was to consider
> splitting gicv3_lpi_init_host_lpis() and defer only the portions that
> depend on ITS quirks being known, specifically the allocation of the
> per-CPU pending table for the boot CPU (gicv3_lpi_allocate_pendtable),
> which is the actual consumer of gicv3_its_get_memflags(). But here, the
> whole gicv3_lpi_init_host_lpis() is moved, so the scope of the deferral
> is broader.
>
> [1]
> https://patchew.org/Xen/cover.1774431310.git.mykola._5Fkvach@xxxxxxxx/a7732487959e777ff1de318cb28c588db69fbaa1.1774431311.git.mykola._5Fkvach@xxxxxxxx/
>
> > - Check gicv3_lpi_init_host_lpis() and panic on failure, matching the fatal
> >    nature of host LPI setup once ITS initialization succeeded.
>
> So, this patch appears to fix two distinct issues:
>
> - ordering issue (LPI init occurring before ITS quirks are known)
> - unchecked return value from gicv3_lpi_init_host_lpis()
>
> Should these warrant Fixes: tag(s)?

Yes, I agree that the patch should carry Fixes tags.

For the ordering issue, I think the relevant tag is:

Fixes: 751ec850ec1d ("ARM: ITS: implement quirks and add support for
Renesas Gen4 ITS")

For the ignored return value, I think the first commit where this became
observable is:

Fixes: dcb6cb263689 ("ARM: GICv3 ITS: introduce host LPI array")

The original call site already ignored the return value, but at that
point gicv3_lpi_init_host_lpis() could not fail in practice.
dcb6cb263689 introduced the host LPI array allocation and made the
function return -ENOMEM, so the ignored return value became meaningful
there. Later, 69589c374a92 added another meaningful failure path through
the boot CPU pending-table allocation, but dcb6cb263689 seems to be the
earliest commit where the return value became relevant.

>
>
> > ---
> >   xen/arch/arm/gic-v3.c | 14 +++++++++++---
> >   1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> > index 17ff85ef5d..acdac22953 100644
> > --- a/xen/arch/arm/gic-v3.c
> > +++ b/xen/arch/arm/gic-v3.c
> > @@ -764,9 +764,6 @@ static void __init gicv3_dist_init(void)
> >       type = readl_relaxed(GICD + GICD_TYPER);
> >       nr_lines = 32 * ((type & GICD_TYPE_LINES) + 1);
> >
> > -    if ( type & GICD_TYPE_LPIS )
> > -        gicv3_lpi_init_host_lpis(GICD_TYPE_ID_BITS(type));
> > -
> >       /* Only 1020 interrupts are supported */
> >       nr_lines = min(1020U, nr_lines);
> >       gicv3_info.nr_lines = nr_lines;
> > @@ -1990,6 +1987,17 @@ static int __init gicv3_init(void)
> >           res = gicv3_its_init();
> >           if ( res )
> >               panic("GICv3: ITS: initialization failed: %d\n", res);
> > +
> > +        /*
> > +         * Host LPI allocation uses ITS-derived memory attributes, so 
> > defer it
> > +         * until after gicv3_its_init() has discovered ITS workarounds.
> > +         */
> > +        if ( gicv3_its_host_has_its() )
>
> This looks like a behaviour change. The condition is narrowed from "GICD
> advertises LPI support" to "host ITS is present". As a result, on a
> system where GICD_TYPE_LPIS is set but no ITS is present, LPI-specific
> variables and data structures will no longer be initialized or
> allocated. If I am not mistaken, software-generated LPIs without ITS
> involvement are currently unsupported, so this change might be safe.
> However, I think the commit message should explicitly document this
> behaviour change and explain why it is safe.

Regarding the behaviour change: yes, it is intentional, and I agree that
it should be documented in the commit message.

The patch narrows the condition from "GICD advertises LPIs" to "a host
ITS was discovered". Xen currently has no supported LPI path without a
host ITS: gicv3_lpi_init_rdist() already rejects that case with
-ENODEV. Therefore, on systems where GICD_TYPE_LPIS is set but no host
ITS is present, deferring and gating gicv3_lpi_init_host_lpis() only
avoids allocating host LPI state which cannot be used by a supported Xen
LPI path.

The CPU notifier is registered from gicv3_lpi_init_host_lpis() itself, so
when host LPI initialization is skipped for the no-host-ITS case, the
secondary-CPU pending-table allocation path is not enabled either.

I kept the whole gicv3_lpi_init_host_lpis() deferral in one piece to
keep the 4.22 release fix small. Splitting out only the boot CPU
pending-table allocation would be possible, but it would make the fix
more invasive.

If the patch is otherwise acceptable, could the Fixes tags and the
clarification below be folded into the commit message when applying it?
Otherwise I can send a v3 of patch 1 only with just commit-message and
trailer updates.

Suggested commit-message fold-in:

This also narrows the condition for host LPI initialization from
"GICD advertises LPIs" to "a host ITS was discovered". This is
intentional: Xen currently has no supported LPI path without a host ITS,
and gicv3_lpi_init_rdist() already rejects that case with -ENODEV.
Therefore, on systems where GICD_TYPE_LPIS is set but no host ITS is
present, skipping gicv3_lpi_init_host_lpis() only avoids allocating host
LPI state that cannot be used by a supported Xen LPI path.

Best regards,
Mykola