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

[Li Zhang]

On 2013年04月24日 19:58, Osier Yang wrote:
> On 19/04/13 16:37, Li Zhang wrote:
>> From: Li Zhang <zhlcindy at 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 at linux.vnet.ibm.com>
>> ---
>> 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.

It's better. Thanks.

>
>> + <p>
>> + Example: usage of NVRAM configuration
>> + </p>
>> +<pre>
>> + ...
>> + <devices>
>> + <nvram>
>> + <address type='spapr-vio' reg='0x3000'/>
>> + </nvram>
>> + </devices>
>> + ...
>> +</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.

It's freed when domain def is freed.
I think it is created when the domain is defined, so it is freed when 
the domain is freed.

But I am not sure whether it's same with 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.