Re: [PATCH v2 3/5] qemu: Add vhost-scsi string for -device parameter

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 09/12/2016 06:10 PM, John Ferlan wrote:

On 09/06/2016 08:58 AM, Eric Farman wrote:
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@xxxxxxxxxxxxxxxxxx>
---
  src/libvirt_private.syms |  1 +
  src/qemu/qemu_command.c  | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
  src/qemu/qemu_command.h  |  6 ++++
  src/util/virscsi.c       | 26 ++++++++++++++++
  src/util/virscsi.h       |  1 +
  5 files changed, 114 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d62c74c..22fe14d 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2265,6 +2265,7 @@ virSCSIDeviceListNew;
  virSCSIDeviceListSteal;
  virSCSIDeviceNew;
  virSCSIDeviceSetUsedBy;
+virSCSIOpenVhost;
# util/virseclabel.h
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 982c33c..479dde2 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4711,6 +4711,52 @@ qemuBuildSCSIiSCSIHostdevDrvStr(virDomainHostdevDefPtr dev)
  }
char *
+qemuBuildHostHostdevDevStr(const virDomainDef *def,
+                           virDomainHostdevDefPtr dev,
+                           virQEMUCapsPtr qemuCaps,
+                           char *vhostfdName,
+                           size_t vhostfdSize)
+{
+    virBuffer buf = VIR_BUFFER_INITIALIZER;
+    virDomainHostdevSubsysHostPtr hostsrc = &dev->source.subsys.u.host;
+
+    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", hostsrc->wwpn);
This is where id add the "naa." prefix, e.g wwpn=naa.%s" - with a
somewhat healthy comment behind it as to why it's being used.


+
+    if (vhostfdSize == 1) {
+        virBufferAsprintf(&buf, ",vhostfd=%s", vhostfdName);
+    } else {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                       _("QEMU supports a single vhost-scsi file descriptor"));
+        goto cleanup;
+    }
+
+    virBufferAsprintf(&buf, ",id=%s", dev->info->alias);
+
+    if (qemuBuildDeviceAddressStr(&buf, def, dev->info, qemuCaps) < 0)
+        goto cleanup;
I think this is what I would think auditing (during hotplug of course)
would end up using as the address rather than what will happen in patch
1 resulting in "?" being displayed.

+
+    if (virBufferCheckError(&buf) < 0)
+        goto cleanup;
+
+    return virBufferContentAndReset(&buf);
+
+ cleanup:
+    virBufferFreeAndReset(&buf);
+    return NULL;
+}
+
+char *
  qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev)
  {
      virBuffer buf = VIR_BUFFER_INITIALIZER;
@@ -5156,6 +5202,40 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
                  return -1;
              }
          }
+
+        /* SCSI_host */
+        if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
+            subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST) {
+            if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) {
I'm conflicted if we should check QEMU_CAPS_DEVICE_VHOST_SCSI here as
well...  I know it's checked later, but we open vhost-scsi which I
assume wouldn't exist if the cap wasn't there.

Of course I see in hotplug things have to be a little different in order
to add that fd and name via monitor rather than command.

+                if (hostdev->source.subsys.u.host.protocol == VIR_DOMAIN_HOSTDEV_HOST_PROTOCOL_TYPE_VHOST) {
80 cols for the line
+                    char *vhostfdName = NULL;
+                    int vhostfd = -1;
+                    size_t vhostfdSize = 1;
+
+                    if (virSCSIOpenVhost(&vhostfd, &vhostfdSize) < 0)
+                        return -1;
+
+                    if (virAsprintf(&vhostfdName, "%d", vhostfd) < 0) {
+                        VIR_FORCE_CLOSE(vhostfd);
+                        return -1;
+                    }
+
+                    virCommandPassFD(cmd, vhostfd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
80 cols for the line.
+
+                    virCommandAddArg(cmd, "-device");
+                    if (!(devstr = qemuBuildHostHostdevDevStr(def, hostdev, qemuCaps, vhostfdName, vhostfdSize)))
This is a really long line - try to keep at 80 cols.

+                        return -1;
We'd leak vhostfdName on failure (I think the vhostfd will be reaped by
Command cleanup.

My bad.


It might just be easier to extract the whole hunk into a function and do
all the error processing there.

Hey - BTW: I see this PCI configfd - I betcha a bit of tracking on that
will help give you examples for security_*.c changes and whether they're
necessary or not. I didn't go chase.

Ooh, good lead.  I said I'll chase down again, that's where I'll start.


+                    virCommandAddArg(cmd, devstr);
+
+                    VIR_FREE(vhostfdName);
+                    VIR_FREE(devstr);
+                }
+            } else {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                               _("SCSI passthrough is not supported by this version of qemu"));
+                return -1;
+            }
+        }
      }
return 0;
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index 9b9ccb6..78179de 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -158,6 +158,12 @@ char *qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev);
  char *qemuBuildSCSIHostdevDevStr(const virDomainDef *def,
                                   virDomainHostdevDefPtr dev,
                                   virQEMUCapsPtr qemuCaps);
+char *
+qemuBuildHostHostdevDevStr(const virDomainDef *def,
+                           virDomainHostdevDefPtr dev,
+                           virQEMUCapsPtr qemuCaps,
+                           char *vhostfdName,
+                           size_t vhostfdSize);
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)
Well this is dangerous... I can pass "any" value value here and really
cause some damage.  Why the need for a loop?

Well, I guess I only half-cleaned it up. I'm not aware of any mechanism to pass multiple fd's for vhost-scsi-pci or vhost-scsi-ccw into QEMU (unlike the virtio-scsi-{ccw|pci} paths), so I started ripping the loops out. Looking at it now, I guess I didn't rip out nearly as much as I thought I did. So, more to remove I guess. Unless leaving it there for an "if the future ever arrives" case is a problem.


I think you just attempt to open the file (you could even to a
virFileExists() if you want to care about checking if the
/dev/vhost-scsi exists before opening ...

I like this.


+{
+    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"));
Simplify your error _("failed to open /dev/vhost-scsi")


John

Many thanks again for the reviews.

 - Eric

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


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]