Re: [Xen-devel] [RFC 1/2] arm, smmu: add support for iommu_fwspec functions

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 14 Jan 2020, Julien Grall wrote:
> Hi Brian,
> 
> On 10/01/2020 01:26, Brian Woods wrote:
> > Modify the smmu driver so that it uses the iommu_fwspec helper
> > functions.  This means both ARM IOMMU drivers will both use the
> > iommu_fwspec helper functions, making enabling generic device tree
> > bindings in the SMMU driver much cleaner.
> > 
> > Signed-off-by: Brian Woods <brian.woods@xxxxxxxxxx>
> > ---
> > RFC especially wanted on:
> >    - Check in device_tree.c.  This is needed, otherwise it won't boot due
> >      to dev_iommu_fwspec_get(dev) being true and returning EEXIST.  I'm
> >      not completely sure what type of check is best here.
> 
> I guess this because the masters are registered during the initialization of
> the SMMU. Could we instead look at registering them from the add_device
> callback?
> 
> I understand this would mean to go through all the SMMU and require a bit more
> work. But I think it will help longer term as the workflow for registering a
> master would be similar whether legacy or generic bindings are used.
> 
> @Stefano any opinions?

Yeah, add_device looks like a better fit for the new bindings.


> >   xen/drivers/passthrough/arm/smmu.c    | 74
> > ++++++++++++++++++++++-------------
> >   xen/drivers/passthrough/device_tree.c |  3 ++ >   2 files changed, 49
> > insertions(+), 28 deletions(-)
> > 
> > diff --git a/xen/drivers/passthrough/arm/smmu.c
> > b/xen/drivers/passthrough/arm/smmu.c
> > index 94662a8..c5db5be 100644
> > --- a/xen/drivers/passthrough/arm/smmu.c
> > +++ b/xen/drivers/passthrough/arm/smmu.c
> > @@ -49,6 +49,7 @@
> >   #include <asm/atomic.h>
> >   #include <asm/device.h>
> >   #include <asm/io.h>
> > +#include <asm/iommu_fwspec.h>
> >   #include <asm/platform.h>
> >     /* Xen: The below defines are redefined within the file. Undef it */
> > @@ -597,8 +598,7 @@ struct arm_smmu_smr {
> >   };
> >     struct arm_smmu_master_cfg {
> > -   int                             num_streamids;
> > -   u16                             streamids[MAX_MASTER_STREAMIDS];
> 
> Now that we use fwspec, do we really need to keep the MAX_MASTER_STREAMIDS
> limit?
> 
> > +   struct iommu_fwspec             *fwspec;
> 
> NIT: Can the content be const?
> 
> >     struct arm_smmu_smr             *smrs;
> >   };
> >   @@ -779,7 +779,7 @@ static int register_smmu_master(struct arm_smmu_device
> > *smmu,
> >                             struct device *dev,
> >                             struct of_phandle_args *masterspec)
> >   {
> > -   int i;
> > +   int i, ret = 0;
> >     struct arm_smmu_master *master;
> >             master = find_smmu_master(smmu, masterspec->np);
> > @@ -798,26 +798,37 @@ static int register_smmu_master(struct arm_smmu_device
> > *smmu,
> >     }
> >             master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL);
> > -   if (!master)
> > +   if (!master) {
> >             return -ENOMEM;
> > +   }
> 
> NIT: May I ask why did you add the {} here?
> 
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

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