On Fri, Mar 22, 2024 at 03:18:39PM +0800, Chao Gao <chao.gao@xxxxxxxxx> wrote: > On Thu, Mar 21, 2024 at 02:24:12PM -0700, Isaku Yamahata wrote: > >On Thu, Mar 21, 2024 at 12:11:11AM +0000, > >"Edgecombe, Rick P" <rick.p.edgecombe@xxxxxxxxx> wrote: > > > >> On Mon, 2024-02-26 at 00:25 -0800, isaku.yamahata@xxxxxxxxx wrote: > >> > To handle private page tables, argument of is_private needs to be > >> > passed > >> > down. Given that already page level is passed down, it would be > >> > cumbersome > >> > to add one more parameter about sp. Instead replace the level > >> > argument with > >> > union kvm_mmu_page_role. Thus the number of argument won't be > >> > increased > >> > and more info about sp can be passed down. > >> > > >> > For private sp, secure page table will be also allocated in addition > >> > to > >> > struct kvm_mmu_page and page table (spt member). The allocation > >> > functions > >> > (tdp_mmu_alloc_sp() and __tdp_mmu_alloc_sp_for_split()) need to know > >> > if the > >> > allocation is for the conventional page table or private page table. > >> > Pass > >> > union kvm_mmu_role to those functions and initialize role member of > >> > struct > >> > kvm_mmu_page. > >> > >> tdp_mmu_alloc_sp() is only called in two places. One for the root, and > >> one for the mid-level tables. > >> > >> In later patches when the kvm_mmu_alloc_private_spt() part is added, > >> the root case doesn't need anything done. So the code has to take > >> special care in tdp_mmu_alloc_sp() to avoid doing anything for the > >> root. > >> > >> It only needs to do the special private spt allocation in non-root > >> case. If we open code that case, I think maybe we could drop this > >> patch, like the below. > >> > >> The benefits are to drop this patch (which looks to already be part of > >> Paolo's series), and simplify "KVM: x86/mmu: Add a private pointer to > >> struct kvm_mmu_page". I'm not sure though, what do you think? Only > >> build tested. > > > >Makes sense. Until v18, it had config to disable private mmu part at > >compile time. Those functions have #ifdef in mmu_internal.h. v19 > >dropped the config for the feedback. > > https://lore.kernel.org/kvm/Zcrarct88veirZx7@xxxxxxxxxx/ > > > >After looking at mmu_internal.h, I think the following three function could be > >open coded. > >kvm_mmu_private_spt(), kvm_mmu_init_private_spt(), kvm_mmu_alloc_private_spt(), > >and kvm_mmu_free_private_spt(). > > It took me a few minutes to figure out why the mirror root page doesn't need > a private_spt. > > Per TDX module spec: > > Secure EPT’s root page (EPML4 or EPML5, depending on whether the host VMM uses > 4-level or 5-level EPT) does not need to be explicitly added. It is created > during TD initialization (TDH.MNG.INIT) and is stored as part of TDCS. > > I suggest adding the above as a comment somewhere even if we decide to open-code > kvm_mmu_alloc_private_spt(). 058/130 has such comment. The citation from the spec would be better. > IMO, some TDX details bleed into KVM MMU regardless of whether we open-code > kvm_mmu_alloc_private_spt() or not. This isn't good though I cannot think of > a better solution. > -- Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
