On 09-Apr-18 1:09 PM, Shreyansh Jain wrote:
On Monday 09 April 2018 04:25 PM, Burakov, Anatoly wrote:
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.j...@nxp.com>
Signed-off-by: Anatoly Burakov <anatoly.bura...@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.
Yes, that and the debug message was not helpful.
This is what I had in mind. (DPAA2_BUS_DEBUG doesn't require an extra \n)
DPAA2_BUS_DEBUG("Request for %s, va=%p, virt_addr=0x%" PRIx64 ","
"iova=0x%" PRIx64 ", map_len=%zu",
type == RTE_MEM_EVENT_ALLOC? "alloc" : "dealloc",
va, virt_addr, iova_addr, map_len);
+ 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.
My policy is not different, but the various variables being dumped
cannot anyway help in grepping - So, keeping the variables on separate
lines for 80chars is ok. "DMA Mapping/Unmapping failed." is enough for
greps.
+ 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.
Agree - that count was only required in the earlier case. It can be
removed.
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;
}
I think there are enough changes, even if trivial. Maybe I can rework
this patch and send you. If that is inconvenient, just extract from that
whatever you want.
There aren't a lot of changes so i'll respin it myself, addressing the
comments above. Thanks!
--
Thanks,
Anatoly
