Re: [PATCH] KVM: x86/mmu: Replace tdp_mmu_page with a bit in the role

[Sean Christopherson <seanjc@google.com>]

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.

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.

[*] https://lore.kernel.org/all/20221208193857.4090582-20-dmatlack@google.com

> 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@redhat.com>
> Link: https://lore.kernel.org/kvm/b0e8eb55-c2ee-ce13-8806-9d0184678984@redhat.com/

Drop the trailing slash, otherwise directly clicking the link goes sideways.

> Signed-off-by: David Matlack <dmatlack@google.com>
> ---