Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OHCI: Fixed a bug in the OHCI implementation from QEMU #1531

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
+168 −62
Open

OHCI: Fixed a bug in the OHCI implementation from QEMU #1531

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
ff79cd5
Fixed a bug in OHCI
faha223 Sep 23, 2023
a3d61ca
Added a missing check for usb_packet_is_inflight
faha223 Oct 7, 2023
b5de99f
Fixed some nits and used QTAILQ_INIT to initialize active_packets
faha223 Oct 22, 2023
28119cf
Merge branch 'master' into ohci_fix
faha223 Nov 23, 2023
7e0734c
Added back usb_buf, async_td, and async_complete
faha223 Nov 27, 2023
d3e4b0e
Merge pull request #23 from faha223/master
faha223 Dec 18, 2023
114d994
Merge branch 'ohci_fix' of https://github.com/faha223/xemu into ohci_fix
faha223 Dec 18, 2023
fe10357
Added logic to clear out the packets in usb_ohci_exit
faha223 Feb 26, 2024
51a0a4e
Removed the unnecessary foreach loop. No need to clear out the packet…
faha223 Feb 26, 2024
d265219
Replicated the change from the FixMacOsBuild branch so that the macos…
faha223 Feb 26, 2024
adb0eb3
ran clang-format on the branch
faha223 Feb 28, 2024
98aa18b
Merge pull request #24 from xemu-project/master
faha223 Mar 10, 2024
950519a
Merge branch 'master' into ohci_fix
faha223 Mar 10, 2024
3b15f6a
Merge branch 'xemu-project:master' into ohci_fix
faha223 Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions hw/usb/hcd-ohci-pci.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,12 @@ static void usb_ohci_exit(PCIDevice *dev)
trace_usb_ohci_exit(s->name);
ohci_bus_stop(s);

