Re: [PATCH v2 0/6] KVM: Fix dirty-ring ordering on weakly ordered architectures

Gavin Shan
2022-09-27T00:58:26Z

On 9/27/22 12:51 AM, Marc Zyngier wrote:
> [Same distribution list as Gavin's dirty-ring on arm64 series]
>
> This is an update on the initial series posted as [0].
>
> As Gavin started posting patches enabling the dirty-ring infrastructure
> on arm64 [1], it quickly became apparent that the API was never intended
> to work on relaxed memory ordering architectures (owing to its x86
> origins).
>
> This series tries to retrofit some ordering into the existing API by:
>
> - relying on acquire/release semantics which are the default on x86,
> but need to be explicit on arm64
>
> - adding a new capability that indicate which flavor is supported, either
> with explicit ordering (arm64) or both implicit and explicit (x86),
> as suggested by Paolo at KVM Forum
>
> - documenting the requirements for this new capability on weakly ordered
> architectures
>
> - updating the selftests to do the right thing
>
> Ideally, this series should be a prefix of Gavin's, plus a small change
> to his series:
>
> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
> index 0309b2d0f2da..7785379c5048 100644
> --- a/arch/arm64/kvm/Kconfig
> +++ b/arch/arm64/kvm/Kconfig
> @@ -32,7 +32,7 @@ menuconfig KVM
> select KVM_VFIO
> select HAVE_KVM_EVENTFD
> select HAVE_KVM_IRQFD
> - select HAVE_KVM_DIRTY_RING
> + select HAVE_KVM_DIRTY_RING_ACQ_REL
> select HAVE_KVM_MSI
> select HAVE_KVM_IRQCHIP
> select HAVE_KVM_IRQ_ROUTING
>
> This has been very lightly tested on an arm64 box with Gavin's v3 [2] series.
>
> * From v1:
> - Repainted the config symbols and new capability so that their
> naming is more acceptable and causes less churn
> - Fixed a couple of blunders as pointed out by Peter and Paolo
> - Updated the documentation
>
> [0] https://lore.kernel.org/r/20220922170133.2617189-1-maz@kernel.org
> [1] https://lore.kernel.org/lkml/YyiV%2Fl7O23aw5aaO@xz-m1.local/T/
> [2] https://lore.kernel.org/r/20220922003214.276736-1-gshan@redhat.com
>
> Marc Zyngier (6):
> KVM: Use acquire/release semantics when accessing dirty ring GFN state
> KVM: Add KVM_CAP_DIRTY_LOG_RING_ACQ_REL capability and config option
> KVM: x86: Select CONFIG_HAVE_KVM_DIRTY_RING_ACQ_REL
> KVM: Document weakly ordered architecture requirements for dirty ring
> KVM: selftests: dirty-log: Upgrade flag accesses to acquire/release
> semantics
> KVM: selftests: dirty-log: Use KVM_CAP_DIRTY_LOG_RING_ACQ_REL if
> available
>
> Documentation/virt/kvm/api.rst | 17 +++++++++++++++--
> arch/x86/kvm/Kconfig | 3 ++-
> include/uapi/linux/kvm.h | 1 +
> tools/testing/selftests/kvm/dirty_log_test.c | 8 +++++---
> tools/testing/selftests/kvm/lib/kvm_util.c | 5 ++++-
> virt/kvm/Kconfig | 14 ++++++++++++++
> virt/kvm/dirty_ring.c | 4 ++--
> virt/kvm/kvm_main.c | 9 ++++++++-
> 8 files changed, 51 insertions(+), 10 deletions(-)
>

This series looks good to me.

Reviewed-by: Gavin Shan <gshan@redhat.com>

Thanks,
Gavin