qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: James Bottomley <jejb@linux.ibm.com>
To: Tom Lendacky <thomas.lendacky@amd.com>, qemu-devel@nongnu.org
Cc: ashish.kalra@amd.com, brijesh.singh@amd.com,
	david.kaplan@amd.com, jon.grimm@amd.com, tobin@ibm.com,
	frankeh@us.ibm.com,
	"Dr . David Alan Gilbert" <dgilbert@redhat.com>,
	dovmurik@linux.vnet.ibm.com, Dov.Murik1@il.ibm.com,
	pbonzini@redhat.com, berrange@redhat.com,
	Laszlo Ersek <lersek@redhat.com>
Subject: Re: [PATCH 3/3] sev: update sev-inject-launch-secret to make gpa optional
Date: Fri, 11 Dec 2020 14:45:17 -0800	[thread overview]
Message-ID: <aa8ebbb5b14821bcaf1d599511f75060d9dcc460.camel@linux.ibm.com> (raw)
In-Reply-To: <afe66ae1-a1d9-c017-b05d-12247350338b@amd.com>

On Fri, 2020-12-11 at 16:00 -0600, Tom Lendacky wrote:
> On 12/9/20 11:23 AM, James Bottomley 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>
> > ---
> >   qapi/misc-target.json |  2 +-
> >   target/i386/monitor.c | 22 +++++++++++++++++++++-
> >   2 files changed, 22 insertions(+), 2 deletions(-)
> > 
> > diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> > index 4486a543ae..1ee4e62f85 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..a99e3dd2b3 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,28 @@ 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;
> > +};
> > +
> 
> Originally, the idea was to allow expanding of these GUID based
> structures by pre-pending data to them, but based on how
> pc_system_ovmf_table_find() returns the pointer to the start of the
> structure (based on the length found in the structure), I believe
> that expansion could be done by appending to the structure, which
> seems more logical. For example, if this structure is ever expanded,
> it can use the third parameter of pc_system_ovmf_table_find() to get
> the length and compare that to the size  of the structure to
> determine if new version of the structure is present in the firmware.

Actually, I don't think it much matters.  It looks like the len it
would return is wrong ... it should be the length of just the returned
data pointer (without the length or guid), so ptr+len would point to
the foot of the data if that's what you want.

> Otherwise you can't do the nice easy assignment below:
>    area = (struct sev_secret_area *)data;
> 
> You actually have to do some math:
>    area = (struct sev_secret_area *)(data + data_len -
>                                      sizeof(QemuUUID) -
> sizeof(uint16_t) -
> 				    sizeof(*area));
> 
> or add the QemuUUID and uint16_t fields to sev_secret_area and:
>    area = (struct sev_secret_area *)(data + data_len -
> sizeof(*area));

Right, that's why I think patch 2/3 should do

    *data_len = len - sizeof(QemuUUID) - sizeof(uint16_t)

> Or we make the decision that these GUID structs should never change,
> just add a new one to the table if more info is needed.

Actually, the fact that the only guid the table depends on is the table
footer GUID, you can always remove guids and add new ones.  So I think
it's up to whoever is using the GUID to decide the policy.

So for this one I'm not checking the length, which argues it wouldn't
be subject to the added length new data rule and I'd have to use a new
guid for new information.  However, I could also see situations where
you would check the length and thus would have the ability to add
fields (either at the beginning or the end).

> Whatever we decide should probably be documented in both the OVMF
> patches and the Qemu patches.

OK, I can add a comment about my use case and you can add one
documenting your length based use case.

James




  reply	other threads:[~2020-12-11 23:16 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-09 17:23 [PATCH 0/3] sev: enable seret injection to a self described area in OVMF James Bottomley
2020-12-09 17:23 ` [PATCH 1/3] sev: add sev-inject-launch-secret James Bottomley
2020-12-09 17:23 ` [PATCH 2/3] pc: add parser for OVMF reset block James Bottomley
2020-12-09 17:23 ` [PATCH 3/3] sev: update sev-inject-launch-secret to make gpa optional James Bottomley
2020-12-11 22:00   ` Tom Lendacky
2020-12-11 22:45     ` James Bottomley [this message]
2020-12-11 22:54       ` Tom Lendacky
2020-12-14 14:14         ` Laszlo Ersek
2020-12-09 17:52 ` [PATCH 0/3] sev: enable seret injection to a self described area in OVMF James Bottomley
2020-12-09 19:33 ` no-reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aa8ebbb5b14821bcaf1d599511f75060d9dcc460.camel@linux.ibm.com \
    --to=jejb@linux.ibm.com \
    --cc=Dov.Murik1@il.ibm.com \
    --cc=ashish.kalra@amd.com \
    --cc=berrange@redhat.com \
    --cc=brijesh.singh@amd.com \
    --cc=david.kaplan@amd.com \
    --cc=dgilbert@redhat.com \
    --cc=dovmurik@linux.vnet.ibm.com \
    --cc=frankeh@us.ibm.com \
    --cc=jon.grimm@amd.com \
    --cc=lersek@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=thomas.lendacky@amd.com \
    --cc=tobin@ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).