Merge pull request #164 from bo3z/bugfix/iommu-mappings

Fix memory mapping of writeback region

Author: JonasDann

contrib/vm/driver-guest/guest_dev.h

@@ -45,10 +45,10 @@
 #define USER_HASH_TABLE_ORDER 8
 
 /* MMAP Regions */
-#define MMAP_CTRL 0x0
+#define MMAP_WB 0x0
 #define MMAP_CNFG 0x1
 #define MMAP_CNFG_AVX 0x2
-#define MMAP_WB 0x3
+#define MMAP_CTRL 0x3
 #define MMAP_BUFF 0x200
 #define MMAP_PR 0x400
 #define FPGA_CTRL_SIZE 256 * 1024

driver/include/coyote_defs.h

@@ -80,6 +80,7 @@
 #include <linux/dma-buf.h>
 #include <linux/dma-direct.h>
 #include <linux/dma-resv.h>
+#include <linux/dma-mapping.h>
 
 // Driver arguments; see coyote_driver.c for details
 extern char *ip_addr;
@@ -269,6 +270,7 @@ extern bool en_hmm;
 #define MAX_N_MAP_PAGES 256  
 #define MAX_N_MAP_HUGE_PAGES 256
 #define MAX_SINGLE_DMA_SYNC 4   // 4 pages
+#define BUFF_NEEDS_EXP_SYNC_RET_CODE 99
 
 // Card memory constants
 #define MEM_START (256UL * 1024UL * 1024UL)
@@ -305,10 +307,10 @@ extern bool en_hmm;
 #define MAX_CHAR_FDEV 32
 
 // Offsets for memory-mapped regions
-#define MMAP_CTRL 0x0
+#define MMAP_WB 0x0
 #define MMAP_CNFG 0x1
 #define MMAP_CNFG_AVX 0x2
-#define MMAP_WB 0x3
+#define MMAP_CTRL 0x3
 #define MMAP_RECONFIG 0x100
 
 // vFPGA IOCTL calls; see vfpga_ops.c for more details
