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 <vis...@os.amperecomputing.com>
[4/05/2024: Issue with total number of PPI available during create GIC]
Suggested-by: Miguel Luis <miguel.l...@oracle.com>
[5/05/2024: Fix the total number of PPIs available as per ARM BSA to avoid
overflow]
Co-developed-by: Keqian Zhu <zhukeqi...@huawei.com>
Signed-off-by: Keqian Zhu <zhukeqi...@huawei.com>
Signed-off-by: Salil Mehta <salil.me...@huawei.com>
---
hw/arm/virt.c | 108 ++++++++++++++++++++++++++++----------------------
1 file changed, 60 insertions(+), 48 deletions(-)
With the following nitpicks addressed:
Reviewed-by: Gavin Shan <gs...@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
