Re: [PATCH v4 3/5] libxl: add support for vscsi

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Apr 17, 2015 at 08:30:58AM +0000, Olaf Hering wrote:
> Port pvscsi support from xend to libxl:
> 
>  vscsi=['pdev,vdev{,options}']
>  xl scsi-attach
>  xl scsi-detach
>  xl scsi-list
> 
> 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>
> ---
>  docs/man/xl.cfg.pod.5                |  55 +++
>  docs/man/xl.pod.1                    |  18 +
>  tools/libxl/Makefile                 |   2 +
>  tools/libxl/libxl.c                  | 441 ++++++++++++++++++++
>  tools/libxl/libxl.h                  |  27 ++
>  tools/libxl/libxl_create.c           |   1 +
>  tools/libxl/libxl_device.c           |   2 +
>  tools/libxl/libxl_internal.h         |  16 +
>  tools/libxl/libxl_types.idl          |  56 +++
>  tools/libxl/libxl_types_internal.idl |   1 +
>  tools/libxl/libxl_vscsi.c            | 271 +++++++++++++
>  tools/libxl/libxlu_vscsi.c           | 750 +++++++++++++++++++++++++++++++++++
>  tools/libxl/libxlutil.h              |  21 +
>  tools/libxl/xl.h                     |   3 +
>  tools/libxl/xl_cmdimpl.c             | 184 ++++++++-
>  tools/libxl/xl_cmdtable.c            |  15 +
>  16 files changed, 1862 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index f936dfc..d395e56 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -510,6 +510,61 @@ 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.

s/dom0/backend/

As in theory you could run an HVM guest with an SCSI device passed in - and
use said backed to pass the SCSI device to another guest.

'as-is' ? I thought there were some filtering done in the backend?

> +
> +Each VSCSI_SPEC_STRING consists of "pdev,vdev[,options]".
> +'pdev' describes the physical device, preferable in a persistant format.

What is 'persistent' format?

> +'vdev' is the domU device in vHOST:CHANNEL:TARGET:LUN notation, all integers.
> +'option' lists additional flags which a backend may recognize.
> +
> +The supported values for "pdev" and "option" depends on the used backend driver:
> +
> +=over 4
> +
> +=item B<Linux pvops>

s/pvops//

> +
> +=over 4
> +
> +=item C<pdev>
> +
> +The backend driver in the pvops kernel is part of the Linux-IO Target framework
> +(LIO). As such the SCSI devices have to be configured first with the tools
> +provided by this framework, such as a xen-scsiback aware targetcli. The "pdev"
> +in domU.cfg has to refer to a config item in that framework instead of the raw
> +device. Ususally this is a WWN in the form of "na.WWN:LUN".

Usually.

> +
> +=item C<option>
> +
> +No options recognized.
> +
> +=back
> +
> +=item B<Linux xenlinux>

xenlinux? Classic Xen?

