[Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Alexey Kardashevskiy]

As it looks like all portio users have migrated to new portio api,
the workaround with memory access to io ports routing is no more
needed.

This also fixes a bug with byte swapping as the io region was marked
as little endian while it should not do any swapping at all.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/ppc/spapr_pci.c | 52 +++-------------------------------------------------
 1 file changed, 3 insertions(+), 49 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index ca588aa..8d76290 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -440,43 +440,6 @@ static void pci_spapr_set_irq(void *opaque, int irq_num, int level)
     qemu_set_irq(spapr_phb_lsi_qirq(phb, irq_num), level);
 }
 
-static uint64_t spapr_io_read(void *opaque, hwaddr addr,
-                              unsigned size)
-{
-    switch (size) {
-    case 1:
-        return cpu_inb(addr);
-    case 2:
-        return cpu_inw(addr);
-    case 4:
-        return cpu_inl(addr);
-    }
-    g_assert_not_reached();
-}
-
-static void spapr_io_write(void *opaque, hwaddr addr,
-                           uint64_t data, unsigned size)
-{
-    switch (size) {
-    case 1:
-        cpu_outb(addr, data);
-        return;
-    case 2:
-        cpu_outw(addr, data);
-        return;
-    case 4:
-        cpu_outl(addr, data);
-        return;
-    }
-    g_assert_not_reached();
-}
-
-static const MemoryRegionOps spapr_io_ops = {
-    .endianness = DEVICE_LITTLE_ENDIAN,
-    .read = spapr_io_read,
-    .write = spapr_io_write
-};
-
 /*
  * MSI/MSIX memory region implementation.
  * The handler handles both MSI and MSIX.
@@ -590,23 +553,14 @@ static int spapr_phb_init(SysBusDevice *s)
     memory_region_add_subregion(get_system_memory(), sphb->mem_win_addr,
                                 &sphb->memwindow);
 
-    /* On ppc, we only have MMIO no specific IO space from the CPU
-     * perspective.  In theory we ought to be able to embed the PCI IO
-     * memory region direction in the system memory space.  However,
-     * if any of the IO BAR subregions use the old_portio mechanism,
-     * that won't be processed properly unless accessed from the
-     * system io address space.  This hack to bounce things via
-     * system_io works around the problem until all the users of
-     * old_portion are updated */
+    /* Initialize IO regions */
     sprintf(namebuf, "%s.io", sphb->dtbusname);
     memory_region_init(&sphb->iospace, OBJECT(sphb),
                        namebuf, SPAPR_PCI_IO_WIN_SIZE);
-    /* FIXME: fix to support multiple PHBs */
-    memory_region_add_subregion(get_system_io(), 0, &sphb->iospace);
 
     sprintf(namebuf, "%s.io-alias", sphb->dtbusname);
-    memory_region_init_io(&sphb->iowindow, OBJECT(sphb), &spapr_io_ops, sphb,
-                          namebuf, SPAPR_PCI_IO_WIN_SIZE);
+    memory_region_init_alias(&sphb->iowindow, OBJECT(sphb),
+                             namebuf, &sphb->iospace, 0, SPAPR_PCI_IO_WIN_SIZE);
     memory_region_add_subregion(get_system_memory(), sphb->io_win_addr,
                                 &sphb->iowindow);
 
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Alexander Graf]

On 15.07.2013, at 05:24, Alexey Kardashevskiy wrote:

> As it looks like all portio users have migrated to new portio api,
> the workaround with memory access to io ports routing is no more
> needed.
> 
> This also fixes a bug with byte swapping as the io region was marked
> as little endian while it should not do any swapping at all.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

This patch breaks VGA with -enable-kvm (PR) for me.


Alex

Re: [Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Alexey Kardashevskiy]

On 07/15/2013 04:06 PM, Alexander Graf wrote:
> 
> On 15.07.2013, at 05:24, Alexey Kardashevskiy wrote:
> 
>> As it looks like all portio users have migrated to new portio api,
>> the workaround with memory access to io ports routing is no more
>> needed.
>>
>> This also fixes a bug with byte swapping as the io region was marked
>> as little endian while it should not do any swapping at all.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> This patch breaks VGA with -enable-kvm (PR) for me.


I wish I could understand the memory region thingy but I cannot :-(

Without this patch, portio_list_add from vga_init adds a port to
address_space_io. With my patch, it adds a port to address_space_memory and
then port accesses cannot reach correct memoryregionops callbacks and VGA
does not work.

Networking (rtl8139, e1000, virtio) still works but in contrast to VGA,
those devices map PCI IO BARs but not individual ports.


Any idea?
Or my initial patch (where I just changed endianness from LITTLE to NATIVE)
was still correct and this is how thing are supposed to work?
Oh...


This is what I get with my patch:

info mtree
memory
0000000000000000-7ffffffffffffffe (prio 0, RW): system
  0000000000000000-000000003fffffff (prio 0, RW): ppc_spapr.ram
  0000010080000000-000001008000ffff (prio 0, RW): alias
pci@800000020000000.io-alias @pci@800000020000000.io
0000000000000000-000000000000ffff
  00000100a0000000-00000100bfffffff (prio 0, RW): alias
pci@800000020000000.mmio-alias @pci@800000020000000.mmio
0000000080000000-000000009fffffff
  0000040000000000-000004000000ffff (prio 0, RW): msi
I/O
0000000000000000-000000000000ffff (prio 0, RW): io
pci@800000020000000
0000000000000000-7fffffffffffffff (prio 0, RW): iommu-spapr
l-lan@71000002
0000000000000000-7fffffffffffffff (prio 0, RW): iommu-spapr
v-scsi@71000003
0000000000000000-7fffffffffffffff (prio 0, RW): iommu-spapr
VGA
0000000000000000-7fffffffffffffff (prio 0, RW): alias bus master
@iommu-spapr 0000000000000000-7fffffffffffffff
pci-ohci
0000000000000000-7fffffffffffffff (prio 0, RW): alias bus master
@iommu-spapr 0000000000000000-7fffffffffffffff
aliases
pci@800000020000000.io
0000000000000000-000000000000ffff (prio 0, RW): pci@800000020000000.io
  00000000000001ce-00000000000001ce (prio 0, RW): vbe
  00000000000001d0-00000000000001d0 (prio 0, RW): vbe
  00000000000003b4-00000000000003b5 (prio 0, RW): vga
  00000000000003ba-00000000000003ba (prio 0, RW): vga
  00000000000003c0-00000000000003cf (prio 0, RW): vga
  00000000000003d4-00000000000003d5 (prio 0, RW): vga
  00000000000003da-00000000000003da (prio 0, RW): vga
pci@800000020000000.mmio
0000000000000000-7ffffffffffffffe (prio 0, RW): pci@800000020000000.mmio
  00000000000a0000-00000000000bffff (prio 1, RW): vga-lowmem
  0000000080000000-0000000080ffffff (prio 1, RW): vga.vram
  0000000090000000-0000000090000fff (prio 1, RW): vga.mmio
    0000000090000400-000000009000041f (prio 0, RW): vga ioports remapped
    0000000090000500-0000000090000515 (prio 0, RW): bochs dispi interface
  0000000090010000-000000009001ffff (prio 1, RW): vga.rom
  0000000090020000-00000000900200ff (prio 1, RW): ohci
iommu-spapr
0000000000000000-7fffffffffffffff (prio 0, RW): iommu-spapr



And this is what I get without the patch (when VGA works). btw here
mtree_print_mr asserts in int128_get64(), had to replace assert with "if
(a.hi) return -1;"


root@erif_root:~# (qemu) info mtree
memory
0000000000000000-7ffffffffffffffe (prio 0, RW): system
  0000000000000000-000000003fffffff (prio 0, RW): ppc_spapr.ram
  0000010080000000-000001008000ffff (prio 0, RW): pci@800000020000000.io-alias
  0000010090000000-00000100901fffff (prio 0, RW): pci@800000020000000.msi
  00000100a0000000-00000100bfffffff (prio 0, RW): alias
pci@800000020000000.mmio-alias @pci@800000020000000.mmio
0000000080000000-000000009fffffff
I/O
0000000000000000-000000000000ffff (prio 0, RW): io
  0000000000000000-000000000000ffff (prio 0, RW): pci@800000020000000.io
    00000000000001ce-00000000000001ce (prio 0, RW): vbe
    00000000000001d0-00000000000001d0 (prio 0, RW): vbe
    00000000000003b4-00000000000003b5 (prio 0, RW): vga
    00000000000003ba-00000000000003ba (prio 0, RW): vga
    00000000000003c0-00000000000003cf (prio 0, RW): vga
    00000000000003d4-00000000000003d5 (prio 0, RW): vga
    00000000000003da-00000000000003da (prio 0, RW): vga
pci@800000020000000
0000000000000000-ffffffffffffffff (prio 0, RW): iommu-spapr
l-lan@71000002
0000000000000000-ffffffffffffffff (prio 0, RW): iommu-spapr
v-scsi@71000003
0000000000000000-ffffffffffffffff (prio 0, RW): iommu-spapr
VGA
0000000000000000-ffffffffffffffff (prio 0, RW): alias bus master
@iommu-spapr 0000000000000000-fffffffffffffffe
pci-ohci
0000000000000000-ffffffffffffffff (prio 0, RW): alias bus master
@iommu-spapr 0000000000000000-fffffffffffffffe
aliases
pci@800000020000000.mmio
0000000000000000-7ffffffffffffffe (prio 0, RW): pci@800000020000000.mmio
  00000000000a0000-00000000000affff (prio 2, RW): alias vga.chain4
@vga.vram 0000000000000000-000000000000ffff
  00000000000a0000-00000000000bffff (prio 1, RW): vga-lowmem
  0000000080000000-0000000080ffffff (prio 1, RW): vga.vram
  0000000090000000-0000000090000fff (prio 1, RW): vga.mmio
    0000000090000400-000000009000041f (prio 0, RW): vga ioports remapped
    0000000090000500-0000000090000515 (prio 0, RW): bochs dispi interface
  0000000090020000-00000000900200ff (prio 1, RW): ohci
iommu-spapr
0000000000000000-ffffffffffffffff (prio 0, RW): iommu-spapr
vga.vram
0000000080000000-0000000080ffffff (prio 1, RW): vga.vram


My command line:

./qemu-system-ppc64 \
 -L "qemu-ppc64-bios/" \
 -trace "events=qemu_trace_events" \
 -vnc :5 \
 -M pseries \
 -serial mon:stdio \
 -enable-kvm \
 -m 1G \
 -machine "pseries" \
 -vga "std" \
 -kernel "guest.vmlinux.n" \
 -initrd "1.cpio"


-- 
Alexey

[Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Alexey Kardashevskiy]

In the past, IO space could not be mapped into the memory address space
so we introduced a workaround for that. Nowadays it does not look
necessary so we can remove the workaround and make sPAPR PCI
configuration simplier.

This also removes all byte swappings as it is not PHB's to take care
of endiannes - devices should do convertion if they want to. And almost
every PCI device which uses IO ports does that by registering IO ports
region with memory_region_init_io() (Intel e1000, RTL8139, virtio).

However VGA uses MemoryRegionPortio which does not support endiannes
but it still expects the convertion to be done. For this case only,
this patch adds LITTLE_ENDIAN flag to portio_ops. Tests on PPC64 show
that other devices are not affected by this change. x86 systems should
not suffer either as having LITTLE_ENDIAN there has no effect.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---

This removes bugs at least from SPAPR so any further fixes should be equal
for all platforms and hopefully will break all platform altogether but not
just PPC64-pSeries :)

Did I miss anything here?


