| Message ID | 20240328122958.83332-1-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Two enhancements to iommu_at[de]tach_device_pasid() | expand |
>-----Original Message----- >From: Liu, Yi L <yi.l.liu@intel.com> >Subject: [PATCH v2 0/2] Two enhancements to >iommu_at[de]tach_device_pasid() > >There are minor mistakes in the iommu set_dev_pasid() and >remove_dev_pasid() >paths. The set_dev_pasid() path updates the group->pasid_array first, and >then call into remove_dev_pasid() in error handling when there are devices >within the group that failed to set_dev_pasid. Not related to this patch, just curious in which cases some of the devices In same group failed to set_dev_pasid while others succeed? Thanks Zhenzhong > The remove_dev_pasid() >callbacks of the underlying iommu drivers get the domain for pasid from the >group->pasid_array. So the remove_dev_pasid() callback may get a wrong >domain >in the set_dev_pasid() path. [1] Even if the group is singleton, the existing >code logic would have unnecessary warnings in the error handling of the >set_dev_pasid() path. e.g. intel iommu driver. > >The above issue can be fixed by improving the error handling in the >set_dev_pasid() path. Also, this reminds that it is not reliable for the >underlying iommu driver callback to get the domain from group- >>pasid_array. >So, the second patch of this series passes the domain to remove_dev_pasid >op. > >[1] https://lore.kernel.org/linux- >iommu/20240320123803.GD159172@nvidia.com/ > >Change log: > >v2: > - Make clear that the patch 1/2 of v1 does not fix the problem (Kevin) > - Swap the order of patch 1/2 and 2/2 of v1. In this new series, patch 1/2 > fixes the real issue, patch 2/2 is to avoid potential issue in the future. > >v1: https://lore.kernel.org/linux-iommu/20240327125433.248946-1- >yi.l.liu@intel.com/ > >Regards, > Yi Liu > >Yi Liu (2): > iommu: Undo pasid attachment only for the devices that have succeeded > iommu: Pass domain to remove_dev_pasid() op > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 9 ++----- > drivers/iommu/intel/iommu.c | 11 +++----- > drivers/iommu/iommu.c | 28 ++++++++++++++------- > include/linux/iommu.h | 3 ++- > 4 files changed, 26 insertions(+), 25 deletions(-) > >-- >2.34.1
On 2024/3/29 10:12, Duan, Zhenzhong wrote: > > >> -----Original Message----- >> From: Liu, Yi L <yi.l.liu@intel.com> >> Subject: [PATCH v2 0/2] Two enhancements to >> iommu_at[de]tach_device_pasid() >> >> There are minor mistakes in the iommu set_dev_pasid() and >> remove_dev_pasid() >> paths. The set_dev_pasid() path updates the group->pasid_array first, and >> then call into remove_dev_pasid() in error handling when there are devices >> within the group that failed to set_dev_pasid. > > Not related to this patch, just curious in which cases some of the devices > In same group failed to set_dev_pasid while others succeed? There are multiple failure reasons. Given to the fact of some devices have already succeeded, the most typical error may be no memory. Not sure about other reasons. Regards, Yi Liu > Thanks > Zhenzhong > >> The remove_dev_pasid() >> callbacks of the underlying iommu drivers get the domain for pasid from the >> group->pasid_array. So the remove_dev_pasid() callback may get a wrong >> domain >> in the set_dev_pasid() path. [1] Even if the group is singleton, the existing >> code logic would have unnecessary warnings in the error handling of the >> set_dev_pasid() path. e.g. intel iommu driver. >> >> The above issue can be fixed by improving the error handling in the >> set_dev_pasid() path. Also, this reminds that it is not reliable for the >> underlying iommu driver callback to get the domain from group- >>> pasid_array. >> So, the second patch of this series passes the domain to remove_dev_pasid >> op. >> >> [1] https://lore.kernel.org/linux- >> iommu/20240320123803.GD159172@nvidia.com/ >> >> Change log: >> >> v2: >> - Make clear that the patch 1/2 of v1 does not fix the problem (Kevin) >> - Swap the order of patch 1/2 and 2/2 of v1. In this new series, patch 1/2 >> fixes the real issue, patch 2/2 is to avoid potential issue in the future. >> >> v1: https://lore.kernel.org/linux-iommu/20240327125433.248946-1- >> yi.l.liu@intel.com/ >> >> Regards, >> Yi Liu >> >> Yi Liu (2): >> iommu: Undo pasid attachment only for the devices that have succeeded >> iommu: Pass domain to remove_dev_pasid() op >> >> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 9 ++----- >> drivers/iommu/intel/iommu.c | 11 +++----- >> drivers/iommu/iommu.c | 28 ++++++++++++++------- >> include/linux/iommu.h | 3 ++- >> 4 files changed, 26 insertions(+), 25 deletions(-) >> >> -- >> 2.34.1 >
>-----Original Message----- >From: Liu, Yi L <yi.l.liu@intel.com> >Subject: Re: [PATCH v2 0/2] Two enhancements to >iommu_at[de]tach_device_pasid() > >On 2024/3/29 10:12, Duan, Zhenzhong wrote: >> >> >>> -----Original Message----- >>> From: Liu, Yi L <yi.l.liu@intel.com> >>> Subject: [PATCH v2 0/2] Two enhancements to >>> iommu_at[de]tach_device_pasid() >>> >>> There are minor mistakes in the iommu set_dev_pasid() and >>> remove_dev_pasid() >>> paths. The set_dev_pasid() path updates the group->pasid_array first, >and >>> then call into remove_dev_pasid() in error handling when there are >devices >>> within the group that failed to set_dev_pasid. >> >> Not related to this patch, just curious in which cases some of the devices >> In same group failed to set_dev_pasid while others succeed? >There are multiple failure reasons. Given to the fact of some devices have >already succeeded, the most typical error may be no memory. Not sure >about >other reasons. Oh, the no memory case, clear, thanks Yi. BRs. Zhenzhong
On 3/29/24 10:12 AM, Duan, Zhenzhong wrote: > >> -----Original Message----- >> From: Liu, Yi L<yi.l.liu@intel.com> >> Subject: [PATCH v2 0/2] Two enhancements to >> iommu_at[de]tach_device_pasid() >> >> There are minor mistakes in the iommu set_dev_pasid() and >> remove_dev_pasid() >> paths. The set_dev_pasid() path updates the group->pasid_array first, and >> then call into remove_dev_pasid() in error handling when there are devices >> within the group that failed to set_dev_pasid. > Not related to this patch, just curious in which cases some of the devices > In same group failed to set_dev_pasid while others succeed? The failure cases could be checked in the set_dev_pasid implementation of the individual iommu driver. For x86 platforms, which are PCI fabric- based, there's no such case as PCI/PASID requires a singleton iommu group. Best regards, baolu
>-----Original Message----- >From: Baolu Lu <baolu.lu@linux.intel.com> >Subject: Re: [PATCH v2 0/2] Two enhancements to >iommu_at[de]tach_device_pasid() > >On 3/29/24 10:12 AM, Duan, Zhenzhong wrote: >> >>> -----Original Message----- >>> From: Liu, Yi L<yi.l.liu@intel.com> >>> Subject: [PATCH v2 0/2] Two enhancements to >>> iommu_at[de]tach_device_pasid() >>> >>> There are minor mistakes in the iommu set_dev_pasid() and >>> remove_dev_pasid() >>> paths. The set_dev_pasid() path updates the group->pasid_array first, >and >>> then call into remove_dev_pasid() in error handling when there are >devices >>> within the group that failed to set_dev_pasid. >> Not related to this patch, just curious in which cases some of the devices >> In same group failed to set_dev_pasid while others succeed? > >The failure cases could be checked in the set_dev_pasid implementation >of the individual iommu driver. For x86 platforms, which are PCI fabric- >based, there's no such case as PCI/PASID requires a singleton iommu >group. Clear, thanks Baolu. BRs. Zhenzhong
On Thu, Mar 28, 2024 at 05:29:56AM -0700, Yi Liu wrote: > Yi Liu (2): > iommu: Undo pasid attachment only for the devices that have succeeded > iommu: Pass domain to remove_dev_pasid() op > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 9 ++----- > drivers/iommu/intel/iommu.c | 11 +++----- > drivers/iommu/iommu.c | 28 ++++++++++++++------- > include/linux/iommu.h | 3 ++- > 4 files changed, 26 insertions(+), 25 deletions(-) Applied, thanks.