Re: [PATCH] x86/viridian: fix calling of viridian_time_domain_{freeze,thaw}()

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Nov 26, 2025 at 01:44:25PM +0100, Jan Beulich wrote:
> On 26.11.2025 12:29, Roger Pau Monne wrote:
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -1547,8 +1547,7 @@ int vcpu_unpause_by_systemcontroller(struct vcpu *v)
> >  static void _domain_pause(struct domain *d, bool sync)
> >  {
> >      struct vcpu *v;
> > -
> > -    atomic_inc(&d->pause_count);
> > +    bool was_paused = atomic_inc_return(&d->pause_count) - 1;
> >  
> >      if ( sync )
> >          for_each_vcpu ( d, v )
> 
> Isn't this racy? Another CPU doing the INC above just afterwards (yielding
> was_paused as false there) might still ...
> 
> > @@ -1557,7 +1556,8 @@ static void _domain_pause(struct domain *d, bool sync)
> >          for_each_vcpu ( d, v )
> >              vcpu_sleep_nosync(v);
> >  
> > -    arch_domain_pause(d);
> > +    if ( !was_paused )
> > +        arch_domain_pause(d);
> 
> ... make it here faster, and then the call would occur to late. Whether that's
> acceptable is a matter of what exactly the arch hook does.

It's acceptable for what the Viridian code does now, as there are no
current callers to domain_pause() that rely on the Viridian timers
being paused.

TBH the Viridian timers would better use the vPT logic, as that
avoids having to do this manual housekeeping.  I suspect vPT wasn't
used in the first place because when using SINTx the same SINTx could
be used for other purposes apart from the timer signaling.

As a result the current logic to attempt to account for missed ticks
is kind of bodged.  It doesn't detect guest EOIs, and hence doesn't
really know whether the previous interrupt has been processed ahead of
injecting a new one.

> Furthermore, is what the arch hook does for x86 actually correct when "sync"
> is false? The vCPU-s might then still be running while the Viridian time is
> already frozen.

I've also wondered about that aspect when using the nosync variant.  I
think it's fine to stop the timer ahead of the vCPU being paused, the
only difference would be whether a tick get delivered in that short
window ahead of the pause or afterwards, but that likely doesn't much
difference for the purpose here.

Maybe it's best to attempt to move the Viridian timers to use vPT
logic, and possibly get rid of the arch_domain_{,un}pause() hooks.

Thanks, Roger.