Re: [PATCH v4 29/66] i386/tdx: Support user configurable mrconfigid/mrowner/mrownerconfig

[Xiaoyao Li]

On 2/19/2024 8:48 PM, Markus Armbruster wrote:
> Xiaoyao Li <xiaoyao.li@intel.com> writes:
>
> > From: Isaku Yamahata <isaku.yamahata@intel.com>
> >
> > Three sha384 hash values, mrconfigid, mrowner and mrownerconfig, of a TD
> > can be provided for TDX attestation. Detailed meaning of them can be
> > found: 
> > https://lore.kernel.org/qemu-devel/31d6dbc1-f453-4cef-ab08-4813f4e0ff92@intel.com/
> >
> > Allow user to specify those values via property mrconfigid, mrowner and
> > mrownerconfig. They are all in base64 format.
> >
> > example
> > -object tdx-guest, \
> >    mrconfigid=ASNFZ4mrze8BI0VniavN7wEjRWeJq83vASNFZ4mrze8BI0VniavN7wEjRWeJq83v,\
> >    mrowner=ASNFZ4mrze8BI0VniavN7wEjRWeJq83vASNFZ4mrze8BI0VniavN7wEjRWeJq83v,\
> >    
> > mrownerconfig=ASNFZ4mrze8BI0VniavN7wEjRWeJq83vASNFZ4mrze8BI0VniavN7wEjRWeJq83v
> >
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> > Co-developed-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> >
> > ---
> > Changes in v4:
> >   - describe more of there fields in qom.json
> >   - free the old value before set new value to avoid memory leak in
> >     _setter(); (Daniel)
> >
> > Changes in v3:
> >   - use base64 encoding instread of hex-string;
> > ---
> >   qapi/qom.json         | 14 ++++++-
> >   target/i386/kvm/tdx.c | 87 +++++++++++++++++++++++++++++++++++++++++++
> >   target/i386/kvm/tdx.h |  3 ++
> >   3 files changed, 103 insertions(+), 1 deletion(-)
> >
> > diff --git a/qapi/qom.json b/qapi/qom.json
> > index 2177f3101382..15445f9e41fc 100644
> > --- a/qapi/qom.json
> > +++ b/qapi/qom.json
> > @@ -905,10 +905,22 @@
> >   #     pages.  Some guest OS (e.g., Linux TD guest) may require this to
> >   #     be set, otherwise they refuse to boot.
> >   #
> > +# @mrconfigid: ID for non-owner-defined configuration of the guest TD,
> > +#     e.g., run-time or OS configuration.  base64 encoded SHA384 digest.
> "base64 encoded SHA384" is not a sentence.
>
> Double-checking: the data being hashed here is the "non-owner-defined
> configuration of the guest TD", and the resulting hash is the "ID"?
yes. The "ID" here means the resulting hash.

The reason to use "ID" here because in the TDX spec, it's description is

  Software-defined ID for non-owner-defined configuration of the guest
  TD - e.g., run-time or OS configuration.

If ID is confusing, how about

  SHA384 hash of non-owner-defined configuration of the guest TD, e.g.,
  run-time of OS configuration.  It's base64 encoded.

> > +#
> > +# @mrowner: ID for the guest TD’s owner.  base64 encoded SHA384 digest.
> Likewise.
>
> > +#
> > +# @mrownerconfig: ID for owner-defined configuration of the guest TD,
> > +#     e.g., specific to the workload rather than the run-time or OS.
> > +#     base64 encoded SHA384 digest.
> Likewise.
>
> > +#
> >   # Since: 9.0
> >   ##
> >   { 'struct': 'TdxGuestProperties',
> > -  'data': { '*sept-ve-disable': 'bool' } }
> > +  'data': { '*sept-ve-disable': 'bool',
> > +            '*mrconfigid': 'str',
> > +            '*mrowner': 'str',
> > +            '*mrownerconfig': 'str' } }
> The new members are optional, but their description in the doc comment
> doesn't explain behavior when present vs. behavior when absent.
>
> >   
> >   ##
> >   # @ThreadContextProperties:
> [...]