| Message ID | 20210930150842.3810-2-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm_gicv3: Support multiple redistributor regions | expand |
On 9/30/21 17:08, Peter Maydell wrote: > The GICv3 devices have an array property redist-region-count. > Currently we check this for errors (bad values) in > gicv3_init_irqs_and_mmio(), just before we use it. Move this error > checking to the arm_gicv3_common_realize() function, where we > sanity-check all of the other base-class properties. (This will > always be before gicv3_init_irqs_and_mmio() is called, because > that function is called in the subclass realize methods, after > they have called the parent-class realize.) > > The motivation for this refactor is: > * we would like to use the redist_region_count[] values in > arm_gicv3_common_realize() in a subsequent patch, so we need > to have already done the sanity-checking first > * this removes the only use of the Error** argument to > gicv3_init_irqs_and_mmio(), so we can remove some error-handling > boilerplate > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > include/hw/intc/arm_gicv3_common.h | 2 +- > hw/intc/arm_gicv3.c | 6 +----- > hw/intc/arm_gicv3_common.c | 26 +++++++++++++------------- > hw/intc/arm_gicv3_kvm.c | 6 +----- > 4 files changed, 16 insertions(+), 24 deletions(-) > > diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h > index aa4f0d67703..cb2b0d0ad45 100644 > --- a/include/hw/intc/arm_gicv3_common.h > +++ b/include/hw/intc/arm_gicv3_common.h > @@ -306,6 +306,6 @@ struct ARMGICv3CommonClass { > }; > > void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler, > - const MemoryRegionOps *ops, Error **errp); > + const MemoryRegionOps *ops); > > #endif > diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c > index 3f24707838c..bcf54a5f0a5 100644 > --- a/hw/intc/arm_gicv3.c > +++ b/hw/intc/arm_gicv3.c > @@ -393,11 +393,7 @@ static void arm_gic_realize(DeviceState *dev, Error **errp) > return; > } > > - gicv3_init_irqs_and_mmio(s, gicv3_set_irq, gic_ops, &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > - return; > - } > + gicv3_init_irqs_and_mmio(s, gicv3_set_irq, gic_ops); > > gicv3_init_cpuif(s); > } > diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c > index 223db16feca..8e47809398b 100644 > --- a/hw/intc/arm_gicv3_common.c > +++ b/hw/intc/arm_gicv3_common.c > @@ -250,22 +250,11 @@ static const VMStateDescription vmstate_gicv3 = { > }; > > void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler, > - const MemoryRegionOps *ops, Error **errp) > + const MemoryRegionOps *ops) > { > SysBusDevice *sbd = SYS_BUS_DEVICE(s); > - int rdist_capacity = 0; > int i; > > - for (i = 0; i < s->nb_redist_regions; i++) { > - rdist_capacity += s->redist_region_count[i]; > - } > - if (rdist_capacity < s->num_cpu) { > - error_setg(errp, "Capacity of the redist regions(%d) " > - "is less than number of vcpus(%d)", > - rdist_capacity, s->num_cpu); > - return; > - } > - > /* For the GIC, also expose incoming GPIO lines for PPIs for each CPU. > * GPIO array layout is thus: > * [0..N-1] spi > @@ -308,7 +297,7 @@ void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler, > static void arm_gicv3_common_realize(DeviceState *dev, Error **errp) > { > GICv3State *s = ARM_GICV3_COMMON(dev); > - int i; > + int i, rdist_capacity; > > /* revision property is actually reserved and currently used only in order > * to keep the interface compatible with GICv2 code, avoiding extra > @@ -350,6 +339,17 @@ static void arm_gicv3_common_realize(DeviceState *dev, Error **errp) > return; > } > > + rdist_capacity = 0; > + for (i = 0; i < s->nb_redist_regions; i++) { > + rdist_capacity += s->redist_region_count[i]; > + } > + if (rdist_capacity < s->num_cpu) { > + error_setg(errp, "Capacity of the redist regions(%d) " > + "is less than number of vcpus(%d)", > + rdist_capacity, s->num_cpu); > + return; > + } > + > s->cpu = g_new0(GICv3CPUState, s->num_cpu); > > for (i = 0; i < s->num_cpu; i++) { > diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c > index 5c09f00dec2..ab58c73306d 100644 > --- a/hw/intc/arm_gicv3_kvm.c > +++ b/hw/intc/arm_gicv3_kvm.c > @@ -787,11 +787,7 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp) > return; > } > > - gicv3_init_irqs_and_mmio(s, kvm_arm_gicv3_set_irq, NULL, &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > - return; > - } > + gicv3_init_irqs_and_mmio(s, kvm_arm_gicv3_set_irq, NULL); > > for (i = 0; i < s->num_cpu; i++) { > ARMCPU *cpu = ARM_CPU(qemu_get_cpu(i)); > The pattern make me think gicv3_init_irqs_and_mmio() should be refactored as a ARMGICv3CommonClass::init_irqs_and_mmio handler, called in arm_gicv3_common_realize(). Or maybe even have ARMGICv3CommonClass::irq_handler and ARMGICv3CommonClass::ops set in each child class_init().
On Thu, 30 Sept 2021 at 22:54, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > The pattern make me think gicv3_init_irqs_and_mmio() should be > refactored as a ARMGICv3CommonClass::init_irqs_and_mmio handler, > called in arm_gicv3_common_realize(). That won't work because the two subclasses want to call it with different arguments. More generally, I find QEMU's class method infrastructure sufficiently clunky that it is better avoided except in the case where it's actually needed, ie where you have one callsite that might have to deal with an object whose exact type may vary such that you don't know which of the methods you want and you need the dynamic-dispatch. In this case that doesn't apply: both of the callsites of gicv3_init_irqs_and_mmio() know exactly what function they want to be calling (there is only one implementation), and moreover both callsites are part of the GICv3 implementation themselves, so this isn't about presenting a nice (or "nice") external interface to other parts of QEMU. -- PMM
On 9/30/21 11:08 AM, Peter Maydell wrote: > The GICv3 devices have an array property redist-region-count. > Currently we check this for errors (bad values) in > gicv3_init_irqs_and_mmio(), just before we use it. Move this error > checking to the arm_gicv3_common_realize() function, where we > sanity-check all of the other base-class properties. (This will > always be before gicv3_init_irqs_and_mmio() is called, because > that function is called in the subclass realize methods, after > they have called the parent-class realize.) > > The motivation for this refactor is: > * we would like to use the redist_region_count[] values in > arm_gicv3_common_realize() in a subsequent patch, so we need > to have already done the sanity-checking first > * this removes the only use of the Error** argument to > gicv3_init_irqs_and_mmio(), so we can remove some error-handling > boilerplate > > Signed-off-by: Peter Maydell<peter.maydell@linaro.org> > --- Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h index aa4f0d67703..cb2b0d0ad45 100644 --- a/include/hw/intc/arm_gicv3_common.h +++ b/include/hw/intc/arm_gicv3_common.h @@ -306,6 +306,6 @@ struct ARMGICv3CommonClass { }; void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler, - const MemoryRegionOps *ops, Error **errp); + const MemoryRegionOps *ops); #endif diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c index 3f24707838c..bcf54a5f0a5 100644 --- a/hw/intc/arm_gicv3.c +++ b/hw/intc/arm_gicv3.c @@ -393,11 +393,7 @@ static void arm_gic_realize(DeviceState *dev, Error **errp) return; } - gicv3_init_irqs_and_mmio(s, gicv3_set_irq, gic_ops, &local_err); - if (local_err) { - error_propagate(errp, local_err); - return; - } + gicv3_init_irqs_and_mmio(s, gicv3_set_irq, gic_ops); gicv3_init_cpuif(s); } diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c index 223db16feca..8e47809398b 100644 --- a/hw/intc/arm_gicv3_common.c +++ b/hw/intc/arm_gicv3_common.c @@ -250,22 +250,11 @@ static const VMStateDescription vmstate_gicv3 = { }; void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler, - const MemoryRegionOps *ops, Error **errp) + const MemoryRegionOps *ops) { SysBusDevice *sbd = SYS_BUS_DEVICE(s); - int rdist_capacity = 0; int i; - for (i = 0; i < s->nb_redist_regions; i++) { - rdist_capacity += s->redist_region_count[i]; - } - if (rdist_capacity < s->num_cpu) { - error_setg(errp, "Capacity of the redist regions(%d) " - "is less than number of vcpus(%d)", - rdist_capacity, s->num_cpu); - return; - } - /* For the GIC, also expose incoming GPIO lines for PPIs for each CPU. * GPIO array layout is thus: * [0..N-1] spi @@ -308,7 +297,7 @@ void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler, static void arm_gicv3_common_realize(DeviceState *dev, Error **errp) { GICv3State *s = ARM_GICV3_COMMON(dev); - int i; + int i, rdist_capacity; /* revision property is actually reserved and currently used only in order * to keep the interface compatible with GICv2 code, avoiding extra @@ -350,6 +339,17 @@ static void arm_gicv3_common_realize(DeviceState *dev, Error **errp) return; } + rdist_capacity = 0; + for (i = 0; i < s->nb_redist_regions; i++) { + rdist_capacity += s->redist_region_count[i]; + } + if (rdist_capacity < s->num_cpu) { + error_setg(errp, "Capacity of the redist regions(%d) " + "is less than number of vcpus(%d)", + rdist_capacity, s->num_cpu); + return; + } + s->cpu = g_new0(GICv3CPUState, s->num_cpu); for (i = 0; i < s->num_cpu; i++) { diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c index 5c09f00dec2..ab58c73306d 100644 --- a/hw/intc/arm_gicv3_kvm.c +++ b/hw/intc/arm_gicv3_kvm.c @@ -787,11 +787,7 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp) return; } - gicv3_init_irqs_and_mmio(s, kvm_arm_gicv3_set_irq, NULL, &local_err); - if (local_err) { - error_propagate(errp, local_err); - return; - } + gicv3_init_irqs_and_mmio(s, kvm_arm_gicv3_set_irq, NULL); for (i = 0; i < s->num_cpu; i++) { ARMCPU *cpu = ARM_CPU(qemu_get_cpu(i));
The GICv3 devices have an array property redist-region-count. Currently we check this for errors (bad values) in gicv3_init_irqs_and_mmio(), just before we use it. Move this error checking to the arm_gicv3_common_realize() function, where we sanity-check all of the other base-class properties. (This will always be before gicv3_init_irqs_and_mmio() is called, because that function is called in the subclass realize methods, after they have called the parent-class realize.) The motivation for this refactor is: * we would like to use the redist_region_count[] values in arm_gicv3_common_realize() in a subsequent patch, so we need to have already done the sanity-checking first * this removes the only use of the Error** argument to gicv3_init_irqs_and_mmio(), so we can remove some error-handling boilerplate Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- include/hw/intc/arm_gicv3_common.h | 2 +- hw/intc/arm_gicv3.c | 6 +----- hw/intc/arm_gicv3_common.c | 26 +++++++++++++------------- hw/intc/arm_gicv3_kvm.c | 6 +----- 4 files changed, 16 insertions(+), 24 deletions(-) -- 2.20.1