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

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Mon, Sep 21, 2015 at 7:17 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hi Vijay,
>
> On 18/09/15 14:08, vijay.kilari@xxxxxxxxx wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
>>
>> Add APIs to add devices to RB-tree, assign and remove
>> devices to domain.
>>
[...]
>> +
>> +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?

its_lpi_free() is called along with its_free_device. So moved into
this code instead of calling it separately. This helps to avoid
chances of missing it out.

>
>> +    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.
>

  It is almost re-used with following changes
1) Rounding of nr_ites, which is missing. However nr_ites is passed
from platform
    file.
2) Freeing of memory on failure. Here I re-used its_free_device() which does
    the same

>> +{
>> +    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.

I should have used XENLOG_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.

With below code it prints message, free's up memory and return error.
The caller can handle as required.

>
>> +    {
>> +        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 */
>
[...]
>
>> +
>> +    /*
>> +     * 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?".

AIUI, by default all devices are assigned to Dom0.
By updating dev->domain in add device to Dom0, we are making
sure that at the time of add device the device is with Dom0.

In assign_device the below check ensures that devices are with Dom0
before assigning to other Doms.

If we don't want to update dev->domain in add_device then
I think, we can explicitly initialize dev->domain = NULL and
check here (assign_device) that dev->domain should always either NULL or
should be hardware_domain.

>
>> +     * 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

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