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

[Julien Grall <julien@xxxxxxx>]

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?

>   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