1:10 p.m.
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(a)linux.vnet.ibm.com>
> ---
> 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,
>