| Message ID | 20230117220523.20911-4-eiakovlev@linux.microsoft.com |
|---|---|
| State | New |
| Headers | show |
| Series | Series of fixes for PL011 char device | expand |
On Tue, 17 Jan 2023 at 22:05, 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 guest-visible flag update will happen > only on next data read or write attempt. As a result of that any guest > that expects RXFE flag to be set (and RXFF to be cleared) after resetting > FIFO will never see that happen. > > Signed-off-by: Evgeny Iakovlev <eiakovlev@linux.microsoft.com> > --- > hw/char/pl011.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/hw/char/pl011.c b/hw/char/pl011.c > index 404d52a3b8..3184949d69 100644 > --- a/hw/char/pl011.c > +++ b/hw/char/pl011.c > @@ -87,6 +87,13 @@ 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; Should this really be resetting all the other flags to 0 ? I think we should set/clear only the FIFO related flags, and leave the others alone. We don't yet implement the modem-status signals, but if/when we ever do, clearing them would be the wrong thing here. (Reset still needs to reset all the flag register bits.) thanks -- PMM
On 1/19/2023 14:30, Peter Maydell wrote: > On Tue, 17 Jan 2023 at 22:05, 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 guest-visible flag update will happen >> only on next data read or write attempt. As a result of that any guest >> that expects RXFE flag to be set (and RXFF to be cleared) after resetting >> FIFO will never see that happen. >> >> Signed-off-by: Evgeny Iakovlev <eiakovlev@linux.microsoft.com> >> --- >> hw/char/pl011.c | 14 +++++++++----- >> 1 file changed, 9 insertions(+), 5 deletions(-) >> >> diff --git a/hw/char/pl011.c b/hw/char/pl011.c >> index 404d52a3b8..3184949d69 100644 >> --- a/hw/char/pl011.c >> +++ b/hw/char/pl011.c >> @@ -87,6 +87,13 @@ 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; > Should this really be resetting all the other flags to 0 ? > I think we should set/clear only the FIFO related flags, and > leave the others alone. We don't yet implement the > modem-status signals, but if/when we ever do, clearing them > would be the wrong thing here. > > (Reset still needs to reset all the flag register bits.) Right, i thought about it, but as you mention we only use FIFO flags currently. Still, your suggestion about future changes makes sense. > > thanks > -- PMM
diff --git a/hw/char/pl011.c b/hw/char/pl011.c index 404d52a3b8..3184949d69 100644 --- a/hw/char/pl011.c +++ b/hw/char/pl011.c @@ -87,6 +87,13 @@ 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) { @@ -234,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; @@ -421,12 +427,10 @@ static void pl011_reset(DeviceState *dev) s->ilpr = 0; s->ibrd = 0; s->fbrd = 0; - s->read_pos = 0; - s->read_count = 0; s->read_trigger = 1; s->ifl = 0x12; s->cr = 0x300; - s->flags = 0x90; + pl011_reset_pipe(s); for (i = 0; i < ARRAY_SIZE(s->irq); i++) { qemu_irq_lower(s->irq[i]);
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 guest-visible flag update will happen only on next data read or write attempt. As a result of that any guest that expects RXFE flag to be set (and RXFF to be cleared) after resetting FIFO will never see that happen. Signed-off-by: Evgeny Iakovlev <eiakovlev@linux.microsoft.com> --- hw/char/pl011.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)