Re: [PATCH 1/6] KVM: Use acquire/release semantics when accessing dirty ring GFN state

[Marc Zyngier <maz@kernel.org>]

On Thu, 22 Sep 2022 22:38:33 +0100,
Peter Xu <peterx@redhat.com> wrote:
> 
> Marc,
> 
> On Thu, Sep 22, 2022 at 06:01:28PM +0100, Marc Zyngier wrote:
> > The current implementation of the dirty ring has an implicit requirement
> > that stores to the dirty ring from userspace must be:
> > 
> > - be ordered with one another
> > 
> > - visible from another CPU executing a ring reset
> > 
> > While these implicit requirements work well for x86 (and any other
> > TSO-like architecture), they do not work for more relaxed architectures
> > such as arm64 where stores to different addresses can be freely
> > reordered, and loads from these addresses not observing writes from
> > another CPU unless the required barriers (or acquire/release semantics)
> > are used.
> > 
> > In order to start fixing this, upgrade the ring reset accesses:
> > 
> > - the kvm_dirty_gfn_harvested() helper now uses acquire semantics
> >   so it is ordered after all previous writes, including that from
> >   userspace
> > 
> > - the kvm_dirty_gfn_set_invalid() helper now uses release semantics
> >   so that the next_slot and next_offset reads don't drift past
> >   the entry invalidation
> > 
> > This is only a partial fix as the userspace side also need upgrading.
> 
> Paolo has one fix 4802bf910e ("KVM: dirty ring: add missing memory
> barrier", 2022-09-01) which has already landed.

What is this commit? It doesn't exist in the kernel as far as I can see.

> 
> I think the other one to reset it was lost too.  I just posted a patch.
> 
> https://lore.kernel.org/qemu-devel/20220922213522.68861-1-peterx@redhat.com/
> (link still not yet available so far, but should be)

That's a QEMU patch, right?

> 
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  virt/kvm/dirty_ring.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/virt/kvm/dirty_ring.c b/virt/kvm/dirty_ring.c
> > index f4c2a6eb1666..784bed80221d 100644
> > --- a/virt/kvm/dirty_ring.c
> > +++ b/virt/kvm/dirty_ring.c
> > @@ -79,12 +79,12 @@ static inline void kvm_dirty_gfn_set_invalid(struct kvm_dirty_gfn *gfn)
> >  
> >  static inline void kvm_dirty_gfn_set_dirtied(struct kvm_dirty_gfn *gfn)
> >  {
> > -	gfn->flags = KVM_DIRTY_GFN_F_DIRTY;
> > +	smp_store_release(&gfn->flags, KVM_DIRTY_GFN_F_DIRTY);
> 
> IIUC you meant kvm_dirty_gfn_set_invalid as the comment says?

Gah, you're right, I redid the patch at the last minute and messed it
up....

> kvm_dirty_gfn_set_dirtied() has been guarded by smp_wmb() and AFAICT that's
> already safe.  Otherwise looks good to me.

Indeed. Let me fix this.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm