| Message ID | 20241103152455.202462-1-salil.mehta@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm/virt: Extract common code to wire GICC<->vCPU IRQs for reuse | expand |
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
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
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
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
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
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
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);