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

[James Bottomley]

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?

> > +        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
> > 
> >