if (s->async_td) {
usb_cancel_packet(&s->usb_packet);
s->async_td = 0;
USBActivePacket *iter;
QTAILQ_FOREACH(iter, &s->active_packets, next) {
if (iter->async_td) {
usb_cancel_packet(&iter->usb_packet);
iter->async_td = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you clean up packets

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No.

I removed this foreach loop because the call right after this (ohci_stop_endpoints) actually calls (ohci_clear_active_packets) and does the work of this foreach loop already.

}
}
ohci_stop_endpoints(s);

Expand Down
214 changes: 154 additions & 60 deletions hw/usb/hcd-ohci.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,10 +281,7 @@ void ohci_stop_endpoints(OHCIState *ohci)
USBDevice *dev;
int i, j;

if (ohci->async_td) {
usb_cancel_packet(&ohci->usb_packet);
ohci->async_td = 0;
}
ohci_clear_active_packets(ohci);
for (i = 0; i < ohci->num_ports; i++) {
dev = ohci->rhport[i].port.dev;
if (dev && dev->attached) {
Expand Down Expand Up @@ -869,18 +866,18 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
return 1;
}

/* See if this TD has already been submitted to the device. */
completion = (addr == ohci->async_td);
if (completion && !ohci->async_complete) {
trace_usb_ohci_td_skip_async();
return 1;
}
if (ohci_read_td(ohci, addr, &td)) {
trace_usb_ohci_td_read_error(addr);
ohci_die(ohci);
return 1;
}

dev = ohci_find_device(ohci, OHCI_BM(ed->flags, ED_FA));
if (dev == NULL) {
trace_usb_ohci_td_dev_error();
return 1;
}

dir = OHCI_BM(ed->flags, ED_D);
switch (dir) {
case OHCI_TD_DIR_OUT:
Expand Down Expand Up @@ -909,6 +906,35 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
trace_usb_ohci_td_bad_direction(dir);
return 1;
}

ep = usb_ep_get(dev, pid, OHCI_BM(ed->flags, ED_EN));
USBActivePacket *packet = NULL;
USBActivePacket *iter;
QTAILQ_FOREACH(iter, &ohci->active_packets, next) {
if(iter->ep == ep) {
packet = iter;
break;
}
}

// A packet for this endpoint doesn't exist yet. Make one
faha223 marked this conversation as resolved.
Show resolved Hide resolved
if(packet == NULL) {
packet = g_malloc(sizeof(USBActivePacket));
usb_packet_init(&packet->usb_packet);
packet->ep = ep;
packet->async_complete = false;
packet->async_td = 0;
packet->ohci = ohci;
QTAILQ_INSERT_TAIL(&ohci->active_packets, packet, next);
}

/* See if this TD has already been submitted to the device. */
completion = (addr == packet->async_td);
if (completion && !packet->async_complete) {
trace_usb_ohci_td_skip_async();
return 1;
}

if (td.cbp && td.be) {
if ((td.cbp & 0xfffff000) != (td.be & 0xfffff000)) {
len = (td.be & 0xfff) + 0x1001 - (td.cbp & 0xfff);
Expand All @@ -920,8 +946,8 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
}
len = (td.be - td.cbp) + 1;
}
if (len > sizeof(ohci->usb_buf)) {
len = sizeof(ohci->usb_buf);
if (len > sizeof(packet->usb_buf)) {
len = sizeof(packet->usb_buf);
}

pktlen = len;
Expand All @@ -932,7 +958,7 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
pktlen = len;
}
if (!completion) {
if (ohci_copy_td(ohci, &td, ohci->usb_buf, pktlen,
if (ohci_copy_td(ohci, &td, packet->usb_buf, pktlen,
DMA_DIRECTION_TO_DEVICE)) {
ohci_die(ohci);
}
Expand All @@ -943,52 +969,42 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
flag_r = (td.flags & OHCI_TD_R) != 0;
trace_usb_ohci_td_pkt_hdr(addr, (int64_t)pktlen, (int64_t)len, str,
flag_r, td.cbp, td.be);
ohci_td_pkt("OUT", ohci->usb_buf, pktlen);
ohci_td_pkt("OUT", packet->usb_buf, pktlen);

if (completion) {
ohci->async_td = 0;
ohci->async_complete = false;
packet->async_td = 0;
packet->async_complete = false;
} else {
dev = ohci_find_device(ohci, OHCI_BM(ed->flags, ED_FA));
if (dev == NULL) {
trace_usb_ohci_td_dev_error();
return 1;
}
ep = usb_ep_get(dev, pid, OHCI_BM(ed->flags, ED_EN));
if (ohci->async_td) {
/* ??? The hardware should allow one active packet per
endpoint. We only allow one active packet per controller.
This should be sufficient as long as devices respond in a
timely manner.
*/
if (packet->async_td) {
// Only allow one active packet per endpoint.
trace_usb_ohci_td_too_many_pending(ep->nr);
return 1;
}
usb_packet_setup(&ohci->usb_packet, pid, ep, 0, addr, !flag_r,
usb_packet_setup(&packet->usb_packet, pid, ep, 0, addr, !flag_r,
OHCI_BM(td.flags, TD_DI) == 0);
usb_packet_addbuf(&ohci->usb_packet, ohci->usb_buf, pktlen);
usb_handle_packet(dev, &ohci->usb_packet);
trace_usb_ohci_td_packet_status(ohci->usb_packet.status);
usb_packet_addbuf(&packet->usb_packet, packet->usb_buf, pktlen);
usb_handle_packet(dev, &packet->usb_packet);
trace_usb_ohci_td_packet_status(packet->usb_packet.status);

if (ohci->usb_packet.status == USB_RET_ASYNC) {
if (packet->usb_packet.status == USB_RET_ASYNC) {
usb_device_flush_ep_queue(dev, ep);
ohci->async_td = addr;
packet->async_td = addr;
return 1;
}
}
if (ohci->usb_packet.status == USB_RET_SUCCESS) {
ret = ohci->usb_packet.actual_length;
if (packet->usb_packet.status == USB_RET_SUCCESS) {
ret = packet->usb_packet.actual_length;
} else {
ret = ohci->usb_packet.status;
ret = packet->usb_packet.status;
}

if (ret >= 0) {
if (dir == OHCI_TD_DIR_IN) {
if (ohci_copy_td(ohci, &td, ohci->usb_buf, ret,
if (ohci_copy_td(ohci, &td, packet->usb_buf, ret,
DMA_DIRECTION_FROM_DEVICE)) {
ohci_die(ohci);
}
ohci_td_pkt("IN", ohci->usb_buf, pktlen);
ohci_td_pkt("IN", packet->usb_buf, pktlen);
} else {
ret = pktlen;
}
Expand Down Expand Up @@ -1078,15 +1094,20 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
/* Service an endpoint list. Returns nonzero if active TD were found. */
static int ohci_service_ed_list(OHCIState *ohci, uint32_t head)
{
struct ohci_td td;
struct ohci_ed ed;
uint32_t next_ed;
uint32_t cur;
int active;
int dir, pid;
uint32_t link_cnt = 0;
active = 0;
USBDevice *dev;
USBEndpoint *ep;

if (head == 0)
if (head == 0) {
return 0;
}

for (cur = head; cur && link_cnt++ < ED_LINK_LIMIT; cur = next_ed) {
if (ohci_read_ed(ohci, cur, &ed)) {
Expand All @@ -1101,11 +1122,62 @@ static int ohci_service_ed_list(OHCIState *ohci, uint32_t head)
uint32_t addr;
/* Cancel pending packets for ED that have been paused. */
addr = ed.head & OHCI_DPTR_MASK;
if (ohci->async_td && addr == ohci->async_td) {
usb_cancel_packet(&ohci->usb_packet);
ohci->async_td = 0;
usb_device_ep_stopped(ohci->usb_packet.ep->dev,
ohci->usb_packet.ep);

if (ohci_read_td(ohci, addr, &td)) {
trace_usb_ohci_td_read_error(addr);
ohci_die(ohci);
return 1;
}

dir = OHCI_BM(ed.flags, ED_D);
switch (dir) {
case OHCI_TD_DIR_OUT:
case OHCI_TD_DIR_IN:
/* Same value. */
break;
default:
dir = OHCI_BM(td.flags, TD_DP);
break;
}

switch (dir) {
case OHCI_TD_DIR_IN:
pid = USB_TOKEN_IN;
break;
case OHCI_TD_DIR_OUT:
pid = USB_TOKEN_OUT;
break;
case OHCI_TD_DIR_SETUP:
pid = USB_TOKEN_SETUP;
break;
default:
continue;
}

dev = ohci_find_device(ohci, OHCI_BM(ed.flags, ED_FA));
if (dev != NULL) {
ep = usb_ep_get(dev, pid, OHCI_BM(ed.flags, ED_EN));

if(ep != NULL) {
USBActivePacket *iter;
QTAILQ_FOREACH(iter, &ohci->active_packets, next) {
if(iter->ep == ep) {
if (iter->async_td && addr == iter->async_td) {
if(usb_packet_is_inflight(&iter->usb_packet))
usb_cancel_packet(&iter->usb_packet);
iter->async_td = 0;
usb_device_ep_stopped(iter->usb_packet.ep->dev,
iter->usb_packet.ep);
}
break;
}
}
}
} else {
if (ohci_put_ed(ohci, cur, &ed)) {
ohci_die(ohci);
return 0;
}
}
continue;
}
Expand Down Expand Up @@ -1751,11 +1823,19 @@ static void ohci_child_detach(USBPort *port1, USBDevice *dev)
{
OHCIState *ohci = port1->opaque;

if (ohci->async_td &&
usb_packet_is_inflight(&ohci->usb_packet) &&
ohci->usb_packet.ep->dev == dev) {
usb_cancel_packet(&ohci->usb_packet);
ohci->async_td = 0;
USBActivePacket *iter, *iter2;
QTAILQ_FOREACH_SAFE(iter, &ohci->active_packets, next, iter2) {
if(iter->usb_packet.ep->dev == dev) {
faha223 marked this conversation as resolved.
Show resolved Hide resolved
if (iter->async_td &&
usb_packet_is_inflight(&iter->usb_packet) &&
iter->usb_packet.ep->dev == dev) {
usb_cancel_packet(&iter->usb_packet);
iter->async_td = 0;
faha223 marked this conversation as resolved.
Show resolved Hide resolved
}

QTAILQ_REMOVE(&ohci->active_packets, iter, next);
g_free(iter);
}
}
}

Expand Down Expand Up @@ -1812,11 +1892,11 @@ static void ohci_wakeup(USBPort *port1)

static void ohci_async_complete_packet(USBPort *port, USBPacket *packet)
{
OHCIState *ohci = container_of(packet, OHCIState, usb_packet);

USBActivePacket *active_packet = container_of(packet, USBActivePacket, usb_packet);
trace_usb_ohci_async_complete();
ohci->async_complete = true;
ohci_process_lists(ohci);
active_packet->async_complete = true;
ohci_process_lists(active_packet->ohci);
}

static USBPortOps ohci_port_ops = {
Expand Down Expand Up @@ -1890,9 +1970,10 @@ void usb_ohci_init(OHCIState *ohci, DeviceState *dev, uint32_t num_ports,
ohci->localmem_base = localmem_base;

ohci->name = object_get_typename(OBJECT(dev));
usb_packet_init(&ohci->usb_packet);

ohci->async_td = 0;

// Inialize the QTAILQ_HEAD. QTAILQ_HEAD_INITIALIZER doesn't work here
faha223 marked this conversation as resolved.
Show resolved Hide resolved
ohci->active_packets.tqh_circ.tql_next = NULL;
ohci->active_packets.tqh_circ.tql_prev = &(ohci->active_packets).tqh_circ;

ohci->eof_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
ohci_frame_boundary, ohci);
Expand All @@ -1906,10 +1987,26 @@ void ohci_sysbus_die(struct OHCIState *ohci)
{
trace_usb_ohci_die();

ohci_clear_active_packets(ohci);
ohci_set_interrupt(ohci, OHCI_INTR_UE);
ohci_bus_stop(ohci);
}

void ohci_clear_active_packets(struct OHCIState *ohci)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why clear and not cancel?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clear because this function makes the list empty. I am not opposed to changing it to cancel

{
while(!QTAILQ_EMPTY(&ohci->active_packets)) {
USBActivePacket *packet = QTAILQ_FIRST(&ohci->active_packets);
if(packet->async_td) {
usb_cancel_packet(&packet->usb_packet);
packet->async_td = 0;
usb_device_ep_stopped(packet->usb_packet.ep->dev,
packet->usb_packet.ep);
}
QTAILQ_REMOVE(&ohci->active_packets, packet, next);
g_free(packet);
}
}

static void ohci_realize_pxa(DeviceState *dev, Error **errp)
{
OHCISysBusState *s = SYSBUS_OHCI(dev);
Expand Down Expand Up @@ -1999,9 +2096,6 @@ const VMStateDescription vmstate_ohci_state = {
VMSTATE_UINT32(hreset, OHCIState),
VMSTATE_UINT32(htest, OHCIState),
VMSTATE_UINT32(old_ctl, OHCIState),
VMSTATE_UINT8_ARRAY(usb_buf, OHCIState, 8192),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • How are in-flight packets handled now during vmsave/load events?
  • The structure change breaks snapshot compatibility

Copy link
Contributor Author

@faha223 faha223 Apr 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When loading a snapshot, vmstate_load_state invokes pre_load function pointer in the VMStateDescription if one was defined. Then it copies the data stored for each field in the VMState into the appropriate memory as defined in the VMStateDescription. Then it invokes the post_load function pointer in the VMStateDescription if one was defined.

Saving a snapshot does the same thing, but using the pre_save and post_save function pointers.

These fields are not defined in vmstate_ohci_state so there's no logic that runs when loading or saving a snapshot that interacts with these fields.

I suspect there's a real possibility that the VMState saving/restoring the async_td and usb_packet fields in snapshots can cause problems, but the structure change that would be needed to save and load the new list of async_packets would break snapshot compatibility.

I tested saving and loading snapshots with a libusb controller attached, both while interacting with the controller and while not interacting with the controller and it seemed to be mostly stable. I was able to crash the emulator during vmload a few times but the point of failure was clearing out the existing async packets before restoring the snapshot data. It didn't have anything to do with the actual snapshot data.

I've added back the fields I removed from OHCIState and vmstate_ohci_state since their removal breaks snapshot compatibility.

VMSTATE_UINT32(async_td, OHCIState),
VMSTATE_BOOL(async_complete, OHCIState),
VMSTATE_END_OF_LIST()
},
.subsections = (const VMStateDescription*[]) {
Expand Down
Loading