Re: [RFC 16/22] xen/arm: gic-v2: Move GICD, GICC and GICV base address in gic_info ...

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

On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
> ... in order to decouple the vGIC driver from the GIC driver.

Making the common physical GIC info struct contain the union of all gic
versions for the benefit of each of the vgic drivers is not how I would
prefer to see this.

I think we should decouple this further and have each physical gic
driver fill in vgic_vN_info structs provided by the vgic-vN.c drivers
for each sort of vgic which could run on it.

So gic-v2.c would fill in just vgic-v2.c:vgic_v2_info while gic-v3.c
would fill in both vgic-v2.c:vgic_v2_info and vgic-v3.c:vgic_v3_info.

This more completely isolates physical gic stuff from virtual.

Please do all three gics in one go, no need to split this into 3
patches.

> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> ---
>  xen/arch/arm/gic-v2.c     | 33 +++++++++++++++------------------
>  xen/include/asm-arm/gic.h |  6 ++++++
>  2 files changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 0327095..bd5603b 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -64,12 +64,9 @@
>  
>  /* Global state */
>  static struct {
> -    paddr_t dbase;            /* Address of distributor registers */
>      void __iomem * map_dbase; /* IO mapped Address of distributor registers */
> -    paddr_t cbase;            /* Address of CPU interface registers */
>      void __iomem * map_cbase[2]; /* IO mapped Address of CPU interface registers */
>      void __iomem * map_hbase; /* IO Address of virtual interface registers */
> -    paddr_t vbase;            /* Address of virtual cpu interface registers */
>      spinlock_t lock;
>  } gicv2;
>  
> @@ -442,8 +439,8 @@ static int gicv2v_setup(struct domain *d)
>       */
>      if ( is_hardware_domain(d) )
>      {
> -        d->arch.vgic.dbase = gicv2.dbase;
> -        d->arch.vgic.cbase = gicv2.cbase;
> +        d->arch.vgic.dbase = gicv2_info.dbase;
> +        d->arch.vgic.cbase = gicv2_info.cbase;
>      }
>      else
>      {
> @@ -459,16 +456,16 @@ static int gicv2v_setup(struct domain *d)
>       * 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.vbase));
> +                            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.vbase + 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.vbase + SZ_64K));
> +                               2, paddr_to_pfn(gicv2._info.vbase + SZ_64K));
>  
>      return ret;
>  }
> @@ -685,11 +682,11 @@ static int __init gicv2_init(void)
>      paddr_t hbase;
>      const struct dt_device_node *node = gicv2_info.node;
>  
> -    res = dt_device_get_address(node, 0, &gicv2.dbase, NULL);
> +    res = dt_device_get_address(node, 0, &gicv2_info.dbase, NULL);
>      if ( res )
>          panic("GICv2: Cannot find a valid address for the distributor");
>  
> -    res = dt_device_get_address(node, 1, &gicv2.cbase, NULL);
> +    res = dt_device_get_address(node, 1, &gicv2_info.cbase, NULL);
>      if ( res )
>          panic("GICv2: Cannot find a valid address for the CPU");
>  
> @@ -697,7 +694,7 @@ static int __init gicv2_init(void)
>      if ( res )
>          panic("GICv2: Cannot find a valid address for the hypervisor");
>  
> -    res = dt_device_get_address(node, 3, &gicv2.vbase, NULL);
> +    res = dt_device_get_address(node, 3, &gicv2_info.vbase, NULL);
>      if ( res )
>          panic("GICv2: Cannot find a valid address for the virtual CPU");
>  
> @@ -714,23 +711,23 @@ static int __init gicv2_init(void)
>                "        gic_hyp_addr=%"PRIpaddr"\n"
>                "        gic_vcpu_addr=%"PRIpaddr"\n"
>                "        gic_maintenance_irq=%u\n",
> -              gicv2.dbase, gicv2.cbase, hbase, gicv2.vbase,
> +              gicv2_info.dbase, gicv2_info.cbase, hbase, gicv2_info.vbase,
>                gicv2_info.maintenance_irq);
>  
> -    if ( (gicv2.dbase & ~PAGE_MASK) || (gicv2.cbase & ~PAGE_MASK) ||
> -         (hbase & ~PAGE_MASK) || (gicv2.vbase & ~PAGE_MASK) )
> +    if ( (gicv2_info.dbase & ~PAGE_MASK) || (gicv2_info.cbase & ~PAGE_MASK) ||
> +         (hbase & ~PAGE_MASK) || (gicv2_info.vbase & ~PAGE_MASK) )
>          panic("GICv2 interfaces not page aligned");
>  
> -    gicv2.map_dbase = ioremap_nocache(gicv2.dbase, SZ_4K);
> +    gicv2.map_dbase = ioremap_nocache(gicv2_info.dbase, SZ_4K);
>      if ( !gicv2.map_dbase )
>          panic("GICv2: Failed to ioremap for GIC distributor\n");
>  
> -    gicv2.map_cbase[0] = ioremap_nocache(gicv2.cbase, SZ_4K);
> +    gicv2.map_cbase[0] = ioremap_nocache(gicv2_info.cbase, SZ_4K);
>  
>      if ( platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
> -        gicv2.map_cbase[1] = ioremap_nocache(gicv2.cbase + SZ_64K, SZ_4K);
> +        gicv2.map_cbase[1] = ioremap_nocache(gicv2_info.cbase + SZ_64K, SZ_4K);
>      else
> -        gicv2.map_cbase[1] = ioremap_nocache(gicv2.cbase + SZ_4K, SZ_4K);
> +        gicv2.map_cbase[1] = ioremap_nocache(gicv2_info.cbase + SZ_4K, SZ_4K);
>  
>      if ( !gicv2.map_cbase[0] || !gicv2.map_cbase[1] )
>          panic("GICv2: Failed to ioremap for GIC CPU interface\n");
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index ef4bf9a..1e569a0 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -290,6 +290,12 @@ struct gic_info {
>      unsigned int maintenance_irq;
>      /* Pointer to the device tree node representing the interrupt controller */
>      const struct dt_device_node *node;
> +    /* Distributor interface address */
> +    paddr_t dbase;
> +    /* CPU interface address (GICv2 compatible only) */
> +    paddr_t cbase;
> +    /* Virtual CPU interface address (GICv2 compatible only) */
> +    paddr_t vbase;
>  };
>  
>  struct gic_hw_operations {