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

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.

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

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

> +{
> +    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
> +            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]