Re: [PATCH 1/3] hw/intc/arm_gicv3: Move checking of redist-region-count to arm_gicv3_common_realize

[Philippe Mathieu-Daudé]

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().