Received-SPF: pass (google.com: domain of linux-kernel+bounces-89656-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Sun, 3 Mar 2024 12:47:19 +0800
From: Xu Yilun <yilun.xu@linux.intel.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, seanjc@google.com,
	michael.roth@amd.com, isaku.yamahata@intel.com,
	thomas.lendacky@amd.com
Subject: Re: [PATCH 11/21] KVM: x86/tdp_mmu: Init role member of struct
 kvm_mmu_page at allocation
Message-ID: <ZeQA12SlPPSgLBGG@yilunxu-OptiPlex-7050>
References: <20240227232100.478238-1-pbonzini@redhat.com>
 <20240227232100.478238-12-pbonzini@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240227232100.478238-12-pbonzini@redhat.com>

On Tue, Feb 27, 2024 at 06:20:50PM -0500, Paolo Bonzini wrote:
> From: Isaku Yamahata <isaku.yamahata@intel.com>
> 
> Refactor tdp_mmu_alloc_sp() and tdp_mmu_init_sp and eliminate
                                  ^
tdp_mmu_init_sp() 

> tdp_mmu_init_child_sp().  Currently tdp_mmu_init_sp() (or
> tdp_mmu_init_child_sp()) sets kvm_mmu_page.role after tdp_mmu_alloc_sp()
> allocating struct kvm_mmu_page and its page table page.  This patch makes
> tdp_mmu_alloc_sp() initialize kvm_mmu_page.role instead of
> tdp_mmu_init_sp().
> 
> 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

This section is hard to understand. I'm lost at which functions are
mentioned here that took the level argument and should be replaced by
role.

> and more info about sp can be passed down.

My understanding of the change is:

Extra handling is need for Allocation of private page tables, so
earlier caculate the kvm_mmu_page_role for the sp and pass it to
tdp_mmu_alloc_sp().  Since the sp.role could be decided on sp
allocation, in turn remove the role argument for tdp_mmu_init_sp(), also
eliminate the helper tdp_mmu_init_child_sp().

> 
> 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
        ^

Should be kvm_mmu_page_role

Thanks,
Yilun