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