On 2/4/2025 3:33 PM, Peter Krempa wrote:
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@xxxxxxxxxxxxx>
Signed-off-by: Praveen K Paladugu <praveenkpaladugu@xxxxxxxxx>
---
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.
Agreed. I will fix VIR_ERR_INTERNAL_ERROR Usage in a future patch.
+ _("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.
Thanks for your feedback Peter. I addressed all your comments in v2.
--
Regards,
Praveen K Paladugu
