[Xen-devel] [PATCH v2] pci: a workaround for nonstandard PCI devices whose PBA shares

[Chao Gao <chao.gao@xxxxxxxxx>]

... the same page with other registers which are not relevant to MSI-X. Xen
marks pages where PBA resides as read-only. When assigning such devices to
guest, device driver writes MSI-X irrelevant registers on those pages would
lead to an EPT violation and the guest is destroyed because no handler is
registered for those address range. In order to make guest capable to use such
kind of devices, trapping very frequent write accesses is not a good idea for
it would significantly impact the performance.

This patch provides a workaround with caveat. Specifically, an option is
introduced to specify a list of devices. For those devices, Xen doesn't
control the access right to pages where PBA resides. Hence, guest device
driver is able to write those pages and functions well. Note that adding an
untrusted device to this option may endanger security of the entire system.

Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
---
Given that parsing parameters starts at very early stage in which xmalloc is
unusable, I choose to continue using an array other than a list to store SBDFs
of such kind devices, like the way how we manage phantom_devs.

changes in v2:
 - use a more informative name, pba_shared_quirk, to describe the quirk
 - use pci_sbdf_t other than uint32_t
 - disable this quirk when PBA shares the same page with other RO mmio
 - exit the loop in alloc_pdev() once a match is found
---
 docs/misc/xen-command-line.markdown | 10 ++++++++
 xen/arch/x86/msi.c                  | 20 ++++++++++++++--
 xen/drivers/passthrough/pci.c       | 48 +++++++++++++++++++++++++++++++++++--
 xen/include/asm-x86/msi.h           |  1 +
 4 files changed, 75 insertions(+), 4 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown 
b/docs/misc/xen-command-line.markdown
index b353352..7c10e63 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1423,6 +1423,16 @@ Defaults to booting secondary processors.
 
 > Default: `on`
 
+### pba\_shared\_quirk
+> `= List of [<seg>:]<bus>:<device>.<function>`
+
+Specify a list of SBDF of devices. When assigning devices in this list
+to guest, reading or writing the page where MSI-X PBA resides are
+allowed. This option provides a workaround for nonstandard PCI devices
+whose MSI-X PBA shares the same 4K-byte page with registers irrelevant
+to MSI-X. Note that adding an untrusted device to this option would
+undermine security of the entire system.
+
 ### pci
 > `= {no-}serr | {no-}perr`
 
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index 5567990..53280ec 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -992,7 +992,22 @@ static int msix_capability_init(struct pci_dev *dev,
         if ( rangeset_add_range(mmio_ro_ranges, msix->table.first,
                                 msix->table.last) )
             WARN();
-        if ( rangeset_add_range(mmio_ro_ranges, msix->pba.first,
+
+        /*
+         * If pages where MSI-X PBA resides overlap with other read-only mmio
+         * range, pba_shared_quirk won't meet our desire. Hence disable it.
+         */
+        if ( msix->pba_shared_quirk_enabled &&
+             rangeset_overlaps_range(mmio_ro_ranges, msix->pba.first,
+                                     msix->pba.last) )
+        {
+            printk("PBA_shared_quirk is disabled for %04x:%02x:%02x.%u",
+                   seg, bus, slot, func);
+            msix->pba_shared_quirk_enabled = false;
+        }
+
+        if ( !msix->pba_shared_quirk_enabled &&
+             rangeset_add_range(mmio_ro_ranges, msix->pba.first,
                                 msix->pba.last) )
             WARN();
 
@@ -1139,7 +1154,8 @@ static void _pci_cleanup_msix(struct arch_msix *msix)
         if ( rangeset_remove_range(mmio_ro_ranges, msix->table.first,
                                    msix->table.last) )
             WARN();
-        if ( rangeset_remove_range(mmio_ro_ranges, msix->pba.first,
+        if ( !msix->pba_shared_quirk_enabled &&
+             rangeset_remove_range(mmio_ro_ranges, msix->pba.first,
                                    msix->pba.last) )
             WARN();
     }
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 1db69d5..993aa22 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -184,6 +184,39 @@ static int __init parse_phantom_dev(const char *str)
 }
 custom_param("pci-phantom", parse_phantom_dev);
 
+static struct pba_shared_quirk_dev {
+    pci_sbdf_t sbdf;
+} pba_shared_quirk_devs[8];
+static unsigned int nr_pba_shared_quirk_devs;
+
+static int __init parse_pba_shared_quirk(const char *str)
+{
+    for ( ; ; )
+    {
+        unsigned int seg, bus, dev, func;
+
+        if ( nr_pba_shared_quirk_devs >= ARRAY_SIZE(pba_shared_quirk_devs) )
+            return -E2BIG;
+
+        str = parse_pci(str, &seg, &bus, &dev, &func);
+        if ( !str )
+            return -EINVAL;
+
+        pba_shared_quirk_devs[nr_pba_shared_quirk_devs++].sbdf.sbdf =
+                                                PCI_SBDF(seg, bus, dev, func);
+
+        if ( *str == ',' )
+            str++;
+        else if ( !*str )
+            break;
+        else
+            return -EINVAL;
+    }
+
+    return 0;
+}
+custom_param("pba_shared_quirk", parse_pba_shared_quirk);
+
 static u16 __read_mostly command_mask;
 static u16 __read_mostly bridge_ctl_mask;
 
@@ -300,6 +333,7 @@ static void check_pdev(const struct pci_dev *pdev)
 
 static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
 {
+    unsigned int i;
     struct pci_dev *pdev;
 
     list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
@@ -328,6 +362,18 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 
bus, u8 devfn)
         }
         spin_lock_init(&msix->table_lock);
         pdev->msix = msix;
+
+        for ( i = 0; i < nr_pba_shared_quirk_devs; i++ )
+        {
+            if ( pba_shared_quirk_devs[i].sbdf.sbdf !=
+                                            PCI_SBDF3(pseg->nr, bus, devfn) )
+                continue;
+
+            pdev->msix->pba_shared_quirk_enabled = true;
+            printk(XENLOG_WARNING "Enable PBA shared quirk for 
%04x:%02x:%02x.%u\n",
+                   pseg->nr, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
+            break;
+        }
     }
 
     list_add(&pdev->alldevs_list, &pseg->alldevs_list);
@@ -371,8 +417,6 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 
bus, u8 devfn)
             }
             else
             {
-                unsigned int i;
-
                 for ( i = 0; i < nr_phantom_devs; ++i )
                     if ( phantom_devs[i].seg == pseg->nr &&
                          phantom_devs[i].bus == bus &&
diff --git a/xen/include/asm-x86/msi.h b/xen/include/asm-x86/msi.h
index 10387dc..039cce7 100644
--- a/xen/include/asm-x86/msi.h
+++ b/xen/include/asm-x86/msi.h
@@ -239,6 +239,7 @@ struct arch_msix {
     int table_idx[MAX_MSIX_TABLE_PAGES];
     spinlock_t table_lock;
     bool host_maskall, guest_maskall;
+    bool pba_shared_quirk_enabled;
     domid_t warned;
 };
 
-- 
1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel