[dpdk-dev] [PATCH v4 66/70] bus/fslmc: enable support for mem event callbacks for vfio

[Burakov, Anatoly]

On 09-Apr-18 11:01 AM, Shreyansh Jain wrote:
> Hi Anatoly,
> 
> On Monday 09 April 2018 01:48 AM, Anatoly Burakov wrote:
>> VFIO needs to map and unmap segments for DMA whenever they
>> become available or unavailable, so register a callback for
>> memory events, and provide map/unmap functions.
>>
>> Signed-off-by: Shreyansh Jain <shreyansh.jain at nxp.com>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov at intel.com>
>> ---

<...>

>> +        DPAA2_BUS_DEBUG("Calling with type=%d, va=%p, virt_addr=0x%" 
>> PRIx64 ", iova=0x%" PRIx64 ", map_len=%zu\n",
> 
> I would like to correct this message (80char + rewording) - What do you 
> suggest? Should I send a new patch to you or just convey what should be 
> changed?
> 

As far as i know, leaving strings on single line is good for grepping. 
However, perhaps having PRIx64 etc in there breaks it anyway.

>> +                type, va, virt_addr, iova_addr, map_len);
>> +
>> +        if (type == RTE_MEM_EVENT_ALLOC)
>> +            ret = fslmc_map_dma(virt_addr, iova_addr, map_len);
>> +        else
>> +            ret = fslmc_unmap_dma(virt_addr, iova_addr, map_len);
>> +
>> +        if (ret != 0) {
>> +            DPAA2_BUS_ERR("DMA Mapping/Unmapping failed. Map=%d, 
>> addr=%p, len=%zu, err:(%d)",
>> +                    type, va, map_len, ret);
> 
> Same as above. 80 Char issue.

Same reasoning - leaving strings unbroken allows for easier grepping the 
codebase, but i'm not sure what's our policy on having formatted strings 
unbroken.

> 
>> +            return;
>> +        }
>> +
>> +        cur_len += map_len;
>> +    }
>> +
>> +    if (type == RTE_MEM_EVENT_ALLOC)
>> +        DPAA2_BUS_DEBUG("Total Mapped: addr=%p, len=%zu\n",
>> +                addr, len);
>> +    else

<...>

>> +    ret = rte_mem_event_callback_register("fslmc_memevent_clb",
>> +                          fslmc_memevent_cb);
>> +    if (ret && rte_errno == ENOTSUP)
>> +        DPAA2_BUS_DEBUG("Memory event callbacks not supported");
>> +    else if (ret)
>> +        DPAA2_BUS_DEBUG("Unable to install memory handler");
>> +    else
>> +        DPAA2_BUS_DEBUG("Installed memory callback handler");
>>       /* Verifying that at least single segment is available */
>>       if (i <= 0) {
>> +        /* TODO: Is this verification required any more? What would
>> +         * happen to non-legacy case where nothing was preallocated
>> +         * thus causing i==0?
>> +         */
> 
> And this as well - if call backs are not going to appear in case of 
> legacy, this needs to be removed.

Callbacks aren't only not going to appear in legacy mode - they will 
also not appear on FreeBSD. We check this above, with checking rte_errno 
value (if callbacks are not supported, it's set to ENOTSUP, and having 
callbacks unsupported is not an error).

> let me know how do you want to take these changes.
> 

Now that i think of it, this error condition is wrong. This is called in 
both legacy and non-legacy mode. This is bus probe, no? For non-legacy 
mode, it is entirely possible to start without any memory whatsoever. It 
just so happens that rte_service API allocates some on init, and hence 
you always have at least one segment - that may not be the case forever. 
So, non-legacy mode, not having memsegs is not an error, it is expected 
behavior, so maybe we should remove this error check altogether.

>>           DPAA2_BUS_ERR("No Segments found for VFIO Mapping");
>> +        rte_rwlock_read_unlock(mem_lock);
>>           return -1;
>>       }
>>       DPAA2_BUS_DEBUG("Total %d segments found.", i);
>> @@ -250,6 +367,11 @@ int rte_fslmc_vfio_dmamap(void)
>>        */
>>       vfio_map_irq_region(&vfio_group);
>> +    /* Existing segments have been mapped and memory callback for 
>> hotplug
>> +     * has been installed.
>> +     */
>> +    rte_rwlock_read_unlock(mem_lock);
>> +
>>       return 0;
>>   }
>>
> 
> 


-- 
Thanks,
Anatoly