Re: [libvirt] [PATCH v5 2/2] qemu: Add command line builder and parser for NVRAM.

On 24/04/13 20:17, Osier Yang wrote: > On 19/04/13 16:37, Li Zhang wrote: >> From: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; >> >> This patch is to add command line builder and parser >> for NVRAM device, and add test cases. >> >> Signed-off-by: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; >> --- >> v5 -> v4: >> * Fix one memory leakage suggested by Eric Blake >> * Fix several code style suggested by Eric Blake >> >> src/qemu/qemu_capabilities.c | 3 ++ >> src/qemu/qemu_capabilities.h | 2 + >> src/qemu/qemu_command.c | 88 >> ++++++++++++++++++++++++++++++++++++++++++-- >> tests/qemuargv2xmltest.c | 2 + >> tests/qemuxml2argvtest.c | 1 + >> tests/qemuxml2xmltest.c | 2 + >> 6 files changed, 95 insertions(+), 3 deletions(-) >> >> diff --git a/src/qemu/qemu_capabilities.c >> b/src/qemu/qemu_capabilities.c >> index ef291c0..1d54477 100644 >> --- a/src/qemu/qemu_capabilities.c >> +++ b/src/qemu/qemu_capabilities.c >> @@ -220,6 +220,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, >> "machine-usb-opt", >> "tpm-passthrough", >> "tpm-tis", >> + >> + "nvram", /* 140 */ >> ); >> struct _virQEMUCaps { >> @@ -1347,6 +1349,7 @@ struct virQEMUCapsStringFlags >> virQEMUCapsObjectTypes[] = { >> { "virtio-rng-ccw", QEMU_CAPS_DEVICE_VIRTIO_RNG }, >> { "rng-random", QEMU_CAPS_OBJECT_RNG_RANDOM }, >> { "rng-egd", QEMU_CAPS_OBJECT_RNG_EGD }, >> + { "spapr-nvram", QEMU_CAPS_DEVICE_NVRAM }, >> }; >> static struct virQEMUCapsStringFlags >> virQEMUCapsObjectPropsVirtioBlk[] = { >> diff --git a/src/qemu/qemu_capabilities.h >> b/src/qemu/qemu_capabilities.h >> index 4e76799..85f47c4 100644 >> --- a/src/qemu/qemu_capabilities.h >> +++ b/src/qemu/qemu_capabilities.h >> @@ -179,6 +179,8 @@ enum virQEMUCapsFlags { >> QEMU_CAPS_DEVICE_TPM_PASSTHROUGH = 138, /* -tpmdev passthrough */ >> QEMU_CAPS_DEVICE_TPM_TIS = 139, /* -device tpm_tis */ >> + QEMU_CAPS_DEVICE_NVRAM = 140, /*-global >> spapr-nvram.reg=xxxx*/ > > /* -global spapr-nvram.reg=xxxx */ > > >> + >> QEMU_CAPS_LAST, /* this must always be the >> last item */ >> }; >> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c >> index 009d42d..41b8d78 100644 >> --- a/src/qemu/qemu_command.c >> +++ b/src/qemu/qemu_command.c >> @@ -53,6 +53,10 @@ >> #define VIR_FROM_THIS VIR_FROM_QEMU >> +#define VIO_ADDR_NET 0x1000ul >> +#define VIO_ADDR_SCSI 0x2000ul >> +#define VIO_ADDR_SERIAL 0x30000000ul >> +#define VIO_ADDR_NVRAM 0x3000ul > > This is much better. > >> VIR_ENUM_DECL(virDomainDiskQEMUBus) >> VIR_ENUM_IMPL(virDomainDiskQEMUBus, VIR_DOMAIN_DISK_BUS_LAST, >> @@ -1148,7 +1152,7 @@ int >> qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def, >> STREQ(def->nets[i]->model, "spapr-vlan")) >> def->nets[i]->info.type = >> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO; >> if (qemuAssignSpaprVIOAddress(def, &def->nets[i]->info, >> - 0x1000ul) < 0) >> + VIO_ADDR_NET) < 0) >> goto cleanup; >> } >> @@ -1163,7 +1167,7 @@ int >> qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def, >> def->controllers[i]->type == >> VIR_DOMAIN_CONTROLLER_TYPE_SCSI) >> def->controllers[i]->info.type = >> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO; >> if (qemuAssignSpaprVIOAddress(def, >> &def->controllers[i]->info, >> - 0x2000ul) < 0) >> + VIO_ADDR_SCSI) < 0) >> goto cleanup; >> } >> @@ -1173,7 +1177,16 @@ int >> qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def, >> STREQ(def->os.machine, "pseries")) >> def->serials[i]->info.type = >> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO; >> if (qemuAssignSpaprVIOAddress(def, &def->serials[i]->info, >> - 0x30000000ul) < 0) >> + VIO_ADDR_SERIAL) < 0) >> + goto cleanup; >> + } >> + >> + if (def->nvram) { >> + if (def->os.arch == VIR_ARCH_PPC64 && >> + STREQ(def->os.machine, "pseries")) >> + def->nvram->info.type = >> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO; >> + if (qemuAssignSpaprVIOAddress(def, &def->nvram->info, >> + VIO_ADDR_NVRAM) < 0) >> goto cleanup; >> } >> @@ -3969,6 +3982,32 @@ error: >> return NULL; >> } >> +static char * >> +qemuBuildNVRAMDevStr(virDomainNVRAMDefPtr dev) >> +{ >> + virBuffer buf = VIR_BUFFER_INITIALIZER; >> + >> + if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO && >> + dev->info.addr.spaprvio.has_reg) { >> + virBufferAsprintf(&buf, "spapr-nvram.reg=0x%llx", >> + dev->info.addr.spaprvio.reg); >> + } else { >> + virReportError(VIR_ERR_XML_ERROR, >> + "%s", _("NVRAM address only can be spaprvio >> currently.\n")); > > "nvram address type must be 'spaprvio'" > > No need for the ".\n". > >> + goto error; >> + } >> + >> + if (virBufferError(&buf)) { >> + virReportOOMError(); >> + goto error; >> + } >> + >> + return virBufferContentAndReset(&buf); >> + >> +error: >> + virBufferFreeAndReset(&buf); >> + return NULL; >> +} >> char * >> qemuBuildUSBInputDevStr(virDomainInputDefPtr dev, >> @@ -7776,6 +7815,30 @@ qemuBuildCommandLine(virConnectPtr conn, >> goto error; >> } >> + if (def->nvram) { >> + if (def->os.arch == VIR_ARCH_PPC64 && >> + STREQ(def->os.machine, "pseries")) { >> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_NVRAM)) { >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, >> + "%s", _("NVRAM device is not >> available " > > s/NVRAM/nvram/, to be consistent with the tag name in XML. > >> + " with this QEMU binary")); > > s/ with/with/ > >> + goto error; >> + } >> + >> + char *optstr; >> + virCommandAddArg(cmd, "-global"); >> + optstr = qemuBuildNVRAMDevStr(def->nvram); >> + if (!optstr) >> + goto error; >> + if (optstr) >> + virCommandAddArg(cmd, optstr); >> + VIR_FREE(optstr); >> + } else { >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >> + _("NVRAM device is only supported for >> PPC64")); > > s/NVRAM/nvram/, > >> + goto error; >> + } >> + } >> if (snapshot) >> virCommandAddArgList(cmd, "-loadvm", snapshot->def->name, >> NULL); >> @@ -9884,6 +9947,25 @@ virDomainDefPtr >> qemuParseCommandLine(virCapsPtr qemuCaps, >> goto error; >> } >> + } else if (STREQ(arg, "-global") && >> + STRPREFIX(progargv[i + 1], "spapr-nvram.reg=")) { >> + > useless blank line. > >> + WANT_VALUE(); >> + >> + if (VIR_ALLOC(def->nvram) < 0) >> + goto no_memory; >> + >> + def->nvram->info.type = >> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO; >> + def->nvram->info.addr.spaprvio.has_reg = true; >> + >> + val += strlen("spapr-nvram.reg="); >> + if (virStrToLong_ull(val, NULL, 16, >> + &def->nvram->info.addr.spaprvio.reg) < 0) { >> + virReportError(VIR_ERR_INTERNAL_ERROR, >> + _("cannot parse nvram's address:" >> + "'%s'"), val); >> + goto error; >> + } >> } else if (STREQ(arg, "-S")) { >> /* ignore, always added by libvirt */ >> } else { >> diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c >> index ee6c7a9..9f1bb24 100644 >> --- a/tests/qemuargv2xmltest.c >> +++ b/tests/qemuargv2xmltest.c >> @@ -244,6 +244,8 @@ mymain(void) >> DO_TEST("hyperv"); >> + DO_TEST("pseries-nvram"); >> + >> DO_TEST_FULL("restore-v1", 0, "stdio"); >> DO_TEST_FULL("restore-v2", 0, "stdio"); >> DO_TEST_FULL("restore-v2", 0, "exec:cat"); >> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c >> index 4bf13f0..1c21a63 100644 >> --- a/tests/qemuxml2argvtest.c >> +++ b/tests/qemuxml2argvtest.c >> @@ -907,6 +907,7 @@ mymain(void) >> QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, >> QEMU_CAPS_NODEFCONFIG); >> DO_TEST_ERROR("pseries-vio-address-clash", QEMU_CAPS_DRIVE, >> QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, >> QEMU_CAPS_NODEFCONFIG); >> + DO_TEST("pseries-nvram", QEMU_CAPS_DEVICE_NVRAM); >> DO_TEST("disk-ide-drive-split", >> QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, >> QEMU_CAPS_IDE_CD); >> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c >> index 7434190..1d10bf2 100644 >> --- a/tests/qemuxml2xmltest.c >> +++ b/tests/qemuxml2xmltest.c >> @@ -259,6 +259,8 @@ mymain(void) >> DO_TEST("virtio-rng-random"); >> DO_TEST("virtio-rng-egd"); >> + DO_TEST("pseries-nvram"); >> + >> /* These tests generate different XML */ >> DO_TEST_DIFFERENT("balloon-device-auto"); >> DO_TEST_DIFFERENT("channel-virtio-auto"); > > ACK with the nits fixed. > I intended to push this with the attached diff squashed in. But it fails with test files missed:

.............................I/O warning : failed to load external entity "/home/osier/work/git/libvirt/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml" !.......... 280 . 281 FAIL FAIL: qemuxml2argvtest TEST: qemuxml2xmltest ........................................ 40 ........................................ 80 ..................................../home/osier/work/git/libvirt/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml: failed to open: No such file or directory /home/osier/work/git/libvirt/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml: failed to open: No such file or directory !... 120 ........... 131 FAIL FAIL: qemuxml2xmltest TEST: qemuxmlnstest ....... 7 OK PASS: qemuxmlnstest TEST: qemuargv2xmltest ........................................ 40 ...................................../home/osier/work/git/libvirt/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.args: failed to open: No such file or directory