[libvirt] [PATCH v3 1/3] Add NVRAM device

[Li Zhang]

On 2013年04月11日 17:36, Daniel P. Berrange wrote:
> On Wed, Mar 27, 2013 at 01:07:54PM +0800, 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>
>> ---
>>   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
>>
>>   src/conf/domain_conf.c  |   76 +++++++++++++++++++++++++++++++++++++++++++++++
>>   src/conf/domain_conf.h  |   10 +++++++
>
>>   src/qemu/qemu_command.c |   67 +++++++++++++++++++++++++++++++++++++++++
>>   src/qemu/qemu_command.h |    2 ++
>>   4 files changed, 155 insertions(+)
> The QEMU pieces should be separate from the parser pieces. Basically instead
> of the 3 patches you have here you should have 2 patches with the following
> files in each one:
>
>   1.  src/conf/domain_conf.*, docs/schemas/* and
>       docs/formatdomain.html.in
>
>   2. src/qemu/* and tests/qemuxml2argv*
OK, got it.
>> @@ -13854,6 +13911,21 @@ virDomainMemballoonDefFormat(virBufferPtr buf,
>>   }
>>   
>>   static int
>> +virDomainNVRAMDefFormat(virBufferPtr buf,
>> +                             virDomainNVRAMDefPtr def,
>> +                             unsigned int flags)
>> +{
>> +    virBufferAsprintf(buf, "    <nvram>\n");
>> +    if (virDomainDeviceInfoIsSet(&def->info, flags))
>> +        if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
>> +            return -1;
>> +
>> +     virBufferAddLit(buf, "    </nvram>\n");
>> +
>> +     return 0;
> There's inconsistent indentation here
>
I will correct it.
>>   qemuBuildUSBInputDevStr(virDomainInputDefPtr dev,
>> @@ -7492,6 +7527,21 @@ qemuBuildCommandLine(virConnectPtr conn,
>>               goto error;
>>       }
>>   
>> +    if (def->nvram) {
>> +        if (def->os.arch == VIR_ARCH_PPC64 &&
>> +            STREQ(def->os.machine, "pseries")) {
>> +            char *optstr;
>> +            virCommandAddArg(cmd, "-global");
>> +            optstr = qemuBuildNVRAMDevStr(def->nvram);
>> +            if (!optstr)
>> +                goto error;
>> +            if (optstr)
>> +                virCommandAddArg(cmd, optstr);
>> +            VIR_FREE(optstr);
>> +        } else
>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> +                          _("NVRAM device is not supported"));
>
> Missing a 'goto error' here

Ok, I will add it.

>> @@ -9584,6 +9634,23 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps,
>>                   goto error;
>>               }
>>   
>> +        } else if (STREQ(arg, "-global") &&
>> +                   STRPREFIX(progargv[i + 1], "spapr-nvram.reg=")) {
>> +
>> +            WANT_VALUE();
>> +
>> +            if (VIR_ALLOC(def->nvram) < 0)
>> +                goto no_memory;
>> +
>> +            val += strlen("spapr-nvram.reg=");
>> +            if (virStrToLong_ull(val, NULL, 16,
>> +                                 &def->nvram->info.addr.spaprvio.reg) < 0) {
> Don't you need to set the address type too.

Ah, yes. I will add it.

>> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> +                               _("invalid value for spapr-nvram.reg :"
>> +                                 "'%s'"), val);
>> +                goto error;
>> +            }
>> +
>>           } else if (STREQ(arg, "-S")) {
>>               /* ignore, always added by libvirt */
>>           } else {
>
>
> Daniel