Re: [PATCH v19 056/130] KVM: x86/tdp_mmu: Init role member of struct kvm_mmu_page at allocation

[Isaku Yamahata]

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().
-- 
Isaku Yamahata <isaku.yamahata@xxxxxxxxx>