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

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 22/09/15 08:33, Vijay Kilari wrote:
> On Mon, Sep 21, 2015 at 7:17 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
>> 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.

I wasn't asking about the call to its_lpi_free... but the line:

xfree(dev->event_map.lpi_map).

It belonged to its_lpi_free before but now it's part of its_free_device.

I don't see any reason to move it 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.
>>
> 
>   It is almost re-used with following changes

Well if you compare the code it looks very different... it was only for
the purpose to keep the code as close as possible as the Linux ITS driver

I guess we already diverge a lot so having one functions more...

> 1) Rounding of nr_ites, which is missing. However nr_ites is passed
> from platform
>     file.

While today it's only called in the Thunder-X platform specific code and
the value are hardcoded. With the addition of PCI passthrough, this
value will be directly retrieved from the PCI configuration space.

Furthermore, AFAICT, the round up to a power of 2 is an ITS restriction
and not mandated by the PCI spec. So you can't really on the caller
giving nicely the correct value.
e caller giving you the correct

> 2) Freeing of memory on failure. Here I re-used its_free_device() which does
>     the same

[...]

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

Yes. I don't see the point to use XENLOG_G_* here.

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

This call can only fail if you are implementation is buggy. It's true
you may get in trouble elsewhere later.

As you know that this call should never fail, you can use a BUG_ON to
check your assertion. This is very common within Xen and Linux.  It
allows you to catch the root cause rather than waiting to have unrelated
error making harder to debug.

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

No, you are not assigning the device to DOM0 in the function add_device.
You are just setting a value and left the ITS incorrectly setup to allow
DOM0 using this device.

You still have to call assign_device to setup the ITS for this device.

Furthermore, that make the requirement to call its_add_device always
after DOM0 has been created. We may want to add device in the ITS before
ITS.

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

For now, you just have to check that d is the hardware domain and
pdev->domain is not NULL. This is fine because the caller is only the
platform code.

We will need to revisit this code when PCI passthrough will be added to
add some missing lock on the device and adding more check on the used
domain.

Regards,

-- 
Julien Grall

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