| Message ID | 20250311153717.206129-1-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Revert "hw/char/pl011: Warn when using disabled receiver" | expand |
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
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
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
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.
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; }
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(-)