Date:   Tue, 11 Jul 2023 13:42:10 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Kai Huang <kai.huang@...el.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org
Cc:     linux-mm@...ck.org, x86@...nel.org, dave.hansen@...el.com,
        kirill.shutemov@...ux.intel.com, tony.luck@...el.com,
        peterz@...radead.org, tglx@...utronix.de, bp@...en8.de,
        mingo@...hat.com, hpa@...or.com, seanjc@...gle.com,
        pbonzini@...hat.com, dan.j.williams@...el.com,
        rafael.j.wysocki@...el.com, ashok.raj@...el.com,
        reinette.chatre@...el.com, len.brown@...el.com, ak@...ux.intel.com,
        isaku.yamahata@...el.com, ying.huang@...el.com, chao.gao@...el.com,
        sathyanarayanan.kuppuswamy@...ux.intel.com, nik.borisov@...e.com,
        bagasdotme@...il.com, sagis@...gle.com, imammedo@...hat.com
Subject: Re: [PATCH v12 12/22] x86/virt/tdx: Allocate and set up PAMTs for
 TDMRs

On 26.06.23 16:12, Kai Huang wrote:
> The TDX module uses additional metadata to record things like which
> guest "owns" a given page of memory.  This metadata, referred as
> Physical Address Metadata Table (PAMT), essentially serves as the
> 'struct page' for the TDX module.  PAMTs are not reserved by hardware
> up front.  They must be allocated by the kernel and then given to the
> TDX module during module initialization.
> 
> TDX supports 3 page sizes: 4K, 2M, and 1G.  Each "TD Memory Region"
> (TDMR) has 3 PAMTs to track the 3 supported page sizes.  Each PAMT must
> be a physically contiguous area from a Convertible Memory Region (CMR).
> However, the PAMTs which track pages in one TDMR do not need to reside
> within that TDMR but can be anywhere in CMRs.  If one PAMT overlaps with
> any TDMR, the overlapping part must be reported as a reserved area in
> that particular TDMR.
> 
> Use alloc_contig_pages() since PAMT must be a physically contiguous area
> and it may be potentially large (~1/256th of the size of the given TDMR).
> The downside is alloc_contig_pages() may fail at runtime.  One (bad)
> mitigation is to launch a TDX guest early during system boot to get
> those PAMTs allocated at early time, but the only way to fix is to add a
> boot option to allocate or reserve PAMTs during kernel boot.
> 
> It is imperfect but will be improved on later.
> 
> TDX only supports a limited number of reserved areas per TDMR to cover
> both PAMTs and memory holes within the given TDMR.  If many PAMTs are
> allocated within a single TDMR, the reserved areas may not be sufficient
> to cover all of them.
> 
> Adopt the following policies when allocating PAMTs for a given TDMR:
> 
>    - Allocate three PAMTs of the TDMR in one contiguous chunk to minimize
>      the total number of reserved areas consumed for PAMTs.
>    - Try to first allocate PAMT from the local node of the TDMR for better
>      NUMA locality.
> 
> Also dump out how many pages are allocated for PAMTs when the TDX module
> is initialized successfully.  This helps answer the eternal "where did
> all my memory go?" questions.
> 
> Signed-off-by: Kai Huang <kai.huang@...el.com>
> Reviewed-by: Isaku Yamahata <isaku.yamahata@...el.com>
> Reviewed-by: Dave Hansen <dave.hansen@...ux.intel.com>
> ---
> 
> v11 -> v12:
>   - Moved TDX_PS_NUM from tdx.c to <asm/tdx.h> (Kirill)
>   - "<= TDX_PS_1G" -> "< TDX_PS_NUM" (Kirill)
>   - Changed tdmr_get_pamt() to return base and size instead of base_pfn
>     and npages and related code directly (Dave).
>   - Simplified PAMT kb counting. (Dave)
>   - tdmrs_count_pamt_pages() -> tdmr_count_pamt_kb() (Kirill/Dave)
> 
> v10 -> v11:
>   - No update
> 
> v9 -> v10:
>   - Removed code change in disable_tdx_module() as it doesn't exist
>     anymore.
> 
> v8 -> v9:
>   - Added TDX_PS_NR macro instead of open-coding (Dave).
>   - Better alignment of 'pamt_entry_size' in tdmr_set_up_pamt() (Dave).
>   - Changed to print out PAMTs in "KBs" instead of "pages" (Dave).
>   - Added Dave's Reviewed-by.
> 
> v7 -> v8: (Dave)
>   - Changelog:
>    - Added a sentence to state PAMT allocation will be improved.
>    - Others suggested by Dave.
>   - Moved 'nid' of 'struct tdx_memblock' to this patch.
>   - Improved comments around tdmr_get_nid().
>   - WARN_ON_ONCE() -> pr_warn() in tdmr_get_nid().
>   - Other changes due to 'struct tdmr_info_list'.
> 
> v6 -> v7:
>   - Changes due to using macros instead of 'enum' for TDX supported page
>     sizes.
> 
> v5 -> v6:
>   - Rebase due to using 'tdx_memblock' instead of memblock.
>   - 'int pamt_entry_nr' -> 'unsigned long nr_pamt_entries' (Dave/Sagis).
>   - Improved comment around tdmr_get_nid() (Dave).
>   - Improved comment in tdmr_set_up_pamt() around breaking the PAMT
>     into PAMTs for 4K/2M/1G (Dave).
>   - tdmrs_get_pamt_pages() -> tdmrs_count_pamt_pages() (Dave).
> 
> - v3 -> v5 (no feedback on v4):
>   - Used memblock to get the NUMA node for given TDMR.
>   - Removed tdmr_get_pamt_sz() helper but use open-code instead.
>   - Changed to use 'switch .. case..' for each TDX supported page size in
>     tdmr_get_pamt_sz() (the original __tdmr_get_pamt_sz()).
>   - Added printing out memory used for PAMT allocation when TDX module is
>     initialized successfully.
>   - Explained downside of alloc_contig_pages() in changelog.
>   - Addressed other minor comments.
> 
> 
> ---
>   arch/x86/Kconfig            |   1 +
>   arch/x86/include/asm/tdx.h  |   1 +
>   arch/x86/virt/vmx/tdx/tdx.c | 215 +++++++++++++++++++++++++++++++++++-
>   arch/x86/virt/vmx/tdx/tdx.h |   1 +
>   4 files changed, 213 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 2226d8a4c749..ad364f01de33 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1959,6 +1959,7 @@ config INTEL_TDX_HOST
>   	depends on KVM_INTEL
>   	depends on X86_X2APIC
>   	select ARCH_KEEP_MEMBLOCK
> +	depends on CONTIG_ALLOC
>   	help
>   	  Intel Trust Domain Extensions (TDX) protects guest VMs from malicious
>   	  host and certain physical attacks.  This option enables necessary TDX
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index d8226a50c58c..91416fd600cd 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -24,6 +24,7 @@
>   #define TDX_PS_4K	0
>   #define TDX_PS_2M	1
>   #define TDX_PS_1G	2
> +#define TDX_PS_NR	(TDX_PS_1G + 1)
>   
>   /*
>    * Used to gather the output registers values of the TDCALL and SEAMCALL
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 2ffc1517a93b..fd5417577f26 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -221,7 +221,7 @@ static int tdx_get_sysinfo(struct tdsysinfo_struct *sysinfo,
>    * overlap.
>    */
>   static int add_tdx_memblock(struct list_head *tmb_list, unsigned long start_pfn,
> -			    unsigned long end_pfn)
> +			    unsigned long end_pfn, int nid)
>   {
>   	struct tdx_memblock *tmb;
>   
> @@ -232,6 +232,7 @@ static int add_tdx_memblock(struct list_head *tmb_list, unsigned long start_pfn,
>   	INIT_LIST_HEAD(&tmb->list);
>   	tmb->start_pfn = start_pfn;
>   	tmb->end_pfn = end_pfn;
> +	tmb->nid = nid;
>   
>   	/* @tmb_list is protected by mem_hotplug_lock */
>   	list_add_tail(&tmb->list, tmb_list);
> @@ -259,9 +260,9 @@ static void free_tdx_memlist(struct list_head *tmb_list)
>   static int build_tdx_memlist(struct list_head *tmb_list)
>   {
>   	unsigned long start_pfn, end_pfn;
> -	int i, ret;
> +	int i, nid, ret;
>   
> -	for_each_mem_pfn_range(i, MAX_NUMNODES, &start_pfn, &end_pfn, NULL) {
> +	for_each_mem_pfn_range(i, MAX_NUMNODES, &start_pfn, &end_pfn, &nid) {
>   		/*
>   		 * The first 1MB is not reported as TDX convertible memory.
>   		 * Although the first 1MB is always reserved and won't end up
> @@ -277,7 +278,7 @@ static int build_tdx_memlist(struct list_head *tmb_list)
>   		 * memblock has already guaranteed they are in address
>   		 * ascending order and don't overlap.
>   		 */
> -		ret = add_tdx_memblock(tmb_list, start_pfn, end_pfn);
> +		ret = add_tdx_memblock(tmb_list, start_pfn, end_pfn, nid);
>   		if (ret)
>   			goto err;

Why did you decide to defer remembering the nid as well? I'd just move 
that part to the patch that adds add_tdx_memblock().

-- 
Cheers,

David / dhildenb

Powered by blists - more mailing lists