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

[Daniel P. Berrange]

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

> @@ -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.

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'

> +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.

> +     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

> +
>      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
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|