On 10/31/22 5:08 PM, Oliver Upton wrote:
On Mon, Oct 31, 2022 at 08:36:17AM +0800, Gavin Shan wrote:
KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP should be enabled only when KVM
device "kvm-arm-vgic-its" is used by userspace. Currently, it's the
only case where a running VCPU is missed for dirty ring. However,
there are potentially other devices introducing similar error in
future.
In order to report those broken devices only, the no-running-vcpu
warning message is escaped from KVM device "kvm-arm-vgic-its". For
this, the function vgic_has_its() needs to be exposed with a more
generic function name (kvm_vgic_has_its()).
Link: https://lore.kernel.org/kvmarm/Y1ghIKrAsRFwSFsO@xxxxxxxxxx
Suggested-by: Sean Christopherson <seanjc@xxxxxxxxxx>
Signed-off-by: Gavin Shan <gshan@xxxxxxxxxx>
I don't think this should be added as a separate patch.
The weak kvm_arch_allow_write_without_running_vcpu() (and adding its
caller) should be rolled into patch 4/9. The arm64 implementation of
that should be introduced in patch 6/9.
Ok, the changes will be distributed in PATCH[4/9] and PATCH[6/9].
---
arch/arm64/kvm/mmu.c | 14 ++++++++++++++
arch/arm64/kvm/vgic/vgic-init.c | 4 ++--
arch/arm64/kvm/vgic/vgic-irqfd.c | 4 ++--
arch/arm64/kvm/vgic/vgic-its.c | 2 +-
arch/arm64/kvm/vgic/vgic-mmio-v3.c | 18 ++++--------------
arch/arm64/kvm/vgic/vgic.c | 10 ++++++++++
arch/arm64/kvm/vgic/vgic.h | 1 -
include/kvm/arm_vgic.h | 1 +
include/linux/kvm_dirty_ring.h | 1 +
virt/kvm/dirty_ring.c | 5 +++++
virt/kvm/kvm_main.c | 2 +-
11 files changed, 41 insertions(+), 21 deletions(-)
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 60ee3d9f01f8..e0855b2b3d66 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -932,6 +932,20 @@ void kvm_arch_mmu_enable_log_dirty_pt_masked(struct kvm *kvm,
kvm_mmu_write_protect_pt_masked(kvm, slot, gfn_offset, mask);
}
+/*
+ * kvm_arch_allow_write_without_running_vcpu - allow writing guest memory
+ * without the running VCPU when dirty ring is enabled.
+ *
+ * The running VCPU is required to track dirty guest pages when dirty ring
+ * is enabled. Otherwise, the backup bitmap should be used to track the
+ * dirty guest pages. When vgic/its is enabled, we need to use the backup
+ * bitmap to track the dirty guest pages for it.
+ */
+bool kvm_arch_allow_write_without_running_vcpu(struct kvm *kvm)
+{
+ return kvm->dirty_ring_with_bitmap && kvm_vgic_has_its(kvm);
+}
It is trivial for userspace to cause a WARN to fire like this. Just set
up the VM with !RING_WITH_BITMAP && ITS.
The goal is to catch KVM bugs, not userspace bugs, so I'd suggest only
checking whether or not an ITS is present.
[...]
Ok. 'kvm->dirty_ring_with_bitmap' needn't to be checked here if we don't
plan to catch userspace bug. Marc had suggestions to escape from the
no-running-vcpu check only when vgic/its tables are being restored [1].
In order to cover Marc's concern, I would introduce a different helper
kvm_vgic_save_its_tables_in_progress(), which simply returns
'bool struct vgic_dist::save_its_tables_in_progress'. The newly added
field is set and cleared in vgic_its_ctrl(). All these changes will be
folded to PATCH[v7 6/9]. Oliver and Marc, could you please let me know
if the changes sounds good?
static int vgic_its_ctrl(struct kvm *kvm, struct vgic_its *its, u64 attr)
{
const struct vgic_its_abi *abi = vgic_its_get_abi(its);
struct vgic_dist *dist = &kvm->arch.vgic;
int ret = 0;
:
switch (attr) {
case KVM_DEV_ARM_ITS_CTRL_RESET:
vgic_its_reset(kvm, its);
break;
case KVM_DEV_ARM_ITS_SAVE_TABLES:
dist->save_its_tables_in_progress = true;
ret = abi->save_tables(its);
dist->save_its_tables_in_progress = false;
break;
case KVM_DEV_ARM_ITS_RESTORE_TABLES:
ret = abi->restore_tables(its);
break;
}
:
}
[1] https://lore.kernel.org/kvmarm/2ce535e9-f57a-0ab6-5c30-2b8afd4472e6@xxxxxxxxxx/T/#mcf10e2d3ca0235ab1cac8793d894c1634666d280
diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index 91201f743033..10218057c176 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -38,20 +38,10 @@ u64 update_64bit_reg(u64 reg, unsigned int offset, unsigned int len,
return reg | ((u64)val << lower);
}
-bool vgic_has_its(struct kvm *kvm)
-{
- struct vgic_dist *dist = &kvm->arch.vgic;
-
- if (dist->vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
- return false;
-
- return dist->has_its;
-}
-
nit: renaming/exposing this helper should be done in a separate patch.
Also, I don't think you need to move it anywhere either.
[...]
As Marc suggested, we tend to escape the site of saving vgic/its tables from
the no-running-vcpu check. So we need a new helper kvm_vgic_save_its_tables_in_progress()
instead, meaning kvm_vgic_has_its() isn't needed.
diff --git a/virt/kvm/dirty_ring.c b/virt/kvm/dirty_ring.c
index 7ce6a5f81c98..f27e038043f3 100644
--- a/virt/kvm/dirty_ring.c
+++ b/virt/kvm/dirty_ring.c
@@ -26,6 +26,11 @@ bool kvm_use_dirty_bitmap(struct kvm *kvm)
return !kvm->dirty_ring_size || kvm->dirty_ring_with_bitmap;
}
+bool __weak kvm_arch_allow_write_without_running_vcpu(struct kvm *kvm)
+{
+ return kvm->dirty_ring_with_bitmap;
+}
+
Same comment on the arm64 implementation applies here. This should just
return false by default.
Ok. It return 'false' and the addition of kvm_arch_allow_write_without_running_vcpu()
will be folded to PATCH[4/9], as you suggested.
Thanks,
Gavin
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
