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 *.
