From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.15]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4B16D3F822F for ; Mon, 24 Aug 2026 09:18:35 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=192.198.163.15 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787563122; cv=none; b=bRcogI/kiRX3SrhZulXH8cigtUWjG+j0IHtSMsEI7U18UodsloU8GpxvRVZGD6dbEPB43E0x0g9u0ttfAgx7lu2HlIr6EnjhKdR0PIVsx36diRHM1OJj7ZfPKfIC8JyI1TVxOautoMDL/q4JTf9jmeoCiy15mwlD6UA7NKFSPGo= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787563122; c=relaxed/simple; bh=6BmFy5jb3jLUCgM6DtkO/km+Rgo/GycIWhXxFsToEN8=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=TttiOwI0c2NqNBPChxjdKZXd4HYkG+brRVg3HOfxvyfeBYIVMnRVDgu33mHiRamH7qIc9rFNjhzO7BCiK3tNnMIjqSU2Gz1oM6n7OxnA2v65AsyyKlvqNpSmIzex+olKB6C0Xvk21CVuvn460bLGvAvNVi3pBBfgvJkcBxax4Rg= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com; spf=pass smtp.mailfrom=linux.intel.com; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b=dRCbCk06; arc=none smtp.client-ip=192.198.163.15 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="dRCbCk06" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1787563115; x=1819099115; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=6BmFy5jb3jLUCgM6DtkO/km+Rgo/GycIWhXxFsToEN8=; b=dRCbCk06MADo/H0XrPfz+gXxHTOY4ArEGZW77Uy2qs4PXgWIwTG8U1yo zCybB9I5t0TelcjmPZSdWsuTD9nhBR3rrd0vf9ktRx503RHe3DqjJg6Ku LD6H9XKevMwGMWYtLwQZXhXl6Sy8Snwfyf8EdXmfFFtRsspMfTVfIWlLX gMsEd4s57LWusejR1p2jxhVylfmcy/wbNvRROaOypFTkgBmSrPizOzKim qed4tDHeIGU+rMaZ1A9YQ6vJaKPTZlW8rI/ww6pTDcCDCQCHny9EXrFaR zU/XjiPngDdXEKQSwfROenbV+k3NrTdp9U4cvVKADmVvU6KM6wA9Vxn28 g==; X-CSE-ConnectionGUID: BCFJwCDoR5apYgb2itAm+A== X-CSE-MsgGUID: UyrQfc1LRWyg9Y73wnY4NA== X-IronPort-AV: E=McAfee;i="6800,10657,11884"; a="88135216" X-IronPort-AV: E=Sophos;i="6.25,240,1779174000"; d="scan'208";a="88135216" Received: from fmviesa002.fm.intel.com ([10.60.135.142]) by fmvoesa109.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 24 Aug 2026 02:18:30 -0700 X-CSE-ConnectionGUID: tzE7G8ZvQ5CtpN3ASW/G8Q== X-CSE-MsgGUID: yoLLhoXySwOwiI6xTI0OMg== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.25,240,1779174000"; d="scan'208";a="290467966" Received: from yilunxu-optiplex-7050.sh.intel.com (HELO localhost) ([10.239.47.46]) by fmviesa002.fm.intel.com with ESMTP; 24 Aug 2026 02:18:27 -0700 Date: Mon, 24 Aug 2026 17:18:25 +0800 From: Xu Yilun To: Kiryl Shutsemau Cc: x86@kernel.org, linux-coco@lists.linux.dev, linux-kernel@vger.kernel.org, rick.p.edgecombe@intel.com, yilun.xu@intel.com, xiaoyao.li@intel.com, sohil.mehta@intel.com, adrian.hunter@intel.com, kishen.maloor@intel.com, tony.lindgren@linux.intel.com, peter.fang@intel.com, baolu.lu@linux.intel.com, zhenzhong.duan@intel.com, chao.gao@intel.com, artem.bityutskiy@linux.intel.com, kvm@vger.kernel.org Subject: Re: [PATCH 4/6] x86/virt/tdx: Add extra memory to TDX module for the extensions Message-ID: References: <20260821032920.256225-1-yilun.xu@linux.intel.com> <20260821032920.256225-5-yilun.xu@linux.intel.com> Precedence: bulk X-Mailing-List: linux-coco@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: > > The TDX module accepts the memory in the form of a PFN array. This array > > is passed via a single 64-bit SEAMCALL leaf parameter, which encodes two > > values: the PFN of the container page holding the array, and the number > > of entries in the array. Create a helper to encode this format and name > > it after the TDX module term: HPA_LIST_INFO. > > The array entries are physical addresses, not PFNs. HPA_LIST_INFO encodes > a PFN, the array does not. OK. I'll change PFN array => HPA array [...] > > +#define TDX_HPA_LIST_MAX_NR_PAGES (PAGE_SIZE / sizeof(u64)) > > + > > +struct tdx_hpa_list { > > + u64 phys[TDX_HPA_LIST_MAX_NR_PAGES]; > > +}; > > + > > +static_assert(sizeof(struct tdx_hpa_list) == PAGE_SIZE); [...] > > + hpa_list = kzalloc_obj(*hpa_list); > > + if (!hpa_list) > > + return -ENOMEM; > > to_hpa_list_info() expects hpa_list to be page-aligned. It happens to > work with kmalloc for PAGE_SIZE allocation. The struct tdx_hpa_list definition follows the TDX ABI and is guarenteed to be PAGE_SIZE by: static_assert(sizeof(struct tdx_hpa_list) == PAGE_SIZE); and kmalloc guarentees the page alignment. 59bb47985c1d ("mm, sl[aou]b: guarantee natural alignment for kmalloc(power-of-two)") So I think it's OK, not "happen to work". > > Maybe it is better to allocate it with buddy allocator instead? It can be, but then we need an extra variable to record the struct page *, which seems redundant? > > > + > > + page = alloc_contig_pages(required_pages, GFP_KERNEL, numa_mem_id(), > > + &node_online_map); > > Why contiguous? TDH.EXT.MEM.ADD takes a list of page addresses and the loop > below writes every one of them out separately. > > alloc_pages_bulk() fits the chunking that is already here, and a short > return can be handled per chunk. alloc_contig_pages() isolates and migrates > to get its range and fails TDX init outright when it cannot find one. PAMT Yeah, this is not the ABI requirement, but the kernel's consideration. A brief reasoning in the commit log: avoiding permanent memory fragmentation and buddy allocator efficiency loss. Also there is some discussion: https://lore.kernel.org/all/167d9540-2d9a-4367-bc68-b96494bc4044@intel.com/ TL;DR - The memory will never return to the kernel. - There is chance that this tens of megabytes will fragment tens of gigabytes of memory forever. - The chance of fragmentation is actually low since at boot up, but let the buddy allocator take care of these never-returned memory is not necessary and lowers its efficiency. > needs it because the TDMR ABI describes each PAMT as base+size. This does > not. > > > + if (!page) { > > + ret = -ENOMEM; > > + goto out_free_hpa_list; > > + } > > + > > + added_pages = 0; > > + while (added_pages < required_pages) { > > + unsigned int chunk_pages = min(required_pages - added_pages, > > + TDX_HPA_LIST_MAX_NR_PAGES); > > + struct page *chunk = page + added_pages; > > + unsigned int i; > > + > > + for (i = 0; i < chunk_pages; i++) > > + hpa_list->phys[i] = page_to_phys(chunk + i); > > + > > + ret = tdx_ext_mem_add(hpa_list, chunk_pages); > > + if (ret) { > > + /* > > + * This SEAMCALL leaf shouldn't fail, and if it does, > > + * things are broken enough that complex error handling > > + * isn't worth it. Intentionally leak all pages, > > + * including un-added pages. > > + */ > > + WARN(1, "Fatal: TDX module rejected memory for extensions, stranded all pages\n"); > > + break; > > It supposed to be > goto out_free_hpa_list; > > No? The difference is to print the memory amount or not. For simple error handling, we stranded all pages, we let users know the cost even if the initialization fails. > > > > + } > > + > > + added_pages += chunk_pages; > > + } > > + > > + /* > > + * Memory for TDX module extensions is never reclaimed and can be tens > > + * of megabytes. Print the amount so users know the cost. > > + */ > > + pr_info("%lu KB consumed for TDX module extensions\n", > > + required_pages * PAGE_SIZE / 1024); > > + > > +out_free_hpa_list: > > + kfree(hpa_list); > > + > > + return ret; > > +} > > + [...] > > --- a/arch/x86/virt/vmx/tdx/tdx_global_metadata.c > > +++ b/arch/x86/virt/vmx/tdx/tdx_global_metadata.c > > @@ -137,6 +137,12 @@ static __init int get_tdx_sys_info_ext(struct tdx_sys_info_ext *sysinfo_ext) > > int ret; > > u64 val; > > > > + ret = read_sys_metadata_field(0x3100000200000000, &val); > > + if (ret) > > + return ret; > > + > > + sysinfo_ext->memory_pool_required_pages = val; > > + > > Why above ext_required read? I want to sort them in ascending order of the field ID, so reviewers can seach them more easily. > Is it even valid to read it in such case? It is OK. The two ext metadata are both valid after TDX feature configurations. The ext_required == 0 && memory_pool_required_pages > 0 is highly suspicious based on our current understanding, but that's more of a module BUG, not caused by metadata reading order. > > > ret = read_sys_metadata_field(0x3100000000000001, &val); > > if (ret) > > return ret; > > -- > > 2.25.1 > > > > -- > Kiryl Shutsemau / Kirill A. Shutemov