Re: [RFC PATCH v3 04/18] xen/arm: gicv3: Refactor redistributor information

[Julien Grall <julien.grall@citrix.com>]

Hi,

On 22/06/15 13:01, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Separate redistributor information into rdist and rdist_prop
> structures.
> 
> The rdist_prop holds the redistributor common information
> and rdist holds the per cpu specific information.
> 
> This percpu rdist defined as global and shared with ITS
> driver

This patch does more than refactoring, you are adding new fields which
are not use here.

Please explain why you need them or move them in the patch where they
are used. Comment on the usage of the field would be nice too.

FWIW, I would be in favor of the later.

> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
>  xen/arch/arm/gic-v3.c             |   15 ++++++++++-----
>  xen/include/asm-arm/gic_v3_defs.h |   15 +++++++++++++++
>  2 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 30682cf..b5c59f6 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -53,6 +53,7 @@ static struct {
>      paddr_t dbase;            /* Address of distributor registers */
>      paddr_t dbase_size;
>      void __iomem *map_dbase;  /* Mapped address of distributor registers */
> +    struct rdist_prop rdist_data;
>      struct rdist_region *rdist_regions;
>      uint32_t  rdist_stride;
>      unsigned int rdist_count; /* Number of rdist regions count */
> @@ -63,10 +64,10 @@ static struct {
>  static struct gic_info gicv3_info;
>  
>  /* per-cpu re-distributor base */
> -static DEFINE_PER_CPU(void __iomem*, rbase);
> +DEFINE_PER_CPU(struct rdist, rdist);
>  
>  #define GICD                   (gicv3.map_dbase)
> -#define GICD_RDIST_BASE        (this_cpu(rbase))
> +#define GICD_RDIST_BASE        (per_cpu(rdist, smp_processor_id()).rbase)
>  #define GICD_RDIST_SGI_BASE    (GICD_RDIST_BASE + SZ_64K)
>  
>  /*
> @@ -613,6 +614,7 @@ static int __init gicv3_populate_rdist(void)
>      uint32_t aff;
>      uint32_t reg;
>      uint64_t typer;
> +    uint64_t offset;
>      uint64_t mpidr = cpu_logical_map(smp_processor_id());
>  
>      /*
> @@ -648,9 +650,12 @@ static int __init gicv3_populate_rdist(void)
>  
>              if ( (typer >> 32) == aff )
>              {
> -                this_cpu(rbase) = ptr;
> -                printk("GICv3: CPU%d: Found redistributor in region %d @%p\n",
> -                        smp_processor_id(), i, ptr);
> +                offset = ptr - gicv3.rdist_regions[i].map_base;
> +                per_cpu(rdist, smp_processor_id()).rbase = ptr;
> +                per_cpu(rdist, smp_processor_id()).phys_base =  gicv3.rdist_regions[i].base + offset;
> +                printk("GICv3: CPU%d: Found redistributor in region %d @%"PRIpaddr"\n",
> +                        smp_processor_id(), i,
> +                        per_cpu(rdist, smp_processor_id()).phys_base);
>                  return 0;
>              }
>              if ( gicv3.rdist_stride )
> diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
> index 556f114..acbb906 100644
> --- a/xen/include/asm-arm/gic_v3_defs.h
> +++ b/xen/include/asm-arm/gic_v3_defs.h
> @@ -152,6 +152,21 @@
>  #define ICH_SGI_IRQ_SHIFT            24
>  #define ICH_SGI_IRQ_MASK             0xf
>  #define ICH_SGI_TARGETLIST_MASK      0xffff
> +
> +struct rdist {
> +    void __iomem *rbase;
> +    void * pend_page;

void *pend_page;

> +    paddr_t phys_base;
> +};
> +
> +struct rdist_prop {

What does "prop" stand for?

> +    void * prop_page;

Ditto.

> +    int    id_bits;
> +    uint64_t flags;
> +};
> +
> +DECLARE_PER_CPU(struct rdist, rdist);
> +
>  #endif /* __ASM_ARM_GIC_V3_DEFS_H__ */
>  
>  /*
> 

Regards,


-- 
Julien Grall