lkml.org 
[lkml]   [2022]   [Jun]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v5 01/22] x86/virt/tdx: Detect TDX during kernel boot
From
Date
On Thu, 2022-06-23 at 13:57 +0800, Chao Gao wrote:
> On Wed, Jun 22, 2022 at 11:15:30PM +1200, Kai Huang wrote:
> > Intel Trust Domain Extensions (TDX) protects guest VMs from malicious
> > host and certain physical attacks. TDX introduces a new CPU mode called
> > Secure Arbitration Mode (SEAM) and a new isolated range pointed by the
> ^ perhaps, range of memory

OK. The spec indeed says "execute out of memory defined by SEAM ranger register
(SEAMRR)".

>
> > +static int detect_tdx_keyids(void)
> > +{
> > + u64 keyid_part;
> > +
> > + rdmsrl(MSR_IA32_MKTME_KEYID_PARTITIONING, keyid_part);
>
> how about:
> rdmsr(MSR_IA32_MKTME_KEYID_PARTITIONING, tdx_keyid_start, tdx_keyid_num);
> tdx_keyid_start++;
>
> Then TDX_KEYID_NUM/START can be dropped.

OK will do.

>
> > +
> > + tdx_keyid_num = TDX_KEYID_NUM(keyid_part);
> > + tdx_keyid_start = TDX_KEYID_START(keyid_part);
> > +
> > + pr_info("TDX private KeyID range: [%u, %u).\n",
> > + tdx_keyid_start, tdx_keyid_start + tdx_keyid_num);
> > +
> > + /*
> > + * TDX guarantees at least two TDX KeyIDs are configured by
> > + * BIOS, otherwise SEAMRR is disabled. Invalid TDX private
> > + * range means kernel bug (TDX is broken).
>
> Maybe it is better to have a comment for why TDX/kernel guarantees
> there should be at least 2 TDX keyIDs.

"TDX guarantees" means it is architectural behaviour. Perhaps I can change to
"TDX architecture guarantee" to be more explicit.

This part is currently not in the public spec, but I am working with others to
add this to the public spec.

>
> > +
> > +/*
> > + * This file contains both macros and data structures defined by the TDX
> > + * architecture and Linux defined software data structures and functions.
> > + * The two should not be mixed together for better readability. The
> > + * architectural definitions come first.
> > + */
> > +
> > +/*
> > + * Intel Trusted Domain CPU Architecture Extension spec:
> > + *
> > + * IA32_MTRRCAP:
> > + * Bit 15: The support of SEAMRR
> > + *
> > + * IA32_SEAMRR_PHYS_MASK (core-scope):
> > + * Bit 10: Lock bit
> > + * Bit 11: Enable bit
> > + */
> > +#define MTRR_CAP_SEAMRR BIT_ULL(15)
>
> Can you move this bit definition to arch/x86/include/asm/msr-index.h
> right after MSR_MTRRcap definition there?

The comment at the beginning of this file says:

/*
* CPU model specific register (MSR) numbers.
*
* Do not add new entries to this file unless the definitions are shared
* between multiple compilation units.
*/

I am not sure whether adding a new bit of one MSR (which is already defined) is
adding a "new entry". Perhaps it is not. But I'd like to leave to maintainers.

--
Thanks,
-Kai


\
 
 \ /
  Last update: 2022-06-23 11:25    [W:0.133 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site