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

[tobin <tobin@linux.vnet.ibm.com>]

On 2020-05-28 17:00, James Bottomley wrote:
> 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.
> 
I think having GPA as an optional argument might make the most sense.
Users may or may not know how to use the argument, but it is probably
a good idea to give another option besides sticking the GPA into the 
ROM.

>> +# 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'
> 
Happy to change this.

>> +  '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?
> 
Yes.
> James