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

[John Ferlan]

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...

> +            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...

> +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...

> +    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).

> +
> +    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).

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

>      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!

> +
> +    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/

> +                          "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.

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