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.

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

Leif Lindholm (3):
  include/hw/arm: move BSA definitions to bsa.h
  {include/}hw/arm: refactor BSA/virt PPI logic
  hw/arm/sbsa-ref: use bsa.h for PPI definitions

 hw/arm/sbsa-ref.c        | 24 +++++++++++-------------
 hw/arm/virt-acpi-build.c |  4 ++--
 hw/arm/virt.c            |  9 +++++----
 include/hw/arm/bsa.h     | 35 +++++++++++++++++++++++++++++++++++
 include/hw/arm/virt.h    | 12 +-----------
 5 files changed, 54 insertions(+), 30 deletions(-)
 create mode 100644 include/hw/arm/bsa.h

-- 
2.30.2

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);
>        |                                  ~~~~~~~~~^~~~~~~~~
> --------------------------------------------