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,
