Re: [PATCH v6 19/31] xen/arm: ITS: Export ITS info to Virtual ITS

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

On 31/08/15 12:06, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Export physical ITS information to virtual ITS driver
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
> v6: - Passed only one physical ITS info
>     - Passed all the values as parameters
>     - Initialize vITS only if physical ITS is available
> ---
>  xen/arch/arm/gic-v3-its.c     |   11 +++++++++++
>  xen/arch/arm/vgic-v3-its.c    |   28 ++++++++++++++++++++++++++++
>  xen/include/asm-arm/gic-its.h |    7 +++++++
>  3 files changed, 46 insertions(+)
> 
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 0865a93..77abbc6 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c

[...]

> @@ -1402,6 +1405,7 @@ int its_cpu_init(void)
>  
>  int __init its_init(struct rdist_prop *rdists)
>  {
> +    struct its_node *its;
>      struct dt_device_node *np = NULL;
>  
>      static const struct dt_device_match its_device_ids[] __initconst =
> @@ -1424,6 +1428,13 @@ int __init its_init(struct rdist_prop *rdists)
>      its_alloc_lpi_tables();
>      its_lpi_init(rdists->id_bits);
>  
> +    its = list_first_entry(&its_nodes, struct its_node, entry);
> +    if ( !its )
> +        return -ENOMEM;

This check is not necessary, you already check is the its_nodes list is
not empty.

> +
> +    vits_setup_hw(its_data.dev_bits, its_data.eventid_bits,
> +                  its->phys_base, its->phys_size);

It would have been nice to have a comment explaining that we always
expose a single ITS to DOM0.

> +
>      return 0;
>  }
>  
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index cef6139..53f2a27 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -68,6 +68,26 @@ static inline uint16_t vits_get_max_collections(struct domain *d)
>      return (d->max_vcpus + 1);
>  }
>  
> +static struct {
> +    bool_t enabled;
> +    uint32_t dev_bits;
> +    uint32_t eventid_bits;


Please have consistent name. I.e

eventID_bits
devID_bits

Also, those fields can be stored each on uint8_t (both are only 4 bits).

> +    /* GITS physical base */
> +    paddr_t phys_base;
> +    /* GITS physical size */
> +    unsigned long phys_size;
> +} vits_hw;
> +
> +void vits_setup_hw(uint32_t dev_bits, uint32_t eventid_bits,
> +                   paddr_t phys_base, unsigned long phys_size)
> +{
> +    vits_hw.enabled = 1;
> +    vits_hw.dev_bits = dev_bits;
> +    vits_hw.eventid_bits = eventid_bits;
> +    vits_hw.phys_base = phys_base;
> +    vits_hw.phys_size = phys_size;
> +}
> +
>  int vits_access_guest_table(struct domain *d, paddr_t entry, void *addr,
>                              uint32_t size, bool_t set)
>  {
> @@ -547,6 +567,14 @@ int vits_domain_init(struct domain *d)
>  
>      ASSERT(is_hardware_domain(d));
>  
> +    if ( !vits_hw.enabled )

An ASSERT would have been enough given that this should never be called
when LPIs is not supported and ITS not enabled (see caller).

> +    {
> +        printk(XENLOG_G_ERR
> +               "%"PRIu16": VITS is not supported on this platform.\n",

s/PRIu16/u/ to be consistent with the rest of Xen.

> +               d->domain_id);
> +        return -ENODEV;
> +    }
> +        
>      d->arch.vgic.nr_lpis = nr_lpis;
>  
>      d->arch.vgic.vits = xzalloc(struct vgic_its);
> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
> index 4327ba2..7077477 100644
> --- a/xen/include/asm-arm/gic-its.h
> +++ b/xen/include/asm-arm/gic-its.h
> @@ -320,6 +320,11 @@ struct vitt {
>      uint32_t vlpi;
>  };
>  
> +struct gic_its_info {
> +    uint32_t eventid_bits;
> +    uint32_t dev_bits;

See my remark on the vits_hw.

> +};
> +

This is only used internally to gic-v3-its.c. So you could have directly
done

static struct
{
	uint32_t eventid_bits;
	uint32_t dev_bits;
} its_data;

>  void irqdesc_set_lpi_event(struct irq_desc *desc, unsigned id);
>  unsigned int irqdesc_get_lpi_event(struct irq_desc *desc);
>  struct its_device *irqdesc_get_its_device(struct irq_desc *desc);
> @@ -337,6 +342,8 @@ int vits_get_vitt_entry(struct domain *d, uint32_t devid,
>                          uint32_t event, struct vitt *entry);
>  int vits_get_vdevice_entry(struct domain *d, uint32_t devid,
>                             struct vdevice_table *entry);
> +void vits_setup_hw(uint32_t dev_bits, uint32_t eventid_bits,
> +                   paddr_t base, unsigned long size);
>  
>  #endif /* __ASM_ARM_GIC_ITS_H__ */
>  /*
> 

Regards,

-- 
Julien Grall