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

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

Hi Vijay,

On 10/07/2015 09:42, vijay.kilari@gmail.com wrote:
> +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;
> +    sz = nr_ites * dev->its->ite_size;
> +    sz = max(sz, ITS_ITT_ALIGN) + ITS_ITT_ALIGN - 1;
> +    itt = xzalloc_bytes(sz);
> +    if ( !itt )
> +        goto err;
> +
> +    lpi_map = its_lpi_alloc_chunks(nr_ites, &lpi_base, &nr_lpis);
> +    if ( !lpi_map || (nr_lpis < nr_ites) )
> +        goto lpi_err;

The check "nr_lpis  !=  nr_ites" is here to ensure that
its_lpi_alloc_chuncks allocate the right number of LPI, right? If so,
given this is the only place you call its_lpi_alloc_chunks, why don't
you put the check within the function? It would avoid one parameter.

> +
> +    dev->itt_addr = itt;
> +    dev->nr_ites = nr_ites;
> +    dev->lpi_map = lpi_map;
> +    dev->lpi_base = lpi_base;
> +    dev->nr_lpis = nr_lpis;
> +    dev->device_id = devid;
> +
> +    return dev;
> +
> +lpi_err:
> +    xfree(itt);
> +    xfree(lpi_map);
> +err:
> +    xfree(dev);
> +
> +    return NULL;
> +}
> +
> +/* Device assignment. Should be called from PHYSDEVOPS_pci_device_add */

This comment is wrong. It's not device assignment but making aware Xen 
that this device exists. Also, I would drop the second part of the 
comment as it may be call from other place than PHYSDEVOP_pci_device_add.

> +int its_add_device(u32 devid)

[...]

> +    /* Map device to its ITT */
> +    its_send_mapd(dev, 1);
> +
> +    /* TODO: Use nr_cpu_ids? */

I though you fixed nr_cpu_ids? If not can you please do it and use this 
value here.

> +    nr_cpus = num_online_cpus();
> +    for ( i = 0; i < dev->nr_lpis; i++ )
> +    {
> +        /* Reserve pLPI */
> +        if ( its_alloc_device_irq(dev, &plpi) )
> +        {
> +            /* Cannot revert MAPVI */

Why not? The invert of MAPVI is DISCARD.

[...]

> +int its_assign_device(struct domain *d, u32 vdevid, u32 pdevid)

[...]

> +    for ( i = 0; i < pdev->nr_lpis; i++ )
> +    {
> +        plpi = its_get_plpi(pdev, i);
> +        route_irq_to_guest(d, i, plpi, "LPI");
> +        desc = irq_to_desc(plpi);
> +        spin_lock(&desc->lock);
> +        set_irq_device(desc, pdev);

This should be part of its_add_device and not its_assign_device.

> +        lpi_set_config(desc, 1);

The goal of route_irq_to_guest is to setup everything in order to route 
correctly the IRQ (here LPI) to the guest.

As said on v3, if the current function doesn't fit you need, please 
introduce a new one.

In this case, lpi_set_config is used to configure the IRQ (i.e set 
property ...). It's likely the same as gic_set_irq_properties.

Implementing there would avoid the is_lpi you added in this function.

Furthermore, enabling the interrupt here is very fragile. As soon as the 
LPI is enabled for the given device, he can send an interrupt. See the 
thread on the draft F [1] for more details.

I don't think this is really important to have it until PCI passthrough 
is added. But at least a comment would be nice.

> +        spin_unlock(&desc->lock);
> +    }
> +
> +    return 0;
> +}
> +
> +int its_detach_device(struct domain *d, u32 vdevid, u32 pdevid)
> +{

I would prefer if you don't introduce its_detach_device until you used 
when PCI passthrough will be added.

> +    for ( i = 0; i < pdev->nr_lpis; i++ )
> +    {
> +        plpi = its_get_plpi(pdev, i);
> +        desc = irq_to_desc(plpi);
> +        spin_lock(&desc->lock);
> +        lpi_set_config(desc, 0);
> +        set_irq_device(desc, NULL);
> +        /* TODO: Fix for lpi */

This would avoid take spend time on reviewing if everything is correctly 
done when you release an IRQ. Which doesn't seem to be the case based on 
the TODO.

> +        release_irq(plpi, d);

release_irq is for IRQ assigned to Xen and not guest IRQ. You would have 
to use release_guest_irq here. Furthermore, there will be some extra 
care to do when a domain crashed...

> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index ba8528a..3806d98 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -181,6 +181,14 @@ u16 gic_get_irq_collection(unsigned int irq)
>       return desc->arch.col_id;
>   }
>
> +void gic_set_irq_collection(unsigned int irq, u16 col_id)
> +{
> +    struct irq_desc *desc = irq_to_desc(irq);
> +
> +    ASSERT(spin_is_locked(&desc->lock));
> +    desc->arch.col_id = col_id;
> +}
> +

This function is not a GIC function but a function which handle 
irq_desc. Please name accordingly i.e irqdesc_set_collection.

Furthermore, given the size of the function, an inline function would 
have been better.

Finally, I think the function can directly get an irq_desc in parameter.

My remarks are the same for gic_get_irq_collection (from patch #5) which 
I didn't spot before.

Regards,

[1] http://lists.xen.org/archives/html/xen-devel/2015-06/msg02591.html

-- 
Julien Grall