Re: [PATCH 0/3] hw/intc/loongarch_extioi: Fix undefined behaviour with bit array APIs

[Peter Maydell]

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(-)