Re: [PATCH v4 08/17] xen/arm: ITS: Add APIs to add and assign device

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

On Fri, 2015-07-10 at 13:12 +0530, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Add APIs to add devices to RB-tree, assign and remove
> devices to domain.
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
> v4: - Introduced helper to populate its_device struct
>     - Fixed freeing of its_device memory
>     - its_device struct holds domain id
> ---
>  xen/arch/arm/gic-v3-its.c     |  362 +++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/irq.c            |    8 +
>  xen/include/asm-arm/gic-its.h |   18 ++
>  xen/include/asm-arm/irq.h     |    1 +
>  4 files changed, 389 insertions(+)
> 
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 9161053..1d2fdde 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -92,6 +92,7 @@ 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()    (per_cpu(rdist, smp_processor_id()))
>  
> @@ -108,6 +109,14 @@ u32 its_get_nr_events(void)
>      return (1 << its_data.id_bits);
>  }
>  
> +static struct its_node * its_get_phys_node(u32 dev_id)
> +{
> +    /* TODO: For now return ITS0 node.
> +     * Need Query PCI helper function to get on which
> +     * ITS node the device is attached
> +     */
> +    return list_first_entry(&its_nodes, struct its_node, entry);
> +}
>  /* RB-tree helpers for its_device */
>  struct its_device *its_find_device(u32 devid)
>  {
> @@ -314,6 +323,30 @@ static void its_send_inv(struct its_device *dev, struct its_collection *col,
>      its_send_single_command(dev->its, &cmd, col);
>  }
>  
> +static void its_send_mapd(struct its_device *dev, int valid)

Some of the decisions made wrt what gets introduced in what patch and in
what order make this series rather hard to follow. i.e. why wasn't this
added along with all the others.

> [...] 
> +static int its_chunk_to_lpi(int chunk)
> +{
> +    return (chunk << IRQS_PER_CHUNK_SHIFT) + 8192;
> +}
[...]
> +static unsigned long *its_lpi_alloc_chunks(int nirqs, int *base, int *nr_ids)

And why wasn't all this in the earlier patch which included the comments
about the lpi allocation chunking strategy.

Oh well.

> +static struct its_device *its_alloc_device(u32 devid)
> +{
> +    struct its_device *dev;
> +    paddr_t *itt;
> +    unsigned long *lpi_map;
> +    int lpi_base, nr_lpis, sz;
> +    u32 nr_ites;
> +
> +    dev = xzalloc(struct its_device);
> +    if ( dev == NULL )
> +        return NULL;
> +
> +    dev->its = its_get_phys_node(devid);
> +    /* TODO: Use pci helper to get nvecs */
> +    nr_ites = 64;

Please add nr_ites as a parameter to this function and to
its_add_device, such that this hardcoding can be pushed all the way down
into the final patch which adds the temporary registration code in
xen/arch/arm/platforms/thunderx.c.

> +int its_assign_device(struct domain *d, u32 vdevid, u32 pdevid)
> +{
> +    struct its_device *pdev;
> +    struct vits_device *vdev;
> +    struct irq_desc *desc;
> +    u32 plpi, i;
> +
> +    DPRINTK("%pv: ITS: Assign request for device 0x%x to domain %d\n",
> +            current, vdevid, d->domain_id);
> +
> +    spin_lock(&d->arch.vits->dev_lock);
> +    vdev = vits_find_device(&d->arch.vits->dev_root, vdevid);
> +    if ( vdev )
> +    {
> +        spin_unlock(&d->arch.vits->dev_lock);
> +        return -EEXIST;
> +    }

This just checks that it isn't assigned to the current domain AFAICT.
Which is insufficient, since it might be assigned to some other device.

I think you need to check some field of pdev, probably pdev->domain_id,
instead.

BTW, I would expect the struct domain * to be of more use than the domid
in the general case and then NULL would serve as a reasonable "not
assigned" flag.

> +int its_detach_device(struct domain *d, u32 vdevid, u32 pdevid)
> +{
> +    struct its_device *pdev;
> +    struct vits_device *vdev;
> +    struct irq_desc *desc;
> +    u32 plpi, i;
> +
> +    DPRINTK("%pv: ITS: Detach request for device 0x%x domain %d\n",
> +            current, pdevid, d->domain_id);
> +
> +    spin_lock(&d->arch.vits->dev_lock);
> +    vdev = vits_find_device(&d->arch.vits->dev_root, vdevid);
> +    if ( !vdev )
> +    {
> +        spin_unlock(&d->arch.vits->dev_lock);
> +        return -EINVAL;
> +    }
> +
> +    pdev = vdev->its_dev;

You can lookup pdev via the pdevid, no need for the vdev.

As a sanity check you could ensure that pdev->virt_device_id == vdevid.

> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
> index a143003..f041efc 100644
> --- a/xen/include/asm-arm/gic-its.h
> +++ b/xen/include/asm-arm/gic-its.h
> @@ -53,6 +53,8 @@ struct vgic_its
>     uint64_t dt_size;
>     /* Radix-tree root of devices attached to this domain */
>     struct rb_root dev_root;
> +   /* Lock to manange virtual devices in rb-tree*/

"manage".

Perhaps having gotten rid of the rb-tree this won't be needed any
longer, or maybe it also protects other stuff?

> +   spinlock_t dev_lock;
>     /* collections mapped */
>     struct its_collection *collections;
>  };
> @@ -189,10 +191,24 @@ typedef union {
>  struct its_device {
>      /* Physical ITS */
>      struct its_node         *its;
> +    /* Device ITT address */
> +    paddr_t                 *itt_addr;
> +    /* Device ITT size */
> +    unsigned long           itt_size;
> +    /* Physical LPI map */
> +    unsigned long           *lpi_map;
> +    /* First Physical LPI number assigned */
> +    u32                     lpi_base;
>      /* Number of Physical LPIs assigned */
>      int                     nr_lpis;
> +    /* Number of ITES entries */

"Number of IT entries" or "Number of ITES" I think?

Ian.