On 06/09/2014 10:26 AM, Olivia Yin wrote:

Signed-off-by: Olivia Yin <Hong-Hua.Yin@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");