---
 hw/ppc/spapr_pci.c | 52 +++-------------------------------------------------
 ioport.c           |  1 +
 2 files changed, 4 insertions(+), 49 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index ca588aa..8d76290 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -440,43 +440,6 @@ static void pci_spapr_set_irq(void *opaque, int irq_num, int level)
     qemu_set_irq(spapr_phb_lsi_qirq(phb, irq_num), level);
 }
 
-static uint64_t spapr_io_read(void *opaque, hwaddr addr,
-                              unsigned size)
-{
-    switch (size) {
-    case 1:
-        return cpu_inb(addr);
-    case 2:
-        return cpu_inw(addr);
-    case 4:
-        return cpu_inl(addr);
-    }
-    g_assert_not_reached();
-}
-
-static void spapr_io_write(void *opaque, hwaddr addr,
-                           uint64_t data, unsigned size)
-{
-    switch (size) {
-    case 1:
-        cpu_outb(addr, data);
-        return;
-    case 2:
-        cpu_outw(addr, data);
-        return;
-    case 4:
-        cpu_outl(addr, data);
-        return;
-    }
-    g_assert_not_reached();
-}
-
-static const MemoryRegionOps spapr_io_ops = {
-    .endianness = DEVICE_LITTLE_ENDIAN,
-    .read = spapr_io_read,
-    .write = spapr_io_write
-};
-
 /*
  * MSI/MSIX memory region implementation.
  * The handler handles both MSI and MSIX.
@@ -590,23 +553,14 @@ static int spapr_phb_init(SysBusDevice *s)
     memory_region_add_subregion(get_system_memory(), sphb->mem_win_addr,
                                 &sphb->memwindow);
 
-    /* On ppc, we only have MMIO no specific IO space from the CPU
-     * perspective.  In theory we ought to be able to embed the PCI IO
-     * memory region direction in the system memory space.  However,
-     * if any of the IO BAR subregions use the old_portio mechanism,
-     * that won't be processed properly unless accessed from the
-     * system io address space.  This hack to bounce things via
-     * system_io works around the problem until all the users of
-     * old_portion are updated */
+    /* Initialize IO regions */
     sprintf(namebuf, "%s.io", sphb->dtbusname);
     memory_region_init(&sphb->iospace, OBJECT(sphb),
                        namebuf, SPAPR_PCI_IO_WIN_SIZE);
-    /* FIXME: fix to support multiple PHBs */
-    memory_region_add_subregion(get_system_io(), 0, &sphb->iospace);
 
     sprintf(namebuf, "%s.io-alias", sphb->dtbusname);
-    memory_region_init_io(&sphb->iowindow, OBJECT(sphb), &spapr_io_ops, sphb,
-                          namebuf, SPAPR_PCI_IO_WIN_SIZE);
+    memory_region_init_alias(&sphb->iowindow, OBJECT(sphb),
+                             namebuf, &sphb->iospace, 0, SPAPR_PCI_IO_WIN_SIZE);
     memory_region_add_subregion(get_system_memory(), sphb->io_win_addr,
                                 &sphb->iowindow);
 
diff --git a/ioport.c b/ioport.c
index 89b17d6..79b7f1a 100644
--- a/ioport.c
+++ b/ioport.c
@@ -183,6 +183,7 @@ static void portio_write(void *opaque, hwaddr addr, uint64_t data,
 static const MemoryRegionOps portio_ops = {
     .read = portio_read,
     .write = portio_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
     .valid.unaligned = true,
     .impl.unaligned = true,
 };
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Paolo Bonzini]

Il 16/07/2013 07:19, Alexey Kardashevskiy ha scritto:
> In the past, IO space could not be mapped into the memory address space
> so we introduced a workaround for that. Nowadays it does not look
> necessary so we can remove the workaround and make sPAPR PCI
> configuration simplier.
> 
> This also removes all byte swappings as it is not PHB's to take care
> of endiannes - devices should do convertion if they want to. And almost
> every PCI device which uses IO ports does that by registering IO ports
> region with memory_region_init_io() (Intel e1000, RTL8139, virtio).
> 
> However VGA uses MemoryRegionPortio which does not support endiannes
> but it still expects the convertion to be done. For this case only,
> this patch adds LITTLE_ENDIAN flag to portio_ops. Tests on PPC64 show
> that other devices are not affected by this change. x86 systems should
> not suffer either as having LITTLE_ENDIAN there has no effect.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> 
> This removes bugs at least from SPAPR so any further fixes should be equal
> for all platforms and hopefully will break all platform altogether but not
> just PPC64-pSeries :)
> 
> Did I miss anything here?

No, I don't think so.  The patch looks good.

Paolo

> 
> ---
>  hw/ppc/spapr_pci.c | 52 +++-------------------------------------------------
>  ioport.c           |  1 +
>  2 files changed, 4 insertions(+), 49 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index ca588aa..8d76290 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -440,43 +440,6 @@ static void pci_spapr_set_irq(void *opaque, int irq_num, int level)
>      qemu_set_irq(spapr_phb_lsi_qirq(phb, irq_num), level);
>  }
>  
> -static uint64_t spapr_io_read(void *opaque, hwaddr addr,
> -                              unsigned size)
> -{
> -    switch (size) {
> -    case 1:
> -        return cpu_inb(addr);
> -    case 2:
> -        return cpu_inw(addr);
> -    case 4:
> -        return cpu_inl(addr);
> -    }
> -    g_assert_not_reached();
> -}
> -
> -static void spapr_io_write(void *opaque, hwaddr addr,
> -                           uint64_t data, unsigned size)
> -{
> -    switch (size) {
> -    case 1:
> -        cpu_outb(addr, data);
> -        return;
> -    case 2:
> -        cpu_outw(addr, data);
> -        return;
> -    case 4:
> -        cpu_outl(addr, data);
> -        return;
> -    }
> -    g_assert_not_reached();
> -}
> -
> -static const MemoryRegionOps spapr_io_ops = {
> -    .endianness = DEVICE_LITTLE_ENDIAN,
> -    .read = spapr_io_read,
> -    .write = spapr_io_write
> -};
> -
>  /*
>   * MSI/MSIX memory region implementation.
>   * The handler handles both MSI and MSIX.
> @@ -590,23 +553,14 @@ static int spapr_phb_init(SysBusDevice *s)
>      memory_region_add_subregion(get_system_memory(), sphb->mem_win_addr,
>                                  &sphb->memwindow);
>  
> -    /* On ppc, we only have MMIO no specific IO space from the CPU
> -     * perspective.  In theory we ought to be able to embed the PCI IO
> -     * memory region direction in the system memory space.  However,
> -     * if any of the IO BAR subregions use the old_portio mechanism,
> -     * that won't be processed properly unless accessed from the
> -     * system io address space.  This hack to bounce things via
> -     * system_io works around the problem until all the users of
> -     * old_portion are updated */
> +    /* Initialize IO regions */
>      sprintf(namebuf, "%s.io", sphb->dtbusname);
>      memory_region_init(&sphb->iospace, OBJECT(sphb),
>                         namebuf, SPAPR_PCI_IO_WIN_SIZE);
> -    /* FIXME: fix to support multiple PHBs */
> -    memory_region_add_subregion(get_system_io(), 0, &sphb->iospace);
>  
>      sprintf(namebuf, "%s.io-alias", sphb->dtbusname);
> -    memory_region_init_io(&sphb->iowindow, OBJECT(sphb), &spapr_io_ops, sphb,
> -                          namebuf, SPAPR_PCI_IO_WIN_SIZE);
> +    memory_region_init_alias(&sphb->iowindow, OBJECT(sphb),
> +                             namebuf, &sphb->iospace, 0, SPAPR_PCI_IO_WIN_SIZE);
>      memory_region_add_subregion(get_system_memory(), sphb->io_win_addr,
>                                  &sphb->iowindow);
>  
> diff --git a/ioport.c b/ioport.c
> index 89b17d6..79b7f1a 100644
> --- a/ioport.c
> +++ b/ioport.c
> @@ -183,6 +183,7 @@ static void portio_write(void *opaque, hwaddr addr, uint64_t data,
>  static const MemoryRegionOps portio_ops = {
>      .read = portio_read,
>      .write = portio_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
>      .valid.unaligned = true,
>      .impl.unaligned = true,
>  };
> 

Re: [Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Alexander Graf]

Am 16.07.2013 um 08:20 schrieb Paolo Bonzini <pbonzini@redhat.com>:

> Il 16/07/2013 07:19, Alexey Kardashevskiy ha scritto:
>> In the past, IO space could not be mapped into the memory address space
>> so we introduced a workaround for that. Nowadays it does not look
>> necessary so we can remove the workaround and make sPAPR PCI
>> configuration simplier.
>> 
>> This also removes all byte swappings as it is not PHB's to take care
>> of endiannes - devices should do convertion if they want to. And almost
>> every PCI device which uses IO ports does that by registering IO ports
>> region with memory_region_init_io() (Intel e1000, RTL8139, virtio).
>> 
>> However VGA uses MemoryRegionPortio which does not support endiannes
>> but it still expects the convertion to be done. For this case only,
>> this patch adds LITTLE_ENDIAN flag to portio_ops. Tests on PPC64 show
>> that other devices are not affected by this change. x86 systems should
>> not suffer either as having LITTLE_ENDIAN there has no effect.
>> 
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> 
>> This removes bugs at least from SPAPR so any further fixes should be equal
>> for all platforms and hopefully will break all platform altogether but not
>> just PPC64-pSeries :)
>> 
>> Did I miss anything here?
> 
> No, I don't think so.  The patch looks good.

... and will break all Mac targets again, no? Not to speak of non-ppc devices.

Alex

