Re: [XEN][RFC PATCH v2 06/12] xen/smmu: Add remove_device callback for smmu_iommu ops

[Julien Grall <julien@xxxxxxx>]

Hi,

On 09/11/2021 08:02, Vikram Garhwal wrote:
> Add remove_device callback for removing the device entry from smmu-master using
> following steps:
> 1. Find if SMMU master exists for the device node.
> 2. Remove the SMMU master
>
> Signed-off-by: Vikram Garhwal <fnu.vikram@xxxxxxxxxx>
> ---
>   xen/drivers/passthrough/arm/smmu.c | 54 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 54 insertions(+)
>
> diff --git a/xen/drivers/passthrough/arm/smmu.c 
> b/xen/drivers/passthrough/arm/smmu.c
> index c9dfc4c..1a32e2c 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -816,6 +816,17 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
>         return 0;
>   }
>   
> +static int remove_smmu_master(struct arm_smmu_device *smmu,
> +                             struct arm_smmu_master *master)
> +{
> +       if (!(smmu->masters.rb_node))
Coding style: The inner () are not necessary.

Also, is this check only there for code hardening purpose? If yes, then 
I would add an ASSERT_UNREACHABLE() in the if body to allow catching the 
error in debug build.
> +               return -ENOENT;
> +
> +       rb_erase(&master->node, &smmu->masters);
> +
> +       return 0;
> +}
> +
>   static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu,
>                                          struct device *dev,
>                                          struct iommu_fwspec *fwspec)
> @@ -853,6 +864,32 @@ static int arm_smmu_dt_add_device_legacy(struct 
> arm_smmu_device *smmu,
>         return insert_smmu_master(smmu, master);
>   }
>   
> +static int arm_smmu_dt_remove_device_legacy(struct arm_smmu_device *smmu,
> +                                        struct device *dev)
> +{
> +       struct arm_smmu_master *master;
> +       struct device_node *dev_node = dev_get_dev_node(dev);
> +       int ret;
> +
> +       master = find_smmu_master(smmu, dev_node);
> +       if (master == NULL) {
> +               dev_err(dev,
> +                       "No registrations found for master device %s\n",
> +                       dev_node->name);
> +               return -EINVAL;
> +       }
Even if the IOMMU subsystem check it, it would be good that the SMMU 
driver also check the device is not currently used before removing it.
If it is, then we should return -EBUSY.

> +
> +       ret = remove_smmu_master(smmu, master);
> +
> +       if (ret)
> +               return ret;
> +
> +    dev_node->is_protected = false;
Coding style: The indentation looks off compare to the other line.

> +
> +       kfree(master);
> +       return 0;
> +}
> +
>   static int register_smmu_master(struct arm_smmu_device *smmu,
>                                 struct device *dev,
>                                 struct of_phandle_args *masterspec)
> @@ -876,6 +913,22 @@ static int register_smmu_master(struct arm_smmu_device 
> *smmu,
>                                              fwspec);
>   }
>   
> +static int arm_smmu_dt_remove_device_generic(u8 devfn, struct device *dev)
> +{
> +       struct arm_smmu_device *smmu;
> +       struct iommu_fwspec *fwspec;
> +
> +       fwspec = dev_iommu_fwspec_get(dev);
> +       if (fwspec == NULL)
> +               return -ENXIO;
> +
> +       smmu = find_smmu(fwspec->iommu_dev);
> +       if (smmu == NULL)
> +               return -ENXIO;
> +
> +       return arm_smmu_dt_remove_device_legacy(smmu, dev);
> +}
> +
>   static int arm_smmu_dt_add_device_generic(u8 devfn, struct device *dev)
>   {
>         struct arm_smmu_device *smmu;
> @@ -2876,6 +2929,7 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
>       .init = arm_smmu_iommu_domain_init,
>       .hwdom_init = arm_smmu_iommu_hwdom_init,
>       .add_device = arm_smmu_dt_add_device_generic,
> +    .remove_device = arm_smmu_dt_remove_device_generic,
>       .teardown = arm_smmu_iommu_domain_teardown,
>       .iotlb_flush = arm_smmu_iotlb_flush,
>       .iotlb_flush_all = arm_smmu_iotlb_flush_all,
Cheers,

--
Julien Grall