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

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

Posted by Paolo Bonzini 3 weeks, 3 days 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 3 weeks, 2 days 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 3 weeks, 2 days 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 3 weeks, 2 days 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é 3 weeks, 2 days 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.