> 
> Paolo
> 
>> 
>> ---
>> hw/ppc/spapr_pci.c | 52 +++-------------------------------------------------
>> ioport.c           |  1 +
>> 2 files changed, 4 insertions(+), 49 deletions(-)
>> 
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index ca588aa..8d76290 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -440,43 +440,6 @@ static void pci_spapr_set_irq(void *opaque, int irq_num, int level)
>>     qemu_set_irq(spapr_phb_lsi_qirq(phb, irq_num), level);
>> }
>> 
>> -static uint64_t spapr_io_read(void *opaque, hwaddr addr,
>> -                              unsigned size)
>> -{
>> -    switch (size) {
>> -    case 1:
>> -        return cpu_inb(addr);
>> -    case 2:
>> -        return cpu_inw(addr);
>> -    case 4:
>> -        return cpu_inl(addr);
>> -    }
>> -    g_assert_not_reached();
>> -}
>> -
>> -static void spapr_io_write(void *opaque, hwaddr addr,
>> -                           uint64_t data, unsigned size)
>> -{
>> -    switch (size) {
>> -    case 1:
>> -        cpu_outb(addr, data);
>> -        return;
>> -    case 2:
>> -        cpu_outw(addr, data);
>> -        return;
>> -    case 4:
>> -        cpu_outl(addr, data);
>> -        return;
>> -    }
>> -    g_assert_not_reached();
>> -}
>> -
>> -static const MemoryRegionOps spapr_io_ops = {
>> -    .endianness = DEVICE_LITTLE_ENDIAN,
>> -    .read = spapr_io_read,
>> -    .write = spapr_io_write
>> -};
>> -
>> /*
>>  * MSI/MSIX memory region implementation.
>>  * The handler handles both MSI and MSIX.
>> @@ -590,23 +553,14 @@ static int spapr_phb_init(SysBusDevice *s)
>>     memory_region_add_subregion(get_system_memory(), sphb->mem_win_addr,
>>                                 &sphb->memwindow);
>> 
>> -    /* On ppc, we only have MMIO no specific IO space from the CPU
>> -     * perspective.  In theory we ought to be able to embed the PCI IO
>> -     * memory region direction in the system memory space.  However,
>> -     * if any of the IO BAR subregions use the old_portio mechanism,
>> -     * that won't be processed properly unless accessed from the
>> -     * system io address space.  This hack to bounce things via
>> -     * system_io works around the problem until all the users of
>> -     * old_portion are updated */
>> +    /* Initialize IO regions */
>>     sprintf(namebuf, "%s.io", sphb->dtbusname);
>>     memory_region_init(&sphb->iospace, OBJECT(sphb),
>>                        namebuf, SPAPR_PCI_IO_WIN_SIZE);
>> -    /* FIXME: fix to support multiple PHBs */
>> -    memory_region_add_subregion(get_system_io(), 0, &sphb->iospace);
>> 
>>     sprintf(namebuf, "%s.io-alias", sphb->dtbusname);
>> -    memory_region_init_io(&sphb->iowindow, OBJECT(sphb), &spapr_io_ops, sphb,
>> -                          namebuf, SPAPR_PCI_IO_WIN_SIZE);
>> +    memory_region_init_alias(&sphb->iowindow, OBJECT(sphb),
>> +                             namebuf, &sphb->iospace, 0, SPAPR_PCI_IO_WIN_SIZE);
>>     memory_region_add_subregion(get_system_memory(), sphb->io_win_addr,
>>                                 &sphb->iowindow);
>> 
>> diff --git a/ioport.c b/ioport.c
>> index 89b17d6..79b7f1a 100644
>> --- a/ioport.c
>> +++ b/ioport.c
>> @@ -183,6 +183,7 @@ static void portio_write(void *opaque, hwaddr addr, uint64_t data,
>> static const MemoryRegionOps portio_ops = {
>>     .read = portio_read,
>>     .write = portio_write,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>>     .valid.unaligned = true,
>>     .impl.unaligned = true,
>> };
> 

Re: [Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Alexey Kardashevskiy]

On 07/16/2013 06:32 PM, Alexander Graf wrote:
> 
> 
> Am 16.07.2013 um 08:20 schrieb Paolo Bonzini <pbonzini@redhat.com>:
> 
>> Il 16/07/2013 07:19, Alexey Kardashevskiy ha scritto:
>>> In the past, IO space could not be mapped into the memory address space
>>> so we introduced a workaround for that. Nowadays it does not look
>>> necessary so we can remove the workaround and make sPAPR PCI
>>> configuration simplier.
>>>
>>> This also removes all byte swappings as it is not PHB's to take care
>>> of endiannes - devices should do convertion if they want to. And almost
>>> every PCI device which uses IO ports does that by registering IO ports
>>> region with memory_region_init_io() (Intel e1000, RTL8139, virtio).
>>>
>>> However VGA uses MemoryRegionPortio which does not support endiannes
>>> but it still expects the convertion to be done. For this case only,
>>> this patch adds LITTLE_ENDIAN flag to portio_ops. Tests on PPC64 show
>>> that other devices are not affected by this change. x86 systems should
>>> not suffer either as having LITTLE_ENDIAN there has no effect.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>>
>>> This removes bugs at least from SPAPR so any further fixes should be equal
>>> for all platforms and hopefully will break all platform altogether but not
>>> just PPC64-pSeries :)
>>>
>>> Did I miss anything here?
>>
>> No, I don't think so.  The patch looks good.
> 
> ... and will break all Mac targets again, no? Not to speak of non-ppc devices.


Like what? I do not mind/argue/discuss, may be it breaks, I am just looking
for good examples to test.



-- 
Alexey

Re: [Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Alexander Graf]

Am 16.07.2013 um 10:37 schrieb Alexey Kardashevskiy <aik@ozlabs.ru>:

> On 07/16/2013 06:32 PM, Alexander Graf wrote:
>> 
>> 
>> Am 16.07.2013 um 08:20 schrieb Paolo Bonzini <pbonzini@redhat.com>:
>> 
>>> Il 16/07/2013 07:19, Alexey Kardashevskiy ha scritto:
>>>> In the past, IO space could not be mapped into the memory address space
>>>> so we introduced a workaround for that. Nowadays it does not look
>>>> necessary so we can remove the workaround and make sPAPR PCI
>>>> configuration simplier.
>>>> 
>>>> This also removes all byte swappings as it is not PHB's to take care
>>>> of endiannes - devices should do convertion if they want to. And almost
>>>> every PCI device which uses IO ports does that by registering IO ports
>>>> region with memory_region_init_io() (Intel e1000, RTL8139, virtio).
>>>> 
>>>> However VGA uses MemoryRegionPortio which does not support endiannes
>>>> but it still expects the convertion to be done. For this case only,
>>>> this patch adds LITTLE_ENDIAN flag to portio_ops. Tests on PPC64 show
>>>> that other devices are not affected by this change. x86 systems should
>>>> not suffer either as having LITTLE_ENDIAN there has no effect.
>>>> 
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> ---
>>>> 
>>>> This removes bugs at least from SPAPR so any further fixes should be equal
>>>> for all platforms and hopefully will break all platform altogether but not
>>>> just PPC64-pSeries :)
>>>> 
>>>> Did I miss anything here?
>>> 
>>> No, I don't think so.  The patch looks good.
>> 
>> ... and will break all Mac targets again, no? Not to speak of non-ppc devices.
> 
> 
> Like what? I do not mind/argue/discuss, may be it breaks, I am just looking
> for good examples to test.

How about you start with

$ qemu-system-ppc

Then? It shouldn't show a prompt with your patch ;)


Alex

> 
> 
> 
> -- 
> Alexey

Re: [Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Benjamin Herrenschmidt]

On Tue, 2013-07-16 at 10:32 +0200, Alexander Graf wrote:
> >> Did I miss anything here?
> > 
> > No, I don't think so.  The patch looks good.
> 
> ... and will break all Mac targets again, no? Not to speak of non-ppc
> devices.

Do you know why it breaks ? Can you suggest what's wrong and how to fix
it maybe ?

Cheers,
Ben,

Re: [Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Alexander Graf]

Am 16.07.2013 um 10:45 schrieb Benjamin Herrenschmidt <benh@kernel.crashing.org>:

> On Tue, 2013-07-16 at 10:32 +0200, Alexander Graf wrote:
>>>> Did I miss anything here?
>>> 
>>> No, I don't think so.  The patch looks good.
>> 
>> ... and will break all Mac targets again, no? Not to speak of non-ppc
>> devices.
> 
> Do you know why it breaks ? Can you suggest what's wrong and how to fix
> it maybe ?

Sure. Get rid of cpu_inX and cpu_outX completely and verify that the conversion is doing the "right thing" for each conversion. Some of them are really tricky - like MIPS which accesses the RTC in native endian (big) in current code. So this patch would definitely break that one too.

I'm actually surprised we have to mark the ioport region little endian for sPAPR. We already did have it converted to a direct memory api alias a while ago and IIRC the only problem back then were the old_portio callbacks. I wonder what changed in between.


Alex

> 
> Cheers,
> Ben,
> 
> 

Re: [Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Alexey Kardashevskiy]

On 07/16/2013 07:01 PM, Alexander Graf wrote:
> 
> 
> Am 16.07.2013 um 10:45 schrieb Benjamin Herrenschmidt
> <benh@kernel.crashing.org>:
> 
>> On Tue, 2013-07-16 at 10:32 +0200, Alexander Graf wrote:
>>>>> Did I miss anything here?
>>>> 
>>>> No, I don't think so.  The patch looks good.
>>> 
>>> ... and will break all Mac targets again, no? Not to speak of
>>> non-ppc devices.
>> 
>> Do you know why it breaks ? Can you suggest what's wrong and how to
>> fix it maybe ?
> 
> Sure. Get rid of cpu_inX and cpu_outX completely and verify that the
> conversion is doing the "right thing" for each conversion. Some of them
> are really tricky - like MIPS which accesses the RTC in native endian
> (big) in current code. So this patch would definitely break that one
> too.
> 

> I'm actually surprised we have to mark the ioport region little endian
> for sPAPR.


It is not for spapr, it is for devices which expect some endianness and do
not do convertions.


> We already did have it converted to a direct memory api alias
> a while ago and IIRC the only problem back then were the old_portio
> callbacks. I wonder what changed in between.


? We have had the IO memory region hack I am trying to remove for a long time.


-- 
Alexey

[Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Greg Kurz]

In the past, IO space could not be mapped into the memory address space
so we introduced a workaround for that. Nowadays it does not look
necessary so we can remove the workaround and make sPAPR PCI
configuration simplier.

This workaround has also an evil side effect with virtio devices: because
all PHBs have their .io region at the same address, the devices
get mapped in the .io-alias region of every PHB (AKA. mapped multiple times).
This breaks the ioeventfd feature and causes qemu to abort() when running
with KVM and asking for more than one PHB:

$ qemu-system-ppc64 -machine type=pseries,accel=kvm -smp 1 -m 4G \
  -hda /local/greg/images/fedora-be.qcow2 \
  -device virtio-9p-pci,fsdev=fsdev0,mount_tag=share,bus=pci,ioeventfd=on \
  -fsdev local,security_model=none,id=fsdev0,path=$HOME/share1 \
  -device spapr-pci-host-bridge,index=15
kvm_mem_ioeventfd_add: error adding ioeventfd: File exists
Aborted

This will prevent to use virtio and VFIO passthrough at the same time, since
VFIO needs a dedicated PHB to work on ppc.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c |   13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 7763149..7d29431 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -564,23 +564,14 @@ static int spapr_phb_init(SysBusDevice *s)
     memory_region_add_subregion(get_system_memory(), sphb->mem_win_addr,
                                 &sphb->memwindow);
 
-    /* On ppc, we only have MMIO no specific IO space from the CPU
-     * perspective.  In theory we ought to be able to embed the PCI IO
-     * memory region direction in the system memory space.  However,
-     * if any of the IO BAR subregions use the old_portio mechanism,
-     * that won't be processed properly unless accessed from the
-     * system io address space.  This hack to bounce things via
-     * system_io works around the problem until all the users of
-     * old_portion are updated */
+    /* Initialize IO regions */
     sprintf(namebuf, "%s.io", sphb->dtbusname);
     memory_region_init(&sphb->iospace, OBJECT(sphb),
                        namebuf, SPAPR_PCI_IO_WIN_SIZE);
