On Tue, Oct 11, 2022, Gavin Shan wrote: > This adds KVM_REQ_RING_SOFT_FULL, which is raised when the dirty "This" is basically "This patch", which is generally frowned upon. Just state what changes are being made. > ring of the specific VCPU becomes softly full in kvm_dirty_ring_push(). > The VCPU is enforced to exit when the request is raised and its > dirty ring is softly full on its entrance. > > The event is checked and handled in the newly introduced helper > kvm_dirty_ring_check_request(). With this, kvm_dirty_ring_soft_full() > becomes a private function. None of this captures why the request is being added. I'm guessing Marc's motivation is to avoid having to check ring on every entry, though there might also be a correctness issue too? It'd also be helpful to explain that KVM re-queues the request to maintain KVM's existing uABI, which enforces the soft_limit even if no entries have been added to the ring since the last KVM_EXIT_DIRTY_RING_FULL exit. And maybe call out the alternative(s) that was discussed in v2[*]? [*] https://lore.kernel.org/all/87illlkqfu.wl-maz@xxxxxxxxxx > Suggested-by: Marc Zyngier <maz@xxxxxxxxxx> > Signed-off-by: Gavin Shan <gshan@xxxxxxxxxx> > Reviewed-by: Peter Xu <peterx@xxxxxxxxxx> > --- > arch/x86/kvm/x86.c | 15 ++++++--------- > include/linux/kvm_dirty_ring.h | 8 ++------ > include/linux/kvm_host.h | 1 + > virt/kvm/dirty_ring.c | 19 ++++++++++++++++++- > 4 files changed, 27 insertions(+), 16 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index b0c47b41c264..0dd0d32073e7 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -10260,16 +10260,13 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > > bool req_immediate_exit = false; > > - /* Forbid vmenter if vcpu dirty ring is soft-full */ > - if (unlikely(vcpu->kvm->dirty_ring_size && > - kvm_dirty_ring_soft_full(&vcpu->dirty_ring))) { > - vcpu->run->exit_reason = KVM_EXIT_DIRTY_RING_FULL; > - trace_kvm_dirty_ring_exit(vcpu); > - r = 0; > - goto out; > - } > - > if (kvm_request_pending(vcpu)) { > + /* Forbid vmenter if vcpu dirty ring is soft-full */ Eh, I'd drop the comment, pretty obvious what the code is doing > + if (kvm_dirty_ring_check_request(vcpu)) { I think it makes to move this check below at KVM_REQ_VM_DEAD. I doubt it will ever matter in practice, but conceptually VM_DEAD is a higher priority event. I'm pretty sure the check can be moved to the very end of the request checks, e.g. to avoid an aborted VM-Enter attempt if one of the other request triggers KVM_REQ_RING_SOFT_FULL. Heh, this might actually be a bug fix of sorts. If anything pushes to the ring after the check at the start of vcpu_enter_guest(), then without the request, KVM would enter the guest while at or above the soft limit, e.g. record_steal_time() can dirty a page, and the big pile of stuff that's behind KVM_REQ_EVENT can certainly dirty pages. > + r = 0; > + goto out; > + } > + > if (kvm_check_request(KVM_REQ_VM_DEAD, vcpu)) { > r = -EIO; > goto out; > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -157,6 +157,7 @@ static inline bool is_error_page(struct page *page) > #define KVM_REQ_VM_DEAD (1 | KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP) > #define KVM_REQ_UNBLOCK 2 > #define KVM_REQ_UNHALT 3 UNHALT is gone, the new request can use '3'. > +#define KVM_REQ_RING_SOFT_FULL 4 Any objection to calling this KVM_REQ_DIRTY_RING_SOFT_FULL? None of the users are in danger of having too long lines, and at first glance it's not clear that this is specifically for the dirty ring. It'd also give us an excuse to replace spaces with tabs in the above alignment :-) #define KVM_REQ_TLB_FLUSH (0 | KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP) #define KVM_REQ_VM_DEAD (1 | KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP) #define KVM_REQ_UNBLOCK 2 #define KVM_REQ_DIRTY_RING_SOFT_FULL 3 #define KVM_REQUEST_ARCH_BASE 8 > @@ -149,6 +149,7 @@ int kvm_dirty_ring_reset(struct kvm *kvm, struct kvm_dirty_ring *ring) > > void kvm_dirty_ring_push(struct kvm_dirty_ring *ring, u32 slot, u64 offset) > { > + struct kvm_vcpu *vcpu = container_of(ring, struct kvm_vcpu, dirty_ring); > struct kvm_dirty_gfn *entry; > > /* It should never get full */ > @@ -166,6 +167,22 @@ void kvm_dirty_ring_push(struct kvm_dirty_ring *ring, u32 slot, u64 offset) > kvm_dirty_gfn_set_dirtied(entry); > ring->dirty_index++; > trace_kvm_dirty_ring_push(ring, slot, offset); > + > + if (kvm_dirty_ring_soft_full(ring)) > + kvm_make_request(KVM_REQ_RING_SOFT_FULL, vcpu); Would it make sense to clear the request in kvm_dirty_ring_reset()? I don't care about the overhead of having to re-check the request, the goal would be to help document what causes the request to go away. E.g. modify kvm_dirty_ring_reset() to take @vcpu and then do: if (!kvm_dirty_ring_soft_full(ring)) kvm_clear_request(KVM_REQ_RING_SOFT_FULL, vcpu); > +} > + > +bool kvm_dirty_ring_check_request(struct kvm_vcpu *vcpu) > +{ > + if (kvm_check_request(KVM_REQ_RING_SOFT_FULL, vcpu) && > + kvm_dirty_ring_soft_full(&vcpu->dirty_ring)) { Align please, if (kvm_check_request(KVM_REQ_RING_SOFT_FULL, vcpu) && kvm_dirty_ring_soft_full(&vcpu->dirty_ring)) { > + kvm_make_request(KVM_REQ_RING_SOFT_FULL, vcpu); A comment would be helpful to explain (a) why KVM needs to re-check on the next KVM_RUN and (b) why this won't indefinitely prevent KVM from entering the guest. For pretty every other request I can think of, re-queueing a request like this will effectively hang the vCPU, i.e. this looks wrong at first glance. > + vcpu->run->exit_reason = KVM_EXIT_DIRTY_RING_FULL; > + trace_kvm_dirty_ring_exit(vcpu); > + return true; > + } > + > + return false; > } > > struct page *kvm_dirty_ring_get_page(struct kvm_dirty_ring *ring, u32 offset) > -- > 2.23.0 > _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
