On 25.07.2016 22:48, Eric Farman wrote: I think this change should be squashed into 06/19. I understand you need these declarations here, so you can do the opposite and squash 06/19 into this one. Michal

On 25.07.2016 22:48, Eric Farman wrote: I wonder if we should turn those if - else if - else into: switch((virDomainHostdevSCSIProtocolType) scsisrc->protocol) { ... } Not a show stopper, could be done in a follow up patch. Michal

We're going to make some changes here, and the boolean logic does not lend itself to be easily expanded. Signed-off-by: Eric Farman <farman(a)linux.vnet.ibm.com&gt; Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; --- src/qemu/qemu_conf.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index fa9d65e..64f85dd 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1096,11 +1096,20 @@ qemuAddSharedDisk(virQEMUDriverPtr driver, static bool qemuIsSharedHostdev(virDomainHostdevDefPtr hostdev) { - return (hostdev->shareable && - (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && - hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && - hostdev->source.subsys.u.scsi.protocol != - VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI)); + if (!hostdev->shareable) + return false; + + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) + return false; + + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) + return false; + + if (hostdev->source.subsys.u.scsi.protocol == + VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) + return false; + + return true; } -- 1.9.1

Add code that will permit a hostdev tag to specify a vhost protocol, and parse the associated XML within it. But don't do anything with that information just yet. Signed-off-by: Eric Farman <farman(a)linux.vnet.ibm.com&gt; Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; --- src/conf/domain_conf.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9681d6c..d6b9c42 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2225,6 +2225,14 @@ virDomainHostdevSubsysSCSIiSCSIClear(virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc iscsisrc->auth = NULL; } +static void +virDomainHostdevSubsysSCSIVhostClear(virDomainHostdevSubsysSCSIVhostPtr vhostsrc) +{ + if (!vhostsrc) + return; + VIR_FREE(vhostsrc->wwpn); +} + void virDomainHostdevDefClear(virDomainHostdevDefPtr def) { if (!def) @@ -2261,6 +2269,9 @@ void virDomainHostdevDefClear(virDomainHostdevDefPtr def) if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) { virDomainHostdevSubsysSCSIiSCSIClear(&scsisrc->u.iscsi); + } else if (scsisrc->protocol == + VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_VHOST) { + virDomainHostdevSubsysSCSIVhostClear(&scsisrc->u.vhost); } else { VIR_FREE(scsisrc->u.host.adapter); } @@ -5943,6 +5954,31 @@ virDomainHostdevSubsysSCSIiSCSIDefParseXML(xmlNodePtr sourcenode, } static int +virDomainHostdevSubsysSCSIVhostDefParseXML(xmlNodePtr sourcenode, + virDomainHostdevSubsysSCSIPtr def) +{ + virDomainHostdevSubsysSCSIVhostPtr vhostsrc = &def->u.vhost; + + if (!(vhostsrc->wwpn = virXMLPropString(sourcenode, "wwpn"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing vhost-scsi hostdev source path name")); + goto cleanup; + } + + if (!STRPREFIX(vhostsrc->wwpn, "naa.") || + strlen(vhostsrc->wwpn) != strlen("naa.") + 16) { + virReportError(VIR_ERR_XML_ERROR, "%s", _("malformed 'wwpn' value")); + goto cleanup; + } + + return 0; + + cleanup: + VIR_FREE(vhostsrc->wwpn); + return -1; +} + +static int virDomainHostdevSubsysSCSIDefParseXML(xmlNodePtr sourcenode, virDomainHostdevSubsysSCSIPtr scsisrc) { @@ -5962,6 +5998,8 @@ virDomainHostdevSubsysSCSIDefParseXML(xmlNodePtr sourcenode, if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) ret = virDomainHostdevSubsysSCSIiSCSIDefParseXML(sourcenode, scsisrc); + else if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_VHOST) + ret = virDomainHostdevSubsysSCSIVhostDefParseXML(sourcenode, scsisrc); else ret = virDomainHostdevSubsysSCSIHostDefParseXML(sourcenode, scsisrc); -- 1.9.1

