Re: [PATCH 04/10] KVM: x86: Set PVCLOCK_GUEST_STOPPED only for kvmclock, not for Xen PV clock

[Paul Durrant <xadimgnik@gmail.com>]

On 21/01/2025 17:09, Sean Christopherson wrote:
> On Tue, Jan 21, 2025, Paul Durrant wrote:
>>> ---
>>>    arch/x86/kvm/x86.c | 20 ++++++++++++++------
>>>    1 file changed, 14 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index d8ee37dd2b57..3c4d210e8a9e 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -3150,11 +3150,6 @@ static void kvm_setup_guest_pvclock(struct kvm_vcpu *v,
>>>    	/* retain PVCLOCK_GUEST_STOPPED if set in guest copy */
>>>    	vcpu->hv_clock.flags |= (guest_hv_clock->flags & PVCLOCK_GUEST_STOPPED);
>>> -	if (vcpu->pvclock_set_guest_stopped_request) {
>>> -		vcpu->hv_clock.flags |= PVCLOCK_GUEST_STOPPED;
>>> -		vcpu->pvclock_set_guest_stopped_request = false;
>>> -	}
>>> -
>>>    	memcpy(guest_hv_clock, &vcpu->hv_clock, sizeof(*guest_hv_clock));
>>>    	if (force_tsc_unstable)
>>> @@ -3264,8 +3259,21 @@ static int kvm_guest_time_update(struct kvm_vcpu *v)
>>>    	if (use_master_clock)
>>>    		vcpu->hv_clock.flags |= PVCLOCK_TSC_STABLE_BIT;
>>> -	if (vcpu->pv_time.active)
>>> +	if (vcpu->pv_time.active) {
>>> +		/*
>>> +		 * GUEST_STOPPED is only supported by kvmclock, and KVM's
>>> +		 * historic behavior is to only process the request if kvmclock
>>> +		 * is active/enabled.
>>> +		 */
>>> +		if (vcpu->pvclock_set_guest_stopped_request) {
>>> +			vcpu->hv_clock.flags |= PVCLOCK_GUEST_STOPPED;
>>> +			vcpu->pvclock_set_guest_stopped_request = false;
>>> +		}
>>>    		kvm_setup_guest_pvclock(v, &vcpu->pv_time, 0, false);
>>> +
>>> +		vcpu->hv_clock.flags &= ~PVCLOCK_GUEST_STOPPED;
>>
>> Is this intentional? The line above your change in kvm_setup_guest_pvclock()
>> clearly keeps the flag enabled if it already set and, without this patch, I
>> don't see anything clearing it.
> 
> Oh, I see what you're getting at.  Hrm.  Yes, clearing the flag is intentional,
> otherwise the patch wouldn't do what it claims to do (set PVCLOCK_GUEST_STOPPED
> only for kvmclock).
> 
> Swapping the order of this patch and the next patch ("don't bleed ...") doesn't
> break the cycle because that would result in PVCLOCK_GUEST_STOPPED only being
> applied to the first active clock (kvmclock).
> 
> The only way I can think of to fully isolate the changes would be to split this
> into two patches: (4a) hoist pvclock_set_guest_stopped_request processing into
> kvm_guest_time_update() and (4b) apply it only to kvmclock, and then make the
> ordering 4a, 5, 4b, i.e. "hoist", "don't bleed", "only kvmclock".
> 
> 4a would be quite ugly, because to avoid introducing a functional change, it
> would need to be:
> 
> 	if (vcpu->pv_time.active || vcpu->xen.vcpu_info_cache.active ||
> 	    vcpu->xen.vcpu_time_info_cache.active) {
> 		vcpu->hv_clock.flags |= PVCLOCK_GUEST_STOPPED;
> 		vcpu->pvclock_set_guest_stopped_request = false;
> 	}
> 
> But it's not the worst intermediate code, so I'm not opposed to going that
> route.
> 

What about putting this change after patch 7. Then you could take a 
local copy of hv_clock in which you could set PVCLOCK_GUEST_STOPPED and 
so avoid bleeding the flag that way?

>>> +	}
>>> +
>>>    #ifdef CONFIG_KVM_XEN
>>>    	if (vcpu->xen.vcpu_info_cache.active)
>>>    		kvm_setup_guest_pvclock(v, &vcpu->xen.vcpu_info_cache,
>>