[PATCH] arm/virt: Extract common code to wire GICC<->vCPU IRQs for reuse

[PATCH] arm/virt: Extract common code to wire GICC<->vCPU IRQs for reuse

[Salil Mehta via]

Extract common GIC and CPU interrupt wiring code to improve code
readability and modularity, supporting reuse in future patch sets. This
refactor is benign and introduces *no* functional changes.

Note: This patch has been isolated from a larger patch set to facilitate
early merging and reduce the complexity of the original set, as it
operates independently. All original tags and author contributions are
retained.

[!] Please note, this is a purely cosmetic change. No functional change.

Reported-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
[4/05/2024: Issue with total number of PPI available during create GIC]
Suggested-by: Miguel Luis <miguel.luis@oracle.com>
[5/05/2024: Fix the total number of PPIs available as per ARM BSA to avoid overflow]
Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
---
 hw/arm/virt.c | 108 ++++++++++++++++++++++++++++----------------------
 1 file changed, 60 insertions(+), 48 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index a0d3bef875..d6892b0266 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -761,6 +761,65 @@ static bool gicv3_nmi_present(VirtMachineState *vms)
            (vms->gic_version != VIRT_GIC_VERSION_2);
 }
 
+/*
+ * Mapping from the output timer irq lines from the CPU to the GIC PPI inputs
+ * we use for the virt board.
+ */
+const int timer_irq[] = {
+    [GTIMER_PHYS] = ARCH_TIMER_NS_EL1_IRQ,
+    [GTIMER_VIRT] = ARCH_TIMER_VIRT_IRQ,
+    [GTIMER_HYP]  = ARCH_TIMER_NS_EL2_IRQ,
+    [GTIMER_SEC]  = ARCH_TIMER_S_EL1_IRQ,
+    [GTIMER_HYPVIRT] = ARCH_TIMER_NS_EL2_VIRT_IRQ,
+};
+
+static void wire_gic_cpu_irqs(VirtMachineState *vms, CPUState *cs)
+{
+    SysBusDevice *gicbusdev = SYS_BUS_DEVICE(vms->gic);
+    unsigned int smp_cpus = MACHINE(vms)->smp.cpus;
+    DeviceState *cpudev = DEVICE(cs);
+    int i = CPU(cs)->cpu_index;
+    int intidbase, irqn;
+
+    intidbase = NUM_IRQS + i * GIC_INTERNAL;
+
+    for (irqn = 0; irqn < ARRAY_SIZE(timer_irq); irqn++) {
+        qdev_connect_gpio_out(cpudev, irqn,
+                              qdev_get_gpio_in(vms->gic,
+                                               intidbase + timer_irq[irqn]));
+    }
+
+
+    if (vms->gic_version != VIRT_GIC_VERSION_2) {
+        qemu_irq irq = qdev_get_gpio_in(vms->gic,
+                                        intidbase + 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,
+                                        intidbase + 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, intidbase
+                                                  + VIRTUAL_PMU_IRQ));
+
+    sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
+    sysbus_connect_irq(gicbusdev, i + smp_cpus,
+                       qdev_get_gpio_in(cpudev, ARM_CPU_FIQ));
+    sysbus_connect_irq(gicbusdev, i + 2 * smp_cpus,
+                       qdev_get_gpio_in(cpudev, ARM_CPU_VIRQ));
+    sysbus_connect_irq(gicbusdev, i + 3 * smp_cpus,
+                       qdev_get_gpio_in(cpudev, ARM_CPU_VFIQ));
+    if (vms->gic_version != VIRT_GIC_VERSION_2) {
+        sysbus_connect_irq(gicbusdev, i + 4 * smp_cpus,
+                           qdev_get_gpio_in(cpudev, ARM_CPU_NMI));
+        sysbus_connect_irq(gicbusdev, i + 5 * smp_cpus,
+                           qdev_get_gpio_in(cpudev, ARM_CPU_VINMI));
+    }
+}
+
 static void create_gic(VirtMachineState *vms, MemoryRegion *mem)
 {
     MachineState *ms = MACHINE(vms);
@@ -862,54 +921,7 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem)
      * CPU's inputs.
      */
     for (i = 0; i < smp_cpus; i++) {
-        DeviceState *cpudev = DEVICE(qemu_get_cpu(i));
-        int intidbase = NUM_IRQS + i * GIC_INTERNAL;
-        /* Mapping from the output timer irq lines from the CPU to the
-         * GIC PPI inputs we use for the virt board.
-         */
-        const int timer_irq[] = {
-            [GTIMER_PHYS] = ARCH_TIMER_NS_EL1_IRQ,
-            [GTIMER_VIRT] = ARCH_TIMER_VIRT_IRQ,
-            [GTIMER_HYP]  = ARCH_TIMER_NS_EL2_IRQ,
-            [GTIMER_SEC]  = ARCH_TIMER_S_EL1_IRQ,
-            [GTIMER_HYPVIRT] = ARCH_TIMER_NS_EL2_VIRT_IRQ,
-        };
-
-        for (unsigned irq = 0; irq < ARRAY_SIZE(timer_irq); irq++) {
-            qdev_connect_gpio_out(cpudev, irq,
-                                  qdev_get_gpio_in(vms->gic,
-                                                   intidbase + timer_irq[irq]));
-        }
-
-        if (vms->gic_version != VIRT_GIC_VERSION_2) {
-            qemu_irq irq = qdev_get_gpio_in(vms->gic,
-                                            intidbase + 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,
-                                            intidbase + 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, intidbase
-                                                     + VIRTUAL_PMU_IRQ));
-
-        sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
-        sysbus_connect_irq(gicbusdev, i + smp_cpus,
-                           qdev_get_gpio_in(cpudev, ARM_CPU_FIQ));
-        sysbus_connect_irq(gicbusdev, i + 2 * smp_cpus,
-                           qdev_get_gpio_in(cpudev, ARM_CPU_VIRQ));
-        sysbus_connect_irq(gicbusdev, i + 3 * smp_cpus,
-                           qdev_get_gpio_in(cpudev, ARM_CPU_VFIQ));
-
-        if (vms->gic_version != VIRT_GIC_VERSION_2) {
-            sysbus_connect_irq(gicbusdev, i + 4 * smp_cpus,
-                               qdev_get_gpio_in(cpudev, ARM_CPU_NMI));
-            sysbus_connect_irq(gicbusdev, i + 5 * smp_cpus,
-                               qdev_get_gpio_in(cpudev, ARM_CPU_VINMI));
-        }
+        wire_gic_cpu_irqs(vms, qemu_get_cpu(i));
     }
 
     fdt_add_gic_node(vms);
