On Wed, Jan 25, 2023 at 2:25 PM Ben Gardon <bgardon@xxxxxxxxxx> wrote: > > On Wed, Jan 25, 2023 at 2:00 PM David Matlack <dmatlack@xxxxxxxxxx> wrote: > > > > On Wed, Jan 25, 2023 at 01:38:57PM -0800, Vipin Sharma wrote: > > > @@ -736,7 +749,12 @@ static u64 __tdp_mmu_set_spte(struct kvm *kvm, int as_id, tdp_ptep_t sptep, > > > > > > old_spte = kvm_tdp_mmu_write_spte(sptep, old_spte, new_spte, level); > > > > > > - __handle_changed_spte(kvm, as_id, gfn, old_spte, new_spte, level, false); > > > + if (record_dirty_log) > > > + __handle_changed_spte(kvm, as_id, gfn, old_spte, new_spte, > > > + level, false); > > > + else > > > + handle_changed_spte_clear_dirty_log(as_id, gfn, old_spte, > > > + new_spte, level); > > > > I find it very non-intuitive to tie this behavior to !record_dirty_log, > > even though it happens to work. It's also risky if any future calls are > > added that pass record_dirty_log=false but change other bits in the > > SPTE. > > > > I wonder if we could get the same effect by optimizing > > __handle_changed_spte() to check for a cleared D-bit *first* and if > > that's the only diff between the old and new SPTE, bail immediately > > rather than doing all the other checks. > > I would also prefer that course. One big lesson I took when building > the TDP MMU was the value of having all the changed PTE handling go > through one function. That's not always the right choice but the > Shadow MMU has a crazy spaghetti code of different functions which > handle different parts of responding to a PTE change and it makes the > code very hard to read. I agree this path is worth optimizing, but the > more we can keep the code together, the better. Let me see if I am able to get the same improvement in __handle_changed_spte().
