Re: [libvirt] [PATCH v5 1/2] Add NVRAM device

On 19/04/13 16:37, Li Zhang wrote: > From: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; > > 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&gt; > --- > v5 -> v4: > * Add NVRAM capability suggested by Osier Yang. > * Define macros for VIO device address. > * Define qemuBuildNVRAMDevStr as static func. > * Correct several error messages. > * Add xml2xml test case > > v4 -> v3: > * Seperate NVRAM definition from qemu command line parser. > > 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 > > docs/formatdomain.html.in | 35 +++++++++++++++++++ > docs/schemas/domaincommon.rng | 10 ++++++ > src/conf/domain_conf.c | 81 > +++++++++++++++++++++++++++++++++++++++++++ > src/conf/domain_conf.h | 10 ++++++ > 4 files changed, 136 insertions(+) > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 0cc56d9..4a7a66f 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -4502,6 +4502,41 @@ qemu-kvm -net nic,model=? /dev/null > </dd> > </dl> > + <h4><a name="elementsNVRAM">NVRAM device</a></h4> > + <p> > + One NVRAM device is always added to pSeries guests on PPC64. > + And on PPC64, NVRAM devices' address type are VIO which s/are/is/, s/VIO which/VIO, which/, > + allows users to change.<code>nvram</code> element in XML file s/change\./change\. /, > + is provided to specify its address. > + Currently, libvirt only considers configuration for pSeries > guests. > + </p> How about rewords the documents like: nvram device is always added to pSeries guest on PPC64, and its address is allowed to be changed. Element <code>nvram</code> is provided to enable the address setting. > + <p> > + Example: usage of NVRAM configuration > + </p> > +<pre> > + ... > + &lt;devices&gt; > + &lt;nvram&gt; > + &lt;address type='spapr-vio' reg='0x3000'/&gt; > + &lt;/nvram&gt; > + &lt;/devices&gt; > + ... > +</pre> > + <dl> > + <dt><code>spapr-vio</code></dt> > + <dd> > + <p> > + VIO device address type, this is only for PPC64. s/this is only for/only valid for/, > + </p> > + </dd> > + <dt><code>reg</code></dt> > + <dd> > + <p> > + Devices' address s/Devices'/Device's/ > + </p> > + </dd> > + </dl> > + > <h3><a name="seclabel">Security label</a></h3> > <p> > diff --git a/docs/schemas/domaincommon.rng > b/docs/schemas/domaincommon.rng > index 3976b82..86ef540 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -2793,6 +2793,13 @@ > </optional> > </element> > </define> > + <define name="nvram"> > + <element name="nvram"> > + <optional> > + <ref name="address"/> > + </optional> > + </element> > + </define> > <define name="memballoon"> > <element name="memballoon"> > <attribute name="model"> > @@ -3280,6 +3287,9 @@ > <optional> > <ref name="memballoon"/> > </optional> > + <optional> > + <ref name="nvram"/> > + </optional> > </interleave> > </element> > </define> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 1643f30..acaec20 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -194,6 +194,7 @@ VIR_ENUM_IMPL(virDomainDevice, > VIR_DOMAIN_DEVICE_LAST, > "smartcard", > "chr", > "memballoon", > + "nvram", > "rng") > VIR_ENUM_IMPL(virDomainDeviceAddress, > VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST, > @@ -1560,6 +1561,16 @@ void > virDomainMemballoonDefFree(virDomainMemballoonDefPtr def) > VIR_FREE(def); > } > +void virDomainNVRAMDefFree(virDomainNVRAMDefPtr def) > +{ > + if (!def) > + return; > + > + virDomainDeviceInfoClear(&def->info); > + > + VIR_FREE(def); > +} > + > void virDomainWatchdogDefFree(virDomainWatchdogDefPtr def) > { > if (!def) > @@ -1743,6 +1754,7 @@ void > virDomainDeviceDefFree(virDomainDeviceDefPtr def) > case VIR_DOMAIN_DEVICE_SMARTCARD: > case VIR_DOMAIN_DEVICE_CHR: > case VIR_DOMAIN_DEVICE_MEMBALLOON: > + case VIR_DOMAIN_DEVICE_NVRAM: Any special reason to not use virDomainNVRAMDefFree here? and same question for other device types. > case VIR_DOMAIN_DEVICE_LAST: > break; > } > @@ -1951,6 +1963,7 @@ void virDomainDefFree(virDomainDefPtr def) > virDomainWatchdogDefFree(def->watchdog); > virDomainMemballoonDefFree(def->memballoon); > + virDomainNVRAMDefFree(def->nvram); > for (i = 0; i < def->nseclabels; i++) > virSecurityLabelDefFree(def->seclabels[i]); > @@ -2519,6 +2532,12 @@ > virDomainDeviceInfoIterateInternal(virDomainDefPtr def, > if (cb(def, &device, &def->rng->info, opaque) < 0) > return -1; > } > + if (def->nvram) { > + device.type = VIR_DOMAIN_DEVICE_NVRAM; > + device.data.nvram = def->nvram; > + if (cb(def, &device, &def->nvram->info, opaque) < 0) > + return -1; > + } > device.type = VIR_DOMAIN_DEVICE_HUB; > for (i = 0; i < def->nhubs ; i++) { > device.data.hub = def->hubs[i]; > @@ -2550,6 +2569,7 @@ > virDomainDeviceInfoIterateInternal(virDomainDefPtr def, > case VIR_DOMAIN_DEVICE_SMARTCARD: > case VIR_DOMAIN_DEVICE_CHR: > case VIR_DOMAIN_DEVICE_MEMBALLOON: > + case VIR_DOMAIN_DEVICE_NVRAM: > case VIR_DOMAIN_DEVICE_LAST: > case VIR_DOMAIN_DEVICE_RNG: > break; > @@ -8130,6 +8150,28 @@ error: > goto cleanup; > } > +static virDomainNVRAMDefPtr > +virDomainNVRAMDefParseXML(const xmlNodePtr node, > + unsigned int flags) > +{ > + virDomainNVRAMDefPtr def; > + > + if (VIR_ALLOC(def) < 0) { > + virReportOOMError(); > + return NULL; > + } > + > + if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) > + goto error; > + > + return def; > + > +error: > + virDomainNVRAMDefFree(def); > + def = NULL; > + return def; "return NULL;" is enough. > +} > + > static virSysinfoDefPtr > virSysinfoParseXML(const xmlNodePtr node, > xmlXPathContextPtr ctxt) > @@ -11304,6 +11346,26 @@ virDomainDefParseXML(xmlDocPtr xml, > } > VIR_FREE(nodes); > + def->nvram = NULL; > + if ((n = virXPathNodeSet("./devices/nvram", ctxt, &nodes)) < 0) { > + goto error; > + } > + > + if (n > 1) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("only a single nvram device is supported")); > + goto error; > + } > + > + if (n > 0) { } else if (n == 1) { > + virDomainNVRAMDefPtr nvram = > + virDomainNVRAMDefParseXML(nodes[0], flags); > + if (!nvram) > + goto error; > + def->nvram = nvram; > + VIR_FREE(nodes); > + } > + > /* analysis of the hub devices */ > if ((n = virXPathNodeSet("./devices/hub", ctxt, &nodes)) < 0) { > goto error; > @@ -14391,6 +14453,21 @@ virDomainMemballoonDefFormat(virBufferPtr buf, > } > static int > +virDomainNVRAMDefFormat(virBufferPtr buf, > + virDomainNVRAMDefPtr def, > + unsigned int flags) > +{ > + virBufferAddLit(buf, " <nvram>\n"); > + if (virDomainDeviceInfoIsSet(&def->info, flags) && > + virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) > + return -1; > + > + virBufferAddLit(buf, " </nvram>\n"); > + > + return 0; > +} > + > +static int > virDomainSysinfoDefFormat(virBufferPtr buf, > virSysinfoDefPtr def) > { > @@ -15720,6 +15797,9 @@ virDomainDefFormatInternal(virDomainDefPtr def, > if (def->rng) > virDomainRNGDefFormat(buf, def->rng, flags); > + if (def->nvram) > + virDomainNVRAMDefFormat(buf, def->nvram, flags); > + > virBufferAddLit(buf, " </devices>\n"); > virBufferAdjustIndent(buf, 2); > @@ -17002,6 +17082,7 @@ virDomainDeviceDefCopy(virDomainDeviceDefPtr > src, > case VIR_DOMAIN_DEVICE_SMARTCARD: > case VIR_DOMAIN_DEVICE_CHR: > case VIR_DOMAIN_DEVICE_MEMBALLOON: > + case VIR_DOMAIN_DEVICE_NVRAM: > case VIR_DOMAIN_DEVICE_LAST: > virReportError(VIR_ERR_INTERNAL_ERROR, > _("Copying definition of '%d' type " > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index f1f01fa..502629a 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -109,6 +109,9 @@ typedef virDomainChrDef *virDomainChrDefPtr; > typedef struct _virDomainMemballoonDef virDomainMemballoonDef; > typedef virDomainMemballoonDef *virDomainMemballoonDefPtr; > +typedef struct _virDomainNVRAMDef virDomainNVRAMDef; > +typedef virDomainNVRAMDef *virDomainNVRAMDefPtr; > + > typedef struct _virDomainSnapshotObj virDomainSnapshotObj; > typedef virDomainSnapshotObj *virDomainSnapshotObjPtr; > @@ -137,6 +140,7 @@ typedef enum { > VIR_DOMAIN_DEVICE_SMARTCARD, > VIR_DOMAIN_DEVICE_CHR, > VIR_DOMAIN_DEVICE_MEMBALLOON, > + VIR_DOMAIN_DEVICE_NVRAM, > VIR_DOMAIN_DEVICE_RNG, > VIR_DOMAIN_DEVICE_LAST > @@ -163,6 +167,7 @@ struct _virDomainDeviceDef { > virDomainSmartcardDefPtr smartcard; > virDomainChrDefPtr chr; > virDomainMemballoonDefPtr memballoon; > + virDomainNVRAMDefPtr nvram; > virDomainRNGDefPtr rng; > } data; > }; > @@ -1469,6 +1474,9 @@ struct _virDomainMemballoonDef { > virDomainDeviceInfo info; > }; > +struct _virDomainNVRAMDef { > + virDomainDeviceInfo info; > +}; > enum virDomainSmbiosMode { > VIR_DOMAIN_SMBIOS_NONE = 0, > @@ -1916,6 +1924,7 @@ struct _virDomainDef { > /* Only 1 */ > virDomainWatchdogDefPtr watchdog; > virDomainMemballoonDefPtr memballoon; > + virDomainNVRAMDefPtr nvram; > virDomainTPMDefPtr tpm; > virCPUDefPtr cpu; > virSysinfoDefPtr sysinfo; > @@ -2076,6 +2085,7 @@ int > virDomainChrSourceDefCopy(virDomainChrSourceDefPtr src, > void virDomainSoundCodecDefFree(virDomainSoundCodecDefPtr def); > void virDomainSoundDefFree(virDomainSoundDefPtr def); > void virDomainMemballoonDefFree(virDomainMemballoonDefPtr def); > +void virDomainNVRAMDefFree(virDomainNVRAMDefPtr def); > void virDomainWatchdogDefFree(virDomainWatchdogDefPtr def); > void virDomainVideoDefFree(virDomainVideoDefPtr def); > virDomainHostdevDefPtr virDomainHostdevDefAlloc(void); ACK with the nits fixed, and if a good answer for the question.