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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi,

On Wed, Mar 19, 2025 at 6:14 PM Grygorii Strashko
<grygorii_strashko@xxxxxxxx> wrote:
>
>
>
> 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.

It was already proposed by Jan Beulich. I'll do it.

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

Sorry, but maybe I missed something. However, this struct and the
previous watchdog timer
are used as fields of the domain struct and correspond to a particular
domain. Also, take a look
at some functions where the watchdog timer field is used: domain_watchdog,
watchdog_domain_init, and watchdog_domain_destroy.
I see a direct connection with a domain..

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

Best regards,
Mykola