Re: [dpdk-dev] [PATCH v3 1/1] vfio: modify spapr iommu support to use static window sizing

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 12-Oct-20 8:19 PM, David Christensen wrote:
> \>>>> -vfio_spapr_unmap_walk(const struct rte_memseg_list *msl,
>>>>> -        const struct rte_memseg *ms, void *arg)
>>>>> +vfio_spapr_size_walk(const struct rte_memseg_list *msl, void *arg)
>>>>>   {
>>>>> -    int *vfio_container_fd = arg;
>>>>> +    struct spapr_size_walk_param *param = arg;
>>>>> +    uint64_t max = (uint64_t) msl->base_va + (uint64_t) msl->len;
>>>>> -    /* skip external memory that isn't a heap */
>>>>> -    if (msl->external && !msl->heap)
>>>>> -        return 0;
>>>>> +    if (msl->external) {
>>>>> +        param->external++;
>>>>> +        if (!msl->heap)
>>>>> +            return 0;
>>>>> +    }
>>>>
>>>> It would be nice to have some comments in the code explaining what 
>>>> we're skipping and why.
>>>
>>> Reviewing this again, my inclination is to skip ALL external memory, 
>>> which by definition would seem to be outside of IOMMU control, so the 
>>> code would read:
>>>
>>>     if (msl->external) {
>>>         param->external++;
>>>         return 0;
>>>     }
>>
>> The external memory can still be mapped for DMA with rte_dev_dma_map() 
>> API. The heap memory is meant to be mapped automatically by DPDK, 
>> while the non-heap memory (created with rte_extmem_register() API) is 
>> meant to be managed by the user and will be mapped using the 
>> user_mem_map functions in this file.
> 
> So for my purpose of identifying the memory range qualified for IOMMU 
> protection, are you saying that external memory in the heap should be 
> included in the DMA window calculation?  Like this:
> 
>          if (msl->external && !msl->heap) {
>                  /* ignore user managed external memory */
>                  param->is_user_managed = true;
>                  return 0;
>          }
> 
> Dave

I would say so, yes. I would also double check the user mem map path to 
see if it makes sense with these changes, and correctly calculates the 
new window, should it be needed.

-- 
Thanks,
Anatoly