| Message ID | 20240628085538.47049-1-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Make set_dev_pasid op supportting domain replacement | expand |
> From: Liu, Yi L <yi.l.liu@intel.com> > Sent: Friday, June 28, 2024 4:56 PM > > This splits the preparation works of the iommu and the Intel iommu driver > out from the iommufd pasid attach/replace series. [1] > > To support domain replacement, the definition of the set_dev_pasid op > needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks > should be extended as well to suit the new definition. > > pasid attach/replace is mandatory on Intel VT-d given the PASID table > locates in the physical address space hence must be managed by the kernel, > both for supporting vSVA and coming SIOV. But it's optional on ARM/AMD > which allow configuring the PASID/CD table either in host physical address > space or nested on top of an GPA address space. This series only extends > the Intel iommu driver as the minimal requirement. Looks above is only within VFIO/IOMMUFD context (copied from the old series). But this series is all in IOMMU and pasid attach is certainly not optional for SVA on all platforms. this needs to be revised. > > This series first prepares the Intel iommu set_dev_pasid op for the new > definition, adds the missing set_dev_pasid support for nested domain, and > in the end enhances the definition of set_dev_pasid op. The ARM and AMD > set_dev_pasid callbacks is extended to fail if the caller tries to do domain > replacement to meet the new definition of set_dev_pasid op. > > This series is on top of Baolu's paging domain alloc refactor, where his > code can be found at [2]. > > [1] https://lore.kernel.org/linux-iommu/20240412081516.31168-1- > yi.l.liu@intel.com/ > [2] https://github.com/LuBaolu/intel-iommu/commits/vtd-paging-domain- > refactor-v1 > > Regards, > Yi Liu > > Lu Baolu (1): > iommu/vt-d: Add set_dev_pasid callback for nested domain > > Yi Liu (5): > iommu: Pass old domain to set_dev_pasid op > iommu/vt-d: Move intel_drain_pasid_prq() into > intel_pasid_tear_down_entry() > iommu/vt-d: Make helpers support modifying present pasid entry > iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain > replacement > iommu: Make set_dev_pasid op support domain replacement > > drivers/iommu/amd/amd_iommu.h | 3 +- > drivers/iommu/amd/pasid.c | 6 +- > .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 6 +- > drivers/iommu/intel/debugfs.c | 2 + > drivers/iommu/intel/iommu.c | 117 ++++++++++++------ > drivers/iommu/intel/iommu.h | 3 + > drivers/iommu/intel/nested.c | 1 + > drivers/iommu/intel/pasid.c | 46 +++---- > drivers/iommu/intel/pasid.h | 5 +- > drivers/iommu/intel/svm.c | 9 +- > drivers/iommu/iommu.c | 3 +- > include/linux/iommu.h | 5 +- > 12 files changed, 132 insertions(+), 74 deletions(-) > > -- > 2.34.1
On Fri, Jun 28, 2024 at 01:55:32AM -0700, Yi Liu wrote: > This splits the preparation works of the iommu and the Intel iommu driver > out from the iommufd pasid attach/replace series. [1] > > To support domain replacement, the definition of the set_dev_pasid op > needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks > should be extended as well to suit the new definition. > > pasid attach/replace is mandatory on Intel VT-d given the PASID table > locates in the physical address space hence must be managed by the kernel, > both for supporting vSVA and coming SIOV. But it's optional on ARM/AMD > which allow configuring the PASID/CD table either in host physical address > space or nested on top of an GPA address space. This series only extends > the Intel iommu driver as the minimal requirement. Sicne this will be pushed to the next cyle that will have my ARM code the smmuv3 will need to be updated too. It is already prepped to support replace, just add this please: diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c index ead83d67421f10..44434978a218ae 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c @@ -350,7 +351,8 @@ void arm_smmu_sva_notifier_synchronize(void) } static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain, - struct device *dev, ioasid_t id) + struct device *dev, ioasid_t id, + struct iommu_domain *old_domain) { struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); struct arm_smmu_master *master = dev_iommu_priv_get(dev); @@ -367,7 +369,7 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain, */ arm_smmu_make_sva_cd(&target, master, domain->mm, smmu_domain->asid, smmu_domain->btm_invalidation); - ret = arm_smmu_set_pasid(master, smmu_domain, id, &target); + ret = arm_smmu_set_pasid(master, smmu_domain, id, &target, old_domain); mmput(domain->mm); return ret; diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index 238968b1709936..140aac5cd4ef57 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2943,7 +2943,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) } static int arm_smmu_s1_set_dev_pasid(struct iommu_domain *domain, - struct device *dev, ioasid_t id) + struct device *dev, ioasid_t id, + struct iommu_domain *old_domain) { struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); struct arm_smmu_master *master = dev_iommu_priv_get(dev); @@ -2969,7 +2970,7 @@ static int arm_smmu_s1_set_dev_pasid(struct iommu_domain *domain, */ arm_smmu_make_s1_cd(&target_cd, master, smmu_domain); return arm_smmu_set_pasid(master, to_smmu_domain(domain), id, - &target_cd); + &target_cd, old_domain); } static void arm_smmu_update_ste(struct arm_smmu_master *master, @@ -2999,7 +3000,7 @@ static void arm_smmu_update_ste(struct arm_smmu_master *master, int arm_smmu_set_pasid(struct arm_smmu_master *master, struct arm_smmu_domain *smmu_domain, ioasid_t pasid, - struct arm_smmu_cd *cd) + struct arm_smmu_cd *cd, struct iommu_domain *old_domain) { struct iommu_domain *sid_domain = iommu_get_domain_for_dev(master->dev); struct arm_smmu_attach_state state = { @@ -3009,6 +3010,7 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master, * already attached, no need to set old_domain. */ .ssid = pasid, + .old_domain = old_domain, }; struct arm_smmu_cd *cdptr; int ret; diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h index bcf9ea9d929f5f..447a3cdf1c4e1c 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h @@ -828,7 +828,7 @@ void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid, int arm_smmu_set_pasid(struct arm_smmu_master *master, struct arm_smmu_domain *smmu_domain, ioasid_t pasid, - struct arm_smmu_cd *cd); + struct arm_smmu_cd *cd, struct iommu_domain *old_domain); int arm_smmu_domain_alloc_id(struct arm_smmu_device *smmu, struct arm_smmu_domain *smmu_domain);
On Wed, Jul 10, 2024 at 08:24:16AM +0000, Tian, Kevin wrote: > > From: Liu, Yi L <yi.l.liu@intel.com> > > Sent: Friday, June 28, 2024 4:56 PM > > > > This splits the preparation works of the iommu and the Intel iommu driver > > out from the iommufd pasid attach/replace series. [1] > > > > To support domain replacement, the definition of the set_dev_pasid op > > needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks > > should be extended as well to suit the new definition. > > > > pasid attach/replace is mandatory on Intel VT-d given the PASID table > > locates in the physical address space hence must be managed by the kernel, > > both for supporting vSVA and coming SIOV. But it's optional on ARM/AMD > > which allow configuring the PASID/CD table either in host physical address > > space or nested on top of an GPA address space. This series only extends > > the Intel iommu driver as the minimal requirement. > > Looks above is only within VFIO/IOMMUFD context (copied from the old > series). But this series is all in IOMMU and pasid attach is certainly not > optional for SVA on all platforms. this needs to be revised. I feel like we should explicitly block replace on AMD by checking if old_domain is !NULL and failing. Then the description is sort of like Replace is useful for iommufd/VFIO to provide perfect HW emulation in case the VM is expecting to be able to change a PASID on the fly. As AMD will only support PASID in VM's using nested translation where we don't use the set_dev_pasid API leave it disabled for now. Jason
On 2024/7/12 02:37, Jason Gunthorpe wrote: > On Fri, Jun 28, 2024 at 01:55:32AM -0700, Yi Liu wrote: >> This splits the preparation works of the iommu and the Intel iommu driver >> out from the iommufd pasid attach/replace series. [1] >> >> To support domain replacement, the definition of the set_dev_pasid op >> needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks >> should be extended as well to suit the new definition. >> >> pasid attach/replace is mandatory on Intel VT-d given the PASID table >> locates in the physical address space hence must be managed by the kernel, >> both for supporting vSVA and coming SIOV. But it's optional on ARM/AMD >> which allow configuring the PASID/CD table either in host physical address >> space or nested on top of an GPA address space. This series only extends >> the Intel iommu driver as the minimal requirement. > > Sicne this will be pushed to the next cyle that will have my ARM code > the smmuv3 will need to be updated too. It is already prepped to > support replace, just add this please: thanks. So your related series has made the internal helpers to support domain replacement in set_dev_pasid path. The below diff just passes the old_domain across the helpers. Is it? Just to double confirm with you. :) > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > index ead83d67421f10..44434978a218ae 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > @@ -350,7 +351,8 @@ void arm_smmu_sva_notifier_synchronize(void) > } > > static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain, > - struct device *dev, ioasid_t id) > + struct device *dev, ioasid_t id, > + struct iommu_domain *old_domain) > { > struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); > struct arm_smmu_master *master = dev_iommu_priv_get(dev); > @@ -367,7 +369,7 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain, > */ > arm_smmu_make_sva_cd(&target, master, domain->mm, smmu_domain->asid, > smmu_domain->btm_invalidation); > - ret = arm_smmu_set_pasid(master, smmu_domain, id, &target); > + ret = arm_smmu_set_pasid(master, smmu_domain, id, &target, old_domain); > > mmput(domain->mm); > return ret; > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > index 238968b1709936..140aac5cd4ef57 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -2943,7 +2943,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > } > > static int arm_smmu_s1_set_dev_pasid(struct iommu_domain *domain, > - struct device *dev, ioasid_t id) > + struct device *dev, ioasid_t id, > + struct iommu_domain *old_domain) > { > struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); > struct arm_smmu_master *master = dev_iommu_priv_get(dev); > @@ -2969,7 +2970,7 @@ static int arm_smmu_s1_set_dev_pasid(struct iommu_domain *domain, > */ > arm_smmu_make_s1_cd(&target_cd, master, smmu_domain); > return arm_smmu_set_pasid(master, to_smmu_domain(domain), id, > - &target_cd); > + &target_cd, old_domain); > } > > static void arm_smmu_update_ste(struct arm_smmu_master *master, > @@ -2999,7 +3000,7 @@ static void arm_smmu_update_ste(struct arm_smmu_master *master, > > int arm_smmu_set_pasid(struct arm_smmu_master *master, > struct arm_smmu_domain *smmu_domain, ioasid_t pasid, > - struct arm_smmu_cd *cd) > + struct arm_smmu_cd *cd, struct iommu_domain *old_domain) > { > struct iommu_domain *sid_domain = iommu_get_domain_for_dev(master->dev); > struct arm_smmu_attach_state state = { > @@ -3009,6 +3010,7 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master, > * already attached, no need to set old_domain. > */ > .ssid = pasid, > + .old_domain = old_domain, > }; > struct arm_smmu_cd *cdptr; > int ret; > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > index bcf9ea9d929f5f..447a3cdf1c4e1c 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > @@ -828,7 +828,7 @@ void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid, > > int arm_smmu_set_pasid(struct arm_smmu_master *master, > struct arm_smmu_domain *smmu_domain, ioasid_t pasid, > - struct arm_smmu_cd *cd); > + struct arm_smmu_cd *cd, struct iommu_domain *old_domain); > > int arm_smmu_domain_alloc_id(struct arm_smmu_device *smmu, > struct arm_smmu_domain *smmu_domain);
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Friday, July 12, 2024 2:41 AM > > On Wed, Jul 10, 2024 at 08:24:16AM +0000, Tian, Kevin wrote: > > > From: Liu, Yi L <yi.l.liu@intel.com> > > > Sent: Friday, June 28, 2024 4:56 PM > > > > > > This splits the preparation works of the iommu and the Intel iommu > driver > > > out from the iommufd pasid attach/replace series. [1] > > > > > > To support domain replacement, the definition of the set_dev_pasid op > > > needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks > > > should be extended as well to suit the new definition. > > > > > > pasid attach/replace is mandatory on Intel VT-d given the PASID table > > > locates in the physical address space hence must be managed by the > kernel, > > > both for supporting vSVA and coming SIOV. But it's optional on > ARM/AMD > > > which allow configuring the PASID/CD table either in host physical > address > > > space or nested on top of an GPA address space. This series only extends > > > the Intel iommu driver as the minimal requirement. > > > > Looks above is only within VFIO/IOMMUFD context (copied from the old > > series). But this series is all in IOMMU and pasid attach is certainly not > > optional for SVA on all platforms. this needs to be revised. > > I feel like we should explicitly block replace on AMD by checking if > old_domain is !NULL and failing. this is what patch06 does. > > Then the description is sort of like > > Replace is useful for iommufd/VFIO to provide perfect HW emulation in > case the VM is expecting to be able to change a PASID on the fly. As > AMD will only support PASID in VM's using nested translation where we > don't use the set_dev_pasid API leave it disabled for now. > yes that's clearer. btw I don't remember whether we have discussed the rationale behind the different driver semantics between RID and PASID. Currently RID replace is same as RID attach, with the driver simply blocking the old translation from the start and then no rollback upon failure when switching to the new domain (expecting the iommu core to recover), while for PASID replace we expect the driver to implement the hitless switch. Is it because there is no need of perfect HW emulation for RID or just to be cleaned up later? This difference at least starts bringing some puzzle to me when reading the related code in the intel-iommu driver.
On 2024/7/12 02:41, Jason Gunthorpe wrote: > On Wed, Jul 10, 2024 at 08:24:16AM +0000, Tian, Kevin wrote: >>> From: Liu, Yi L <yi.l.liu@intel.com> >>> Sent: Friday, June 28, 2024 4:56 PM >>> >>> This splits the preparation works of the iommu and the Intel iommu driver >>> out from the iommufd pasid attach/replace series. [1] >>> >>> To support domain replacement, the definition of the set_dev_pasid op >>> needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks >>> should be extended as well to suit the new definition. >>> >>> pasid attach/replace is mandatory on Intel VT-d given the PASID table >>> locates in the physical address space hence must be managed by the kernel, >>> both for supporting vSVA and coming SIOV. But it's optional on ARM/AMD >>> which allow configuring the PASID/CD table either in host physical address >>> space or nested on top of an GPA address space. This series only extends >>> the Intel iommu driver as the minimal requirement. >> >> Looks above is only within VFIO/IOMMUFD context (copied from the old >> series). But this series is all in IOMMU and pasid attach is certainly not >> optional for SVA on all platforms. this needs to be revised. > > I feel like we should explicitly block replace on AMD by checking if > old_domain is !NULL and failing. yes, patch 6 of this series has made it fail in AMD's set_dev_pasid callback. > Then the description is sort of like > > Replace is useful for iommufd/VFIO to provide perfect HW emulation in > case the VM is expecting to be able to change a PASID on the fly. As > AMD will only support PASID in VM's using nested translation where we > don't use the set_dev_pasid API leave it disabled for now. Does it apply to SMMUv3 as well? IIRC. ARM SMMUv3 also has the CD table (a.k.a PASID table) within the guest. And I think this is the major reason for your above statement. right?
On Mon, Jul 15, 2024 at 04:11:43PM +0800, Yi Liu wrote: > On 2024/7/12 02:37, Jason Gunthorpe wrote: > > On Fri, Jun 28, 2024 at 01:55:32AM -0700, Yi Liu wrote: > > > This splits the preparation works of the iommu and the Intel iommu driver > > > out from the iommufd pasid attach/replace series. [1] > > > > > > To support domain replacement, the definition of the set_dev_pasid op > > > needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks > > > should be extended as well to suit the new definition. > > > > > > pasid attach/replace is mandatory on Intel VT-d given the PASID table > > > locates in the physical address space hence must be managed by the kernel, > > > both for supporting vSVA and coming SIOV. But it's optional on ARM/AMD > > > which allow configuring the PASID/CD table either in host physical address > > > space or nested on top of an GPA address space. This series only extends > > > the Intel iommu driver as the minimal requirement. > > > > Sicne this will be pushed to the next cyle that will have my ARM code > > the smmuv3 will need to be updated too. It is already prepped to > > support replace, just add this please: > > thanks. So your related series has made the internal helpers to support > domain replacement in set_dev_pasid path. The below diff just passes the > old_domain across the helpers. Is it? Just to double confirm with you. :) Yes Jason
On Mon, Jul 15, 2024 at 08:16:40AM +0000, Tian, Kevin wrote: > > Then the description is sort of like > > > > Replace is useful for iommufd/VFIO to provide perfect HW emulation in > > case the VM is expecting to be able to change a PASID on the fly. As > > AMD will only support PASID in VM's using nested translation where we > > don't use the set_dev_pasid API leave it disabled for now. > > > > yes that's clearer. > > btw I don't remember whether we have discussed the rationale behind > the different driver semantics between RID and PASID. Currently RID > replace is same as RID attach, with the driver simply blocking the old > translation from the start and then no rollback upon failure when > switching to the new domain (expecting the iommu core to recover), > while for PASID replace we expect the driver to implement the hitless > switch. > > Is it because there is no need of perfect HW emulation for RID or just > to be cleaned up later? Yeah, too much legacy it would be hard to go in and do something about this for RID across every driver. PASID can be a bit more well defined from the start since we only have three drivers and two of them support replace. An ideal driver should be like ARM and support hitless replace whenever possible in all paths so there is no confusion within the driver. Jason
On Mon, Jul 15, 2024 at 04:23:07PM +0800, Yi Liu wrote: > > Then the description is sort of like > > > > Replace is useful for iommufd/VFIO to provide perfect HW emulation in > > case the VM is expecting to be able to change a PASID on the fly. As > > AMD will only support PASID in VM's using nested translation where we > > don't use the set_dev_pasid API leave it disabled for now. > > Does it apply to SMMUv3 as well? IIRC. ARM SMMUv3 also has the CD table > (a.k.a PASID table) within the guest. And I think this is the major reason > for your above statement. right? It does, but it also supports replace so no need to explain why we don't enable it :) Jason
On 2024/7/15 20:19, Jason Gunthorpe wrote: > On Mon, Jul 15, 2024 at 04:23:07PM +0800, Yi Liu wrote: >>> Then the description is sort of like >>> >>> Replace is useful for iommufd/VFIO to provide perfect HW emulation in >>> case the VM is expecting to be able to change a PASID on the fly. As >>> AMD will only support PASID in VM's using nested translation where we >>> don't use the set_dev_pasid API leave it disabled for now. >> >> Does it apply to SMMUv3 as well? IIRC. ARM SMMUv3 also has the CD table >> (a.k.a PASID table) within the guest. And I think this is the major reason >> for your above statement. right? > > It does, but it also supports replace so no need to explain why we > don't enable it :) got it. :)
Hi All, On 6/28/2024 2:25 PM, Yi Liu wrote: > This splits the preparation works of the iommu and the Intel iommu driver > out from the iommufd pasid attach/replace series. [1] > > To support domain replacement, the definition of the set_dev_pasid op > needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks > should be extended as well to suit the new definition. IIUC this will remove PASID from old SVA domain and attaches to new SVA domain. (basically attaching same dev/PASID to different process). Is that the correct? So the expectation is replace existing PASID from PASID table only if old_domain is passed. Otherwise sev_dev_pasid() should throw an error right? -Vasant
On 2024/8/16 01:49, Vasant Hegde wrote: > Hi All, > > On 6/28/2024 2:25 PM, Yi Liu wrote: >> This splits the preparation works of the iommu and the Intel iommu driver >> out from the iommufd pasid attach/replace series. [1] >> >> To support domain replacement, the definition of the set_dev_pasid op >> needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks >> should be extended as well to suit the new definition. > > IIUC this will remove PASID from old SVA domain and attaches to new SVA domain. > (basically attaching same dev/PASID to different process). Is that the correct? In brief, yes. But it's not only for SVA domain. Remember that SIOVr1 extends the usage of PASID. At least on Intel side, a PASID may be attached to paging domains. > So the expectation is replace existing PASID from PASID table only if old_domain > is passed. Otherwise sev_dev_pasid() should throw an error right? > yes. If no old_domain passed in, then it is just a normal attachment. As you are working on AMD iommu, it would be great if you can have a patch to make the AMD set_dev_pasid() op suit this expectation. Then it can be incorporated in this series. :)
On 8/16/24 9:19 AM, Yi Liu wrote: > On 2024/8/16 01:49, Vasant Hegde wrote: >> Hi All, >> >> On 6/28/2024 2:25 PM, Yi Liu wrote: >>> This splits the preparation works of the iommu and the Intel iommu >>> driver >>> out from the iommufd pasid attach/replace series. [1] >>> >>> To support domain replacement, the definition of the set_dev_pasid op >>> needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks >>> should be extended as well to suit the new definition. >> >> IIUC this will remove PASID from old SVA domain and attaches to new >> SVA domain. >> (basically attaching same dev/PASID to different process). Is that the >> correct? > > In brief, yes. But it's not only for SVA domain. Remember that SIOVr1 > extends the usage of PASID. At least on Intel side, a PASID may be > attached to paging domains. You are correct. The idxd driver attaches a paging domain to a non-zero PASID for kernel DMA with PASID. From an architectural perspective, other architectures, like ARM, AMD, and RISC-V, also support this. Therefore, attaching a paging domain to a PASID is not Intel-specific but a generic feature. Thanks, baolu
On 8/16/24 9:19 AM, Yi Liu wrote: >> So the expectation is replace existing PASID from PASID table only if >> old_domain >> is passed. Otherwise sev_dev_pasid() should throw an error right? >> > > yes. If no old_domain passed in, then it is just a normal attachment. As > you are working on AMD iommu, it would be great if you can have a patch to > make the AMD set_dev_pasid() op suit this expectation. Then it can be > incorporated in this series.
Baolu, On 8/16/2024 8:19 AM, Baolu Lu wrote: > On 8/16/24 9:19 AM, Yi Liu wrote: >> On 2024/8/16 01:49, Vasant Hegde wrote: >>> Hi All, >>> >>> On 6/28/2024 2:25 PM, Yi Liu wrote: >>>> This splits the preparation works of the iommu and the Intel iommu driver >>>> out from the iommufd pasid attach/replace series. [1] >>>> >>>> To support domain replacement, the definition of the set_dev_pasid op >>>> needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks >>>> should be extended as well to suit the new definition. >>> >>> IIUC this will remove PASID from old SVA domain and attaches to new SVA domain. >>> (basically attaching same dev/PASID to different process). Is that the correct? >> >> In brief, yes. But it's not only for SVA domain. Remember that SIOVr1 >> extends the usage of PASID. At least on Intel side, a PASID may be >> attached to paging domains. > > You are correct. Thanks. > > The idxd driver attaches a paging domain to a non-zero PASID for kernel > DMA with PASID. From an architectural perspective, other architectures, > like ARM, AMD, and RISC-V, also support this. Therefore, attaching a > paging domain to a PASID is not Intel-specific but a generic feature. Right. We can enhance AMD driver to support attaching PASID to paging/UNMANAGED domains. It needs some more rework/cleanup before we do that. -Vasant
Yi, On 8/16/2024 6:49 AM, Yi Liu wrote: > On 2024/8/16 01:49, Vasant Hegde wrote: >> Hi All, >> >> On 6/28/2024 2:25 PM, Yi Liu wrote: >>> This splits the preparation works of the iommu and the Intel iommu driver >>> out from the iommufd pasid attach/replace series. [1] >>> >>> To support domain replacement, the definition of the set_dev_pasid op >>> needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks >>> should be extended as well to suit the new definition. >> >> IIUC this will remove PASID from old SVA domain and attaches to new SVA domain. >> (basically attaching same dev/PASID to different process). Is that the correct? > > In brief, yes. But it's not only for SVA domain. Remember that SIOVr1 > extends the usage of PASID. At least on Intel side, a PASID may be > attached to paging domains. Right. I missed SIOV case. > >> So the expectation is replace existing PASID from PASID table only if old_domain >> is passed. Otherwise sev_dev_pasid() should throw an error right? >> > > yes. If no old_domain passed in, then it is just a normal attachment. As > you are working on AMD iommu, it would be great if you can have a patch to > make the AMD set_dev_pasid() op suit this expectation. Then it can be > incorporated in this series. :)' Sure. It should be simple. I will try to get the patch next week. -Vasant
On 2024/8/16 10:52, Baolu Lu wrote: > On 8/16/24 9:19 AM, Yi Liu wrote: >>> So the expectation is replace existing PASID from PASID table only if >>> old_domain >>> is passed. Otherwise sev_dev_pasid() should throw an error right? >>> >> >> yes. If no old_domain passed in, then it is just a normal attachment. As >> you are working on AMD iommu, it would be great if you can have a patch to >> make the AMD set_dev_pasid() op suit this expectation. Then it can be >> incorporated in this series.
This splits the preparation works of the iommu and the Intel iommu driver out from the iommufd pasid attach/replace series. [1] To support domain replacement, the definition of the set_dev_pasid op needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks should be extended as well to suit the new definition. pasid attach/replace is mandatory on Intel VT-d given the PASID table locates in the physical address space hence must be managed by the kernel, both for supporting vSVA and coming SIOV. But it's optional on ARM/AMD which allow configuring the PASID/CD table either in host physical address space or nested on top of an GPA address space. This series only extends the Intel iommu driver as the minimal requirement. This series first prepares the Intel iommu set_dev_pasid op for the new definition, adds the missing set_dev_pasid support for nested domain, and in the end enhances the definition of set_dev_pasid op. The ARM and AMD set_dev_pasid callbacks is extended to fail if the caller tries to do domain replacement to meet the new definition of set_dev_pasid op. This series is on top of Baolu's paging domain alloc refactor, where his code can be found at [2]. [1] https://lore.kernel.org/linux-iommu/20240412081516.31168-1-yi.l.liu@intel.com/ [2] https://github.com/LuBaolu/intel-iommu/commits/vtd-paging-domain-refactor-v1 Regards, Yi Liu Lu Baolu (1): iommu/vt-d: Add set_dev_pasid callback for nested domain Yi Liu (5): iommu: Pass old domain to set_dev_pasid op iommu/vt-d: Move intel_drain_pasid_prq() into intel_pasid_tear_down_entry() iommu/vt-d: Make helpers support modifying present pasid entry iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain replacement iommu: Make set_dev_pasid op support domain replacement drivers/iommu/amd/amd_iommu.h | 3 +- drivers/iommu/amd/pasid.c | 6 +- .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 6 +- drivers/iommu/intel/debugfs.c | 2 + drivers/iommu/intel/iommu.c | 117 ++++++++++++------ drivers/iommu/intel/iommu.h | 3 + drivers/iommu/intel/nested.c | 1 + drivers/iommu/intel/pasid.c | 46 +++---- drivers/iommu/intel/pasid.h | 5 +- drivers/iommu/intel/svm.c | 9 +- drivers/iommu/iommu.c | 3 +- include/linux/iommu.h | 5 +- 12 files changed, 132 insertions(+), 74 deletions(-)