| Message ID | 20180118034732.10439-1-aik@ozlabs.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18/01/18 14:47, Alexey Kardashevskiy wrote: > At the moment the sPAPR PHB MMIO space does not have an address space > object as it does not really need one - guest accesses it via virtual > addresses (and we provide mappings to the CPU space), the device drivers > in QEMU access MMIO directly (as they own MRs). The only case when > a driver in QEMU might need an address space (AS) is DMA so we provide one > with only IOMMUs on it. > > So PHB AS looks like: > address-space: pci@800000020000000 > 0000000000000000-ffffffffffffffff (prio 0, i/o): pci@800000020000000.iommu-root > 0000000000000000-ffffffffffffffff (prio 0, i/o): tce-root-80000001 > 0800000000000000-080000007fffffff (prio 0, i/o): tce-iommu-80000001 > 0000000000000000-ffffffffffffffff (prio 0, i/o): tce-root-80000000 > 0000000000000000-000000003fffffff (prio 0, i/o): tce-iommu-80000000 > 0000040000000000-000004000000ffff (prio 0, i/o): msi > > with no mention of any PCI device, and so does the flatview: > > FlatView #2 > AS "pci@800000020000000", root: pci@800000020000000.iommu-root > AS "nec-usb-xhci", root: bus master container > Root memory region: pci@800000020000000.iommu-root > 0000000000000000-000000003fffffff (prio 0, i/o): tce-iommu-80000000 > > This adds 2 more aliases (for MMIO32 and MMIO64 windows) on PHB AS so > whatever appears on the bus can be seen in "info mtree [-f]". > > Here are the chunks from "diff" for a QEMU with an emulater XHCI adapter: > > address-space: pci@800000020000000 > 0000000000000000-ffffffffffffffff (prio 0, i/o): pci@800000020000000.iommu-root > 0000000000000000-ffffffffffffffff (prio 0, i/o): tce-root-80000001 > 0800000000000000-080000007fffffff (prio 0, i/o): tce-iommu-80000001 > 0000000000000000-ffffffffffffffff (prio 0, i/o): tce-root-80000000 > 0000000000000000-000000003fffffff (prio 0, i/o): tce-iommu-80000000 > + 0000000080000000-00000000ffffffff (prio 0, i/o): alias pci@800000020000000.mmio32-alias-pci @pci@800000020000000.mmio3 > +2-alias 0000000000000000-000000007fffffff > 0000040000000000-000004000000ffff (prio 0, i/o): msi > + 0000210000000000-000021ffffffffff (prio 0, i/o): alias pci@800000020000000.mmio64-alias-pci @pci@800000020000000.mmio6 > +4-alias 0000000000000000-000000ffffffffff > > > FlatView #2 > AS "pci@800000020000000", root: pci@800000020000000.iommu-root > AS "nec-usb-xhci", root: bus master container > Root memory region: pci@800000020000000.iommu-root > 0000000000000000-000000003fffffff (prio 0, i/o): tce-iommu-80000000 > + 0000000080000000-000000008000003f (prio 0, i/o): capabilities > + 0000000080000040-000000008000043f (prio 0, i/o): operational > + 0000000080000440-000000008000044f (prio 0, i/o): usb3 port #1 > + 0000000080000450-000000008000045f (prio 0, i/o): usb3 port #2 > + 0000000080000460-000000008000046f (prio 0, i/o): usb3 port #3 > + 0000000080000470-000000008000047f (prio 0, i/o): usb3 port #4 > + 0000000080000480-000000008000048f (prio 0, i/o): usb2 port #1 > + 0000000080000490-000000008000049f (prio 0, i/o): usb2 port #2 > + 00000000800004a0-00000000800004af (prio 0, i/o): usb2 port #3 > + 00000000800004b0-00000000800004bf (prio 0, i/o): usb2 port #4 > + 0000000080001000-000000008000121f (prio 0, i/o): runtime > + 0000000080002000-000000008000281f (prio 0, i/o): doorbell > + 0000000080003000-00000000800030ff (prio 0, i/o): msix-table > + 0000000080003800-0000000080003807 (prio 0, i/o): msix-pba > 0000040000000000-000004000000ffff (prio 0, i/o): msi > 0800000000000000-080000007fffffff (prio 0, i/o): tce-iommu-80000001 > > The major point for this is to help debugging overlapped regions. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > > Should not we do this in the common code really? By this I meant not literally map memory space from pci_register_root_bus() (as it is 64bit wide, subregions are the windows) but still do something to see devices on a PCI AS. Now PHB does not have an AS on my x86 setup. and ... > Does this look useful at all? I was quite confused when I realized that > I do not see an AS with DMA windows and MMIO while they co-exist > on a bus. > > > --- > include/hw/pci-host/spapr.h | 1 + > hw/ppc/spapr_pci.c | 12 ++++++++++++ > 2 files changed, 13 insertions(+) > > diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h > index 0fae4fc..4550192 100644 > --- a/include/hw/pci-host/spapr.h > +++ b/include/hw/pci-host/spapr.h > @@ -57,6 +57,7 @@ struct sPAPRPHBState { > uint64_t mem64_win_pciaddr; > hwaddr io_win_addr, io_win_size; > MemoryRegion mem32window, mem64window, iowindow, msiwindow; > + MemoryRegion mem32windowpci, mem64windowpci; > > uint32_t dma_liobn[SPAPR_PCI_DMA_MAX_WINDOWS]; > hwaddr dma_win_addr, dma_win_size; > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index 37f18b3..d142f74 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -1642,6 +1642,18 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) > address_space_init(&sphb->iommu_as, &sphb->iommu_root, > sphb->dtbusname); > > + /* Map memory space onto the PHB address space */ > + namebuf = g_strdup_printf("%s.mmio32-alias-pci", sphb->dtbusname); > + memory_region_init_alias(&sphb->mem32windowpci, OBJECT(sphb), namebuf, > + &sphb->mem32window, 0, sphb->mem_win_size); > + memory_region_add_subregion(&sphb->iommu_root, SPAPR_PCI_MEM_WIN_BUS_OFFSET, > + &sphb->mem32windowpci); > + namebuf = g_strdup_printf("%s.mmio64-alias-pci", sphb->dtbusname); > + memory_region_init_alias(&sphb->mem64windowpci, OBJECT(sphb), namebuf, > + &sphb->mem64window, 0, sphb->mem64_win_size); > + memory_region_add_subregion(&sphb->iommu_root, sphb->mem64_win_pciaddr, > + &sphb->mem64windowpci); ... I just noticed - this leaks @namebuf memory, I'll fix it in v2 if no other comment arrive. > + > /* > * As MSI/MSIX interrupts trigger by writing at MSI/MSIX vectors, > * we need to allocate some memory to catch those writes coming >
On 18/01/18 16:33, Alexey Kardashevskiy wrote: > On 18/01/18 14:47, Alexey Kardashevskiy wrote: >> At the moment the sPAPR PHB MMIO space does not have an address space >> object as it does not really need one - guest accesses it via virtual >> addresses (and we provide mappings to the CPU space), the device drivers >> in QEMU access MMIO directly (as they own MRs). The only case when >> a driver in QEMU might need an address space (AS) is DMA so we provide one >> with only IOMMUs on it. >> >> So PHB AS looks like: >> address-space: pci@800000020000000 >> 0000000000000000-ffffffffffffffff (prio 0, i/o): pci@800000020000000.iommu-root >> 0000000000000000-ffffffffffffffff (prio 0, i/o): tce-root-80000001 >> 0800000000000000-080000007fffffff (prio 0, i/o): tce-iommu-80000001 >> 0000000000000000-ffffffffffffffff (prio 0, i/o): tce-root-80000000 >> 0000000000000000-000000003fffffff (prio 0, i/o): tce-iommu-80000000 >> 0000040000000000-000004000000ffff (prio 0, i/o): msi >> >> with no mention of any PCI device, and so does the flatview: >> >> FlatView #2 >> AS "pci@800000020000000", root: pci@800000020000000.iommu-root >> AS "nec-usb-xhci", root: bus master container >> Root memory region: pci@800000020000000.iommu-root >> 0000000000000000-000000003fffffff (prio 0, i/o): tce-iommu-80000000 >> >> This adds 2 more aliases (for MMIO32 and MMIO64 windows) on PHB AS so >> whatever appears on the bus can be seen in "info mtree [-f]". >> >> Here are the chunks from "diff" for a QEMU with an emulater XHCI adapter: >> >> address-space: pci@800000020000000 >> 0000000000000000-ffffffffffffffff (prio 0, i/o): pci@800000020000000.iommu-root >> 0000000000000000-ffffffffffffffff (prio 0, i/o): tce-root-80000001 >> 0800000000000000-080000007fffffff (prio 0, i/o): tce-iommu-80000001 >> 0000000000000000-ffffffffffffffff (prio 0, i/o): tce-root-80000000 >> 0000000000000000-000000003fffffff (prio 0, i/o): tce-iommu-80000000 >> + 0000000080000000-00000000ffffffff (prio 0, i/o): alias pci@800000020000000.mmio32-alias-pci @pci@800000020000000.mmio3 >> +2-alias 0000000000000000-000000007fffffff >> 0000040000000000-000004000000ffff (prio 0, i/o): msi >> + 0000210000000000-000021ffffffffff (prio 0, i/o): alias pci@800000020000000.mmio64-alias-pci @pci@800000020000000.mmio6 >> +4-alias 0000000000000000-000000ffffffffff >> >> >> FlatView #2 >> AS "pci@800000020000000", root: pci@800000020000000.iommu-root >> AS "nec-usb-xhci", root: bus master container >> Root memory region: pci@800000020000000.iommu-root >> 0000000000000000-000000003fffffff (prio 0, i/o): tce-iommu-80000000 >> + 0000000080000000-000000008000003f (prio 0, i/o): capabilities >> + 0000000080000040-000000008000043f (prio 0, i/o): operational >> + 0000000080000440-000000008000044f (prio 0, i/o): usb3 port #1 >> + 0000000080000450-000000008000045f (prio 0, i/o): usb3 port #2 >> + 0000000080000460-000000008000046f (prio 0, i/o): usb3 port #3 >> + 0000000080000470-000000008000047f (prio 0, i/o): usb3 port #4 >> + 0000000080000480-000000008000048f (prio 0, i/o): usb2 port #1 >> + 0000000080000490-000000008000049f (prio 0, i/o): usb2 port #2 >> + 00000000800004a0-00000000800004af (prio 0, i/o): usb2 port #3 >> + 00000000800004b0-00000000800004bf (prio 0, i/o): usb2 port #4 >> + 0000000080001000-000000008000121f (prio 0, i/o): runtime >> + 0000000080002000-000000008000281f (prio 0, i/o): doorbell >> + 0000000080003000-00000000800030ff (prio 0, i/o): msix-table >> + 0000000080003800-0000000080003807 (prio 0, i/o): msix-pba >> 0000040000000000-000004000000ffff (prio 0, i/o): msi >> 0800000000000000-080000007fffffff (prio 0, i/o): tce-iommu-80000001 Things are actually worse, with this applied + VFIO, an MMIO/RAM MR "0001:01:00.3 BAR 0 mmaps[0]" appears on "pci@800000020000000": FlatView #2 AS "pci@800000020000000", root: pci@800000020000000.iommu-root AS "vfio-pci", root: bus master container Root memory region: pci@800000020000000.iommu-root 0000000000000000-000000003fffffff (prio 0, i/o): tce-iommu-80000000 0000040000000000-000004000000ffff (prio 0, i/o): msi 0000210000000000-0000210000001fff (prio 0, i/o): 0001:01:00.3 BAR 0 0000210000002000-00002100000020bf (prio 0, i/o): msix-table 00002100000020c0-000021000000ffff (prio 0, i/o): 0001:01:00.3 BAR 0 @00000000000020c0 0000210000010000-0000210001ffffff (prio 0, ramd): 0001:01:00.3 BAR 0 mmaps[0] 0800000000000000-080000007fffffff (prio 0, i/o): tce-iommu-80000001 vfio_listener_region_add() tries creating a window as it does only check the container type (was sufficient so far). So I am putting this on hold but I still wonder if the while idea makes sense. Thanks. Alex, I did not cc: you as I believed it is a spapr business but turns out this affects how we proceed with vfio_listener_skipped_section() and vfio_listener_region_add() as not hw_error-ing on vfio_dma_map() if memory_region_is_ram_device() won't be enough on spapr. >> >> The major point for this is to help debugging overlapped regions. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- >> >> Should not we do this in the common code really? > > > By this I meant not literally map memory space from pci_register_root_bus() > (as it is 64bit wide, subregions are the windows) but still do something to > see devices on a PCI AS. Now PHB does not have an AS on my x86 setup. > > > and ... > > > >> Does this look useful at all? I was quite confused when I realized that >> I do not see an AS with DMA windows and MMIO while they co-exist >> on a bus. >> >> >> --- >> include/hw/pci-host/spapr.h | 1 + >> hw/ppc/spapr_pci.c | 12 ++++++++++++ >> 2 files changed, 13 insertions(+) >> >> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h >> index 0fae4fc..4550192 100644 >> --- a/include/hw/pci-host/spapr.h >> +++ b/include/hw/pci-host/spapr.h >> @@ -57,6 +57,7 @@ struct sPAPRPHBState { >> uint64_t mem64_win_pciaddr; >> hwaddr io_win_addr, io_win_size; >> MemoryRegion mem32window, mem64window, iowindow, msiwindow; >> + MemoryRegion mem32windowpci, mem64windowpci; >> >> uint32_t dma_liobn[SPAPR_PCI_DMA_MAX_WINDOWS]; >> hwaddr dma_win_addr, dma_win_size; >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >> index 37f18b3..d142f74 100644 >> --- a/hw/ppc/spapr_pci.c >> +++ b/hw/ppc/spapr_pci.c >> @@ -1642,6 +1642,18 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) >> address_space_init(&sphb->iommu_as, &sphb->iommu_root, >> sphb->dtbusname); >> >> + /* Map memory space onto the PHB address space */ >> + namebuf = g_strdup_printf("%s.mmio32-alias-pci", sphb->dtbusname); >> + memory_region_init_alias(&sphb->mem32windowpci, OBJECT(sphb), namebuf, >> + &sphb->mem32window, 0, sphb->mem_win_size); >> + memory_region_add_subregion(&sphb->iommu_root, SPAPR_PCI_MEM_WIN_BUS_OFFSET, >> + &sphb->mem32windowpci); >> + namebuf = g_strdup_printf("%s.mmio64-alias-pci", sphb->dtbusname); >> + memory_region_init_alias(&sphb->mem64windowpci, OBJECT(sphb), namebuf, >> + &sphb->mem64window, 0, sphb->mem64_win_size); >> + memory_region_add_subregion(&sphb->iommu_root, sphb->mem64_win_pciaddr, >> + &sphb->mem64windowpci); > > > ... I just noticed - this leaks @namebuf memory, I'll fix it in v2 if no > other comment arrive. > > >> + >> /* >> * As MSI/MSIX interrupts trigger by writing at MSI/MSIX vectors, >> * we need to allocate some memory to catch those writes coming >> > >
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h index 0fae4fc..4550192 100644 --- a/include/hw/pci-host/spapr.h +++ b/include/hw/pci-host/spapr.h @@ -57,6 +57,7 @@ struct sPAPRPHBState { uint64_t mem64_win_pciaddr; hwaddr io_win_addr, io_win_size; MemoryRegion mem32window, mem64window, iowindow, msiwindow; + MemoryRegion mem32windowpci, mem64windowpci; uint32_t dma_liobn[SPAPR_PCI_DMA_MAX_WINDOWS]; hwaddr dma_win_addr, dma_win_size; diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index 37f18b3..d142f74 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -1642,6 +1642,18 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) address_space_init(&sphb->iommu_as, &sphb->iommu_root, sphb->dtbusname); + /* Map memory space onto the PHB address space */ + namebuf = g_strdup_printf("%s.mmio32-alias-pci", sphb->dtbusname); + memory_region_init_alias(&sphb->mem32windowpci, OBJECT(sphb), namebuf, + &sphb->mem32window, 0, sphb->mem_win_size); + memory_region_add_subregion(&sphb->iommu_root, SPAPR_PCI_MEM_WIN_BUS_OFFSET, + &sphb->mem32windowpci); + namebuf = g_strdup_printf("%s.mmio64-alias-pci", sphb->dtbusname); + memory_region_init_alias(&sphb->mem64windowpci, OBJECT(sphb), namebuf, + &sphb->mem64window, 0, sphb->mem64_win_size); + memory_region_add_subregion(&sphb->iommu_root, sphb->mem64_win_pciaddr, + &sphb->mem64windowpci); + /* * As MSI/MSIX interrupts trigger by writing at MSI/MSIX vectors, * we need to allocate some memory to catch those writes coming
At the moment the sPAPR PHB MMIO space does not have an address space object as it does not really need one - guest accesses it via virtual addresses (and we provide mappings to the CPU space), the device drivers in QEMU access MMIO directly (as they own MRs). The only case when a driver in QEMU might need an address space (AS) is DMA so we provide one with only IOMMUs on it. So PHB AS looks like: address-space: pci@800000020000000 0000000000000000-ffffffffffffffff (prio 0, i/o): pci@800000020000000.iommu-root 0000000000000000-ffffffffffffffff (prio 0, i/o): tce-root-80000001 0800000000000000-080000007fffffff (prio 0, i/o): tce-iommu-80000001 0000000000000000-ffffffffffffffff (prio 0, i/o): tce-root-80000000 0000000000000000-000000003fffffff (prio 0, i/o): tce-iommu-80000000 0000040000000000-000004000000ffff (prio 0, i/o): msi with no mention of any PCI device, and so does the flatview: FlatView #2 AS "pci@800000020000000", root: pci@800000020000000.iommu-root AS "nec-usb-xhci", root: bus master container Root memory region: pci@800000020000000.iommu-root 0000000000000000-000000003fffffff (prio 0, i/o): tce-iommu-80000000 This adds 2 more aliases (for MMIO32 and MMIO64 windows) on PHB AS so whatever appears on the bus can be seen in "info mtree [-f]". Here are the chunks from "diff" for a QEMU with an emulater XHCI adapter: address-space: pci@800000020000000 0000000000000000-ffffffffffffffff (prio 0, i/o): pci@800000020000000.iommu-root 0000000000000000-ffffffffffffffff (prio 0, i/o): tce-root-80000001 0800000000000000-080000007fffffff (prio 0, i/o): tce-iommu-80000001 0000000000000000-ffffffffffffffff (prio 0, i/o): tce-root-80000000 0000000000000000-000000003fffffff (prio 0, i/o): tce-iommu-80000000 + 0000000080000000-00000000ffffffff (prio 0, i/o): alias pci@800000020000000.mmio32-alias-pci @pci@800000020000000.mmio3 +2-alias 0000000000000000-000000007fffffff 0000040000000000-000004000000ffff (prio 0, i/o): msi + 0000210000000000-000021ffffffffff (prio 0, i/o): alias pci@800000020000000.mmio64-alias-pci @pci@800000020000000.mmio6 +4-alias 0000000000000000-000000ffffffffff FlatView #2 AS "pci@800000020000000", root: pci@800000020000000.iommu-root AS "nec-usb-xhci", root: bus master container Root memory region: pci@800000020000000.iommu-root 0000000000000000-000000003fffffff (prio 0, i/o): tce-iommu-80000000 + 0000000080000000-000000008000003f (prio 0, i/o): capabilities + 0000000080000040-000000008000043f (prio 0, i/o): operational + 0000000080000440-000000008000044f (prio 0, i/o): usb3 port #1 + 0000000080000450-000000008000045f (prio 0, i/o): usb3 port #2 + 0000000080000460-000000008000046f (prio 0, i/o): usb3 port #3 + 0000000080000470-000000008000047f (prio 0, i/o): usb3 port #4 + 0000000080000480-000000008000048f (prio 0, i/o): usb2 port #1 + 0000000080000490-000000008000049f (prio 0, i/o): usb2 port #2 + 00000000800004a0-00000000800004af (prio 0, i/o): usb2 port #3 + 00000000800004b0-00000000800004bf (prio 0, i/o): usb2 port #4 + 0000000080001000-000000008000121f (prio 0, i/o): runtime + 0000000080002000-000000008000281f (prio 0, i/o): doorbell + 0000000080003000-00000000800030ff (prio 0, i/o): msix-table + 0000000080003800-0000000080003807 (prio 0, i/o): msix-pba 0000040000000000-000004000000ffff (prio 0, i/o): msi 0800000000000000-080000007fffffff (prio 0, i/o): tce-iommu-80000001 The major point for this is to help debugging overlapped regions. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- Should not we do this in the common code really? Does this look useful at all? I was quite confused when I realized that I do not see an AS with DMA windows and MMIO while they co-exist on a bus. --- include/hw/pci-host/spapr.h | 1 + hw/ppc/spapr_pci.c | 12 ++++++++++++ 2 files changed, 13 insertions(+)