[PATCH 3/5] iommu/tegra-smmu: Fix client enablement order
Enable clients' translation only after setting up the swgroups.
Signed-off-by: Navneet Kumar
---
drivers/iommu/tegra-smmu.c | 23 ++-
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index fa175d9..1a44cf6 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -353,6 +353,20 @@ static void tegra_smmu_enable(struct tegra_smmu *smmu,
unsigned int swgroup,
unsigned int i;
u32 value;
+ group = tegra_smmu_find_swgroup(smmu, swgroup);
+ if (group) {
+ value = smmu_readl(smmu, group->reg);
+ value &= ~SMMU_ASID_MASK;
+ value |= SMMU_ASID_VALUE(asid);
+ value |= SMMU_ASID_ENABLE;
+ smmu_writel(smmu, value, group->reg);
+ } else {
+ pr_warn("%s group from swgroup %u not found\n", __func__,
+ swgroup);
+ /* No point moving ahead if group was not found */
+ return;
+ }
+
for (i = 0; i < smmu->soc->num_clients; i++) {
const struct tegra_mc_client *client = >soc->clients[i];
@@ -363,15 +377,6 @@ static void tegra_smmu_enable(struct tegra_smmu *smmu,
unsigned int swgroup,
value |= BIT(client->smmu.bit);
smmu_writel(smmu, value, client->smmu.reg);
}
-
- group = tegra_smmu_find_swgroup(smmu, swgroup);
- if (group) {
- value = smmu_readl(smmu, group->reg);
- value &= ~SMMU_ASID_MASK;
- value |= SMMU_ASID_VALUE(asid);
- value |= SMMU_ASID_ENABLE;
- smmu_writel(smmu, value, group->reg);
- }
}
static void tegra_smmu_disable(struct tegra_smmu *smmu, unsigned int swgroup,
--
2.7.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 2/5] iommu/tegra-smmu: Use non-secure register for flushing
Use PTB_ASID instead of SMMU_CONFIG to flush smmu.
PTB_ASID can be accessed from non-secure mode, SMMU_CONFIG cannot be.
Using SMMU_CONFIG could pose a problem when kernel doesn't have secure
mode access enabled from boot.
Signed-off-by: Navneet Kumar
---
drivers/iommu/tegra-smmu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index ee4d8a8..fa175d9 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -235,7 +235,7 @@ static inline void smmu_flush_tlb_group(struct tegra_smmu
*smmu,
static inline void smmu_flush(struct tegra_smmu *smmu)
{
- smmu_readl(smmu, SMMU_CONFIG);
+ smmu_readl(smmu, SMMU_PTB_ASID);
}
static int tegra_smmu_alloc_asid(struct tegra_smmu *smmu, unsigned int *idp)
--
2.7.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 4/5] iommu/tegra-smmu: Add PCI support
Add support for PCI devices.
Signed-off-by: Navneet Kumar
---
drivers/iommu/tegra-smmu.c | 47 +-
1 file changed, 34 insertions(+), 13 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 1a44cf6..4b43c63 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
#include
@@ -820,7 +821,8 @@ tegra_smmu_find_group(struct tegra_smmu *smmu, unsigned int
swgroup)
return NULL;
}
-static struct iommu_group *tegra_smmu_group_get(struct tegra_smmu *smmu,
+static struct iommu_group *tegra_smmu_group_get(struct device *dev,
+ struct tegra_smmu *smmu,
unsigned int swgroup)
{
const struct tegra_smmu_group_soc *soc;
@@ -847,7 +849,11 @@ static struct iommu_group *tegra_smmu_group_get(struct
tegra_smmu *smmu,
INIT_LIST_HEAD(>list);
group->soc = soc;
- group->group = iommu_group_alloc();
+ if (dev_is_pci(dev))
+ group->group = pci_device_group(dev);
+ else
+ group->group = generic_device_group(dev);
+
if (IS_ERR(group->group)) {
devm_kfree(smmu->dev, group);
mutex_unlock(>lock);
@@ -864,13 +870,8 @@ static struct iommu_group *tegra_smmu_device_group(struct
device *dev)
{
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct tegra_smmu *smmu = dev->archdata.iommu;
- struct iommu_group *group;
- group = tegra_smmu_group_get(smmu, fwspec->ids[0]);
- if (!group)
- group = generic_device_group(dev);
-
- return group;
+ return tegra_smmu_group_get(dev, smmu, fwspec->ids[0]);
}
static int tegra_smmu_of_xlate(struct device *dev,
@@ -989,6 +990,28 @@ static void tegra_smmu_debugfs_exit(struct tegra_smmu
*smmu)
debugfs_remove_recursive(smmu->debugfs);
}
+static int tegra_smmu_iommu_bus_init(struct tegra_smmu *smmu)
+{
+ int err;
+
+ err = bus_set_iommu(_bus_type, _smmu_ops);
+ if (err < 0) {
+ iommu_device_unregister(>iommu);
+ iommu_device_sysfs_remove(>iommu);
+ return err;
+ }
+
+#ifdef CONFIG_PCI
+ if (!iommu_present(_bus_type)) {
+ pci_request_acs();
+ err = bus_set_iommu(_bus_type, _smmu_ops);
+ if (err < 0)
+ return err;
+ }
+#endif
+ return 0;
+}
+
struct tegra_smmu *tegra_smmu_probe(struct device *dev,
const struct tegra_smmu_soc *soc,
struct tegra_mc *mc)
@@ -1072,12 +1095,10 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
return ERR_PTR(err);
}
- err = bus_set_iommu(_bus_type, _smmu_ops);
- if (err < 0) {
- iommu_device_unregister(>iommu);
- iommu_device_sysfs_remove(>iommu);
+ err = tegra_smmu_iommu_bus_init(smmu);
+ if (err)
return ERR_PTR(err);
- }
+ err = bus_set_iommu(_bus_type, _smmu_ops);
if (IS_ENABLED(CONFIG_DEBUG_FS))
tegra_smmu_debugfs_init(smmu);
--
2.7.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 1/5] iommu/tegra-smmu: Fix domain_alloc
* Allocate dma iova cookie for a domain while adding dma iommu
devices.
* Perform a stricter check for domain type parameter.
Signed-off-by: Navneet Kumar
---
drivers/iommu/tegra-smmu.c | 43 +++
1 file changed, 27 insertions(+), 16 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 543f7c9..ee4d8a8 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -16,6 +16,7 @@
#include
#include
#include
+#include
#include
#include
@@ -271,8 +272,10 @@ static bool tegra_smmu_capable(enum iommu_cap cap)
static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
{
struct tegra_smmu_as *as;
+ int ret;
- if (type != IOMMU_DOMAIN_UNMANAGED)
+ if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA &&
+ type != IOMMU_DOMAIN_IDENTITY)
return NULL;
as = kzalloc(sizeof(*as), GFP_KERNEL);
@@ -281,26 +284,22 @@ static struct iommu_domain
*tegra_smmu_domain_alloc(unsigned type)
as->attr = SMMU_PD_READABLE | SMMU_PD_WRITABLE | SMMU_PD_NONSECURE;
+ ret = (type == IOMMU_DOMAIN_DMA) ? iommu_get_dma_cookie(>domain) :
+ -ENODEV;
+ if (ret)
+ goto free_as;
+
as->pd = alloc_page(GFP_KERNEL | __GFP_DMA | __GFP_ZERO);
- if (!as->pd) {
- kfree(as);
- return NULL;
- }
+ if (!as->pd)
+ goto put_dma_cookie;
as->count = kcalloc(SMMU_NUM_PDE, sizeof(u32), GFP_KERNEL);
- if (!as->count) {
- __free_page(as->pd);
- kfree(as);
- return NULL;
- }
+ if (!as->count)
+ goto free_pd_range;
as->pts = kcalloc(SMMU_NUM_PDE, sizeof(*as->pts), GFP_KERNEL);
- if (!as->pts) {
- kfree(as->count);
- __free_page(as->pd);
- kfree(as);
- return NULL;
- }
+ if (!as->pts)
+ goto free_pts;
/* setup aperture */
as->domain.geometry.aperture_start = 0;
@@ -308,6 +307,18 @@ static struct iommu_domain
*tegra_smmu_domain_alloc(unsigned type)
as->domain.geometry.force_aperture = true;
return >domain;
+
+free_pts:
+ kfree(as->pts);
+free_pd_range:
+ __free_page(as->pd);
+put_dma_cookie:
+ if (type == IOMMU_DOMAIN_DMA)
+ iommu_put_dma_cookie(>domain);
+free_as:
+ kfree(as);
+
+ return NULL;
}
static void tegra_smmu_domain_free(struct iommu_domain *domain)
--
2.7.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/intel-iommu: fix memory leak in intel_iommu_put_resv_regions()
Hi Gerald,
On 1/16/19 8:11 PM, Gerald Schaefer wrote:
> Commit 9d3a4de4cb8d ("iommu: Disambiguate MSI region types") changed
> the reserved region type in intel_iommu_get_resv_regions() from
> IOMMU_RESV_RESERVED to IOMMU_RESV_MSI, but it forgot to also change
> the type in intel_iommu_put_resv_regions().
>
> This leads to a memory leak, because now the check in
> intel_iommu_put_resv_regions() for IOMMU_RESV_RESERVED will never
> be true, and no allocated regions will be freed.
>
> Fix this by changing the region type in intel_iommu_put_resv_regions()
> to IOMMU_RESV_MSI, matching the type of the allocated regions.
>
> Fixes: 9d3a4de4cb8d ("iommu: Disambiguate MSI region types")
> Cc: # v4.11+
> Signed-off-by: Gerald Schaefer
Reviewed-by: Eric Auger
Thanks
Eric
> ---
> drivers/iommu/intel-iommu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 048b5ab36a02..b83e0f2025bb 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -5295,7 +5295,7 @@ static void intel_iommu_put_resv_regions(struct device
> *dev,
> struct iommu_resv_region *entry, *next;
>
> list_for_each_entry_safe(entry, next, head, list) {
> - if (entry->type == IOMMU_RESV_RESERVED)
> + if (entry->type == IOMMU_RESV_MSI)
> kfree(entry);
> }
> }
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu/intel-iommu: fix memory leak in intel_iommu_put_resv_regions()
Commit 9d3a4de4cb8d ("iommu: Disambiguate MSI region types") changed
the reserved region type in intel_iommu_get_resv_regions() from
IOMMU_RESV_RESERVED to IOMMU_RESV_MSI, but it forgot to also change
the type in intel_iommu_put_resv_regions().
This leads to a memory leak, because now the check in
intel_iommu_put_resv_regions() for IOMMU_RESV_RESERVED will never
be true, and no allocated regions will be freed.
Fix this by changing the region type in intel_iommu_put_resv_regions()
to IOMMU_RESV_MSI, matching the type of the allocated regions.
Fixes: 9d3a4de4cb8d ("iommu: Disambiguate MSI region types")
Cc: # v4.11+
Signed-off-by: Gerald Schaefer
---
drivers/iommu/intel-iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 048b5ab36a02..b83e0f2025bb 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -5295,7 +5295,7 @@ static void intel_iommu_put_resv_regions(struct device
*dev,
struct iommu_resv_region *entry, *next;
list_for_each_entry_safe(entry, next, head, list) {
- if (entry->type == IOMMU_RESV_RESERVED)
+ if (entry->type == IOMMU_RESV_MSI)
kfree(entry);
}
}
--
2.16.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC v3 14/21] iommu: introduce device fault data
Hi Jean,
On 1/16/19 4:52 PM, Jean-Philippe Brucker wrote:
> On 14/01/2019 22:32, Jacob Pan wrote:
>>> [...]
> +/**
> + * struct iommu_fault - Generic fault data
> + *
> + * @type contains fault type
> + * @reason fault reasons if relevant outside IOMMU driver.
> + * IOMMU driver internal faults are not reported.
> + * @addr: tells the offending page address
> + * @fetch_addr: tells the address that caused an abort, if any
> + * @pasid: contains process address space ID, used in shared
> virtual memory
> + * @page_req_group_id: page request group index
> + * @last_req: last request in a page request group
> + * @pasid_valid: indicates if the PRQ has a valid PASID
> + * @prot: page access protection flag:
> + * IOMMU_FAULT_READ, IOMMU_FAULT_WRITE
> + */
> +
> +struct iommu_fault {
> + __u32 type; /* enum iommu_fault_type */
> + __u32 reason; /* enum iommu_fault_reason */
> + __u64 addr;
> + __u64 fetch_addr;
> + __u32 pasid;
> + __u32 page_req_group_id;
> + __u32 last_req;
> + __u32 pasid_valid;
> + __u32 prot;
> + __u32 access;
>>>
>>> What does @access contain? Can it be squashed into @prot?
>>>
>> I agreed.
>>
>> how about this?
>> #define IOMMU_FAULT_VERSION_V1 0x1
>> struct iommu_fault {
>> __u16 version;
>
> Right, but the version field becomes redundant when we present a batch
> of these to userspace, in patch 18 (assuming we don't want to mix fault
> structure versions within a batch... I certainly don't).>
> When introducing IOMMU_FAULT_VERSION_V2, in a distant future, I think we
> still need to support a userspace that uses IOMMU_FAULT_VERSION_V1. One
> strategy for this:
>
> * We define structs iommu_fault_v1 (the current iommu_fault) and
> iommu_fault_v2.
> * Userspace selects IOMMU_FAULT_VERSION_V1 when registering the fault
> queue
> * The IOMMU driver fills iommu_fault_v2 and passes it to VFIO
> * VFIO does its best to translate this into a iommu_fault_v1 struct
>
> So what we need now, is a way for userspace to tell the kernel which
> structure version it expects. I'm not sure we even need to pass the
> actual version number we're using back to userspace. Agreeing on one
> version at registration should be sufficient.
As we expose a VFIO region we can report its size, entry size, max
supported entry version, actual entry version in the region capabilities.
Conveying the version along with the eventfd at registration time will
require to introduce a new flag at vfio_irq_set level (if we still plan
to use this VFIO_DEVICE_SET_IRQS API) but that should be feasible.
>
>> __u16 type;
>> __u32 reason;
>> __u64 addr;
>
> I'm in favor of keeping @fetch_addr as well, it can contain useful
> information. For example, while attempting to translate an IOVA
> 0xf000, the IOMMU can't find the PASID table that we installed with
> address 0xdead - the guest passed an invalid address to
> bind_pasid_table(). We can then report 0xf000 in @addr, and 0xdead
> in @fetch_addr.
agreed
>
>> __u32 pasid;
>> __u32 page_req_group_id;
>> __u32 last_req : 1;
>> __u32 pasid_valid : 1;
>
> Agreed, with some explicit padding or combined as a @flag field. In fact
> if we do add the @fetch_addr field, I think we need a bit that indicates
> its validity as well.
Can't we simply state fetch_addr is valid for
IOMMU_FAULT_REASON_PASID_FETCH, IOMMU_FAULT_REASON_DEVICE_CONTEXT_FETCH,
IOMMU_FAULT_REASON_WALK_EABT which maps to its usage in SMMU spec.
Thanks
Eric
>
> Thanks,
> Jean
>
>> __u32 prot;
>> __u64 device_private[2];
>> __u8 padding[48];
>> };
>>
>>
>>> Thanks,
>>> Jean
>>>
relocated to uapi, Yi can you confirm?
__u64 device_private[2];
> +};
> #endif /* _UAPI_IOMMU_H */
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>>
>>
>> [Jacob Pan]
>> ___
>> iommu mailing list
>> iommu@lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>
>
> ___
> kvmarm mailing list
> kvm...@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: Fail to boot Dom0 on Xen Arm64 after "dma-mapping: bypass indirect calls for dma-direct"
Does this fix your problem?
diff --git a/arch/arm/include/asm/xen/page-coherent.h
b/arch/arm/include/asm/xen/page-coherent.h
index b3ef061d8b74..2c403e7c782d 100644
--- a/arch/arm/include/asm/xen/page-coherent.h
+++ b/arch/arm/include/asm/xen/page-coherent.h
@@ -1 +1,95 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_ARM_XEN_PAGE_COHERENT_H
+#define _ASM_ARM_XEN_PAGE_COHERENT_H
+
+#include
+#include
#include
+
+static inline const struct dma_map_ops *xen_get_dma_ops(struct device *dev)
+{
+ if (dev && dev->archdata.dev_dma_ops)
+ return dev->archdata.dev_dma_ops;
+ return get_arch_dma_ops(NULL);
+}
+
+static inline void *xen_alloc_coherent_pages(struct device *hwdev, size_t size,
+ dma_addr_t *dma_handle, gfp_t flags, unsigned long attrs)
+{
+ return xen_get_dma_ops(hwdev)->alloc(hwdev, size, dma_handle, flags,
attrs);
+}
+
+static inline void xen_free_coherent_pages(struct device *hwdev, size_t size,
+ void *cpu_addr, dma_addr_t dma_handle, unsigned long attrs)
+{
+ xen_get_dma_ops(hwdev)->free(hwdev, size, cpu_addr, dma_handle, attrs);
+}
+
+static inline void xen_dma_map_page(struct device *hwdev, struct page *page,
+dma_addr_t dev_addr, unsigned long offset, size_t size,
+enum dma_data_direction dir, unsigned long attrs)
+{
+ unsigned long page_pfn = page_to_xen_pfn(page);
+ unsigned long dev_pfn = XEN_PFN_DOWN(dev_addr);
+ unsigned long compound_pages =
+ (1unmap_page)
+ xen_get_dma_ops(hwdev)->unmap_page(hwdev, handle, size,
dir, attrs);
+ } else
+ __xen_dma_unmap_page(hwdev, handle, size, dir, attrs);
+}
+
+static inline void xen_dma_sync_single_for_cpu(struct device *hwdev,
+ dma_addr_t handle, size_t size, enum dma_data_direction dir)
+{
+ unsigned long pfn = PFN_DOWN(handle);
+ if (pfn_valid(pfn)) {
+ if (xen_get_dma_ops(hwdev)->sync_single_for_cpu)
+ xen_get_dma_ops(hwdev)->sync_single_for_cpu(hwdev,
handle, size, dir);
+ } else
+ __xen_dma_sync_single_for_cpu(hwdev, handle, size, dir);
+}
+
+static inline void xen_dma_sync_single_for_device(struct device *hwdev,
+ dma_addr_t handle, size_t size, enum dma_data_direction dir)
+{
+ unsigned long pfn = PFN_DOWN(handle);
+ if (pfn_valid(pfn)) {
+ if (xen_get_dma_ops(hwdev)->sync_single_for_device)
+ xen_get_dma_ops(hwdev)->sync_single_for_device(hwdev,
handle, size, dir);
+ } else
+ __xen_dma_sync_single_for_device(hwdev, handle, size, dir);
+}
+
+#endif /* _ASM_ARM_XEN_PAGE_COHERENT_H */
diff --git a/arch/arm64/include/asm/xen/page-coherent.h
b/arch/arm64/include/asm/xen/page-coherent.h
index b3ef061d8b74..61006385a437 100644
--- a/arch/arm64/include/asm/xen/page-coherent.h
+++ b/arch/arm64/include/asm/xen/page-coherent.h
@@ -1 +1,84 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_ARM64_XEN_PAGE_COHERENT_H
+#define _ASM_ARM64_XEN_PAGE_COHERENT_H
+
+#include
+#include
#include
+
+static inline void *xen_alloc_coherent_pages(struct device *hwdev, size_t size,
+ dma_addr_t *dma_handle, gfp_t flags, unsigned long attrs)
+{
+ return dma_direct_alloc(hwdev, size, dma_handle, flags, attrs);
+}
+
+static inline void xen_free_coherent_pages(struct device *hwdev, size_t size,
+ void *cpu_addr, dma_addr_t dma_handle, unsigned long attrs)
+{
+ dma_direct_free(hwdev, size, cpu_addr, dma_handle, attrs);
+}
+
+static inline void xen_dma_map_page(struct device *hwdev, struct page *page,
+dma_addr_t dev_addr, unsigned long offset, size_t size,
+enum dma_data_direction dir, unsigned long attrs)
+{
+ unsigned long page_pfn = page_to_xen_pfn(page);
+ unsigned long dev_pfn = XEN_PFN_DOWN(dev_addr);
+ unsigned long compound_pages =
+ (1mmap(dev, vma, cpu_addr,
Re: [RFC PATCH] i2c: imx: dma map the i2c data i/o register
On 16/01/2019 16:17, Laurentiu Tudor wrote:
This is an attempt to fix an iommu exception when doing dma to the
i2c controller with EDMA. Without these mappings the smmu raises a
context fault [1] exactly with the address of the i2c data i/o reg.
This was seen on an NXP LS1043A chip while working on enabling SMMU.
Rather than gradually adding much the same code to potentially every
possible client driver, can it not be implemented once in the edma
driver as was done for pl330 and rcar-dmac? That also sidesteps any of
the nastiness of smuggling a dma_addr_t via a phys_addr_t variable.
Robin.
[1] arm-smmu 900.iommu: Unhandled context fault: fsr=0x402,
iova=0x02180004, fsynr=0x150021, cb=7
Signed-off-by: Laurentiu Tudor
---
drivers/i2c/busses/i2c-imx.c | 57 +---
1 file changed, 47 insertions(+), 10 deletions(-)
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 4e34b1572756..07cc8f4b45b9 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -202,6 +202,9 @@ struct imx_i2c_struct {
struct pinctrl_state *pinctrl_pins_gpio;
struct imx_i2c_dma *dma;
+
+ dma_addr_t dma_tx_addr;
+ dma_addr_t dma_rx_addr;
};
static const struct imx_i2c_hwdata imx1_i2c_hwdata = {
@@ -274,17 +277,20 @@ static inline unsigned char imx_i2c_read_reg(struct
imx_i2c_struct *i2c_imx,
/* Functions for DMA support */
static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
- dma_addr_t phy_addr)
+ phys_addr_t phy_addr)
{
struct imx_i2c_dma *dma;
struct dma_slave_config dma_sconfig;
struct device *dev = _imx->adapter.dev;
int ret;
+ phys_addr_t i2dr_pa;
dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL);
if (!dma)
return -ENOMEM;
+ i2dr_pa = phy_addr + (IMX_I2C_I2DR << i2c_imx->hwdata->regshift);
+
dma->chan_tx = dma_request_chan(dev, "tx");
if (IS_ERR(dma->chan_tx)) {
ret = PTR_ERR(dma->chan_tx);
@@ -293,15 +299,25 @@ static int i2c_imx_dma_request(struct imx_i2c_struct
*i2c_imx,
goto fail_al;
}
- dma_sconfig.dst_addr = phy_addr +
- (IMX_I2C_I2DR << i2c_imx->hwdata->regshift);
+ i2c_imx->dma_tx_addr = dma_map_resource(dma->chan_tx->device->dev,
+ i2dr_pa,
+ DMA_SLAVE_BUSWIDTH_1_BYTE,
+ DMA_MEM_TO_DEV, 0);
+ ret = dma_mapping_error(dma->chan_tx->device->dev,
+ i2c_imx->dma_tx_addr);
+ if (ret) {
+ dev_err(dev, "can't dma map tx destination (%d)\n", ret);
+ goto fail_tx;
+ }
+
+ dma_sconfig.dst_addr = i2c_imx->dma_tx_addr;
dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
dma_sconfig.dst_maxburst = 1;
dma_sconfig.direction = DMA_MEM_TO_DEV;
ret = dmaengine_slave_config(dma->chan_tx, _sconfig);
if (ret < 0) {
dev_err(dev, "can't configure tx channel (%d)\n", ret);
- goto fail_tx;
+ goto fail_tx_dma;
}
dma->chan_rx = dma_request_chan(dev, "rx");
@@ -309,18 +325,28 @@ static int i2c_imx_dma_request(struct imx_i2c_struct
*i2c_imx,
ret = PTR_ERR(dma->chan_rx);
if (ret != -ENODEV && ret != -EPROBE_DEFER)
dev_err(dev, "can't request DMA rx channel (%d)\n",
ret);
- goto fail_tx;
+ goto fail_tx_dma;
}
- dma_sconfig.src_addr = phy_addr +
- (IMX_I2C_I2DR << i2c_imx->hwdata->regshift);
+ i2c_imx->dma_rx_addr = dma_map_resource(dma->chan_rx->device->dev,
+ i2dr_pa,
+ DMA_SLAVE_BUSWIDTH_1_BYTE,
+ DMA_DEV_TO_MEM, 0);
+ ret = dma_mapping_error(dma->chan_rx->device->dev,
+ i2c_imx->dma_rx_addr);
+ if (ret) {
+ dev_err(dev, "can't dma map rx source (%d)\n", ret);
+ goto fail_rx;
+ }
+
+ dma_sconfig.src_addr = i2c_imx->dma_rx_addr;
dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
dma_sconfig.src_maxburst = 1;
dma_sconfig.direction = DMA_DEV_TO_MEM;
ret = dmaengine_slave_config(dma->chan_rx, _sconfig);
if (ret < 0) {
dev_err(dev, "can't configure rx channel (%d)\n", ret);
- goto fail_rx;
+ goto fail_rx_dma;
}
i2c_imx->dma = dma;
@@ -330,8 +356,14 @@ static int i2c_imx_dma_request(struct imx_i2c_struct
*i2c_imx,
return 0;
+fail_rx_dma:
+
Re: [PATCH] dma-debug: add dumping facility via debugfs
On 16/01/2019 13:44, Corentin Labbe wrote:
While debugging a DMA mapping leak, I needed to access
debug_dma_dump_mappings() but easily from user space.
This patch adds a /sys/kernel/debug/dma-api/dump file which contain all
current DMA mapping.
Signed-off-by: Corentin Labbe
---
kernel/dma/debug.c | 47 ++
1 file changed, 47 insertions(+)
diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
index 23cf5361bcf1..9253382f5729 100644
--- a/kernel/dma/debug.c
+++ b/kernel/dma/debug.c
@@ -144,6 +144,7 @@ static struct dentry *num_free_entries_dent __read_mostly;
static struct dentry *min_free_entries_dent __read_mostly;
static struct dentry *nr_total_entries_dent __read_mostly;
static struct dentry *filter_dent __read_mostly;
+static struct dentry *dump_dent __read_mostly;
/* per-driver filter related state */
@@ -840,6 +841,47 @@ static const struct file_operations filter_fops = {
.llseek = default_llseek,
};
+static int dump_read(struct seq_file *seq, void *v)
+{
+ int idx;
+
+ for (idx = 0; idx < HASH_SIZE; idx++) {
+ struct hash_bucket *bucket = _entry_hash[idx];
+ struct dma_debug_entry *entry;
+ unsigned long flags;
+
+ spin_lock_irqsave(>lock, flags);
+
+ list_for_each_entry(entry, >list, list) {
+ seq_printf(seq,
+ "%s %s %s idx %d P=%llx N=%lx D=%llx L=%llx %s
%s\n",
+ dev_name(entry->dev),
+ dev_driver_string(entry->dev),
+ type2name[entry->type], idx,
+ phys_addr(entry), entry->pfn,
+ entry->dev_addr, entry->size,
+ dir2name[entry->direction],
+ maperr2str[entry->map_err_type]);
+ }
+
+ spin_unlock_irqrestore(>lock, flags);
+ }
+ return 0;
+}
It's a shame that this is pretty much a duplication of
debug_dma_dump_mappings(), but there seems no straightforward way to
define one in terms of the other :/
(although given that we'd rather not have that weird external interface
anyway, maybe "now you can use debugfs instead" might be justification
enough for cleaning it up...)
+
+static int dump_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, dump_read, inode->i_private);
+}
+
+static const struct file_operations dump_fops = {
+ .owner = THIS_MODULE,
+ .open = dump_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
DEFINE_SHOW_ATTRIBUTE()?
Robin.
static int dma_debug_fs_init(void)
{
dma_debug_dent = debugfs_create_dir("dma-api", NULL);
@@ -894,6 +936,11 @@ static int dma_debug_fs_init(void)
if (!filter_dent)
goto out_err;
+ dump_dent = debugfs_create_file("dump", 0444,
+ dma_debug_dent, NULL, _fops);
+ if (!dump_dent)
+ goto out_err;
+
return 0;
out_err:
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu: Check for iommu_ops == NULL in iommu_probe_device()
Hi Jean, Joerg, On 1/16/19 5:43 PM, Joerg Roedel wrote: > On Wed, Jan 16, 2019 at 04:33:51PM +, Jean-Philippe Brucker wrote: >> There is a fix on the list: >> https://www.spinics.net/lists/arm-kernel/msg698371.html > > It is also in my tree already, I will send it upstream this week. OK thanks! Eric > > Regards, > > Joerg > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Fail to boot Dom0 on Xen Arm64 after "dma-mapping: bypass indirect calls for dma-direct"
Hi, I made an attempt to boot Linux 5.0-rc2 as Dom0 on Xen Arm64 and got the following splat: [4.074264] Unable to handle kernel NULL pointer dereference at virtual address [4.083074] Mem abort info: [4.085870] ESR = 0x9604 [4.089050] Exception class = DABT (current EL), IL = 32 bits [4.095065] SET = 0, FnV = 0 [4.098138] EA = 0, S1PTW = 0 [4.101405] Data abort info: [4.104362] ISV = 0, ISS = 0x0004 [4.108289] CM = 0, WnR = 0 [4.111328] user pgtable: 4k pages, 48-bit VAs, pgdp = (ptrval) [4.118025] [] pgd= [4.123058] Internal error: Oops: 9604 [#1] PREEMPT SMP [4.128590] Modules linked in: [4.131727] CPU: 4 PID: 1 Comm: swapper/0 Not tainted 5.0.0-rc2-00154-gc5dbed6dcf60 #1191 [4.139997] Hardware name: ARM Juno development board (r2) (DT) [4.145995] pstate: 2005 (nzCv daif -PAN -UAO) [4.150876] pc : xen_swiotlb_alloc_coherent+0x64/0x1e8 [4.156092] lr : dma_alloc_attrs+0xf4/0x110 [4.160359] sp : 1003b960 [4.163743] x29: 1003b960 x28: 112cfbb4 [4.169137] x27: 116ed000 x26: 1003ba90 [4.174533] x25: 10d6c350 x24: 0005 [4.179937] x23: 0002 x22: 0001f000 [4.185323] x21: x20: 8008b904b0b0 [4.190727] x19: 114d4000 x18: 14033fff [4.196113] x17: x16: [4.201509] x15: 4000 x14: 110fc000 [4.206903] x13: 114dd000 x12: 0068 [4.212299] x11: 0001 x10: [4.217693] x9 : x8 : 8008b9b05b00 [4.223089] x7 : x6 : [4.228483] x5 : 106d4c38 x4 : [4.233879] x3 : 006000c0 x2 : 1003ba90 [4.239274] x1 : 0002 x0 : [4.244671] Process swapper/0 (pid: 1, stack limit = 0x(ptrval)) [4.251456] Call trace: [4.253985] xen_swiotlb_alloc_coherent+0x64/0x1e8 [4.258857] dma_alloc_attrs+0xf4/0x110 [4.262774] dmam_alloc_attrs+0x64/0xb8 [4.266691] sil24_port_start+0x6c/0x100 [4.270704] ata_host_start.part.10+0x104/0x210 [4.275304] ata_host_activate+0x64/0x148 [4.279392] sil24_init_one+0x1ac/0x2b0 [4.283312] pci_device_probe+0xdc/0x168 [4.287313] really_probe+0x1f0/0x298 [4.291054] driver_probe_device+0x58/0x100 [4.295316] __driver_attach+0xdc/0xe0 [4.299145] bus_for_each_dev+0x74/0xc8 [4.303061] driver_attach+0x20/0x28 [4.306716] bus_add_driver+0x1b0/0x220 [4.310641] driver_register+0x60/0x110 [4.314549] __pci_register_driver+0x58/0x68 [4.318902] sil24_pci_driver_init+0x20/0x28 [4.323251] do_one_initcall+0xb8/0x348 [4.327166] kernel_init_freeable+0x3cc/0x474 [4.331606] kernel_init+0x10/0x100 [4.335171] ret_from_fork+0x10/0x1c [4.338829] Code: f941fa80 aa1503e4 aa1a03e2 aa1703e1 (f945) [4.345028] ---[ end trace 277757f27697a72b ]--- The bisector fingered the following commit: commit 356da6d0cde3323236977fce54c1f9612a742036 Author: Christoph Hellwig Date: Thu Dec 6 13:39:32 2018 -0800 dma-mapping: bypass indirect calls for dma-direct Avoid expensive indirect calls in the fast path DMA mapping operations by directly calling the dma_direct_* ops if we are using the directly mapped DMA operations. Signed-off-by: Christoph Hellwig Acked-by: Jesper Dangaard Brouer Tested-by: Jesper Dangaard Brouer Tested-by: Tony Luck Discussing with Robin, it looks like that the current wrappers in for Xen (see include/xen/arm/page-coherent.h) are not able to cope with direct calls. Those wrappers are used by swiotlb to call the underlying DMA ops of the device. They are unable to cope with NULL (aka direct-mapping), hence the NULL dereference crash. Do you have any suggestion how this should be fixed? Best regards, -- Julien Grall ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Fix IOMMU page flush when detach all devices from a domain
On Wed, Jan 16, 2019 at 02:40:59PM +, Suthikulpanit, Suravee wrote: > Actually, device_flush_dte(alias) should be needed regardless of this patch. > Are you planning to add this? Yes, I stumbled over this while writing the diff. I'll submit that as a separate patch. Thanks, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Fix IOMMU page flush when detach all devices from a domain
On Wed, Jan 16, 2019 at 02:08:55PM +, Suthikulpanit, Suravee wrote: > Actually, I am not sure how we would be missing the flush on the last device. > In my test, I am seeing the flush command being issued correctly during > vfio_unmap_unpin(), which is after all devices are detached. > Although, I might be missing your point here. Could you please elaborate? Okay, you are right, the patch effectivly adds an unconditional flush of the domain on all iommus when the last device is detached. So it is correct in that regard. But that code path is also used in the iommu_unmap() path. The problem now is, that with your change we send flush commands to all IOMMUs in the unmap path when no device is attached to the domain. This will hurt performance there, no? Regards, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC v3 14/21] iommu: introduce device fault data
On 15/01/2019 21:27, Auger Eric wrote:
[...]
/* iommu fault flags */
-#define IOMMU_FAULT_READ 0x0
-#define IOMMU_FAULT_WRITE 0x1
+#define IOMMU_FAULT_READ (1 << 0)
+#define IOMMU_FAULT_WRITE (1 << 1)
+#define IOMMU_FAULT_EXEC (1 << 2)
+#define IOMMU_FAULT_PRIV (1 << 3)
typedef int (*iommu_fault_handler_t)(struct iommu_domain *,
struct device *, unsigned long, int, void *);
+typedef int (*iommu_dev_fault_handler_t)(struct iommu_fault_event *,
void *);
struct iommu_domain_geometry {
dma_addr_t aperture_start; /* First address that can be
mapped*/ @@ -255,6 +259,52 @@ struct iommu_device {
struct device *dev;
};
+/**
+ * struct iommu_fault_event - Generic per device fault data
+ *
+ * - PCI and non-PCI devices
+ * - Recoverable faults (e.g. page request), information based on
PCI ATS
+ * and PASID spec.
+ * - Un-recoverable faults of device interest
+ * - DMA remapping and IRQ remapping faults
+ *
+ * @fault: fault descriptor
+ * @device_private: if present, uniquely identify device-specific
+ * private data for an individual page request.
+ * @iommu_private: used by the IOMMU driver for storing
fault-specific
+ * data. Users should not modify this field before
+ * sending the fault response.
+ */
+struct iommu_fault_event {
+ struct iommu_fault fault;
+ u64 device_private;
>>> I think we want to move device_private to uapi since it gets injected
>>> into the guest, then returned by guest in case of page response. For
>>> VT-d we also need 128 bits of private data. VT-d spec. 7.7.1
>>
>> Ah, I didn't notice the format changed in VT-d rev3. On that topic, how
>> do we manage future extensions to the iommu_fault struct? Should we add
>> ~48 bytes of padding after device_private, along with some flags telling
>> which field is valid, or deal with it using a structure version like we
>> do for the invalidate and bind structs? In the first case, iommu_fault
>> wouldn't fit in a 64-byte cacheline anymore, but I'm not sure we care.
>>
>>> For exception tracking (e.g. unanswered page request), I can add timer
>>> and list info later when I include PRQ. sounds ok?
+ u64 iommu_private;
>> [...]
+/**
+ * struct iommu_fault - Generic fault data
+ *
+ * @type contains fault type
+ * @reason fault reasons if relevant outside IOMMU driver.
+ * IOMMU driver internal faults are not reported.
+ * @addr: tells the offending page address
+ * @fetch_addr: tells the address that caused an abort, if any
+ * @pasid: contains process address space ID, used in shared virtual
memory
+ * @page_req_group_id: page request group index
+ * @last_req: last request in a page request group
+ * @pasid_valid: indicates if the PRQ has a valid PASID
+ * @prot: page access protection flag:
+ *IOMMU_FAULT_READ, IOMMU_FAULT_WRITE
+ */
+
+struct iommu_fault {
+ __u32 type; /* enum iommu_fault_type */
+ __u32 reason; /* enum iommu_fault_reason */
+ __u64 addr;
+ __u64 fetch_addr;
+ __u32 pasid;
+ __u32 page_req_group_id;
+ __u32 last_req;
+ __u32 pasid_valid;
+ __u32 prot;
+ __u32 access;
>>
>> What does @access contain? Can it be squashed into @prot?
> it was related to F_ACCESS event record and was a placeholder for
> reporting access attributes of the input transaction (Rnw, InD, PnU
> fields). But I wonder whether this is needed to implement such fine
> level fault reporting. Do we really care?
I think we do, to properly inject PRI/Stall later. But RnW, InD and PnU
can already be described with the IOMMU_FAULT_* flags defined above.
We're missing CLASS and S2, which could also be useful for debugging.
CLASS is specific to SMMUv3 but could probably be represented with
@reason. For S2, we could keep printing stage-2 faults in the driver,
and not report them to userspace.
Thanks,
Jean
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu: Check for iommu_ops == NULL in iommu_probe_device()
On Wed, Jan 16, 2019 at 04:33:51PM +, Jean-Philippe Brucker wrote: > There is a fix on the list: > https://www.spinics.net/lists/arm-kernel/msg698371.html It is also in my tree already, I will send it upstream this week. Regards, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu: Check for iommu_ops == NULL in iommu_probe_device()
On 16/01/2019 15:27, Auger Eric wrote:
> Hi Joerg,
>
> On 12/20/18 10:08 AM, Joerg Roedel wrote:
>> From: Joerg Roedel
>>
>> This check needs to be there and got lost at some point
>> during development. Add it again.
>>
>> Fixes: 641fb0efbff0 ('iommu/of: Don't call iommu_ops->add_device directly')
> I experience a regression with those 2 patches. I have virtio-pci device
> protected by virtual smmuv3 and in that case I get err = -EPROBE_DEFER
> after pci_for_each_dma_alias() in of_iommu_configure(). When
> iommu_probe_device is called, ops = NULL so it returns -EINVAL and err
> is overwritten. So there is no deferred probing while it happened before.
There is a fix on the list:
https://www.spinics.net/lists/arm-kernel/msg698371.html
Thanks,
Jean
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[RFC PATCH] i2c: imx: dma map the i2c data i/o register
This is an attempt to fix an iommu exception when doing dma to the
i2c controller with EDMA. Without these mappings the smmu raises a
context fault [1] exactly with the address of the i2c data i/o reg.
This was seen on an NXP LS1043A chip while working on enabling SMMU.
[1] arm-smmu 900.iommu: Unhandled context fault: fsr=0x402,
iova=0x02180004, fsynr=0x150021, cb=7
Signed-off-by: Laurentiu Tudor
---
drivers/i2c/busses/i2c-imx.c | 57 +---
1 file changed, 47 insertions(+), 10 deletions(-)
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 4e34b1572756..07cc8f4b45b9 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -202,6 +202,9 @@ struct imx_i2c_struct {
struct pinctrl_state *pinctrl_pins_gpio;
struct imx_i2c_dma *dma;
+
+ dma_addr_t dma_tx_addr;
+ dma_addr_t dma_rx_addr;
};
static const struct imx_i2c_hwdata imx1_i2c_hwdata = {
@@ -274,17 +277,20 @@ static inline unsigned char imx_i2c_read_reg(struct
imx_i2c_struct *i2c_imx,
/* Functions for DMA support */
static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
- dma_addr_t phy_addr)
+ phys_addr_t phy_addr)
{
struct imx_i2c_dma *dma;
struct dma_slave_config dma_sconfig;
struct device *dev = _imx->adapter.dev;
int ret;
+ phys_addr_t i2dr_pa;
dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL);
if (!dma)
return -ENOMEM;
+ i2dr_pa = phy_addr + (IMX_I2C_I2DR << i2c_imx->hwdata->regshift);
+
dma->chan_tx = dma_request_chan(dev, "tx");
if (IS_ERR(dma->chan_tx)) {
ret = PTR_ERR(dma->chan_tx);
@@ -293,15 +299,25 @@ static int i2c_imx_dma_request(struct imx_i2c_struct
*i2c_imx,
goto fail_al;
}
- dma_sconfig.dst_addr = phy_addr +
- (IMX_I2C_I2DR << i2c_imx->hwdata->regshift);
+ i2c_imx->dma_tx_addr = dma_map_resource(dma->chan_tx->device->dev,
+ i2dr_pa,
+ DMA_SLAVE_BUSWIDTH_1_BYTE,
+ DMA_MEM_TO_DEV, 0);
+ ret = dma_mapping_error(dma->chan_tx->device->dev,
+ i2c_imx->dma_tx_addr);
+ if (ret) {
+ dev_err(dev, "can't dma map tx destination (%d)\n", ret);
+ goto fail_tx;
+ }
+
+ dma_sconfig.dst_addr = i2c_imx->dma_tx_addr;
dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
dma_sconfig.dst_maxburst = 1;
dma_sconfig.direction = DMA_MEM_TO_DEV;
ret = dmaengine_slave_config(dma->chan_tx, _sconfig);
if (ret < 0) {
dev_err(dev, "can't configure tx channel (%d)\n", ret);
- goto fail_tx;
+ goto fail_tx_dma;
}
dma->chan_rx = dma_request_chan(dev, "rx");
@@ -309,18 +325,28 @@ static int i2c_imx_dma_request(struct imx_i2c_struct
*i2c_imx,
ret = PTR_ERR(dma->chan_rx);
if (ret != -ENODEV && ret != -EPROBE_DEFER)
dev_err(dev, "can't request DMA rx channel (%d)\n",
ret);
- goto fail_tx;
+ goto fail_tx_dma;
}
- dma_sconfig.src_addr = phy_addr +
- (IMX_I2C_I2DR << i2c_imx->hwdata->regshift);
+ i2c_imx->dma_rx_addr = dma_map_resource(dma->chan_rx->device->dev,
+ i2dr_pa,
+ DMA_SLAVE_BUSWIDTH_1_BYTE,
+ DMA_DEV_TO_MEM, 0);
+ ret = dma_mapping_error(dma->chan_rx->device->dev,
+ i2c_imx->dma_rx_addr);
+ if (ret) {
+ dev_err(dev, "can't dma map rx source (%d)\n", ret);
+ goto fail_rx;
+ }
+
+ dma_sconfig.src_addr = i2c_imx->dma_rx_addr;
dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
dma_sconfig.src_maxburst = 1;
dma_sconfig.direction = DMA_DEV_TO_MEM;
ret = dmaengine_slave_config(dma->chan_rx, _sconfig);
if (ret < 0) {
dev_err(dev, "can't configure rx channel (%d)\n", ret);
- goto fail_rx;
+ goto fail_rx_dma;
}
i2c_imx->dma = dma;
@@ -330,8 +356,14 @@ static int i2c_imx_dma_request(struct imx_i2c_struct
*i2c_imx,
return 0;
+fail_rx_dma:
+ dma_unmap_resource(dma->chan_rx->device->dev, i2c_imx->dma_rx_addr,
+ DMA_SLAVE_BUSWIDTH_1_BYTE, DMA_DEV_TO_MEM, 0);
fail_rx:
dma_release_channel(dma->chan_rx);
+fail_tx_dma:
+ dma_unmap_resource(dma->chan_tx->device->dev, i2c_imx->dma_tx_addr,
+ DMA_SLAVE_BUSWIDTH_1_BYTE,
Re: [RFC v3 14/21] iommu: introduce device fault data
On 14/01/2019 22:32, Jacob Pan wrote:
>> [...]
+/**
+ * struct iommu_fault - Generic fault data
+ *
+ * @type contains fault type
+ * @reason fault reasons if relevant outside IOMMU driver.
+ * IOMMU driver internal faults are not reported.
+ * @addr: tells the offending page address
+ * @fetch_addr: tells the address that caused an abort, if any
+ * @pasid: contains process address space ID, used in shared
virtual memory
+ * @page_req_group_id: page request group index
+ * @last_req: last request in a page request group
+ * @pasid_valid: indicates if the PRQ has a valid PASID
+ * @prot: page access protection flag:
+ *IOMMU_FAULT_READ, IOMMU_FAULT_WRITE
+ */
+
+struct iommu_fault {
+ __u32 type; /* enum iommu_fault_type */
+ __u32 reason; /* enum iommu_fault_reason */
+ __u64 addr;
+ __u64 fetch_addr;
+ __u32 pasid;
+ __u32 page_req_group_id;
+ __u32 last_req;
+ __u32 pasid_valid;
+ __u32 prot;
+ __u32 access;
>>
>> What does @access contain? Can it be squashed into @prot?
>>
> I agreed.
>
> how about this?
> #define IOMMU_FAULT_VERSION_V1 0x1
> struct iommu_fault {
> __u16 version;
Right, but the version field becomes redundant when we present a batch
of these to userspace, in patch 18 (assuming we don't want to mix fault
structure versions within a batch... I certainly don't).
When introducing IOMMU_FAULT_VERSION_V2, in a distant future, I think we
still need to support a userspace that uses IOMMU_FAULT_VERSION_V1. One
strategy for this:
* We define structs iommu_fault_v1 (the current iommu_fault) and
iommu_fault_v2.
* Userspace selects IOMMU_FAULT_VERSION_V1 when registering the fault
queue
* The IOMMU driver fills iommu_fault_v2 and passes it to VFIO
* VFIO does its best to translate this into a iommu_fault_v1 struct
So what we need now, is a way for userspace to tell the kernel which
structure version it expects. I'm not sure we even need to pass the
actual version number we're using back to userspace. Agreeing on one
version at registration should be sufficient.
> __u16 type;
> __u32 reason;
> __u64 addr;
I'm in favor of keeping @fetch_addr as well, it can contain useful
information. For example, while attempting to translate an IOVA
0xf000, the IOMMU can't find the PASID table that we installed with
address 0xdead - the guest passed an invalid address to
bind_pasid_table(). We can then report 0xf000 in @addr, and 0xdead
in @fetch_addr.
> __u32 pasid;
> __u32 page_req_group_id;
> __u32 last_req : 1;
> __u32 pasid_valid : 1;
Agreed, with some explicit padding or combined as a @flag field. In fact
if we do add the @fetch_addr field, I think we need a bit that indicates
its validity as well.
Thanks,
Jean
> __u32 prot;
> __u64 device_private[2];
> __u8 padding[48];
> };
>
>
>> Thanks,
>> Jean
>>
>>> relocated to uapi, Yi can you confirm?
>>> __u64 device_private[2];
>>>
+};
#endif /* _UAPI_IOMMU_H */
>>>
>>> ___
>>> iommu mailing list
>>> iommu@lists.linux-foundation.org
>>> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>>
>>
>
> [Jacob Pan]
> ___
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] dma-debug: add dumping facility via debugfs
While debugging a DMA mapping leak, I needed to access
debug_dma_dump_mappings() but easily from user space.
This patch adds a /sys/kernel/debug/dma-api/dump file which contain all
current DMA mapping.
Signed-off-by: Corentin Labbe
---
kernel/dma/debug.c | 47 ++
1 file changed, 47 insertions(+)
diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
index 23cf5361bcf1..9253382f5729 100644
--- a/kernel/dma/debug.c
+++ b/kernel/dma/debug.c
@@ -144,6 +144,7 @@ static struct dentry *num_free_entries_dent __read_mostly;
static struct dentry *min_free_entries_dent __read_mostly;
static struct dentry *nr_total_entries_dent __read_mostly;
static struct dentry *filter_dent __read_mostly;
+static struct dentry *dump_dent __read_mostly;
/* per-driver filter related state */
@@ -840,6 +841,47 @@ static const struct file_operations filter_fops = {
.llseek = default_llseek,
};
+static int dump_read(struct seq_file *seq, void *v)
+{
+ int idx;
+
+ for (idx = 0; idx < HASH_SIZE; idx++) {
+ struct hash_bucket *bucket = _entry_hash[idx];
+ struct dma_debug_entry *entry;
+ unsigned long flags;
+
+ spin_lock_irqsave(>lock, flags);
+
+ list_for_each_entry(entry, >list, list) {
+ seq_printf(seq,
+ "%s %s %s idx %d P=%llx N=%lx D=%llx L=%llx
%s %s\n",
+ dev_name(entry->dev),
+ dev_driver_string(entry->dev),
+ type2name[entry->type], idx,
+ phys_addr(entry), entry->pfn,
+ entry->dev_addr, entry->size,
+ dir2name[entry->direction],
+ maperr2str[entry->map_err_type]);
+ }
+
+ spin_unlock_irqrestore(>lock, flags);
+ }
+ return 0;
+}
+
+static int dump_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, dump_read, inode->i_private);
+}
+
+static const struct file_operations dump_fops = {
+ .owner = THIS_MODULE,
+ .open = dump_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
static int dma_debug_fs_init(void)
{
dma_debug_dent = debugfs_create_dir("dma-api", NULL);
@@ -894,6 +936,11 @@ static int dma_debug_fs_init(void)
if (!filter_dent)
goto out_err;
+ dump_dent = debugfs_create_file("dump", 0444,
+ dma_debug_dent, NULL, _fops);
+ if (!dump_dent)
+ goto out_err;
+
return 0;
out_err:
--
2.19.2
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu: Check for iommu_ops == NULL in iommu_probe_device()
Hi Joerg,
On 12/20/18 10:08 AM, Joerg Roedel wrote:
> From: Joerg Roedel
>
> This check needs to be there and got lost at some point
> during development. Add it again.
>
> Fixes: 641fb0efbff0 ('iommu/of: Don't call iommu_ops->add_device directly')
I experience a regression with those 2 patches. I have virtio-pci device
protected by virtual smmuv3 and in that case I get err = -EPROBE_DEFER
after pci_for_each_dma_alias() in of_iommu_configure(). When
iommu_probe_device is called, ops = NULL so it returns -EINVAL and err
is overwritten. So there is no deferred probing while it happened before.
Thanks
Eric
> Reported-by: Marek Szyprowski
> Reported-by: kernelci.org bot
> Signed-off-by: Joerg Roedel
> ---
> drivers/iommu/iommu.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index a2131751dcff..3ed4db334341 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -114,10 +114,14 @@ void iommu_device_unregister(struct iommu_device *iommu)
> int iommu_probe_device(struct device *dev)
> {
> const struct iommu_ops *ops = dev->bus->iommu_ops;
> + int ret = -EINVAL;
>
> WARN_ON(dev->iommu_group);
>
> - return ops->add_device(dev);
> + if (ops)
> + ret = ops->add_device(dev);
> +
> + return ret;
> }
>
> void iommu_release_device(struct device *dev)
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] swiotlb: clear io_tlb_start and io_tlb_end in swiotlb_exit
On Mon, Jan 14, 2019 at 09:33:23PM +0100, Christoph Hellwig wrote:
> On Mon, Jan 14, 2019 at 03:25:13PM -0500, Konrad Rzeszutek Wilk wrote:
> > On Mon, Jan 14, 2019 at 09:14:08PM +0100, Christoph Hellwig wrote:
> > > Otherwise is_swiotlb_buffer will return false positives when
> > > we first initialize a swiotlb buffer, but then free it because
> > > we have an IOMMU available.
> > >
> > > Fixes: 55897af63091 ("dma-direct: merge swiotlb_dma_ops into the
> > > dma_direct code")
> > > Reported-by: Sibren Vasse
> > > Signed-off-by: Christoph Hellwig
> > > Tested-by: Sibren Vasse
> >
> > Reviewed-by: Konrad Rzeszutek Wilk
>
> Do you want to send this to Linus, or do you want me to pick it up
> through the dma-mapping tree?
I will do it. Thanks!
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Fix IOMMU page flush when detach all devices from a domain
Joerg,
On 1/16/19 8:26 PM, j...@8bytes.org wrote:
> How about the attached diff? If
> I understand the problem correctly, it should fix the problem more
> reliably.
>
> Thanks,
>
> Joerg
>
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 87ba23a75b38..dc1e2a8a19d7 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -1991,25 +1991,36 @@ static void do_attach(struct iommu_dev_data *dev_data,
>
> static void do_detach(struct iommu_dev_data *dev_data)
> {
> + struct protection_domain *domain = dev_data->domain;
> struct amd_iommu *iommu;
> u16 alias;
>
> iommu = amd_iommu_rlookup_table[dev_data->devid];
> alias = dev_data->alias;
>
> - /* decrease reference counters */
> - dev_data->domain->dev_iommu[iommu->index] -= 1;
> - dev_data->domain->dev_cnt -= 1;
> -
> /* Update data structures */
> dev_data->domain = NULL;
> list_del(_data->list);
> - clear_dte_entry(dev_data->devid);
> - if (alias != dev_data->devid)
> - clear_dte_entry(alias);
>
> + clear_dte_entry(dev_data->devid);
> /* Flush the DTE entry */
> device_flush_dte(dev_data);
> +
> + if (alias != dev_data->devid) {
> + clear_dte_entry(alias);
> + /* Flush the Alias DTE entry */
> + device_flush_dte(alias);
> + }
> +
Actually, device_flush_dte(alias) should be needed regardless of this patch.
Are you planning to add this?
Regards,
Suravee
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Mark translation invalid during device detach
Joerg, On 1/16/19 8:03 PM, j...@8bytes.org wrote: > Hi Suravee, > > On Wed, Jan 16, 2019 at 04:15:10AM +, Suthikulpanit, Suravee wrote: >> From: Suravee Suthikulpanit >> >> When a device switches domain, IOMMU driver detach device from the old >> domain, and attach device to the new domain. During this period >> the host table root pointer is not set, which means DMA translation >> should be marked as invalid (clear TV bit). >> >> So, clear the TV bit when detach the device. The TV bit will be set >> again when attaching device to the new domain. > > Is there a specific problem with setting the TV bit? We are not currently seeing issue. > Note that the update will clear all other fields in the first 128 bits > of the DTE, which means that IR, IW and Mode are all set to 0. This > effectivly blocks all DMA requests from the device, which is by design. Ahh.. This makes sense now. I missed the IR/IW/Mode=0 part. It was not clear to us earlier. Thanks for clarification. Regards, Suravee ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/3] virtio-blk: Consider dma_max_mapping_size() for maximum segment size
On Wed, Jan 16, 2019 at 09:05:40AM -0500, Michael S. Tsirkin wrote: > On Tue, Jan 15, 2019 at 02:22:57PM +0100, Joerg Roedel wrote: > > + max_size = dma_max_mapping_size(>dev); > > + > > > Should this be limited to ACCESS_PLATFORM? > > I see no reason to limit this without as guest can > access any memory. Actually, yes. This should be inside a use_dma_api check. I had it in v1, but it went away without the vring layer for propagating the limit. I'll add that again. Thanks, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 0/3] Fix virtio-blk issue with SWIOTLB
On Tue, Jan 15, 2019 at 02:20:19PM +0100, Christoph Hellwig wrote: > On Tue, Jan 15, 2019 at 09:37:42AM +0100, Joerg Roedel wrote: > > On Mon, Jan 14, 2019 at 01:20:45PM -0500, Michael S. Tsirkin wrote: > > > Which would be fine especially if we can manage not to introduce a bunch > > > of indirect calls all over the place and hurt performance. > > > > Which indirect calls? In case of unset dma_ops the DMA-API functions > > call directly into the dma-direct implementation, no indirect calls at > > all. > > True. But the NULL-ops dma direct case still has two issues that might > not work for virtio: > > (a) if the architecture supports devices that are not DMA coherent it > will dip into a special allocator and do cache maintainance for > streaming mappings. Although it would be a bit of a hack we could > work around this in virtio doings something like: > > #if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \ > defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \ > defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL) > dev->dma_coherent = true; > #endif > > except that won't work for mips, which has a mode where it does > a system instead of device level coherency flag and thus doesn't > look at this struct device field > > (b) we can still mangle the DMA address, either using the > dma_pfn_offset field in struct device, or by a full override > of __phys_to_dma / __dma_to_phys by the architecture code. > The first could be solved using a hack like the one above, > but the latter would be a little harder. In the long run > I'd love to get rid of that hook and have everyone use the > generic offset code, but for that we first need to support > multiple ranges with different offset and do quite some > nasty arch code surgery. > > So for the legacy virtio case I fear we need to keep local dma mapping > implementation for now. I just wish now recent hypervisor would ever > offer devices in this broken legacy mode.. IIUC some emulated hardware has no reasonable way to specify that some devices bypass the IOMMU, and no cheap way to cover all memory with an IOMMU mapping. So I suspect !ACCESS_PLATFORM will stay a supported configuration forever :( > > > > Regards, > > > > Joerg > ---end quoted text--- ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Fix IOMMU page flush when detach all devices from a domain
Joerg,
On 1/16/19 8:26 PM, j...@8bytes.org wrote:
> On Wed, Jan 16, 2019 at 04:16:25AM +, Suthikulpanit, Suravee wrote:
>> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
>> index 525659b88ade..ab31ba75da1b 100644
>> --- a/drivers/iommu/amd_iommu.c
>> +++ b/drivers/iommu/amd_iommu.c
>> @@ -1248,7 +1248,13 @@ static void __domain_flush_pages(struct
>> protection_domain *domain,
>> build_inv_iommu_pages(, address, size, domain->id, pde);
>>
>> for (i = 0; i < amd_iommu_get_num_iommus(); ++i) {
>> -if (!domain->dev_iommu[i])
>> +/*
>> + * The dev_cnt is zero when all devices are detached
>> + * from the domain. This is the case when VFIO detaches
>> + * all devices from the group before flushing IOMMU pages.
>> + * So, always issue the flush command.
>> + */
>> +if (domain->dev_cnt && !domain->dev_iommu[i])
>> continue;
>
> This doesn't look like the correct fix. We still miss the flush if we
> detach the last device from the domain.
Actually, I am not sure how we would be missing the flush on the last device.
In my test, I am seeing the flush command being issued correctly during
vfio_unmap_unpin(), which is after all devices are detached.
Although, I might be missing your point here. Could you please elaborate?
> How about the attached diff? If
> I understand the problem correctly, it should fix the problem more
> reliably.
>
> Thanks,
>
> Joerg
>
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 87ba23a75b38..dc1e2a8a19d7 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -1991,25 +1991,36 @@ static void do_attach(struct iommu_dev_data *dev_data,
>
> static void do_detach(struct iommu_dev_data *dev_data)
> {
> + struct protection_domain *domain = dev_data->domain;
> struct amd_iommu *iommu;
> u16 alias;
>
> iommu = amd_iommu_rlookup_table[dev_data->devid];
> alias = dev_data->alias;
>
> - /* decrease reference counters */
> - dev_data->domain->dev_iommu[iommu->index] -= 1;
> - dev_data->domain->dev_cnt -= 1;
> -
> /* Update data structures */
> dev_data->domain = NULL;
> list_del(_data->list);
> - clear_dte_entry(dev_data->devid);
> - if (alias != dev_data->devid)
> - clear_dte_entry(alias);
>
> + clear_dte_entry(dev_data->devid);
> /* Flush the DTE entry */
> device_flush_dte(dev_data);
> +
> + if (alias != dev_data->devid) {
> + clear_dte_entry(alias);
> + /* Flush the Alias DTE entry */
> + device_flush_dte(alias);
> + }
> +
> + /* Flush IOTLB */
> + domain_flush_tlb_pde(domain);
> +
> + /* Wait for the flushes to finish */
> + domain_flush_complete(domain);
> +
> + /* decrease reference counters - needs to happen after the flushes */
> + domain->dev_iommu[iommu->index] -= 1;
> + domain->dev_cnt -= 1;
> }
I have also considered this. This would also work. But since we are already
doing page flushes during page unmapping later on after all devices are
detached.
So, would this be necessary? Please see vfio_iommu_type1_detach_group().
Also, if we consider the case where there are more than one devices sharing
the domain. This would issue page flush every time we detach a device,
and while other devices still attached to the domain.
Regards,
Suravee
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/3] virtio-blk: Consider dma_max_mapping_size() for maximum segment size
On Tue, Jan 15, 2019 at 02:22:57PM +0100, Joerg Roedel wrote: > From: Joerg Roedel > > Segments can't be larger than the maximum DMA mapping size > supported on the platform. Take that into account when > setting the maximum segment size for a block device. > > Signed-off-by: Joerg Roedel > --- > drivers/block/virtio_blk.c | 10 ++ > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c > index b16a887bbd02..6193962a7fec 100644 > --- a/drivers/block/virtio_blk.c > +++ b/drivers/block/virtio_blk.c > @@ -723,7 +723,7 @@ static int virtblk_probe(struct virtio_device *vdev) > struct request_queue *q; > int err, index; > > - u32 v, blk_size, sg_elems, opt_io_size; > + u32 v, blk_size, max_size, sg_elems, opt_io_size; > u16 min_io_size; > u8 physical_block_exp, alignment_offset; > > @@ -826,14 +826,16 @@ static int virtblk_probe(struct virtio_device *vdev) > /* No real sector limit. */ > blk_queue_max_hw_sectors(q, -1U); > > + max_size = dma_max_mapping_size(>dev); > + Should this be limited to ACCESS_PLATFORM? I see no reason to limit this without as guest can access any memory. I'd like a bit of time to consider this point. > /* Host can optionally specify maximum segment size and number of >* segments. */ > err = virtio_cread_feature(vdev, VIRTIO_BLK_F_SIZE_MAX, > struct virtio_blk_config, size_max, ); > if (!err) > - blk_queue_max_segment_size(q, v); > - else > - blk_queue_max_segment_size(q, -1U); > + max_size = min(max_size, v); > + > + blk_queue_max_segment_size(q, max_size); > > /* Host can optionally specify the block size of the device */ > err = virtio_cread_feature(vdev, VIRTIO_BLK_F_BLK_SIZE, > -- > 2.17.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: remove block layer bounce buffering for MMC
On Wed, Jan 16, 2019 at 2:51 PM Linus Walleij wrote: > > On Wed, Jan 16, 2019 at 11:25 AM Arnd Bergmann wrote: > > On Mon, Jan 14, 2019 at 11:27 AM Ulf Hansson wrote: > > > > > > +Linus Walleij (recently made a cleanup of the mmc bounce buffering code). > > Nah it's not THAT bounce buffer. > > > Linus probably knows more here, but I have a vague recollection of > > the MMC bounce buffer code being needed mostly for performance > > reasons: when the scatterlist is discontiguous, that can result in > > a request being split up into separate MMC commands, which due > > to the lack of queued commands combined with the need for > > garbage collection on sub-page writes results in a huge slowdown > > compared to having larger bounce buffers all the time. > > > > We had discussed finding a different way to do this (separate > > from the bounce buffering), but I don't know if that ever happened, > > or if this is even the code that you are changing here. > > Nope not the same code. > > The term "bounce buffer" is sadly used as ambigously as > __underscores in front of function names. > > That other "bounce buffer" was first deleted and then > reimplemented as a local hack in the SDHCI driver core > after it caused performance regressions on the i.MX and > some laptops, see commit: > > commit bd9b902798ab14d19ca116b10bde581ddff8f905 > mmc: sdhci: Implement an SDHCI-specific bounce buffer > > That should be orthogonal to Christoph's changes in this > patch series. Ok, thanks for the clarification. Please ignore my comments then. Arnd ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: remove block layer bounce buffering for MMC
On Wed, Jan 16, 2019 at 11:25 AM Arnd Bergmann wrote: > On Mon, Jan 14, 2019 at 11:27 AM Ulf Hansson wrote: > > > > +Linus Walleij (recently made a cleanup of the mmc bounce buffering code). Nah it's not THAT bounce buffer. > Linus probably knows more here, but I have a vague recollection of > the MMC bounce buffer code being needed mostly for performance > reasons: when the scatterlist is discontiguous, that can result in > a request being split up into separate MMC commands, which due > to the lack of queued commands combined with the need for > garbage collection on sub-page writes results in a huge slowdown > compared to having larger bounce buffers all the time. > > We had discussed finding a different way to do this (separate > from the bounce buffering), but I don't know if that ever happened, > or if this is even the code that you are changing here. Nope not the same code. The term "bounce buffer" is sadly used as ambigously as __underscores in front of function names. That other "bounce buffer" was first deleted and then reimplemented as a local hack in the SDHCI driver core after it caused performance regressions on the i.MX and some laptops, see commit: commit bd9b902798ab14d19ca116b10bde581ddff8f905 mmc: sdhci: Implement an SDHCI-specific bounce buffer That should be orthogonal to Christoph's changes in this patch series. Yours, Linus Walleij ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 00/24] Tegra GART driver clean up and optimization
16.01.2019 15:55, Joerg Roedel пишет: > On Tue, Jan 15, 2019 at 07:33:54PM +0300, Dmitry Osipenko wrote: >> Should I expect that the patches will appear in the IOMMU git and in >> -next consequently? Please let me know if there is any problem with >> the applying of the patches. > > Hmm, I thought I applied these patches already, but obviously they > didn't show up in the iommu-tree. I have no idea how that happened, but > they are now applied. > > Thanks for the reminder, > >Joerg > Thanks! ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Fix IOMMU page flush when detach all devices from a domain
On Wed, Jan 16, 2019 at 04:16:25AM +, Suthikulpanit, Suravee wrote:
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 525659b88ade..ab31ba75da1b 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -1248,7 +1248,13 @@ static void __domain_flush_pages(struct
> protection_domain *domain,
> build_inv_iommu_pages(, address, size, domain->id, pde);
>
> for (i = 0; i < amd_iommu_get_num_iommus(); ++i) {
> - if (!domain->dev_iommu[i])
> + /*
> + * The dev_cnt is zero when all devices are detached
> + * from the domain. This is the case when VFIO detaches
> + * all devices from the group before flushing IOMMU pages.
> + * So, always issue the flush command.
> + */
> + if (domain->dev_cnt && !domain->dev_iommu[i])
> continue;
This doesn't look like the correct fix. We still miss the flush if we
detach the last device from the domain. How about the attached diff? If
I understand the problem correctly, it should fix the problem more
reliably.
Thanks,
Joerg
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 87ba23a75b38..dc1e2a8a19d7 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -1991,25 +1991,36 @@ static void do_attach(struct iommu_dev_data *dev_data,
static void do_detach(struct iommu_dev_data *dev_data)
{
+ struct protection_domain *domain = dev_data->domain;
struct amd_iommu *iommu;
u16 alias;
iommu = amd_iommu_rlookup_table[dev_data->devid];
alias = dev_data->alias;
- /* decrease reference counters */
- dev_data->domain->dev_iommu[iommu->index] -= 1;
- dev_data->domain->dev_cnt -= 1;
-
/* Update data structures */
dev_data->domain = NULL;
list_del(_data->list);
- clear_dte_entry(dev_data->devid);
- if (alias != dev_data->devid)
- clear_dte_entry(alias);
+ clear_dte_entry(dev_data->devid);
/* Flush the DTE entry */
device_flush_dte(dev_data);
+
+ if (alias != dev_data->devid) {
+ clear_dte_entry(alias);
+ /* Flush the Alias DTE entry */
+ device_flush_dte(alias);
+ }
+
+ /* Flush IOTLB */
+ domain_flush_tlb_pde(domain);
+
+ /* Wait for the flushes to finish */
+ domain_flush_complete(domain);
+
+ /* decrease reference counters - needs to happen after the flushes */
+ domain->dev_iommu[iommu->index] -= 1;
+ domain->dev_cnt -= 1;
}
/*
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Mark translation invalid during device detach
Hi Suravee, On Wed, Jan 16, 2019 at 04:15:10AM +, Suthikulpanit, Suravee wrote: > From: Suravee Suthikulpanit > > When a device switches domain, IOMMU driver detach device from the old > domain, and attach device to the new domain. During this period > the host table root pointer is not set, which means DMA translation > should be marked as invalid (clear TV bit). > > So, clear the TV bit when detach the device. The TV bit will be set > again when attaching device to the new domain. Is there a specific problem with setting the TV bit? Note that the update will clear all other fields in the first 128 bits of the DTE, which means that IR, IW and Mode are all set to 0. This effectivly blocks all DMA requests from the device, which is by design. Regards, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 00/24] Tegra GART driver clean up and optimization
On Tue, Jan 15, 2019 at 07:33:54PM +0300, Dmitry Osipenko wrote: > Should I expect that the patches will appear in the IOMMU git and in > -next consequently? Please let me know if there is any problem with > the applying of the patches. Hmm, I thought I applied these patches already, but obviously they didn't show up in the iommu-tree. I have no idea how that happened, but they are now applied. Thanks for the reminder, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC v3 17/21] iommu/smmuv3: Report non recoverable faults
Hi Jean, On 1/16/19 1:25 PM, Jean-Philippe Brucker wrote: > On 15/01/2019 21:06, Auger Eric wrote: + iommu_report_device_fault(master->dev, ); >>> >>> We should return here if the fault is successfully injected >> >> Even if the fault gets injected in the guest can't it be still useful to >> get the message below on host side? > > I don't think we should let the guest flood the host log by issuing > invalid DMA (or are there other cases where the guest can freely print > stuff in the host?) We do print all errors at the moment, but we should > tighten this once there is an upstream solution to let the guest control > DMA mappings. OK thank you for the clarification. Yes it makes sense to skip those traces then. Thanks Eric > > Thanks, > Jean > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC v3 17/21] iommu/smmuv3: Report non recoverable faults
On 15/01/2019 21:06, Auger Eric wrote: >>> + iommu_report_device_fault(master->dev, ); >> >> We should return here if the fault is successfully injected > > Even if the fault gets injected in the guest can't it be still useful to > get the message below on host side? I don't think we should let the guest flood the host log by issuing invalid DMA (or are there other cases where the guest can freely print stuff in the host?) We do print all errors at the moment, but we should tighten this once there is an upstream solution to let the guest control DMA mappings. Thanks, Jean ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] Revert "ARM: dma-mapping: Set proper DMA ops in arm_iommu_detach_device()"
Hi Robin, On 2019-01-14 17:38, Robin Murphy wrote: > On 14/01/2019 16:09, Thierry Reding wrote: >> On Mon, Jan 14, 2019 at 02:22:40PM +0100, Marek Szyprowski wrote: >>> This reverts commit 1874619a7df4b14b23b14877e705ae15325773e3. >>> >>> That patch broke IOMMU support for devices, which fails to probe for >>> the >>> first time and use deferred probe approach. When non-NULL dma_ops is >>> set >>> in arm_iommu_detach_device(), the given device later ignored by >>> arch_setup_dma_ops() and stays with non-IOMMU dma_ops. >>> >>> Reported-by: Tobias Jakobi >>> Fixes: 1874619a7df4 "ARM: dma-mapping: Set proper DMA ops in >>> arm_iommu_detach_device()" >>> Signed-off-by: Marek Szyprowski >>> --- >>> arch/arm/mm/dma-mapping.c | 12 ++-- >>> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> Can you point out exactly what drivers break because of this change? We >> need to find a solution that works for everyone. Reverting is only >> marginally useful because somebody will just end up wanting to revert >> the revert because a different driver is now broken. > > At first glance, it sounds like what we really want is for > arch_teardown_iommu_ops() to completely clear any ops that > arch_setup_dma_ops() installed - does the below suffice? I've initially also thought about similar fix, but then I found the commit 1874619a7df4b14b23b14877e705ae15325773e3, which was the source of this problem. Robin: do you plan to send this fix as a complete patch or do you want me to resend it with the above commit message and your's suggested-by? > > Robin. > > ->8- > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c > index f1e2922e447c..1e3e08a1c456 100644 > --- a/arch/arm/mm/dma-mapping.c > +++ b/arch/arm/mm/dma-mapping.c > @@ -2390,4 +2390,6 @@ void arch_teardown_dma_ops(struct device *dev) > return; > > arm_teardown_iommu_dma_ops(dev); > + /* Let arch_setup_dma_ops() start again from scratch upon > re-probe */ > + set_dma_ops(dev, NULL); > } > > Best regards -- Marek Szyprowski, PhD Samsung R Institute Poland ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: remove block layer bounce buffering for MMC
On Mon, Jan 14, 2019 at 11:27 AM Ulf Hansson wrote: > > +Linus Walleij (recently made a cleanup of the mmc bounce buffering code). > > On Mon, 14 Jan 2019 at 10:58, Christoph Hellwig wrote: > > > > Hi everyone, > > > > this series converts the remaining MMC host drivers to properly kmap the > > scatterlist entries it does PIO operations on, and then goes on to > > remove the usage of block layer bounce buffering (which I plan to remove > > eventually) from the MMC layer. > > > > As a bonus I've converted various drivers to the proper scatterlist > > helpers so that at least in theory they are ready for chained > > scatterlists. > > > > All the changes are compile tested only as I don't have any of the > > hardware, so a careful review would be appreciated. > > Thanks for posting this. I will have a look as soon as I can, but > first needs to catch up with things since the holidays. Linus probably knows more here, but I have a vague recollection of the MMC bounce buffer code being needed mostly for performance reasons: when the scatterlist is discontiguous, that can result in a request being split up into separate MMC commands, which due to the lack of queued commands combined with the need for garbage collection on sub-page writes results in a huge slowdown compared to having larger bounce buffers all the time. We had discussed finding a different way to do this (separate from the bounce buffering), but I don't know if that ever happened, or if this is even the code that you are changing here. Arnd ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
