[dpdk-dev] [PATCH] bus/fslmc: support for hotplugging of memory

[Burakov, Anatoly]

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.jain at 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).

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?

> +
> +	/* 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?).

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?

Anyway, i'll leave this as is.

>   	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" :)

>   {
> -	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.

> -	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.

-- 
Thanks,
Anatoly