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

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

On 07-Oct-20 6:44 PM, David Christensen wrote:
> 
> 
> On 9/17/20 4:13 AM, Burakov, Anatoly wrote:
>> On 10-Aug-20 10:07 PM, David Christensen wrote:
>>> The SPAPR IOMMU requires that a DMA window size be defined before memory
>>> can be mapped for DMA. Current code dynamically modifies the DMA window
>>> size in response to every new memory allocation which is potentially
>>> dangerous because all existing mappings need to be unmapped/remapped in
>>> order to resize the DMA window, leaving hardware holding IOVA addresses
>>> that are temporarily unmapped.  The new SPAPR code statically assigns
>>> the DMA window size on first use, using the largest physical memory
>>> memory address when IOVA=PA and the highest existing memseg virtual
>>> address when IOVA=VA.
>>>
>>> Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
>>> ---
>>
>> <snip>
>>
>>> +struct spapr_size_walk_param {
>>> +    uint64_t max_va;
>>> +    uint64_t page_sz;
>>> +    int external;
>>> +};
>>> +
>>> +/*
>>> + * In order to set the DMA window size required for the SPAPR IOMMU
>>> + * we need to walk the existing virtual memory allocations as well as
>>> + * find the hugepage size used.
>>> + */
>>>   static int
>>> -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.

> 
> Not sure why existing code such as vfio_spapr_map_walk() distinguishes 
> between heap and non-heap in this situation.  Are there instances in x86 
> where it would matter?
> 
>> Also, seems that you're using param->external as bool? This is a 
>> non-public API so using stdbool is not an issue here, perhaps replace 
>> it with bool param->has_external?
> 
> Why do you think the distinction is necessary?
> 

It's not *necessary*, i just don't like the ancient C style where ints 
are used as booleans :D Not a serious issue though, your choice.

> Dave


-- 
Thanks,
Anatoly