Revert "hw/char/pl011: Warn when using disabled receiver"

[PATCH] Revert "hw/char/pl011: Warn when using disabled receiver"

Posted by Paolo Bonzini 11 months ago

The guest does not control whether characters are sent on the UART.
Sending them before the guest happens to boot will now result in a
"guest error" log entry that is only because of timing, even if the
guest _would_ later setup the receiver correctly.

This reverts commit abf2b6a028670bd2890bb3aee7e103fe53e4b0df, apart
from adding the comment.

Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
Cc: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/pl011.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 23a9db8c57c..efca8baecd7 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -85,7 +85,6 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr)
 #define CR_OUT1     (1 << 12)
 #define CR_RTS      (1 << 11)
 #define CR_DTR      (1 << 10)
-#define CR_RXE      (1 << 9)
 #define CR_TXE      (1 << 8)
 #define CR_LBE      (1 << 7)
 #define CR_UARTEN   (1 << 0)
@@ -490,16 +489,9 @@ static int pl011_can_receive(void *opaque)
     unsigned fifo_depth = pl011_get_fifo_depth(s);
     unsigned fifo_available = fifo_depth - s->read_count;
 
-    if (!(s->cr & CR_UARTEN)) {
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "PL011 receiving data on disabled UART\n");
-    }
-    if (!(s->cr & CR_RXE)) {
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "PL011 receiving data on disabled RX UART\n");
-    }
+    /* Should check enable and return 0? */
+
     trace_pl011_can_receive(s->lcr, s->read_count, fifo_depth, fifo_available);
-
     return fifo_available;
 }
 
-- 
2.48.1

Re: [PATCH] Revert "hw/char/pl011: Warn when using disabled receiver"

Posted by Peter Maydell 11 months ago

On Tue, 11 Mar 2025 at 15:37, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> The guest does not control whether characters are sent on the UART.
> Sending them before the guest happens to boot will now result in a
> "guest error" log entry that is only because of timing, even if the
> guest _would_ later setup the receiver correctly.
>
> This reverts commit abf2b6a028670bd2890bb3aee7e103fe53e4b0df, apart
> from adding the comment.
>
> Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/char/pl011.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index 23a9db8c57c..efca8baecd7 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -85,7 +85,6 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr)
>  #define CR_OUT1     (1 << 12)
>  #define CR_RTS      (1 << 11)
>  #define CR_DTR      (1 << 10)
> -#define CR_RXE      (1 << 9)
>  #define CR_TXE      (1 << 8)
>  #define CR_LBE      (1 << 7)
>  #define CR_UARTEN   (1 << 0)
> @@ -490,16 +489,9 @@ static int pl011_can_receive(void *opaque)
>      unsigned fifo_depth = pl011_get_fifo_depth(s);
>      unsigned fifo_available = fifo_depth - s->read_count;
>
> -    if (!(s->cr & CR_UARTEN)) {
> -        qemu_log_mask(LOG_GUEST_ERROR,
> -                      "PL011 receiving data on disabled UART\n");
> -    }
> -    if (!(s->cr & CR_RXE)) {
> -        qemu_log_mask(LOG_GUEST_ERROR,
> -                      "PL011 receiving data on disabled RX UART\n");
> -    }
> +    /* Should check enable and return 0? */

We decided deliberately not to check the enable and return 0
here, as described in the commit message of abf2b6a028670bd:
we think there's too likely to be existing works-on-QEMU code
out there that doesn't ever set the enable bits.

Otherwise, yes, agreed with the revert.

thanks
-- PMM

Re: [PATCH] Revert "hw/char/pl011: Warn when using disabled receiver"

Posted by Peter Maydell 11 months ago

On Wed, 12 Mar 2025 at 13:36, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 11 Mar 2025 at 15:37, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> > The guest does not control whether characters are sent on the UART.
> > Sending them before the guest happens to boot will now result in a
> > "guest error" log entry that is only because of timing, even if the
> > guest _would_ later setup the receiver correctly.
> >
> > This reverts commit abf2b6a028670bd2890bb3aee7e103fe53e4b0df, apart
> > from adding the comment.
> >
> > Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Cc: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  hw/char/pl011.c | 12 ++----------
> >  1 file changed, 2 insertions(+), 10 deletions(-)
> >
> > diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> > index 23a9db8c57c..efca8baecd7 100644
> > --- a/hw/char/pl011.c
> > +++ b/hw/char/pl011.c
> > @@ -85,7 +85,6 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr)
> >  #define CR_OUT1     (1 << 12)
> >  #define CR_RTS      (1 << 11)
> >  #define CR_DTR      (1 << 10)
> > -#define CR_RXE      (1 << 9)
> >  #define CR_TXE      (1 << 8)
> >  #define CR_LBE      (1 << 7)
> >  #define CR_UARTEN   (1 << 0)
> > @@ -490,16 +489,9 @@ static int pl011_can_receive(void *opaque)
> >      unsigned fifo_depth = pl011_get_fifo_depth(s);
> >      unsigned fifo_available = fifo_depth - s->read_count;
> >
> > -    if (!(s->cr & CR_UARTEN)) {
> > -        qemu_log_mask(LOG_GUEST_ERROR,
> > -                      "PL011 receiving data on disabled UART\n");
> > -    }
> > -    if (!(s->cr & CR_RXE)) {
> > -        qemu_log_mask(LOG_GUEST_ERROR,
> > -                      "PL011 receiving data on disabled RX UART\n");
> > -    }
> > +    /* Should check enable and return 0? */
>
> We decided deliberately not to check the enable and return 0
> here, as described in the commit message of abf2b6a028670bd:
> we think there's too likely to be existing works-on-QEMU code
> out there that doesn't ever set the enable bits.
>
> Otherwise, yes, agreed with the revert.