-- 
2.34.1

Re: [PATCH] arm/virt: Extract common code to wire GICC<->vCPU IRQs for reuse

[Peter Maydell]

On Sun, 3 Nov 2024 at 15:25, Salil Mehta <salil.mehta@huawei.com> wrote:
>
> Extract common GIC and CPU interrupt wiring code to improve code
> readability and modularity, supporting reuse in future patch sets. This
> refactor is benign and introduces *no* functional changes.
>
> Note: This patch has been isolated from a larger patch set to facilitate
> early merging and reduce the complexity of the original set, as it
> operates independently. All original tags and author contributions are
> retained.

I would prefer to see refactoring patches in the context of
the series that makes them necessary. As it stands, there
doesn't really seem to be much benefit to this change.

thanks
-- PMM

Re: [PATCH] arm/virt: Extract common code to wire GICC<->vCPU IRQs for reuse

[Gavin Shan]

On 11/4/24 11:26 PM, Peter Maydell wrote:
> On Sun, 3 Nov 2024 at 15:25, Salil Mehta <salil.mehta@huawei.com> wrote:
>>
>> Extract common GIC and CPU interrupt wiring code to improve code
>> readability and modularity, supporting reuse in future patch sets. This
>> refactor is benign and introduces *no* functional changes.
>>
>> Note: This patch has been isolated from a larger patch set to facilitate
>> early merging and reduce the complexity of the original set, as it
>> operates independently. All original tags and author contributions are
>> retained.
> 
> I would prefer to see refactoring patches in the context of
> the series that makes them necessary. As it stands, there
> doesn't really seem to be much benefit to this change.
> 

