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

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

Hi Vijay,

On 18/09/15 14:08, 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>
> ---
> v7: - Added check for domain in its_assign_device() to avoid
>       assigning device to DomU
>     - Added comments whereever requested
>     - Called its_remove_device to remove from rb-tree
>       when failed to add device
>     - Changed dprintk to printk
>     - Store domain pointer instead of domain id in its_device struct
>     - Free msi_desc and reset irq_desc when lpis are discarded
>     - Add function its_free_msi_descs() to free msi_desc
> v6: - Moved this patch #19 to patch #8
>     - Used col_map to store collection id
>     - Use helper functions to update msi_desc members
> v5: - Removed its_detach_device API
>     - Pass nr_ites as parameter to its_add_device
> 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     |  261 +++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/gic-its.h |    6 +
>  2 files changed, 267 insertions(+)
> 
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 4b7d9ed..bc3b73c 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -145,6 +145,19 @@ static struct its_collection *dev_event_to_col(struct its_device *dev,
>      return its->collections + dev->event_map.col_map[event];
>  }
>  
> +static struct its_node *its_get_phys_node(struct dt_device_node *dt)
> +{
> +    struct its_node *its;
> +
> +    list_for_each_entry(its, &its_nodes, entry)
> +    {
> +        if ( its->dt_node == dt )
> +            return its;
> +    }
> +
> +    return NULL;
> +}
> +
>  /* RB-tree helpers for its_device */
>  static struct its_device *its_find_device(u32 devid)
>  {
> @@ -537,8 +550,256 @@ static void its_lpi_free(struct its_device *dev)
>      }
>  
>      spin_unlock(&lpi_lock);
> +}
> +
> +static void its_discard_lpis(struct its_device *dev, u32 ids)
> +{
> +    int i;

u32 i;

> +
> +    for ( i = 0; i < ids; i++ )
> +       its_send_discard(dev, i);
> +}
> +
> +static void its_free_msi_descs(struct its_device *dev, int count)

its_free_msi_descs can be merge with its_discard_lpis. It would avoid
looping twice which can be expensive with device having a lot of MSI and
make clear the dependency.

For instance its_free_msi_descs should never be called before
its_discard_lpis.

> +{
> +    int i, irq;

Why do you need them signed? Same for the parameter "count".

> +    struct irq_desc *desc;
> +
> +    for ( i = 0; i < count; i++ )
> +    {
> +       irq = dev->event_map.lpi_base + i;

You could have use its_get_plpi here.

> +       desc = irq_to_desc(irq);
> +
> +       spin_lock(&desc->lock);
> +       irqdesc_set_lpi_event(desc, 0);
> +       irqdesc_set_its_device(desc, NULL);

Why do you need these 2 calls?

> +       xfree(irq_get_msi_desc(desc));
> +       irq_set_msi_desc(desc, NULL);

After this, the IRQ desc is still left in an unknown state (give a look
to gic_remove_irq_from_guest).

Although, it may not need necessary for now. So I would be fine with a
TODO in the code.

> +       spin_unlock(&desc->lock);
> +    }
> +}
> +
> +static inline u32 its_get_plpi(struct its_device *dev, u32 event)
> +{
> +    return dev->event_map.lpi_base + event;
> +}
> +
> +static int its_alloc_device_irq(struct its_device *dev, u32 *hwirq)
> +{
> +    int idx;
> +
> +    idx = find_first_zero_bit(dev->event_map.lpi_map, dev->event_map.nr_lpis);
> +    if ( idx == dev->event_map.nr_lpis )
> +        return -ENOSPC;
> +
> +    *hwirq = its_get_plpi(dev, idx);
> +    set_bit(idx, dev->event_map.lpi_map);
>  
> +    return 0;
> +}
> +
> +static void its_free_device(struct its_device *dev)
> +{
> +    xfree(dev->itt);
> +    its_lpi_free(dev);
>      xfree(dev->event_map.lpi_map);

Why did you move this call from its_lpi_free to here?

> +    xfree(dev->event_map.col_map);

This line should have been added in its_lpi_free in patch #5.

> +    xfree(dev);
> +}
> +
> +static struct its_device *its_alloc_device(u32 devid, u32 nr_ites,
> +                                           struct dt_device_node *dt_its)

Can you explain why you weren't able to re-use its_create_device from Linux?

AFAICT there is nothing different and you miss key code such as rounding
to a power of 2 the number of event IDs.

