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

[Vijay Kilari <vijay.kilari@gmail.com>]

On Tue, Jul 28, 2015 at 11:44 PM, Julien Grall <julien.grall@citrix.com> wrote:
> 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);
>

   Passing every value as argument makes this function take too many arguments.
Validation can be done member variable to catch errors.

>>      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