The current hook placement in domain_{,un}pause() calls arch_domain_pause()
unconditionally, regardless of whether the domain is already paused or not
unpaused (so no state change).  However the underlying Viridian unpause
function (time_ref_count_thaw()) expects the call to only be done once
there's a transition from paused to active.

Adjust the calling of arch_domain_{,un}pause() so it's only done for state
changes, to accommodate for the behavior expected by the Viridian hooks.
Note there are no other implementations of arch_domain_{,un}pause() apart
from the Viridian use cases, and hence the change is non-functional for all
other architectures (or for x86 if Viridian is disabled).

Fixes: f6a07643e1cc ('x86/viridian: freeze time reference counter when domain is paused')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/common/domain.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 3b6e9471c413..e0ca71a53bc1 100644
--- 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 )
@@ -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);
 }
 
 void domain_pause(struct domain *d)
@@ -1575,11 +1575,12 @@ void domain_unpause(struct domain *d)
 {
     struct vcpu *v;
 
-    arch_domain_unpause(d);
-
     if ( atomic_dec_and_test(&d->pause_count) )
+    {
+        arch_domain_unpause(d);
         for_each_vcpu( d, v )
             vcpu_wake(v);
+    }
 }
 
 static int _domain_pause_by_systemcontroller(struct domain *d, bool sync)
-- 
2.51.0

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.

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.

Jan

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.

On 26.11.2025 14:49, Roger Pau Monné wrote:
> 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.

That may be more intrusive a change. I was kind of hoping to confine the
less invasive change here to the Viridian freeze/thaw functions.

Jan