On Mon, Jan 30, 2023, Vipin Sharma wrote: > On Mon, Jan 30, 2023 at 10:09 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > > > On Sat, Jan 28, 2023, Vipin Sharma wrote: > > > On Wed, Jan 25, 2023 at 5:49 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > > > -static void handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn, > > > > - u64 old_spte, u64 new_spte, int level, > > > > - bool shared) > > > > -{ > > > > - __handle_changed_spte(kvm, as_id, gfn, old_spte, new_spte, level, > > > > - shared); > > > > handle_changed_spte_acc_track(old_spte, new_spte, level); > > > > - handle_changed_spte_dirty_log(kvm, as_id, gfn, old_spte, > > > > - new_spte, level); > > > > + > > > > + /* COMMENT GOES HERE. */ > > > > > > Current "shared" callers are not making a page dirty. If a new > > > "shared" caller makes a page dirty then make sure > > > handle_changed_spte_dirty_log is called. > > > > > > How is this? > > > > I was hoping for a more definitive "rule" than "KVM doesn't currently do XYZ". > > > > > > + if (!shared) > > > > + handle_changed_spte_dirty_log(kvm, as_id, gfn, old_spte, > > > > + new_spte, level); > > > > } > > > > > > What if implementation is changed a little more? I can think of two options: > > Option 1: > Remove handle_changed_spte_dirty_log() and let the callers handle > mark_page_dirty_in_slot(). Similar to how fast page faults do this. > This will get rid of the "shared" variable and defining its rules for > the shared and nonshared flow. > > Option 2: > Changing meaning of this variable from "shared" to something like > "handle_dirty_log" > Callers will know if they want dirty log to be handled or not. > > I am preferring option 1. Sorry, what I meant by "definitive rule" was an explanation of why KVM doesn't need to do dirty log tracking for tdp_mmu_set_spte_atomic(). Figured things out after a bit o' archaeology. Commit bcc4f2bc5026 ("KVM: MMU: mark page dirty in make_spte") shifted the dirtying of the memslot to make_spte(), and then commit 6ccf44388206 ("KVM: MMU: unify tdp_mmu_map_set_spte_atomic and tdp_mmu_set_spte_atomic_no_dirty_log") covered up the crime. Egad! I believe that means handle_changed_spte_dirty_log() is dead code for all intents and purposes, as there is no path that creates a WRITABLE 4KiB SPTE without bouncing through make_spte(). set_spte_gfn() => kvm_mmu_changed_pte_notifier_make_spte() only creates !WRITABLE SPTEs, ignoring for the moment that that's currently dead code too (see commit c13fda237f08 ("KVM: Assert that notifier count is elevated in .change_pte()")). So I _think_ we can do option #1 simply by deleting code.