From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wei Liu Subject: Re: [PATCH V6 3/7] libxl: add pvusb API Date: Thu, 13 Aug 2015 10:09:38 +0100 Message-ID: <20150813090938.GI7460@zion.uk.xensource.com> References: <1439202928-24813-1-git-send-email-cyliu@suse.com> <1439202928-24813-4-git-send-email-cyliu@suse.com> <20150811112702.GF7460@zion.uk.xensource.com> <55CB1EC10200006600057E99@relay2.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <55CB1EC10200006600057E99@relay2.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Chun Yan Liu Cc: Juergen Gross , wei.liu2@citrix.com, ian.campbell@citrix.com, george.dunlap@eu.citrix.com, Ian.Jackson@eu.citrix.com, xen-devel@lists.xen.org, Jim Fehlig , Simon Cao List-Id: xen-devel@lists.xenproject.org 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@zion.uk.xensource.com>, Wei Liu > 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 > > > Signed-off-by: Simon Cao > > > > > > --- > > > 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? Wei. > Thanks, > Chunyan > > > > > But I'm not very familiar with the AO machinery, I will let Ian comment > > on this. > > > > Wei. > > > >