[PATCH v1 0/3] Refactor PPI logic/definitions for virt/sbsa-ref

[PATCH v1 0/3] Refactor PPI logic/definitions for virt/sbsa-ref

[Leif Lindholm]

This set reworks the handling of private peripheral interrupts in virt
to use INTIDs instead of PPI IDs, to make it easier to cross reference
against Arm's Base System Architecture specification.

It then breaks those definitions out into a separate header and switches
sbsa-ref to use the same header instead of defining its own values
locally.

Changes since RFC:
- Compilation tested
- Reordered patches 1-2 as suggested by Philippe.

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

 hw/arm/sbsa-ref.c        | 23 +++++++++++------------
 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(+), 29 deletions(-)
 create mode 100644 include/hw/arm/bsa.h

-- 
2.30.2

[PATCH v1 1/3] {include/}hw/arm: refactor virt PPI logic

[Leif Lindholm]

GIC Private Peripheral Interrupts (PPI) are defined as GIC INTID 16-31.
As in, PPI0 is INTID16 .. PPI15 is INTID31.
Arm's Base System Architecture specification (BSA) lists the mandated and
recommended private interrupt IDs by INTID, not by PPI index. But current
definitions in virt define them by PPI index, complicating cross
referencing.

Meanwhile, the PPI(x) macro counterintuitively adds 16 to the input value,
converting a PPI index to an INTID.

Resolve this by redefining the BSA-allocated PPIs by their INTIDs,
inverting the logic of the PPI(x) macro and flipping where it is used.

Signed-off-by: Leif Lindholm <quic_llindhol@quicinc.com>
---
 hw/arm/virt-acpi-build.c |  4 ++--
 hw/arm/virt.c            |  9 +++++----
 include/hw/arm/virt.h    | 14 +++++++-------
 3 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 6b674231c2..963c58a88a 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -729,9 +729,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     for (i = 0; i < MACHINE(vms)->smp.cpus; i++) {
         ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i));
         uint64_t physical_base_address = 0, gich = 0, gicv = 0;
-        uint32_t vgic_interrupt = vms->virt ? PPI(ARCH_GIC_MAINT_IRQ) : 0;
+        uint32_t vgic_interrupt = vms->virt ? ARCH_GIC_MAINT_IRQ : 0;
         uint32_t pmu_interrupt = arm_feature(&armcpu->env, ARM_FEATURE_PMU) ?
