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

[Eric Farman]

On 09/13/2016 04:55 PM, John Ferlan wrote:

[...]


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.


Doh - search on 'vhost-net' to find qemuInterfaceOpenVhostNet...  That's
a multiple open model and depending on virtio.queues which is something
I believe I was asking about for an attribute num_queues...


Yeah, that was the starting point.  What I'd discovered, and why I 
started ripping this apart, is that QEMU itself doesn't have an option 
to pass multiple fd's for a vhost-scsi device.  So while for vhost-net 
has fd= and (vhost)fds=, vhost-scsi only has vhostfd=.


qemu-system-s390x: -device 
vhost-scsi-ccw,wwpn=naa.500140594524c195,vhostfds=5: Property 
'.vhostfds' not found


The queues parameter operates in conjunction with the number of 
virtqueue rings that are instantiated for the guest process.


 - Eric

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

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

[John Ferlan]

[...]

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

Doh - search on 'vhost-net' to find qemuInterfaceOpenVhostNet...  That's
a multiple open model and depending on virtio.queues which is something
I believe I was asking about for an attribute num_queues...

John

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

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

[Eric Farman]

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 
---
  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 = >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(, "vhost-scsi-ccw");
+else
+virBufferAddLit(, "vhost-scsi-pci");
+
+virBufferAsprintf(, ",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(, ",vhostfd=%s", vhostfdName);
+} else {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("QEMU supports a single vhost-scsi file descriptor"));
+goto cleanup;
+}
+
+virBufferAsprintf(, ",id=%s", dev->info->alias);
+
+if (qemuBuildDeviceAddressStr(, 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() < 0)
+goto cleanup;
+
+return virBufferContentAndReset();
+
+ cleanup:
+virBufferFreeAndReset();
+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(, ) < 0)
+return -1;
+
+if (virAsprintf(, "%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 

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

[John Ferlan]

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 
> ---
>  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 = >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(, "vhost-scsi-ccw");
> +else
> +virBufferAddLit(, "vhost-scsi-pci");
> +
> +virBufferAsprintf(, ",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(, ",vhostfd=%s", vhostfdName);
> +} else {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("QEMU supports a single vhost-scsi file 
> descriptor"));
> +goto cleanup;
> +}
> +
> +virBufferAsprintf(, ",id=%s", dev->info->alias);
> +
> +if (qemuBuildDeviceAddressStr(, 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() < 0)
> +goto cleanup;
> +
> +return virBufferContentAndReset();
> +
> + cleanup:
> +virBufferFreeAndReset();
> +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(, ) < 0)
> +return -1;
> +
> +if (virAsprintf(, "%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