Messages in this thread | | | Date | Fri, 24 Jun 2022 14:02:34 +0800 | Subject | Re: [PATCH v3 1/1] iommu/vt-d: Fix RID2PASID setup/teardown failure | From | Ethan Zhao <> |
| |
Hi,
在 2022/6/23 14:57, Lu Baolu 写道: > The IOMMU driver shares the pasid table for PCI alias devices. When the > RID2PASID entry of the shared pasid table has been filled by the first > device, the subsequent device will encounter the "DMAR: Setup RID2PASID > failed" failure as the pasid entry has already been marked as present. > As the result, the IOMMU probing process will be aborted. > > On the contrary, when any alias device is hot-removed from the system, > for example, by writing to /sys/bus/pci/devices/.../remove, the shared > RID2PASID will be cleared without any notifications to other devices. > As the result, any DMAs from those rest devices are blocked. > > Sharing pasid table among PCI alias devices could save two memory pages > for devices underneath the PCIe-to-PCI bridges. Anyway, considering that > those devices are rare on modern platforms that support VT-d in scalable > mode and the saved memory is negligible, it's reasonable to remove this > part of immature code to make the driver feasible and stable. In my understanding, thus cleanning will make the pasid table become per-dev datastructure whatever the dev is pci-alias or not, and the pasid_pte_is_present(pte)will only check against every pci-alias' own private pasid table,the setup stagewouldn't break, so does the detach/release path, and little value to code otherreference counter like complex implenmataion, looks good to me !
Thanks,
Ethan
> Fixes: ef848b7e5a6a0 ("iommu/vt-d: Setup pasid entry for RID2PASID support") > Reported-by: Chenyi Qiang <chenyi.qiang@intel.com> > Reported-by: Ethan Zhao <haifeng.zhao@linux.intel.com> > Cc: stable@vger.kernel.org > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > --- > include/linux/intel-iommu.h | 3 -- > drivers/iommu/intel/pasid.h | 1 - > drivers/iommu/intel/iommu.c | 24 ------------- > drivers/iommu/intel/pasid.c | 69 ++----------------------------------- > 4 files changed, 3 insertions(+), 94 deletions(-) > > Change log: > v3: > - Ethan pointed out that there's also problem in the device release > path. Let's remove this part of immature code for now. > > v2: > - Add domain validity check in RID2PASID entry setup. > > diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h > index 4f29139bbfc3..5fcf89faa31a 100644 > --- a/include/linux/intel-iommu.h > +++ b/include/linux/intel-iommu.h > @@ -612,7 +612,6 @@ struct intel_iommu { > struct device_domain_info { > struct list_head link; /* link to domain siblings */ > struct list_head global; /* link to global list */ > - struct list_head table; /* link to pasid table */ > u32 segment; /* PCI segment number */ > u8 bus; /* PCI bus number */ > u8 devfn; /* PCI devfn number */ > @@ -729,8 +728,6 @@ extern int dmar_ir_support(void); > void *alloc_pgtable_page(int node); > void free_pgtable_page(void *vaddr); > struct intel_iommu *domain_get_iommu(struct dmar_domain *domain); > -int for_each_device_domain(int (*fn)(struct device_domain_info *info, > - void *data), void *data); > void iommu_flush_write_buffer(struct intel_iommu *iommu); > int intel_iommu_enable_pasid(struct intel_iommu *iommu, struct device *dev); > struct intel_iommu *device_to_iommu(struct device *dev, u8 *bus, u8 *devfn); > diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h > index 583ea67fc783..bf5b937848b4 100644 > --- a/drivers/iommu/intel/pasid.h > +++ b/drivers/iommu/intel/pasid.h > @@ -74,7 +74,6 @@ struct pasid_table { > void *table; /* pasid table pointer */ > int order; /* page order of pasid table */ > u32 max_pasid; /* max pasid */ > - struct list_head dev; /* device list */ > }; > > /* Get PRESENT bit of a PASID directory entry. */ > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c > index 44016594831d..5c0dce78586a 100644 > --- a/drivers/iommu/intel/iommu.c > +++ b/drivers/iommu/intel/iommu.c > @@ -320,30 +320,6 @@ EXPORT_SYMBOL_GPL(intel_iommu_gfx_mapped); > DEFINE_SPINLOCK(device_domain_lock); > static LIST_HEAD(device_domain_list); > > -/* > - * Iterate over elements in device_domain_list and call the specified > - * callback @fn against each element. > - */ > -int for_each_device_domain(int (*fn)(struct device_domain_info *info, > - void *data), void *data) > -{ > - int ret = 0; > - unsigned long flags; > - struct device_domain_info *info; > - > - spin_lock_irqsave(&device_domain_lock, flags); > - list_for_each_entry(info, &device_domain_list, global) { > - ret = fn(info, data); > - if (ret) { > - spin_unlock_irqrestore(&device_domain_lock, flags); > - return ret; > - } > - } > - spin_unlock_irqrestore(&device_domain_lock, flags); > - > - return 0; > -} > - > const struct iommu_ops intel_iommu_ops; > > static bool translation_pre_enabled(struct intel_iommu *iommu) > diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c > index cb4c1d0cf25c..17cad7c1f62d 100644 > --- a/drivers/iommu/intel/pasid.c > +++ b/drivers/iommu/intel/pasid.c > @@ -86,54 +86,6 @@ void vcmd_free_pasid(struct intel_iommu *iommu, u32 pasid) > /* > * Per device pasid table management: > */ > -static inline void > -device_attach_pasid_table(struct device_domain_info *info, > - struct pasid_table *pasid_table) > -{ > - info->pasid_table = pasid_table; > - list_add(&info->table, &pasid_table->dev); > -} > - > -static inline void > -device_detach_pasid_table(struct device_domain_info *info, > - struct pasid_table *pasid_table) > -{ > - info->pasid_table = NULL; > - list_del(&info->table); > -} > - > -struct pasid_table_opaque { > - struct pasid_table **pasid_table; > - int segment; > - int bus; > - int devfn; > -}; > - > -static int search_pasid_table(struct device_domain_info *info, void *opaque) > -{ > - struct pasid_table_opaque *data = opaque; > - > - if (info->iommu->segment == data->segment && > - info->bus == data->bus && > - info->devfn == data->devfn && > - info->pasid_table) { > - *data->pasid_table = info->pasid_table; > - return 1; > - } > - > - return 0; > -} > - > -static int get_alias_pasid_table(struct pci_dev *pdev, u16 alias, void *opaque) > -{ > - struct pasid_table_opaque *data = opaque; > - > - data->segment = pci_domain_nr(pdev->bus); > - data->bus = PCI_BUS_NUM(alias); > - data->devfn = alias & 0xff; > - > - return for_each_device_domain(&search_pasid_table, data); > -} > > /* > * Allocate a pasid table for @dev. It should be called in a > @@ -143,28 +95,18 @@ int intel_pasid_alloc_table(struct device *dev) > { > struct device_domain_info *info; > struct pasid_table *pasid_table; > - struct pasid_table_opaque data; > struct page *pages; > u32 max_pasid = 0; > - int ret, order; > - int size; > + int order, size; > > might_sleep(); > info = dev_iommu_priv_get(dev); > if (WARN_ON(!info || !dev_is_pci(dev) || info->pasid_table)) > return -EINVAL; > > - /* DMA alias device already has a pasid table, use it: */ > - data.pasid_table = &pasid_table; > - ret = pci_for_each_dma_alias(to_pci_dev(dev), > - &get_alias_pasid_table, &data); > - if (ret) > - goto attach_out; > - > pasid_table = kzalloc(sizeof(*pasid_table), GFP_KERNEL); > if (!pasid_table) > return -ENOMEM; > - INIT_LIST_HEAD(&pasid_table->dev); > > if (info->pasid_supported) > max_pasid = min_t(u32, pci_max_pasids(to_pci_dev(dev)), > @@ -182,9 +124,7 @@ int intel_pasid_alloc_table(struct device *dev) > pasid_table->table = page_address(pages); > pasid_table->order = order; > pasid_table->max_pasid = 1 << (order + PAGE_SHIFT + 3); > - > -attach_out: > - device_attach_pasid_table(info, pasid_table); > + info->pasid_table = pasid_table; > > return 0; > } > @@ -202,10 +142,7 @@ void intel_pasid_free_table(struct device *dev) > return; > > pasid_table = info->pasid_table; > - device_detach_pasid_table(info, pasid_table); > - > - if (!list_empty(&pasid_table->dev)) > - return; > + info->pasid_table = NULL; > > /* Free scalable mode PASID directory tables: */ > dir = pasid_table->table;
-- "firm, enduring, strong, and long-lived"
| |