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.
Signed-off-by: Evgeny Iakovlev <eiakovlev@linux.microsoft.com>
---
hw/char/pl011.c | 35 +++++++++++++++++++++--------------
include/hw/char/pl011.h | 5 ++++-
2 files changed, 25 insertions(+), 15 deletions(-)
diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index c076813423..9108ed2be9 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -81,6 +81,18 @@ static void pl011_update(PL011State *s)
}
}
+static inline unsigned pl011_get_fifo_depth(PL011State *s)
+{
+ return s->lcr & 0x10 ? PL011_FIFO_DEPTH : 1;
+}
+
+static inline void pl011_reset_pipe(PL011State *s)
+{
+ s->read_count = 0;
+ s->read_pos = 0;
+ s->flags = PL011_FLAG_RXFE | PL011_FLAG_TXFE;
+}
+
static uint64_t pl011_read(void *opaque, hwaddr offset,
unsigned size)
{
@@ -94,7 +106,7 @@ static uint64_t pl011_read(void *opaque, hwaddr offset,
c = s->read_fifo[s->read_pos];
if (s->read_count > 0) {
s->read_count--;
- if (++s->read_pos == 16)
+ if (++s->read_pos == PL011_FIFO_DEPTH)
s->read_pos = 0;
}
if (s->read_count == 0) {
@@ -229,8 +241,7 @@ static void pl011_write(void *opaque, hwaddr offset,
case 11: /* UARTLCR_H */
/* Reset the FIFO state on FIFO enable or disable */
if ((s->lcr ^ value) & 0x10) {
- s->read_count = 0;
- s->read_pos = 0;
+ pl011_reset_pipe(s);
}
if ((s->lcr ^ value) & 0x1) {
int break_enable = value & 0x1;
@@ -273,11 +284,7 @@ static int pl011_can_receive(void *opaque)
PL011State *s = (PL011State *)opaque;
int r;
- if (s->lcr & 0x10) {
- r = s->read_count < 16;
- } else {
- r = s->read_count < 1;
- }
+ r = s->read_count < pl011_get_fifo_depth(s);
trace_pl011_can_receive(s->lcr, s->read_count, r);
return r;
}
@@ -286,15 +293,15 @@ static void pl011_put_fifo(void *opaque, uint32_t value)
{
PL011State *s = (PL011State *)opaque;
int slot;
+ unsigned pipe_depth;
- slot = s->read_pos + s->read_count;
- if (slot >= 16)
- slot -= 16;
+ pipe_depth = pl011_get_fifo_depth(s);
+ slot = (s->read_pos + s->read_count) % pipe_depth;
s->read_fifo[slot] = value;
s->read_count++;
s->flags &= ~PL011_FLAG_RXFE;
trace_pl011_put_fifo(value, s->read_count);
- if (!(s->lcr & 0x10) || s->read_count == 16) {
+ if (s->read_count == pipe_depth) {
trace_pl011_put_fifo_full();
s->flags |= PL011_FLAG_RXFF;
}
@@ -359,7 +366,7 @@ static const VMStateDescription vmstate_pl011 = {
VMSTATE_UINT32(dmacr, PL011State),
VMSTATE_UINT32(int_enabled, PL011State),
VMSTATE_UINT32(int_level, PL011State),
- VMSTATE_UINT32_ARRAY(read_fifo, PL011State, 16),
+ VMSTATE_UINT32_ARRAY(read_fifo, PL011State, PL011_FIFO_DEPTH),
VMSTATE_UINT32(ilpr, PL011State),
VMSTATE_UINT32(ibrd, PL011State),
VMSTATE_UINT32(fbrd, PL011State),
@@ -399,7 +406,7 @@ static void pl011_init(Object *obj)
s->read_trigger = 1;
s->ifl = 0x12;
s->cr = 0x300;
- s->flags = 0x90;
+ pl011_reset_pipe(s);
s->id = pl011_id_arm;
}
diff --git a/include/hw/char/pl011.h b/include/hw/char/pl011.h
index dc2c90eedc..926322e242 100644
--- a/include/hw/char/pl011.h
+++ b/include/hw/char/pl011.h
@@ -27,6 +27,9 @@ OBJECT_DECLARE_SIMPLE_TYPE(PL011State, PL011)
/* This shares the same struct (and cast macro) as the base pl011 device */
#define TYPE_PL011_LUMINARY "pl011_luminary"
+/* Depth of UART FIFO in bytes, when FIFO mode is enabled (else depth == 1) */
+#define PL011_FIFO_DEPTH 16
+
struct PL011State {
SysBusDevice parent_obj;
@@ -39,7 +42,7 @@ struct PL011State {
uint32_t dmacr;
uint32_t int_enabled;
uint32_t int_level;
- uint32_t read_fifo[16];
+ uint32_t read_fifo[PL011_FIFO_DEPTH];
uint32_t ilpr;
uint32_t ibrd;
uint32_t fbrd;
--
2.34.1On 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
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
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
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
>
© 2016 - 2026 Red Hat, Inc.