[Qemu-devel] [PATCH] spapr_pci: map the MSI window in each PHB

[Qemu-devel] [PATCH] spapr_pci: map the MSI window in each PHB

[Greg Kurz]

On sPAPR, virtio devices are connected to the PCI bus and use MSI-X.
Commit cc943c36faa192cd4b32af8fe5edb31894017d35 has modified MSI-X
so that writes are made using the bus master address space.
Unfortunately, this address space does not have a MSI window: the
notification is silently dropped in unassigned_mem_write instead
of reaching the guest... The most visible effect is that all
virtio devices are non-fonctionnal on sPAPR since then. :(

This patch does the following:
1) map the MSI window into the IOMMU address space for each PHB
   - since each PHB instantiates its own IOMMU address space, we
     can safely map the window at a fixed address (SPAPR_PCI_MSI_WINDOW)
   - no real need to keep the MSI window setup in a separate function,
     the spapr_pci_msi_init() code moves to spapr_phb_realize().

2) kill the global MSI window as it is not needed in the end

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c              |    1 -
 hw/ppc/spapr_pci.c          |   53 +++++++++++++++++++------------------------
 include/hw/pci-host/spapr.h |    2 +-
 include/hw/ppc/spapr.h      |    2 --
 4 files changed, 25 insertions(+), 33 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index d01978f..4196a70 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1377,7 +1377,6 @@ static void ppc_spapr_init(MachineState *machine)
     spapr_create_nvram(spapr);
 
     /* Set up PCI */
-    spapr_pci_msi_init(spapr, SPAPR_PCI_MSI_WINDOW);
     spapr_pci_rtas_init();
 
     phb = spapr_create_phb(spapr, 0);
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 9ed39a9..dadba5f 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -341,7 +341,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     }
 
     /* Setup MSI/MSIX vectors in the device (via cfgspace or MSIX BAR) */
-    spapr_msi_setmsg(pdev, spapr->msi_win_addr, ret_intr_type == RTAS_TYPE_MSIX,
+    spapr_msi_setmsg(pdev, SPAPR_PCI_MSI_WINDOW, ret_intr_type == RTAS_TYPE_MSIX,
                      irq, req_num);
 
     /* Add MSI device to cache */
@@ -465,34 +465,6 @@ static const MemoryRegionOps spapr_msi_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN
 };
 
-void spapr_pci_msi_init(sPAPREnvironment *spapr, hwaddr addr)
-{
-    uint64_t window_size = 4096;
-
-    /*
-     * 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().
-     * As MSIMessage:addr is going to be the same and MSIMessage:data
-     * is going to be a VIRQ number, 4 bytes of the MSI MR will only
-     * be used.
-     *
-     * For KVM we want to ensure that this memory is a full page so that
-     * our memory slot is of page size granularity.
-     */
-#ifdef CONFIG_KVM
-    if (kvm_enabled()) {
-        window_size = getpagesize();
-    }
-#endif
-
-    spapr->msi_win_addr = addr;
-    memory_region_init_io(&spapr->msiwindow, NULL, &spapr_msi_ops, spapr,
-                          "msi", window_size);
-    memory_region_add_subregion(get_system_memory(), spapr->msi_win_addr,
-                                &spapr->msiwindow);
-}
-
 /*
  * PHB PCI device
  */
@@ -512,6 +484,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
     char *namebuf;
     int i;
     PCIBus *bus;
