|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] libbxl: add support for pvscsi, iteration 1
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@xxxxxxxxx>
> Cc: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
> Cc: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> Cc: Ondrej Holecek <oholecek@xxxxxxxx>
> ---
> 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.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |