On Thu, Jan 26, 2023 at 2:38 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Thu, Jan 26, 2023, David Matlack wrote:
> > Replace sp->tdp_mmu_page with a bit in the page role. This reduces the
> > size of struct kvm_mmu_page by a byte.
>
> No it doesn't. I purposely squeezed the flag into an open byte in commit
>
> ca41c34cab1f ("KVM: x86/mmu: Relocate kvm_mmu_page.tdp_mmu_page for better cache locality")
>
> I double checked just to make sure: the size is 184 bytes before and after.
My mistake, thanks for pointing it out.
>
> I'm not opposed to this change, but I also don't see the point. The common code
> ends up with an arch hook in the appropriate place anyways[*], and I think we'll
> want to pay careful attention to the cache locality of the struct as whole, e.g.
> naively dumping the arch crud at the end of the common kvm_mmu_page structure may
> impact performance, especially for shadow paging.
>
> And just drop the WARN_ON() sanity check in kvm_tdp_mmu_put_root() .
>
> Hmm, actually, if we invert the order for the shadow MMU, e.g. embed "struct
> kvm_mmu_page" in a "struct kvm_shadow_mmu_page" or whatever, then the size of
> TDP MMU pages should shrink substantially.
>
> So my vote is to hold off for now and take a closer look at this in the common
> MMU series proper.
Sounds good to me.
>
> [*] https://lore.kernel.org/all/20221208193857.4090582-20-dmatlack@xxxxxxxxxx
>
> > Note that in tdp_mmu_init_sp() there is no need to explicitly set
> > sp->role.tdp_mmu=1 for every SP since the role is already copied to all
> > child SPs.
> >
> > Suggested-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > Link: https://lore.kernel.org/kvm/b0e8eb55-c2ee-ce13-8806-9d0184678984@xxxxxxxxxx/
>
> Drop the trailing slash, otherwise directly clicking the link goes sideways.
I don't have any problem clicking this link, but I can try to omit the
trailing slash in the future.
