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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

On Fri, Sep 25, 2026 at 8:45 PM Volodymyr Babchuk
<Volodymyr_Babchuk@xxxxxxxx> wrote:
>
> 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.

Ack.

Best regards,
Mykola