Hello Anatoly,
On Sunday 08 April 2018 10:44 PM, Burakov, Anatoly wrote:
On 05-Apr-18 3:14 PM, Shreyansh Jain wrote:
Restructure VFIO DMA code for handling hotplug memory events
(callbacks) and --legacy case.
Signed-off-by: Shreyansh Jain <shreyansh.j...@nxp.com>
---
###
This is based on the 16fbfef04a3 github repository. This is assuming
that changes already exists as done in patch 26/68.
Though, this can be a standalone, replacing 26/88. Though, the Makefile
changes don't exist in this.
Also, this just a first draft. I will push any review changes after this
incrementally over v4.
###
Hi Shreyansh,
I think we can keep the 26/68 as it still works within the context of
the patchset. I would like to add these changes closer to the end, where
we enable support for callbacks in VFIO (this could/should come as the
next patch).
But then it would also mean that dpaa2 would be broken within the memory
hotplug patches?
I think it would be broken once the memseg ceases to be continuous
physical sets.
That said, i took some liberties when integrating this patch, hopefully
for the better. I know you mentioned it's a draft, so you can post any
comments for the inevitable v4 :)
drivers/bus/fslmc/fslmc_bus.c | 15 ++++
drivers/bus/fslmc/fslmc_vfio.c | 161
+++++++++++++++++++++++++++++++++++----
drivers/bus/fslmc/fslmc_vfio.h | 1 +
drivers/net/dpaa2/dpaa2_ethdev.c | 1 -
4 files changed, 163 insertions(+), 15 deletions(-)
diff --git a/drivers/bus/fslmc/fslmc_bus.c
b/drivers/bus/fslmc/fslmc_bus.c
index 5ee0beb85..50884ff3a 100644
--- a/drivers/bus/fslmc/fslmc_bus.c
+++ b/drivers/bus/fslmc/fslmc_bus.c
@@ -266,6 +266,21 @@ rte_fslmc_probe(void)
return 0;
}
+ if (rte_log_get_global_level() >= RTE_LOG_DEBUG)
+ rte_dump_physmem_layout(stdout);
Presumably, this is not needed - just debug leftovers?
Yes, and can be removed.
+
+ /* Map existing segments as well as, in case of hotpluggable memory,
+ * install callback handler.
+ */
+ ret = rte_fslmc_vfio_dmamap();
+ if (ret) {
+ FSLMC_BUS_LOG(ERR, "Unable to DMA map existing VAs: (%d)",
+ ret);
+ /* Not continuing ahead */
+ FSLMC_BUS_LOG(ERR, "FSLMC VFIO Mapping failed");
+ return 0;
+ }
+
What happens if there are no devices on the bus that can be used by
DPDK? As far as i can tell, it would return error, which may or may not
be desirable (failing to map anything because there aren't any fslmc
devices is not an error?).
## If no devices found on the bus:
Call wouldn't have reached here. There is a jump out in rte_fslmc_probe
in case no devices were scanned on the bus.
--->8--- rte_fslmc_probe() ---
...
if (TAILQ_EMPTY(&rte_fslmc_bus.device_list))
return 0;
...
--->8---
## Assuming you are pointing to 'return 0':
So, the rte_eal_scan/probe doesn't expect to be interrupted just because
one of the buses had issues (whether no devices found, not a real
issue). It would continue ahead with scan/probe.
But, I think error should be returned so that rte_eal_probe can dump to
logs the error and continue ahead normally. I will fix this.
For "regular" VFIO, the container is an empty shell unless you add
groups into it - does fslmc VFIO support work differently, and container
is already working/initialized by the time we reach this point?
FSLMC VFIO Container is not much different from generic container. But,
at this point in code, the container is already initialized (group is
connected to it, and relevant device fds extracted).
Only thing pending beyond this point is to look into the container and
initialize various fslmc specific devices contained *within* the group.
And, dma mapping of regions which is done using the newly introduced code.
Anyway, i'll leave this as is.
OK.
ret = fslmc_vfio_process_group();
if (ret) {
FSLMC_BUS_LOG(ERR, "Unable to setup devices %d", ret);
diff --git a/drivers/bus/fslmc/fslmc_vfio.c
b/drivers/bus/fslmc/fslmc_vfio.c
index 31831e3ce..60725fb70 100644
--- a/drivers/bus/fslmc/fslmc_vfio.c
+++ b/drivers/bus/fslmc/fslmc_vfio.c
@@ -30,6 +30,7 @@
#include <rte_kvargs.h>
#include <rte_dev.h>
#include <rte_bus.h>
+#include <rte_eal_memconfig.h>
<...>
+}
+
static int
-fslmc_vfio_map(const struct rte_memseg_list *msl __rte_unused,
- const struct rte_memseg *ms, void *arg)
+#ifdef RTE_LIBRTE_DPAA2_USE_PHYS_IOVA
+fslmc_map_dma(uint64_t vaddr, rte_iova_t iovaddr, size_t len)
+#else
+fslmc_map_dma(uint64_t vaddr, rte_iova_t iovaddr __rte_unused, size_t
len)
+#endif
I think i'll leave this as just "rte_iova_t iovaaddr __rte_unused" :)
Hmm, it is harmless if used without the enclosing #defines.
But, i don't know if any compiler has any side-effects attached to this
- for example, clang reporting this as error, etc.
{
- int *n_segs = arg;
struct fslmc_vfio_group *group;
struct vfio_iommu_type1_dma_map dma_map = {
.argsz = sizeof(struct vfio_iommu_type1_dma_map),
@@ -205,10 +263,11 @@ fslmc_vfio_map(const struct rte_memseg_list *msl
__rte_unused,
};
int ret;
- dma_map.size = ms->len;
- dma_map.vaddr = ms->addr_64;
+ dma_map.size = len;
+ dma_map.vaddr = vaddr;
<...>
if (is_dma_done)
return 0;
I suspect this check was needed because you've done VFIO mapping on
device probe as opposed to bus probe, so VFIO mapping function could've
been called multiple times. Is that still the case, or is this check no
longer needed? I took the liberty to remove it.
Yes, that is correct. Now that device probe vfio is disabled, it is safe
to remove this check.
- if (rte_memseg_walk(fslmc_vfio_map, &i) < 0)
+ /* Lock before parsing and registering callback to memory
subsystem */
+ rte_rwlock_read_lock(mem_lock);
+
<...>
return 0;
diff --git a/drivers/bus/fslmc/fslmc_vfio.h
b/drivers/bus/fslmc/fslmc_vfio.h
index e8fb3445f..e77e4c4ac 100644
--- a/drivers/bus/fslmc/fslmc_vfio.h
+++ b/drivers/bus/fslmc/fslmc_vfio.h
@@ -9,6 +9,7 @@
#define _FSLMC_VFIO_H_
#include <rte_vfio.h>
+#include <rte_memory.h>
#include "eal_vfio.h"
I suspect this change is not needed? I took the liberty to remove it.
I remember when I was using your initial patch, this I added based on
structures being unresolved. It is possible that is not longer the case
(and that I too have moved a lot of code beyond the first internal
draft). If it compiles OK, no need for this.
-
Shreyansh
