| Message ID | 20230522153144.30610-10-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop | expand |
Philippe Mathieu-Daudé <philmd@linaro.org> writes: > Do not transmit characters when UART or transmitter are disabled. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
On Mon, 22 May 2023 at 16:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > Do not transmit characters when UART or transmitter are disabled. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> Last time somebody tried to add checks on the tx/rx enable bits for the PL011 it broke 'make check' because the hand-rolled UART code in boot-serial-test and migration-test doesn't set up the UART control register strictly correctly: https://lore.kernel.org/qemu-devel/CAFEAcA8ZDmjP7G0eVpxcB1jiSGarZAbqPV0xr5WquR213mBUBg@mail.gmail.com/ Given that imposing these checks doesn't help anything much and might break naive bare-metal tested-only-on-QEMU code, is it worthwhile ? > static void pl011_write_tx(PL011State *s, const uint8_t *buf, int length) > { > - /* ??? Check if transmitter is enabled. */ > + if (!(s->cr & (CR_UARTEN | CR_TXE))) { This will allow TX if either UARTEN or TXE is set, which probably isn't what you meant. > + return; > + } > > /* XXX this blocks entire thread. Rewrite to use > * qemu_chr_fe_write and background I/O callbacks */ thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Mon, 22 May 2023 at 16:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> >> Do not transmit characters when UART or transmitter are disabled. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > Last time somebody tried to add checks on the tx/rx enable bits > for the PL011 it broke 'make check' because the hand-rolled > UART code in boot-serial-test and migration-test doesn't > set up the UART control register strictly correctly: > > https://lore.kernel.org/qemu-devel/CAFEAcA8ZDmjP7G0eVpxcB1jiSGarZAbqPV0xr5WquR213mBUBg@mail.gmail.com/ > > Given that imposing these checks doesn't help anything > much and might break naive bare-metal tested-only-on-QEMU > code, is it worthwhile ? Surely we aim to be a correct model so the fix should be in our naive and incorrect code? > >> static void pl011_write_tx(PL011State *s, const uint8_t *buf, int length) >> { >> - /* ??? Check if transmitter is enabled. */ >> + if (!(s->cr & (CR_UARTEN | CR_TXE))) { > > This will allow TX if either UARTEN or TXE is set, which > probably isn't what you meant. > >> + return; >> + } >> >> /* XXX this blocks entire thread. Rewrite to use >> * qemu_chr_fe_write and background I/O callbacks */ > > thanks > -- PMM
On Thu, 25 May 2023 at 13:52, Alex Bennée <alex.bennee@linaro.org> wrote: > > > Peter Maydell <peter.maydell@linaro.org> writes: > > > On Mon, 22 May 2023 at 16:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > >> > >> Do not transmit characters when UART or transmitter are disabled. > >> > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > > > Last time somebody tried to add checks on the tx/rx enable bits > > for the PL011 it broke 'make check' because the hand-rolled > > UART code in boot-serial-test and migration-test doesn't > > set up the UART control register strictly correctly: > > > > https://lore.kernel.org/qemu-devel/CAFEAcA8ZDmjP7G0eVpxcB1jiSGarZAbqPV0xr5WquR213mBUBg@mail.gmail.com/ > > > > Given that imposing these checks doesn't help anything > > much and might break naive bare-metal tested-only-on-QEMU > > code, is it worthwhile ? > > Surely we aim to be a correct model so the fix should be in our naive > and incorrect code? In our own test suites, sure -- we should probably fix that even if we don't change the PL011 model to require it. But if we let this kind of thing get past us in our own testsuite, it suggests there's probably a lot of similar naive code out there in the world -- these Arm boards with PL011s are pretty commonly used for "my first bare metal assembly program" stuff and there's a lot of cargo-culting of how to do things like serial output, and programs that were never tested on any real hardware... thanks -- PMM
On 25/5/23 14:55, Peter Maydell wrote: > On Thu, 25 May 2023 at 13:52, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> >> Peter Maydell <peter.maydell@linaro.org> writes: >> >>> On Mon, 22 May 2023 at 16:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >>>> >>>> Do not transmit characters when UART or transmitter are disabled. >>>> >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> >>> Last time somebody tried to add checks on the tx/rx enable bits >>> for the PL011 it broke 'make check' because the hand-rolled >>> UART code in boot-serial-test and migration-test doesn't >>> set up the UART control register strictly correctly: >>> >>> https://lore.kernel.org/qemu-devel/CAFEAcA8ZDmjP7G0eVpxcB1jiSGarZAbqPV0xr5WquR213mBUBg@mail.gmail.com/ Hmm I ran 'make check-qtest' my build directory only targets aarch64, I probably missed the arm (32-bit) tests :/ >>> Given that imposing these checks doesn't help anything >>> much and might break naive bare-metal tested-only-on-QEMU >>> code, is it worthwhile ? >> >> Surely we aim to be a correct model so the fix should be in our naive >> and incorrect code? > > In our own test suites, sure -- we should probably fix that > even if we don't change the PL011 model to require it. > But if we let this kind of thing get past us in our own testsuite, > it suggests there's probably a lot of similar naive code out > there in the world -- these Arm boards with PL011s are pretty > commonly used for "my first bare metal assembly program" stuff > and there's a lot of cargo-culting of how to do things like > serial output, and programs that were never tested on any > real hardware... OK. I'll add a comment, keep the current behavior when TX is disabled, but add a GUEST_ERROR log message.
diff --git a/hw/char/pl011.c b/hw/char/pl011.c index c55ef41fbf..30bedeac15 100644 --- a/hw/char/pl011.c +++ b/hw/char/pl011.c @@ -76,6 +76,10 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr) #define LCR_FEN (1 << 4) #define LCR_BRK (1 << 0) +/* Control Register, UARTCR */ +#define CR_TXE (1 << 8) +#define CR_UARTEN (1 << 0) + static const unsigned char pl011_id_arm[8] = { 0x11, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1 }; static const unsigned char pl011_id_luminary[8] = @@ -151,7 +155,9 @@ static inline void pl011_reset_tx_fifo(PL011State *s) static void pl011_write_tx(PL011State *s, const uint8_t *buf, int length) { - /* ??? Check if transmitter is enabled. */ + if (!(s->cr & (CR_UARTEN | CR_TXE))) { + return; + } /* XXX this blocks entire thread. Rewrite to use * qemu_chr_fe_write and background I/O callbacks */
Do not transmit characters when UART or transmitter are disabled. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/char/pl011.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)