[libvirt] [PATCHv2] QEMU: Modify qemuParseCommandLinePCI() to parsee '-device vfio-pci'

[Laine Stump]

On 06/09/2014 10:26 AM, Olivia Yin wrote:
> Signed-off-by: Olivia Yin <Hong-Hua.Yin at freescale.com>
>
> Modify qemuParseCommandLinePCI() to support parsing '-device vfio-pci,host=bus:slot.func'.
> Add test case 'hostdev-vfio' into qemuargv2xmltest to validate this function.
>
> The case related to QEMU_CAPS_HOST_PCI_MULTIDOMAIN which uses
> '-device vfio-pci,host=domain:bus:slot.func' is not supported yet.

1) please squash in the change to the hostdev-vfio test data here (it's
not of any use by itself):

  https://www.redhat.com/archives/libvir-list/2014-June/msg00372.html

2) As long as you're changing the single existing function to work for
both -device vfio-pci and -pcidevice, you should also use it to add
support for "-device pci-assign", which has identical syntax to "-device
vfio-pci" (aside from the name, of course :-)

3) You are still causing unrecognized -device args to be silently
discarded, but they should instead be added to the qemu namespace. See
my comment inline below.


> ---
>  src/qemu/qemu_command.c  | 36 ++++++++++++++++++++++++++++++------
>  tests/qemuargv2xmltest.c |  2 +-
>  2 files changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 3cf279e..ae7f94e 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -10193,7 +10193,7 @@ qemuParseCommandLineNet(virDomainXMLOptionPtr xmlopt,
>   * Tries to parse a QEMU PCI device
>   */
>  static virDomainHostdevDefPtr
> -qemuParseCommandLinePCI(const char *val)
> +qemuParseCommandLinePCI(const char *val, bool vfio)
>  {
>      int bus = 0, slot = 0, func = 0;
>      const char *start;
> @@ -10222,10 +10222,20 @@ qemuParseCommandLinePCI(const char *val)
>          goto error;
>      }
>      start = end + 1;
> -    if (virStrToLong_i(start, NULL, 16, &func) < 0) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("cannot extract PCI device function '%s'"), val);
> -        goto error;
> +
> +    if (!vfio) {
> +        if (virStrToLong_i(start, NULL, 16, &func) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("cannot extract PCI device function '%s'"), val);
> +            goto error;
> +        }
> +    } else {
> +        if (virStrToLong_i(start, &end, 16, &func) < 0 || *end != ',') {

I don't think this difference between the two is needed. If you really
want to get that nit-picky, you should be going all the way and using
the helper function qemuParseKeywords to get *all* of the options, then
cycling through them in a loop - it is totally legal for those options
to come in any order, so not only might the host= option be the last
(and thus it won't be followed by a ","), but there may be other options
preceding it (for example the "bus", "addr", and "id" options, all used
by libvirt). Your current parsing is tailored specifically to
commandlines following the pattern used by libvirt, but the aim of
qemuCommandLineParseString() is to parse commandlines from other sources
as well.


> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("cannot extract PCI device function '%s'"), val);
> +            goto error;
> +        } else
> +            def->source.subsys.u.pci.backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO;
>      }
>  
>      def->mode = VIR_DOMAIN_HOSTDEV_MODE_SUBSYS;
> @@ -11347,12 +11357,26 @@ qemuParseCommandLine(virCapsPtr qemuCaps,
>          } else if (STREQ(arg, "-pcidevice")) {
>              virDomainHostdevDefPtr hostdev;
>              WANT_VALUE();
> -            if (!(hostdev = qemuParseCommandLinePCI(val)))
> +            if (!(hostdev = qemuParseCommandLinePCI(val,0)))

Since the 2nd arg is a bool, you should send "true" or "false", not "1"
and "0". And you need to put a space after any comma in an arglist.


>                  goto error;
>              if (VIR_APPEND_ELEMENT(def->hostdevs, def->nhostdevs, hostdev) < 0) {
>                  virDomainHostdevDefFree(hostdev);
>                  goto error;
>              }
> +        } else if (STREQ(arg, "-device")) {
> +            WANT_VALUE();
> +            if (STRPREFIX(val, "vfio-pci,")) {
> +                const char *start;
> +                start = val;
> +                virDomainHostdevDefPtr hostdev;
> +                start += strlen("vfio-pci,");
> +                if (!(hostdev = qemuParseCommandLinePCI(start,1)))

Again, use "true" instead of "1", and don't forget the leading space.

> +                    goto error;
> +                if (VIR_APPEND_ELEMENT(def->hostdevs, def->nhostdevs, hostdev) < 0) {
> +                    virDomainHostdevDefFree(hostdev);
> +                    goto error;
> +                }
> +            }

As I mentioned before, this code will cause any unrecognized -device to
be ignored. Instead, you really want something like this:

          } else if (STREQ(arg, "-device") && progargv[i + 1] &&
                     STRPREFIX(progargv[i + 1], "vfio-pci")) {
              const char *start;
              virDomainHostdevDefPtr hostdev;
              WANT_VALUE();
              start = val + strlen("vfio-pci,");
              if (!(hostdev = qemuParseCommandLinePCI(start, true)))
                  goto error;
              if (VIR_APPEND_ELEMENT(def->hostdevs, def->nhostdevs,
hostdev) < 0) {
                  virDomainHostdevDefFree(hostdev);
                  goto error;
              }
          }


>          } else if (STREQ(arg, "-soundhw")) {
>              const char *start;
>              WANT_VALUE();
> diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c
> index 0fc9fcb..b4ba97a 100644
> --- a/tests/qemuargv2xmltest.c
> +++ b/tests/qemuargv2xmltest.c
> @@ -251,8 +251,8 @@ mymain(void)
>      DO_TEST("watchdog");
>  
>      DO_TEST("hostdev-usb-address");
> -
>      DO_TEST("hostdev-pci-address");
> +    DO_TEST("hostdev-vfio");
>  
>      DO_TEST("smp");
>