On Tue, May 28, 2024 at 11:32 AM Peter Maydell <peter.mayd...@linaro.org> wrote:
> On Tue, 28 May 2024 at 16:37, Cord Amfmgm <dmamf...@gmail.com> wrote: > > > > > > > > On Tue, May 28, 2024 at 9:03 AM Peter Maydell <peter.mayd...@linaro.org> > wrote: > >> > >> On Mon, 20 May 2024 at 23:24, Cord Amfmgm <dmamf...@gmail.com> wrote: > >> > On Mon, May 20, 2024 at 12:05 PM Peter Maydell < > peter.mayd...@linaro.org> wrote: > >> >> For the "zero length buffer" case, do you have a more detailed > >> >> pointer to the bit of the spec that says that "cbp = be + 1" is a > >> >> valid way to specify a zero length buffer? Table 4-2 in the copy I > >> >> have says for CurrentBufferPointer "a value of 0 indicates > >> >> a zero-length data packet or that all bytes have been transferred", > >> >> and the sample host OS driver function QueueGeneralRequest() > >> >> later in the spec handles a 0 length packet by setting > >> >> TD->HcTD.CBP = TD->HcTD.BE = 0; > >> >> (which our emulation's code does handle). > >> > > >> > > >> > Reading the spec carefully, a CBP set to 0 should always mean the > zero-length buffer case (or that all bytes have been transferred, so the > buffer is now zero-length - the same thing). > >> > >> Yeah, I can see the argument for "we should treat all cbp == 0 as > >> zero-length buffer, not just cbp == be == 0". > >> > >> > Table 4-2 is the correct reference, and this part is clear. It's the > part you quoted. "Contains the physical address of the next memory location > that will be accessed for transfer to/from the endpoint. A value of 0 > indicates a zero-length data packet or that all bytes have been > transferred." > >> > > >> > Table 4-2 has this additional nugget that may be confusingly worded, > for BufferEnd: "Contains physical address of the last byte in the buffer > for this TD" > >> > >> Mmm, but for a zero length buffer there is no "last byte" to > >> have this be the physical address of. > >> > >> > And here's an example buffer of length 0 -- you probably already know > what I'm going to do here: > >> > > >> > char buf[0]; > >> > char * CurrentBufferPointer = &buf[0]; > >> > char * BufferEnd = &buf[-1]; // "address of the last byte in the > buffer" > >> > // The OHCI Host Controller than advances CurrentBufferPointer like > this: CurrentBufferPointer += 0 > >> > // After the transfer: > >> > // CurrentBufferPointer = &buf[0]; > >> > // BufferEnd = &buf[-1]; > >> > >> Right, but why do you think this is valid, rather than > >> being a guest software bug? My reading of the spec is that it's > >> pretty clear about how to say "zero length buffer", and this > >> isn't it. > >> > >> Is there some real-world guest OS that programs the OHCI > >> controller this way that we're trying to accommodate? > > > > > > qemu versions 4.2 and before allowed this behavior. > > So? That might just mean we had a bug and we fixed it. > 4.2 is a very old version of QEMU and nobody seems to have > complained in the four years since we released 5.0 about this, > which suggests that generally guest OS drivers don't try > to send zero-length buffers in this way. > > > I don't think it's valid to ask for a *popular* guest OS as a > proof-of-concept because I'm not an expert on those. > > I didn't ask for "popular"; I asked for "real-world". > What is the actual guest code you're running that falls over > because of the behaviour change? > > More generally, why do you want this behaviour to be > changed? Reasonable reasons might include: > * we're out of spec based on reading the documentation > * you're trying to run some old Windows VM/QNX/etc image, > and it doesn't work any more > * all the real hardware we tested behaves this way > > But don't necessarily include: > * something somebody wrote and only tested on QEMU happens to > assume the old behaviour rather than following the hw spec > > QEMU occasionally works around guest OS bugs, but only as > when we really have to. It's usually better to fix the > bug in the guest. > It's not, and I've already demonstrated that real hardware is consistent with the fix in this patch. Please check your tone. > > thanks > -- PMM >
