Re: [PATCH v2 20/58] i386/tdx: Allows mrconfigid/mrowner/mrownerconfig for TDX_INIT_VM

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, Aug 18, 2023 at 05:50:03AM -0400, Xiaoyao Li wrote:
>> From: Isaku Yamahata <isaku.yamahata@intel.com>
>> 
>> When creating TDX vm, three sha384 hash values can be provided for
>> TDX attestation.
>> 
>> So far they were hard coded as 0. Now allow user to specify those values
>> via property mrconfigid, mrowner and mrownerconfig. Choose hex-encoded
>> string as format since it's friendly for user to input.
>> 
>> example
>> -object tdx-guest, \
>>   
>> mrconfigid=0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef,
>>  \
>>   
>> mrowner=fedcba9876543210fedcba9876543210fedcba9876543210fedcba9876543210fedcba9876543210fedcba9876543210,
>>  \
>>   
>> mrownerconfig=0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef
>> 
>> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>> TODO:
>>  - community requests to use base64 encoding if no special reason
>> ---
>>  qapi/qom.json         | 11 ++++++++++-
>>  target/i386/kvm/tdx.c | 13 +++++++++++++
>>  target/i386/kvm/tdx.h |  3 +++
>>  3 files changed, 26 insertions(+), 1 deletion(-)
>> 
>> diff --git a/qapi/qom.json b/qapi/qom.json
>> index cc08b9a98df9..87c1d440f331 100644
>> --- a/qapi/qom.json
>> +++ b/qapi/qom.json
>> @@ -873,10 +873,19 @@
>>  #
>>  # @sept-ve-disable: bit 28 of TD attributes (default: 0)
>>  #
>> +# @mrconfigid: MRCONFIGID SHA384 hex string of 48 * 2 length (default: 0)
>> +#
>> +# @mrowner: MROWNER SHA384 hex string of 48 * 2 length (default: 0)
>> +#
>> +# @mrownerconfig: MROWNERCONFIG SHA384 hex string of 48 * 2 length 
>> (default: 0)
>
> Per previous patch, I suggest these should all be passed in base64
> instead of hex.

I'm upgrading this suggestion to a demand: we use base64 for encoding
binary data everywhere in QAPI/QMP.  Consistency matters.

>                 Also 'default: 0' makes no sense for a string,
> which would be 'default: nil', and no need to document that as
> the default is implicit from the fact that its an optional string
> field. So eg
>
>   @mrconfigid: base64 encoded MRCONFIGID SHA384 digest

Agree.

The member names are abbreviations all run together, wheras QAPI/QMP
favors words-separated-with-dashes.  If you invented them, please change
them to QAPI/QMP style.  If they are established TDX terminology, keep
them as they are, but please point to your evidence.

>> +#
>>  # Since: 8.2
>>  ##
>>  { 'struct': 'TdxGuestProperties',
>> -  'data': { '*sept-ve-disable': 'bool' } }
>> +  'data': { '*sept-ve-disable': 'bool',
>> +            '*mrconfigid': 'str',
>> +            '*mrowner': 'str',
>> +            '*mrownerconfig': 'str' } }
>>  
>>  ##
>>  # @ThreadContextProperties:

[...]