+1. Please include those 3 (refactoring) patches into a series where
the background is given.

Thanks,
Gavin

Re: [PATCH] arm/virt: Extract common code to wire GICC<->vCPU IRQs for reuse

[Gavin Shan]

On 11/4/24 1:24 AM, Salil Mehta wrote:
> Extract common GIC and CPU interrupt wiring code to improve code
> readability and modularity, supporting reuse in future patch sets. This
> refactor is benign and introduces *no* functional changes.
> 
> Note: This patch has been isolated from a larger patch set to facilitate
> early merging and reduce the complexity of the original set, as it
> operates independently. All original tags and author contributions are
> retained.
> 
> [!] Please note, this is a purely cosmetic change. No functional change.
> 
> Reported-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
> [4/05/2024: Issue with total number of PPI available during create GIC]
> Suggested-by: Miguel Luis <miguel.luis@oracle.com>
> [5/05/2024: Fix the total number of PPIs available as per ARM BSA to avoid overflow]
> Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> ---
>   hw/arm/virt.c | 108 ++++++++++++++++++++++++++++----------------------
>   1 file changed, 60 insertions(+), 48 deletions(-)
> 

With the following nitpicks addressed:

Reviewed-by: Gavin Shan <gshan@redhat.com>

> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index a0d3bef875..d6892b0266 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -761,6 +761,65 @@ static bool gicv3_nmi_present(VirtMachineState *vms)
>              (vms->gic_version != VIRT_GIC_VERSION_2);
>   }
>   
> +/*
> + * Mapping from the output timer irq lines from the CPU to the GIC PPI inputs
> + * we use for the virt board.
> + */
> +const int timer_irq[] = {
> +    [GTIMER_PHYS] = ARCH_TIMER_NS_EL1_IRQ,
> +    [GTIMER_VIRT] = ARCH_TIMER_VIRT_IRQ,
> +    [GTIMER_HYP]  = ARCH_TIMER_NS_EL2_IRQ,
> +    [GTIMER_SEC]  = ARCH_TIMER_S_EL1_IRQ,
> +    [GTIMER_HYPVIRT] = ARCH_TIMER_NS_EL2_VIRT_IRQ,
> +};
> +

'static' is needed at least since it's a file-scoped array.

