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. 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. The spec says "last byte" which can (not "should" just "can") be interpreted as "cbp - 1". Therefore, I raised this patch request to re-enable the previous qemu behavior, in the sense of ""be conservative in what you do, be liberal in what you accept from others" - https://en.wikipedia.org/wiki/Robustness_principle It is *not* valid to say the spec disallows "cbp - 1" to mean "empty buffer." That is what I am arguing :) > > thanks > -- PMM >