I've applied this to target-arm.next with the comment expanded
(and I left the define of CR_RXE in too):

+    /*
+     * In theory we should check the UART and RX enable bits here and
+     * return 0 if they are not set (so the guest can't receive data
+     * until you have enabled the UART). In practice we suspect there
+     * is at least some guest code out there which has been tested only
+     * on QEMU and which never bothers to enable the UART because we
+     * historically never enforced that. So we effectively keep the
+     * UART continuously enabled regardless of the enable bits.
+     */

thanks
-- PMM

Re: [PATCH] Revert "hw/char/pl011: Warn when using disabled receiver"

Posted by Peter Maydell 11 months ago

On Wed, 12 Mar 2025 at 13:36, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 11 Mar 2025 at 15:37, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> > The guest does not control whether characters are sent on the UART.
> > Sending them before the guest happens to boot will now result in a
> > "guest error" log entry that is only because of timing, even if the
> > guest _would_ later setup the receiver correctly.
> >
> > This reverts commit abf2b6a028670bd2890bb3aee7e103fe53e4b0df, apart
> > from adding the comment.
> >
> > Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Cc: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  hw/char/pl011.c | 12 ++----------
> >  1 file changed, 2 insertions(+), 10 deletions(-)
> >
> > diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> > index 23a9db8c57c..efca8baecd7 100644
> > --- a/hw/char/pl011.c
> > +++ b/hw/char/pl011.c
> > @@ -85,7 +85,6 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr)
> >  #define CR_OUT1     (1 << 12)
> >  #define CR_RTS      (1 << 11)
> >  #define CR_DTR      (1 << 10)
> > -#define CR_RXE      (1 << 9)
> >  #define CR_TXE      (1 << 8)
> >  #define CR_LBE      (1 << 7)
> >  #define CR_UARTEN   (1 << 0)
> > @@ -490,16 +489,9 @@ static int pl011_can_receive(void *opaque)
> >      unsigned fifo_depth = pl011_get_fifo_depth(s);
> >      unsigned fifo_available = fifo_depth - s->read_count;
> >
> > -    if (!(s->cr & CR_UARTEN)) {
> > -        qemu_log_mask(LOG_GUEST_ERROR,
> > -                      "PL011 receiving data on disabled UART\n");
> > -    }
> > -    if (!(s->cr & CR_RXE)) {
> > -        qemu_log_mask(LOG_GUEST_ERROR,
> > -                      "PL011 receiving data on disabled RX UART\n");
> > -    }
> > +    /* Should check enable and return 0? */
>
> We decided deliberately not to check the enable and return 0
> here, as described in the commit message of abf2b6a028670bd:
> we think there's too likely to be existing works-on-QEMU code
> out there that doesn't ever set the enable bits.
>
> Otherwise, yes, agreed with the revert.

Oh, and I just realized that the right place to diagnose
"guest didn't enable the UART" would be when it reads/writes
the data register while the enable bits are clear.

-- PMM

Re: [PATCH] Revert "hw/char/pl011: Warn when using disabled receiver"

Posted by Philippe Mathieu-Daudé 11 months ago

On 12/3/25 14:43, Peter Maydell wrote:
> On Wed, 12 Mar 2025 at 13:36, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Tue, 11 Mar 2025 at 15:37, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>
>>> The guest does not control whether characters are sent on the UART.
>>> Sending them before the guest happens to boot will now result in a
>>> "guest error" log entry that is only because of timing, even if the
>>> guest _would_ later setup the receiver correctly.
>>>
>>> This reverts commit abf2b6a028670bd2890bb3aee7e103fe53e4b0df, apart
>>> from adding the comment.
>>>
>>> Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> Cc: Peter Maydell <peter.maydell@linaro.org>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>   hw/char/pl011.c | 12 ++----------
>>>   1 file changed, 2 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
>>> index 23a9db8c57c..efca8baecd7 100644
>>> --- a/hw/char/pl011.c
>>> +++ b/hw/char/pl011.c
>>> @@ -85,7 +85,6 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr)
>>>   #define CR_OUT1     (1 << 12)
>>>   #define CR_RTS      (1 << 11)
>>>   #define CR_DTR      (1 << 10)
>>> -#define CR_RXE      (1 << 9)
>>>   #define CR_TXE      (1 << 8)
>>>   #define CR_LBE      (1 << 7)
>>>   #define CR_UARTEN   (1 << 0)
>>> @@ -490,16 +489,9 @@ static int pl011_can_receive(void *opaque)
>>>       unsigned fifo_depth = pl011_get_fifo_depth(s);
>>>       unsigned fifo_available = fifo_depth - s->read_count;
>>>
>>> -    if (!(s->cr & CR_UARTEN)) {
>>> -        qemu_log_mask(LOG_GUEST_ERROR,
>>> -                      "PL011 receiving data on disabled UART\n");
>>> -    }
>>> -    if (!(s->cr & CR_RXE)) {
>>> -        qemu_log_mask(LOG_GUEST_ERROR,
>>> -                      "PL011 receiving data on disabled RX UART\n");
>>> -    }
>>> +    /* Should check enable and return 0? */
>>
>> We decided deliberately not to check the enable and return 0
>> here, as described in the commit message of abf2b6a028670bd:
>> we think there's too likely to be existing works-on-QEMU code
>> out there that doesn't ever set the enable bits.
>>
>> Otherwise, yes, agreed with the revert.
> 
> Oh, and I just realized that the right place to diagnose
> "guest didn't enable the UART" would be when it reads/writes
> the data register while the enable bits are clear.

Doh, sorry. I wonder how I ended putting this code here... Since I
rebased this a lot, maybe something went wrong. Anyway, I'll post
a fix.

Regards,

Phil.