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

[Jan Beulich <jbeulich@xxxxxxxx>]

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