Re: [PATCH v3 1/4] xen/arm: its: initialize host LPI state before activating ITSes

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Mykola,

Mykola Kvach <xakep.amatop@xxxxxxxxx> writes:

> Hi Volodymyr,
>
> Thank you for the review.
>
> On Wed, Sep 23, 2026 at 2:56 AM Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx> wrote:
>>
>> Hi Mykola,
>>
>> Mykola Kvach <mykola_kvach@xxxxxxxx> writes:
>>
>> > The boot CPU pending table must use the memory attributes selected by
>> > ITS quirks. gicv3_lpi_init_host_lpis() is therefore called after
>> > gicv3_its_init(). However, gicv3_its_init() also programs and enables
>> > each ITS before host LPI state is allocated. No ITS commands are
>> > submitted at that point, but this ordering relies on that implementation
>> > detail.
>> >
>> > Split per-ITS initialization into preparation and activation phases.
>> > First map and disable every ITS and collect its quirks. Then initialize
>> > host LPI state. Only after that, allocate and program the ITS tables and
>> > command queue, and enable each ITS.
>> >
>> > The subsequent gicv3_cpu_init() sequence remains unchanged: it programs
>> > the Redistributor LPI tables, enables LPIs, and then submits the first
>> > MAPC and SYNC commands.
>> >
>> > Suggested-by: Julien Grall <julien@xxxxxxx>
>> > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
>> > ---
>> > Changes in v3:
>> > - New patch implementing the post-4.22 initialization order discussed
>> >   during review of the ordering fix.
>> >
>> > Link: 
>> > https://patchew.org/Xen/341edd8de63dcd84ccc6e7b6c03e9e8fc7105184.1781847061.git.mykola._5Fkvach@xxxxxxxx/
>> > ---
>> >  xen/arch/arm/gic-v3-its.c             | 32 ++++++++++++++++++++++-----
>> >  xen/arch/arm/gic-v3.c                 | 15 ++-----------
>> >  xen/arch/arm/include/asm/gic_v3_its.h |  4 ++--
>> >  3 files changed, 31 insertions(+), 20 deletions(-)
>> >
>> > diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
>> > index 325835b0ad..972825bf06 100644
>> > --- a/xen/arch/arm/gic-v3-its.c
>> > +++ b/xen/arch/arm/gic-v3-its.c
>> > @@ -11,6 +11,7 @@
>> >  #include <xen/lib.h>
>> >  #include <xen/delay.h>
>> >  #include <xen/iocap.h>
>> > +#include <xen/init.h>
>> >  #include <xen/libfdt/libfdt.h>
>> >  #include <xen/mm.h>
>> >  #include <xen/rbtree.h>
>> > @@ -549,10 +550,9 @@ static int gicv3_disable_its(struct host_its *hw_its)
>> >      return -ETIMEDOUT;
>> >  }
>> >
>> > -static int gicv3_its_init_single_its(struct host_its *hw_its)
>> > +static int __init gicv3_its_prepare_single_its(struct host_its *hw_its)
>> >  {
>> > -    uint64_t reg;
>> > -    int i, ret;
>> > +    int ret;
>> >
>> >      hw_its->its_base = ioremap_nocache(hw_its->addr, hw_its->size);
>> >      if ( !hw_its->its_base )
>> > @@ -564,6 +564,14 @@ static int gicv3_its_init_single_its(struct host_its 
>> > *hw_its)
>> >
>> >      gicv3_its_enable_quirks(hw_its);
>> >
>> > +    return 0;
>> > +}
>> > +
>> > +static int __init gicv3_its_init_single_its(struct host_its *hw_its)
>> > +{
>> > +    uint64_t reg;
>> > +    int i, ret;
>> > +
>> >      reg = readq_relaxed(hw_its->its_base + GITS_TYPER);
>> >      hw_its->devid_bits = GITS_TYPER_DEVICE_ID_BITS(reg);
>> >      hw_its->evid_bits = GITS_TYPER_EVENT_ID_BITS(reg);
>> > @@ -1189,7 +1197,7 @@ static void gicv3_its_acpi_init(void)
>> >
>> >  #endif
>> >
>> > -int gicv3_its_init(void)
>> > +int __init gicv3_its_init(unsigned int host_lpi_bits)
>> >  {
>> >      struct host_its *hw_its;
>> >      int ret;
>> > @@ -1201,13 +1209,27 @@ int gicv3_its_init(void)
>> >
>> >      list_for_each_entry(hw_its, &host_its_list, entry)
>> >      {
>> > -        ret = gicv3_its_init_single_its(hw_its);
>> > +        ret = gicv3_its_prepare_single_its(hw_its);
>> >          if ( ret )
>> >              return ret;
>> >      }
>> >
>> >      gicv3_its_validate_quirks();
>> >
>> > +    if ( list_empty(&host_its_list) )
>> > +        return 0;
>>
>> What is the purpose of this check here? I'd expect to see it before the
>> first list_for_each_entry() loop.
>
> The check skips host LPI initialization when no host ITS is present,
> preserving the existing behavior. Xen currently does not support LPIs
> without an ITS.

This does not answer why you need to run list_for_each_entry() loop. I
understand that it will not loop and then you will just call
gicv3_its_validate_quirks(), which also will do nothing. This is why I
am asking, why not move this if() check right before the loop?

> gicv3_its_validate_quirks() already checks for an empty list, so the
> current placement does not cause a functional issue. The following
> patch also removes that function and its call.
>
> Would you be OK with keeping the current placement?

I think it will be more sensible to have this check before the
loop. Unless I miss something, of course.

--
WBR, Volodymyr