| Message ID | 20241007144732.2491331-1-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/char/pl011: Use correct masks for IBRD and FBRD | expand |
Peter Maydell <peter.maydell@linaro.org> writes: > In commit b88cfee90268cad we defined masks for the IBRD and FBRD > integer and fractional baud rate divider registers, to prevent the > guest from writing invalid values which could cause division-by-zero. > Unfortunately we got the mask values the wrong way around: the FBRD > register is six bits and the IBRD register is 16 bits, not > vice-versa. > > You would only run into this bug if you programmed the UART to a baud > rate of less than 9600, because for 9600 baud and above the IBRD > value will fit into 6 bits, as per the table in > https://developer.arm.com/documentation/ddi0183/g/programmers-model/register-descriptions/fractional-baud-rate-register--uartfbrd > > The only visible effects would be that the value read back from > the register by the guest would be truncated, and we would > print an incorrect baud rate in the debug logs. > > Cc: qemu-stable@nongnu.org > Fixes: b88cfee90268 ("hw/char/pl011: Avoid division-by-zero in pl011_get_baudrate()") > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2610 > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
On 7/10/24 11:47, Peter Maydell wrote: > In commit b88cfee90268cad we defined masks for the IBRD and FBRD > integer and fractional baud rate divider registers, to prevent the > guest from writing invalid values which could cause division-by-zero. > Unfortunately we got the mask values the wrong way around: the FBRD > register is six bits and the IBRD register is 16 bits, not > vice-versa. Oops indeed, I didn't check this closely enough. Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > You would only run into this bug if you programmed the UART to a baud > rate of less than 9600, because for 9600 baud and above the IBRD > value will fit into 6 bits, as per the table in > https://developer.arm.com/documentation/ddi0183/g/programmers-model/register-descriptions/fractional-baud-rate-register--uartfbrd > > The only visible effects would be that the value read back from > the register by the guest would be truncated, and we would > print an incorrect baud rate in the debug logs. > > Cc: qemu-stable@nongnu.org > Fixes: b88cfee90268 ("hw/char/pl011: Avoid division-by-zero in pl011_get_baudrate()") > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2610 > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/char/pl011.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-)
On 10/8/24 12:47 AM, Peter Maydell wrote: > In commit b88cfee90268cad we defined masks for the IBRD and FBRD > integer and fractional baud rate divider registers, to prevent the > guest from writing invalid values which could cause division-by-zero. > Unfortunately we got the mask values the wrong way around: the FBRD > register is six bits and the IBRD register is 16 bits, not > vice-versa. > > You would only run into this bug if you programmed the UART to a baud > rate of less than 9600, because for 9600 baud and above the IBRD > value will fit into 6 bits, as per the table in > https://developer.arm.com/documentation/ddi0183/g/programmers-model/register-descriptions/fractional-baud-rate-register--uartfbrd > > The only visible effects would be that the value read back from > the register by the guest would be truncated, and we would > print an incorrect baud rate in the debug logs. > > Cc: qemu-stable@nongnu.org > Fixes: b88cfee90268 ("hw/char/pl011: Avoid division-by-zero in pl011_get_baudrate()") > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2610 > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/char/pl011.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > Reviewed-by: Gavin Shan <gshan@redhat.com>
diff --git a/hw/char/pl011.c b/hw/char/pl011.c index 15df7c1e1ca..0fd1334fab4 100644 --- a/hw/char/pl011.c +++ b/hw/char/pl011.c @@ -90,10 +90,10 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr) #define CR_UARTEN (1 << 0) /* Integer Baud Rate Divider, UARTIBRD */ -#define IBRD_MASK 0x3f +#define IBRD_MASK 0xffff /* Fractional Baud Rate Divider, UARTFBRD */ -#define FBRD_MASK 0xffff +#define FBRD_MASK 0x3f static const unsigned char pl011_id_arm[8] = { 0x11, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1 };
In commit b88cfee90268cad we defined masks for the IBRD and FBRD integer and fractional baud rate divider registers, to prevent the guest from writing invalid values which could cause division-by-zero. Unfortunately we got the mask values the wrong way around: the FBRD register is six bits and the IBRD register is 16 bits, not vice-versa. You would only run into this bug if you programmed the UART to a baud rate of less than 9600, because for 9600 baud and above the IBRD value will fit into 6 bits, as per the table in https://developer.arm.com/documentation/ddi0183/g/programmers-model/register-descriptions/fractional-baud-rate-register--uartfbrd The only visible effects would be that the value read back from the register by the guest would be truncated, and we would print an incorrect baud rate in the debug logs. Cc: qemu-stable@nongnu.org Fixes: b88cfee90268 ("hw/char/pl011: Avoid division-by-zero in pl011_get_baudrate()") Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2610 Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/char/pl011.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)