* [PATCH] libbxl: add support for pvscsi, iteration 1
@ 2014-04-30 11:03 Olaf Hering
2014-05-02 14:36 ` Olaf Hering
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Olaf Hering @ 2014-04-30 11:03 UTC (permalink / raw)
To: xen-devel; +Cc: Ondrej Holecek, Olaf Hering, Ian Jackson, Ian Campbell
Add support for vscsi= in domU.cfg, add new xl commands scsi-attach,
scsi-list, scsi-detach. The syntax follows what xend understood.
pvscsi provides a dom0 SCSI device as-is to a domU. The backend and
frontend drivers can be found in xen-linux, or its forward-ported
variants. This patch was tested with an openSUSE dom0 and a SLES12
guest. Any openSUSE/SLE11/SLE12 dom0/domU combination will work.
The domU.cfg syntax is "vscsi=[ 'pdev,vdev[,option]' ]". pdev is the
dom0 device in either /dev/$device or HOST:CHANNEL:TARGET:LUN notation.
vdev is the domU device in HOST:CHANNEL:TARGET:LUN notation.
xl scsi-attach will attach a SCSI device at runtime.
xl scsi-detach will remove a SCSI drivce at runtime.
xl scsi-list lists SCSI devices for a list of domains.
The backend driver uses a single_host:many_devices notation to manage
domU devices. The xenstore layout looks like this:
/local/domain/0/backend/vscsi/<domid>/<vhost>/feature-host = "0"
/local/domain/0/backend/vscsi/<domid>/<vhost>/frontend = "/local/domain/<domid>/device/vscsi/0"
/local/domain/0/backend/vscsi/<domid>/<vhost>/frontend-id = "<domid>"
/local/domain/0/backend/vscsi/<domid>/<vhost>/online = "1"
/local/domain/0/backend/vscsi/<domid>/<vhost>/state = "4"
/local/domain/0/backend/vscsi/<domid>/<vhost>/vscsi-devs/dev-0/p-dev = "8:0:2:1"
/local/domain/0/backend/vscsi/<domid>/<vhost>/vscsi-devs/dev-0/state = "4"
/local/domain/0/backend/vscsi/<domid>/<vhost>/vscsi-devs/dev-0/v-dev = "0:0:0:0"
/local/domain/0/backend/vscsi/<domid>/<vhost>/vscsi-devs/dev-1/p-dev = "8:0:2:2"
/local/domain/0/backend/vscsi/<domid>/<vhost>/vscsi-devs/dev-1/state = "4"
/local/domain/0/backend/vscsi/<domid>/<vhost>/vscsi-devs/dev-1/v-dev = "0:0:1:0"
/local/domain/<domid>/device/vscsi/<vhost>/backend = "/local/domain/0/backend/vscsi/<domid>/<vhost>"
/local/domain/<domid>/device/vscsi/<vhost>/backend-id = "0"
/local/domain/<domid>/device/vscsi/<vhost>/event-channel = "20"
/local/domain/<domid>/device/vscsi/<vhost>/ring-ref = "43"
/local/domain/<domid>/device/vscsi/<vhost>/state = "4"
/local/domain/<domid>/device/vscsi/<vhost>/vscsi-devs/dev-0/state = "4"
/local/domain/<domid>/device/vscsi/<vhost>/vscsi-devs/dev-1/state = "4"
This is a challenge for libxl because libxl currently handles only
single_host:single_device in its device helper functions. Due to this
limitation in libxl, scsi-detach will remove the whole virtual scsi host
with all its virtual devices, instead of just the requested single
virtual device. scsi-attach can cope with this limitation because it
could write the required files directly into xenstore to attach yet
another virtual scsi device to an existing virtual scsi host. However,
this functionality is currently not implemented.
To support the pdev notation '/dev/$device' it is required to parse
sysfs to translate the path to HST:CHN:TGT:LUN notation. In its current
variant /sys/dev/ is used, which is available since at least Linux 3.0.
Support for dom0 kernels which lack this interface will be added later.
This is a draft patch to get it out of the door, to get help with
solving the libxl limitation above, and of course for public code review.
In this first iteration:
- the domU.cfg parser for vscsi=[] is functional.
- xl scsi-list will print output similar to its xm counter part.
- xl scsi-detach will remove entire scsi hosts, unlike its xm counter part.
- xl scsi-attach is not yet implemented.
- coding style may be way off in some parts of the new code.
Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Campbell <Ian.Campbell@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ondrej Holecek <oholecek@suse.com>
---
docs/man/xl.cfg.pod.5 | 30 +++
docs/man/xl.pod.1 | 20 ++
tools/libxl/libxl.c | 201 +++++++++++++++++++
tools/libxl/libxl.h | 20 ++
tools/libxl/libxl_create.c | 1 +
tools/libxl/libxl_device.c | 1 +
tools/libxl/libxl_internal.h | 9 +
tools/libxl/libxl_types.idl | 42 ++++
tools/libxl/libxl_types_internal.idl | 1 +
tools/libxl/xl.h | 3 +
tools/libxl/xl_cmdimpl.c | 360 ++++++++++++++++++++++++++++++++++-
tools/libxl/xl_cmdtable.c | 15 ++
12 files changed, 702 insertions(+), 1 deletion(-)
diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index c8ce6c1..413e4be 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -388,6 +388,36 @@ value is optional if this is a guest domain.
=back
+=item B<vscsi=[ "VSCSI_SPEC_STRING", "VSCSI_SPEC_STRING", ...]>
+
+Specifies the PVSCSI devices to be provided to the guest. PVSCSI passes
+dom0 SCSI devices as-is to the guest.
+
+Each B<VSCSI_SPEC_STRING> is a mapping from dom0 SCSI devices to guest visible
+SCSI devices, like 'pvdev,vdev[,option]'. Example: '/dev/sdm,3:0:4:5,feature-host'
+
+=over 4
+
+=item C<pdev>
+
+Specifies the dom0 visible SCSI device. The string can be either a device path
+like to a block device like /dev/disk/by-id/scsi-XYZ. Or it can be a device path
+to a char device like /dev/sg5. Or it can be specified in the SCSI notation
+HOST:CHANNEL:TARGET:LUN. Note that the latter format is unreliable because
+the HOST value can change across dom0 reboots.
+
+=item C<vdev>
+
+Specifies how the SCSI device is mapped into the guest. The notation is in
+SCSI notation HOST:CHANNEL:TARGET:LUN. HOST in this case means a virthal
+SCSI host within the guest.
+
+=item C<option>
+
+Right now only one option is recognized: feature-host.
+
+=back
+
=item B<vfb=[ "VFB_SPEC_STRING", "VFB_SPEC_STRING", ...]>
Specifies the paravirtual framebuffer devices which should be supplied
diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 30bd4bf..5f89fca 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -1219,6 +1219,26 @@ List virtual trusted platform modules for a domain.
=back
+=head2 PVSCSI DEVICES
+
+=over 4
+
+=item B<scsi-attach> I<domain-id> I<pdev> I<vdev> I<[,feature-host]>
+
+Creates a new vscsi device in the domain specified by I<domain-id>.
+
+=item B<scsi-detach> I<domain-id> I<vdev>
+
+Removes the vscsi device from domain specified by I<domain-id>.
+Note that the whole virtual SCSI host with all its devices is removed.
+This is a BUG!
+
+=item B<vscsi-list> I<domain-id> I<[domain-id] ...>
+
+List vscsi devices for the domain specified by I<domain-id>.
+
+=back
+
=head1 PCI PASS-THROUGH
=over 4
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 236a3f7..945b7c9 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2009,6 +2009,197 @@ int libxl_devid_to_device_vtpm(libxl_ctx *ctx,
return rc;
}
+/******************************************************************************/
+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);
+ flexarray_t *front;
+ flexarray_t *back;
+ libxl__device *device;
+ unsigned int rc, i;
+
+ i = 2 * (4 + (3 * vscsi->num_vscsi_devs));
+ front = flexarray_make(gc, 4, 1);
+ back = flexarray_make(gc, i, 1);
+
+ if (vscsi->devid == -1) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ GCNEW(device);
+ rc = libxl__device_from_vscsi(gc, domid, vscsi, device);
+ if ( rc != 0 ) goto out;
+
+ 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));
+ for (i = 0; i < vscsi->num_vscsi_devs; i++) {
+ libxl_vscsi_dev *v = vscsi->vscsi_devs + i;
+ flexarray_append_pair(back, GCSPRINTF("vscsi-devs/dev-%u/p-dev", v->vscsi_dev_id),
+ GCSPRINTF("%u:%u:%u:%u", v->p_hst, v->p_chn, v->p_tgt, v->p_lun));
+ flexarray_append_pair(back, GCSPRINTF("vscsi-devs/dev-%u/v-dev", v->vscsi_dev_id),
+ GCSPRINTF("%u:%u:%u:%u", vscsi->v_hst, v->v_chn, v->v_tgt, v->v_lun));
+ flexarray_append_pair(back, GCSPRINTF("vscsi-devs/dev-%u/state", v->vscsi_dev_id), "1");
+ }
+
+ flexarray_append_pair(front, "backend-id", GCSPRINTF("%d", vscsi->backend_domid));
+ flexarray_append_pair(front, "state", "1");
+
+ 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->dev = device;
+ aodev->action = LIBXL__DEVICE_ACTION_ADD;
+ libxl__wait_device_connection(egc, aodev);
+
+ rc = 0;
+out:
+ aodev->rc = rc;
+ if(rc) aodev->callback(egc, aodev);
+ return;
+}
+
+libxl_device_vscsi *libxl_device_vscsi_list(libxl_ctx *ctx, uint32_t domid, int *num)
+{
+ GC_INIT(ctx);
+
+ libxl_device_vscsi *vscsi_hosts = NULL;
+ char *fe_path;
+ char **dir;
+ unsigned int ndirs = 0;
+
+ fe_path = libxl__sprintf(gc, "%s/device/vscsi", libxl__xs_get_dompath(gc, domid));
+ dir = libxl__xs_directory(gc, XBT_NULL, fe_path, &ndirs);
+ if (dir && ndirs) {
+ libxl_device_vscsi *vscsi, *end;
+ vscsi_hosts = calloc(ndirs, sizeof(*vscsi_hosts));
+ for (vscsi = vscsi_hosts, end = vscsi_hosts + ndirs; vscsi_hosts && vscsi < end; ++vscsi, ++dir) {
+ unsigned int vd_dirs = 0, i;
+ char *tmp;
+ char **vscsi_devs_dir;
+ const char *vscsi_devs_path, *be_path = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/%s/backend", fe_path, *dir));
+
+ libxl_device_vscsi_init(vscsi);
+
+ vscsi->devid = atoi(*dir);
+
+ tmp = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/%s/backend-id", fe_path, *dir));
+ vscsi->backend_domid = atoi(tmp);
+
+ vscsi_devs_path = libxl__sprintf(gc, "%s/vscsi-devs", be_path);
+ vscsi_devs_dir = libxl__xs_directory(gc, XBT_NULL, vscsi_devs_path, &vd_dirs);
+ if (vscsi_devs_dir && vd_dirs) {
+ vscsi->vscsi_devs = calloc(vd_dirs, sizeof(*vscsi->vscsi_devs));
+ vscsi->num_vscsi_devs = vd_dirs;
+ for (i = 0; i < vd_dirs; i++, vscsi_devs_dir++) {
+ unsigned int vscsi_dev_id;
+ if (sscanf(*vscsi_devs_dir, "dev-%u", &vscsi_dev_id) != 1) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "%s/scsi-devs/%s failed to parse", be_path, *vscsi_devs_dir);
+ continue;
+ }
+ vscsi->vscsi_devs[i].vscsi_dev_id = vscsi_dev_id;
+ tmp = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/vscsi-devs/dev-%u/p-dev", be_path, vscsi_dev_id));
+ if (tmp)
+ sscanf(tmp, "%u:%u:%u:%u", &vscsi->vscsi_devs[i].p_hst, &vscsi->vscsi_devs[i].p_chn, &vscsi->vscsi_devs[i].p_tgt, &vscsi->vscsi_devs[i].p_lun);
+ tmp = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/vscsi-devs/dev-%u/v-dev", be_path, vscsi_dev_id));
+ if (tmp)
+ sscanf(tmp, "%u:%u:%u:%u", &vscsi->v_hst, &vscsi->vscsi_devs[i].v_chn, &vscsi->vscsi_devs[i].v_tgt, &vscsi->vscsi_devs[i].v_lun);
+ }
+ }
+ }
+ }
+ *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 = 0;
+
+ libxl_vscsiinfo_init(vscsiinfo);
+ dompath = libxl__xs_get_dompath(gc, domid);
+ vscsiinfo->devid = vscsi_host->devid;
+ vscsiinfo->p_hst = vscsi_dev->p_hst;
+ vscsiinfo->p_chn = vscsi_dev->p_chn;
+ vscsiinfo->p_tgt = vscsi_dev->p_tgt;
+ vscsiinfo->p_lun = vscsi_dev->p_lun;
+ vscsiinfo->v_hst = vscsi_host->v_hst;
+ vscsiinfo->v_chn = vscsi_dev->v_chn;
+ vscsiinfo->v_tgt = vscsi_dev->v_tgt;
+ vscsiinfo->v_lun = vscsi_dev->v_lun;
+
+ 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 err;
+ }
+ if(!libxl__xs_read(gc, XBT_NULL, vscsiinfo->backend)) {
+ goto err;
+ }
+
+ 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;
+
+ goto exit;
+err:
+ rc = ERROR_FAIL;
+exit:
+ GC_FREE;
+ return rc;
+}
/******************************************************************************/
@@ -3469,6 +3660,8 @@ out:
* libxl_device_vkb_destroy
* libxl_device_vfb_remove
* libxl_device_vfb_destroy
+ * libxl_device_vscsi_remove
+ * libxl_device_vscsi_destroy
*/
#define DEFINE_DEVICE_REMOVE(type, removedestroy, f) \
int libxl_device_##type##_##removedestroy(libxl_ctx *ctx, \
@@ -3520,6 +3713,10 @@ DEFINE_DEVICE_REMOVE(vfb, destroy, 1)
DEFINE_DEVICE_REMOVE(vtpm, remove, 0)
DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
+/* vscsi */
+DEFINE_DEVICE_REMOVE(vscsi, remove, 0)
+DEFINE_DEVICE_REMOVE(vscsi, destroy, 0)
+
#undef DEFINE_DEVICE_REMOVE
/******************************************************************************/
@@ -3529,6 +3726,7 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
* libxl_device_disk_add
* libxl_device_nic_add
* libxl_device_vtpm_add
+ * libxl_device_vscsi_add
*/
#define DEFINE_DEVICE_ADD(type) \
@@ -3558,6 +3756,9 @@ DEFINE_DEVICE_ADD(nic)
/* vtpm */
DEFINE_DEVICE_ADD(vtpm)
+/* vscsi */
+DEFINE_DEVICE_ADD(vscsi)
+
#undef DEFINE_DEVICE_ADD
/******************************************************************************/
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index ea94895..ed6dced 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -951,6 +951,26 @@ libxl_device_vtpm *libxl_device_vtpm_list(libxl_ctx *ctx, uint32_t domid, int *n
int libxl_device_vtpm_getinfo(libxl_ctx *ctx, uint32_t domid,
libxl_device_vtpm *vtpm, libxl_vtpminfo *vtpminfo);
+/* 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);
+
/* Keyboard */
int libxl_device_vkb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vkb *vkb,
const libxl_asyncop_how *ao_how)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index d015cf4..4202db4 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1037,6 +1037,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 fa99f77..856e147 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -542,6 +542,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_internal.h b/tools/libxl/libxl_internal.h
index c2b73c4..3f4f2f8 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1053,6 +1053,7 @@ _hidden int libxl__device_disk_setdefault(libxl__gc *gc,
_hidden int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic,
uint32_t domid);
_hidden int libxl__device_vtpm_setdefault(libxl__gc *gc, libxl_device_vtpm *vtpm);
+_hidden int libxl__device_vscsi_setdefault(libxl__gc *gc, libxl_device_vscsi *vscsi);
_hidden int libxl__device_vfb_setdefault(libxl__gc *gc, libxl_device_vfb *vfb);
_hidden int libxl__device_vkb_setdefault(libxl__gc *gc, libxl_device_vkb *vkb);
_hidden int libxl__device_pci_setdefault(libxl__gc *gc, libxl_device_pci *pci);
@@ -2212,6 +2213,10 @@ _hidden void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
libxl_device_vtpm *vtpm,
libxl__ao_device *aodev);
+_hidden void libxl__device_vscsi_add(libxl__egc *egc, uint32_t domid,
+ libxl_device_vscsi *vscsi,
+ libxl__ao_device *aodev);
+
/* Internal function to connect a vkb device */
_hidden int libxl__device_vkb_add(libxl__gc *gc, uint32_t domid,
libxl_device_vkb *vkb);
@@ -2653,6 +2658,10 @@ _hidden void libxl__add_vtpms(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);
+
/*----- device model creation -----*/
/* First layer; wraps libxl__spawn_spawn. */
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 80d00a7..c6795af 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -456,6 +456,25 @@ libxl_device_vtpm = Struct("device_vtpm", [
("uuid", libxl_uuid),
])
+libxl_vscsi_dev = Struct("vscsi_dev", [
+ ("vscsi_dev_id", libxl_devid),
+ ("p_hst", uint32),
+ ("p_chn", uint32),
+ ("p_tgt", uint32),
+ ("p_lun", uint32),
+ ("v_chn", uint32),
+ ("v_tgt", uint32),
+ ("v_lun", uint32),
+ ])
+
+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),
@@ -466,6 +485,7 @@ libxl_domain_config = Struct("domain_config", [
("vfbs", Array(libxl_device_vfb, "num_vfbs")),
("vkbs", Array(libxl_device_vkb, "num_vkbs")),
("vtpms", Array(libxl_device_vtpm, "num_vtpms")),
+ ("vscsis", Array(libxl_device_vscsi, "num_vscsis")),
("on_poweroff", libxl_action_on_shutdown),
("on_reboot", libxl_action_on_shutdown),
@@ -508,6 +528,28 @@ libxl_vtpminfo = Struct("vtpminfo", [
("uuid", libxl_uuid),
], dir=DIR_OUT)
+libxl_vscsiinfo = Struct("vscsiinfo", [
+ ("backend", string),
+ ("backend_id", uint32),
+ ("frontend", string),
+ ("frontend_id", uint32),
+ ("devid", libxl_devid),
+ ("p_hst", uint16),
+ ("p_chn", uint16),
+ ("p_tgt", uint16),
+ ("p_lun", uint16),
+ ("vscsi_dev_id", libxl_devid),
+ ("v_hst", uint16),
+ ("v_chn", uint16),
+ ("v_tgt", uint16),
+ ("v_lun", uint16),
+ ("feature_host", bool),
+ ("vscsi_host_state", integer),
+ ("vscsi_dev_state", integer),
+ ("evtch", integer),
+ ("rref", integer),
+ ], dir=DIR_OUT)
+
libxl_vcpuinfo = Struct("vcpuinfo", [
("vcpuid", uint32),
("cpu", uint32),
diff --git a/tools/libxl/libxl_types_internal.idl b/tools/libxl/libxl_types_internal.idl
index cb9444f..8bf4e71 100644
--- a/tools/libxl/libxl_types_internal.idl
+++ b/tools/libxl/libxl_types_internal.idl
@@ -20,6 +20,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/xl.h b/tools/libxl/xl.h
index 10a2e66..c43f2d4 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -81,6 +81,9 @@ int main_networkdetach(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 5195914..65b3841 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -17,6 +17,7 @@
#include "libxl_osdeps.h"
#include <stdio.h>
+#include <stddef.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
@@ -34,6 +35,7 @@
#include <ctype.h>
#include <inttypes.h>
#include <limits.h>
+#include <dirent.h>
#include "libxl.h"
#include "libxl_utils.h"
@@ -725,6 +727,122 @@ static void parse_top_level_sdl_options(XLU_Config *config,
xlu_cfg_replace_string (config, "xauthority", &sdl->xauthority, 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;
+}
+
+static void parse_vscsi_config(libxl_device_vscsi *vscsi_host,
+ libxl_vscsi_dev *vscsi_dev,
+ char *buf)
+{
+ char *pdev, *vdev, *fhost;
+ unsigned int hst, chn, tgt, lun;
+
+ libxl_device_vscsi_init(vscsi_host);
+ pdev = strtok(buf, ",");
+ vdev = strtok(NULL, ",");
+ fhost = strtok(NULL, ",");
+ if (!(pdev && vdev)) {
+ fprintf(stderr, "invalid vscsi= devspec: '%s'\n", buf);
+ exit(1);
+ }
+
+ pdev = vscsi_trim_string(pdev);
+ vdev = vscsi_trim_string(vdev);
+
+ if (strncmp(pdev, "/dev/", 5) == 0) {
+#ifdef __linux__
+ struct stat pdev_stat;
+ char pdev_sysfs_path[PATH_MAX];
+ const char *type;
+ int result = 0;
+ DIR *dirp;
+ struct dirent *de;
+
+ /* stat pdev to get device's sysfs entry */
+ if (stat (pdev, &pdev_stat) == -1) {
+ fprintf(stderr, "vscsi: invalid %s '%s' in vscsi= devspec: '%s', device not found or cannot be read\n", "pdev", pdev, buf);
+ exit(1);
+ }
+ if (S_ISBLK (pdev_stat.st_mode)) {
+ type = "block";
+ } else if (S_ISCHR (pdev_stat.st_mode)) {
+ type = "char";
+ } else {
+ fprintf(stderr, "vscsi: invalid %s '%s' in vscsi= devspec: '%s', not a valid block or char device\n", "pdev", pdev, buf);
+ exit(1);
+ }
+
+ /* get pdev scsi address - subdir of scsi_device sysfs entry */
+ snprintf(pdev_sysfs_path, sizeof(pdev_sysfs_path), "/sys/dev/%s/%u:%u/device/scsi_device",
+ type,
+ major(pdev_stat.st_rdev),
+ minor(pdev_stat.st_rdev));
+
+ dirp = opendir(pdev_sysfs_path);
+ if (!dirp) {
+ fprintf(stderr, "vscsi: invalid %s '%s' in vscsi= devspec: '%s', cannot find scsi device\n", "pdev", pdev, buf);
+ exit(1);
+ }
+
+ while ((de = readdir(dirp))) {
+ if (!strcmp(de->d_name, ".") || !strcmp(de->d_name, ".."))
+ continue;
+
+ if (sscanf(de->d_name, "%u:%u:%u:%u", &hst, &chn, &tgt, &lun) != 4) {
+ fprintf(stderr, "vscsi: ignoring unknown devspec '%s' for device '%s'\n",
+ de->d_name, pdev);
+ continue;
+ }
+ result = 1;
+ break;
+ }
+ closedir(dirp);
+
+ if (!result) {
+ fprintf(stderr, "vscsi: invalid %s '%s' in vscsi= devspec: '%s', cannot find scsi device in sysfs\n", "pdev", pdev, buf);
+ exit(1);
+ }
+#else
+ fprintf(stderr, "vscsi: invalid %s '%s' in vscsi= devspec: '%s', expecting hst:chn:tgt:lun\n", "pdev", pdev, buf);
+ exit(1);
+#endif
+ } else if (sscanf(pdev, "%u:%u:%u:%u", &hst, &chn, &tgt, &lun) != 4) {
+ fprintf(stderr, "vscsi: invalid %s '%s' in vscsi= devspec: '%s', expecting hst:chn:tgt:lun\n", "pdev", pdev, buf);
+ exit(1);
+ }
+ vscsi_dev->p_hst = hst;
+ vscsi_dev->p_chn = chn;
+ vscsi_dev->p_tgt = tgt;
+ vscsi_dev->p_lun = lun;
+
+ if (sscanf(vdev, "%u:%u:%u:%u", &hst, &chn, &tgt, &lun) != 4) {
+ fprintf(stderr, "vscsi: invalid %s '%s' in vscsi= devspec: '%s', expecting hst:chn:tgt:lun\n", "vdev", vdev, buf);
+ exit(1);
+ }
+ vscsi_host->v_hst = hst;
+ vscsi_dev->v_chn = chn;
+ vscsi_dev->v_tgt = tgt;
+ vscsi_dev->v_lun = lun;
+
+ if (fhost) {
+ fhost = vscsi_trim_string(fhost);
+ vscsi_host->feature_host = strcmp(fhost, "feature-host") == 0;
+ if (!vscsi_host->feature_host) {
+ fprintf(stderr, "vscsi: invalid option '%s' in vscsi= devspec: '%s', expecting %s\n", fhost, buf, "feature-host");
+ exit(1);
+ }
+ }
+}
+
static void parse_config_data(const char *config_source,
const char *config_data,
int config_len,
@@ -735,7 +853,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 *ioports, *irqs, *iomem;
int num_ioports, num_irqs, num_iomem;
int pci_power_mgmt = 0;
@@ -1242,6 +1360,56 @@ static void parse_config_data(const char *config_source,
}
}
+ if (!xlu_cfg_get_list(config, "vscsi", &vscsis, 0, 0)) {
+ int cnt_vscsi_devs = 0;
+ d_config->num_vscsis = 0;
+ d_config->vscsis = NULL;
+ while ((buf = xlu_cfg_get_listitem (vscsis, cnt_vscsi_devs)) != NULL) {
+ libxl_vscsi_dev vscsi_dev = { };
+ libxl_device_vscsi vscsi_host = { };
+ libxl_device_vscsi *host;
+ char *tmp_buf;
+ int num_vscsis, host_found = 0;
+
+ /*
+ * #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
+ */
+ tmp_buf = strdup(buf);
+ parse_vscsi_config(&vscsi_host, &vscsi_dev, tmp_buf);
+ free(tmp_buf);
+
+ if (d_config->vscsis) {
+ for (num_vscsis = 0; num_vscsis < d_config->num_vscsis; num_vscsis++) {
+ if (d_config->vscsis[num_vscsis].v_hst == vscsi_host.v_hst) {
+ host = d_config->vscsis + num_vscsis;
+ host->vscsi_devs = realloc(host->vscsi_devs, sizeof(libxl_vscsi_dev) * (host->num_vscsi_devs + 1));
+ vscsi_dev.vscsi_dev_id = host->num_vscsi_devs;
+ memcpy(host->vscsi_devs + host->num_vscsi_devs, &vscsi_dev, sizeof(vscsi_dev));
+ host->num_vscsi_devs++;
+ host_found = 1;
+ break;
+ }
+ }
+ }
+ if (!host_found || !d_config->vscsis) {
+ d_config->vscsis = realloc(d_config->vscsis, sizeof(libxl_device_vscsi) * (d_config->num_vscsis + 1));
+ vscsi_host.vscsi_devs = malloc(sizeof(libxl_vscsi_dev));
+ vscsi_dev.vscsi_dev_id = 0;
+ memcpy(vscsi_host.vscsi_devs, &vscsi_dev, sizeof(vscsi_dev));
+ vscsi_host.num_vscsi_devs++;
+ vscsi_host.devid = d_config->num_vscsis;
+ memcpy(d_config->vscsis + d_config->num_vscsis, &vscsi_host, sizeof(vscsi_host));
+ d_config->num_vscsis++;
+ }
+ cnt_vscsi_devs++;
+ }
+ }
+
if (!xlu_cfg_get_list(config, "vtpm", &vtpms, 0, 0)) {
d_config->num_vtpms = 0;
d_config->vtpms = NULL;
@@ -6017,6 +6185,196 @@ int main_blockdetach(int argc, char **argv)
return rc;
}
+int main_vscsiattach(int argc, char **argv)
+{
+ int opt;
+ libxl_vscsi_dev *vscsi_dev;
+ libxl_device_vscsi *vscsi_host;
+ uint32_t domid;
+ char *tmp_buf, *feat_buf = NULL;
+
+ if (argv) {
+ fprintf(stderr, "scsi-attach is not yet implemented.\n");
+ return 1;
+ }
+
+ SWITCH_FOREACH_OPT(opt, "", NULL, "scsi-attach", 1) {
+ /* No options */
+ }
+
+ if (argc < 4) {
+ 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;
+
+ fprintf(stderr, "%s(%u) optind %x argc %d\n", __func__, __LINE__, optind, argc);
+ if (argc < 4) {
+ fprintf(stderr, "scsi-attach: 3 options required.\n");
+ return 1;
+ }
+ if (argc == 5) {
+ if (asprintf(&feat_buf, ",%s", argv[4]) < 0) {
+ perror("asprintf");
+ return 1;
+ }
+ }
+ if (asprintf(&tmp_buf, "%s,%s%s", argv[2], argv[3], feat_buf ?: "") < 0) {
+ perror("asprintf");
+ return 1;
+ }
+ fprintf(stderr, "%s(%u) tmp_buf '%s'\n", __func__, __LINE__, tmp_buf);
+ vscsi_dev = calloc(1, sizeof(*vscsi_dev));
+ vscsi_host = calloc(1, sizeof(*vscsi_host));
+ if (!(vscsi_dev && vscsi_host)) {
+ fprintf(stderr, "%s ENOMEM\n", __func__);
+ free(tmp_buf);
+ free(feat_buf);
+ return 1;
+ }
+ parse_vscsi_config(vscsi_host, vscsi_dev, tmp_buf);
+
+ free(tmp_buf);
+ free(feat_buf);
+
+ if (dryrun_only) {
+ char* json = libxl_device_vscsi_to_json(ctx, vscsi_host);
+ printf("vscsi: %s\n", json);
+ free(json);
+ libxl_device_vscsi_dispose(vscsi_host);
+ if (ferror(stdout) || fflush(stdout)) { perror("stdout"); exit(-1); }
+ return 0;
+ }
+
+ if (libxl_device_vscsi_add(ctx, domid, vscsi_host, 0)) {
+ fprintf(stderr, "libxl_device_vscsi_add failed.\n");
+ return 1;
+ }
+ libxl_device_vscsi_dispose(vscsi_host);
+ return 0;
+}
+
+int main_vscsilist(int argc, char **argv)
+{
+ int opt;
+ 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) {
+ uint32_t domid;
+ 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.p_hst, vscsiinfo.p_chn, vscsiinfo.p_tgt, vscsiinfo.p_lun);
+ snprintf(vdev, sizeof(vdev), "%u:%u:%u:%u",
+ vscsiinfo.v_hst, vscsiinfo.v_chn, vscsiinfo.v_tgt, vscsiinfo.v_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 *vscsi_dev;
+ libxl_device_vscsi *vscsi_host;
+ libxl_device_vscsi *vscsi_hosts;
+ libxl_vscsiinfo vscsiinfo;
+ char *tmp_buf, *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;
+
+ if (asprintf(&tmp_buf, "0:0:0:0,%s", vdev) < 0) {
+ perror("asprintf");
+ return 1;
+ }
+ vscsi_dev = calloc(1, sizeof(*vscsi_dev));
+ vscsi_host = calloc(1, sizeof(*vscsi_host));
+ if (!(vscsi_dev && vscsi_host)) {
+ fprintf(stderr, "%s ENOMEM\n", __func__);
+ goto done;
+ }
+ parse_vscsi_config(vscsi_host, vscsi_dev, tmp_buf);
+
+ for (h = 0; h < num_hosts; ++h) {
+ for (d = 0; !found && 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)) {
+#define CMP(val) (vscsiinfo.val == vscsi_dev->val)
+ if (vscsiinfo.v_hst == vscsi_host->v_hst && CMP(v_chn) && CMP(v_tgt) && CMP(v_lun)) {
+ if (vscsi_hosts[h].num_vscsi_devs > 1)
+ fprintf(stderr, "%s(%u) TROUBLE AHEAD: vhost %u has %u devices! All will disappear now.\n", __func__, __LINE__, vscsiinfo.v_hst, vscsi_hosts[h].num_vscsi_devs);
+ found = 1;
+ libxl_device_vscsi_remove(ctx, domid, &vscsi_hosts[h], 0);
+ }
+ libxl_vscsiinfo_dispose(&vscsiinfo);
+#undef CMP
+ }
+ }
+ libxl_device_vscsi_dispose(&vscsi_hosts[h]);
+ }
+ if (!found)
+ fprintf(stderr, "%s(%u) vdev %s does not exist in domain %s\n", __func__, __LINE__, vdev, dom);
+done:
+ free(vscsi_hosts);
+ free(vscsi_host);
+ free(vscsi_dev);
+ free(tmp_buf);
+ 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 4279b9f..b27cdf2 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -350,6 +350,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.",
+ "<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",
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] libbxl: add support for pvscsi, iteration 1
2014-04-30 11:03 [PATCH] libbxl: add support for pvscsi, iteration 1 Olaf Hering
@ 2014-05-02 14:36 ` Olaf Hering
2014-05-02 15:30 ` Ian Campbell
2014-05-02 15:11 ` Ian Campbell
2014-05-02 15:54 ` Ian Jackson
2 siblings, 1 reply; 8+ messages in thread
From: Olaf Hering @ 2014-05-02 14:36 UTC (permalink / raw)
To: xen-devel; +Cc: Ondrej Holecek, Ian Jackson, Ian Campbell
On Wed, Apr 30, Olaf Hering wrote:
> Add support for vscsi= in domU.cfg, add new xl commands scsi-attach,
> scsi-list, scsi-detach. The syntax follows what xend understood.
>
> pvscsi provides a dom0 SCSI device as-is to a domU. The backend and
> frontend drivers can be found in xen-linux, or its forward-ported
> variants. This patch was tested with an openSUSE dom0 and a SLES12
> guest. Any openSUSE/SLE11/SLE12 dom0/domU combination will work.
One question regarding device index handling in xenstore:
If devices get removed and added at runtime, they will appear as
/local/domain/0/backend/<devtype>/<domid>/<index>, like
/local/domain/0/backend/vif/3/0.
What is supposed to happen with index if, in this example,
network-attach adds another device, network-detach removes it again,
later network-attach adds yet another device? The first attach will most
likely use index==1. network-detach will remove the device with index 1.
Is the last network-attach supposed to use index 2, or is it allowed to
reuse index 1? Right now I'm not seeing an index counter for
/local/domain/0/backend/vif/3, so I assume it will pick 1.
While working on scsi-attach/scsi-detach I was wondering if the new code
should maintain some counter so that new vscsi hosts get a new number
which was not in use before. Same for vscsi devs. It looks like xend
maintained an internal global counter for (at least) all devs. Surely
such counter was easy to maintain for xend as it controls everything.
If no such counter is required I have to assume that by the time the
toolstack reuses index 1 everything in backend/frontend has released all
references to index 1. No races should happen.
Is that assumption correct for all kind of backend devices?
Olaf
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libbxl: add support for pvscsi, iteration 1
2014-04-30 11:03 [PATCH] libbxl: add support for pvscsi, iteration 1 Olaf Hering
2014-05-02 14:36 ` Olaf Hering
@ 2014-05-02 15:11 ` Ian Campbell
2014-05-02 15:54 ` Olaf Hering
2014-05-02 15:54 ` Ian Jackson
2 siblings, 1 reply; 8+ messages in thread
From: Ian Campbell @ 2014-05-02 15:11 UTC (permalink / raw)
To: Olaf Hering; +Cc: Ondrej Holecek, Ian Jackson, xen-devel
On Wed, 2014-04-30 at 13:03 +0200, Olaf Hering wrote:
> Add support for vscsi= in domU.cfg, add new xl commands scsi-attach,
> scsi-list, scsi-detach. The syntax follows what xend understood.
>
> pvscsi provides a dom0 SCSI device as-is to a domU. The backend and
> frontend drivers can be found in xen-linux, or its forward-ported
> variants. This patch was tested with an openSUSE dom0 and a SLES12
> guest. Any openSUSE/SLE11/SLE12 dom0/domU combination will work.
>
> The domU.cfg syntax is "vscsi=[ 'pdev,vdev[,option]' ]". pdev is the
> dom0 device in either /dev/$device or HOST:CHANNEL:TARGET:LUN notation.
> vdev is the domU device in HOST:CHANNEL:TARGET:LUN notation.
>
> xl scsi-attach will attach a SCSI device at runtime.
> xl scsi-detach will remove a SCSI drivce at runtime.
Is a drivce a device or a drive ;-)
> xl scsi-list lists SCSI devices for a list of domains.
>
> The backend driver uses a single_host:many_devices notation to manage
> domU devices. The xenstore layout looks like this:
Please can you add ~/device/vscsi/$DEVID/* and
~/backend/vif/$DOMID/$DEVID/* to docs/misc/xenstore-paths.markdown.
If you were inclined to improve xen/include/public/io/vscsiif.h by using
some of the content from here then that would be awesome too.
>
> /local/domain/0/backend/vscsi/<domid>/<vhost>/feature-host = "0"
> /local/domain/0/backend/vscsi/<domid>/<vhost>/frontend = "/local/domain/<domid>/device/vscsi/0"
> /local/domain/0/backend/vscsi/<domid>/<vhost>/frontend-id = "<domid>"
> /local/domain/0/backend/vscsi/<domid>/<vhost>/online = "1"
> /local/domain/0/backend/vscsi/<domid>/<vhost>/state = "4"
> /local/domain/0/backend/vscsi/<domid>/<vhost>/vscsi-devs/dev-0/p-dev = "8:0:2:1"
> /local/domain/0/backend/vscsi/<domid>/<vhost>/vscsi-devs/dev-0/state = "4"
> /local/domain/0/backend/vscsi/<domid>/<vhost>/vscsi-devs/dev-0/v-dev = "0:0:0:0"
> /local/domain/0/backend/vscsi/<domid>/<vhost>/vscsi-devs/dev-1/p-dev = "8:0:2:2"
> /local/domain/0/backend/vscsi/<domid>/<vhost>/vscsi-devs/dev-1/state = "4"
> /local/domain/0/backend/vscsi/<domid>/<vhost>/vscsi-devs/dev-1/v-dev = "0:0:1:0"
>
> /local/domain/<domid>/device/vscsi/<vhost>/backend = "/local/domain/0/backend/vscsi/<domid>/<vhost>"
> /local/domain/<domid>/device/vscsi/<vhost>/backend-id = "0"
> /local/domain/<domid>/device/vscsi/<vhost>/event-channel = "20"
> /local/domain/<domid>/device/vscsi/<vhost>/ring-ref = "43"
> /local/domain/<domid>/device/vscsi/<vhost>/state = "4"
> /local/domain/<domid>/device/vscsi/<vhost>/vscsi-devs/dev-0/state = "4"
> /local/domain/<domid>/device/vscsi/<vhost>/vscsi-devs/dev-1/state = "4"
>
> This is a challenge for libxl because libxl currently handles only
> single_host:single_device in its device helper functions.
This is a bit like pci I suppose?
> Due to this
> limitation in libxl, scsi-detach will remove the whole virtual scsi host
> with all its virtual devices, instead of just the requested single
> virtual device.
That's rather unfortunate!
Are multiple <vhost> allowed/supported? e.g. could I (or better, libxl
by default) setup a device-per-host situation to alleviate this?
For pcifront/back we support bus reconfiguration (via
XenbusStateReconfigur{ing,ed}) which is how this works there. Does vscsi
not support anything like that?
> scsi-attach can cope with this limitation because it
> could write the required files directly into xenstore to attach yet
> another virtual scsi device to an existing virtual scsi host. However,
> this functionality is currently not implemented.
>
> To support the pdev notation '/dev/$device' it is required to parse
> sysfs to translate the path to HST:CHN:TGT:LUN notation. In its current
> variant /sys/dev/ is used, which is available since at least Linux 3.0.
> Support for dom0 kernels which lack this interface will be added later.
Are there any such kernels which anyone cares about any more?
> This is a draft patch to get it out of the door, to get help with
> solving the libxl limitation above, and of course for public code review.
> In this first iteration:
> - the domU.cfg parser for vscsi=[] is functional.
> - xl scsi-list will print output similar to its xm counter part.
> - xl scsi-detach will remove entire scsi hosts, unlike its xm counter part.
What did xm do instead? Fail?
> - xl scsi-attach is not yet implemented.
> - coding style may be way off in some parts of the new code.
I shall keep that in mind and not comment on it on the expectation that
you want a chance to go through it before I get all pernickety.
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> Cc: Ian Campbell <Ian.Campbell@citrix.com>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Ondrej Holecek <oholecek@suse.com>
> ---
> docs/man/xl.cfg.pod.5 | 30 +++
> docs/man/xl.pod.1 | 20 ++
> tools/libxl/libxl.c | 201 +++++++++++++++++++
> tools/libxl/libxl.h | 20 ++
> tools/libxl/libxl_create.c | 1 +
> tools/libxl/libxl_device.c | 1 +
> tools/libxl/libxl_internal.h | 9 +
> tools/libxl/libxl_types.idl | 42 ++++
> tools/libxl/libxl_types_internal.idl | 1 +
> tools/libxl/xl.h | 3 +
> tools/libxl/xl_cmdimpl.c | 360 ++++++++++++++++++++++++++++++++++-
> tools/libxl/xl_cmdtable.c | 15 ++
> 12 files changed, 702 insertions(+), 1 deletion(-)
>
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index c8ce6c1..413e4be 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -388,6 +388,36 @@ value is optional if this is a guest domain.
>
> =back
>
> +=item B<vscsi=[ "VSCSI_SPEC_STRING", "VSCSI_SPEC_STRING", ...]>
> +
> +Specifies the PVSCSI devices to be provided to the guest. PVSCSI passes
> +dom0 SCSI devices as-is to the guest.
> +
> +Each B<VSCSI_SPEC_STRING> is a mapping from dom0 SCSI devices to guest visible
> +SCSI devices, like 'pvdev,vdev[,option]'. Example: '/dev/sdm,3:0:4:5,feature-host'
> +
> +=over 4
> +
> +=item C<pdev>
> +
> +Specifies the dom0 visible SCSI device. The string can be either a device path
> +like to a block device like /dev/disk/by-id/scsi-XYZ. Or it can be a device path
"like to a ... like" I think you can drop the first "like".
Actually, I think the either is misplaced. "Can be a device path either
to a block device like ... or to a char device like ...
> +to a char device like /dev/sg5. Or it can be specified in the SCSI notation
> +HOST:CHANNEL:TARGET:LUN.
Hrm, that's now the third or in this either, which is making the wording
a bit unwieldy (i.e. my suggestion above no long really applies).
Perhaps a bulletted list is the way to go?
> Note that the latter format is unreliable because
> +the HOST value can change across dom0 reboots.
/dev/sgN would be unreliable too I think? As might block devices not
using the /dev/disk/by-* links you give in the example. You might be
best saying that only /dev/disk/by-* block device references are
reliable because all of the others might change over a reboot.
> +
> +=item C<vdev>
> +
> +Specifies how the SCSI device is mapped into the guest. The notation is in
> +SCSI notation HOST:CHANNEL:TARGET:LUN. HOST in this case means a virthal
Typo "virtual"
> +SCSI host within the guest.
All of HOST:CHANNEL:TARGET:LUN are numbers I think, are they
hex/dec/oct? Are there limits?
> +
> +=item C<option>
> +
> +Right now only one option is recognized: feature-host.
What does it do?
> +
> +=back
> +
> =item B<vfb=[ "VFB_SPEC_STRING", "VFB_SPEC_STRING", ...]>
>
> Specifies the paravirtual framebuffer devices which should be supplied
> diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> index 30bd4bf..5f89fca 100644
> --- a/docs/man/xl.pod.1
> +++ b/docs/man/xl.pod.1
> @@ -1219,6 +1219,26 @@ List virtual trusted platform modules for a domain.
>
> =back
>
> +=head2 PVSCSI DEVICES
> +
> +=over 4
> +
> +=item B<scsi-attach> I<domain-id> I<pdev> I<vdev> I<[,feature-host]>
Is the comma in I<[,feature-host]> literally required, as in
xl scsi-attach mydom 0:0:0:0 0:0:0:0 ,feature-host
?
> +
> +Creates a new vscsi device in the domain specified by I<domain-id>.
> +
> +=item B<scsi-detach> I<domain-id> I<vdev>
> +
> +Removes the vscsi device from domain specified by I<domain-id>.
> +Note that the whole virtual SCSI host with all its devices is removed.
> +This is a BUG!
I have a feeling it would be preferable in the first instance to either
just not provide the detach portion of this interface, or to require
that vdev is only a host and not a specific device (... unless the host
only has a single device, but maybe that's getting too far)
> +
> +=item B<vscsi-list> I<domain-id> I<[domain-id] ...>
> +
> +List vscsi devices for the domain specified by I<domain-id>.
> +
> +=back
> +
> =head1 PCI PASS-THROUGH
>
> =over 4
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 236a3f7..945b7c9 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> +
> +void libxl__device_vscsi_add(libxl__egc *egc, uint32_t domid,
> + libxl_device_vscsi *vscsi,
> + libxl__ao_device *aodev)
> +{
> + STATE_AO_GC(aodev->ao);
> + flexarray_t *front;
> + flexarray_t *back;
> + libxl__device *device;
> + unsigned int rc, i;
> +
> + i = 2 * (4 + (3 * vscsi->num_vscsi_devs));
That's quite exciting ;-)
A flexarray will grow as neccessary so some sort of static best guest of
a starting point would be fine here I think (or a comment about 2 4 and
3)!
I suppose the num_vscsi_devs here is an artefact of the short coming you
mentioned before. The way PCI handles this is to create the "bus" (aka
vhost here) either when the first device is attached or in the higher
level code before it calls the individual device adds (i.e. this
function), or something (I must confess my memory is a bit fuzzy,
libxl__create_pci_backend might be a place to look). Having done that
then libxl_device_pci is just a single device.
I think you should look to do something similar here, so that each
libxl_device_vscsi is actually a single device. The alternative is some
sort of libxl_bus_vscsi thing, which would be unconventional compared
with everything else
(hotplug needs considering BTW i.e. in case you need to create an empty
bus at start of day to plug things into).
PCI only ever has a single bus (at least now), so how closely you map
onto that schema depends a bit on the intended vscsi host structure you
are aiming for.
(warning: the libxl PCI stuff is "interesting" in places, look to it for
inspiration perhaps but if you start to think "this is mad" it probably
is, if you find yourself here do say something!)
> + front = flexarray_make(gc, 4, 1);
> + back = flexarray_make(gc, i, 1);
> +
> + if (vscsi->devid == -1) {
> + rc = ERROR_FAIL;
> + goto out;
> + }
[...]
> +libxl_device_vscsi *libxl_device_vscsi_list(libxl_ctx *ctx, uint32_t domid, int *num)
> +{
> + GC_INIT(ctx);
> +
> + libxl_device_vscsi *vscsi_hosts = NULL;
> + char *fe_path;
> + char **dir;
> + unsigned int ndirs = 0;
> +
> + fe_path = libxl__sprintf(gc, "%s/device/vscsi", libxl__xs_get_dompath(gc, domid));
Need to take care trusting this too much...
> + dir = libxl__xs_directory(gc, XBT_NULL, fe_path, &ndirs);
> + if (dir && ndirs) {
> + libxl_device_vscsi *vscsi, *end;
> + vscsi_hosts = calloc(ndirs, sizeof(*vscsi_hosts));
> + for (vscsi = vscsi_hosts, end = vscsi_hosts + ndirs; vscsi_hosts && vscsi < end; ++vscsi, ++dir) {
> + unsigned int vd_dirs = 0, i;
> + char *tmp;
> + char **vscsi_devs_dir;
> + const char *vscsi_devs_path, *be_path = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/%s/backend", fe_path, *dir));
Can we shorten/wrap this line please.
Please can you refactor the body of this loop into a _from_xs_be type
function. Also this will all look a bit different if you go over to a
device-per libxl_device_vscsi model.
> +
> + vscsi->vscsi_devs[i].vscsi_dev_id = vscsi_dev_id;
> + tmp = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/vscsi-devs/dev-%u/p-dev", be_path, vscsi_dev_id));
> + if (tmp)
> + sscanf(tmp, "%u:%u:%u:%u", &vscsi->vscsi_devs[i].p_hst, &vscsi->vscsi_devs[i].p_chn, &vscsi->vscsi_devs[i].p_tgt, &vscsi->vscsi_devs[i].p_lun);
> + tmp = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/vscsi-devs/dev-%u/v-dev", be_path, vscsi_dev_id));
> + if (tmp)
> + sscanf(tmp, "%u:%u:%u:%u", &vscsi->v_hst, &vscsi->vscsi_devs[i].v_chn, &vscsi->vscsi_devs[i].v_tgt, &vscsi->vscsi_devs[i].v_lun);
(lots of long lines here, Oh I said I wouldn't comment on cding style.
oh well it's done now)
> +
> +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 = 0;
> +
> + libxl_vscsiinfo_init(vscsiinfo);
> + dompath = libxl__xs_get_dompath(gc, domid);
> + vscsiinfo->devid = vscsi_host->devid;
> + vscsiinfo->p_hst = vscsi_dev->p_hst;
> + vscsiinfo->p_chn = vscsi_dev->p_chn;
> + vscsiinfo->p_tgt = vscsi_dev->p_tgt;
> + vscsiinfo->p_lun = vscsi_dev->p_lun;
> + vscsiinfo->v_hst = vscsi_host->v_hst;
> + vscsiinfo->v_chn = vscsi_dev->v_chn;
> + vscsiinfo->v_tgt = vscsi_dev->v_tgt;
> + vscsiinfo->v_lun = vscsi_dev->v_lun;
I'll be sure when I get to the IDL, but it looks like you might want to
introduce a type for holding host+chn+tgt+lun and just have pdev and
vdev of those as members of vscsiinfo.
> /******************************************************************************/
>
> @@ -3469,6 +3660,8 @@ out:
> * libxl_device_vkb_destroy
> * libxl_device_vfb_remove
> * libxl_device_vfb_destroy
> + * libxl_device_vscsi_remove
> + * libxl_device_vscsi_destroy
> */
> #define DEFINE_DEVICE_REMOVE(type, removedestroy, f) \
> int libxl_device_##type##_##removedestroy(libxl_ctx *ctx, \
> @@ -3520,6 +3713,10 @@ DEFINE_DEVICE_REMOVE(vfb, destroy, 1)
> DEFINE_DEVICE_REMOVE(vtpm, remove, 0)
> DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
>
> +/* vscsi */
> +DEFINE_DEVICE_REMOVE(vscsi, remove, 0)
> +DEFINE_DEVICE_REMOVE(vscsi, destroy, 0)
Should the second one not be (vscsi, destroy, 1) ?
>
> +/* Virtual SCSI */
I didn't confirm but I expect this will conform to the comment about the
standard device interfaces.
> +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);
> +
> /* Keyboard */
> int libxl_device_vkb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vkb *vkb,
> const libxl_asyncop_how *ao_how)
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 80d00a7..c6795af 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -456,6 +456,25 @@ libxl_device_vtpm = Struct("device_vtpm", [
> ("uuid", libxl_uuid),
> ])
>
> +libxl_vscsi_dev = Struct("vscsi_dev", [
> + ("vscsi_dev_id", libxl_devid),
> + ("p_hst", uint32),
> + ("p_chn", uint32),
> + ("p_tgt", uint32),
> + ("p_lun", uint32),
> + ("v_chn", uint32),
> + ("v_tgt", uint32),
> + ("v_lun", uint32),
Yep, you definitely want a type for those tuples.
> + ])
> +
> +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),
Not defbool (I don't know what this does...)
> @@ -508,6 +528,28 @@ libxl_vtpminfo = Struct("vtpminfo", [
> ("uuid", libxl_uuid),
> ], dir=DIR_OUT)
>
> +libxl_vscsiinfo = Struct("vscsiinfo", [
> + ("backend", string),
> + ("backend_id", uint32),
> + ("frontend", string),
> + ("frontend_id", uint32),
> + ("devid", libxl_devid),
> + ("p_hst", uint16),
> + ("p_chn", uint16),
> + ("p_tgt", uint16),
> + ("p_lun", uint16),
Type, also the widths here differ from the device...
> + ("vscsi_dev_id", libxl_devid),
> + ("v_hst", uint16),
> + ("v_chn", uint16),
> + ("v_tgt", uint16),
> + ("v_lun", uint16),
and again
> + ("feature_host", bool),
> + ("vscsi_host_state", integer),
> + ("vscsi_dev_state", integer),
PCI just omits state, presumably to sidestep this issue of which state
to report. I recommend you do the same unless you have a requirement.
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 5195914..65b3841 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -17,6 +17,7 @@
> #include "libxl_osdeps.h"
>
> #include <stdio.h>
> +#include <stddef.h>
> #include <stdlib.h>
> #include <string.h>
> #include <unistd.h>
> @@ -34,6 +35,7 @@
> #include <ctype.h>
> #include <inttypes.h>
> #include <limits.h>
> +#include <dirent.h>
>
> #include "libxl.h"
> #include "libxl_utils.h"
> @@ -725,6 +727,122 @@ static void parse_top_level_sdl_options(XLU_Config *config,
> xlu_cfg_replace_string (config, "xauthority", &sdl->xauthority, 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;
Doesn't seem to be anything vscsi specific here (and it seems like a
generally useful thing. I expect at least thevif parsing would benefit
from deploying this too (in so much as all this wouldn't be better with
a proper parser using flex).
> +}
> +
> +static void parse_vscsi_config(libxl_device_vscsi *vscsi_host,
> + libxl_vscsi_dev *vscsi_dev,
> + char *buf)
> +{
> + char *pdev, *vdev, *fhost;
> + unsigned int hst, chn, tgt, lun;
> +
> + libxl_device_vscsi_init(vscsi_host);
> + pdev = strtok(buf, ",");
> + vdev = strtok(NULL, ",");
> + fhost = strtok(NULL, ",");
I guess we are single threaded here so this is ok.
I wonder if this might be useful to put in libxlu alongside the disk
parser? The question is whether anything else wants to parse xm style
vscsi strings, like libvirt perhaps. (arguably a bunch of these parsers
here have that property and belong in libxlu).
> + pdev = vscsi_trim_string(pdev);
> + vdev = vscsi_trim_string(vdev);
> +
> + if (strncmp(pdev, "/dev/", 5) == 0) {
> +#ifdef __linux__
Urk. Normally we do this by splitting by files, e.g. into a linux
version and a nop version and compiling the appropriate one.
This seems like a candidate for libxlu too. We probably need to have a
think about how much of this stuff is done in libxl too -- e.g. for
disks we pass the pdev in as a string (path) and interpret it there.
That allows us to choose between alternative backend providers etc. Also
libxl is a slightly more palatable place to deal with Linux vs non-Linux
bits. I think that might be the right answer here too.
> @@ -1242,6 +1360,56 @@ static void parse_config_data(const char *config_source,
> }
> }
>
> + if (!xlu_cfg_get_list(config, "vscsi", &vscsis, 0, 0)) {
> + int cnt_vscsi_devs = 0;
> + d_config->num_vscsis = 0;
> + d_config->vscsis = NULL;
> + while ((buf = xlu_cfg_get_listitem (vscsis, cnt_vscsi_devs)) != NULL) {
> + libxl_vscsi_dev vscsi_dev = { };
> + libxl_device_vscsi vscsi_host = { };
> + libxl_device_vscsi *host;
> + char *tmp_buf;
> + int num_vscsis, host_found = 0;
> +
> + /*
> + * #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
Would all be a lot simpler if a libxl_device_vscsi was a single device
and all the worrying about which bus was which was in libxl
Ian.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libbxl: add support for pvscsi, iteration 1
2014-05-02 14:36 ` Olaf Hering
@ 2014-05-02 15:30 ` Ian Campbell
0 siblings, 0 replies; 8+ messages in thread
From: Ian Campbell @ 2014-05-02 15:30 UTC (permalink / raw)
To: Olaf Hering; +Cc: Ondrej Holecek, Ian Jackson, xen-devel
On Fri, 2014-05-02 at 16:36 +0200, Olaf Hering wrote:
> On Wed, Apr 30, Olaf Hering wrote:
>
> > Add support for vscsi= in domU.cfg, add new xl commands scsi-attach,
> > scsi-list, scsi-detach. The syntax follows what xend understood.
> >
> > pvscsi provides a dom0 SCSI device as-is to a domU. The backend and
> > frontend drivers can be found in xen-linux, or its forward-ported
> > variants. This patch was tested with an openSUSE dom0 and a SLES12
> > guest. Any openSUSE/SLE11/SLE12 dom0/domU combination will work.
>
> One question regarding device index handling in xenstore:
>
> If devices get removed and added at runtime, they will appear as
> /local/domain/0/backend/<devtype>/<domid>/<index>, like
> /local/domain/0/backend/vif/3/0.
>
> What is supposed to happen with index if, in this example,
> network-attach adds another device, network-detach removes it again,
> later network-attach adds yet another device? The first attach will most
> likely use index==1. network-detach will remove the device with index 1.
> Is the last network-attach supposed to use index 2, or is it allowed to
> reuse index 1? Right now I'm not seeing an index counter for
> /local/domain/0/backend/vif/3, so I assume it will pick 1.
The function to look at is libxl__device_nextid, which I think is buggy
in the scenario you describe since it only looks at the number of
entries, not which ones are used.
> While working on scsi-attach/scsi-detach I was wondering if the new code
> should maintain some counter so that new vscsi hosts get a new number
> which was not in use before. Same for vscsi devs. It looks like xend
> maintained an internal global counter for (at least) all devs. Surely
> such counter was easy to maintain for xend as it controls everything.
>
> If no such counter is required I have to assume that by the time the
> toolstack reuses index 1 everything in backend/frontend has released all
> references to index 1. No races should happen.
>
> Is that assumption correct for all kind of backend devices?
I'm not sure.
Perhaps libxl__device_nextid should use a xenstore transaction to read ,
increment and write a suitable counter, perhaps under /libxl?
Ian.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libbxl: add support for pvscsi, iteration 1
2014-04-30 11:03 [PATCH] libbxl: add support for pvscsi, iteration 1 Olaf Hering
2014-05-02 14:36 ` Olaf Hering
2014-05-02 15:11 ` Ian Campbell
@ 2014-05-02 15:54 ` Ian Jackson
2014-05-02 16:00 ` Olaf Hering
2 siblings, 1 reply; 8+ messages in thread
From: Ian Jackson @ 2014-05-02 15:54 UTC (permalink / raw)
To: Olaf Hering; +Cc: Ondrej Holecek, Ian Campbell, xen-devel
Olaf Hering writes ("[PATCH] libbxl: add support for pvscsi, iteration 1"):
> Add support for vscsi= in domU.cfg, add new xl commands scsi-attach,
> scsi-list, scsi-detach. The syntax follows what xend understood.
...
> +Specifies the PVSCSI devices to be provided to the guest. PVSCSI passes
> +dom0 SCSI devices as-is to the guest.
> +
> +Each B<VSCSI_SPEC_STRING> is a mapping from dom0 SCSI devices to guest visible
> +SCSI devices, like 'pvdev,vdev[,option]'. Example: '/dev/sdm,3:0:4:5,feature-host'
I think this docs patch dould do with some more linewrapping.
I would say "in the format" rather than "like".
> +Specifies the dom0 visible SCSI device. The string can be either a device path
> +like to a block device like /dev/disk/by-id/scsi-XYZ. Or it can be a device path
> +to a char device like /dev/sg5. Or it can be specified in the SCSI notation
> +HOST:CHANNEL:TARGET:LUN. Note that the latter format is unreliable because
> +the HOST value can change across dom0 reboots.
/dev/sg5 might have an unstable name too, in some cases.
You should specify what format HOST CHANNEL TARGET LUN are in. Are
they all decimal integers ? Can we use 0x to specify hex ?
> +=item C<vdev>
> +
> +Specifies how the SCSI device is mapped into the guest. The notation is in
> +SCSI notation HOST:CHANNEL:TARGET:LUN. HOST in this case means a virthal
> +SCSI host within the guest.
I think I don't understand "HOST". Is it an integer ? Does the Xen
PV SCSI interface then offer multiple virtual SCSI hosts ? How many ?
> +=item C<option>
> +
> +Right now only one option is recognized: feature-host.
What effect does that option have ?
> diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> index 30bd4bf..5f89fca 100644
> --- a/docs/man/xl.pod.1
> +++ b/docs/man/xl.pod.1
> @@ -1219,6 +1219,26 @@ List virtual trusted platform modules for a domain.
...
> +=item B<scsi-attach> I<domain-id> I<pdev> I<vdev> I<[,feature-host]>
Does one really have to specify the comma there ?
> +=item B<scsi-detach> I<domain-id> I<vdev>
> +
> +Removes the vscsi device from domain specified by I<domain-id>.
> +Note that the whole virtual SCSI host with all its devices is removed.
> +This is a BUG!
Ouch! Surely that needs to be fixed before we apply the patch ?
Hrm, I see I'm largely duplicating comments made by Ian Campbell.
I'll wait for a repost.
Thanks,
Ian.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libbxl: add support for pvscsi, iteration 1
2014-05-02 15:11 ` Ian Campbell
@ 2014-05-02 15:54 ` Olaf Hering
2014-05-02 16:35 ` Ian Campbell
0 siblings, 1 reply; 8+ messages in thread
From: Olaf Hering @ 2014-05-02 15:54 UTC (permalink / raw)
To: Ian Campbell; +Cc: Ondrej Holecek, Ian Jackson, xen-devel
On Fri, May 02, Ian Campbell wrote:
> > The backend driver uses a single_host:many_devices notation to manage
> > domU devices. The xenstore layout looks like this:
>
> Please can you add ~/device/vscsi/$DEVID/* and
> ~/backend/vif/$DOMID/$DEVID/* to docs/misc/xenstore-paths.markdown.
>
> If you were inclined to improve xen/include/public/io/vscsiif.h by using
> some of the content from here then that would be awesome too.
I think its a good idea to document what the current backend/frontend
code does, as this will also aid with review of the libxl changes.
> > This is a challenge for libxl because libxl currently handles only
> > single_host:single_device in its device helper functions.
>
> This is a bit like pci I suppose?
Maybe. But today Ondrej found a way to attach additional devices,
detaching them will most likely work as well. Basically one creates the
new vscsi-devs/dev-N/ subdir with state==1, then write state==7 to the
backend. Similar with detach, state==5 to the dev-N, then state==7 to
the backend.
> > Due to this
> > limitation in libxl, scsi-detach will remove the whole virtual scsi host
> > with all its virtual devices, instead of just the requested single
> > virtual device.
>
> That's rather unfortunate!
>
> Are multiple <vhost> allowed/supported? e.g. could I (or better, libxl
> by default) setup a device-per-host situation to alleviate this?
>
> For pcifront/back we support bus reconfiguration (via
> XenbusStateReconfigur{ing,ed}) which is how this works there. Does vscsi
> not support anything like that?
Many vhosts are supported, I just did not realize that reconfigure was
used here as well. So maybe libxl does not need further changes. I will
hack some code to show how attach/detach works in theory.
> > To support the pdev notation '/dev/$device' it is required to parse
> > sysfs to translate the path to HST:CHN:TGT:LUN notation. In its current
> > variant /sys/dev/ is used, which is available since at least Linux 3.0.
> > Support for dom0 kernels which lack this interface will be added later.
>
> Are there any such kernels which anyone cares about any more?
I have to check when /sys/dev was introduced. In the end it would be
nice to support also older dom0 kernels.
> > - xl scsi-detach will remove entire scsi hosts, unlike its xm counter part.
>
> What did xm do instead? Fail?
No, it will use the xenstore reconfigure state to indicate a change.
> > Note that the latter format is unreliable because
> > +the HOST value can change across dom0 reboots.
>
> /dev/sgN would be unreliable too I think? As might block devices not
> using the /dev/disk/by-* links you give in the example. You might be
> best saying that only /dev/disk/by-* block device references are
> reliable because all of the others might change over a reboot.
I will reword this part.
One customer reported that his SCSI device was not working with xend
because this device was accessible only via a char device. And I think
udev was able to create some by-id path for the sgN node. Have to dig
into that old report.
> > +
> > +=item C<vdev>
> > +
> > +Specifies how the SCSI device is mapped into the guest. The notation is in
> > +SCSI notation HOST:CHANNEL:TARGET:LUN. HOST in this case means a virthal
>
> Typo "virtual"
>
> > +SCSI host within the guest.
>
> All of HOST:CHANNEL:TARGET:LUN are numbers I think, are they
> hex/dec/oct? Are there limits?
I have to find out, its most likely all decimal.
> > +Right now only one option is recognized: feature-host.
> What does it do?
I have to find out ;-)
Last time I checked the command was passed as is from domU to dom0, but
I dont really know as it is not documented.
> > +=item B<scsi-attach> I<domain-id> I<pdev> I<vdev> I<[,feature-host]>
>
> Is the comma in I<[,feature-host]> literally required, as in
> xl scsi-attach mydom 0:0:0:0 0:0:0:0 ,feature-host
> ?
This is a typo I noticed after submission.
Not sure how to specify the option. Either a '0:0:0:0,feature-host' or
as two strings.
> > +Removes the vscsi device from domain specified by I<domain-id>.
> > +Note that the whole virtual SCSI host with all its devices is removed.
> > +This is a BUG!
>
> I have a feeling it would be preferable in the first instance to either
> just not provide the detach portion of this interface, or to require
> that vdev is only a host and not a specific device (... unless the host
> only has a single device, but maybe that's getting too far)
Attach/detach can probably be implemented without much work.
> > +++ b/tools/libxl/libxl.c
> > +
> > +void libxl__device_vscsi_add(libxl__egc *egc, uint32_t domid,
> > + libxl_device_vscsi *vscsi,
> > + libxl__ao_device *aodev)
> > +{
> > + STATE_AO_GC(aodev->ao);
> > + flexarray_t *front;
> > + flexarray_t *back;
> > + libxl__device *device;
> > + unsigned int rc, i;
> > +
> > + i = 2 * (4 + (3 * vscsi->num_vscsi_devs));
>
> That's quite exciting ;-)
>
> A flexarray will grow as neccessary so some sort of static best guest of
> a starting point would be fine here I think (or a comment about 2 4 and
> 3)!
Yes, that was just an attempt to avoid realloc. As usual, optimization
of non-critical code paths...
> I suppose the num_vscsi_devs here is an artefact of the short coming you
> mentioned before. The way PCI handles this is to create the "bus" (aka
> vhost here) either when the first device is attached or in the higher
> level code before it calls the individual device adds (i.e. this
> function), or something (I must confess my memory is a bit fuzzy,
> libxl__create_pci_backend might be a place to look). Having done that
> then libxl_device_pci is just a single device.
>
> I think you should look to do something similar here, so that each
> libxl_device_vscsi is actually a single device. The alternative is some
> sort of libxl_bus_vscsi thing, which would be unconventional compared
> with everything else
The thing with host:dev[,dev[,dev]] is that within the guest something
may rely on the way SCSI devices are presented (ordering, naming, no
idea). Surely the vscsi= and scsi-attach/detach parsing code could do
some sort of translation from a group of devices-per-vhost into a single
vhost:dev. But now that we have found that reconfiguration is used in
xenstore to attach/detach devices it may be possible to reuse existing
libxl helpers while preserving existing domU.cfg files and backend
drivers.
> > +libxl_device_vscsi *libxl_device_vscsi_list(libxl_ctx *ctx, uint32_t domid, int *num)
> > +{
> > + GC_INIT(ctx);
> > +
> > + libxl_device_vscsi *vscsi_hosts = NULL;
> > + char *fe_path;
> > + char **dir;
> > + unsigned int ndirs = 0;
> > +
> > + fe_path = libxl__sprintf(gc, "%s/device/vscsi", libxl__xs_get_dompath(gc, domid));
>
> Need to take care trusting this too much...
Not sure what you mean with this sentence?
> > @@ -3520,6 +3713,10 @@ DEFINE_DEVICE_REMOVE(vfb, destroy, 1)
> > DEFINE_DEVICE_REMOVE(vtpm, remove, 0)
> > DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
> >
> > +/* vscsi */
> > +DEFINE_DEVICE_REMOVE(vscsi, remove, 0)
> > +DEFINE_DEVICE_REMOVE(vscsi, destroy, 0)
>
> Should the second one not be (vscsi, destroy, 1) ?
Right.
> > +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),
>
> Not defbool (I don't know what this does...)
This is just flag for "feature-host" yes/no. Have to find out what this
backend option actually does.
> > @@ -508,6 +528,28 @@ libxl_vtpminfo = Struct("vtpminfo", [
> > ("uuid", libxl_uuid),
> > ], dir=DIR_OUT)
> >
> > +libxl_vscsiinfo = Struct("vscsiinfo", [
> > + ("backend", string),
> > + ("backend_id", uint32),
> > + ("frontend", string),
> > + ("frontend_id", uint32),
> > + ("devid", libxl_devid),
> > + ("p_hst", uint16),
> > + ("p_chn", uint16),
> > + ("p_tgt", uint16),
> > + ("p_lun", uint16),
>
> Type, also the widths here differ from the device...
>
> > + ("vscsi_dev_id", libxl_devid),
> > + ("v_hst", uint16),
> > + ("v_chn", uint16),
> > + ("v_tgt", uint16),
> > + ("v_lun", uint16),
>
> and again
Some format code was unhappy with u16, cant remember. I was just trying
to save space. Again, optimization of non-critical code paths.
> > +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;
>
> Doesn't seem to be anything vscsi specific here (and it seems like a
> generally useful thing. I expect at least thevif parsing would benefit
> from deploying this too (in so much as all this wouldn't be better with
> a proper parser using flex).
I figured that could be a generic helper. Have to check existing code if
it can make use of it.
> > +}
> > +
> > +static void parse_vscsi_config(libxl_device_vscsi *vscsi_host,
> > + libxl_vscsi_dev *vscsi_dev,
> > + char *buf)
> > +{
> > + char *pdev, *vdev, *fhost;
> > + unsigned int hst, chn, tgt, lun;
> > +
> > + libxl_device_vscsi_init(vscsi_host);
> > + pdev = strtok(buf, ",");
> > + vdev = strtok(NULL, ",");
> > + fhost = strtok(NULL, ",");
>
> I guess we are single threaded here so this is ok.
>
> I wonder if this might be useful to put in libxlu alongside the disk
> parser? The question is whether anything else wants to parse xm style
> vscsi strings, like libvirt perhaps. (arguably a bunch of these parsers
> here have that property and belong in libxlu).
I was wondering how libvirt support for vscsi would look like, looks
like it has to reimplement all that parsing to fill struct
libxl_domain_config. Maybe that parser could be generic, depends how
external callers deal with such parsing today.
> > + pdev = vscsi_trim_string(pdev);
> > + vdev = vscsi_trim_string(vdev);
> > +
> > + if (strncmp(pdev, "/dev/", 5) == 0) {
> > +#ifdef __linux__
>
> Urk. Normally we do this by splitting by files, e.g. into a linux
> version and a nop version and compiling the appropriate one.
>
> This seems like a candidate for libxlu too. We probably need to have a
> think about how much of this stuff is done in libxl too -- e.g. for
> disks we pass the pdev in as a string (path) and interpret it there.
> That allows us to choose between alternative backend providers etc. Also
> libxl is a slightly more palatable place to deal with Linux vs non-Linux
> bits. I think that might be the right answer here too.
There is a libxl_linux.c file, but thats for libxl not xl. So we went
with the ifdef for the time being.
> > @@ -1242,6 +1360,56 @@ static void parse_config_data(const char *config_source,
> > }
> > }
> >
> > + if (!xlu_cfg_get_list(config, "vscsi", &vscsis, 0, 0)) {
> > + int cnt_vscsi_devs = 0;
> > + d_config->num_vscsis = 0;
> > + d_config->vscsis = NULL;
> > + while ((buf = xlu_cfg_get_listitem (vscsis, cnt_vscsi_devs)) != NULL) {
> > + libxl_vscsi_dev vscsi_dev = { };
> > + libxl_device_vscsi vscsi_host = { };
> > + libxl_device_vscsi *host;
> > + char *tmp_buf;
> > + int num_vscsis, host_found = 0;
> > +
> > + /*
> > + * #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
>
> Would all be a lot simpler if a libxl_device_vscsi was a single device
> and all the worrying about which bus was which was in libxl
As said above, domU.cfg code like this may serve a purpose. Devices are
grouped per vhost (0: and 1: in this example):
### vscsi = [
### '/dev/sdm, 0:0:0:0',
### '/dev/sdn, 0:0:0:1',
### '/dev/sdo, 0:0:1:0',
### '/dev/sdp, 0:2:0:1 ',
### '/dev/sdq, 1:0:0:0, feature-host ',
### '/dev/sdr, 1:1:1:0, feature-host',
### ]
Thanks for the quick review!
Olaf
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libbxl: add support for pvscsi, iteration 1
2014-05-02 15:54 ` Ian Jackson
@ 2014-05-02 16:00 ` Olaf Hering
0 siblings, 0 replies; 8+ messages in thread
From: Olaf Hering @ 2014-05-02 16:00 UTC (permalink / raw)
To: Ian Jackson; +Cc: Ondrej Holecek, Ian Campbell, xen-devel
On Fri, May 02, Ian Jackson wrote:
> > +Specifies the dom0 visible SCSI device. The string can be either a device path
> > +like to a block device like /dev/disk/by-id/scsi-XYZ. Or it can be a device path
> > +to a char device like /dev/sg5. Or it can be specified in the SCSI notation
> > +HOST:CHANNEL:TARGET:LUN. Note that the latter format is unreliable because
> > +the HOST value can change across dom0 reboots.
>
> /dev/sg5 might have an unstable name too, in some cases.
>
> You should specify what format HOST CHANNEL TARGET LUN are in. Are
> they all decimal integers ? Can we use 0x to specify hex ?
I will find out, and clearify this part of the docs.
> > +=item C<vdev>
> > +
> > +Specifies how the SCSI device is mapped into the guest. The notation is in
> > +SCSI notation HOST:CHANNEL:TARGET:LUN. HOST in this case means a virthal
> > +SCSI host within the guest.
>
> I think I don't understand "HOST". Is it an integer ? Does the Xen
> PV SCSI interface then offer multiple virtual SCSI hosts ? How many ?
HOST is a number.
Many hosts are allowed, have to check the upper limit. Its likely $HUGE.
> > +=item C<option>
> > +
> > +Right now only one option is recognized: feature-host.
>
> What effect does that option have ?
I have to find out ;-(
Olaf
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libbxl: add support for pvscsi, iteration 1
2014-05-02 15:54 ` Olaf Hering
@ 2014-05-02 16:35 ` Ian Campbell
0 siblings, 0 replies; 8+ messages in thread
From: Ian Campbell @ 2014-05-02 16:35 UTC (permalink / raw)
To: Olaf Hering; +Cc: Ondrej Holecek, Ian Jackson, xen-devel
On Fri, 2014-05-02 at 17:54 +0200, Olaf Hering wrote:
> On Fri, May 02, Ian Campbell wrote:
>
> > > The backend driver uses a single_host:many_devices notation to manage
> > > domU devices. The xenstore layout looks like this:
> >
> > Please can you add ~/device/vscsi/$DEVID/* and
> > ~/backend/vif/$DOMID/$DEVID/* to docs/misc/xenstore-paths.markdown.
> >
> > If you were inclined to improve xen/include/public/io/vscsiif.h by using
> > some of the content from here then that would be awesome too.
>
> I think its a good idea to document what the current backend/frontend
> code does, as this will also aid with review of the libxl changes.
>
> > > This is a challenge for libxl because libxl currently handles only
> > > single_host:single_device in its device helper functions.
> >
> > This is a bit like pci I suppose?
>
> Maybe. But today Ondrej found a way to attach additional devices,
> detaching them will most likely work as well. Basically one creates the
> new vscsi-devs/dev-N/ subdir with state==1, then write state==7 to the
> backend. Similar with detach, state==5 to the dev-N, then state==7 to
> the backend.
This sounds remarkably similar to how the pci stuff works. Glad there is
a least some sort of consistency.
>
> > > Due to this
> > > limitation in libxl, scsi-detach will remove the whole virtual scsi host
> > > with all its virtual devices, instead of just the requested single
> > > virtual device.
> >
> > That's rather unfortunate!
> >
> > Are multiple <vhost> allowed/supported? e.g. could I (or better, libxl
> > by default) setup a device-per-host situation to alleviate this?
> >
> > For pcifront/back we support bus reconfiguration (via
> > XenbusStateReconfigur{ing,ed}) which is how this works there. Does vscsi
> > not support anything like that?
>
> Many vhosts are supported, I just did not realize that reconfigure was
> used here as well. So maybe libxl does not need further changes. I will
> hack some code to show how attach/detach works in theory.
Brilliant!
> > > To support the pdev notation '/dev/$device' it is required to parse
> > > sysfs to translate the path to HST:CHN:TGT:LUN notation. In its current
> > > variant /sys/dev/ is used, which is available since at least Linux 3.0.
> > > Support for dom0 kernels which lack this interface will be added later.
> >
> > Are there any such kernels which anyone cares about any more?
>
> I have to check when /sys/dev was introduced. In the end it would be
> nice to support also older dom0 kernels.
Certainly it would be nice and it's up to you how nice you are
feeling ;-)
>
> > > - xl scsi-detach will remove entire scsi hosts, unlike its xm counter part.
> >
> > What did xm do instead? Fail?
>
> No, it will use the xenstore reconfigure state to indicate a change.
Lets do that ;-)
> > > +Right now only one option is recognized: feature-host.
> > What does it do?
>
> I have to find out ;-)
:-D
> Last time I checked the command was passed as is from domU to dom0, but
> I dont really know as it is not documented.
You have my sympathies.
> > > +=item B<scsi-attach> I<domain-id> I<pdev> I<vdev> I<[,feature-host]>
> >
> > Is the comma in I<[,feature-host]> literally required, as in
> > xl scsi-attach mydom 0:0:0:0 0:0:0:0 ,feature-host
> > ?
>
> This is a typo I noticed after submission.
> Not sure how to specify the option. Either a '0:0:0:0,feature-host' or
> as two strings.
The right answer probably depends on the meaning of feature host and how
closely bound to the concept of a vdev it is, but by default I'd say a
feature ought to be separate from the vdev.
> > I suppose the num_vscsi_devs here is an artefact of the short coming you
> > mentioned before. The way PCI handles this is to create the "bus" (aka
> > vhost here) either when the first device is attached or in the higher
> > level code before it calls the individual device adds (i.e. this
> > function), or something (I must confess my memory is a bit fuzzy,
> > libxl__create_pci_backend might be a place to look). Having done that
> > then libxl_device_pci is just a single device.
> >
> > I think you should look to do something similar here, so that each
> > libxl_device_vscsi is actually a single device. The alternative is some
> > sort of libxl_bus_vscsi thing, which would be unconventional compared
> > with everything else
>
> The thing with host:dev[,dev[,dev]] is that within the guest something
> may rely on the way SCSI devices are presented (ordering, naming, no
> idea). Surely the vscsi= and scsi-attach/detach parsing code could do
> some sort of translation from a group of devices-per-vhost into a single
> vhost:dev.
Sorry I didn't mean to suggest not obeying the users state prefrence for
host:dev etc.
> But now that we have found that reconfiguration is used in
> xenstore to attach/detach devices it may be possible to reuse existing
> libxl helpers while preserving existing domU.cfg files and backend
> drivers.
Great.
> > > +libxl_device_vscsi *libxl_device_vscsi_list(libxl_ctx *ctx, uint32_t domid, int *num)
> > > +{
> > > + GC_INIT(ctx);
> > > +
> > > + libxl_device_vscsi *vscsi_hosts = NULL;
> > > + char *fe_path;
> > > + char **dir;
> > > + unsigned int ndirs = 0;
> > > +
> > > + fe_path = libxl__sprintf(gc, "%s/device/vscsi", libxl__xs_get_dompath(gc, domid));
> >
> > Need to take care trusting this too much...
>
> Not sure what you mean with this sentence?
This is a frontend path, a malicious guest might play games with the
content to try and get the toolstack to do something (e.g. redirecting
the backend path to a directory which the guest controls instead of the
toolstack).
> > > +}
> > > +
> > > +static void parse_vscsi_config(libxl_device_vscsi *vscsi_host,
> > > + libxl_vscsi_dev *vscsi_dev,
> > > + char *buf)
> > > +{
> > > + char *pdev, *vdev, *fhost;
> > > + unsigned int hst, chn, tgt, lun;
> > > +
> > > + libxl_device_vscsi_init(vscsi_host);
> > > + pdev = strtok(buf, ",");
> > > + vdev = strtok(NULL, ",");
> > > + fhost = strtok(NULL, ",");
> >
> > I guess we are single threaded here so this is ok.
> >
> > I wonder if this might be useful to put in libxlu alongside the disk
> > parser? The question is whether anything else wants to parse xm style
> > vscsi strings, like libvirt perhaps. (arguably a bunch of these parsers
> > here have that property and belong in libxlu).
>
> I was wondering how libvirt support for vscsi would look like, looks
> like it has to reimplement all that parsing to fill struct
> libxl_domain_config. Maybe that parser could be generic, depends how
> external callers deal with such parsing today.
FWIW the reason for libxlu_disk was precisely libvirt, although I don't
know if it actually uses it or not.
>
> > > + pdev = vscsi_trim_string(pdev);
> > > + vdev = vscsi_trim_string(vdev);
> > > +
> > > + if (strncmp(pdev, "/dev/", 5) == 0) {
> > > +#ifdef __linux__
> >
> > Urk. Normally we do this by splitting by files, e.g. into a linux
> > version and a nop version and compiling the appropriate one.
> >
> > This seems like a candidate for libxlu too. We probably need to have a
> > think about how much of this stuff is done in libxl too -- e.g. for
> > disks we pass the pdev in as a string (path) and interpret it there.
> > That allows us to choose between alternative backend providers etc. Also
> > libxl is a slightly more palatable place to deal with Linux vs non-Linux
> > bits. I think that might be the right answer here too.
>
> There is a libxl_linux.c file, but thats for libxl not xl. So we went
> with the ifdef for the time being.
I think the right answer is likely to be to put this parsing code, or at
least the translation code, into libxl anyhow.
> > > @@ -1242,6 +1360,56 @@ static void parse_config_data(const char *config_source,
> > > }
> > > }
> > >
> > > + if (!xlu_cfg_get_list(config, "vscsi", &vscsis, 0, 0)) {
> > > + int cnt_vscsi_devs = 0;
> > > + d_config->num_vscsis = 0;
> > > + d_config->vscsis = NULL;
> > > + while ((buf = xlu_cfg_get_listitem (vscsis, cnt_vscsi_devs)) != NULL) {
> > > + libxl_vscsi_dev vscsi_dev = { };
> > > + libxl_device_vscsi vscsi_host = { };
> > > + libxl_device_vscsi *host;
> > > + char *tmp_buf;
> > > + int num_vscsis, host_found = 0;
> > > +
> > > + /*
> > > + * #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
> >
> > Would all be a lot simpler if a libxl_device_vscsi was a single device
> > and all the worrying about which bus was which was in libxl
>
> As said above, domU.cfg code like this may serve a purpose. Devices are
> grouped per vhost (0: and 1: in this example):
That's fine isn't it? You would create a list of 6 libxl_device_vscsi's,
and four of them would have host == 0 and two of them have host == 1.
libxl should be written to cope with that, by creating the host on
demand when the first device demands it, or whatever.
> ### vscsi = [
> ### '/dev/sdm, 0:0:0:0',
> ### '/dev/sdn, 0:0:0:1',
> ### '/dev/sdo, 0:0:1:0',
> ### '/dev/sdp, 0:2:0:1 ',
> ### '/dev/sdq, 1:0:0:0, feature-host ',
> ### '/dev/sdr, 1:1:1:0, feature-host',
> ### ]
>
>
> Thanks for the quick review!
>
> Olaf
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-05-02 16:35 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-04-30 11:03 [PATCH] libbxl: add support for pvscsi, iteration 1 Olaf Hering
2014-05-02 14:36 ` Olaf Hering
2014-05-02 15:30 ` Ian Campbell
2014-05-02 15:11 ` Ian Campbell
2014-05-02 15:54 ` Olaf Hering
2014-05-02 16:35 ` Ian Campbell
2014-05-02 15:54 ` Ian Jackson
2014-05-02 16:00 ` Olaf Hering
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.