Starting two guests using the same vhost-scsi target results in an error when opening /dev/vhost-scsi. Since "shareable" suggests there's a way around this, let's disallow this tag when using vhost-scsi protocol for our hostdev device. Suggested-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; Signed-off-by: Eric Farman <farman(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; --- src/conf/domain_conf.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d6b9c42..b67d0ea 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12840,6 +12840,14 @@ virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt, def->readonly = true; if (virXPathBoolean("boolean(./shareable)", ctxt)) def->shareable = true; + if (def->source.subsys.u.scsi.protocol == + VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_VHOST && + def->shareable) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Devices connected with vhost-scsi protocol " + "cannot be marked shareable")); + goto error; + } break; } } -- 1.9.1

Open /dev/vhost-scsi, and record the resulting file descriptor, so that the guest has access to the host device outside of the libvirt daemon. Pass this information, along with data parsed from the XML file, to build a device string for the qemu command line. That device string will be for either a vhost-scsi-ccw device in the case of an s390 machine, or vhost-scsi-pci for any others. Signed-off-by: Eric Farman <farman(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; --- src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_command.h | 5 ++++ src/util/virscsi.c | 26 +++++++++++++++++++ src/util/virscsi.h | 1 + 5 files changed, 99 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9396c4e..b91c7a8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2253,6 +2253,7 @@ virSCSIDeviceListNew; virSCSIDeviceListSteal; virSCSIDeviceNew; virSCSIDeviceSetUsedBy; +virSCSIOpenVhost; # util/virseclabel.h diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index dd15ff8..31b30a4 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4533,6 +4533,67 @@ qemuBuildSCSIiSCSIHostdevDrvStr(virDomainHostdevDefPtr dev) } char * +qemuBuildSCSIVhostHostdevDevStr(const virDomainDef *def, + virDomainHostdevDefPtr dev, + virQEMUCapsPtr qemuCaps, + virCommandPtr cmd) +{ + size_t i; + virBuffer buf = VIR_BUFFER_INITIALIZER; + virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi; + virDomainHostdevSubsysSCSIVhostPtr vhostsrc = &scsisrc->u.vhost; + int *vhostfd = NULL; + size_t vhostfdSize = 1; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VHOST_SCSI)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support vhost-scsi devices")); + goto cleanup; + } + + if (ARCH_IS_S390(def->os.arch)) + virBufferAddLit(&buf, "vhost-scsi-ccw"); + else + virBufferAddLit(&buf, "vhost-scsi-pci"); + + virBufferAsprintf(&buf, ",wwpn=%s", vhostsrc->wwpn); + + if (VIR_ALLOC_N(vhostfd, vhostfdSize) < 0) + goto cleanup; + + memset(vhostfd, -1, sizeof(*vhostfd) * vhostfdSize); + + if (virSCSIOpenVhost(vhostfd, &vhostfdSize) < 0) + goto cleanup; + + for (i = 0; i < vhostfdSize; i++) { + if (cmd) { + virCommandPassFD(cmd, vhostfd[i], + VIR_COMMAND_PASS_FD_CLOSE_PARENT); + } + } + + if (vhostfdSize == 1) { + virBufferAsprintf(&buf, ",vhostfd=%d", vhostfd[0]); + } else { + /* FIXME if 'vhostfds' became a valid vhost-scsi property in QEMU */ + goto cleanup; + } + + virBufferAsprintf(&buf, ",id=%s", dev->info->alias); + + VIR_FREE(vhostfd); + return virBufferContentAndReset(&buf); + + cleanup: + for (i = 0; vhostfd && i < vhostfdSize; i++) + VIR_FORCE_CLOSE(vhostfd[i]); + VIR_FREE(vhostfd); + virBufferFreeAndReset(&buf); + return NULL; +} + +char * qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -4954,7 +5015,11 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) { - if (hostdev->source.subsys.u.scsi.protocol != VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_VHOST) { + if (hostdev->source.subsys.u.scsi.protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_VHOST) { + virCommandAddArg(cmd, "-device"); + if (!(devstr = qemuBuildSCSIVhostHostdevDevStr(def, hostdev, qemuCaps, cmd))) + return -1; + } else { char *drvstr; virCommandAddArg(cmd, "-drive"); diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index c4d0567..5c2dcb0 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -159,6 +159,11 @@ char *qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev); char *qemuBuildSCSIHostdevDevStr(const virDomainDef *def, virDomainHostdevDefPtr dev, virQEMUCapsPtr qemuCaps); +char * +qemuBuildSCSIVhostHostdevDevStr(const virDomainDef *def, + virDomainHostdevDefPtr dev, + virQEMUCapsPtr qemuCaps, + virCommandPtr cmd); char *qemuBuildRedirdevDevStr(const virDomainDef *def, virDomainRedirdevDefPtr dev, diff --git a/src/util/virscsi.c b/src/util/virscsi.c index 4843367..290b692 100644 --- a/src/util/virscsi.c +++ b/src/util/virscsi.c @@ -105,6 +105,32 @@ virSCSIDeviceGetAdapterId(const char *adapter, return -1; } +int +virSCSIOpenVhost(int *vhostfd, + size_t *vhostfdSize) +{ + size_t i; + + for (i = 0; i < *vhostfdSize; i++) { + vhostfd[i] = open("/dev/vhost-scsi", O_RDWR); + + if (vhostfd[i] < 0) { + virReportSystemError(errno, "%s", + _("vhost-scsi was requested for an interface, " + "but is unavailable")); + goto error; + } + } + + return 0; + + error: + while (i--) + VIR_FORCE_CLOSE(vhostfd[i]); + + return -1; +} + char * virSCSIDeviceGetSgName(const char *sysfs_prefix, const char *adapter, diff --git a/src/util/virscsi.h b/src/util/virscsi.h index df40d7f..cb37da8 100644 --- a/src/util/virscsi.h +++ b/src/util/virscsi.h @@ -33,6 +33,7 @@ typedef virSCSIDevice *virSCSIDevicePtr; typedef struct _virSCSIDeviceList virSCSIDeviceList; typedef virSCSIDeviceList *virSCSIDeviceListPtr; +int virSCSIOpenVhost(int *vhostfd, size_t *vhostfdSize); char *virSCSIDeviceGetSgName(const char *sysfs_prefix, const char *adapter, unsigned int bus, -- 1.9.1

On Mon, Jul 25, 2016 at 04:48:11PM -0400, Eric Farman wrote: This is missing setting device address info - ie a call to qemuBuildDeviceAddressStr() 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 :|

On 25.07.2016 22:48, Eric Farman wrote: AAh, this is the change I was missing in 06/19. Squash it there please. The reasoning behind is to make life easier for downstream maintainers It's very easy to forget to backport one patch - and now that we have schema validation enabled before actually parsing the XML downstream packages might be broken. Moreover, it is okay if the feature can be configured in domain XML but yet is declined on driver level. Michal

On 25.07.2016 22:48, Eric Farman wrote: Yet again, this should be squashed into 06/19. Michal

Adjust the device string that is built for vhost-scsi devices so that it can be invoked from hotplug. However, for hotplug, the file descriptors are expected to begin with at least one alphabetic character else this error occurs: # virsh attach-device guest_0001 ~/vhost.xml error: Failed to attach device from /root/vhost.xml error: internal error: unable to execute QEMU command 'getfd': Parameter 'fdname' expects a name not starting with a digit We also close the file descriptor in this case, so that shutting down the guest cleans up the host cgroup entries and allows future guests to use vhost-scsi devices. (Otherwise the guest will silently end.) Signed-off-by: Eric Farman <farman(a)linux.vnet.ibm.com&gt; Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; --- src/qemu/qemu_command.c | 34 ++++++++++++++++++++++++++++---- src/qemu/qemu_command.h | 3 ++- src/qemu/qemu_hotplug.c | 52 ++++++++++++++++++++++++++++++++++++++----------- src/qemu/qemu_monitor.c | 21 ++++++++++++++++++++ src/qemu/qemu_monitor.h | 4 ++++ 5 files changed, 98 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 31b30a4..6677c28 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4536,12 +4536,14 @@ char * qemuBuildSCSIVhostHostdevDevStr(const virDomainDef *def, virDomainHostdevDefPtr dev, virQEMUCapsPtr qemuCaps, - virCommandPtr cmd) + virCommandPtr cmd, + qemuMonitorPtr mon) { size_t i; virBuffer buf = VIR_BUFFER_INITIALIZER; virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi; virDomainHostdevSubsysSCSIVhostPtr vhostsrc = &scsisrc->u.vhost; + char **vhostfdName = NULL; int *vhostfd = NULL; size_t vhostfdSize = 1; @@ -4551,6 +4553,9 @@ qemuBuildSCSIVhostHostdevDevStr(const virDomainDef *def, goto cleanup; } + if ((cmd && mon) || (!cmd && !mon)) + goto cleanup; + if (ARCH_IS_S390(def->os.arch)) virBufferAddLit(&buf, "vhost-scsi-ccw"); else @@ -4563,18 +4568,28 @@ qemuBuildSCSIVhostHostdevDevStr(const virDomainDef *def, memset(vhostfd, -1, sizeof(*vhostfd) * vhostfdSize); + if (VIR_ALLOC_N(vhostfdName, vhostfdSize) < 0) + goto cleanup; + if (virSCSIOpenVhost(vhostfd, &vhostfdSize) < 0) goto cleanup; + if (mon) { + if (qemuMonitorAttachVhostSCSI(mon, vhostfd, vhostfdName, vhostfdSize) < 0) + goto cleanup; + } + for (i = 0; i < vhostfdSize; i++) { if (cmd) { virCommandPassFD(cmd, vhostfd[i], VIR_COMMAND_PASS_FD_CLOSE_PARENT); + if (virAsprintf(&vhostfdName[i], "%d", vhostfd[i]) < 0) + goto cleanup; } } if (vhostfdSize == 1) { - virBufferAsprintf(&buf, ",vhostfd=%d", vhostfd[0]); + virBufferAsprintf(&buf, ",vhostfd=%s", vhostfdName[0]); } else { /* FIXME if 'vhostfds' became a valid vhost-scsi property in QEMU */ goto cleanup; @@ -4582,13 +4597,24 @@ qemuBuildSCSIVhostHostdevDevStr(const virDomainDef *def, virBufferAsprintf(&buf, ",id=%s", dev->info->alias); + for (i = 0; vhostfd && i < vhostfdSize; i++) { + if (mon) + VIR_FORCE_CLOSE(vhostfd[i]); + if (vhostfdName) + VIR_FREE(vhostfdName[i]); + } VIR_FREE(vhostfd); + VIR_FREE(vhostfdName); return virBufferContentAndReset(&buf); cleanup: - for (i = 0; vhostfd && i < vhostfdSize; i++) + for (i = 0; vhostfd && i < vhostfdSize; i++) { VIR_FORCE_CLOSE(vhostfd[i]); + if (vhostfdName) + VIR_FREE(vhostfdName[i]); + } VIR_FREE(vhostfd); + VIR_FREE(vhostfdName); virBufferFreeAndReset(&buf); return NULL; } @@ -5017,7 +5043,7 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) { if (hostdev->source.subsys.u.scsi.protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_VHOST) { virCommandAddArg(cmd, "-device"); - if (!(devstr = qemuBuildSCSIVhostHostdevDevStr(def, hostdev, qemuCaps, cmd))) + if (!(devstr = qemuBuildSCSIVhostHostdevDevStr(def, hostdev, qemuCaps, cmd, NULL))) return -1; } else { char *drvstr; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 5c2dcb0..d5a6256 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -163,7 +163,8 @@ char * qemuBuildSCSIVhostHostdevDevStr(const virDomainDef *def, virDomainHostdevDefPtr dev, virQEMUCapsPtr qemuCaps, - virCommandPtr cmd); + virCommandPtr cmd, + qemuMonitorPtr mon); char *qemuBuildRedirdevDevStr(const virDomainDef *def, virDomainRedirdevDefPtr dev, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 02f7017..77e436c 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2003,16 +2003,29 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, return -1; } - /* Let's make sure the disk has a controller defined and loaded before - * trying to add it. The controller used by the disk must exist before a - * qemu command line string is generated. - * - * Ensure that the given controller and all controllers with a smaller index - * exist; there must not be any missing index in between. - */ - for (i = 0; i <= hostdev->info->addr.drive.controller; i++) { - if (!qemuDomainFindOrCreateSCSIDiskController(driver, vm, i)) - return -1; + if (hostdev->source.subsys.u.scsi.protocol != VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_VHOST) { + /* Let's make sure the disk has a controller defined and loaded before + * trying to add it. The controller used by the disk must exist before a + * qemu command line string is generated. + * + * Ensure that the given controller and all controllers with a smaller index + * exist; there must not be any missing index in between. + */ + for (i = 0; i <= hostdev->info->addr.drive.controller; i++) { + if (!qemuDomainFindOrCreateSCSIDiskController(driver, vm, i)) + return -1; + } + } else { + if (hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + if (qemuDomainMachineIsS390CCW(vm->def) && + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW)) + hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW; + } + if (hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { + if (virDomainCCWAddressAssign(hostdev->info, priv->ccwaddrs, + !hostdev->info->addr.ccw.assigned) < 0) + goto cleanup; + } } if (qemuHostdevPrepareSCSIDevices(driver, vm->def->name, @@ -2023,6 +2036,11 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to prepare scsi hostdev for iSCSI: %s"), iscsisrc->path); + } else if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_VHOST) { + virDomainHostdevSubsysSCSIVhostPtr vhostsrc = &scsisrc->u.vhost; + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to prepare scsi hostdev for vhost: %s"), + vhostsrc->wwpn); } else { virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; virReportError(VIR_ERR_INTERNAL_ERROR, @@ -2048,7 +2066,19 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, if (qemuDomainSecretHostdevPrepare(conn, priv, hostdev) < 0) goto cleanup; - if (hostdev->source.subsys.u.scsi.protocol != VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_VHOST) { + if (hostdev->source.subsys.u.scsi.protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_VHOST) { + qemuDomainObjEnterMonitor(driver, vm); + if (!(devstr = qemuBuildSCSIVhostHostdevDevStr(vm->def, hostdev, priv->qemuCaps, NULL, priv->mon))) + goto cleanup; + + if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0) + goto cleanup; + + if ((ret = qemuMonitorAddDevice(priv->mon, devstr)) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm) < 0); + goto cleanup; + } + } else { if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev))) goto cleanup; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 098e654..51c3531 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2689,6 +2689,27 @@ qemuMonitorGetChardevInfo(qemuMonitorPtr mon, } +int +qemuMonitorAttachVhostSCSI(qemuMonitorPtr mon, + int *vhostfd, + char **vhostfdName, + int vhostfdSize) +{ + size_t i; + + QEMU_CHECK_MONITOR(mon); + + for (i = 0; i < vhostfdSize; i++) { + if (virAsprintf(&vhostfdName[i], "vhost-%d", vhostfd[i]) < 0) + return -1; + if (qemuMonitorSendFileHandle(mon, vhostfdName[i], vhostfd[i]) < 0) + return -1; + } + + return 0; +} + + /** * qemuMonitorDriveDel: * @mon: monitor object diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index cb4cca8..67050c5 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -679,6 +679,10 @@ void qemuMonitorChardevInfoFree(void *data, const void *name); int qemuMonitorGetChardevInfo(qemuMonitorPtr mon, virHashTablePtr *retinfo); +int qemuMonitorAttachVhostSCSI(qemuMonitorPtr mon, + int *vhostfd, + char **vhostfdName, + int vhostfdSize); int qemuMonitorAttachPCIDiskController(qemuMonitorPtr mon, const char *bus, virPCIDeviceAddress *guestAddr); -- 1.9.1

When issuing virsh dumpxml and parsing a hostdev element, the vhost-scsi protocol needs to be handled separately from the existing options, to create valid XML. Signed-off-by: Eric Farman <farman(a)linux.vnet.ibm.com&gt; Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; --- src/conf/domain_conf.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b67d0ea..79087b2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -20441,6 +20441,7 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf, virDomainHostdevSubsysSCSIPtr scsisrc = &def->source.subsys.u.scsi; virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi; + virDomainHostdevSubsysSCSIVhostPtr vhostsrc = &scsisrc->u.vhost; if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI && pcisrc->backend != VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT) { @@ -20470,13 +20471,22 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf, virBufferAddLit(buf, " missing='yes'"); } - if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && - scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) { + if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { const char *protocol = virDomainHostdevSubsysSCSIProtocolTypeToString(scsisrc->protocol); - virBufferAsprintf(buf, " protocol='%s' name='%s'", - protocol, iscsisrc->path); + switch (scsisrc->protocol) { + case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI: + virBufferAsprintf(buf, " protocol='%s' name='%s'", + protocol, iscsisrc->path); + break; + case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_VHOST: + virBufferAsprintf(buf, " protocol='%s' wwpn='%s'", + protocol, vhostsrc->wwpn); + break; + default: + break; + } } virBufferAddLit(buf, ">\n"); @@ -20522,6 +20532,8 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf, virBufferEscapeString(buf, " name='%s'", iscsisrc->hosts[0].name); virBufferEscapeString(buf, " port='%s'", iscsisrc->hosts[0].port); virBufferAddLit(buf, "/>\n"); + } else if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_VHOST) { + /* Nothing to do here */ } else { virBufferAsprintf(buf, "<adapter name='%s'/>\n", scsihostsrc->adapter); -- 1.9.1

Since the vhost-scsi protocol for hostdev tags has no child elements within its source tag, we can use a self-closing source tag to make the resulting XML a little cleaner. Suggested-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; Signed-off-by: Eric Farman <farman(a)linux.vnet.ibm.com&gt; Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; --- src/conf/domain_conf.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 79087b2..8d6d208 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -20436,6 +20436,7 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf, unsigned int flags, bool includeTypeInAddr) { + bool closedSource = false; virDomainHostdevSubsysUSBPtr usbsrc = &def->source.subsys.u.usb; virDomainHostdevSubsysPCIPtr pcisrc = &def->source.subsys.u.pci; virDomainHostdevSubsysSCSIPtr scsisrc = &def->source.subsys.u.scsi; @@ -20481,7 +20482,8 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf, protocol, iscsisrc->path); break; case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_VHOST: - virBufferAsprintf(buf, " protocol='%s' wwpn='%s'", + closedSource = true; + virBufferAsprintf(buf, " protocol='%s' wwpn='%s'/", protocol, vhostsrc->wwpn); break; default: @@ -20533,7 +20535,7 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf, virBufferEscapeString(buf, " port='%s'", iscsisrc->hosts[0].port); virBufferAddLit(buf, "/>\n"); } else if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_VHOST) { - /* Nothing to do here */ + /* Nothing to do here; closedSource already set to true */ } else { virBufferAsprintf(buf, "<adapter name='%s'/>\n", scsihostsrc->adapter); @@ -20559,7 +20561,8 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf, } virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</source>\n"); + if (!closedSource) + virBufferAddLit(buf, "</source>\n"); return 0; } -- 1.9.1

