Re: [PATCH 08/16] xen/arm: add watchdog domain suspend/resume helpers

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.03.2025 12:00, Mykola Kvach wrote:
> On Fri, Mar 21, 2025 at 4:04 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 21.03.2025 10:50, Mykola Kvach wrote:
>>> On Thu, Mar 13, 2025 at 5:34 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>> On 05.03.2025 10:11, Mykola Kvach wrote:
>>>>> +void watchdog_domain_resume(struct domain *d)
>>>>> +{
>>>>> +    unsigned int i;
>>>>> +
>>>>> +    spin_lock(&d->watchdog_lock);
>>>>> +
>>>>> +    for ( i = 0; i < NR_DOMAIN_WATCHDOG_TIMERS; i++ )
>>>>> +    {
>>>>> +        if ( test_bit(i, &d->watchdog_inuse_map) )
>>>>> +        {
>>>>> +            set_timer(&d->watchdog_timer[i].timer,
>>>>> +                      NOW() + SECONDS(d->watchdog_timer[i].timeout));
>>>>
>>>> The timeout may have almost expired before suspending; restoring to the
>>>> full original period feels wrong. At the very least, if that's indeed
>>>> intended behavior, imo this needs spelling out explicitly.
>>>
>>> It takes some time to wake up the domain, but the watchdog timeout is
>>> reset by a userspace daemon. As a result, we can easily encounter a
>>> watchdog trigger during the  resume process.
>>
>> Which may mean the restoring is done too early, or needs doing in two
>> phases.
>>
>>> It looks like we should
>>> stop the watchdog timer from the guest, and in that case, we can drop
>>> all changes related to the watchdog in this patch series.

Noting this, ...

>> Except that then you require a guest to be aware of host suspend. Which
>> may not be desirable.
> 
> I think this is not a problem; at least, I don't see how the guest
> could be aware of the host suspend.

... perhaps it is me who is confused, but: With an unaware guest, how can
the stopping be done from the guest? I.e. ..

> For now, we have three cases:
> 
> 1) The guest is suspended (actually paused) because the system
> suspends, and we pause all non-hardware domains.

... in this case in particular, which this series is about aiui.

Jan

> 2) The guest is suspended via the `xl` tool (x86 only, at least for now).
> 3) The guest requests S2R via `echo mem > /sys/power/state` or
> `systemctl suspend`.
> 
> Let's review all these cases:
> 
> 1) There is no action required here; it should be handled correctly by
> domain pause. However, I think it is not handled properly right
> now—but that is not an issue with the current patch series.
> 2) There are potential problems here. We should either notify the
> domain that it will be suspended (which is hard to implement and the
> guest will be aware of the host suspending) or suspend watchdog
> directly during the execution of `xl` commands (more preferable).
> 3) As far as I know, if `watchdogd` is running, we can simply add an
> action to it on suspend/resume events (need to review not Linux kernel
> cases). In the case of the Linux kernel driver, it already handles
> suspending/resuming the Xen watchdog correctly.
> 
> So, if I am not mistaken, we can drop all patches related to watchdog
> suspend in Xen until `xl suspend/resume` for ARM64 is implemented. For
> other cases, we should handle suspend/resume of the watchdog via the
> `watchdogd` service.
> 
> Note: As far as I know, only the control domain has `watchdogd`
> (though we could potentially set it up for other domains). DomUs can
> only use the Xen watchdog Linux kernel driver.
> 
> ~Mykola