* [Qemu-devel] [PATCH v2 1/2] xen: when removing a backend don't remove many of them
2016-08-02 12:14 [Qemu-devel] [PATCH v2 0/2] xen: bug fixes in Xen backend handling Juergen Gross
@ 2016-08-02 12:14 ` Juergen Gross
2016-08-02 12:14 ` [Qemu-devel] [PATCH v2 2/2] xen: drain submit queue in xen-usb before removing device Juergen Gross
1 sibling, 0 replies; 4+ messages in thread
From: Juergen Gross @ 2016-08-02 12:14 UTC (permalink / raw)
To: qemu-devel, xen-devel; +Cc: kraxel, sstabellini, anthony.perard, Juergen Gross
When a Xenstore watch fires indicating a backend has to be removed
don't remove all backends for that domain with the specified device
index, but just the one which has the correct type.
The easiest way to achieve this is to use the already determined
xendev as parameter for xen_be_del_xendev() instead of only the domid
and device index.
This at once removes the open coded QTAILQ_FOREACH_SAVE() in
xen_be_del_xendev() as there is no need to search for the correct
xendev any longer.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
hw/xen/xen_backend.c | 58 +++++++++++++++++-----------------------------------
1 file changed, 19 insertions(+), 39 deletions(-)
diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
index bab79b1..3ceb778 100644
--- a/hw/xen/xen_backend.c
+++ b/hw/xen/xen_backend.c
@@ -321,48 +321,28 @@ static struct XenDevice *xen_be_get_xendev(const char *type, int dom, int dev,
/*
* release xen backend device.
*/
-static struct XenDevice *xen_be_del_xendev(int dom, int dev)
+static void xen_be_del_xendev(struct XenDevice *xendev)
{
- struct XenDevice *xendev, *xnext;
-
- /*
- * This is pretty much like QTAILQ_FOREACH(xendev, &xendevs, next) but
- * we save the next pointer in xnext because we might free xendev.
- */
- xnext = xendevs.tqh_first;
- while (xnext) {
- xendev = xnext;
- xnext = xendev->next.tqe_next;
-
- if (xendev->dom != dom) {
- continue;
- }
- if (xendev->dev != dev && dev != -1) {
- continue;
- }
-
- if (xendev->ops->free) {
- xendev->ops->free(xendev);
- }
-
- if (xendev->fe) {
- char token[XEN_BUFSIZE];
- snprintf(token, sizeof(token), "fe:%p", xendev);
- xs_unwatch(xenstore, xendev->fe, token);
- g_free(xendev->fe);
- }
+ if (xendev->ops->free) {
+ xendev->ops->free(xendev);
+ }
- if (xendev->evtchndev != NULL) {
- xenevtchn_close(xendev->evtchndev);
- }
- if (xendev->gnttabdev != NULL) {
- xengnttab_close(xendev->gnttabdev);
- }
+ if (xendev->fe) {
+ char token[XEN_BUFSIZE];
+ snprintf(token, sizeof(token), "fe:%p", xendev);
+ xs_unwatch(xenstore, xendev->fe, token);
+ g_free(xendev->fe);
+ }
- QTAILQ_REMOVE(&xendevs, xendev, next);
- g_free(xendev);
+ if (xendev->evtchndev != NULL) {
+ xenevtchn_close(xendev->evtchndev);
}
- return NULL;
+ if (xendev->gnttabdev != NULL) {
+ xengnttab_close(xendev->gnttabdev);
+ }
+
+ QTAILQ_REMOVE(&xendevs, xendev, next);
+ g_free(xendev);
}
/*
@@ -682,7 +662,7 @@ static void xenstore_update_be(char *watch, char *type, int dom,
if (xendev != NULL) {
bepath = xs_read(xenstore, 0, xendev->be, &len);
if (bepath == NULL) {
- xen_be_del_xendev(dom, dev);
+ xen_be_del_xendev(xendev);
} else {
free(bepath);
xen_be_backend_changed(xendev, path);
--
2.6.6
^ permalink raw reply related [flat|nested] 4+ messages in thread* [Qemu-devel] [PATCH v2 2/2] xen: drain submit queue in xen-usb before removing device
2016-08-02 12:14 [Qemu-devel] [PATCH v2 0/2] xen: bug fixes in Xen backend handling Juergen Gross
2016-08-02 12:14 ` [Qemu-devel] [PATCH v2 1/2] xen: when removing a backend don't remove many of them Juergen Gross
@ 2016-08-02 12:14 ` Juergen Gross
2016-08-02 15:26 ` Anthony PERARD
1 sibling, 1 reply; 4+ messages in thread
From: Juergen Gross @ 2016-08-02 12:14 UTC (permalink / raw)
To: qemu-devel, xen-devel; +Cc: kraxel, sstabellini, anthony.perard, Juergen Gross
When unplugging a device in the Xen pvusb backend drain the submit
queue before deallocation of the control structures. Otherwise there
will be bogus memory accesses when I/O contracts are finished.
Correlated to this issue is the handling of cancel requests: a packet
cancelled will still lead to the call of complete, so add a flag
to the request indicating it should be just dropped on complete.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
hw/usb/xen-usb.c | 94 +++++++++++++++++++++++++++++++++++++-------------------
1 file changed, 62 insertions(+), 32 deletions(-)
diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c
index 7992456..174d715 100644
--- a/hw/usb/xen-usb.c
+++ b/hw/usb/xen-usb.c
@@ -90,6 +90,8 @@ struct usbback_req {
void *buffer;
void *isoc_buffer;
struct libusb_transfer *xfer;
+
+ bool cancelled;
};
struct usbback_hotplug {
@@ -301,20 +303,23 @@ static void usbback_do_response(struct usbback_req *usbback_req, int32_t status,
usbback_req->isoc_buffer = NULL;
}
- res = RING_GET_RESPONSE(&usbif->urb_ring, usbif->urb_ring.rsp_prod_pvt);
- 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 (usbif->urb_sring) {
+ res = RING_GET_RESPONSE(&usbif->urb_ring, usbif->urb_ring.rsp_prod_pvt);
+ 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);
+ if (notify) {
+ xen_be_send_notify(xendev);
+ }
}
- usbback_put_req(usbback_req);
+ if (!usbback_req->cancelled)
+ usbback_put_req(usbback_req);
}
static void usbback_do_response_ret(struct usbback_req *usbback_req,
@@ -366,15 +371,14 @@ static void usbback_set_address(struct usbback_info *usbif,
}
}
-static bool usbback_cancel_req(struct usbback_req *usbback_req)
+static void usbback_cancel_req(struct usbback_req *usbback_req)
{
- bool ret = false;
-
if (usb_packet_is_inflight(&usbback_req->packet)) {
usb_cancel_packet(&usbback_req->packet);
- ret = true;
+ QTAILQ_REMOVE(&usbback_req->stub->submit_q, usbback_req, q);
+ usbback_req->cancelled = true;
+ usbback_do_response_ret(usbback_req, -EPROTO);
}
- return ret;
}
static void usbback_process_unlink_req(struct usbback_req *usbback_req)
@@ -391,7 +395,7 @@ static void usbback_process_unlink_req(struct usbback_req *usbback_req)
devnum = usbif_pipedevice(usbback_req->req.pipe);
if (unlikely(devnum == 0)) {
usbback_req->stub = usbif->ports +
- usbif_pipeportnum(usbback_req->req.pipe);
+ usbif_pipeportnum(usbback_req->req.pipe) - 1;
if (unlikely(!usbback_req->stub)) {
ret = -ENODEV;
goto fail_response;
@@ -406,9 +410,7 @@ static void usbback_process_unlink_req(struct usbback_req *usbback_req)
QTAILQ_FOREACH(unlink_req, &usbback_req->stub->submit_q, q) {
if (unlink_req->req.id == id) {
- if (usbback_cancel_req(unlink_req)) {
- usbback_do_response_ret(unlink_req, -EPROTO);
- }
+ usbback_cancel_req(unlink_req);
break;
}
}
@@ -681,6 +683,33 @@ static void usbback_hotplug_enq(struct usbback_info *usbif, unsigned port)
usbback_hotplug_notify(usbif);
}
+static void usbback_portid_drain(struct usbback_info *usbif, unsigned port)
+{
+ struct usbback_req *req, *tmp;
+ bool sched = false;
+
+ QTAILQ_FOREACH_SAFE(req, &usbif->ports[port - 1].submit_q, q, tmp) {
+ usbback_cancel_req(req);
+ sched = true;
+ }
+
+ if (sched) {
+ qemu_bh_schedule(usbif->bh);
+ }
+}
+
+static void usbback_portid_detach(struct usbback_info *usbif, unsigned port)
+{
+ if (!usbif->ports[port - 1].attached) {
+ return;
+ }
+
+ usbif->ports[port - 1].speed = USBIF_SPEED_NONE;
+ usbif->ports[port - 1].attached = false;
+ usbback_portid_drain(usbif, port);
+ usbback_hotplug_enq(usbif, port);
+}
+
static void usbback_portid_remove(struct usbback_info *usbif, unsigned port)
{
USBPort *p;
@@ -694,9 +723,7 @@ static void usbback_portid_remove(struct usbback_info *usbif, unsigned port)
object_unparent(OBJECT(usbif->ports[port - 1].dev));
usbif->ports[port - 1].dev = NULL;
- usbif->ports[port - 1].speed = USBIF_SPEED_NONE;
- usbif->ports[port - 1].attached = false;
- usbback_hotplug_enq(usbif, port);
+ usbback_portid_detach(usbif, port);
TR_BUS(&usbif->xendev, "port %d removed\n", port);
}
@@ -801,7 +828,6 @@ static void usbback_process_port(struct usbback_info *usbif, unsigned port)
static void usbback_disconnect(struct XenDevice *xendev)
{
struct usbback_info *usbif;
- struct usbback_req *req, *tmp;
unsigned int i;
TR_BUS(xendev, "start\n");
@@ -820,11 +846,8 @@ static void usbback_disconnect(struct XenDevice *xendev)
}
for (i = 0; i < usbif->num_ports; i++) {
- if (!usbif->ports[i].dev) {
- continue;
- }
- QTAILQ_FOREACH_SAFE(req, &usbif->ports[i].submit_q, q, tmp) {
- usbback_cancel_req(req);
+ if (usbif->ports[i].dev) {
+ usbback_portid_drain(usbif, i + 1);
}
}
@@ -944,8 +967,7 @@ static void xen_bus_detach(USBPort *port)
usbif = port->opaque;
TR_BUS(&usbif->xendev, "\n");
- usbif->ports[port->index].attached = false;
- usbback_hotplug_enq(usbif, port->index + 1);
+ usbback_portid_detach(usbif, port->index + 1);
}
static void xen_bus_child_detach(USBPort *port, USBDevice *child)
@@ -958,9 +980,16 @@ static void xen_bus_child_detach(USBPort *port, USBDevice *child)
static void xen_bus_complete(USBPort *port, USBPacket *packet)
{
+ struct usbback_req *usbback_req;
struct usbback_info *usbif;
- usbif = port->opaque;
+ usbback_req = container_of(packet, struct usbback_req, packet);
+ if (usbback_req->cancelled) {
+ g_free(usbback_req);
+ return;
+ }
+
+ usbif = usbback_req->usbif;
TR_REQ(&usbif->xendev, "\n");
usbback_packet_complete(packet);
}
@@ -1037,6 +1066,7 @@ static int usbback_free(struct XenDevice *xendev)
}
usb_bus_release(&usbif->bus);
+ object_unparent(OBJECT(&usbif->bus));
TR_BUS(xendev, "finished\n");
--
2.6.6
^ permalink raw reply related [flat|nested] 4+ messages in thread