| Message ID | 20230424152833.1334136-1-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/intc/allwinner-a10-pic: Don't use set_bit()/clear_bit() | expand |
On 24/04/2023 17.28, Peter Maydell wrote: > The Allwinner PIC model uses set_bit() and clear_bit() to update the > values in its irq_pending[] array when an interrupt arrives. However > it is using these functions wrongly: they work on an array of type > 'long', and it is passing an array of type 'uint32_t'. Because the > code manually figures out the right array element, this works on > little-endian hosts and on 32-bit big-endian hosts, where bits 0..31 > in a 'long' are in the same place as they are in a 'uint32_t'. > However it breaks on 64-bit big-endian hosts. > > Remove the use of set_bit() and clear_bit() in favour of using > deposit32() on the array element. This fixes a bug where on > big-endian 64-bit hosts the guest kernel would hang early on in > bootup. > > Cc: qemu-stable@nongnu.org > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/intc/allwinner-a10-pic.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c > index 8cca1248073..4875e68ba6a 100644 > --- a/hw/intc/allwinner-a10-pic.c > +++ b/hw/intc/allwinner-a10-pic.c > @@ -49,12 +49,9 @@ static void aw_a10_pic_update(AwA10PICState *s) > static void aw_a10_pic_set_irq(void *opaque, int irq, int level) > { > AwA10PICState *s = opaque; > + uint32_t *pending_reg = &s->irq_pending[irq / 32]; > > - if (level) { > - set_bit(irq % 32, (void *)&s->irq_pending[irq / 32]); > - } else { > - clear_bit(irq % 32, (void *)&s->irq_pending[irq / 32]); > - } > + *pending_reg = deposit32(*pending_reg, irq % 32, 1, level); > aw_a10_pic_update(s); > } > Reviewed-by: Thomas Huth <thuth@redhat.com>
On 24/4/23 17:28, Peter Maydell wrote: > The Allwinner PIC model uses set_bit() and clear_bit() to update the > values in its irq_pending[] array when an interrupt arrives. However > it is using these functions wrongly: they work on an array of type > 'long', and it is passing an array of type 'uint32_t'. Because the > code manually figures out the right array element, this works on > little-endian hosts and on 32-bit big-endian hosts, where bits 0..31 > in a 'long' are in the same place as they are in a 'uint32_t'. > However it breaks on 64-bit big-endian hosts. > > Remove the use of set_bit() and clear_bit() in favour of using > deposit32() on the array element. This fixes a bug where on > big-endian 64-bit hosts the guest kernel would hang early on in > bootup. > > Cc: qemu-stable@nongnu.org > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/intc/allwinner-a10-pic.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On Mon, 24 Apr 2023 at 16:28, Peter Maydell <peter.maydell@linaro.org> wrote: > > The Allwinner PIC model uses set_bit() and clear_bit() to update the > values in its irq_pending[] array when an interrupt arrives. However > it is using these functions wrongly: they work on an array of type > 'long', and it is passing an array of type 'uint32_t'. Because the > code manually figures out the right array element, this works on > little-endian hosts and on 32-bit big-endian hosts, where bits 0..31 > in a 'long' are in the same place as they are in a 'uint32_t'. > However it breaks on 64-bit big-endian hosts. > > Remove the use of set_bit() and clear_bit() in favour of using > deposit32() on the array element. This fixes a bug where on > big-endian 64-bit hosts the guest kernel would hang early on in > bootup. > > Cc: qemu-stable@nongnu.org > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Applied to target-arm.next, thanks. -- PMM
diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c index 8cca1248073..4875e68ba6a 100644 --- a/hw/intc/allwinner-a10-pic.c +++ b/hw/intc/allwinner-a10-pic.c @@ -49,12 +49,9 @@ static void aw_a10_pic_update(AwA10PICState *s) static void aw_a10_pic_set_irq(void *opaque, int irq, int level) { AwA10PICState *s = opaque; + uint32_t *pending_reg = &s->irq_pending[irq / 32]; - if (level) { - set_bit(irq % 32, (void *)&s->irq_pending[irq / 32]); - } else { - clear_bit(irq % 32, (void *)&s->irq_pending[irq / 32]); - } + *pending_reg = deposit32(*pending_reg, irq % 32, 1, level); aw_a10_pic_update(s); }
The Allwinner PIC model uses set_bit() and clear_bit() to update the values in its irq_pending[] array when an interrupt arrives. However it is using these functions wrongly: they work on an array of type 'long', and it is passing an array of type 'uint32_t'. Because the code manually figures out the right array element, this works on little-endian hosts and on 32-bit big-endian hosts, where bits 0..31 in a 'long' are in the same place as they are in a 'uint32_t'. However it breaks on 64-bit big-endian hosts. Remove the use of set_bit() and clear_bit() in favour of using deposit32() on the array element. This fixes a bug where on big-endian 64-bit hosts the guest kernel would hang early on in bootup. Cc: qemu-stable@nongnu.org Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/intc/allwinner-a10-pic.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)