Re: [PATCH 2/6] KVM: Add KVM_CAP_DIRTY_LOG_RING_ORDERED capability and config option

[Peter Xu <peterx@redhat.com>]

On Sat, Sep 24, 2022 at 12:26:53PM +0100, Marc Zyngier wrote:
> On Sat, 24 Sep 2022 09:51:39 +0100,
> Marc Zyngier <maz@kernel.org> wrote:
> > 
> > On Fri, 23 Sep 2022 19:26:18 +0100,
> > Peter Xu <peterx@redhat.com> wrote:
> > > 
> > > On Fri, Sep 23, 2022 at 03:28:34PM +0100, Marc Zyngier wrote:
> > > > On Thu, 22 Sep 2022 22:48:19 +0100,
> > > > Peter Xu <peterx@redhat.com> wrote:
> > > > > 
> > > > > On Thu, Sep 22, 2022 at 06:01:29PM +0100, Marc Zyngier wrote:
> > > > > > In order to differenciate between architectures that require no extra
> > > > > > synchronisation when accessing the dirty ring and those who do,
> > > > > > add a new capability (KVM_CAP_DIRTY_LOG_RING_ORDERED) that identify
> > > > > > the latter sort. TSO architectures can obviously advertise both, while
> > > > > > relaxed architectures most only advertise the ORDERED version.
> > > > > > 
> > > > > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > > > > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > > > > > ---
> > > > > >  include/linux/kvm_dirty_ring.h |  6 +++---
> > > > > >  include/uapi/linux/kvm.h       |  1 +
> > > > > >  virt/kvm/Kconfig               | 14 ++++++++++++++
> > > > > >  virt/kvm/Makefile.kvm          |  2 +-
> > > > > >  virt/kvm/kvm_main.c            | 11 +++++++++--
> > > > > >  5 files changed, 28 insertions(+), 6 deletions(-)
> > > > > > 
> > > > > > diff --git a/include/linux/kvm_dirty_ring.h b/include/linux/kvm_dirty_ring.h
> > > > > > index 906f899813dc..7a0c90ae9a3f 100644
> > > > > > --- a/include/linux/kvm_dirty_ring.h
> > > > > > +++ b/include/linux/kvm_dirty_ring.h
> > > > > > @@ -27,7 +27,7 @@ struct kvm_dirty_ring {
> > > > > >  	int index;
> > > > > >  };
> > > > > >  
> > > > > > -#ifndef CONFIG_HAVE_KVM_DIRTY_RING
> > > > > > +#ifndef CONFIG_HAVE_KVM_DIRTY_LOG
> > > > > 
> > > > > s/LOG/LOG_RING/ according to the commit message? Or the name seems too
> > > > > generic.
> > > > 
> > > > The commit message talks about the capability, while the above is the
> > > > config option. If you find the names inappropriate, feel free to
> > > > suggest alternatives (for all I care, they could be called FOO, BAR
> > > > and BAZ).
> > > 
> > > The existing name from David looks better than the new one.. to me.
> > 
> > I'm happy to bikeshed, but please spell it out for me. If we follow
> > the current scheme, we need 3 configuration symbols (of which we
> > already have one), and 2 capabilities (of which we already have one).

I hope it's not bikeshedding.  I normally don't comment on namings at all
because many of them can be "bikeshedding" to me.  But this one is so
special because it directly collides with KVM_GET_DIRTY_LOG, which is other
method of dirty tracking.

> > 
> > Do you have any concrete proposal for those?
> 
> In order to make some forward progress, I've reworked the series[1]
> with another proposal for those:
> 
> Config symbols:
> 
> - HAVE_KVM_DIRTY_RING:
>   * mostly the same meaning as today
>   * not directly selected by any architecture
>   * doesn't expose any capability on its own
> 
> - HAVE_KVM_DIRTY_RING_TSO:
>   * only for strongly ordered architectures
>   * selects HAVE_KVM_DIRTY_RING
>   * exposes KVM_CAP_DIRTY_LOG_RING
>   * selected by x86
> 
> - HAVE_KVM_DIRTY_RING_ACQ_REL:
>   * selects HAVE_KVM_DIRTY_RING
>   * exposes KVM_CAP_DIRTY_LOG_RING_ACQ_REL
>   * selected by arm64 and x86
> 
> Capabilities:
> 
> - KVM_CAP_DIRTY_LOG_RING: the good old x86-specific stuff, advertised
>   when HAVE_KVM_DIRTY_RING_TSO is selected
> 
> - KVM_CAP_DIRTY_LOG_RING_ACQ_REL: the new acquire/release semantics,
>   advertised when HAVE_KVM_DIRTY_RING_ACQ_REL is selected
> 
> This significantly reduces the churn and makes things slightly more
> explicit.

This looks good to me, thanks.

-- 
Peter Xu

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm