On Fri, Apr 08, 2022 at 10:48:44 -0700, Rohit Kumar wrote:

Currently, libvirt allows only local filepaths to specify a NVRAM disk. Since, VMs can migrate across hosts, so making

Note that this is not strictly needed for migration to work. In fact qemu migratest he contents of the nvram inside the migration stream and flushes it out on the destination, thus migration will work as expected even when the storage is backed locally.

it to virStorageSource type would help in uninturrupted access NVRAM disks over network.

Thus this statement is not accurate. I understand though that this gives you the flexibility to start the VM on any host without having to worry about where to get the latest nvram image.

Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com> Signed-off-by: Florian Schmidt <flosch@nutanix.com> Signed-off-by: Rohit Kumar <rohit.kumar3@nutanix.com> --- src/conf/domain_conf.c | 13 ++++++++++--- src/conf/domain_conf.h | 2 +- src/qemu/qemu_cgroup.c | 3 ++- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain.c | 14 ++++++++------ src/qemu/qemu_driver.c | 5 +++-- src/qemu/qemu_firmware.c | 17 ++++++++++++----- src/qemu/qemu_namespace.c | 5 +++-- src/qemu/qemu_process.c | 5 +++-- src/security/security_dac.c | 6 ++++-- src/security/security_selinux.c | 6 ++++-- src/security/virt-aa-helper.c | 5 +++-- src/vbox/vbox_common.c | 2 +- 13 files changed, 55 insertions(+), 30 deletions(-)

[...]

@@ -18266,7 +18266,11 @@ virDomainDefParseBootLoaderOptions(virDomainDef *def, fwAutoSelect) < 0) return -1;

- def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt); + if (virXPathNode("./os/nvram[1]", ctxt)) { + def->os.loader->nvram = g_new0(virStorageSource, 1);

[1] This is wrong. virStorageSource is an virObject instance so allocating it like this doesn't initialize the virObject part. You must use virStorageSourceNew exclusively to do this.

+ def->os.loader->nvram->path = virXPathString("string(./os/nvram[1])", ctxt); + def->os.loader->nvram->type = VIR_STORAGE_TYPE_FILE; + } if (!fwAutoSelect) def->os.loader->nvramTemplate = virXPathString("string(./os/nvram[1]/@template)", ctxt);

[...]

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index aa0c927578..d46c9ff36a 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -581,7 +581,8 @@ qemuSetupFirmwareCgroup(virDomainObj *vm) return -1;

if (vm->def->os.loader->nvram && - qemuSetupImagePathCgroup(vm, vm->def->os.loader->nvram, false) < 0) + vm->def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&

[1] This is not future proof, this should rather use 'virStorageSourceIsLocalStorage()' so that it also covers cases when e.g. user deliberately selects a 'block' backed nvram.

+ qemuSetupImagePathCgroup(vm, vm->def->os.loader->nvram->path, false) < 0) return -1;

return 0;

[...]

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a4334de158..70e96cc5a5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4663,8 +4663,12 @@ qemuDomainDefPostParse(virDomainDef *def, }

if (virDomainDefHasOldStyleROUEFI(def) && - !def->os.loader->nvram) - qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram); + (!def->os.loader->nvram || !def->os.loader->nvram->path)) { + if (!def->os.loader->nvram) + def->os.loader->nvram = g_new0(virStorageSource, 1);

See [1]

+ def->os.loader->nvram->type = VIR_STORAGE_TYPE_FILE; + qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram->path); + }

if (qemuDomainDefAddDefaultDevices(driver, def, qemuCaps) < 0) return -1;

[...]

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8c0e36e9b2..2b2bd8d20c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6615,8 +6615,9 @@ qemuDomainUndefineFlags(virDomainPtr dom, } }

- if (vm->def->os.loader && vm->def->os.loader->nvram) { - nvram_path = g_strdup(vm->def->os.loader->nvram); + if (vm->def->os.loader && vm->def->os.loader->nvram && + vm->def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE) { + nvram_path = g_strdup(vm->def->os.loader->nvram->path);

Consider [2].

} else if (vm->def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_EFI) { qemuDomainNVRAMPathFormat(cfg, vm->def, &nvram_path); } diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 51223faadf..6556a613a8 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -1192,13 +1192,16 @@ qemuFirmwareEnableFeatures(virQEMUDriver *driver, VIR_FREE(def->os.loader->nvramTemplate); def->os.loader->nvramTemplate = g_strdup(flash->nvram_template.filename);

- if (!def->os.loader->nvram) - qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram); + if (!def->os.loader->nvram) { + def->os.loader->nvram = g_new0(virStorageSource, 1);

See [1].

+ def->os.loader->nvram->type = VIR_STORAGE_TYPE_FILE; + qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram->path); + }

VIR_DEBUG("decided on firmware '%s' template '%s' NVRAM '%s'", def->os.loader->path, def->os.loader->nvramTemplate, - def->os.loader->nvram); + def->os.loader->nvram->path); break;

case QEMU_FIRMWARE_DEVICE_KERNEL: @@ -1364,8 +1367,12 @@ qemuFirmwareFillDomain(virQEMUDriver *driver, * its path in domain XML) but no template for NVRAM was * specified and the varstore doesn't exist ... */ if (!virDomainDefHasOldStyleROUEFI(def) || - def->os.loader->nvramTemplate || - (!reset_nvram && virFileExists(def->os.loader->nvram))) + def->os.loader->nvramTemplate) + return 0; + + if (!reset_nvram && def->os.loader->nvram && + def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE

Consider [2]

+ && virFileExists(def->os.loader->nvram->path))

Also put the && on end of previous line as is common in the surrounding code.

return 0;

/* ... then we want to consult JSON FW descriptors first, diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c index 23681b14a4..18a24635ad 100644 --- a/src/qemu/qemu_namespace.c +++ b/src/qemu/qemu_namespace.c @@ -572,8 +572,9 @@ qemuDomainSetupLoader(virDomainObj *vm, case VIR_DOMAIN_LOADER_TYPE_PFLASH: *paths = g_slist_prepend(*paths, g_strdup(loader->path));

- if (loader->nvram) - *paths = g_slist_prepend(*paths, g_strdup(loader->nvram)); + if (loader->nvram && + loader->nvram->type == VIR_STORAGE_TYPE_FILE)

[2]

+ *paths = g_slist_prepend(*paths, g_strdup(loader->nvram->path)); break;

case VIR_DOMAIN_LOADER_TYPE_NONE: diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2e11d24be2..e53a26c6fd 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4472,7 +4472,8 @@ qemuPrepareNVRAM(virQEMUDriver *driver, struct qemuPrepareNVRAMHelperData data;

if (!loader || !loader->nvram || - (virFileExists(loader->nvram) && !reset_nvram)) + loader->nvram->type != VIR_STORAGE_TYPE_FILE ||

[2]

+ (virFileExists(loader->nvram->path) && !reset_nvram)) return 0;

master_nvram_path = loader->nvramTemplate;

[...]

diff --git a/src/security/security_dac.c b/src/security/security_dac.c index e9e316551e..66c36c57a3 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1971,7 +1971,8 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr, }

if (def->os.loader && def->os.loader->nvram && - virSecurityDACRestoreFileLabel(mgr, def->os.loader->nvram) < 0) + def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&

[2]

+ virSecurityDACRestoreFileLabel(mgr, def->os.loader->nvram->path) < 0) rc = -1;

if (def->os.kernel && @@ -2182,8 +2183,9 @@ virSecurityDACSetAllLabel(virSecurityManager *mgr, }

if (def->os.loader && def->os.loader->nvram && + def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&

[2]

virSecurityDACSetOwnership(mgr, NULL, - def->os.loader->nvram, + def->os.loader->nvram->path, user, group, true) < 0) return -1;

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 6f02baf2ce..1b6b67e8c7 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2806,7 +2806,8 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManager *mgr, }

if (def->os.loader && def->os.loader->nvram && - virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram, true) < 0) + def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&

[2]

+ virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram->path, true) < 0) rc = -1;

if (def->os.kernel && @@ -3212,8 +3213,9 @@ virSecuritySELinuxSetAllLabel(virSecurityManager *mgr, /* This is different than kernel or initrd. The nvram store * is really a disk, qemu can read and write to it. */ if (def->os.loader && def->os.loader->nvram && + def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&

[2]

secdef && secdef->imagelabel && - virSecuritySELinuxSetFilecon(mgr, def->os.loader->nvram, + virSecuritySELinuxSetFilecon(mgr, def->os.loader->nvram->path, secdef->imagelabel, true) < 0) return -1;

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 1f1cce8b3d..d4eb8b08c9 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1006,8 +1006,9 @@ get_files(vahControl * ctl) if (vah_add_file(&buf, ctl->def->os.loader->path, "rk") != 0) goto cleanup;

- if (ctl->def->os.loader && ctl->def->os.loader->nvram) - if (vah_add_file(&buf, ctl->def->os.loader->nvram, "rwk") != 0) + if (ctl->def->os.loader && ctl->def->os.loader->nvram && + ctl->def->os.loader->nvram->type = VIR_STORAGE_TYPE_FILE)

[2]

+ if (vah_add_file(&buf, ctl->def->os.loader->nvram->path, "rwk") != 0) goto cleanup;

for (i = 0; i < ctl->def->ngraphics; i++) { diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 34e555644c..214d5e84b8 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -992,7 +992,7 @@ vboxSetBootDeviceOrder(virDomainDef *def, struct _vboxDriver *data, VIR_DEBUG("def->os.loader->path %s", def->os.loader->path); VIR_DEBUG("def->os.loader->readonly %d", def->os.loader->readonly); VIR_DEBUG("def->os.loader->type %d", def->os.loader->type); - VIR_DEBUG("def->os.loader->nvram %s", def->os.loader->nvram); + VIR_DEBUG("def->os.loader->nvram->path %s", def->os.loader->nvram->path);

Is 'def->os.loader->nvram' guaranteed to be allocated here? You are dereferencing it unconditionally. Previously the code was technically still wrong, but luckily 'printf' is tollerating NULL strings.

} VIR_DEBUG("def->os.bootloader %s", def->os.bootloader); VIR_DEBUG("def->os.bootloaderArgs %s", def->os.bootloaderArgs); -- 2.25.1

Thanks for reviewing the patches ! On 20/04/22 6:18 pm, Peter Krempa wrote:

On Fri, Apr 08, 2022 at 10:48:44 -0700, Rohit Kumar wrote:

...

Currently, libvirt allows only local filepaths to specify a NVRAM disk. Since, VMs can migrate across hosts, so making Note that this is not strictly needed for migration to work. In fact qemu migratest he contents of the nvram inside the migration stream and flushes it out on the destination, thus migration will work as expected even when the storage is backed locally. Right, this does not help with migration. This will provide us flexibilty to access NVRAM disk over network, just like libvirt support disks over network. I will update the commit message.

...

it to virStorageSource type would help in uninturrupted access NVRAM disks over network. Thus this statement is not accurate.

I understand though that this gives you the flexibility to start the VM on any host without having to worry about where to get the latest nvram image. Yes, right. I will update it in the next patch.

...

Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com> Signed-off-by: Florian Schmidt <flosch@nutanix.com> Signed-off-by: Rohit Kumar <rohit.kumar3@nutanix.com> --- src/conf/domain_conf.c | 13 ++++++++++--- src/conf/domain_conf.h | 2 +- src/qemu/qemu_cgroup.c | 3 ++- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain.c | 14 ++++++++------ src/qemu/qemu_driver.c | 5 +++-- src/qemu/qemu_firmware.c | 17 ++++++++++++----- src/qemu/qemu_namespace.c | 5 +++-- src/qemu/qemu_process.c | 5 +++-- src/security/security_dac.c | 6 ++++-- src/security/security_selinux.c | 6 ++++-- src/security/virt-aa-helper.c | 5 +++-- src/vbox/vbox_common.c | 2 +- 13 files changed, 55 insertions(+), 30 deletions(-) [...]

...

@@ -18266,7 +18266,11 @@ virDomainDefParseBootLoaderOptions(virDomainDef *def, fwAutoSelect) < 0) return -1;

- def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt); + if (virXPathNode("./os/nvram[1]", ctxt)) { + def->os.loader->nvram = g_new0(virStorageSource, 1); [1]

This is wrong. virStorageSource is an virObject instance so allocating it like this doesn't initialize the virObject part.

You must use virStorageSourceNew exclusively to do this. Ack. Thanks!

...

+ def->os.loader->nvram->path = virXPathString("string(./os/nvram[1])", ctxt); + def->os.loader->nvram->type = VIR_STORAGE_TYPE_FILE; + } if (!fwAutoSelect) def->os.loader->nvramTemplate = virXPathString("string(./os/nvram[1]/@template)", ctxt);

[...]

...

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index aa0c927578..d46c9ff36a 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -581,7 +581,8 @@ qemuSetupFirmwareCgroup(virDomainObj *vm) return -1;

if (vm->def->os.loader->nvram && - qemuSetupImagePathCgroup(vm, vm->def->os.loader->nvram, false) < 0) + vm->def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE && [1]

This is not future proof, this should rather use 'virStorageSourceIsLocalStorage()' so that it also covers cases when e.g. user deliberately selects a 'block' backed nvram. Sure, thanks! I will update it.

...

+ qemuSetupImagePathCgroup(vm, vm->def->os.loader->nvram->path, false) < 0) return -1;

return 0; [...]

...

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a4334de158..70e96cc5a5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4663,8 +4663,12 @@ qemuDomainDefPostParse(virDomainDef *def, }

if (virDomainDefHasOldStyleROUEFI(def) && - !def->os.loader->nvram) - qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram); + (!def->os.loader->nvram || !def->os.loader->nvram->path)) { + if (!def->os.loader->nvram) + def->os.loader->nvram = g_new0(virStorageSource, 1); See [1] Ack.

...

+ def->os.loader->nvram->type = VIR_STORAGE_TYPE_FILE; + qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram->path); + }

if (qemuDomainDefAddDefaultDevices(driver, def, qemuCaps) < 0) return -1; [...]

...

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8c0e36e9b2..2b2bd8d20c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6615,8 +6615,9 @@ qemuDomainUndefineFlags(virDomainPtr dom, } }

