Re: [PATCH v2 1/4] tsm: Add TVM Measurement Register support

[Huang, Kai]

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@xxxxxxxxxxxxxxxxxxx

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.