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

[Evgeny Iakovlev]

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? :)


> 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