Re: [PATCH v2 02/11] PCI: Add ats_supported host bridge flag
On Wed, Mar 11, 2020 at 01:44:57PM +0100, Jean-Philippe Brucker wrote: > Each vendor has their own way of describing whether a host bridge > supports ATS. The Intel and AMD ACPI tables selectively enable or > disable ATS per device or sub-tree, while Arm has a single bit for each > host bridge. For those that need it, add an ats_supported bit to the > host bridge structure. Can you mention the specific ACPI tables here in the commit log? Maybe elaborate on the "for those that need it" bit? I'm not sure if you need it for the cases where DT or ACPI tells us directly for the host bridge, or if you need it for the more selective cases? I guess in one sense you *always* need it since you check the cached bit later. I don't understand the implications of this, especially the selective situation. Given your comment from the first posting, I thought this was a property of the host bridge, so I don't know what it means to say some devices support ATS but others don't. > Signed-off-by: Jean-Philippe Brucker > --- > v1->v2: try to improve the comment > --- > drivers/pci/probe.c | 8 > include/linux/pci.h | 1 + > 2 files changed, 9 insertions(+) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 512cb4312ddd..b5e36f06b40a 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -598,6 +598,14 @@ static void pci_init_host_bridge(struct pci_host_bridge > *bridge) > bridge->native_shpc_hotplug = 1; > bridge->native_pme = 1; > bridge->native_ltr = 1; > + > + /* > + * Some systems (ACPI IORT, device-tree) declare ATS support at the host > + * bridge, and clear this bit when ATS isn't supported. Others (ACPI > + * DMAR and IVRS) declare ATS support with a smaller granularity, and > + * need this bit set. > + */ > + bridge->ats_supported = 1; > } > > struct pci_host_bridge *pci_alloc_host_bridge(size_t priv) > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 3840a541a9de..9fe2e84d74d7 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -511,6 +511,7 @@ struct pci_host_bridge { > unsigned intnative_pme:1; /* OS may use PCIe PME */ > unsigned intnative_ltr:1; /* OS may use PCIe LTR */ > unsigned intpreserve_config:1; /* Preserve FW resource setup */ > + unsigned intats_supported:1; > > /* Resource alignment requirements */ > resource_size_t (*align_resource)(struct pci_dev *dev, > -- > 2.25.1 > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 05/11] PCI/ATS: Gather checks into pci_ats_supported()
On Wed, Mar 11, 2020 at 01:45:00PM +0100, Jean-Philippe Brucker wrote: > IOMMU drivers need to perform several tests when checking if a device > supports ATS. Move them all into a new function that returns true when > a device and its host bridge support ATS. > > Since pci_enable_ats() now calls pci_ats_supported(), the following > new checks are now common: > * whether a device is trusted. Devices plugged into external-facing > ports such as thunderbolt are untrusted. > * whether the host bridge supports ATS, which defaults to true unless > the firmware description states that ATS isn't supported by the host > bridge. > > Signed-off-by: Jean-Philippe Brucker Acked-by: Bjorn Helgaas > --- > drivers/pci/ats.c | 30 +- > include/linux/pci-ats.h | 3 +++ > 2 files changed, 32 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c > index 390e92f2d8d1..bbfd0d42b8b9 100644 > --- a/drivers/pci/ats.c > +++ b/drivers/pci/ats.c > @@ -30,6 +30,34 @@ void pci_ats_init(struct pci_dev *dev) > dev->ats_cap = pos; > } > > +/** > + * pci_ats_supported - check if the device can use ATS > + * @dev: the PCI device > + * > + * Returns true if the device supports ATS and is allowed to use it, false > + * otherwise. > + */ > +bool pci_ats_supported(struct pci_dev *dev) > +{ > + struct pci_host_bridge *bridge; > + > + if (!dev->ats_cap) > + return false; > + > + if (dev->untrusted) > + return false; > + > + bridge = pci_find_host_bridge(dev->bus); > + if (!bridge) > + return false; > + > + if (!bridge->ats_supported) > + return false; > + > + return true; I assume this is the same as return bridge->ats_supported; Only "assuming" because I'm not a C language lawyer, but I assume it does the obvious conversion from unsigned:1 to bool. > +} ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 04/11] ACPI/IORT: Check ATS capability in root complex node
On Wed, Mar 11, 2020 at 01:44:59PM +0100, Jean-Philippe Brucker wrote: > When initializing a PCI root bridge, copy its "ATS supported" attribute > into the root bridge. > > Acked-by: Hanjun Guo > Signed-off-by: Jean-Philippe Brucker > --- > drivers/acpi/arm64/iort.c | 27 +++ > drivers/acpi/pci_root.c | 3 +++ > include/linux/acpi_iort.h | 8 > 3 files changed, 38 insertions(+) > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > index ed3d2d1a7ae9..d99d7f5b51e1 100644 > --- a/drivers/acpi/arm64/iort.c > +++ b/drivers/acpi/arm64/iort.c > @@ -1633,6 +1633,33 @@ static void __init iort_enable_acs(struct > acpi_iort_node *iort_node) > } > } > } > + > +static acpi_status iort_match_host_bridge_callback(struct acpi_iort_node > *node, > +void *context) > +{ > + struct acpi_iort_root_complex *pci_rc; > + struct pci_host_bridge *host_bridge = context; > + > + pci_rc = (struct acpi_iort_root_complex *)node->node_data; > + > + return pci_domain_nr(host_bridge->bus) == pci_rc->pci_segment_number ? > + AE_OK : AE_NOT_FOUND; > +} > + > +void iort_pci_host_bridge_setup(struct pci_host_bridge *host_bridge) > +{ > + struct acpi_iort_node *node; > + struct acpi_iort_root_complex *pci_rc; > + > + node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX, > + iort_match_host_bridge_callback, host_bridge); > + if (!node) > + return; > + > + pci_rc = (struct acpi_iort_root_complex *)node->node_data; > + host_bridge->ats_supported = !!(pci_rc->ats_attribute & > + ACPI_IORT_ATS_SUPPORTED); > +} > #else > static inline void iort_enable_acs(struct acpi_iort_node *iort_node) { } > #endif > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index d1e666ef3fcc..eb2fb8f17c0b 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -6,6 +6,7 @@ > * Copyright (C) 2001, 2002 Paul Diefenbaugh > */ > > +#include > #include > #include > #include > @@ -917,6 +918,8 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root > *root, > if (!(root->osc_control_set & OSC_PCI_EXPRESS_LTR_CONTROL)) > host_bridge->native_ltr = 0; > > + iort_pci_host_bridge_setup(host_bridge); Similar comment as on the OF side. You mentioned at [1] that "it's important that we only enable ATS if the host bridge supports it". That should be captured in a commit log and comment somewhere here. That suggests to me that we should not set bridge->ats_supported = 1; by default in pci_init_host_bridge(), but rather leave it zero as it is by default, and then do things like: if (iort_pci_host_bridge_ats_supported(bridge)) bridge->ats_supported = 1; if (of_pci_host_bridge_ats_supported(bridge)) bridge->ats_supported = 1; I don't know what you do about IVRS and DMAR, which don't appear in this series except in the comment. [1] https://lore.kernel.org/r/20200213165049.508908-1-jean-phili...@linaro.org > /* >* Evaluate the "PCI Boot Configuration" _DSM Function. If it >* exists and returns 0, we must preserve any PCI resource > diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h > index 8e7e2ec37f1b..7b06871cc3aa 100644 > --- a/include/linux/acpi_iort.h > +++ b/include/linux/acpi_iort.h > @@ -10,6 +10,7 @@ > #include > #include > #include > +#include > > #define IORT_IRQ_MASK(irq) (irq & 0xULL) > #define IORT_IRQ_TRIGGER_MASK(irq) ((irq >> 32) & 0xULL) > @@ -55,4 +56,11 @@ int iort_iommu_msi_get_resv_regions(struct device *dev, > struct list_head *head) > { return 0; } > #endif > > +#if defined(CONFIG_ACPI_IORT) && defined(CONFIG_PCI) > +void iort_pci_host_bridge_setup(struct pci_host_bridge *host_bridge); > +#else > +static inline > +void iort_pci_host_bridge_setup(struct pci_host_bridge *host_bridge) { } > +#endif > + > #endif /* __ACPI_IORT_H__ */ > -- > 2.25.1 > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 03/11] PCI: OF: Check whether the host bridge supports ATS
On Wed, Mar 11, 2020 at 01:44:58PM +0100, Jean-Philippe Brucker wrote: > When setting up a generic host on a device-tree based system, copy the > ats-supported flag into the pci_host_bridge structure. > > Signed-off-by: Jean-Philippe Brucker > --- > v1->v2: keep the helper in pci-host-common.c > --- > drivers/pci/controller/pci-host-common.c | 11 +++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/pci/controller/pci-host-common.c > b/drivers/pci/controller/pci-host-common.c > index 250a3fc80ec6..2e800bc6ae7a 100644 > --- a/drivers/pci/controller/pci-host-common.c > +++ b/drivers/pci/controller/pci-host-common.c > @@ -54,6 +54,16 @@ static struct pci_config_window *gen_pci_init(struct > device *dev, > return ERR_PTR(err); > } > > +static void of_pci_host_check_ats(struct pci_host_bridge *bridge) > +{ > + struct device_node *np = bridge->bus->dev.of_node; > + > + if (!np) > + return; > + > + bridge->ats_supported = of_property_read_bool(np, "ats-supported"); > +} > + > int pci_host_common_probe(struct platform_device *pdev, > struct pci_ecam_ops *ops) > { > @@ -92,6 +102,7 @@ int pci_host_common_probe(struct platform_device *pdev, > return ret; > } > > + of_pci_host_check_ats(bridge); I would prefer to write this as a predicate instead of having the assignment be a side-effect, e.g., bridge->ats_supported = of_pci_host_ats_supported(bridge); If that works for you, Acked-by: Bjorn Helgaas > platform_set_drvdata(pdev, bridge->bus); > return 0; > } > -- > 2.25.1 > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[RFC PATCH] vfio: Ignore -ENODEV when getting MSI cookie
When we try to get an MSI cookie for a VFIO device, that can fail if CONFIG_IOMMU_DMA is not set. In this case iommu_get_msi_cookie() returns -ENODEV, and that should not be fatal. Ignore that case and proceed with the initialisation. This fixes VFIO with a platform device on the Calxeda Midway (no MSIs). Signed-off-by: Andre Przywara --- Hi, not sure this is the right fix, or we should rather check if the platform doesn't support MSIs at all (which doesn't seem to be easy to do). Or is this because arm-smmu.c always reserves an IOMMU_RESV_SW_MSI region? Also this seems to be long broken, actually since Eric introduced MSI support in 4.10-rc3, but at least since the initialisation order was fixed with f6810c15cf9. Grateful for any insight. Cheers, Andre drivers/vfio/vfio_iommu_type1.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index a177bf2c6683..467e217ef09a 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -1786,7 +1786,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data, if (resv_msi) { ret = iommu_get_msi_cookie(domain->domain, resv_msi_base); - if (ret) + if (ret && ret != -ENODEV) goto out_detach; } -- 2.17.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v3] [dma-coherent] Fix integer overflow in the reserved-memory dma allocation
pageno is an int and the PAGE_SHIFT shift is done on an int, overflowing if the memory is bigger than 2G This can be reproduced using for example a reserved-memory of 4G reserved-memory { #address-cells = <2>; #size-cells = <2>; ranges; reserved_dma: buffer@0 { compatible = "shared-dma-pool"; no-map; reg = <0x5 0x 0x1 0x0>; }; }; Signed-off-by: Kevin Grandemange --- Changes v1 -> v2: - removed mem_offset tmp variable - use dma_addr_t instead of ssize_t - Fix reserved-memory size in the dts example Changes v2 -> v3: - Fix several other site where PAGE_SHIFT shifts are done on ints. kernel/dma/coherent.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/kernel/dma/coherent.c b/kernel/dma/coherent.c index 551b0eb7028a..d322cb786e7e 100644 --- a/kernel/dma/coherent.c +++ b/kernel/dma/coherent.c @@ -134,7 +134,7 @@ static void *__dma_alloc_from_coherent(struct device *dev, spin_lock_irqsave(&mem->spinlock, flags); - if (unlikely(size > (mem->size << PAGE_SHIFT))) + if (unlikely(size > ((dma_addr_t)mem->size << PAGE_SHIFT))) goto err; pageno = bitmap_find_free_region(mem->bitmap, mem->size, order); @@ -144,8 +144,8 @@ static void *__dma_alloc_from_coherent(struct device *dev, /* * Memory was found in the coherent area. */ - *dma_handle = dma_get_device_base(dev, mem) + (pageno << PAGE_SHIFT); - ret = mem->virt_base + (pageno << PAGE_SHIFT); + *dma_handle = dma_get_device_base(dev, mem) + ((dma_addr_t)pageno << PAGE_SHIFT); + ret = mem->virt_base + ((dma_addr_t)pageno << PAGE_SHIFT); spin_unlock_irqrestore(&mem->spinlock, flags); memset(ret, 0, size); return ret; @@ -194,7 +194,7 @@ static int __dma_release_from_coherent(struct dma_coherent_mem *mem, int order, void *vaddr) { if (mem && vaddr >= mem->virt_base && vaddr < - (mem->virt_base + (mem->size << PAGE_SHIFT))) { + (mem->virt_base + ((dma_addr_t)mem->size << PAGE_SHIFT))) { int page = (vaddr - mem->virt_base) >> PAGE_SHIFT; unsigned long flags; @@ -238,7 +238,7 @@ static int __dma_mmap_from_coherent(struct dma_coherent_mem *mem, struct vm_area_struct *vma, void *vaddr, size_t size, int *ret) { if (mem && vaddr >= mem->virt_base && vaddr + size <= - (mem->virt_base + (mem->size << PAGE_SHIFT))) { + (mem->virt_base + ((dma_addr_t)mem->size << PAGE_SHIFT))) { unsigned long off = vma->vm_pgoff; int start = (vaddr - mem->virt_base) >> PAGE_SHIFT; int user_count = vma_pages(vma); @@ -248,7 +248,7 @@ static int __dma_mmap_from_coherent(struct dma_coherent_mem *mem, if (off < count && user_count <= count - off) { unsigned long pfn = mem->pfn_base + start + off; *ret = remap_pfn_range(vma, vma->vm_start, pfn, - user_count << PAGE_SHIFT, + (unsigned long)user_count << PAGE_SHIFT, vma->vm_page_prot); } return 1; -- 2.20.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/vt-d: Unlock on error paths
On Thu, Mar 12, 2020 at 08:02:41PM +0800, Lu Baolu wrote: > On 2020/3/12 19:37, Dan Carpenter wrote: > > There were a couple places where we need to unlock before returning. > > > > Fixes: 91391b919e19 ("iommu/vt-d: Populate debugfs if IOMMUs are detected") > > Signed-off-by: Dan Carpenter > > --- > > drivers/iommu/intel-iommu-debugfs.c | 6 -- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iommu/intel-iommu-debugfs.c > > b/drivers/iommu/intel-iommu-debugfs.c > > index 8d24c4d85cc2..6a495b103972 100644 > > --- a/drivers/iommu/intel-iommu-debugfs.c > > +++ b/drivers/iommu/intel-iommu-debugfs.c > > @@ -289,11 +289,12 @@ static int dmar_translation_struct_show(struct > > seq_file *m, void *unused) > > sts = dmar_readl(iommu->reg + DMAR_GSTS_REG); > > if (!(sts & DMA_GSTS_TES)) { > > seq_puts(m, "DMA Remapping is not enabled\n"); > > - return 0; > > + goto unlock; > > } > > root_tbl_walk(m, iommu); > > seq_putc(m, '\n'); > > } > > +unlock: > > rcu_read_unlock(); > > return 0; > > @@ -444,7 +445,7 @@ static int ir_translation_struct_show(struct seq_file > > *m, void *unused) > > sts = dmar_readl(iommu->reg + DMAR_GSTS_REG); > > if (!(sts & DMA_GSTS_IRES)) { > > seq_puts(m, "Interrupt Remapping is not enabled\n"); > > - return 0; > > + goto unlock; > > } > > if (iommu->ir_table) { > > @@ -475,6 +476,7 @@ static int ir_translation_struct_show(struct seq_file > > *m, void *unused) > > } > > seq_putc(m, '\n'); > > } > > +unlock: > > rcu_read_unlock(); > > return 0; > > > > Thanks a lot for the catch. I think it could be further cleanup. How > about below changes? Obviously that solves the issues with forgetting to drop the lock but I'm not qualified to comment on the rest. (And I can't really review it anyway because the patch was damaged in sending the email). regards, dan carepnter ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 0/3] Request direct mapping for modem firmware subdevice
On 2020-03-12 6:28 am, Sai Prakash Ranjan wrote: Hi Robin, On 2020-03-10 22:14, Robin Murphy wrote: On 10/03/2020 4:23 pm, Joerg Roedel wrote: On Tue, Mar 10, 2020 at 07:30:50PM +0530, Sibi Sankar wrote: The accesses are initiated by the firmware and they access modem reserved regions. However as explained in ^^ any accesses outside the region will result in a violation and is controlled through XPUs (protection units). Okay, this sounds like a case for arm_smmu_get_resv_region(). It should return an entry for the reserved memory region the firmware needs to access, so that generic iommu can setup this mapping. Note that it should return that entry only for your device, not for all devices. Maybe there is a property in DT or IORT you can set to transport this information into the arm-smmu driver. This is pretty similar to RMRR mapping on the Intel VT-d IOMMU or Unity-mapped ranges in the AMD-Vi IOMMU. Yup, a way to describe boot-time memory regions in IORT is in the process of being specced out; the first attempt at an equivalent for DT is here: https://lore.kernel.org/linux-iommu/20191209150748.2471814-1-thierry.red...@gmail.com/ If that's not enough and the SMMU still needs to treat certain Stream IDs specially because they may be untranslatable (due to having direct access to memory as a side-channel), then that should be handled in the SoC-specific corner of the SMMU driver, not delegated to individual endpoint drivers. Are you talking about this one for SoC specific change - https://lore.kernel.org/patchwork/patch/1183530/ Exactly - this particular wheel needs no reinventing at all. [ I guess I should go review those patches properly... :) ] Robin. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/vt-d: Unlock on error paths
On 2020/3/12 19:37, Dan Carpenter wrote: There were a couple places where we need to unlock before returning. Fixes: 91391b919e19 ("iommu/vt-d: Populate debugfs if IOMMUs are detected") Signed-off-by: Dan Carpenter --- drivers/iommu/intel-iommu-debugfs.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/iommu/intel-iommu-debugfs.c b/drivers/iommu/intel-iommu-debugfs.c index 8d24c4d85cc2..6a495b103972 100644 --- a/drivers/iommu/intel-iommu-debugfs.c +++ b/drivers/iommu/intel-iommu-debugfs.c @@ -289,11 +289,12 @@ static int dmar_translation_struct_show(struct seq_file *m, void *unused) sts = dmar_readl(iommu->reg + DMAR_GSTS_REG); if (!(sts & DMA_GSTS_TES)) { seq_puts(m, "DMA Remapping is not enabled\n"); - return 0; + goto unlock; } root_tbl_walk(m, iommu); seq_putc(m, '\n'); } +unlock: rcu_read_unlock(); return 0; @@ -444,7 +445,7 @@ static int ir_translation_struct_show(struct seq_file *m, void *unused) sts = dmar_readl(iommu->reg + DMAR_GSTS_REG); if (!(sts & DMA_GSTS_IRES)) { seq_puts(m, "Interrupt Remapping is not enabled\n"); - return 0; + goto unlock; } if (iommu->ir_table) { @@ -475,6 +476,7 @@ static int ir_translation_struct_show(struct seq_file *m, void *unused) } seq_putc(m, '\n'); } +unlock: rcu_read_unlock(); return 0; Thanks a lot for the catch. I think it could be further cleanup. How about below changes? index 8d24c4d85cc2..2583a6743dd0 100644 --- a/drivers/iommu/intel-iommu-debugfs.c +++ b/drivers/iommu/intel-iommu-debugfs.c @@ -288,8 +288,9 @@ static int dmar_translation_struct_show(struct seq_file *m, void *unused) for_each_active_iommu(iommu, drhd) { sts = dmar_readl(iommu->reg + DMAR_GSTS_REG); if (!(sts & DMA_GSTS_TES)) { - seq_puts(m, "DMA Remapping is not enabled\n"); - return 0; + seq_printf(m, "DMA Remapping is not enabled on %s\n", + iommu->name); + continue; } root_tbl_walk(m, iommu); seq_putc(m, '\n'); @@ -431,7 +432,6 @@ static int ir_translation_struct_show(struct seq_file *m, void *unused) struct dmar_drhd_unit *drhd; struct intel_iommu *iommu; u64 irta; - u32 sts; rcu_read_lock(); for_each_active_iommu(iommu, drhd) { @@ -441,12 +441,6 @@ static int ir_translation_struct_show(struct seq_file *m, void *unused) seq_printf(m, "Remapped Interrupt supported on IOMMU: %s\n", iommu->name); - sts = dmar_readl(iommu->reg + DMAR_GSTS_REG); - if (!(sts & DMA_GSTS_IRES)) { - seq_puts(m, "Interrupt Remapping is not enabled\n"); - return 0; - } - if (iommu->ir_table) { irta = virt_to_phys(iommu->ir_table->base); seq_printf(m, " IR table address:%llx\n", irta); Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu/vt-d: Unlock on error paths
There were a couple places where we need to unlock before returning. Fixes: 91391b919e19 ("iommu/vt-d: Populate debugfs if IOMMUs are detected") Signed-off-by: Dan Carpenter --- drivers/iommu/intel-iommu-debugfs.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/iommu/intel-iommu-debugfs.c b/drivers/iommu/intel-iommu-debugfs.c index 8d24c4d85cc2..6a495b103972 100644 --- a/drivers/iommu/intel-iommu-debugfs.c +++ b/drivers/iommu/intel-iommu-debugfs.c @@ -289,11 +289,12 @@ static int dmar_translation_struct_show(struct seq_file *m, void *unused) sts = dmar_readl(iommu->reg + DMAR_GSTS_REG); if (!(sts & DMA_GSTS_TES)) { seq_puts(m, "DMA Remapping is not enabled\n"); - return 0; + goto unlock; } root_tbl_walk(m, iommu); seq_putc(m, '\n'); } +unlock: rcu_read_unlock(); return 0; @@ -444,7 +445,7 @@ static int ir_translation_struct_show(struct seq_file *m, void *unused) sts = dmar_readl(iommu->reg + DMAR_GSTS_REG); if (!(sts & DMA_GSTS_IRES)) { seq_puts(m, "Interrupt Remapping is not enabled\n"); - return 0; + goto unlock; } if (iommu->ir_table) { @@ -475,6 +476,7 @@ static int ir_translation_struct_show(struct seq_file *m, void *unused) } seq_putc(m, '\n'); } +unlock: rcu_read_unlock(); return 0; -- 2.20.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu/amd: Fix IOMMU AVIC not properly update the is_run bit in IRTE
Commit b9c6ff94e43a ("iommu/amd: Re-factor guest virtual APIC (de-)activation code") accidentally left out the ir_data pointer when calling modity_irte_ga(), which causes the function amd_iommu_update_ga() to return prematurely due to struct amd_ir_data.ref is NULL and the "is_run" bit of IRTE does not get updated properly. This results in bad I/O performance since IOMMU AVIC always generate GA Log entry and notify IOMMU driver and KVM when it receives interrupt from the PCI pass-through device instead of directly inject interrupt to the vCPU. Fixes by passing ir_data when calling modify_irte_ga() as done previously. Fixes: b9c6ff94e43a ("iommu/amd: Re-factor guest virtual APIC (de-)activation code") Signed-off-by: Suravee Suthikulpanit --- drivers/iommu/amd_iommu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c index aac132b..20cce36 100644 --- a/drivers/iommu/amd_iommu.c +++ b/drivers/iommu/amd_iommu.c @@ -3826,7 +3826,7 @@ int amd_iommu_activate_guest_mode(void *data) entry->lo.fields_vapic.ga_tag = ir_data->ga_tag; return modify_irte_ga(ir_data->irq_2_irte.devid, - ir_data->irq_2_irte.index, entry, NULL); + ir_data->irq_2_irte.index, entry, ir_data); } EXPORT_SYMBOL(amd_iommu_activate_guest_mode); @@ -3852,7 +3852,7 @@ int amd_iommu_deactivate_guest_mode(void *data) APICID_TO_IRTE_DEST_HI(cfg->dest_apicid); return modify_irte_ga(ir_data->irq_2_irte.devid, - ir_data->irq_2_irte.index, entry, NULL); + ir_data->irq_2_irte.index, entry, ir_data); } EXPORT_SYMBOL(amd_iommu_deactivate_guest_mode); -- 1.8.3.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 08/11] iommu/vt-d: Use pci_ats_supported()
On 2020/3/12 15:54, Jean-Philippe Brucker wrote: Hi Baolu, On Thu, Mar 12, 2020 at 09:44:16AM +0800, Lu Baolu wrote: Hi Jean, On 2020/3/11 20:45, Jean-Philippe Brucker wrote: The pci_ats_supported() function checks if a device supports ATS and is allowed to use it. Signed-off-by: Jean-Philippe Brucker --- drivers/iommu/intel-iommu.c | 9 +++-- 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c index 6fa6de2b6ad5..17208280ef5c 100644 --- a/drivers/iommu/intel-iommu.c +++ b/drivers/iommu/intel-iommu.c @@ -1454,8 +1454,7 @@ static void iommu_enable_dev_iotlb(struct device_domain_info *info) !pci_reset_pri(pdev) && !pci_enable_pri(pdev, 32)) info->pri_enabled = 1; #endif - if (!pdev->untrusted && info->ats_supported && - pci_ats_page_aligned(pdev) && + if (info->ats_supported && pci_ats_page_aligned(pdev) && !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) { info->ats_enabled = 1; domain_update_iotlb(info->domain); @@ -2611,10 +2610,8 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu, if (dev && dev_is_pci(dev)) { struct pci_dev *pdev = to_pci_dev(info->dev); - if (!pdev->untrusted && - !pci_ats_disabled() && The pci_ats_disabled() couldn't be replaced by pci_ats_supported(). Even pci_ats_supported() returns true, user still can disable it. Or move ats_disabled into pci_ats_supported()? It is already there, but hidden behind the "if (!dev->ats_cap)": pci_ats_init() only sets dev->ats_cap after checking that pci_ats_disabled() returns false. Ah! Yes. Acked-by: Lu Baolu Thanks, Jean Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 08/11] iommu/vt-d: Use pci_ats_supported()
Hi Baolu, On Thu, Mar 12, 2020 at 09:44:16AM +0800, Lu Baolu wrote: > Hi Jean, > > On 2020/3/11 20:45, Jean-Philippe Brucker wrote: > > The pci_ats_supported() function checks if a device supports ATS and is > > allowed to use it. > > > > Signed-off-by: Jean-Philippe Brucker > > --- > > drivers/iommu/intel-iommu.c | 9 +++-- > > 1 file changed, 3 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c > > index 6fa6de2b6ad5..17208280ef5c 100644 > > --- a/drivers/iommu/intel-iommu.c > > +++ b/drivers/iommu/intel-iommu.c > > @@ -1454,8 +1454,7 @@ static void iommu_enable_dev_iotlb(struct > > device_domain_info *info) > > !pci_reset_pri(pdev) && !pci_enable_pri(pdev, 32)) > > info->pri_enabled = 1; > > #endif > > - if (!pdev->untrusted && info->ats_supported && > > - pci_ats_page_aligned(pdev) && > > + if (info->ats_supported && pci_ats_page_aligned(pdev) && > > !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) { > > info->ats_enabled = 1; > > domain_update_iotlb(info->domain); > > @@ -2611,10 +2610,8 @@ static struct dmar_domain > > *dmar_insert_one_dev_info(struct intel_iommu *iommu, > > if (dev && dev_is_pci(dev)) { > > struct pci_dev *pdev = to_pci_dev(info->dev); > > - if (!pdev->untrusted && > > - !pci_ats_disabled() && > > The pci_ats_disabled() couldn't be replaced by pci_ats_supported(). Even > pci_ats_supported() returns true, user still can disable it. Or move > ats_disabled into pci_ats_supported()? It is already there, but hidden behind the "if (!dev->ats_cap)": pci_ats_init() only sets dev->ats_cap after checking that pci_ats_disabled() returns false. Thanks, Jean > > Best regards, > baolu > > > - ecap_dev_iotlb_support(iommu->ecap) && > > - pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ATS) && > > + if (ecap_dev_iotlb_support(iommu->ecap) && > > + pci_ats_supported(pdev) && > > dmar_find_matched_atsr_unit(pdev)) > > info->ats_supported = 1; ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu