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

-- 
2.34.1

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>

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