[libvirt] [PATCH v2 3/5] qemu: Add vhost-scsi string for -device parameter
Eric Farman
farman at linux.vnet.ibm.com
Tue Sep 13 18:10:19 UTC 2016
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 at 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,
>>
More information about the libvir-list
mailing list