@@ -652,6 +654,9 @@ struct user_pages {
 
     /// Array of physical addresses on the host, one for each page in the pages array, simply obtained by calling page_to_phys on each page
     uint64_t *hpages;
+
+    /// Set to true if explicit synchronization (i.e. dma_sync_single_for_{device,cpu}) is needed for this buffer, false otherwise
+    bool needs_explicit_sync;
 };
 
 /**

driver/src/coyote_setup.c

@@ -394,10 +394,17 @@ int setup_vfpga_devices(struct bus_driver_data *data) {
                 pr_err("failed to allocate writeback memory\n");
                 goto err_wb;
             }
+            
+            int ret_val = set_memory_uc((uint64_t) data->vfpga_dev[i].wb_addr_virt, N_WB_PAGES);
+            if (ret_val) {
+                pr_err("failed so set UC for writeback memory\n");
+                goto err_wb;
+            }
 
             for (int j = 0; j < WB_BLOCKS; j++) {
                 data->vfpga_dev[i].cnfg_regs->wback[j] = data->vfpga_dev[i].wb_phys_addr + j * (N_CTID_MAX * sizeof(uint32_t));
             }
+
             dbg_info(
                 "allocated memory for descriptor writeback, vaddr %llx, paddr %llx\n",
                 (uint64_t)data->vfpga_dev[i].wb_addr_virt, data->vfpga_dev[i].wb_phys_addr

driver/src/vfpga/vfpga_gup.c

@@ -92,6 +92,13 @@ int mmu_handler_gup(struct vfpga_dev *device, uint64_t vaddr, uint64_t len, int3
             return -ENOMEM;
         }
 
+        // In case there are caching effects, return a non-zero code to the user space
+        // The application continues as normal, but this code will generate a warning
+        // in the user space
+        if (user_pg->needs_explicit_sync) {
+            ret_val = BUFF_NEEDS_EXP_SYNC_RET_CODE;
+        }
+
         if(stream) {  
            tlb_map_gup(device, &pf_desc, user_pg, hpid);           
         } else {
@@ -257,6 +264,7 @@ void tlb_unmap_gup(struct vfpga_dev *device, struct user_pages *user_pg, pid_t h
 
 struct user_pages* tlb_get_user_pages(struct vfpga_dev *device, struct pf_aligned_desc *pf_desc, pid_t hpid, struct task_struct *curr_task, struct mm_struct *curr_mm) {
     int ret_val = 0;
+    int pg_inc, pg_size;
     struct bus_driver_data *bd_data = device->bd_data;
 
     // Error handling
@@ -287,27 +295,97 @@ struct user_pages* tlb_get_user_pages(struct vfpga_dev *device, struct pf_aligne
     dbg_info("pages=0x%p\n", user_pg->pages);
 
     // Pin the pages
+    // On newer kernels, pin_user_pages_remote is preferred over get_user_pages_remote for DMA,
+    // as it guarantees that the pages remain pinned (and not just the page struct) until explicitly unpinned
     #if LINUX_VERSION_CODE >= KERNEL_VERSION(6, 5, 0)
-       ret_val = get_user_pages_remote(curr_mm, (unsigned long) pf_desc->vaddr << PAGE_SHIFT, pf_desc->n_pages, 1, user_pg->pages, NULL);
+        ret_val = pin_user_pages_remote(curr_mm, (unsigned long) pf_desc->vaddr << PAGE_SHIFT, pf_desc->n_pages, FOLL_WRITE | FOLL_LONGTERM, user_pg->pages, NULL);
     #elif LINUX_VERSION_CODE >= KERNEL_VERSION(5, 9, 0)
-        ret_val = get_user_pages_remote(curr_mm, (unsigned long) pf_desc->vaddr << PAGE_SHIFT, pf_desc->n_pages, 1, user_pg->pages, NULL, NULL);
-    #else 
-        ret_val = get_user_pages_remote(curr_task, curr_mm, (unsigned long)pf_desc->vaddr << PAGE_SHIFT, pf_desc->n_pages, 1, user_pg->pages, NULL, NULL);
+        ret_val = pin_user_pages_remote(curr_mm, (unsigned long) pf_desc->vaddr << PAGE_SHIFT, pf_desc->n_pages, FOLL_WRITE | FOLL_LONGTERM, user_pg->pages, NULL, NULL);
+    #elif LINUX_VERSION_CODE >= KERNEL_VERSION(5, 4, 0)
+        ret_val = pin_user_pages_remote(curr_task, curr_mm, (unsigned long) pf_desc->vaddr << PAGE_SHIFT, pf_desc->n_pages, FOLL_WRITE | FOLL_LONGTERM, user_pg->pages, NULL, NULL);
+    #else
+        ret_val = get_user_pages_remote(curr_task, curr_mm, (unsigned long) pf_desc->vaddr << PAGE_SHIFT, pf_desc->n_pages, 1, user_pg->pages, NULL, NULL);
     #endif
-    dbg_info("get_user_pages_remote(%llx, n_pages = %d, page start = %lx, hugepages = %d)\n", pf_desc->vaddr, pf_desc->n_pages, page_to_pfn(user_pg->pages[0]), pf_desc->hugepages);
+    dbg_info("pin_user_pages_remote(%llx, n_pages = %d, page start = %lx, hugepages = %d)\n", pf_desc->vaddr, pf_desc->n_pages, page_to_pfn(user_pg->pages[0]), pf_desc->hugepages);
 
-    if(ret_val < pf_desc->n_pages) {
-        dbg_info("could not get all user pages, %d\n", ret_val);
-        goto fail_host_unmap;
+    if (ret_val < pf_desc->n_pages) {
+        pr_warn("could not get all user pages, %d\n", ret_val);
+        goto fail_host_alloc;
     }
 
     // Flush cache
-    for(int i = 0; i < pf_desc->n_pages; i++)
+    for (int i = 0; i < pf_desc->n_pages; i++) {
         flush_dcache_page(user_pg->pages[i]);
+    }
 
     // Find the physical address of the pages
-    for(int i = 0;i < pf_desc->n_pages; i++)
-        user_pg->hpages[i] = page_to_phys(user_pg->pages[i]);
+    // NOTE: The CPU's physical address and the physical address 
+    // that the FPGA should write to may be different if there is
+    // an additional layer of translation via the system IOMMU
+    // Therefore, we use the dma_map_single to obtain a physical address
+    // suitable for FPGA accesses.
+    user_pg->needs_explicit_sync = false;
+    if (pf_desc->hugepages) {
+        // Map each hugepage (e.g., 2MB) chunk
+        for (int i = 0; i < pf_desc->n_pages; i+=device->bd_data->n_pages_in_huge) {
+            // Obtain the physical address of this hugepage chunk
+            // Generally, for the hardware TLB, only the starting address of the page is needed
+            // The exact physical address is calculated from the starting address and the virtual address offset
+            user_pg->hpages[i] = dma_map_single(
+                &device->bd_data->pci_dev->dev,
+                page_to_virt(user_pg->pages[i]),
+                device->bd_data->ltlb_meta->page_size,
+                DMA_BIDIRECTIONAL
+            );
+
+            if (dma_mapping_error(&device->bd_data->pci_dev->dev, user_pg->hpages[i])) {
+                pr_warn("failed to map user pages and obtain physical address");
+                goto fail_dma_map;
+            }
+
+            if (dma_need_sync(&device->bd_data->pci_dev->dev, user_pg->hpages[i])) {
+                pr_warn("the DMA buffer with virt_addr %lx, phys_addr %lx, may be subject to cache coherency issues and may require explicit synchronization which is not supported out of the box by Coyote\n", 
+                    (unsigned long) page_to_virt(user_pg->pages[i]), (unsigned long) user_pg->hpages[i]
+                );
+                user_pg->needs_explicit_sync = true;
+            }
+
+            // However, in some cases (e.g., migrating data between the host and FPGA memory)
+            // Coyote still needs all the entries in the hpages array; since the transfers
+            // are issued in 4k granularity from the driver
+            for (int j = i + 1; j < i + device->bd_data->n_pages_in_huge; j++) {
+                user_pg->hpages[j] = user_pg->hpages[i] + (j - i) * PAGE_SIZE;
+
+                if (j >= pf_desc->n_pages) {
+                    break;
+                }
+            }
+
+        }
+    } else {
+        // For each page, map it to the FPGA char dev
+        // and obtain its physical address
+        for (int i = 0; i < pf_desc->n_pages; i++) {
+            user_pg->hpages[i] = dma_map_single(
+                &device->bd_data->pci_dev->dev,
+                page_to_virt(user_pg->pages[i]),
+                PAGE_SIZE,
+                DMA_BIDIRECTIONAL
+            );
+
+            if (dma_mapping_error(&device->bd_data->pci_dev->dev, user_pg->hpages[i])) {
+                pr_warn("failed to map user pages and obtain physical address");
+                goto fail_dma_map;
+            }
+
+            if (dma_need_sync(&device->bd_data->pci_dev->dev, user_pg->hpages[i])) {
+                pr_warn("the DMA buffer with virt_addr %lx, phys_addr %lx, may be subject to cache coherency issues and may require explicit synchronization which is not supported out of the box by Coyote\n", 
+                    (unsigned long) page_to_virt(user_pg->pages[i]), (unsigned long) user_pg->hpages[i]
+                );
+                user_pg->needs_explicit_sync = true;
+            }
+        }
+    }
 
     // Allocate memory on the card if available
     if(bd_data->en_mem) {
@@ -317,7 +395,7 @@ struct user_pages* tlb_get_user_pages(struct vfpga_dev *device, struct pf_aligne
         ret_val = alloc_card_memory(device, user_pg->cpages, pf_desc->n_pages, pf_desc->hugepages);
         if (ret_val) {
             dbg_info("could not get all card pages, %d\n", ret_val);
-            goto fail_card_unmap;
+            goto fail_card_alloc;
         }
     }
 
@@ -332,25 +410,64 @@ struct user_pages* tlb_get_user_pages(struct vfpga_dev *device, struct pf_aligne
 
     return user_pg;
 
-fail_host_unmap:
-    // Release the pages
-    for(int i = 0; i < ret_val; i++) {
-        put_page(user_pg->pages[i]);
+fail_host_alloc:
+    // Unpin the pages
+    #if LINUX_VERSION_CODE >= KERNEL_VERSION(5, 4, 0)
+        unpin_user_pages(user_pg->pages, ret_val);
+    #else
+        for(int i = 0; i < ret_val; i++) {
+            put_page(user_pg->pages[i]);
+        }
+    #endif
+
+    // Free the dynamically allocated memory
+    vfree(user_pg->pages);
+    vfree(user_pg->hpages);
+    kfree(user_pg);
+
+    return NULL;
+
+fail_dma_map:
+    // Unmap DMA
+    pg_inc = pf_desc->hugepages ? device->bd_data->n_pages_in_huge : 1;
+    pg_size = pf_desc->hugepages ? device->bd_data->ltlb_meta->page_size : PAGE_SIZE;
+    for (int i = 0; i < pf_desc->n_pages; i+=pg_inc) {
+        dma_unmap_single(&device->bd_data->pci_dev->dev, user_pg->hpages[i], pg_size, DMA_BIDIRECTIONAL);
     }
 
+    // Unpin the pages
+    #if LINUX_VERSION_CODE >= KERNEL_VERSION(5, 4, 0)
+        unpin_user_pages(user_pg->pages, pf_desc->n_pages);
+    #else
+        for(int i = 0; i < pf_desc->n_pages; i++) {
+            put_page(user_pg->pages[i]);
+        }
+    #endif
+    
     // Free the dynamically allocated memory
     vfree(user_pg->pages);
     vfree(user_pg->hpages);
     kfree(user_pg);
 
     return NULL;
 
-fail_card_unmap:
-    // Release the pages
-    for(int i = 0; i < user_pg->n_pages; i++) {
-        put_page(user_pg->pages[i]);
+fail_card_alloc:
+    // Unmap DMA
+    pg_inc = pf_desc->hugepages ? device->bd_data->n_pages_in_huge : 1;
+    pg_size = pf_desc->hugepages ? device->bd_data->ltlb_meta->page_size : PAGE_SIZE;
+    for (int i = 0; i < pf_desc->n_pages; i+=pg_inc) {
+        dma_unmap_single(&device->bd_data->pci_dev->dev, user_pg->hpages[i], pg_size, DMA_BIDIRECTIONAL);
     }
 
+    // Unpin the pages
+    #if LINUX_VERSION_CODE >= KERNEL_VERSION(5, 4, 0)
+        unpin_user_pages(user_pg->pages, pf_desc->n_pages);
+    #else
+        for(int i = 0; i < pf_desc->n_pages; i++) {
+            put_page(user_pg->pages[i]);
+        }
+    #endif
+
     // Free the dynamically allocated memory
     vfree(user_pg->pages);
     vfree(user_pg->hpages);
@@ -403,12 +520,23 @@ int tlb_put_user_pages(struct vfpga_dev *device, uint64_t vaddr, int32_t ctid, p
                         SetPageDirty(tmp_entry->pages[i]);
                     }
                 }
-                
-                // Release page
-                for(int i = 0; i < tmp_entry->n_pages; i++) {
-                    put_page(tmp_entry->pages[i]);
+
+                // Unmap DMA
+                int pg_inc = tmp_entry->huge ? device->bd_data->n_pages_in_huge : 1;
+                int pg_size = tmp_entry->huge ? device->bd_data->ltlb_meta->page_size : PAGE_SIZE;
+                for (int i = 0; i < tmp_entry->n_pages; i+=pg_inc) {
+                    dma_unmap_single(&device->bd_data->pci_dev->dev, tmp_entry->hpages[i], pg_size, DMA_BIDIRECTIONAL);
                 }
 
+                // Unpin the pages
+                #if LINUX_VERSION_CODE >= KERNEL_VERSION(5, 4, 0)
+                    unpin_user_pages(tmp_entry->pages, tmp_entry->n_pages);
+                #else
+                    for(int i = 0; i < tmp_entry->n_pages; i++) {
+                        put_page(tmp_entry->pages[i]);
+                    }
+                #endif
+                
                 // Release memory to hold pages
                 vfree(tmp_entry->pages);
             }
@@ -460,14 +588,29 @@ int tlb_put_user_pages_ctid(struct vfpga_dev *device, int32_t ctid, pid_t hpid,
                 return -1;
             #endif
         } else {
-            if(dirtied)
-                for(i = 0; i < tmp_entry->n_pages; i++)
+            if (dirtied) {
+                for (i = 0; i < tmp_entry->n_pages; i++) {
                     SetPageDirty(tmp_entry->pages[i]);
+                }
+            }
+            
+            // Unmap DMA
+            int pg_inc = tmp_entry->huge ? device->bd_data->n_pages_in_huge : 1;
+            int pg_size = tmp_entry->huge ? device->bd_data->ltlb_meta->page_size : PAGE_SIZE;
+            for (int i = 0; i < tmp_entry->n_pages; i+=pg_inc) {
+                dma_unmap_single(&device->bd_data->pci_dev->dev, tmp_entry->hpages[i], pg_size, DMA_BIDIRECTIONAL);
+            }
+            
+            // Unpin the pages
+            #if LINUX_VERSION_CODE >= KERNEL_VERSION(5, 4, 0)
+                unpin_user_pages(tmp_entry->pages, tmp_entry->n_pages);
+            #else
+                for(int i = 0; i < tmp_entry->n_pages; i++) {
+                    put_page(tmp_entry->pages[i]);
+                }
+            #endif
+                
 
-            // Release host pages
-            for(i = 0; i < tmp_entry->n_pages; i++)
-                put_page(tmp_entry->pages[i]);
-
             // Release memory to hold pages
             vfree(tmp_entry->pages);
         }

driver/src/vfpga/vfpga_isr.c

@@ -175,7 +175,7 @@ void vfpga_pfault_handler(struct work_struct *work) {
         ret_val = mmu_handler_gup(device, irq_pf->vaddr, irq_pf->len, irq_pf->ctid, irq_pf->stream, hpid);
     #endif
 
-    if (ret_val) {
+    if (ret_val && ret_val != BUFF_NEEDS_EXP_SYNC_RET_CODE) {
         drop_irq_pfault(device, irq_pf->wr, irq_pf->ctid);
         pr_err("MMU handler error, vFPGA %d, error %d\n", device->id, ret_val);
         goto err_mmu;

driver/src/vfpga/vfpga_ops.c

@@ -303,7 +303,7 @@ long vfpga_dev_ioctl(struct file *file, unsigned int command, unsigned long arg)
                 #endif            
                     ret_val = mmu_handler_gup(device, tmp[0], tmp[1], ctid, true, hpid);
 
-                if (ret_val) {
+                if (ret_val && ret_val != BUFF_NEEDS_EXP_SYNC_RET_CODE) {
                     dbg_info("buffer could not be mapped, ret_val: %d\n", ret_val);
                 }
 
@@ -654,20 +654,18 @@ int vfpga_dev_mmap(struct file *file, struct vm_area_struct *vma) {
 
     // Memory map writeback region
     if (vma->vm_pgoff == MMAP_WB) {
-        set_memory_uc((uint64_t) device->wb_addr_virt, N_WB_PAGES);
         dbg_info(
             "fpga dev. %d, memory mapping writeback regions at %llx of size %lx\n",
             device->id, device->wb_phys_addr, WB_SIZE
         );
-        int ret_val = remap_pfn_range(
-            vma, 
-            vma->vm_start, 
-            (device->wb_phys_addr) >> PAGE_SHIFT,
-            WB_SIZE, 
-            vma->vm_page_prot
+        
+        // dma_mmap_coherent expects vma->pg_offs to be 0; hence MMAP_WB was changed to 0 and MMAP_CTRL to 3
+        int ret_val = dma_mmap_coherent(
+            &device->bd_data->pci_dev->dev, vma, (void *) device->wb_addr_virt, device->wb_phys_addr, PAGE_SIZE 
         );
+
         if (ret_val) {
-            pr_warn("remap_pfn_range failed for writeback region, ret_val: %d\n", ret_val);
+            pr_warn("dma_mmap_coherent failed for writeback region, ret_val: %d\n", ret_val);
             return -EIO;
         } else {
             return 0;