+    uint64_t msi_window_size = 4096;
 
     if (sphb->index != -1) {
         hwaddr windows_base;
@@ -604,6 +577,28 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
     address_space_init(&sphb->iommu_as, &sphb->iommu_root,
                        sphb->dtbusname);
 
+    /*
+     * 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().
+     * As MSIMessage:addr is going to be the same and MSIMessage:data
+     * is going to be a VIRQ number, 4 bytes of the MSI MR will only
+     * be used.
+     *
+     * For KVM we want to ensure that this memory is a full page so that
+     * our memory slot is of page size granularity.
+     */
+#ifdef CONFIG_KVM
+    if (kvm_enabled()) {
+        msi_window_size = getpagesize();
+    }
+#endif
+
+    memory_region_init_io(&sphb->msiwindow, NULL, &spapr_msi_ops, spapr,
+                          "msi", msi_window_size);
+    memory_region_add_subregion(&sphb->iommu_root, SPAPR_PCI_MSI_WINDOW,
+                                &sphb->msiwindow);
+
     pci_setup_iommu(bus, spapr_pci_dma_iommu, sphb);
 
     pci_bus_set_route_irq_fn(bus, spapr_route_intx_pin_to_irq);
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 32f0aa7..4ea2a0d 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -70,7 +70,7 @@ struct sPAPRPHBState {
 
     MemoryRegion memspace, iospace;
     hwaddr mem_win_addr, mem_win_size, io_win_addr, io_win_size;
-    MemoryRegion memwindow, iowindow;
+    MemoryRegion memwindow, iowindow, msiwindow;
 
     uint32_t dma_liobn;
     AddressSpace iommu_as;
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index bbba51a..832ad6b 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -13,8 +13,6 @@ struct sPAPRNVRAM;
 typedef struct sPAPREnvironment {
     struct VIOsPAPRBus *vio_bus;
     QLIST_HEAD(, sPAPRPHBState) phbs;
-    hwaddr msi_win_addr;
-    MemoryRegion msiwindow;
     struct sPAPRNVRAM *nvram;
     XICSState *icp;
 

Re: [Qemu-devel] [PATCH] spapr_pci: map the MSI window in each PHB

[Greg Kurz]

On Wed, 27 Aug 2014 18:17:12 +0200
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> On sPAPR, virtio devices are connected to the PCI bus and use MSI-X.
> Commit cc943c36faa192cd4b32af8fe5edb31894017d35 has modified MSI-X
> so that writes are made using the bus master address space.

...and follow the IOMMU path.

> Unfortunately, this address space does not have a MSI window: the

s/this/the IOMMU address space/

Sorry :)

--
Greg

> notification is silently dropped in unassigned_mem_write instead
> of reaching the guest... The most visible effect is that all
> virtio devices are non-fonctionnal on sPAPR since then. :(
> 
> This patch does the following:
> 1) map the MSI window into the IOMMU address space for each PHB
>    - since each PHB instantiates its own IOMMU address space, we
>      can safely map the window at a fixed address (SPAPR_PCI_MSI_WINDOW)
>    - no real need to keep the MSI window setup in a separate function,
>      the spapr_pci_msi_init() code moves to spapr_phb_realize().
> 
> 2) kill the global MSI window as it is not needed in the end
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c              |    1 -
>  hw/ppc/spapr_pci.c          |   53 +++++++++++++++++++------------------------
>  include/hw/pci-host/spapr.h |    2 +-
>  include/hw/ppc/spapr.h      |    2 --
>  4 files changed, 25 insertions(+), 33 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d01978f..4196a70 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1377,7 +1377,6 @@ static void ppc_spapr_init(MachineState *machine)
>      spapr_create_nvram(spapr);
> 
>      /* Set up PCI */
> -    spapr_pci_msi_init(spapr, SPAPR_PCI_MSI_WINDOW);
>      spapr_pci_rtas_init();
> 
>      phb = spapr_create_phb(spapr, 0);
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 9ed39a9..dadba5f 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -341,7 +341,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>      }
> 
>      /* Setup MSI/MSIX vectors in the device (via cfgspace or MSIX BAR) */
> -    spapr_msi_setmsg(pdev, spapr->msi_win_addr, ret_intr_type == RTAS_TYPE_MSIX,
> +    spapr_msi_setmsg(pdev, SPAPR_PCI_MSI_WINDOW, ret_intr_type == RTAS_TYPE_MSIX,
>                       irq, req_num);
> 
>      /* Add MSI device to cache */
> @@ -465,34 +465,6 @@ static const MemoryRegionOps spapr_msi_ops = {
>      .endianness = DEVICE_LITTLE_ENDIAN
>  };
> 
> -void spapr_pci_msi_init(sPAPREnvironment *spapr, hwaddr addr)
> -{
> -    uint64_t window_size = 4096;
> -
> -    /*
> -     * 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().
> -     * As MSIMessage:addr is going to be the same and MSIMessage:data
> -     * is going to be a VIRQ number, 4 bytes of the MSI MR will only
> -     * be used.
> -     *
> -     * For KVM we want to ensure that this memory is a full page so that
> -     * our memory slot is of page size granularity.
> -     */
> -#ifdef CONFIG_KVM
> -    if (kvm_enabled()) {
> -        window_size = getpagesize();
> -    }
> -#endif
> -
> -    spapr->msi_win_addr = addr;
> -    memory_region_init_io(&spapr->msiwindow, NULL, &spapr_msi_ops, spapr,
> -                          "msi", window_size);
> -    memory_region_add_subregion(get_system_memory(), spapr->msi_win_addr,
> -                                &spapr->msiwindow);
> -}
> -
>  /*
>   * PHB PCI device
>   */
> @@ -512,6 +484,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>      char *namebuf;
>      int i;
>      PCIBus *bus;
> +    uint64_t msi_window_size = 4096;
> 
>      if (sphb->index != -1) {
>          hwaddr windows_base;
> @@ -604,6 +577,28 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>      address_space_init(&sphb->iommu_as, &sphb->iommu_root,
>                         sphb->dtbusname);
> 
> +    /*
> +     * 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().
> +     * As MSIMessage:addr is going to be the same and MSIMessage:data
> +     * is going to be a VIRQ number, 4 bytes of the MSI MR will only
> +     * be used.
> +     *
> +     * For KVM we want to ensure that this memory is a full page so that
> +     * our memory slot is of page size granularity.
> +     */
> +#ifdef CONFIG_KVM
> +    if (kvm_enabled()) {
> +        msi_window_size = getpagesize();
> +    }
> +#endif
> +
> +    memory_region_init_io(&sphb->msiwindow, NULL, &spapr_msi_ops, spapr,
> +                          "msi", msi_window_size);
> +    memory_region_add_subregion(&sphb->iommu_root, SPAPR_PCI_MSI_WINDOW,
> +                                &sphb->msiwindow);
> +
>      pci_setup_iommu(bus, spapr_pci_dma_iommu, sphb);
> 
>      pci_bus_set_route_irq_fn(bus, spapr_route_intx_pin_to_irq);
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index 32f0aa7..4ea2a0d 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -70,7 +70,7 @@ struct sPAPRPHBState {
> 
>      MemoryRegion memspace, iospace;
>      hwaddr mem_win_addr, mem_win_size, io_win_addr, io_win_size;
> -    MemoryRegion memwindow, iowindow;
> +    MemoryRegion memwindow, iowindow, msiwindow;
> 
>      uint32_t dma_liobn;
>      AddressSpace iommu_as;
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index bbba51a..832ad6b 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -13,8 +13,6 @@ struct sPAPRNVRAM;
>  typedef struct sPAPREnvironment {
>      struct VIOsPAPRBus *vio_bus;
>      QLIST_HEAD(, sPAPRPHBState) phbs;
> -    hwaddr msi_win_addr;
> -    MemoryRegion msiwindow;
>      struct sPAPRNVRAM *nvram;
>      XICSState *icp;
> 
> 
> 



-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

Re: [Qemu-devel] [PATCH] spapr_pci: map the MSI window in each PHB

[Alexander Graf]

On 27.08.14 19:10, Greg Kurz wrote:
> On Wed, 27 Aug 2014 18:17:12 +0200
> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> 
>> On sPAPR, virtio devices are connected to the PCI bus and use MSI-X.
>> Commit cc943c36faa192cd4b32af8fe5edb31894017d35 has modified MSI-X
>> so that writes are made using the bus master address space.
> 
> ...and follow the IOMMU path.
> 
>> Unfortunately, this address space does not have a MSI window: the
> 
> s/this/the IOMMU address space/
> 
> Sorry :)

Thanks, applied to ppc-next with fixed commit message.


Alex