Re: [PATCH 03/16] xen/arm: introduce a separate struct for watchdog timers

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

On 05.03.25 11:11, Mykola Kvach wrote:
> From: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
>
> Introduce a separate struct for watchdog timers. It is needed to properly
> implement the suspend/resume actions for the watchdog timers. To be able
> to restart watchdog timer after suspend we need to remember their
> frequency somewhere. To not bloat the struct timer a new struct
> watchdog_timer is introduced, containing the original timer and the last
> set timeout.
>
> Signed-off-by: Mykyta Poturai <mykyta_poturai@xxxxxxxx>
> Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> ---
> This commit was introduced in patch series V2.
> ---
>   xen/common/keyhandler.c    |  2 +-
>   xen/common/sched/core.c    | 11 ++++++-----
>   xen/include/xen/sched.h    |  3 ++-
>   xen/include/xen/watchdog.h |  6 ++++++
>   4 files changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
> index 0bb842ec00..caf614c0c2 100644
> --- a/xen/common/keyhandler.c
> +++ b/xen/common/keyhandler.c
> @@ -305,7 +305,7 @@ static void cf_check dump_domains(unsigned char key)
>           for ( i = 0 ; i < NR_DOMAIN_WATCHDOG_TIMERS; i++ )
>               if ( test_bit(i, &d->watchdog_inuse_map) )
>                   printk("    watchdog %d expires in %d seconds\n",
> -                       i, (u32)((d->watchdog_timer[i].expires - NOW()) >> 30));
> +                       i, (u32)((d->watchdog_timer[i].timer.expires - NOW()) 
> >> 30));
I'd like to propose to add watchdog API wrapper here, like

watchdog_domain_expires_sec(d,id)

or

watchdog_domain_dump(d)

and so hide implementation internals.

>   
>           arch_dump_domain_info(d);
>   
> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> index d6296d99fd..b1c6b6b9fa 100644
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -1556,7 +1556,8 @@ static long domain_watchdog(struct domain *d, uint32_t 
> id, uint32_t timeout)
>           {
>               if ( test_and_set_bit(id, &d->watchdog_inuse_map) )
>                   continue;
> -            set_timer(&d->watchdog_timer[id], NOW() + SECONDS(timeout));
> +            d->watchdog_timer[id].timeout = timeout;
> +            set_timer(&d->watchdog_timer[id].timer, NOW() + SECONDS(timeout));
>               break;
>           }
>           spin_unlock(&d->watchdog_lock);
> @@ -1572,12 +1573,12 @@ static long domain_watchdog(struct domain *d, uint32_t 
> id, uint32_t timeout)
>   
>       if ( timeout == 0 )
>       {
> -        stop_timer(&d->watchdog_timer[id]);
> +        stop_timer(&d->watchdog_timer[id].timer);
>           clear_bit(id, &d->watchdog_inuse_map);
>       }
>       else
>       {
> -        set_timer(&d->watchdog_timer[id], NOW() + SECONDS(timeout));
> +        set_timer(&d->watchdog_timer[id].timer, NOW() + SECONDS(timeout));
>       }
>   
>       spin_unlock(&d->watchdog_lock);
> @@ -1593,7 +1594,7 @@ void watchdog_domain_init(struct domain *d)
>       d->watchdog_inuse_map = 0;
>   
>       for ( i = 0; i < NR_DOMAIN_WATCHDOG_TIMERS; i++ )
> -        init_timer(&d->watchdog_timer[i], domain_watchdog_timeout, d, 0);
> +        init_timer(&d->watchdog_timer[i].timer, domain_watchdog_timeout, d, 0);
>   }
>   
>   void watchdog_domain_destroy(struct domain *d)
> @@ -1601,7 +1602,7 @@ void watchdog_domain_destroy(struct domain *d)
>       unsigned int i;
>   
>       for ( i = 0; i < NR_DOMAIN_WATCHDOG_TIMERS; i++ )
> -        kill_timer(&d->watchdog_timer[i]);
> +        kill_timer(&d->watchdog_timer[i].timer);
>   }
>   
>   /*
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 177784e6da..d0d10612ce 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -24,6 +24,7 @@
>   #include <asm/current.h>
>   #include <xen/vpci.h>
>   #include <xen/wait.h>
> +#include <xen/watchdog.h>
>   #include <public/xen.h>
>   #include <public/domctl.h>
>   #include <public/sysctl.h>
I think struct watchdog_timer (or whatever you going to add) need to be moved 
in sched.h
because...

> @@ -569,7 +570,7 @@ struct domain
>   #define NR_DOMAIN_WATCHDOG_TIMERS 2
>       spinlock_t watchdog_lock;
>       uint32_t watchdog_inuse_map;
> -    struct timer watchdog_timer[NR_DOMAIN_WATCHDOG_TIMERS];
> +    struct watchdog_timer watchdog_timer[NR_DOMAIN_WATCHDOG_TIMERS];
>   
>       struct rcu_head rcu;
>   
> diff --git a/xen/include/xen/watchdog.h b/xen/include/xen/watchdog.h
> index 4c2840bd91..2b7169632d 100644
> --- a/xen/include/xen/watchdog.h
> +++ b/xen/include/xen/watchdog.h
> @@ -8,6 +8,12 @@
>   #define __XEN_WATCHDOG_H__
>   
>   #include <xen/types.h>
> +#include <xen/timer.h>
...this interface is not related to domain's watchdogs.
From x86 code, it seems like some sort of HW watchdog used to check pCPUs state
and not domains/vcpu. And it's Not enabled for Arm now.

> +
> +struct watchdog_timer {
> +    struct timer timer;
> +    uint32_t timeout;
> +};
>   
>   #ifdef CONFIG_WATCHDOG
>