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

[Daniel P. Berrange]

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*

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

>  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

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

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