linux-kernel - Re: [PATCH v19 058/130] KVM: x86/mmu: Add a private pointer to struct kvm_mmu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b0e82a9c262ba335cc27bc9921b8f86bb0a6676f.camel@intel.com>
Date: Thu, 14 Mar 2024 21:52:58 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "Yamahata, Isaku" <isaku.yamahata@...el.com>
CC: "Zhang, Tina" <tina.zhang@...el.com>, "isaku.yamahata@...ux.intel.com"
	<isaku.yamahata@...ux.intel.com>, "seanjc@...gle.com" <seanjc@...gle.com>,
	"Huang, Kai" <kai.huang@...el.com>, "binbin.wu@...ux.intel.com"
	<binbin.wu@...ux.intel.com>, "Chen, Bo2" <chen.bo@...el.com>,
	"sagis@...gle.com" <sagis@...gle.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "Yuan, Hang" <hang.yuan@...el.com>, "Aktas,
 Erdem" <erdemaktas@...gle.com>, "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
	"pbonzini@...hat.com" <pbonzini@...hat.com>, "isaku.yamahata@...il.com"
	<isaku.yamahata@...il.com>
Subject: Re: [PATCH v19 058/130] KVM: x86/mmu: Add a private pointer to struct
 kvm_mmu_page

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