[PATCH v5 4/8] iommu/vt-d: Aux-domain specific domain attach/detach
When multiple domains per device has been enabled by the device driver, the device will tag the default PASID for the domain to all DMA traffics out of the subset of this device; and the IOMMU should translate the DMA requests in PASID granularity. This adds the intel_iommu_aux_attach/detach_device() ops to support managing PASID granular translation structures when the device driver has enabled multiple domains per device. Cc: Ashok Raj Cc: Jacob Pan Cc: Kevin Tian Signed-off-by: Sanjay Kumar Signed-off-by: Liu Yi L Signed-off-by: Lu Baolu --- drivers/iommu/intel-iommu.c | 152 include/linux/intel-iommu.h | 10 +++ 2 files changed, 162 insertions(+) diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c index e9119d45a29d..b8fb6a4bd447 100644 --- a/drivers/iommu/intel-iommu.c +++ b/drivers/iommu/intel-iommu.c @@ -2482,6 +2482,7 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu, info->iommu = iommu; info->pasid_table = NULL; info->auxd_enabled = 0; + INIT_LIST_HEAD(&info->auxiliary_domains); if (dev && dev_is_pci(dev)) { struct pci_dev *pdev = to_pci_dev(info->dev); @@ -5058,6 +5059,131 @@ static void intel_iommu_domain_free(struct iommu_domain *domain) domain_exit(to_dmar_domain(domain)); } +/* + * Check whether a @domain could be attached to the @dev through the + * aux-domain attach/detach APIs. + */ +static inline bool +is_aux_domain(struct device *dev, struct iommu_domain *domain) +{ + struct device_domain_info *info = dev->archdata.iommu; + + return info && info->auxd_enabled && + domain->type == IOMMU_DOMAIN_UNMANAGED; +} + +static void auxiliary_link_device(struct dmar_domain *domain, + struct device *dev) +{ + struct device_domain_info *info = dev->archdata.iommu; + + assert_spin_locked(&device_domain_lock); + if (WARN_ON(!info)) + return; + + domain->auxd_refcnt++; + list_add(&domain->auxd, &info->auxiliary_domains); +} + +static void auxiliary_unlink_device(struct dmar_domain *domain, + struct device *dev) +{ + struct device_domain_info *info = dev->archdata.iommu; + + assert_spin_locked(&device_domain_lock); + if (WARN_ON(!info)) + return; + + list_del(&domain->auxd); + domain->auxd_refcnt--; + + if (!domain->auxd_refcnt && domain->default_pasid > 0) + intel_pasid_free_id(domain->default_pasid); +} + +static int aux_domain_add_dev(struct dmar_domain *domain, + struct device *dev) +{ + int ret; + u8 bus, devfn; + unsigned long flags; + struct intel_iommu *iommu; + + iommu = device_to_iommu(dev, &bus, &devfn); + if (!iommu) + return -ENODEV; + + if (domain->default_pasid <= 0) { + int pasid; + + pasid = intel_pasid_alloc_id(domain, PASID_MIN, +pci_max_pasids(to_pci_dev(dev)), +GFP_KERNEL); + if (pasid <= 0) { + pr_err("Can't allocate default pasid\n"); + return -ENODEV; + } + domain->default_pasid = pasid; + } + + spin_lock_irqsave(&device_domain_lock, flags); + /* +* iommu->lock must be held to attach domain to iommu and setup the +* pasid entry for second level translation. +*/ + spin_lock(&iommu->lock); + ret = domain_attach_iommu(domain, iommu); + if (ret) + goto attach_failed; + + /* Setup the PASID entry for mediated devices: */ + ret = intel_pasid_setup_second_level(iommu, domain, dev, +domain->default_pasid); + if (ret) + goto table_failed; + spin_unlock(&iommu->lock); + + auxiliary_link_device(domain, dev); + + spin_unlock_irqrestore(&device_domain_lock, flags); + + return 0; + +table_failed: + domain_detach_iommu(domain, iommu); +attach_failed: + spin_unlock(&iommu->lock); + spin_unlock_irqrestore(&device_domain_lock, flags); + if (!domain->auxd_refcnt && domain->default_pasid > 0) + intel_pasid_free_id(domain->default_pasid); + + return ret; +} + +static void aux_domain_remove_dev(struct dmar_domain *domain, + struct device *dev) +{ + struct device_domain_info *info; + struct intel_iommu *iommu; + unsigned long flags; + + if (!is_aux_domain(dev, &domain->domain)) + return; + + spin_lock_irqsave(&device_domain_lock, flags); + info = dev->archdata.iommu; + iommu = info->iommu; + + auxiliary_unlink_device(domain, dev); + + spin_loc
Re: [PATCH v5 4/8] iommu/vt-d: Aux-domain specific domain attach/detach
On Thu, 10 Jan 2019 11:00:23 +0800 Lu Baolu wrote: > When multiple domains per device has been enabled by the > device driver, the device will tag the default PASID for > the domain to all DMA traffics out of the subset of this > device; and the IOMMU should translate the DMA requests > in PASID granularity. > > This adds the intel_iommu_aux_attach/detach_device() ops > to support managing PASID granular translation structures > when the device driver has enabled multiple domains per > device. > > Cc: Ashok Raj > Cc: Jacob Pan > Cc: Kevin Tian > Signed-off-by: Sanjay Kumar > Signed-off-by: Liu Yi L > Signed-off-by: Lu Baolu The following is probably a rather naive review given I don't know the driver or hardware well at all. Still, it seems like things are a lot less balanced than I'd expect and isn't totally obvious to me why that is. > --- > drivers/iommu/intel-iommu.c | 152 > include/linux/intel-iommu.h | 10 +++ > 2 files changed, 162 insertions(+) > > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c > index e9119d45a29d..b8fb6a4bd447 100644 > --- a/drivers/iommu/intel-iommu.c > +++ b/drivers/iommu/intel-iommu.c > @@ -2482,6 +2482,7 @@ static struct dmar_domain > *dmar_insert_one_dev_info(struct intel_iommu *iommu, > info->iommu = iommu; > info->pasid_table = NULL; > info->auxd_enabled = 0; > + INIT_LIST_HEAD(&info->auxiliary_domains); > > if (dev && dev_is_pci(dev)) { > struct pci_dev *pdev = to_pci_dev(info->dev); > @@ -5058,6 +5059,131 @@ static void intel_iommu_domain_free(struct > iommu_domain *domain) > domain_exit(to_dmar_domain(domain)); > } > > +/* > + * Check whether a @domain could be attached to the @dev through the > + * aux-domain attach/detach APIs. > + */ > +static inline bool > +is_aux_domain(struct device *dev, struct iommu_domain *domain) I'm finding the distinction between an aux domain capability on a given device and whether one is actually in use to be obscured slightly in the function naming. This one for example is actually checking if we have a domain that is capable of being enabled for aux domain use, but not yet actually in that mode? Mind you I'm not sure I have a better answer for the naming. can_aux_domain_be_enabled? is_unattached_aux_domain? > +{ > + struct device_domain_info *info = dev->archdata.iommu; > + > + return info && info->auxd_enabled && > + domain->type == IOMMU_DOMAIN_UNMANAGED; > +} > + > +static void auxiliary_link_device(struct dmar_domain *domain, > + struct device *dev) > +{ > + struct device_domain_info *info = dev->archdata.iommu; > + > + assert_spin_locked(&device_domain_lock); > + if (WARN_ON(!info)) > + return; > + > + domain->auxd_refcnt++; > + list_add(&domain->auxd, &info->auxiliary_domains); > +} > + > +static void auxiliary_unlink_device(struct dmar_domain *domain, > + struct device *dev) > +{ > + struct device_domain_info *info = dev->archdata.iommu; > + > + assert_spin_locked(&device_domain_lock); > + if (WARN_ON(!info)) > + return; > + > + list_del(&domain->auxd); > + domain->auxd_refcnt--; > + > + if (!domain->auxd_refcnt && domain->default_pasid > 0) > + intel_pasid_free_id(domain->default_pasid); This seems unbalanced wrt to what is happening in auxiliary_link_device. If this is necessary then it would be good to have comments saying why. To my uniformed eye, looks like we could do this at the end of aux_domain_remove_dev, except that we need to hold the lock. As such perhaps it makes sense to do the pasid allocation under that lock in the first place? I'm not 100% sure, but is there a race if you get the final remove running against a new add currently? > +} > + > +static int aux_domain_add_dev(struct dmar_domain *domain, > + struct device *dev) > +{ > + int ret; > + u8 bus, devfn; > + unsigned long flags; > + struct intel_iommu *iommu; > + > + iommu = device_to_iommu(dev, &bus, &devfn); > + if (!iommu) > + return -ENODEV; > + > + if (domain->default_pasid <= 0) { device_domain_lock isn't held, so we might be in process of removing, see the pasid as set, just as it becomes unset and hence leave here without one set? > + int pasid; > + > + pasid = intel_pasid_alloc_id(domain, PASID_MIN, > + pci_max_pasids(to_pci_dev(dev)), > + GFP_KERNEL); > + if (pasid <= 0) { > + pr_err("Can't allocate default pasid\n"); > + return -ENODEV; > + } > + domain->default_pasid = pasid; > + } > + > + spin_lock_irqsave(&device_domain_lock, flags); > + /* > + * iommu->lock must be held to at
Re: [PATCH v5 4/8] iommu/vt-d: Aux-domain specific domain attach/detach
Hi, On 1/14/19 8:26 PM, Jonathan Cameron wrote: On Thu, 10 Jan 2019 11:00:23 +0800 Lu Baolu wrote: When multiple domains per device has been enabled by the device driver, the device will tag the default PASID for the domain to all DMA traffics out of the subset of this device; and the IOMMU should translate the DMA requests in PASID granularity. This adds the intel_iommu_aux_attach/detach_device() ops to support managing PASID granular translation structures when the device driver has enabled multiple domains per device. Cc: Ashok Raj Cc: Jacob Pan Cc: Kevin Tian Signed-off-by: Sanjay Kumar Signed-off-by: Liu Yi L Signed-off-by: Lu Baolu The following is probably a rather naive review given I don't know the driver or hardware well at all. Still, it seems like things are a lot less balanced than I'd expect and isn't totally obvious to me why that is. Thank you! --- drivers/iommu/intel-iommu.c | 152 include/linux/intel-iommu.h | 10 +++ 2 files changed, 162 insertions(+) diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c index e9119d45a29d..b8fb6a4bd447 100644 --- a/drivers/iommu/intel-iommu.c +++ b/drivers/iommu/intel-iommu.c @@ -2482,6 +2482,7 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu, info->iommu = iommu; info->pasid_table = NULL; info->auxd_enabled = 0; + INIT_LIST_HEAD(&info->auxiliary_domains); if (dev && dev_is_pci(dev)) { struct pci_dev *pdev = to_pci_dev(info->dev); @@ -5058,6 +5059,131 @@ static void intel_iommu_domain_free(struct iommu_domain *domain) domain_exit(to_dmar_domain(domain)); } +/* + * Check whether a @domain could be attached to the @dev through the + * aux-domain attach/detach APIs. + */ +static inline bool +is_aux_domain(struct device *dev, struct iommu_domain *domain) I'm finding the distinction between an aux domain capability on a given device and whether one is actually in use to be obscured slightly in the function naming. This one for example is actually checking if we have a domain that is capable of being enabled for aux domain use, but not yet actually in that mode? Mind you I'm not sure I have a better answer for the naming. can_aux_domain_be_enabled? is_unattached_aux_domain? device aux mode vs. normal mode === When we talk about the auxiliary mode (simply aux-mode), it means "the device works in aux-mode or normal mode". "normal mode" means that the device (and it's corresponding IOMMU) supports only RID (PCI Request ID) based DMA translation; while, aux-mode means the the device (and it's IOMMU) supports fine-grained DMA translation, like PASID based DMA translation with Intel VT-d scalable mode. We are adding below APIs to switch a device between these two modes: int iommu_dev_enable/disable_feature(dev, IOMMU_DEV_FEAT_AUX) And this API (still under discussion) to check which mode the device is working in: bool iommu_dev_has_feature(dev, IOMMU_DEV_FEAT_AUX) aux-domain == If a device is working in aux-mode and we are going to attach a domain to this device, we say "this domain will be attached to the device in aux mode", and simply "aux domain". So a domain is "normal" when it is going to attach to a device in normal mode; and is "aux-domain" when it is going to attach to a device in aux mode. +{ + struct device_domain_info *info = dev->archdata.iommu; + + return info && info->auxd_enabled && + domain->type == IOMMU_DOMAIN_UNMANAGED; +} + +static void auxiliary_link_device(struct dmar_domain *domain, + struct device *dev) +{ + struct device_domain_info *info = dev->archdata.iommu; + + assert_spin_locked(&device_domain_lock); + if (WARN_ON(!info)) + return; + + domain->auxd_refcnt++; + list_add(&domain->auxd, &info->auxiliary_domains); +} + +static void auxiliary_unlink_device(struct dmar_domain *domain, + struct device *dev) +{ + struct device_domain_info *info = dev->archdata.iommu; + + assert_spin_locked(&device_domain_lock); + if (WARN_ON(!info)) + return; + + list_del(&domain->auxd); + domain->auxd_refcnt--; + + if (!domain->auxd_refcnt && domain->default_pasid > 0) + intel_pasid_free_id(domain->default_pasid); This seems unbalanced wrt to what is happening in auxiliary_link_device. If this is necessary then it would be good to have comments saying why. To my uniformed eye, looks like we could do this at the end of aux_domain_remove_dev, except that we need to hold the lock. As such perhaps it makes sense to do the pasid allocation under that lock in the first place? I'm not 100% sure, but is there a race if you get the final remove running against a new add currently? Yes. This is unbalanced. I will move pasi
Re: [PATCH v5 4/8] iommu/vt-d: Aux-domain specific domain attach/detach
On Tue, 15 Jan 2019 10:10:21 +0800 Lu Baolu wrote: > Hi, > > On 1/14/19 8:26 PM, Jonathan Cameron wrote: > > On Thu, 10 Jan 2019 11:00:23 +0800 > > Lu Baolu wrote: > > > >> When multiple domains per device has been enabled by the > >> device driver, the device will tag the default PASID for > >> the domain to all DMA traffics out of the subset of this > >> device; and the IOMMU should translate the DMA requests > >> in PASID granularity. > >> > >> This adds the intel_iommu_aux_attach/detach_device() ops > >> to support managing PASID granular translation structures > >> when the device driver has enabled multiple domains per > >> device. > >> > >> Cc: Ashok Raj > >> Cc: Jacob Pan > >> Cc: Kevin Tian > >> Signed-off-by: Sanjay Kumar > >> Signed-off-by: Liu Yi L > >> Signed-off-by: Lu Baolu > > > > The following is probably a rather naive review given I don't know > > the driver or hardware well at all. Still, it seems like things > > are a lot less balanced than I'd expect and isn't totally obvious > > to me why that is. > > Thank you! You are welcome. ... > >> +/* > >> + * Check whether a @domain could be attached to the @dev through the > >> + * aux-domain attach/detach APIs. > >> + */ > >> +static inline bool > >> +is_aux_domain(struct device *dev, struct iommu_domain *domain) > > > > I'm finding the distinction between an aux domain capability on > > a given device and whether one is actually in use to be obscured > > slightly in the function naming. > > > > This one for example is actually checking if we have a domain > > that is capable of being enabled for aux domain use, but not > > yet actually in that mode? > > > > Mind you I'm not sure I have a better answer for the naming. > > can_aux_domain_be_enabled? is_unattached_aux_domain? > > > > > > device aux mode vs. normal mode > === > > When we talk about the auxiliary mode (simply aux-mode), it means "the > device works in aux-mode or normal mode". "normal mode" means that the > device (and it's corresponding IOMMU) supports only RID (PCI Request ID) > based DMA translation; while, aux-mode means the the device (and it's > IOMMU) supports fine-grained DMA translation, like PASID based DMA > translation with Intel VT-d scalable mode. > > We are adding below APIs to switch a device between these two modes: > > int iommu_dev_enable/disable_feature(dev, IOMMU_DEV_FEAT_AUX) > > And this API (still under discussion) to check which mode the device is > working in: > > bool iommu_dev_has_feature(dev, IOMMU_DEV_FEAT_AUX) > > aux-domain > == > > If a device is working in aux-mode and we are going to attach a domain > to this device, we say "this domain will be attached to the device in > aux mode", and simply "aux domain". So a domain is "normal" when it is > going to attach to a device in normal mode; and is "aux-domain" when it > is going to attach to a device in aux mode. Hmm.. OK I guess. It still feels like there is more need to refer to the docs than there should be. Still, your code and I may well never read it again so I don't mind :) > > > > >> +{ ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu