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

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel