Re: [PATCH 3/3] hw/intc/loongarch_extioi: Use set_bit32() and clear_bit32() for s->isr

[maobibo]

On 2024/11/8 下午9:55, Peter Maydell wrote:
> In extioi_setirq() we try to operate on a bit array stored as an
> array of uint32_t using the set_bit() and clear_bit() functions
> by casting the pointer to 'unsigned long *'.
> This has two problems:
>   * the alignment of 'uint32_t' is less than that of 'unsigned long'
>     so we pass an insufficiently aligned pointer, which is
>     undefined behaviour
>   * on big-endian hosts the 64-bit 'unsigned long' will have
>     its two halves the wrong way around, and we will produce
>     incorrect results
>
> The undefined behaviour is shown by the clang undefined-behaviour
> sanitizer when running the loongarch64-virt functional test:
>
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/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 
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/include/qemu/bitops.h:41:9
>      #1 0x555556fb81c4 in extioi_setirq 
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/clang/../../hw/intc/loongarch_extioi.c:65:9
>      #2 0x555556fb6e90 in pch_pic_irq_handler 
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/clang/../../hw/intc/loongarch_pch_pic.c:75:5
>      #3 0x555556710265 in serial_ioport_write 
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/clang/../../hw/char/serial.c
>
> Fix these problems by using set_bit32() and clear_bit32(),
> which work with bit arrays stored as an array of uint32_t.
>
> Cc: qemu-stable@nongnu.org
> Fixes: cbff2db1e92f8759 ("hw/intc: Add LoongArch extioi interrupt 
> controller(EIOINTC)")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/intc/loongarch_extioi.c | 11 +++--------
>   1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/hw/intc/loongarch_extioi.c b/hw/intc/loongarch_extioi.c
> index 02dc4e6db3b..97d1af5ccc2 100644
> --- a/hw/intc/loongarch_extioi.c
> +++ b/hw/intc/loongarch_extioi.c
> @@ -57,14 +57,9 @@ static void extioi_setirq(void *opaque, int irq, int level)
>       LoongArchExtIOI *s = LOONGARCH_EXTIOI(opaque);
>       trace_loongarch_extioi_setirq(irq, level);
>       if (level) {
> -        /*
> -         * s->isr should be used in vmstate structure,
> -         * but it not support 'unsigned long',
> -         * so we have to switch it.
> -         */
> -        set_bit(irq, (unsigned long *)s->isr);
> +        set_bit32(irq, s->isr);
>       } else {
> -        clear_bit(irq, (unsigned long *)s->isr);
> +        clear_bit32(irq, s->isr);
>       }
>       extioi_update_irq(s, irq, level);
>   }
> @@ -154,7 +149,7 @@ static inline void extioi_update_sw_coremap(LoongArchExtIOI 
> *s, int irq,
>               continue;
>           }
>   
> -        if (notify && test_bit(irq + i, (unsigned long *)s->isr)) {
> +        if (notify && test_bit32(irq + i, s->isr)) {
>               /*
>                * lower irq at old cpu and raise irq at new cpu
>                */
Hi Peter,

Thanks for finding and fixing this issue, it looks good to me.

Reviewed-by: Bibo Mao <maobibo@loongson.cn>

Regards
Bibo Mao