Re: [PATCH] hw/usb/hcd-ohci: Fix ohci_service_td: accept valid TDs

["Alex Bennée" <alex.bennee@linaro.org>]

Cord Amfmgm <dmamfmgm@gmail.com> writes:

> On Wed, Jun 12, 2024 at 9:21 AM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>  David Hubbard <dmamfmgm@gmail.com> writes:
>
>  > From: Cord Amfmgm <dmamfmgm@gmail.com>
>  >
>  > This changes the way the ohci emulation handles a Transfer Descriptor with
>  > "Current Buffer Pointer" set to "Buffer End" + 1.
>  >
>  > The OHCI spec 4.3.1.2 Table 4-2 allows td.cbp to be one byte more than td.be
>  > to signal the buffer has zero length. Currently qemu only accepts zero-length
>  > Transfer Descriptors if the td.cbp is equal to 0, while actual OHCI hardware
>  > accepts both cases.
>  >
>  > The qemu ohci emulation has a regression in ohci_service_td. Version 4.2
>  > and earlier matched the spec. (I haven't taken the time to bisect exactly
>  > where the logic was changed.)
>
>  I find it hard to characterise this as a regression because we've
>  basically gone from no checks to actually doing bounds checks:
>
>    1328fe0c32 (hw: usb: hcd-ohci: check len and frame_number variables)
>
>  The argument here seems to be that real hardware is laxer than the specs
>  in what it accepts.
>
<snip>
>
>  With the updated commit message:
>
>  Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>
> Please forgive my lack of experience on this mailing list. I don't see a suggested commit message from Alex but in case that
> was what is being considered, here is one. Feedback welcome, also if this is not what is wanted, please just say it.
>

Something along the lines of what you suggest here (where did this come
from?)

