[PATCH 0/4] libbxl: add support for pvscsi, iteration 2

[PATCH 0/4] libbxl: add support for pvscsi, iteration 2

[Olaf Hering]

Port vscsi=[] and scsi-{attach,detach,list} commands from xend to libxl.

Initial attempt was sent a year ago:
http://lists.xenproject.org/archives/html/xen-devel/2014-04/msg03958.html
Most comments are addressed.

This version has been tested with SLES as backend and frontend.

So far I had no success to configure a pvops backend, which has to be
configured independent from libxl. Will continue to poke at that.

Please review. The first two changes could be applied independent from
the libxl changes.

Olaf



Olaf Hering (4):
  vscsiif.h: remove reference to WWN
  docs: add vscsi to xenstore-paths.markdown
  docs: add pvscsi.txt
  libxl: add support for vscsi

 docs/misc/pvscsi.txt                 | 147 +++++++++++++++
 docs/misc/xenstore-paths.markdown    |  10 +
 tools/libxl/Makefile                 |   1 +
 tools/libxl/libxl.c                  | 157 ++++++++++++++++
 tools/libxl/libxl.h                  |  29 +++
 tools/libxl/libxl_create.c           |   1 +
 tools/libxl/libxl_device.c           |   2 +
 tools/libxl/libxl_freebsd.c          |   8 +
 tools/libxl/libxl_internal.h         |  12 ++
 tools/libxl/libxl_linux.c            |  60 ++++++
 tools/libxl/libxl_netbsd.c           |   8 +
 tools/libxl/libxl_types.idl          |  41 +++++
 tools/libxl/libxl_types_internal.idl |   1 +
 tools/libxl/libxl_vscsi.c            | 343 +++++++++++++++++++++++++++++++++++
 tools/libxl/xl.h                     |   3 +
 tools/libxl/xl_cmdimpl.c             | 249 ++++++++++++++++++++++++-
 tools/libxl/xl_cmdtable.c            |  15 ++
 xen/include/public/io/vscsiif.h      |   5 +-
 18 files changed, 1088 insertions(+), 4 deletions(-)
 create mode 100644 docs/misc/pvscsi.txt
 create mode 100644 tools/libxl/libxl_vscsi.c

[PATCH 1/4] vscsiif.h: remove reference to WWN

[Olaf Hering]

The code in xend, xenlinux and pvops 3.18 expects the h:c:t:l notation.
Remove incorrect reference to WWN because it was never supported. And
its not clear how it could be supported. p-devname can be used to store
a config string.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---
 xen/include/public/io/vscsiif.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/xen/include/public/io/vscsiif.h b/xen/include/public/io/vscsiif.h
index 7a1db05..373ca3f 100644
--- a/xen/include/public/io/vscsiif.h
+++ b/xen/include/public/io/vscsiif.h
@@ -58,9 +58,8 @@
  * p-dev
  *      Values:         string
  *
- *      A string specifying the backend device: either a 4-tuple "h:c:t:l"
- *      (host, controller, target, lun, all integers), or a WWN (e.g.
- *      "naa.60014054ac780582").
+ *      A string specifying the backend device in form of a 4-tuple "h:c:t:l"
+ *      (host, controller, target, lun, all integers).
  *
  * v-dev
  *      Values:         string

[PATCH 2/4] docs: add vscsi to xenstore-paths.markdown

[Olaf Hering]

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---

NOTE: the URLS are still broken because the "unstable" directory does not exist.

 docs/misc/xenstore-paths.markdown | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown
index d94ea9d..77ba2dd 100644
--- a/docs/misc/xenstore-paths.markdown
+++ b/docs/misc/xenstore-paths.markdown
@@ -232,6 +232,11 @@ A virtual keyboard device frontend. Described by
 A virtual network device frontend. Described by
 [xen/include/public/io/netif.h][NETIF]
 
+#### ~/device/vscsi/$DEVID/* []
+
+A virtual scsi device frontend. Described by
+[xen/include/public/io/vscsiif.h][SCSIIF]
+
 #### ~/console/* []
 
 The primary PV console device. Described in [console.txt](console.txt)
@@ -302,6 +307,10 @@ A virtual keyboard device backend. Described by
 A virtual network device backend. Described by
 [xen/include/public/io/netif.h][NETIF]
 
+#### ~/backend/vscsi/$DOMID/$DEVID/* []
+
+A PV SCSI backend. Described in [pvscsi.txt](pvscsi.txt)
+
 #### ~/backend/console/$DOMID/$DEVID/* []
 
 A PV console backend. Described in [console.txt](console.txt)
@@ -403,6 +412,7 @@ ifb device used by Remus to buffer network output from the associated vif.
 [KBDIF]: http://xenbits.xen.org/docs/unstable/hypercall/include,public,io,kbdif.h.html
 [LIBXLMEM]: http://xenbits.xen.org/docs/unstable/misc/libxl_memory.txt
 [NETIF]: http://xenbits.xen.org/docs/unstable/hypercall/include,public,io,netif.h.html
+[SCSIIF]: http://xenbits.xen.org/docs/unstable/hypercall/include,public,vscsiif.h.html
 [SI]: http://xenbits.xen.org/docs/unstable/hypercall/include,public,xen.h.html#Struct_start_info
 [VCPU]: http://xenbits.xen.org/docs/unstable/hypercall/include,public,vcpu.h.html
 [XSWIRE]: http://xenbits.xen.org/docs/unstable/hypercall/include,public,io,xs_wire.h.html

[PATCH 3/4] docs: add pvscsi.txt

[Olaf Hering]

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---
 docs/misc/pvscsi.txt | 147 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 147 insertions(+)

diff --git a/docs/misc/pvscsi.txt b/docs/misc/pvscsi.txt
new file mode 100644
index 0000000..2af050f
--- /dev/null
+++ b/docs/misc/pvscsi.txt
@@ -0,0 +1,147 @@
+. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .
+PVSCSI
+
+== Overview ==
+
+PVSCSI allows to assing a "physical" SCSI device from dom0 to a domU. The device
+is not limited to be a native SCSI device. Everything visible as SCSI device in
+dom0 can be used. Currently PVSCSI is only available in Linux dom0 and domU.
+
+== TODO ===
+
+How to do live migration?
+ - pdev will likely be evaluated again on the target host if it came from
+   domU.cfg. But what about pdev from 'xl scsi-attach pdev vdev'? Its required
+   to adjust h:c:t:l on the target host.
+
+Implement checks for duplicate pdev assignments?
+ - Not sure if SCSI devices can be shared. Will the state be consistent during
+   concurrent access? Should the host admin assume cooperative access from
+   multiple domUs?
+ - The easiest way would be to do nothing and assume the host admin knows what
+   to do.
+
+How to handle FIXME in libxl_retrieve_domain_configuration?
+ - "MERGE(vscsi, vscsis, COMPARE_DEVID, {});", when does this code run?
+ 
+Implement libvirt integration.
+ - Is the API used by xl usable for libvirt?
+ - How does libvirt assign a raw SCSI device?
+   Jim said:
+    > vscsi=[ '/dev/sda,0:1:2:3', '4:3:2:1,1:1:1:1,feature-host' ]
+    In libvirt-speak, this is SCSI host device assignment
+    http://libvirt.org/formatdomain.html#elementsHostDev
+    Here's the SCSI example
+      <devices>
+       <hostdev mode='subsystem' type='scsi' sgio='filtered' rawio='yes'>
+        <source>
+         <adapter name='scsi_host0'/>
+         <address type='scsi' bus='0' target='0' unit='0'/>
+        </source>
+        <readonly/>
+        <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+       </hostdev>
+      ...
+      </devices>
+    I suppose 'feature-host' maps to attributes such as 'sgio' and 'rawio'.
+    The source element describes the SCSI device as seen from the host.
+
+
+== History ==
+
+Support for PVSCSI was added to the classic xenlinux kernel and to xend in
+xen-3.3 in 2008. Support for PVSCSI was added to the upstream pvops kernel 3.18
+in 2014. Backends exists just for Linux. A frontend exists for Linux, and maybe
+for FreeBSD according to google.
+
+
+== Config Format ==
+
+The domU.cfg syntax is "vscsi=[ 'pdev,vdev[,option]' ]". pdev is the dom0 device
+in either /dev/$device or pHOST:CHANNEL:TARGET:LUN notation.  vdev is the domU
+device in vHOST:CHANNEL:TARGET:LUN notation. 'option' lists additional flags.
+Currently only the value "feature-host" is recognized. SCSI command emulation in
+backend driver is bypassed when "feature-host" is specified.
+
+
+== Xenstore Format ==
+
+The backend driver uses a single_host:many_devices notation to manage domU
+devices. Everything is stored in /local/domain/0/backend/vscsi/. The xenstore
+layout looks like this:
+
+    <domid>/<vhost>/feature-host = "0"
+    <domid>/<vhost>/frontend = "/local/domain/<domid>/device/vscsi/0"
+    <domid>/<vhost>/frontend-id = "<domid>"
+    <domid>/<vhost>/online = "1"
+    <domid>/<vhost>/state = "4"
+    <domid>/<vhost>/vscsi-devs/dev-0/p-dev = "8:0:2:1"
+    <domid>/<vhost>/vscsi-devs/dev-0/state = "4"
+    <domid>/<vhost>/vscsi-devs/dev-0/v-dev = "0:0:0:0"
+    <domid>/<vhost>/vscsi-devs/dev-1/p-dev = "8:0:2:2"
+    <domid>/<vhost>/vscsi-devs/dev-1/state = "4"
+    <domid>/<vhost>/vscsi-devs/dev-1/v-dev = "0:0:1:0"
+
+The frontend driver maintains its state in /local/domain/<domid>/device/vscsi/.
+
+    <vhost>/backend = "/local/domain/0/backend/vscsi/<domid>/<vhost>"
+    <vhost>/backend-id = "0"
+    <vhost>/event-channel = "20"
+    <vhost>/ring-ref = "43"
+    <vhost>/state = "4"
+    <vhost>/vscsi-devs/dev-0/state = "4"
+    <vhost>/vscsi-devs/dev-1/state = "4"
+
+In addition to the entries for backend and frontend these flags are stored for
+the toolstack:
+
+    <domid>/<vhost>/vscsi-devs/dev-1/p-devname = "/dev/$device"
+
+
+== Backend - Frontend Protocol ==
+
+To create a vhost along with a device:
+    <domid>/<vhost>/feature-host = "0"
+    <domid>/<vhost>/frontend = "/local/domain/<domid>/device/vscsi/0"
+    <domid>/<vhost>/frontend-id = "<domid>"
+    <domid>/<vhost>/online = "1"
+    <domid>/<vhost>/state = "1"
+    <domid>/<vhost>/vscsi-devs/dev-0/p-dev = "8:0:2:1"
+    <domid>/<vhost>/vscsi-devs/dev-0/state = "1"
+    <domid>/<vhost>/vscsi-devs/dev-0/v-dev = "0:0:0:0"
+Wait for <domid>/<vhost>/state + <domid>/<vhost>/vscsi-devs/dev-0/state become 4
+
+To add another device to a vhost:
+    <domid>/<vhost>/state = "7"
+    <domid>/<vhost>/vscsi-devs/dev-1/p-dev = "8:0:2:2"
+    <domid>/<vhost>/vscsi-devs/dev-1/state = "1"
+    <domid>/<vhost>/vscsi-devs/dev-1/v-dev = "0:0:1:0"
+Wait for <domid>/<vhost>/state + <domid>/<vhost>/vscsi-devs/dev-1/state become 4
+
+To remove a device from a vhost:
+    <domid>/<vhost>/state = "7"
+    <domid>/<vhost>/vscsi-devs/dev-1/state = "5"
+Wait for <domid>/<vhost>/state to become 4
+Wait for <domid>/<vhost>/vscsi-devs/dev-1/state become 6
+Remove <domid>/<vhost>/vscsi-devs/dev-1/{state,p-dev,v-dev,p-devname}
+Remove <domid>/<vhost>/vscsi-devs/dev-1/
+
+
+== Interface in xl ==
+
+xl scsi-attach domid <PhysDevice> <VirtDevice>[,<option>]
+xl scsi-detach domid <VirtDevice>
+xl scsi-list domid [domid, ...]
+
+
+== Interface in libxl ==
+
+To be defined so that both xl and libvirt pass in the same data.
+
+
+== Interface in libvirt ==
+
+Has yet to be found.
+
+. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .
+# vim: tw=80 et ts=4

[PATCH 4/4] libxl: add support for vscsi

[Olaf Hering]

Port pvscsi support from xend to libxl. See pvscsi.txt for details.

Outstanding work is listed in the TODO section.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/Makefile                 |   1 +
 tools/libxl/libxl.c                  | 157 ++++++++++++++++
 tools/libxl/libxl.h                  |  29 +++
 tools/libxl/libxl_create.c           |   1 +
 tools/libxl/libxl_device.c           |   2 +
 tools/libxl/libxl_freebsd.c          |   8 +
 tools/libxl/libxl_internal.h         |  12 ++
 tools/libxl/libxl_linux.c            |  60 ++++++
 tools/libxl/libxl_netbsd.c           |   8 +
 tools/libxl/libxl_types.idl          |  41 +++++
 tools/libxl/libxl_types_internal.idl |   1 +
 tools/libxl/libxl_vscsi.c            | 343 +++++++++++++++++++++++++++++++++++
 tools/libxl/xl.h                     |   3 +
 tools/libxl/xl_cmdimpl.c             | 249 ++++++++++++++++++++++++-
 tools/libxl/xl_cmdtable.c            |  15 ++
 15 files changed, 929 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 7329521..9622d66 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -91,6 +91,7 @@ endif
 LIBXL_LIBS += -lyajl
 
 LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \
+			libxl_vscsi.o \
 			libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o \
 			libxl_internal.o libxl_utils.o libxl_uuid.o \
 			libxl_json.o libxl_aoutils.o libxl_numa.o \
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index b9a1941..07748ea 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1960,6 +1960,151 @@ static int libxl__resolve_domid(libxl__gc *gc, const char *name,
 }
 
 /******************************************************************************/
