| Message ID | 20230825080222.14247-13-vikram.garhwal@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dynamic node programming using overlay dtbo | expand |
On 25/08/2023 10: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. Check if device is currently in use. Since you removed a call to iommu_dt_device_is_assigned_locked(), you do not check it from SMMU, right? You are relying on a check done in iommu_remove_dt_device(). This wants to be mentioned. However, Julien suggested to do the check for internal SMMU state. Looking at the code, when the device is assigned, we do: dev_iommu_domain(dev) = domain; and when de-assigned: dev_iommu_domain(dev) = NULL; This means that before calling remove_smmu_master() you could do: /* Make sure device is not assigned */ if (dev_iommu_domain(dev)) return -EBUSY; @Julien, @Stefano? ~Michal
On Tue, 29 Aug 2023, Michal Orzel wrote: > On 25/08/2023 10: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. Check if device is currently in use. > Since you removed a call to iommu_dt_device_is_assigned_locked(), you do not check it from SMMU, right? > You are relying on a check done in iommu_remove_dt_device(). > This wants to be mentioned. However, Julien suggested to do the check for internal SMMU state. > Looking at the code, when the device is assigned, we do: > dev_iommu_domain(dev) = domain; > and when de-assigned: > dev_iommu_domain(dev) = NULL; > > This means that before calling remove_smmu_master() you could do: > > /* Make sure device is not assigned */ > if (dev_iommu_domain(dev)) > return -EBUSY; > > @Julien, @Stefano? I think it is OK without it, as we have a call to iommu_dt_device_is_assigned_locked(np) already in iommu_remove_dt_device?
On 30/08/2023 00:45, Stefano Stabellini wrote: > > > On Tue, 29 Aug 2023, Michal Orzel wrote: >> On 25/08/2023 10: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. Check if device is currently in use. >> Since you removed a call to iommu_dt_device_is_assigned_locked(), you do not check it from SMMU, right? >> You are relying on a check done in iommu_remove_dt_device(). >> This wants to be mentioned. However, Julien suggested to do the check for internal SMMU state. >> Looking at the code, when the device is assigned, we do: >> dev_iommu_domain(dev) = domain; >> and when de-assigned: >> dev_iommu_domain(dev) = NULL; >> >> This means that before calling remove_smmu_master() you could do: >> >> /* Make sure device is not assigned */ >> if (dev_iommu_domain(dev)) >> return -EBUSY; >> >> @Julien, @Stefano? > > I think it is OK without it, as we have a call to > iommu_dt_device_is_assigned_locked(np) already in > iommu_remove_dt_device? Yes, but this would add an extra layer of protection by checking for IOMMU domain for this device. ~Michal
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c index c37fa9af13..71799064f8 100644 --- a/xen/drivers/passthrough/arm/smmu.c +++ b/xen/drivers/passthrough/arm/smmu.c @@ -815,6 +815,19 @@ 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) { + ASSERT_UNREACHABLE(); + 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) @@ -852,6 +865,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; + } + + ret = remove_smmu_master(smmu, master); + if (ret) + return ret; + + /* Protected by dt_host_lock and dtdevs_lock as caller holds these locks. */ + dev_node->is_protected = false; + + kfree(master); + return 0; +} + static int register_smmu_master(struct arm_smmu_device *smmu, struct device *dev, struct of_phandle_args *masterspec) @@ -875,6 +914,26 @@ static int register_smmu_master(struct arm_smmu_device *smmu, fwspec); } +/* + * The driver which supports generic IOMMU DT bindings must have this + * callback implemented. + */ +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; @@ -2859,6 +2918,7 @@ static const struct iommu_ops arm_smmu_iommu_ops = { .init = arm_smmu_iommu_domain_init, .hwdom_init = arch_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, .assign_device = arm_smmu_assign_dev,
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. Check if device is currently in use. 3. Remove the SMMU master. Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> --- Changes from v9: Remove iommu_dt_device_is_assigned_locked(). Add comment regarding caller holding the locks to protect concurrent access. Changes from v7: Added comments on arm_smmu_dt_remove_device_generic(). --- --- xen/drivers/passthrough/arm/smmu.c | 60 ++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+)