| Message ID | 20230801063125.34995-2-baolu.lu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu: Make pasid array per device | expand |
> From: Lu Baolu <baolu.lu@linux.intel.com> > Sent: Tuesday, August 1, 2023 2:31 PM > > When switching device DMA ownership, it is required that all the device's > pasid DMA be disabled. This is done by checking if the pasid array of the > group is empty. Consolidate all the open code into a single helper. No > intentional functionality change. ... > /** > * iommu_device_use_default_domain() - Device driver wants to handle > device > * DMA through the kernel DMA API. > @@ -3052,14 +3063,14 @@ int iommu_device_use_default_domain(struct > device *dev) > > mutex_lock(&group->mutex); > if (group->owner_cnt) { > - if (group->owner || !iommu_is_default_domain(group) || > - !xa_empty(&group->pasid_array)) { > + if (group->owner || !iommu_is_default_domain(group)) { > ret = -EBUSY; > goto unlock_out; > } > } > > group->owner_cnt++; > + assert_pasid_dma_ownership(group); Old code returns error if pasid_xrrary is not empty. New code continues to take ownership with a warning. this is a functional change. Is it intended or not?
On 2023/8/1 15:03, Tian, Kevin wrote: >> /** >> * iommu_device_use_default_domain() - Device driver wants to handle >> device >> * DMA through the kernel DMA API. >> @@ -3052,14 +3063,14 @@ int iommu_device_use_default_domain(struct >> device *dev) >> >> mutex_lock(&group->mutex); >> if (group->owner_cnt) { >> - if (group->owner || !iommu_is_default_domain(group) || >> - !xa_empty(&group->pasid_array)) { >> + if (group->owner || !iommu_is_default_domain(group)) { >> ret = -EBUSY; >> goto unlock_out; >> } >> } >> >> group->owner_cnt++; >> + assert_pasid_dma_ownership(group); > Old code returns error if pasid_xrrary is not empty. > > New code continues to take ownership with a warning. > > this is a functional change. Is it intended or not? If iommu_device_use_default_domain() is called with pasid_array not empty, there must be a bug somewhere in the device driver. We should WARN it instead of returning an error. Probably this is a functional change? If so, I can add this in the commit message. Best regards, baolu
> From: Baolu Lu <baolu.lu@linux.intel.com> > Sent: Tuesday, August 1, 2023 3:44 PM > > On 2023/8/1 15:03, Tian, Kevin wrote: > >> /** > >> * iommu_device_use_default_domain() - Device driver wants to handle > >> device > >> * DMA through the kernel DMA API. > >> @@ -3052,14 +3063,14 @@ int > iommu_device_use_default_domain(struct > >> device *dev) > >> > >> mutex_lock(&group->mutex); > >> if (group->owner_cnt) { > >> - if (group->owner || !iommu_is_default_domain(group) || > >> - !xa_empty(&group->pasid_array)) { > >> + if (group->owner || !iommu_is_default_domain(group)) { > >> ret = -EBUSY; > >> goto unlock_out; > >> } > >> } > >> > >> group->owner_cnt++; > >> + assert_pasid_dma_ownership(group); > > Old code returns error if pasid_xrrary is not empty. > > > > New code continues to take ownership with a warning. > > > > this is a functional change. Is it intended or not? > > If iommu_device_use_default_domain() is called with pasid_array not > empty, there must be a bug somewhere in the device driver. We should > WARN it instead of returning an error. Probably this is a functional > change? If so, I can add this in the commit message. > IMHO we should WARN *and* return an error.
On 2023/8/2 9:39, Tian, Kevin wrote: >> From: Baolu Lu<baolu.lu@linux.intel.com> >> Sent: Tuesday, August 1, 2023 3:44 PM >> >> On 2023/8/1 15:03, Tian, Kevin wrote: >>>> /** >>>> * iommu_device_use_default_domain() - Device driver wants to handle >>>> device >>>> * DMA through the kernel DMA API. >>>> @@ -3052,14 +3063,14 @@ int >> iommu_device_use_default_domain(struct >>>> device *dev) >>>> >>>> mutex_lock(&group->mutex); >>>> if (group->owner_cnt) { >>>> - if (group->owner || !iommu_is_default_domain(group) || >>>> - !xa_empty(&group->pasid_array)) { >>>> + if (group->owner || !iommu_is_default_domain(group)) { >>>> ret = -EBUSY; >>>> goto unlock_out; >>>> } >>>> } >>>> >>>> group->owner_cnt++; >>>> + assert_pasid_dma_ownership(group); >>> Old code returns error if pasid_xrrary is not empty. >>> >>> New code continues to take ownership with a warning. >>> >>> this is a functional change. Is it intended or not? >> If iommu_device_use_default_domain() is called with pasid_array not >> empty, there must be a bug somewhere in the device driver. We should >> WARN it instead of returning an error. Probably this is a functional >> change? If so, I can add this in the commit message. >> > IMHO we should WARN*and* return an error. Okay, fine to me. Will make this in the next version. Best regards, baolu
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 4352a149a935..1a8fb30341e6 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -3034,6 +3034,17 @@ static bool iommu_is_default_domain(struct iommu_group *group) return false; } +/* + * Assert no PASID DMA when claiming or releasing group's DMA ownership. + * The iommu_xxx_device_pasid() interfaces are only for device drivers + * that have claimed the DMA ownership. Otherwise, it's a driver bug. + */ +static void assert_pasid_dma_ownership(struct iommu_group *group) +{ + lockdep_assert_held(&group->mutex); + WARN_ON(!xa_empty(&group->pasid_array)); +} + /** * iommu_device_use_default_domain() - Device driver wants to handle device * DMA through the kernel DMA API. @@ -3052,14 +3063,14 @@ int iommu_device_use_default_domain(struct device *dev) mutex_lock(&group->mutex); if (group->owner_cnt) { - if (group->owner || !iommu_is_default_domain(group) || - !xa_empty(&group->pasid_array)) { + if (group->owner || !iommu_is_default_domain(group)) { ret = -EBUSY; goto unlock_out; } } group->owner_cnt++; + assert_pasid_dma_ownership(group); unlock_out: mutex_unlock(&group->mutex); @@ -3084,7 +3095,8 @@ void iommu_device_unuse_default_domain(struct device *dev) return; mutex_lock(&group->mutex); - if (!WARN_ON(!group->owner_cnt || !xa_empty(&group->pasid_array))) + assert_pasid_dma_ownership(group); + if (!WARN_ON(!group->owner_cnt)) group->owner_cnt--; mutex_unlock(&group->mutex); @@ -3118,8 +3130,7 @@ static int __iommu_take_dma_ownership(struct iommu_group *group, void *owner) { int ret; - if ((group->domain && group->domain != group->default_domain) || - !xa_empty(&group->pasid_array)) + if (group->domain && group->domain != group->default_domain) return -EBUSY; ret = __iommu_group_alloc_blocking_domain(group); @@ -3129,8 +3140,10 @@ static int __iommu_take_dma_ownership(struct iommu_group *group, void *owner) if (ret) return ret; + assert_pasid_dma_ownership(group); group->owner = owner; group->owner_cnt++; + return 0; } @@ -3206,10 +3219,10 @@ EXPORT_SYMBOL_GPL(iommu_device_claim_dma_owner); static void __iommu_release_dma_ownership(struct iommu_group *group) { - if (WARN_ON(!group->owner_cnt || !group->owner || - !xa_empty(&group->pasid_array))) + if (WARN_ON(!group->owner_cnt || !group->owner)) return; + assert_pasid_dma_ownership(group); group->owner_cnt = 0; group->owner = NULL; __iommu_group_set_domain_nofail(group, group->default_domain);
When switching device DMA ownership, it is required that all the device's pasid DMA be disabled. This is done by checking if the pasid array of the group is empty. Consolidate all the open code into a single helper. No intentional functionality change. Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> --- drivers/iommu/iommu.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-)