OHCI/usb pass through

OHCI/usb pass through

[BALATON Zoltan]

Hello,

We're trying to find out why passing through an usb sound card fails with 
MacOS/OS X on mac99 and came across some things in OHCI that I don't 
understand so some help from those who know more about USB handling in 
QEMU or OHCI would be needed.

From traces Howard collected we see that a packet is submitted to libusb 
which does not complete immediately so it gets recorded as async but never 
seems to complete afterwards. Meanwhile some isochronous traffic is 
happening on a different endpoint but it is then rejected with too many 
pending packets due to the waiting async packet and things seem to stop at 
this point.

There is a comment in hw/usb/hcd-ohci.c:1031 in ohci_service_td() that 
says this is something that is not modelled correctly as it should allow 
active packets per endpoint while we only have one packet per controller 
(but maybe there are other problems than this too). The problem seems to 
be that currently we have this active packet recorded in OHCIState in 
these fields:

[...]
     /* Active packets.  */
     uint32_t old_ctl;
     USBPacket usb_packet;
     uint8_t usb_buf[8192];
     uint32_t async_td;
     bool async_complete;

     void (*ohci_die)(struct OHCIState *ohci);
} OHCIState;

Then everything in hcd-ohci seems to reuse ohci->usb_packet and I wonder 
if it can happen that it's overwritten while an async packet is still 
using it. It seems to be reset calling usb_packet_setup() in two places: 
ohci_service_td() and ohci_service_iso_td(). While ohci_service_td() 
checks for ohci->async_td, ohci_service_iso_td() doesn't seem to so maybe 
it can break the pending async packet if an isochronous request comes in 
while the other endpoint is waiting for the async packet. If so maybe when 
the completion is called it won't notice because ohci->usb_packet is 
already a different packet overwritten by ohci_service_iso_td(). Did I 
miss some other checks elsewhere that prevent this from happening? (I 
don't know how USB is handled in QEMU or how OHCI works so it could be I'm 
not understing this correctly.)

In any case to both fix the device model and to avoid this possible 
problem described above it seems we would need to ditch the packet and 
async_td fields from OHCIState and move them to the endpoint to allow one 
active packet per endpoint. We can get the endpoint from a packet and from 
ohci so I wonder if we can get the active packet from ep->queue (and how 
to do that) and then can we find out if it's waiting by checking if this 
packet's status is USB_PACKET_ASYNC so we don't need to keep track of 
these in OHCIState. I don't understand this code enough to try to do it 
but maybe some hints could help.

Thanks,
BALATON Zoltan

Re: OHCI/usb pass through

[Gerd Hoffmann]

  Hi,

> [...]
>     /* Active packets.  */
>     uint32_t old_ctl;
>     USBPacket usb_packet;
>     uint8_t usb_buf[8192];
>     uint32_t async_td;
>     bool async_complete;
> 
>     void (*ohci_die)(struct OHCIState *ohci);
> } OHCIState;
> 
> Then everything in hcd-ohci seems to reuse ohci->usb_packet and I wonder if
> it can happen that it's overwritten while an async packet is still using it.

Plausible theory.  That also nicely explains why you need traffic on two
endpoints at the same time to trigger it.

> In any case to both fix the device model and to avoid this possible problem
> described above it seems we would need to ditch the packet and async_td
> fields from OHCIState and move them to the endpoint to allow one active
> packet per endpoint.

Either that, or maintain a linked list of packets.

> We can get the endpoint from a packet and from ohci so
> I wonder if we can get the active packet from ep->queue (and how to do that)

I think ohci never looks beyond the active td so there should never be
more than one packet on the list.

HTH,
  Gerd

Re: OHCI/usb pass through

[BALATON Zoltan]

Hello,

On Fri, 1 Oct 2021, Gerd Hoffmann wrote:
>> [...]
>>     /* Active packets.  */
>>     uint32_t old_ctl;
>>     USBPacket usb_packet;
>>     uint8_t usb_buf[8192];
>>     uint32_t async_td;
>>     bool async_complete;
>>
>>     void (*ohci_die)(struct OHCIState *ohci);
>> } OHCIState;
>>
>> Then everything in hcd-ohci seems to reuse ohci->usb_packet and I wonder if
>> it can happen that it's overwritten while an async packet is still using it.
>
> Plausible theory.  That also nicely explains why you need traffic on two
> endpoints at the same time to trigger it.

We've added an assert to check this and Howard could trigger it at least 
once (hope he'll answer with details) so I think that proves this but 
there may be other problems too as it does not work even when the assert 
is not triggered but maybe that's becuase not allowing traffic while an 
async packet is pending. It looks like it starts an interrupt transfer on 
an endpoint while sends iso data to another. I don't know usb audio 
protocol but maybe it's waiting for the iso transfer to finish which won't 
as those packets are now rejected due to too many waiting. Howard has some 
logs but I've only seen excerpts and did not reproduce it myself so I 
don't know exactly. In any case, fixing both of this possible breakage of 
ohci->usb_packet and emulating multiple pending packets correctly should 
improve the OHCI model and maybe get us further or at least prove we have 
more problems to look for.

>> In any case to both fix the device model and to avoid this possible problem
>> described above it seems we would need to ditch the packet and async_td
>> fields from OHCIState and move them to the endpoint to allow one active
>> packet per endpoint.
>
> Either that, or maintain a linked list of packets.

I'd like to avoid duplicating state and keep everyting attached to the 
endpoint so we don't need to keep track of it in ohci. We'd need a packet 
for each endpoint and also make async_td an array then so if instead we 
can get this info from the endpoint that we already have a pointer to, we 
could use that and remove both of these duplicate values from OHCIState so 
I'd try that as it looks less error prone.

>> We can get the endpoint from a packet and from ohci so
>> I wonder if we can get the active packet from ep->queue (and how to do that)
>
> I think ohci never looks beyond the active td so there should never be
> more than one packet on the list.

OK, how to get the packet from that QTAILQ list? If there are multiple 
packets is the active one first or last? How to get that? I could try to 
find the answers in the code but I realy did not want to spend much time 
with it just trying to help Howard so I'd like to ask for some help with 
this.

Regards,
BALATON Zoltan

Re: OHCI/usb pass through

[Gerd Hoffmann]

  Hi,

> > > We can get the endpoint from a packet and from ohci so
> > > I wonder if we can get the active packet from ep->queue (and how to do that)
> > 
> > I think ohci never looks beyond the active td so there should never be
> > more than one packet on the list.
> 
> OK, how to get the packet from that QTAILQ list? If there are multiple
> packets is the active one first or last? How to get that? I could try to
> find the answers in the code but I realy did not want to spend much time
> with it just trying to help Howard so I'd like to ask for some help with
> this.

See include/qemu/queue.h, QTAILQ_FIRST() is what you need.  Returns NULL
when the queue is empty.  There is also a separate QLIST_EMPTY() helper.

grepping in hw/usb/*.c should find you code examples in other host
adapters code.

take care,
  Gerd