- if (vm->def->os.loader && vm->def->os.loader->nvram) { - nvram_path = g_strdup(vm->def->os.loader->nvram); + if (vm->def->os.loader && vm->def->os.loader->nvram && + vm->def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE) { + nvram_path = g_strdup(vm->def->os.loader->nvram->path); Consider [2]. Ack. } else if (vm->def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_EFI) { qemuDomainNVRAMPathFormat(cfg, vm->def, &nvram_path); } diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 51223faadf..6556a613a8 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -1192,13 +1192,16 @@ qemuFirmwareEnableFeatures(virQEMUDriver *driver, VIR_FREE(def->os.loader->nvramTemplate); def->os.loader->nvramTemplate = g_strdup(flash->nvram_template.filename);

- if (!def->os.loader->nvram) - qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram); + if (!def->os.loader->nvram) { + def->os.loader->nvram = g_new0(virStorageSource, 1); See [1]. Ack.

...

+ def->os.loader->nvram->type = VIR_STORAGE_TYPE_FILE; + qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram->path); + }

VIR_DEBUG("decided on firmware '%s' template '%s' NVRAM '%s'", def->os.loader->path, def->os.loader->nvramTemplate, - def->os.loader->nvram); + def->os.loader->nvram->path); break;

case QEMU_FIRMWARE_DEVICE_KERNEL: @@ -1364,8 +1367,12 @@ qemuFirmwareFillDomain(virQEMUDriver *driver, * its path in domain XML) but no template for NVRAM was * specified and the varstore doesn't exist ... */ if (!virDomainDefHasOldStyleROUEFI(def) || - def->os.loader->nvramTemplate || - (!reset_nvram && virFileExists(def->os.loader->nvram))) + def->os.loader->nvramTemplate) + return 0; + + if (!reset_nvram && def->os.loader->nvram && + def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE Consider [2] Ack.

...

+ && virFileExists(def->os.loader->nvram->path)) Also put the && on end of previous line as is common in the surrounding code. Ack. Thanks! return 0;

