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

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

On 6/5/26 09:28, Mykola Kvach wrote:
> Hi Oleksandr,
Hello Mykola

> 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.
Thanks, the justification is reasonable to me.

With the Fixes tags added and the suggested commit-message paragraph 
folded in:
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>

Regarding fold-in vs v3:
That is a maintainer preference question. Since only the commit message 
and trailers need updating, a fold-in at apply time might be efficient. 
However, I think that you still need the Arm maintainer's approval on 
the patch itself.
> Best regards,
> Mykola