[Qemu-devel] [PATCH 1/3] spapr pci msi: rework

[Alexey Kardashevskiy]

Previously every PCI host bridge implemented its own MSI memory window
in order to catch msi_notify()/msix_notify() calls from various QEMU
MSI-capable devives such as virtio-pci or vfio and redirect them to
the guest via qemu_pulse_irq().

The encoded MSIMessage used to be encoded as:
* .addr - address in a MSI window, this is how QEMU knows which PHB
is the message for;
* .data - number of a device on a specific PHB and vector number.

As a PHB has a destriptor for every device, and every descriptor has
first IRQ number and the number of IRQs, it can calculate global IRQ
number to use in qemu_pulse_irq().

However the total number of IRQs is not really big (at the moment it is
1024 IRQs which start from 4096) and the existing system looks overdesigned.
The patch simplifies it. Specifically:

1. MSI windows were removed from PHB.
2. Added one memory region for all MSIs.
3. Now MSIMessage::addr is a number of first IRQ of a device,
MSIMessage:data is a number of a vector.

Putting IRQ number to .data and not using .addr would make it even simpler
for MSI-X but it will not work for MSI with multiple vectors unless a first
IRQ number of a device is aligned to the MSI vectors number.

The simplified scheme also allows easier MSIMessage->IRQ translation
for upcoming IRQFD support.

Signed-off-by: Alexey Kardashevskiy <address@hidden>
---
 hw/ppc/spapr.c              |    2 +
 hw/ppc/spapr_pci.c          |   97 ++++++++++++++++++++-----------------------
 include/hw/pci-host/spapr.h |    8 ++--
 include/hw/ppc/spapr.h      |    2 +
 4 files changed, 53 insertions(+), 56 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b18e99b..5660dba 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1243,6 +1243,8 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
 
     /* Set up PCI */
     spapr_pci_rtas_init();
+    spapr_pci_msi_window_init(spapr, SPAPR_PCI_MSI_WINDOW,
+                              XICS_IRQ_BASE + XICS_IRQS);
 
     phb = spapr_create_phb(spapr, 0);
 
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 3666c8b..cb8596b 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -257,30 +257,6 @@ static int spapr_msicfg_find(sPAPRPHBState *phb, uint32_t 
config_addr,
     return -1;
 }
 
