From: Dave Hansen <dave.hansen@intel.com>
To: Kai Huang <kai.huang@intel.com>,
linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Cc: linux-mm@kvack.org, seanjc@google.com, pbonzini@redhat.com,
dan.j.williams@intel.com, rafael.j.wysocki@intel.com,
kirill.shutemov@linux.intel.com, ying.huang@intel.com,
reinette.chatre@intel.com, len.brown@intel.com,
tony.luck@intel.com, peterz@infradead.org, ak@linux.intel.com,
isaku.yamahata@intel.com, chao.gao@intel.com,
sathyanarayanan.kuppuswamy@linux.intel.com, bagasdotme@gmail.com,
sagis@google.com, imammedo@redhat.com
Subject: Re: [PATCH v7 11/20] x86/virt/tdx: Add placeholder to construct TDMRs to cover all TDX memory regions
Date: Wed, 23 Nov 2022 14:17:19 -0800 [thread overview]
Message-ID: <6d4d429a-ade2-771d-0e4c-788bef45041a@intel.com> (raw)
In-Reply-To: <32c1968fe34c8cf3cb834e3a9966cd2a201efc5b.1668988357.git.kai.huang@intel.com>
On 11/20/22 16:26, Kai Huang wrote:
> TDX provides increased levels of memory confidentiality and integrity.
> This requires special hardware support for features like memory
> encryption and storage of memory integrity checksums. Not all memory
> satisfies these requirements.
>
> As a result, the TDX introduced the concept of a "Convertible Memory
s/the TDX introduced/TDX introduces/
> Region" (CMR). During boot, the firmware builds a list of all of the
> memory ranges which can provide the TDX security guarantees. The list
> of these ranges is available to the kernel by querying the TDX module.
>
> The TDX architecture needs additional metadata to record things like
> which TD guest "owns" a given page of memory. This metadata essentially
> serves as the 'struct page' for the TDX module. The space for this
> metadata is not reserved by the hardware up front and must be allocated
> by the kernel and given to the TDX module.
>
> Since this metadata consumes space, the VMM can choose whether or not to
> allocate it for a given area of convertible memory. If it chooses not
> to, the memory cannot receive TDX protections and can not be used by TDX
> guests as private memory.
>
> For every memory region that the VMM wants to use as TDX memory, it sets
> up a "TD Memory Region" (TDMR). Each TDMR represents a physically
> contiguous convertible range and must also have its own physically
> contiguous metadata table, referred to as a Physical Address Metadata
> Table (PAMT), to track status for each page in the TDMR range.
>
> Unlike a CMR, each TDMR requires 1G granularity and alignment. To
> support physical RAM areas that don't meet those strict requirements,
> each TDMR permits a number of internal "reserved areas" which can be
> placed over memory holes. If PAMT metadata is placed within a TDMR it
> must be covered by one of these reserved areas.
>
> Let's summarize the concepts:
>
> CMR - Firmware-enumerated physical ranges that support TDX. CMRs are
> 4K aligned.
> TDMR - Physical address range which is chosen by the kernel to support
> TDX. 1G granularity and alignment required. Each TDMR has
> reserved areas where TDX memory holes and overlapping PAMTs can
> be put into.
s/put into/represented/
> PAMT - Physically contiguous TDX metadata. One table for each page size
> per TDMR. Roughly 1/256th of TDMR in size. 256G TDMR = ~1G
> PAMT.
>
> As one step of initializing the TDX module, the kernel configures
> TDX-usable memory regions by passing an array of TDMRs to the TDX module.
>
> Constructing the array of TDMRs consists below steps:
>
> 1) Create TDMRs to cover all memory regions that the TDX module can use;
Slight tweak:
1) Create TDMRs to cover all memory regions that the TDX module will use
for TD memory
The TDX module "uses" more memory than strictly the TMDR's.
> 2) Allocate and set up PAMT for each TDMR;
> 3) Set up reserved areas for each TDMR.
s/Set up/Designate/
> Add a placeholder to construct TDMRs to do the above steps after all
> TDX memory regions are verified to be truly convertible. Always free
> TDMRs at the end of the initialization (no matter successful or not)
> as TDMRs are only used during the initialization.
The changelog here actually looks really good to me so far.
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 32af86e31c47..26048c6b0170 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -445,6 +445,63 @@ static int build_tdx_memory(void)
> return ret;
> }
>
> +/* Calculate the actual TDMR_INFO size */
> +static inline int cal_tdmr_size(void)
I think we can spare the bytes to add "culate" in the function name so
we don't think these are California TDMRs.
> +{
> + int tdmr_sz;
> +
> + /*
> + * The actual size of TDMR_INFO depends on the maximum number
> + * of reserved areas.
> + *
> + * Note: for TDX1.0 the max_reserved_per_tdmr is 16, and
> + * TDMR_INFO size is aligned up to 512-byte. Even it is
> + * extended in the future, it would be insane if TDMR_INFO
> + * becomes larger than 4K. The tdmr_sz here should never
> + * overflow.
> + */
> + tdmr_sz = sizeof(struct tdmr_info);
> + tdmr_sz += sizeof(struct tdmr_reserved_area) *
> + tdx_sysinfo.max_reserved_per_tdmr;
First, I think 'tdx_sysinfo' should probably be a local variable in
init_tdx_module() and have its address passed in here. Having global
variables always makes it more opaque about who is initializing it.
Second, if this code is making assumptions about
'max_reserved_per_tdmr', then let's actually add assertions or sanity
checks. For instance:
if (tdx_sysinfo.max_reserved_per_tdmr > MAX_TDMRS)
return -1;
or even:
if (tdmr_sz > PAGE_SIZE)
return -1;
It does almost no good to just assert what the limits are in a comment.
> + /*
> + * TDX requires each TDMR_INFO to be 512-byte aligned. Always
> + * round up TDMR_INFO size to the 512-byte boundary.
> + */
<sigh> More silly comments.
The place to document this is TDMR_INFO_ALIGNMENT. If anyone wants to
know what the alignment is, exactly, they can look at the definition.
They don't need to be told *TWICE* what TDMR_INFO_ALIGNMENT #defines to
in one comment.
> + return ALIGN(tdmr_sz, TDMR_INFO_ALIGNMENT);
> +}
> +
> +static struct tdmr_info *alloc_tdmr_array(int *array_sz)
> +{
> + /*
> + * TDX requires each TDMR_INFO to be 512-byte aligned.
> + * Use alloc_pages_exact() to allocate all TDMRs at once.
> + * Each TDMR_INFO will still be 512-byte aligned since
> + * cal_tdmr_size() always returns 512-byte aligned size.
> + */
OK, I think you're just trolling me now. Two *MORE* mentions of the
512-byte alignment?
> + *array_sz = cal_tdmr_size() * tdx_sysinfo.max_tdmrs;
> +
> + /*
> + * Zero the buffer so 'struct tdmr_info::size' can be
> + * used to determine whether a TDMR is valid.
> + *
> + * Note: for TDX1.0 the max_tdmrs is 64 and TDMR_INFO size
> + * is 512-byte. Even they are extended in the future, it
> + * would be insane if the total size exceeds 4MB.
> + */
> + return alloc_pages_exact(*array_sz, GFP_KERNEL | __GFP_ZERO);
> +}
This looks massively over complicated.
Get rid of this function entirely. Then create:
static int tdmr_array_size(void)
{
return tdmr_size_single() * tdx_sysinfo.max_tdmrs;
}
The *caller* can do:
tdmr_array = alloc_pages_exact(tdmr_array_size(),
GFP_KERNEL | __GFP_ZERO);
if (!tdmr_array) {
...
Then the error path is:
free_pages_exact(tdmr_array, tdmr_array_size());
Then, there are no size pointers going back and forth. Easy peasy. I'm
OK with a little arithmetic being repeated.
> +/*
> + * Construct an array of TDMRs to cover all TDX memory ranges.
> + * The actual number of TDMRs is kept to @tdmr_num.
> + */
> +static int construct_tdmrs(struct tdmr_info *tdmr_array, int *tdmr_num)
> +{
> + /* Return -EINVAL until constructing TDMRs is done */
> + return -EINVAL;
> +}
> +
> /*
> * Detect and initialize the TDX module.
> *
> @@ -454,6 +511,9 @@ static int build_tdx_memory(void)
> */
> static int init_tdx_module(void)
> {
> + struct tdmr_info *tdmr_array;
> + int tdmr_array_sz;
> + int tdmr_num;
I tend to write these like:
"tdmr_num" is the number of *a* TDMR.
"nr_tdmrs" is the number of TDMRs.
> int ret;
>
> /*
> @@ -506,11 +566,34 @@ static int init_tdx_module(void)
> ret = build_tdx_memory();
> if (ret)
> goto out;
> +
> + /* Prepare enough space to construct TDMRs */
> + tdmr_array = alloc_tdmr_array(&tdmr_array_sz);
> + if (!tdmr_array) {
> + ret = -ENOMEM;
> + goto out_free_tdx_mem;
> + }
> +
> + /* Construct TDMRs to cover all TDX memory ranges */
> + ret = construct_tdmrs(tdmr_array, &tdmr_num);
> + if (ret)
> + goto out_free_tdmrs;
> +
> /*
> * Return -EINVAL until all steps of TDX module initialization
> * process are done.
> */
> ret = -EINVAL;
> +out_free_tdmrs:
> + /*
> + * The array of TDMRs is freed no matter the initialization is
> + * successful or not. They are not needed anymore after the
> + * module initialization.
> + */
> + free_pages_exact(tdmr_array, tdmr_array_sz);
> +out_free_tdx_mem:
> + if (ret)
> + free_tdx_memory();
> out:
> /*
> * Memory hotplug checks the hot-added memory region against the
> diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
> index 8e273756098c..a737f2b51474 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.h
> +++ b/arch/x86/virt/vmx/tdx/tdx.h
> @@ -80,6 +80,29 @@ struct tdsysinfo_struct {
> };
> } __packed __aligned(TDSYSINFO_STRUCT_ALIGNMENT);
>
> +struct tdmr_reserved_area {
> + u64 offset;
> + u64 size;
> +} __packed;
> +
> +#define TDMR_INFO_ALIGNMENT 512
> +
> +struct tdmr_info {
> + u64 base;
> + u64 size;
> + u64 pamt_1g_base;
> + u64 pamt_1g_size;
> + u64 pamt_2m_base;
> + u64 pamt_2m_size;
> + u64 pamt_4k_base;
> + u64 pamt_4k_size;
> + /*
> + * Actual number of reserved areas depends on
> + * 'struct tdsysinfo_struct'::max_reserved_per_tdmr.
> + */
> + struct tdmr_reserved_area reserved_areas[0];
> +} __packed __aligned(TDMR_INFO_ALIGNMENT);
> +
> /*
> * Do not put any hardware-defined TDX structure representations below
> * this comment!
next prev parent reply other threads:[~2022-11-23 22:17 UTC|newest]
Thread overview: 163+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-21 0:26 [PATCH v7 00/20] TDX host kernel support Kai Huang
2022-11-21 0:26 ` [PATCH v7 01/20] x86/tdx: Define TDX supported page sizes as macros Kai Huang
2022-11-21 2:52 ` Sathyanarayanan Kuppuswamy
2022-11-21 9:15 ` Huang, Kai
2022-11-21 17:23 ` Sathyanarayanan Kuppuswamy
2022-11-21 18:12 ` Dave Hansen
2022-11-21 23:48 ` Dave Hansen
2022-11-22 0:01 ` Huang, Kai
2022-11-21 0:26 ` [PATCH v7 02/20] x86/virt/tdx: Detect TDX during kernel boot Kai Huang
2022-11-21 3:07 ` Sathyanarayanan Kuppuswamy
2022-11-21 9:37 ` Huang, Kai
2022-11-21 23:57 ` Sathyanarayanan Kuppuswamy
2022-11-22 0:10 ` Dave Hansen
2022-11-22 11:28 ` Huang, Kai
2022-11-22 16:50 ` Dave Hansen
2022-11-22 23:21 ` Huang, Kai
2022-11-21 0:26 ` [PATCH v7 03/20] x86/virt/tdx: Disable TDX if X2APIC is not enabled Kai Huang
2022-11-21 3:51 ` Sathyanarayanan Kuppuswamy
2022-11-21 9:44 ` Huang, Kai
2022-11-21 22:00 ` Sathyanarayanan Kuppuswamy
2022-11-21 23:40 ` Huang, Kai
2022-11-21 23:46 ` Dave Hansen
2022-11-22 0:30 ` Huang, Kai
2022-11-22 0:44 ` Dave Hansen
2022-11-22 0:58 ` Huang, Kai
2022-11-21 0:26 ` [PATCH v7 04/20] x86/virt/tdx: Add skeleton to initialize TDX on demand Kai Huang
2022-11-22 9:02 ` Peter Zijlstra
2022-11-22 10:31 ` Thomas Gleixner
2022-11-22 15:35 ` Dave Hansen
2022-11-22 20:03 ` Thomas Gleixner
2022-11-22 20:11 ` Sean Christopherson
2022-11-23 0:30 ` Huang, Kai
2022-11-23 1:12 ` Huang, Kai
2022-11-23 11:05 ` Thomas Gleixner
2022-11-23 12:22 ` Huang, Kai
2022-11-22 18:05 ` Dave Hansen
2022-11-23 10:18 ` Huang, Kai
2022-11-23 16:58 ` Dave Hansen
2022-11-23 21:58 ` Huang, Kai
2022-11-21 0:26 ` [PATCH v7 05/20] x86/virt/tdx: Implement functions to make SEAMCALL Kai Huang
2022-11-22 9:06 ` Peter Zijlstra
2022-11-23 8:53 ` Huang, Kai
2022-11-22 18:20 ` Dave Hansen
2022-11-23 10:43 ` Huang, Kai
2022-11-21 0:26 ` [PATCH v7 06/20] x86/virt/tdx: Shut down TDX module in case of error Kai Huang
2022-11-22 9:10 ` Peter Zijlstra
2022-11-22 9:13 ` Peter Zijlstra
2022-11-22 15:14 ` Dave Hansen
2022-11-22 19:13 ` Peter Zijlstra
2022-11-22 19:24 ` Dave Hansen
2022-11-22 19:33 ` Peter Zijlstra
2022-11-23 1:14 ` Huang, Kai
2022-11-29 21:40 ` Dave Hansen
2022-11-30 11:09 ` Thomas Gleixner
2022-11-23 0:58 ` Huang, Kai
2022-11-23 1:04 ` Dave Hansen
2022-11-23 1:22 ` Huang, Kai
2022-11-23 16:20 ` Sean Christopherson
2022-11-23 16:41 ` Dave Hansen
2022-11-23 17:37 ` Sean Christopherson
2022-11-23 18:18 ` Dave Hansen
2022-11-23 19:03 ` Sean Christopherson
2022-11-22 9:20 ` Peter Zijlstra
2022-11-22 15:06 ` Thomas Gleixner
2022-11-22 19:06 ` Peter Zijlstra
2022-11-22 19:31 ` Sean Christopherson
2022-11-23 9:39 ` Huang, Kai
2022-11-22 15:20 ` Dave Hansen
2022-11-22 16:52 ` Thomas Gleixner
2022-11-22 18:57 ` Dave Hansen
2022-11-22 19:14 ` Peter Zijlstra
2022-11-23 1:24 ` Huang, Kai
2022-11-21 0:26 ` [PATCH v7 07/20] x86/virt/tdx: Do TDX module global initialization Kai Huang
2022-11-22 19:14 ` Dave Hansen
2022-11-23 11:45 ` Huang, Kai
2022-11-21 0:26 ` [PATCH v7 08/20] x86/virt/tdx: Do logical-cpu scope TDX module initialization Kai Huang
2022-11-21 0:26 ` [PATCH v7 09/20] x86/virt/tdx: Get information about TDX module and TDX-capable memory Kai Huang
2022-11-22 23:39 ` Dave Hansen
2022-11-23 11:40 ` Huang, Kai
2022-11-23 16:44 ` Dave Hansen
2022-11-23 22:53 ` Huang, Kai
2022-12-02 11:19 ` Huang, Kai
2022-12-02 17:25 ` Dave Hansen
2022-12-02 21:57 ` Huang, Kai
2022-12-02 11:11 ` Huang, Kai
2022-12-02 17:06 ` Dave Hansen
2022-12-02 21:56 ` Huang, Kai
2022-11-21 0:26 ` [PATCH v7 10/20] x86/virt/tdx: Use all system memory when initializing TDX module as TDX memory Kai Huang
2022-11-21 5:37 ` Huang, Ying
2022-11-21 9:09 ` Huang, Kai
2022-11-22 1:54 ` Huang, Ying
2022-11-22 9:16 ` Huang, Kai
2022-11-24 0:47 ` Huang, Ying
2022-11-22 10:10 ` Peter Zijlstra
2022-11-22 11:40 ` Huang, Kai
2022-11-23 0:21 ` Dave Hansen
2022-11-23 9:29 ` Peter Zijlstra
2022-11-24 1:04 ` Huang, Kai
2022-11-24 1:22 ` Dave Hansen
2022-11-24 2:27 ` Huang, Kai
2022-11-24 1:50 ` Dan Williams
2022-11-24 9:06 ` Huang, Kai
2022-11-25 9:28 ` David Hildenbrand
2022-11-28 8:38 ` Huang, Kai
2022-11-28 8:43 ` David Hildenbrand
2022-11-28 9:21 ` Huang, Kai
2022-11-28 9:26 ` David Hildenbrand
2022-11-28 9:50 ` Huang, Kai
2022-11-24 9:26 ` Peter Zijlstra
2022-11-24 10:02 ` Huang, Kai
2022-11-30 22:26 ` Dave Hansen
2022-11-21 0:26 ` [PATCH v7 11/20] x86/virt/tdx: Add placeholder to construct TDMRs to cover all TDX memory regions Kai Huang
2022-11-23 22:17 ` Dave Hansen [this message]
2022-11-24 9:51 ` Huang, Kai
2022-11-24 12:02 ` Huang, Kai
2022-11-28 15:59 ` Dave Hansen
2022-11-28 22:13 ` Huang, Kai
2022-11-28 22:19 ` Dave Hansen
2022-11-28 22:50 ` Huang, Kai
2022-12-07 11:47 ` Huang, Kai
2022-12-08 12:56 ` Huang, Kai
2022-12-08 14:58 ` Dave Hansen
2022-12-08 23:29 ` Huang, Kai
2022-11-21 0:26 ` [PATCH v7 12/20] x86/virt/tdx: Create " Kai Huang
2022-11-23 22:41 ` Dave Hansen
2022-11-24 11:29 ` Huang, Kai
2022-11-21 0:26 ` [PATCH v7 13/20] x86/virt/tdx: Allocate and set up PAMTs for TDMRs Kai Huang
2022-11-23 22:57 ` Dave Hansen
2022-11-24 11:46 ` Huang, Kai
2022-11-28 16:39 ` Dave Hansen
2022-11-28 22:48 ` Huang, Kai
2022-11-28 22:56 ` Dave Hansen
2022-11-28 23:14 ` Huang, Kai
2022-11-21 0:26 ` [PATCH v7 14/20] x86/virt/tdx: Set up reserved areas for all TDMRs Kai Huang
2022-11-23 23:39 ` Dave Hansen
2022-11-28 9:14 ` Huang, Kai
2022-11-28 13:18 ` Dave Hansen
2022-11-28 22:24 ` Huang, Kai
2022-11-28 22:58 ` Dave Hansen
2022-11-28 23:10 ` Huang, Kai
2022-11-21 0:26 ` [PATCH v7 15/20] x86/virt/tdx: Reserve TDX module global KeyID Kai Huang
2022-11-23 23:40 ` Dave Hansen
2022-11-24 22:39 ` Huang, Kai
2022-11-21 0:26 ` [PATCH v7 16/20] x86/virt/tdx: Configure TDX module with TDMRs and " Kai Huang
2022-11-23 23:56 ` Dave Hansen
2022-11-25 0:59 ` Huang, Kai
2022-11-25 1:18 ` Dave Hansen
2022-11-25 1:44 ` Huang, Kai
2022-11-21 0:26 ` [PATCH v7 17/20] x86/virt/tdx: Configure global KeyID on all packages Kai Huang
2022-11-24 0:28 ` Dave Hansen
2022-11-24 22:28 ` Huang, Kai
2022-11-25 0:08 ` Huang, Kai
2022-11-30 3:35 ` Binbin Wu
2022-11-30 8:34 ` Huang, Kai
2022-11-30 14:04 ` kirill.shutemov
2022-11-30 15:13 ` Dave Hansen
2022-11-30 20:17 ` Huang, Kai
2022-11-30 17:37 ` Dave Hansen
2022-11-21 0:26 ` [PATCH v7 18/20] x86/virt/tdx: Initialize all TDMRs Kai Huang
2022-11-24 0:42 ` Dave Hansen
2022-11-25 2:27 ` Huang, Kai
2022-11-21 0:26 ` [PATCH v7 19/20] x86/virt/tdx: Flush cache in kexec() when TDX is enabled Kai Huang
2022-11-21 0:26 ` [PATCH v7 20/20] Documentation/x86: Add documentation for TDX host support Kai Huang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6d4d429a-ade2-771d-0e4c-788bef45041a@intel.com \
--to=dave.hansen@intel.com \
--cc=ak@linux.intel.com \
--cc=bagasdotme@gmail.com \
--cc=chao.gao@intel.com \
--cc=dan.j.williams@intel.com \
--cc=imammedo@redhat.com \
--cc=isaku.yamahata@intel.com \
--cc=kai.huang@intel.com \
--cc=kirill.shutemov@linux.intel.com \
--cc=kvm@vger.kernel.org \
--cc=len.brown@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=pbonzini@redhat.com \
--cc=peterz@infradead.org \
--cc=rafael.j.wysocki@intel.com \
--cc=reinette.chatre@intel.com \
--cc=sagis@google.com \
--cc=sathyanarayanan.kuppuswamy@linux.intel.com \
--cc=seanjc@google.com \
--cc=tony.luck@intel.com \
--cc=ying.huang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).