[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/3] hw/intc/arm_gicv3: Move checking of redist-region-count
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH 1/3] hw/intc/arm_gicv3: Move checking of redist-region-count to arm_gicv3_common_realize |
Date: |
Thu, 30 Sep 2021 23:54:14 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.1.0 |
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().