Re: [PATCH v10 05/13] xen/arm: gic-v3: add ITS suspend/resume support

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Luca,

Thank you for the review.

On Thu, May 28, 2026 at 9:12 PM Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
>
> Hi Mykola,
>
> >
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > +int gicv3_its_suspend(void)
> > +{
> > +    struct host_its *its;
> > +    int ret;
> > +
> > +    list_for_each_entry( its, &host_its_list, entry )
> > +    {
> > +        unsigned int i;
> > +        void __iomem *base = its->its_base;
> > +
> > +        /*
> > +         * By the time Xen reaches gic_suspend(), every domain is already 
> > in
> > +         * SHUTDOWN_suspend, so ITS-targeting interrupt sources are 
> > expected
> > +         * to have been quiesced by the owning OS before SYSTEM_SUSPEND.
> > +         */
> > +        /* Preserve saved GITS_CTLR state, excluding read-only QUIESCENT. 
> > */
> > +        its->suspend_ctx.ctlr = readl_relaxed(base + GITS_CTLR) &
> > +                                ~GITS_CTLR_QUIESCENT;
> > +        ret = gicv3_disable_its(its);
> > +        if ( ret )
> > +        {
> > +            writel_relaxed(its->suspend_ctx.ctlr, base + GITS_CTLR);
>
> This is writing enable from 0 to 1, while quiescent is still 0, which is 
> unpredictable,
> however it’s the same happening on Linux, so I would leave it to the 
> maintainer preference.

I think you are right, thanks for spotting this.

After gicv3_disable_its() times out, the ITS has already had
GITS_CTLR.Enabled cleared, but GITS_CTLR.Quiescent is still 0. Writing back
the saved CTLR may set Enabled from 0 to 1 while Quiescent is 0, which is
UNPREDICTABLE according to the spec.

So restoring GITS_CTLR in this error path does not look safe. We could extend
the quiesce timeout if the current 100ms is considered too short, but once the
wait has failed there is no architecturally safe way to restore the ITS state.
In that case I think the suspend path should panic.

This would also let us drop the reverse rollback loop from this patch. The
definition of list_for_each_entry_continue_reverse() can then be moved to the
later SMMUv3 patch, where it is still needed.

What do you think?

Best regards,
Mykola