Cord Amfmgm <dmamf...@gmail.com> writes:
> On Wed, Jun 12, 2024 at 9:21 AM Alex Bennée <alex.ben...@linaro.org> wrote:
>
> David Hubbard <dmamf...@gmail.com> writes:
>
> > From: Cord Amfmgm <dmamf...@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.ben...@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 <dmamf...@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 phi...@linaro.org, m...@tls.msk.ru,
>> and kra...@redhat.com:
>>
>> * testCbpOffBy1.img.xz
>> * sha256:
>> f87baddcb86de845de12f002c698670a426affb40946025cc32694f9daa3abed
>>
>> Signed-off-by: Cord Amfmgm <dmamf...@gmail.com>
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
