[dpdk-stable] [PATCH 1/6] mem: add function for checking memsegs IOVAs addresses

[Alejandro Lucero]

On Tue, Jul 3, 2018 at 10:07 AM, Burakov, Anatoly <anatoly.burakov at intel.com
> wrote:

> On 02-Jul-18 6:26 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.
>>
>> Signed-off-by: Alejandro Lucero <alejandro.lucero at netronome.com>
>> ---
>>
>
> <snip>
>
> +       const struct rte_mem_config *mcfg;
>> +       uint64_t mask;
>> +       int i;
>> +       int ret = 0;
>> +
>> +       /* create dma mask */
>> +       mask = 1ULL << maskbits;
>> +       mask -= 1;
>>
>
> mask = ~((1ULL << maskbits) - 1);
>
> ? IMO this makes it much more clear what you're trying to do.
>
>
Sure. I will change it.


> +
>> +       /* get pointer to global configuration */
>> +       mcfg = rte_eal_get_configuration()->mem_config;
>> +
>> +       for (i = 0; i < RTE_MAX_MEMSEG; i++) {
>> +               if (mcfg->memseg[i].addr == NULL)
>> +                       break;
>> +
>> +               if (mcfg->memseg[i].iova & ~mask) {
>> +                       ret = -1;
>> +                       break;
>> +               }
>> +       }
>> +
>> +       if (!ret)
>> +               return ret;
>> +
>> +       RTE_LOG(INFO, EAL, "memseg[%d] iova %"PRIx64" out of range:\n",
>> +                          i, mcfg->memseg[i].iova);
>> +       RTE_LOG(INFO, EAL, "\tusing dma mask %"PRIx64"\n", mask);
>> +
>> +       return -1;
>>
>
> The control flow looks weird to me. You break if iova has any bits that
> are in the mask, then you display log messages and return -1. How about
> just logging error and returning -1, and simply returning 0 after the loop?
>
>
I agree. The truth is I did that initially, but the log lines were too long
with the indent. I will go back to the original.

Thanks


> --
> Thanks,
> Anatoly
>