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

[Oleksandr Tyshchenko <olekstysh@gmail.com>]

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@gmail.com> wrote:
>>
>>
>>
>> On 5/28/26 03:25, Mykola Kvach wrote:
>>
>> Hello Mykola
>>
>>> From: Mykola Kvach <mykola_kvach@epam.com>
>>>
>>> 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@epam.com>
>>> ---
>>> 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@epam.com/a7732487959e777ff1de318cb28c588db69fbaa1.1774431311.git.mykola._5Fkvach@epam.com/
>>
>>> - 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@epam.com>

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