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

[Shreyansh Jain <shreyansh.jain@nxp.com>]

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