> +{
> +    struct its_device *dev;
> +    unsigned long *lpi_map;
> +    int lpi_base, sz;
> +    u16 *col_map = NULL;
> +
> +    dev = xzalloc(struct its_device);
> +    if ( dev == NULL )
> +        return NULL;
> +
> +    dev->its = its_get_phys_node(dt_its);

You can re-order the code to get the ITS before allocate the memory. And
then you can drop one goto.

> +    if (dev->its == NULL)
> +    {
> +        printk(XENLOG_G_ERR

As already asked on v6, why XENLOG_G_ERR? Any call to this function will
be done via privileged guest.

> +               "ITS: Failed to find ITS node for devid 0x%"PRIx32"\n", devid);
> +        goto err;
> +    }
> +
> +    sz = nr_ites * dev->its->ite_size;
> +    sz = max(sz, ITS_ITT_ALIGN) + ITS_ITT_ALIGN - 1;
> +    dev->itt = xzalloc_bytes(sz);
> +    if ( !dev->itt )
> +        goto err;
> +
> +    lpi_map = its_lpi_alloc_chunks(nr_ites, &lpi_base);
> +    if ( !lpi_map )
> +        goto lpi_err;
> +
> +    col_map = xzalloc_bytes(sizeof(*col_map) * nr_ites);
> +    if ( !col_map )
> +        goto col_err;
> +    dev->event_map.lpi_map = lpi_map;
> +    dev->event_map.lpi_base = lpi_base;
> +    dev->event_map.col_map = col_map;
> +    dev->event_map.nr_lpis = nr_ites;
> +    dev->device_id = devid;
> +
> +    return dev;
> +
> +col_err:
> +    its_free_device(dev);
> +    return NULL;
> +lpi_err:
> +    xfree(dev->itt);
> +err:
> +    xfree(dev);
> +
> +    return NULL;
> +}
> +
> +/* Device assignment */
> +int its_add_device(u32 devid, u32 nr_ites, struct dt_device_node *dt_its)
> +{
> +    struct its_device *dev;
> +    u32 i, plpi = 0;
> +    struct its_collection *col;
> +    struct irq_desc *desc;
> +    struct msi_desc *msi = NULL;
> +    int res = 0;
> +
> +    spin_lock(&rb_its_dev_lock);
> +    dev = its_find_device(devid);
> +    if ( dev )
> +    {
> +        printk(XENLOG_G_ERR "ITS: Device already exists 0x%"PRIx32"\n",

Same question as before for XENLOG_G_ERR.

> +               dev->device_id);
> +        res = -EEXIST;
> +        goto err_unlock;
> +    }
> +
> +    dev = its_alloc_device(devid, nr_ites, dt_its);
> +    if ( !dev )
> +    {
> +        res = -ENOMEM;
> +        goto err_unlock;
> +    }
> +
> +    if ( its_insert_device(dev) )

The only way that this call can fail is because the device already
exists in the RB-tree. Although, this can never happen because you have
the lock taken and check beforehand if someone has inserted a device
with this devID.

So I would turn this if into an BUG_ON(its_insert_device(dev)) and save
6 lines.

> +    {
> +        its_free_device(dev);
> +        printk(XENLOG_G_ERR "ITS: Failed to insert device 0x%"PRIx32"\n", devid);
> +        res = -EINVAL;
> +        goto err_unlock;
> +    }
> +    /* Assign device to dom0 by default */

This wasn't present on the previous version. Why do you need that? DOM0
is just a guest and you make the code in the assignation function (see
its_assign_device) for nothing.

IHMO, the code to add a device and assign to a guest should be
completely separate.

> +    dev->domain = hardware_domain;
> +    spin_unlock(&rb_its_dev_lock);
> +
> +    DPRINTK("ITS:Add Device 0x%"PRIx32" lpis %"PRIu32" base 0x%"PRIx32"\n",
> +            dev->device_id, dev->event_map.nr_lpis, dev->event_map.lpi_base);
> +
> +    /* Map device to ITS ITT */
> +    its_send_mapd(dev, 1);
> +
> +    for ( i = 0; i < dev->event_map.nr_lpis; i++ )
> +    {
> +        msi = xzalloc(struct msi_desc);
> +        if ( its_alloc_device_irq(dev, &plpi) || !msi )
> +        {
> +            /* Discard LPIs and free device on failure to allocate pLPI */
> +            its_discard_lpis(dev, i);
> +            its_free_msi_descs(dev, i);
> +            its_send_mapd(dev, 0);
> +
> +            spin_lock(&rb_its_dev_lock);
> +            its_remove_device(dev);
> +            spin_unlock(&rb_its_dev_lock);
> +
> +            its_free_device(dev);
> +
> +            printk(XENLOG_G_ERR "ITS: Cannot add device 0x%"PRIx32"\n", devid);

Same question as before for XENLOG_G_ERR.

> +            res = -ENOSPC;
> +            goto err;
> +        }
> +
> +        /*
> +         * Each Collection is mapped to one physical CPU and
> +         * each pLPI allocated to this device is mapped one collection
> +         * in a roundrobin fashion. Hence all pLPIs are distributed

s/roundrobin/round robin/

> +         * across all processors in the system.
> +         */
> +        col = &dev->its->collections[(i % nr_cpu_ids)];

Your round robin is done per device. So if we have 10 devices with a
single event ID, all the LPIs will be routed to CPU0.

I guess this is fine for now, but it worth mentioning it in the comment.

> +        desc = irq_to_desc(plpi);
> +
> +        spin_lock(&desc->lock);
> +        dev->event_map.col_map[i] = col->col_id;
> +        irq_set_msi_desc(desc, msi);
> +        irqdesc_set_lpi_event(desc, i);
> +        irqdesc_set_its_device(desc, dev);

While I gave my reviewed-by on the patch with add those helpers (see
#7), I'm continuing to think that they are not useful and make the usage
more difficult due the ordering requirement.

Something like below would have been better:

msi->eventID = i;
msi->dev = dev;
irq_set_msi_desc(msi);

This will also turn 5 functions call (2 for each irqdesc_* + 1 for
irq_set_msi_*) into a single one.

This code is still ok, but it would make a different in the LPI handling
code later.

> +        spin_unlock(&desc->lock);
> +
> +        /* For each pLPI send MAPVI command */
> +        its_send_mapvi(dev, plpi, i);
> +    }
> +
> +    return 0;
> +
> +err_unlock:
> +    spin_unlock(&rb_its_dev_lock);
> +err:
> +    return res;
> +}
> +
> +int its_assign_device(struct domain *d, u32 vdevid, u32 pdevid)
> +{
> +    struct its_device *pdev;
> +    u32 plpi, i;
> +
> +    DPRINTK("ITS: Assign request for dev 0x%"PRIx32" to domain %"PRIu16"\n",
> +            vdevid, d->domain_id);
> +
> +    spin_lock(&rb_its_dev_lock);
> +    pdev = its_find_device(pdevid);
> +    if ( !pdev )
> +    {
> +        spin_unlock(&rb_its_dev_lock);
> +        return -ENODEV;
> +    }
> +    spin_unlock(&rb_its_dev_lock);

You can rework this code to avoid 2 spin_unlock:

spin_lock(&rb...)
pdev = its_find_device(pdevid);
spin_unlock(&rb...)

if ( !pdev )
  return -ENODEV;

> +
> +    /*
> +     * TODO: For pass-through following has to be implemented
> +     * 1) Allow device to be assigned to other domains (Dom0 -> DomU).
> +     * 2) Allow device to be re-assigned to Dom0 (DomU -> Dom0).
> +     * Implement separate function to handle this or rework this function.
> +     * For now do not allow assigning devices other than Dom0.
> +     *
> +     * By default all devices are assigned to Dom0.

See my question above about the "why?".

> +     * Device should be with Dom0 before assigning to other domain.
> +     */
> +    if ( !is_hardware_domain(d) || !is_hardware_domain(pdev->domain) )
> +    {
> +        printk(XENLOG_ERR
> +               "ITS: PCI-Passthrough not supported!! to assign from d%d to d%d",
> +               pdev->domain->domain_id, d->domain_id);
> +        return -ENXIO;
> +    }
> +
> +    pdev->domain = d;
> +    pdev->virt_device_id = vdevid;
> +
> +    DPRINTK("ITS: Assign pdevid 0x%"PRIx32" lpis %"PRIu32" for dom %"PRIu16"\n",
> +            pdevid, pdev->event_map.nr_lpis, d->domain_id);
> +
> +    for ( i = 0; i < pdev->event_map.nr_lpis; i++ )
> +    {
> +        plpi = its_get_plpi(pdev, i);
> +        /* TODO: Route lpi */
> +    }
> +
> +    return 0;
>  }

Regards,

-- 
Julien Grall