From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id 97A9A1B65D for ; Sun, 8 Apr 2018 19:14:47 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 08 Apr 2018 10:14:44 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,424,1517904000"; d="scan'208";a="214942435" Received: from unknown (HELO [10.249.35.182]) ([10.249.35.182]) by orsmga005.jf.intel.com with ESMTP; 08 Apr 2018 10:14:43 -0700 To: Shreyansh Jain Cc: dev@dpdk.org References: <20180405141414.11146-1-shreyansh.jain@nxp.com> From: "Burakov, Anatoly" Message-ID: <1040d76d-a000-93ed-1cae-34f76007d97f@intel.com> Date: Sun, 8 Apr 2018 18:14:40 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180405141414.11146-1-shreyansh.jain@nxp.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] bus/fslmc: support for hotplugging of memory X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 08 Apr 2018 17:14:48 -0000 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 > --- > > ### > 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 > #include > #include > +#include <...> > +} > + > 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 > +#include > > #include "eal_vfio.h" > I suspect this change is not needed? I took the liberty to remove it. -- Thanks, Anatoly