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

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(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. :) > 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.

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.