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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Thank you for the fast response.

On Tue, Sep 9, 2025 at 9:57 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 09.09.2025 08:29, Mykola Kvach wrote:
> > On Wed, Sep 3, 2025 at 7:31 AM Mykola Kvach <xakep.amatop@xxxxxxxxx> wrote:
> >> 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.
>
> That is, you effectively take back what you said earlier (as to not needing
> to call domain_shutdown())?

Sure. Looking more closely, I see that for the vCPUs, for example, many flags
are checked. In the case of the control domain initializing shutdown, I need
to see the __domain_finalise_shutdown() call.

We currently don’t have any functionality inside arch_domain_shutdown()
for ARM, but it would be nice to have it in the future. Calling
domain_shutdown() for every domain makes the code more consistent.

The flow for all domains will be the same during suspend, at least within
Xen’s internal code.

>
> > 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);
> >     }
>
> If I see a call to a function of this name, I'd expect the "hardware
> domain" nature already having been checked. I.e. a call site would
> rather look like
>
>     if ( is_hardware_domain(d) && need_hwdom_shutdown(d, reason) )
>         ...;
>

For me, the name simply indicates whether we need to call
hwdom_shutdown() or not, and I expect it to perform the check for whether
the domain is a hardware domain inside the function itself.

> > 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?
>
> Well, there's still the CONFIG_ARM check in there that I would like to
> see gone. (As a nit, the use of u8 would also want to go away.)

We could combine your proposal from v5 of this patch series, i.e., using the
HAS_HWDOM_SUSPEND extra config together with this helper function:

    static inline bool need_hwdom_shutdown(const struct domain *d)
    {
        bool is_hw_dom = is_hardware_domain(d);

        if ( !IS_ENABLED(CONFIG_HAS_HWDOM_SUSPEND) )
            return is_hw_dom && d->shutdown_code != SHUTDOWN_suspend;

        return is_hw_dom;
    }

As for the second argument (reason), I can extract it directly from the
domain structure, as is done in the function above.

>
> Furthermore with continuing to (ab)use domain_shutdown() also for the
> suspend case (Dom0 isn't really shut down when suspending, aiui), you
> retain the widening of the issue with the bogus setting of
> d->is_shutting_down (and hence the need for later clearing the flag
> again) that I mentioned elsewhere. (Yes, I remain of the opinion that
> you don't need to sort that as a prereq to your work, yet at the same
> time I think the goal should be to at least not make a bad situation
> worse.)

>From the perspective of ARM logic inside Xen, we perform the exact same
shutdown steps as for other domains, except that in the end we need to
call Xen suspend.

For a domain with a toolstack, it is possible to have a running Xen
watchdog service. For example, if we have systemd, it can be easily stopped
from the guest because we have hooks and can perform some actions before
suspend.

The same story applies to a Linux kernel driver: if it has PM ops installed
for the Xen watchdog driver, nothing bad happens.

However, in the case of using init.d, it isn’t easy to stop the Xen WDT
automatically right before suspend. Therefore, Xen code has an extra check
(see domain_watchdog_timeout) where it checks the is_shutting_down flag
and does nothing if it is set.

The is_shutting_down flag is easily reset on Xen resume via a
domain_resume call, so I don’t see any problems with that.

>
> Jan

Best regards,
Mykola