[libvirt] [PATCH v2 1/1][RESEND] Add NVRAM device

[Li Zhang]

On 2013年03月14日 19:02, Daniel P. Berrange wrote:
> On Thu, Mar 14, 2013 at 02:54:19PM +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>
>> ---
>>   * v2 -> v1:
>>      Add NVRAM parameters parsing in qemuParseCommandLine.
>>
>>   src/conf/domain_conf.c  |   83 ++++++++++++++++++++++++++++++++++++++++++++++-
>>   src/conf/domain_conf.h  |   10 ++++++
>>   src/qemu/qemu_command.c |   48 +++++++++++++++++++++++++++
>>   src/qemu/qemu_command.h |    2 ++
>>   4 files changed, 142 insertions(+), 1 deletion(-)
> When adding new XML, you also need to update docs/schemas/domaincommon.rng
> and docs/formatdomain.html.in
>
> In addition for anything which extends the QEMU command line generator
> you should be adding test case(s) to tests/qemuxml2argvtest.c and also
> tests/qemuargv2xmltest.c
Sure, I will do that in my next version.

>
>> @@ -10572,6 +10608,32 @@ virDomainDefParseXML(virCapsPtr caps,
>>           }
>>       }
>>   
>> +    def->nvram = NULL;
>> +    if ((n = virXPathNodeSet("./devices/nvram", ctxt, &nodes)) < 0) {
>> +        goto error;
>> +    }
>> +
>> +    if (n > 1) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                       _("only a single nvram device is supported"));
>> +        goto error;
>> +    }
>> +
>> +    if (n > 0) {
>> +        virDomainNVRAMDefPtr nvram =
>> +            virDomainNVRAMDefParseXML(nodes[0], flags);
>> +        if (!nvram)
>> +            goto error;
>> +        def->nvram = nvram;
>> +        VIR_FREE(nodes);
>> +    } else {
>> +        virDomainNVRAMDefPtr nvram;
>> +        if (VIR_ALLOC(nvram) < 0)
>> +            goto no_memory;
>> +        nvram->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
>> +        def->nvram = nvram;
> This adds a <nvram> device to every single guest whether it wants
> one or not, which is wrong. Just delete this entire 'else' block.

In QEMU, NVRAM device is always enabled.
So I would like to add this device in libvirt with default.
>
> NB if you had run 'make check' you would see this flaw.
>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index dee493f..30694d6 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -941,6 +941,13 @@ int qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def,
>>               goto cleanup;
>>       }
>>   
>> +    if (def->os.arch == VIR_ARCH_PPC64 &&
>> +        STREQ(def->os.machine, "pseries"))
>> +        def->nvram->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO;
>> +     if (qemuAssignSpaprVIOAddress(def, &def->nvram->info,
>> +                                   0x3000ul) < 0)
>> +        goto cleanup;
> Bad indentation level on the second 'if'
I will correct it.

>
>> +char *
>> +qemuBuildNVRAMDevStr(virDomainNVRAMDefPtr dev)
>> +{
>> +     virBuffer buf = VIR_BUFFER_INITIALIZER;
>> +     if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO &&
>> +         dev->info.addr.spaprvio.has_reg)
>> +        virBufferAsprintf(&buf, "spapr-nvram.reg=0x%llx",
>> +                          dev->info.addr.spaprvio.reg);
>> +
>
> You should have an 'else' clause here to report an error in that
> scenario
>
> You must also check  virBufferError() and raise an OOM error if
> required.
OK. I will add that.
>
>> +     return virBufferContentAndReset(&buf);
>> +}
>>   
>>   char *
>>   qemuBuildUSBInputDevStr(virDomainInputDefPtr dev,
>> @@ -7006,6 +7024,19 @@ qemuBuildCommandLine(virConnectPtr conn,
>>           }
>>       }
>>   
>> +    if (def->nvram &&
>> +        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);
>> +    }
> You must have an else clause here and report VIR_ERR_CONFIG_UNSUPPORTED
OK, I will add that in next version.

>
>> +
>>       if (snapshot)
>>           virCommandAddArgList(cmd, "-loadvm", snapshot->def->name, NULL);
>>   
>> @@ -9139,6 +9170,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) {
>> +                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 {
>
>
> Regards,
> Daniel