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

On 2013年04月24日 19:58, Osier Yang wrote: > 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. It's better. Thanks. > >> + <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. 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. :)