Message ID | 20230914120124.55410-1-quic_llindhol@quicinc.com |
---|---|
Headers | show |
Series | Refactor PPI logic/definitions for virt/sbsa-ref | expand |
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); | ~~~~~~~~~^~~~~~~~~ --------------------------------------------
On 2023-09-14 14:15, Marcin Juszkiewicz wrote: > 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. Ah, yes. In my rush, I failed to spot that the -virt gic_create function contained shadows of different type variables called irq. Will address if there's a v1. Thanks! / Leif > -------------------------------------------- > [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); > | ~~~~~~~~~^~~~~~~~~ > --------------------------------------------