-                                             PPI(VIRTUAL_PMU_IRQ) : 0;
+                                             VIRTUAL_PMU_IRQ : 0;
 
         if (vms->gic_version == VIRT_GIC_VERSION_2) {
             physical_base_address = memmap[VIRT_GIC_CPU].base;
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 8ad78b23c2..bb70f3eec8 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -815,23 +815,24 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem)
         for (irq = 0; irq < ARRAY_SIZE(timer_irq); irq++) {
             qdev_connect_gpio_out(cpudev, irq,
                                   qdev_get_gpio_in(vms->gic,
-                                                   ppibase + timer_irq[irq]));
+                                                   ppibase
+                                                   + PPI(timer_irq[irq])));
         }
 
         if (vms->gic_version != VIRT_GIC_VERSION_2) {
             qemu_irq irq = qdev_get_gpio_in(vms->gic,
-                                            ppibase + ARCH_GIC_MAINT_IRQ);
+                                            ppibase + PPI(ARCH_GIC_MAINT_IRQ));
             qdev_connect_gpio_out_named(cpudev, "gicv3-maintenance-interrupt",
                                         0, irq);
         } else if (vms->virt) {
             qemu_irq irq = qdev_get_gpio_in(vms->gic,
-                                            ppibase + ARCH_GIC_MAINT_IRQ);
+                                            ppibase + PPI(ARCH_GIC_MAINT_IRQ));
             sysbus_connect_irq(gicbusdev, i + 4 * smp_cpus, irq);
         }
 
         qdev_connect_gpio_out_named(cpudev, "pmu-interrupt", 0,
                                     qdev_get_gpio_in(vms->gic, ppibase
-                                                     + VIRTUAL_PMU_IRQ));
+                                                     + PPI(VIRTUAL_PMU_IRQ)));
 
         sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
         sysbus_connect_irq(gicbusdev, i + smp_cpus,
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index e1ddbea96b..8ba4e5b836 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -43,16 +43,16 @@
 #define NUM_VIRTIO_TRANSPORTS 32
 #define NUM_SMMU_IRQS          4
 
-#define ARCH_GIC_MAINT_IRQ  9
+#define ARCH_GIC_MAINT_IRQ  25
 
-#define ARCH_TIMER_VIRT_IRQ   11
-#define ARCH_TIMER_S_EL1_IRQ  13
-#define ARCH_TIMER_NS_EL1_IRQ 14
-#define ARCH_TIMER_NS_EL2_IRQ 10
+#define ARCH_TIMER_VIRT_IRQ   27
+#define ARCH_TIMER_S_EL1_IRQ  29
+#define ARCH_TIMER_NS_EL1_IRQ 30
+#define ARCH_TIMER_NS_EL2_IRQ 26
 
-#define VIRTUAL_PMU_IRQ 7
+#define VIRTUAL_PMU_IRQ 23
 
-#define PPI(irq) ((irq) + 16)
+#define PPI(irq) ((irq) - 16)
 
 /* See Linux kernel arch/arm64/include/asm/pvclock-abi.h */
 #define PVTIME_SIZE_PER_CPU 64
-- 
2.30.2

[PATCH v1 2/3] include/hw/arm: move BSA definitions to bsa.h

[Leif Lindholm]

virt.h defines a number of IRQs that are ultimately described by Arm's
Base System Architecture specification. Move these to a dedicated header
so that they can be reused by other platforms that do the same.
Include that header from virt.h to minimise churn.

Signed-off-by: Leif Lindholm <quic_llindhol@quicinc.com>
---
 include/hw/arm/bsa.h  | 35 +++++++++++++++++++++++++++++++++++
 include/hw/arm/virt.h | 12 +-----------
 2 files changed, 36 insertions(+), 11 deletions(-)
 create mode 100644 include/hw/arm/bsa.h

diff --git a/include/hw/arm/bsa.h b/include/hw/arm/bsa.h
new file mode 100644
index 0000000000..b7db1cacf1
--- /dev/null
+++ b/include/hw/arm/bsa.h
@@ -0,0 +1,35 @@
+/*
+ * Common definitions for Arm Base System Architecture (BSA) platforms.
+ *
+ * Copyright (c) 2015 Linaro Limited
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#ifndef QEMU_ARM_BSA_H
+#define QEMU_ARM_BSA_H
+
+#define ARCH_GIC_MAINT_IRQ  25
+
+#define ARCH_TIMER_VIRT_IRQ   27
+#define ARCH_TIMER_S_EL1_IRQ  29
+#define ARCH_TIMER_NS_EL1_IRQ 30
+#define ARCH_TIMER_NS_EL2_IRQ 26
+
+#define VIRTUAL_PMU_IRQ 23
+
+#define PPI(irq) ((irq) - 16)
+
+#endif /* QEMU_ARM_BSA_H */
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 8ba4e5b836..f69239850e 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -34,6 +34,7 @@
 #include "qemu/notify.h"
 #include "hw/boards.h"
 #include "hw/arm/boot.h"
+#include "hw/arm/bsa.h"
 #include "hw/block/flash.h"
 #include "sysemu/kvm.h"
 #include "hw/intc/arm_gicv3_common.h"
@@ -43,17 +44,6 @@
 #define NUM_VIRTIO_TRANSPORTS 32
 #define NUM_SMMU_IRQS          4
 
-#define ARCH_GIC_MAINT_IRQ  25
-
-#define ARCH_TIMER_VIRT_IRQ   27
-#define ARCH_TIMER_S_EL1_IRQ  29
-#define ARCH_TIMER_NS_EL1_IRQ 30
-#define ARCH_TIMER_NS_EL2_IRQ 26
-
-#define VIRTUAL_PMU_IRQ 23
-
-#define PPI(irq) ((irq) - 16)
-
 /* See Linux kernel arch/arm64/include/asm/pvclock-abi.h */
 #define PVTIME_SIZE_PER_CPU 64
 
-- 
2.30.2

[PATCH v1 3/3] hw/arm/sbsa-ref: use bsa.h for PPI definitions

[Leif Lindholm]

Use the private peripheral interrupt definitions from bsa.h instead of
defining them locally. Refactor to use PPI() to convert from INTID macro
where necessary.

Signed-off-by: Leif Lindholm <quic_llindhol@quicinc.com>
---
 hw/arm/sbsa-ref.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index bc89eb4806..3a4ea4dfdd 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -2,6 +2,7 @@
  * ARM SBSA Reference Platform emulation
  *
  * Copyright (c) 2018 Linaro Limited
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
  * Written by Hongbo Zhang <hongbo.zhang@linaro.org>
  *
  * This program is free software; you can redistribute it and/or modify it
@@ -30,6 +31,7 @@
 #include "exec/hwaddr.h"
 #include "kvm_arm.h"
 #include "hw/arm/boot.h"
+#include "hw/arm/bsa.h"
 #include "hw/arm/fdt.h"
 #include "hw/arm/smmuv3.h"
 #include "hw/block/flash.h"
@@ -55,13 +57,6 @@
 #define NUM_SMMU_IRQS   4
 #define NUM_SATA_PORTS  6
 
-#define VIRTUAL_PMU_IRQ        7
-#define ARCH_GIC_MAINT_IRQ     9
-#define ARCH_TIMER_VIRT_IRQ    11
-#define ARCH_TIMER_S_EL1_IRQ   13
-#define ARCH_TIMER_NS_EL1_IRQ  14
-#define ARCH_TIMER_NS_EL2_IRQ  10
-
 enum {
     SBSA_FLASH,
     SBSA_MEM,
@@ -494,15 +489,19 @@ static void create_gic(SBSAMachineState *sms, MemoryRegion *mem)
         for (irq = 0; irq < ARRAY_SIZE(timer_irq); irq++) {
             qdev_connect_gpio_out(cpudev, irq,
                                   qdev_get_gpio_in(sms->gic,
-                                                   ppibase + timer_irq[irq]));
+                                                   ppibase
+                                                   + PPI(timer_irq[irq])));
         }
 
         qdev_connect_gpio_out_named(cpudev, "gicv3-maintenance-interrupt", 0,
-                                    qdev_get_gpio_in(sms->gic, ppibase
-                                                     + ARCH_GIC_MAINT_IRQ));
+                                    qdev_get_gpio_in(sms->gic,
+                                                     ppibase
+                                                     + PPI(ARCH_GIC_MAINT_IRQ)));
+
         qdev_connect_gpio_out_named(cpudev, "pmu-interrupt", 0,
-                                    qdev_get_gpio_in(sms->gic, ppibase
-                                                     + VIRTUAL_PMU_IRQ));
+                                    qdev_get_gpio_in(sms->gic,
+                                                     ppibase
+                                                     + PPI(VIRTUAL_PMU_IRQ)));
 
         sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
         sysbus_connect_irq(gicbusdev, i + smp_cpus,
-- 
2.30.2

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

[Marcin Juszkiewicz]

W dniu 15.09.2023 o 13:55, Leif Lindholm pisze:
> This set reworks the handling of private peripheral interrupts in virt
> to use INTIDs instead of PPI IDs, to make it easier to cross reference
> against Arm's Base System Architecture specification.
> 
> It then breaks those definitions out into a separate header and switches
> sbsa-ref to use the same header instead of defining its own values
> locally.
> 
> Changes since RFC:
> - Compilation tested
> - Reordered patches 1-2 as suggested by Philippe.
> 
> Leif Lindholm (3):
>    {include/}hw/arm: refactor virt PPI logic
>    include/hw/arm: move BSA definitions to bsa.h
>    hw/arm/sbsa-ref: use bsa.h for PPI definitions
> 
>   hw/arm/sbsa-ref.c        | 23 +++++++++++------------
>   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(+), 29 deletions(-)
>   create mode 100644 include/hw/arm/bsa.h
> 

Tested-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
Reviewed-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>

Re: [PATCH v1 1/3] {include/}hw/arm: refactor virt PPI logic

[Peter Maydell]

On Fri, 15 Sept 2023 at 12:56, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> GIC Private Peripheral Interrupts (PPI) are defined as GIC INTID 16-31.
> As in, PPI0 is INTID16 .. PPI15 is INTID31.
> Arm's Base System Architecture specification (BSA) lists the mandated and
> recommended private interrupt IDs by INTID, not by PPI index. But current
> definitions in virt define them by PPI index, complicating cross
> referencing.
>
> Meanwhile, the PPI(x) macro counterintuitively adds 16 to the input value,
> converting a PPI index to an INTID.
>
> Resolve this by redefining the BSA-allocated PPIs by their INTIDs,
> inverting the logic of the PPI(x) macro and flipping where it is used.

This patch changes the values of ARCH_TIMER_S_EL1_IRQ etc, but doesn't
change the code that writes those values into device tree properties, eg
in fdt_add_timer_nodes(). The device tree bindings want the actual PPI
numbers, not the INTIDs.

Plus keeping the PPI macro name but changing its function is
a bit fragile if we end up needing to backport a patch from after
this change to a QEMU release branch that predates it. If we want
to have the base values be INTIDs then probably the macro should
be INTID_TO_PPI() or similar.

thanks
-- PMM

Re: [PATCH v1 3/3] hw/arm/sbsa-ref: use bsa.h for PPI definitions

[Peter Maydell]

On Fri, 15 Sept 2023 at 12:55, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> Use the private peripheral interrupt definitions from bsa.h instead of
> defining them locally. Refactor to use PPI() to convert from INTID macro
> where necessary.
>
> Signed-off-by: Leif Lindholm <quic_llindhol@quicinc.com>
> ---
>  hw/arm/sbsa-ref.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index bc89eb4806..3a4ea4dfdd 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -2,6 +2,7 @@
>   * ARM SBSA Reference Platform emulation
>   *
>   * Copyright (c) 2018 Linaro Limited
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
>   * Written by Hongbo Zhang <hongbo.zhang@linaro.org>
>   *
>   * This program is free software; you can redistribute it and/or modify it
> @@ -30,6 +31,7 @@
>  #include "exec/hwaddr.h"
>  #include "kvm_arm.h"
>  #include "hw/arm/boot.h"
> +#include "hw/arm/bsa.h"
>  #include "hw/arm/fdt.h"
>  #include "hw/arm/smmuv3.h"
>  #include "hw/block/flash.h"
> @@ -55,13 +57,6 @@
>  #define NUM_SMMU_IRQS   4
>  #define NUM_SATA_PORTS  6
>
> -#define VIRTUAL_PMU_IRQ        7
> -#define ARCH_GIC_MAINT_IRQ     9
> -#define ARCH_TIMER_VIRT_IRQ    11
> -#define ARCH_TIMER_S_EL1_IRQ   13
> -#define ARCH_TIMER_NS_EL1_IRQ  14
> -#define ARCH_TIMER_NS_EL2_IRQ  10
> -
>  enum {
>      SBSA_FLASH,
>      SBSA_MEM,
> @@ -494,15 +489,19 @@ static void create_gic(SBSAMachineState *sms, MemoryRegion *mem)
>          for (irq = 0; irq < ARRAY_SIZE(timer_irq); irq++) {
>              qdev_connect_gpio_out(cpudev, irq,
>                                    qdev_get_gpio_in(sms->gic,
> -                                                   ppibase + timer_irq[irq]));
> +                                                   ppibase
> +                                                   + PPI(timer_irq[irq])));
>          }
>
>          qdev_connect_gpio_out_named(cpudev, "gicv3-maintenance-interrupt", 0,
> -                                    qdev_get_gpio_in(sms->gic, ppibase
> -                                                     + ARCH_GIC_MAINT_IRQ));
> +                                    qdev_get_gpio_in(sms->gic,
> +                                                     ppibase
> +                                                     + PPI(ARCH_GIC_MAINT_IRQ)));
> +
>          qdev_connect_gpio_out_named(cpudev, "pmu-interrupt", 0,
> -                                    qdev_get_gpio_in(sms->gic, ppibase
> -                                                     + VIRTUAL_PMU_IRQ));
> +                                    qdev_get_gpio_in(sms->gic,
> +                                                     ppibase
> +                                                     + PPI(VIRTUAL_PMU_IRQ)));
>
>          sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
>          sysbus_connect_irq(gicbusdev, i + smp_cpus,

You could also change the definition of ppibase not to add GIC_NR_SGIS
(perhaps renaming it) and then you wouldn't need to use the PPI() macro here...

-- PMM

Re: [PATCH v1 2/3] include/hw/arm: move BSA definitions to bsa.h

[Peter Maydell]

On Fri, 15 Sept 2023 at 12:55, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> virt.h defines a number of IRQs that are ultimately described by Arm's
> Base System Architecture specification. Move these to a dedicated header
> so that they can be reused by other platforms that do the same.
> Include that header from virt.h to minimise churn.
>
> Signed-off-by: Leif Lindholm <quic_llindhol@quicinc.com>
> ---
>  include/hw/arm/bsa.h  | 35 +++++++++++++++++++++++++++++++++++
>  include/hw/arm/virt.h | 12 +-----------
>  2 files changed, 36 insertions(+), 11 deletions(-)
>  create mode 100644 include/hw/arm/bsa.h
>
> diff --git a/include/hw/arm/bsa.h b/include/hw/arm/bsa.h
> new file mode 100644
> index 0000000000..b7db1cacf1
> --- /dev/null
> +++ b/include/hw/arm/bsa.h
> @@ -0,0 +1,35 @@
> +/*
> + * Common definitions for Arm Base System Architecture (BSA) platforms.
> + *
> + * Copyright (c) 2015 Linaro Limited
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2 or later, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#ifndef QEMU_ARM_BSA_H
> +#define QEMU_ARM_BSA_H
> +
> +#define ARCH_GIC_MAINT_IRQ  25

Given the confusion over indexing that seems to be endemic in the
GIC world, a comment
 /* These are architectural INTID values */

might help.

> +
> +#define ARCH_TIMER_VIRT_IRQ   27
> +#define ARCH_TIMER_S_EL1_IRQ  29
> +#define ARCH_TIMER_NS_EL1_IRQ 30
> +#define ARCH_TIMER_NS_EL2_IRQ 26
> +
> +#define VIRTUAL_PMU_IRQ 23
> +
> +#define PPI(irq) ((irq) - 16)
> +
> +#endif /* QEMU_ARM_BSA_H */

thanks
-- PMM