Re: [libvirt] [PATCH v3 1/3] Add NVRAM device
On 2013年04月11日 17:36, Daniel P. Berrange wrote:
On Wed, Mar 27, 2013 at 01:07:54PM +0800, Li Zhang wrote:
> From: Li Zhang <zhlcindy(a)linux.vnet.ibm.com>
>
> For pSeries guest in QEMU, NVRAM is one kind of spapr-vio device.
> Users are allowed to specify spapr-vio devices'address.
> But NVRAM is not supported in libvirt. So this patch is to
> add NVRAM device to allow users to specify its address.
>
> In QEMU, NVRAM device's address is specified by
> "-global spapr-nvram.reg=xxxxx".
>
> In libvirt, XML file is defined as the following:
>
> <nvram>
> <address type='spapr-vio' reg='0x3000'/>
> </nvram>
>
> Signed-off-by: Li Zhang <zhlcindy(a)linux.vnet.ibm.com>
> ---
> v3 -> v2:
> * Add NVRAM in domaincommon.rng and formatdomain.xml.in suggested by Daniel
P.Berrange
> * Add NVRAM test cases suggested by Daniel P.Berrange
> * Remove nvram allocation when it is not specified suggested by Daniel P.Berrange
> * Add several error reports suggested by Daniel P.Berrange
>
> v2 -> v1:
> * Add NVRAM parameters parsing in qemuParseCommandLine
>
> src/conf/domain_conf.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++
> src/conf/domain_conf.h | 10 +++++++
> src/qemu/qemu_command.c | 67 +++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_command.h | 2 ++
> 4 files changed, 155 insertions(+)
The QEMU pieces should be separate from the parser pieces. Basically instead
of the 3 patches you have here you should have 2 patches with the following
files in each one:
1. src/conf/domain_conf.*, docs/schemas/* and
docs/formatdomain.html.in
2. src/qemu/* and tests/qemuxml2argv*
OK, got it.
> @@ -13854,6 +13911,21 @@
virDomainMemballoonDefFormat(virBufferPtr buf,
> }
>
> static int
> +virDomainNVRAMDefFormat(virBufferPtr buf,
> + virDomainNVRAMDefPtr def,
> + unsigned int flags)
> +{
> + virBufferAsprintf(buf, " <nvram>\n");
> + if (virDomainDeviceInfoIsSet(&def->info, flags))
> + if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
> + return -1;
> +
> + virBufferAddLit(buf, " </nvram>\n");
> +
> + return 0;
There's inconsistent indentation here
I will correct it.
> qemuBuildUSBInputDevStr(virDomainInputDefPtr dev,
> @@ -7492,6 +7527,21 @@ qemuBuildCommandLine(virConnectPtr conn,
> goto error;
> }
>
> + if (def->nvram) {
> + if (def->os.arch == VIR_ARCH_PPC64 &&
> + STREQ(def->os.machine, "pseries")) {
> + 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 not supported"));
Missing a 'goto error' here
Ok, I will add it.
> @@ -9584,6 +9634,23 @@ virDomainDefPtr
qemuParseCommandLine(virCapsPtr qemuCaps,
> goto error;
> }
>
> + } else if (STREQ(arg, "-global") &&
> + STRPREFIX(progargv[i + 1], "spapr-nvram.reg=")) {
> +
> + WANT_VALUE();
> +
> + if (VIR_ALLOC(def->nvram) < 0)
> + goto no_memory;
> +
> + val += strlen("spapr-nvram.reg=");
> + if (virStrToLong_ull(val, NULL, 16,
> + &def->nvram->info.addr.spaprvio.reg) <
0) {
Don't you need to set the address type too.
Ah, yes. I will add it.
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("invalid value for spapr-nvram.reg :"
> + "'%s'"), val);
> + goto error;
> + }
> +
> } else if (STREQ(arg, "-S")) {
> /* ignore, always added by libvirt */
> } else {
Daniel