Re: [Qemu-devel] "usb: uhci: Look up queue by address, not token"

[Jan Kiszka]

On 2012-11-16 14:29, Hans de Goede wrote:
> Hi,
> 
> On 11/15/2012 04:40 PM, Jan Kiszka wrote:
>> Hi Hans,
>>
>> On 2012-11-15 16:19, Hans de Goede wrote:
>>> Hi Jan,
>>>
>>> I just saw your $subject patch in Gerd's usb-next tree, and I've a question
>>> about it. The token should be enough to uniquely identify a device + ep,
>>> and unless a guest uses multiple qhs for a singe ep, that _should_ be 
>>> enough.
>>
>> But what disallows that the guest issues multiple requests (QH + series
>> of TDs) for a single endpoint? I'm not finding any trace in the spec
>> that disallows this.
> 
> It may not be explicitly forbidden in the spec, but the whole text of the
> spec implies a 1 on 1 relationship between qhs and eps, and this is how
> all major guests do it.

Was afraid of this...

> 
>> And my special guest is stumbling over that limitation in QEMU.
> 
> I guess you're not at liberty to disclose what guest this is ?
> 

Hairy legacy code with additions I'm not yet able to share.

> <snip>
> 
>>> The reason why I want to know, is that identifying the queue by qh has a
>>> disadvantage, to be precise I believe the following can then happen:
>>>
>>> 1) The guest queues up multiple requests for a queue
>>> 2) We execute one, go async
>>> 3) The guest changes it mind an unlinks the qh
>>> 4) The guest will think the queue is cancelled after frindex has
>>> changed by 1, but we keep it around for 64 frames (which sucks,
>>> and I want to improve on this, but we need to for various reasons)
>>> 5) The guests submits some new requests to the queue, using a
>>> new qh (which possibly has a different address).
>>> 6) We see the new qh, and execute a request from there
>>> 7) The 1st request on the old qh completes, and we execute the next
>>> 8) Now things are a mess as we're executing requests from the old
>>> (cancelled from the guest pov) and new queue intermixed...
>>>
>>> Using the token to identify the queue fixes this, cause we will
>>> find the same queue for the old and new qh, and uhci_queue_verify()
>>> will then fail because of the qh addr change, and then we cancel
>>> the old queue. IOW then we do the right thing.
>>
>> I was afraid of triggering some conflict but, as pointed out above, the
>> current code also does something wrong. It associates two different
>> active QH with the same queue and then triggers a failure for the first
>> QH as it wrongly thinks the guest has unlinked it.
>>
>>>
>>> So I'm wondering if there is another, potentially better fix for
>>> what you are seeing?
>>
>> /me too. I'm not fully understanding your scenario above yet,
>> specifically when and how the guest can correctly remove an active QH,
> 
> A guest can unlink an active qh when it wants to cancel the requests
> in there (ie a driver level timeout kicks in). This is a quite normal
> thing to do (unlike using multiple qhs for a single ep).
> 
> For ehci we can reliable detect this happening (except for interrupt
> endpoints, sigh) thanks to the "doorbell" mechanism. But for uhci it
> is harder, and things are further complicated by the fact that
> Linux' full speed recovery code for bulk endpoints temporarily unlinks
> and then relinks a pending requests to make some changes to the qh
> while requests are in flight ... (bad Linux, bad!).
> 
> So we rely on a combination of measures to detect cancels in the
> qemu uhci code, of which the qh validation is one, and this gets
> broken by your patch.

How should unlink be defined here? A qh is no longer present in any
chain of any frame? For how long?

> 
>> so I cannot provide good suggestions at this point.
> 
> I see 2 solutions at this point:
> 1) You modify your special guest if you can
> 2) We add a property to the uhci emulation to search for queues
> by address rather then by token, which would of course default
> to false.

Well, this particular thing worked on real hw, so I thought it would be
good to adjust QEMU. But as I'm struggling with my guest anyway, it may
turn out that we will no longer need this in the end - at least in this
case. Still, I'm with a bad feeling regarding the current heuristics for
identifying invalidated queues.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux