Re: [PATCH v6 11/13] xen/arm: Add support for system suspend triggered by hardware domain

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Jan,

On Wed, Sep 3, 2025 at 7:31 AM Mykola Kvach <xakep.amatop@xxxxxxxxx> wrote:
>
> Hi Jan,
>
> On Tue, Sep 2, 2025 at 5:33 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >
> > On 02.09.2025 00:10, Mykola Kvach wrote:
> > > --- a/xen/common/domain.c
> > > +++ b/xen/common/domain.c
> > > @@ -1317,7 +1317,11 @@ int domain_shutdown(struct domain *d, u8 reason)
> > >          d->shutdown_code = reason;
> > >      reason = d->shutdown_code;
> > >
> > > +#if defined(CONFIG_SYSTEM_SUSPEND) && defined(CONFIG_ARM)
> > > +    if ( reason != SHUTDOWN_suspend && is_hardware_domain(d) )
> > > +#else
> > >      if ( is_hardware_domain(d) )
> > > +#endif
> > >          hwdom_shutdown(reason);
> >
> > I still don't follow why Arm-specific code needs to live here. If this
> > can't be properly abstracted, then at the very least I'd expect some
> > code comment here, or at the very, very least something in the description.
>
> Looks like I missed your comment about this in the previous version of
> the patch series.
>
> >
> > From looking at hwdom_shutdown() I get the impression that it doesn't
> > expect to be called with SHUTDOWN_suspend, yet then the question is why we
> > make it into domain_shutdown() with that reason code.
>
> Thank you for the question, it is a good one.
>
> Thinking about it, with the current implementation (i.e. when the HW domain
> requests system suspend), we don't really need to call domain_shutdown().
> It would be enough to pause the last running vCPU (the current one) just to
> make sure that we don't return control to the domain after exiting from the
> hvc trap on the PSCI SYSTEM_SUSPEND command. We also need to set
> shutting_down to ensure that any asynchronous code or timer callbacks
> behave properly during suspend (i.e. skip their normal actions).

If we avoid calling domain_shutdown() for the hardware domain during
suspend, we would need to duplicate most of its logic except for the
hwdom_shutdown() call, which is not ideal.

To improve this, I suggest introducing a helper function:

    static inline bool need_hwdom_shutdown(const struct domain *d, u8 reason)
    {
        if ( IS_ENABLED(CONFIG_SYSTEM_SUSPEND) && IS_ENABLED(CONFIG_ARM) )
            return is_hardware_domain(d) && reason != SHUTDOWN_suspend;

        return is_hardware_domain(d);
    }

Then, in domain_shutdown(), we can call need_hwdom_shutdown() instead
of directly checking is_hardware_domain(d). This keeps the logic
readable and avoids code duplication.

What do you think about this approach?

>
> However, if we consider a setup with two separate domains -- one control and
> one HW -- where the control domain makes the final decision about system
> suspend, then we would need to call __domain_finalise_shutdown() during the
> HW domain suspend in order to notify the control domain that the HW domain
> state has changed. The control domain would then check this state and call
> system suspend for itself after confirming that all other domains are in a
> suspended state.
>
> I already added a TODO about moving this logic to the control domain. So, at
> first sight (unless I am missing something), we can avoid extra modifications
> inside domain_shutdown() and simply avoid calling it in case of HW domain.
>
> >
> > Jan
>
> Best regards,
> Mykola

Best regards,
Mykola