[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH] Qemu MSI Cleaning Up



>>> On 02.12.11 at 13:40, Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx> 
>>> wrote:
> The two mapping and unmapping big chuncks of code looks very similar
> apart from the DPCI_ADD_MAPPING/DPCI_REMOVE_MAPPING parameter.
> Could they be refactored into a single function called 
> "change_msix_mappings"?
> This is more or less what I have in mind:
> 
> change_msix_mappings(assigned_device, DPCI_REMOVE_MAPPING);
> 
> update mmio_base_addr
> 
> change_msix_mappings(assigned_device, DPCI_ADD_MAPPING);

Please see below/attached for the outcome. Without MSI-X capable
devices to pass through, I have to rely on others to do some testing
on this.

Jan

qemu: clean up MSI-X table handling

This patch does cleaning up of QEMU MSI handling. The fixes are:
1. Changes made to MSI-X table mapping handling to eliminate the small
windows in which guest could have access to physical MSI-X table.
2. MSI-X table is mapped as read-only to QEMU, as masking of MSI-X is
already in Xen now.
3. For registers that coexists inside the MSI-X table (this could be
only PBA I think), value read from physical page would be returned.

Signed-off-by:  Shan Haitao <maillists.shan@xxxxxxxxx>

Consolidated duplicate code into _pt_iomem_helper(). Fixed formatting.

Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

--- a/hw/pass-through.c
+++ b/hw/pass-through.c
@@ -92,6 +92,7 @@
 
 #include <unistd.h>
 #include <sys/ioctl.h>
+#include <assert.h>
 
 extern int gfx_passthru;
 int igd_passthru = 0;
@@ -1097,6 +1098,44 @@ uint8_t pci_intx(struct pt_dev *ptdev)
     return r_val;
 }
 
+static int _pt_iomem_helper(struct pt_dev *assigned_device, int i,
+                            uint32_t e_base, uint32_t e_size, int op)
+{
+    if ( has_msix_mapping(assigned_device, i) )
+    {
+        uint32_t msix_last_pfn = (assigned_device->msix->mmio_base_addr - 1 +
+            assigned_device->msix->total_entries * 16) >> XC_PAGE_SHIFT;
+        uint32_t bar_last_pfn = (e_base + e_size - 1) >> XC_PAGE_SHIFT;
+        int ret = 0;
+
+        if ( assigned_device->msix->table_off )
+            ret = xc_domain_memory_mapping(xc_handle, domid,
+                e_base >> XC_PAGE_SHIFT,
+                assigned_device->bases[i].access.maddr >> XC_PAGE_SHIFT,
+                (assigned_device->msix->mmio_base_addr >> XC_PAGE_SHIFT)
+                - (e_base >> XC_PAGE_SHIFT), op);
+
+        if ( ret == 0 && msix_last_pfn != bar_last_pfn )
+        {
+            assert(msix_last_pfn < bar_last_pfn);
+            ret = xc_domain_memory_mapping(xc_handle, domid,
+                msix_last_pfn + 1,
+                (assigned_device->bases[i].access.maddr +
+                 assigned_device->msix->table_off +
+                 assigned_device->msix->total_entries * 16 +
+                 XC_PAGE_SIZE - 1) >> XC_PAGE_SHIFT,
+                bar_last_pfn - msix_last_pfn, op);
+        }
+
+        return ret;
+    }
+
+    return xc_domain_memory_mapping(xc_handle, domid,
+        e_base >> XC_PAGE_SHIFT,
+        assigned_device->bases[i].access.maddr >> XC_PAGE_SHIFT,
+        (e_size + XC_PAGE_SIZE - 1) >> XC_PAGE_SHIFT, op);
+}
+
 /* Being called each time a mmio region has been updated */
 static void pt_iomem_map(PCIDevice *d, int i, uint32_t e_phys, uint32_t e_size,
                          int type)