The qemuxml2argv test was cloned from hostdev-scsi-virtio-scsi Signed-off-by: Eric Farman <farman(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; --- .../qemuxml2argv-hostdev-scsi-vhost-scsi.args | 24 ++++++++++++++++ .../qemuxml2argv-hostdev-scsi-vhost-scsi.xml | 33 ++++++++++++++++++++++ tests/qemuxml2argvmock.c | 12 ++++++++ tests/qemuxml2argvtest.c | 3 ++ 4 files changed, 72 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-vhost-scsi.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-vhost-scsi.xml diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-vhost-scsi.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-vhost-scsi.args new file mode 100644 index 0000000..a2c7c18 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-vhost-scsi.args @@ -0,0 +1,24 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest2 \ +-S \ +-M pc \ +-m 214 \ +-smp 1 \ +-uuid c7a5fdbd-edaf-9466-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-QEMUGuest2/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x3 \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-device vhost-scsi-pci,wwpn=naa.5123456789abcde0,vhostfd=3,id=hostdev0 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-vhost-scsi.xml b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-vhost-scsi.xml new file mode 100644 index 0000000..3fc4ef0 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-vhost-scsi.xml @@ -0,0 +1,33 @@ +<domain type='qemu'> + <name>QEMUGuest2</name> + <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='scsi' index='0' model='virtio-scsi'/> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <hostdev mode='subsystem' type='scsi'> + <source protocol='vhost' wwpn='naa.5123456789abcde0'/> + </hostdev> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index 78a224b..568dcc0 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -107,6 +107,18 @@ virSCSIDeviceGetSgName(const char *sysfs_prefix ATTRIBUTE_UNUSED, } int +virSCSIOpenVhost(int *vhostfd, + size_t *vhostfdSize) +{ + size_t i; + + for (i = 0; i < *vhostfdSize; i++) + vhostfd[i] = STDERR_FILENO + 1 + i; + + return 0; +} + +int virNetDevTapCreate(char **ifname, const char *tunpath ATTRIBUTE_UNUSED, int *tapfd, diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 356f843..6ec4a78 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1749,6 +1749,9 @@ mymain(void) DO_TEST("hostdev-scsi-virtio-iscsi-auth", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_DEVICE_SCSI_GENERIC); + DO_TEST("hostdev-scsi-vhost-scsi", + QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_DEVICE_VHOST_SCSI, + QEMU_CAPS_DEVICE_SCSI_GENERIC); DO_TEST("mlock-on", QEMU_CAPS_MLOCK); DO_TEST_FAILURE("mlock-on", NONE); -- 1.9.1

On Tue, Aug 09, 2016 at 02:13:29PM +0200, Michal Privoznik wrote: I'm not sure this syntax really makes sense. IIRC, currently <hostdev type="scsi"> is used to passthrough an individual SCSI LUNs from the host to the guest. With vhost-scsi, IIUC, you are passing through an entire (virtual) SCSI HBA to the guest. So I think it better for us to not reuse type="scsi" for that purpose. Instead it feels like we should be adding a type="scsi_host" for passthrough of entire HBAs Obviously this has major implications for almost all patches in this series 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 :|

On Tue, Aug 09, 2016 at 10:44:44AM -0400, Eric Farman wrote: No, it should be type=scsi_host, so that in the future if we want to pass through entire SCSI or iSCSI HBAs without vhost-scsi, then we can do it. The IQN name has to include the LUN in it - if that's left out, QEMU would complain that no LUN was specified. 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 :|

The idea of vhost-scsi is to have a controller-less host device that carries all LUNs, so (silently) creating a virtio-scsi controller is not necessary here. Let's get adjust the comparison so we don't accidentally make one. Signed-off-by: Eric Farman <farman(a)linux.vnet.ibm.com&gt; Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; --- src/conf/domain_conf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8d6d208..1768292 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15401,6 +15401,7 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def) hostdev = def->hostdevs[i]; if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && + hostdev->source.subsys.u.scsi.protocol != VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_VHOST && (int)hostdev->info->addr.drive.controller > maxController) { maxController = hostdev->info->addr.drive.controller; } -- 1.9.1