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

[Li Zhang]

On 2013年04月25日 11:19, Osier Yang wrote:
> 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 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. :)
>>> 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.
>

Okay. So I will resend you [2/2] patch with files added later.

Thanks for your review. :)