Re: [PATCH v2 2/2] xen: add pvUSB backend

[Juergen Gross <jgross@suse.com>]

On 03/05/16 17:06, Anthony PERARD wrote:
> On Thu, Mar 10, 2016 at 04:19:30PM +0100, Juergen Gross wrote:
>> Add a backend for para-virtualized USB devices for xen domains.
>>
>> The backend is using host-libusb to forward USB requests from a
>> domain via libusb to the real device(s) passed through.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
> 
> [...]
> 
>> diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c
>> new file mode 100644
>> index 0000000..b12077f
>> --- /dev/null
>> +++ b/hw/usb/xen-usb.c
>> +static struct usbback_req *usbback_get_req(struct usbback_info *usbif)
>> +{
>> +    struct usbback_req *usbback_req;
>> +
>> +    if (QTAILQ_EMPTY(&usbif->req_free_q)) {
>> +        usbback_req = g_malloc0(sizeof(*usbback_req));
> 
> You could use g_new0(struct usbback_req, 1) here.

Okay.

>> +    } else {
>> +        usbback_req = QTAILQ_FIRST(&usbif->req_free_q);
>> +        QTAILQ_REMOVE(&usbif->req_free_q, usbback_req, q);
>> +    }
>> +    return usbback_req;
>> +}
>> +static int usbback_gnttab_map(struct usbback_info *usbif,
>> +                              struct usbback_req *usbback_req)
> 
> Looks like this function is the only one to have both usbif and usbback_req
> as parameters.

Will change.

>> +{
>> +    unsigned int nr_segs, i, prot;
>> +    uint32_t ref[USBIF_MAX_SEGMENTS_PER_REQUEST];
>> +    struct XenDevice *xendev = &usbif->xendev;
>> +    struct usbif_request_segment *seg;
>> +    void *addr;
>> +
>> +    nr_segs = usbback_req->nr_buffer_segs + usbback_req->nr_extra_segs;
>> +    if (!nr_segs) {
>> +        return 0;
>> +    }
>> +
>> +    if (nr_segs > USBIF_MAX_SEGMENTS_PER_REQUEST) {
>> +        xen_be_printf(xendev, 0, "bad number of segments in request (%d)\n",
>> +                      nr_segs);
>> +        return -EINVAL;
>> +    }
>> +
>> +    for (i = 0; i < nr_segs; i++) {
>> +        if ((unsigned)usbback_req->req.seg[i].offset +
>> +            (unsigned)usbback_req->req.seg[i].length > PAGE_SIZE) {
>> +            xen_be_printf(xendev, 0, "segment crosses page boundary\n");
>> +            return -EINVAL;
>> +        }
>> +    }
>> +
>> +    if (usbback_req->nr_buffer_segs) {
>> +        prot = PROT_READ;
>> +        if (usbif_pipein(usbback_req->req.pipe)) {
>> +                prot |= PROT_WRITE;
>> +        }
>> +        for (i = 0; i < usbback_req->nr_buffer_segs; i++) {
>> +            ref[i] = usbback_req->req.seg[i].gref;
>> +        }
>> +        usbback_req->buffer = xengnttab_map_domain_grant_refs(xendev->gnttabdev,
>> +            usbback_req->nr_buffer_segs, xendev->dom, ref, prot);
>> +
>> +        if (!usbback_req->buffer) {
>> +            return -ENOMEM;
>> +        }
>> +
>> +        for (i = 0; i < usbback_req->nr_buffer_segs; i++) {
>> +            seg = usbback_req->req.seg + i;
>> +            addr = usbback_req->buffer + i * PAGE_SIZE + seg->offset;
>> +            qemu_iovec_add(&usbback_req->packet.iov, addr, seg->length);
>> +        }
>> +    }
>> +
>> +    if (!usbif_pipeisoc(usbback_req->req.pipe)) {
>> +        return 0;
>> +    }
> 
> The code below would never be reached since isoc request are discared in
> usbback_init_packet. Is this some left over from previous version?

Yes, but on purpose. This is infrastructure stuff which will be needed
regardless how isoc support will be added later. I'll add a comment.

> 
>> +    if (!usbback_req->nr_extra_segs) {
>> +        xen_be_printf(xendev, 0, "iso request without descriptor segments\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    prot = PROT_READ | PROT_WRITE;
>> +    for (i = 0; i < usbback_req->nr_extra_segs; i++) {
>> +        ref[i] = usbback_req->req.seg[i + usbback_req->req.nr_buffer_segs].gref;
>> +    }
>> +    usbback_req->isoc_buffer = xengnttab_map_domain_grant_refs(
>> +         xendev->gnttabdev, usbback_req->nr_extra_segs, xendev->dom, ref, prot);
>> +
>> +    if (!usbback_req->isoc_buffer) {
>> +        return -ENOMEM;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int usbback_init_packet(struct usbback_req *usbback_req)
>> +{
>> +    struct XenDevice *xendev = &usbback_req->usbif->xendev;
>> +    USBPacket *packet = &usbback_req->packet;
>> +    USBDevice *dev = usbback_req->stub->dev;
>> +    USBEndpoint *ep;
>> +    unsigned int pid, ep_nr;
>> +    bool sok;
>> +    int ret = 0;
>> +
>> +    qemu_iovec_init(&packet->iov, USBIF_MAX_SEGMENTS_PER_REQUEST);
>> +    pid = usbif_pipein(usbback_req->req.pipe) ? USB_TOKEN_IN : USB_TOKEN_OUT;
>> +    ep_nr = usbif_pipeendpoint(usbback_req->req.pipe);
>> +    sok = !!(usbback_req->req.transfer_flags & 1);
> 
> Why 1, where this came from?

Well spotted!

I'll have to define this in the pvusb interface header. This is the
URB_SHORT_NOT_OK flag from the frontend.

> 
>> +    if (usbif_pipetype(usbback_req->req.pipe) == USBIF_PIPE_TYPE_CTRL) {
> 
> usbif_pipectrl()?

Yepp.

> 
>> +        ep_nr = 0;
>> +        sok = false;
>> +    }
>> +    ep = usb_ep_get(dev, pid, ep_nr);
>> +    usb_packet_setup(packet, pid, ep, 0, 1, sok, true);
>> +
>> +    switch (usbif_pipetype(usbback_req->req.pipe)) {
>> +    case USBIF_PIPE_TYPE_ISOC:
>> +        TR_REQ(xendev, "iso transfer %s: buflen: %x, %d frames\n",
>> +               (pid == USB_TOKEN_IN) ? "in" : "out",
>> +               usbback_req->req.buffer_length,
>> +               usbback_req->req.u.isoc.nr_frame_desc_segs);
>> +        ret = -EINVAL;  /* isoc not implemented yet */
>> +        break;
>> +
>> +    case USBIF_PIPE_TYPE_INT:
>> +        TR_REQ(xendev, "int transfer %s: buflen: %x\n",
>> +               (pid == USB_TOKEN_IN) ? "in" : "out",
>> +               usbback_req->req.buffer_length);
>> +        break;
>> +
>> +    case USBIF_PIPE_TYPE_CTRL:
>> +        packet->parameter = *(uint64_t *)usbback_req->req.u.ctrl;
>> +        TR_REQ(xendev, "ctrl parameter: %lx, buflen: %x\n", packet->parameter,
>> +               usbback_req->req.buffer_length);
>> +        break;
>> +
>> +    case USBIF_PIPE_TYPE_BULK:
>> +        TR_REQ(xendev, "bulk transfer %s: buflen: %x\n",
>> +               (pid == USB_TOKEN_IN) ? "in" : "out",
>> +               usbback_req->req.buffer_length);
>> +        break;
>> +    default:
>> +        ret = -EINVAL;
>> +        break;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static void usbback_do_response(struct usbback_req *usbback_req, int32_t status,
>> +                                int32_t actual_length, int32_t error_count)
>> +{
>> +    struct usbback_info *usbif;
>> +    struct usbif_urb_response *res;
>> +    struct XenDevice *xendev;
>> +    unsigned int notify;
>> +
>> +    usbif = usbback_req->usbif;
>> +    xendev = &usbif->xendev;
>> +
>> +    TR_REQ(xendev, "id %d, status %d, length %d, errcnt %d\n",
>> +           usbback_req->req.id, status, actual_length, error_count);
>> +
>> +    if (usbback_req->packet.iov.iov) {
>> +        qemu_iovec_destroy(&usbback_req->packet.iov);
>> +    }
>> +
>> +    if (usbback_req->buffer) {
>> +        xengnttab_unmap(xendev->gnttabdev, usbback_req->buffer,
>> +                        usbback_req->nr_buffer_segs);
>> +        usbback_req->buffer = NULL;
>> +    }
>> +
>> +    if (usbback_req->isoc_buffer) {
>> +        xengnttab_unmap(xendev->gnttabdev, usbback_req->isoc_buffer,
>> +                        usbback_req->nr_extra_segs);
>> +        usbback_req->isoc_buffer = NULL;
>> +    }
>> +
>> +    res = RING_GET_RESPONSE(&usbif->urb_ring, usbif->urb_ring.rsp_prod_pvt);
> 
> What happen if the ring is full?

Can't happen if frontend behaves correctly (and won't hurt backend
if frontend misbehaves): RING_FULL() is required to be used on
frontend side. This will ensure there are never more than RING_SIZE()
active requests, so there will always be a response slot available.

>> +    res->id = usbback_req->req.id;
>> +    res->status = status;
>> +    res->actual_length = actual_length;
>> +    res->error_count = error_count;
>> +    res->start_frame = 0;
>> +    usbif->urb_ring.rsp_prod_pvt++;
>> +    RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&usbif->urb_ring, notify);
>> +
>> +    if (notify) {
>> +        xen_be_send_notify(xendev);
>> +    }
>> +
>> +    usbback_put_req(usbback_req);
>> +}
>> +
>> +static void usbback_do_response_ret(struct usbback_req *usbback_req,
>> +                                    int32_t status)
>> +{
>> +    usbback_do_response(usbback_req, status, 0, 0);
>> +}
>> +
>> +static int32_t usbback_xlat_status(int32_t status)
> 
> USBPacket->status is int, so I think the parameter status should be int as
> well.

Okay.

>> +{
>> +    int32_t ret = -ESHUTDOWN;
>> +
>> +    switch (status) {
>> +    case USB_RET_SUCCESS:
>> +        ret = 0;
>> +        break;
>> +    case USB_RET_NODEV:
>> +        ret = -ENODEV;
>> +        break;
>> +    case USB_RET_STALL:
>> +        ret = -EPIPE;
>> +        break;
>> +    case USB_RET_BABBLE:
>> +        ret = -EOVERFLOW;
>> +        break;
>> +    case USB_RET_IOERROR:
>> +        ret = -EPROTO;
>> +        break;
>> +    }
> 
> You could return from the switch instead of using a variable.

Okay.

>> +
>> +    return ret;
>> +}
>> +
>> +static void usbback_packet_complete(USBPacket *packet)
>> +{
>> +    struct usbback_req *usbback_req;
>> +    int32_t status;
>> +
>> +    usbback_req = container_of(packet, struct usbback_req, packet);
>> +
>> +    QTAILQ_REMOVE(&usbback_req->stub->submit_q, usbback_req, q);
>> +
>> +    status = usbback_xlat_status(packet->status);
>> +    usbback_do_response(usbback_req, status, packet->actual_length, 0);
>> +}
>> +
>> +static void usbback_set_address(struct usbback_info *usbif,
>> +                                struct usbback_stub *stub, int cur_addr,
>> +                                int new_addr)
> 
> unsigned int for both cur_addr and new_addr?

Yes.

>> +{
>> +    if (cur_addr) {
>> +        usbif->addr_table[cur_addr] = NULL;
>> +    }
>> +    if (new_addr) {
>> +        usbif->addr_table[new_addr] = stub;
>> +    }
>> +}
>> +/*
>> + * Checks whether a request can be handled at once or should be forwarded
>> + * to the usb framework.
>> + * Return value is:
>> + * 0 in case of usb framework is needed
>> + * >0 in case of local handling (no error)
>> + * <0 parameter error
> 
> The function only return 1 or 0. The comment those not match the function?

Uuh, yes. This is a leftover from a previous version.

>> + * The request response has been queued already if return value not 0.
>> + */
>> +static int usbback_check_and_submit(struct usbback_req *usbback_req)
>> +{
>> +    struct usbback_info *usbif;
>> +    unsigned int devnum;
>> +    struct usbback_stub *stub;
>> +    struct usbif_ctrlrequest *ctrl;
>> +    int ret;
>> +    uint16_t wValue;
>> +
>> +    usbif = usbback_req->usbif;
>> +    stub = NULL;
>> +    devnum = usbif_pipedevice(usbback_req->req.pipe);
>> +    ctrl = (struct usbif_ctrlrequest *)usbback_req->req.u.ctrl;
>> +    wValue = le16_to_cpu(ctrl->wValue);
>> +
>> +    /*
>> +     * When the device is first connected or resetted, USB device has no
>> +     * address. In this initial state, following requests are sent to device
>> +     * address (#0),
>> +     *
>> +     *  1. GET_DESCRIPTOR (with Descriptor Type is "DEVICE") is sent,
>> +     *     and OS knows what device is connected to.
>> +     *
>> +     *  2. SET_ADDRESS is sent, and then device has its address.
>> +     *
>> +     * In the next step, SET_CONFIGURATION is sent to addressed device, and
>> +     * then the device is finally ready to use.
>> +     */
>> +    if (unlikely(devnum == 0)) {
>> +        stub = usbif->ports + usbif_pipeportnum(usbback_req->req.pipe) - 1;
>> +        if (!stub->dev || !stub->attached) {
>> +            ret = -ENODEV;
>> +            goto do_response;
>> +        }
>> +
>> +        switch (ctrl->bRequest) {
>> +        case USB_REQ_GET_DESCRIPTOR:
>> +            /*
>> +             * GET_DESCRIPTOR request to device #0.
>> +             * through normal transfer.
>> +             */
>> +            TR_REQ(&usbif->xendev, "devnum 0 GET_DESCRIPTOR\n");
>> +            usbback_req->stub = stub;
>> +            return 0;
>> +        case USB_REQ_SET_ADDRESS:
>> +            /*
>> +             * SET_ADDRESS request to device #0.
>> +             * add attached device to addr_table.
>> +             */
>> +            TR_REQ(&usbif->xendev, "devnum 0 SET_ADDRESS\n");
>> +            usbback_set_address(usbif, stub, 0, wValue);
>> +            ret = 0;
>> +            break;
>> +        default:
>> +            ret = -EINVAL;
>> +            break;
>> +        }
>> +        goto do_response;
>> +    }
>> +
>> +    if (unlikely(!usbif->addr_table[devnum])) {
>> +            ret = -ENODEV;
>> +            goto do_response;
>> +    }
>> +    usbback_req->stub = usbif->addr_table[devnum];
>> +
>> +    /*
>> +     * Check special request
>> +     */
>> +    if (ctrl->bRequest != USB_REQ_SET_ADDRESS) {
>> +        return 0;
>> +    }
>> +
>> +    /*
>> +     * SET_ADDRESS request to addressed device.
>> +     * change addr or remove from addr_table.
>> +     */
>> +    usbback_set_address(usbif, usbback_req->stub, devnum, wValue);
>> +    ret = 0;
>> +
>> +do_response:
>> +    usbback_do_response_ret(usbback_req, ret);
>> +    return 1;
>> +}
>> +
>> +static void usbback_bh(void *opaque)
>> +{
>> +    struct usbback_info *usbif;
>> +    struct usbif_urb_back_ring *urb_ring;
>> +    struct usbback_req *usbback_req;
>> +    RING_IDX rc, rp;
>> +    unsigned int more_to_do;
>> +
>> +    usbif = opaque;
>> +    if (usbif->ring_error) {
>> +        return;
>> +    }
>> +
>> +    urb_ring = &usbif->urb_ring;
>> +    rc = urb_ring->req_cons;
>> +    rp = urb_ring->sring->req_prod;
> 
> Maybe use atomic_read() here to avoid req_prod been read more than once.

Hmm. This isn't done in the other backends.

TBH: what would happen if req_prod would be read multiple times? In the
worst case we would see a new request from the guest which we would have
missed without the atomic_read().

>> +    xen_rmb(); /* Ensure we see queued requests up to 'rp'. */
>> +
>> +    if (RING_REQUEST_PROD_OVERFLOW(urb_ring, rp)) {
>> +        rc = urb_ring->rsp_prod_pvt;
>> +        xen_be_printf(&usbif->xendev, 0, "domU provided bogus ring requests "
>> +                      "(%#x - %#x = %u). Halting ring processing.\n",
>> +                      rp, rc, rp - rc);
>> +        usbif->ring_error = true;
>> +        return;
>> +    }
>> +
>> +    while (rc != rp) {
>> +        if (RING_REQUEST_CONS_OVERFLOW(urb_ring, rc)) {
>> +            break;
>> +        }
>> +        usbback_req = usbback_get_req(usbif);
>> +
>> +        usbback_req->req = *RING_GET_REQUEST(urb_ring, rc);
>> +        usbback_req->usbif = usbif;
>> +
>> +        usbback_dispatch(usbback_req);
>> +
>> +        urb_ring->req_cons = ++rc;
>> +    }
>> +
>> +    RING_FINAL_CHECK_FOR_REQUESTS(urb_ring, more_to_do);
>> +    if (more_to_do) {
>> +        qemu_bh_schedule(usbif->bh);
>> +    }
>> +}
>> +
>> +static void usbback_hotplug_notify(struct usbback_info *usbif, unsigned port)
>> +{
>> +    struct usbif_conn_back_ring *ring = &usbif->conn_ring;
>> +    struct usbif_conn_request *req;
>> +    struct usbif_conn_response *res;
>> +    uint16_t id;
>> +    unsigned int notify;
>> +
>> +    if (!usbif->conn_sring) {
>> +        return;
>> +    }
>> +
>> +    req = RING_GET_REQUEST(ring, ring->req_cons);
> 
> Should not we check if there is something to consume here?

No. We just have to dequeue a request in order to stay in sync with the
number of enqueued responses (which are events).

What we should do, however, is to check for the ring full condition here
already, as we shouldn't start with the dequeueing in this situation.
This requires introducing a hotplug notify queue which is drained in
usbback_bh().

>> +    id = req->id;
>> +    ring->req_cons++;
>> +    ring->sring->req_event = ring->req_cons + 1;
>> +
>> +    res = RING_GET_RESPONSE(ring, ring->rsp_prod_pvt);
> 
> What if the ring is already full?

See above.

>> +    res->id = id;
>> +    res->portnum = port;
>> +    res->speed = usbif->ports[port - 1].speed;
>> +    ring->rsp_prod_pvt++;
>> +    RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(ring, notify);
>> +
>> +    if (notify) {
>> +        xen_be_send_notify(&usbif->xendev);
>> +    }
>> +
>> +    TR_BUS(&usbif->xendev, "hotplug port %d speed %d\n", port, res->speed);
>> +}
>> +

Thanks for the review,


Juergen