Re: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs
Hi Alex,
On 7/30/20 4:32 AM, Alex Williamson wrote:
On Tue, 14 Jul 2020 13:57:03 +0800
Lu Baolu wrote:
Replace iommu_aux_at(de)tach_device() with iommu_aux_at(de)tach_group().
It also saves the IOMMU_DEV_FEAT_AUX-capable physcail device in the
vfio_group data structure so that it could be reused in other places.
Signed-off-by: Lu Baolu
---
drivers/vfio/vfio_iommu_type1.c | 44 ++---
1 file changed, 7 insertions(+), 37 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 5e556ac9102a..f8812e68de77 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -100,6 +100,7 @@ struct vfio_dma {
struct vfio_group {
struct iommu_group *iommu_group;
struct list_headnext;
+ struct device *iommu_device;
boolmdev_group; /* An mdev group */
boolpinned_page_dirty_scope;
};
@@ -1627,45 +1628,13 @@ static struct device *vfio_mdev_get_iommu_device(struct
device *dev)
return NULL;
}
-static int vfio_mdev_attach_domain(struct device *dev, void *data)
-{
- struct iommu_domain *domain = data;
- struct device *iommu_device;
-
- iommu_device = vfio_mdev_get_iommu_device(dev);
- if (iommu_device) {
- if (iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX))
- return iommu_aux_attach_device(domain, iommu_device);
- else
- return iommu_attach_device(domain, iommu_device);
- }
-
- return -EINVAL;
-}
-
-static int vfio_mdev_detach_domain(struct device *dev, void *data)
-{
- struct iommu_domain *domain = data;
- struct device *iommu_device;
-
- iommu_device = vfio_mdev_get_iommu_device(dev);
- if (iommu_device) {
- if (iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX))
- iommu_aux_detach_device(domain, iommu_device);
- else
- iommu_detach_device(domain, iommu_device);
- }
-
- return 0;
-}
-
static int vfio_iommu_attach_group(struct vfio_domain *domain,
struct vfio_group *group)
{
if (group->mdev_group)
- return iommu_group_for_each_dev(group->iommu_group,
- domain->domain,
- vfio_mdev_attach_domain);
+ return iommu_aux_attach_group(domain->domain,
+ group->iommu_group,
+ group->iommu_device);
No, we previously iterated all devices in the group and used the aux
interface only when we have an iommu_device supporting aux. If we
simply assume an mdev group only uses an aux domain we break existing
users, ex. SR-IOV VF backed mdevs. Thanks,
Oh, yes. Sorry! I didn't consider the physical device backed mdevs
cases.
Looked into this part of code, it seems that there's a lock issue here.
The group->mutex is held in iommu_group_for_each_dev() and will be
acquired again in iommu_attach_device().
How about making it like:
static int vfio_iommu_attach_group(struct vfio_domain *domain,
struct vfio_group *group)
{
if (group->mdev_group) {
struct device *iommu_device = group->iommu_device;
if (WARN_ON(!iommu_device))
return -EINVAL;
if (iommu_dev_feature_enabled(iommu_device,
IOMMU_DEV_FEAT_AUX))
return iommu_aux_attach_device(domain->domain,
iommu_device);
else
return iommu_attach_device(domain->domain,
iommu_device);
} else {
return iommu_attach_group(domain->domain,
group->iommu_group);
}
}
The caller (vfio_iommu_type1_attach_group) has guaranteed that all mdevs
in an iommu group should be derived from a same physical device.
Any thoughts?
Alex
Best regards,
baolu
else
return iommu_attach_group(domain->domain, group->iommu_group);
}
@@ -1674,8 +1643,8 @@ static void vfio_iommu_detach_group(struct vfio_domain
*domain,
struct vfio_group *group)
{
if (group->mdev_group)
- iommu_group_for_each_dev(group->iommu_group, domain->domain,
-vfio_mdev_detach_domain);
+ iommu_aux_detach_group(domain->domain, group->iommu_group,
+ group->iommu_device);
else
iommu_detach_group(domain->domain, group->iommu_group);
}
@@ -2007,6 +1976,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
return 0;
}
+ group->iommu_device = iommu_device;
bus = iommu_
Re: [PATCH 11/15] memblock: reduce number of parameters in for_each_mem_range()
On 07/28/20 at 08:11am, Mike Rapoport wrote:
> From: Mike Rapoport
>
> Currently for_each_mem_range() iterator is the most generic way to traverse
> memblock regions. As such, it has 8 parameters and it is hardly convenient
> to users. Most users choose to utilize one of its wrappers and the only
> user that actually needs most of the parameters outside memblock is s390
> crash dump implementation.
>
> To avoid yet another naming for memblock iterators, rename the existing
> for_each_mem_range() to __for_each_mem_range() and add a new
> for_each_mem_range() wrapper with only index, start and end parameters.
>
> The new wrapper nicely fits into init_unavailable_mem() and will be used in
> upcoming changes to simplify memblock traversals.
>
> Signed-off-by: Mike Rapoport
> ---
> .clang-format | 1 +
> arch/arm64/kernel/machine_kexec_file.c | 6 ++
> arch/s390/kernel/crash_dump.c | 8
> include/linux/memblock.h | 18 ++
> mm/page_alloc.c| 3 +--
> 5 files changed, 22 insertions(+), 14 deletions(-)
Reviewed-by: Baoquan He
>
> diff --git a/.clang-format b/.clang-format
> index a0a96088c74f..52ededab25ce 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -205,6 +205,7 @@ ForEachMacros:
>- 'for_each_memblock_type'
>- 'for_each_memcg_cache_index'
>- 'for_each_mem_pfn_range'
> + - '__for_each_mem_range'
>- 'for_each_mem_range'
>- 'for_each_mem_range_rev'
>- 'for_each_migratetype_order'
> diff --git a/arch/arm64/kernel/machine_kexec_file.c
> b/arch/arm64/kernel/machine_kexec_file.c
> index 361a1143e09e..5b0e67b93cdc 100644
> --- a/arch/arm64/kernel/machine_kexec_file.c
> +++ b/arch/arm64/kernel/machine_kexec_file.c
> @@ -215,8 +215,7 @@ static int prepare_elf_headers(void **addr, unsigned long
> *sz)
> phys_addr_t start, end;
>
> nr_ranges = 1; /* for exclusion of crashkernel region */
> - for_each_mem_range(i, &memblock.memory, NULL, NUMA_NO_NODE,
> - MEMBLOCK_NONE, &start, &end, NULL)
> + for_each_mem_range(i, &start, &end)
> nr_ranges++;
>
> cmem = kmalloc(struct_size(cmem, ranges, nr_ranges), GFP_KERNEL);
> @@ -225,8 +224,7 @@ static int prepare_elf_headers(void **addr, unsigned long
> *sz)
>
> cmem->max_nr_ranges = nr_ranges;
> cmem->nr_ranges = 0;
> - for_each_mem_range(i, &memblock.memory, NULL, NUMA_NO_NODE,
> - MEMBLOCK_NONE, &start, &end, NULL) {
> + for_each_mem_range(i, &start, &end) {
> cmem->ranges[cmem->nr_ranges].start = start;
> cmem->ranges[cmem->nr_ranges].end = end - 1;
> cmem->nr_ranges++;
> diff --git a/arch/s390/kernel/crash_dump.c b/arch/s390/kernel/crash_dump.c
> index f96a5857bbfd..e28085c725ff 100644
> --- a/arch/s390/kernel/crash_dump.c
> +++ b/arch/s390/kernel/crash_dump.c
> @@ -549,8 +549,8 @@ static int get_mem_chunk_cnt(void)
> int cnt = 0;
> u64 idx;
>
> - for_each_mem_range(idx, &memblock.physmem, &oldmem_type, NUMA_NO_NODE,
> -MEMBLOCK_NONE, NULL, NULL, NULL)
> + __for_each_mem_range(idx, &memblock.physmem, &oldmem_type, NUMA_NO_NODE,
> + MEMBLOCK_NONE, NULL, NULL, NULL)
> cnt++;
> return cnt;
> }
> @@ -563,8 +563,8 @@ static void loads_init(Elf64_Phdr *phdr, u64 loads_offset)
> phys_addr_t start, end;
> u64 idx;
>
> - for_each_mem_range(idx, &memblock.physmem, &oldmem_type, NUMA_NO_NODE,
> -MEMBLOCK_NONE, &start, &end, NULL) {
> + __for_each_mem_range(idx, &memblock.physmem, &oldmem_type, NUMA_NO_NODE,
> + MEMBLOCK_NONE, &start, &end, NULL) {
> phdr->p_filesz = end - start;
> phdr->p_type = PT_LOAD;
> phdr->p_offset = start;
> diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> index e6a23b3db696..d70c2835e913 100644
> --- a/include/linux/memblock.h
> +++ b/include/linux/memblock.h
> @@ -142,7 +142,7 @@ void __next_reserved_mem_region(u64 *idx, phys_addr_t
> *out_start,
> void __memblock_free_late(phys_addr_t base, phys_addr_t size);
>
> /**
> - * for_each_mem_range - iterate through memblock areas from type_a and not
> + * __for_each_mem_range - iterate through memblock areas from type_a and not
> * included in type_b. Or just type_a if type_b is NULL.
> * @i: u64 used as loop variable
> * @type_a: ptr to memblock_type to iterate
> @@ -153,7 +153,7 @@ void __memblock_free_late(phys_addr_t base, phys_addr_t
> size);
> * @p_end: ptr to phys_addr_t for end address of the range, can be %NULL
> * @p_nid: ptr to int for nid of the range, can be %NULL
> */
> -#define for_each_mem_range(i, type_a, type_b, nid, flags,\
> +#define __for_each_mem_range(i, type_a, type_b, nid, flags, \
> p_
Re: [PATCH 10/15] memblock: make memblock_debug and related functionality private
On 07/28/20 at 08:11am, Mike Rapoport wrote:
> From: Mike Rapoport
>
> The only user of memblock_dbg() outside memblock was s390 setup code and it
> is converted to use pr_debug() instead.
> This allows to stop exposing memblock_debug and memblock_dbg() to the rest
> of the kernel.
>
> Signed-off-by: Mike Rapoport
> ---
> arch/s390/kernel/setup.c | 4 ++--
> include/linux/memblock.h | 12 +---
> mm/memblock.c| 13 +++--
> 3 files changed, 14 insertions(+), 15 deletions(-)
Nice clean up.
Reviewed-by: Baoquan He
>
> diff --git a/arch/s390/kernel/setup.c b/arch/s390/kernel/setup.c
> index 07aa15ba43b3..8b284cf6e199 100644
> --- a/arch/s390/kernel/setup.c
> +++ b/arch/s390/kernel/setup.c
> @@ -776,8 +776,8 @@ static void __init memblock_add_mem_detect_info(void)
> unsigned long start, end;
> int i;
>
> - memblock_dbg("physmem info source: %s (%hhd)\n",
> - get_mem_info_source(), mem_detect.info_source);
> + pr_debug("physmem info source: %s (%hhd)\n",
> + get_mem_info_source(), mem_detect.info_source);
> /* keep memblock lists close to the kernel */
> memblock_set_bottom_up(true);
> for_each_mem_detect_block(i, &start, &end) {
> diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> index 220b5f0dad42..e6a23b3db696 100644
> --- a/include/linux/memblock.h
> +++ b/include/linux/memblock.h
> @@ -90,7 +90,6 @@ struct memblock {
> };
>
> extern struct memblock memblock;
> -extern int memblock_debug;
>
> #ifndef CONFIG_ARCH_KEEP_MEMBLOCK
> #define __init_memblock __meminit
> @@ -102,9 +101,6 @@ void memblock_discard(void);
> static inline void memblock_discard(void) {}
> #endif
>
> -#define memblock_dbg(fmt, ...) \
> - if (memblock_debug) printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
> -
> phys_addr_t memblock_find_in_range(phys_addr_t start, phys_addr_t end,
> phys_addr_t size, phys_addr_t align);
> void memblock_allow_resize(void);
> @@ -456,13 +452,7 @@ bool memblock_is_region_memory(phys_addr_t base,
> phys_addr_t size);
> bool memblock_is_reserved(phys_addr_t addr);
> bool memblock_is_region_reserved(phys_addr_t base, phys_addr_t size);
>
> -extern void __memblock_dump_all(void);
> -
> -static inline void memblock_dump_all(void)
> -{
> - if (memblock_debug)
> - __memblock_dump_all();
> -}
> +void memblock_dump_all(void);
>
> /**
> * memblock_set_current_limit - Set the current allocation limit to allow
> diff --git a/mm/memblock.c b/mm/memblock.c
> index a5b9b3df81fc..824938849f6d 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -134,7 +134,10 @@ struct memblock memblock __initdata_memblock = {
>i < memblock_type->cnt;\
>i++, rgn = &memblock_type->regions[i])
>
> -int memblock_debug __initdata_memblock;
> +#define memblock_dbg(fmt, ...) \
> + if (memblock_debug) printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
> +
> +static int memblock_debug __initdata_memblock;
> static bool system_has_some_mirror __initdata_memblock = false;
> static int memblock_can_resize __initdata_memblock;
> static int memblock_memory_in_slab __initdata_memblock = 0;
> @@ -1919,7 +1922,7 @@ static void __init_memblock memblock_dump(struct
> memblock_type *type)
> }
> }
>
> -void __init_memblock __memblock_dump_all(void)
> +static void __init_memblock __memblock_dump_all(void)
> {
> pr_info("MEMBLOCK configuration:\n");
> pr_info(" memory size = %pa reserved size = %pa\n",
> @@ -1933,6 +1936,12 @@ void __init_memblock __memblock_dump_all(void)
> #endif
> }
>
> +void __init_memblock memblock_dump_all(void)
> +{
> + if (memblock_debug)
> + __memblock_dump_all();
> +}
> +
> void __init memblock_allow_resize(void)
> {
> memblock_can_resize = 1;
> --
> 2.26.2
>
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 09/15] memblock: make for_each_memblock_type() iterator private
On 07/28/20 at 08:11am, Mike Rapoport wrote:
> From: Mike Rapoport
>
> for_each_memblock_type() is not used outside mm/memblock.c, move it there
> from include/linux/memblock.h
>
> ---
> include/linux/memblock.h | 5 -
> mm/memblock.c| 5 +
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> index 017fae833d4a..220b5f0dad42 100644
> --- a/include/linux/memblock.h
> +++ b/include/linux/memblock.h
> @@ -532,11 +532,6 @@ static inline unsigned long
> memblock_region_reserved_end_pfn(const struct memblo
>region < (memblock.memblock_type.regions +
> memblock.memblock_type.cnt);\
>region++)
>
> -#define for_each_memblock_type(i, memblock_type, rgn)
> \
> - for (i = 0, rgn = &memblock_type->regions[0]; \
> - i < memblock_type->cnt;\
> - i++, rgn = &memblock_type->regions[i])
> -
> extern void *alloc_large_system_hash(const char *tablename,
>unsigned long bucketsize,
>unsigned long numentries,
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 39aceafc57f6..a5b9b3df81fc 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -129,6 +129,11 @@ struct memblock memblock __initdata_memblock = {
> .current_limit = MEMBLOCK_ALLOC_ANYWHERE,
> };
>
> +#define for_each_memblock_type(i, memblock_type, rgn)
> \
> + for (i = 0, rgn = &memblock_type->regions[0]; \
> + i < memblock_type->cnt;\
> + i++, rgn = &memblock_type->regions[i])
> +
Reviewed-by: Baoquan He
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/4] iommu: Check IOMMU_DEV_FEAT_AUX feature in aux api's
Hi Alex,
On 7/30/20 4:03 AM, Alex Williamson wrote:
On Tue, 14 Jul 2020 13:57:00 +0800
Lu Baolu wrote:
The iommu aux-domain api's work only when IOMMU_DEV_FEAT_AUX is enabled
for the device. Add this check to avoid misuse.
Shouldn't this really be the IOMMU driver's responsibility to test? If
nothing else, iommu_dev_feature_enabled() needs to get the iommu_ops
from dev->bus->iommu_ops, which is presumably the same iommu_ops we're
then calling from domain->ops to attach/detach the device, so it'd be
more efficient for the IOMMU driver to error on devices that don't
support aux. Thanks,
Fair enough. The vendor iommu driver always knows the status of aux-
domain support. So this check is duplicated. I will drop this patch.
Best regards,
baolu
Alex
Signed-off-by: Lu Baolu
---
drivers/iommu/iommu.c | 16 +---
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 1ed1e14a1f0c..e1fdd3531d65 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2725,11 +2725,13 @@ EXPORT_SYMBOL_GPL(iommu_dev_feature_enabled);
*/
int iommu_aux_attach_device(struct iommu_domain *domain, struct device *dev)
{
- int ret = -ENODEV;
+ int ret;
- if (domain->ops->aux_attach_dev)
- ret = domain->ops->aux_attach_dev(domain, dev);
+ if (!iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX) ||
+ !domain->ops->aux_attach_dev)
+ return -ENODEV;
+ ret = domain->ops->aux_attach_dev(domain, dev);
if (!ret)
trace_attach_device_to_domain(dev);
@@ -2748,12 +2750,12 @@ EXPORT_SYMBOL_GPL(iommu_aux_detach_device);
int iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev)
{
- int ret = -ENODEV;
+ if (!iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX) ||
+ !domain->ops->aux_get_pasid)
+ return -ENODEV;
- if (domain->ops->aux_get_pasid)
- ret = domain->ops->aux_get_pasid(domain, dev);
+ return domain->ops->aux_get_pasid(domain, dev);
- return ret;
}
EXPORT_SYMBOL_GPL(iommu_aux_get_pasid);
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v7 7/7] iommu/vt-d: Check UAPI data processed by IOMMU core
IOMMU generic layer already does sanity checks UAPI data for version match and argsz range under generic information. Remove the redundant version check from VT-d driver and check for vendor specific data size. Signed-off-by: Jacob Pan --- drivers/iommu/intel/iommu.c | 3 +-- drivers/iommu/intel/svm.c | 7 +-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 021f62078f52..7e03cca31a0e 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -5391,8 +5391,7 @@ intel_iommu_sva_invalidate(struct iommu_domain *domain, struct device *dev, int ret = 0; u64 size = 0; - if (!inv_info || !dmar_domain || - inv_info->version != IOMMU_CACHE_INVALIDATE_INFO_VERSION_1) + if (!inv_info || !dmar_domain) return -EINVAL; if (!dev || !dev_is_pci(dev)) diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c index 713b3a218483..55ea11e9c0f5 100644 --- a/drivers/iommu/intel/svm.c +++ b/drivers/iommu/intel/svm.c @@ -240,8 +240,11 @@ int intel_svm_bind_gpasid(struct iommu_domain *domain, struct device *dev, if (WARN_ON(!iommu) || !data) return -EINVAL; - if (data->version != IOMMU_GPASID_BIND_VERSION_1 || - data->format != IOMMU_PASID_FORMAT_INTEL_VTD) + if (data->format != IOMMU_PASID_FORMAT_INTEL_VTD) + return -EINVAL; + + /* IOMMU core ensures argsz is more than the start of the union */ + if (data->argsz < offsetofend(struct iommu_gpasid_bind_data, vendor.vtd)) return -EINVAL; if (!dev_is_pci(dev)) -- 2.7.4 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v7 1/7] docs: IOMMU user API
IOMMU UAPI is newly introduced to support communications between guest virtual IOMMU and host IOMMU. There has been lots of discussions on how it should work with VFIO UAPI and userspace in general. This document is intended to clarify the UAPI design and usage. The mechanics of how future extensions should be achieved are also covered in this documentation. Signed-off-by: Liu Yi L Signed-off-by: Jacob Pan --- Documentation/userspace-api/iommu.rst | 212 ++ MAINTAINERS | 1 + 2 files changed, 213 insertions(+) create mode 100644 Documentation/userspace-api/iommu.rst diff --git a/Documentation/userspace-api/iommu.rst b/Documentation/userspace-api/iommu.rst new file mode 100644 index ..b2f5b3256d85 --- /dev/null +++ b/Documentation/userspace-api/iommu.rst @@ -0,0 +1,212 @@ +.. SPDX-License-Identifier: GPL-2.0 +.. iommu: + += +IOMMU Userspace API += + +IOMMU UAPI is used for virtualization cases where communications are +needed between physical and virtual IOMMU drivers. For baremetal +usage, the IOMMU is a system device which does not need to communicate +with user space directly. + +The primary use cases are guest Shared Virtual Address (SVA) and +guest IO virtual address (IOVA), wherin the vIOMMU implementation +relies on the physical IOMMU and for this reason requires interactions +with the host driver. + +.. contents:: :local: + +Functionalities +=== +Communications of user and kernel involve both directions. The +supported user-kernel APIs are as follows: + +1. Alloc/Free PASID +2. Bind/unbind guest PASID (e.g. Intel VT-d) +3. Bind/unbind guest PASID table (e.g. ARM SMMU) +4. Invalidate IOMMU caches requested by guests +5. Report errors to the guest and serve page requests + +Requirements + +The IOMMU UAPIs are generic and extensible to meet the following +requirements: + +1. Emulated and para-virtualised vIOMMUs +2. Multiple vendors (Intel VT-d, ARM SMMU, etc.) +3. Extensions to the UAPI shall not break existing user space + +Interfaces +== +Although the data structures defined in IOMMU UAPI are self-contained, +there is no user API functions introduced. Instead, IOMMU UAPI is +designed to work with existing user driver frameworks such as VFIO. + +Extension Rules & Precautions +- +When IOMMU UAPI gets extended, the data structures can *only* be +modified in two ways: + +1. Adding new fields by re-purposing the padding[] field. No size change. +2. Adding new union members at the end. May increase the structure sizes. + +No new fields can be added *after* the variable sized union in that it +will break backward compatibility when offset moves. A new flag must +be introduced whenever a change affects the structure using either +method. The IOMMU driver processes the data based on flags which +ensures backward compatibility. + +Version field is only reserved for the unlikely event of UAPI upgrade +at its entirety. + +It's *always* the caller's responsibility to indicate the size of the +structure passed by setting argsz appropriately. +Though at the same time, argsz is user provided data which is not +trusted. The argsz field allows the user app to indicate how much data +it is providing, it's still the kernel's responsibility to validate +whether it's correct and sufficient for the requested operation. + +Compatibility Checking +-- +When IOMMU UAPI extension results in some structure size increase, +IOMMU UAPI code shall handle the following cases: + +1. User and kernel has exact size match +2. An older user with older kernel header (smaller UAPI size) running on a + newer kernel (larger UAPI size) +3. A newer user with newer kernel header (larger UAPI size) running + on an older kernel. +4. A malicious/misbehaving user pass illegal/invalid size but within + range. The data may contain garbage. + +Feature Checking + +While launching a guest with vIOMMU, it is important to ensure that host +can support the UAPI data structures to be used for vIOMMU-pIOMMU +communications. Without upfront compatibility checking, the future errors +can lead to catastrophic failures for the users. + +User applications such as QEMU are expected to import kernel UAPI +headers. Backward compatibility is supported per feature flags. +For example, an older QEMU (with older kernel header) can run on newer +kernel. Newer QEMU (with new kernel header) may refuse to initialize +on an older kernel if new feature flags are not supported by older +kernel. Simply recompiling existing code with newer kernel header should +not be an issue in that only existing flags are used. + +IOMMU vendor driver should report the below features to IOMMU UAPI +consumers (e.g. via VFIO). + +1. IOMMU_NESTING_FEAT_SYSWIDE_PASID +2. IOMMU_NESTING_FEAT_BIND_PGTBL +3. IOMMU_NESTING_FEAT_BIND_PASID_TABLE +4. IOMM
[PATCH v7 5/7] iommu/uapi: Rename uapi functions
User APIs such as iommu_sva_unbind_gpasid() may also be used by the
kernel. Since we introduced user pointer to the UAPI functions,
in-kernel callers cannot share the same APIs. In-kernel callers are also
trusted, there is no need to validate the data.
This patch renames all UAPI functions with iommu_uapi_ prefix such that
is clear to the intended callers.
Suggested-by: Alex Williamson
Signed-off-by: Jacob Pan
---
drivers/iommu/iommu.c | 18 +-
include/linux/iommu.h | 31 ---
2 files changed, 25 insertions(+), 24 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index b6858adc4f17..3a913ce94a3d 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1950,35 +1950,35 @@ int iommu_attach_device(struct iommu_domain *domain,
struct device *dev)
}
EXPORT_SYMBOL_GPL(iommu_attach_device);
-int iommu_cache_invalidate(struct iommu_domain *domain, struct device *dev,
- struct iommu_cache_invalidate_info *inv_info)
+int iommu_uapi_cache_invalidate(struct iommu_domain *domain, struct device
*dev,
+ struct iommu_cache_invalidate_info *inv_info)
{
if (unlikely(!domain->ops->cache_invalidate))
return -ENODEV;
return domain->ops->cache_invalidate(domain, dev, inv_info);
}
-EXPORT_SYMBOL_GPL(iommu_cache_invalidate);
+EXPORT_SYMBOL_GPL(iommu_uapi_cache_invalidate);
-int iommu_sva_bind_gpasid(struct iommu_domain *domain,
- struct device *dev, struct iommu_gpasid_bind_data
*data)
+int iommu_uapi_sva_bind_gpasid(struct iommu_domain *domain,
+ struct device *dev, struct
iommu_gpasid_bind_data *data)
{
if (unlikely(!domain->ops->sva_bind_gpasid))
return -ENODEV;
return domain->ops->sva_bind_gpasid(domain, dev, data);
}
-EXPORT_SYMBOL_GPL(iommu_sva_bind_gpasid);
+EXPORT_SYMBOL_GPL(iommu_uapi_sva_bind_gpasid);
-int iommu_sva_unbind_gpasid(struct iommu_domain *domain, struct device *dev,
-ioasid_t pasid)
+int iommu_uapi_sva_unbind_gpasid(struct iommu_domain *domain, struct device
*dev,
+ioasid_t pasid)
{
if (unlikely(!domain->ops->sva_unbind_gpasid))
return -ENODEV;
return domain->ops->sva_unbind_gpasid(dev, pasid);
}
-EXPORT_SYMBOL_GPL(iommu_sva_unbind_gpasid);
+EXPORT_SYMBOL_GPL(iommu_uapi_sva_unbind_gpasid);
static void __iommu_detach_device(struct iommu_domain *domain,
struct device *dev)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 5f0b7859d2eb..2dcc1a33f6dc 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -430,13 +430,13 @@ extern int iommu_attach_device(struct iommu_domain
*domain,
struct device *dev);
extern void iommu_detach_device(struct iommu_domain *domain,
struct device *dev);
-extern int iommu_cache_invalidate(struct iommu_domain *domain,
- struct device *dev,
- struct iommu_cache_invalidate_info *inv_info);
-extern int iommu_sva_bind_gpasid(struct iommu_domain *domain,
- struct device *dev, struct iommu_gpasid_bind_data *data);
-extern int iommu_sva_unbind_gpasid(struct iommu_domain *domain,
- struct device *dev, ioasid_t pasid);
+extern int iommu_uapi_cache_invalidate(struct iommu_domain *domain,
+ struct device *dev,
+ struct iommu_cache_invalidate_info
*inv_info);
+extern int iommu_uapi_sva_bind_gpasid(struct iommu_domain *domain,
+ struct device *dev, struct
iommu_gpasid_bind_data *data);
+extern int iommu_uapi_sva_unbind_gpasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid);
extern struct iommu_domain *iommu_get_domain_for_dev(struct device *dev);
extern struct iommu_domain *iommu_get_dma_domain(struct device *dev);
extern int iommu_map(struct iommu_domain *domain, unsigned long iova,
@@ -1054,21 +1054,22 @@ static inline int iommu_sva_get_pasid(struct iommu_sva
*handle)
return IOMMU_PASID_INVALID;
}
-static inline int
-iommu_cache_invalidate(struct iommu_domain *domain,
- struct device *dev,
- struct iommu_cache_invalidate_info *inv_info)
+static inline int iommu_uapi_cache_invalidate(struct iommu_domain *domain,
+ struct device *dev,
+ struct
iommu_cache_invalidate_info *inv_info)
{
return -ENODEV;
}
-static inline int iommu_sva_bind_gpasid(struct iommu_domain *domain,
- struct device *dev, struct
iommu_gpasid_bind_data *data)
+
+static inline int iomm
[PATCH v7 4/7] iommu/uapi: Use named union for user data
IOMMU UAPI data size is filled by the user space which must be validated
by the kernel. To ensure backward compatibility, user data can only be
extended by either re-purpose padding bytes or extend the variable sized
union at the end. No size change is allowed before the union. Therefore,
the minimum size is the offset of the union.
To use offsetof() on the union, we must make it named.
Link: https://lore.kernel.org/linux-iommu/20200611145518.0c281...@x1.home/
Signed-off-by: Jacob Pan
Reviewed-by: Lu Baolu
Reviewed-by: Eric Auger
---
drivers/iommu/intel/iommu.c | 22 +++---
drivers/iommu/intel/svm.c | 2 +-
include/uapi/linux/iommu.h | 4 ++--
3 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 92c7ad66e64c..021f62078f52 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -5417,8 +5417,8 @@ intel_iommu_sva_invalidate(struct iommu_domain *domain,
struct device *dev,
/* Size is only valid in address selective invalidation */
if (inv_info->granularity == IOMMU_INV_GRANU_ADDR)
- size = to_vtd_size(inv_info->addr_info.granule_size,
- inv_info->addr_info.nb_granules);
+ size = to_vtd_size(inv_info->granu.addr_info.granule_size,
+ inv_info->granu.addr_info.nb_granules);
for_each_set_bit(cache_type,
(unsigned long *)&inv_info->cache,
@@ -5439,20 +5439,20 @@ intel_iommu_sva_invalidate(struct iommu_domain *domain,
struct device *dev,
* granularity.
*/
if (inv_info->granularity == IOMMU_INV_GRANU_PASID &&
- (inv_info->pasid_info.flags & IOMMU_INV_PASID_FLAGS_PASID))
- pasid = inv_info->pasid_info.pasid;
+ (inv_info->granu.pasid_info.flags &
IOMMU_INV_PASID_FLAGS_PASID))
+ pasid = inv_info->granu.pasid_info.pasid;
else if (inv_info->granularity == IOMMU_INV_GRANU_ADDR &&
-(inv_info->addr_info.flags &
IOMMU_INV_ADDR_FLAGS_PASID))
- pasid = inv_info->addr_info.pasid;
+(inv_info->granu.addr_info.flags &
IOMMU_INV_ADDR_FLAGS_PASID))
+ pasid = inv_info->granu.addr_info.pasid;
switch (BIT(cache_type)) {
case IOMMU_CACHE_INV_TYPE_IOTLB:
/* HW will ignore LSB bits based on address mask */
if (inv_info->granularity == IOMMU_INV_GRANU_ADDR &&
size &&
- (inv_info->addr_info.addr & ((BIT(VTD_PAGE_SHIFT +
size)) - 1))) {
+ (inv_info->granu.addr_info.addr &
((BIT(VTD_PAGE_SHIFT + size)) - 1))) {
pr_err_ratelimited("User address not aligned,
0x%llx, size order %llu\n",
- inv_info->addr_info.addr, size);
+ inv_info->granu.addr_info.addr,
size);
}
/*
@@ -5460,9 +5460,9 @@ intel_iommu_sva_invalidate(struct iommu_domain *domain,
struct device *dev,
* We use npages = -1 to indicate that.
*/
qi_flush_piotlb(iommu, did, pasid,
- mm_to_dma_pfn(inv_info->addr_info.addr),
+
mm_to_dma_pfn(inv_info->granu.addr_info.addr),
(granu == QI_GRAN_NONG_PASID) ? -1 : 1
<< size,
- inv_info->addr_info.flags &
IOMMU_INV_ADDR_FLAGS_LEAF);
+ inv_info->granu.addr_info.flags &
IOMMU_INV_ADDR_FLAGS_LEAF);
if (!info->ats_enabled)
break;
@@ -5485,7 +5485,7 @@ intel_iommu_sva_invalidate(struct iommu_domain *domain,
struct device *dev,
size = 64 - VTD_PAGE_SHIFT;
addr = 0;
} else if (inv_info->granularity ==
IOMMU_INV_GRANU_ADDR)
- addr = inv_info->addr_info.addr;
+ addr = inv_info->granu.addr_info.addr;
if (info->ats_enabled)
qi_flush_dev_iotlb_pasid(iommu, sid,
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index d386853121a2..713b3a218483 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -338,7 +338,7 @@ int intel_svm_bind_gpasid(struct iommu_domain *domain,
struct device *dev,
spin_lock(&iommu->lock);
ret = intel_pasid_setup_nested(iommu, dev,
(pgd_t *)(uintptr_t)data->gpgd,
-
[PATCH v7 2/7] iommu/uapi: Add argsz for user filled data
As IOMMU UAPI gets extended, user data size may increase. To support
backward compatibiliy, this patch introduces a size field to each UAPI
data structures. It is *always* the responsibility for the user to fill in
the correct size. Padding fields are adjusted to ensure 8 byte alignment.
Specific scenarios for user data handling are documented in:
Documentation/userspace-api/iommu.rst
Signed-off-by: Liu Yi L
Signed-off-by: Jacob Pan
---
include/uapi/linux/iommu.h | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
index e907b7091a46..d5e9014f690e 100644
--- a/include/uapi/linux/iommu.h
+++ b/include/uapi/linux/iommu.h
@@ -135,6 +135,7 @@ enum iommu_page_response_code {
/**
* struct iommu_page_response - Generic page response information
+ * @argsz: User filled size of this data
* @version: API version of this structure
* @flags: encodes whether the corresponding fields are valid
* (IOMMU_FAULT_PAGE_RESPONSE_* values)
@@ -143,6 +144,7 @@ enum iommu_page_response_code {
* @code: response code from &enum iommu_page_response_code
*/
struct iommu_page_response {
+ __u32 argsz;
#define IOMMU_PAGE_RESP_VERSION_1 1
__u32 version;
#define IOMMU_PAGE_RESP_PASID_VALID(1 << 0)
@@ -218,6 +220,7 @@ struct iommu_inv_pasid_info {
/**
* struct iommu_cache_invalidate_info - First level/stage invalidation
* information
+ * @argsz: User filled size of this data
* @version: API version of this structure
* @cache: bitfield that allows to select which caches to invalidate
* @granularity: defines the lowest granularity used for the invalidation:
@@ -246,6 +249,7 @@ struct iommu_inv_pasid_info {
* must support the used granularity.
*/
struct iommu_cache_invalidate_info {
+ __u32 argsz;
#define IOMMU_CACHE_INVALIDATE_INFO_VERSION_1 1
__u32 version;
/* IOMMU paging structure cache */
@@ -255,7 +259,7 @@ struct iommu_cache_invalidate_info {
#define IOMMU_CACHE_INV_TYPE_NR(3)
__u8cache;
__u8granularity;
- __u8padding[2];
+ __u8padding[6];
union {
struct iommu_inv_pasid_info pasid_info;
struct iommu_inv_addr_info addr_info;
@@ -292,6 +296,7 @@ struct iommu_gpasid_bind_data_vtd {
/**
* struct iommu_gpasid_bind_data - Information about device and guest PASID
binding
+ * @argsz: User filled size of this data
* @version: Version of this data structure
* @format:PASID table entry format
* @flags: Additional information on guest bind request
@@ -309,17 +314,18 @@ struct iommu_gpasid_bind_data_vtd {
* PASID to host PASID based on this bind data.
*/
struct iommu_gpasid_bind_data {
+ __u32 argsz;
#define IOMMU_GPASID_BIND_VERSION_11
__u32 version;
#define IOMMU_PASID_FORMAT_INTEL_VTD 1
__u32 format;
+ __u32 addr_width;
#define IOMMU_SVA_GPASID_VAL (1 << 0) /* guest PASID valid */
__u64 flags;
__u64 gpgd;
__u64 hpasid;
__u64 gpasid;
- __u32 addr_width;
- __u8 padding[12];
+ __u8 padding[8];
/* Vendor specific data */
union {
struct iommu_gpasid_bind_data_vtd vtd;
--
2.7.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v7 6/7] iommu/uapi: Handle data and argsz filled by users
IOMMU user APIs are responsible for processing user data. This patch
changes the interface such that user pointers can be passed into IOMMU
code directly. Separate kernel APIs without user pointers are introduced
for in-kernel users of the UAPI functionality.
IOMMU UAPI data has a user filled argsz field which indicates the data
length of the structure. User data is not trusted, argsz must be
validated based on the current kernel data size, mandatory data size,
and feature flags.
User data may also be extended, resulting in possible argsz increase.
Backward compatibility is ensured based on size and flags (or
the functional equivalent fields) checking.
This patch adds sanity checks in the IOMMU layer. In addition to argsz,
reserved/unused fields in padding, flags, and version are also checked.
Details are documented in Documentation/userspace-api/iommu.rst
Signed-off-by: Liu Yi L
Signed-off-by: Jacob Pan
---
drivers/iommu/iommu.c | 201 --
include/linux/iommu.h | 28 ---
2 files changed, 212 insertions(+), 17 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 3a913ce94a3d..1ee55c4b3a3a 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1950,33 +1950,218 @@ int iommu_attach_device(struct iommu_domain *domain,
struct device *dev)
}
EXPORT_SYMBOL_GPL(iommu_attach_device);
+/*
+ * Check flags and other user provided data for valid combinations. We also
+ * make sure no reserved fields or unused flags are set. This is to ensure
+ * not breaking userspace in the future when these fields or flags are used.
+ */
+static int iommu_check_cache_invl_data(struct iommu_cache_invalidate_info
*info)
+{
+ u32 mask;
+ int i;
+
+ if (info->version != IOMMU_CACHE_INVALIDATE_INFO_VERSION_1)
+ return -EINVAL;
+
+ mask = (1 << IOMMU_CACHE_INV_TYPE_NR) - 1;
+ if (info->cache & ~mask)
+ return -EINVAL;
+
+ if (info->granularity >= IOMMU_INV_GRANU_NR)
+ return -EINVAL;
+
+ switch (info->granularity) {
+ case IOMMU_INV_GRANU_ADDR:
+ if (info->cache & IOMMU_CACHE_INV_TYPE_PASID)
+ return -EINVAL;
+
+ mask = IOMMU_INV_ADDR_FLAGS_PASID |
+ IOMMU_INV_ADDR_FLAGS_ARCHID |
+ IOMMU_INV_ADDR_FLAGS_LEAF;
+
+ if (info->granu.addr_info.flags & ~mask)
+ return -EINVAL;
+ break;
+ case IOMMU_INV_GRANU_PASID:
+ mask = IOMMU_INV_PASID_FLAGS_PASID |
+ IOMMU_INV_PASID_FLAGS_ARCHID;
+ if (info->granu.pasid_info.flags & ~mask)
+ return -EINVAL;
+
+ break;
+ case IOMMU_INV_GRANU_DOMAIN:
+ if (info->cache & IOMMU_CACHE_INV_TYPE_DEV_IOTLB)
+ return -EINVAL;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ /* Check reserved padding fields */
+ for (i = 0; i < sizeof(info->padding); i++) {
+ if (info->padding[i])
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
int iommu_uapi_cache_invalidate(struct iommu_domain *domain, struct device
*dev,
- struct iommu_cache_invalidate_info *inv_info)
+ void __user *uinfo)
{
+ struct iommu_cache_invalidate_info inv_info = { 0 };
+ u32 minsz;
+ int ret = 0;
+
if (unlikely(!domain->ops->cache_invalidate))
return -ENODEV;
- return domain->ops->cache_invalidate(domain, dev, inv_info);
+ /*
+* No new spaces can be added before the variable sized union, the
+* minimum size is the offset to the union.
+*/
+ minsz = offsetof(struct iommu_cache_invalidate_info, granu);
+
+ /* Copy minsz from user to get flags and argsz */
+ if (copy_from_user(&inv_info, uinfo, minsz))
+ return -EFAULT;
+
+ /* Fields before variable size union is mandatory */
+ if (inv_info.argsz < minsz)
+ return -EINVAL;
+
+ /* PASID and address granu require additional info beyond minsz */
+ if (inv_info.argsz == minsz &&
+ ((inv_info.granularity == IOMMU_INV_GRANU_PASID) ||
+ (inv_info.granularity == IOMMU_INV_GRANU_ADDR)))
+ return -EINVAL;
+
+ if (inv_info.granularity == IOMMU_INV_GRANU_PASID &&
+ inv_info.argsz < offsetofend(struct iommu_cache_invalidate_info,
granu.pasid_info))
+ return -EINVAL;
+
+ if (inv_info.granularity == IOMMU_INV_GRANU_ADDR &&
+ inv_info.argsz < offsetofend(struct iommu_cache_invalidate_info,
granu.addr_info))
+ return -EINVAL;
+
+ /*
+* User might be using a newer UAPI header which has a larger data
+* size, we shall support the existing flag
[PATCH v7 3/7] iommu/uapi: Introduce enum type for PASID data format
There can be multiple vendor-specific PASID data formats used in UAPI
structures. This patch adds enum type with a last entry which makes
range checking much easier.
Suggested-by: Alex Williamson
Signed-off-by: Jacob Pan
---
include/uapi/linux/iommu.h | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
index d5e9014f690e..abf4455a3495 100644
--- a/include/uapi/linux/iommu.h
+++ b/include/uapi/linux/iommu.h
@@ -294,11 +294,16 @@ struct iommu_gpasid_bind_data_vtd {
IOMMU_SVA_VTD_GPASID_PCD | \
IOMMU_SVA_VTD_GPASID_PWT)
+enum iommu_pasid_data_format {
+ IOMMU_PASID_FORMAT_INTEL_VTD = 1,
+ IOMMU_PASID_FORMAT_LAST,
+};
+
/**
* struct iommu_gpasid_bind_data - Information about device and guest PASID
binding
* @argsz: User filled size of this data
* @version: Version of this data structure
- * @format:PASID table entry format
+ * @format:PASID table entry format of enum iommu_pasid_data_format type
* @flags: Additional information on guest bind request
* @gpgd: Guest page directory base of the guest mm to bind
* @hpasid:Process address space ID used for the guest mm in host IOMMU
@@ -317,7 +322,6 @@ struct iommu_gpasid_bind_data {
__u32 argsz;
#define IOMMU_GPASID_BIND_VERSION_11
__u32 version;
-#define IOMMU_PASID_FORMAT_INTEL_VTD 1
__u32 format;
__u32 addr_width;
#define IOMMU_SVA_GPASID_VAL (1 << 0) /* guest PASID valid */
--
2.7.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v7 0/7] IOMMU user API enhancement
IOMMU user API header was introduced to support nested DMA translation and related fault handling. The current UAPI data structures consist of three areas that cover the interactions between host kernel and guest: - fault handling - cache invalidation - bind guest page tables, i.e. guest PASID Future extensions are likely to support more architectures and vIOMMU features. In the previous discussion, using user-filled data size and feature flags is made a preferred approach over a unified version number. https://lkml.org/lkml/2020/1/29/45 In addition to introduce argsz field to data structures, this patchset is also trying to document the UAPI design, usage, and extension rules. VT-d driver changes to utilize the new argsz field is included, VFIO usage is to follow. This set is available at: https://github.com/jacobpan/linux.git vsva_v5.8_uapi_v7 Thanks, Jacob Changeog: v7 - Added PASID data format enum for range checking - Tidy up based on reviews from Alex W. - Removed doc section for vIOMMU fault handling v6 - Renamed all UAPI functions with iommu_uapi_ prefix - Replaced argsz maxsz checking with flag specific size checks - Documentation improvements based on suggestions by Eric Auger Replaced example code with a pointer to the actual code - Added more checks for illegal flags combinations - Added doc file to MAINTAINERS v5 - Addjusted paddings in UAPI data to be 8 byte aligned - Do not clobber argsz in IOMMU core before passing on to vendor driver - Removed pr_warn_ for invalid UAPI data check, just return -EINVAL - Clarified VFIO responsibility in UAPI data handling - Use iommu_uapi prefix to differentiate APIs has in-kernel caller - Added comment for unchecked flags of invalidation granularity - Added example in doc to show vendor data checking v4 - Added checks of UAPI data for reserved fields, version, and flags. - Removed version check from vendor driver (vt-d) - Relaxed argsz check to match the UAPI struct size instead of variable union size - Updated documentation v3: - Rewrote backward compatibility rule to support existing code re-compiled with newer kernel UAPI header that runs on older kernel. Based on review comment from Alex W. https://lore.kernel.org/linux-iommu/20200611094741.6d118...@w520.home/ - Take user pointer directly in UAPI functions. Perform argsz check and copy_from_user() in IOMMU driver. Eliminate the need for VFIO or other upper layer to parse IOMMU data. - Create wrapper function for in-kernel users of UAPI functions v2: - Removed unified API version and helper - Introduced argsz for each UAPI data - Introduced UAPI doc Jacob Pan (7): docs: IOMMU user API iommu/uapi: Add argsz for user filled data iommu/uapi: Introduce enum type for PASID data format iommu/uapi: Use named union for user data iommu/uapi: Rename uapi functions iommu/uapi: Handle data and argsz filled by users iommu/vt-d: Check UAPI data processed by IOMMU core Documentation/userspace-api/iommu.rst | 212 ++ MAINTAINERS | 1 + drivers/iommu/intel/iommu.c | 25 ++-- drivers/iommu/intel/svm.c | 9 +- drivers/iommu/iommu.c | 205 ++-- include/linux/iommu.h | 35 -- include/uapi/linux/iommu.h| 24 ++-- 7 files changed, 466 insertions(+), 45 deletions(-) create mode 100644 Documentation/userspace-api/iommu.rst -- 2.7.4 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev()
> From: Alex Williamson
> Sent: Thursday, July 30, 2020 4:25 AM
>
> On Tue, 14 Jul 2020 13:57:02 +0800
> Lu Baolu wrote:
>
> > The device driver needs an API to get its aux-domain. A typical usage
> > scenario is:
> >
> > unsigned long pasid;
> > struct iommu_domain *domain;
> > struct device *dev = mdev_dev(mdev);
> > struct device *iommu_device = vfio_mdev_get_iommu_device(dev);
> >
> > domain = iommu_aux_get_domain_for_dev(dev);
> > if (!domain)
> > return -ENODEV;
> >
> > pasid = iommu_aux_get_pasid(domain, iommu_device);
> > if (pasid <= 0)
> > return -EINVAL;
> >
> > /* Program the device context */
> >
> >
> > This adds an API for such use case.
> >
> > Suggested-by: Alex Williamson
> > Signed-off-by: Lu Baolu
> > ---
> > drivers/iommu/iommu.c | 18 ++
> > include/linux/iommu.h | 7 +++
> > 2 files changed, 25 insertions(+)
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index cad5a19ebf22..434bf42b6b9b 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -2817,6 +2817,24 @@ void iommu_aux_detach_group(struct
> iommu_domain *domain,
> > }
> > EXPORT_SYMBOL_GPL(iommu_aux_detach_group);
> >
> > +struct iommu_domain *iommu_aux_get_domain_for_dev(struct device
> *dev)
> > +{
> > + struct iommu_domain *domain = NULL;
> > + struct iommu_group *group;
> > +
> > + group = iommu_group_get(dev);
> > + if (!group)
> > + return NULL;
> > +
> > + if (group->aux_domain_attached)
> > + domain = group->domain;
>
> Why wouldn't the aux domain flag be on the domain itself rather than
> the group? Then if we wanted sanity checking in patch 1/ we'd only
> need to test the flag on the object we're provided.
>
> If we had such a flag, we could create an iommu_domain_is_aux()
> function and then simply use iommu_get_domain_for_dev() and test that
> it's an aux domain in the example use case. It seems like that would
IOMMU layer manages domains per parent device. Here given a
dev (of mdev), we need a way to find its associated domain under its
parent device. And we cannot simply use iommu_get_domain_for_dev
on the parent device of the mdev, as it will give us the primary domain
of parent device.
Thanks
Kevin
> resolve the jump from a domain to an aux-domain just as well as adding
> this separate iommu_aux_get_domain_for_dev() interface. The is_aux
> test might also be useful in other cases too. Thanks,
>
> Alex
>
> > +
> > + iommu_group_put(group);
> > +
> > + return domain;
> > +}
> > +EXPORT_SYMBOL_GPL(iommu_aux_get_domain_for_dev);
> > +
> > /**
> > * iommu_sva_bind_device() - Bind a process address space to a device
> > * @dev: the device
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index 9506551139ab..cda6cef7579e 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -639,6 +639,7 @@ int iommu_aux_attach_group(struct
> iommu_domain *domain,
> >struct iommu_group *group, struct device *dev);
> > void iommu_aux_detach_group(struct iommu_domain *domain,
> >struct iommu_group *group, struct device *dev);
> > +struct iommu_domain *iommu_aux_get_domain_for_dev(struct device
> *dev);
> >
> > struct iommu_sva *iommu_sva_bind_device(struct device *dev,
> > struct mm_struct *mm,
> > @@ -1040,6 +1041,12 @@ iommu_aux_detach_group(struct
> iommu_domain *domain,
> > {
> > }
> >
> > +static inline struct iommu_domain *
> > +iommu_aux_get_domain_for_dev(struct device *dev)
> > +{
> > + return NULL;
> > +}
> > +
> > static inline struct iommu_sva *
> > iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void
> *drvdata)
> > {
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()
> From: Alex Williamson
> Sent: Thursday, July 30, 2020 4:04 AM
>
> On Thu, 16 Jul 2020 09:07:46 +0800
> Lu Baolu wrote:
>
> > Hi Jacob,
> >
> > On 7/16/20 12:01 AM, Jacob Pan wrote:
> > > On Wed, 15 Jul 2020 08:47:36 +0800
> > > Lu Baolu wrote:
> > >
> > >> Hi Jacob,
> > >>
> > >> On 7/15/20 12:39 AM, Jacob Pan wrote:
> > >>> On Tue, 14 Jul 2020 13:57:01 +0800
> > >>> Lu Baolu wrote:
> > >>>
> > This adds two new aux-domain APIs for a use case like vfio/mdev
> > where sub-devices derived from an aux-domain capable device are
> > created and put in an iommu_group.
> >
> > /**
> > * iommu_aux_attach_group - attach an aux-domain to an
> iommu_group
> > which
> > * contains sub-devices (for example
> > mdevs) derived
> > * from @dev.
> > * @domain: an aux-domain;
> > * @group: an iommu_group which contains sub-devices derived
> from
> > @dev;
> > * @dev:the physical device which supports
> IOMMU_DEV_FEAT_AUX.
> > *
> > * Returns 0 on success, or an error value.
> > */
> > int iommu_aux_attach_group(struct iommu_domain *domain,
> > struct iommu_group *group,
> > struct device *dev)
> >
> > /**
> > * iommu_aux_detach_group - detach an aux-domain from an
> > iommu_group *
> > * @domain: an aux-domain;
> > * @group: an iommu_group which contains sub-devices derived
> from
> > @dev;
> > * @dev:the physical device which supports
> IOMMU_DEV_FEAT_AUX.
> > *
> > * @domain must have been attached to @group via
> > iommu_aux_attach_group(). */
> > void iommu_aux_detach_group(struct iommu_domain *domain,
> > struct iommu_group *group,
> > struct device *dev)
> >
> > It also adds a flag in the iommu_group data structure to identify
> > an iommu_group with aux-domain attached from those normal ones.
> >
> > Signed-off-by: Lu Baolu
> > ---
> > drivers/iommu/iommu.c | 58
> > +++
> include/linux/iommu.h |
> > 17 + 2 files changed, 75 insertions(+)
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index e1fdd3531d65..cad5a19ebf22 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -45,6 +45,7 @@ struct iommu_group {
> > struct iommu_domain *default_domain;
> > struct iommu_domain *domain;
> > struct list_head entry;
> > + unsigned int aux_domain_attached:1;
> > };
> >
> > struct group_device {
> > @@ -2759,6 +2760,63 @@ int iommu_aux_get_pasid(struct
> iommu_domain
> > *domain, struct device *dev) }
> > EXPORT_SYMBOL_GPL(iommu_aux_get_pasid);
> >
> > +/**
> > + * iommu_aux_attach_group - attach an aux-domain to an
> iommu_group
> > which
> > + * contains sub-devices (for example
> > mdevs) derived
> > + * from @dev.
> > + * @domain: an aux-domain;
> > + * @group: an iommu_group which contains sub-devices derived
> from
> > @dev;
> > + * @dev:the physical device which supports
> IOMMU_DEV_FEAT_AUX.
> > + *
> > + * Returns 0 on success, or an error value.
> > + */
> > +int iommu_aux_attach_group(struct iommu_domain *domain,
> > + struct iommu_group *group, struct
> > device *dev) +{
> > + int ret = -EBUSY;
> > +
> > + mutex_lock(&group->mutex);
> > + if (group->domain)
> > + goto out_unlock;
> > +
> > >>> Perhaps I missed something but are we assuming only one mdev per
> > >>> mdev group? That seems to change the logic where vfio does:
> > >>> iommu_group_for_each_dev()
> > >>> iommu_aux_attach_device()
> > >>>
> > >>
> > >> It has been changed in PATCH 4/4:
> > >>
> > >> static int vfio_iommu_attach_group(struct vfio_domain *domain,
> > >> struct vfio_group *group)
> > >> {
> > >> if (group->mdev_group)
> > >> return iommu_aux_attach_group(domain->domain,
> > >> group->iommu_group,
> > >> group->iommu_device);
> > >> else
> > >> return iommu_attach_group(domain->domain,
> > >> group->iommu_group);
> > >> }
> > >>
> > >> So, for both normal domain and aux-domain, we use the same concept:
> > >> attach a domain to a group.
> > >>
> > > I get that, but don't you have to attach all the devices within the
> >
> > This iommu_group includes only mediated devices derived from
Re: [PATCH v6 5/6] iommu/uapi: Handle data and argsz filled by users
On Tue, 28 Jul 2020 13:19:44 -0600
Alex Williamson wrote:
> On Thu, 23 Jul 2020 10:25:39 -0700
> Jacob Pan wrote:
>
> > IOMMU user APIs are responsible for processing user data. This patch
> > changes the interface such that user pointers can be passed into
> > IOMMU code directly. Separate kernel APIs without user pointers are
> > introduced for in-kernel users of the UAPI functionality.
> >
> > IOMMU UAPI data has a user filled argsz field which indicates the
> > data length of the structure. User data is not trusted, argsz must
> > be validated based on the current kernel data size, mandatory data
> > size, and feature flags.
> >
> > User data may also be extended, resulting in possible argsz
> > increase. Backward compatibility is ensured based on size and flags
> > (or the functional equivalent fields) checking.
> >
> > This patch adds sanity checks in the IOMMU layer. In addition to
> > argsz, reserved/unused fields in padding, flags, and version are
> > also checked. Details are documented in
> > Documentation/userspace-api/iommu.rst
> >
> > Signed-off-by: Liu Yi L
> > Signed-off-by: Jacob Pan
> > ---
> > drivers/iommu/iommu.c | 202
> > --
> > include/linux/iommu.h | 28 --- 2 files changed, 213
> > insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 3a913ce94a3d..1ce2a61058c6 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -1950,33 +1950,219 @@ int iommu_attach_device(struct
> > iommu_domain *domain, struct device *dev) }
> > EXPORT_SYMBOL_GPL(iommu_attach_device);
> >
> > +/*
> > + * Check flags and other user provided data for valid
> > combinations. We also
> > + * make sure no reserved fields or unused flags are set. This is
> > to ensure
> > + * not breaking userspace in the future when these fields or flags
> > are used.
> > + */
> > +static int iommu_check_cache_invl_data(struct
> > iommu_cache_invalidate_info *info) +{
> > + u32 mask;
> > + int i;
> > +
> > + if (info->version != IOMMU_CACHE_INVALIDATE_INFO_VERSION_1)
> > + return -EINVAL;
> > +
> > + mask = (1 << IOMMU_CACHE_INV_TYPE_NR) - 1;
> > + if (info->cache & ~mask)
> > + return -EINVAL;
> > +
> > + if (info->granularity >= IOMMU_INV_GRANU_NR)
> > + return -EINVAL;
> > +
> > + switch (info->granularity) {
> > + case IOMMU_INV_GRANU_ADDR:
> > + if (info->cache & IOMMU_CACHE_INV_TYPE_PASID)
> > + return -EINVAL;
> > +
> > + mask = IOMMU_INV_ADDR_FLAGS_PASID |
> > + IOMMU_INV_ADDR_FLAGS_ARCHID |
> > + IOMMU_INV_ADDR_FLAGS_LEAF;
> > +
> > + if (info->granu.addr_info.flags & ~mask)
> > + return -EINVAL;
> > + break;
> > + case IOMMU_INV_GRANU_PASID:
> > + mask = IOMMU_INV_PASID_FLAGS_PASID |
> > + IOMMU_INV_PASID_FLAGS_ARCHID;
> > + if (info->granu.pasid_info.flags & ~mask)
> > + return -EINVAL;
> > +
> > + break;
> > + case IOMMU_INV_GRANU_DOMAIN:
> > + if (info->cache & IOMMU_CACHE_INV_TYPE_DEV_IOTLB)
> > + return -EINVAL;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + /* Check reserved padding fields */
> > + for (i = 0; i < 6; i++) {
>
> We could use sizeof(info->padding) here to avoid future issues
> relative to the number of padding bytes.
>
yes, much better.
> > + if (info->padding[i])
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > int iommu_uapi_cache_invalidate(struct iommu_domain *domain,
> > struct device *dev,
> > - struct iommu_cache_invalidate_info
> > *inv_info)
> > + void __user *uinfo)
> > {
> > + struct iommu_cache_invalidate_info inv_info = { 0 };
> > + u32 minsz;
> > + int ret = 0;
> > +
> > if (unlikely(!domain->ops->cache_invalidate))
> > return -ENODEV;
> >
> > - return domain->ops->cache_invalidate(domain, dev,
> > inv_info);
> > + /*
> > +* No new spaces can be added before the variable sized
> > union, the
> > +* minimum size is the offset to the union.
> > +*/
> > + minsz = offsetof(struct iommu_cache_invalidate_info,
> > granu); +
> > + /* Copy minsz from user to get flags and argsz */
> > + if (copy_from_user(&inv_info, uinfo, minsz))
> > + return -EFAULT;
> > +
> > + /* Fields before variable size union is mandatory */
> > + if (inv_info.argsz < minsz)
> > + return -EINVAL;
> > +
> > + /* PASID and address granu require additional info beyond
> > minsz */
> > + if (inv_info.argsz == minsz &&
> > + ((inv_info.granularity == IOMMU_INV_GRANU_PASID) ||
> > + (inv_info.granularity ==
> > IOMMU_INV_GRANU_ADDR)))
> > + return -EINVAL;
> > +
> > + if
Re: [PATCH v6 1/6] docs: IOMMU user API
On Wed, 29 Jul 2020 01:18:04 + "Tian, Kevin" wrote: > > From: Alex Williamson > > Sent: Wednesday, July 29, 2020 3:20 AM > > > [...] > > > + > > > +For example, IOTLB invalidations should always succeed. There is > > > no +architectural way to report back to the vIOMMU if the UAPI > > > data is +incompatible. If that happens, in order to guarantee > > > IOMMU iosolation, > > > > isolation > > > > > +we have to resort to not giving completion status in vIOMMU. > > > This may +result in VM hang. > > > + > > > +For this reason the following IOMMU UAPIs cannot fail without > > > +catastrophic effect: > > > + > > > +1. Free PASID > > > +2. Unbind guest PASID > > > +3. Unbind guest PASID table (SMMU) > > > +4. Cache invalidate > > > > I'm not entirely sure what we're trying to assert here. Clearly > > cache invalidation can fail and patch 5/6 goes on to add over a > > dozen checks of the user provided data that return an -errno. Any > > user ioctl can fail if the user botches the parameters. So are we > > just trying to explain the feature checking that should allow the > > user to know supported combinations and if they adhere to them, > > these should not fail? It's not quite worded to that effect. > > Thanks, > > I guess the above wording is messed by what a UAPI should > behave and whether the vIOMMU reports associated errors. > UAPI can always fail, as you pointed out. vIOMMU may not > have a matching error code though, e.g. on Intel VT-d there is no > error reporting mechanism for cache invalidation. However, > it is not wise to assert UAPI behavior according to vIOMMU > definition. An error is an error. vIOMMU should just react to > UAPI errors according to its architecture definition (e.g. ignore, > forward to guest, hang, etc.). From this matter I feel above > section could better be removed. > Yes, I agreed, the scope is not drawn clearly. This section is kind of the relic of a previous version where responsibility of feature checking lies with IOMMU UAPI instead of VFIO. How about just briefly mention that upfront feature checking is encouraged to avoid complex and catastrophic error at runtime? I will remove the rest. > Thanks > Kevin [Jacob Pan] ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs
On Tue, 14 Jul 2020 13:57:03 +0800
Lu Baolu wrote:
> Replace iommu_aux_at(de)tach_device() with iommu_aux_at(de)tach_group().
> It also saves the IOMMU_DEV_FEAT_AUX-capable physcail device in the
> vfio_group data structure so that it could be reused in other places.
>
> Signed-off-by: Lu Baolu
> ---
> drivers/vfio/vfio_iommu_type1.c | 44 ++---
> 1 file changed, 7 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 5e556ac9102a..f8812e68de77 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -100,6 +100,7 @@ struct vfio_dma {
> struct vfio_group {
> struct iommu_group *iommu_group;
> struct list_headnext;
> + struct device *iommu_device;
> boolmdev_group; /* An mdev group */
> boolpinned_page_dirty_scope;
> };
> @@ -1627,45 +1628,13 @@ static struct device
> *vfio_mdev_get_iommu_device(struct device *dev)
> return NULL;
> }
>
> -static int vfio_mdev_attach_domain(struct device *dev, void *data)
> -{
> - struct iommu_domain *domain = data;
> - struct device *iommu_device;
> -
> - iommu_device = vfio_mdev_get_iommu_device(dev);
> - if (iommu_device) {
> - if (iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX))
> - return iommu_aux_attach_device(domain, iommu_device);
> - else
> - return iommu_attach_device(domain, iommu_device);
> - }
> -
> - return -EINVAL;
> -}
> -
> -static int vfio_mdev_detach_domain(struct device *dev, void *data)
> -{
> - struct iommu_domain *domain = data;
> - struct device *iommu_device;
> -
> - iommu_device = vfio_mdev_get_iommu_device(dev);
> - if (iommu_device) {
> - if (iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX))
> - iommu_aux_detach_device(domain, iommu_device);
> - else
> - iommu_detach_device(domain, iommu_device);
> - }
> -
> - return 0;
> -}
> -
> static int vfio_iommu_attach_group(struct vfio_domain *domain,
> struct vfio_group *group)
> {
> if (group->mdev_group)
> - return iommu_group_for_each_dev(group->iommu_group,
> - domain->domain,
> - vfio_mdev_attach_domain);
> + return iommu_aux_attach_group(domain->domain,
> + group->iommu_group,
> + group->iommu_device);
No, we previously iterated all devices in the group and used the aux
interface only when we have an iommu_device supporting aux. If we
simply assume an mdev group only uses an aux domain we break existing
users, ex. SR-IOV VF backed mdevs. Thanks,
Alex
> else
> return iommu_attach_group(domain->domain, group->iommu_group);
> }
> @@ -1674,8 +1643,8 @@ static void vfio_iommu_detach_group(struct vfio_domain
> *domain,
> struct vfio_group *group)
> {
> if (group->mdev_group)
> - iommu_group_for_each_dev(group->iommu_group, domain->domain,
> - vfio_mdev_detach_domain);
> + iommu_aux_detach_group(domain->domain, group->iommu_group,
> +group->iommu_device);
> else
> iommu_detach_group(domain->domain, group->iommu_group);
> }
> @@ -2007,6 +1976,7 @@ static int vfio_iommu_type1_attach_group(void
> *iommu_data,
> return 0;
> }
>
> + group->iommu_device = iommu_device;
> bus = iommu_device->bus;
> }
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH V5 0/3] iommu: Add support to change default domain of an iommu group
Hi Joerg, > -Original Message- > From: Joerg Roedel > Sent: Wednesday, July 29, 2020 5:37 AM > To: Prakhya, Sai Praneeth > Cc: iommu@lists.linux-foundation.org; Christoph Hellwig ; Raj, > Ashok ; Will Deacon ; Lu Baolu > ; Mehta, Sohil ; Robin > Murphy ; Jacob Pan > Subject: Re: [PATCH V5 0/3] iommu: Add support to change default domain of > an iommu group > > Hi, > > On Fri, Jul 24, 2020 at 12:51:57PM -0700, Sai Praneeth Prakhya wrote: > > Tested only for intel_iommu/vt-d. Would appreciate if someone could > > test on AMD and ARM based machines. > > This looks good to me now, but I want to test it some more on other hardware > and look up the implications of the probe_finalize calls when changing the > default domain. Makes sense to me. > Since I am technically on vacation this week and next and don't have the > resources around to do proper testing, I defer this for the next round. > Please re-send this patch-set when the merge window closes. Sure! Makes sense. I will resend the patch-set after this merge window closes. Regards, Sai ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev()
On Tue, 14 Jul 2020 13:57:02 +0800
Lu Baolu wrote:
> The device driver needs an API to get its aux-domain. A typical usage
> scenario is:
>
> unsigned long pasid;
> struct iommu_domain *domain;
> struct device *dev = mdev_dev(mdev);
> struct device *iommu_device = vfio_mdev_get_iommu_device(dev);
>
> domain = iommu_aux_get_domain_for_dev(dev);
> if (!domain)
> return -ENODEV;
>
> pasid = iommu_aux_get_pasid(domain, iommu_device);
> if (pasid <= 0)
> return -EINVAL;
>
> /* Program the device context */
>
>
> This adds an API for such use case.
>
> Suggested-by: Alex Williamson
> Signed-off-by: Lu Baolu
> ---
> drivers/iommu/iommu.c | 18 ++
> include/linux/iommu.h | 7 +++
> 2 files changed, 25 insertions(+)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index cad5a19ebf22..434bf42b6b9b 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2817,6 +2817,24 @@ void iommu_aux_detach_group(struct iommu_domain
> *domain,
> }
> EXPORT_SYMBOL_GPL(iommu_aux_detach_group);
>
> +struct iommu_domain *iommu_aux_get_domain_for_dev(struct device *dev)
> +{
> + struct iommu_domain *domain = NULL;
> + struct iommu_group *group;
> +
> + group = iommu_group_get(dev);
> + if (!group)
> + return NULL;
> +
> + if (group->aux_domain_attached)
> + domain = group->domain;
Why wouldn't the aux domain flag be on the domain itself rather than
the group? Then if we wanted sanity checking in patch 1/ we'd only
need to test the flag on the object we're provided.
If we had such a flag, we could create an iommu_domain_is_aux()
function and then simply use iommu_get_domain_for_dev() and test that
it's an aux domain in the example use case. It seems like that would
resolve the jump from a domain to an aux-domain just as well as adding
this separate iommu_aux_get_domain_for_dev() interface. The is_aux
test might also be useful in other cases too. Thanks,
Alex
> +
> + iommu_group_put(group);
> +
> + return domain;
> +}
> +EXPORT_SYMBOL_GPL(iommu_aux_get_domain_for_dev);
> +
> /**
> * iommu_sva_bind_device() - Bind a process address space to a device
> * @dev: the device
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 9506551139ab..cda6cef7579e 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -639,6 +639,7 @@ int iommu_aux_attach_group(struct iommu_domain *domain,
> struct iommu_group *group, struct device *dev);
> void iommu_aux_detach_group(struct iommu_domain *domain,
> struct iommu_group *group, struct device *dev);
> +struct iommu_domain *iommu_aux_get_domain_for_dev(struct device *dev);
>
> struct iommu_sva *iommu_sva_bind_device(struct device *dev,
> struct mm_struct *mm,
> @@ -1040,6 +1041,12 @@ iommu_aux_detach_group(struct iommu_domain *domain,
> {
> }
>
> +static inline struct iommu_domain *
> +iommu_aux_get_domain_for_dev(struct device *dev)
> +{
> + return NULL;
> +}
> +
> static inline struct iommu_sva *
> iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void
> *drvdata)
> {
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/4] iommu: Check IOMMU_DEV_FEAT_AUX feature in aux api's
On Tue, 14 Jul 2020 13:57:00 +0800
Lu Baolu wrote:
> The iommu aux-domain api's work only when IOMMU_DEV_FEAT_AUX is enabled
> for the device. Add this check to avoid misuse.
Shouldn't this really be the IOMMU driver's responsibility to test? If
nothing else, iommu_dev_feature_enabled() needs to get the iommu_ops
from dev->bus->iommu_ops, which is presumably the same iommu_ops we're
then calling from domain->ops to attach/detach the device, so it'd be
more efficient for the IOMMU driver to error on devices that don't
support aux. Thanks,
Alex
> Signed-off-by: Lu Baolu
> ---
> drivers/iommu/iommu.c | 16 +---
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 1ed1e14a1f0c..e1fdd3531d65 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2725,11 +2725,13 @@ EXPORT_SYMBOL_GPL(iommu_dev_feature_enabled);
> */
> int iommu_aux_attach_device(struct iommu_domain *domain, struct device *dev)
> {
> - int ret = -ENODEV;
> + int ret;
>
> - if (domain->ops->aux_attach_dev)
> - ret = domain->ops->aux_attach_dev(domain, dev);
> + if (!iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX) ||
> + !domain->ops->aux_attach_dev)
> + return -ENODEV;
>
> + ret = domain->ops->aux_attach_dev(domain, dev);
> if (!ret)
> trace_attach_device_to_domain(dev);
>
> @@ -2748,12 +2750,12 @@ EXPORT_SYMBOL_GPL(iommu_aux_detach_device);
>
> int iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev)
> {
> - int ret = -ENODEV;
> + if (!iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX) ||
> + !domain->ops->aux_get_pasid)
> + return -ENODEV;
>
> - if (domain->ops->aux_get_pasid)
> - ret = domain->ops->aux_get_pasid(domain, dev);
> + return domain->ops->aux_get_pasid(domain, dev);
>
> - return ret;
> }
> EXPORT_SYMBOL_GPL(iommu_aux_get_pasid);
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()
On Thu, 16 Jul 2020 09:07:46 +0800
Lu Baolu wrote:
> Hi Jacob,
>
> On 7/16/20 12:01 AM, Jacob Pan wrote:
> > On Wed, 15 Jul 2020 08:47:36 +0800
> > Lu Baolu wrote:
> >
> >> Hi Jacob,
> >>
> >> On 7/15/20 12:39 AM, Jacob Pan wrote:
> >>> On Tue, 14 Jul 2020 13:57:01 +0800
> >>> Lu Baolu wrote:
> >>>
> This adds two new aux-domain APIs for a use case like vfio/mdev
> where sub-devices derived from an aux-domain capable device are
> created and put in an iommu_group.
>
> /**
> * iommu_aux_attach_group - attach an aux-domain to an iommu_group
> which
> * contains sub-devices (for example
> mdevs) derived
> * from @dev.
> * @domain: an aux-domain;
> * @group: an iommu_group which contains sub-devices derived from
> @dev;
> * @dev:the physical device which supports IOMMU_DEV_FEAT_AUX.
> *
> * Returns 0 on success, or an error value.
> */
> int iommu_aux_attach_group(struct iommu_domain *domain,
> struct iommu_group *group,
> struct device *dev)
>
> /**
> * iommu_aux_detach_group - detach an aux-domain from an
> iommu_group *
> * @domain: an aux-domain;
> * @group: an iommu_group which contains sub-devices derived from
> @dev;
> * @dev:the physical device which supports IOMMU_DEV_FEAT_AUX.
> *
> * @domain must have been attached to @group via
> iommu_aux_attach_group(). */
> void iommu_aux_detach_group(struct iommu_domain *domain,
> struct iommu_group *group,
> struct device *dev)
>
> It also adds a flag in the iommu_group data structure to identify
> an iommu_group with aux-domain attached from those normal ones.
>
> Signed-off-by: Lu Baolu
> ---
> drivers/iommu/iommu.c | 58
> +++ include/linux/iommu.h |
> 17 + 2 files changed, 75 insertions(+)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index e1fdd3531d65..cad5a19ebf22 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -45,6 +45,7 @@ struct iommu_group {
> struct iommu_domain *default_domain;
> struct iommu_domain *domain;
> struct list_head entry;
> +unsigned int aux_domain_attached:1;
> };
>
> struct group_device {
> @@ -2759,6 +2760,63 @@ int iommu_aux_get_pasid(struct iommu_domain
> *domain, struct device *dev) }
> EXPORT_SYMBOL_GPL(iommu_aux_get_pasid);
>
> +/**
> + * iommu_aux_attach_group - attach an aux-domain to an iommu_group
> which
> + * contains sub-devices (for example
> mdevs) derived
> + * from @dev.
> + * @domain: an aux-domain;
> + * @group: an iommu_group which contains sub-devices derived from
> @dev;
> + * @dev:the physical device which supports IOMMU_DEV_FEAT_AUX.
> + *
> + * Returns 0 on success, or an error value.
> + */
> +int iommu_aux_attach_group(struct iommu_domain *domain,
> + struct iommu_group *group, struct
> device *dev) +{
> +int ret = -EBUSY;
> +
> +mutex_lock(&group->mutex);
> +if (group->domain)
> +goto out_unlock;
> +
> >>> Perhaps I missed something but are we assuming only one mdev per
> >>> mdev group? That seems to change the logic where vfio does:
> >>> iommu_group_for_each_dev()
> >>> iommu_aux_attach_device()
> >>>
> >>
> >> It has been changed in PATCH 4/4:
> >>
> >> static int vfio_iommu_attach_group(struct vfio_domain *domain,
> >> struct vfio_group *group)
> >> {
> >> if (group->mdev_group)
> >> return iommu_aux_attach_group(domain->domain,
> >> group->iommu_group,
> >> group->iommu_device);
> >> else
> >> return iommu_attach_group(domain->domain,
> >> group->iommu_group);
> >> }
> >>
> >> So, for both normal domain and aux-domain, we use the same concept:
> >> attach a domain to a group.
> >>
> > I get that, but don't you have to attach all the devices within the
>
> This iommu_group includes only mediated devices derived from an
> IOMMU_DEV_FEAT_AUX-capable device. Different from iommu_attach_group(),
> iommu_aux_attach_group() doesn't need to attach the domain to each
> device in group, instead it only needs to attach the domain to the
> physical device where the mdev's were created from.
>
> > group? Here
Re: [PATCH v9 08/12] device core: Introduce DMA range map, supplanting dma_pfn_offset
On Wed, Jul 29, 2020 at 12:19 AM Christoph Hellwig wrote: > > On Tue, Jul 28, 2020 at 02:24:51PM -0400, Jim Quinlan wrote: > > I started using devm_kcalloc() but at least two reviewers convinced me > > to just use kcalloc(). In addition, when I was using devm_kcalloc() > > it was awkward because 'dev' is not available to this function. > > > > It comes down to whether unbind/binding the device N times is actually > > a reasonable usage. As for my experience I've seen two cases: (1) my > > overnight "bind/unbind the PCIe RC driver" script, and we have a > > customer who does an unbind/bind as a hail mary to bring back life to > > their dead EP device. If the latter case happens repeatedly, there > > are bigger problems. > > We can't just leak the allocations. Do you have a pointer to the > arguments against managed resources? I'm generally not a huge fan > of the managed resources, but for a case like this they actually seem > useful. If we don't use the managed resources we'll at leat need > to explicitly free the resources when freeing the device. The lifetime for devm_kcalloc may not be what we want here. devm allocs are freed on probe fail or remove, not on freeing the device (there is a just in case free there too though). Rob ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 08/12] device core: Introduce DMA range map, supplanting dma_pfn_offset
On Wed, Jul 29, 2020 at 2:19 AM Christoph Hellwig wrote: > > On Tue, Jul 28, 2020 at 02:24:51PM -0400, Jim Quinlan wrote: > > I started using devm_kcalloc() but at least two reviewers convinced me > > to just use kcalloc(). In addition, when I was using devm_kcalloc() > > it was awkward because 'dev' is not available to this function. > > > > It comes down to whether unbind/binding the device N times is actually > > a reasonable usage. As for my experience I've seen two cases: (1) my > > overnight "bind/unbind the PCIe RC driver" script, and we have a > > customer who does an unbind/bind as a hail mary to bring back life to > > their dead EP device. If the latter case happens repeatedly, there > > are bigger problems. > > We can't just leak the allocations. Do you have a pointer to the > arguments against managed resources? I'm generally not a huge fan > of the managed resources, but for a case like this they actually seem > useful. If we don't use the managed resources we'll at leat need > to explicitly free the resources when freeing the device. Actually, there were no arguments for using an unmanaged kcalloc, just comments [1], [2]. When it was rightly suggested to have 'dev' dropped from of_dma_range() [3], I submitted v6 to accomplish this. But now of_dma_range() would call kcalloc(), and then of_dma_configure() was required to "dup" the result, requiring a devm_kcalloc(), memcpy(), and kfree(). This was awkward, so considering [1], [2], I dropped the devm_kcalloc() and submitted v7. So I can easily revert to the awkward code of v6. But I'm hoping you have a more elegant solution :-) Thanks, Jim [1] [v6, Andy Shevchenko] > + /* We want the offset map to be device-managed, so alloc & copy > */ > + dev->dma_range_map = devm_kcalloc(dev, num_ranges + 1, > sizeof(*r), > + GFP_KERNEL); The question is how many times per device lifetime this can be called? [2] [v2, Andy Shevchenko] > + r = devm_kzalloc(dev, r_size, GFP_KERNEL); devm_?! Looking at r_size it should be rather kcalloc(). [3] [v3, Nicolas Saenz Julienne] > I agree with you. The reason I needed a "struct device *" in the call is > because I wanted to make sure the memory that is alloc'd belongs to the > device that needs it. If I do a regular kzalloc(), this memory will become > a leak once someone starts unbinding/binding their device. Also, in all > uses of of_dma_rtange() -- there is only one -- a dev is required as one > can't attach an offset map to NULL. > > I do see that there are a number of functions in drivers/of/*.c that > take 'struct device *dev' as an argument so there is precedent for > something like this. Regardless, I need an owner to the memory I > alloc(). I understand the need for dev to be around, devm_*() is key. But also it's important to keep the functions on purpose. And if of_dma_get_range() starts setting ranges it calls, for the very least, for a function rename. Although I'd rather split the parsing and setting of ranges as mentioned earlier. That said, I get that's a more drastic move. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V5 0/3] iommu: Add support to change default domain of an iommu group
Hi, On Fri, Jul 24, 2020 at 12:51:57PM -0700, Sai Praneeth Prakhya wrote: > Tested only for intel_iommu/vt-d. Would appreciate if someone could test on > AMD > and ARM based machines. This looks good to me now, but I want to test it some more on other hardware and look up the implications of the probe_finalize calls when changing the default domain. Since I am technically on vacation this week and next and don't have the resources around to do proper testing, I defer this for the next round. Please re-send this patch-set when the merge window closes. Thanks, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: dma-pool fixes
On Wed, 29 Jul 2020 at 16:15, Nicolas Saenz Julienne
wrote:
>
> On Tue, 2020-07-28 at 17:30 +0200, Christoph Hellwig wrote:
> > On Tue, Jul 28, 2020 at 06:18:41PM +0530, Amit Pundir wrote:
> > > > Oh well, this leaves me confused again. It looks like your setup
> > > > really needs a CMA in zone normal for the dma or dma32 pool.
> > >
> > > Anything I should look up in the downstream kernel/dts?
> >
> > I don't have a good idea right now. Nicolas, can you think of something
> > else?
>
> To summarise, the device is:
> - Using the dma-direct code path.
> - Requesting ZONE_DMA memory to then fail when provided memory falls in
>ZONE_DMA. Actually, the only acceptable memory comes from CMA, which is
>located topmost of the 4GB boundary.
>
> My wild guess is that we may be abusing an iommu identity mapping setup by
> firmware.
>
> That said, what would be helpful to me is to find out the troublesome device.
> Amit, could you try adding this patch along with Christoph's modified series
> (so the board boots). Ultimately DMA atomic allocations are not that common,
> so
> we should get only a few hits:
Hi, still not hitting dma_alloc_from_pool().
I hit the following direct alloc path only once, at starting:
dma_alloc_coherent ()
-> dma_alloc_attrs()
-> dma_is_direct() -> dma_direct_alloc()
-> dma_direct_alloc_pages()
-> dma_should_alloc_from_pool() #returns FALSE from here
After that I'm hitting following iommu dma alloc path all the time:
dma_alloc_coherent()
-> dma_alloc_attrs()
-> (ops->alloc) -> iommu_dma_alloc()
-> iommu_dma_alloc_remap() #always returns from here
So dma_alloc_from_pool() is not getting called at all in either of the
above cases.
>
> diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
> index 83fda1039493..de93fce6d5d2 100644
> --- a/kernel/dma/pool.c
> +++ b/kernel/dma/pool.c
> @@ -276,8 +276,11 @@ struct page *dma_alloc_from_pool(struct device *dev,
> size_t size,
> while ((pool = dma_guess_pool(pool, gfp))) {
> page = __dma_alloc_from_pool(dev, size, pool, cpu_addr,
> phys_addr_ok);
> - if (page)
> + if (page) {
> + dev_err(dev, "%s: phys addr 0x%llx, size %lu,
> dev->coherent_dma_mask 0x%llx, dev->bus_dma_limit 0x%llx\n",
> + __func__, (phys_addr_t)*cpu_addr, size,
> dev->coherent_dma_mask, dev->bus_dma_limit);
> return page;
> + }
> }
>
> WARN(1, "Failed to get suitable pool for %s\n", dev_name(dev));
>
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [GIT PULL] iommu/arm-smmu: Move driver files into their own subdir
On Mon, Jul 27, 2020 at 01:05:11PM +0100, Will Deacon wrote: > As requested in [1], here is a second Arm SMMU pull request for 5.9, moving > the driver files into their own subdirectory to avoid cluttering > drivers/iommu/. Pulled, thanks a lot, Will. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/2] iommu: Move AMD and Intel Kconfig + Makefile bits into their directories
On Mon, Jul 27, 2020 at 03:47:58PM -0700, Jerry Snitselaar wrote: > Looks like I forgot to cc you on this cover letter for v2. > Does this work for you now? Got it, applied now, thanks Jerry. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 05/15] h8300, nds32, openrisc: simplify detection of memory extents
On Tue, Jul 28, 2020 at 08:11:43AM +0300, Mike Rapoport wrote:
> From: Mike Rapoport
>
> Instead of traversing memblock.memory regions to find memory_start and
> memory_end, simply query memblock_{start,end}_of_DRAM().
>
> Signed-off-by: Mike Rapoport
> ---
> arch/h8300/kernel/setup.c| 8 +++-
> arch/nds32/kernel/setup.c| 8 ++--
> arch/openrisc/kernel/setup.c | 9 ++---
> 3 files changed, 7 insertions(+), 18 deletions(-)
Hi Mike,
For the openrisc part:
Acked-by: Stafford Horne
> --- a/arch/openrisc/kernel/setup.c
> +++ b/arch/openrisc/kernel/setup.c
> @@ -48,17 +48,12 @@ static void __init setup_memory(void)
> unsigned long ram_start_pfn;
> unsigned long ram_end_pfn;
> phys_addr_t memory_start, memory_end;
> - struct memblock_region *region;
>
> memory_end = memory_start = 0;
>
> /* Find main memory where is the kernel, we assume its the only one */
> - for_each_memblock(memory, region) {
> - memory_start = region->base;
> - memory_end = region->base + region->size;
> - printk(KERN_INFO "%s: Memory: 0x%x-0x%x\n", __func__,
> -memory_start, memory_end);
> - }
> + memory_start = memblock_start_of_DRAM();
> + memory_end = memblock_end_of_DRAM();
>
> if (!memory_end) {
> panic("No memory!");
> --
> 2.26.2
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v4 1/2] dma-direct: provide the ability to reserve per-numa CMA
> -Original Message-
> From: Christoph Hellwig [mailto:h...@lst.de]
> Sent: Wednesday, July 29, 2020 12:23 AM
> To: Song Bao Hua (Barry Song)
> Cc: Christoph Hellwig ; m.szyprow...@samsung.com;
> robin.mur...@arm.com; w...@kernel.org; ganapatrao.kulka...@cavium.com;
> catalin.mari...@arm.com; iommu@lists.linux-foundation.org; Linuxarm
> ; linux-arm-ker...@lists.infradead.org;
> linux-ker...@vger.kernel.org; Zengtao (B) ;
> huangdaode ; Jonathan Cameron
> ; Nicolas Saenz Julienne
> ; Steve Capper ; Andrew
> Morton ; Mike Rapoport
> Subject: Re: [PATCH v4 1/2] dma-direct: provide the ability to reserve
> per-numa CMA
>
> On Tue, Jul 28, 2020 at 12:19:03PM +, Song Bao Hua (Barry Song) wrote:
> > I am sorry I haven't got your point yet. Do you mean something like the
> below?
> >
> > arch/arm64/Kconfig:
> > config CMDLINE
> > string "Default kernel command string"
> > - default ""
> > + default "pernuma_cma=16M"
> > help
> > Provide a set of default command-line options at build time by
> > entering them here. As a minimum, you should specify the the
> > root device (e.g. root=/dev/nfs).
>
> Yes.
>
> > A background of the current code is that Linux distributions can usually use
> arch/arm64/configs/defconfig
> > directly to build kernel. cmdline can be easily ignored during the
> > generation
> of Linux distributions.
>
> I've not actually heard of a distro shipping defconfig yet..
>
> >
> > > if a way to expose this in the device tree might be useful, but people
> > > more familiar with the device tree and the arm code will have to chime
> > > in on that.
> >
> > Not sure if it is an useful user case as we are using ACPI but not device
> > tree
> since it is an ARM64
> > server with NUMA.
>
> Well, than maybe ACPI experts need to chime in on this.
>
> > > This seems to have lost the dma_contiguous_default_area NULL check.
> >
> > cma_alloc() is doing the check by returning NULL if cma is NULL.
> >
> > struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
> >bool no_warn)
> > {
> > ...
> > if (!cma || !cma->count)
> > return NULL;
> > }
> >
> > But I agree here the code can check before calling cma_alloc_aligned.
>
> Oh, indeed. Please split the removal of the NULL check in to a prep
> patch then.
Do you mean removing the NULL check in cma_alloc()? If so, it seems lot of
places
need to be changed:
struct page *dma_alloc_from_contiguous(struct device *dev, size_t count,
unsigned int align, bool no_warn)
{
if (align > CONFIG_CMA_ALIGNMENT)
align = CONFIG_CMA_ALIGNMENT;
+ code to check dev_get_cma_area(dev) is not NULL
return cma_alloc(dev_get_cma_area(dev), count, align, no_warn);
}
bool dma_release_from_contiguous(struct device *dev, struct page *pages,
int count)
{
+ code to check dev_get_cma_area(dev) is not NULL
return cma_release(dev_get_cma_area(dev), pages, count);
}
bool cma_release(struct cma *cma, const struct page *pages, unsigned int count)
{
unsigned long pfn;
+ do we need to remove this !cma too if we remove it in cma_alloc()?
if (!cma || !pages)
return false;
...
}
And some other places where cma_alloc() and cma_release() are called:
arch/powerpc/kvm/book3s_hv_builtin.c
drivers/dma-buf/heaps/cma_heap.c
drivers/s390/char/vmcp.c
drivers/staging/android/ion/ion_cma_heap.c
mm/hugetlb.c
it seems many code were written with the assumption that cma_alloc/release will
check if cma is null so they don't check it before calling cma_alloc().
And I am not sure if kernel robot will report error like pointer reference
before checking
it if !cma is removed in cma_alloc().
Thanks
Barry
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: dma-pool fixes
On Tue, 2020-07-28 at 17:30 +0200, Christoph Hellwig wrote:
> On Tue, Jul 28, 2020 at 06:18:41PM +0530, Amit Pundir wrote:
> > > Oh well, this leaves me confused again. It looks like your setup
> > > really needs a CMA in zone normal for the dma or dma32 pool.
> >
> > Anything I should look up in the downstream kernel/dts?
>
> I don't have a good idea right now. Nicolas, can you think of something
> else?
To summarise, the device is:
- Using the dma-direct code path.
- Requesting ZONE_DMA memory to then fail when provided memory falls in
ZONE_DMA. Actually, the only acceptable memory comes from CMA, which is
located topmost of the 4GB boundary.
My wild guess is that we may be abusing an iommu identity mapping setup by
firmware.
That said, what would be helpful to me is to find out the troublesome device.
Amit, could you try adding this patch along with Christoph's modified series
(so the board boots). Ultimately DMA atomic allocations are not that common, so
we should get only a few hits:
diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
index 83fda1039493..de93fce6d5d2 100644
--- a/kernel/dma/pool.c
+++ b/kernel/dma/pool.c
@@ -276,8 +276,11 @@ struct page *dma_alloc_from_pool(struct device *dev,
size_t size,
while ((pool = dma_guess_pool(pool, gfp))) {
page = __dma_alloc_from_pool(dev, size, pool, cpu_addr,
phys_addr_ok);
- if (page)
+ if (page) {
+ dev_err(dev, "%s: phys addr 0x%llx, size %lu,
dev->coherent_dma_mask 0x%llx, dev->bus_dma_limit 0x%llx\n",
+ __func__, (phys_addr_t)*cpu_addr, size,
dev->coherent_dma_mask, dev->bus_dma_limit);
return page;
+ }
}
WARN(1, "Failed to get suitable pool for %s\n", dev_name(dev));
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 04/15] arm64: numa: simplify dummy_numa_init()
On Tue, 28 Jul 2020 08:11:42 +0300
Mike Rapoport wrote:
> From: Mike Rapoport
>
> dummy_numa_init() loops over memblock.memory and passes nid=0 to
> numa_add_memblk() which essentially wraps memblock_set_node(). However,
> memblock_set_node() can cope with entire memory span itself, so the loop
> over memblock.memory regions is redundant.
>
> Replace the loop with a single call to memblock_set_node() to the entire
> memory.
Hi Mike,
I had a similar patch I was going to post shortly so can add a bit more
on the advantages of this one.
Beyond cleaning up, it also fixes an issue with a buggy ACPI firmware in which
the SRAT
table covers some but not all of the memory in the EFI memory map. Stealing
bits
from the draft cover letter I had for that...
> This issue can be easily triggered by having an SRAT table which fails
> to cover all elements of the EFI memory map.
>
> This firmware error is detected and a warning printed. e.g.
> "NUMA: Warning: invalid memblk node 64 [mem 0x24000-0x27fff]"
> At that point we fall back to dummy_numa_init().
>
> However, the failed ACPI init has left us with our memblocks all broken
> up as we split them when trying to assign them to NUMA nodes.
>
> We then iterate over the memblocks and add them to node 0.
>
> for_each_memblock(memory, mblk) {
> ret = numa_add_memblk(0, mblk->base, mblk->base + mblk->size);
> if (!ret)
> continue;
> pr_err("NUMA init failed\n");
> return ret;
> }
>
> numa_add_memblk() calls memblock_set_node() which merges regions that
> were previously split up during the earlier attempt to add them to different
> nodes during parsing of SRAT.
>
> This means elements are moved in the memblock array and we can end up
> in a different memblock after the call to numa_add_memblk().
> Result is:
>
> Unable to handle kernel paging request at virtual address 3a40
> Mem abort info:
> ESR = 0x9604
> EC = 0x25: DABT (current EL), IL = 32 bits
> SET = 0, FnV = 0
> EA = 0, S1PTW = 0
> Data abort info:
> ISV = 0, ISS = 0x0004
> CM = 0, WnR = 0
> [3a40] user address but active_mm is swapper
> Internal error: Oops: 9604 [#1] PREEMPT SMP
>
> ...
>
> Call trace:
> sparse_init_nid+0x5c/0x2b0
> sparse_init+0x138/0x170
> bootmem_init+0x80/0xe0
> setup_arch+0x2a0/0x5fc
> start_kernel+0x8c/0x648
>
> As an illustrative example:
> EFI table has one block of memory.
> memblks[0] = [0...0x2f] so we start with a single memblock.
>
> SRAT has
> [0x00...0x0f] in node 0
> [0x10...0x1f] in node 1
> but no entry covering
> [0x20...0x2f].
>
> Whilst parsing SRAT the single memblock is broken into 3.
> memblks[0] = [0x00...0x0f] in node 0
> memblks[1] = [0x10...0x1f] in node 1
> memblks[2] = [0x20...0x2f] in node MAX_NUM_NODES (invalid value)
>
> A sanity check parse then detects the invalid section and acpi_numa_init
> fails. We then fall back to the dummy path.
>
> That iterates over the memblocks. We'll use i an index in the array of
> memblocks
>
> i = 0;
> memblks[0] = [0x00...0x0f] set to node0.
>merge doesn't do anything because the neighbouring memblock is still in
> node1.
>
> i = 1
> memblks[1] = [0x10...0x1f] set to node 0.
>merge combines memblock 0 and 1 to give a new set of memblocks.
>
> memblks[0] = [0x00..0x1f] in node 0
> memblks[1] = [0x20..0x2f] in node MAX_NUM_NODES.
>
> i = 2 off the end of the now reduced array of memblocks, so exit the loop.
> (if we restart the loop here everything will be fine).
>
> Later sparse_init_nid tries to use the node of the second memblock to index
> somethings and boom.
>
> Signed-off-by: Mike Rapoport
Acked-by: Jonathan Cameron
> ---
> arch/arm64/mm/numa.c | 13 +
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
> index aafcee3e3f7e..0cbdbcc885fb 100644
> --- a/arch/arm64/mm/numa.c
> +++ b/arch/arm64/mm/numa.c
> @@ -423,19 +423,16 @@ static int __init numa_init(int (*init_func)(void))
> */
> static int __init dummy_numa_init(void)
> {
> + phys_addr_t start = memblock_start_of_DRAM();
> + phys_addr_t end = memblock_end_of_DRAM();
> int ret;
> - struct memblock_region *mblk;
>
> if (numa_off)
> pr_info("NUMA disabled\n"); /* Forced off on command line. */
> - pr_info("Faking a node at [mem %#018Lx-%#018Lx]\n",
> - memblock_start_of_DRAM(), memblock_end_of_DRAM() - 1);
> -
> - for_each_memblock(memory, mblk) {
> - ret = numa_add_memblk(0, mblk->base, mblk->base + mblk->size);
> - if (!ret)
> - continue;
> + pr_info("Faking a node at [mem %#018Lx-%#018Lx]\n", start, end - 1);
>
> + ret = numa_add_memblk(0, start, end);
> + if (ret) {
> pr_err("NUMA init failed\n");
> return ret;
> }
___
iommu mailing list
iommu@lists
