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

[Jean-Philippe Brucker]

On Tue, Nov 26, 2024 at 08:20:42AM +0100, Markus Armbruster wrote:
> > +# @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?

No, I think base64 is fine for this

> 
> Is the "hex string" to be mapped to binary in little or big endian?  Not
> an issue with base64.

(It was big endian with padding on the right)

> 
> Nitpick: (default: all-zero), please, for consistency with similar
> documentation in SevSnpGuestProperties.
> 
> > +#
> > +# Since: 9.3
> > +##
> > +{ '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.

Yes as Daniel noted this belongs in an earlier patch

> 
> > @@ -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',
> > diff --git a/target/arm/kvm-rme.c b/target/arm/kvm-rme.c
> > index 83a29421df..0be55867ee 100644
> > --- a/target/arm/kvm-rme.c
> > +++ b/target/arm/kvm-rme.c
> > @@ -23,11 +23,15 @@ OBJECT_DECLARE_SIMPLE_TYPE(RmeGuest, RME_GUEST)
> >  
> >  #define RME_PAGE_SIZE qemu_real_host_page_size()
> >  
> > +#define RME_MAX_CFG         1
> > +
> >  struct RmeGuest {
> >      ConfidentialGuestSupport parent_obj;
> >      Notifier rom_load_notifier;
> >      GSList *ram_regions;
> >  
> > +    uint8_t *personalization_value;
> > +
> >      hwaddr ram_base;
> >      size_t ram_size;
> >  };
> > @@ -43,6 +47,48 @@ typedef struct {
> >  
> >  static RmeGuest *rme_guest;
> >  
> > +static int rme_configure_one(RmeGuest *guest, uint32_t cfg, Error **errp)
> > +{
> > +    int ret;
> > +    const char *cfg_str;
> > +    struct kvm_cap_arm_rme_config_item args = {
> > +        .cfg = cfg,
> > +    };
> > +
> > +    switch (cfg) {
> > +    case KVM_CAP_ARM_RME_CFG_RPV:
> > +        if (!guest->personalization_value) {
> > +            return 0;
> > +        }
> > +        memcpy(args.rpv, guest->personalization_value, 
> > KVM_CAP_ARM_RME_RPV_SIZE);
> > +        cfg_str = "personalization value";
> > +        break;
> > +    default:
> > +        g_assert_not_reached();
> > +    }
> 
> This function gets called with @cfg arguments 0 .. RME_MAX_CFG-1, from
> rme_configure() right below.  RME_MAX_CFG is defined to 1 above.
> 
> The switch assumes KVM_CAP_ARM_RME_CFG_RPV is zero.  Such assumptions
> are ideally avoided, and alternatively checked at build time.

Ok, I'll change this

Thanks,
Jean