On Fri, Jun 7, 2024 at 8:23 AM Peter Maydell <peter.mayd...@linaro.org> wrote:
> On Fri, 31 May 2024 at 19:16, Cord Amfmgm <dmamf...@gmail.com> wrote: > > On Fri, May 31, 2024 at 9:03 AM Peter Maydell <peter.mayd...@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.
