On Wed, Jun 12, 2024 at 2:36 PM Alex Bennée <alex.ben...@linaro.org> wrote:
> 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>
>
>
Oh, I just manually put it in blockquotes to clearly delineate what it is.
I authored this new message in the previous message.
