On Fri, Nov 11, 2022 at 06:19:15PM +0000, Sean Christopherson wrote: > TL;DR: I'm going to try to add more aggressive patches for this into my > series to > clean up the KVM side of things, along with many more patches to clean up the > page > track APIs. > > I'll post patches next week if things go well (fingers crossed), and if not > I'll > give an update > > On Fri, Nov 11, 2022, Yan Zhao wrote: > > Page track hook track_remove_slot is used to notify users that a slot > > has been removed and is called when a slot DELETE/MOVE is about to be > > completed. > > Phrase this as a command, and explain _why_ the new hook is being added, e.g. > > Add a new page track hook, track_remove_slot(), that is called when a > memslot DELETE/MOVE operation is about to be committed. The "remove" > hook will be used by KVMGT and will effectively replace the existing > track_flush_slot() altogether now that KVM itself doesn't rely on the > "flush" hook either. > > The "flush" hook is flawed as it's invoked before the memslot operation > is guaranteed, i.e. KVM might ultimately keep the existing memslot without > notifying external page track users, a.k.a. KVMGT. > > > Users of this hook can drop write protections in the removed slot. > > Hmm, actually, on second thought, after thinking about what KVGT is doing in > response to the memslot update, I think we should be more aggressive and > actively > prevent MOVE if there are external page trackers, i.e. if KVMGT is attached. > > Dropping write protections when a memslot is being deleted is a waste of > cycles. > The memslot and thus gfn_track is literally being deleted immediately after > invoking > the hook, updating gfn_track from KVMGT is completely unecessary.
> I.e. if we kill off the MOVE path, then KVMGT just needs to delete its hash > table > entry. > > Oooh! Looking at this code again made me realize that the larger page track > cleanup > that I want to do might actually work. Long story short, I want to stop > forcing > KVMGT to poke into KVM internals, but I thought there was a lock inversion > problem. > > But AFAICT, there is no such problem. And the cleanup I want to do will > actually > fix an existing KVMGT bug: kvmgt_page_track_write() invokes > kvmgt_gfn_is_write_protected() > without holding mmu_lock, and thus could consume garbage when walking the hash > table. > > static void kvmgt_page_track_write(struct kvm_vcpu *vcpu, gpa_t gpa, > const u8 *val, int len, > struct kvm_page_track_notifier_node *node) > { > struct intel_vgpu *info = > container_of(node, struct intel_vgpu, track_node); > > if (kvmgt_gfn_is_write_protected(info, gpa_to_gfn(gpa))) > intel_vgpu_page_track_handler(info, gpa, > (void *)val, len); > } > > Acquiring mmu_lock isn't an option as intel_vgpu_page_track_handler() might > sleep, > e.g. when acquiring vgpu_lock. > I totally agree with you and actually had the same feeling as you when examined the code yesterday. But I thought I'd better send this series first and do the cleanup later :) And I'm also not sure if a slots_arch_lock is required for kvm_slot_page_track_add_page() and kvm_slot_page_track_remove_page(). (Though it doesn't matter for a removing slot and we actually needn't to call kvm_slot_page_track_remove_page() in response to a slot removal, the two interfaces are still required elsewhere.) > Let me see if the clean up I have in mind will actually work. If it does, I > think > the end result will be quite nice for both KVM and KVMGT. Yes, it would be great. Thanks Yan