/* ... then we want to consult JSON FW descriptors first, diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c index 23681b14a4..18a24635ad 100644 --- a/src/qemu/qemu_namespace.c +++ b/src/qemu/qemu_namespace.c @@ -572,8 +572,9 @@ qemuDomainSetupLoader(virDomainObj *vm, case VIR_DOMAIN_LOADER_TYPE_PFLASH: *paths = g_slist_prepend(*paths, g_strdup(loader->path));

- if (loader->nvram) - *paths = g_slist_prepend(*paths, g_strdup(loader->nvram)); + if (loader->nvram && + loader->nvram->type == VIR_STORAGE_TYPE_FILE) [2] Ack.

...

+ *paths = g_slist_prepend(*paths, g_strdup(loader->nvram->path)); break;

case VIR_DOMAIN_LOADER_TYPE_NONE: diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2e11d24be2..e53a26c6fd 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4472,7 +4472,8 @@ qemuPrepareNVRAM(virQEMUDriver *driver, struct qemuPrepareNVRAMHelperData data;

if (!loader || !loader->nvram || - (virFileExists(loader->nvram) && !reset_nvram)) + loader->nvram->type != VIR_STORAGE_TYPE_FILE || [2] Ack.

...

+ (virFileExists(loader->nvram->path) && !reset_nvram)) return 0;

master_nvram_path = loader->nvramTemplate; [...]

...

diff --git a/src/security/security_dac.c b/src/security/security_dac.c index e9e316551e..66c36c57a3 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1971,7 +1971,8 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr, }

if (def->os.loader && def->os.loader->nvram && - virSecurityDACRestoreFileLabel(mgr, def->os.loader->nvram) < 0) + def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE && [2] Ack.

...

+ virSecurityDACRestoreFileLabel(mgr, def->os.loader->nvram->path) < 0) rc = -1;

if (def->os.kernel && @@ -2182,8 +2183,9 @@ virSecurityDACSetAllLabel(virSecurityManager *mgr, }

if (def->os.loader && def->os.loader->nvram && + def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE && [2] Ack.

...

virSecurityDACSetOwnership(mgr, NULL, - def->os.loader->nvram, + def->os.loader->nvram->path, user, group, true) < 0) return -1;

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 6f02baf2ce..1b6b67e8c7 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2806,7 +2806,8 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManager *mgr, }

if (def->os.loader && def->os.loader->nvram && - virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram, true) < 0) + def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&

[2] Ack.

...

+ virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram->path, true) < 0) rc = -1;

if (def->os.kernel && @@ -3212,8 +3213,9 @@ virSecuritySELinuxSetAllLabel(virSecurityManager *mgr, /* This is different than kernel or initrd. The nvram store * is really a disk, qemu can read and write to it. */ if (def->os.loader && def->os.loader->nvram && + def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE && [2] Ack.

...

secdef && secdef->imagelabel && - virSecuritySELinuxSetFilecon(mgr, def->os.loader->nvram, + virSecuritySELinuxSetFilecon(mgr, def->os.loader->nvram->path, secdef->imagelabel, true) < 0) return -1;

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 1f1cce8b3d..d4eb8b08c9 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1006,8 +1006,9 @@ get_files(vahControl * ctl) if (vah_add_file(&buf, ctl->def->os.loader->path, "rk") != 0) goto cleanup;

- if (ctl->def->os.loader && ctl->def->os.loader->nvram) - if (vah_add_file(&buf, ctl->def->os.loader->nvram, "rwk") != 0) + if (ctl->def->os.loader && ctl->def->os.loader->nvram && + ctl->def->os.loader->nvram->type = VIR_STORAGE_TYPE_FILE)

[2] Ack.

...

+ if (vah_add_file(&buf, ctl->def->os.loader->nvram->path, "rwk") != 0) goto cleanup;

for (i = 0; i < ctl->def->ngraphics; i++) { diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 34e555644c..214d5e84b8 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -992,7 +992,7 @@ vboxSetBootDeviceOrder(virDomainDef *def, struct _vboxDriver *data, VIR_DEBUG("def->os.loader->path %s", def->os.loader->path); VIR_DEBUG("def->os.loader->readonly %d", def->os.loader->readonly); VIR_DEBUG("def->os.loader->type %d", def->os.loader->type); - VIR_DEBUG("def->os.loader->nvram %s", def->os.loader->nvram); + VIR_DEBUG("def->os.loader->nvram->path %s", def->os.loader->nvram->path); Is 'def->os.loader->nvram' guaranteed to be allocated here? You are dereferencing it unconditionally.

Previously the code was technically still wrong, but luckily 'printf' is tollerating NULL strings. Yes, right. I will add an if condition before accessing nvram->path.  Thanks!

...

} VIR_DEBUG("def->os.bootloader %s", def->os.bootloader); VIR_DEBUG("def->os.bootloaderArgs %s", def->os.bootloaderArgs); -- 2.25.1

On Fri, Apr 08, 2022 at 10:48:45 -0700, Rohit Kumar wrote:

This patch introduces the logic to support remote NVRAM and adds 'type' attribute to nvram element.

Sample XML with new annotation:

<nvram type='network'> <source protocol='iscsi' name='iqn.2013-07.com.example:iscsi-nopool/0'> <host name='example.com' port='6000'/> </source> </nvram>

or

<nvram type='file'> <source file='/var/lib/libvirt/nvram/guest_VARS.fd'/> </nvram>

[1]

Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com> Signed-off-by: Florian Schmidt <flosch@nutanix.com> Signed-off-by: Rohit Kumar <rohit.kumar3@nutanix.com> --- src/conf/domain_conf.c | 81 +++++++++++++++++++++++++++++++++------- src/qemu/qemu_firmware.c | 6 +++ 2 files changed, 74 insertions(+), 13 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b83c2f0e6a..bc8c7e0d6f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17866,6 +17866,42 @@ virDomainLoaderDefParseXML(xmlNodePtr node, }

+static int +virDomainNvramDefParseXML(virStorageSource *nvram, + xmlXPathContextPtr ctxt, + virDomainXMLOption *opt, + unsigned int flags) +{ + g_autofree char *nvramType = NULL; + xmlNodePtr source; + + nvramType = virXPathString("string(./os/nvram/@type)", ctxt); + + /* if nvramType==NULL read old-style, else + * if there's a type, read new style */ + if (!nvramType) { + nvram->type = VIR_STORAGE_TYPE_FILE; + nvram->path = virXPathString("string(./os/nvram[1])", ctxt); + return 0;

Note that this code path doesn't remember that the old-style config was used.

+ } else {

'source' can be declared here.

+ if ((nvram->type = virStorageTypeFromString(nvramType)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown disk type '%s'"), nvramType); + return -1; + } + source = virXPathNode("./os/nvram/source[1]", ctxt); + if (!source) { + virReportError(VIR_ERR_XML_ERROR, + _("Missing source element with nvram type '%s'"), + nvramType); + return -1; + }

You'll also need to add a form of validation either in a separate commit prior to this commit or into this commit which will reject unsupported source types, such as VIR_STORAGE_TYPE_DIR, or VIR_STORAGE_TYPE_VOLUME (and others) ... even VIR_STORAGE_TYPE_NETWORK at this point.

+ return virDomainStorageSourceParse(source, ctxt, nvram, flags, opt); + } + return 0; +} + + static int virDomainSchedulerParseCommonAttrs(xmlNodePtr node, virProcessSchedPolicy *policy,

[...]

-static void +static int virDomainLoaderDefFormat(virBuffer *buf, - virDomainLoaderDef *loader) + virDomainLoaderDef *loader, + virDomainXMLOption *xmlopt, + unsigned int flags) { g_auto(virBuffer) loaderAttrBuf = VIR_BUFFER_INITIALIZER; g_auto(virBuffer) loaderChildBuf = VIR_BUFFER_INITIALIZER; @@ -26976,10 +27020,20 @@ virDomainLoaderDefFormat(virBuffer *buf,

virBufferEscapeString(&nvramAttrBuf, " template='%s'", loader->nvramTemplate); if (loader->nvram) { - if (loader->nvram->type == VIR_STORAGE_TYPE_FILE) + if (loader->nvram->type == VIR_STORAGE_TYPE_FILE) {

Since you don't remember that the old-style config was used and you force it for all VIR_STORAGE_TYPE_FILE configs the example [1] you added in the commit message will work but will be reformatted.

virBufferEscapeString(&nvramChildBuf, "%s", loader->nvram->path); + virXMLFormatElementInternal(buf, "nvram", &nvramAttrBuf, &nvramChildBuf, false, false); + } else { + virBufferEscapeString(&nvramAttrBuf, " type='%s'", virStorageTypeToString(loader->nvram->type));

Use virBufferAsprintf as we are not dealing with user-supplied data.

+ if (virDomainDiskSourceFormat(&nvramChildBuf, loader->nvram, "source", 0, + 0, false, flags, true, xmlopt) < 0) { + return -1; + } + virXMLFormatElementInternal(buf, "nvram", &nvramAttrBuf, &nvramChildBuf, false, true);

Use the 'virXMLFormatElement' instead, or add a boolean for the last argument ...

+ } } - virXMLFormatElementInternal(buf, "nvram", &nvramAttrBuf, &nvramChildBuf, false, false);

And use just one invocation.

+ + return 0; }

static void

[...]

diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 6556a613a8..22ad7d42a1 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -1375,6 +1375,12 @@ qemuFirmwareFillDomain(virQEMUDriver *driver, && virFileExists(def->os.loader->nvram->path)) return 0;

+ + if (!reset_nvram && def->os.loader->nvram && + def->os.loader->nvram->type == VIR_STORAGE_TYPE_NETWORK && + def->os.loader->nvram->path) + return 0;

This bit doesn't seem to belong to this patch.

On 09/04/22 8:19 pm, Ani Sinha wrote:

...

Remote store NVRAM feature is being enabled only if it supports 'blockdev' capability.

Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com> Signed-off-by: Florian Schmidt <flosch@nutanix.com> Signed-off-by: Rohit Kumar <rohit.kumar3@nutanix.com> Please add negative unit tests to check the validation along with this

On Fri, Apr 8, 2022 at 11:19 PM Rohit Kumar <rohit.kumar3@nutanix.com> wrote: patch. Prefix the patch with "qemu:". Thanks for pointing this out, Ani. Sure, I will add it the next patch series.

...

--- src/qemu/qemu_validate.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 96f5427678..2a961b1f50 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -611,6 +611,25 @@ qemuValidateDomainDefBoot(const virDomainDef *def) }

+static int +qemuValidateDomainDefNvram(const virDomainDef *def, + virQEMUCaps *qemuCaps) +{ + if (def->os.loader && def->os.loader->nvram) { + if (def->os.loader->nvram->type != VIR_STORAGE_TYPE_FILE && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This Qemu does not support 'blockdev' capability " + "for remote store NVRAM. NVRAM type other than " + "'file' is not supported with this QEMU")); + return -1; + } + } + + return 0; +} + + /** * qemuValidateDefGetVcpuHotplugGranularity: * @def: domain definition @@ -1185,6 +1204,9 @@ qemuValidateDomainDef(const virDomainDef *def, if (qemuValidateDomainDefBoot(def) < 0) return -1;

+ if (qemuValidateDomainDefNvram(def, qemuCaps) < 0) + return -1; + if (qemuValidateDomainVCpuTopology(def, qemuCaps) < 0) return -1;

-- 2.25.1

On Thu, Apr 21, 2022 at 16:52:54 +0200, Peter Krempa wrote:

On Fri, Apr 08, 2022 at 10:48:46 -0700, Rohit Kumar wrote:

...

Remote store NVRAM feature is being enabled only if it supports 'blockdev' capability.

Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com> Signed-off-by: Florian Schmidt <flosch@nutanix.com> Signed-off-by: Rohit Kumar <rohit.kumar3@nutanix.com> --- src/qemu/qemu_validate.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 96f5427678..2a961b1f50 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -611,6 +611,25 @@ qemuValidateDomainDefBoot(const virDomainDef *def) }

As noted in 2/8, this will need to be moved earlier.

...

+static int +qemuValidateDomainDefNvram(const virDomainDef *def, + virQEMUCaps *qemuCaps) +{

Return early if there's nothing to validate to decrease indentation level.

...

+ if (def->os.loader && def->os.loader->nvram) { + if (def->os.loader->nvram->type != VIR_STORAGE_TYPE_FILE && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This Qemu does not support 'blockdev' capability " + "for remote store NVRAM. NVRAM type other than " + "'file' is not supported with this QEMU"));

"Remote NVRAM is not supported by this qemu".

...

+ return -1; + } + }

Also you need to reject all the other unsupported configs here.

Additionally you'll also need to call the function that validates a storage source definition too at this point.

On Tue, Apr 26, 2022 at 16:16:11 +0530, Rohit Kumar wrote:

On 21/04/22 8:36 pm, Peter Krempa wrote:

...

On Thu, Apr 21, 2022 at 16:52:54 +0200, Peter Krempa wrote:

...

On Fri, Apr 08, 2022 at 10:48:46 -0700, Rohit Kumar wrote:

...

Remote store NVRAM feature is being enabled only if it supports 'blockdev' capability.

Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com> Signed-off-by: Florian Schmidt <flosch@nutanix.com> Signed-off-by: Rohit Kumar <rohit.kumar3@nutanix.com> --- src/qemu/qemu_validate.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 96f5427678..2a961b1f50 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -611,6 +611,25 @@ qemuValidateDomainDefBoot(const virDomainDef *def) } As noted in 2/8, this will need to be moved earlier. Ack.

If you don't have any additional points to raise or a discussion to start, there's no need to acknowledge every point I make in a review. For anything you acknowledge, simply do what was requested in the next version. Here I had to spend time opening a message where simply isn't anything for me to respond to.

On Fri, Apr 08, 2022 at 10:48:47 -0700, Rohit Kumar wrote:

Moving diskSourceNetwork and diskSourceFile's Source definition under 'define' element, so that it will be easier to reuse it at multiple places.

Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com> Signed-off-by: Florian Schmidt <flosch@nutanix.com> Signed-off-by: Rohit Kumar <rohit.kumar3@nutanix.com> --- src/conf/schemas/domaincommon.rng | 60 +++++++++++++++++-------------- 1 file changed, 34 insertions(+), 26 deletions(-)

diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index c0c14fe558..58eb9670d4 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng

While this commit is correct I don't think you will benefit too much with having it.

@@ -1724,6 +1724,31 @@ </choice> </define>

+ <define name="diskSourceFileElement"> + <element name="source"> + <interleave> + <optional> + <attribute name="file"> + <choice> + <ref name="absFilePath"/> + <ref name="vmwarePath"/>

This IMO won't make sense for nvram.

+ </choice> + </attribute> + </optional> + <ref name="diskSourceCommon"/> + <optional> + <ref name="storageStartupPolicy"/> + </optional>

Startup policy definitely makes no sense for the nvram.

+ <optional> + <ref name="encryption"/>

Due to the way how the 'pflash1' storage source is initialized encryption is NOT yet supported. You'll also need to reject it after parsing.

+ </optional> + <zeroOrMore> + <ref name="devSeclabel"/>

I don't think the code is wired up to actually handle seclabels even if the parser parses them.

+ </zeroOrMore> + </interleave> + </element> + </define> + <define name="diskSourceFile"> <optional> <attribute name="type">

On Fri, Apr 08, 2022 at 10:48:48 -0700, Rohit Kumar wrote:

This patch updates NVRAM element schema to support network backed NVRAM. It introduces 'type' attribute to NVRAM element.

Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com> Signed-off-by: Florian Schmidt <flosch@nutanix.com> Signed-off-by: Rohit Kumar <rohit.kumar3@nutanix.com> --- src/conf/schemas/domaincommon.rng | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index 58eb9670d4..cd61d00b33 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -333,7 +333,25 @@ </attribute> </optional> <optional> - <ref name="absFilePath"/> + <attribute name="type"> + <choice> + <value>file</value> + <value>network</value>

So this schema would e.g. allow a type='network' nvram ...

+ </choice> + </attribute> + </optional> + <optional> + <choice> + <group> + <ref name="absFilePath"/> + </group> + <group> + <ref name="diskSourceFileElement"/>

... with a 'file' source. These will need to be re-grouped differently so that only the corresponding attribute value is allowed with appropriate contents.

+ </group> + <group> + <ref name="diskSourceNetworkElement"/> + </group> + </choice> </optional> </element> </optional> -- 2.25.1

On Tue, Apr 26, 2022 at 16:17:05 +0530, Rohit Kumar wrote:

On 21/04/22 8:31 pm, Peter Krempa wrote:

...

On Fri, Apr 08, 2022 at 10:48:48 -0700, Rohit Kumar wrote:

...

This patch updates NVRAM element schema to support network backed NVRAM. It introduces 'type' attribute to NVRAM element.

Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com> Signed-off-by: Florian Schmidt <flosch@nutanix.com> Signed-off-by: Rohit Kumar <rohit.kumar3@nutanix.com> --- src/conf/schemas/domaincommon.rng | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index 58eb9670d4..cd61d00b33 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -333,7 +333,25 @@

[...]

...

...

+ </group> + <group> + <ref name="diskSourceFileElement"/> ... with a 'file' source. These will need to be re-grouped differently so that only the corresponding attribute value is allowed with appropriate contents. Thanks for pointing this. I will re-group these. Also, having this grouping in schema is enough ? or do we need to validation for this as well ?

As I've mentioned in my previous reply, libvirt unfortunately does not validate the XML against the schema unless requested by the user, thus you need to reject in the code anything that is parsed but not inteded to be actually supported. This is an unfortunate historic decision we've made which can't be changed as quirks in the schema could lead to rejection of previously working configs.

On Fri, Apr 08, 2022 at 10:48:49 -0700, Rohit Kumar wrote:

Updating the doc as now NVRAM can be remote as well.

Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com> Signed-off-by: Florian Schmidt <flosch@nutanix.com> Signed-off-by: Rohit Kumar <rohit.kumar3@nutanix.com> --- docs/formatdomain.rst | 43 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 8 deletions(-)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 24fbfd8670..53361c7996 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst

[....]

@@ -222,14 +247,16 @@ harddisk, cdrom, network) determining where to obtain/find the boot image. :since:`Since 2.1.0` ``nvram`` Some UEFI firmwares may want to use a non-volatile memory to store some - variables. In the host, this is represented as a file and the absolute path - to the file is stored in this element. Moreover, when the domain is started - up libvirt copies so called master NVRAM store file defined in ``qemu.conf``. - If needed, the ``template`` attribute can be used to per domain override map - of master NVRAM stores from the config file. Note, that for transient domains - if the NVRAM file has been created by libvirt it is left behind and it is - management application's responsibility to save and remove file (if needed to - be persistent). :since:`Since 1.2.8` + variables. They can be present on host or on some network storage.

Missing ':since:' annotation of when that was introduced.

+ In the host, this is represented as a file and the absolute path + to the file is stored in this element. On network storage, + this can be accessed by providing network path.

There's no such thing as 'network path'. You'll need to refer back to the disk configuration section.

Moreover, when the domain + is started up libvirt copies so called master NVRAM store file defined + in ``qemu.conf``. If needed, the ``template`` attribute can be used to per + domain override map of master NVRAM stores from the config file. Note, that + for transient domains if the NVRAM file has been created by libvirt + it is left behind and it is management application's responsibility to + save and remove file (if needed to be persistent). :since:`Since 1.2.8` ``boot`` The ``dev`` attribute takes one of the values "fd", "hd", "cdrom" or "network" and is used to specify the next boot device to consider. The

This can be also theoretically merged into one commit which is adding the schema. We do it like that usually.

On Fri, Apr 08, 2022 at 10:48:50 -0700, Rohit Kumar wrote:

This patch adds unit test for network backed NVRAM

Example: <nvram type='network'> <source protocol='iscsi' name='iqn.2013-07.com.example:iscsi-nopool/0'> <host name='example.com' port='6000'/> </source> </nvram>

Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com> Signed-off-by: Florian Schmidt <flosch@nutanix.com> Signed-off-by: Rohit Kumar <rohit.kumar3@nutanix.com> --- .../bios-nvram-network.x86_64-latest.args | 37 +++++++++++++++++++ tests/qemuxml2argvdata/bios-nvram-network.xml | 25 +++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 63 insertions(+) create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.xml

diff --git a/tests/qemuxml2argvdata/bios-nvram-network.xml b/tests/qemuxml2argvdata/bios-nvram-network.xml new file mode 100644 index 0000000000..ffc590ddfa --- /dev/null +++ b/tests/qemuxml2argvdata/bios-nvram-network.xml @@ -0,0 +1,25 @@ +<domain type='qemu'> + <name>test-bios</name> + <uuid>362d1fc1-df7d-193e-5c18-49a71bd1da66</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <loader readonly='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader> + <nvram type='network'> + <source protocol='iscsi' name='iqn.2013-07.com.example:iscsi-nopool'> + <host name='example.com' port='6000'/> + </source> + </nvram> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index ed41b7a7a2..7700ed6cb1 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1198,6 +1198,7 @@ mymain(void) QEMU_CAPS_ICH9_AHCI, QEMU_CAPS_VIRTIO_SCSI); DO_TEST_CAPS_LATEST("bios-nvram-template"); + DO_TEST_CAPS_LATEST("bios-nvram-network");

/* Make sure all combinations of ACPI and UEFI behave as expected */ DO_TEST_NOCAPS("q35-acpi-uefi");

You'll also need to add qemuxml2xmltest variants of this test and also for the one in 8/8. Do you want to use this with iSCSI or did you pick this just as an example?