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

[Marc Zyngier <maz@kernel.org>]

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).
> 
> 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.

Thoughts?

	M.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/dirty-log-ordered-bikeshed

-- 
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