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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Mykola,

> On 3 Jun 2026, at 06:56, Mykola Kvach <xakep.amatop@xxxxxxxxx> wrote:
> 
> 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.

yes might be an option, but because we would diverge from Linux, I would leave 
it
to the maintainers.

Cheers,
Luca