[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 3/5] libxl: add support for vscsi
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@xxxxxxxxx> > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx> > Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx> > Cc: Ian Campbell <ian.campbell@xxxxxxxxxx> > Cc: Wei Liu <wei.liu2@xxxxxxxxxx> > --- > 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. > + > +Each VSCSI_SPEC_STRING consists of "pdev,vdev[,options]". > +'pdev' describes the physical device, preferable in a persistant format. "persistent", and please explain when "persistent format" is. > +'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> > + > +=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" What is "targetcli"? > +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". > + > +=item C<option> > + > +No options recognized. > + > +=back > + > +=item B<Linux xenlinux> > + > +=over 4 > + > +=item C<pdev> > + > +The dom0 device in either /dev/scsidev or pHOST:CHANNEL:TARGET:LUN notation. > + What is vHOST? What is pHOST? > +Its recommended to use persistant names "/dev/disk/by-*/*" to refer to a > "pdev". "It's" "persistent" > +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, const char *be_path > + int dev_wait) > +{ > + char *path, *val; > + libxl_ctx *ctx = libxl__gc_owner(gc); You can just use CTX macro. > + > + 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); Some lines are too long. Please wrap them properly. But can you not simplify this by removing be_path all together? Also this function should return error number -- you probably don't want caller to commence should things go wrong here. > + } 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)); Please fix the lines that are too long. > + 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)); > + Lines too long. > + 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; > + } > + > + 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; > +} > + > +static int libxl__device_vscsi_reconfigure(libxl__egc *egc, > + libxl__ao_device *aodev, > + libxl_device_vscsi *vscsi, > + libxl_domain_config *d_config, > + char *be_path, const char * > + int *dev_wait) > +{ > + STATE_AO_GC(aodev->ao); > + int rc, i, be_state, be_wait; > + char *dev_path, *state_path, *state_val; > + flexarray_t *back; > + libxl_vscsi_dev *v; > + xs_transaction_t t = XBT_NULL; > + bool do_reconfigure = false; > + > + /* Prealloc key+value: 1 toplevel + 4 per device */ > + i = 2 * (1 + (4 * vscsi->num_vscsi_devs)); > + back = flexarray_make(gc, i, 1); > + > + state_path = GCSPRINTF("%s/state", be_path); > + > + for (;;) { > + rc = libxl__xs_transaction_start(gc, &t); > + if (rc) goto out; > + > + state_val = libxl__xs_read(gc, t, state_path); > + LOG(DEBUG, "%s is %s", state_path, state_val); > + if (!state_val) { > + rc = ERROR_FAIL; > + goto out; > + } > + > + be_state = atoi(state_val); > + switch (be_state) { > + case XenbusStateUnknown: > + case XenbusStateInitialising: > + case XenbusStateClosing: > + case XenbusStateClosed: > + default: > + /* The backend is in a bad state */ > + rc = ERROR_FAIL; > + goto out; > + case XenbusStateInitialised: > + case XenbusStateReconfiguring: > + case XenbusStateReconfigured: > + /* Backend is still busy, caller has to retry */ > + rc = ERROR_NOT_READY; > + goto out; > + case XenbusStateInitWait: > + /* The frontend did not connect yet */ > + be_wait = XenbusStateInitWait; > + if (dev_wait) > + *dev_wait = XenbusStateInitialising; > + do_reconfigure = false; > + break; > + case XenbusStateConnected: > + /* The backend can handle reconfigure */ > + be_wait = XenbusStateConnected; > + if (dev_wait) > + *dev_wait = XenbusStateClosed; > + flexarray_append_pair(back, "state", GCSPRINTF("%d", > XenbusStateReconfiguring)); > + do_reconfigure = true; > + break; > + } > + > + /* Append new device or trigger removal */ > + for (i = 0; i < vscsi->num_vscsi_devs; i++) { > + v = vscsi->vscsi_devs + i; > + unsigned int nb = 0; Move this line at the beginning of this block. I think you will hit a gcc warning here, won't you? > + dev_path = GCSPRINTF("%s/vscsi-devs/dev-%u", be_path, > v->vscsi_dev_id); > + /* Preserve existing device */ > + if (libxl__xs_directory(gc, XBT_NULL, dev_path, &nb) && nb) { > + /* Trigger device removal by forwarding state to > XenbusStateClosing */ > + if (do_reconfigure && v->remove) > + flexarray_append_pair(back, > + > GCSPRINTF("vscsi-devs/dev-%u/state", v->vscsi_dev_id), > + GCSPRINTF("%d", > XenbusStateClosing)); > + continue; > + } > + rc = libxl__device_vscsi_dev_backend_set(gc, v, back); > + if (rc) goto out; > + } > + > + if (aodev->update_json) { > + rc = libxl__set_domain_configuration(gc, aodev->dev->domid, > d_config); > + if (rc) goto out; > + } > + > + libxl__xs_writev(gc, t, be_path, > + libxl__xs_kvs_of_flexarray(gc, back, back->count)); > + > + rc = libxl__xs_transaction_commit(gc, &t); > + if (!rc) break; > + if (rc < 0) goto out; > + } > + > + if (do_reconfigure) { > + /* Poll for backend change */ > + rc = libxl__wait_for_backend(gc, be_path, GCSPRINTF("%d", be_wait)); > + if (rc) goto out; > + } > + rc = 0; > + > +out: > + LOG(ERROR, "%u: rc %d", __LINE__, rc); Remove this line. > + libxl__xs_transaction_abort(gc, &t); > + 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); > + libxl__device *device; > + char *be_path; > + unsigned int be_dirs = 0; > + int rc; > + libxl_domain_config d_config; > + libxl_device_vscsi vscsi_saved; > + libxl__domain_userdata_lock *lock = NULL; > + > + libxl_domain_config_init(&d_config); > + > + /* In case of vscsi the copy remains identical to the provided input */ This comment is confusing. > + libxl_device_vscsi_init(&vscsi_saved); > + libxl_device_vscsi_copy(CTX, &vscsi_saved, vscsi); > + > + if (vscsi->devid == -1) { > + if ((vscsi->devid = libxl__device_nextid(gc, domid, "vscsi")) < 0) { > + rc = ERROR_FAIL; > + goto out; > + } > + } > + > + /* Adjust copy */ > + libxl__update_config_vscsi(gc, &vscsi_saved, vscsi); > + > + GCNEW(device); > + rc = libxl__device_from_vscsi(gc, domid, vscsi, device); > + if (rc) goto out; > + > + if (aodev->update_json) { > + lock = libxl__lock_domain_userdata(gc, domid); > + if (!lock) { > + rc = ERROR_LOCK_FAIL; > + goto out; > + } > + > + rc = libxl__get_domain_configuration(gc, domid, &d_config); > + if (rc) goto out; > + > + /* Replace or append the copy to the domain config */ > + DEVICE_ADD(vscsi, vscsis, domid, &vscsi_saved, COMPARE_VSCSI, > &d_config); > + } > + > + aodev->dev = device; > + > + be_path = libxl__device_backend_path(gc, aodev->dev); > + if (libxl__xs_directory(gc, XBT_NULL, be_path, &be_dirs)) { This looks bogus. Shouldn't you use a real transaction here and in the following two functions? > + rc = libxl__device_vscsi_reconfigure(egc, aodev, vscsi, &d_config, > be_path, NULL); > + if (rc) > + goto out; > + /* Notify that this is done */ > + aodev->callback(egc, aodev); And this is for? You can just do this in out path, right? > + } else { > + rc = libxl__device_vscsi_new_backend(egc, aodev, vscsi, &d_config); > + if (rc) > + goto out; > + } > + > + rc = 0; > +out: > + if (lock) libxl__unlock_domain_userdata(lock); > + libxl_device_vscsi_dispose(&vscsi_saved); > + libxl_domain_config_dispose(&d_config); > + aodev->rc = rc; > + if (rc) aodev->callback(egc, aodev); > + return; > +} > + > +static void libxl__device_vscsi_dev_rm(libxl__egc *egc, > + libxl_device_vscsi *vscsi, > + libxl__ao_device *aodev) > +{ > + STATE_AO_GC(aodev->ao); > + char *be_path; > + int rc, i, dev_wait; > + libxl_domain_config d_config; > + libxl__domain_userdata_lock *lock = NULL; > + libxl_vscsi_dev *v; > + xs_transaction_t t = XBT_NULL; > + > + libxl_domain_config_init(&d_config); > + > + if (vscsi->devid == -1) { > + rc = ERROR_INVAL; > + goto out; > + } > + > + /* No other code will traverse device list, update json with removal > info */ > + if (aodev->update_json) { When is update_json set to true? Note that in DEFINE_DEVICE_ADD update_json is set to true. But in you patch there doesn't seem to be code that does this. > + lock = libxl__lock_domain_userdata(gc, aodev->dev->domid); > + if (!lock) { > + rc = ERROR_LOCK_FAIL; > + goto out; > + } > + > + rc = libxl__get_domain_configuration(gc, aodev->dev->domid, > &d_config); > + if (rc) goto out; > + > + /* Replace the item in the domain config */ > + DEVICE_ADD(vscsi, vscsis, aodev->dev->domid, vscsi, COMPARE_VSCSI, > &d_config); > + } > + Actually, what fields inside vscsi structure are updated in this case? > + be_path = libxl__device_backend_path(gc, aodev->dev); > + rc = libxl__device_vscsi_reconfigure(egc, aodev, vscsi, &d_config, > be_path, &dev_wait); > + if (rc) goto out; > + > + for (;;) { > + rc = libxl__xs_transaction_start(gc, &t); > + if (rc) goto out; > + > + for (i = 0; i < vscsi->num_vscsi_devs; i++) { > + v = vscsi->vscsi_devs + i; > + if (v->remove) > + libxl__device_vscsi_dev_backend_rm(gc, v, t, be_path, > dev_wait); > + } > + > + rc = libxl__xs_transaction_commit(gc, &t); > + if (!rc) break; > + if (rc < 0) goto out; > + } > + > + rc = 0; > + > +out: > + if (lock) libxl__unlock_domain_userdata(lock); > + libxl_domain_config_dispose(&d_config); > + aodev->rc = rc; > + /* Notify that this is done */ > + aodev->callback(egc, aodev); > +} > + > +/* Extended variant of DEFINE_DEVICE_REMOVE to handle reconfigure */ I have a very dumb question, what is "reconfigure"? Is this property of vscsi device? What does this suppose to do? > + libxl_device_vscsi *vscsi, > + const libxl_asyncop_how *ao_how) > +{ > + AO_CREATE(ctx, domid, ao_how); > + libxl__device *device; > + libxl__ao_device *aodev; > + int rc, i; > + unsigned int remaining = vscsi->num_vscsi_devs; > + > + GCNEW(device); > + rc = libxl__device_from_vscsi(gc, domid, vscsi, device); > + if (rc != 0) goto out; > + > + GCNEW(aodev); > + libxl__prepare_ao_device(ao, aodev); > + aodev->action = LIBXL__DEVICE_ACTION_REMOVE; > + aodev->dev = device; > + aodev->callback = device_addrm_aocomplete; > + aodev->force = 0; > + > + for (i = 0; i < vscsi->num_vscsi_devs; i++) { > + if (vscsi->vscsi_devs[i].remove) > + remaining--; > + } > + > + LOG(DEBUG, "%u: v_hst %u, %u of %u remaining", domid, vscsi->v_hst, > remaining, vscsi->num_vscsi_devs); > + if (remaining) > + libxl__device_vscsi_dev_rm(egc, vscsi, aodev); > + else > + libxl__initiate_device_remove(egc, aodev); > + > +out: > + if (rc) return AO_ABORT(rc); > + return AO_INPROGRESS; > +} > + The rest of this patch is mostly parser, declarations and xl stuff. I've omitted the reset for now... Wei. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |