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));
--
2.20.1
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~
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
© 2016 - 2026 Red Hat, Inc.