> +static void wire_gic_cpu_irqs(VirtMachineState *vms, CPUState *cs)
> +{
> +    SysBusDevice *gicbusdev = SYS_BUS_DEVICE(vms->gic);
> +    unsigned int smp_cpus = MACHINE(vms)->smp.cpus;
> +    DeviceState *cpudev = DEVICE(cs);
> +    int i = CPU(cs)->cpu_index;
> +    int intidbase, irqn;
> +
> +    intidbase = NUM_IRQS + i * GIC_INTERNAL;
> +

The function name wire_gic_cpu_irqs() looks not standard enough. How about to
rename it to virt_set_cpu_irqs(), or virt_connect_cpu_irqs() since we already
had virt_set_cpu_properties()? The subject and changelog need to be adjusted
accordingly.

Lets make some of the variant's names a bit meaningful, and CPU() isn't needed
since @cs is already CPUState?

     int index = cs->cpu_index;
     int n, intidbase = NUM_IRQS + i * GIC_INTERNAL;

> +    for (irqn = 0; irqn < ARRAY_SIZE(timer_irq); irqn++) {
> +        qdev_connect_gpio_out(cpudev, irqn,
> +                              qdev_get_gpio_in(vms->gic,
> +                                               intidbase + timer_irq[irqn]));
> +    }
> +
> +
> +    if (vms->gic_version != VIRT_GIC_VERSION_2) {
> +        qemu_irq irq = qdev_get_gpio_in(vms->gic,
> +                                        intidbase + 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,
> +                                        intidbase + 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, intidbase
> +                                                  + VIRTUAL_PMU_IRQ));
> +
> +    sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
> +    sysbus_connect_irq(gicbusdev, i + smp_cpus,
> +                       qdev_get_gpio_in(cpudev, ARM_CPU_FIQ));
> +    sysbus_connect_irq(gicbusdev, i + 2 * smp_cpus,
> +                       qdev_get_gpio_in(cpudev, ARM_CPU_VIRQ));
> +    sysbus_connect_irq(gicbusdev, i + 3 * smp_cpus,
> +                       qdev_get_gpio_in(cpudev, ARM_CPU_VFIQ));
> +    if (vms->gic_version != VIRT_GIC_VERSION_2) {
> +        sysbus_connect_irq(gicbusdev, i + 4 * smp_cpus,
> +                           qdev_get_gpio_in(cpudev, ARM_CPU_NMI));
> +        sysbus_connect_irq(gicbusdev, i + 5 * smp_cpus,
> +                           qdev_get_gpio_in(cpudev, ARM_CPU_VINMI));
> +    }
> +}
> +
>   static void create_gic(VirtMachineState *vms, MemoryRegion *mem)
>   {
>       MachineState *ms = MACHINE(vms);
> @@ -862,54 +921,7 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem)
>        * CPU's inputs.
>        */
>       for (i = 0; i < smp_cpus; i++) {
> -        DeviceState *cpudev = DEVICE(qemu_get_cpu(i));
> -        int intidbase = NUM_IRQS + i * GIC_INTERNAL;
> -        /* Mapping from the output timer irq lines from the CPU to the
> -         * GIC PPI inputs we use for the virt board.
> -         */
> -        const int timer_irq[] = {
> -            [GTIMER_PHYS] = ARCH_TIMER_NS_EL1_IRQ,
> -            [GTIMER_VIRT] = ARCH_TIMER_VIRT_IRQ,
> -            [GTIMER_HYP]  = ARCH_TIMER_NS_EL2_IRQ,
> -            [GTIMER_SEC]  = ARCH_TIMER_S_EL1_IRQ,
> -            [GTIMER_HYPVIRT] = ARCH_TIMER_NS_EL2_VIRT_IRQ,
> -        };
> -
> -        for (unsigned irq = 0; irq < ARRAY_SIZE(timer_irq); irq++) {
> -            qdev_connect_gpio_out(cpudev, irq,
> -                                  qdev_get_gpio_in(vms->gic,
> -                                                   intidbase + timer_irq[irq]));
> -        }
> -
> -        if (vms->gic_version != VIRT_GIC_VERSION_2) {
> -            qemu_irq irq = qdev_get_gpio_in(vms->gic,
> -                                            intidbase + 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,
> -                                            intidbase + 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, intidbase
> -                                                     + VIRTUAL_PMU_IRQ));
> -
> -        sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
> -        sysbus_connect_irq(gicbusdev, i + smp_cpus,
> -                           qdev_get_gpio_in(cpudev, ARM_CPU_FIQ));
> -        sysbus_connect_irq(gicbusdev, i + 2 * smp_cpus,
> -                           qdev_get_gpio_in(cpudev, ARM_CPU_VIRQ));
> -        sysbus_connect_irq(gicbusdev, i + 3 * smp_cpus,
> -                           qdev_get_gpio_in(cpudev, ARM_CPU_VFIQ));
> -
> -        if (vms->gic_version != VIRT_GIC_VERSION_2) {
> -            sysbus_connect_irq(gicbusdev, i + 4 * smp_cpus,
> -                               qdev_get_gpio_in(cpudev, ARM_CPU_NMI));
> -            sysbus_connect_irq(gicbusdev, i + 5 * smp_cpus,
> -                               qdev_get_gpio_in(cpudev, ARM_CPU_VINMI));
> -        }
> +        wire_gic_cpu_irqs(vms, qemu_get_cpu(i));
>       }
>   
>       fdt_add_gic_node(vms);

Thanks,
Gavin

RE: [PATCH] arm/virt: Extract common code to wire GICC<->vCPU IRQs for reuse

[Salil Mehta via]

HI Peter,

