Re: [PATCH 09/12] hw/char/pl011: Check if transmitter is enabled

[Alex Bennée]

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


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro