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

[Peter Maydell]

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.

thanks
-- PMM