-/*
- * Set MSI/MSIX message data.
- * This is required for msi_notify()/msix_notify() which
- * will write at the addresses via spapr_msi_write().
- */
-static void spapr_msi_setmsg(PCIDevice *pdev, hwaddr addr,
-                             bool msix, unsigned req_num)
-{
-    unsigned i;
-    MSIMessage msg = { .address = addr, .data = 0 };
-
-    if (!msix) {
-        msi_set_message(pdev, msg);
-        trace_spapr_pci_msi_setup(pdev->name, 0, msg.address);
-        return;
-    }
-
-    for (i = 0; i < req_num; ++i) {
-        msg.address = addr | (i << 2);
-        msix_set_message(pdev, i, msg);
-        trace_spapr_pci_msi_setup(pdev->name, i, msg.address);
-    }
-}
-
 static void rtas_ibm_change_msi(sPAPREnvironment *spapr,
                                 uint32_t token, uint32_t nargs,
                                 target_ulong args, uint32_t nret,
@@ -292,9 +268,10 @@ static void rtas_ibm_change_msi(sPAPREnvironment *spapr,
     unsigned int req_num = rtas_ld(args, 4); /* 0 == remove all */
     unsigned int seq_num = rtas_ld(args, 5);
     unsigned int ret_intr_type;
-    int ndev, irq;
+    int i, ndev, irq;
     sPAPRPHBState *phb = NULL;
     PCIDevice *pdev = NULL;
+    MSIMessage msg;
 
     switch (func) {
     case RTAS_CHANGE_MSI_FN:
@@ -366,9 +343,29 @@ static void rtas_ibm_change_msi(sPAPREnvironment *spapr,
         phb->msi_table[ndev].config_addr = config_addr;
     }
 
-    /* Setup MSI/MSIX vectors in the device (via cfgspace or MSIX BAR) */
-    spapr_msi_setmsg(pdev, phb->msi_win_addr | (ndev << 16),
-                     ret_intr_type == RTAS_TYPE_MSIX, req_num);
+    /*
+     * Set MSI/MSIX message data.
+     * This is required for msi_notify()/msix_notify() which
+     * will write at the addresses via spapr_msi_write().
+     *
+     * The MSI address is an IRQ number lshifted by 2 in order to align
+     * it to 32bit.
+     * The MSI data is:
+     * MSI: zero for MSI as the caller is expected to pass the vector number 
there
+     * MSIX: the vector number (to do the same what MSI does)
+     */
+    msg.address = spapr->msi_win_addr + (phb->msi_table[ndev].irq << 2);
+    msg.data = 0;
+    if (ret_intr_type == RTAS_TYPE_MSI) {
+        msi_set_message(pdev, msg);
+        trace_spapr_pci_msi_setup(pdev->name, 0, msg.address);
+    } else {
+        for (i = 0; i < phb->msi_table[ndev].nvec; ++i) {
+            msg.data = i;
+            msix_set_message(pdev, i, msg);
+            trace_spapr_pci_msi_setup(pdev->name, i, msg.address);
+        }
+    }
 
     rtas_st(rets, 0, 0);
     rtas_st(rets, 1, req_num);
@@ -490,10 +487,7 @@ static const MemoryRegionOps spapr_io_ops = {
 static void spapr_msi_write(void *opaque, hwaddr addr,
                             uint64_t data, unsigned size)
 {
-    sPAPRPHBState *phb = opaque;
-    int ndev = addr >> 16;
-    int vec = ((addr & 0xFFFF) >> 2) | data;
-    uint32_t irq = phb->msi_table[ndev].irq + vec;
+    uint32_t irq = (addr >> 2) + data;
 
     trace_spapr_pci_msi_write(addr, data, irq);
 
@@ -507,6 +501,23 @@ static const MemoryRegionOps spapr_msi_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN
 };
 
+void spapr_pci_msi_window_init(sPAPREnvironment *spapr, hwaddr addr, int nvec)
+{
+    if (!msi_supported) {
+        return;
+    }
+    /*
+     * As MSI/MSIX interrupts trigger by writing at MSI/MSIX vectors,
+     * we need to allocate some memory to catch those writes coming
+     * from msi_notify()/msix_notify().
+     */
+    spapr->msi_win_addr = addr;
+    memory_region_init_io(&spapr->msiwindow, &spapr_msi_ops, spapr,
+                          "msi", nvec << 2);
+    memory_region_add_subregion(get_system_memory(), spapr->msi_win_addr,
+                                &spapr->msiwindow);
+}
+
 /*
  * PHB PCI device
  */
@@ -535,8 +546,7 @@ static int _spapr_phb_init(SysBusDevice *s)
 
         if ((sphb->buid != -1) || (sphb->dma_liobn != -1)
             || (sphb->mem_win_addr != -1)
-            || (sphb->io_win_addr != -1)
-            || (sphb->msi_win_addr != -1)) {
+            || (sphb->io_win_addr != -1)) {
             fprintf(stderr, "Either \"index\" or other parameters must"
                     " be specified for PAPR PHB, not both\n");
             return -1;
@@ -549,7 +559,6 @@ static int _spapr_phb_init(SysBusDevice *s)
             + sphb->index * SPAPR_PCI_WINDOW_SPACING;
         sphb->mem_win_addr = windows_base + SPAPR_PCI_MMIO_WIN_OFF;
         sphb->io_win_addr = windows_base + SPAPR_PCI_IO_WIN_OFF;
-        sphb->msi_win_addr = windows_base + SPAPR_PCI_MSI_WIN_OFF;
     }
 
     if (sphb->buid == -1) {
@@ -572,11 +581,6 @@ static int _spapr_phb_init(SysBusDevice *s)
         return -1;
     }
 
-    if (sphb->msi_win_addr == -1) {
-        fprintf(stderr, "MSI window address not specified for PHB\n");
-        return -1;
-    }
-
     if (find_phb(spapr, sphb->buid)) {
         fprintf(stderr, "PCI host bridges must have unique BUIDs\n");
         return -1;
@@ -615,17 +619,6 @@ static int _spapr_phb_init(SysBusDevice *s)
     memory_region_add_subregion(get_system_memory(), sphb->io_win_addr,
                                 &sphb->iowindow);
 
-    /* As MSI/MSIX interrupts trigger by writing at MSI/MSIX vectors,
-     * we need to allocate some memory to catch those writes coming
-     * from msi_notify()/msix_notify() */
-    if (msi_supported) {
-        sprintf(namebuf, "%s.msi", sphb->dtbusname);
-        memory_region_init_io(&sphb->msiwindow, &spapr_msi_ops, sphb,
-                              namebuf, SPAPR_MSIX_MAX_DEVS * 0x10000);
-        memory_region_add_subregion(get_system_memory(), sphb->msi_win_addr,
-                                    &sphb->msiwindow);
-    }
-
     /*
      * Selecting a busname is more complex than you'd think, due to
      * interacting constraints.  If the user has specified an id
@@ -710,7 +703,6 @@ static Property spapr_phb_properties[] = {
     DEFINE_PROP_HEX64("io_win_addr", sPAPRPHBState, io_win_addr, -1),
     DEFINE_PROP_HEX64("io_win_size", sPAPRPHBState, io_win_size,
                       SPAPR_PCI_IO_WIN_SIZE),
-    DEFINE_PROP_HEX64("msi_win_addr", sPAPRPHBState, msi_win_addr, -1),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -760,7 +752,6 @@ static const VMStateDescription vmstate_spapr_pci = {
         VMSTATE_UINT64_EQUAL(mem_win_size, sPAPRPHBState),
         VMSTATE_UINT64_EQUAL(io_win_addr, sPAPRPHBState),
         VMSTATE_UINT64_EQUAL(io_win_size, sPAPRPHBState),
-        VMSTATE_UINT64_EQUAL(msi_win_addr, sPAPRPHBState),
         VMSTATE_STRUCT_ARRAY(lsi_table, sPAPRPHBState, PCI_NUM_PINS, 0,
                              vmstate_spapr_pci_lsi, struct spapr_pci_lsi),
         VMSTATE_STRUCT_ARRAY(msi_table, sPAPRPHBState, SPAPR_MSIX_MAX_DEVS, 0,
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 398dec5..cf1b22e 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -47,8 +47,7 @@ typedef struct sPAPRPHBState {
 
     MemoryRegion memspace, iospace;
     hwaddr mem_win_addr, mem_win_size, io_win_addr, io_win_size;
-    hwaddr msi_win_addr;
-    MemoryRegion memwindow, iowindow, msiwindow;
+    MemoryRegion memwindow, iowindow;
 
     uint32_t dma_liobn;
     uint64_t dma_window_start;
@@ -85,7 +84,8 @@ typedef struct sPAPRPHBVFIOState {
 #define SPAPR_PCI_MMIO_WIN_SIZE      0x20000000
 #define SPAPR_PCI_IO_WIN_OFF         0x80000000
 #define SPAPR_PCI_IO_WIN_SIZE        0x10000
-#define SPAPR_PCI_MSI_WIN_OFF        0x90000000
+
+#define SPAPR_PCI_MSI_WINDOW         0x20000000000ULL
 
 #define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
 
@@ -100,6 +100,8 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
                           uint32_t xics_phandle,
                           void *fdt);
 
+void spapr_pci_msi_window_init(sPAPREnvironment *spapr, hwaddr addr, int nvec);
+
 void spapr_pci_rtas_init(void);
 
 #endif /* __HW_SPAPR_PCI_H__ */
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 0dc005c..598c0e9 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -14,6 +14,8 @@ struct icp_state;
 typedef struct sPAPREnvironment {
     struct VIOsPAPRBus *vio_bus;
     QLIST_HEAD(, sPAPRPHBState) phbs;
+    hwaddr msi_win_addr;
+    MemoryRegion msiwindow;
     struct sPAPRNVRAM *nvram;
     struct icp_state *icp;
 
-- 
1.7.10.4