Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <d6d7d42f494320500ce3fbbdd4810482348dc5d4.camel@intel.com>
Subject: Re: [PATCH v5 01/22] x86/virt/tdx: Detect TDX during kernel boot
From:   Kai Huang <kai.huang@intel.com>
To:     Chao Gao <chao.gao@intel.com>
Cc:     linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        seanjc@google.com, pbonzini@redhat.com, dave.hansen@intel.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
Date:   Thu, 23 Jun 2022 21:23:41 +1200
In-Reply-To: <20220623055658.GA2934@gao-cwp>
References: <cover.1655894131.git.kai.huang@intel.com>
         <062075b36150b119bf2d0a1262de973b0a2b11a7.1655894131.git.kai.huang@intel.com>
         <20220623055658.GA2934@gao-cwp>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.2 (3.44.2-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

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 calle=
d
> > 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 reg=
ister
(SEAMRR)".

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

OK will do.

>=20
> > +
> > +	tdx_keyid_num =3D TDX_KEYID_NUM(keyid_part);
> > +	tdx_keyid_start =3D 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).
>=20
> 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.

>=20
> > +
> > +/*
> > + * This file contains both macros and data structures defined by the T=
DX
> > + * architecture and Linux defined software data structures and functio=
ns.
> > + * 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)
>=20
> 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 maintai=
ners.

--=20
Thanks,
-Kai