| Message ID | 20241104131842.13303-12-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Make set_dev_pasid op supporting domain replacement | expand |
On 11/4/24 21:18, Yi Liu wrote: > From: Lu Baolu <baolu.lu@linux.intel.com> > > Add intel_nested_set_dev_pasid() to set a nested type domain to a PASID > of a device. > Co-developed-by: Lu Baolu <baolu.lu@linux.intel.com> > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > Co-developed-by: Yi Liu <yi.l.liu@intel.com> > Signed-off-by: Yi Liu <yi.l.liu@intel.com> And convert the patch author to you. > --- > drivers/iommu/intel/iommu.c | 2 +- > drivers/iommu/intel/iommu.h | 7 ++++++ > drivers/iommu/intel/nested.c | 43 ++++++++++++++++++++++++++++++++++++ > drivers/iommu/intel/pasid.h | 11 +++++++++ > 4 files changed, 62 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c > index 7e82b3a4bba7..7f1ca3c342a3 100644 > --- a/drivers/iommu/intel/iommu.c > +++ b/drivers/iommu/intel/iommu.c > @@ -1944,7 +1944,7 @@ static int domain_setup_first_level(struct intel_iommu *iommu, > flags); > } > > -static bool dev_is_real_dma_subdevice(struct device *dev) > +bool dev_is_real_dma_subdevice(struct device *dev) How about making this a static inline in the header? > { > return dev && dev_is_pci(dev) && > pci_real_dma_dev(to_pci_dev(dev)) != to_pci_dev(dev); > diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h > index 8e7ffb421ac4..55478d7b64cf 100644 > --- a/drivers/iommu/intel/iommu.h > +++ b/drivers/iommu/intel/iommu.h > @@ -818,6 +818,11 @@ domain_id_iommu(struct dmar_domain *domain, struct intel_iommu *iommu) > return info->did; > } > > +static inline int domain_type_is_nested(struct dmar_domain *domain) > +{ > + return domain->domain.type == IOMMU_DOMAIN_NESTED; > +} Why do you need this? > + > /* > * 0: readable > * 1: writable > @@ -1225,6 +1230,8 @@ void __iommu_flush_iotlb(struct intel_iommu *iommu, u16 did, u64 addr, > */ > #define QI_OPT_WAIT_DRAIN BIT(0) > > +bool dev_is_real_dma_subdevice(struct device *dev); > + > int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu); > void domain_detach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu); > void device_block_translation(struct device *dev); > diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c > index 3ce3c4fd210e..890087f3509f 100644 > --- a/drivers/iommu/intel/nested.c > +++ b/drivers/iommu/intel/nested.c > @@ -130,8 +130,51 @@ static int intel_nested_cache_invalidate_user(struct iommu_domain *domain, > return ret; > } > > +static int intel_nested_set_dev_pasid(struct iommu_domain *domain, > + struct device *dev, ioasid_t pasid, > + struct iommu_domain *old) > +{ > + struct device_domain_info *info = dev_iommu_priv_get(dev); > + struct dmar_domain *dmar_domain = to_dmar_domain(domain); > + struct intel_iommu *iommu = info->iommu; > + struct dev_pasid_info *dev_pasid; > + int ret; > + > + /* No SVA domain replacement usage so far */ > + if (old && old->type == IOMMU_DOMAIN_SVA) > + return -EOPNOTSUPP; No need for this check from driver's point of view. If there is really a need, it should go to the iommu core. > + > + if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev)) > + return -EOPNOTSUPP;> + > + if (context_copied(iommu, info->bus, info->devfn)) > + return -EBUSY; > + > + ret = prepare_domain_attach_device(&dmar_domain->s2_domain->domain, > + dev); > + if (ret) > + return ret; > + > + dev_pasid = domain_add_dev_pasid(domain, dev, pasid); > + if (IS_ERR(dev_pasid)) > + return PTR_ERR(dev_pasid); > + > + ret = domain_setup_nested(iommu, dmar_domain, dev, pasid, old); > + if (ret) > + goto out_remove_dev_pasid; > + > + domain_remove_dev_pasid(old, dev, pasid); > + > + return 0; > + > +out_remove_dev_pasid: > + domain_remove_dev_pasid(domain, dev, pasid); > + return ret; > +} > + > static const struct iommu_domain_ops intel_nested_domain_ops = { > .attach_dev = intel_nested_attach_dev, > + .set_dev_pasid = intel_nested_set_dev_pasid, > .free = intel_nested_domain_free, > .cache_invalidate_user = intel_nested_cache_invalidate_user, > }; > diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h > index a3b5945a1812..31a4e7c01853 100644 > --- a/drivers/iommu/intel/pasid.h > +++ b/drivers/iommu/intel/pasid.h > @@ -335,6 +335,17 @@ static inline int domain_setup_passthrough(struct intel_iommu *iommu, > return intel_pasid_setup_pass_through(iommu, dev, pasid); > } > > +static inline int domain_setup_nested(struct intel_iommu *iommu, > + struct dmar_domain *domain, > + struct device *dev, ioasid_t pasid, > + struct iommu_domain *old) > +{ > + if (old) > + return intel_pasid_replace_nested(iommu, dev, > + pasid, domain); > + return intel_pasid_setup_nested(iommu, dev, pasid, domain); > +} > + > void intel_pasid_tear_down_entry(struct intel_iommu *iommu, > struct device *dev, u32 pasid, > bool fault_ignore); Others look good to me. -- baolu
On 2024/11/5 11:38, Baolu Lu wrote: > On 11/4/24 21:18, Yi Liu wrote: >> From: Lu Baolu <baolu.lu@linux.intel.com> >> >> Add intel_nested_set_dev_pasid() to set a nested type domain to a PASID >> of a device. >> > > Co-developed-by: Lu Baolu <baolu.lu@linux.intel.com> >> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> >> Co-developed-by: Yi Liu <yi.l.liu@intel.com> >> Signed-off-by: Yi Liu <yi.l.liu@intel.com> > > And convert the patch author to you. ok. > >> --- >> drivers/iommu/intel/iommu.c | 2 +- >> drivers/iommu/intel/iommu.h | 7 ++++++ >> drivers/iommu/intel/nested.c | 43 ++++++++++++++++++++++++++++++++++++ >> drivers/iommu/intel/pasid.h | 11 +++++++++ >> 4 files changed, 62 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c >> index 7e82b3a4bba7..7f1ca3c342a3 100644 >> --- a/drivers/iommu/intel/iommu.c >> +++ b/drivers/iommu/intel/iommu.c >> @@ -1944,7 +1944,7 @@ static int domain_setup_first_level(struct >> intel_iommu *iommu, >> flags); >> } >> -static bool dev_is_real_dma_subdevice(struct device *dev) >> +bool dev_is_real_dma_subdevice(struct device *dev) > > How about making this a static inline in the header? got it. > >> { >> return dev && dev_is_pci(dev) && >> pci_real_dma_dev(to_pci_dev(dev)) != to_pci_dev(dev); >> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h >> index 8e7ffb421ac4..55478d7b64cf 100644 >> --- a/drivers/iommu/intel/iommu.h >> +++ b/drivers/iommu/intel/iommu.h >> @@ -818,6 +818,11 @@ domain_id_iommu(struct dmar_domain *domain, struct >> intel_iommu *iommu) >> return info->did; >> } >> +static inline int domain_type_is_nested(struct dmar_domain *domain) >> +{ >> + return domain->domain.type == IOMMU_DOMAIN_NESTED; >> +} > > Why do you need this? sigh, it should be dropped as we non longer share set_dev_pasid callback with paging domain. >> + >> /* >> * 0: readable >> * 1: writable >> @@ -1225,6 +1230,8 @@ void __iommu_flush_iotlb(struct intel_iommu *iommu, >> u16 did, u64 addr, >> */ >> #define QI_OPT_WAIT_DRAIN BIT(0) >> +bool dev_is_real_dma_subdevice(struct device *dev); >> + >> int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu >> *iommu); >> void domain_detach_iommu(struct dmar_domain *domain, struct intel_iommu >> *iommu); >> void device_block_translation(struct device *dev); >> diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c >> index 3ce3c4fd210e..890087f3509f 100644 >> --- a/drivers/iommu/intel/nested.c >> +++ b/drivers/iommu/intel/nested.c >> @@ -130,8 +130,51 @@ static int intel_nested_cache_invalidate_user(struct >> iommu_domain *domain, >> return ret; >> } >> +static int intel_nested_set_dev_pasid(struct iommu_domain *domain, >> + struct device *dev, ioasid_t pasid, >> + struct iommu_domain *old) >> +{ >> + struct device_domain_info *info = dev_iommu_priv_get(dev); >> + struct dmar_domain *dmar_domain = to_dmar_domain(domain); >> + struct intel_iommu *iommu = info->iommu; >> + struct dev_pasid_info *dev_pasid; >> + int ret; >> + >> + /* No SVA domain replacement usage so far */ >> + if (old && old->type == IOMMU_DOMAIN_SVA) >> + return -EOPNOTSUPP; > > No need for this check from driver's point of view. If there is really a > need, it should go to the iommu core. I'm going to drop it per the discussion in patch 10. > >> + >> + if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev)) > > + return -EOPNOTSUPP;> + >> + if (context_copied(iommu, info->bus, info->devfn)) >> + return -EBUSY; >> + >> + ret = prepare_domain_attach_device(&dmar_domain->s2_domain->domain, >> + dev); >> + if (ret) >> + return ret; >> + >> + dev_pasid = domain_add_dev_pasid(domain, dev, pasid); >> + if (IS_ERR(dev_pasid)) >> + return PTR_ERR(dev_pasid); >> + >> + ret = domain_setup_nested(iommu, dmar_domain, dev, pasid, old); >> + if (ret) >> + goto out_remove_dev_pasid; >> + >> + domain_remove_dev_pasid(old, dev, pasid); >> + >> + return 0; >> + >> +out_remove_dev_pasid: >> + domain_remove_dev_pasid(domain, dev, pasid); >> + return ret; >> +} >> + >> static const struct iommu_domain_ops intel_nested_domain_ops = { >> .attach_dev = intel_nested_attach_dev, >> + .set_dev_pasid = intel_nested_set_dev_pasid, >> .free = intel_nested_domain_free, >> .cache_invalidate_user = intel_nested_cache_invalidate_user, >> }; >> diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h >> index a3b5945a1812..31a4e7c01853 100644 >> --- a/drivers/iommu/intel/pasid.h >> +++ b/drivers/iommu/intel/pasid.h >> @@ -335,6 +335,17 @@ static inline int domain_setup_passthrough(struct >> intel_iommu *iommu, >> return intel_pasid_setup_pass_through(iommu, dev, pasid); >> } >> +static inline int domain_setup_nested(struct intel_iommu *iommu, >> + struct dmar_domain *domain, >> + struct device *dev, ioasid_t pasid, >> + struct iommu_domain *old) >> +{ >> + if (old) >> + return intel_pasid_replace_nested(iommu, dev, >> + pasid, domain); >> + return intel_pasid_setup_nested(iommu, dev, pasid, domain); >> +} >> + >> void intel_pasid_tear_down_entry(struct intel_iommu *iommu, >> struct device *dev, u32 pasid, >> bool fault_ignore); > > Others look good to me. > > -- > baolu
> From: Liu, Yi L <yi.l.liu@intel.com> > Sent: Monday, November 4, 2024 9:19 PM > > From: Lu Baolu <baolu.lu@linux.intel.com> > > Add intel_nested_set_dev_pasid() to set a nested type domain to a PASID > of a device. > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > Co-developed-by: Yi Liu <yi.l.liu@intel.com> > Signed-off-by: Yi Liu <yi.l.liu@intel.com> with Baolo's comment addressed: Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> From: Liu, Yi L <yi.l.liu@intel.com> > Sent: Monday, November 4, 2024 9:19 PM > > + > + dev_pasid = domain_add_dev_pasid(domain, dev, pasid); > + if (IS_ERR(dev_pasid)) > + return PTR_ERR(dev_pasid); > + > + ret = domain_setup_nested(iommu, dmar_domain, dev, pasid, old); > + if (ret) > + goto out_remove_dev_pasid; > + > + domain_remove_dev_pasid(old, dev, pasid); > + forgot one thing. Why not required to create a debugfs entry for nested dev_pasid?
On 2024/11/6 16:17, Tian, Kevin wrote: >> From: Liu, Yi L <yi.l.liu@intel.com> >> Sent: Monday, November 4, 2024 9:19 PM >> >> + >> + dev_pasid = domain_add_dev_pasid(domain, dev, pasid); >> + if (IS_ERR(dev_pasid)) >> + return PTR_ERR(dev_pasid); >> + >> + ret = domain_setup_nested(iommu, dmar_domain, dev, pasid, old); >> + if (ret) >> + goto out_remove_dev_pasid; >> + >> + domain_remove_dev_pasid(old, dev, pasid); >> + > > forgot one thing. Why not required to create a debugfs entry for > nested dev_pasid? This debugfs node is only created for paging domain.
On 2024/11/6 16:41, Baolu Lu wrote: > On 2024/11/6 16:17, Tian, Kevin wrote: >>> From: Liu, Yi L <yi.l.liu@intel.com> >>> Sent: Monday, November 4, 2024 9:19 PM >>> >>> + >>> + dev_pasid = domain_add_dev_pasid(domain, dev, pasid); >>> + if (IS_ERR(dev_pasid)) >>> + return PTR_ERR(dev_pasid); >>> + >>> + ret = domain_setup_nested(iommu, dmar_domain, dev, pasid, old); >>> + if (ret) >>> + goto out_remove_dev_pasid; >>> + >>> + domain_remove_dev_pasid(old, dev, pasid); >>> + >> >> forgot one thing. Why not required to create a debugfs entry for >> nested dev_pasid? > > This debugfs node is only created for paging domain. I think Kevin got one point. The debugfs is added when paging domain is attached. How about the paging domains that is only used as nested parent domain. We seem to lack a debugfs node for such paging domains.
On 2024/11/6 17:14, Yi Liu wrote: > On 2024/11/6 16:41, Baolu Lu wrote: >> On 2024/11/6 16:17, Tian, Kevin wrote: >>>> From: Liu, Yi L <yi.l.liu@intel.com> >>>> Sent: Monday, November 4, 2024 9:19 PM >>>> >>>> + >>>> + dev_pasid = domain_add_dev_pasid(domain, dev, pasid); >>>> + if (IS_ERR(dev_pasid)) >>>> + return PTR_ERR(dev_pasid); >>>> + >>>> + ret = domain_setup_nested(iommu, dmar_domain, dev, pasid, old); >>>> + if (ret) >>>> + goto out_remove_dev_pasid; >>>> + >>>> + domain_remove_dev_pasid(old, dev, pasid); >>>> + >>> >>> forgot one thing. Why not required to create a debugfs entry for >>> nested dev_pasid? >> >> This debugfs node is only created for paging domain. > > I think Kevin got one point. The debugfs is added when paging domain > is attached. How about the paging domains that is only used as nested > parent domain. We seem to lack a debugfs node for such paging domains. Are you talking about the nested parent domain? It's also a paging domain, hence a debugfs node will be created.
On 2024/11/6 18:45, Baolu Lu wrote: > On 2024/11/6 17:14, Yi Liu wrote: >> On 2024/11/6 16:41, Baolu Lu wrote: >>> On 2024/11/6 16:17, Tian, Kevin wrote: >>>>> From: Liu, Yi L <yi.l.liu@intel.com> >>>>> Sent: Monday, November 4, 2024 9:19 PM >>>>> >>>>> + >>>>> + dev_pasid = domain_add_dev_pasid(domain, dev, pasid); >>>>> + if (IS_ERR(dev_pasid)) >>>>> + return PTR_ERR(dev_pasid); >>>>> + >>>>> + ret = domain_setup_nested(iommu, dmar_domain, dev, pasid, old); >>>>> + if (ret) >>>>> + goto out_remove_dev_pasid; >>>>> + >>>>> + domain_remove_dev_pasid(old, dev, pasid); >>>>> + >>>> >>>> forgot one thing. Why not required to create a debugfs entry for >>>> nested dev_pasid? >>> >>> This debugfs node is only created for paging domain. >> >> I think Kevin got one point. The debugfs is added when paging domain >> is attached. How about the paging domains that is only used as nested >> parent domain. We seem to lack a debugfs node for such paging domains. > > Are you talking about the nested parent domain? It's also a paging > domain, hence a debugfs node will be created. yes, nested parent domains. But as I mentioned, the debugfs node is created only in the attach point so far. While the nested attach does not attach the nested parent, it is subjected to the paging_domain_compatible() check and contributes its pgd to act as SS page table in the pasid entry. So it's missed though it should be in another patch to add it.
On 2024/11/6 19:00, Yi Liu wrote: > On 2024/11/6 18:45, Baolu Lu wrote: >> On 2024/11/6 17:14, Yi Liu wrote: >>> On 2024/11/6 16:41, Baolu Lu wrote: >>>> On 2024/11/6 16:17, Tian, Kevin wrote: >>>>>> From: Liu, Yi L <yi.l.liu@intel.com> >>>>>> Sent: Monday, November 4, 2024 9:19 PM >>>>>> >>>>>> + >>>>>> + dev_pasid = domain_add_dev_pasid(domain, dev, pasid); >>>>>> + if (IS_ERR(dev_pasid)) >>>>>> + return PTR_ERR(dev_pasid); >>>>>> + >>>>>> + ret = domain_setup_nested(iommu, dmar_domain, dev, pasid, old); >>>>>> + if (ret) >>>>>> + goto out_remove_dev_pasid; >>>>>> + >>>>>> + domain_remove_dev_pasid(old, dev, pasid); >>>>>> + >>>>> >>>>> forgot one thing. Why not required to create a debugfs entry for >>>>> nested dev_pasid? >>>> >>>> This debugfs node is only created for paging domain. >>> >>> I think Kevin got one point. The debugfs is added when paging domain >>> is attached. How about the paging domains that is only used as nested >>> parent domain. We seem to lack a debugfs node for such paging domains. >> >> Are you talking about the nested parent domain? It's also a paging >> domain, hence a debugfs node will be created. > > yes, nested parent domains. But as I mentioned, the debugfs node is created > only in the attach point so far. While the nested attach does not attach > the nested parent, it is subjected to the paging_domain_compatible() > check and contributes its pgd to act as SS page table in the pasid entry. > So it's missed though it should be in another patch to add it. I see. Thanks!
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 7e82b3a4bba7..7f1ca3c342a3 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -1944,7 +1944,7 @@ static int domain_setup_first_level(struct intel_iommu *iommu, flags); } -static bool dev_is_real_dma_subdevice(struct device *dev) +bool dev_is_real_dma_subdevice(struct device *dev) { return dev && dev_is_pci(dev) && pci_real_dma_dev(to_pci_dev(dev)) != to_pci_dev(dev); diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h index 8e7ffb421ac4..55478d7b64cf 100644 --- a/drivers/iommu/intel/iommu.h +++ b/drivers/iommu/intel/iommu.h @@ -818,6 +818,11 @@ domain_id_iommu(struct dmar_domain *domain, struct intel_iommu *iommu) return info->did; } +static inline int domain_type_is_nested(struct dmar_domain *domain) +{ + return domain->domain.type == IOMMU_DOMAIN_NESTED; +} + /* * 0: readable * 1: writable @@ -1225,6 +1230,8 @@ void __iommu_flush_iotlb(struct intel_iommu *iommu, u16 did, u64 addr, */ #define QI_OPT_WAIT_DRAIN BIT(0) +bool dev_is_real_dma_subdevice(struct device *dev); + int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu); void domain_detach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu); void device_block_translation(struct device *dev); diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c index 3ce3c4fd210e..890087f3509f 100644 --- a/drivers/iommu/intel/nested.c +++ b/drivers/iommu/intel/nested.c @@ -130,8 +130,51 @@ static int intel_nested_cache_invalidate_user(struct iommu_domain *domain, return ret; } +static int intel_nested_set_dev_pasid(struct iommu_domain *domain, + struct device *dev, ioasid_t pasid, + struct iommu_domain *old) +{ + struct device_domain_info *info = dev_iommu_priv_get(dev); + struct dmar_domain *dmar_domain = to_dmar_domain(domain); + struct intel_iommu *iommu = info->iommu; + struct dev_pasid_info *dev_pasid; + int ret; + + /* No SVA domain replacement usage so far */ + if (old && old->type == IOMMU_DOMAIN_SVA) + return -EOPNOTSUPP; + + if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev)) + return -EOPNOTSUPP; + + if (context_copied(iommu, info->bus, info->devfn)) + return -EBUSY; + + ret = prepare_domain_attach_device(&dmar_domain->s2_domain->domain, + dev); + if (ret) + return ret; + + dev_pasid = domain_add_dev_pasid(domain, dev, pasid); + if (IS_ERR(dev_pasid)) + return PTR_ERR(dev_pasid); + + ret = domain_setup_nested(iommu, dmar_domain, dev, pasid, old); + if (ret) + goto out_remove_dev_pasid; + + domain_remove_dev_pasid(old, dev, pasid); + + return 0; + +out_remove_dev_pasid: + domain_remove_dev_pasid(domain, dev, pasid); + return ret; +} + static const struct iommu_domain_ops intel_nested_domain_ops = { .attach_dev = intel_nested_attach_dev, + .set_dev_pasid = intel_nested_set_dev_pasid, .free = intel_nested_domain_free, .cache_invalidate_user = intel_nested_cache_invalidate_user, }; diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h index a3b5945a1812..31a4e7c01853 100644 --- a/drivers/iommu/intel/pasid.h +++ b/drivers/iommu/intel/pasid.h @@ -335,6 +335,17 @@ static inline int domain_setup_passthrough(struct intel_iommu *iommu, return intel_pasid_setup_pass_through(iommu, dev, pasid); } +static inline int domain_setup_nested(struct intel_iommu *iommu, + struct dmar_domain *domain, + struct device *dev, ioasid_t pasid, + struct iommu_domain *old) +{ + if (old) + return intel_pasid_replace_nested(iommu, dev, + pasid, domain); + return intel_pasid_setup_nested(iommu, dev, pasid, domain); +} + void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev, u32 pasid, bool fault_ignore);