Re: [Qemu-devel] [PATCH v3 3/3] linux-user: Implement special usbfs ioctls.
On Thu, Oct 18, 2018 at 11:48 AM Laurent Vivier wrote:
>
> Le 08/10/2018 à 18:35, Cortland Tölva a écrit :
> > Userspace submits a USB Request Buffer to the kernel, optionally
> > discards it, and finally reaps the URB. Thunk buffers from target
> > to host and back.
> >
> > Tested by running an i386 scanner driver on ARMv7 and by running
> > the PowerPC lsusb utility on x86_64. The discardurb ioctl is
> > not exercised in these tests.
> >
> > Signed-off-by: Cortland Tölva
> > ---
> > There are two alternatives for the strategy of holding lock_user on
> > memory from submit until reap. v3 of this series tries to determine
> > the access permissions for user memory from endpoint direction, but
> > the logic for this is complex. The first alternative is to request
> > write access. If that fails, request read access. If that fails, try
> > to submit the ioctl with no buffer - perhaps the user code filled in
> > fields the kernel will ignore. The second alternative is to read user
> > memory into an allocated buffer, pass it to the kernel, and write back
> > to target memory only if the kernel indicates that writes occurred.
> >
> > Changes from v1:
> > improve pointer cast to int compatibility
> > remove unimplemented types for usb streams
> > struct definitions moved to this patch where possible
> >
> > Changes from v2:
> > organize urb thunk metadata in a struct
> > hold lock_user from submit until discard
> > fixes for 64-bit hosts
> >
> > linux-user/ioctls.h| 8 ++
> > linux-user/syscall.c | 177
> > +
> > linux-user/syscall_defs.h | 4 +
> > linux-user/syscall_types.h | 20 +
> > 4 files changed, 209 insertions(+)
> >
> > diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h
> > index 92f6177f1d..ae8951625f 100644
> ...
> > index 2641260186..9b7ea96cfb 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -96,6 +96,7 @@
> > #include
> > #if defined(CONFIG_USBFS)
> > #include
> > +#include
> > #endif
> > #include
> > #include
> > @@ -4199,6 +4200,182 @@ static abi_long do_ioctl_ifconf(const IOCTLEntry
> > *ie, uint8_t *buf_temp,
> > return ret;
> > }
> >
> > +#if defined(CONFIG_USBFS)
> > +#if HOST_LONG_BITS > 64
> > +#error USBDEVFS thunks do not support >64 bit hosts yet.
> > +#endif
> > +struct live_urb {
> > +uint64_t target_urb_adr;
> > +uint64_t target_buf_adr;
> > +char *target_buf_ptr;
> > +struct usbdevfs_urb host_urb;
> > +};
> > +
> > +static GHashTable *usbdevfs_urb_hashtable(void)
> > +{
> > +static GHashTable *urb_hashtable;
> > +
> > +if (!urb_hashtable) {
> > +urb_hashtable = g_hash_table_new(g_int64_hash, g_int64_equal);
g_int64_hash means this GHashTable expects to be given a pointer to an int64_t,
and the int64_t is used as the key.
> > +}
> > +return urb_hashtable;
> > +}
> > +
> > +static void urb_hashtable_insert(struct live_urb *urb)
> > +{
> > +GHashTable *urb_hashtable = usbdevfs_urb_hashtable();
> > +g_hash_table_insert(urb_hashtable, urb, urb);
>
> Here the key of the hashtable seems to be the pointer to the host live_urb.
>
The key is pointed to by urb. It is the first member of the struct,
target_urb_adr.
> > +}
> > +
> > +static struct live_urb *urb_hashtable_lookup(uint64_t target_urb_adr)
> > +{
> > +GHashTable *urb_hashtable = usbdevfs_urb_hashtable();
> > +return g_hash_table_lookup(urb_hashtable, _urb_adr);
>
> And here the key is the pointer to the target_urb_adr
>
target_urb_adr is on the stack here, and it contains the key. Both
g_hash_table_lookup
and g_hash_table_insert take a pointer to the key, which is an int64_t
in this code.
> So I think urb_hashtable_insert() should be:
>
> g_hash_table_insert(urb_hashtable, urb->target_urb_adr, urb);
>
> and urb_hashtable_lookup() should be:
>
> return g_hash_table_lookup(urb_hashtable, target_urb_adr);
> ...
> Did I miss something?
My code might have been clearer if urb_hashtable_insert and
urb_hashtable_lookup had the same argument type. However,
I did not want to treat target_urb_adr as the first element in a
struct live_urb until checking that it was tracked in the hashtable.
The thing we are looking up has to be a target-sized pointer, so I
cannot use the g_direct_hash with host-sized pointers as keys.
The next best option was to use int64_t as the key.
>
> Thanks,
> Laurent
Thanks,
Cortland.
Re: [Qemu-devel] [PATCH v3 3/3] linux-user: Implement special usbfs ioctls.
ping, please review v3, patch 3/3.
http://patchwork.ozlabs.org/patch/980667/
On Mon, Oct 8, 2018 at 9:35 AM Cortland Tölva wrote:
>
> Userspace submits a USB Request Buffer to the kernel, optionally
> discards it, and finally reaps the URB. Thunk buffers from target
> to host and back.
>
> Tested by running an i386 scanner driver on ARMv7 and by running
> the PowerPC lsusb utility on x86_64. The discardurb ioctl is
> not exercised in these tests.
>
> Signed-off-by: Cortland Tölva
> ---
> There are two alternatives for the strategy of holding lock_user on
> memory from submit until reap. v3 of this series tries to determine
> the access permissions for user memory from endpoint direction, but
> the logic for this is complex. The first alternative is to request
> write access. If that fails, request read access. If that fails, try
> to submit the ioctl with no buffer - perhaps the user code filled in
> fields the kernel will ignore. The second alternative is to read user
> memory into an allocated buffer, pass it to the kernel, and write back
> to target memory only if the kernel indicates that writes occurred.
>
> Changes from v1:
> improve pointer cast to int compatibility
> remove unimplemented types for usb streams
> struct definitions moved to this patch where possible
>
> Changes from v2:
> organize urb thunk metadata in a struct
> hold lock_user from submit until discard
> fixes for 64-bit hosts
>
> linux-user/ioctls.h| 8 ++
> linux-user/syscall.c | 177
> +
> linux-user/syscall_defs.h | 4 +
> linux-user/syscall_types.h | 20 +
> 4 files changed, 209 insertions(+)
>
> diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h
> index 92f6177f1d..ae8951625f 100644
> --- a/linux-user/ioctls.h
> +++ b/linux-user/ioctls.h
> @@ -143,6 +143,14 @@
>IOCTL(USBDEVFS_SETCONFIGURATION, IOC_W, MK_PTR(TYPE_INT))
>IOCTL(USBDEVFS_GETDRIVER, IOC_R,
> MK_PTR(MK_STRUCT(STRUCT_usbdevfs_getdriver)))
> + IOCTL_SPECIAL(USBDEVFS_SUBMITURB, IOC_W, do_ioctl_usbdevfs_submiturb,
> + MK_PTR(MK_STRUCT(STRUCT_usbdevfs_urb)))
> + IOCTL_SPECIAL(USBDEVFS_DISCARDURB, IOC_RW, do_ioctl_usbdevfs_discardurb,
> + MK_PTR(MK_STRUCT(STRUCT_usbdevfs_urb)))
> + IOCTL_SPECIAL(USBDEVFS_REAPURB, IOC_R, do_ioctl_usbdevfs_reapurb,
> + MK_PTR(TYPE_PTRVOID))
> + IOCTL_SPECIAL(USBDEVFS_REAPURBNDELAY, IOC_R, do_ioctl_usbdevfs_reapurb,
> + MK_PTR(TYPE_PTRVOID))
>IOCTL(USBDEVFS_DISCSIGNAL, IOC_W,
> MK_PTR(MK_STRUCT(STRUCT_usbdevfs_disconnectsignal)))
>IOCTL(USBDEVFS_CLAIMINTERFACE, IOC_W, MK_PTR(TYPE_INT))
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 2641260186..9b7ea96cfb 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -96,6 +96,7 @@
> #include
> #if defined(CONFIG_USBFS)
> #include
> +#include
> #endif
> #include
> #include
> @@ -4199,6 +4200,182 @@ static abi_long do_ioctl_ifconf(const IOCTLEntry *ie,
> uint8_t *buf_temp,
> return ret;
> }
>
> +#if defined(CONFIG_USBFS)
> +#if HOST_LONG_BITS > 64
> +#error USBDEVFS thunks do not support >64 bit hosts yet.
> +#endif
> +struct live_urb {
> +uint64_t target_urb_adr;
> +uint64_t target_buf_adr;
> +char *target_buf_ptr;
> +struct usbdevfs_urb host_urb;
> +};
> +
> +static GHashTable *usbdevfs_urb_hashtable(void)
> +{
> +static GHashTable *urb_hashtable;
> +
> +if (!urb_hashtable) {
> +urb_hashtable = g_hash_table_new(g_int64_hash, g_int64_equal);
> +}
> +return urb_hashtable;
> +}
> +
> +static void urb_hashtable_insert(struct live_urb *urb)
> +{
> +GHashTable *urb_hashtable = usbdevfs_urb_hashtable();
> +g_hash_table_insert(urb_hashtable, urb, urb);
> +}
> +
> +static struct live_urb *urb_hashtable_lookup(uint64_t target_urb_adr)
> +{
> +GHashTable *urb_hashtable = usbdevfs_urb_hashtable();
> +return g_hash_table_lookup(urb_hashtable, _urb_adr);
> +}
> +
> +static void urb_hashtable_remove(struct live_urb *urb)
> +{
> +GHashTable *urb_hashtable = usbdevfs_urb_hashtable();
> +g_hash_table_remove(urb_hashtable, urb);
> +}
> +
> +static abi_long
> +do_ioctl_usbdevfs_reapurb(const IOCTLEntry *ie, uint8_t *buf_temp,
> + int fd, int cmd, abi_long arg)
> +{
> +const argtype usbfsurb_arg_type[] = { MK_STRUCT(STRUCT_usbdevfs_urb) };
> +const argtype ptrvoid_arg_type[] = { TYPE_PTRVOID, 0, 0 };
> +struct live_urb *lurb;
> +void *argptr;
> +uint64_t hurb;
> +int target_size;
> +uintptr_t target_urb_adr;
> +abi_long ret;
> +
> +target_size = thunk_type_size(usbfsurb_arg_type, THUNK_TARGET);
> +
> +memset(buf_temp, 0, sizeof(uint64_t));
> +ret = get_errno(safe_ioctl(fd, ie->host_cmd, buf_temp));
> +if (is_error(ret)) {
> +return ret;
> +}
> +
> +memcpy(, buf_temp, sizeof(uint64_t));
> +lurb = (void *)((uintptr_t)hurb - offsetof(struct live_urb, host_urb));
> +
Re: [Qemu-devel] [PATCH 0/2] linux-user: usbfs improvements
On Sun, Oct 7, 2018 at 11:59 PM Laurent Vivier wrote: > > Le 08/10/2018 à 06:27, Cortland Tölva a écrit : > > From: Cortland Setlow Tölva > > > > This patch series enables programs running under QEMU Linux user mode > > emulation to implement user-space USB drivers via the USBFS ioctl()s. > > Support is limited to control, bulk, and possibly interrupt transfers. > > > > Usbfs ioctl codes were incorrect whenever host and target disagreed on > > struct size. The submit, discard, and reap usbfs ioctls require special > > memory buffer handling and the second commit implements this but not for > > USB3 streams or isochronous transfers. > > > > Cortland Tölva (2): > > linux-user: Use calculated sizes for usbfs ioctls. > > linux-user: Implement special usbfs ioctls. > > > > linux-user/ioctls.h| 8 ++ > > linux-user/syscall.c | 177 + > > linux-user/syscall_defs.h | 42 - > > linux-user/syscall_types.h | 20 + > > 4 files changed, 227 insertions(+), 20 deletions(-) > > > > As I didn't push the previous series, could you merge the both in one on > top of master? Yes, I will do this as a v2 of this patch series. The version here is based on your linux user tree as of last night. > > I think it would be clearer. > > Thanks, > Laurent
Re: [Qemu-devel] [PATCH 1/3] Check for Linux USBFS in configure
Hi Laurent, The deeper view on part 3 can wait - I have found an issue with 64-bit hosts. In linux-user/syscall_defs.h the references to struct usbdevfs_urb and so forth are incorrect and I need to define struct target_usbdevfs_urb for the size and the ioctl code to be calculated with the target's ABI. I will send updated patches. --CST On Mon, Sep 24, 2018 at 10:22 AM Laurent Vivier wrote: > Le 23/09/2018 à 06:44, Blake Tölva a écrit : > > Thanks Laurent, I have updated the patch. Should I resend the whole > > series to the list? > > yes. > > > I believe the third patch may also need revision as the call to > > thunk_convert at the end > > of do_ioctl_usbdevfs_reapurb seems clunky. Would love some comments. > > > I didn't read your comment before reviewing this patch. I'm going to > have deeper view on this part. > > Thanks, > Laurent >
