Re: [RFC 19/22] xen/arm: Merge gicv_setup with vgic_domain_init

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
> Currently, it's hard to decide whether a part of the domain
> initialization  should live in gicv_setup (part of the GIC
> driver) and domain_init (part of the vGIC driver).
> 
> The code to initialize the domain for a specific vGIC version is always
> the same no matter the version of the GIC.
> 
> Move all the domain initialization code for the vGIC in the respective
> domain_init callback of each vGIC drivers.

Pure code motion? If yes:

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> Cc: Zoltan Kiss <zoltan.kiss@huawei.com>
> ---
>  xen/arch/arm/domain.c     |  3 ---
>  xen/arch/arm/gic-hip04.c  | 42 ----------------------------------
>  xen/arch/arm/gic-v2.c     | 42 ----------------------------------
>  xen/arch/arm/gic-v3.c     | 58 -----------------------------------------------
>  xen/arch/arm/gic.c        | 10 ++++----
>  xen/arch/arm/vgic-v2.c    | 42 +++++++++++++++++++++++++++++++++-
>  xen/arch/arm/vgic-v3.c    | 54 ++++++++++++++++++++++++++++++++++++++++++-
>  xen/include/asm-arm/gic.h |  4 ++--
>  8 files changed, 101 insertions(+), 154 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index e4d6fc8..9b113eb 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -587,9 +587,6 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>      }
>      config->gic_version = gic_version;
>  
> -    if ( (rc = gicv_setup(d)) != 0 )
> -        goto fail;
> -
>      if ( (rc = domain_vgic_init(d)) != 0 )
>          goto fail;
>  
> diff --git a/xen/arch/arm/gic-hip04.c b/xen/arch/arm/gic-hip04.c
> index 416ef83..9693666 100644
> --- a/xen/arch/arm/gic-hip04.c
> +++ b/xen/arch/arm/gic-hip04.c
> @@ -439,47 +439,6 @@ static void hip04gic_clear_lr(int lr)
>      writel_gich(0, HIP04_GICH_LR + lr * 4);
>  }
>  
> -static int hip04gicv_setup(struct domain *d)
> -{
> -    int ret;
> -
> -    /*
> -     * The hardware domain gets the hardware address.
> -     * Guests get the virtual platform layout.
> -     */
> -    if ( is_hardware_domain(d) )
> -    {
> -        d->arch.vgic.dbase = gicv2_info.dbase;
> -        d->arch.vgic.cbase = gicv2_info.cbase;
> -    }
> -    else
> -    {
> -        d->arch.vgic.dbase = GUEST_GICD_BASE;
> -        d->arch.vgic.cbase = GUEST_GICC_BASE;
> -    }
> -
> -    /*
> -     * Map the gic virtual cpu interface in the gic cpu interface
> -     * region of the guest.
> -     *
> -     * The second page is always mapped at +4K irrespective of the
> -     * GIC_64K_STRIDE quirk. The DTB passed to the guest reflects this.
> -     */
> -    ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase), 1,
> -                            paddr_to_pfn(gicv2_info.vbase));
> -    if ( ret )
> -        return ret;
> -
> -    if ( !platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
> -        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
> -                               2, paddr_to_pfn(gicv2_info.vbase + SZ_4K));
> -    else
> -        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
> -                               2, paddr_to_pfn(gicv2_info.vbase + SZ_64K));
> -
> -    return ret;
> -}
> -
>  static void hip04gic_read_lr(int lr, struct gic_lr *lr_reg)
>  {
>      uint32_t lrv;
> @@ -771,7 +730,6 @@ const static struct gic_hw_operations hip04gic_ops = {
>      .save_state          = hip04gic_save_state,
>      .restore_state       = hip04gic_restore_state,
>      .dump_state          = hip04gic_dump_state,
> -    .gicv_setup          = hip04gicv_setup,
>      .gic_host_irq_type   = &hip04gic_host_irq_type,
>      .gic_guest_irq_type  = &hip04gic_guest_irq_type,
>      .eoi_irq             = hip04gic_eoi_irq,
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index bd5603b..f53560e 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -429,47 +429,6 @@ static void gicv2_clear_lr(int lr)
>      writel_gich(0, GICH_LR + lr * 4);
>  }
>  
> -static int gicv2v_setup(struct domain *d)
> -{
> -    int ret;
> -
> -    /*
> -     * The hardware domain gets the hardware address.
> -     * Guests get the virtual platform layout.
> -     */
> -    if ( is_hardware_domain(d) )
> -    {
> -        d->arch.vgic.dbase = gicv2_info.dbase;
> -        d->arch.vgic.cbase = gicv2_info.cbase;
> -    }
> -    else
> -    {
> -        d->arch.vgic.dbase = GUEST_GICD_BASE;
> -        d->arch.vgic.cbase = GUEST_GICC_BASE;
> -    }
> -
> -    /*
> -     * Map the gic virtual cpu interface in the gic cpu interface
> -     * region of the guest.
> -     *
> -     * The second page is always mapped at +4K irrespective of the
> -     * GIC_64K_STRIDE quirk. The DTB passed to the guest reflects this.
> -     */
> -    ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase), 1,
> -                            paddr_to_pfn(gicv2_info.vbase));
> -    if ( ret )
> -        return ret;
> -
> -    if ( !platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
> -        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
> -                               2, paddr_to_pfn(gicv2_info.vbase + SZ_4K));
> -    else
> -        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
> -                               2, paddr_to_pfn(gicv2._info.vbase + SZ_64K));
> -
> -    return ret;
> -}
> -
>  static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
>  {
>      uint32_t lrv;
> @@ -756,7 +715,6 @@ const static struct gic_hw_operations gicv2_ops = {
>      .save_state          = gicv2_save_state,
>      .restore_state       = gicv2_restore_state,
>      .dump_state          = gicv2_dump_state,
> -    .gicv_setup          = gicv2v_setup,
>      .gic_host_irq_type   = &gicv2_host_irq_type,
>      .gic_guest_irq_type  = &gicv2_guest_irq_type,
>      .eoi_irq             = gicv2_eoi_irq,
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 13250c5..7603a2c 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -894,63 +894,6 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
>      gicv3_ich_write_lr(lr_reg, lrv);
>  }
>  
> -static int gicv_v3_init(struct domain *d)
> -{
> -    int i;
> -
> -    /*
> -     * Domain 0 gets the hardware address.
> -     * Guests get the virtual platform layout.
> -     */
> -    if ( is_hardware_domain(d) )
> -    {
> -        unsigned int first_cpu = 0;
> -
> -        d->arch.vgic.dbase = gicv3_info.dbase;
> -
> -        d->arch.vgic.rdist_stride = gicv3_info.rdist_stride;
> -        /*
> -         * If the stride is not set, the default stride for GICv3 is 2 * 64K:
> -         *     - first 64k page for Control and Physical LPIs
> -         *     - second 64k page for Control and Generation of SGIs
> -         */
> -        if ( !d->arch.vgic.rdist_stride )
> -            d->arch.vgic.rdist_stride = 2 * SZ_64K;
> -
> -        for ( i = 0; i < gicv3_info.nr_rdist_regions; i++ )
> -        {
> -            paddr_t size = gicv3_info.rdist_regions[i].size;
> -
> -            d->arch.vgic.rdist_regions[i].base = gicv3_info.rdist_regions[i].base;
> -            d->arch.vgic.rdist_regions[i].size = size;
> -
> -            /* Set the first CPU handled by this region */
> -            d->arch.vgic.rdist_regions[i].first_cpu = first_cpu;
> -
> -            first_cpu += size / d->arch.vgic.rdist_stride;
> -        }
> -        d->arch.vgic.nr_regions = gicv3_info.nr_rdist_regions;
> -    }
> -    else
> -    {
> -        d->arch.vgic.dbase = GUEST_GICV3_GICD_BASE;
> -
> -        /* XXX: Only one Re-distributor region mapped for the guest */
> -        BUILD_BUG_ON(GUEST_GICV3_RDIST_REGIONS != 1);
> -
> -        d->arch.vgic.nr_regions = GUEST_GICV3_RDIST_REGIONS;
> -        d->arch.vgic.rdist_stride = GUEST_GICV3_RDIST_STRIDE;
> -
> -        /* The first redistributor should contain enough space for all CPUs */
> -        BUILD_BUG_ON((GUEST_GICV3_GICR0_SIZE / GUEST_GICV3_RDIST_STRIDE) < MAX_VIRT_CPUS);
> -        d->arch.vgic.rdist_regions[0].base = GUEST_GICV3_GICR0_BASE;
> -        d->arch.vgic.rdist_regions[0].size = GUEST_GICV3_GICR0_SIZE;
> -        d->arch.vgic.rdist_regions[0].first_cpu = 0;
> -    }
> -
> -    return 0;
> -}
> -
>  static void gicv3_hcr_status(uint32_t flag, bool_t status)
>  {
>      uint32_t hcr;
> @@ -1284,7 +1227,6 @@ static const struct gic_hw_operations gicv3_ops = {
>      .save_state          = gicv3_save_state,
>      .restore_state       = gicv3_restore_state,
>      .dump_state          = gicv3_dump_state,
> -    .gicv_setup          = gicv_v3_init,
>      .gic_host_irq_type   = &gicv3_host_irq_type,
>      .gic_guest_irq_type  = &gicv3_guest_irq_type,
>      .eoi_irq             = gicv3_eoi_irq,
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 5f34997..4d8adb9 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -67,6 +67,11 @@ unsigned int gic_number_lines(void)
>      return gic_hw_ops->info->nr_lines;
>  }
>  
> +const struct gic_info *gic_info(void)
> +{
> +    return gic_hw_ops->info;
> +}
> +
>  void gic_save_state(struct vcpu *v)
>  {
>      ASSERT(!local_irq_is_enabled());
> @@ -620,11 +625,6 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
>      } while (1);
>  }
>  
> -int gicv_setup(struct domain *d)
> -{
> -    return gic_hw_ops->gicv_setup(d);
> -}
> -
>  static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
>  {
>      /*
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 9eecabc..3be1a51 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -24,10 +24,12 @@
>  #include <xen/softirq.h>
>  #include <xen/irq.h>
>  #include <xen/sched.h>
> +#include <xen/sizes.h>
>  
>  #include <asm/current.h>
>  
>  #include <asm/mmio.h>
> +#include <asm/platform.h>
>  #include <asm/gic.h>
>  #include <asm/vgic.h>
>  
> @@ -525,7 +527,45 @@ static int vgic_v2_vcpu_init(struct vcpu *v)
>  
>  static int vgic_v2_domain_init(struct domain *d)
>  {
> -    int i;
> +    int i, ret;
> +    const struct gic_info *info = gic_info();
> +
> +    /*
> +     * The hardware domain gets the hardware address.
> +     * Guests get the virtual platform layout.
> +     */
> +    if ( is_hardware_domain(d) )
> +    {
> +        d->arch.vgic.dbase = info->dbase;
> +        d->arch.vgic.cbase = info->cbase;
> +    }
> +    else
> +    {
> +        d->arch.vgic.dbase = GUEST_GICD_BASE;
> +        d->arch.vgic.cbase = GUEST_GICC_BASE;
> +    }
> +
> +    /*
> +     * Map the gic virtual cpu interface in the gic cpu interface
> +     * region of the guest.
> +     *
> +     * The second page is always mapped at +4K irrespective of the
> +     * GIC_64K_STRIDE quirk. The DTB passed to the guest reflects this.
> +     */
> +    ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase), 1,
> +                           paddr_to_pfn(info->vbase));
> +    if ( ret )
> +        return ret;
> +
> +    if ( !platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
> +        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
> +                               2, paddr_to_pfn(info->vbase + SZ_64K));
> +    else
> +        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
> +                               2, paddr_to_pfn(info->vbase + SZ_64K));
> +
> +    if ( ret )
> +        return ret;
>  
>      /* By default deliver to CPU0 */
>      for ( i = 0; i < DOMAIN_NR_RANKS(d); i++ )
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 77ada20..45d54a2 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -1146,6 +1146,57 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
>  static int vgic_v3_domain_init(struct domain *d)
>  {
>      int i, idx;
> +    const struct gic_info *info = gic_info();
> +
> +    /*
> +     * Domain 0 gets the hardware address.
> +     * Guests get the virtual platform layout.
> +     */
> +    if ( is_hardware_domain(d) )
> +    {
> +        unsigned int first_cpu = 0;
> +
> +        d->arch.vgic.dbase = info->dbase;
> +
> +        d->arch.vgic.rdist_stride = info->rdist_stride;
> +        /*
> +         * If the stride is not set, the default stride for GICv3 is 2 * 64K:
> +         *     - first 64k page for Control and Physical LPIs
> +         *     - second 64k page for Control and Generation of SGIs
> +         */
> +        if ( !d->arch.vgic.rdist_stride )
> +            d->arch.vgic.rdist_stride = 2 * SZ_64K;
> +
> +        for ( i = 0; i < info->nr_rdist_regions; i++ )
> +        {
> +            paddr_t size = info->rdist_regions[i].size;
> +
> +            d->arch.vgic.rdist_regions[i].base = info->rdist_regions[i].base;
> +            d->arch.vgic.rdist_regions[i].size = size;
> +
> +            /* Set the first CPU handled by this region */
> +            d->arch.vgic.rdist_regions[i].first_cpu = first_cpu;
> +
> +            first_cpu += size / d->arch.vgic.rdist_stride;
> +        }
> +        d->arch.vgic.nr_regions = info->nr_rdist_regions;
> +    }
> +    else
> +    {
> +        d->arch.vgic.dbase = GUEST_GICV3_GICD_BASE;
> +
> +        /* XXX: Only one Re-distributor region mapped for the guest */
> +        BUILD_BUG_ON(GUEST_GICV3_RDIST_REGIONS != 1);
> +
> +        d->arch.vgic.nr_regions = GUEST_GICV3_RDIST_REGIONS;
> +        d->arch.vgic.rdist_stride = GUEST_GICV3_RDIST_STRIDE;
> +
> +        /* The first redistributor should contain enough space for all CPUs */
> +        BUILD_BUG_ON((GUEST_GICV3_GICR0_SIZE / GUEST_GICV3_RDIST_STRIDE) < MAX_VIRT_CPUS);
> +        d->arch.vgic.rdist_regions[0].base = GUEST_GICV3_GICR0_BASE;
> +        d->arch.vgic.rdist_regions[0].size = GUEST_GICV3_GICR0_SIZE;
> +        d->arch.vgic.rdist_regions[0].first_cpu = 0;
> +    }
>  
>      /* By default deliver to CPU0 */
>      for ( i = 0; i < DOMAIN_NR_RANKS(d); i++ )
> @@ -1153,7 +1204,8 @@ static int vgic_v3_domain_init(struct domain *d)
>          for ( idx = 0; idx < 32; idx++ )
>              d->arch.vgic.shared_irqs[i].v3.irouter[idx] = 0;
>      }
> -    /* We rely on gicv init to get dbase and size */
> +
> +    /* Register mmio handle for the Distributor */
>      register_mmio_handler(d, &vgic_distr_mmio_handler, d->arch.vgic.dbase,
>                            SZ_64K);
>  
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index be0f610..4319ac4 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -307,6 +307,8 @@ struct gic_info {
>  #endif
>  };
>  
> +const struct gic_info *gic_info(void);
> +
>  struct gic_hw_operations {
>      /* Hold GIC HW information */
>      const struct gic_info *info;
> @@ -318,8 +320,6 @@ struct gic_hw_operations {
>      void (*restore_state)(const struct vcpu *);
>      /* Dump GIC LR register information */
>      void (*dump_state)(const struct vcpu *);
> -    /* Map MMIO region of GIC */
> -    int (*gicv_setup)(struct domain *);
>  
>      /* hw_irq_controller to enable/disable/eoi host irq */
>      hw_irq_controller *gic_host_irq_type;