On Thu, Nov 1, 2018 at 11:30 AM Alejandro Lucero < alejandro.luc...@netronome.com> wrote:
> > > On Thu, Nov 1, 2018 at 10:57 AM Burakov, Anatoly < > anatoly.bura...@intel.com> wrote: > >> On 01-Nov-18 10:48 AM, Alejandro Lucero wrote: >> > >> > >> > On Thu, Nov 1, 2018 at 10:11 AM Burakov, Anatoly >> > <anatoly.bura...@intel.com <mailto:anatoly.bura...@intel.com>> wrote: >> > >> > On 31-Oct-18 5:29 PM, Alejandro Lucero wrote: >> > > This patch adds the possibility of setting a dma mask to be used >> > > once the memory initialization is done. >> > > >> > > This is currently needed when IOVA mode is set by PCI related >> > > code and an x86 IOMMU hardware unit is present. Current code >> calls >> > > rte_mem_check_dma_mask but it is wrong to do so at that point >> > > because the memory has not been initialized yet. >> > > >> > > Signed-off-by: Alejandro Lucero <alejandro.luc...@netronome.com >> > <mailto:alejandro.luc...@netronome.com>> >> > > --- >> > > lib/librte_eal/common/eal_common_memory.c | 10 ++++++++++ >> > > lib/librte_eal/common/include/rte_memory.h | 10 ++++++++++ >> > > lib/librte_eal/rte_eal_version.map | 1 + >> > > 3 files changed, 21 insertions(+) >> > > >> > > diff --git a/lib/librte_eal/common/eal_common_memory.c >> > b/lib/librte_eal/common/eal_common_memory.c >> > > index e0f08f39a..24b72fcb0 100644 >> > > --- a/lib/librte_eal/common/eal_common_memory.c >> > > +++ b/lib/librte_eal/common/eal_common_memory.c >> > > @@ -480,6 +480,16 @@ rte_mem_check_dma_mask(uint8_t maskbits) >> > > return 0; >> > > } >> > > >> > > +/* set dma mask to use when memory initialization is done */ >> > > +void __rte_experimental >> > > +rte_mem_set_dma_mask(uint8_t maskbits) >> > > +{ >> > > + struct rte_mem_config *mcfg = >> > rte_eal_get_configuration()->mem_config; >> > > + >> > > + mcfg->dma_maskbits = mcfg->dma_maskbits == 0 ? maskbits : >> > > + RTE_MIN(mcfg->dma_maskbits, maskbits); >> > > +} >> > > + >> > > /* return the number of memory channels */ >> > > unsigned rte_memory_get_nchannel(void) >> > > { >> > > diff --git a/lib/librte_eal/common/include/rte_memory.h >> > b/lib/librte_eal/common/include/rte_memory.h >> > > index ad3f3cfb0..eff028db1 100644 >> > > --- a/lib/librte_eal/common/include/rte_memory.h >> > > +++ b/lib/librte_eal/common/include/rte_memory.h >> > > @@ -466,6 +466,16 @@ unsigned rte_memory_get_nrank(void); >> > > /* check memsegs iovas are within a range based on dma mask */ >> > > int __rte_experimental rte_mem_check_dma_mask(uint8_t >> maskbits); >> > > >> > > +/** >> > > + * * @warning >> > > + * @b EXPERIMENTAL: this API may change without prior notice >> > >> > I think there's a copy-paste error here (extra star and indent >> before >> > warning). >> > >> > >> > I will fix this. >> > >> > Thanks. >> > >> > >> > > + * >> > > + * Set dma mask to use once memory initialization is done. >> > > + * Previous function rte_mem_check_dma_mask can not be used >> > > + * safely until memory has been initialized. >> > >> > IMO this really requires a big bold warning saying that this >> function >> > must only be called early during initialization, before memory is >> > initialized, and not to be called in user code. >> > >> > >> > what about adding a new EAL variable for keeping memory initialization >> > status? >> > It would be something like "uninit" until is changed to "done" after >> the >> > memory initialization is completed. >> > That variable would help to avoid effective calls to set the dma mask >> > after initialization. >> >> I'm not opposed to it in principle, however, this is a slippery slope >> towards having each and every subsystem storing their init status :) >> >> That said, while it's not a complete solution because it won't prevent >> spurious calls to this function _after_ memory has initialized, there is >> a variable in internal_config that you can use to deny calls to this >> function after EAL init is complete (see internal_config->init_complete). >> >> You could also store a static variable in eal_common_memory.c, and >> change it at the end of rte_eal_memory_init() call, but that does not >> really complete the memory initialization, because we then go to >> rte_eal_malloc_heap_init(), which actually completes the memory init. >> >> I would greatly prefer using internal_config->init_complete - it is >> enough to forbid user code from calling it, while driver/DPDK developers >> presumably know what they're doing enough to read the warning and not >> try to call this after memory init :) >> > > I agree that we should avoid new status variables as I proposed, and in > this case > I think using internal_config->init_complete will be enough. > > I'm wrong. And you already said it, that this will not avoid a bad usage from PMDs. I think I will follow your first advice about adding a warning about its usage. > I will add the check in the next version. > > Thanks > > >> >> > >> > > + */ >> > > +void __rte_experimental rte_mem_set_dma_mask(uint8_t maskbits); >> > > + >> > > /** >> > > * Drivers based on uio will not load unless physical >> > > * addresses are obtainable. It is only possible to get >> > > diff --git a/lib/librte_eal/rte_eal_version.map >> > b/lib/librte_eal/rte_eal_version.map >> > > index ef8126a97..ae24b5c73 100644 >> > > --- a/lib/librte_eal/rte_eal_version.map >> > > +++ b/lib/librte_eal/rte_eal_version.map >> > > @@ -296,6 +296,7 @@ EXPERIMENTAL { >> > > rte_devargs_remove; >> > > rte_devargs_type_count; >> > > rte_mem_check_dma_mask; >> > > + rte_mem_set_dma_mask; >> > >> > Same, this needs to be alphabetic. >> > >> > > rte_eal_cleanup; >> > > rte_fbarray_attach; >> > > rte_fbarray_destroy; >> > > >> > >> > >> > -- >> > Thanks, >> > Anatoly >> > >> >> >> -- >> Thanks, >> Anatoly >> >
