Re: [PATCH 1/2] sev: add sev-inject-launch-secret

[James Bottomley]

On Thu, 2020-05-28 at 16:51 -0400, Tobin Feldman-Fitzthum wrote:
> --- a/qapi/misc-target.json
> +++ b/qapi/misc-target.json
> @@ -200,6 +200,26 @@
>  { 'command': 'query-sev-capabilities', 'returns': 'SevCapability',
>    'if': 'defined(TARGET_I386)' }
>  
> +##
> +# @sev-inject-launch-secret:
> +#
> +# This command injects a secret blob into memory of SEV guest.
> +#
> +# @packet-header: the launch secret packet header encoded in base64
> +#
> +# @secret: the launch secret data to be injected encoded in base64
> +#
> +# @gpa: the guest physical address where secret will be injected.
> +        GPA provided here will be ignored if guest ROM specifies 
> +        the a launch secret GPA.

Shouldn't we eliminate the gpa argument to this now the gpa is
extracted from OVMF?  You add it here but don't take it out in the next
patch.

> +# Since: 5.0.0
> +#
> +##
> +{ 'command': 'sev-inject-launch-secret',
> +  'data': { 'packet_hdr': 'str', 'secret': 'str', 'gpa': 'uint64' },

Java (i.e. Json) people hate underscores and abbreviations.  I bet
they'll want this to be 'packet-header'

> +  'if': 'defined(TARGET_I386)' }
> +
>  ##
>  # @dump-skeys:
>  #
> diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> index 27ebfa3ad2..5c2b7d2c17 100644
> --- a/target/i386/monitor.c
> +++ b/target/i386/monitor.c
> @@ -736,3 +736,11 @@ SevCapability *qmp_query_sev_capabilities(Error
> **errp)
>  
>      return data;
>  }
> +
> +void qmp_sev_inject_launch_secret(const char *packet_hdr,
> +                                  const char *secret, uint64_t gpa,
> +                                  Error **errp)
> +{
> +    if (sev_inject_launch_secret(packet_hdr,secret,gpa) != 0)
> +      error_setg(errp, "SEV inject secret failed");
> +}
> diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
> index e5ee13309c..2b8c5f1f53 100644
> --- a/target/i386/sev-stub.c
> +++ b/target/i386/sev-stub.c
> @@ -48,3 +48,8 @@ SevCapability *sev_get_capabilities(void)
>  {
>      return NULL;
>  }
> +int sev_inject_launch_secret(const char *hdr, const char *secret,
> +                                          uint64_t gpa)
> +{
> +         return 1;
> +}
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 846018a12d..774e47d9d1 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -28,6 +28,7 @@
>  #include "sysemu/runstate.h"
>  #include "trace.h"
>  #include "migration/blocker.h"
> +#include "exec/address-spaces.h"
>  
>  #define DEFAULT_GUEST_POLICY    0x1 /* disable debug */
>  #define DEFAULT_SEV_DEVICE      "/dev/sev"
> @@ -743,6 +744,88 @@ sev_encrypt_data(void *handle, uint8_t *ptr,
> uint64_t len)
>      return 0;
>  }
>  
> +
> +static void *
> +gpa2hva(hwaddr addr, uint64_t size)
> +{
> +    MemoryRegionSection mrs =
> memory_region_find(get_system_memory(),
> +                                                 addr, size);
> +
> +    if (!mrs.mr) {
> +        error_report("No memory is mapped at address 0x%"
> HWADDR_PRIx, addr);
> +        return NULL;
> +    }
> +
> +    if (!memory_region_is_ram(mrs.mr) &&
> !memory_region_is_romd(mrs.mr)) {
> +        error_report("Memory at address 0x%" HWADDR_PRIx "is not
> RAM", addr);
> +        memory_region_unref(mrs.mr);
> +        return NULL;
> +    }

We can still check this, but it should be like an assertion failure. 
Since the GPA is selected by the OVMF build there should be no way it
can't be mapped into the host.

[...]
> --- a/tests/qtest/qmp-cmd-test.c
> +++ b/tests/qtest/qmp-cmd-test.c
> @@ -93,10 +93,10 @@ static bool query_is_blacklisted(const char *cmd)
>          /* Success depends on target-specific build configuration:
> */
>          "query-pci",              /* CONFIG_PCI */
>          /* Success depends on launching SEV guest */
> -        "query-sev-launch-measure",
> +        // "query-sev-launch-measure",
>          /* Success depends on Host or Hypervisor SEV support */
> -        "query-sev",
> -        "query-sev-capabilities",
> +        // "query-sev",
> +        // "query-sev-capabilities",

We're eliminating existing tests ... is that just a stray hunk that you
forgot to remove?

James