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

From: Gavin Shan <gshan@redhat.com>
To: Peter Xu <peterx@redhat.com>, Marc Zyngier <maz@kernel.org>
Cc: kvm@vger.kernel.org, catalin.marinas@arm.com,
	andrew.jones@linux.dev, will@kernel.org, shan.gavin@gmail.com,
	bgardon@google.com, dmatlack@google.com, pbonzini@redhat.com,
	zhenyzha@redhat.com, shuah@kernel.org,
	kvmarm@lists.cs.columbia.edu
Subject: Re: [PATCH 2/6] KVM: Add KVM_CAP_DIRTY_LOG_RING_ORDERED capability and config option
Date: Fri, 23 Sep 2022 10:04:18 +1000	[thread overview]
Message-ID: <71ebc42f-7148-87e5-4bdc-47924e583a88@redhat.com> (raw)
In-Reply-To: <YyzYI/bvp/JnbcxS@xz-m1.local>

Hi Peter and Marc,

On 9/23/22 7:48 AM, Peter Xu 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.
> 
> Pure question to ask: is it required to have a new cap just for the
> ordering?  IIUC if x86 was the only supported anyway before, it means all
> released old kvm binaries are always safe even without the strict
> orderings.  As long as we rework all the memory ordering bits before
> declaring support of yet another arch, we're good.  Or am I wrong?
> 

I have same questions. The name of CONFIG_HAVE_KVM_DIRTY_LOG is too
generic at least. I'm wandering why we even need other two kernel config
options, which are HAVE_KVM_DIRTY_{RING, RING_ORDER}.

- The ordering because of smp_load_acquire/smp_store_release is unconditionally
   applied to kvm_dirty_gfn_set_dirtied() and kvm_dirty_gfn_harvested() in PATCH[1/6].
- Both kernel config options are enabled on x86 in PATCH[3/6]

It means we needn't to differentiate strict/relaxed ordering by the extra
capability and kernel config options. If it makes sense, how about to let user
space decide strict ordering is needed base on the architecture (x86 vs ARM64 for now).

Thanks,
Gavin

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