Re: [PATCH v2 2/2] sev: update sev-inject-launch-secret to make gpa optional

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* James Bottomley (jejb@linux.ibm.com) wrote:
> On Tue, 2021-01-26 at 12:32 +0000, Dr. David Alan Gilbert wrote:
> > * James Bottomley (jejb@linux.ibm.com) wrote:
> > > If the gpa isn't specified, it's value is extracted from the OVMF
> > > properties table located below the reset vector (and if this
> > > doesn't
> > > exist, an error is returned).  OVMF has defined the GUID for the
> > > SEV
> > > secret area as 4c2eb361-7d9b-4cc3-8081-127c90d3d294 and the format
> > > of
> > > the <data> is: <base>|<size> where both are uint32_t.  We extract
> > > <base> and use it as the gpa for the injection.
> > > 
> > > Note: it is expected that the injected secret will also be GUID
> > > described but since qemu can't interpret it, the format is left
> > > undefined here.
> > > 
> > > Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> > > 
> > > ---
> > > 
> > > v2: fix line length warning, add more comments about sev area
> > > ---
> > >  qapi/misc-target.json |  2 +-
> > >  target/i386/monitor.c | 27 ++++++++++++++++++++++++++-
> > >  2 files changed, 27 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> > > index 06ef8757f0..0c7491cd82 100644
> > > --- a/qapi/misc-target.json
> > > +++ b/qapi/misc-target.json
> > > @@ -216,7 +216,7 @@
> > >  #
> > >  ##
> > >  { 'command': 'sev-inject-launch-secret',
> > > -  'data': { 'packet-header': 'str', 'secret': 'str', 'gpa':
> > > 'uint64' },
> > > +  'data': { 'packet-header': 'str', 'secret': 'str', '*gpa':
> > > 'uint64' },
> > >    'if': 'defined(TARGET_I386)' }
> > >  
> > >  ##
> > > diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> > > index 1bc91442b1..11bdb04155 100644
> > > --- a/target/i386/monitor.c
> > > +++ b/target/i386/monitor.c
> > > @@ -34,6 +34,7 @@
> > >  #include "sev_i386.h"
> > >  #include "qapi/qapi-commands-misc-target.h"
> > >  #include "qapi/qapi-commands-misc.h"
> > > +#include "hw/i386/pc.h"
> > >  
> > >  /* Perform linear address sign extension */
> > >  static hwaddr addr_canonical(CPUArchState *env, hwaddr addr)
> > > @@ -730,9 +731,33 @@ SevCapability
> > > *qmp_query_sev_capabilities(Error **errp)
> > >      return sev_get_capabilities(errp);
> > >  }
> > >  
> > > +#define SEV_SECRET_GUID "4c2eb361-7d9b-4cc3-8081-127c90d3d294"
> > > +struct sev_secret_area {
> > > +    uint32_t base;
> > > +    uint32_t size;
> > > +};
> > > +
> > >  void qmp_sev_inject_launch_secret(const char *packet_hdr,
> > > -                                  const char *secret, uint64_t
> > > gpa,
> > > +                                  const char *secret,
> > > +                                  bool has_gpa, uint64_t gpa,
> > >                                    Error **errp)
> > >  {
> > > +    if (!has_gpa) {
> > > +        uint8_t *data;
> > > +        struct sev_secret_area *area;
> > > +
> > > +        /*
> > > +         * not checking length means that this area can't be
> > > versioned
> > > +         * by length and would have to be replaced if updated
> > > +         */
> > 
> > Can you just explain that a bit more?
> 
> It's referring back to the original concept that the reset vector
> length would tell you what version of the thing you were using.  So if
> you were looking for a property at offset 10 and the length came in as
> 8 the version was too early.  If it was 18 you had a later version and
> your property was present.
> 
> The current scheme uses guids which can be versioned by length if you
> think you'll add extra properties to them.  This one I don't think
> would ever get an extra property, so there's no point checking the
> length.  Not checking the length means if I'm wrong and we do need an
> extra property it will have to be attached to a new guid.
> 
> That's a bit confusing to add to the comment ... how about I just leave
> out the comment entirely?

Yes, I don't think it makes much sense unless you knew the history.

Dave

> > > +        if (!pc_system_ovmf_table_find(SEV_SECRET_GUID, &data,
> > > NULL)) {
> > > +            error_setg(errp, "SEV: no secret area found in OVMF,"
> > > +                       " gpa must be specified.");
> > > +            return;
> > > +        }
> > > +        area = (struct sev_secret_area *)data;
> > > +        gpa = area->base;
> > > +    }
> > > +
> > >      sev_inject_launch_secret(packet_hdr, secret, gpa, errp);
> > 
> > Other than me not understanding that comment, I think we're fine:
> 
> Thanks.
> 
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > 
> > >  }
> > > -- 
> > > 2.26.2
> > > 
> > > 
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK