[libvirt] [PATCH v5 05/10] qemu: add support to launch SEV guest

[Brijesh Singh]

On 4/2/18 6:04 PM, John Ferlan wrote:
>
> On 04/02/2018 10:18 AM, Brijesh Singh wrote:
>> QEMU >= 2.12 provides 'sev-guest' object which is used to launch encrypted
>> VMs on AMD platform using SEV feature. The various inputs required to
>> launch SEV guest is provided through the <launch-security> tag. A typical
>> SEV guest launch command line looks like this:
>>
>> # $QEMU ...\
>>   -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=5 ...\
>>   -machine memory-encryption=sev0 \
>>
>> Signed-off-by: Brijesh Singh <brijesh.singh at amd.com>
>> ---
>>  src/qemu/qemu_command.c | 35 +++++++++++++++++++++++++++++
>>  src/qemu/qemu_process.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 93 insertions(+)
>>
> (slight delay for next part of review - today was rocket launch day and
> then we headed out for a bit ;-))
>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 682d714..55bbfa2 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -7405,6 +7405,9 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
>>              virQEMUCapsGet(qemuCaps, QEMU_CAPS_LOADPARM))
>>              qemuAppendLoadparmMachineParm(&buf, def);
>>  
>> +        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST) && def->sev)
> Since we already checked sev-guest at prepare host storage (mostly
> unconditionally), I don't think we have to make the check here as well -
> although I could be wrong...


Yes, probably we can avoid the SEV_GUEST cap check in this case. I will
make the changes in next rev.

>> +            virBufferAddLit(&buf, ",memory-encryption=sev0");
>> +
>>          virCommandAddArgBuffer(cmd, &buf);
>>      }
>>  
>> @@ -9750,6 +9753,35 @@ qemuBuildTPMCommandLine(virCommandPtr cmd,
>>      return 0;
>>  }
>>  
>> +static void
> This probably should be an int...

I think it will be unlikely that this function will fail hence I was
using void. The only time it can fail is when we fail to create a
dh-cert and session blob file (e.g disk is full). I will propagate the
error.

>> +qemuBuildSevCommandLine(virDomainObjPtr vm, virCommandPtr cmd,
>> +                        virDomainSevDefPtr sev)
>> +{
>> +    virBuffer obj = VIR_BUFFER_INITIALIZER;
>> +    qemuDomainObjPrivatePtr priv = vm->privateData;
>> +    char *path = NULL;
>> +
>     if (!dev->sev)
>         return 0;
>
> again, since prepare host storage checked the sev-guest capability we
> should be good to go here...

OK.

>> +    VIR_DEBUG("policy=0x%x cbitpos=%d reduced_phys_bits=%d",
>> +              sev->policy, sev->cbitpos, sev->reduced_phys_bits);
>> +
>> +    virBufferAsprintf(&obj, "sev-guest,id=sev0,cbitpos=%d", sev->cbitpos);
>> +    virBufferAsprintf(&obj, ",reduced-phys-bits=%d", sev->reduced_phys_bits);
>> +    virBufferAsprintf(&obj, ",policy=0x%x", sev->policy);
> Here I would say:
>
>     if (sev->policy > 0)
>         virBufferAsprintf(&obj, ",policy=0x%x", sev->policy);
>
> and let qemu pick the default (which is 0x1 as I read that code).

OK, then I will remove the comment from html about the default value
since I was trying to not depend on QEMU default.

>> +
>> +    if (sev->dh_cert) {
>> +        ignore_value(virAsprintf(&path, "%s/dh_cert.base64", priv->libDir));
>> +        virBufferAsprintf(&obj, ",dh-cert-file=%s", path);
>> +        VIR_FREE(path);
>> +    }
>> +
>> +    if (sev->session) {
>> +        ignore_value(virAsprintf(&path, "%s/session.base64", priv->libDir));
>> +        virBufferAsprintf(&obj, ",session-file=%s", path);
>> +        VIR_FREE(path);
>> +    }
> ...since I don't believe we can ignore_value on the paths - especially
> since we're using it to create a path to a file containing some sort of
> session info or DH key (base64 encrypted).

Sure, will do.

>> +
>> +    virCommandAddArgList(cmd, "-object", virBufferContentAndReset(&obj), NULL);
>> +}
>>  
>>  static int
>>  qemuBuildVMCoreInfoCommandLine(virCommandPtr cmd,
>> @@ -10195,6 +10227,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
>>      if (qemuBuildVMCoreInfoCommandLine(cmd, def, qemuCaps) < 0)
>>          goto error;
>>  
>> +    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST) && def->sev)
>> +        qemuBuildSevCommandLine(vm, cmd, def->sev);
>> +
> I think we're save to change this to:
>
>     if (qemuBuildSevCommandLine(vm, cmd, dev->sev) < 0)
>         goto error;
>

Yep

>>      if (snapshot)
>>          virCommandAddArgList(cmd, "-loadvm", snapshot->def->name, NULL);
>>  
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index c0105c8..0c93f15 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -5745,6 +5745,61 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver,
>>      return ret;
>>  }
>>  
> Two blank lines
>
>> +static int
>> +qemuBuildSevCreateFile(const char *configDir, const char *name,
>> +                       const char *data)
> 3 lines for args
>
>> +{
>> +    char *configFile;
>> +
>> +    if (!(configFile = virFileBuildPath(configDir, name, ".base64")))
>> +        return -1;
>> +
>> +    if (virFileRewriteStr(configFile, S_IRUSR | S_IWUSR, data) < 0) {
>> +        virReportSystemError(errno, _("failed to write data to config '%s'"),
>> +                             configFile);
>> +        goto error;
>> +    }
> Check out storageBackendCreateQemuImgSecretPath which just goes straight
> to safewrite when writing to the file or qemuDomainWriteMasterKeyFile
> which is a similar w/r/t a single key file for the domain.
>
> The one thing to think about being the privileges for the file being
> created and written and the expectations for QEMU's usage. I think this
> is more like the storage secret code, but I could be wrong!

The data is public in this case, we do not need to protect it with
secret. Hence I am keeping all this certificate keys in unsecure place.
>> +
>> +    VIR_FREE(configFile);
>> +    return 0;
>> +
>> + error:
>> +    VIR_FREE(configFile);
>> +    return -1;
>> +}
>> +
> 2 blank lines
>
>> +static int
>> +qemuProcessPrepareSevGuestInput(virDomainObjPtr vm)
>> +{
>> +    qemuDomainObjPrivatePtr priv = vm->privateData;
>> +    virDomainDefPtr def = vm->def;
>> +    virQEMUCapsPtr qemuCaps = priv->qemuCaps;
>> +    virDomainSevDefPtr sev = def->sev;
>> +
>> +    if (!sev)
>> +        return 0;
>> +
>> +    VIR_DEBUG("Prepare SEV guest");
>> +
>> +    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                        _("Domain %s asked for 'sev' launch but "
> s/but/but this/

Noted.

>
>> +                          "QEMU does not support SEV feature"), vm->def->name);
>> +        return -1;
>> +    }
> Since we check for this here - I think this should be good enough for
> command line building.  That is - qemuProcessPrepareHostStorage will
> come before command line building - so the check is already made.
>

Got it.

>> +
>> +    if (sev->dh_cert) {
>> +        if (qemuBuildSevCreateFile(priv->libDir, "dh_cert", sev->dh_cert) < 0)
>> +            return -1;
>> +    }
>> +
>> +    if (sev->session) {
>> +        if (qemuBuildSevCreateFile(priv->libDir, "session", sev->session) < 0)
>> +            return -1;
>> +    }
>> +
>> +    return 0;
>> +}
> 2 blank lines
>
> John
>
>>  
>>  static int
>>  qemuProcessPrepareHostStorage(virQEMUDriverPtr driver,
>> @@ -5870,6 +5925,9 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver,
>>      if (qemuProcessPrepareHostStorage(driver, vm, flags) < 0)
>>          goto cleanup;
>>  
>> +    if (qemuProcessPrepareSevGuestInput(vm) < 0)
>> +        goto cleanup;
>> +
>>      ret = 0;
>>   cleanup:
>>      virObjectUnref(cfg);
>>