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

[Gavin Shan <gshan@redhat.com>]

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