Re: [RFC PATCH 0/3] Refactor PPI logic/definitions for virt/sbsa-ref

[Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>]

W dniu 14.09.2023 o 14:01, Leif Lindholm pisze:
> While reviewing Marcin's patch this morning, cross referencing different
> specifications and looking at various places around the source code in
> order to convinced myself he really hadn't missed something out (the
> existing plumbing made it *so* clean to add), my brain broke slightly
> at keeping track of PPIs/INTIDs between the various sources.
> 
> Moreover, I found the PPI() macro in virt.h to be doing the exact
> opposite of what I would have expected it to (it converts a PPI to an
> INTID rather than the other way around).
> 
> So I refactored stuff so that:
> - PPIs defined by BSA are moved to a (new) common header.
> - The _IRQ definitions for those PPIs refer to the INTIDs.
> - sbsa-ref and virt both use these definitions.
> 
> This change does objectively add a bit more noise to the code, since it
> means more locations need to use the PPI macro than before, but it felt
> like a readability improvement to me.

I like how code looks after those changes. No more adding 16 to irq
numbers to fit them into BSA spec numbers is nice to have.

Will rebase my "non-secure EL2 virtual timer" patch on top of it.

> Not even compilation tested, just the least confusing way of asking
> whether the change could be accepted at all.

There are build warnings and final binary segfaults on start.

--------------------------------------------
[1799/2931] Compiling C object libqemu-aarch64-softmmu.fa.p/hw_arm_sbsa-ref.c.o
../hw/arm/sbsa-ref.c: In function ‘create_gic’:
../hw/arm/sbsa-ref.c:496:13: warning: assignment to ‘int’ from ‘qemu_irq’ {aka ‘struct IRQState *’} makes integer from pointer without a cast [-Wint-conversion]
   496 |         irq = qdev_get_gpio_in(sms->gic,
       |             ^
../hw/arm/sbsa-ref.c:499:37: warning: passing argument 4 of ‘qdev_connect_gpio_out_named’ makes pointer from integer without a cast [-Wint-conversion]
   499 |                                     irq);
       |                                     ^~~
       |                                     |
       |                                     int
In file included from /home/marcin/devel/linaro/sbsa-qemu/code/qemu/include/hw/core/cpu.h:23,
                  from ../target/arm/cpu-qom.h:23,
                  from ../target/arm/cpu.h:26,
                  from /home/marcin/devel/linaro/sbsa-qemu/code/qemu/include/sysemu/kvm.h:244,
                  from ../hw/arm/sbsa-ref.c:27:
/home/marcin/devel/linaro/sbsa-qemu/code/qemu/include/hw/qdev-core.h:699:43: note: expected ‘qemu_irq’ {aka ‘struct IRQState *’} but argument is of type ‘int’
   699 |                                  qemu_irq input_pin);
       |                                  ~~~~~~~~~^~~~~~~~~
../hw/arm/sbsa-ref.c:501:13: warning: assignment to ‘int’ from ‘qemu_irq’ {aka ‘struct IRQState *’} makes integer from pointer without a cast [-Wint-conversion]
   501 |         irq = qdev_get_gpio_in(sms->gic,
       |             ^
../hw/arm/sbsa-ref.c:503:65: warning: passing argument 4 of ‘qdev_connect_gpio_out_named’ makes pointer from integer without a cast [-Wint-conversion]
   503 |         qdev_connect_gpio_out_named(cpudev, "pmu-interrupt", 0, irq);
       |                                                                 ^~~
       |                                                                 |
       |                                                                 int
/home/marcin/devel/linaro/sbsa-qemu/code/qemu/include/hw/qdev-core.h:699:43: note: expected ‘qemu_irq’ {aka ‘struct IRQState *’} but argument is of type ‘int’
   699 |                                  qemu_irq input_pin);
       |                                  ~~~~~~~~~^~~~~~~~~
--------------------------------------------