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
