Re: [PATCH v5 09/22] xen/arm: ITS: Export ITS info to Virtual ITS

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

Hi Vijay,

On 27/07/15 12:11, vijay.kilari@gmail.com wrote:
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index aa4d3c5..e16fa03 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -94,6 +94,7 @@ static LIST_HEAD(its_nodes);
>  static DEFINE_SPINLOCK(its_lock);
>  static struct rdist_prop  *gic_rdists;
>  static struct rb_root rb_its_dev;
> +static struct gic_its_info its_data;
>  static DEFINE_SPINLOCK(rb_its_dev_lock);
>  
>  #define gic_data_rdist()    (this_cpu(rdist))
> @@ -942,6 +943,8 @@ static int its_probe(struct dt_device_node *node)
>      its->phys_size = its_size;
>      typer = readl_relaxed(its_base + GITS_TYPER);
>      its->ite_size = ((typer >> 4) & 0xf) + 1;
> +    its_data.eventid_bits = GITS_TYPER_IDBITS(typer);
> +    its_data.dev_bits = GITS_TYPER_DEVBITS(typer);
>  
>      its->cmd_base = xzalloc_bytes(ITS_CMD_QUEUE_SZ);
>      if ( !its->cmd_base )
> @@ -1032,7 +1035,10 @@ int its_cpu_init(void)
>  
>  int __init its_init(struct rdist_prop *rdists)
>  {
> +    struct its_node *its;
> +    struct its_node_info *info;
>      struct dt_device_node *np = NULL;
> +    uint32_t i, nr_its = 0;
>  
>      static const struct dt_device_match its_device_ids[] __initconst =
>      {
> @@ -1042,7 +1048,10 @@ int __init its_init(struct rdist_prop *rdists)
>  
>      for (np = dt_find_matching_node(NULL, its_device_ids); np;
>               np = dt_find_matching_node(np, its_device_ids))
> -        its_probe(np);
> +    {
> +        if ( !its_probe(np) )
> +            nr_its++;
> +    }
>      if ( list_empty(&its_nodes) )
>      {
> @@ -1050,6 +1059,22 @@ int __init its_init(struct rdist_prop *rdists)
>          return -ENXIO;
>      }
>  
> +    info = xzalloc_array(struct its_node_info, nr_its);
> +    if ( !info )
> +        return -ENOMEM;
> +
> +    i = 0;
> +    list_for_each_entry(its, &its_nodes, entry)
> +    {
> +         info[i].phys_base = its->phys_base;
> +         info[i].phys_size = its->phys_size;
> +         i++;
> +    }
> +
> +    its_data.nr_its = nr_its;
> +    its_data.its_hw = info;
> +    vits_setup_hw(&its_data);
> +

The vITS will only expose 1 ITS to dom0. So there is no need to try to
allocate memory in order to expose all the ITS.

We can extend vits_setup_hw if we ever want to support more ITS in the
future.

Furthermore I'd like to see all the ITS data pass one by one rather than
in a structure. This would help to catch error if we ever extend the
structure.

Something like:

void vits_hw_setup(paddr_t its0_base, unsigned long its0_size,
                   uint32_t eventID_bits, uint32_t devID_bits);

>      gic_rdists = rdists;
>      its_lpi_init(rdists->id_bits);
>      its_alloc_lpi_tables();
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index dfa3435..3a003d4 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -51,6 +51,15 @@ static void dump_cmd(its_cmd_block *cmd)
>  static void dump_cmd(its_cmd_block *cmd) { do {} while ( 0 ); }
>  #endif
>  
> +static struct {
> +    struct gic_its_info *info;
> +} vits_hw;

Please add a boolean valid in the structure. It would help to know if
the vITS can be used or not later.

Regards,

-- 
Julien Grall