>> From: Cord Amfmgm <dmamfmgm@gmail.com>
>>
>> This changes the way the ohci emulation handles a Transfer Descriptor with
>> "Buffer End" set to "Current Buffer Pointer" - 1, specifically in the case of a
>> zero-length packet.
>>
>> The OHCI spec 4.3.1.2 Table 4-2 specifies td.cbp to be zero for a zero-length
>> packet. Peter Maydell tracked down commit
>> 1328fe0c32 (hw: usb: hcd-ohci: check len and frame_number variables)
>> where qemu started checking this according to the spec.
>>
>> What this patch does is loosen the qemu ohci implementation to allow a
>> zero-length packet if td.be (Buffer End) is set to td.cbp - 1, and with a
>> non-zero td.cbp value.
>>
>> Is this correct? It appears to not follow the spec, so no. But actual hw
>> seems to be ok with it.
>>
>> Does any OS rely on this behavior? There have been no reports to
>> qemu-devel of this problem.
>>
>> This is attempting to have qemu behave like actual hardware,
>> but this is just a minor change.
>>
>> With a tiny OS[1] that boots and executes a test, the behavior is
>> reproducible:
>>
>> * OS that sends USB requests to a USB mass storage device
>>   but sends td.be = td.cbp - 1
>> * qemu 4.2
>> * qemu HEAD (4e66a0854)
>> * Actual OHCI controller (hardware)
>>
>> Command line:
>> qemu-system-x86_64 -m 20 \
>>  -device pci-ohci,id=ohci \
>>  -drive if=none,format=raw,id=d,file=testmbr.raw \
>>  -device usb-storage,bus=ohci.0,drive=d \
>>  --trace "usb_*" --trace "ohci_*" -D qemu.log
>>
>> Results are:
>>
>>  qemu 4.2   | qemu HEAD  | actual HW
>> ------------+------------+------------
>>  works fine | ohci_die() | works fine
>>
>> Tip: if the flags "-serial pty -serial stdio" are added to the command line
>> the test will output USB requests like this:
>>
>> Testing qemu HEAD:
>>
>>> Free mem 2M ohci port2 conn FS
>>> setup { 80 6 0 1 0 0 8 0 }
>>> ED info=80000 { mps=8 en=0 d=0 } tail=c20920
>>>   td0 c20880 nxt=c20960 f2000000 setup cbp=c20900 be=c20907
>>>   td1 c20960 nxt=c20980 f3140000    in cbp=c20908 be=c2090f
>>>   td2 c20980 nxt=c20920 f3080000   out cbp=c20910 be=c2090f ohci20 host err
>>> usb stopped
>>
>> And in qemu.log:
>>
>> usb_ohci_iso_td_bad_cc_overrun ISO_TD start_offset=0x00c20910 > next_offset=0x00c2090f
>>
>> Testing qemu 4.2:
>>
>>> Free mem 2M ohci port2 conn FS
>>> setup { 80 6 0 1 0 0 8 0 }
>>> ED info=80000 { mps=8 en=0 d=0 } tail=620920
>>>   td0 620880 nxt=620960 f2000000 setup cbp=620900 be=620907       cbp=0 be=620907
>>>   td1 620960 nxt=620980 f3140000    in cbp=620908 be=62090f       cbp=0 be=62090f
>>>   td2 620980 nxt=620920 f3080000   out cbp=620910 be=62090f       cbp=0 be=62090f
>>>    rx { 12 1 0 2 0 0 0 8 }
>>> setup { 0 5 1 0 0 0 0 0 } tx {}
>>> ED info=80000 { mps=8 en=0 d=0 } tail=620880
>>>   td0 620920 nxt=620960 f2000000 setup cbp=620900 be=620907       cbp=0 be=620907
>>>   td1 620960 nxt=620880 f3100000    in cbp=620908 be=620907       cbp=0 be=620907
>>> setup { 80 6 0 1 0 0 12 0 }
>>> ED info=80001 { mps=8 en=0 d=1 } tail=620960
>>>   td0 620880 nxt=6209c0 f2000000 setup cbp=620920 be=620927       cbp=0 be=620927
>>>   td1 6209c0 nxt=6209e0 f3140000    in cbp=620928 be=620939       cbp=0 be=620939
>>>   td2 6209e0 nxt=620960 f3080000   out cbp=62093a be=620939       cbp=0 be=620939
>>>    rx { 12 1 0 2 0 0 0 8 f4 46 1 0 0 0 1 2 3 1 }
>>> setup { 80 6 0 2 0 0 0 1 }
>>> ED info=80001 { mps=8 en=0 d=1 } tail=620880
>>>   td0 620960 nxt=6209a0 f2000000 setup cbp=620a20 be=620a27       cbp=0 be=620a27
>>>   td1 6209a0 nxt=6209c0 f3140004    in cbp=620a28 be=620b27       cbp=620a48 be=620b27
>>>   td2 6209c0 nxt=620880 f3080000   out cbp=620b28 be=620b27       cbp=0 be=620b27
>>>    rx { 9 2 20 0 1 1 4 c0 0 9 4 0 0 2 8 6 50 0 7 5 81 2 40 0 0 7 5 2 2 40 0 0 }
>>> setup { 0 9 1 0 0 0 0 0 } tx {}
>>> ED info=80001 { mps=8 en=0 d=1 } tail=620900
>>>   td0 620880 nxt=620940 f2000000 setup cbp=620a00 be=620a07       cbp=0 be=620a07
>>>   td1 620940 nxt=620900 f3100000    in cbp=620a08 be=620a07       cbp=0 be=620a07
>>
>> [1] The OS disk image has been emailed to philmd@linaro.org, mjt@tls.msk.ru,
>> and kraxel@redhat.com:
>>
>> * testCbpOffBy1.img.xz
>> * sha256:
>> f87baddcb86de845de12f002c698670a426affb40946025cc32694f9daa3abed
>>
>> Signed-off-by: Cord Amfmgm <dmamfmgm@gmail.com>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro