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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

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.

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?

Best regards,
Mykola