|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API
>>> On 8/13/2015 at 05:09 PM, in message
<20150813090938.GI7460@xxxxxxxxxxxxxxxxxxxxx>, Wei Liu <wei.liu2@xxxxxxxxxx>
wrote:
> On Tue, Aug 11, 2015 at 08:24:01PM -0600, Chun Yan Liu wrote:
> >
> >
> > >>> On 8/11/2015 at 07:27 PM, in message
> > <20150811112702.GF7460@xxxxxxxxxxxxxxxxxxxxx>, Wei Liu
> > <wei.liu2@xxxxxxxxxx>
> > wrote:
> > > On Mon, Aug 10, 2015 at 06:35:24PM +0800, Chunyan Liu wrote:
> > > > Add pvusb APIs, including:
> > > > - attach/detach (create/destroy) virtual usb controller.
> > > > - attach/detach usb device
> > > > - list usb controller and usb devices
> > > > - some other helper functions
> > > >
> > > > Signed-off-by: Chunyan Liu <cyliu@xxxxxxxx>
> > > > Signed-off-by: Simon Cao <caobosimon@xxxxxxxxx>
> > > >
> > > > ---
> > > > changes:
> > > > - Address George's comments:
> > > > * Update libxl_device_usb_getinfo to read ctrl/port only and
> > > > get other information.
> > > > * Update backend path according to xenstore frontend 'xxx/backend'
> > > > entry instead of using TOOLSTACK_DOMID.
> > > > * Use 'type' to indicate qemu/pv instead of previous naming
> > > > 'protocol'.
>
> > > > * Add USB 'devtype' union, currently only includes "hostdev"
> > > >
> > >
> > > I will leave this to Ian and George since they had strong opinions on
> > > this.
> > >
> > > I only skimmed this patch. Some comments below.
> > >
> > > [...]
> > > > +
> > > > +int libxl_device_usb_getinfo(libxl_ctx *ctx, uint32_t domid,
> > > > + libxl_device_usb *usb,
> > > > + libxl_usbinfo *usbinfo);
> > > > +
> > > > /* Network Interfaces */
> > > > int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid,
> libxl_device_nic
> > > *nic,
> > > > const libxl_asyncop_how *ao_how)
> > > > diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> > > > index bee5ed5..935f25b 100644
> > > > --- a/tools/libxl/libxl_device.c
> > > > +++ b/tools/libxl/libxl_device.c
> > > > @@ -676,6 +676,10 @@ void libxl__devices_destroy(libxl__egc *egc,
> > > libxl__devices_remove_state *drs)
> > > > aodev->action = LIBXL__DEVICE_ACTION_REMOVE;
> > > > aodev->dev = dev;
> > > > aodev->force = drs->force;
> > > > + if (dev->backend_kind == LIBXL__DEVICE_KIND_VUSB) {
> > > > + libxl__initiate_device_usbctrl_remove(egc, aodev);
> > > >
> > > > + continue;
> > > > + }
> > >
> > > Is there a risk that this races with individual device removal? I think
> > > you get away with it because removal of individual device is idempotent?
> >
> > You mean races with other device removal (like 'vbd') ? Yes, it is
> idempotent.
> > Only for 'vusb' (corresponding to USB controller), before removing USB
> controller
> > it will first removing all USB devices under it.
> >
>
> No. What I mean is, the removal of usbctrl triggers removal of all
> assigned usb devices. And then this function initiates removal of
> assigned usb devices again. Is this a possible scenario?
>
> > >
> > > > libxl__initiate_device_remove(egc, aodev);
> > > > }
> > > > }
> > > > diff --git a/tools/libxl/libxl_internal.h
> > > > b/tools/libxl/libxl_internal.h
> > > > index f98f089..5be3b3a 100644
> > > > --- a/tools/libxl/libxl_internal.h
> > > > +++ b/tools/libxl/libxl_internal.h
> > > > @@ -2553,6 +2553,14 @@ _hidden void libxl__device_vtpm_add(libxl__egc
> *egc,
> > > uint32_t domid,
> > > > libxl_device_vtpm *vtpm,
> > > > libxl__ao_device *aodev);
> > > >
> > > > +_hidden void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t
> > > > domid,
> > > > + libxl_device_usbctrl *usbctrl,
> > > > + libxl__ao_device *aodev);
> > > > +
> > > > +_hidden void libxl__device_usb_add(libxl__egc *egc, uint32_t domid,
> > > > + libxl_device_usb *usb,
> > > > + 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);
> > > > @@ -2585,6 +2593,13 @@ _hidden void
> > > libxl__wait_device_connection(libxl__egc*,
> > > > _hidden void libxl__initiate_device_remove(libxl__egc *egc,
> > > > libxl__ao_device *aodev);
> > > >
> > > > +_hidden int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
> > > [...]
> > > > +void libxl__device_usb_add(libxl__egc *egc, uint32_t domid,
> > > > + libxl_device_usb *usb,
> > > > + libxl__ao_device *aodev)
> > > > +{
> > > > + STATE_AO_GC(aodev->ao);
> > > > + int rc = -1;
> > > > + char *busid = NULL;
> > > > +
> > > > + assert(usb->u.hostdev.hostbus > 0 && usb->u.hostdev.hostaddr > 0);
> > > >
> > > > +
> > > > + busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus,
> > > > + usb->u.hostdev.hostaddr);
> > > > + if (!busid) {
> > > > + LOG(ERROR, "USB device doesn't exist in sysfs");
> > > > + goto out;
> > > > + }
> > > > +
> > > > + if (!is_usb_assignable(gc, usb)) {
> > > > + LOG(ERROR, "USB device is not assignable.");
> > > > + goto out;
> > > > + }
> > > > +
> > > > + /* check usb device is already assigned */
> > > > + if (is_usb_assigned(gc, usb)) {
> > > > + LOG(ERROR, "USB device is already attached to a domain.");
> > > > + goto out;
> > > > + }
> > > > +
> > > > + rc = libxl__device_usb_setdefault(gc, domid, usb,
> > > > aodev->update_json);
>
> > > > + if (rc) goto out;
> > > > +
> > > > + rc = libxl__device_usb_add_xenstore(gc, domid, usb,
> aodev->update_json);
> > > > + if (rc) goto out;
> > > > +
> > > > + rc = usbback_dev_assign(gc, usb);
> > > > + if (rc) {
> > > > + libxl__device_usb_remove_xenstore(gc, domid, usb);
> > > > + goto out;
> > > > + }
> > > > +
> > > > + libxl__ao_complete(egc, ao, 0);
> > > > + rc = 0;
> > > > +
> > > > +out:
> > >
> > > You forget to complete ao in failure path.
> >
> > It will complete ao in aodev->callback(egc, aodev) in "out:" section, here:
> > if (rc) aodev->callback(egc, aodev);
> >
>
> I'm still confused by the way it is structured. If aodev->callback
> completes the AO nonetheless, why don't you just call that
> unconditionally?
In general case, it won't call libxl__ao_complete directly. In correct path,
it will call libxl__wait_device_connection (it will wait for front/backend
driver
status change and deal with hotplug script, and then call callback function
'addrmcompelte' or in some path call libxl__ao_compelete to complete the ao);
in error path, it will call aodev->callback function 'addrmcomplete' to complete
the ao.
Here, we try to follow the general routine, so keep the error path handling;
but for correct path, since there is no need to wait for device, so we
explicitly
call libxl__ao_compelte to complete ao. That may keep the function easier to
read? (since it keeps the same framework as others.) I see the pending scsi
patch
series when adding scsi device does in the same way.
Thanks,
Chunyan
>
> Wei.
>
>
> > Thanks,
> > Chunyan
> >
> > >
> > > But I'm not very familiar with the AO machinery, I will let Ian comment
> > > on this.
> > >
> > > Wei.
> > >
> > >
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |