Re: [Xen-devel] [PATCH V5 7/8] iommu/arm: Introduce iommu_add_dt_device API

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

Hi Oleksandr,

On 9/24/19 5:22 PM, Oleksandr wrote:
> 
> On 24.09.19 18:57, Julien Grall wrote:
>> Hi,
> 
> Hi Julien
> 
> 
>>
>> On 9/24/19 4:30 PM, Oleksandr Tyshchenko wrote:
>>> @@ -1263,15 +1264,22 @@ static int __init handle_device(struct domain 
>>> *d, struct dt_device_node *dev,
>>>       dt_dprintk("%s passthrough = %d nirq = %d naddr = %u\n",
>>>                  dt_node_full_name(dev), need_mapping, nirq, naddr);
>>>   -    if ( dt_device_is_protected(dev) && need_mapping )
>>> +    if ( need_mapping )
>>>       {
>>> -        dt_dprintk("%s setup iommu\n", dt_node_full_name(dev));
>>> -        res = iommu_assign_dt_device(d, dev);
>>> -        if ( res )
>>> -        {
>>> -            printk(XENLOG_ERR "Failed to setup the IOMMU for %s\n",
>>> +        dt_dprintk("Check if %s is behind the IOMMU and add it\n",
>>>                      dt_node_full_name(dev));
>>> -            return res;
>>> +
>>> +        iommu_add_dt_device(dev);
>>
>> Return value should always be checked or explain why this is not done.
> 
> Yes, I will add a check. The positive result for us is non-negative 
> (either "device is protected" or "device doesn't need to be protected").
> 
> 
>>
>>
>> [...]
>>
>>>   int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>>>                          XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>   {
>>> @@ -177,6 +241,13 @@ int iommu_do_dt_domctl(struct xen_domctl 
>>> *domctl, struct domain *d,
>>>               break;
>>>           }
>>>   +        iommu_add_dt_device(dev);
>>
>> Same here.
> 
> Yes, I think, we don't need to check for return value, because the only 
> one positive result "here" is the fact that "device is protected" (which 
> is checked below).
> 
> What is more, if we add a check for the return value to be strictly 0, 
> we will get an error after guest's reboot (since iommu_add_dt_device() 
> will return -EEXIST).
> 
> So, I will add a comment explaining why we don't check. What do you think?

Why don't you do the following code?

if ( ret < 0 && ret != -EEXIST )

This would allow the code to return the corrrect error to the upper 
layer. A suitable comment on top explaing the check would also be useful.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel