Re: [PATCH 1/2] hw/char/pl011: better handling of FIFO flags on LCR reset

[eiakovlev@linux.microsoft.com]

On 1/17/23 5:02 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On Tue, 17 Jan 2023 at 15:54, Evgeny Iakovlev
> <eiakovlev@linux.microsoft.com> wrote:
> >
> >
> > On 1/17/2023 16:24, Peter Maydell wrote:
> >> On Fri, 6 Jan 2023 at 17:28, Evgeny Iakovlev
> >> <eiakovlev@linux.microsoft.com> wrote:
> >>> Current FIFO handling code does not reset RXFE/RXFF flags when guest
> >>> resets FIFO by writing to UARTLCR register, although internal FIFO state
> >>> is reset to 0 read count. Actual flag update will happen only on next
> >>> read or write to UART. As a result of that any guest that expects RXFE
> >>> flag to be set (and RXFF to be cleared) after resetting FIFO will just
> >>> hang.
> >>>
> >>> Correctly reset FIFO flags on FIFO reset. Also, clean up some FIFO
> >>> depth handling code based on current FIFO mode.
> >> This patch is doing multiple things at once ("also" in a
> >> commit message is often a sign of that) and I think it
> >> would be helpful to split it. There are three things here:
> >>    * refactorings which aren't making any real code change
> >>      (eg calling pl011_get_fifo_depth() instead of doing the
> >>      "if LCR bit set then 16 else 1" inline)
> >
> >
> > Yeah, tbh i also though i should do it..
> >
> >
> >>    * the fix to the actual bug
> >>    * changes to the handling of the FIFO which should in theory
> >>      not be visible to the guest (I think it now when the FIFO
> >>      is disabled always puts the incoming character in read_fifo[0],
> >>      whereas previously it would allow read_pos to increment all
> >>      the way around the FIFO even though we only keep one character
> >>      at a time).
> >
> >
> > That last part i don't understand. If by changes to the FIFO you are
> > referring to the flags handling, then it will be visible to the guest by
> > design, since that's what I'm fixing. Could you maybe explain that one
> > again? :)
> 
> I meant the bit where the existing code for the FIFO-disabled
> case puts incoming characters in s->read_fifo[s->read_pos] and
> reads from UARTDR always increment s->read_pos; whereas the
> changed code always puts characters in s->read_fifo[0] and
> avoids incrementing read_pos. I think your version is more
> sensible (and also matches what the TRM claims the hardware
> is doing), although the guest-visible behaviour doesn't change.

I see, thanks. I tried separating this particular piece into its own commit, but i just feel like it makes the part that's left pointless, because pl011_get_fifo_depth replaces mostly just those 2 occurences anyway.. I've added a paragraph to a commit message to document a functional change. Let me know if that's not good enough. I've sent out a v2 with the rest of comments addressed + a reset method for PL011.

> 
> thanks
> -- PMM
>