> +
> +=over 4
> +
> +=item C<pdev>
> +
> +The dom0 device in either /dev/scsidev or pHOST:CHANNEL:TARGET:LUN notation.
> +
> +Its recommended to use persistant names "/dev/disk/by-*/*" to refer to a "pdev".
> +The toolstack will translate this internally to "h:c:t:l" notation, which is how
> +the backend driver will access the device. Using the "h:c:t:l" notation for
> +"pdev" in domU.cfg is discouraged because this value will change across reboots,
> +depending on the detection order in the OS.
> +
> +=item C<option>
> +
> +Currently only the option value "feature-host" is recognized. SCSI command
> +emulation in backend driver is bypassed when "feature-host" is specified.
> +
> +=back
> +
> +=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 16783c8..19bdbfa 100644
> --- a/docs/man/xl.pod.1
> +++ b/docs/man/xl.pod.1
> @@ -1328,6 +1328,24 @@ 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>.
> +
> +=item B<scsi-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/Makefile b/tools/libxl/Makefile
> index 1b16598..79b3867 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 libxl_vnuma.o \
> @@ -122,6 +123,7 @@ AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h _libxl_list.h _paths.h \
>  AUTOSRCS= libxlu_cfg_y.c libxlu_cfg_l.c
>  AUTOSRCS += _libxl_save_msgs_callout.c _libxl_save_msgs_helper.c
>  LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o \
> +	libxlu_vscsi.o \
>  	libxlu_disk_l.o libxlu_disk.o libxlu_vif.o libxlu_pci.o
>  $(LIBXLU_OBJS): CFLAGS += $(CFLAGS_libxenctrl) # For xentoollog.h
>  
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 511eef1..abe7d75 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2014,6 +2014,437 @@ static int libxl__resolve_domid(libxl__gc *gc, const char *name,
>  }
>  
>  /******************************************************************************/
> +
> +static void libxl__device_vscsi_dev_backend_rm(libxl__gc *gc,
> +                                              libxl_vscsi_dev *v,
> +                                              xs_transaction_t t,
> +                                              char *be_path,
> +                                              int dev_wait)
> +{
> +    char *path, *val;
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +
> +    path = GCSPRINTF("%s/vscsi-devs/dev-%u/state", be_path, v->vscsi_dev_id);
> +    val = libxl__xs_read(gc, t, path);
> +    LOG(DEBUG, "%s is %s", path, val);
> +    if (val && strcmp(val, GCSPRINTF("%d", dev_wait)) == 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 {
> +        LOG(ERROR, "%s has %s, expected %d", path, val, dev_wait);
> +    }
> +}
> +
> +static int libxl__device_vscsi_dev_backend_set(libxl__gc *gc,
> +                                               libxl_vscsi_dev *v,
> +                                               flexarray_t *back)
> +{
> +    int rc;
> +    libxl_vscsi_hctl *hctl;
> +
> +    switch (v->pdev.type) {
> +        case LIBXL_VSCSI_PDEV_TYPE_WWN:
> +            flexarray_append_pair(back,
> +                                  GCSPRINTF("vscsi-devs/dev-%u/p-dev", v->vscsi_dev_id),
> +                                  v->pdev.u.wwn.m);
> +            break;
> +        case LIBXL_VSCSI_PDEV_TYPE_HCTL:
> +            hctl = &v->pdev.u.hctl.m;
> +            flexarray_append_pair(back,
> +                                  GCSPRINTF("vscsi-devs/dev-%u/p-dev", v->vscsi_dev_id),
> +                                  GCSPRINTF("%u:%u:%u:%u", hctl->hst, hctl->chn, hctl->tgt, hctl->lun));
> +            break;
> +        case LIBXL_VSCSI_PDEV_TYPE_INVALID:
> +        default:
> +            rc = ERROR_FAIL;
> +            goto out;
> +    }
> +    flexarray_append_pair(back, GCSPRINTF("vscsi-devs/dev-%u/p-devname", v->vscsi_dev_id),
> +                          v->pdev.p_devname);
> +    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),
> +                          GCSPRINTF("%d", XenbusStateInitialising));
> +    rc = 0;
> +out:
> +    return rc;
> +}
> +
> +static int libxl__device_vscsi_new_backend(libxl__egc *egc,
> +                                           libxl__ao_device *aodev,
> +                                           libxl_device_vscsi *vscsi,
> +                                           libxl_domain_config *d_config)
> +{
> +    STATE_AO_GC(aodev->ao);
> +    int rc, i;
> +    flexarray_t *back;
> +    flexarray_t *front;
> +    libxl_vscsi_dev *v;
> +    xs_transaction_t t = XBT_NULL;
> +
> +    /* 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);
> +
> +    flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", aodev->dev->domid));
> +    flexarray_append_pair(back, "online", "1");
> +    flexarray_append_pair(back, "state", GCSPRINTF("%d", XenbusStateInitialising));
> +    flexarray_append_pair(back, "feature-host",
> +                          libxl_defbool_val(vscsi->feature_host) ?
> +                          "1" : "0");
> +
> +    flexarray_append_pair(front, "backend-id", GCSPRINTF("%d", vscsi->backend_domid));
> +    flexarray_append_pair(front, "state", GCSPRINTF("%d", XenbusStateInitialising));
> +
> +    for (i = 0; i < vscsi->num_vscsi_devs; i++) {
> +        v = vscsi->vscsi_devs + i;
> +        if (v->remove)
> +            continue;
> +        rc = libxl__device_vscsi_dev_backend_set(gc, v, back);
> +        if (rc) goto out;

Don't you want 'return rc' as the out will try to call libxl__xs_transaction_abort?

> +    }
> +
> +    for (;;) {
> +        rc = libxl__xs_transaction_start(gc, &t);
> +        if (rc) goto out;
> +
> +        rc = libxl__device_exists(gc, t, aodev->dev);
> +        if (rc < 0) goto out;
> +        if (rc == 1) {              /* already exists in xenstore */
> +            LOG(ERROR, "device already exists in xenstore");
> +            rc = ERROR_DEVICE_EXISTS;
> +            goto out;
> +        }
> +
> +        if (aodev->update_json) {
> +            rc = libxl__set_domain_configuration(gc, aodev->dev->domid, d_config);
> +            if (rc) goto out;
> +        }
> +
> +        libxl__device_generic_add(gc, t, aodev->dev,
> +                                  libxl__xs_kvs_of_flexarray(gc, back,
> +                                                             back->count),
> +                                  libxl__xs_kvs_of_flexarray(gc, front,
> +                                                             front->count),
> +                                  NULL);
> +
> +        rc = libxl__xs_transaction_commit(gc, &t);
> +        if (!rc) break;
> +        if (rc < 0) goto out;
> +    }
> +
> +    libxl__wait_device_connection(egc, aodev);
> +    rc = 0;
> +
> +out:
> +    libxl__xs_transaction_abort(gc, &t);
> +    return rc;
> +}

Stopped here as I had to run to a meeting.