From: "Chun Yan Liu" <cyliu@suse.com>
To: wei.liu2@citrix.com
Cc: Juergen Gross <JGross@suse.com>,
ian.campbell@citrix.com, george.dunlap@eu.citrix.com,
Ian.Jackson@eu.citrix.com, xen-devel@lists.xen.org,
Jim Fehlig <JFEHLIG@suse.com>, Simon Cao <caobosimon@gmail.com>
Subject: Re: [PATCH V6 3/7] libxl: add pvusb API
Date: Thu, 13 Aug 2015 19:49:41 -0600 [thread overview]
Message-ID: <55CDB9B5020000660005910B@relay2.provo.novell.com> (raw)
In-Reply-To: <20150813090938.GI7460@zion.uk.xensource.com>
>>> On 8/13/2015 at 05:09 PM, in message
<20150813090938.GI7460@zion.uk.xensource.com>, Wei Liu <wei.liu2@citrix.com>
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@zion.uk.xensource.com>, Wei Liu <wei.liu2@citrix.com>
> > 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@suse.com>
> > > > Signed-off-by: Simon Cao <caobosimon@gmail.com>
> > > >
> > > > ---
> > > > 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.
h >
>
> 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?
No, it's not possible. libxl__devices_destroy is used in domain destroy, it's
trying to scan each device type in xenstore and destroy them. Since USB device
is NOT presented as a separate device type but inside USB controller (which is
represented by a 'vusb' device in xenstore), so when scanning 'vusb' type, it
tries to destroy USB controller, within that it will destroy all USB devices under
that controller. No entry to remove USB device alone.
Thanks,
Chunyan
>
> > >
> > > > 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.
> > >
> > >
>
>
next prev parent reply other threads:[~2015-08-14 1:49 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-08-10 10:35 [PATCH V6 0/7] xen pvusb toolstack work Chunyan Liu
2015-08-10 10:35 ` [PATCH V6 1/7] libxl: export some functions for pvusb use Chunyan Liu
2015-08-11 11:26 ` Wei Liu
2015-08-10 10:35 ` [PATCH V6 2/7] libxl_read_file_contents: add new entry to read sysfs file Chunyan Liu
2015-08-11 11:26 ` Wei Liu
2015-08-12 2:37 ` Chun Yan Liu
2015-08-13 9:11 ` Wei Liu
2015-08-10 10:35 ` [PATCH V6 3/7] libxl: add pvusb API Chunyan Liu
2015-08-11 11:27 ` Wei Liu
2015-08-12 2:24 ` Chun Yan Liu
2015-08-13 9:09 ` Wei Liu
2015-08-14 1:49 ` Chun Yan Liu [this message]
2015-08-18 2:31 ` Chun Yan Liu
2015-08-31 6:10 ` Chun Yan Liu
2015-09-08 14:17 ` Ian Campbell
2015-09-08 16:52 ` George Dunlap
2015-09-09 7:38 ` Chun Yan Liu
2015-09-17 8:19 ` Chun Yan Liu
2015-09-17 9:54 ` George Dunlap
2015-09-29 17:19 ` Wei Liu
2015-09-17 8:20 ` Chun Yan Liu
2015-09-11 5:42 ` Chun Yan Liu
2015-09-11 13:26 ` Ian Campbell
2015-09-11 13:55 ` Juergen Gross
2015-09-11 14:09 ` Ian Campbell
2015-09-11 14:18 ` Juergen Gross
2015-09-11 14:41 ` Ian Campbell
2015-09-11 15:42 ` Ian Jackson
2015-09-14 3:48 ` Juergen Gross
2015-09-14 10:36 ` George Dunlap
2015-09-14 10:53 ` Juergen Gross
2015-09-14 11:12 ` Ian Jackson
2015-09-14 11:23 ` Juergen Gross
2015-09-14 14:03 ` George Dunlap
2015-09-17 8:24 ` Chun Yan Liu
2015-09-15 8:14 ` Chun Yan Liu
2015-08-10 10:35 ` [PATCH V6 4/7] libxl: add libxl_device_usb_assignable_list API Chunyan Liu
2015-08-10 10:35 ` [PATCH V6 5/7] xl: add pvusb commands Chunyan Liu
2015-08-10 10:35 ` [PATCH V6 6/7] xl: add usb-assignable-list command Chunyan Liu
2015-08-10 10:35 ` [PATCH V6 7/7] domcreate: support pvusb in configuration file Chunyan Liu
2015-08-11 11:27 ` Wei Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55CDB9B5020000660005910B@relay2.provo.novell.com \
--to=cyliu@suse.com \
--cc=Ian.Jackson@eu.citrix.com \
--cc=JFEHLIG@suse.com \
--cc=JGross@suse.com \
--cc=caobosimon@gmail.com \
--cc=george.dunlap@eu.citrix.com \
--cc=ian.campbell@citrix.com \
--cc=wei.liu2@citrix.com \
--cc=xen-devel@lists.xen.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.