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

[Oleksandr <olekstysh@xxxxxxxxx>]

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?


> > +        if ( !dt_device_is_protected(dev) )
> > +        {
> > +            ret = -EINVAL;
> > +            break;
> > +        }
> This is already checked in iommu_assign_dt_device().
Will drop here.


--
Regards,

Oleksandr Tyshchenko


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