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

[Jan Beulich <jbeulich@xxxxxxxx>]

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())?

> 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) )
        ...;

> 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.)

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.)

Jan