Re: [Xen-devel] [PATCH v5 19/22] xen/arm: ITS: Add APIs to add and assign device

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Vijay,

On 27/07/2015 04:12, vijay.kilari@xxxxxxxxx wrote:
> +    for ( i = 0; i < dev->nr_lpis; i++ )
> +    {
> +        msi = xzalloc(struct msi_desc);
> +        /* Reserve pLPI */
> +        if ( its_alloc_device_irq(dev, &plpi) || !msi )
> +        {
> +            its_discard_lpis(dev, i);
> +            its_lpi_free(dev);
> +            its_send_mapd(dev, 0);
> +            its_free_device(dev);
> +            dprintk(XENLOG_G_ERR,"ITS: Cannot add device 0x%"PRIx32"\n", 
> devid);
> +            res = -ENOSPC;
> +            goto err;
> +        }
> +
> +        /* For each pLPI send MAPVI command */
> +        col = &dev->its->collections[(i % nr_cpu_ids)];
> +        /* Store collection for this plpi in irq_desc */
> +        desc = irq_to_desc(plpi);
> +        spin_lock(&desc->lock);
> +        desc->msi_desc = msi;
It's rather strange to directly use msi_desc field here when you 
introduced helper for all the other fields.
As I said on a previous mail, I would prefer to see an helper to set the 
msi_desc and avoid introducing ITS specific helpers for all the other 
fields in irq.c.
> +        set_lpi_event(desc, i);
> +        set_irq_its_device(desc, dev);
> +        irqdesc_set_collection(desc, col->col_id);
Please be consistent with the name. 3 helpers doing the same (setting 
fields in MSI), 3 completely different naming...
> +        spin_unlock(&desc->lock);
> +        its_send_mapvi(dev, col, plpi, i);
> +    }
> +
> +err:
> +    spin_unlock(&rb_its_dev_lock);
> +
> +    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 %"PRId32"\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);
> +
> +    pdev->domain_id = d->domain_id;
> +    pdev->virt_device_id = vdevid;
> +
> +    DPRINTK("ITS: Assign pdevid 0x%"PRIx32" lpis %"PRId32" for dom 
> %"PRId32"\n",
> +            pdevid, pdev->nr_lpis, d->domain_id);
> +
> +    for ( i = 0; i < pdev->nr_lpis; i++ )
> +    {
> +        plpi = its_get_plpi(pdev, i);
> +        /* TODO: Route lpi */
Seriously? Why did you drop the code since v4 here?

We are at v5, I'm expecting to see this series working if I'm applying 
to my tree. Without the routing, I don't see how PCI can work with ITS 
on DOM0... You didn't even mention it in the cover letter!
You should test your series without any additional patch on upstream Xen 
before sending it.
Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel