[PATCH v8 01/17] hw/vfio/pci: Ensure MSI and MSI-X do not overlap

From: Akihiko Odaki <akihiko.odaki@daynix.com>
Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org,
	qemu-arm@nongnu.org, "Michael S . Tsirkin" <mst@redhat.com>,
	"Marcel Apfelbaum" <marcel.apfelbaum@gmail.com>,
	"Gerd Hoffmann" <kraxel@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Richard Henderson" <richard.henderson@linaro.org>,
	"Eduardo Habkost" <eduardo@habkost.net>,
	"John Snow" <jsnow@redhat.com>,
	"Dmitry Fleytman" <dmitry.fleytman@gmail.com>,
	"Jason Wang" <jasowang@redhat.com>,
	"Stefan Weil" <sw@weilnetz.de>, "Keith Busch" <kbusch@kernel.org>,
	"Klaus Jensen" <its@irrelevant.dk>,
	"Peter Maydell" <peter.maydell@linaro.org>,
	"Andrey Smirnov" <andrew.smirnov@gmail.com>,
	"Paul Burton" <paulburton@kernel.org>,
	"Aleksandar Rikalo" <aleksandar.rikalo@syrmia.com>,
	"Yan Vugenfirer" <yan@daynix.com>,
	"Yuri Benditovich" <yuri.benditovich@daynix.com>,
	"Philippe Mathieu-Daudé" <philmd@linaro.org>,
	"Akihiko Odaki" <akihiko.odaki@daynix.com>
Subject: [PATCH v8 01/17] hw/vfio/pci: Ensure MSI and MSI-X do not overlap
Date: Tue,  1 Nov 2022 22:57:33 +0900	[thread overview]
Message-ID: <20221101135749.4477-2-akihiko.odaki@daynix.com> (raw)
In-Reply-To: <20221101135749.4477-1-akihiko.odaki@daynix.com>

pci_add_capability() checks whether capabilities overlap, and notifies
its caller so that it can properly handle the case. However, in the
most cases, the capabilities actually never overlap, and the interface
incurred extra error handling code, which is often incorrect or
suboptimal. For such cases, pci_add_capability() can simply abort the
execution if the capabilities actually overlap since it should be a
programming error.

This change handles the other cases: hw/vfio/pci depends on the check to
decide MSI and MSI-X capabilities overlap with another. As they are
quite an exceptional and hw/vfio/pci knows much about PCI capabilities,
adding code specific to the cases to hw/vfio/pci still results in less
code than having error handling code everywhere in total.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/pci/pci.c         | 34 ++++++++++++++++++++++------------
 hw/vfio/pci.c        | 15 ++++++++++++++-
 include/hw/pci/pci.h |  3 +++
 3 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 2f450f6a72..b53649d1fd 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2512,6 +2512,25 @@ static void pci_del_option_rom(PCIDevice *pdev)
     pdev->has_rom = false;
 }
 
+bool pci_check_capability_overlap(PCIDevice *pdev, uint8_t cap_id,
+                                  uint8_t offset, uint8_t size, Error **errp)
+{
+    int i;
+
+    for (i = offset; i < offset + size; i++) {
+        if (pdev->used[i]) {
+            error_setg(errp,
+                       "%s:%02x:%02x.%x PCI capability %x at offset %x overlaps existing capability %x at offset %x",
+                       pci_root_bus_path(pdev), pci_dev_bus_num(pdev),
+                       PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn),
+                       cap_id, offset, pci_find_capability_at_offset(pdev, i), i);
+            return true;
+        }
+    }
+
+    return false;
+}
+
 /*
  * On success, pci_add_capability() returns a positive value
  * that the offset of the pci capability.
@@ -2523,7 +2542,6 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
                        Error **errp)
 {
     uint8_t *config;
-    int i, overlapping_cap;
 
     if (!offset) {
         offset = pci_find_space(pdev, size);
@@ -2534,17 +2552,9 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
          * depends on this check to verify that the device is not broken.
          * Should never trigger for emulated devices, but it's helpful
          * for debugging these. */
-        for (i = offset; i < offset + size; i++) {
-            overlapping_cap = pci_find_capability_at_offset(pdev, i);
-            if (overlapping_cap) {
-                error_setg(errp, "%s:%02x:%02x.%x "
-                           "Attempt to add PCI capability %x at offset "
-                           "%x overlaps existing capability %x at offset %x",
-                           pci_root_bus_path(pdev), pci_dev_bus_num(pdev),
-                           PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn),
-                           cap_id, offset, overlapping_cap, i);
-                return -EINVAL;
-            }
+        pci_check_capability_overlap(pdev, cap_id, offset, size, errp);
+        if (errp) {
+            return -EINVAL;
         }
     }
 
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 939dcc3d4a..0ca6b5ff4b 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1298,6 +1298,14 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
 
     trace_vfio_msi_setup(vdev->vbasedev.name, pos);
 
+    vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 0);
+
+    ret = pci_check_capability_overlap(&vdev->pdev, PCI_CAP_ID_MSI,
+                                       pos, vdev->msi_cap_size, errp);
+    if (!ret) {
+        return -EINVAL;
+    }
+
     ret = msi_init(&vdev->pdev, pos, entries, msi_64bit, msi_maskbit, &err);
     if (ret < 0) {
         if (ret == -ENOTSUP) {
@@ -1306,7 +1314,6 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
         error_propagate_prepend(errp, err, "msi_init failed: ");
         return ret;
     }
-    vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 0);
 
     return 0;
 }
@@ -1575,6 +1582,12 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
     int ret;
     Error *err = NULL;
 
+    ret = pci_check_capability_overlap(&vdev->pdev, PCI_CAP_ID_MSIX,
+                                       pos, MSIX_CAP_LENGTH, errp);
+    if (!ret) {
+        return -EINVAL;
+    }
+
     vdev->msix->pending = g_new0(unsigned long,
                                  BITS_TO_LONGS(vdev->msix->entries));
     ret = msix_init(&vdev->pdev, vdev->msix->entries,
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index b54b6ef88f..77b264c17e 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -390,6 +390,9 @@ void pci_register_vga(PCIDevice *pci_dev, MemoryRegion *mem,
 void pci_unregister_vga(PCIDevice *pci_dev);
 pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int region_num);
 
+bool pci_check_capability_overlap(PCIDevice *pdev, uint8_t cap_id,
+                                  uint8_t offset, uint8_t size, Error **errp);
+
 int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
                        uint8_t offset, uint8_t size,
                        Error **errp);
-- 
2.38.1