@@ -1118,13 +1157,11 @@ static void pt_iomem_map(PCIDevice *d, i
 
     if ( !first_map && old_ebase != -1 )
     {
-        add_msix_mapping(assigned_device, i);
-        /* Remove old mapping */
-        ret = xc_domain_memory_mapping(xc_handle, domid,
-                old_ebase >> XC_PAGE_SHIFT,
-                assigned_device->bases[i].access.maddr >> XC_PAGE_SHIFT,
-                (e_size+XC_PAGE_SIZE-1) >> XC_PAGE_SHIFT,
-                DPCI_REMOVE_MAPPING);
+        if ( has_msix_mapping(assigned_device, i) )
+            unregister_iomem(assigned_device->msix->mmio_base_addr);
+
+        ret = _pt_iomem_helper(assigned_device, i, old_ebase, e_size,
+                               DPCI_REMOVE_MAPPING);
         if ( ret != 0 )
         {
             PT_LOG("Error: remove old mapping failed!\n");
@@ -1135,22 +1172,26 @@ static void pt_iomem_map(PCIDevice *d, i
     /* map only valid guest address */
     if (e_phys != -1)
     {
-        /* Create new mapping */
-        ret = xc_domain_memory_mapping(xc_handle, domid,
-                assigned_device->bases[i].e_physbase >> XC_PAGE_SHIFT,
-                assigned_device->bases[i].access.maddr >> XC_PAGE_SHIFT,
-                (e_size+XC_PAGE_SIZE-1) >> XC_PAGE_SHIFT,
-                DPCI_ADD_MAPPING);
+        if ( has_msix_mapping(assigned_device, i) )
+        {
+            assigned_device->msix->mmio_base_addr =
+                assigned_device->bases[i].e_physbase
+                + assigned_device->msix->table_off;
+
+            cpu_register_physical_memory(assigned_device->msix->mmio_base_addr,
+                 (assigned_device->msix->total_entries * 16 + XC_PAGE_SIZE - 1)
+                  & XC_PAGE_MASK,
+                 assigned_device->msix->mmio_index);
+        }
 
+        ret = _pt_iomem_helper(assigned_device, i, e_phys, e_size,
+                               DPCI_ADD_MAPPING);
         if ( ret != 0 )
         {
             PT_LOG("Error: create new mapping failed!\n");
+            return;
         }
 
-        ret = remove_msix_mapping(assigned_device, i);
-        if ( ret != 0 )
-            PT_LOG("Error: remove MSI-X mmio mapping failed!\n");
-
         if ( old_ebase != e_phys && old_ebase != -1 )
             pt_msix_update_remap(assigned_device, i);
     }
--- a/hw/pt-msi.c
+++ b/hw/pt-msi.c
@@ -284,15 +284,6 @@ void pt_disable_msi_translate(struct pt_
     dev->msi_trans_en = 0;
 }
 
-/* MSI-X virtulization functions */
-static void mask_physical_msix_entry(struct pt_dev *dev, int entry_nr, int 
mask)
-{
-    void *phys_off;
-
-    phys_off = dev->msix->phys_iomem_base + 16 * entry_nr + 12;
-    *(uint32_t *)phys_off = mask;
-}
-
 static int pt_msix_update_one(struct pt_dev *dev, int entry_nr)
 {
     struct msix_entry_info *entry = &dev->msix->msix_entry[entry_nr];
@@ -486,7 +477,6 @@ static void pci_msix_writel(void *opaque
     {
         if ( msix->enabled && !(val & 0x1) )
             pt_msix_update_one(dev, entry_nr);
-        mask_physical_msix_entry(dev, entry_nr, entry->io_mem[3] & 0x1);
     }
 }
 
@@ -519,7 +509,11 @@ static uint32_t pci_msix_readl(void *opa
     entry_nr = (addr - msix->mmio_base_addr) / 16;
     offset = ((addr - msix->mmio_base_addr) % 16) / 4;
 
-    return msix->msix_entry[entry_nr].io_mem[offset];
+    if ( addr - msix->mmio_base_addr < msix->total_entries * 16 )
+        return msix->msix_entry[entry_nr].io_mem[offset];
+    else
+        return *(uint32_t *)(msix->phys_iomem_base +
+                             (addr - msix->mmio_base_addr));
 }
 
 static CPUReadMemoryFunc *pci_msix_read[] = {
@@ -528,39 +522,12 @@ static CPUReadMemoryFunc *pci_msix_read[
     pci_msix_readl
 };
 
-int add_msix_mapping(struct pt_dev *dev, int bar_index)
+int has_msix_mapping(struct pt_dev *dev, int bar_index)
 {
     if ( !(dev->msix && dev->msix->bar_index == bar_index) )
         return 0;
 
-    return xc_domain_memory_mapping(xc_handle, domid,
-                dev->msix->mmio_base_addr >> XC_PAGE_SHIFT,
-                (dev->bases[bar_index].access.maddr
-                + dev->msix->table_off) >> XC_PAGE_SHIFT,
-                (dev->msix->total_entries * 16
-                + XC_PAGE_SIZE -1) >> XC_PAGE_SHIFT,
-                DPCI_ADD_MAPPING);
-}
-
-int remove_msix_mapping(struct pt_dev *dev, int bar_index)
-{
-    if ( !(dev->msix && dev->msix->bar_index == bar_index) )
-        return 0;
-
-    dev->msix->mmio_base_addr = dev->bases[bar_index].e_physbase
-                                + dev->msix->table_off;
-
-    cpu_register_physical_memory(dev->msix->mmio_base_addr,
-                                 dev->msix->total_entries * 16,
-                                 dev->msix->mmio_index);
-
-    return xc_domain_memory_mapping(xc_handle, domid,
-                dev->msix->mmio_base_addr >> XC_PAGE_SHIFT,
-                (dev->bases[bar_index].access.maddr
-                + dev->msix->table_off) >> XC_PAGE_SHIFT,
-                (dev->msix->total_entries * 16
-                + XC_PAGE_SIZE -1) >> XC_PAGE_SHIFT,
-                DPCI_REMOVE_MAPPING);
+    return 1;
 }
 
 int pt_msix_init(struct pt_dev *dev, int pos)
@@ -616,7 +583,7 @@ int pt_msix_init(struct pt_dev *dev, int
     PT_LOG("table_off = %x, total_entries = %d\n", table_off, total_entries);
     dev->msix->table_offset_adjust = table_off & 0x0fff;
     dev->msix->phys_iomem_base = mmap(0, total_entries * 16 + 
dev->msix->table_offset_adjust,
-                          PROT_WRITE | PROT_READ, MAP_SHARED | MAP_LOCKED,
+                          PROT_READ, MAP_SHARED | MAP_LOCKED,
                           fd, dev->msix->table_base + table_off - 
dev->msix->table_offset_adjust);
     dev->msix->phys_iomem_base = (void *)((char *)dev->msix->phys_iomem_base + 
                           dev->msix->table_offset_adjust);
--- a/hw/pt-msi.h
+++ b/hw/pt-msi.h
@@ -107,10 +107,7 @@ void
 pt_msix_disable(struct pt_dev *dev);
 
 int
-remove_msix_mapping(struct pt_dev *dev, int bar_index);
-
-int
-add_msix_mapping(struct pt_dev *dev, int bar_index);
+has_msix_mapping(struct pt_dev *dev, int bar_index);
 
 int
 pt_msix_init(struct pt_dev *dev, int pos);


Attachment: qemu-msix-cleanup.patch
Description: Text document

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.