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

[Tom Lendacky]

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.

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));

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.

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

Thanks,
Tom

>   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;
> +
> +        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);
>   }