-    /* FIXME: fix to support multiple PHBs */
-    memory_region_add_subregion(get_system_io(), 0, &sphb->iospace);
 
     sprintf(namebuf, "%s.io-alias", sphb->dtbusname);
     memory_region_init_alias(&sphb->iowindow, OBJECT(sphb), namebuf,
-                             get_system_io(), 0, SPAPR_PCI_IO_WIN_SIZE);
+                             &sphb->iospace, 0, SPAPR_PCI_IO_WIN_SIZE);
     memory_region_add_subregion(get_system_memory(), sphb->io_win_addr,
                                 &sphb->iowindow);
     /*

Re: [Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Alexey Kardashevskiy]

On 12/10/2013 03:33 AM, Greg Kurz wrote:
> In the past, IO space could not be mapped into the memory address space
> so we introduced a workaround for that. Nowadays it does not look
> necessary so we can remove the workaround and make sPAPR PCI
> configuration simplier.
> 
> This workaround has also an evil side effect with virtio devices: because
> all PHBs have their .io region at the same address, the devices
> get mapped in the .io-alias region of every PHB (AKA. mapped multiple times).
> This breaks the ioeventfd feature and causes qemu to abort() when running
> with KVM and asking for more than one PHB:
> 
> $ qemu-system-ppc64 -machine type=pseries,accel=kvm -smp 1 -m 4G \
>   -hda /local/greg/images/fedora-be.qcow2 \
>   -device virtio-9p-pci,fsdev=fsdev0,mount_tag=share,bus=pci,ioeventfd=on \
>   -fsdev local,security_model=none,id=fsdev0,path=$HOME/share1 \
>   -device spapr-pci-host-bridge,index=15
> kvm_mem_ioeventfd_add: error adding ioeventfd: File exists
> Aborted
> 
> This will prevent to use virtio and VFIO passthrough at the same time, since
> VFIO needs a dedicated PHB to work on ppc.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>


I have not seen this version yet so please remove me from "SOB". The patch
you replied to was eventually reworked and went to upstream as
66aab867cedd2a2d81b4d64eff7c3e0f6f272bbf

This one might be correct too but I want to try this first :)


> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr_pci.c |   13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 7763149..7d29431 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -564,23 +564,14 @@ static int spapr_phb_init(SysBusDevice *s)
>      memory_region_add_subregion(get_system_memory(), sphb->mem_win_addr,
>                                  &sphb->memwindow);
>  
> -    /* On ppc, we only have MMIO no specific IO space from the CPU
> -     * perspective.  In theory we ought to be able to embed the PCI IO
> -     * memory region direction in the system memory space.  However,
> -     * if any of the IO BAR subregions use the old_portio mechanism,
> -     * that won't be processed properly unless accessed from the
> -     * system io address space.  This hack to bounce things via
> -     * system_io works around the problem until all the users of
> -     * old_portion are updated */
> +    /* Initialize IO regions */
>      sprintf(namebuf, "%s.io", sphb->dtbusname);
>      memory_region_init(&sphb->iospace, OBJECT(sphb),
>                         namebuf, SPAPR_PCI_IO_WIN_SIZE);
> -    /* FIXME: fix to support multiple PHBs */
> -    memory_region_add_subregion(get_system_io(), 0, &sphb->iospace);
>  
>      sprintf(namebuf, "%s.io-alias", sphb->dtbusname);
>      memory_region_init_alias(&sphb->iowindow, OBJECT(sphb), namebuf,
> -                             get_system_io(), 0, SPAPR_PCI_IO_WIN_SIZE);
> +                             &sphb->iospace, 0, SPAPR_PCI_IO_WIN_SIZE);
>      memory_region_add_subregion(get_system_memory(), sphb->io_win_addr,
>                                  &sphb->iowindow);
>      /*
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Greg Kurz]

On Tue, 10 Dec 2013 13:43:05 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> On 12/10/2013 03:33 AM, Greg Kurz wrote:
> > In the past, IO space could not be mapped into the memory address space
> > so we introduced a workaround for that. Nowadays it does not look
> > necessary so we can remove the workaround and make sPAPR PCI
> > configuration simplier.
> > 
> > This workaround has also an evil side effect with virtio devices:
> > because all PHBs have their .io region at the same address, the devices
> > get mapped in the .io-alias region of every PHB (AKA. mapped multiple
> > times). This breaks the ioeventfd feature and causes qemu to abort()
> > when running with KVM and asking for more than one PHB:
> > 
> > $ qemu-system-ppc64 -machine type=pseries,accel=kvm -smp 1 -m 4G \
> >   -hda /local/greg/images/fedora-be.qcow2 \
> >   -device
> > virtio-9p-pci,fsdev=fsdev0,mount_tag=share,bus=pci,ioeventfd=on \
> > -fsdev local,security_model=none,id=fsdev0,path=$HOME/share1 \ -device
> > spapr-pci-host-bridge,index=15 kvm_mem_ioeventfd_add: error adding
> > ioeventfd: File exists Aborted
> > 
> > This will prevent to use virtio and VFIO passthrough at the same time,
> > since VFIO needs a dedicated PHB to work on ppc.
> > 
> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> 
> I have not seen this version yet so please remove me from "SOB". The patch
> you replied to was eventually reworked and went to upstream as
> 66aab867cedd2a2d81b4d64eff7c3e0f6f272bbf
> 

Hi Alex,

I agree you have not seen this version yet... The patch I replied to was my
primary source of inspiration and contains these bits, hence the SOB. 
Anyway, the SOB is now removed until you decide to add one yourself. :)

> This one might be correct too but I want to try this first :)
> 

Well, I hope it is. Please try it.

Thanks for the feedback.

Cheers.

--
Greg

> 
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> >  hw/ppc/spapr_pci.c |   13 ++-----------
> >  1 file changed, 2 insertions(+), 11 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 7763149..7d29431 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -564,23 +564,14 @@ static int spapr_phb_init(SysBusDevice *s)
> >      memory_region_add_subregion(get_system_memory(),
> > sphb->mem_win_addr, &sphb->memwindow);
> >  
> > -    /* On ppc, we only have MMIO no specific IO space from the CPU
> > -     * perspective.  In theory we ought to be able to embed the PCI IO
> > -     * memory region direction in the system memory space.  However,
> > -     * if any of the IO BAR subregions use the old_portio mechanism,
> > -     * that won't be processed properly unless accessed from the
> > -     * system io address space.  This hack to bounce things via
> > -     * system_io works around the problem until all the users of
> > -     * old_portion are updated */
> > +    /* Initialize IO regions */
> >      sprintf(namebuf, "%s.io", sphb->dtbusname);
> >      memory_region_init(&sphb->iospace, OBJECT(sphb),
> >                         namebuf, SPAPR_PCI_IO_WIN_SIZE);
> > -    /* FIXME: fix to support multiple PHBs */
> > -    memory_region_add_subregion(get_system_io(), 0, &sphb->iospace);
> >  
> >      sprintf(namebuf, "%s.io-alias", sphb->dtbusname);
> >      memory_region_init_alias(&sphb->iowindow, OBJECT(sphb), namebuf,
> > -                             get_system_io(), 0, 
SPAPR_PCI_IO_WIN_SIZE);
> > +                             &sphb->iospace, 0, SPAPR_PCI_IO_WIN_SIZE);
> >      memory_region_add_subregion(get_system_memory(), sphb->io_win_addr,
> >                                  &sphb->iowindow);
> >      /*
> > 
> 
> 

Re: [Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Alexey Kardashevskiy]

On 12/10/2013 06:47 PM, Greg Kurz wrote:
> On Tue, 10 Dec 2013 13:43:05 +1100
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>> On 12/10/2013 03:33 AM, Greg Kurz wrote:
>>> In the past, IO space could not be mapped into the memory address space
>>> so we introduced a workaround for that. Nowadays it does not look
>>> necessary so we can remove the workaround and make sPAPR PCI
>>> configuration simplier.
>>>
>>> This workaround has also an evil side effect with virtio devices:
>>> because all PHBs have their .io region at the same address, the devices
>>> get mapped in the .io-alias region of every PHB (AKA. mapped multiple
>>> times). This breaks the ioeventfd feature and causes qemu to abort()
>>> when running with KVM and asking for more than one PHB:
>>>
>>> $ qemu-system-ppc64 -machine type=pseries,accel=kvm -smp 1 -m 4G \
>>>   -hda /local/greg/images/fedora-be.qcow2 \
>>>   -device
>>> virtio-9p-pci,fsdev=fsdev0,mount_tag=share,bus=pci,ioeventfd=on \
>>> -fsdev local,security_model=none,id=fsdev0,path=$HOME/share1 \ -device
>>> spapr-pci-host-bridge,index=15 kvm_mem_ioeventfd_add: error adding
>>> ioeventfd: File exists Aborted
>>>
>>> This will prevent to use virtio and VFIO passthrough at the same time,
>>> since VFIO needs a dedicated PHB to work on ppc.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>
>>
>> I have not seen this version yet so please remove me from "SOB". The patch
>> you replied to was eventually reworked and went to upstream as
>> 66aab867cedd2a2d81b4d64eff7c3e0f6f272bbf
>>
> 
> Hi Alex,
> 
> I agree you have not seen this version yet... The patch I replied to was my
> primary source of inspiration and contains these bits, hence the SOB. 
> Anyway, the SOB is now removed until you decide to add one yourself. :)
> 
>> This one might be correct too but I want to try this first :)
>>
> 
> Well, I hope it is. Please try it.


Yep. Tried. Looks good, did not break a thing as far as I can tell, even
VGA works :)


Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>


-- 
Alexey

Re: [Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Alexey Kardashevskiy]

On 12/11/2013 05:47 PM, Alexey Kardashevskiy wrote:
> On 12/10/2013 06:47 PM, Greg Kurz wrote:
>> On Tue, 10 Dec 2013 13:43:05 +1100
>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>> On 12/10/2013 03:33 AM, Greg Kurz wrote:
>>>> In the past, IO space could not be mapped into the memory address space
>>>> so we introduced a workaround for that. Nowadays it does not look
>>>> necessary so we can remove the workaround and make sPAPR PCI
>>>> configuration simplier.
>>>>
>>>> This workaround has also an evil side effect with virtio devices:
>>>> because all PHBs have their .io region at the same address, the devices
>>>> get mapped in the .io-alias region of every PHB (AKA. mapped multiple
>>>> times). This breaks the ioeventfd feature and causes qemu to abort()
>>>> when running with KVM and asking for more than one PHB:
>>>>
>>>> $ qemu-system-ppc64 -machine type=pseries,accel=kvm -smp 1 -m 4G \
>>>>   -hda /local/greg/images/fedora-be.qcow2 \
>>>>   -device
>>>> virtio-9p-pci,fsdev=fsdev0,mount_tag=share,bus=pci,ioeventfd=on \
>>>> -fsdev local,security_model=none,id=fsdev0,path=$HOME/share1 \ -device
>>>> spapr-pci-host-bridge,index=15 kvm_mem_ioeventfd_add: error adding
>>>> ioeventfd: File exists Aborted
>>>>
>>>> This will prevent to use virtio and VFIO passthrough at the same time,
>>>> since VFIO needs a dedicated PHB to work on ppc.
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>
>>>
>>> I have not seen this version yet so please remove me from "SOB". The patch
>>> you replied to was eventually reworked and went to upstream as
>>> 66aab867cedd2a2d81b4d64eff7c3e0f6f272bbf
>>>
>>
>> Hi Alex,
>>
>> I agree you have not seen this version yet... The patch I replied to was my
>> primary source of inspiration and contains these bits, hence the SOB. 
>> Anyway, the SOB is now removed until you decide to add one yourself. :)
>>
>>> This one might be correct too but I want to try this first :)
>>>
>>
>> Well, I hope it is. Please try it.
> 
> 
> Yep. Tried. Looks good, did not break a thing as far as I can tell, even
> VGA works :)
> 
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Hm. Nack. This fails:

./qemu-system-ppc64 \
 -trace "events=qemu_trace_events" \
 -L "qemu-ppc64-bios/" \
 -nographic \
 -vga "none" \
 -device \
 virtio-blk-pci,id=virtioiblk0,drive=drive0,bootindex=20,ioeventfd=on \
 -drive \
file=virtimg/fc19_16GB.qcow2,if=none,id=drive0,readonly=off,format=qcow2,media=disk,werror=stop,rerror=stop,discard=on
\
 -S \
 -m "2048" \
 -machine "pseries" \
 -enable-kvm




command line: BOOT_IMAGE=/vmlinux-3.10.0-rc7-aik-guest+
root=UUID=27cde746-128e-4528-b4de-44a00d807ea0 ro rd.md=0 rd.lvm=0 rd.dm=0
vconsole.font=latarcyrheb-sun16 vconsole.keymap=us rd.luks=0 console=hvc0 debug
memory layout at init:
  memory_limit : 0000000000000000 (16 MB aligned)
  alloc_bottom : 0000000003400000
  alloc_top    : 0000000030000000
  alloc_top_hi : 0000000080000000
  rmo_top      : 0000000030000000
  ram_top      : 0000000080000000
instantiating rtas at 0x000000002fff0000... done
boot cpu hw idx 0
copying OF device tree...
Building dt strings...
Building dt structure...
Device tree strings 0x0000000003410000 -> 0x0000000003410817
Device tree struct  0x0000000003420000 -> 0x0000000003430000
[Switching to Thread 0x3fff8ca4eee0 (LWP 10370)]

Breakpoint 8, unassigned_mem_accepts (opaque=0x0, addr=0x10080000052,
size=0x1, is_write=0x1)
    at /home/alexey/p/qemu/memory.c:892
892	    return false;
Missing separate debuginfos, use: debuginfo-install
glusterfs-api-3.4.0-8.fc19.ppc64 glusterfs-libs-3.4.0-8.fc19.ppc64
gnutls-3.1.16-1.fc19.ppc64 keyutils-libs-1.5.6-1.fc19.ppc64
libgcc-4.8.2-1.fc19.ppc64 libgcrypt-1.5.3-2.fc19.ppc64
libibverbs-1.1.7-3.fc19.ppc64 libiscsi-1.7.0-5.fc19.ppc64
libpng-1.5.13-2.fc19.ppc64 librdmacm-1.0.17-1.fc19.ppc64
libusbx-1.0.16-3.fc19.ppc64 systemd-libs-204-17.fc19.ppc64
usbredir-0.6-2.fc19.ppc64
(gdb) bt
#0  unassigned_mem_accepts (opaque=0x0, addr=0x10080000052, size=0x1,
is_write=0x1)
    at /home/alexey/p/qemu/memory.c:892
#1  0x00000000103cb238 in memory_region_access_valid (mr=0x10b76ec8
<io_mem_unassigned>,
    addr=0x10080000052, size=0x1, is_write=0x1) at
/home/alexey/p/qemu/memory.c:928
#2  0x00000000103cb4bc in memory_region_dispatch_write (mr=0x10b76ec8
<io_mem_unassigned>,
    addr=0x10080000052, data=0x80, size=0x1) at
/home/alexey/p/qemu/memory.c:976
#3  0x00000000103cebec in io_mem_write (mr=0x10b76ec8 <io_mem_unassigned>,
addr=0x10080000052,
    val=0x80, size=0x1) at /home/alexey/p/qemu/memory.c:1748
#4  0x0000000010329b54 in address_space_rw (as=0x10b80cf0
<address_space_memory>,
    addr=0x10080000052, buf=0x3fff8ad9e190 "\200", len=0x1, is_write=0x1)
    at /home/alexey/p/qemu/exec.c:1941
#5  0x000000001032a000 in cpu_physical_memory_rw (addr=0x10080000052,
    buf=0x3fff8ad9e190 "\200", len=0x1, is_write=0x1) at
/home/alexey/p/qemu/exec.c:2010
#6  0x00000000103231c4 in cpu_physical_memory_write (addr=0x10080000052,
buf=0x3fff8ad9e190,
    len=0x1) at /home/alexey/p/qemu/include/exec/cpu-common.h:68
#7  0x000000001032b5b0 in stb_phys (addr=0x10080000052, val=0x80)
    at /home/alexey/p/qemu/exec.c:2506
#8  0x00000000103a0c10 in h_logical_store (cpu=0x3fff8ada0010,
spapr=0x100118ca410,
    opcode=0x40, args=0x3fff8bfc0030) at
/home/alexey/p/qemu/hw/ppc/spapr_hcall.c:564
#9  0x00000000103a140c in spapr_hypercall (cpu=0x3fff8ada0010, opcode=0x40,
args=0x3fff8bfc0030)
    at /home/alexey/p/qemu/hw/ppc/spapr_hcall.c:737
#10 0x000000001041b080 in kvm_arch_handle_exit (cs=0x3fff8ada0010,
run=0x3fff8bfc0000)
    at /home/alexey/p/qemu/target-ppc/kvm.c:1223
#11 0x00000000103c5cbc in kvm_cpu_exec (cpu=0x3fff8ada0010)
    at /home/alexey/p/qemu/kvm-all.c:1726
#12 0x000000001031902c in qemu_kvm_cpu_thread_fn (arg=0x3fff8ada0010)
    at /home/alexey/p/qemu/cpus.c:874
#13 0x00000080bcd0c29c in start_thread (arg=0x3fff8ad9eee0) at
pthread_create.c:310
#14 0x00000080bcb1ddb0 in .__clone ()
    at ../sysdeps/unix/sysv/linux/powerpc/powerpc64/clone.S:111



Without your patch, unassigned_mem_accepts() is not called so it tells us
that IO stopped working after your patch.

Unfortunately I do not have good 3D imagination, do not understand this
memory region well enough to give advice and cannot tell quickly what
exactly is wrong here :)



-- 
Alexey

Re: [Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Greg Kurz]

On Wed, 11 Dec 2013 18:07:58 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
> Hm. Nack. This fails:
> 
> ./qemu-system-ppc64 \
>  -trace "events=qemu_trace_events" \
>  -L "qemu-ppc64-bios/" \
>  -nographic \
>  -vga "none" \
>  -device \
>  virtio-blk-pci,id=virtioiblk0,drive=drive0,bootindex=20,ioeventfd=on \
>  -drive \
> file=virtimg/fc19_16GB.qcow2,if=none,id=drive0,readonly=off,format=qcow2,media=disk,werror=stop,rerror=stop,discard=on
> \
>  -S \
>  -m "2048" \
>  -machine "pseries" \
>  -enable-kvm
> 
> 
> 
> 
> command line: BOOT_IMAGE=/vmlinux-3.10.0-rc7-aik-guest+
> root=UUID=27cde746-128e-4528-b4de-44a00d807ea0 ro rd.md=0 rd.lvm=0 rd.dm=0
> vconsole.font=latarcyrheb-sun16 vconsole.keymap=us rd.luks=0 console=hvc0
> debug memory layout at init:
>   memory_limit : 0000000000000000 (16 MB aligned)
>   alloc_bottom : 0000000003400000
>   alloc_top    : 0000000030000000
>   alloc_top_hi : 0000000080000000
>   rmo_top      : 0000000030000000
>   ram_top      : 0000000080000000
> instantiating rtas at 0x000000002fff0000... done
> boot cpu hw idx 0
> copying OF device tree...
> Building dt strings...
> Building dt structure...
> Device tree strings 0x0000000003410000 -> 0x0000000003410817
> Device tree struct  0x0000000003420000 -> 0x0000000003430000
> [Switching to Thread 0x3fff8ca4eee0 (LWP 10370)]
> 
> Breakpoint 8, unassigned_mem_accepts (opaque=0x0, addr=0x10080000052,
> size=0x1, is_write=0x1)
>     at /home/alexey/p/qemu/memory.c:892
> 892	    return false;
> Missing separate debuginfos, use: debuginfo-install
> glusterfs-api-3.4.0-8.fc19.ppc64 glusterfs-libs-3.4.0-8.fc19.ppc64
> gnutls-3.1.16-1.fc19.ppc64 keyutils-libs-1.5.6-1.fc19.ppc64
> libgcc-4.8.2-1.fc19.ppc64 libgcrypt-1.5.3-2.fc19.ppc64
> libibverbs-1.1.7-3.fc19.ppc64 libiscsi-1.7.0-5.fc19.ppc64
> libpng-1.5.13-2.fc19.ppc64 librdmacm-1.0.17-1.fc19.ppc64
> libusbx-1.0.16-3.fc19.ppc64 systemd-libs-204-17.fc19.ppc64
> usbredir-0.6-2.fc19.ppc64
> (gdb) bt
> #0  unassigned_mem_accepts (opaque=0x0, addr=0x10080000052, size=0x1,
> is_write=0x1)
>     at /home/alexey/p/qemu/memory.c:892
> #1  0x00000000103cb238 in memory_region_access_valid (mr=0x10b76ec8
> <io_mem_unassigned>,
>     addr=0x10080000052, size=0x1, is_write=0x1) at
> /home/alexey/p/qemu/memory.c:928
> #2  0x00000000103cb4bc in memory_region_dispatch_write (mr=0x10b76ec8
> <io_mem_unassigned>,
>     addr=0x10080000052, data=0x80, size=0x1) at
> /home/alexey/p/qemu/memory.c:976
> #3  0x00000000103cebec in io_mem_write (mr=0x10b76ec8 <io_mem_unassigned>,
> addr=0x10080000052,
>     val=0x80, size=0x1) at /home/alexey/p/qemu/memory.c:1748
> #4  0x0000000010329b54 in address_space_rw (as=0x10b80cf0
> <address_space_memory>,
>     addr=0x10080000052, buf=0x3fff8ad9e190 "\200", len=0x1, is_write=0x1)
>     at /home/alexey/p/qemu/exec.c:1941
> #5  0x000000001032a000 in cpu_physical_memory_rw (addr=0x10080000052,
>     buf=0x3fff8ad9e190 "\200", len=0x1, is_write=0x1) at
> /home/alexey/p/qemu/exec.c:2010
> #6  0x00000000103231c4 in cpu_physical_memory_write (addr=0x10080000052,
> buf=0x3fff8ad9e190,
>     len=0x1) at /home/alexey/p/qemu/include/exec/cpu-common.h:68
> #7  0x000000001032b5b0 in stb_phys (addr=0x10080000052, val=0x80)
>     at /home/alexey/p/qemu/exec.c:2506
> #8  0x00000000103a0c10 in h_logical_store (cpu=0x3fff8ada0010,
> spapr=0x100118ca410,
>     opcode=0x40, args=0x3fff8bfc0030) at
> /home/alexey/p/qemu/hw/ppc/spapr_hcall.c:564
> #9  0x00000000103a140c in spapr_hypercall (cpu=0x3fff8ada0010,
> opcode=0x40, args=0x3fff8bfc0030)
>     at /home/alexey/p/qemu/hw/ppc/spapr_hcall.c:737
> #10 0x000000001041b080 in kvm_arch_handle_exit (cs=0x3fff8ada0010,
> run=0x3fff8bfc0000)
>     at /home/alexey/p/qemu/target-ppc/kvm.c:1223
> #11 0x00000000103c5cbc in kvm_cpu_exec (cpu=0x3fff8ada0010)
>     at /home/alexey/p/qemu/kvm-all.c:1726
> #12 0x000000001031902c in qemu_kvm_cpu_thread_fn (arg=0x3fff8ada0010)
>     at /home/alexey/p/qemu/cpus.c:874
> #13 0x00000080bcd0c29c in start_thread (arg=0x3fff8ad9eee0) at
> pthread_create.c:310
> #14 0x00000080bcb1ddb0 in .__clone ()
>     at ../sysdeps/unix/sysv/linux/powerpc/powerpc64/clone.S:111
> 
> 
> 
> Without your patch, unassigned_mem_accepts() is not called so it tells us
> that IO stopped working after your patch.
> 

Alex,

Can you elaborate ? Does the kernel fail to boot ?

> Unfortunately I do not have good 3D imagination, do not understand this
> memory region well enough to give advice and cannot tell quickly what
> exactly is wrong here :)
> 

Heh no problem. Let me share my findings then ! :)

First, if I pass kernel/initrd/cmdline directly to the qemu command
line, I don't get this weird access to 0x10080000052 at all (with or
without my patch).

Second, if I run the very same test WITHOUT my patch and set a breakpoint
in unassigned_io_write(), it pops with the same 0x10080000052 address.

Third but not least, I have not hit a single issue so far... I mean, when
the kernel is booted, virtio is functional with my patch, as far as I could
test (having / mounted on virtio, multiple 9p shares).

It proves the patch is not responsible for the "unassigned" thing, IMHO.

Thanks for your time anyway.

Cheers.

--
Greg

Re: [Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Alexey Kardashevskiy]

On 12/17/2013 06:52 PM, Greg Kurz wrote:
> On Wed, 11 Dec 2013 18:07:58 +1100
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>
>> Hm. Nack. This fails:
>>
>> ./qemu-system-ppc64 \
>>  -trace "events=qemu_trace_events" \
>>  -L "qemu-ppc64-bios/" \
>>  -nographic \
>>  -vga "none" \
>>  -device \
>>  virtio-blk-pci,id=virtioiblk0,drive=drive0,bootindex=20,ioeventfd=on \
>>  -drive \
>> file=virtimg/fc19_16GB.qcow2,if=none,id=drive0,readonly=off,format=qcow2,media=disk,werror=stop,rerror=stop,discard=on
>> \
>>  -S \
>>  -m "2048" \
>>  -machine "pseries" \
>>  -enable-kvm
>>
>>
>>
>>
>> command line: BOOT_IMAGE=/vmlinux-3.10.0-rc7-aik-guest+
>> root=UUID=27cde746-128e-4528-b4de-44a00d807ea0 ro rd.md=0 rd.lvm=0 rd.dm=0
>> vconsole.font=latarcyrheb-sun16 vconsole.keymap=us rd.luks=0 console=hvc0
>> debug memory layout at init:
>>   memory_limit : 0000000000000000 (16 MB aligned)
>>   alloc_bottom : 0000000003400000
>>   alloc_top    : 0000000030000000
>>   alloc_top_hi : 0000000080000000
>>   rmo_top      : 0000000030000000
>>   ram_top      : 0000000080000000
>> instantiating rtas at 0x000000002fff0000... done
>> boot cpu hw idx 0
>> copying OF device tree...
>> Building dt strings...
>> Building dt structure...
>> Device tree strings 0x0000000003410000 -> 0x0000000003410817
>> Device tree struct  0x0000000003420000 -> 0x0000000003430000
>> [Switching to Thread 0x3fff8ca4eee0 (LWP 10370)]
>>
>> Breakpoint 8, unassigned_mem_accepts (opaque=0x0, addr=0x10080000052,
>> size=0x1, is_write=0x1)
>>     at /home/alexey/p/qemu/memory.c:892
>> 892	    return false;
>> Missing separate debuginfos, use: debuginfo-install
>> glusterfs-api-3.4.0-8.fc19.ppc64 glusterfs-libs-3.4.0-8.fc19.ppc64
>> gnutls-3.1.16-1.fc19.ppc64 keyutils-libs-1.5.6-1.fc19.ppc64
>> libgcc-4.8.2-1.fc19.ppc64 libgcrypt-1.5.3-2.fc19.ppc64
>> libibverbs-1.1.7-3.fc19.ppc64 libiscsi-1.7.0-5.fc19.ppc64
>> libpng-1.5.13-2.fc19.ppc64 librdmacm-1.0.17-1.fc19.ppc64
>> libusbx-1.0.16-3.fc19.ppc64 systemd-libs-204-17.fc19.ppc64
>> usbredir-0.6-2.fc19.ppc64
>> (gdb) bt
>> #0  unassigned_mem_accepts (opaque=0x0, addr=0x10080000052, size=0x1,
>> is_write=0x1)
>>     at /home/alexey/p/qemu/memory.c:892
>> #1  0x00000000103cb238 in memory_region_access_valid (mr=0x10b76ec8
>> <io_mem_unassigned>,
>>     addr=0x10080000052, size=0x1, is_write=0x1) at
>> /home/alexey/p/qemu/memory.c:928
>> #2  0x00000000103cb4bc in memory_region_dispatch_write (mr=0x10b76ec8
>> <io_mem_unassigned>,
>>     addr=0x10080000052, data=0x80, size=0x1) at
>> /home/alexey/p/qemu/memory.c:976
>> #3  0x00000000103cebec in io_mem_write (mr=0x10b76ec8 <io_mem_unassigned>,
>> addr=0x10080000052,
>>     val=0x80, size=0x1) at /home/alexey/p/qemu/memory.c:1748
>> #4  0x0000000010329b54 in address_space_rw (as=0x10b80cf0
>> <address_space_memory>,
>>     addr=0x10080000052, buf=0x3fff8ad9e190 "\200", len=0x1, is_write=0x1)
>>     at /home/alexey/p/qemu/exec.c:1941
>> #5  0x000000001032a000 in cpu_physical_memory_rw (addr=0x10080000052,
>>     buf=0x3fff8ad9e190 "\200", len=0x1, is_write=0x1) at
>> /home/alexey/p/qemu/exec.c:2010
>> #6  0x00000000103231c4 in cpu_physical_memory_write (addr=0x10080000052,
>> buf=0x3fff8ad9e190,
>>     len=0x1) at /home/alexey/p/qemu/include/exec/cpu-common.h:68
>> #7  0x000000001032b5b0 in stb_phys (addr=0x10080000052, val=0x80)
>>     at /home/alexey/p/qemu/exec.c:2506
>> #8  0x00000000103a0c10 in h_logical_store (cpu=0x3fff8ada0010,
>> spapr=0x100118ca410,
>>     opcode=0x40, args=0x3fff8bfc0030) at
>> /home/alexey/p/qemu/hw/ppc/spapr_hcall.c:564
>> #9  0x00000000103a140c in spapr_hypercall (cpu=0x3fff8ada0010,
>> opcode=0x40, args=0x3fff8bfc0030)
>>     at /home/alexey/p/qemu/hw/ppc/spapr_hcall.c:737
>> #10 0x000000001041b080 in kvm_arch_handle_exit (cs=0x3fff8ada0010,
>> run=0x3fff8bfc0000)
>>     at /home/alexey/p/qemu/target-ppc/kvm.c:1223
>> #11 0x00000000103c5cbc in kvm_cpu_exec (cpu=0x3fff8ada0010)
>>     at /home/alexey/p/qemu/kvm-all.c:1726
>> #12 0x000000001031902c in qemu_kvm_cpu_thread_fn (arg=0x3fff8ada0010)
>>     at /home/alexey/p/qemu/cpus.c:874
>> #13 0x00000080bcd0c29c in start_thread (arg=0x3fff8ad9eee0) at
>> pthread_create.c:310
>> #14 0x00000080bcb1ddb0 in .__clone ()
>>     at ../sysdeps/unix/sysv/linux/powerpc/powerpc64/clone.S:111
>>
>>
>>
>> Without your patch, unassigned_mem_accepts() is not called so it tells us
>> that IO stopped working after your patch.
>>
> 
> Alex,
> 
> Can you elaborate ? Does the kernel fail to boot ?
> 
>> Unfortunately I do not have good 3D imagination, do not understand this
>> memory region well enough to give advice and cannot tell quickly what
>> exactly is wrong here :)
>>
> 
> Heh no problem. Let me share my findings then ! :)
> 
> First, if I pass kernel/initrd/cmdline directly to the qemu command
> line, I don't get this weird access to 0x10080000052 at all (with or
> without my patch).
> 
> Second, if I run the very same test WITHOUT my patch and set a breakpoint
> in unassigned_io_write(), it pops with the same 0x10080000052 address.


It does not stop there when I try it "WITHOUT my patch", that's my entire
point.

I retried right now, with the upstream QEMU + KVM breakpoint stubs patch.

With your patch - it still 100% reproducible - gdb stops at
unassigned_io_write().

Without your patch there is no stopping in unassigned_io_write().

The exact command line is:
./qemu-system-ppc64 \
	-enable-kvm \
	-m 2048 \
	-L qemu-ppc64-bios/ \
	-machine pseries \
	-trace events=qemu_trace_events \
	-nographic \
	-vga none \
	-drive
id=id0,if=none,readonly=off,werror=stop,rerror=stop,discard=on,file=virtimg/fc19_16GB.qcow2,format=qcow2
\
	-device virtio-blk-pci,id=id1,drive=id0


There is always a chance that you have fixed one bug and make another show
up, this happens sometime, but we do not know for sure that this is the case :)


> Third but not least, I have not hit a single issue so far... I mean, when
> the kernel is booted, virtio is functional with my patch, as far as I could
> test (having / mounted on virtio, multiple 9p shares).


Yes, it is functional in this particular example. However something else
might get broken, something what makes use of IO ports, I do not even know
what it could be (need to test every emulated PCI device with all supported
distros to make sure OR understand what the difference is and fix it).



> It proves the patch is not responsible for the "unassigned" thing, IMHO.
> 
> Thanks for your time anyway.

Welcome :)


-- 
Alexey

Re: [Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Alexander Graf]

On 11.12.2013, at 07:47, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> On 12/10/2013 06:47 PM, Greg Kurz wrote:
>> On Tue, 10 Dec 2013 13:43:05 +1100
>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>> On 12/10/2013 03:33 AM, Greg Kurz wrote:
>>>> In the past, IO space could not be mapped into the memory address space
>>>> so we introduced a workaround for that. Nowadays it does not look
>>>> necessary so we can remove the workaround and make sPAPR PCI
>>>> configuration simplier.
>>>> 
>>>> This workaround has also an evil side effect with virtio devices:
>>>> because all PHBs have their .io region at the same address, the devices
>>>> get mapped in the .io-alias region of every PHB (AKA. mapped multiple
>>>> times). This breaks the ioeventfd feature and causes qemu to abort()
>>>> when running with KVM and asking for more than one PHB:
>>>> 
>>>> $ qemu-system-ppc64 -machine type=pseries,accel=kvm -smp 1 -m 4G \
>>>>  -hda /local/greg/images/fedora-be.qcow2 \
>>>>  -device
>>>> virtio-9p-pci,fsdev=fsdev0,mount_tag=share,bus=pci,ioeventfd=on \
>>>> -fsdev local,security_model=none,id=fsdev0,path=$HOME/share1 \ -device
>>>> spapr-pci-host-bridge,index=15 kvm_mem_ioeventfd_add: error adding
>>>> ioeventfd: File exists Aborted
>>>> 
>>>> This will prevent to use virtio and VFIO passthrough at the same time,
>>>> since VFIO needs a dedicated PHB to work on ppc.
>>>> 
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> 
>>> 
>>> I have not seen this version yet so please remove me from "SOB". The patch
>>> you replied to was eventually reworked and went to upstream as
>>> 66aab867cedd2a2d81b4d64eff7c3e0f6f272bbf
>>> 
>> 
>> Hi Alex,
>> 
>> I agree you have not seen this version yet... The patch I replied to was my
>> primary source of inspiration and contains these bits, hence the SOB. 
>> Anyway, the SOB is now removed until you decide to add one yourself. :)
>> 
>>> This one might be correct too but I want to try this first :)
>>> 
>> 
>> Well, I hope it is. Please try it.
> 
> 
> Yep. Tried. Looks good, did not break a thing as far as I can tell, even
> VGA works :)
> 
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Thanks, applied to ppc-next.


Alex

Re: [Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Alexey Kardashevskiy]

On 01/03/2014 08:04 AM, Alexander Graf wrote:
> 
> On 11.12.2013, at 07:47, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
>> On 12/10/2013 06:47 PM, Greg Kurz wrote:
>>> On Tue, 10 Dec 2013 13:43:05 +1100
>>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>> On 12/10/2013 03:33 AM, Greg Kurz wrote:
>>>>> In the past, IO space could not be mapped into the memory address space
>>>>> so we introduced a workaround for that. Nowadays it does not look
>>>>> necessary so we can remove the workaround and make sPAPR PCI
>>>>> configuration simplier.
>>>>>
>>>>> This workaround has also an evil side effect with virtio devices:
>>>>> because all PHBs have their .io region at the same address, the devices
>>>>> get mapped in the .io-alias region of every PHB (AKA. mapped multiple
>>>>> times). This breaks the ioeventfd feature and causes qemu to abort()
>>>>> when running with KVM and asking for more than one PHB:
>>>>>
>>>>> $ qemu-system-ppc64 -machine type=pseries,accel=kvm -smp 1 -m 4G \
>>>>>  -hda /local/greg/images/fedora-be.qcow2 \
>>>>>  -device
>>>>> virtio-9p-pci,fsdev=fsdev0,mount_tag=share,bus=pci,ioeventfd=on \
>>>>> -fsdev local,security_model=none,id=fsdev0,path=$HOME/share1 \ -device
>>>>> spapr-pci-host-bridge,index=15 kvm_mem_ioeventfd_add: error adding
>>>>> ioeventfd: File exists Aborted
>>>>>
>>>>> This will prevent to use virtio and VFIO passthrough at the same time,
>>>>> since VFIO needs a dedicated PHB to work on ppc.
>>>>>
>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>
>>>>
>>>> I have not seen this version yet so please remove me from "SOB". The patch
>>>> you replied to was eventually reworked and went to upstream as
>>>> 66aab867cedd2a2d81b4d64eff7c3e0f6f272bbf
>>>>
>>>
>>> Hi Alex,
>>>
>>> I agree you have not seen this version yet... The patch I replied to was my
>>> primary source of inspiration and contains these bits, hence the SOB. 
>>> Anyway, the SOB is now removed until you decide to add one yourself. :)
>>>
>>>> This one might be correct too but I want to try this first :)
>>>>
>>>
>>> Well, I hope it is. Please try it.
>>
>>
>> Yep. Tried. Looks good, did not break a thing as far as I can tell, even
>> VGA works :)
>>
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> Thanks, applied to ppc-next.


Please read the rest of this thread. It does not visibly break things but
with this patch QEMU starts calling unassigned_mem_accepts() (normally
silent) which is not a good sign.



-- 
Alexey

Re: [Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Alexander Graf]

On 02.01.2014, at 23:08, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> On 01/03/2014 08:04 AM, Alexander Graf wrote:
>> 
>> On 11.12.2013, at 07:47, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>> 
>>> On 12/10/2013 06:47 PM, Greg Kurz wrote:
>>>> On Tue, 10 Dec 2013 13:43:05 +1100
>>>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>>> On 12/10/2013 03:33 AM, Greg Kurz wrote:
>>>>>> In the past, IO space could not be mapped into the memory address space
>>>>>> so we introduced a workaround for that. Nowadays it does not look
>>>>>> necessary so we can remove the workaround and make sPAPR PCI
>>>>>> configuration simplier.
>>>>>> 
>>>>>> This workaround has also an evil side effect with virtio devices:
>>>>>> because all PHBs have their .io region at the same address, the devices
>>>>>> get mapped in the .io-alias region of every PHB (AKA. mapped multiple
>>>>>> times). This breaks the ioeventfd feature and causes qemu to abort()
>>>>>> when running with KVM and asking for more than one PHB:
>>>>>> 
>>>>>> $ qemu-system-ppc64 -machine type=pseries,accel=kvm -smp 1 -m 4G \
>>>>>> -hda /local/greg/images/fedora-be.qcow2 \
>>>>>> -device
>>>>>> virtio-9p-pci,fsdev=fsdev0,mount_tag=share,bus=pci,ioeventfd=on \
>>>>>> -fsdev local,security_model=none,id=fsdev0,path=$HOME/share1 \ -device
>>>>>> spapr-pci-host-bridge,index=15 kvm_mem_ioeventfd_add: error adding
>>>>>> ioeventfd: File exists Aborted
>>>>>> 
>>>>>> This will prevent to use virtio and VFIO passthrough at the same time,
>>>>>> since VFIO needs a dedicated PHB to work on ppc.
>>>>>> 
>>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>> 
>>>>> 
>>>>> I have not seen this version yet so please remove me from "SOB". The patch
>>>>> you replied to was eventually reworked and went to upstream as
>>>>> 66aab867cedd2a2d81b4d64eff7c3e0f6f272bbf
>>>>> 
>>>> 
>>>> Hi Alex,
>>>> 
>>>> I agree you have not seen this version yet... The patch I replied to was my
>>>> primary source of inspiration and contains these bits, hence the SOB. 
>>>> Anyway, the SOB is now removed until you decide to add one yourself. :)
>>>> 
>>>>> This one might be correct too but I want to try this first :)
>>>>> 
>>>> 
>>>> Well, I hope it is. Please try it.
>>> 
>>> 
>>> Yep. Tried. Looks good, did not break a thing as far as I can tell, even
>>> VGA works :)
>>> 
>>> 
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> 
>> Thanks, applied to ppc-next.
> 
> 
> Please read the rest of this thread. It does not visibly break things but
> with this patch QEMU starts calling unassigned_mem_accepts() (normally
> silent) which is not a good sign.

Oops, I thought your comment meant this was fixed. I took it off the queue again :).


Alex

Re: [Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Alexey Kardashevskiy]

On 03.01.2014 9:09, Alexander Graf wrote:
> 
> On 02.01.2014, at 23:08, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
>> On 01/03/2014 08:04 AM, Alexander Graf wrote:
>>>
>>> On 11.12.2013, at 07:47, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>
>>>> On 12/10/2013 06:47 PM, Greg Kurz wrote:
>>>>> On Tue, 10 Dec 2013 13:43:05 +1100
>>>>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>>>> On 12/10/2013 03:33 AM, Greg Kurz wrote:
>>>>>>> In the past, IO space could not be mapped into the memory address space
>>>>>>> so we introduced a workaround for that. Nowadays it does not look
>>>>>>> necessary so we can remove the workaround and make sPAPR PCI
>>>>>>> configuration simplier.
>>>>>>>
>>>>>>> This workaround has also an evil side effect with virtio devices:
>>>>>>> because all PHBs have their .io region at the same address, the devices
>>>>>>> get mapped in the .io-alias region of every PHB (AKA. mapped multiple
>>>>>>> times). This breaks the ioeventfd feature and causes qemu to abort()
>>>>>>> when running with KVM and asking for more than one PHB:
>>>>>>>
>>>>>>> $ qemu-system-ppc64 -machine type=pseries,accel=kvm -smp 1 -m 4G \
>>>>>>> -hda /local/greg/images/fedora-be.qcow2 \
>>>>>>> -device
>>>>>>> virtio-9p-pci,fsdev=fsdev0,mount_tag=share,bus=pci,ioeventfd=on \
>>>>>>> -fsdev local,security_model=none,id=fsdev0,path=$HOME/share1 \ -device
>>>>>>> spapr-pci-host-bridge,index=15 kvm_mem_ioeventfd_add: error adding
>>>>>>> ioeventfd: File exists Aborted
>>>>>>>
>>>>>>> This will prevent to use virtio and VFIO passthrough at the same time,
>>>>>>> since VFIO needs a dedicated PHB to work on ppc.
>>>>>>>
>>>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>>>
>>>>>>
>>>>>> I have not seen this version yet so please remove me from "SOB". The patch
>>>>>> you replied to was eventually reworked and went to upstream as
>>>>>> 66aab867cedd2a2d81b4d64eff7c3e0f6f272bbf
>>>>>>
>>>>>
>>>>> Hi Alex,
>>>>>
>>>>> I agree you have not seen this version yet... The patch I replied to was my
>>>>> primary source of inspiration and contains these bits, hence the SOB. 
>>>>> Anyway, the SOB is now removed until you decide to add one yourself. :)
>>>>>
>>>>>> This one might be correct too but I want to try this first :)
>>>>>>
>>>>>
>>>>> Well, I hope it is. Please try it.
>>>>
>>>>
>>>> Yep. Tried. Looks good, did not break a thing as far as I can tell, even
>>>> VGA works :)
>>>>
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>
>>> Thanks, applied to ppc-next.
>>
>>
>> Please read the rest of this thread. It does not visibly break things but
>> with this patch QEMU starts calling unassigned_mem_accepts() (normally
>> silent) which is not a good sign.
> 
> Oops, I thought your comment meant this was fixed. I took it off the queue again :).

Thanks :)

And I have another bug like that - SPR registers are never
saved/restored via KVM's set-one-reg mechanism (with the loop through
all 1024 SPRs) because one_reg_id in SPR's descriptor is never
initialized (the macro misses assignment) and I made a patch for that
but cannot post it as some registers (AMR?) breaks HV KVM :)


-- 
With best regards

Alexey Kardashevskiy -- icq: 52150396

Re: [Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Greg Kurz]

On Fri, 03 Jan 2014 09:08:21 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
> Please read the rest of this thread. It does not visibly break things but
> with this patch QEMU starts calling unassigned_mem_accepts() (normally
> silent) which is not a good sign.
> 
> 
> 

Hmm... this is only because this patch moves the PHB io region from the
system IO to the system memory space, but the bogus(?) write to unassigned
memory already exists.

I have tested against the current ppc-next (62d529a), with no
additional patch:

qemu-system-ppc64 \
-snapshot -S -monitor stdio -serial pty \
-nographic -nodefaults \
-machine type=pseries,accel=kvm -smp 1 -m 4G \
-device virtio-blk-pci,id=virtioiblk0,drive=drive0,bootindex=20,ioeventfd=on \
-drive file=/local/greg/qemu/fedora-be.qcow2,if=none,id=drive0,readonly=off,\
       format=qcow2,media=disk,werror=stop,rerror=stop,discard=on

where fedora-be.qcow2 contains a stock fedora 19 for ppc64.

I have attached gdb to qemu and set a breakpoint in unassigned_io_write(), and
here is what I get again:

(gdb) b unassigned_io_write 
Breakpoint 1 at 0x1045d308: file /home/greg/Work/ibm/linux/qemu-agraf/ioport.c, line 54.
(gdb) c
Continuing.
[Thread 0x1ffffc5deef0 (LWP 11946) exited]
[New Thread 0x1ffffc5deef0 (LWP 11955)]
[Switching to Thread 0x1ffffbdaeef0 (LWP 11947)]

Breakpoint 1, unassigned_io_write (opaque=0x0, addr=82, val=128, size=1) at /home/greg/Work/ibm/linux/qemu-agraf/ioport.c:54
54      {
(gdb) where
#0  unassigned_io_write (opaque=0x0, addr=82, val=128, size=1) at /home/greg/Work/ibm/linux/qemu-agraf/ioport.c:54
#1  0x0000000010468f38 in memory_region_write_accessor (mr=0x10027615940, addr=82, value=0x1ffffbdadd68, size=1, shift=0, mask=255) at /home/greg/Work/ibm/linux/qemu-agraf/memory.c:440
#2  0x00000000104690c4 in access_with_adjusted_size (addr=82, value=0x1ffffbdadd68, size=1, access_size_min=1, access_size_max=4, access=@0x107ca670: 0x10468e5c <memory_region_write_accessor>, mr=0x10027615940)
    at /home/greg/Work/ibm/linux/qemu-agraf/memory.c:472
#3  0x000000001046bc64 in memory_region_dispatch_write (mr=0x10027615940, addr=82, data=128, size=1) at /home/greg/Work/ibm/linux/qemu-agraf/memory.c:984
#4  0x000000001046fdc4 in io_mem_write (mr=0x10027615940, addr=82, val=128, size=1) at /home/greg/Work/ibm/linux/qemu-agraf/memory.c:1749
#5  0x00000000103aca0c in address_space_rw (as=0x10c19638 <address_space_memory>, addr=1101659111506, buf=0x1ffffbdae117 "\200", len=1, is_write=true) at /home/greg/Work/ibm/linux/qemu-agraf/exec.c:2002
#6  0x00000000103acf3c in cpu_physical_memory_rw (addr=1101659111506, buf=0x1ffffbdae117 "\200", len=1, is_write=1) at /home/greg/Work/ibm/linux/qemu-agraf/exec.c:2071
#7  0x00000000103a44c4 in cpu_physical_memory_write (addr=1101659111506, buf=0x1ffffbdae117, len=1) at /home/greg/Work/ibm/linux/qemu-agraf/include/exec/cpu-common.h:68
#8  0x00000000103aeb2c in stb_phys (addr=1101659111506, val=128) at /home/greg/Work/ibm/linux/qemu-agraf/exec.c:2600
#9  0x0000000010438550 in h_logical_store (cpu=0x10027d0f0d0, spapr=0x100276bb210, opcode=64, args=0x1ffffff80030) at /home/greg/Work/ibm/linux/qemu-agraf/hw/ppc/spapr_hcall.c:564
#10 0x0000000010438e74 in spapr_hypercall (cpu=0x10027d0f0d0, opcode=64, args=0x1ffffff80030) at /home/greg/Work/ibm/linux/qemu-agraf/hw/ppc/spapr_hcall.c:737
#11 0x00000000104cf424 in kvm_arch_handle_exit (cs=0x10027d0f0d0, run=0x1ffffff80000) at /home/greg/Work/ibm/linux/qemu-agraf/target-ppc/kvm.c:1223
#12 0x00000000104648a4 in kvm_cpu_exec (cpu=0x10027d0f0d0) at /home/greg/Work/ibm/linux/qemu-agraf/kvm-all.c:1736
#13 0x0000000010397f00 in qemu_kvm_cpu_thread_fn (arg=0x10027d0f0d0) at /home/greg/Work/ibm/linux/qemu-agraf/cpus.c:874
#14 0x00001fffff92c29c in start_thread (arg=0x1ffffbdaeef0) at pthread_create.c:310
#15 0x00001ffffde5de10 in .__clone ()
at ../sysdeps/unix/sysv/linux/powerpc/powerpc64/clone.S:111

All I can say for the moment, is that I don't get that if I run qemu with
-kernel/-append/-initrd instead of following the grub2 path.

Any clue ?

Thanks.

-- 
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: remove io ports workaround

[Alexey Kardashevskiy]

On 01/06/2014 10:12 PM, Greg Kurz wrote:
> On Fri, 03 Jan 2014 09:08:21 +1100
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>
>> Please read the rest of this thread. It does not visibly break things but
>> with this patch QEMU starts calling unassigned_mem_accepts() (normally
>> silent) which is not a good sign.
>>
>>
>>
> 
> Hmm... this is only because this patch moves the PHB io region from the
> system IO to the system memory space, but the bogus(?) write to unassigned
> memory already exists.
> 
> I have tested against the current ppc-next (62d529a), with no
> additional patch:
> 
> qemu-system-ppc64 \
> -snapshot -S -monitor stdio -serial pty \
> -nographic -nodefaults \
> -machine type=pseries,accel=kvm -smp 1 -m 4G \
> -device virtio-blk-pci,id=virtioiblk0,drive=drive0,bootindex=20,ioeventfd=on \
> -drive file=/local/greg/qemu/fedora-be.qcow2,if=none,id=drive0,readonly=off,\
>        format=qcow2,media=disk,werror=stop,rerror=stop,discard=on
> 
> where fedora-be.qcow2 contains a stock fedora 19 for ppc64.
> 
> I have attached gdb to qemu and set a breakpoint in unassigned_io_write(), and
> here is what I get again:
> 
> (gdb) b unassigned_io_write 
> Breakpoint 1 at 0x1045d308: file /home/greg/Work/ibm/linux/qemu-agraf/ioport.c, line 54.
> (gdb) c
> Continuing.
> [Thread 0x1ffffc5deef0 (LWP 11946) exited]
> [New Thread 0x1ffffc5deef0 (LWP 11955)]
> [Switching to Thread 0x1ffffbdaeef0 (LWP 11947)]
> 
> Breakpoint 1, unassigned_io_write (opaque=0x0, addr=82, val=128, size=1) at /home/greg/Work/ibm/linux/qemu-agraf/ioport.c:54
> 54      {
> (gdb) where
> #0  unassigned_io_write (opaque=0x0, addr=82, val=128, size=1) at /home/greg/Work/ibm/linux/qemu-agraf/ioport.c:54
> #1  0x0000000010468f38 in memory_region_write_accessor (mr=0x10027615940, addr=82, value=0x1ffffbdadd68, size=1, shift=0, mask=255) at /home/greg/Work/ibm/linux/qemu-agraf/memory.c:440
> #2  0x00000000104690c4 in access_with_adjusted_size (addr=82, value=0x1ffffbdadd68, size=1, access_size_min=1, access_size_max=4, access=@0x107ca670: 0x10468e5c <memory_region_write_accessor>, mr=0x10027615940)
>     at /home/greg/Work/ibm/linux/qemu-agraf/memory.c:472
> #3  0x000000001046bc64 in memory_region_dispatch_write (mr=0x10027615940, addr=82, data=128, size=1) at /home/greg/Work/ibm/linux/qemu-agraf/memory.c:984
> #4  0x000000001046fdc4 in io_mem_write (mr=0x10027615940, addr=82, val=128, size=1) at /home/greg/Work/ibm/linux/qemu-agraf/memory.c:1749
> #5  0x00000000103aca0c in address_space_rw (as=0x10c19638 <address_space_memory>, addr=1101659111506, buf=0x1ffffbdae117 "\200", len=1, is_write=true) at /home/greg/Work/ibm/linux/qemu-agraf/exec.c:2002
> #6  0x00000000103acf3c in cpu_physical_memory_rw (addr=1101659111506, buf=0x1ffffbdae117 "\200", len=1, is_write=1) at /home/greg/Work/ibm/linux/qemu-agraf/exec.c:2071
> #7  0x00000000103a44c4 in cpu_physical_memory_write (addr=1101659111506, buf=0x1ffffbdae117, len=1) at /home/greg/Work/ibm/linux/qemu-agraf/include/exec/cpu-common.h:68
> #8  0x00000000103aeb2c in stb_phys (addr=1101659111506, val=128) at /home/greg/Work/ibm/linux/qemu-agraf/exec.c:2600
> #9  0x0000000010438550 in h_logical_store (cpu=0x10027d0f0d0, spapr=0x100276bb210, opcode=64, args=0x1ffffff80030) at /home/greg/Work/ibm/linux/qemu-agraf/hw/ppc/spapr_hcall.c:564
> #10 0x0000000010438e74 in spapr_hypercall (cpu=0x10027d0f0d0, opcode=64, args=0x1ffffff80030) at /home/greg/Work/ibm/linux/qemu-agraf/hw/ppc/spapr_hcall.c:737
> #11 0x00000000104cf424 in kvm_arch_handle_exit (cs=0x10027d0f0d0, run=0x1ffffff80000) at /home/greg/Work/ibm/linux/qemu-agraf/target-ppc/kvm.c:1223
> #12 0x00000000104648a4 in kvm_cpu_exec (cpu=0x10027d0f0d0) at /home/greg/Work/ibm/linux/qemu-agraf/kvm-all.c:1736
> #13 0x0000000010397f00 in qemu_kvm_cpu_thread_fn (arg=0x10027d0f0d0) at /home/greg/Work/ibm/linux/qemu-agraf/cpus.c:874
> #14 0x00001fffff92c29c in start_thread (arg=0x1ffffbdaeef0) at pthread_create.c:310
> #15 0x00001ffffde5de10 in .__clone ()
> at ../sysdeps/unix/sysv/linux/powerpc/powerpc64/clone.S:111
> 
> All I can say for the moment, is that I don't get that if I run qemu with
> -kernel/-append/-initrd instead of following the grub2 path.
> 
> Any clue ?


I've got nothing... Can you try without "ioeventfd=on"?
If you post gdb output next time, do "set radix 0x10" first :)


-- 
Alexey

Re: [Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Alexey Kardashevskiy]

On 02/08/2014 01:06 AM, Alexander Graf wrote:
> 
> On 07.02.2014, at 14:44, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> 
>> In the past, IO space could not be mapped into the memory address space
>> so we introduced a workaround for that. Nowadays it does not look
>> necessary so we can remove the workaround and make sPAPR PCI
>> configuration simplier.
>>
>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> 
> Alexey, I would like at least your ack for this. This patch would be the 3rd or 4th attempt to do this shift and every time we saw unexpected breakage :).


Seems to be all right, cannot find any problem this time :) Thanks!

Acked-by: Alexey Kardashevskiy <aik@ozlabs.ru>



-- 
Alexey

Re: [Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Alexander Graf]

On 07.02.2014, at 14:44, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> In the past, IO space could not be mapped into the memory address space
> so we introduced a workaround for that. Nowadays it does not look
> necessary so we can remove the workaround and make sPAPR PCI
> configuration simplier.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

Alexey, I would like at least your ack for this. This patch would be the 3rd or 4th attempt to do this shift and every time we saw unexpected breakage :).


Alex

[Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Greg Kurz]

In the past, IO space could not be mapped into the memory address space
so we introduced a workaround for that. Nowadays it does not look
necessary so we can remove the workaround and make sPAPR PCI
configuration simplier.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---

There has been a previous post for this patch:

        http://patchwork.ozlabs.org/patch/299123/

Since then, things have evolved:
- the kvm_mem_ioeventfd_add failure was fixed by the "KVM: fix addr type 
  for KVM_IOEVENTFD" commit (584f2be).
- Alexey's NACK got addressed in SLOF:

        https://github.com/aik/SLOF/commit/020220e

Cheers.

--
Greg

 hw/ppc/spapr_pci.c |   13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 66ddf10..3b7d978 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -564,23 +564,14 @@ static int spapr_phb_init(SysBusDevice *s)
     memory_region_add_subregion(get_system_memory(), sphb->mem_win_addr,
                                 &sphb->memwindow);
 
-    /* On ppc, we only have MMIO no specific IO space from the CPU
-     * perspective.  In theory we ought to be able to embed the PCI IO
-     * memory region direction in the system memory space.  However,
-     * if any of the IO BAR subregions use the old_portio mechanism,
-     * that won't be processed properly unless accessed from the
-     * system io address space.  This hack to bounce things via
-     * system_io works around the problem until all the users of
-     * old_portion are updated */
+    /* Initialize IO regions */
     sprintf(namebuf, "%s.io", sphb->dtbusname);
     memory_region_init(&sphb->iospace, OBJECT(sphb),
                        namebuf, SPAPR_PCI_IO_WIN_SIZE);
-    /* FIXME: fix to support multiple PHBs */
-    memory_region_add_subregion(get_system_io(), 0, &sphb->iospace);
 
     sprintf(namebuf, "%s.io-alias", sphb->dtbusname);
     memory_region_init_alias(&sphb->iowindow, OBJECT(sphb), namebuf,
-                             get_system_io(), 0, SPAPR_PCI_IO_WIN_SIZE);
+                             &sphb->iospace, 0, SPAPR_PCI_IO_WIN_SIZE);
     memory_region_add_subregion(get_system_memory(), sphb->io_win_addr,
                                 &sphb->iowindow);
     /*