From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chun Yan Liu" Subject: Re: [PATCH V6 3/7] libxl: add pvusb API Date: Mon, 31 Aug 2015 00:10:44 -0600 Message-ID: <55E46064020000660005DC55@relay2.provo.novell.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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150811112702.GF7460@zion.uk.xensource.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: wei.liu2@citrix.com Cc: Juergen Gross , 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 Ian and George, could I have your comments? Thanks, Chunyan >>> 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? > > > 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. > > But I'm not very familiar with the AO machinery, I will let Ian comment > on this. > > Wei. > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel > >