Re: [PATCH v2 4/9] qemu: add support to launch SEV guest

[Brijesh Singh <brijesh.singh@xxxxxxx>]

On 03/12/2018 08:41 AM, Daniel P. Berrangé wrote:
> On Thu, Mar 08, 2018 at 11:12:03AM -0600, 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@xxxxxxx>
> > ---
> >   src/qemu/qemu_command.c | 33 ++++++++++++++++++
> >   src/qemu/qemu_process.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 124 insertions(+)
> >
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index fa0aa5d5c3d4..39f136a389cb 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -9663,6 +9663,36 @@ qemuBuildTPMCommandLine(virCommandPtr cmd,
> >       return 0;
> >   }
> >   
> > +static void
> > +qemuBuildSevCommandLine(virCommandPtr cmd,
> > +                        virDomainSevDefPtr sev)
> > +{
> > +    virBuffer obj = VIR_BUFFER_INITIALIZER;
> > +    char *path = NULL;
> > +
> > +    VIR_DEBUG("policy=0x%x cbitpos=%d reduced_phys_bits=%d",
> > +              sev->policy, sev->cbitpos, sev->reduced_phys_bits);
> > +
> > +    virCommandAddArgList(cmd, "-machine", "memory-encryption=sev0", NULL);
>
>
> Hmm, so this will result in us passing  '-machine' many times, and relying
> on QEMU to acculate the args. It should work, but IME it could be a little
> surprising to people debuggnig qemu to see repeated args for -machine.
>
> Could we just call this method from the place that  constructs the
> current -machine arg so we append to that.


OK, I will take a look and try to avoid duplicate "-machine".

> > +
> > +    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);
> > +
> > +    if (sev->dh_cert) {
> > +        ignore_value(virAsprintf(&path, "%s/dh_cert.base64", sev->configDir));
> > +        virBufferAsprintf(&obj, ",dh-cert-file=%s", path);
> > +        VIR_FREE(path);
> > +    }
> > +
> > +    if (sev->session) {
> > +        ignore_value(virAsprintf(&path, "%s/session.base64", sev->configDir));
> > +        virBufferAsprintf(&obj, ",session-file=%s", path);
> > +        VIR_FREE(path);
> > +    }
> > +
> > +    virCommandAddArgList(cmd, "-object", virBufferContentAndReset(&obj), NULL);
> > +}
> >   
> >   static int
> >   qemuBuildVMCoreInfoCommandLine(virCommandPtr cmd,
> > @@ -10108,6 +10138,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
> >       if (qemuBuildVMCoreInfoCommandLine(cmd, def, qemuCaps) < 0)
> >           goto error;
> >   
> > +    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV) && def->sev)
> > +        qemuBuildSevCommandLine(cmd, def->sev);
> > +
> >       if (snapshot)
> >           virCommandAddArgList(cmd, "-loadvm", snapshot->def->name, NULL);
> >   
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index 57c06c7c1550..349e12b6dc12 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> > @@ -3457,6 +3457,16 @@ qemuProcessBuildDestroyMemoryPathsImpl(virQEMUDriverPtr driver,
> >   }
> >   
> >   
> > +static void
> > +qemuProcessDestroySevPaths(virDomainSevDefPtr sev)
> > +{
> > +    if (!sev)
> > +        return;
> > +
> > +    virFileDeleteTree(sev->configDir);
>
> Just delete the individual files we created, no recursive delete please.

Will do.


> > +    VIR_FREE(sev->configDir);
> > +}
> > +
> >   int
> >   qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver,
> >                                      virDomainObjPtr vm,
> > @@ -5741,6 +5751,83 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver,
> >       return ret;
> >   }
> >   
> > +static int
> > +qemuBuildSevCreateFile(const char *configDir, const char *name,
> > +                       const char *data)
> > +{
> > +    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;
> > +    }
> > +
> > +    VIR_FREE(configFile);
> > +    return 0;
> > +
> > + error:
> > +    VIR_FREE(configFile);
> > +    return -1;
> > +}
> > +
> > +static int
> > +qemuProcessPrepareSevGuestInput(virQEMUDriverPtr driver,
> > +                                virDomainObjPtr vm)
> > +{
> > +    qemuDomainObjPrivatePtr priv = vm->privateData;
> > +    virDomainDefPtr def = vm->def;
> > +    virQEMUCapsPtr qemuCaps = priv->qemuCaps;
> > +    virDomainSevDefPtr sev = def->sev;
> > +    char *configDir = NULL;
> > +    char *domPath = virDomainDefGetShortName(def);
> > +    virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> > +
> > +    if (!sev)
> > +        return 0;
> > +
> > +    VIR_DEBUG("Prepare SEV guest");
> > +
> > +    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV)) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                        _("Domain %s asked for 'sev' launch but "
> > +                          "QEMU does not support SEV feature"), vm->def->name);
> > +        return -1;
> > +    }
> > +
> > +    if (virAsprintf(&configDir, "%s/sev/%s", cfg->configDir, domPath) < 0)
> > +        goto error;
>
> We already have a directory where we put transient things like the QEMU
> monitor socket, pid file, etc. This is defineed by 'priv->libDir', so
> just use that directory and don't create extra sub-dirs.

Will use priv->libDir in next rev. thanks


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list