On Tue, Feb 7, 2023 at 9:47 AM David Matlack <dmatlack@xxxxxxxxxx> wrote: > > On Tue, Feb 7, 2023 at 9:37 AM Vipin Sharma <vipinsh@xxxxxxxxxx> wrote: > > > > On Mon, Feb 6, 2023 at 3:41 PM David Matlack <dmatlack@xxxxxxxxxx> wrote: > > > > > > On Fri, Feb 03, 2023 at 11:28:19AM -0800, Vipin Sharma wrote: > > > > > > if (wrprot || spte_ad_need_write_protect(iter.old_spte)) > > > clear_bits = PT_WRITABLE_MASK; > > > else > > > clear_bits = shadow_dirty_mask; > > > > > > if (!(iter->old_spte & clear_bits)) > > > continue; > > > > > > iter.old_spte = kvm_tdp_mmu_clear_spte_bit(&iter, clear_bits); > > > > > > > Yeah, this is better. Even better if I just initialize like: > > > > u64 clear_bits = shadow_dirty_mask; > > > > This will also get rid of the else part. > > On that topic... Do we need to recalculate clear_bits for every spte? > wrprot is passed as a parameter so that will not change. And > spte_ad_need_write_protect() should either return true or false for > all SPTEs in the TDP MMU. Specifically, make_spte() has this code: > > if (sp->role.ad_disabled) > spte |= SPTE_TDP_AD_DISABLED_MASK; > else if (kvm_mmu_page_ad_need_write_protect(sp)) > spte |= SPTE_TDP_AD_WRPROT_ONLY_MASK; > > sp->role.ad_disabled is never modified in TDP MMU pages. So it should > be constant for all pages. And kvm_mmu_page_ad_need_write_protect() > will always return false for TDP MMU pages since sp->role.guest_mode > is always false. > > So this can just be: > > u64 clear_bit = (wrprot || !kvm_ad_enabled()) ? PT_WRITABLE_MASK : > shadow_dirty_mask; I discussed it offline with David to understand more about it. It makes sense as TDP MMU pages will not have nested SPTEs (they are in rmaps). So, this looks good, I will add it in the next version. Thanks
