[v2,1/5] mem: add function for checking memsegs IOVAs addresses
Checks
Commit Message
A device can suffer addressing limitations. This functions checks
memsegs have iovas within the supported range based on dma mask.
PMD should use this during initialization if supported devices
suffer addressing limitations, returning an error if this function
returns memsegs out of range.
Another potential usage is for emulated IOMMU hardware with addressing
limitations.
It is necessary to save the most restricted dma mask for checking
memory allocated dynamically after initialization.
Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
---
lib/librte_eal/common/eal_common_memory.c | 56 +++++++++++++++++++++++
lib/librte_eal/common/include/rte_eal_memconfig.h | 3 ++
lib/librte_eal/common/include/rte_memory.h | 3 ++
lib/librte_eal/common/malloc_heap.c | 12 +++++
lib/librte_eal/linuxapp/eal/eal.c | 2 +
lib/librte_eal/rte_eal_version.map | 1 +
6 files changed, 77 insertions(+)
Comments
On 31-Aug-18 1:50 PM, Alejandro Lucero wrote:
> A device can suffer addressing limitations. This functions checks
> memsegs have iovas within the supported range based on dma mask.
>
> PMD should use this during initialization if supported devices
> suffer addressing limitations, returning an error if this function
> returns memsegs out of range.
>
> Another potential usage is for emulated IOMMU hardware with addressing
> limitations.
>
> It is necessary to save the most restricted dma mask for checking
> memory allocated dynamically after initialization.
>
> Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> ---
> lib/librte_eal/common/eal_common_memory.c | 56 +++++++++++++++++++++++
> lib/librte_eal/common/include/rte_eal_memconfig.h | 3 ++
> lib/librte_eal/common/include/rte_memory.h | 3 ++
> lib/librte_eal/common/malloc_heap.c | 12 +++++
> lib/librte_eal/linuxapp/eal/eal.c | 2 +
> lib/librte_eal/rte_eal_version.map | 1 +
> 6 files changed, 77 insertions(+)
>
> diff --git a/lib/librte_eal/common/eal_common_memory.c b/lib/librte_eal/common/eal_common_memory.c
> index fbfb1b0..bdd8f44 100644
> --- a/lib/librte_eal/common/eal_common_memory.c
> +++ b/lib/librte_eal/common/eal_common_memory.c
> @@ -383,6 +383,62 @@ struct virtiova {
> rte_memseg_walk(dump_memseg, f);
> }
>
> +static int
> +check_iova(const struct rte_memseg_list *msl __rte_unused,
> + const struct rte_memseg *ms, void *arg)
> +{
> + uint64_t *mask = arg;
> + rte_iova_t iova;
> +
> + /* higher address within segment */
> + iova = (ms->iova + ms->len) - 1;
> + if (!(iova & *mask))
> + return 0;
> +
> + RTE_LOG(INFO, EAL, "memseg iova %"PRIx64", len %zx, out of range\n",
> + ms->iova, ms->len);
> +
> + RTE_LOG(INFO, EAL, "\tusing dma mask %"PRIx64"\n", *mask);
IMO putting these as INFO is overkill. I'd prefer not to spam the output
unless it's really important. Can this go under DEBUG?
Also, the message is misleading. You stop before you have a chance to
check other masks, which may restrict them even further. You're
outputting the message about using DMA mask XXX but this may not be the
final DMA mask.
> + /* Stop the walk and change mask */
> + *mask = 0;
> + return 1;
> +}
> +
> +#if defined(RTE_ARCH_64)
> +#define MAX_DMA_MASK_BITS 63
> +#else
> +#define MAX_DMA_MASK_BITS 31
> +#endif
> +
> +/* check memseg iovas are within the required range based on dma mask */
> +int __rte_experimental
> +rte_eal_check_dma_mask(uint8_t maskbits)
> +{
> + struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
> + uint64_t mask;
> +
> + /* sanity check */
> + if (maskbits > MAX_DMA_MASK_BITS) {
> + RTE_LOG(INFO, EAL, "wrong dma mask size %u (Max: %u)\n",
> + maskbits, MAX_DMA_MASK_BITS);
Should be ERR, not INFO.
> + return -1;
> + }
> +
> + /* keep the more restricted maskbit */
> + if (!mcfg->dma_maskbits || maskbits < mcfg->dma_maskbits)
> + mcfg->dma_maskbits = maskbits;
Do we need to modify mcfg->dma_maskbits before we know if we're going to
fail? Suggest using a local variable maybe?
Also, i think it's a good case for ternary:
bits = mcfg->dma_maskbits == 0 ?
maskbits :
RTE_MIN(maskbits, mcfg->dma_maskbits);
IMO the intention looks much clearer.
> +
> + /* create dma mask */
> + mask = ~((1ULL << maskbits) - 1);
> +
> + rte_memseg_walk(check_iova, &mask);
> +
> + if (!mask)
> + return -1;
> +
> + return 0;
> +}
> +
> /* return the number of memory channels */
> unsigned rte_memory_get_nchannel(void)
> {
> diff --git a/lib/librte_eal/common/include/rte_eal_memconfig.h b/lib/librte_eal/common/include/rte_eal_memconfig.h
> index aff0688..aea44cb 100644
> --- a/lib/librte_eal/common/include/rte_eal_memconfig.h
> +++ b/lib/librte_eal/common/include/rte_eal_memconfig.h
> @@ -77,6 +77,9 @@ struct rte_mem_config {
> * exact same address the primary process maps it.
> */
> uint64_t mem_cfg_addr;
> +
> + /* keeps the more restricted dma mask */
> + uint8_t dma_maskbits;
This needs to be documented as an ABI break in the 18.11 release notes.
I sent this email only to Anatoly. Sending it again to mailing list.
On Wed, Oct 3, 2018 at 1:43 PM Burakov, Anatoly <anatoly.burakov@intel.com>
wrote:
> On 31-Aug-18 1:50 PM, Alejandro Lucero wrote:
> > A device can suffer addressing limitations. This functions checks
> > memsegs have iovas within the supported range based on dma mask.
> >
> > PMD should use this during initialization if supported devices
> > suffer addressing limitations, returning an error if this function
> > returns memsegs out of range.
> >
> > Another potential usage is for emulated IOMMU hardware with addressing
> > limitations.
> >
> > It is necessary to save the most restricted dma mask for checking
> > memory allocated dynamically after initialization.
> >
> > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> > ---
> > lib/librte_eal/common/eal_common_memory.c | 56
> +++++++++++++++++++++++
> > lib/librte_eal/common/include/rte_eal_memconfig.h | 3 ++
> > lib/librte_eal/common/include/rte_memory.h | 3 ++
> > lib/librte_eal/common/malloc_heap.c | 12 +++++
> > lib/librte_eal/linuxapp/eal/eal.c | 2 +
> > lib/librte_eal/rte_eal_version.map | 1 +
> > 6 files changed, 77 insertions(+)
> >
> > diff --git a/lib/librte_eal/common/eal_common_memory.c
> b/lib/librte_eal/common/eal_common_memory.c
> > index fbfb1b0..bdd8f44 100644
> > --- a/lib/librte_eal/common/eal_common_memory.c
> > +++ b/lib/librte_eal/common/eal_common_memory.c
> > @@ -383,6 +383,62 @@ struct virtiova {
> > rte_memseg_walk(dump_memseg, f);
> > }
> >
> > +static int
> > +check_iova(const struct rte_memseg_list *msl __rte_unused,
> > + const struct rte_memseg *ms, void *arg)
> > +{
> > + uint64_t *mask = arg;
> > + rte_iova_t iova;
> > +
> > + /* higher address within segment */
> > + iova = (ms->iova + ms->len) - 1;
> > + if (!(iova & *mask))
> > + return 0;
> > +
> > + RTE_LOG(INFO, EAL, "memseg iova %"PRIx64", len %zx, out of
> range\n",
> > + ms->iova, ms->len);
> > +
> > + RTE_LOG(INFO, EAL, "\tusing dma mask %"PRIx64"\n", *mask);
>
> IMO putting these as INFO is overkill. I'd prefer not to spam the output
> unless it's really important. Can this go under DEBUG?
>
>
This checks comes from a device or from the alloc_pages_on_heap when
expanding memory. If the check discovers an address out of mask, a device
can not be used or the new memory can not be allocated. I think having this
info will help to understand why the device initialization or the memory
allocation are failing.
> Also, the message is misleading. You stop before you have a chance to
> check other masks, which may restrict them even further. You're
> outputting the message about using DMA mask XXX but this may not be the
> final DMA mask.
>
Well, this is the first triggering, and it is enough for reporting the
problem and avoiding the device or the new memory to be used.
Note that the mask is per device, and for the memory allocation case, it is
the most restrictive dma mask. So there are no other masks to try.
>
> > + /* Stop the walk and change mask */
> > + *mask = 0;
> > + return 1;
> > +}
> > +
> > +#if defined(RTE_ARCH_64)
> > +#define MAX_DMA_MASK_BITS 63
> > +#else
> > +#define MAX_DMA_MASK_BITS 31
> > +#endif
> > +
> > +/* check memseg iovas are within the required range based on dma mask */
> > +int __rte_experimental
> > +rte_eal_check_dma_mask(uint8_t maskbits)
> > +{
> > + struct rte_mem_config *mcfg =
> rte_eal_get_configuration()->mem_config;
> > + uint64_t mask;
> > +
> > + /* sanity check */
> > + if (maskbits > MAX_DMA_MASK_BITS) {
> > + RTE_LOG(INFO, EAL, "wrong dma mask size %u (Max: %u)\n",
> > + maskbits, MAX_DMA_MASK_BITS);
>
> Should be ERR, not INFO.
>
>
Right. I will change it.
> > + return -1;
> > + }
> > +
> > + /* keep the more restricted maskbit */
> > + if (!mcfg->dma_maskbits || maskbits < mcfg->dma_maskbits)
> > + mcfg->dma_maskbits = maskbits;
>
> Do we need to modify mcfg->dma_maskbits before we know if we're going to
> fail? Suggest using a local variable maybe?
>
>
Yes, that's true. If the check fails, the device will not be used therefore
we do not need to keep that dma mask at all.
I will change the order here.
Thanks!
> Also, i think it's a good case for ternary:
>
> bits = mcfg->dma_maskbits == 0 ?
> maskbits :
> RTE_MIN(maskbits, mcfg->dma_maskbits);
>
> IMO the intention looks much clearer.
>
>
Agree.
> > +
> > + /* create dma mask */
> > + mask = ~((1ULL << maskbits) - 1);
> > +
> > + rte_memseg_walk(check_iova, &mask);
> > +
> > + if (!mask)
> > + return -1;
> > +
> > + return 0;
> > +}
> > +
> > /* return the number of memory channels */
> > unsigned rte_memory_get_nchannel(void)
> > {
> > diff --git a/lib/librte_eal/common/include/rte_eal_memconfig.h
> b/lib/librte_eal/common/include/rte_eal_memconfig.h
> > index aff0688..aea44cb 100644
> > --- a/lib/librte_eal/common/include/rte_eal_memconfig.h
> > +++ b/lib/librte_eal/common/include/rte_eal_memconfig.h
> > @@ -77,6 +77,9 @@ struct rte_mem_config {
> > * exact same address the primary process maps it.
> > */
> > uint64_t mem_cfg_addr;
> > +
> > + /* keeps the more restricted dma mask */
> > + uint8_t dma_maskbits;
>
> This needs to be documented as an ABI break in the 18.11 release notes.
>
>
Ok. I'll add that in the next version.
Thanks
>
> --
> Thanks,
> Anatoly
>
On 04-Oct-18 1:59 PM, Alejandro Lucero wrote:
> I sent this email only to Anatoly. Sending it again to mailing list.
>
> On Wed, Oct 3, 2018 at 1:43 PM Burakov, Anatoly <anatoly.burakov@intel.com>
> wrote:
>
>> On 31-Aug-18 1:50 PM, Alejandro Lucero wrote:
>>> A device can suffer addressing limitations. This functions checks
>>> memsegs have iovas within the supported range based on dma mask.
>>>
>>> PMD should use this during initialization if supported devices
>>> suffer addressing limitations, returning an error if this function
>>> returns memsegs out of range.
>>>
>>> Another potential usage is for emulated IOMMU hardware with addressing
>>> limitations.
>>>
>>> It is necessary to save the most restricted dma mask for checking
>>> memory allocated dynamically after initialization.
>>>
>>> Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
>>> ---
>>> lib/librte_eal/common/eal_common_memory.c | 56
>> +++++++++++++++++++++++
>>> lib/librte_eal/common/include/rte_eal_memconfig.h | 3 ++
>>> lib/librte_eal/common/include/rte_memory.h | 3 ++
>>> lib/librte_eal/common/malloc_heap.c | 12 +++++
>>> lib/librte_eal/linuxapp/eal/eal.c | 2 +
>>> lib/librte_eal/rte_eal_version.map | 1 +
>>> 6 files changed, 77 insertions(+)
>>>
>>> diff --git a/lib/librte_eal/common/eal_common_memory.c
>> b/lib/librte_eal/common/eal_common_memory.c
>>> index fbfb1b0..bdd8f44 100644
>>> --- a/lib/librte_eal/common/eal_common_memory.c
>>> +++ b/lib/librte_eal/common/eal_common_memory.c
>>> @@ -383,6 +383,62 @@ struct virtiova {
>>> rte_memseg_walk(dump_memseg, f);
>>> }
>>>
>>> +static int
>>> +check_iova(const struct rte_memseg_list *msl __rte_unused,
>>> + const struct rte_memseg *ms, void *arg)
>>> +{
>>> + uint64_t *mask = arg;
>>> + rte_iova_t iova;
>>> +
>>> + /* higher address within segment */
>>> + iova = (ms->iova + ms->len) - 1;
>>> + if (!(iova & *mask))
>>> + return 0;
>>> +
>>> + RTE_LOG(INFO, EAL, "memseg iova %"PRIx64", len %zx, out of
>> range\n",
>>> + ms->iova, ms->len);
>>> +
>>> + RTE_LOG(INFO, EAL, "\tusing dma mask %"PRIx64"\n", *mask);
>>
>> IMO putting these as INFO is overkill. I'd prefer not to spam the output
>> unless it's really important. Can this go under DEBUG?
>>
>>
> This checks comes from a device or from the alloc_pages_on_heap when
> expanding memory. If the check discovers an address out of mask, a device
> can not be used or the new memory can not be allocated. I think having this
> info will help to understand why the device initialization or the memory
> allocation are failing.
>
If this text is only displayed whenever there's an error, the log output
should be ERR, not INFO. If the error may or may not happen depending on
who called this function, then this information is not important enough
to display to the user (it should be displayed in the error handler of
the caller), and DEBUG should suffice.
>
>> Also, the message is misleading. You stop before you have a chance to
>> check other masks, which may restrict them even further. You're
>> outputting the message about using DMA mask XXX but this may not be the
>> final DMA mask.
>>
>
> Well, this is the first triggering, and it is enough for reporting the
> problem and avoiding the device or the new memory to be used.
>
> Note that the mask is per device, and for the memory allocation case, it is
> the most restrictive dma mask. So there are no other masks to try.
Fair enough.
>
>
>
>>
>>> + /* Stop the walk and change mask */
>>> + *mask = 0;
>>> + return 1;
No need for out-of-band communication, _walk() function will return 1 if
walk was stopped prematurely. Just check return value of walk().
On Thu, Oct 4, 2018 at 4:39 PM Burakov, Anatoly <anatoly.burakov@intel.com>
wrote:
> On 04-Oct-18 1:59 PM, Alejandro Lucero wrote:
> > I sent this email only to Anatoly. Sending it again to mailing list.
> >
> > On Wed, Oct 3, 2018 at 1:43 PM Burakov, Anatoly <
> anatoly.burakov@intel.com>
> > wrote:
> >
> >> On 31-Aug-18 1:50 PM, Alejandro Lucero wrote:
> >>> A device can suffer addressing limitations. This functions checks
> >>> memsegs have iovas within the supported range based on dma mask.
> >>>
> >>> PMD should use this during initialization if supported devices
> >>> suffer addressing limitations, returning an error if this function
> >>> returns memsegs out of range.
> >>>
> >>> Another potential usage is for emulated IOMMU hardware with addressing
> >>> limitations.
> >>>
> >>> It is necessary to save the most restricted dma mask for checking
> >>> memory allocated dynamically after initialization.
> >>>
> >>> Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> >>> ---
> >>> lib/librte_eal/common/eal_common_memory.c | 56
> >> +++++++++++++++++++++++
> >>> lib/librte_eal/common/include/rte_eal_memconfig.h | 3 ++
> >>> lib/librte_eal/common/include/rte_memory.h | 3 ++
> >>> lib/librte_eal/common/malloc_heap.c | 12 +++++
> >>> lib/librte_eal/linuxapp/eal/eal.c | 2 +
> >>> lib/librte_eal/rte_eal_version.map | 1 +
> >>> 6 files changed, 77 insertions(+)
> >>>
> >>> diff --git a/lib/librte_eal/common/eal_common_memory.c
> >> b/lib/librte_eal/common/eal_common_memory.c
> >>> index fbfb1b0..bdd8f44 100644
> >>> --- a/lib/librte_eal/common/eal_common_memory.c
> >>> +++ b/lib/librte_eal/common/eal_common_memory.c
> >>> @@ -383,6 +383,62 @@ struct virtiova {
> >>> rte_memseg_walk(dump_memseg, f);
> >>> }
> >>>
> >>> +static int
> >>> +check_iova(const struct rte_memseg_list *msl __rte_unused,
> >>> + const struct rte_memseg *ms, void *arg)
> >>> +{
> >>> + uint64_t *mask = arg;
> >>> + rte_iova_t iova;
> >>> +
> >>> + /* higher address within segment */
> >>> + iova = (ms->iova + ms->len) - 1;
> >>> + if (!(iova & *mask))
> >>> + return 0;
> >>> +
> >>> + RTE_LOG(INFO, EAL, "memseg iova %"PRIx64", len %zx, out of
> >> range\n",
> >>> + ms->iova, ms->len);
> >>> +
> >>> + RTE_LOG(INFO, EAL, "\tusing dma mask %"PRIx64"\n", *mask);
> >>
> >> IMO putting these as INFO is overkill. I'd prefer not to spam the output
> >> unless it's really important. Can this go under DEBUG?
> >>
> >>
> > This checks comes from a device or from the alloc_pages_on_heap when
> > expanding memory. If the check discovers an address out of mask, a device
> > can not be used or the new memory can not be allocated. I think having
> this
> > info will help to understand why the device initialization or the memory
> > allocation are failing.
> >
>
> If this text is only displayed whenever there's an error, the log output
> should be ERR, not INFO. If the error may or may not happen depending on
> who called this function, then this information is not important enough
> to display to the user (it should be displayed in the error handler of
> the caller), and DEBUG should suffice.
>
>
Ok. Makes sense. I will change it.
Thanks
> >
> >> Also, the message is misleading. You stop before you have a chance to
> >> check other masks, which may restrict them even further. You're
> >> outputting the message about using DMA mask XXX but this may not be the
> >> final DMA mask.
> >>
> >
> > Well, this is the first triggering, and it is enough for reporting the
> > problem and avoiding the device or the new memory to be used.
> >
> > Note that the mask is per device, and for the memory allocation case, it
> is
> > the most restrictive dma mask. So there are no other masks to try.
>
> Fair enough.
>
> >
> >
> >
> >>
> >>> + /* Stop the walk and change mask */
> >>> + *mask = 0;
> >>> + return 1;
>
> No need for out-of-band communication, _walk() function will return 1 if
> walk was stopped prematurely. Just check return value of walk().
>
>
Yes, that's right. I will use the return from the walk function instead.
Thanks
> --
> Thanks,
> Anatoly
>
@@ -383,6 +383,62 @@ struct virtiova {
rte_memseg_walk(dump_memseg, f);
}
+static int
+check_iova(const struct rte_memseg_list *msl __rte_unused,
+ const struct rte_memseg *ms, void *arg)
+{
+ uint64_t *mask = arg;
+ rte_iova_t iova;
+
+ /* higher address within segment */
+ iova = (ms->iova + ms->len) - 1;
+ if (!(iova & *mask))
+ return 0;
+
+ RTE_LOG(INFO, EAL, "memseg iova %"PRIx64", len %zx, out of range\n",
+ ms->iova, ms->len);
+
+ RTE_LOG(INFO, EAL, "\tusing dma mask %"PRIx64"\n", *mask);
+ /* Stop the walk and change mask */
+ *mask = 0;
+ return 1;
+}
+
+#if defined(RTE_ARCH_64)
+#define MAX_DMA_MASK_BITS 63
+#else
+#define MAX_DMA_MASK_BITS 31
+#endif
+
+/* check memseg iovas are within the required range based on dma mask */
+int __rte_experimental
+rte_eal_check_dma_mask(uint8_t maskbits)
+{
+ struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
+ uint64_t mask;
+
+ /* sanity check */
+ if (maskbits > MAX_DMA_MASK_BITS) {
+ RTE_LOG(INFO, EAL, "wrong dma mask size %u (Max: %u)\n",
+ maskbits, MAX_DMA_MASK_BITS);
+ return -1;
+ }
+
+ /* keep the more restricted maskbit */
+ if (!mcfg->dma_maskbits || maskbits < mcfg->dma_maskbits)
+ mcfg->dma_maskbits = maskbits;
+
+ /* create dma mask */
+ mask = ~((1ULL << maskbits) - 1);
+
+ rte_memseg_walk(check_iova, &mask);
+
+ if (!mask)
+ return -1;
+
+ return 0;
+}
+
/* return the number of memory channels */
unsigned rte_memory_get_nchannel(void)
{
@@ -77,6 +77,9 @@ struct rte_mem_config {
* exact same address the primary process maps it.
*/
uint64_t mem_cfg_addr;
+
+ /* keeps the more restricted dma mask */
+ uint8_t dma_maskbits;
} __attribute__((__packed__));
@@ -357,6 +357,9 @@ typedef int (*rte_memseg_list_walk_t)(const struct rte_memseg_list *msl,
*/
unsigned rte_memory_get_nrank(void);
+/* check memsegs iovas are within a range based on dma mask */
+int rte_eal_check_dma_mask(uint8_t maskbits);
+
/**
* Drivers based on uio will not load unless physical
* addresses are obtainable. It is only possible to get
@@ -259,11 +259,13 @@ struct malloc_elem *
int socket, unsigned int flags, size_t align, size_t bound,
bool contig, struct rte_memseg **ms, int n_segs)
{
+ struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
struct rte_memseg_list *msl;
struct malloc_elem *elem = NULL;
size_t alloc_sz;
int allocd_pages;
void *ret, *map_addr;
+ uint64_t mask;
alloc_sz = (size_t)pg_sz * n_segs;
@@ -291,6 +293,16 @@ struct malloc_elem *
goto fail;
}
+ if (mcfg->dma_maskbits) {
+ mask = ~((1ULL << mcfg->dma_maskbits) - 1);
+ if (rte_eal_check_dma_mask(mask)) {
+ RTE_LOG(DEBUG, EAL,
+ "%s(): couldn't allocate memory due to DMA mask\n",
+ __func__);
+ goto fail;
+ }
+ }
+
/* add newly minted memsegs to malloc heap */
elem = malloc_heap_add_memory(heap, msl, map_addr, alloc_sz);
@@ -263,6 +263,8 @@ enum rte_iova_mode
* processes could later map the config into this exact location */
rte_config.mem_config->mem_cfg_addr = (uintptr_t) rte_mem_cfg_addr;
+ rte_config.mem_config->dma_maskbits = 0;
+
}
/* attach to an existing shared memory config */
@@ -284,6 +284,7 @@ EXPERIMENTAL {
rte_devargs_parsef;
rte_devargs_remove;
rte_devargs_type_count;
+ rte_eal_check_dma_mask;
rte_eal_cleanup;
rte_eal_hotplug_add;
rte_eal_hotplug_remove;