From: Kai Huang <kai.huang@intel.com>
To: Dave Hansen <dave.hansen@intel.com>,
linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Cc: seanjc@google.com, pbonzini@redhat.com, len.brown@intel.com,
tony.luck@intel.com, rafael.j.wysocki@intel.com,
reinette.chatre@intel.com, dan.j.williams@intel.com,
peterz@infradead.org, ak@linux.intel.com,
kirill.shutemov@linux.intel.com,
sathyanarayanan.kuppuswamy@linux.intel.com,
isaku.yamahata@intel.com
Subject: Re: [PATCH v3 09/21] x86/virt/tdx: Get information about TDX module and convertible memory
Date: Thu, 28 Apr 2022 12:15:14 +1200 [thread overview]
Message-ID: <0bab7221179229317a11311386c968bd0d40e344.camel@intel.com> (raw)
In-Reply-To: <f929fb7a-5bdc-2567-77aa-762a098c8513@intel.com>
On Wed, 2022-04-27 at 15:15 -0700, Dave Hansen wrote:
> On 4/5/22 21:49, 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, TDX introduced the concept of a "Convertible Memory 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, along with TDX module information, is available to the kernel by
> > querying the TDX module via TDH.SYS.INFO SEAMCALL.
> >
> > Host kernel can choose whether or not to use all convertible memory
> > regions as TDX memory. Before TDX module is ready to create any TD
> > guests, all TDX memory regions that host kernel intends to use must be
> > configured to the TDX module, using specific data structures defined by
> > TDX architecture. Constructing those structures requires information of
> > both TDX module and the Convertible Memory Regions. Call TDH.SYS.INFO
> > to get this information as preparation to construct those structures.
> >
> > Signed-off-by: Kai Huang <kai.huang@intel.com>
> > ---
> > arch/x86/virt/vmx/tdx/tdx.c | 131 ++++++++++++++++++++++++++++++++++++
> > arch/x86/virt/vmx/tdx/tdx.h | 61 +++++++++++++++++
> > 2 files changed, 192 insertions(+)
> >
> > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> > index ef2718423f0f..482e6d858181 100644
> > --- a/arch/x86/virt/vmx/tdx/tdx.c
> > +++ b/arch/x86/virt/vmx/tdx/tdx.c
> > @@ -80,6 +80,11 @@ static DEFINE_MUTEX(tdx_module_lock);
> >
> > static struct p_seamldr_info p_seamldr_info;
> >
> > +/* Base address of CMR array needs to be 512 bytes aligned. */
> > +static struct cmr_info tdx_cmr_array[MAX_CMRS] __aligned(CMR_INFO_ARRAY_ALIGNMENT);
> > +static int tdx_cmr_num;
> > +static struct tdsysinfo_struct tdx_sysinfo;
>
> I really dislike mixing hardware and software structures. Please make
> it clear which of these are fully software-defined and which are part of
> the hardware ABI.
Both 'struct tdsysinfo_struct' and 'struct cmr_info' are hardware structures.
They are defined in tdx.h which has a comment saying the data structures below
this comment is hardware structures:
+/*
+ * TDX architectural data structures
+ */
It is introduced in the P-SEAMLDR patch.
Should I explicitly add comments around the variables saying they are used by
hardware, something like:
/*
* Data structures used by TDH.SYS.INFO SEAMCALL to return CMRs and
* TDX module system information.
*/
?
>
> > static bool __seamrr_enabled(void)
> > {
> > return (seamrr_mask & SEAMRR_ENABLED_BITS) == SEAMRR_ENABLED_BITS;
> > @@ -468,6 +473,127 @@ static int tdx_module_init_cpus(void)
> > return seamcall_on_each_cpu(&sc);
> > }
> >
> > +static inline bool cmr_valid(struct cmr_info *cmr)
> > +{
> > + return !!cmr->size;
> > +}
> > +
> > +static void print_cmrs(struct cmr_info *cmr_array, int cmr_num,
> > + const char *name)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < cmr_num; i++) {
> > + struct cmr_info *cmr = &cmr_array[i];
> > +
> > + pr_info("%s : [0x%llx, 0x%llx)\n", name,
> > + cmr->base, cmr->base + cmr->size);
> > + }
> > +}
> > +
> > +static int sanitize_cmrs(struct cmr_info *cmr_array, int cmr_num)
> > +{
> > + int i, j;
> > +
> > + /*
> > + * Intel TDX module spec, 20.7.3 CMR_INFO:
> > + *
> > + * TDH.SYS.INFO leaf function returns a MAX_CMRS (32) entry
> > + * array of CMR_INFO entries. The CMRs are sorted from the
> > + * lowest base address to the highest base address, and they
> > + * are non-overlapping.
> > + *
> > + * This implies that BIOS may generate invalid empty entries
> > + * if total CMRs are less than 32. Skip them manually.
> > + */
> > + for (i = 0; i < cmr_num; i++) {
> > + struct cmr_info *cmr = &cmr_array[i];
> > + struct cmr_info *prev_cmr = NULL;
> > +
> > + /* Skip further invalid CMRs */
> > + if (!cmr_valid(cmr))
> > + break;
> > +
> > + if (i > 0)
> > + prev_cmr = &cmr_array[i - 1];
> > +
> > + /*
> > + * It is a TDX firmware bug if CMRs are not
> > + * in address ascending order.
> > + */
> > + if (prev_cmr && ((prev_cmr->base + prev_cmr->size) >
> > + cmr->base)) {
> > + pr_err("Firmware bug: CMRs not in address ascending order.\n");
> > + return -EFAULT;
>
> -EFAULT is a really weird return code to use for this. I'd use -EINVAL.
OK thanks.
>
> > + }
> > + }
> > +
> > + /*
> > + * Also a sane BIOS should never generate invalid CMR(s) between
> > + * two valid CMRs. Sanity check this and simply return error in
> > + * this case.
> > + *
> > + * By reaching here @i is the index of the first invalid CMR (or
> > + * cmr_num). Starting with next entry of @i since it has already
> > + * been checked.
> > + */
> > + for (j = i + 1; j < cmr_num; j++)
> > + if (cmr_valid(&cmr_array[j])) {
> > + pr_err("Firmware bug: invalid CMR(s) among valid CMRs.\n");
> > + return -EFAULT;
> > + }
>
> Please add brackets for the for().
OK.
>
> > + /*
> > + * Trim all tail invalid empty CMRs. BIOS should generate at
> > + * least one valid CMR, otherwise it's a TDX firmware bug.
> > + */
> > + tdx_cmr_num = i;
> > + if (!tdx_cmr_num) {
> > + pr_err("Firmware bug: No valid CMR.\n");
> > + return -EFAULT;
> > + }
> > +
> > + /* Print kernel sanitized CMRs */
> > + print_cmrs(tdx_cmr_array, tdx_cmr_num, "Kernel-sanitized-CMR");
> > +
> > + return 0;
> > +}
> > +
> > +static int tdx_get_sysinfo(void)
> > +{
> > + struct tdx_module_output out;
> > + u64 tdsysinfo_sz, cmr_num;
> > + int ret;
> > +
> > + BUILD_BUG_ON(sizeof(struct tdsysinfo_struct) != TDSYSINFO_STRUCT_SIZE);
> > +
> > + ret = seamcall(TDH_SYS_INFO, __pa(&tdx_sysinfo), TDSYSINFO_STRUCT_SIZE,
> > + __pa(tdx_cmr_array), MAX_CMRS, NULL, &out);
> > + if (ret)
> > + return ret;
> > +
> > + /*
> > + * If TDH.SYS.CONFIG succeeds, RDX contains the actual bytes
> > + * written to @tdx_sysinfo and R9 contains the actual entries
> > + * written to @tdx_cmr_array. Sanity check them.
> > + */
> > + tdsysinfo_sz = out.rdx;
> > + cmr_num = out.r9;
>
> Please vertically align things like this:
>
> tdsysinfo_sz = out.rdx;
> cmr_num = out.r9;
OK.
>
> > + if (WARN_ON_ONCE((tdsysinfo_sz > sizeof(tdx_sysinfo)) || !tdsysinfo_sz ||
> > + (cmr_num > MAX_CMRS) || !cmr_num))
> > + return -EFAULT;
>
> Sanity checking is good, but this makes me wonder how much is too much.
> I don't see a lot of code for instance checking if sys_write() writes
> more than how much it was supposed to.
>
> Why are these sanity checks necessary here? Is the TDX module expected
> to be *THAT* buggy? The thing that's providing, oh, basically all of
> the security guarantees of this architecture. It's overflowing the
> buffers you hand it?
I think this check can be removed. Will remove.
>
> > + pr_info("TDX module: vendor_id 0x%x, major_version %u, minor_version %u, build_date %u, build_num %u",
> > + tdx_sysinfo.vendor_id, tdx_sysinfo.major_version,
> > + tdx_sysinfo.minor_version, tdx_sysinfo.build_date,
> > + tdx_sysinfo.build_num);
> > +
> > + /* Print BIOS provided CMRs */
> > + print_cmrs(tdx_cmr_array, cmr_num, "BIOS-CMR");
> > +
> > + return sanitize_cmrs(tdx_cmr_array, cmr_num);
> > +}
>
> Does sanitize_cmrs() sanitize anything? It looks to me like it *checks*
> the CMRs. But, sanitizing is an active operation that writes to the
> data being sanitized. This looks read-only to me. check_cmrs() would
> be a better name for a passive check.
Sure will change to check_cmrs().
>
> > static int init_tdx_module(void)
> > {
> > int ret;
> > @@ -482,6 +608,11 @@ static int init_tdx_module(void)
> > if (ret)
> > goto out;
> >
> > + /* Get TDX module information and CMRs */
> > + ret = tdx_get_sysinfo();
> > + if (ret)
> > + goto out;
>
> Couldn't we get rid of that comment if you did something like:
>
> ret = tdx_get_sysinfo(&tdx_cmr_array, &tdx_sysinfo);
Yes will do.
>
> and preferably make the variables function-local.
'tdx_sysinfo' will be used by KVM too.
--
Thanks,
-Kai
next prev parent reply other threads:[~2022-04-28 0:15 UTC|newest]
Thread overview: 156+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-06 4:49 [PATCH v3 00/21] TDX host kernel support Kai Huang
2022-04-06 4:49 ` [PATCH v3 01/21] x86/virt/tdx: Detect SEAM Kai Huang
2022-04-18 22:29 ` Sathyanarayanan Kuppuswamy
2022-04-18 22:50 ` Sean Christopherson
2022-04-19 3:38 ` Kai Huang
2022-04-26 20:21 ` Dave Hansen
2022-04-26 23:12 ` Kai Huang
2022-04-26 23:28 ` Dave Hansen
2022-04-26 23:49 ` Kai Huang
2022-04-27 0:22 ` Sean Christopherson
2022-04-27 0:44 ` Kai Huang
2022-04-27 14:22 ` Dave Hansen
2022-04-27 22:39 ` Kai Huang
2022-04-06 4:49 ` [PATCH v3 02/21] x86/virt/tdx: Detect TDX private KeyIDs Kai Huang
2022-04-19 5:39 ` Sathyanarayanan Kuppuswamy
2022-04-19 9:41 ` Kai Huang
2022-04-19 5:42 ` Sathyanarayanan Kuppuswamy
2022-04-19 10:07 ` Kai Huang
2022-04-06 4:49 ` [PATCH v3 03/21] x86/virt/tdx: Implement the SEAMCALL base function Kai Huang
2022-04-19 14:07 ` Sathyanarayanan Kuppuswamy
2022-04-20 4:16 ` Kai Huang
2022-04-20 7:29 ` Sathyanarayanan Kuppuswamy
2022-04-20 10:39 ` Kai Huang
2022-04-26 20:37 ` Dave Hansen
2022-04-26 23:29 ` Kai Huang
2022-04-06 4:49 ` [PATCH v3 04/21] x86/virt/tdx: Add skeleton for detecting and initializing TDX on demand Kai Huang
2022-04-19 14:53 ` Sathyanarayanan Kuppuswamy
2022-04-20 4:37 ` Kai Huang
2022-04-20 5:21 ` Dave Hansen
2022-04-20 14:30 ` Sathyanarayanan Kuppuswamy
2022-04-20 22:35 ` Kai Huang
2022-04-26 20:53 ` Dave Hansen
2022-04-27 0:43 ` Kai Huang
2022-04-27 14:49 ` Dave Hansen
2022-04-28 0:00 ` Kai Huang
2022-04-28 14:27 ` Dave Hansen
2022-04-28 23:44 ` Kai Huang
2022-04-28 23:53 ` Dave Hansen
2022-04-29 0:11 ` Kai Huang
2022-04-29 0:26 ` Dave Hansen
2022-04-29 0:59 ` Kai Huang
2022-04-06 4:49 ` [PATCH v3 05/21] x86/virt/tdx: Detect P-SEAMLDR and TDX module Kai Huang
2022-04-26 20:56 ` Dave Hansen
2022-04-27 0:01 ` Kai Huang
2022-04-27 14:24 ` Dave Hansen
2022-04-27 21:30 ` Kai Huang
2022-04-06 4:49 ` [PATCH v3 06/21] x86/virt/tdx: Shut down TDX module in case of error Kai Huang
2022-04-23 15:39 ` Sathyanarayanan Kuppuswamy
2022-04-25 23:41 ` Kai Huang
2022-04-26 1:48 ` Sathyanarayanan Kuppuswamy
2022-04-26 2:12 ` Kai Huang
2022-04-26 20:59 ` Dave Hansen
2022-04-27 0:06 ` Kai Huang
2022-05-18 16:19 ` Sagi Shahar
2022-05-18 23:51 ` Kai Huang
2022-04-06 4:49 ` [PATCH v3 07/21] x86/virt/tdx: Do TDX module global initialization Kai Huang
2022-04-20 22:27 ` Sathyanarayanan Kuppuswamy
2022-04-20 22:37 ` Kai Huang
2022-04-06 4:49 ` [PATCH v3 08/21] x86/virt/tdx: Do logical-cpu scope TDX module initialization Kai Huang
2022-04-24 1:27 ` Sathyanarayanan Kuppuswamy
2022-04-25 23:55 ` Kai Huang
2022-04-06 4:49 ` [PATCH v3 09/21] x86/virt/tdx: Get information about TDX module and convertible memory Kai Huang
2022-04-25 2:58 ` Sathyanarayanan Kuppuswamy
2022-04-26 0:05 ` Kai Huang
2022-04-27 22:15 ` Dave Hansen
2022-04-28 0:15 ` Kai Huang [this message]
2022-04-28 14:06 ` Dave Hansen
2022-04-28 23:14 ` Kai Huang
2022-04-29 17:47 ` Dave Hansen
2022-05-02 5:04 ` Kai Huang
2022-05-25 4:47 ` Kai Huang
2022-05-25 4:57 ` Kai Huang
2022-05-25 16:00 ` Kai Huang
2022-05-18 22:30 ` Sagi Shahar
2022-05-18 23:56 ` Kai Huang
2022-04-06 4:49 ` [PATCH v3 10/21] x86/virt/tdx: Add placeholder to coveret all system RAM as TDX memory Kai Huang
2022-04-20 20:48 ` Isaku Yamahata
2022-04-20 22:38 ` Kai Huang
2022-04-27 22:24 ` Dave Hansen
2022-04-28 0:53 ` Kai Huang
2022-04-28 1:07 ` Dave Hansen
2022-04-28 1:35 ` Kai Huang
2022-04-28 3:40 ` Dave Hansen
2022-04-28 3:55 ` Kai Huang
2022-04-06 4:49 ` [PATCH v3 11/21] x86/virt/tdx: Choose to use " Kai Huang
2022-04-20 20:55 ` Isaku Yamahata
2022-04-20 22:39 ` Kai Huang
2022-04-28 15:54 ` Dave Hansen
2022-04-29 7:32 ` Kai Huang
2022-04-06 4:49 ` [PATCH v3 12/21] x86/virt/tdx: Create TDMRs to cover all system RAM Kai Huang
2022-04-28 16:22 ` Dave Hansen
2022-04-29 7:24 ` Kai Huang
2022-04-29 13:52 ` Dave Hansen
2022-04-06 4:49 ` [PATCH v3 13/21] x86/virt/tdx: Allocate and set up PAMTs for TDMRs Kai Huang
2022-04-28 17:12 ` Dave Hansen
2022-04-29 7:46 ` Kai Huang
2022-04-29 14:20 ` Dave Hansen
2022-04-29 14:30 ` Sean Christopherson
2022-04-29 17:46 ` Dave Hansen
2022-04-29 18:19 ` Sean Christopherson
2022-04-29 18:32 ` Dave Hansen
2022-05-02 5:59 ` Kai Huang
2022-05-02 14:17 ` Dave Hansen
2022-05-02 21:55 ` Kai Huang
2022-04-06 4:49 ` [PATCH v3 14/21] x86/virt/tdx: Set up reserved areas for all TDMRs Kai Huang
2022-04-06 4:49 ` [PATCH v3 15/21] x86/virt/tdx: Reserve TDX module global KeyID Kai Huang
2022-04-06 4:49 ` [PATCH v3 16/21] x86/virt/tdx: Configure TDX module with TDMRs and " Kai Huang
2022-04-06 4:49 ` [PATCH v3 17/21] x86/virt/tdx: Configure global KeyID on all packages Kai Huang
2022-04-06 4:49 ` [PATCH v3 18/21] x86/virt/tdx: Initialize all TDMRs Kai Huang
2022-04-06 4:49 ` [PATCH v3 19/21] x86: Flush cache of TDX private memory during kexec() Kai Huang
2022-04-06 4:49 ` [PATCH v3 20/21] x86/virt/tdx: Add kernel command line to opt-in TDX host support Kai Huang
2022-04-28 17:25 ` Dave Hansen
2022-04-06 4:49 ` [PATCH v3 21/21] Documentation/x86: Add documentation for " Kai Huang
2022-04-14 10:19 ` [PATCH v3 00/21] TDX host kernel support Kai Huang
2022-04-26 20:13 ` Dave Hansen
2022-04-27 1:15 ` Kai Huang
2022-04-27 21:59 ` Dave Hansen
2022-04-28 0:37 ` Kai Huang
2022-04-28 0:50 ` Dave Hansen
2022-04-28 0:58 ` Kai Huang
2022-04-29 1:40 ` Kai Huang
2022-04-29 3:04 ` Dan Williams
2022-04-29 5:35 ` Kai Huang
2022-05-03 23:59 ` Kai Huang
2022-05-04 0:25 ` Dave Hansen
2022-05-04 1:15 ` Kai Huang
2022-05-05 9:54 ` Kai Huang
2022-05-05 13:51 ` Dan Williams
2022-05-05 22:14 ` Kai Huang
2022-05-06 0:22 ` Dan Williams
2022-05-06 0:45 ` Kai Huang
2022-05-06 1:15 ` Dan Williams
2022-05-06 1:46 ` Kai Huang
2022-05-06 15:57 ` Dan Williams
2022-05-09 2:46 ` Kai Huang
2022-05-10 10:25 ` Kai Huang
2022-05-07 0:09 ` Mike Rapoport
2022-05-08 10:00 ` Kai Huang
2022-05-09 10:33 ` Mike Rapoport
2022-05-09 23:27 ` Kai Huang
2022-05-04 14:31 ` Dan Williams
2022-05-04 22:50 ` Kai Huang
2022-04-28 1:01 ` Dan Williams
2022-04-28 1:21 ` Kai Huang
2022-04-29 2:58 ` Dan Williams
2022-04-29 5:43 ` Kai Huang
2022-04-29 14:39 ` Dave Hansen
2022-04-29 15:18 ` Dan Williams
2022-04-29 17:18 ` Dave Hansen
2022-04-29 17:48 ` Dan Williams
2022-04-29 18:34 ` Dave Hansen
2022-04-29 18:47 ` Dan Williams
2022-04-29 19:20 ` Dave Hansen
2022-04-29 21:20 ` Dan Williams
2022-04-29 21:27 ` Dave Hansen
2022-05-02 10:18 ` 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=0bab7221179229317a11311386c968bd0d40e344.camel@intel.com \
--to=kai.huang@intel.com \
--cc=ak@linux.intel.com \
--cc=dan.j.williams@intel.com \
--cc=dave.hansen@intel.com \
--cc=isaku.yamahata@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=pbonzini@redhat.com \
--cc=peterz@infradead.org \
--cc=rafael.j.wysocki@intel.com \
--cc=reinette.chatre@intel.com \
--cc=sathyanarayanan.kuppuswamy@linux.intel.com \
--cc=seanjc@google.com \
--cc=tony.luck@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).