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);
> > }
> >
> > /*
> > * tdp_mmu_set_spte_atomic - Set a TDP MMU SPTE atomically
> > - * and handle the associated bookkeeping. Do not mark the page dirty
> > - * in KVM's dirty bitmaps.
> > + * and handle the associated bookkeeping.
> > *
> > * If setting the SPTE fails because it has changed, iter->old_spte will be
> > * refreshed to the current value of the spte.
...
> > @@ -1703,9 +1657,11 @@ static void clear_dirty_pt_masked(struct kvm *kvm, struct kvm_mmu_page *root,
> > new_spte = iter.old_spte & ~shadow_dirty_mask;
> > else
> > continue;
> > +
> > + kvm_set_pfn_dirty(spte_to_pfn(iter.old_spte));
> > }
> >
>
> Shouldn't we handle spte_ad_need_write_protect(iter.old_spte)
> separately and if this function returns true then on clearing
> PT_WRITABLE_MASK, kvm_set_pfn_dirty be called?
> My understanding is that the spte_ad_need_write_protect() will return
> true for nested VM sptes when PML mode is enabled.
Ah rats. I missed that is_dirty_spte() checks WRITABLE in that case. So yeah,
kvm_set_pfn_dirty() should be called in both paths. I was thinking KVM would mark
the page dirty when faulting the PFN for write, but I have my flows all mixed up.
