| Message ID | 1455541449-5008-1-git-send-email-arei.gonglei@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, > diff --git a/hw/usb/core.c b/hw/usb/core.c > index bea5e1e..6fbcf00 100644 > --- a/hw/usb/core.c > +++ b/hw/usb/core.c > @@ -716,7 +716,6 @@ struct USBEndpoint *usb_ep_get(USBDevice *dev, int pid, int ep) > if (ep == 0) { > return &dev->ep_ctl; > } > - assert(pid == USB_TOKEN_IN || pid == USB_TOKEN_OUT); Please keep this. > assert(ep > 0 && ep <= USB_MAX_ENDPOINTS); > return eps + ep - 1; > } > diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c > index 5ccfb83..ff66397 100644 > --- a/hw/usb/hcd-uhci.c > +++ b/hw/usb/hcd-uhci.c > @@ -883,6 +883,7 @@ static int uhci_handle_td(UHCIState *s, UHCIQueue *q, uint32_t qh_addr, > /* invalid pid : frame interrupted */ > uhci_async_free(async); > s->status |= UHCI_STS_HCPERR; > + s->cmd &= ~UHCI_CMD_RS; When clearing RS in cmd we should also set HALTED in status I think. How do we reach the assert above? Maybe it is enough to move this pid check to the start of the uhci_handle_td function to avoid triggering the assert? cheers, Gerd
> > Hi, > > > diff --git a/hw/usb/core.c b/hw/usb/core.c > > index bea5e1e..6fbcf00 100644 > > --- a/hw/usb/core.c > > +++ b/hw/usb/core.c > > @@ -716,7 +716,6 @@ struct USBEndpoint *usb_ep_get(USBDevice *dev, > int pid, int ep) > > if (ep == 0) { > > return &dev->ep_ctl; > > } > > - assert(pid == USB_TOKEN_IN || pid == USB_TOKEN_OUT); > > Please keep this. > > > assert(ep > 0 && ep <= USB_MAX_ENDPOINTS); > > return eps + ep - 1; > > } > > diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c > > index 5ccfb83..ff66397 100644 > > --- a/hw/usb/hcd-uhci.c > > +++ b/hw/usb/hcd-uhci.c > > @@ -883,6 +883,7 @@ static int uhci_handle_td(UHCIState *s, UHCIQueue > *q, uint32_t qh_addr, > > /* invalid pid : frame interrupted */ > > uhci_async_free(async); > > s->status |= UHCI_STS_HCPERR; > > + s->cmd &= ~UHCI_CMD_RS; > > When clearing RS in cmd we should also set HALTED in status I think. > Actually, uhci_frame_timer() had done this work. if (!(s->cmd & UHCI_CMD_RS)) { /* Full stop */ trace_usb_uhci_schedule_stop(); qemu_del_timer(s->frame_timer); uhci_async_cancel_all(s); /* set hchalted bit in status - UHCI11D 2.1.2 */ s->status |= UHCI_STS_HCHALTED; return; } > How do we reach the assert above? Maybe it is enough to move this pid > check to the start of the uhci_handle_td function to avoid triggering > the assert? > If Qemu read a wrong td, and then get a wrong pid, assertion will be reached. I thought that method, but I gave up as more complicated. Regards, -Gonglei
Hi, > > When clearing RS in cmd we should also set HALTED in status I think. > Actually, uhci_frame_timer() had done this work. > > if (!(s->cmd & UHCI_CMD_RS)) { > /* Full stop */ > trace_usb_uhci_schedule_stop(); > qemu_del_timer(s->frame_timer); > uhci_async_cancel_all(s); > /* set hchalted bit in status - UHCI11D 2.1.2 */ > s->status |= UHCI_STS_HCHALTED; > return; > } Ok, all fine then. > > > How do we reach the assert above? Maybe it is enough to move this pid > > > check to the start of the uhci_handle_td function to avoid triggering > > > the assert? > > > > > If Qemu read a wrong td, and then get a wrong pid, assertion will be reached. > I thought that method, but I gave up as more complicated. I think if we avoid calling usb_packet_setup with an invalid pid things should work fine. So checking whenever the pid is valid as very first thing in uhci_handle_td() should work, no? cheers, Gerd
diff --git a/hw/usb/core.c b/hw/usb/core.c index bea5e1e..6fbcf00 100644 --- a/hw/usb/core.c +++ b/hw/usb/core.c @@ -716,7 +716,6 @@ struct USBEndpoint *usb_ep_get(USBDevice *dev, int pid, int ep) if (ep == 0) { return &dev->ep_ctl; } - assert(pid == USB_TOKEN_IN || pid == USB_TOKEN_OUT); assert(ep > 0 && ep <= USB_MAX_ENDPOINTS); return eps + ep - 1; } diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c index 5ccfb83..ff66397 100644 --- a/hw/usb/hcd-uhci.c +++ b/hw/usb/hcd-uhci.c @@ -883,6 +883,7 @@ static int uhci_handle_td(UHCIState *s, UHCIQueue *q, uint32_t qh_addr, /* invalid pid : frame interrupted */ uhci_async_free(async); s->status |= UHCI_STS_HCPERR; + s->cmd &= ~UHCI_CMD_RS; uhci_update_irq(s); return TD_RESULT_STOP_FRAME; }
pid can be gotten from uhci device memory in uhci_handle_td(), so the guest can trigger assert qemu if we get an invalid pid. And the uhci spec 2.1.2 tells us The Host Controller sets Host Controller Process Error bit to 1 when it detects a fatal error and indicates that the Host Controller suffered a consistency check failure while processing a Transfer Descriptor. An example of a consistency check failure would be finding an illegal PID field while processing the packet header portion of the TD. When this error occurs, the Host Controller clears the Run/Stop bit in the Command register to prevent further schedule execution. We'd better to set UHCI_STS_HCPERR and kick an interrupt, but active assert Qemu, which follow the real hardware's spec. [Also fixed BZ 1070027] Signed-off-by: Gonglei <arei.gonglei@huawei.com> --- v2: Clears the Run/Stop bit in the Command register to prevent further schedule execution. hw/usb/core.c | 1 - hw/usb/hcd-uhci.c | 1 + 2 files changed, 1 insertion(+), 1 deletion(-)