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

[Cord Amfmgm <dmamfmgm@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3720 bytes --]

On Fri, Jun 7, 2024 at 8:23 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Fri, 31 May 2024 at 19:16, Cord Amfmgm <dmamfmgm@gmail.com> wrote:
> > On Fri, May 31, 2024 at 9:03 AM Peter Maydell <peter.maydell@linaro.org>
> wrote:
> >> What I would like to see is what we could classify under
> >> "rationale", which is to say "what prompted us to make this
> >> change?". In my experience it's important to record this
> >> (including in the commit message). There are of course
> >> many cases in QEMU's git history where we failed to do that,
> >> but in general I think it's a good standard to meet. (I
> >> am also erring on the side of caution in reviewing this
> >> particular patch, because I don't know the relevant standards
> >> or this bit of the code very well.)
>
> > Thanks, in responding to that, I'm breaking down my responses into the
> following answers:
> >
> > Q1: (summarizing) What prompted us to make this change?
> >
> > A1: I'm summarizing what I wrote in previous emails and the commit
> message -
> >
> > * Bring qemu closer to actual hw with a neatly packaged test case to
> demonstrate the behavior
> > * I interpret the spec (Table 4-2) as saying the "be = cbp - 1" is
> valid, in addition to setting "cbp = 0"
> > * I interpret it that way due to a comment in an old linux kernel
> version in the 2.x range, ohci-hcd.c file. It said (paraphrasing) some
> misbehaving ohci controllers would fetch physical memory at 0 when cbp = 0
> was in the Transfer Descriptor
>
> Interesting. Do you have a more specific version for that?
> I had a look at various 2.x Linux OHCI drivers but they all seem to do
> zero-length packets "by the spec" with CBP=BE=0. eg 2.6.39.4:
>
> https://elixir.bootlin.com/linux/v2.6.39.4/source/drivers/usb/host/ohci-q.c#L539
> and there's no hardware-quirk handling to do it differently on
> some controllers. (The USB OHCI driver seems to have gone through
> a couple of rewrites in the 2.x kernel timeframe; the earlier 2.3.51
> version does the TD fill here:
> https://elixir.bootlin.com/linux/2.3.51/source/drivers/usb/usb-ohci.c#L812
> and again it handles zero length as BE=CBP=0.)
>
> > But I only have a test case I created myself, and am not an expert on
> computer history here. I think "be liberal in what you accept, by
> conservative in what you send" applies when I don't know which historical
> OSes, if any, need this behavior. I think the behavior of actual hardware
> weighs more heavily since that *is* available and testable. Are there
> additional checks that would expand on what's known about actual ohci hw?
>
> The other side of the argument is "if in doubt and you don't
> know of any concrete problem being caused, don't change
> working code". If there are any historical OSes that rely on
> being able to send zero packets with be=cbp-1 for some nonzero
> be, and anybody wants to run them on QEMU, then presumably they
> can send us a bug report saying "XYZ's USB support doesn't work".
> That nobody has ever does this is evidence on the side of
> "there is no such OS out there, everybody writing an OHCI driver
> read the spec and made their driver send zero length packets the
> way the spec very clearly says you should". Please correct me
> if I'm wrong, but my interpretation of your helpful explanation
> above is that this is essentially a theoretical problem rather
> than one that's caused you a problem you need to fix.
>

 I think that's fair.

I sent in the patch more out of a desire for qemu to have the greatest
possible ohci implementation, than due to knowledge of an actual OS that
couldn't work.

Up to you what to do from here.

[-- Attachment #2: Type: text/html, Size: 4834 bytes --]