Hi Marc-André Lureau <[email protected]> > Subject: Re: [PATCH v6 7/8] hw/usb/redirect-server: Stream interrupt IN > endpoints > > > Interrupt IN is not a request and an answer in usbredir. The host asks > > once with start_interrupt_receiving. After that it expects a packet > > every time the device has data. > > > > Park one IN packet on the device for each streaming endpoint. A device > > that answers USB_RET_ASYNC holds the packet and completes it when it > > has data. A device that answers USB_RET_NAK gives the packet back at > > once, so park a new one when the bus reports a wakeup on that endpoint. > > A slow timer parks one too, for a device that does not report a wakeup. > > > > Wait for the endpoint's bInterval before parking the next one. Some > > devices answer every time. Without the wait we would ask as fast as > > the main loop runs, and the guest would get no time. > > > > Give every endpoint its own timer and bottom half. We ask each > > endpoint at its own rate. > > > > Signed-off-by: Jamin Lin <[email protected]> > > Message-ID: <[email protected]> > > > > diff --git a/hw/usb/redirect-server.c b/hw/usb/redirect-server.c index > > f81dbae2d0b2..6b2e65c1268e 100644 > > --- a/hw/usb/redirect-server.c > > +++ b/hw/usb/redirect-server.c > > @@ -56,6 +56,7 @@ > > #include "qemu/main-loop.h" > > #include "qemu/module.h" > > #include "migration/vmstate.h" > > +#include "qemu/timer.h" > > #include "qemu/cutils.h" > > #include "hw/core/sysbus.h" > > #include "hw/usb/usb.h" > > @@ -82,16 +83,30 @@ OBJECT_DECLARE_SIMPLE_TYPE(USBRedirServer, > > USB_REDIR_SERVER) #define USBREDIR_SERVER_MAX_EP 32 #define > > USBREDIR_SERVER_EP_IN_BASE 16 > > > > +/* An endpoint number is 4 bits, so 0 to 15. */ #define > > +USBREDIR_SERVER_MAX_EP_NR 16 > > + > > /* > > * The bulk length field is 32 bits, so the host can ask for up to 4 GB. > > * This is the largest transfer accepted. > > */ > > #define USBREDIR_SERVER_MAX_BULK (1 * MiB) > > > > +/* Buffer size for an interrupt IN endpoint before its descriptor is > > +seen. */ #define USBREDIR_SERVER_INTR_DEFAULT_LEN 64 > > + > > +/* > > + * Ask again this often after an endpoint answered NAK. Most devices > > +wake > > + * the bus when data arrives, and then the next ask happens at once. > > +This > > + * timer is for the devices that do not wake the bus. > > + */ > > +#define USBREDIR_SERVER_INTR_RETRY_MS 1000 > > + > > #define USBREDIR_SERVER_CTRL_SETUP 0 > > #define USBREDIR_SERVER_CTRL_STATUS 1 #define > USBREDIR_SERVER_BULK 2 > > #define USBREDIR_SERVER_INTR 3 > > +#define USBREDIR_SERVER_INTR_STREAM 4 > > > > /* Which message answers the host when a control transfer ends. */ > > typedef enum { @@ -120,6 +135,14 @@ typedef struct USBRedirServerPkt { > > int type; > > } USBRedirServerPkt; > > > > +typedef struct USBRedirServerIntrStream { > > + USBRedirServer *s; > > + QEMUTimer *timer; > > + bool started; > > + QEMUBH *bh; > > + uint8_t nr; > > +} USBRedirServerIntrStream; > > + > > struct USBRedirServer { > > SysBusDevice parent_obj; > > > > @@ -140,6 +163,15 @@ struct USBRedirServer { > > bool host_connected; > > bool device_announced; > > > > + /* > > + * Interrupt IN streaming, indexed by endpoint number. Every endpoint > > + * has its own timer and bottom half, because every one wants to be > > + * asked at its own rate. The timer asks the device again, after the > > + * endpoint's interval or after a NAK. The bottom half asks at once > > + * when the device says it has data. > > + */ > > + USBRedirServerIntrStream > intr_stream[USBREDIR_SERVER_MAX_EP_NR]; > > + > > /* In-flight packet tracking */ > > QTAILQ_HEAD(, USBRedirServerPkt) inflight; > > uint64_t next_id; > > @@ -580,6 +612,90 @@ static void > usbredir_server_intr_complete(USBRedirServer *s, > > usbredirparser_do_write(s->parser); > > } > > > > +/* > > + * How long to wait before asking the device again. bInterval means > > + * different things at different speeds: a number of frames at low > > +and > > + * full speed, an exponent at high speed. > > + */ > > +static int64_t usbredir_server_intr_period_ns(USBRedirServer *s, int > > +ep_nr) { > > + uint8_t interval = s->ep_interval[ep_nr + > USBREDIR_SERVER_EP_IN_BASE]; > > + USBDevice *device = usbredir_server_device(s); > > + > > + if (interval == 0) { > > + interval = 1; > > + } > > + > > + if (device && device->speed == USB_SPEED_HIGH) { > > + interval = MIN(interval, 16); > > + return (1LL << (interval - 1)) * 125 * SCALE_US; > > + } > > + > > + return (int64_t)interval * SCALE_MS; } > > + > > +/* > > + * The device answered a parked request. Send the answer to the host, > > +then > > + * ask again, because the host still wants more. Do not ask from > > +here: the > > + * device may answer at once, and this function would call itself > > +over and > > + * over. Arm the timer instead. It also waits for the endpoint's bInterval. > > + */ > > +static void usbredir_server_intr_stream_complete(USBRedirServer *s, > > + > USBRedirServerPkt > > +*rp) { > > + struct usb_redir_interrupt_packet_header resp = rp->intr_hdr; > > + struct usb_redir_interrupt_receiving_status_header st; > > + int ep_nr = resp.endpoint & 0x0f; > > + USBPacket *p = &rp->pkt; > > + int actual = p->actual_length; > > + bool retry = false; > > + int64_t delay_ns; > > + > > + trace_usbredir_server_intr_stream_complete(ep_nr, p->status, > > + actual); > > + > > + switch (p->status) { > > + case USB_RET_SUCCESS: > > + resp.status = usb_redir_success; > > + resp.length = actual; > > + usbredirparser_send_interrupt_packet(s->parser, 0, &resp, > > + actual ? rp->data : > NULL, > > + actual); > > + usbredirparser_do_write(s->parser); > > + break; > > + case USB_RET_NAK: > > + /* nothing to report yet; ask again shortly */ > > + retry = true; > > + break; > > + default: > > + /* > > + * The endpoint failed. There is no data, so send a status > > + * message instead. The host gives it to its guest. > > + * > > + * redirect.c ends the stream only on a stall, so do the same > > + * here. If we end it on any other error, the host still > > + * thinks the stream runs, and it never asks again. > > + */ > > + st.endpoint = resp.endpoint; > > + st.status = usbredir_server_status(p->status); > > + usbredirparser_send_interrupt_receiving_status(s->parser, 0, &st); > > + usbredirparser_do_write(s->parser); > > + if (p->status == USB_RET_STALL) { > > + s->intr_stream[ep_nr].started = false; > > + } > > + break; > > + } > > Minor: for a persistent non-stall error (e.g. a usb-host device returning > ioerror), > the stream stays started and the timer below re-arms at bInterval. > That sends an interrupt_receiving_status every period, 1000 messages a > second for a 1 ms endpoint, Would it make sense to back off with > USBREDIR_SERVER_INTR_RETRY_MS after an error, as is done for NAK? > Thanks for the review and suggestion.
Will make errors other than a stall wait USBREDIR_SERVER_INTR_RETRY_MS before asking again, to cut down the status messages. Jamin > -- > Marc-André Lureau <[email protected]>
