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

[Julien Grall <julien@xen.org>]

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