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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 19/09/2019 13:25, Oleksandr wrote:
> On 19.09.19 14:35, Julien Grall wrote:
> > Hi Oleksandr,
> > 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.
Some of the device_tree.c already contains Arm specific code (such as device.h).

DT has been only used by Arm so far, so it is sadly fairly tie to the 
architecture. But it should be easy to make it generic if needs be (such as for 
RISCv).
While iommu_fwspec is been implemented in Arm headers, this could potentially be 
made common. So I would still prefer this that function is moved in device_tree.c
Cheers,

--
Julien Grall

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