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)
+    /* 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.

Reply via email to