>  From: Peter Maydell <peter.maydell@linaro.org>
>  Sent: Monday, November 4, 2024 1:27 PM
>  To: Salil Mehta <salil.mehta@huawei.com>
>  
>  On Sun, 3 Nov 2024 at 15:25, Salil Mehta <salil.mehta@huawei.com> wrote:
>  >
>  > Extract common GIC and CPU interrupt wiring code to improve code
>  > readability and modularity, supporting reuse in future patch sets.
>  > This refactor is benign and introduces *no* functional changes.
>  >
>  > Note: This patch has been isolated from a larger patch set to
>  > facilitate early merging and reduce the complexity of the original
>  > set, as it operates independently. All original tags and author
>  > contributions are retained.
>  
>  I would prefer to see refactoring patches in the context of the series that
>  makes them necessary. As it stands, there doesn't really seem to be much
>  benefit to this change.


Ok no issues. Just to confirm, your above comment is just specific to this patch
or all the 3 individual patches I had sent for review?

Thanks
Salil.
>  
>  thanks
>  -- PMM

Re: [PATCH] arm/virt: Extract common code to wire GICC<->vCPU IRQs for reuse

[Peter Maydell]

On Tue, 5 Nov 2024 at 22:20, Salil Mehta <salil.mehta@huawei.com> wrote:
>
> HI Peter,
>
> >  From: Peter Maydell <peter.maydell@linaro.org>
> >  Sent: Monday, November 4, 2024 1:27 PM
> >  To: Salil Mehta <salil.mehta@huawei.com>
> >
> >  On Sun, 3 Nov 2024 at 15:25, Salil Mehta <salil.mehta@huawei.com> wrote:
> >  >
> >  > Extract common GIC and CPU interrupt wiring code to improve code
> >  > readability and modularity, supporting reuse in future patch sets.
> >  > This refactor is benign and introduces *no* functional changes.
> >  >
> >  > Note: This patch has been isolated from a larger patch set to
> >  > facilitate early merging and reduce the complexity of the original
> >  > set, as it operates independently. All original tags and author
> >  > contributions are retained.
> >
> >  I would prefer to see refactoring patches in the context of the series that
> >  makes them necessary. As it stands, there doesn't really seem to be much
> >  benefit to this change.
>
>
> Ok no issues. Just to confirm, your above comment is just specific to this patch
> or all the 3 individual patches I had sent for review?

It applies to all of them (and as a general principle).

thanks
-- PMM

RE: [PATCH] arm/virt: Extract common code to wire GICC<->vCPU IRQs for reuse

[Salil Mehta via]

HI Peter,

>  From: Peter Maydell <peter.maydell@linaro.org>
>  Sent: Wednesday, November 6, 2024 1:01 PM
>  To: Salil Mehta <salil.mehta@huawei.com>
>  
>  On Tue, 5 Nov 2024 at 22:20, Salil Mehta <salil.mehta@huawei.com> wrote:
>  >
>  > HI Peter,
>  >
>  > >  From: Peter Maydell <peter.maydell@linaro.org>
>  > >  Sent: Monday, November 4, 2024 1:27 PM
>  > >  To: Salil Mehta <salil.mehta@huawei.com>
>  > >
>  > >  On Sun, 3 Nov 2024 at 15:25, Salil Mehta <salil.mehta@huawei.com>
>  wrote:
>  > >  >
>  > >  > Extract common GIC and CPU interrupt wiring code to improve code
>  > > > readability and modularity, supporting reuse in future patch sets.
>  > >  > This refactor is benign and introduces *no* functional changes.
>  > >  >
>  > >  > Note: This patch has been isolated from a larger patch set to  >
>  > > facilitate early merging and reduce the complexity of the original
>  > > > set, as it operates independently. All original tags and author  >
>  > > contributions are retained.
>  > >
>  > >  I would prefer to see refactoring patches in the context of the
>  > > series that  makes them necessary. As it stands, there doesn't
>  > > really seem to be much  benefit to this change.
>  >
>  >
>  > Ok no issues. Just to confirm, your above comment is just specific to
>  > this patch or all the 3 individual patches I had sent for review?
>  
>  It applies to all of them (and as a general principle).


Got it. I thought changes in below patches were in any case useful as they were
improving the code?

https://lore.kernel.org/qemu-devel/20241103152256.202444-1-salil.mehta@huawei.com/
https://lore.kernel.org/qemu-devel/20241103152639.202480-1-salil.mehta@huawei.com/

Thanks

>  
>  thanks
>  -- PMM