Re: [PATCH] ch: Enable SEV SNP support

On Tue, Feb 04, 2025 at 14:36:48 -0600, Praveen K Paladugu wrote: > Enable SEV-SNP support for ch guests. > > Co-Authored-by: Smit Gardhariya <sgardhariya(a)microsoft.com&gt; > Signed-off-by: Praveen K Paladugu <praveenkpaladugu(a)gmail.com&gt; > --- > src/ch/ch_monitor.c | 74 +++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 62 insertions(+), 12 deletions(-) Disclaimer: I never paid attention how the CH driver works so this is not a proper review. I just have a suggestion for simpler JSON object construction [1]. > diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c > index bedcde2dde..1d9e45219e 100644 > --- a/src/ch/ch_monitor.c > +++ b/src/ch/ch_monitor.c > @@ -130,29 +130,60 @@ static int > virCHMonitorBuildPayloadJson(virJSONValue *content, virDomainDef *vmdef) > { > g_autoptr(virJSONValue) payload = virJSONValueNewObject(); > - > + g_autofree unsigned char *tmp = NULL; > + size_t len; > + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; > + g_autofree char *host_data = NULL; > + size_t host_data_len = 32; This is used as a constant so declare it as such ... > > if (vmdef->os.kernel == NULL) { > virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > - _("Kernel image path in this domain is not defined")); > + _("Kernel image path in this domain is not defined. With sev_snp=on, pass an igvm path")); > return -1; > - } else { > - if (virJSONValueObjectAppendString(payload, "kernel", vmdef->os.kernel) < 0) > - return -1; > } > > - if (vmdef->os.cmdline) { > - if (virJSONValueObjectAppendString(payload, "cmdline", vmdef->os.cmdline) < 0) > + if (vmdef->sec && > + vmdef->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV_SNP) { > + if (virJSONValueObjectAppendString(payload, "igvm", vmdef->os.kernel) < 0) > return -1; > - } > - > - if (vmdef->os.initrd != NULL) { > - if (virJSONValueObjectAppendString(payload, "initramfs", vmdef->os.initrd) < 0) > + if (vmdef->sec->data.sev_snp.host_data) { > + /* Libvirt provided host_data is base64 encoded and cloud-hypervisor > + requires host_data as hex encoded. Base64 decode and hex encode > + before sending to cloud-hypervisor.*/ > + tmp = g_base64_decode(vmdef->sec->data.sev_snp.host_data, &len); > + if (len != host_data_len) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", Is this really an internal error? Doesn't this value come from the user? The code seems to abuse VIR_ERR_INTERNAL_ERROR for stuff that is clearly a misconfiguration also in other places in the condext.

> + _("Invalid host_data provdied. Expected 32 bytes")); ... and also use it in the error message. > + return -1; > + } > + while (len > 0) { > + virBufferAsprintf(&buf, "%02x", tmp[host_data_len-len]); > + len--; > + } > + host_data = virBufferContentAndReset(&buf); > + if (virJSONValueObjectAppendString(payload, "host_data", > + host_data) < 0) > + return -1; > + } > + } else { > + if (virJSONValueObjectAppendString(payload, "kernel", > + vmdef->os.kernel) < 0) > return -1; > + if (vmdef->os.cmdline) { > + if (virJSONValueObjectAppendString(payload, "cmdline", > + vmdef->os.cmdline) < 0) > + return -1; > + } > + > + if (vmdef->os.initrd != NULL) { > + if (virJSONValueObjectAppendString(payload, "initramfs", > + vmdef->os.initrd) < 0) > + return -1; > + } [1] So in other places where we construct multi-field JSON objects we tend to use virJSONValueObjectAdd, which would repace all of the else section by: if (virJSONValueObjectAdd(&payload, "s:kernel", vmdef->os.kernel, "S:cmdline", vmdef->os.cmdline, "S:initramfs", vmdef->os.initrd, NULL) < 0) return -1; The 's' conversion signifies a mandatory string, a 'S' conversion an optional string (omitted when NULL). For other coversions look for the docs at virJSONValueObjectAddVArgs. The first argument, if it is non-NULL is ammended, otherwise it's created. The above is much shorter and also avoids the broken code alignment of the snippet above and also inconsistent style of NULL-checks.