Message-ID: <8e3736c1a0b650179dab177feafdef1a596f81c7.camel@intel.com>
Date: Thu, 6 Mar 2025 01:20:52 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
	"bp@...en8.de" <bp@...en8.de>, "x86@...nel.org" <x86@...nel.org>,
	"mingo@...hat.com" <mingo@...hat.com>, "Williams, Dan J"
	<dan.j.williams@...el.com>, "kirill.shutemov@...ux.intel.com"
	<kirill.shutemov@...ux.intel.com>, "Xing, Cedric" <cedric.xing@...el.com>,
	"tglx@...utronix.de" <tglx@...utronix.de>, "hpa@...or.com" <hpa@...or.com>
CC: "sathyanarayanan.kuppuswamy@...ux.intel.com"
	<sathyanarayanan.kuppuswamy@...ux.intel.com>, "dionnaglaze@...gle.com"
	<dionnaglaze@...gle.com>, "linux-coco@...ts.linux.dev"
	<linux-coco@...ts.linux.dev>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "James.Bottomley@...senPartnership.com"
	<James.Bottomley@...senPartnership.com>, "mikko.ylinen@...ux.intel.com"
	<mikko.ylinen@...ux.intel.com>, "dan.middleton@...ux.intel.com"
	<dan.middleton@...ux.intel.com>
Subject: Re: [PATCH v2 1/4] tsm: Add TVM Measurement Register support

On Sun, 2025-02-23 at 21:20 -0600, Cedric Xing wrote:
> Add new TSM APIs for TVM guest drivers to register and expose measurement
> registers (MRs) as sysfs attributes (files).

Hi Cedric,

The current TSM is done in configfs, but not sysfs.  The reason, quoted from
commit 70e6f7e2b9857 ("configfs-tsm: Introduce a shared ABI for attestation
reports"), is:

    Review of previous iterations of this interface identified that there is
    a need to scale report generation for multiple container environments
    [2]. Configfs enables a model where each container can bind mount one or
    more report generation item instances. Still, within a container only a
    single thread can be manipulating a given configuration instance at a
    time. A 'generation' count is provided to detect conflicts between
    multiple threads racing to configure a report instance.

And the link [2] (where you can find the relevant discussion) is:

http://lore.kernel.org/r/57f3a05e-8fcd-4656-beea-56bb8365ae64@linux.microsoft.com

Could you elaborate why do you choose to expose MRs via sysfs rather than
configfs?  Is the above reason not valid anymore?


> 
> New TSM APIs:
> 
> - `tsm_register_measurement(struct tsm_measurement *)`: Register a set of
>   MRs with the TSM core.
> - `tsm_unregister_measurement(struct tsm_measurement *)`: Unregister a
>   previously registered set of MRs.
> 
> These APIs are centered around `struct tsm_measurement`, which includes an
> array of `struct tsm_measurement_register`s with properties
> (`tsm_measurement_register::mr_flags`) like *Readable* (`TSM_MR_F_R`) and
> *Extensible* (`TSM_MR_F_X`). For details, see include/linux/tsm.h.

Nit:

We can see those details from the code.  Personally I think you don't need to
describe them again in the changelog.  It would be more helpful if you could put
more _why_ here.

E.g., Wwhat is userspace's requirement/flow that involves reading/extending
those MRs?  An example is even better.

> 
> Upon successful registration, the TSM core exposes MRs in sysfs at
> /sys/kernel/tsm/MR_PROVIDER/, where MR_PROVIDER is the measurement
> provider's name (`tsm_measurement::name`). Each MR is accessible either as
> a file (/sys/kernel/tsm/MR_PROVIDER/MR_NAME contains the MR value) or a
> directory (/sys/kernel/tsm/MR_PROVIDER/MR_NAME/HASH/digest contains the MR
> value) depending on whether `TSM_MR_F_F` is set or cleared (in
> `tsm_measurement_register::mr_flags`). MR_NAME is the name
> (`tsm_measurement_register::mr_name`) of the MR, while HASH is the hash
> algorithm (`tsm_measurement_register::mr_hash`) name in the latter case.

Please correct me if I am wrong: in my understanding, the purpose is to provide
a "unified ABI for usrspace" for MRs, but not just some common infrastructure in
the kernel to support exposing MRs, right?

Configfs-tsm provides consistent names for all attributes for all vendors:
'inblob', 'outblob', 'generation', 'provider' etc, so it provides a unified ABI
for userspace.

But here actually each vendor will have its own directory.  E.g., for TDX we
have:

	/sys/kernel/tsm/tdx/...

And the actual MRs under the vendor-specific directory are completely vendor-
specific.  E.g., as shown in the last patch, for TDX we have: mrconfigid,
mrowner etc.  And for other vendors they are free to register MRs on their own.

Could you elaborate how userspace is supposed to use those MRs in a common way?
 
Or this is not the purpose?

> 
> *Crypto Agility* is supported by merging independent MRs with a common name
> into a single directory, each represented by its HASH/digest file. Note
> that HASH must be distinct or behavior is undefined.

Ditto.  I think it would be more helpful if you can provide _why_ we need to
support crypto agility rather than _how_ is it implemented, which can be seen
from the actual code.  Once merged, the _why_ will be helpful when some random
guy in the future tries to git blame and figure out the story behind.

Powered by blists - more mailing lists