On 07-Oct-20 6:44 PM, David Christensen wrote:
On 9/17/20 4:13 AM, Burakov, Anatoly wrote:
On 10-Aug-20 10:07 PM, David Christensen wrote:
The SPAPR IOMMU requires that a DMA window size be defined before memory
can be mapped for DMA. Current code dynamically modifies the DMA window
size in response to every new memory allocation which is potentially
dangerous because all existing mappings need to be unmapped/remapped in
order to resize the DMA window, leaving hardware holding IOVA addresses
that are temporarily unmapped. The new SPAPR code statically assigns
the DMA window size on first use, using the largest physical memory
memory address when IOVA=PA and the highest existing memseg virtual
address when IOVA=VA.
Signed-off-by: David Christensen <d...@linux.vnet.ibm.com>
---
<snip>
+struct spapr_size_walk_param {
+ uint64_t max_va;
+ uint64_t page_sz;
+ int external;
+};
+
+/*
+ * In order to set the DMA window size required for the SPAPR IOMMU
+ * we need to walk the existing virtual memory allocations as well as
+ * find the hugepage size used.
+ */
static int
-vfio_spapr_unmap_walk(const struct rte_memseg_list *msl,
- const struct rte_memseg *ms, void *arg)
+vfio_spapr_size_walk(const struct rte_memseg_list *msl, void *arg)
{
- int *vfio_container_fd = arg;
+ struct spapr_size_walk_param *param = arg;
+ uint64_t max = (uint64_t) msl->base_va + (uint64_t) msl->len;
- /* skip external memory that isn't a heap */
- if (msl->external && !msl->heap)
- return 0;
+ if (msl->external) {
+ param->external++;
+ if (!msl->heap)
+ return 0;
+ }
It would be nice to have some comments in the code explaining what
we're skipping and why.
Reviewing this again, my inclination is to skip ALL external memory,
which by definition would seem to be outside of IOMMU control, so the
code would read:
if (msl->external) {
param->external++;
return 0;
}
The external memory can still be mapped for DMA with rte_dev_dma_map()
API. The heap memory is meant to be mapped automatically by DPDK, while
the non-heap memory (created with rte_extmem_register() API) is meant to
be managed by the user and will be mapped using the user_mem_map
functions in this file.
Not sure why existing code such as vfio_spapr_map_walk() distinguishes
between heap and non-heap in this situation. Are there instances in x86
where it would matter?
Also, seems that you're using param->external as bool? This is a
non-public API so using stdbool is not an issue here, perhaps replace
it with bool param->has_external?
Why do you think the distinction is necessary?
It's not *necessary*, i just don't like the ancient C style where ints
are used as booleans :D Not a serious issue though, your choice.
Dave
--
Thanks,
Anatoly
