6:04 p.m.
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(a)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);