| Message ID | 20241108135514.4006953-1-peter.maydell@linaro.org |
|---|---|
| Headers | show |
| Series | hw/intc/loongarch_extioi: Fix undefined behaviour with bit array APIs | expand |
Any chance of a review on patches 1 and 2 here? (I guess I should have cc'd qemu-arm on this, since patch 2 is for GICv3.) thanks -- PMM On Fri, 8 Nov 2024 at 13:55, Peter Maydell <peter.maydell@linaro.org> wrote: > > The primary aim of this series is to fix some undefined behaviour in > loongarch_extioi which you can see if you run the functional test > loongarch64-virt with a QEMU built with the clang undefined-behaviour > sanitizer: > > include/qemu/bitops.h:41:5: runtime error: store to misaligned address 0x555559745d9c for type 'unsigned long', which requires 8 byte alignment > 0x555559745d9c: note: pointer points here > ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ^ > #0 0x555556fb81c4 in set_bit include/qemu/bitops.h:41:9 > #1 0x555556fb81c4 in extioi_setirq hw/intc/loongarch_extioi.c:65:9 > #2 0x555556fb6e90 in pch_pic_irq_handler hw/intc/loongarch_pch_pic.c:75:5 > #3 0x555556710265 in serial_ioport_write hw/char/serial.c > > The underlying cause of this is a mismatch between our bit array APIs > in bitops.h and what QEMU devices tend to want. The bit array APIs are > historically based on those from the Linux kernel; they work with > underlying storage that is an array of 'unsigned long'. This is fine > for the kernel, but awkward for QEMU devices because the 'unsigned > long' type varies in size between hosts. That means that you can't use > it for a data structure that needs to be migrated between devices and > it's awkward for devices where the bit array is visible to the guest > (e.g. via a set of registers). > > In the Arm GICv3 device I worked around this mismatch by implementing > a set of local functions which were like the bitops.h APIs but used a > uint32_t array. The loongarch_extioi code attempts to use the stock > bitops.h functions by casting the uint32_t* to an unsigned long* when > calling them. This doesn't work for two reasons: > * the alignment of uint32_t is less than that of unsigned long, > so the pointer isn't guaranteed to be sufficiently aligned; > this is the cause of the sanitizer UB error > * on a big-endian host we will get the wrong results because the > two halves of the unsigned long will be the opposite way round > > In this series I fix this by creating new functions set_bit32(), > clear_bit32(), etc in bitops.h which are like the existing ones but > work with a bit array whose underlying storage is a uint32_t array. > Then we can use these both in the GICv3 (where this is just a > cleanup) and in loongarch_extioi (where it fixes the bug). > > (There are other uses of set_bit() in the loongarch_extioi code but > I have left those alone because they define the bitmaps as > arrays of unsigned long so they are at least consistent. I do > wonder if it's really OK not to migrate those bitmaps, though.) > > thanks > -- PMM > > Peter Maydell (3): > bitops.h: Define bit operations on 'uint32_t' arrays > hw/intc/arm_gicv3: Use bitops.h uint32_t bit array functions > hw/intc/loongarch_extioi: Use set_bit32() and clear_bit32() for s->isr > > include/hw/intc/arm_gicv3_common.h | 54 +++------ > include/qemu/bitmap.h | 8 ++ > include/qemu/bitops.h | 172 ++++++++++++++++++++++++++++- > hw/intc/loongarch_extioi.c | 11 +- > 4 files changed, 194 insertions(+), 51 deletions(-)
On 8/11/24 14:55, Peter Maydell wrote: > Peter Maydell (3): > bitops.h: Define bit operations on 'uint32_t' arrays > hw/intc/arm_gicv3: Use bitops.h uint32_t bit array functions > hw/intc/loongarch_extioi: Use set_bit32() and clear_bit32() for s->isr Series: Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
The primary aim of this series is to fix some undefined behaviour in loongarch_extioi which you can see if you run the functional test loongarch64-virt with a QEMU built with the clang undefined-behaviour sanitizer: include/qemu/bitops.h:41:5: runtime error: store to misaligned address 0x555559745d9c for type 'unsigned long', which requires 8 byte alignment 0x555559745d9c: note: pointer points here ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ^ #0 0x555556fb81c4 in set_bit include/qemu/bitops.h:41:9 #1 0x555556fb81c4 in extioi_setirq hw/intc/loongarch_extioi.c:65:9 #2 0x555556fb6e90 in pch_pic_irq_handler hw/intc/loongarch_pch_pic.c:75:5 #3 0x555556710265 in serial_ioport_write hw/char/serial.c The underlying cause of this is a mismatch between our bit array APIs in bitops.h and what QEMU devices tend to want. The bit array APIs are historically based on those from the Linux kernel; they work with underlying storage that is an array of 'unsigned long'. This is fine for the kernel, but awkward for QEMU devices because the 'unsigned long' type varies in size between hosts. That means that you can't use it for a data structure that needs to be migrated between devices and it's awkward for devices where the bit array is visible to the guest (e.g. via a set of registers). In the Arm GICv3 device I worked around this mismatch by implementing a set of local functions which were like the bitops.h APIs but used a uint32_t array. The loongarch_extioi code attempts to use the stock bitops.h functions by casting the uint32_t* to an unsigned long* when calling them. This doesn't work for two reasons: * the alignment of uint32_t is less than that of unsigned long, so the pointer isn't guaranteed to be sufficiently aligned; this is the cause of the sanitizer UB error * on a big-endian host we will get the wrong results because the two halves of the unsigned long will be the opposite way round In this series I fix this by creating new functions set_bit32(), clear_bit32(), etc in bitops.h which are like the existing ones but work with a bit array whose underlying storage is a uint32_t array. Then we can use these both in the GICv3 (where this is just a cleanup) and in loongarch_extioi (where it fixes the bug). (There are other uses of set_bit() in the loongarch_extioi code but I have left those alone because they define the bitmaps as arrays of unsigned long so they are at least consistent. I do wonder if it's really OK not to migrate those bitmaps, though.) thanks -- PMM Peter Maydell (3): bitops.h: Define bit operations on 'uint32_t' arrays hw/intc/arm_gicv3: Use bitops.h uint32_t bit array functions hw/intc/loongarch_extioi: Use set_bit32() and clear_bit32() for s->isr include/hw/intc/arm_gicv3_common.h | 54 +++------ include/qemu/bitmap.h | 8 ++ include/qemu/bitops.h | 172 ++++++++++++++++++++++++++++- hw/intc/loongarch_extioi.c | 11 +- 4 files changed, 194 insertions(+), 51 deletions(-)