[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kvm
Subject:    Re: [PATCH v7 5/9] KVM: arm64: Improve no-running-vcpu report for dirty ring
From:       Gavin Shan <gshan () redhat ! com>
Date:       2022-10-31 23:08:32
Message-ID: d3a4278a-94e2-7af4-da2d-946c903d8825 () redhat ! com
[Download RAW message or body]

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@google.com
> > Suggested-by: Sean Christopherson <seanjc@google.com>
> > Signed-off-by: Gavin Shan <gshan@redhat.com>
> 
> 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@redhat.com/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


[prev in list] [next in list] [prev in thread] [next in thread]