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

[Peter Maydell]

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)
 * 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).

Type 3 in particular is tricky and could use a commit message
explaining what it's doing.

I think the actual code changes are OK, but the FIFO handling
change is a bit complicated and at first I thought it had
introduced a bug.

thanks
-- PMM