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 <address@hidden> 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 <address@hidden>
> ---
>  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                                     address@hidden
                                                 address@hidden
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

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