+static int libxl__device_from_vscsi(libxl__gc *gc, uint32_t domid,
+                                    libxl_device_vscsi *vscsi,
+                                    libxl__device *device)
+{
+    device->backend_domid = vscsi->backend_domid;
+    device->devid         = vscsi->devid;
+    device->domid         = domid;
+    device->backend_kind  = LIBXL__DEVICE_KIND_VSCSI;
+    device->kind          = LIBXL__DEVICE_KIND_VSCSI;
+
+    return 0;
+}
+
+void libxl__device_vscsi_add(libxl__egc *egc, uint32_t domid,
+                             libxl_device_vscsi *vscsi,
+                             libxl__ao_device *aodev)
+{
+    STATE_AO_GC(aodev->ao);
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+    flexarray_t *front;
+    flexarray_t *back;
+    libxl__device *device;
+    char *be_path;
+    unsigned int be_dirs = 0, rc, i;
+
+    if (vscsi->devid == -1) {
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    /* Prealloc key+value: 4 toplevel + 4 per device */
+    i = 2 * (4 + (4 * vscsi->num_vscsi_devs));
+    back = flexarray_make(gc, i, 1);
+    front = flexarray_make(gc, 2 * 2, 1);
+
+    GCNEW(device);
+    rc = libxl__device_from_vscsi(gc, domid, vscsi, device);
+    if ( rc != 0 ) goto out;
+
+    /* Check if backend device path is already present */
+    be_path = libxl__device_backend_path(gc, device);
+    if (!libxl__xs_directory(gc, XBT_NULL, be_path, &be_dirs) || !be_dirs) {
+        /* backend does not exist, create a new one */
+        flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid));
+        flexarray_append_pair(back, "online", "1");
+        flexarray_append_pair(back, "state", "1");
+        flexarray_append_pair(back, "feature-host", GCSPRINTF("%d", !!vscsi->feature_host));
+
+        flexarray_append_pair(front, "backend-id", GCSPRINTF("%d", vscsi->backend_domid));
+        flexarray_append_pair(front, "state", "1");
+    }
+
+    for (i = 0; i < vscsi->num_vscsi_devs; i++) {
+        libxl_vscsi_dev *v = vscsi->vscsi_devs + i;
+        /* Trigger removal, otherwise create new device */
+        if (be_dirs) {
+            unsigned int nb = 0;
+            /* Preserve existing device */
+            if (libxl__xs_directory(gc, XBT_NULL, GCSPRINTF("%s/vscsi-devs/dev-%u", be_path, v->vscsi_dev_id), &nb) && nb) {
+                /* Trigger device removal by forwarding state to XenbusStateClosing */
+                if (v->remove)
+                    flexarray_append_pair(back, GCSPRINTF("vscsi-devs/dev-%u/state", v->vscsi_dev_id), "5");
+                continue;
+            }
+        }
+        flexarray_append_pair(back, GCSPRINTF("vscsi-devs/dev-%u/p-devname", v->vscsi_dev_id), v->p_devname);
+        flexarray_append_pair(back, GCSPRINTF("vscsi-devs/dev-%u/p-dev", v->vscsi_dev_id),
+                              GCSPRINTF("%u:%u:%u:%u", v->pdev.hst, v->pdev.chn, v->pdev.tgt, v->pdev.lun));
+        flexarray_append_pair(back, GCSPRINTF("vscsi-devs/dev-%u/v-dev", v->vscsi_dev_id),
+                              GCSPRINTF("%u:%u:%u:%u", v->vdev.hst, v->vdev.chn, v->vdev.tgt, v->vdev.lun));
+        flexarray_append_pair(back, GCSPRINTF("vscsi-devs/dev-%u/state", v->vscsi_dev_id), "1");
+    }
+
+    aodev->dev = device;
+    /* Either create new host or reconfigure existing host */
+    if (be_dirs == 0) {
+        libxl__device_generic_add(gc, XBT_NULL, device,
+                                  libxl__xs_kvs_of_flexarray(gc, back, back->count),
+                                  libxl__xs_kvs_of_flexarray(gc, front, front->count),
+                                  NULL);
+        aodev->action = LIBXL__DEVICE_ACTION_ADD;
+        libxl__wait_device_connection(egc, aodev);
+        rc = 0;
+        /* Done with new host */
+        goto out;
+    }
+
+    /* Only new devices, write them and do vscsi host reconfiguration */
+    xs_transaction_t t;
+retry_transaction:
+    t = xs_transaction_start(ctx->xsh);
+    libxl__xs_writev(gc, t, be_path,
+                     libxl__xs_kvs_of_flexarray(gc, back, back->count));
+    xs_write(ctx->xsh, t, GCSPRINTF("%s/state", be_path), "7", 2);
+    if (!xs_transaction_end(ctx->xsh, t, 0)) {
+        if (errno == EAGAIN)
+            goto retry_transaction;
+        LOGE(ERROR, "xs transaction failed");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+    libxl__wait_for_backend(gc, be_path, "4");
+
+retry_transaction2:
+    t = xs_transaction_start(ctx->xsh);
+    for (i = 0; i < vscsi->num_vscsi_devs; i++) {
+        libxl_vscsi_dev *v = vscsi->vscsi_devs + i;
+        if (v->remove) {
+            char *path, *val;
+            path = GCSPRINTF("%s/vscsi-devs/dev-%u/state", be_path, v->vscsi_dev_id);
+            val = libxl__xs_read(gc, t, path);
+            if (val && strcmp(val, "6") == 0) {
+                path = GCSPRINTF("%s/vscsi-devs/dev-%u/state", be_path, v->vscsi_dev_id);
+                xs_rm(ctx->xsh, t, path);
+                path = GCSPRINTF("%s/vscsi-devs/dev-%u/p-devname", be_path, v->vscsi_dev_id);
+                xs_rm(ctx->xsh, t, path);
+                path = GCSPRINTF("%s/vscsi-devs/dev-%u/p-dev", be_path, v->vscsi_dev_id);
+                xs_rm(ctx->xsh, t, path);
+                path = GCSPRINTF("%s/vscsi-devs/dev-%u/v-dev", be_path, v->vscsi_dev_id);
+                xs_rm(ctx->xsh, t, path);
+                path = GCSPRINTF("%s/vscsi-devs/dev-%u", be_path, v->vscsi_dev_id);
+                xs_rm(ctx->xsh, t, path);
+            } else {
+                LOGE(ERROR, "%s: %s has %s, expected 6", __func__, path, val);
+            }
+        }
+    }
+
+    if (!xs_transaction_end(ctx->xsh, t, 0)) {
+        if (errno == EAGAIN)
+            goto retry_transaction2;
+        LOGE(ERROR, "xs transaction failed");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+    /* As we are not adding new device, skip waiting for it */
+    libxl__ao_complete(egc, aodev->ao, 0);
+
+    rc = 0;
+out:
+    aodev->rc = rc;
+    if(rc) aodev->callback(egc, aodev);
+    return;
+}
+
 int libxl__device_vtpm_setdefault(libxl__gc *gc, libxl_device_vtpm *vtpm)
 {
     int rc;
@@ -4104,6 +4249,8 @@ out:
  * libxl_device_disk_destroy
  * libxl_device_nic_remove
  * libxl_device_nic_destroy
+ * libxl_device_vscsi_remove
+ * libxl_device_vscsi_destroy
  * libxl_device_vtpm_remove
  * libxl_device_vtpm_destroy
  * libxl_device_vkb_remove
@@ -4148,6 +4295,10 @@ DEFINE_DEVICE_REMOVE(disk, destroy, 1)
 DEFINE_DEVICE_REMOVE(nic, remove, 0)
 DEFINE_DEVICE_REMOVE(nic, destroy, 1)
 
+/* vtpm */
+DEFINE_DEVICE_REMOVE(vscsi, remove, 0)
+DEFINE_DEVICE_REMOVE(vscsi, destroy, 1)
+
 /* vkb */
 DEFINE_DEVICE_REMOVE(vkb, remove, 0)
 DEFINE_DEVICE_REMOVE(vkb, destroy, 1)
@@ -4202,6 +4353,9 @@ DEFINE_DEVICE_ADD(disk)
 /* nic */
 DEFINE_DEVICE_ADD(nic)
 
+/* vscsi */
+DEFINE_DEVICE_ADD(vscsi)
+
 /* vtpm */
 DEFINE_DEVICE_ADD(vtpm)
 
@@ -6702,6 +6856,9 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
 
     MERGE(nic, nics, COMPARE_DEVID, {});
 
+    /* FIXME */
+    MERGE(vscsi, vscsis, COMPARE_DEVID, {});
+
     MERGE(vtpm, vtpms, COMPARE_DEVID, {});
 
     MERGE(pci, pcidevs, COMPARE_PCI, {});
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index c219f59..486e4cc 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1218,6 +1218,35 @@ int libxl_device_channel_getinfo(libxl_ctx *ctx, uint32_t domid,
                                  libxl_device_channel *channel,
                                  libxl_channelinfo *channelinfo);
 
+/* Virtual SCSI */
+int libxl_device_vscsi_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vscsi *vscsi,
+                           const libxl_asyncop_how *ao_how)
+                           LIBXL_EXTERNAL_CALLERS_ONLY;
+int libxl_device_vscsi_remove(libxl_ctx *ctx, uint32_t domid,
+                              libxl_device_vscsi *vscsi,
+                              const libxl_asyncop_how *ao_how)
+                              LIBXL_EXTERNAL_CALLERS_ONLY;
+int libxl_device_vscsi_destroy(libxl_ctx *ctx, uint32_t domid,
+                               libxl_device_vscsi *vscsi,
+                               const libxl_asyncop_how *ao_how)
+                               LIBXL_EXTERNAL_CALLERS_ONLY;
+
+libxl_device_vscsi *libxl_device_vscsi_list(libxl_ctx *ctx, uint32_t domid, int *num);
+int libxl_device_vscsi_getinfo(libxl_ctx *ctx,
+                               uint32_t domid,
+                               libxl_device_vscsi *vscsi_host,
+                               libxl_vscsi_dev *vscsi_dev,
+                               libxl_vscsiinfo *vscsiinfo);
+void libxl_device_vscsi_append_dev(libxl_ctx *ctx, libxl_device_vscsi *hst,
+                                   libxl_vscsi_dev *dev);
+int libxl_device_vscsi_get_host(libxl_ctx *ctx,
+                                uint32_t domid,
+                                const char *cfg,
+                                libxl_device_vscsi **vscsi_host);
+int libxl_device_vscsi_parse(libxl_ctx *ctx, const char *cfg,
+                             libxl_device_vscsi *vscsi_host,
+                             libxl_vscsi_dev *vscsi_dev);
+
 /* Virtual TPMs */
 int libxl_device_vtpm_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vtpm *vtpm,
                           const libxl_asyncop_how *ao_how)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 98687bd..5cacc30 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1136,6 +1136,7 @@ static void domcreate_rebuild_done(libxl__egc *egc,
     libxl__multidev_begin(ao, &dcs->multidev);
     dcs->multidev.callback = domcreate_launch_dm;
     libxl__add_disks(egc, ao, domid, d_config, &dcs->multidev);
+    libxl__add_vscsis(egc, ao, domid, d_config, &dcs->multidev);
     libxl__multidev_prepared(egc, &dcs->multidev, 0);
 
     return;
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 0f50d04..035a4b6 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -543,6 +543,7 @@ void libxl__multidev_prepared(libxl__egc *egc,
  * The following functions are defined:
  * libxl__add_disks
  * libxl__add_nics
+ * libxl__add_vscsis
  * libxl__add_vtpms
  */
 
@@ -562,6 +563,7 @@ void libxl__multidev_prepared(libxl__egc *egc,
 
 DEFINE_DEVICES_ADD(disk)
 DEFINE_DEVICES_ADD(nic)
+DEFINE_DEVICES_ADD(vscsi)
 DEFINE_DEVICES_ADD(vtpm)
 
 #undef DEFINE_DEVICES_ADD
diff --git a/tools/libxl/libxl_freebsd.c b/tools/libxl/libxl_freebsd.c
index e8b88b3..71bfae6 100644
--- a/tools/libxl/libxl_freebsd.c
+++ b/tools/libxl/libxl_freebsd.c
@@ -131,3 +131,11 @@ libxl_device_model_version libxl__default_device_model(libxl__gc *gc)
 {
     return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
 }
+
+int libxl_device_vscsi_parse_pdev(libxl__gc *gc, char *pdev, unsigned int *hst,
+                                unsigned int *chn, unsigned int *tgt,
+                                unsigned int *lun)
+{
+
+    return ERROR_NOPARAVIRT;
+}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 934465a..76f6056 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1794,6 +1794,10 @@ _hidden libxl__json_object *libxl__json_parse(libxl__gc *gc_opt, const char *s);
 _hidden int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
   /* Return the system-wide default device model */
 _hidden libxl_device_model_version libxl__default_device_model(libxl__gc *gc);
+  /* Convert h:c:t:l string to int */
+_hidden int libxl_device_vscsi_parse_hctl(libxl__gc *gc, char *str, libxl_vscsi_hctl *hctl);
+  /* Convert /dev/scsi to h:c:t:l */
+_hidden int libxl_device_vscsi_parse_pdev(libxl__gc *gc, char *pdev, libxl_vscsi_hctl *hctl);
 
 /* Check how executes hotplug script currently */
 int libxl__hotplug_settings(libxl__gc *gc, xs_transaction_t t);
@@ -2386,6 +2390,10 @@ _hidden void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
                                    libxl_device_nic *nic,
                                    libxl__ao_device *aodev);
 
+_hidden void libxl__device_vscsi_add(libxl__egc *egc, uint32_t domid,
+                                     libxl_device_vscsi *vscsi,
+                                     libxl__ao_device *aodev);
+
 _hidden void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
                                    libxl_device_vtpm *vtpm,
                                    libxl__ao_device *aodev);
@@ -3005,6 +3013,10 @@ _hidden void libxl__add_nics(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
                              libxl_domain_config *d_config,
                              libxl__multidev *multidev);
 
+_hidden void libxl__add_vscsis(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
+                               libxl_domain_config *d_config,
+                               libxl__multidev *multidev);
+
 _hidden void libxl__add_vtpms(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
                              libxl_domain_config *d_config,
                              libxl__multidev *multidev);
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index b51930c..305948e 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -279,3 +279,63 @@ libxl_device_model_version libxl__default_device_model(libxl__gc *gc)
 {
     return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
 }
+
+int libxl_device_vscsi_parse_pdev(libxl__gc *gc, char *pdev, libxl_vscsi_hctl *hctl)
+{
+    struct stat dentry;
+    char *sysfs;
+    const char *type;
+    int rc, found = 0;
+    DIR *dirp;
+    struct dirent *de;
+
+    /* stat pdev to get device's sysfs entry */
+    if (stat (pdev, &dentry) < 0) {
+        LOG(ERROR, "vscsi: %s, device node not found", pdev);
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    if (S_ISBLK (dentry.st_mode)) {
+        type = "block";
+    } else if (S_ISCHR (dentry.st_mode)) {
+        type = "char";
+    } else {
+        LOG(ERROR, "vscsi: %s, device node not a block or char device", pdev);
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    /* /sys/dev/type/major:minor symlink added in 2.6.27 */
+    sysfs = GCSPRINTF("/sys/dev/%s/%u:%u/device/scsi_device", type,
+                      major(dentry.st_rdev), minor(dentry.st_rdev));
+
+    dirp = opendir(sysfs);
+    if (!dirp) {
+        LOG(ERROR, "vscsi: %s, no major:minor link in sysfs", pdev);
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    while ((de = readdir(dirp))) {
+        if (!strcmp(de->d_name, ".") || !strcmp(de->d_name, ".."))
+            continue;
+
+        if (libxl_device_vscsi_parse_hctl(gc, de->d_name, hctl))
+            continue;
+
+        found = 1;
+        break;
+    }
+    closedir(dirp);
+
+    if (!found) {
+        LOG(ERROR, "vscsi: %s, no h:c:t:l link in sysfs", pdev);
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    rc = 0;
+out:
+    return rc;
+}
diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
index 898e160..b8972f0 100644
--- a/tools/libxl/libxl_netbsd.c
+++ b/tools/libxl/libxl_netbsd.c
@@ -95,3 +95,11 @@ libxl_device_model_version libxl__default_device_model(libxl__gc *gc)
 {
     return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
 }
+
+int libxl_device_vscsi_parse_pdev(libxl__gc *gc, char *pdev, unsigned int *hst,
+                                unsigned int *chn, unsigned int *tgt,
+                                unsigned int *lun)
+{
+
+    return ERROR_NOPARAVIRT;
+}
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 02be466..6ac5da6 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -542,6 +542,29 @@ libxl_device_channel = Struct("device_channel", [
            ])),
 ])
 
+libxl_vscsi_hctl = Struct("vscsi_hctl", [
+    ("hst", uint32),
+    ("chn", uint32),
+    ("tgt", uint32),
+    ("lun", uint32),
+    ])
+
+libxl_vscsi_dev = Struct("vscsi_dev", [
+    ("vscsi_dev_id",     libxl_devid),
+    ("remove",           bool),
+    ("p_devname",        string),
+    ("pdev",             libxl_vscsi_hctl),
+    ("vdev",             libxl_vscsi_hctl),
+    ])
+
+libxl_device_vscsi = Struct("device_vscsi", [
+    ("backend_domid",    libxl_domid),
+    ("devid",            libxl_devid),
+    ("v_hst",            uint32),
+    ("vscsi_devs",       Array(libxl_vscsi_dev, "num_vscsi_devs")),
+    ("feature_host",     bool),
+    ])
+
 libxl_domain_config = Struct("domain_config", [
     ("c_info", libxl_domain_create_info),
     ("b_info", libxl_domain_build_info),
@@ -551,6 +574,7 @@ libxl_domain_config = Struct("domain_config", [
     ("pcidevs", Array(libxl_device_pci, "num_pcidevs")),
     ("vfbs", Array(libxl_device_vfb, "num_vfbs")),
     ("vkbs", Array(libxl_device_vkb, "num_vkbs")),
+    ("vscsis", Array(libxl_device_vscsi, "num_vscsis")),
     ("vtpms", Array(libxl_device_vtpm, "num_vtpms")),
     # a channel manifests as a console with a name,
     # see docs/misc/channels.txt
@@ -585,6 +609,23 @@ libxl_nicinfo = Struct("nicinfo", [
     ("rref_rx", integer),
     ], dir=DIR_OUT)
 
+libxl_vscsiinfo = Struct("vscsiinfo", [
+    ("backend", string),
+    ("backend_id", uint32),
+    ("frontend", string),
+    ("frontend_id", uint32),
+    ("devid", libxl_devid),
+    ("p_devname", string),
+    ("pdev", libxl_vscsi_hctl),
+    ("vdev", libxl_vscsi_hctl),
+    ("vscsi_dev_id", libxl_devid),
+    ("feature_host", bool),
+    ("vscsi_host_state", integer),
+    ("vscsi_dev_state", integer),
+    ("evtch", integer),
+    ("rref", integer),
+    ], dir=DIR_OUT)
+
 libxl_vtpminfo = Struct("vtpminfo", [
     ("backend", string),
     ("backend_id", uint32),
diff --git a/tools/libxl/libxl_types_internal.idl b/tools/libxl/libxl_types_internal.idl
index 5e55685..84c44f5 100644
--- a/tools/libxl/libxl_types_internal.idl
+++ b/tools/libxl/libxl_types_internal.idl
@@ -22,6 +22,7 @@ libxl__device_kind = Enumeration("device_kind", [
     (6, "VKBD"),
     (7, "CONSOLE"),
     (8, "VTPM"),
+    (9, "VSCSI"),
     ])
 
 libxl__console_backend = Enumeration("console_backend", [
diff --git a/tools/libxl/libxl_vscsi.c b/tools/libxl/libxl_vscsi.c
new file mode 100644
index 0000000..b27a586
--- /dev/null
+++ b/tools/libxl/libxl_vscsi.c
@@ -0,0 +1,343 @@
+#include "libxl_osdeps.h" /* must come before any other headers */
+#include "libxl_internal.h"
+
+int libxl_device_vscsi_parse_hctl(libxl__gc *gc, char *str, libxl_vscsi_hctl *hctl)
+{
+    unsigned int hst, chn, tgt, lun;
+
+    if (sscanf(str, "%u:%u:%u:%u", &hst, &chn, &tgt, &lun) != 4)
+        return ERROR_INVAL;
+
+    hctl->hst = hst;
+    hctl->chn = chn;
+    hctl->tgt = tgt;
+    hctl->lun = lun;
+    return 0;
+}
+
+static char *vscsi_trim_string(char *s)
+{
+    unsigned int len;
+
+    while (isspace(*s))
+        s++;
+    len = strlen(s);
+    while (len-- > 1 && isspace(s[len]))
+        s[len] = '\0';
+    return s;
+}
+
+int libxl_device_vscsi_parse(libxl_ctx *ctx, const char *cfg,
+                             libxl_device_vscsi *new_host,
+                             libxl_vscsi_dev *new_dev)
+{
+    GC_INIT(ctx);
+    int rc;
+    char *buf, *pdev, *vdev, *fhost;
+
+    buf = libxl__strdup(gc, cfg);
+
+    pdev = strtok(buf, ",");
+    vdev = strtok(NULL, ",");
+    fhost = strtok(NULL, ",");
+    if (!(pdev && vdev)) {
+        LOG(ERROR, "invalid vscsi= devspec: '%s'\n", cfg);
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    pdev = vscsi_trim_string(pdev);
+    vdev = vscsi_trim_string(vdev);
+
+    if (strncmp(pdev, "/dev/", 5) == 0) {
+        if (libxl_device_vscsi_parse_pdev(gc, pdev, &new_dev->pdev)) {
+            LOG(ERROR, "vscsi: invalid pdev '%s'", pdev);
+            rc = ERROR_INVAL;
+            goto out;
+        }
+    } else if (libxl_device_vscsi_parse_hctl(gc, pdev, &new_dev->pdev)) {
+        LOG(ERROR, "vscsi: invalid '%s', expecting hst:chn:tgt:lun", pdev);
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    new_dev->p_devname = libxl__strdup(NOGC, pdev);
+
+    if (libxl_device_vscsi_parse_hctl(gc, vdev, &new_dev->vdev)) {
+        LOG(ERROR, "vscsi: invalid '%s', expecting hst:chn:tgt:lun", pdev);
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    /* Record group index */
+    new_host->v_hst = new_dev->vdev.hst;
+
+    if (fhost) {
+        fhost = vscsi_trim_string(fhost);
+        new_host->feature_host = strcmp(fhost, "feature-host") == 0;
+        if (!new_host->feature_host) {
+            LOG(ERROR, "vscsi: invalid option '%s', expecting %s", fhost, "feature-host");
+            rc = ERROR_INVAL;
+            goto out;
+        }
+    }
+    rc = 0;
+
+out:
+    GC_FREE;
+    return rc;
+}
+
+void libxl_device_vscsi_append_dev(libxl_ctx *ctx, libxl_device_vscsi *hst,
+                                   libxl_vscsi_dev *dev)
+{
+    GC_INIT(ctx);
+    hst->vscsi_devs = libxl__realloc(NOGC, hst->vscsi_devs,
+                                     sizeof(*dev) * (hst->num_vscsi_devs + 1));
+    libxl_vscsi_dev_init(hst->vscsi_devs + hst->num_vscsi_devs);
+    dev->vscsi_dev_id = hst->num_vscsi_devs;
+    libxl_vscsi_dev_copy(ctx, hst->vscsi_devs + hst->num_vscsi_devs, dev);
+    hst->num_vscsi_devs++;
+    GC_FREE;
+}
+
+int libxl_device_vscsi_get_host(libxl_ctx *ctx, uint32_t domid, const char *cfg, libxl_device_vscsi **vscsi_host)
+{
+    GC_INIT(ctx);
+    libxl_vscsi_dev *new_dev = NULL;
+    libxl_device_vscsi *new_host, *vscsi_hosts = NULL, *tmp;
+    int rc, found_host = -1, i, j;
+    int num_hosts;
+
+    GCNEW(new_host);
+    libxl_device_vscsi_init(new_host);
+
+    GCNEW(new_dev);
+    libxl_vscsi_dev_init(new_dev);
+
+    if (libxl_device_vscsi_parse(ctx, cfg, new_host, new_dev)) {
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    /* FIXME: foreach domain, because pdev is not multiplexed by backend */
+    /* FIXME: other device types do not have the multiplexing issue */
+    /* FIXME: pci can solve it by unbinding the native driver */
+
+    /* Look for existing vscsi_host for given domain */
+    vscsi_hosts = libxl_device_vscsi_list(ctx, domid, &num_hosts);
+    if (vscsi_hosts) {
+        for (i = 0; i < num_hosts; ++i) {
+            for (j = 0; j < vscsi_hosts[i].num_vscsi_devs; j++) {
+                if (vscsi_hosts[i].vscsi_devs[j].pdev.hst == new_dev->pdev.hst &&
+                    vscsi_hosts[i].vscsi_devs[j].pdev.chn == new_dev->pdev.chn &&
+                    vscsi_hosts[i].vscsi_devs[j].pdev.tgt == new_dev->pdev.tgt &&
+                    vscsi_hosts[i].vscsi_devs[j].pdev.lun == new_dev->pdev.lun) {
+                    LOG(ERROR, "Host device '%u:%u:%u:%u' is already in use"
+                        " by guest vscsi specification '%u:%u:%u:%u'.\n",
+                        new_dev->pdev.hst, new_dev->pdev.chn, new_dev->pdev.tgt, new_dev->pdev.lun,
+                        new_dev->vdev.hst, new_dev->vdev.chn, new_dev->vdev.tgt, new_dev->vdev.lun);
+                    rc = ERROR_INVAL;
+                    goto out;
+                }
+            }
+            if (vscsi_hosts[i].v_hst == new_host->v_hst) {
+                found_host = i;
+                break;
+            }
+        }
+    }
+
+    if (found_host == -1) {
+        /* Not found, create new host */
+        new_host->devid = 0;
+        tmp = new_host;
+    } else {
+        tmp = vscsi_hosts + found_host;
+
+        /* Check if the vdev address is already taken */
+        for (i = 0; i < tmp->num_vscsi_devs; ++i) {
+            if (tmp->vscsi_devs[i].vdev.chn == new_dev->vdev.chn &&
+                tmp->vscsi_devs[i].vdev.tgt == new_dev->vdev.tgt &&
+                tmp->vscsi_devs[i].vdev.lun == new_dev->vdev.lun) {
+                fprintf(stderr, "Target vscsi specification '%u:%u:%u:%u' is already taken\n",
+                        new_dev->vdev.hst, new_dev->vdev.chn, new_dev->vdev.tgt, new_dev->vdev.lun);
+                rc = ERROR_INVAL;
+                goto out;
+            }
+        }
+    }
+
+    /* The caller gets a copy along with appended new_dev */
+    *vscsi_host = libxl__malloc(NOGC, sizeof(*new_host));
+    libxl_device_vscsi_init(*vscsi_host);
+    libxl_device_vscsi_copy(ctx, *vscsi_host, tmp);
+    libxl_device_vscsi_append_dev(ctx, *vscsi_host, new_dev);
+
+    rc = 0;
+
+out:
+    if (vscsi_hosts) {
+        for (i = 0; i < num_hosts; ++i){
+            libxl_device_vscsi_dispose(&vscsi_hosts[i]);
+        }
+        free(vscsi_hosts);
+    }
+    libxl_device_vscsi_dispose(new_host);
+    libxl_vscsi_dev_dispose(new_dev);
+    GC_FREE;
+    return rc;
+}
+
+libxl_device_vscsi *libxl_device_vscsi_list(libxl_ctx *ctx, uint32_t domid, int *num)
+{
+    GC_INIT(ctx);
+    libxl_vscsi_dev *v_dev;
+    libxl_device_vscsi *v_hst, *vscsi_hosts = NULL;
+    char *fe_path, *tmp, *c, *p, *v;
+    char **dir, **devs_dir;
+    const char *devs_path, *be_path;
+    int r;
+    bool parsed_ok;
+    unsigned int ndirs = 0, ndevs_dirs = 0, i;
+    unsigned int vscsi_dev_id;
+
+    fe_path = libxl__sprintf(gc, "%s/device/vscsi", libxl__xs_get_dompath(gc, domid));
+    dir = libxl__xs_directory(gc, XBT_NULL, fe_path, &ndirs);
+    /* Nothing to do */
+    if (!(dir && ndirs))
+        goto out;
+
+    /* List of hosts to be returned to the caller */
+    vscsi_hosts = libxl__malloc(NOGC, ndirs * sizeof(*vscsi_hosts));
+
+    /* Fill each host */
+    for (v_hst = vscsi_hosts; v_hst < vscsi_hosts + ndirs; ++v_hst, ++dir) {
+        libxl_device_vscsi_init(v_hst);
+
+        v_hst->devid = atoi(*dir);
+
+        tmp = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/%s/backend-id",
+                             fe_path, *dir));
+        /* FIXME what if xenstore is broken? */
+        if (tmp)
+            v_hst->backend_domid = atoi(tmp);
+
+        be_path = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/%s/backend",
+                                 fe_path, *dir));
+        /* FIXME what if xenstore is broken? */
+        if (be_path) {
+            devs_path = libxl__sprintf(gc, "%s/vscsi-devs", be_path);
+            devs_dir = libxl__xs_directory(gc, XBT_NULL, devs_path, &ndevs_dirs);
+        } else {
+            devs_dir = NULL;
+        }
+
+        if (devs_dir && ndevs_dirs) {
+            v_hst->vscsi_devs = libxl__malloc(NOGC, ndevs_dirs * sizeof(*v_dev));
+            v_hst->num_vscsi_devs = ndevs_dirs;
+            /* Fill each device connected to the host */
+            for (i = 0; i < ndevs_dirs; i++, devs_dir++) {
+                v_dev = &v_hst->vscsi_devs[i];
+                libxl_vscsi_dev_init(v_dev);
+                parsed_ok = false;
+                r = sscanf(*devs_dir, "dev-%u", &vscsi_dev_id);
+                if (r == 1) {
+                    c = libxl__xs_read(gc, XBT_NULL,
+                                         GCSPRINTF("%s/vscsi-devs/dev-%u/p-devname",
+                                         be_path, vscsi_dev_id));
+                    p = libxl__xs_read(gc, XBT_NULL,
+                                         GCSPRINTF("%s/vscsi-devs/dev-%u/p-dev",
+                                         be_path, vscsi_dev_id));
+                    v = libxl__xs_read(gc, XBT_NULL,
+                                          GCSPRINTF("%s/vscsi-devs/dev-%u/v-dev",
+                                          be_path, vscsi_dev_id));
+                    if (c && p && v) {
+                        v_dev->p_devname = libxl__strdup(NOGC, c);
+                        if (libxl_device_vscsi_parse_hctl(gc, p, &v_dev->pdev) == 0 &&
+                            libxl_device_vscsi_parse_hctl(gc, v, &v_dev->vdev) == 0)
+                            parsed_ok = true;
+                        v_dev->vscsi_dev_id = vscsi_dev_id;
+                        v_hst->v_hst = v_dev->vdev.hst;
+                    }
+                }
+
+                if (!parsed_ok) {
+                    /* FIXME what if xenstore is broken? */
+                    LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "%s/scsi-devs/%s failed to parse", be_path, *devs_dir);
+                    continue;
+                }
+            }
+        }
+    }
+
+out:
+    *num = ndirs;
+
+    GC_FREE;
+    return vscsi_hosts;
+}
+
+int libxl_device_vscsi_getinfo(libxl_ctx *ctx, uint32_t domid,
+                               libxl_device_vscsi *vscsi_host,
+                               libxl_vscsi_dev *vscsi_dev,
+                               libxl_vscsiinfo *vscsiinfo)
+{
+    GC_INIT(ctx);
+    char *dompath, *vscsipath;
+    char *val;
+    int rc = ERROR_FAIL;
+
+    libxl_vscsiinfo_init(vscsiinfo);
+    dompath = libxl__xs_get_dompath(gc, domid);
+    vscsiinfo->devid = vscsi_host->devid;
+    libxl_vscsi_hctl_copy(ctx, &vscsiinfo->pdev, &vscsi_dev->pdev);
+    libxl_vscsi_hctl_copy(ctx, &vscsiinfo->vdev, &vscsi_dev->vdev);
+
+    vscsipath = GCSPRINTF("%s/device/vscsi/%d", dompath, vscsiinfo->devid);
+    vscsiinfo->backend = xs_read(ctx->xsh, XBT_NULL,
+                                 GCSPRINTF("%s/backend", vscsipath), NULL);
+    if (!vscsiinfo->backend)
+        goto out;
+    if(!libxl__xs_read(gc, XBT_NULL, vscsiinfo->backend))
+        goto out;
+
+    val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/backend-id", vscsipath));
+    vscsiinfo->backend_id = val ? strtoul(val, NULL, 10) : -1;
+
+    val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/state", vscsipath));
+    vscsiinfo->vscsi_host_state = val ? strtoul(val, NULL, 10) : -1;
+
+    val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/event-channel", vscsipath));
+    vscsiinfo->evtch = val ? strtoul(val, NULL, 10) : -1;
+
+    val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/ring-ref", vscsipath));
+    vscsiinfo->rref = val ? strtoul(val, NULL, 10) : -1;
+
+    vscsiinfo->frontend = xs_read(ctx->xsh, XBT_NULL,
+                                  GCSPRINTF("%s/frontend", vscsiinfo->backend), NULL);
+
+    val = libxl__xs_read(gc, XBT_NULL,
+                         GCSPRINTF("%s/frontend-id", vscsiinfo->backend));
+    vscsiinfo->frontend_id = val ? strtoul(val, NULL, 10) : -1;
+
+    val = libxl__xs_read(gc, XBT_NULL,
+                         GCSPRINTF("%s/vscsi-devs/dev-%u/state",
+                         vscsiinfo->backend, vscsi_dev->vscsi_dev_id));
+    vscsiinfo->vscsi_dev_state = val ? strtoul(val, NULL, 10) : -1;
+
+    rc = 0;
+out:
+    GC_FREE;
+    return rc;
+}
+
+
+
+/*
+ * Local variables:
+ * mode: C
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index 5bc138c..5c82688 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -83,6 +83,9 @@ int main_channellist(int argc, char **argv);
 int main_blockattach(int argc, char **argv);
 int main_blocklist(int argc, char **argv);
 int main_blockdetach(int argc, char **argv);
+int main_vscsiattach(int argc, char **argv);
+int main_vscsilist(int argc, char **argv);
+int main_vscsidetach(int argc, char **argv);
 int main_vtpmattach(int argc, char **argv);
 int main_vtpmlist(int argc, char **argv);
 int main_vtpmdetach(int argc, char **argv);
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 53c16eb..3f0af24 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -985,7 +985,7 @@ static void parse_config_data(const char *config_source,
     const char *buf;
     long l;
     XLU_Config *config;
-    XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms;
+    XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms, *vscsis;
     XLU_ConfigList *channels, *ioports, *irqs, *iomem, *viridian;
     int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian;
     int pci_power_mgmt = 0;
@@ -1488,6 +1488,59 @@ static void parse_config_data(const char *config_source,
         }
     }
 
+    if (!xlu_cfg_get_list(config, "vscsi", &vscsis, 0, 0)) {
+        int num_vscsi_items = 0;
+        d_config->num_vscsis = 0;
+        d_config->vscsis = NULL;
+        while ((buf = xlu_cfg_get_listitem (vscsis, num_vscsi_items)) != NULL) {
+            libxl_vscsi_dev v_dev = { };
+            libxl_device_vscsi *tmp, v_hst = { };
+            bool hst_found = false;
+
+            /*
+             * #1: parse the devspec and place it in temporary host+dev part
+             * #2: find existing vscsi_host with number v_hst
+             *     if found, append the vscsi_dev to this vscsi_host
+             * #3: otherwise, create new vscsi_host and append vscsi_dev
+             * Note: v_hst does not represent the index named "num_vscsis",
+             *       it is a private index used just in the config file
+             */
+            libxl_device_vscsi_init(&v_hst);
+            libxl_vscsi_dev_init(&v_dev);
+
+            if (libxl_device_vscsi_parse(ctx, buf, &v_hst, &v_dev))
+                exit (-1);
+
+            if (d_config->num_vscsis) {
+                for (i = 0; i < d_config->num_vscsis; i++) {
+                    if (d_config->vscsis[i].v_hst == v_hst.v_hst) {
+                        tmp = &d_config->vscsis[i];
+                        libxl_device_vscsi_append_dev(ctx, tmp, &v_dev);
+                        hst_found = true;
+                        break;
+                    }
+                }
+            }
+
+            if (!hst_found || !d_config->num_vscsis) {
+                d_config->vscsis = realloc(d_config->vscsis, sizeof(v_hst) * (d_config->num_vscsis + 1));
+                tmp = &d_config->vscsis[d_config->num_vscsis];
+                libxl_device_vscsi_init(tmp);
+
+                v_hst.devid = d_config->num_vscsis;
+                libxl_device_vscsi_copy(ctx, tmp, &v_hst);
+
+                libxl_device_vscsi_append_dev(ctx, tmp, &v_dev);
+
+                d_config->num_vscsis++;
+            }
+
+            libxl_vscsi_dev_dispose(&v_dev);
+            libxl_device_vscsi_dispose(&v_hst);
+            num_vscsi_items++;
+        }
+    }
+
     if (!xlu_cfg_get_list(config, "vtpm", &vtpms, 0, 0)) {
         d_config->num_vtpms = 0;
         d_config->vtpms = NULL;
@@ -6439,6 +6492,200 @@ int main_blockdetach(int argc, char **argv)
     return rc;
 }
 
+int main_vscsiattach(int argc, char **argv)
+{
+    uint32_t domid;
+    int opt, rc;
+    libxl_device_vscsi *vscsi_host = NULL;
+    char *cfg = NULL, *feat_buf = NULL;
+
+    SWITCH_FOREACH_OPT(opt, "", NULL, "scsi-attach", 1) {
+        /* No options */
+    }
+
+    if (argc < 4 || argc > 5) {
+        help("scsi-attach");
+        return 1;
+    }
+
+    if (libxl_domain_qualifier_to_domid(ctx, argv[optind], &domid) < 0) {
+        fprintf(stderr, "%s is an invalid domain identifier\n", argv[optind]);
+        return 1;
+    }
+
+    optind++;
+
+    if (argc == 5) {
+        if (asprintf(&feat_buf, ",%s", argv[4]) < 0) {
+            perror("asprintf");
+            return 1;
+        }
+    }
+
+    if (asprintf(&cfg, "%s,%s%s", argv[2], argv[3], feat_buf ?: "") < 0) {
+        perror("asprintf");
+        rc = 1;
+        goto out;;
+    }
+
+    /* Parse config string and store result */
+    rc = libxl_device_vscsi_get_host(ctx, domid, cfg, &vscsi_host);
+    if (rc < 0)
+        goto out;
+
+    if (dryrun_only) {
+        char *json = libxl_device_vscsi_to_json(ctx, vscsi_host);
+        printf("vscsi: %s\n", json);
+        free(json);
+        if (ferror(stdout) || fflush(stdout)) { perror("stdout"); exit(-1); }
+        rc = 0;
+        goto out;
+    }
+
+    /* Finally add the device */
+    if (libxl_device_vscsi_add(ctx, domid, vscsi_host, NULL)) {
+        fprintf(stderr, "libxl_device_vscsi_add failed.\n");
+        rc = 1;
+        goto out;
+    }
+
+    rc = 0;
+out:
+    if (vscsi_host)
+        libxl_device_vscsi_dispose(vscsi_host);
+    free(vscsi_host);
+    free(cfg);
+    free(feat_buf);
+    return rc;
+}
+
+int main_vscsilist(int argc, char **argv)
+{
+    int opt;
+    uint32_t domid;
+    libxl_device_vscsi *vscsi_hosts;
+    libxl_vscsiinfo vscsiinfo;
+    int num_hosts, h, d;
+
+    SWITCH_FOREACH_OPT(opt, "", NULL, "scsi-list", 1) {
+        /* No options */
+    }
+    if (argc < 2) {
+        help("scsi-list");
+        return 1;
+    }
+
+    /*      Idx  BE  state host p_hst v_hst state */
+    printf("%-3s %-3s %-5s %-5s %-10s %-10s %-5s\n",
+           "Idx", "BE", "state", "host", "phy-hctl", "vir-hctl", "devstate");
+    for (argv += optind, argc -= optind; argc > 0; --argc, ++argv) {
+        if (libxl_domain_qualifier_to_domid(ctx, *argv, &domid) < 0) {
+            fprintf(stderr, "%s is an invalid domain identifier\n", *argv);
+            continue;
+        }
+        if (!(vscsi_hosts = libxl_device_vscsi_list(ctx, domid, &num_hosts))) {
+            continue;
+        }
+        for (h = 0; h < num_hosts; ++h) {
+            for (d = 0; d < vscsi_hosts[h].num_vscsi_devs; d++) {
+                if (!libxl_device_vscsi_getinfo(ctx, domid, &vscsi_hosts[h], &vscsi_hosts[h].vscsi_devs[d], &vscsiinfo)) {
+                    char pdev[64], vdev[64];
+                    snprintf(pdev, sizeof(pdev), "%u:%u:%u:%u",
+                             vscsiinfo.pdev.hst, vscsiinfo.pdev.chn, vscsiinfo.pdev.tgt, vscsiinfo.pdev.lun);
+                    snprintf(vdev, sizeof(vdev), "%u:%u:%u:%u",
+                             vscsiinfo.vdev.hst, vscsiinfo.vdev.chn, vscsiinfo.vdev.tgt, vscsiinfo.vdev.lun);
+                    /*      Idx  BE  state Sta */
+                    printf("%-3d %-3d %-5d %-5d %-10s %-10s %d\n",
+                           vscsiinfo.devid,
+                           vscsiinfo.backend_id,
+                           vscsiinfo.vscsi_host_state,
+                           vscsiinfo.backend_id,
+                           pdev, vdev,
+                           vscsiinfo.vscsi_dev_state);
+
+                    libxl_vscsiinfo_dispose(&vscsiinfo);
+                }
+            }
+            libxl_device_vscsi_dispose(&vscsi_hosts[h]);
+        }
+        free(vscsi_hosts);
+
+    }
+
+    return 0;
+}
+
+int main_vscsidetach(int argc, char **argv)
+{
+    int opt;
+    libxl_vscsi_dev v_dev = { }, *vd;
+    libxl_device_vscsi v_hst = { }, *vh;
+    libxl_device_vscsi *vscsi_hosts;
+    char *tmp = NULL, *dom = argv[1], *vdev = argv[2];
+    uint32_t domid;
+    int num_hosts, h, d, found = 0;
+
+    SWITCH_FOREACH_OPT(opt, "", NULL, "scsi-detach", 1) {
+        /* No options */
+    }
+
+    if (argc < 3) {
+        help("scsi-detach");
+        return 1;
+    }
+
+    if (libxl_domain_qualifier_to_domid(ctx, dom, &domid) < 0) {
+        fprintf(stderr, "%s is an invalid domain identifier\n", dom);
+        return 1;
+    }
+
+    vscsi_hosts = libxl_device_vscsi_list(ctx, domid, &num_hosts);
+    if (!vscsi_hosts)
+        return 0;
+
+    /* Create a dummy cfg */
+    if (asprintf(&tmp, "0:0:0:0,%s", vdev) < 0) {
+        perror("asprintf");
+        goto done;
+    }
+
+    libxl_vscsi_dev_init(&v_dev);
+    libxl_device_vscsi_init(&v_hst);
+    if (libxl_device_vscsi_parse(ctx, tmp, &v_hst, &v_dev))
+        goto done;
+
+    for (h = 0; h < num_hosts; ++h) {
+        vh = &vscsi_hosts[h];
+        for (d = 0; !found && d < vh->num_vscsi_devs; d++) {
+#define CMP(member) (vd->vdev.member == v_dev.vdev.member)
+            vd = &vh->vscsi_devs[d];
+            if (CMP(hst) && CMP(chn) && CMP(tgt) && CMP(lun)) {
+                if (vh->num_vscsi_devs > 1) {
+                    vd->remove = true;
+                    if (libxl_device_vscsi_add(ctx, domid, vh, 0)) {
+                        fprintf(stderr, "libxl_device_vscsi_remove failed.\n");
+                        goto done;
+                    }
+                } else {
+                    libxl_device_vscsi_remove(ctx, domid, vh, 0);
+                }
+                found = 1;
+            }
+#undef CMP
+        }
+    }
+    if (!found)
+        fprintf(stderr, "%s(%u) vdev %s does not exist in domain %s\n", __func__, __LINE__, vdev, dom);
+done:
+    if (vscsi_hosts) {
+        for (h = 0; h < num_hosts; ++h)
+            libxl_device_vscsi_dispose(&vscsi_hosts[h]);
+        free(vscsi_hosts);
+    }
+    free(tmp);
+    return !found;
+}
+
 int main_vtpmattach(int argc, char **argv)
 {
     int opt;
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 4b30d3d..b727684 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -365,6 +365,21 @@ struct cmd_spec cmd_table[] = {
       "Destroy a domain's virtual block device",
       "<Domain> <DevId>",
     },
+    { "scsi-attach",
+      &main_vscsiattach, 1, 1,
+      "Attach a dom0 SCSI device to a domain.",
+      "<Domain> <PhysDevice> <VirtDevice>",
+    },
+    { "scsi-list",
+      &main_vscsilist, 0, 0,
+      "List all dom0 SCSI devices currently attached to a domain.",
+      "<Domain(s)>",
+    },
+    { "scsi-detach",
+      &main_vscsidetach, 0, 1,
+      "Detach a specified SCSI device from a domain.",
+      "<Domain> <VirtDevice>",
+    },
     { "vtpm-attach",
       &main_vtpmattach, 1, 1,
       "Create a new virtual TPM device",

Re: [PATCH 1/4] vscsiif.h: remove reference to WWN

[Jan Beulich]

>>> On 02.03.15 at 11:15, <olaf@aepfle.de> wrote:
> The code in xend, xenlinux and pvops 3.18 expects the h:c:t:l notation.
> Remove incorrect reference to WWN because it was never supported. And
> its not clear how it could be supported. p-devname can be used to store
> a config string.

I'm not sure - even if a particular implementation doesn't support
certain functionality, I would say the interface definition can still
name this as a possibility.

Jan

> --- a/xen/include/public/io/vscsiif.h
> +++ b/xen/include/public/io/vscsiif.h
> @@ -58,9 +58,8 @@
>   * p-dev
>   *      Values:         string
>   *
> - *      A string specifying the backend device: either a 4-tuple "h:c:t:l"
> - *      (host, controller, target, lun, all integers), or a WWN (e.g.
> - *      "naa.60014054ac780582").
> + *      A string specifying the backend device in form of a 4-tuple "h:c:t:l"
> + *      (host, controller, target, lun, all integers).
>   *
>   * v-dev
>   *      Values:         string

Re: [PATCH 3/4] docs: add pvscsi.txt

[Jan Beulich]

>>> On 02.03.15 at 11:16, <olaf@aepfle.de> wrote:
> +== Config Format ==
> +
> +The domU.cfg syntax is "vscsi=[ 'pdev,vdev[,option]' ]". pdev is the dom0 device
> +in either /dev/$device or pHOST:CHANNEL:TARGET:LUN notation.  vdev is the domU

Since /dev/... is OS (Linux) specific, I think the text should explicitly
say so. For the other notation I simply don't know how OS-agnostic
it is...

Jan

> +device in vHOST:CHANNEL:TARGET:LUN notation. 'option' lists additional flags.
> +Currently only the value "feature-host" is recognized. SCSI command emulation in
> +backend driver is bypassed when "feature-host" is specified.

Re: [PATCH 1/4] vscsiif.h: remove reference to WWN

[Andrew Cooper]

On 02/03/15 10:46, Jan Beulich wrote:
>>>> On 02.03.15 at 11:15, <olaf@aepfle.de> wrote:
>> The code in xend, xenlinux and pvops 3.18 expects the h:c:t:l notation.
>> Remove incorrect reference to WWN because it was never supported. And
>> its not clear how it could be supported. p-devname can be used to store
>> a config string.
> I'm not sure - even if a particular implementation doesn't support
> certain functionality, I would say the interface definition can still
> name this as a possibility.

It would be misleading to suggest it is available if it is not
implemented.  Furthermore, Olaf's comment suggests that it cant be
supported with this key, at which point the reference should absolutely
be removed.

~Andrew

Re: [PATCH 1/4] vscsiif.h: remove reference to WWN

[Jürgen Groß]

On 03/02/2015 11:50 AM, Andrew Cooper wrote:
> On 02/03/15 10:46, Jan Beulich wrote:
>>>>> On 02.03.15 at 11:15, <olaf@aepfle.de> wrote:
>>> The code in xend, xenlinux and pvops 3.18 expects the h:c:t:l notation.
>>> Remove incorrect reference to WWN because it was never supported. And
>>> its not clear how it could be supported. p-devname can be used to store
>>> a config string.
>> I'm not sure - even if a particular implementation doesn't support
>> certain functionality, I would say the interface definition can still
>> name this as a possibility.
>
> It would be misleading to suggest it is available if it is not
> implemented.  Furthermore, Olaf's comment suggests that it cant be
> supported with this key, at which point the reference should absolutely
> be removed.

The pvops backend supports this variant.


Juergen

Re: [PATCH 1/4] vscsiif.h: remove reference to WWN

[Jan Beulich]

>>> On 02.03.15 at 11:50, <andrew.cooper3@citrix.com> wrote:
> On 02/03/15 10:46, Jan Beulich wrote:
>>>>> On 02.03.15 at 11:15, <olaf@aepfle.de> wrote:
>>> The code in xend, xenlinux and pvops 3.18 expects the h:c:t:l notation.
>>> Remove incorrect reference to WWN because it was never supported. And
>>> its not clear how it could be supported. p-devname can be used to store
>>> a config string.
>> I'm not sure - even if a particular implementation doesn't support
>> certain functionality, I would say the interface definition can still
>> name this as a possibility.
> 
> It would be misleading to suggest it is available if it is not
> implemented.  Furthermore, Olaf's comment suggests that it cant be
> supported with this key, at which point the reference should absolutely
> be removed.

"It's not clear" != "can't" (at least in my understanding of the words).
And no, as said I don't think the interface description should be in any
way tailored towards a specific implementation. As that aspect is a
host local thing only anyway, amending the text to say "in host
specific ways" or "as supported by the particular backend
implementation" or some such may be desirable.

Jan

Re: [PATCH 1/4] vscsiif.h: remove reference to WWN

[Juergen Gross]

On 03/02/2015 11:15 AM, Olaf Hering wrote:
> The code in xend, xenlinux and pvops 3.18 expects the h:c:t:l notation.
> Remove incorrect reference to WWN because it was never supported. And
> its not clear how it could be supported. p-devname can be used to store
> a config string.

The pvops backend driver should support the WWN notation. In case it
isn't working this is a bug which should be repaired.

I implemented the WWn version before adding the possibility to use
h:c:t:l due to compatibility with xend...

So: NAK.


Juergen

>
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Tim Deegan <tim@xen.org>
> ---
>   xen/include/public/io/vscsiif.h | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/xen/include/public/io/vscsiif.h b/xen/include/public/io/vscsiif.h
> index 7a1db05..373ca3f 100644
> --- a/xen/include/public/io/vscsiif.h
> +++ b/xen/include/public/io/vscsiif.h
> @@ -58,9 +58,8 @@
>    * p-dev
>    *      Values:         string
>    *
> - *      A string specifying the backend device: either a 4-tuple "h:c:t:l"
> - *      (host, controller, target, lun, all integers), or a WWN (e.g.
> - *      "naa.60014054ac780582").
> + *      A string specifying the backend device in form of a 4-tuple "h:c:t:l"
> + *      (host, controller, target, lun, all integers).
>    *
>    * v-dev
>    *      Values:         string
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

Re: [PATCH 1/4] vscsiif.h: remove reference to WWN

[Andrew Cooper]

On 02/03/15 10:55, Jürgen Groß wrote:
> On 03/02/2015 11:50 AM, Andrew Cooper wrote:
>> On 02/03/15 10:46, Jan Beulich wrote:
>>>>>> On 02.03.15 at 11:15, <olaf@aepfle.de> wrote:
>>>> The code in xend, xenlinux and pvops 3.18 expects the h:c:t:l
>>>> notation.
>>>> Remove incorrect reference to WWN because it was never supported. And
>>>> its not clear how it could be supported. p-devname can be used to
>>>> store
>>>> a config string.
>>> I'm not sure - even if a particular implementation doesn't support
>>> certain functionality, I would say the interface definition can still
>>> name this as a possibility.
>>
>> It would be misleading to suggest it is available if it is not
>> implemented.  Furthermore, Olaf's comment suggests that it cant be
>> supported with this key, at which point the reference should absolutely
>> be removed.
>
> The pvops backend supports this variant.

In which case it really should stay.

~Andrew

Re: [PATCH 3/4] docs: add pvscsi.txt

[Juergen Gross]

On 03/02/2015 11:16 AM, Olaf Hering wrote:
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Tim Deegan <tim@xen.org>
> ---
>   docs/misc/pvscsi.txt | 147 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 147 insertions(+)
>
> diff --git a/docs/misc/pvscsi.txt b/docs/misc/pvscsi.txt
> new file mode 100644
> index 0000000..2af050f
> --- /dev/null
> +++ b/docs/misc/pvscsi.txt
> @@ -0,0 +1,147 @@
> +. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .
> +PVSCSI
> +
> +== Overview ==
> +
> +PVSCSI allows to assing a "physical" SCSI device from dom0 to a domU. The device
> +is not limited to be a native SCSI device. Everything visible as SCSI device in
> +dom0 can be used. Currently PVSCSI is only available in Linux dom0 and domU.
> +
> +== TODO ===
> +
> +How to do live migration?
> + - pdev will likely be evaluated again on the target host if it came from
> +   domU.cfg. But what about pdev from 'xl scsi-attach pdev vdev'? Its required
> +   to adjust h:c:t:l on the target host.

Use WWN notation? :-)

This was one of the reason the backend should support it.

> +
> +Implement checks for duplicate pdev assignments?
> + - Not sure if SCSI devices can be shared. Will the state be consistent during
> +   concurrent access? Should the host admin assume cooperative access from
> +   multiple domUs?

SCSI does support device sharing. That's the reason for the existence of
SCSI commands like "reserve" and "release".


Juergen

Re: [PATCH 4/4] libxl: add support for vscsi

[Olaf Hering]

On Mon, Mar 02, Olaf Hering wrote:

> +++ b/tools/libxl/libxl_types.idl

> +libxl_vscsi_hctl = Struct("vscsi_hctl", [
> +    ("hst", uint32),
> +    ("chn", uint32),
> +    ("tgt", uint32),
> +    ("lun", uint32),
> +    ])
> +
> +libxl_vscsi_dev = Struct("vscsi_dev", [
> +    ("vscsi_dev_id",     libxl_devid),
> +    ("remove",           bool),
> +    ("p_devname",        string),
> +    ("pdev",             libxl_vscsi_hctl),
> +    ("vdev",             libxl_vscsi_hctl),
> +    ])
> +
> +libxl_device_vscsi = Struct("device_vscsi", [
> +    ("backend_domid",    libxl_domid),
> +    ("devid",            libxl_devid),
> +    ("v_hst",            uint32),
> +    ("vscsi_devs",       Array(libxl_vscsi_dev, "num_vscsi_devs")),
> +    ("feature_host",     bool),
> +    ])

IanC, you made a comment about "vscsi_dev_id" in the pastebin.com
comment about libxl_vscsi_hctl:
'- devid is just "devid" in all other places, not "TYPE_dev_id".'

Its used as <domid>/<vhost>/vscsi-devs/dev-<vscsi_dev_id>/.  What name
should be used to reference the devices? Also just "devid" even if it
refers to devices "private" to the vscsi host?

Olaf

Re: [PATCH 4/4] libxl: add support for vscsi

[Olaf Hering]

On Mon, Mar 02, Olaf Hering wrote:

> +libxl_device_vscsi *libxl_device_vscsi_list(libxl_ctx *ctx, uint32_t domid, int *num)

> +    fe_path = libxl__sprintf(gc, "%s/device/vscsi", libxl__xs_get_dompath(gc, domid));

IanC,

last year you made a comment that this path should not be trusted
because its guest controlled. Other code does not seem to care much
about that fact. What should be done here to have a robust parser?

Olaf

Re: [PATCH 4/4] libxl: add support for vscsi

[Ian Campbell]

On Mon, 2015-03-02 at 15:20 +0100, Olaf Hering wrote:
> On Mon, Mar 02, Olaf Hering wrote:
> 
> > +++ b/tools/libxl/libxl_types.idl
> 
> > +libxl_vscsi_hctl = Struct("vscsi_hctl", [
> > +    ("hst", uint32),
> > +    ("chn", uint32),
> > +    ("tgt", uint32),
> > +    ("lun", uint32),
> > +    ])
> > +
> > +libxl_vscsi_dev = Struct("vscsi_dev", [
> > +    ("vscsi_dev_id",     libxl_devid),
> > +    ("remove",           bool),
> > +    ("p_devname",        string),
> > +    ("pdev",             libxl_vscsi_hctl),
> > +    ("vdev",             libxl_vscsi_hctl),
> > +    ])
> > +
> > +libxl_device_vscsi = Struct("device_vscsi", [
> > +    ("backend_domid",    libxl_domid),
> > +    ("devid",            libxl_devid),
> > +    ("v_hst",            uint32),
> > +    ("vscsi_devs",       Array(libxl_vscsi_dev, "num_vscsi_devs")),
> > +    ("feature_host",     bool),
> > +    ])
> 
> IanC, you made a comment about "vscsi_dev_id" in the pastebin.com
> comment about libxl_vscsi_hctl:
> '- devid is just "devid" in all other places, not "TYPE_dev_id".'

> Its used as <domid>/<vhost>/vscsi-devs/dev-<vscsi_dev_id>/.  What name
> should be used to reference the devices? Also just "devid" even if it
> refers to devices "private" to the vscsi host?

Not sure. PCI uses vdevfn, which by extension would be vdevid here
(assuming I've understood what you are suggesting correctly and
therefore the analogy is valid).

Or could it be the case that there is no need to expose this value to
the user in the first place and libxl could just make up the index
numbers internally? Why would they want to configure it or retrieve it?

Is it actually the same as the offset into the vscsi_devs array?

How is it intended to be different from the vdev field?

Ian.

Re: [PATCH 1/4] vscsiif.h: remove reference to WWN

[Olaf Hering]

On Mon, Mar 02, Juergen Gross wrote:

> On 03/02/2015 11:15 AM, Olaf Hering wrote:
> >The code in xend, xenlinux and pvops 3.18 expects the h:c:t:l notation.
> >Remove incorrect reference to WWN because it was never supported. And
> >its not clear how it could be supported. p-devname can be used to store
> >a config string.
> 
> The pvops backend driver should support the WWN notation. In case it
> isn't working this is a bug which should be repaired.
> 
> I implemented the WWn version before adding the possibility to use
> h:c:t:l due to compatibility with xend...

This requires changes to the libxl parser. I was not aware of that pvops
uses such notation.

Olaf

Re: [PATCH 1/4] vscsiif.h: remove reference to WWN

[Olaf Hering]

On Mon, Mar 02, Olaf Hering wrote:

> On Mon, Mar 02, Juergen Gross wrote:
> 
> > On 03/02/2015 11:15 AM, Olaf Hering wrote:
> > >The code in xend, xenlinux and pvops 3.18 expects the h:c:t:l notation.
> > >Remove incorrect reference to WWN because it was never supported. And
> > >its not clear how it could be supported. p-devname can be used to store
> > >a config string.
> > 
> > The pvops backend driver should support the WWN notation. In case it
> > isn't working this is a bug which should be repaired.
> > 
> > I implemented the WWn version before adding the possibility to use
> > h:c:t:l due to compatibility with xend...
> 
> This requires changes to the libxl parser. I was not aware of that pvops
> uses such notation.

And in fact it expects a colon in scsiback_add_translation_entry(). So
my patch is still valid. If another implementation comes along which
requires something else than h:c:t:l the comment will be adjusted.
But, maybe the comment is just incomplete and the WWN example lacks the
colon.

Olaf

Re: [PATCH 1/4] vscsiif.h: remove reference to WWN

[Juergen Gross]

On 03/02/2015 03:40 PM, Olaf Hering wrote:
> On Mon, Mar 02, Olaf Hering wrote:
>
>> On Mon, Mar 02, Juergen Gross wrote:
>>
>>> On 03/02/2015 11:15 AM, Olaf Hering wrote:
>>>> The code in xend, xenlinux and pvops 3.18 expects the h:c:t:l notation.
>>>> Remove incorrect reference to WWN because it was never supported. And
>>>> its not clear how it could be supported. p-devname can be used to store
>>>> a config string.
>>>
>>> The pvops backend driver should support the WWN notation. In case it
>>> isn't working this is a bug which should be repaired.
>>>
>>> I implemented the WWn version before adding the possibility to use
>>> h:c:t:l due to compatibility with xend...
>>
>> This requires changes to the libxl parser. I was not aware of that pvops
>> uses such notation.
>
> And in fact it expects a colon in scsiback_add_translation_entry(). So
> my patch is still valid. If another implementation comes along which
> requires something else than h:c:t:l the comment will be adjusted.
> But, maybe the comment is just incomplete and the WWN example lacks the
> colon.

I was just about to write this. :-)

It should be: WWN:l (with l being the LUN).

To be even more precise: The backend expects name:l. The name is
checked against the WWN of the related device entry under
/sys/kernel/config/target/core or against the contents of the alias
entry under /sys/kernel/config/target/xen-pvscsi/*/tpgt_0/param/alias,
so any string could be used as long as there is a match between the
alias and the xenstore entry. "l" is in both cases the LUN number.


Juergen

Re: [PATCH 4/4] libxl: add support for vscsi

[Olaf Hering]

On Mon, Mar 02, Ian Campbell wrote:

> On Mon, 2015-03-02 at 15:20 +0100, Olaf Hering wrote:
> > Its used as <domid>/<vhost>/vscsi-devs/dev-<vscsi_dev_id>/.  What name
> > should be used to reference the devices? Also just "devid" even if it
> > refers to devices "private" to the vscsi host?
> Not sure. PCI uses vdevfn, which by extension would be vdevid here
> (assuming I've understood what you are suggesting correctly and
> therefore the analogy is valid).

After a quick grep its not clear what vdevfn represents. Will study that
code later. libxl_device_pci indicates that single_host:many_devices is
not used for PCI.

> Or could it be the case that there is no need to expose this value to
> the user in the first place and libxl could just make up the index
> numbers internally? Why would they want to configure it or retrieve it?

I think the actual number is internal to the backend/frontend.
Lets see -- the layout in the backend node is:

 <domid>/<N>/vscsi-devs/dev-<M>/{p-dev,v-dev,state}

N is libxl_device_vscsi->devid, just as every other backend.
M is libxl_device_vscsi->vscsi_devs->vscsi_dev_id.
One N can have several M (single host, many devices) which is different
to all other backends.

I think the issue is that if two M exists and 'scsi-detach' removes M==0
the next time something walks <domid>/<N>/vscsi-devs/ it will find just
a single entry with M==1. So this number has to be recorded.

> Is it actually the same as the offset into the vscsi_devs array?

Per the above it can not be the offset, the code has to handle holes.

> How is it intended to be different from the vdev field?

The vdev field is used to group several devices into a virtual
controller. The HOST in HOST:CHN:TGT:LUN matters. In this sense the
vscsi_dev_id is unrelated to the vdev field.

Olaf

Re: [PATCH 3/4] docs: add pvscsi.txt

[Olaf Hering]

On Mon, Mar 02, Juergen Gross wrote:

> On 03/02/2015 11:16 AM, Olaf Hering wrote:
> >+How to do live migration?
> >+ - pdev will likely be evaluated again on the target host if it came from
> >+   domU.cfg. But what about pdev from 'xl scsi-attach pdev vdev'? Its required
> >+   to adjust h:c:t:l on the target host.
> Use WWN notation? :-)
> 
> This was one of the reason the backend should support it.

How does it handle WWN internally, given that
scsiback_add_translation_entry does not seem to understand it?

> >+Implement checks for duplicate pdev assignments?
> >+ - Not sure if SCSI devices can be shared. Will the state be consistent during
> >+   concurrent access? Should the host admin assume cooperative access from
> >+   multiple domUs?
> SCSI does support device sharing. That's the reason for the existence of
> SCSI commands like "reserve" and "release".

In this case the code which checks pdev usage in
libxl_device_vscsi_get_host can just be removed.


Olaf

Re: [PATCH 3/4] docs: add pvscsi.txt

[Juergen Gross]

On 03/02/2015 04:14 PM, Olaf Hering wrote:
> On Mon, Mar 02, Juergen Gross wrote:
>
>> On 03/02/2015 11:16 AM, Olaf Hering wrote:
>>> +How to do live migration?
>>> + - pdev will likely be evaluated again on the target host if it came from
>>> +   domU.cfg. But what about pdev from 'xl scsi-attach pdev vdev'? Its required
>>> +   to adjust h:c:t:l on the target host.
>> Use WWN notation? :-)
>>
>> This was one of the reason the backend should support it.
>
> How does it handle WWN internally, given that
> scsiback_add_translation_entry does not seem to understand it?

It does understand it. WWN:l is okay. A pscsi device can have multiple
LUNs. The WWN is per target, not per LUN. The target infrastructure
allows far more complex configurations (e.g. specific LUN mappings,
access control for each LUN).

>
>>> +Implement checks for duplicate pdev assignments?
>>> + - Not sure if SCSI devices can be shared. Will the state be consistent during
>>> +   concurrent access? Should the host admin assume cooperative access from
>>> +   multiple domUs?
>> SCSI does support device sharing. That's the reason for the existence of
>> SCSI commands like "reserve" and "release".
>
> In this case the code which checks pdev usage in
> libxl_device_vscsi_get_host can just be removed.

Hmm, maybe issuing a warning would be a sensible thing to do?


Juergen

Re: [PATCH 3/4] docs: add pvscsi.txt

[Olaf Hering]

On Mon, Mar 02, Juergen Gross wrote:

> On 03/02/2015 04:14 PM, Olaf Hering wrote:
> >In this case the code which checks pdev usage in
> >libxl_device_vscsi_get_host can just be removed.
> Hmm, maybe issuing a warning would be a sensible thing to do?

This gets us back to square #1 with the question how to do
 foreach domid ; foreach vscsi

If domains are started parallel a simple check will miss devices.
Perhaps not an issue if its just a warning message.


Olaf