Re: [PATCH 4/4] xen/arm: its: pre-initialize ITS quirks before LPI setup

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Oleksandr,

Thank you for the review.

On Tue, Apr 28, 2026 at 7:31 PM Oleksandr Tyshchenko
<olekstysh@xxxxxxxxx> wrote:
>
>
>
> On 3/25/26 12:38, Mykola Kvach wrote:
>
> Hello Mykola
>
> > From: Mykola Kvach <mykola_kvach@xxxxxxxx>
> >
> > In the current initialization flow, gicv3_init() calls gicv3_dist_init()
> > before gicv3_its_init().
> >
> > When LPIs are supported, gicv3_dist_init() calls
> > gicv3_lpi_init_host_lpis(), which initializes host LPI state and allocates
> > the boot CPU pending table before ITS quirk flags are computed. Non-boot
> > CPUs allocate their pending tables later from the CPU_UP_PREPARE notifier,
> > while redistributor LPI programming happens separately in
> > gicv3_lpi_init_rdist().
> >
> > This means the boot CPU LPI setup can observe default ITS memory attributes
> > before dma-noncoherent and other ITS quirks are applied.
> >
> > Introduce gicv3_its_preinit() and call it before gicv3_dist_init(). This
> > keeps the actual ITS hardware initialization in gicv3_its_init(), but moves
> > ITS discovery, quirk validation and quirk flag setup early enough for the
> > subsequent LPI initialization to use the correct attributes.
>
>
> Have you considered an alternative approach that might be less invasive?
> I am thinking of something the other way around: perhaps we could
> allocate the LPI pending table for the boot CPU later.
>
> Would a diff below work?
>
>
> ---
>   xen/arch/arm/gic-v3-lpi.c             | 9 +++++++++
>   xen/arch/arm/gic-v3.c                 | 2 ++
>   xen/arch/arm/include/asm/gic_v3_its.h | 6 ++++++
>   3 files changed, 17 insertions(+)
>
> diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c
> index 9ee338edc2..61cc45d347 100644
> --- a/xen/arch/arm/gic-v3-lpi.c
> +++ b/xen/arch/arm/gic-v3-lpi.c
> @@ -450,6 +450,15 @@ int gicv3_lpi_init_host_lpis(unsigned int
> host_lpi_bits)
>
>       printk("GICv3: using at most %lu LPIs on the host.\n",
> MAX_NR_HOST_LPIS);
>
> +    return rc;
> +}
> +
> +int gicv3_lpi_post_init_host_lpis(void)
> +{
> +    int rc;
> +
> +    ASSERT(smp_processor_id() == 0);
> +
>       /* Register the CPU notifier and allocate memory for the boot CPU */
>       register_cpu_notifier(&cpu_nfb);
>       rc = gicv3_lpi_allocate_pendtable(smp_processor_id());
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 7f365cdbe9..8b9059c5c9 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1981,6 +1981,8 @@ static int __init gicv3_init(void)
>           res = gicv3_its_init();
>           if ( res )
>               panic("GICv3: ITS: initialization failed: %d\n", res);
> +
> +        gicv3_lpi_post_init_host_lpis();
>       }
>
>       res = gicv3_cpu_init();
> diff --git a/xen/arch/arm/include/asm/gic_v3_its.h
> b/xen/arch/arm/include/asm/gic_v3_its.h
> index fc5a84892c..288cc1d42f 100644
> --- a/xen/arch/arm/include/asm/gic_v3_its.h
> +++ b/xen/arch/arm/include/asm/gic_v3_its.h
> @@ -156,6 +156,7 @@ int gicv3_lpi_init_rdist(void __iomem * rdist_base);
>
>   /* Initialize the host structures for LPIs and the host ITSes. */
>   int gicv3_lpi_init_host_lpis(unsigned int host_lpi_bits);
> +int gicv3_lpi_post_init_host_lpis(void);
>   int gicv3_its_init(void);
>
>   /* Store the physical address and ID for each redistributor as read
> from DT. */
> @@ -245,6 +246,11 @@ static inline int gicv3_lpi_init_host_lpis(unsigned
> int host_lpi_bits)
>       return 0;
>   }
>
> +static inline int gicv3_lpi_post_init_host_lpis(void)
> +{
> +    return 0;
> +}
> +
>   static inline int gicv3_its_init(void)
>   {
>       return 0;

Yes, I think this direction is better and less invasive.
Thank you for the suggestion.

Best regards,
Mykola

> --
> 2.34.1
>
>
> >
> > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > ---
> > TODO: Think about separating Redistributor/LPI attributes from ITS.
> > ---
> >   xen/arch/arm/gic-v3-its.c             | 36 +++++++++++++++++----------
> >   xen/arch/arm/gic-v3.c                 |  7 ++++++
> >   xen/arch/arm/include/asm/gic_v3_its.h |  5 ++++
> >   3 files changed, 35 insertions(+), 13 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> > index ee432088cd..0251d91630 100644
> > --- a/xen/arch/arm/gic-v3-its.c
> > +++ b/xen/arch/arm/gic-v3-its.c
> > @@ -63,6 +63,7 @@ struct its_quirk {
> >       uint32_t flags;
> >   };
> >
> > +/* TODO: Separate Redistributor/LPI attributes from ITS quirks. */
> >   static uint32_t __ro_after_init its_quirk_flags;
> >
> >   static bool gicv3_its_match_quirk_gen4(const struct host_its *hw_its)
> > @@ -148,9 +149,15 @@ static uint32_t gicv3_its_collect_quirks(const struct 
> > host_its *hw_its,
> >       return flags;
> >   }
> >
> > -static void gicv3_its_enable_quirks(struct host_its *hw_its)
> > +static void gicv3_its_enable_quirks(void)
> >   {
> >       const struct its_quirk *quirk;
> > +    const struct host_its *hw_its;
> > +
> > +    if ( list_empty(&host_its_list) )
> > +        return;
> > +
> > +    hw_its = list_first_entry(&host_its_list, struct host_its, entry);
> >
> >       its_quirk_flags = gicv3_its_collect_quirks(hw_its, &quirk);
> >
> > @@ -603,16 +610,10 @@ static int gicv3_its_init_single_its(struct host_its 
> > *hw_its)
> >       uint64_t reg;
> >       int i, ret;
> >
> > -    hw_its->its_base = ioremap_nocache(hw_its->addr, hw_its->size);
> > -    if ( !hw_its->its_base )
> > -        return -ENOMEM;
> > -
> >       ret = gicv3_disable_its(hw_its);
> >       if ( ret )
> >           return ret;
> >
> > -    gicv3_its_enable_quirks(hw_its);
> > -
> >       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);
> > @@ -1161,6 +1162,11 @@ static void add_to_host_its_list(paddr_t addr, 
> > paddr_t size,
> >       its_data->size = size;
> >       its_data->dt_node = node;
> >
> > +    its_data->its_base = ioremap_nocache(its_data->addr, its_data->size);
> > +    if ( !its_data->its_base )
> > +        panic("GICv3: Cannot map ITS frame: 0x%lx, 0x%lx\n",
> > +            its_data->addr, its_data->size);
> > +
> >       printk("GICv3: Found ITS @0x%lx\n", addr);
> >
> >       list_add_tail(&its_data->entry, &host_its_list);
> > @@ -1238,16 +1244,22 @@ static void gicv3_its_acpi_init(void)
> >
> >   #endif
> >
> > -int gicv3_its_init(void)
> > +void __init gicv3_its_preinit(void)
> >   {
> > -    struct host_its *hw_its;
> > -    int ret;
> > -
> >       if ( acpi_disabled )
> >           gicv3_its_dt_init(dt_interrupt_controller);
> >       else
> >           gicv3_its_acpi_init();
> >
> > +    gicv3_its_validate_quirks();
> > +    gicv3_its_enable_quirks();
> > +}
> > +
> > +int gicv3_its_init(void)
> > +{
> > +    struct host_its *hw_its;
> > +    int ret;
> > +
> >       list_for_each_entry(hw_its, &host_its_list, entry)
> >       {
> >           ret = gicv3_its_init_single_its(hw_its);
> > @@ -1255,8 +1267,6 @@ int gicv3_its_init(void)
> >               return ret;
> >       }
> >
> > -    gicv3_its_validate_quirks();
> > -
> >       return 0;
> >   }
> >
> > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> > index bc07f97c16..6e44d23d64 100644
> > --- a/xen/arch/arm/gic-v3.c
> > +++ b/xen/arch/arm/gic-v3.c
> > @@ -1974,6 +1974,13 @@ static int __init gicv3_init(void)
> >
> >       spin_lock(&gicv3.lock);
> >
> > +    if ( gic_dist_supports_lpis() )
> > +        /*
> > +         * Apply ITS quirks before gicv3_dist_init() sets up host LPIs,
> > +         * so pending tables use the correct ITS memory attributes.
> > +         */
> > +        gicv3_its_preinit();
> > +
> >       gicv3_dist_init();
> >
> >       if ( gic_dist_supports_lpis() )
> > diff --git a/xen/arch/arm/include/asm/gic_v3_its.h 
> > b/xen/arch/arm/include/asm/gic_v3_its.h
> > index fc5a84892c..e1d7522ea5 100644
> > --- a/xen/arch/arm/include/asm/gic_v3_its.h
> > +++ b/xen/arch/arm/include/asm/gic_v3_its.h
> > @@ -156,6 +156,7 @@ int gicv3_lpi_init_rdist(void __iomem * rdist_base);
> >
> >   /* Initialize the host structures for LPIs and the host ITSes. */
> >   int gicv3_lpi_init_host_lpis(unsigned int host_lpi_bits);
> > +void gicv3_its_preinit(void);
> >   int gicv3_its_init(void);
> >
> >   /* Store the physical address and ID for each redistributor as read from 
> > DT. */
> > @@ -219,6 +220,10 @@ static inline int gicv3_its_deny_access(struct domain 
> > *d)
> >       return 0;
> >   }
> >
> > +static inline void gicv3_its_preinit(void)
> > +{
> > +}
> > +
> >   static inline bool gicv3_its_host_has_its(void)
> >   {
> >       return false;
>