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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 19.09.19 14:35, Julien Grall wrote:
> Hi Oleksandr,
Hi, Julien.


> On 13/09/2019 16:35, Oleksandr Tyshchenko wrote:
> > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> >
> > The main puprose of this patch is to add a way to register DT device
> > (which is behind the IOMMU) using the generic IOMMU DT bindings [1]
> > before assigning that device to a domain.
> >
> > So, this patch adds new "iommu_add_dt_device" API for adding DT device
> > to the IOMMU using generic IOMMU DT bindings and previously added
> > "iommu_fwspec" support. It is called when constructing Dom0 since
> > "iommu_assign_dt_device" can be called for Dom0 also.
> The last sentence is misleading. Yes some devices may be assigned to 
> the hardware domain (aka dom0) but other may be assigned to other 
> domains.
> Here you are (ab)using the construction of the hardware domain to add 
> all the devices to the IOMMU. This works fine now because the hardware 
> domain will always be the first domain to be initialized. But I am not 
> sure that whether this is correct to do long term.
> As mentioned earlier, my preference is to keep "add" and "assign" 
> separated. So maybe what we want to do is:
> if ( need_mapping )
> {
>    iommu_add_dt_device(d, dev);
>    if ( dt_device_is_protected(d) )
>    {
>      res = iommu_assign_dt_device(...);
>      if ( res )
>        /* error */
>    }
> }
> We would need similar code in iommu_do_dt_domctl. Although, device 
> should alway be protected in this case.

Well, will modify this way.


> >   diff --git a/xen/drivers/passthrough/arm/iommu.c 
> > b/xen/drivers/passthrough/arm/iommu.c
> > index 5a3d1d5..dea79ed 100644
> > --- a/xen/drivers/passthrough/arm/iommu.c
> > +++ b/xen/drivers/passthrough/arm/iommu.c
> > @@ -20,6 +20,7 @@
> >   #include <xen/lib.h>
> >     #include <asm/device.h>
> > +#include <asm/iommu_fwspec.h>
> >     /*
> >    * Deferred probe list is used to keep track of devices for which 
> > driver
> > @@ -141,3 +142,65 @@ int arch_iommu_populate_page_table(struct domain 
> > *d)
> >   void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
> >   {
> >   }
> > +
> > +int __init iommu_add_dt_device(struct dt_device_node *np)
> Sorry to only realise it now. Would it make sense to have this 
> function implemented in xen/passthrough/device_tree.c? 
Not entirely sure. device_tree.c is a common code. The iommu_fwspec 
stuff (widely used in this function) is ARM code.

--
Regards,

Oleksandr Tyshchenko


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