Hi Marc-André Lureau > Subject: Re: [PATCH v6 6/8] hw/usb/redirect-server: Implement bulk and > interrupt transfers > > > Send bulk and interrupt packets to the device and return the result to > > the host. Each one is a request and an answer, so they reuse the > > packet tracking added for control transfers. > > > > A bulk request carries a 32 bit length, so the host can ask us to > > allocate up to 4 GB. Refuse a request that big. Do not answer it with > > fewer bytes instead: the host counts the bytes it gets back, and it > > resets the device when some are missing. > > > > Signed-off-by: Jamin Lin <[email protected]> > > Reviewed-by: Marc-André Lureau <[email protected]> > > Can you drop my reviewed-by, we are undergoing still significant changes. > Thanks > Thanks for the review and suggestion. Will remove it.
> > Message-ID: <[email protected]> > > > > diff --git a/hw/usb/redirect-server.c b/hw/usb/redirect-server.c index > > 29a9fbc94178..f81dbae2d0b2 100644 > > --- a/hw/usb/redirect-server.c > > +++ b/hw/usb/redirect-server.c > > @@ -82,8 +82,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(USBRedirServer, > > USB_REDIR_SERVER) #define USBREDIR_SERVER_MAX_EP 32 #define > > USBREDIR_SERVER_EP_IN_BASE 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) > > + > > #define USBREDIR_SERVER_CTRL_SETUP 0 > > #define USBREDIR_SERVER_CTRL_STATUS 1 > > +#define USBREDIR_SERVER_BULK 2 > > +#define USBREDIR_SERVER_INTR 3 > > > > /* Which message answers the host when a control transfer ends. */ > > typedef enum { @@ -96,7 +104,9 @@ typedef struct USBRedirServerPkt { > > USBPacket pkt; > > > > /* Saved headers for the usbredir response */ > > + struct usb_redir_interrupt_packet_header intr_hdr; > > struct usb_redir_control_packet_header ctrl_hdr; > > + struct usb_redir_bulk_packet_header bulk_hdr; > > > > /* > > * The IOV points here. IN data lands in it, OUT data is copied in. > > @@ -524,6 +534,52 @@ static void > usbredir_server_ctrl_status_complete(USBRedirServer *s, > > usbredir_server_send_ep_info(s); > > } > > > > +static void usbredir_server_bulk_complete(USBRedirServer *s, > > + USBRedirServerPkt *rp) > { > > + struct usb_redir_bulk_packet_header resp = rp->bulk_hdr; > > + bool is_in = !!(rp->bulk_hdr.endpoint & USB_DIR_IN); > > + int actual = rp->pkt.actual_length; > > + int len; > > + > > + resp.status = usbredir_server_status(rp->pkt.status); > > A bulk IN that the device answers with USB_RET_NAK ends up here, since > submit_to_device() completes everything that is not USB_RET_ASYNC, and > usbredir_server_status() turns it into usb_redir_ioerror. > > usbredirhost, the libusb backend, keeps the transfer pending until data > arrives. > The server should do the same here: on NAK keep the packet on the inflight > list > and submit it again from wakeup_endpoint, like the interrupt stream in patch 7 > does. > I'll keep bulk IN packets pending on NAK and retry them from wakeup_endpoint > > + > > + /* Only an IN transfer carries data back. */ > > + len = is_in ? actual : 0; > > + resp.length = len; > > + resp.length_high = len >> 16; > > + > > + trace_usbredir_server_bulk_complete(rp->redir_id, resp.endpoint, > > + resp.status, actual); > > + > > + usbredirparser_send_bulk_packet(s->parser, rp->redir_id, &resp, > > + len > 0 ? rp->data : NULL, len); > > + usbredirparser_do_write(s->parser); > > +} > > + > > +static void usbredir_server_intr_complete(USBRedirServer *s, > > + USBRedirServerPkt *rp) > { > > + struct usb_redir_interrupt_packet_header resp = rp->intr_hdr; > > + bool is_in = !!(rp->intr_hdr.endpoint & USB_DIR_IN); > > + int actual = rp->pkt.actual_length; > > + int len; > > + > > + resp.status = usbredir_server_status(rp->pkt.status); > > + > > + /* Only an IN transfer carries data back. */ > > + len = is_in ? actual : 0; > > + resp.length = len; > > + > > + trace_usbredir_server_intr_complete(rp->redir_id, resp.endpoint, > > + resp.status, actual); > > + > > + usbredirparser_send_interrupt_packet(s->parser, rp->redir_id, &resp, > > + len > 0 ? rp->data : NULL, > > + len); > > + usbredirparser_do_write(s->parser); > > +} > > + > > /* > > * USB port ops > > */ > > @@ -600,6 +656,12 @@ static void > usbredir_server_packet_complete(USBPort *port, USBPacket *p) > > case USBREDIR_SERVER_CTRL_STATUS: > > usbredir_server_ctrl_status_complete(s, rp); > > break; > > + case USBREDIR_SERVER_BULK: > > + usbredir_server_bulk_complete(s, rp); > > + break; > > + case USBREDIR_SERVER_INTR: > > + usbredir_server_intr_complete(s, rp); > > + break; > > } > > > > usbredir_server_pkt_free(rp); > > @@ -979,6 +1041,110 @@ static void usbredir_server_get_alt_setting(void > *priv, uint64_t id, > > USBREDIR_SERVER_REPLY_ALT); } > > > > +static void usbredir_server_bulk_packet(void *priv, uint64_t id, > > + struct usb_redir_bulk_packet_header *hdr, > > + uint8_t *data, int data_len) > > +{ > > + USBRedirServer *s = priv; > > + USBDevice *device = usbredir_server_device(s); > > + bool is_in = !!(hdr->endpoint & USB_DIR_IN); > > + int pid = is_in ? USB_TOKEN_IN : USB_TOKEN_OUT; > > + struct usb_redir_bulk_packet_header resp; > > + int ep_nr = hdr->endpoint & 0x0f; > > + USBRedirServerPkt *rp; > > + USBEndpoint *ep; > > + uint32_t len; > > + > > + if (!s->host_connected || !device || !device->attached) { > > + usbredirparser_free_packet_data(s->parser, data); > > + return; > > + } > > + > > + /* IN: what the host asked for. OUT: what the host sent. */ > > + len = is_in ? (((uint32_t)hdr->length_high << 16) | hdr->length) > > + : (uint32_t)data_len; > > + > > + /* > > + * Too big. Tell the host the transfer failed. > > + * Do not send back less data instead. The host would see the > > + * missing bytes as an error and reset the device. > > + */ > > + if (len > USBREDIR_SERVER_MAX_BULK) { > > + resp = *hdr; > > + resp.status = usb_redir_inval; > > + resp.length = 0; > > + resp.length_high = 0; > > + > > + trace_usbredir_server_bulk_too_big(id, hdr->endpoint, len); > > + usbredirparser_send_bulk_packet(s->parser, id, &resp, NULL, 0); > > + usbredirparser_do_write(s->parser); > > + usbredirparser_free_packet_data(s->parser, data); > > + return; > > + } > > + > > + ep = usb_ep_get(device, pid, ep_nr); > > Nothing rejects ep_nr == 0. usb_ep_get() then returns the control endpoint and > usb_process_one() runs do_token_in()/do_token_out() on it, mixing a "bulk" > packet into ep0's setup state machine. Same for > usbredir_server_interrupt_packet() below. > > Answer usb_redir_inval for endpoint 0, as start_interrupt_receiving does in > patch 7? > I'll return usb_redir_inval for endpoint 0 and for endpoints with an incorrect type in both bulk and interrupt transfers. Jamin > -- > Marc-André Lureau <[email protected]>
