Re: [PATCH v2 0/3] iommu: Remove OMAP IOVMM driver
On Mon, Jul 21, 2014 at 11:19:29PM -0700, Tony Lindgren wrote: Tony, is there still time to get this (and especially patch 2/3, which touches arch/ code) in v3.17 ? Yes as long as Joerg is OK to merge that branch in :) Fine with me, I can take only patch 1 or all 3 into my arm/omap branch, given Tony's Acked-by. Then you guys can merge in this branch wherever you want :) Cheers, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/3] iommu: Remove OMAP IOVMM driver
Hi Joerg, On Wednesday 23 July 2014 15:52:17 Joerg Roedel wrote: On Mon, Jul 21, 2014 at 11:19:29PM -0700, Tony Lindgren wrote: Tony, is there still time to get this (and especially patch 2/3, which touches arch/ code) in v3.17 ? Yes as long as Joerg is OK to merge that branch in :) Fine with me, I can take only patch 1 or all 3 into my arm/omap branch, given Tony's Acked-by. Then you guys can merge in this branch wherever you want :) Thank you. Assuming there's currently no conflict to be resolved, I believe the easiest would be for both you and Tony to merge my branch in your trees. -- Regards, Laurent Pinchart ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/vt-d: fix race between free_irte() and get_irte()
On Wed, Jul 23, 2014 at 04:40:24PM +0200, Joerg Roedel wrote: On Tue, Jul 22, 2014 at 08:27:19AM -0600, Greg Edwards wrote: get_irte() can race with free_irte() and dereference a NULL iommu pointer. Have you seen any real occurance of this race? Get_irte is called in the set_affinity path, how can that race with the irq being freed? Yes, that's how we hit it. A process was setting the CPU affinity while QEMU was releasing the IRQ. We have a CI stress test that turned this up. Greg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/vt-d: fix race between free_irte() and get_irte()
On Wed, Jul 23, 2014 at 08:49:17AM -0600, Greg Edwards wrote: On Wed, Jul 23, 2014 at 04:40:24PM +0200, Joerg Roedel wrote: On Tue, Jul 22, 2014 at 08:27:19AM -0600, Greg Edwards wrote: get_irte() can race with free_irte() and dereference a NULL iommu pointer. Have you seen any real occurance of this race? Get_irte is called in the set_affinity path, how can that race with the irq being freed? Yes, that's how we hit it. A process was setting the CPU affinity while QEMU was releasing the IRQ. We have a CI stress test that turned this up. Can you update the commit message with the details of how this race can be triggered, ideally with a stack-trace of a real crash you triggered because of this issue? Thanks, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [Patch Part3 V4 16/21] iommu/vt-d: Implement DMAR unit hotplug framework
On Fri, Jul 11, 2014 at 02:19:40PM +0800, Jiang Liu wrote: On Intel platforms, an IO Hub (PCI/PCIe host bridge) may contain DMAR units, so we need to support DMAR hotplug when supporting PCI host bridge hotplug on Intel platforms. According to Section 8.8 Remapping Hardware Unit Hot Plug in Intel Virtualization Technology for Directed IO Architecture Specification Rev 2.2, ACPI BIOS should implement ACPI _DSM method under the ACPI object for the PCI host bridge to support DMAR hotplug. This patch introduces interfaces to parse ACPI _DSM method for DMAR unit hotplug. It also implements state machines for DMAR unit hot-addition and hot-removal. The PCI host bridge hotplug driver should call dmar_hotplug_hotplug() before scanning PCI devices connected for hot-addition and after destroying all PCI devices for hot-removal. Signed-off-by: Jiang Liu jiang@linux.intel.com --- drivers/iommu/dmar.c| 268 +-- drivers/iommu/intel-iommu.c | 78 +- drivers/iommu/intel_irq_remapping.c |5 + include/linux/dmar.h| 33 + 4 files changed, 370 insertions(+), 14 deletions(-) This patch triggers a compile error with allnoconfig on amd64: CC arch/x86/kernel/pci-dma.o In file included from arch/x86/kernel/pci-dma.c:3:0: include/linux/dmar.h:168:35: error: unknown type name ‘acpi_handle’ static inline int dmar_device_add(acpi_handle handle) ^ include/linux/dmar.h:173:38: error: unknown type name ‘acpi_handle’ static inline int dmar_device_remove(acpi_handle handle) ^ make[2]: *** [arch/x86/kernel/pci-dma.o] Error 1 make[1]: *** [arch/x86/kernel] Error 2 make: *** [arch/x86] Error 2 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2] iommu/vt-d: race setting IRQ CPU affinity while freeing IRQ
A user process setting the CPU affinity of an IRQ for a KVM
direct-assigned device via /proc/irq/IRQ#/smp_affinity can race with
the IRQ being released by QEMU, resulting in a NULL iommu pointer
dereference in get_irte().
Signed-off-by: Greg Edwards gedwa...@ddn.com
---
Dropped the Cc: for stable since this likely wouldn't ever be seen in
the real world. We saw it on an automated CI stress test.
drivers/iommu/intel_irq_remapping.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/iommu/intel_irq_remapping.c
b/drivers/iommu/intel_irq_remapping.c
index 9b17489..d926676 100644
--- a/drivers/iommu/intel_irq_remapping.c
+++ b/drivers/iommu/intel_irq_remapping.c
@@ -70,6 +70,11 @@ static int get_irte(int irq, struct irte *entry)
raw_spin_lock_irqsave(irq_2_ir_lock, flags);
+ if (unlikely(!irq_iommu-iommu)) {
+ raw_spin_unlock_irqrestore(irq_2_ir_lock, flags);
+ return -1;
+ }
+
index = irq_iommu-irte_index + irq_iommu-sub_handle;
*entry = *(irq_iommu-iommu-ir_table-base + index);
--
1.9.3
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/1] iommu-api: Add map_range/unmap_range functions
On 7/22/2014 12:45 AM, Thierry Reding wrote:
On Mon, Jul 21, 2014 at 05:59:22PM -0700, Olav Haugan wrote:
On 7/17/2014 1:21 AM, Thierry Reding wrote:
On Wed, Jul 16, 2014 at 06:01:57PM -0700, Olav Haugan wrote:
[...]
Additionally, the mapping operation would be faster in general since
clients does not have to keep calling map API over and over again for
each physically contiguous chunk of memory that needs to be mapped to a
virtually contiguous region.
Signed-off-by: Olav Haugan ohau...@codeaurora.org
---
drivers/iommu/iommu.c | 48
include/linux/iommu.h | 25 +
2 files changed, 73 insertions(+)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 1698360..a0eebb7 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1089,6 +1089,54 @@ size_t iommu_unmap(struct iommu_domain *domain,
unsigned long iova, size_t size)
EXPORT_SYMBOL_GPL(iommu_unmap);
+int iommu_map_range(struct iommu_domain *domain, unsigned int iova,
Maybe iova should be dma_addr_t? Or at least unsigned long? And perhaps
iommu_map_sg() would be more consistent with the equivalent function in
struct dma_ops?
+ struct scatterlist *sg, unsigned int len, int opt)
The length argument seems to be the size of the mapping. Again, the
struct dma_ops function uses this argument to denote the number of
entries in the scatterlist.
opt is somewhat opaque. Perhaps this should be turned into unsigned long
flags? Although given that there aren't any users yet it's difficult to
say what's best here. Perhaps the addition of this argument should be
postponed until there are actual users?
I am thinking something like this:
int iommu_map_sg(struct iommu_domain *domain, struct scatterlist *sg,
unsigned int nents, int prot, unsigned long flags);
int iommu_unmap_sg(struct iommu_domain *domain, struct scatterlist *sg,
unsigned int nents, unsigned long flags);
Looks good.
The iova is contained within sg so we don't need that argument really
I'm not sure. I think a common use-case for this function is for some
driver to map an imported DMA-BUF. In that case you'll get a struct
sg_table, but I think it won't have sg.dma_address set to anything
useful. So if we don't have iova as a parameter to this function, the
driver would have to make it a two-step process, like this:
sg_dma_address(sg) = iova;
err = iommu_map_sg(...);
And drivers that use the IOMMU API directly need to manage IOVA space
themselves anyway, so I think passing around the IOVA within the SG
won't be a very common case. It will almost always be the driver that
calls this function which allocates the IOVA range.
Yes, I see your point. Rob is not a fan either...
So what about this:
int iommu_map_sg(struct iommu_domain *domain, unsigned long iova, struct
scatterlist *sg, unsigned int nents, int prot, unsigned long flags);
int iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova,
size_t size, unsigned long flags);
No need for sg in the unmap call then. Keeping iova an unsigned long to
match the existing iommu_map/iommu_unmap calls.
and I would like to keep the flags argument. I would prefer not to
change the API after it has been published which could potentially
affect a lot of call sites.
We have pretty good tools to help with this kind of mechanical
conversion, so I don't think changing the API will be much of a problem.
However it seems likely that we'll want to specify flags eventually, so
I don't have any strong objections to keeping that parameter.
+ phys_addr_t phys = page_to_phys(sg_page(sg));
+ u32 page_len = PAGE_ALIGN(sg-offset + sg-length);
Shouldn't this alignment be left to iommu_map() to handle? It has code
to deal with that already.
I don't see page alignment in the iommu_map function. I only see a check
whether the (iova | paddr | size) is aligned to the minimum page size
and then it errors out if it isn't
Indeed, the above doesn't do what I thought it did.
Perhaps rather than check for a -map_range implementation everytime a
better option may be to export this generic implementation so that
drivers can set it in their iommu_ops if they don't implement it? So the
contents of the if () block could become a new function:
int iommu_map_range_generic(...)
{
...
}
EXPORT_SYMBOL(iommu_map_range_generic);
And drivers would do this:
static const struct iommu_ops driver_iommu_ops = {
...
.map_range = iommu_map_range_generic,
...
};
I'd like to keep the new API consistent with the rest of the API. Most
if not all of the other APIs always check if the operation is non-NULL.
But that's because the other operations are either optional or they
don't provide a fallback implementation. For .map_sg() the issue is
different because
iommu/vt-d: Fix build error caused by unknown definition of acpi_handle
When both CONFIG_ACPI and CONFIG_DMAR_TABLE are disabled, commit
Implement DMAR unit hotplug framework causes build failure as below:
CC arch/x86/kernel/pci-dma.o
In file included from arch/x86/kernel/pci-dma.c:3:0:
include/linux/dmar.h:168:35: error: unknown type name ‘acpi_handle’
static inline int dmar_device_add(acpi_handle handle)
^
include/linux/dmar.h:173:38: error: unknown type name ‘acpi_handle’
static inline int dmar_device_remove(acpi_handle handle)
^
make[2]: *** [arch/x86/kernel/pci-dma.o] Error 1
make[1]: *** [arch/x86/kernel] Error 2
make: *** [arch/x86] Error 2
Signed-off-by: Jiang Liu jiang@linux.intel.com
---
Hi Joerg,
Could you please help to merge or fold this patch to fix the
build error?
Regards!
Gerry
---
include/linux/dmar.h |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/dmar.h b/include/linux/dmar.h
index 9c06bb4b5b14..594d4ac79e75 100644
--- a/include/linux/dmar.h
+++ b/include/linux/dmar.h
@@ -165,12 +165,12 @@ static inline int dmar_ir_hotplug(struct dmar_drhd_unit
*dmaru, bool insert)
#else /* CONFIG_DMAR_TABLE */
-static inline int dmar_device_add(acpi_handle handle)
+static inline int dmar_device_add(void *handle)
{
return 0;
}
-static inline int dmar_device_remove(acpi_handle handle)
+static inline int dmar_device_remove(void *handle)
{
return 0;
}
--
1.7.10.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
