Re: [PATCH v19 058/130] KVM: x86/mmu: Add a private pointer to struct kvm_mmu_page

[Edgecombe, Rick P]

On Thu, 2024-03-14 at 11:10 -0700, Isaku Yamahata wrote:
> > I think the point of putting these in a union is that they only
> > apply
> > to shadow paging and so can't be used with TDX. I think you are
> > putting
> > more than the sizeof(void *) in there as there are multiple in the
> > same
> > category.
> 
> I'm not sure if I'm following you.
> On x86_64, sizeof(unsigned int) = 4, sizeof(atomic_t) = 4,
> sizeof(void *) = 8.
> I moved write_flooding_count to have 8 bytes.

Ah, I see. Yes you are write about it summing to 8. Ok, what do you
think about putting a comment that these will always be unused with
TDX?

> 
> 
> > But there seems to be a new one added, *shadowed_translation.
> > Should it go in there too? Is the union because there wasn't room
> > before, or just to be tidy?
> 
> Originally TDX MMU support was implemented for legacy tdp mmu.  It
> used
> shadowed_translation.  It was not an option at that time.  Later we
> switched to
> (new) TDP MMU.  Now we have choice to which member to overlay.
> 
> 
> > I think the commit log should have more discussion of this union
> > and
> > maybe a comment in the struct to explain the purpose of the
> > organization. Can you explain the reasoning now for the sake of
> > discussion?
> 
> Sure.  We'd like to add void * pointer to struct kvm_mmu_page.  Given
> some
> members are used only for legacy KVM MMUs and not used for TDP MMU,
> we can save
> memory overhead with union.  We have options.
> - u64 *shadowed_translation
>   This was not chosen for the old implementation. Now this is option.

This seems a little more straighforward, but I'm on the fence if it's
worth changing.

> - pack unsync_children and write_flooding_count for 8 bytes
>   This patch chosen this for historical reason. Other two option is
> possible.
> - unsync_child_bitmap
>   Historically it was unioned with other members. But now it's not.
> 
> I don't have strong preference for TDX support as long as we can have
> void *.