[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

On 25/04/13 11:07, Li Zhang wrote:
On 2013年04月25日 10:42, Osier Yang wrote:
On 25/04/13 10:13, Li Zhang wrote:
On 2013年04月24日 19:58, Osier Yang wrote:
On 19/04/13 16:37, Li Zhang wrote:
From: Li Zhang <zhlcindy 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:

<address type='spapr-vio' reg='0x3000'/>

Signed-off-by: Li Zhang <zhlcindy 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
+ <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>
+ ...
+ &lt;devices&gt;
+ &lt;nvram&gt;
+ &lt;address type='spapr-vio' reg='0x3000'/&gt;
+ &lt;/nvram&gt;
+ &lt;/devices&gt;
+ ...
+ <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


+ </p>
+ </dd>
+ </dl>
<h3><a name="seclabel">Security label</a></h3>
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 @@
+ <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 @@
<ref name="memballoon"/>
+ <optional>
+ <ref name="nvram"/>
+ </optional>
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,
+ "nvram",
VIR_ENUM_IMPL(virDomainDeviceAddress, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST, @@ -1560,6 +1561,16 @@ void virDomainMemballoonDefFree(virDomainMemballoonDefPtr 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)

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. :)
No, you misunderstood, virDomainDeviceDefFree is a wrapper of the all the free helpers of
each device type. Itself can be used as helper in the source too.

I see you add virDomainNVRAMDeviceFree in virDomainDefFree, but it's different things, sometimes you will want to use virDomainDeviceDefFree to free the device def, without careing about what
the device type is..

Ah, I am considering when this device is freed.
For example, when deleting one device from this domain,
it may call this this to free this device.
But actually, nvram device can't be deleted from this domain.

There are other places one will need to free the device def, not only
when detaching...

Is it okay if freeing the device def?
Logically, it seems that this should be fine.
But I am not sure whether this is reasonable.

It's fine to keep it not freed in virDomainDeviceFree, just like what you do in this patch, because same other device type, such as SMARTCARD also has the problem, we can
fix it as a later patch together.

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]