| Message ID | 20250118005552.2626804-6-seanjc@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: x86: pvclock fixes and cleanups | expand |
On 18/01/2025 00:55, Sean Christopherson wrote: > When updating a specific PV clock, make a full copy of KVM's reference > copy/cache so that PVCLOCK_GUEST_STOPPED doesn't bleed across clocks. > E.g. in the unlikely scenario the guest has enabled both kvmclock and Xen > PV clock, a dangling GUEST_STOPPED in kvmclock would bleed into Xen PV > clock. ... but the line I queried in the previous patch squashes the flag before the Xen PV clock is set up, so no bleed? > > Using a local copy of the pvclock structure also sets the stage for > eliminating the per-vCPU copy/cache (only the TSC frequency information > actually "needs" to be cached/persisted). > > Fixes: aa096aa0a05f ("KVM: x86/xen: setup pvclock updates") > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/x86.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 3c4d210e8a9e..5f3ad13a8ac7 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -3123,8 +3123,11 @@ static void kvm_setup_guest_pvclock(struct kvm_vcpu *v, > { > struct kvm_vcpu_arch *vcpu = &v->arch; > struct pvclock_vcpu_time_info *guest_hv_clock; > + struct pvclock_vcpu_time_info hv_clock; > unsigned long flags; > > + memcpy(&hv_clock, &vcpu->hv_clock, sizeof(hv_clock)); > + > read_lock_irqsave(&gpc->lock, flags); > while (!kvm_gpc_check(gpc, offset + sizeof(*guest_hv_clock))) { > read_unlock_irqrestore(&gpc->lock, flags); > @@ -3144,25 +3147,25 @@ static void kvm_setup_guest_pvclock(struct kvm_vcpu *v, > * it is consistent. > */ > > - guest_hv_clock->version = vcpu->hv_clock.version = (guest_hv_clock->version + 1) | 1; > + guest_hv_clock->version = hv_clock.version = (guest_hv_clock->version + 1) | 1; > smp_wmb(); > > /* retain PVCLOCK_GUEST_STOPPED if set in guest copy */ > - vcpu->hv_clock.flags |= (guest_hv_clock->flags & PVCLOCK_GUEST_STOPPED); > + hv_clock.flags |= (guest_hv_clock->flags & PVCLOCK_GUEST_STOPPED); > > - memcpy(guest_hv_clock, &vcpu->hv_clock, sizeof(*guest_hv_clock)); > + memcpy(guest_hv_clock, &hv_clock, sizeof(*guest_hv_clock)); > > if (force_tsc_unstable) > guest_hv_clock->flags &= ~PVCLOCK_TSC_STABLE_BIT; > > smp_wmb(); > > - guest_hv_clock->version = ++vcpu->hv_clock.version; > + guest_hv_clock->version = ++hv_clock.version; > > kvm_gpc_mark_dirty_in_slot(gpc); > read_unlock_irqrestore(&gpc->lock, flags); > > - trace_kvm_pvclock_update(v->vcpu_id, &vcpu->hv_clock); > + trace_kvm_pvclock_update(v->vcpu_id, &hv_clock); > } > > static int kvm_guest_time_update(struct kvm_vcpu *v)
On Tue, Jan 21, 2025, Paul Durrant wrote: > On 18/01/2025 00:55, Sean Christopherson wrote: > > When updating a specific PV clock, make a full copy of KVM's reference > > copy/cache so that PVCLOCK_GUEST_STOPPED doesn't bleed across clocks. > > E.g. in the unlikely scenario the guest has enabled both kvmclock and Xen > > PV clock, a dangling GUEST_STOPPED in kvmclock would bleed into Xen PV > > clock. > > ... but the line I queried in the previous patch squashes the flag before > the Xen PV clock is set up, so no bleed? Yeah, in practice, no bleed after the previous patch. But very theoretically, there could be bleed if the guest set PVCLOCK_GUEST_STOPPED in the compat clock *and* had both compat and non-compat Xen PV clocks active (is that even possible?) > > Using a local copy of the pvclock structure also sets the stage for > > eliminating the per-vCPU copy/cache (only the TSC frequency information > > actually "needs" to be cached/persisted). > > > > Fixes: aa096aa0a05f ("KVM: x86/xen: setup pvclock updates") > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- > > arch/x86/kvm/x86.c | 13 ++++++++----- > > 1 file changed, 8 insertions(+), 5 deletions(-) > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index 3c4d210e8a9e..5f3ad13a8ac7 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -3123,8 +3123,11 @@ static void kvm_setup_guest_pvclock(struct kvm_vcpu *v, > > { > > struct kvm_vcpu_arch *vcpu = &v->arch; > > struct pvclock_vcpu_time_info *guest_hv_clock; > > + struct pvclock_vcpu_time_info hv_clock; > > unsigned long flags; > > + memcpy(&hv_clock, &vcpu->hv_clock, sizeof(hv_clock)); > > + > > read_lock_irqsave(&gpc->lock, flags); > > while (!kvm_gpc_check(gpc, offset + sizeof(*guest_hv_clock))) { > > read_unlock_irqrestore(&gpc->lock, flags); > > @@ -3144,25 +3147,25 @@ static void kvm_setup_guest_pvclock(struct kvm_vcpu *v, > > * it is consistent. > > */ > > - guest_hv_clock->version = vcpu->hv_clock.version = (guest_hv_clock->version + 1) | 1; > > + guest_hv_clock->version = hv_clock.version = (guest_hv_clock->version + 1) | 1; > > smp_wmb(); > > /* retain PVCLOCK_GUEST_STOPPED if set in guest copy */ > > - vcpu->hv_clock.flags |= (guest_hv_clock->flags & PVCLOCK_GUEST_STOPPED); > > + hv_clock.flags |= (guest_hv_clock->flags & PVCLOCK_GUEST_STOPPED); > > - memcpy(guest_hv_clock, &vcpu->hv_clock, sizeof(*guest_hv_clock)); > > + memcpy(guest_hv_clock, &hv_clock, sizeof(*guest_hv_clock)); > > if (force_tsc_unstable) > > guest_hv_clock->flags &= ~PVCLOCK_TSC_STABLE_BIT; > > smp_wmb(); > > - guest_hv_clock->version = ++vcpu->hv_clock.version; > > + guest_hv_clock->version = ++hv_clock.version; > > kvm_gpc_mark_dirty_in_slot(gpc); > > read_unlock_irqrestore(&gpc->lock, flags); > > - trace_kvm_pvclock_update(v->vcpu_id, &vcpu->hv_clock); > > + trace_kvm_pvclock_update(v->vcpu_id, &hv_clock); > > } > > static int kvm_guest_time_update(struct kvm_vcpu *v) >
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 3c4d210e8a9e..5f3ad13a8ac7 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -3123,8 +3123,11 @@ static void kvm_setup_guest_pvclock(struct kvm_vcpu *v, { struct kvm_vcpu_arch *vcpu = &v->arch; struct pvclock_vcpu_time_info *guest_hv_clock; + struct pvclock_vcpu_time_info hv_clock; unsigned long flags; + memcpy(&hv_clock, &vcpu->hv_clock, sizeof(hv_clock)); + read_lock_irqsave(&gpc->lock, flags); while (!kvm_gpc_check(gpc, offset + sizeof(*guest_hv_clock))) { read_unlock_irqrestore(&gpc->lock, flags); @@ -3144,25 +3147,25 @@ static void kvm_setup_guest_pvclock(struct kvm_vcpu *v, * it is consistent. */ - guest_hv_clock->version = vcpu->hv_clock.version = (guest_hv_clock->version + 1) | 1; + guest_hv_clock->version = hv_clock.version = (guest_hv_clock->version + 1) | 1; smp_wmb(); /* retain PVCLOCK_GUEST_STOPPED if set in guest copy */ - vcpu->hv_clock.flags |= (guest_hv_clock->flags & PVCLOCK_GUEST_STOPPED); + hv_clock.flags |= (guest_hv_clock->flags & PVCLOCK_GUEST_STOPPED); - memcpy(guest_hv_clock, &vcpu->hv_clock, sizeof(*guest_hv_clock)); + memcpy(guest_hv_clock, &hv_clock, sizeof(*guest_hv_clock)); if (force_tsc_unstable) guest_hv_clock->flags &= ~PVCLOCK_TSC_STABLE_BIT; smp_wmb(); - guest_hv_clock->version = ++vcpu->hv_clock.version; + guest_hv_clock->version = ++hv_clock.version; kvm_gpc_mark_dirty_in_slot(gpc); read_unlock_irqrestore(&gpc->lock, flags); - trace_kvm_pvclock_update(v->vcpu_id, &vcpu->hv_clock); + trace_kvm_pvclock_update(v->vcpu_id, &hv_clock); } static int kvm_guest_time_update(struct kvm_vcpu *v)
When updating a specific PV clock, make a full copy of KVM's reference copy/cache so that PVCLOCK_GUEST_STOPPED doesn't bleed across clocks. E.g. in the unlikely scenario the guest has enabled both kvmclock and Xen PV clock, a dangling GUEST_STOPPED in kvmclock would bleed into Xen PV clock. Using a local copy of the pvclock structure also sets the stage for eliminating the per-vCPU copy/cache (only the TSC frequency information actually "needs" to be cached/persisted). Fixes: aa096aa0a05f ("KVM: x86/xen: setup pvclock updates") Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/x86.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)