Re: [PATCH v3 10/26] target/arm/kvm-rme: Add Realm Personalization Value parameter

[Daniel P . Berrangé]

On Tue, Nov 26, 2024 at 08:20:42AM +0100, Markus Armbruster wrote:
> Jean-Philippe Brucker <jean-philippe@linaro.org> writes:
> 
> > The Realm Personalization Value (RPV) is provided by the user to
> > distinguish Realms that have the same initial measurement.
> >
> > The user provides up to 64 hexadecimal bytes. They are stored into the
> > RPV in the same order, zero-padded on the right.
> >
> > Cc: Eric Blake <eblake@redhat.com>
> > Cc: Markus Armbruster <armbru@redhat.com>
> > Cc: Daniel P. Berrangé <berrange@redhat.com>
> > Cc: Eduardo Habkost <eduardo@habkost.net>
> > Acked-by: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > ---
> > v2->v3: Fix documentation
> > ---
> >  qapi/qom.json        |  15 ++++++
> >  target/arm/kvm-rme.c | 111 +++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 126 insertions(+)
> >
> > diff --git a/qapi/qom.json b/qapi/qom.json
> > index a8beeabf1f..f982850bca 100644
> > --- a/qapi/qom.json
> > +++ b/qapi/qom.json
> > @@ -1068,6 +1068,19 @@
> >    'data': { '*cpu-affinity': ['uint16'],
> >              '*node-affinity': ['uint16'] } }
> >  
> > +##
> > +# @RmeGuestProperties:
> > +#
> > +# Properties for rme-guest objects.
> > +#
> > +# @personalization-value: Realm personalization value, as a 64-byte
> > +#     hex string. This optional parameter allows to uniquely identify
> > +#     the VM instance during attestation. (default: 0)
> 
> QMP commonly uses base64 for encoding binary data.  Any particular
> reason to deviate?
> 
> Is the "hex string" to be mapped to binary in little or big endian?  Not
> an issue with base64.

Agreed, using base64 is preferrable for consistency.

> 
> Nitpick: (default: all-zero), please, for consistency with similar
> documentation in SevSnpGuestProperties.
> 
> > +#
> > +# Since: 9.3

There is never any x.3 version of QEMU, as we bump the major
version once a year. IOW, next release you could target this
for will be 10.0

> > +##
> > +{ 'struct': 'RmeGuestProperties',
> > +  'data': { '*personalization-value': 'str' } }
> >  
> >  ##
> >  # @ObjectType:
> > @@ -1121,6 +1134,7 @@
> >      { 'name': 'pr-manager-helper',
> >        'if': 'CONFIG_LINUX' },
> >      'qtest',
> > +    'rme-guest',
> >      'rng-builtin',
> >      'rng-egd',
> >      { 'name': 'rng-random',
> 
> The commit message claims the patch adds a parameter.  It actually adds
> an entire object type.

The object should be added to qom.json earlier in this series when
the RmeGuest class is defined, then this patch would merely be adding
the parameter.

> 
> > @@ -1195,6 +1209,7 @@
> >        'pr-manager-helper':          { 'type': 'PrManagerHelperProperties',
> >                                        'if': 'CONFIG_LINUX' },
> >        'qtest':                      'QtestProperties',
> > +      'rme-guest':                  'RmeGuestProperties',
> >        'rng-builtin':                'RngProperties',
> >        'rng-egd':                    'RngEgdProperties',
> >        'rng-random':                 { 'type': 'RngRandomProperties',

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|