| Message ID | 20241018055824.24880-2-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Support attaching PASID to the blocked_domain | expand |
On Thu, Oct 17, 2024 at 10:58:22PM -0700, Yi Liu wrote: > The iommu drivers are on the way to drop the remove_dev_pasid op by > extending the blocked_domain to support PASID. However, this cannot be > done in one shot. So far, the Intel iommu and the ARM SMMUv3 driver have > supported it, while the AMD iommu driver has not yet. During this > transition, the IOMMU core needs to support both ways to destroy the > attachment of device/PASID and domain. Let's just fix AMD? diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c index 9e25b92c68affa..806849cc997631 100644 --- a/drivers/iommu/amd/iommu.c +++ b/drivers/iommu/amd/iommu.c @@ -2437,10 +2437,18 @@ static int blocked_domain_attach_device(struct iommu_domain *domain, return 0; } +static int blocked_domain_set_dev_pasid(struct iommu_domain *domain, + struct device *dev, ioasid_t pasid) +{ + amd_iommu_remove_dev_pasid(dev, pasid, domain); + return 0; +} + static struct iommu_domain blocked_domain = { .type = IOMMU_DOMAIN_BLOCKED, .ops = &(const struct iommu_domain_ops) { .attach_dev = blocked_domain_attach_device, + .set_dev_pasid = blocked_domain_set_dev_pasid, } }; Jason
On 2024/10/18 22:39, Jason Gunthorpe wrote: > On Thu, Oct 17, 2024 at 10:58:22PM -0700, Yi Liu wrote: >> The iommu drivers are on the way to drop the remove_dev_pasid op by >> extending the blocked_domain to support PASID. However, this cannot be >> done in one shot. So far, the Intel iommu and the ARM SMMUv3 driver have >> supported it, while the AMD iommu driver has not yet. During this >> transition, the IOMMU core needs to support both ways to destroy the >> attachment of device/PASID and domain. > > Let's just fix AMD? cool. > diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c > index 9e25b92c68affa..806849cc997631 100644 > --- a/drivers/iommu/amd/iommu.c > +++ b/drivers/iommu/amd/iommu.c > @@ -2437,10 +2437,18 @@ static int blocked_domain_attach_device(struct iommu_domain *domain, > return 0; > } > > +static int blocked_domain_set_dev_pasid(struct iommu_domain *domain, > + struct device *dev, ioasid_t pasid) > +{ > + amd_iommu_remove_dev_pasid(dev, pasid, domain); > + return 0; > +} > + > static struct iommu_domain blocked_domain = { > .type = IOMMU_DOMAIN_BLOCKED, > .ops = &(const struct iommu_domain_ops) { > .attach_dev = blocked_domain_attach_device, > + .set_dev_pasid = blocked_domain_set_dev_pasid, > } > }; > > Jason
On Mon, Oct 21, 2024 at 05:35:38PM +0800, Yi Liu wrote: > On 2024/10/18 22:39, Jason Gunthorpe wrote: > > On Thu, Oct 17, 2024 at 10:58:22PM -0700, Yi Liu wrote: > > > The iommu drivers are on the way to drop the remove_dev_pasid op by > > > extending the blocked_domain to support PASID. However, this cannot be > > > done in one shot. So far, the Intel iommu and the ARM SMMUv3 driver have > > > supported it, while the AMD iommu driver has not yet. During this > > > transition, the IOMMU core needs to support both ways to destroy the > > > attachment of device/PASID and domain. > > > > Let's just fix AMD? > > cool. You could probably do better on this and fixup amd_iommu_remove_dev_pasid() to have the right signature directly, like the other drivers did Jason
On 2024/10/21 20:33, Jason Gunthorpe wrote: > On Mon, Oct 21, 2024 at 05:35:38PM +0800, Yi Liu wrote: >> On 2024/10/18 22:39, Jason Gunthorpe wrote: >>> On Thu, Oct 17, 2024 at 10:58:22PM -0700, Yi Liu wrote: >>>> The iommu drivers are on the way to drop the remove_dev_pasid op by >>>> extending the blocked_domain to support PASID. However, this cannot be >>>> done in one shot. So far, the Intel iommu and the ARM SMMUv3 driver have >>>> supported it, while the AMD iommu driver has not yet. During this >>>> transition, the IOMMU core needs to support both ways to destroy the >>>> attachment of device/PASID and domain. >>> >>> Let's just fix AMD? >> >> cool. > > You could probably do better on this and fixup > amd_iommu_remove_dev_pasid() to have the right signature directly, > like the other drivers did It might make sense to move the amd_iommu_remove_dev_pasid() to the drivers/iommu/amd/iommu.c and make it to be the blocked_domain_set_dev_pasid(). diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h index b11b014fa82d..55ac1ad10fb3 100644 --- a/drivers/iommu/amd/amd_iommu.h +++ b/drivers/iommu/amd/amd_iommu.h @@ -54,8 +54,8 @@ void amd_iommu_domain_free(struct iommu_domain *dom); int iommu_sva_set_dev_pasid(struct iommu_domain *domain, struct device *dev, ioasid_t pasid, struct iommu_domain *old); -void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, - struct iommu_domain *domain); +void remove_pdom_dev_pasid(struct protection_domain *pdom, + struct device *dev, ioasid_t pasid); /* SVA/PASID */ bool amd_iommu_pasid_supported(void); diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c index 8364cd6fa47d..f807c4956a75 100644 --- a/drivers/iommu/amd/iommu.c +++ b/drivers/iommu/amd/iommu.c @@ -2437,6 +2437,30 @@ static int blocked_domain_attach_device(struct iommu_domain *domain, return 0; } +static int blocked_domain_set_dev_pasid(struct iommu_domain *domain, + struct device *dev, ioasid_t pasid, + struct iommu_domain *old) +{ + struct protection_domain *pdom = to_pdomain(old); + unsigned long flags; + + if (old->type != IOMMU_DOMAIN_SVA) + return -EINVAL; + + if (!is_pasid_valid(dev_iommu_priv_get(dev), pasid)) + return 0; + + pdom = to_pdomain(domain); + + spin_lock_irqsave(&pdom->lock, flags); + + /* Remove PASID from dev_data_list */ + remove_pdom_dev_pasid(pdom, dev, pasid); + + spin_unlock_irqrestore(&pdom->lock, flags); + return 0; +} + static struct iommu_domain blocked_domain = { .type = IOMMU_DOMAIN_BLOCKED, .ops = &(const struct iommu_domain_ops) { diff --git a/drivers/iommu/amd/pasid.c b/drivers/iommu/amd/pasid.c index 8c73a30c2800..c43c7286c872 100644 --- a/drivers/iommu/amd/pasid.c +++ b/drivers/iommu/amd/pasid.c @@ -39,8 +39,8 @@ static void remove_dev_pasid(struct pdom_dev_data *pdom_dev_data) } /* Clear PASID from device GCR3 table and remove pdom_dev_data from list */ -static void remove_pdom_dev_pasid(struct protection_domain *pdom, - struct device *dev, ioasid_t pasid) +void remove_pdom_dev_pasid(struct protection_domain *pdom, + struct device *dev, ioasid_t pasid) { struct pdom_dev_data *pdom_dev_data; struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev); @@ -145,25 +145,6 @@ int iommu_sva_set_dev_pasid(struct iommu_domain *domain, return ret; } -void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, - struct iommu_domain *domain) -{ - struct protection_domain *sva_pdom; - unsigned long flags; - - if (!is_pasid_valid(dev_iommu_priv_get(dev), pasid)) - return; - - sva_pdom = to_pdomain(domain); - - spin_lock_irqsave(&sva_pdom->lock, flags); - - /* Remove PASID from dev_data_list */ - remove_pdom_dev_pasid(sva_pdom, dev, pasid); - - spin_unlock_irqrestore(&sva_pdom->lock, flags); -} - static void iommu_sva_domain_free(struct iommu_domain *domain) { struct protection_domain *sva_pdom = to_pdomain(domain);
Hi Yi, On 10/22/2024 6:21 PM, Yi Liu wrote: > On 2024/10/21 20:33, Jason Gunthorpe wrote: >> On Mon, Oct 21, 2024 at 05:35:38PM +0800, Yi Liu wrote: >>> On 2024/10/18 22:39, Jason Gunthorpe wrote: >>>> On Thu, Oct 17, 2024 at 10:58:22PM -0700, Yi Liu wrote: >>>>> The iommu drivers are on the way to drop the remove_dev_pasid op by >>>>> extending the blocked_domain to support PASID. However, this cannot be >>>>> done in one shot. So far, the Intel iommu and the ARM SMMUv3 driver have >>>>> supported it, while the AMD iommu driver has not yet. During this >>>>> transition, the IOMMU core needs to support both ways to destroy the >>>>> attachment of device/PASID and domain. >>>> >>>> Let's just fix AMD? >>> >>> cool. >> >> You could probably do better on this and fixup >> amd_iommu_remove_dev_pasid() to have the right signature directly, >> like the other drivers did > > It might make sense to move the amd_iommu_remove_dev_pasid() to the > drivers/iommu/amd/iommu.c and make it to be the blocked_domain_set_dev_pasid(). I wanted to keep all PASID code in pasid.c. I'd say for now lets keep it in pasid.c only. > > > diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h > index b11b014fa82d..55ac1ad10fb3 100644 > --- a/drivers/iommu/amd/amd_iommu.h > +++ b/drivers/iommu/amd/amd_iommu.h > @@ -54,8 +54,8 @@ void amd_iommu_domain_free(struct iommu_domain *dom); > int iommu_sva_set_dev_pasid(struct iommu_domain *domain, > struct device *dev, ioasid_t pasid, > struct iommu_domain *old); > -void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, > - struct iommu_domain *domain); > +void remove_pdom_dev_pasid(struct protection_domain *pdom, > + struct device *dev, ioasid_t pasid); > > /* SVA/PASID */ > bool amd_iommu_pasid_supported(void); > diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c > index 8364cd6fa47d..f807c4956a75 100644 > --- a/drivers/iommu/amd/iommu.c > +++ b/drivers/iommu/amd/iommu.c > @@ -2437,6 +2437,30 @@ static int blocked_domain_attach_device(struct > iommu_domain *domain, > return 0; > } > May be we should add comment here or at least explain it in patch description. Otherwise it may create confusion. Something like below Remove PASID from old domain and device GCR3 table. No need to attach PASID to blocked domain as clearing PASID from GCR3 table will make sure all DMAs for that PASID is blocked. > +static int blocked_domain_set_dev_pasid(struct iommu_domain *domain, > + struct device *dev, ioasid_t pasid, > + struct iommu_domain *old) > +{ > + struct protection_domain *pdom = to_pdomain(old); > + unsigned long flags; > + > + if (old->type != IOMMU_DOMAIN_SVA) > + return -EINVAL; > + > + if (!is_pasid_valid(dev_iommu_priv_get(dev), pasid)) > + return 0; > + > + pdom = to_pdomain(domain); This is redundant as you already set pdom to old domain. > + > + spin_lock_irqsave(&pdom->lock, flags); > + > + /* Remove PASID from dev_data_list */ > + remove_pdom_dev_pasid(pdom, dev, pasid); > + > + spin_unlock_irqrestore(&pdom->lock, flags); > + return 0; > +} > + > static struct iommu_domain blocked_domain = { > .type = IOMMU_DOMAIN_BLOCKED, > .ops = &(const struct iommu_domain_ops) { > diff --git a/drivers/iommu/amd/pasid.c b/drivers/iommu/amd/pasid.c > index 8c73a30c2800..c43c7286c872 100644 > --- a/drivers/iommu/amd/pasid.c > +++ b/drivers/iommu/amd/pasid.c > @@ -39,8 +39,8 @@ static void remove_dev_pasid(struct pdom_dev_data *pdom_dev_data) > } > > /* Clear PASID from device GCR3 table and remove pdom_dev_data from list */ > -static void remove_pdom_dev_pasid(struct protection_domain *pdom, > - struct device *dev, ioasid_t pasid) > +void remove_pdom_dev_pasid(struct protection_domain *pdom, > + struct device *dev, ioasid_t pasid) > { > struct pdom_dev_data *pdom_dev_data; > struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev); > @@ -145,25 +145,6 @@ int iommu_sva_set_dev_pasid(struct iommu_domain *domain, > return ret; > } > > -void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, > - struct iommu_domain *domain) > -{ > - struct protection_domain *sva_pdom; > - unsigned long flags; > - > - if (!is_pasid_valid(dev_iommu_priv_get(dev), pasid)) > - return; > - > - sva_pdom = to_pdomain(domain); > - > - spin_lock_irqsave(&sva_pdom->lock, flags); > - > - /* Remove PASID from dev_data_list */ > - remove_pdom_dev_pasid(sva_pdom, dev, pasid); > - > - spin_unlock_irqrestore(&sva_pdom->lock, flags); > -} > - > static void iommu_sva_domain_free(struct iommu_domain *domain) > { > struct protection_domain *sva_pdom = to_pdomain(domain); > >
On 2024/10/23 19:10, Vasant Hegde wrote: > Hi Yi, > > > On 10/22/2024 6:21 PM, Yi Liu wrote: >> On 2024/10/21 20:33, Jason Gunthorpe wrote: >>> On Mon, Oct 21, 2024 at 05:35:38PM +0800, Yi Liu wrote: >>>> On 2024/10/18 22:39, Jason Gunthorpe wrote: >>>>> On Thu, Oct 17, 2024 at 10:58:22PM -0700, Yi Liu wrote: >>>>>> The iommu drivers are on the way to drop the remove_dev_pasid op by >>>>>> extending the blocked_domain to support PASID. However, this cannot be >>>>>> done in one shot. So far, the Intel iommu and the ARM SMMUv3 driver have >>>>>> supported it, while the AMD iommu driver has not yet. During this >>>>>> transition, the IOMMU core needs to support both ways to destroy the >>>>>> attachment of device/PASID and domain. >>>>> >>>>> Let's just fix AMD? >>>> >>>> cool. >>> >>> You could probably do better on this and fixup >>> amd_iommu_remove_dev_pasid() to have the right signature directly, >>> like the other drivers did >> >> It might make sense to move the amd_iommu_remove_dev_pasid() to the >> drivers/iommu/amd/iommu.c and make it to be the blocked_domain_set_dev_pasid(). > > I wanted to keep all PASID code in pasid.c. I'd say for now lets keep it in > pasid.c only. ok. If so, we may just let the blocked_domain_set_dev_pasid() call amd_iommu_remove_dev_pasid(). >> >> >> diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h >> index b11b014fa82d..55ac1ad10fb3 100644 >> --- a/drivers/iommu/amd/amd_iommu.h >> +++ b/drivers/iommu/amd/amd_iommu.h >> @@ -54,8 +54,8 @@ void amd_iommu_domain_free(struct iommu_domain *dom); >> int iommu_sva_set_dev_pasid(struct iommu_domain *domain, >> struct device *dev, ioasid_t pasid, >> struct iommu_domain *old); >> -void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, >> - struct iommu_domain *domain); >> +void remove_pdom_dev_pasid(struct protection_domain *pdom, >> + struct device *dev, ioasid_t pasid); >> >> /* SVA/PASID */ >> bool amd_iommu_pasid_supported(void); >> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c >> index 8364cd6fa47d..f807c4956a75 100644 >> --- a/drivers/iommu/amd/iommu.c >> +++ b/drivers/iommu/amd/iommu.c >> @@ -2437,6 +2437,30 @@ static int blocked_domain_attach_device(struct >> iommu_domain *domain, >> return 0; >> } >> > > May be we should add comment here or at least explain it in patch description. > Otherwise it may create confusion. Something like below > > > Remove PASID from old domain and device GCR3 table. No need to attach PASID to > blocked domain as clearing PASID from GCR3 table will make sure all DMAs for > that PASID is blocked. got it. > > > > >> +static int blocked_domain_set_dev_pasid(struct iommu_domain *domain, >> + struct device *dev, ioasid_t pasid, >> + struct iommu_domain *old) >> +{ >> + struct protection_domain *pdom = to_pdomain(old); >> + unsigned long flags; >> + >> + if (old->type != IOMMU_DOMAIN_SVA) >> + return -EINVAL; >> + >> + if (!is_pasid_valid(dev_iommu_priv_get(dev), pasid)) >> + return 0; >> + >> + pdom = to_pdomain(domain); > > This is redundant as you already set pdom to old domain. yes. >> + >> + spin_lock_irqsave(&pdom->lock, flags); >> + >> + /* Remove PASID from dev_data_list */ >> + remove_pdom_dev_pasid(pdom, dev, pasid); >> + >> + spin_unlock_irqrestore(&pdom->lock, flags); >> + return 0; >> +} >> + >> static struct iommu_domain blocked_domain = { >> .type = IOMMU_DOMAIN_BLOCKED, >> .ops = &(const struct iommu_domain_ops) { >> diff --git a/drivers/iommu/amd/pasid.c b/drivers/iommu/amd/pasid.c >> index 8c73a30c2800..c43c7286c872 100644 >> --- a/drivers/iommu/amd/pasid.c >> +++ b/drivers/iommu/amd/pasid.c >> @@ -39,8 +39,8 @@ static void remove_dev_pasid(struct pdom_dev_data *pdom_dev_data) >> } >> >> /* Clear PASID from device GCR3 table and remove pdom_dev_data from list */ >> -static void remove_pdom_dev_pasid(struct protection_domain *pdom, >> - struct device *dev, ioasid_t pasid) >> +void remove_pdom_dev_pasid(struct protection_domain *pdom, >> + struct device *dev, ioasid_t pasid) >> { >> struct pdom_dev_data *pdom_dev_data; >> struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev); >> @@ -145,25 +145,6 @@ int iommu_sva_set_dev_pasid(struct iommu_domain *domain, >> return ret; >> } >> >> -void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, >> - struct iommu_domain *domain) >> -{ >> - struct protection_domain *sva_pdom; >> - unsigned long flags; >> - >> - if (!is_pasid_valid(dev_iommu_priv_get(dev), pasid)) >> - return; >> - >> - sva_pdom = to_pdomain(domain); >> - >> - spin_lock_irqsave(&sva_pdom->lock, flags); >> - >> - /* Remove PASID from dev_data_list */ >> - remove_pdom_dev_pasid(sva_pdom, dev, pasid); >> - >> - spin_unlock_irqrestore(&sva_pdom->lock, flags); >> -} >> - >> static void iommu_sva_domain_free(struct iommu_domain *domain) >> { >> struct protection_domain *sva_pdom = to_pdomain(domain); >> >>
Yi, On 10/29/2024 10:50 AM, Yi Liu wrote: > On 2024/10/23 19:10, Vasant Hegde wrote: >> Hi Yi, >> >> >> On 10/22/2024 6:21 PM, Yi Liu wrote: >>> On 2024/10/21 20:33, Jason Gunthorpe wrote: >>>> On Mon, Oct 21, 2024 at 05:35:38PM +0800, Yi Liu wrote: >>>>> On 2024/10/18 22:39, Jason Gunthorpe wrote: >>>>>> On Thu, Oct 17, 2024 at 10:58:22PM -0700, Yi Liu wrote: >>>>>>> The iommu drivers are on the way to drop the remove_dev_pasid op by >>>>>>> extending the blocked_domain to support PASID. However, this cannot be >>>>>>> done in one shot. So far, the Intel iommu and the ARM SMMUv3 driver have >>>>>>> supported it, while the AMD iommu driver has not yet. During this >>>>>>> transition, the IOMMU core needs to support both ways to destroy the >>>>>>> attachment of device/PASID and domain. >>>>>> >>>>>> Let's just fix AMD? >>>>> >>>>> cool. >>>> >>>> You could probably do better on this and fixup >>>> amd_iommu_remove_dev_pasid() to have the right signature directly, >>>> like the other drivers did >>> >>> It might make sense to move the amd_iommu_remove_dev_pasid() to the >>> drivers/iommu/amd/iommu.c and make it to be the blocked_domain_set_dev_pasid(). >> >> I wanted to keep all PASID code in pasid.c. I'd say for now lets keep it in >> pasid.c only. > > ok. If so, we may just let the blocked_domain_set_dev_pasid() call > amd_iommu_remove_dev_pasid(). Sure. Lets do that for now. -Vasant
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index f3f81c04b8fb..9266e4ebebc2 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -3324,6 +3324,28 @@ bool iommu_group_dma_owner_claimed(struct iommu_group *group) } EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed); +/* + * This is gated by AMD's blocked domain pasid support, it should be + * dropped once AMD iommu driver is ready. + */ +static void iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, + struct iommu_domain *domain) +{ + const struct iommu_ops *ops = dev_iommu_ops(dev); + struct iommu_domain *blocked_domain = ops->blocked_domain; + int ret = 1; + + if (blocked_domain->ops->set_dev_pasid) { + ret = blocked_domain->ops->set_dev_pasid(blocked_domain, + dev, pasid, domain); + } else if (ops->remove_dev_pasid) { + ops->remove_dev_pasid(dev, pasid, domain); + ret = 0; + } + + WARN_ON(ret); +} + static int __iommu_set_group_pasid(struct iommu_domain *domain, struct iommu_group *group, ioasid_t pasid) { @@ -3342,11 +3364,9 @@ static int __iommu_set_group_pasid(struct iommu_domain *domain, err_revert: last_gdev = device; for_each_group_device(group, device) { - const struct iommu_ops *ops = dev_iommu_ops(device->dev); - if (device == last_gdev) break; - ops->remove_dev_pasid(device->dev, pasid, domain); + iommu_remove_dev_pasid(device->dev, pasid, domain); } return ret; } @@ -3356,11 +3376,9 @@ static void __iommu_remove_group_pasid(struct iommu_group *group, struct iommu_domain *domain) { struct group_device *device; - const struct iommu_ops *ops; for_each_group_device(group, device) { - ops = dev_iommu_ops(device->dev); - ops->remove_dev_pasid(device->dev, pasid, domain); + iommu_remove_dev_pasid(device->dev, pasid, domain); } }