Re: [PATCH v4][PART 2 07/10] xen/arm: Add support for system suspend triggered by hardware domain

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi, @Jan Beulich

On Mon, Jun 2, 2025 at 12:26 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 02.06.2025 11:04, Mykola Kvach wrote:
> > @@ -857,8 +860,24 @@ void arch_domain_destroy(struct domain *d)
> >      domain_io_free(d);
> >  }
> >
> > -void arch_domain_shutdown(struct domain *d)
> > +int arch_domain_shutdown(struct domain *d)
> >  {
> > +    switch ( d->shutdown_code )
> > +    {
> > +    case SHUTDOWN_suspend:
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > +        if ( !is_hardware_domain(d) )
> > +            break;
> > +
> > +        return host_system_suspend();
> > +#else
> > +        break;
> > +#endif
> > +    default:
> > +        break;
> > +    }
> > +
> > +    return 0;
> >  }
>
> What's wrong with
>
> int arch_domain_shutdown(struct domain *d)
> {
>     switch ( d->shutdown_code )
>     {
> #ifdef CONFIG_SYSTEM_SUSPEND
>     case SHUTDOWN_suspend:
>         if ( !is_hardware_domain(d) )
>             break;
>
>         return host_system_suspend();
> #endif
>
>     default:
>         break;
>     }
>
>     return 0;
> }
>
> ?

You're right — your version is cleaner and avoids unnecessary
preprocessor spaghetti.

My original intention was to keep the SHUTDOWN_suspend case visible
even when CONFIG_SYSTEM_SUSPEND is disabled, so future changes
wouldn't require reintroducing the case label. But thinking about it
again, if future support is needed, the code can be easily adjusted at
that time.

I'll switch to your suggested version — it's simpler and more readable.

>
> > @@ -1311,13 +1316,13 @@ int domain_shutdown(struct domain *d, u8 reason)
> >          v->paused_for_shutdown = 1;
> >      }
> >
> > -    arch_domain_shutdown(d);
> > +    ret = arch_domain_shutdown(d);
>
> If non-zero comes back here, is ...
>
> >      __domain_finalise_shutdown(d);
>
> ... this still appropriate to call?

Thank you for catching that — you’re absolutely right, that was my oversight.

If arch_domain_shutdown returns a non-zero value, we need to undo
any previous actions performed before the call to properly restore the
domain’s state. Calling __domain_finalise_shutdown unconditionally
in that case could lead to an inconsistent state.

I will update the code accordingly to ensure proper cleanup and state
restoration.

>
> >      spin_unlock(&d->shutdown_lock);
> >
> > -    return 0;
> > +    return ret;
> >  }
>
> Most callers don't care about the return value of this function. That likely
> needs to change, even if _for now_ you only alter the SHUTDOWN_suspend case
> (and hence some of the callers aren't immediately impacted)?

Thanks for pointing this out. You’re right that most callers currently
ignore the return value of domain_shutdown(). This will need to be
updated to properly handle non-zero returns, at least for the
SHUTDOWN_suspend case where it matters.

Even if some callers aren’t immediately impacted, I agree it’s better
to fix this proactively to avoid silent failures or inconsistent states.

I’ll start reviewing the callers and update them accordingly.

>
> Jan

Thank you for reviewing this patch series.

Best regards,
Mykola