Refactor portio dispatching

From: Jan Kiszka <[email protected]>

The current ioport dispatcher is a complex beast, mostly due to the need to deal with old portio interface users. But we can overcome it without converting all portio users by embedding the required base address of a MemoryRegionPortio access into that data structure. That removes the need to have the additional MemoryRegionIORange structure in the loop on every access.

To handle old portio memory ops, we simply install dispatching handlers for portio memory regions when registering them with the memory core. This removes the need for the old_portio field.

We can drop the additional aliasing of ioport regions and also the special address space listener. cpu_in and cpu_out now simply call address_space_read/write. And we can concentrate portio handling in a single source file.

Signed-off-by Jan Kiszka <[email protected]>

---

ioport.c +14 -0

+
+static void portio_write(void *opaque, hwaddr addr, uint64_t data,
+                         unsigned size)
+{
+    MemoryRegionPortioList *mrpio = opaque;
+    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size, true);
+
+    if (mrp) {
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
+    } else if (size == 2) {
+        mrp = find_portio(mrpio, addr, 1, true);
+        assert(mrp);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data >> 8);

Quoted 20 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

The current ioport dispatcher is a complex beast, mostly due to the need to deal with old portio interface users. But we can overcome it without converting all portio users by embedding the required base address of a MemoryRegionPortio access into that data structure. That removes the need to have the additional MemoryRegionIORange structure in the loop on every access.

To handle old portio memory ops, we simply install dispatching handlers for portio memory regions when registering them with the memory core. This removes the need for the old_portio field.

We can drop the additional aliasing of ioport regions and also the special address space listener. cpu_in and cpu_out now simply call address_space_read/write. And we can concentrate portio handling in a single source file.

Signed-off-by Jan Kiszka <[email protected]>

---

...

Quoted 16 lines

ioport.c +6 -0

+
+static void portio_write(void *opaque, hwaddr addr, uint64_t data,
+                         unsigned size)
+{
+    MemoryRegionPortioList *mrpio = opaque;
+    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size,

true);

ioport.c +8 -0

+
+    if (mrp) {
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
+    } else if (size == 2) {
+        mrp = find_portio(mrpio, addr, 1, true);
+        assert(mrp);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data

Quoted 1 line

Quoted 1 line

8);

ioport.c void portio_list_init(PortioList *piolist, · line 425

Quoted 10 lines

ioport.c +10 -0

+    }
+}
+
+static const MemoryRegionOps portio_ops = {
+    .read = portio_read,
+    .write = portio_write,
+    .valid.unaligned = true,
+    .impl.unaligned = true,
+};
+

You need to mark these operations as DEVICE_LITTLE_ENDIAN. In portio_write above, you clearly assume that data is in LE format.

This fixes PPC PReP emulation, which would otherwise be broken with this patchset.

Quoted 40 lines · Hervé Poussineau

Quoted 20 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

The current ioport dispatcher is a complex beast, mostly due to the need to deal with old portio interface users. But we can overcome it without converting all portio users by embedding the required base address of a MemoryRegionPortio access into that data structure. That removes the need to have the additional MemoryRegionIORange structure in the loop on every access.

To handle old portio memory ops, we simply install dispatching handlers for portio memory regions when registering them with the memory core. This removes the need for the old_portio field.

We can drop the additional aliasing of ioport regions and also the special address space listener. cpu_in and cpu_out now simply call address_space_read/write. And we can concentrate portio handling in a single source file.

Signed-off-by Jan Kiszka <[email protected]>

---

...

Quoted 16 lines

ioport.c +6 -0

+
+static void portio_write(void *opaque, hwaddr addr, uint64_t data,
+                         unsigned size)
+{
+    MemoryRegionPortioList *mrpio = opaque;
+    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size,

true);

ioport.c +8 -0

+
+    if (mrp) {
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
+    } else if (size == 2) {
+        mrp = find_portio(mrpio, addr, 1, true);
+        assert(mrp);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data

Quoted 1 line

Quoted 1 line

8);

Quoted 13 lines

ioport.c void portio_list_init(PortioList *piolist, · line 425

Quoted 10 lines

ioport.c +10 -0

+    }
+}
+
+static const MemoryRegionOps portio_ops = {
+    .read = portio_read,
+    .write = portio_write,
+    .valid.unaligned = true,
+    .impl.unaligned = true,
+};
+

You need to mark these operations as DEVICE_LITTLE_ENDIAN. In portio_write above, you clearly assume that data is in LE format.

Anything behind PIO is little endian, of course. Will add this.

Quoted 4 lines

This fixes PPC PReP emulation, which would otherwise be broken with this patchset.

Thanks,
Jan

Quoted 55 lines · Jan Kiszka

Quoted 53 lines · Hervé Poussineau

Quoted 20 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

The current ioport dispatcher is a complex beast, mostly due to the need to deal with old portio interface users. But we can overcome it without converting all portio users by embedding the required base address of a MemoryRegionPortio access into that data structure. That removes the need to have the additional MemoryRegionIORange structure in the loop on every access.

To handle old portio memory ops, we simply install dispatching handlers for portio memory regions when registering them with the memory core. This removes the need for the old_portio field.

We can drop the additional aliasing of ioport regions and also the special address space listener. cpu_in and cpu_out now simply call address_space_read/write. And we can concentrate portio handling in a single source file.

Signed-off-by Jan Kiszka <[email protected]>

---

...

Quoted 16 lines

ioport.c +6 -0

+
+static void portio_write(void *opaque, hwaddr addr, uint64_t data,
+                         unsigned size)
+{
+    MemoryRegionPortioList *mrpio = opaque;
+    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size,

true);

ioport.c +8 -0

+
+    if (mrp) {
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
+    } else if (size == 2) {
+        mrp = find_portio(mrpio, addr, 1, true);
+        assert(mrp);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data

Quoted 1 line

Quoted 1 line

8);

ioport.c void portio_list_init(PortioList *piolist, · line 425

Quoted 10 lines

ioport.c +10 -0

+    }
+}
+
+static const MemoryRegionOps portio_ops = {
+    .read = portio_read,
+    .write = portio_write,
+    .valid.unaligned = true,
+    .impl.unaligned = true,
+};
+

You need to mark these operations as DEVICE_LITTLE_ENDIAN. In portio_write above, you clearly assume that data is in LE format.

Quoted 1 line

Anything behind PIO is little endian, of course. Will add this.

This patch breaks VGA on PPC as it is in master today.

Quoted 59 lines · Jan Kiszka

Quoted 56 lines · Hervé Poussineau

Quoted 53 lines

Quoted 20 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

The current ioport dispatcher is a complex beast, mostly due to the need to deal with old portio interface users. But we can overcome it without converting all portio users by embedding the required base address of a MemoryRegionPortio access into that data structure. That removes the need to have the additional MemoryRegionIORange structure in the loop on every access.

To handle old portio memory ops, we simply install dispatching handlers for portio memory regions when registering them with the memory core. This removes the need for the old_portio field.

We can drop the additional aliasing of ioport regions and also the special address space listener. cpu_in and cpu_out now simply call address_space_read/write. And we can concentrate portio handling in a single source file.

Signed-off-by Jan Kiszka <[email protected]>

---

...

Quoted 16 lines

ioport.c +6 -0

+
+static void portio_write(void *opaque, hwaddr addr, uint64_t data,
+                         unsigned size)
+{
+    MemoryRegionPortioList *mrpio = opaque;
+    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size,

true);

ioport.c +8 -0

+
+    if (mrp) {
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
+    } else if (size == 2) {
+        mrp = find_portio(mrpio, addr, 1, true);
+        assert(mrp);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data

Quoted 1 line

Quoted 1 line

8);

ioport.c void portio_list_init(PortioList *piolist, · line 425

Quoted 10 lines

ioport.c +10 -0

+    }
+}
+
+static const MemoryRegionOps portio_ops = {
+    .read = portio_read,
+    .write = portio_write,
+    .valid.unaligned = true,
+    .impl.unaligned = true,
+};
+

You need to mark these operations as DEVICE_LITTLE_ENDIAN. In portio_write above, you clearly assume that data is in LE format.

Quoted 3 lines

Quoted 1 line

Anything behind PIO is little endian, of course. Will add this.

This patch breaks VGA on PPC as it is in master today.

If I don't mark portio as little endian it works as expected. There's probably someone swapping things twice.

Am 11.07.2013 14:34, schrieb Alexander Graf:

Quoted 61 lines · Alexander Graf

Quoted 59 lines · Jan Kiszka

Quoted 56 lines · Hervé Poussineau

Quoted 53 lines

Quoted 20 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

The current ioport dispatcher is a complex beast, mostly due to the need to deal with old portio interface users. But we can overcome it without converting all portio users by embedding the required base address of a MemoryRegionPortio access into that data structure. That removes the need to have the additional MemoryRegionIORange structure in the loop on every access.

To handle old portio memory ops, we simply install dispatching handlers for portio memory regions when registering them with the memory core. This removes the need for the old_portio field.

We can drop the additional aliasing of ioport regions and also the special address space listener. cpu_in and cpu_out now simply call address_space_read/write. And we can concentrate portio handling in a single source file.

Signed-off-by Jan Kiszka <[email protected]>

---

...

Quoted 16 lines

ioport.c +6 -0

+
+static void portio_write(void *opaque, hwaddr addr, uint64_t data,
+                         unsigned size)
+{
+    MemoryRegionPortioList *mrpio = opaque;
+    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size,

true);

ioport.c +8 -0

+
+    if (mrp) {
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
+    } else if (size == 2) {
+        mrp = find_portio(mrpio, addr, 1, true);
+        assert(mrp);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data

Quoted 1 line

Quoted 1 line

8);

ioport.c void portio_list_init(PortioList *piolist, · line 425

Quoted 10 lines

ioport.c +10 -0

+    }
+}
+
+static const MemoryRegionOps portio_ops = {
+    .read = portio_read,
+    .write = portio_write,
+    .valid.unaligned = true,
+    .impl.unaligned = true,
+};
+

You need to mark these operations as DEVICE_LITTLE_ENDIAN. In portio_write above, you clearly assume that data is in LE format.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Anything behind PIO is little endian, of course. Will add this.

This patch breaks VGA on PPC as it is in master today.

If I don't mark portio as little endian it works as expected. There's probably someone swapping things twice.

sPAPR has its MemoryRegion marked Little Endian:

Quoted 64 lines · Andreas Färber

Am 11.07.2013 14:34, schrieb Alexander Graf:

Quoted 63 lines · Alexander Graf

Quoted 61 lines · Jan Kiszka

Quoted 57 lines · Hervé Poussineau

Quoted 40 lines

Quoted 20 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

The current ioport dispatcher is a complex beast, mostly due to the need to deal with old portio interface users. But we can overcome it without converting all portio users by embedding the required base address of a MemoryRegionPortio access into that data structure. That removes the need to have the additional MemoryRegionIORange structure in the loop on every access.

To handle old portio memory ops, we simply install dispatching handlers for portio memory regions when registering them with the memory core. This removes the need for the old_portio field.

We can drop the additional aliasing of ioport regions and also the special address space listener. cpu_in and cpu_out now simply call address_space_read/write. And we can concentrate portio handling in a single source file.

Signed-off-by Jan Kiszka <[email protected]>

---

...

Quoted 16 lines

(quoted diff) +6 -0

+
+static void portio_write(void *opaque, hwaddr addr, uint64_t data,
+                         unsigned size)
+{
+    MemoryRegionPortioList *mrpio = opaque;
+    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size,

true);

(quoted diff) +8 -0

+
+    if (mrp) {
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
+    } else if (size == 2) {
+        mrp = find_portio(mrpio, addr, 1, true);
+        assert(mrp);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data

Quoted 1 line

Quoted 1 line

8);

Quoted 13 lines

Review

Quoted 10 lines

(quoted diff) +10 -0

+    }
+}
+
+static const MemoryRegionOps portio_ops = {
+    .read = portio_read,
+    .write = portio_write,
+    .valid.unaligned = true,
+    .impl.unaligned = true,
+};
+

You need to mark these operations as DEVICE_LITTLE_ENDIAN. In portio_write above, you clearly assume that data is in LE format.

Anything behind PIO is little endian, of course. Will add this.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

This patch breaks VGA on PPC as it is in master today.

If I don't mark portio as little endian it works as expected. There's probably someone swapping things twice.

sPAPR has its MemoryRegion marked Little Endian:

sPAPR works, it's only the Mac machines that break.

Quoted 71 lines · Alexander Graf

Quoted 68 lines · Andreas Färber

Am 11.07.2013 14:34, schrieb Alexander Graf:

Quoted 66 lines · Alexander Graf

Quoted 59 lines · Jan Kiszka

Quoted 56 lines · Hervé Poussineau

Quoted 53 lines

Quoted 20 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

The current ioport dispatcher is a complex beast, mostly due to the need to deal with old portio interface users. But we can overcome it without converting all portio users by embedding the required base address of a MemoryRegionPortio access into that data structure. That removes the need to have the additional MemoryRegionIORange structure in the loop on every access.

To handle old portio memory ops, we simply install dispatching handlers for portio memory regions when registering them with the memory core. This removes the need for the old_portio field.

We can drop the additional aliasing of ioport regions and also the special address space listener. cpu_in and cpu_out now simply call address_space_read/write. And we can concentrate portio handling in a single source file.

Signed-off-by Jan Kiszka <[email protected]>

---

...

Quoted 16 lines

(quoted diff) +6 -0

+
+static void portio_write(void *opaque, hwaddr addr, uint64_t data,
+                         unsigned size)
+{
+    MemoryRegionPortioList *mrpio = opaque;
+    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size,

true);

(quoted diff) +8 -0

+
+    if (mrp) {
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
+    } else if (size == 2) {
+        mrp = find_portio(mrpio, addr, 1, true);
+        assert(mrp);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data

Quoted 1 line

Quoted 1 line

8);

Review

Quoted 10 lines

(quoted diff) +10 -0

+    }
+}
+
+static const MemoryRegionOps portio_ops = {
+    .read = portio_read,
+    .write = portio_write,
+    .valid.unaligned = true,
+    .impl.unaligned = true,
+};
+

You need to mark these operations as DEVICE_LITTLE_ENDIAN. In portio_write above, you clearly assume that data is in LE format.

Quoted 3 lines

Quoted 1 line

Anything behind PIO is little endian, of course. Will add this.

This patch breaks VGA on PPC as it is in master today.

If I don't mark portio as little endian it works as expected. There's probably someone swapping things twice.

Quoted 3 lines

Quoted 1 line

sPAPR has its MemoryRegion marked Little Endian:

sPAPR works, it's only the Mac machines that break.

So IIUC before cpu_inw and inl were doing portio accesses in host native endianness. Now with this change they do little endian accesses. All sensible callers of cpu_inX assume that data is passed in native endianness though and already do the little endian conversion themselves.

Semantically having your PCI host bridge do the endianness conversion is the correct way of handling it, as that's where the conversion happens. If it makes life easier to do it in the isa bridging code, that's fine for me too though. But then we'll have to get rid of all endianness swaps that already happen in PCI bridges.

So IIUC before cpu_inw and inl were doing portio accesses in host native endianness. Now with this change they do little endian accesses. All sensible callers of cpu_inX assume that data is passed in native endianness though and already do the little endian conversion themselves.

Semantically having your PCI host bridge do the endianness conversion is the correct way of handling it, as that's where the conversion happens. If it makes life easier to do it in the isa bridging code, that's fine for me too though. But then we'll have to get rid of all endianness swaps that already happen in PCI bridges.

Semantically having your PCI host bridge do the endianness conversion is the correct way of handling it, as that's where the conversion happens. If it makes life easier to do it in the isa bridging code, that's fine for me too though. But then we'll have to get rid of all endianness swaps that already happen in PCI bridges.

Quoted 6 lines

Semantically having your PCI host bridge do the endianness conversion is the correct way of handling it, as that's where the conversion happens. If it makes life easier to do it in the isa bridging code, that's fine for me too though. But then we'll have to get rid of all endianness swaps that already happen in PCI bridges.

BTW. Why would IOs to a PCI VGA device go through an "ISA bridge" of any kind ? PCI IO space is PCI IO space... whether there is an ISA bridge or not is a separate thing and PCI VGA cards certainly don't sit behind one.

Ben.

We model a single system wide io space today and access to that one happens through you pci host controller. I just messed up the terminology here.

Quoted 3 lines · Alexander Graf

We model a single system wide io space today and access to that one happens through you pci host controller. I just messed up the terminology here.

Single system wide IO space is broken. We have separate IO space per PHB. That was working afaik.

In any case, I completely object to all that business with conversion in bridges.

That's fundamentally WRONG.

The whole business of endianness in qemu is a mess. In the end what matters and the only thing that does is:

* The endianness of a given memory access by the guest (which may or
may not be the endianness of the guest -> MSR:LE, byteswap load/store instsructions, etc..)

When only one of them is changed, this means some "interesting" things can happen. For example, in [1]:
"When PowerPC little-endian is in effect but before system's little-endian is in effect, the address munge becomes effective. If we need to access system endian port (byte) address, say 0x80000092, the program needs to issue 0x80000095 instead, to compensate (unmunge) for the effect of PowerPC address modification."

Those will be required for QEMU to be able to run BeOS, AIX, or Windows NT or PowerPC PReP.

In the light of this, I think there should only be one endianness for all memory accesses (which could be changed at runtime),

and all
bridges/devices should ask for "same endianness as parent" or "reverse endianness as parent", but not for big, little, or native endianness.

Regards,

Hervé

[1] ftp://ftp.software.ibm.com/rs6000/technology/spec/endian.ps)

Quoted 1 line

In the light of this, I think there should only be one endianness

for

Quoted 1 line

all memory accesses (which could be changed at runtime),

We already do this, it's "host native endian".

Quoted 2 lines

and all
bridges/devices should ask for "same endianness as parent" or

"reverse

Quoted 1 line

endianness as parent", but not for big, little, or native

endianness.

I/O doesn't propagate so there's no way to say something like this. That's the fundamental problem IMHO.

"When PowerPC little-endian is in effect but before system's little-endian is in effect, the address munge becomes effective. If we need to access system endian port (byte) address, say 0x80000092, the program needs to issue 0x80000095 instead, to compensate (unmunge) for the effect of PowerPC address modification."

Those will be required for QEMU to be able to run BeOS, AIX, or Windows NT or PowerPC PReP.

In the light of this, I think there should only be one endianness for all memory accesses (which could be changed at runtime), and all bridges/devices should ask for "same endianness as parent" or "reverse endianness as parent", but not for big, little, or native endianness.

Quoted 3 lines · Alexander Graf

We model a single system wide io space today and access to that one happens through you pci host controller. I just messed up the terminology here.

Single system wide IO space is broken. We have separate IO space per PHB. That was working afaik.

In any case, I completely object to all that business with conversion in bridges.

That's fundamentally WRONG.

The whole business of endianness in qemu is a mess. In the end what matters and the only thing that does is:

* The endianness of a given memory access by the guest (which may or
may not be the endianness of the guest -> MSR:LE, byteswap load/store instsructions, etc..)

vs.

* The endianness of the target device register (and I say register ...
a framebuffer does NOT have endianness per-se and thus accesses to BAR mapping a "memory" range (framebuffer, ROM, ...) should go such that the *byte order* of individual bytes is preserved, which typically means untranslated).

Unless they are completely broken (and those exist, don't get me wrong, though mostly they are a thing of a past long gone), bridges and busses have no effect on endianness.

So I'm not sure what you guys are up to, but from what I read, it's wrong, and the fact at this stage is that your broke IO space (and thus virtio and VGA) on powerpc (including pseries).

Cheers,
Ben.

Quoted 7 lines · Benjamin Herrenschmidt

Quoted 3 lines · Alexander Graf

We model a single system wide io space today and access to that one happens through you pci host controller. I just messed up the terminology here.

Single system wide IO space is broken. We have separate IO space per PHB. That was working afaik.

Hrm, probably not. We don't propagate I/O spaces very well today.

Quoted 4 lines

In any case, I completely object to all that business with conversion in bridges.

That's fundamentally WRONG.

Quoted 2 lines

The whole business of endianness in qemu is a mess. In the end what matters and the only thing that does is:

It's not as bad as you think I suspect.

Quoted 3 lines

* The endianness of a given memory access by the guest (which may or
may not be the endianness of the guest -> MSR:LE, byteswap load/store instsructions, etc..)

Correct.

Quoted 7 lines

vs.

* The endianness of the target device register (and I say register ...
a framebuffer does NOT have endianness per-se and thus accesses to BAR mapping a "memory" range (framebuffer, ROM, ...) should go such that the *byte order* of individual bytes is preserved, which typically means untranslated).

Yes. To put it another way, an MMIO write is a store and depending on the VCPU, that will result in a write with a certain byte order. That byte order should be preserved.

However, what we don't model today, and why we have the silly endianness in MemoryRegionOps, is the fact that I/O may pass through multiple layers and those layers may change byte ordering.

We jump through great hoops to have a flat dispatch table. I've never liked it but that's what we do. That means that in cases where a host bridge may do byte swapping, we cannot easily support that.

It's not wrong when the hardware actually does a byteswap at some level in the memory hierarchy. You can see this for instance on ARMv7M systems, where byteswapping for bigendian happens at an intermediate level that not all accesses go through:

[CPU] ----> [byteswap here] --> [memory and ext. devices]

         |
         -->  [internal memory mapped devices]

so some things see always little endian regardless.

Quoted 23 lines

Quoted 2 lines

The whole business of endianness in qemu is a mess. In the end what matters and the only thing that does is:

It's not as bad as you think I suspect.

Quoted 3 lines

* The endianness of a given memory access by the guest (which may or
may not be the endianness of the guest -> MSR:LE, byteswap load/store instsructions, etc..)

Correct.

Quoted 7 lines

vs.

* The endianness of the target device register (and I say register ...
a framebuffer does NOT have endianness per-se and thus accesses to BAR mapping a "memory" range (framebuffer, ROM, ...) should go such that the *byte order* of individual bytes is preserved, which typically means untranslated).

Yes. To put it another way, an MMIO write is a store and depending on the VCPU, that will result in a write with a certain byte order. That byte order should be preserved.

Quoted 7 lines

However, what we don't model today, and why we have the silly endianness in MemoryRegionOps, is the fact that I/O may pass through multiple layers and those layers may change byte ordering.

We jump through great hoops to have a flat dispatch table. I've never liked it but that's what we do. That means that in cases where a host bridge may do byte swapping, we cannot easily support that.

We could support that if we cared to -- you just have to have a container MemoryRegion type which is a byte-swapping container (or just have a flag on existing containers, I suppose). Then as you flatten the regions into the flat table you keep track of how many levels of byteswapping each region goes through, and you end up with a single 'byteswap or not?' flag for each section of your flat dispatch table.

(Our other serious endianness problem is that we don't really do very well at supporting a TCG CPU arbitrarily flipping endianness -- TARGET_WORDS_BIGENDIAN is a compile time setting and ideally it should not be.)

Quoted 10 lines · Peter Maydell

It's not wrong when the hardware actually does a byteswap at some level in the memory hierarchy. You can see this for instance on ARMv7M systems, where byteswapping for bigendian happens at an intermediate level that not all accesses go through:

[CPU] ----> [byteswap here] --> [memory and ext. devices]

         |
         -->  [internal memory mapped devices]

so some things see always little endian regardless.

Ugh ? That's so completely fucked up, if that's indeed what the HW is doing this is a piece of trash and the designers are in urgent need of being turned into fertilizer.

Unless again you are talking about "lane swapping" which allows to preserve the byte address invariance when the CPU decides to flip its bus around, but I would have thought that modern CPUs do not do that sort of shit anymore.

Again, the only endian
attribute that exists are the byte order of the original access (which byte has the lowest address, regardless of significance of those bytes in the target, ie, purely from a qemu standpoint, in the variable that carries the access around inside qemu, which byte has the lowest address)

, and the same on the target device (at which point a concept of significance does apply, but it's a guest driver business to get it right, qemu just need to make sure byte 0 goes to byte 0).

If a bridge flips things around in a way that breaks the model,

then add some property describing the flipping
properties but don't call it "big
endian" or "little endian" at the bridge level, that has no meaning, confuses things and introduces breakage like we have seen.

Quoted 4 lines

(Our other serious endianness problem is that we don't really do very well at supporting a TCG CPU arbitrarily flipping endianness -- TARGET_WORDS_BIGENDIAN is a compile time setting and ideally it should not be.)

Our experience is that it actually works fine for almost everything except virtio :-) ie mostly TARGET_WORDS_BIGENDIAN is irrelevant (and should be).

The block marked "byteswap here" does "byte invariant bigendian", so byte accesses are unchanged, 16 bit accesses have the two words of data flipped, and 32 bit accesses have the four bytes flipped; this happens as the data passes through; addresses are unchanged. It only happens if the CPU is configured by the guest to operate in big-endian mode, obviously.
(Contrast 'word invariant bigendian', which is what ARM used to do, where the addresses are changed but the data is not. That would be pretty painful to implement in the memory region API though it is of course trivial in hardware since it is just XORing of the low address bits according to the access size...)

Quoted 6 lines

Again, the only endian
attribute that exists are the byte order of the original access (which byte has the lowest address, regardless of significance of those bytes in the target, ie, purely from a qemu standpoint, in the variable that carries the access around inside qemu, which byte has the lowest address)

What does this even mean? At the point where a memory access leaves the CPU (emulation or real hardware) it has (a) an address and
(b) a width -- a 16 bit access is neither big nor little endian,

it's just a request for 16 bits of data (on real hardware it's typically a bus transaction on a bunch of data lines with some control lines indicating transaction width). Now the CPU emulation may internally be intending to put that data into its emulated register one way round or the other, but that's an internal detail of the CPU emulation. (Similarly for stores.)

Quoted 3 lines

, and the same on the target device (at which point a concept of significance does apply, but it's a guest driver business to get it right, qemu just need to make sure byte 0 goes to byte 0).

Similarly, at the target device end there is no concept of a "big endian access" -- we make a request for 16 bits of data at a particular address (via the MemoryRegion API) and the device returns 16 bits of data.

It's entirely
possible to design hardware so that byte access to address X, halfword access to address X and word access to address X all return entirely different data (though it would be a bit perverse.) (As an implementation convenience we may choose to provide helper infrastructure so you don't have to actually implement all of byte/halfword/word access by hand.)

Quoted 2 lines

If a bridge flips things around in a way that breaks the model,

That breaks what model?

Quoted 4 lines

then add some property describing the flipping
properties but don't call it "big
endian" or "little endian" at the bridge level, that has no meaning, confuses things and introduces breakage like we have seen.

I'm happy to call the property "byteswap", yes, because that's what it does. If you did two of these in a row you'd get a no-op.

Quoted 8 lines

Quoted 4 lines

(Our other serious endianness problem is that we don't really do very well at supporting a TCG CPU arbitrarily flipping endianness -- TARGET_WORDS_BIGENDIAN is a compile time setting and ideally it should not be.)

Our experience is that it actually works fine for almost everything except virtio :-) ie mostly TARGET_WORDS_BIGENDIAN is irrelevant (and should be).

I agree that TARGET_WORDS_BIGENDIAN *should* go away, but it exists currently. Do you actually implement a CPU which does dynamic endianness flipping? Is it at all efficient in the config which is the opposite of whatever TARGET_WORDS_BIGENDIAN says?

Quoted 3 lines · Benjamin Herrenschmidt

Our experience is that it actually works fine for almost everything except virtio :-) ie mostly TARGET_WORDS_BIGENDIAN is irrelevant (and should be).

I agree that TARGET_WORDS_BIGENDIAN *should* go away, but it exists currently. Do you actually implement a CPU which does dynamic endianness flipping?

Is it at all efficient
in the config which is the opposite of whatever TARGET_WORDS_BIGENDIAN says?

thanks
-- PMM

Quoted 4 lines · Peter Maydell

On 12 July 2013 23:50, Benjamin Herrenschmidt <[email protected]> wrote:
I agree that TARGET_WORDS_BIGENDIAN *should* go away, but
it exists currently. Do you actually implement a CPU which
does dynamic endianness flipping?

TCG already supports bi-endianness for PPC. Grep for 'le_mode' in target-ppc/translate.c to see how the TCG instruction stream is affected by it.

On PPC, le_mode only really affects load/stores and instruction decoding. Any shared data structures between the CPU and OS remain big endian.

So TARGET_WORDS_BIGENDIAN is still accurate even when le_mode=1. It's not the semantic equivalent of changing TARGET_WORDS.

On PPC, le_mode only really affects load/stores and instruction decoding. Any shared data structures between the CPU and OS remain big endian.

So TARGET_WORDS_BIGENDIAN is still accurate even when le_mode=1. It's not the semantic equivalent of changing TARGET_WORDS.

Quoted 4 lines

Unless they are completely broken (and those exist, don't get me wrong, though mostly they are a thing of a past long gone), bridges and busses have no effect on endianness.

That's simply not true. There are programmable PCI host bridges that support byte swapping. Some allow this to be done on a per-device basis too.

Quoted 3 lines

So I'm not sure what you guys are up to, but from what I read, it's wrong, and the fact at this stage is that your broke IO space (and thus virtio and VGA) on powerpc (including pseries).

I'm not sure what this patch was trying to do but it was certainly wrong.

sPAPR has its MemoryRegion marked Little Endian:

http://git.qemu.org/?p=qemu.git;a=blobdiff;f=hw/spapr_pci.c;h=a08ed11166595bdc493065beb64d4ce5b7b0dded;hp=c2c3079d21d5be2647faf85a8c608ac995d2ca62;hb=a3cfa18eb075c7ef78358ca1956fe7b01caa1724;hpb=286d52ebfc0d0d53c2a878e454292fea14bad41b

Possibly we can now apply Hervé's patches on top to remove that hack again?

Il 11/07/2013 14:46, Andreas Färber ha scritto:

Quoted 5 lines

sPAPR has its MemoryRegion marked Little Endian:

http://git.qemu.org/?p=qemu.git;a=blobdiff;f=hw/spapr_pci.c;h=a08ed11166595bdc493065beb64d4ce5b7b0dded;hp=c2c3079d21d5be2647faf85a8c608ac995d2ca62;hb=a3cfa18eb075c7ef78358ca1956fe7b01caa1724;hpb=286d52ebfc0d0d53c2a878e454292fea14bad41b

Possibly we can now apply Hervé's patches on top to remove that hack again?

I can post a pull request with Herve's patch, if we agree that it's the right thing.

Paolo

Quoted 8 lines · Paolo Bonzini

Il 11/07/2013 14:46, Andreas Färber ha scritto:

Quoted 5 lines

sPAPR has its MemoryRegion marked Little Endian:

http://git.qemu.org/?p=qemu.git;a=blobdiff;f=hw/spapr_pci.c;h=a08ed11166595bdc493065beb64d4ce5b7b0dded;hp=c2c3079d21d5be2647faf85a8c608ac995d2ca62;hb=a3cfa18eb075c7ef78358ca1956fe7b01caa1724;hpb=286d52ebfc0d0d53c2a878e454292fea14bad41b

Possibly we can now apply Hervé's patches on top to remove that hack again?

I can post a pull request with Herve's patch, if we agree that it's the right thing.

http://permalink.gmane.org/gmane.comp.emulators.qemu/221950

Here's what's happening:

1) MMIO request goes to sPAPR PIO area, the vCPU was in BE mode but by the time the handler is called, the value is in host byte order.

2) sPAPR (incorrectly) byte swaps by marking the region as little endian (data is now garbage)

3) The portio layer (incorrectly) byte swaps because it is marked as little endian (data is now good)

4) Dispatch happens to VGA device which (incorrectly) byte swaps because it is marked as little endian (data is now bad)

(2), (3), and (4) are all wrong. By removing either (2) or (3) we can "fix" the regression but that's just because two wrongs make a right in this situation.

We should remove *all* of the LE markings from ISA devices, remove the portio mark, and the sPAPR mark. That's the right fix.

1) MMIO request goes to sPAPR PIO area, the vCPU was in BE mode but by the time the handler is called, the value is in host byte order.

2) sPAPR (incorrectly) byte swaps by marking the region as little endian (data is now garbage)

3) The portio layer (incorrectly) byte swaps because it is marked as little endian (data is now good)

4) Dispatch happens to VGA device which (incorrectly) byte swaps because it is marked as little endian (data is now bad)

(2), (3), and (4) are all wrong. By removing either (2) or (3) we can "fix" the regression but that's just because two wrongs make a right in this situation.

We should remove *all* of the LE markings from ISA devices, remove the portio mark, and the sPAPR mark. That's the right fix.

Il 13/07/2013 17:22, Anthony Liguori ha scritto:

Quoted 16 lines

1) MMIO request goes to sPAPR PIO area, the vCPU was in BE mode but by the time the handler is called, the value is in host byte order.

2) sPAPR (incorrectly) byte swaps by marking the region as little endian (data is now garbage)

3) The portio layer (incorrectly) byte swaps because it is marked as little endian (data is now good)

4) Dispatch happens to VGA device which (incorrectly) byte swaps because it is marked as little endian (data is now bad)

(2), (3), and (4) are all wrong. By removing either (2) or (3) we can "fix" the regression but that's just because two wrongs make a right in this situation.

Quoted 2 lines

We should remove *all* of the LE markings from ISA devices, remove the portio mark, and the sPAPR mark. That's the right fix.

So the bug here is that we have multiple levels of dispatch. Byte swapping in the dispatch level only works as long as every dispatch is merged, which is not the case.

However, I do suspect that you have broken PREP again, because PREP has 1/3/4 but not 2.

Removing (2) IIUC amounts to re-applying commit a178274efabcbbc5d44805b51def874e47051325, and I think that's a better fix.

Also, what devices exactly would have a non-native byte order?!? I'm confused...

Paolo

Quoted 2 lines

Also, what devices exactly would have a non-native byte order?!? I'm confused...

MMIO/PIO requests don't have a byte order. It's literally 64 or 32 data pins that are numbered D0..D31 whereas D0 is the LSB. It doesn't matter how the pins are arranged.

Quoted 6 lines · Anthony Liguori

Quoted 2 lines

Also, what devices exactly would have a non-native byte order?!? I'm confused...

MMIO/PIO requests don't have a byte order. It's literally 64 or 32 data pins that are numbered D0..D31 whereas D0 is the LSB. It doesn't matter how the pins are arranged.

Devices themselves do have a byte order, though, right? Specifically, if you do a 32 bit read of address 0 on a device and an 8 bit read,

then you can distinguish a BE device from an LE one. (Most notably, RAM in QEMU is always host-endian...) Devices which only allow 32 bit reads and abort any others wouldn't have an endianness though.

(I need to sit down and think about this all and draw diagrams and look at what we currently do, though. BE guests on LE hosts with and without KVM look particularly thorny.)

-- PMM

Quoted 2 lines · Peter Maydell

Devices themselves do have a byte order, though, right? Specifically, if you do a 32 bit read of address 0 on a device and an 8 bit read,

It depends on the bus and device. Busses don't necessary pass the I/O size down to the device like that. If it does, the device may do any number of things (including ignoring the request entirely.

What's most common AFAIK is that the access is treated as a word access and then truncated. IOW, the device sees the 32-bit word read but somewhere along the way, the top 24 bits are discarded.

The real interesting question is what happens when you do a byte access at address 1. I think most devices simply don't allow that.

Quoted 10 lines · Peter Maydell

Quoted 6 lines · Anthony Liguori

Quoted 2 lines

Also, what devices exactly would have a non-native byte order?!? I'm confused...

MMIO/PIO requests don't have a byte order. It's literally 64 or 32 data pins that are numbered D0..D31 whereas D0 is the LSB. It doesn't matter how the pins are arranged.

Devices themselves do have a byte order, though, right? Specifically, if you do a 32 bit read of address 0 on a device and an 8 bit read,

It depends on the bus and device. Busses don't necessary pass the I/O size down to the device like that. If it does, the device may do any number of things (including ignoring the request entirely.

What's most common AFAIK is that the access is treated as a word access and then truncated. IOW, the device sees the 32-bit word read but somewhere along the way, the top 24 bits are discarded.

The real interesting question is what happens when you do a byte access at address 1. I think most devices simply don't allow that.

Quoted 4 lines

then you can distinguish a BE device from an LE one. (Most notably, RAM in QEMU is always host-endian...) Devices which only allow 32 bit reads and abort any others wouldn't have an endianness though.

My guess is that if you do this with a PCI device we have marked as LE, you'll get a truncated 32-bit read which will make it appear LE. But that doesn't mean it's LE.

Quoted 3 lines

(I need to sit down and think about this all and draw diagrams and look at what we currently do, though. BE guests on LE hosts with and without KVM look particularly thorny.)

I took a first pass at cleaning this up and broke PPC so I'm investigating it further. There may be another layer of silliness hidden somewhere too.

Sorry for sending out invitations and then being late to this party
- vacation. What is the status now? Do we have a short-term plan to avoid the regression or is this better solved by cleaning up the whole endianess thing? Is anyone actively on it, or should I take a drink, sit down and join the discussion?

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 16/07/2013 09:18, Jan Kiszka ha scritto:

Quoted 5 lines

Sorry for sending out invitations and then being late to this party
- vacation. What is the status now? Do we have a short-term plan to avoid the regression or is this better solved by cleaning up the whole endianess thing? Is anyone actively on it, or should I take a drink, sit down and join the discussion?

Basically, we need testing. The current state of the tree is before Herve's patch, which means PREP is (should be) broken.

Alexey posted a patch that reintroduces the DEVICE_LITTLE_ENDIAN and removes the cpu_{in,out}{b,w,l} indirection.

http://permalink.gmane.org/gmane.comp.emulators.qemu/222345

We need to test platforms that used a cpu_{in,out}{b,w,l} indirection (MIPS, PPC, SPARC) with and without Alexey's patch.

The other occurrences of indirections are:

- - hw/isa/i82378.c: This is PREP. Again, removing the indirection should be tested by Herve or Andreas on top of Alexey's patch.

- - hw/isa/isa_mmio: bamboo, g3beige and mac99 could be tested by Alex. I don't know about MIPS. If anything is broken, the solution is to
replace isa_mmio_{setup,init} with an alias to get_system_io(). This stops using isa_mmio altogether, so it can be done only on those platforms where it's needed.

- - hw/pci-host/apb.c: this is SPARC. Perhaps Mark Cave-Ayland can test it and see if it is broken---again with and without Alexey's patch. There is a small difference. This file uses DEVICE_NATIVE_ENDIAN and does the byte swap itself in pci_apb_io{read,write}{b,w,l}.

There is also Alpha. It doesn't matter because it's little endian, but anyway rth is removing the indirection.

Quoted 21 lines · Paolo Bonzini

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 16/07/2013 09:18, Jan Kiszka ha scritto:

Quoted 5 lines

Sorry for sending out invitations and then being late to this party
- vacation. What is the status now? Do we have a short-term plan to avoid the regression or is this better solved by cleaning up the whole endianess thing? Is anyone actively on it, or should I take a drink, sit down and join the discussion?

Basically, we need testing. The current state of the tree is before Herve's patch, which means PREP is (should be) broken.

Alexey posted a patch that reintroduces the DEVICE_LITTLE_ENDIAN and removes the cpu_{in,out}{b,w,l} indirection.

http://permalink.gmane.org/gmane.comp.emulators.qemu/222345

We need to test platforms that used a cpu_{in,out}{b,w,l} indirection (MIPS, PPC, SPARC) with and without Alexey's patch.

Quoted 4 lines

The other occurrences of indirections are:

- - hw/isa/i82378.c: This is PREP. Again, removing the indirection should be tested by Herve or Andreas on top of Alexey's patch.

For i82378, I have a big patch for it, which rewrites large parts of the emulation. Moreover, as i82378 is only used in PReP machine, this indirection can be ignored for now.

Quoted 5 lines

- - hw/isa/isa_mmio: bamboo, g3beige and mac99 could be tested by Alex. I don't know about MIPS. If anything is broken, the solution is to
replace isa_mmio_{setup,init} with an alias to get_system_io(). This stops using isa_mmio altogether, so it can be done only on those platforms where it's needed.

For MIPS Jazz, it is currently broken in SCSI emulation when installing Windows NT 4.0/MIPS. However, it seems quite a general problem on Windows NT 4, as Neozeed also reported the same freeze when running NT4/x86 on http://virtuallyfun.superglobalmegacorp.com/?p=3065 .

Quoted 7 lines

- - hw/pci-host/apb.c: this is SPARC. Perhaps Mark Cave-Ayland can test it and see if it is broken---again with and without Alexey's patch. There is a small difference. This file uses DEVICE_NATIVE_ENDIAN and does the byte swap itself in pci_apb_io{read,write}{b,w,l}.

There is also Alpha. It doesn't matter because it's little endian, but anyway rth is removing the indirection.

Hervé

Quoted 1 line · Alexander Graf

This patch breaks VGA on PPC as it is in master today.

If I don't mark portio as little endian it works as expected. There's probably someone swapping things twice.

Alex

Quoted 6 lines · Alexander Graf

Quoted 1 line · Alexander Graf

This patch breaks VGA on PPC as it is in master today.

If I don't mark portio as little endian it works as expected. There's probably someone swapping things twice.

This is the correct fix. Can you please send a patch?

I/O dispatch functions take a native endian argument and when cpu_{in,out][bwl] is called it's passed a native endian argument. There is no need to ever byte swap.

The ISA bus is going to have data pins numbered D0..D15. There is no concept of endianness. D0 is the LSB. How the CPU stores bytes in memory is orthogonal to what is happening on the bus.

Regards,

Anthony Liguori

Quoted 3 lines

Alex

Quoted 55 lines · Jan Kiszka

Quoted 40 lines · Hervé Poussineau

Quoted 20 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

The current ioport dispatcher is a complex beast, mostly due to the need to deal with old portio interface users. But we can overcome it without converting all portio users by embedding the required base address of a MemoryRegionPortio access into that data structure. That removes the need to have the additional MemoryRegionIORange structure in the loop on every access.

To handle old portio memory ops, we simply install dispatching handlers for portio memory regions when registering them with the memory core. This removes the need for the old_portio field.

We can drop the additional aliasing of ioport regions and also the special address space listener. cpu_in and cpu_out now simply call address_space_read/write. And we can concentrate portio handling in a single source file.

Signed-off-by Jan Kiszka <[email protected]>

---

...

Quoted 16 lines

ioport.c +6 -0

+
+static void portio_write(void *opaque, hwaddr addr, uint64_t data,
+                         unsigned size)
+{
+    MemoryRegionPortioList *mrpio = opaque;
+    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size,

true);

ioport.c +8 -0

+
+    if (mrp) {
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
+    } else if (size == 2) {
+        mrp = find_portio(mrpio, addr, 1, true);
+        assert(mrp);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data

Quoted 1 line

Quoted 1 line

8);

Quoted 13 lines

ioport.c void portio_list_init(PortioList *piolist, · line 425

Quoted 10 lines

ioport.c +10 -0

+    }
+}
+
+static const MemoryRegionOps portio_ops = {
+    .read = portio_read,
+    .write = portio_write,
+    .valid.unaligned = true,
+    .impl.unaligned = true,
+};
+

You need to mark these operations as DEVICE_LITTLE_ENDIAN. In portio_write above, you clearly assume that data is in LE format.

Quoted 1 line

Anything behind PIO is little endian, of course. Will add this.

This patch breaks VGA on PPC as it is in master today.

Alex

Quoted 9 lines

Quoted 4 lines

This fixes PPC PReP emulation, which would otherwise be broken with this patchset.

Thanks,
Jan

Hi!

This patch also breaks virtio on powerpc. I thought it was fixed (reverted?) in the master branch from qemu.org but it is still there. What did I miss?

Trying to load: from: disk ... virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
[many of those]

Quoted 76 lines · Alexander Graf

Quoted 55 lines · Jan Kiszka

Quoted 40 lines · Hervé Poussineau

Quoted 20 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

The current ioport dispatcher is a complex beast, mostly due to the need to deal with old portio interface users. But we can overcome it without converting all portio users by embedding the required base address of a MemoryRegionPortio access into that data structure. That removes the need to have the additional MemoryRegionIORange structure in the loop on every access.

To handle old portio memory ops, we simply install dispatching handlers for portio memory regions when registering them with the memory core. This removes the need for the old_portio field.

We can drop the additional aliasing of ioport regions and also the special address space listener. cpu_in and cpu_out now simply call address_space_read/write. And we can concentrate portio handling in a single source file.

Signed-off-by Jan Kiszka <[email protected]>

---

...

Quoted 16 lines

ioport.c +6 -0

+
+static void portio_write(void *opaque, hwaddr addr, uint64_t data,
+                         unsigned size)
+{
+    MemoryRegionPortioList *mrpio = opaque;
+    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size,

true);

ioport.c +8 -0

+
+    if (mrp) {
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
+    } else if (size == 2) {
+        mrp = find_portio(mrpio, addr, 1, true);
+        assert(mrp);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data

Quoted 1 line

Quoted 1 line

8);

Quoted 13 lines

ioport.c void portio_list_init(PortioList *piolist, · line 425

Quoted 10 lines

ioport.c +10 -0

+    }
+}
+
+static const MemoryRegionOps portio_ops = {
+    .read = portio_read,
+    .write = portio_write,
+    .valid.unaligned = true,
+    .impl.unaligned = true,
+};
+

You need to mark these operations as DEVICE_LITTLE_ENDIAN. In portio_write above, you clearly assume that data is in LE format.

Quoted 1 line

Anything behind PIO is little endian, of course. Will add this.

This patch breaks VGA on PPC as it is in master today.

Alex

Quoted 9 lines

Quoted 4 lines

This fixes PPC PReP emulation, which would otherwise be broken with this patchset.

Thanks,
Jan

Il 19/07/2013 13:09, Alexey Kardashevskiy ha scritto:

Quoted 5 lines

Hi!

This patch also breaks virtio on powerpc. I thought it was fixed (reverted?) in the master branch from qemu.org but it is still there. What did I miss?

It was not reverted, only the "DEVICE_LITTLE_ENDIAN" marking was.

Let me check if I can reproduce this, it looks like a endianness problems reading virtio-blk config space.

Paolo

Quoted 95 lines

Trying to load: from: disk ... virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
[many of those]

Quoted 76 lines · Alexander Graf

Quoted 55 lines · Jan Kiszka

Quoted 40 lines · Hervé Poussineau

Quoted 20 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

The current ioport dispatcher is a complex beast, mostly due to the need to deal with old portio interface users. But we can overcome it without converting all portio users by embedding the required base address of a MemoryRegionPortio access into that data structure. That removes the need to have the additional MemoryRegionIORange structure in the loop on every access.

To handle old portio memory ops, we simply install dispatching handlers for portio memory regions when registering them with the memory core. This removes the need for the old_portio field.

We can drop the additional aliasing of ioport regions and also the special address space listener. cpu_in and cpu_out now simply call address_space_read/write. And we can concentrate portio handling in a single source file.

Signed-off-by Jan Kiszka <[email protected]>

---

...

Quoted 16 lines

ioport.c +6 -0

+
+static void portio_write(void *opaque, hwaddr addr, uint64_t data,
+                         unsigned size)
+{
+    MemoryRegionPortioList *mrpio = opaque;
+    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size,

true);

ioport.c +8 -0

+
+    if (mrp) {
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
+    } else if (size == 2) {
+        mrp = find_portio(mrpio, addr, 1, true);
+        assert(mrp);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data

Quoted 1 line

Quoted 1 line

8);

Quoted 13 lines

ioport.c void portio_list_init(PortioList *piolist, · line 425

Quoted 10 lines

ioport.c +10 -0

+    }
+}
+
+static const MemoryRegionOps portio_ops = {
+    .read = portio_read,
+    .write = portio_write,
+    .valid.unaligned = true,
+    .impl.unaligned = true,
+};
+

You need to mark these operations as DEVICE_LITTLE_ENDIAN. In portio_write above, you clearly assume that data is in LE format.

Quoted 1 line

Anything behind PIO is little endian, of course. Will add this.

This patch breaks VGA on PPC as it is in master today.

Alex

Quoted 9 lines

Quoted 4 lines

This fixes PPC PReP emulation, which would otherwise be broken with this patchset.

Thanks,
Jan

Ok. So.

What broke is...
I could try explaining but backtraces are lot better :)

Shortly - virtio_pci_config_ops.endianness was ignored before (was bad but we had a workaround in spapr_io_ops), now it works so double swap happens and everything gets broken.

If we talk about VGA (in powerpc, it is all about powerpc), I guess memory_region_iorange_write() will go through mr->ops->old_portio branch and won't do any byte swapping (so spapr_io_ops will do the job), so we are fine here. I do not understand yet why it works on mac99 though, too late here :)

h_logical_store is a hypercall for system firmware to do cache inhibited read/write.

This is with the patch applied (git checkout b40acf9):

#0 virtqueue_init (vq=0x11014ac0) at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:90
#1 0x0000000010371f28 in virtio_queue_set_addr (vdev=0x11019dd0, n=0x0, addr=0xd0fb0000000)
at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:662
#2 0x00000000102027f0 in virtio_ioport_write (opaque=0x11019580, addr=0x8, val=0xd0fb0000)
at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:278
#3 0x0000000010202f08 in virtio_pci_config_write (opaque=0x11019580, addr=0x8, val=0xd0fb0000, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:416
#4 0x000000001037e220 in memory_region_write_accessor (opaque=0x11019c78, addr=0x8, value=0x1fffff0edc00,

    size=0x4, shift=0x0, mask=0xffffffff) at
/home/alexey/pcipassthru/qemu-impreza/memory.c:364
#5  0x000000001037e36c in access_with_adjusted_size (addr=0x8,
value=0x1fffff0edc00, size=0x4,
    access_size_min=0x1, access_size_max=0x4, access=
    @0x1069df40: 0x1037e164 <memory_region_write_accessor>, opaque=0x11019c78)
    at /home/alexey/pcipassthru/qemu-impreza/memory.c:396

#6 0x0000000010380b5c in memory_region_dispatch_write (mr=0x11019c78, addr=0x8, data=0xd0fb0000, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/memory.c:905
#7 0x0000000010383fa4 in io_mem_write (mr=0x11019c78, addr=0x8, val=0xfbd0, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/memory.c:1608
#8 0x00000000102e2fdc in address_space_rw (as=0x10ef4350 <address_space_io>, addr=0x48,

    buf=0x1fffff0edde0 "", len=0x4, is_write=0x1) at
/home/alexey/pcipassthru/qemu-impreza/exec.c:1918
#9  0x00000000102e33c8 in address_space_write (as=0x10ef4350
<address_space_io>, addr=0x48,
    buf=0x1fffff0edde0 "", len=0x4) at

/home/alexey/pcipassthru/qemu-impreza/exec.c:1969
#10 0x0000000010375754 in cpu_outl (addr=0x48, val=0xfbd0)
at /home/alexey/pcipassthru/qemu-impreza/ioport.c:309
#11 0x0000000010358240 in spapr_io_write (opaque=0x11016a00, addr=0x48, data=0xfbd0, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_pci.c:468
#12 0x000000001037e220 in memory_region_write_accessor (opaque=0x110191f8, addr=0x48, value=0x1fffff0ee060,

    size=0x4, shift=0x0, mask=0xffffffff) at
/home/alexey/pcipassthru/qemu-impreza/memory.c:364
#13 0x000000001037e36c in access_with_adjusted_size (addr=0x48,
value=0x1fffff0ee060, size=0x4,
    access_size_min=0x1, access_size_max=0x4, access=
    @0x1069df40: 0x1037e164 <memory_region_write_accessor>, opaque=0x110191f8)
    at /home/alexey/pcipassthru/qemu-impreza/memory.c:396

#14 0x0000000010380b5c in memory_region_dispatch_write (mr=0x110191f8, addr=0x48, data=0xfbd0, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/memory.c:905
#15 0x0000000010383fa4 in io_mem_write (mr=0x110191f8, addr=0x48, val=0xd0fb0000, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/memory.c:1608
#16 0x00000000102e47ac in stl_phys_internal (addr=0x10080000048, val=0xd0fb0000, endian=
DEVICE_NATIVE_ENDIAN) at /home/alexey/pcipassthru/qemu-impreza/exec.c:2420
#17 0x00000000102e48a8 in stl_phys (addr=0x10080000048, val=0xd0fb0000)
at /home/alexey/pcipassthru/qemu-impreza/exec.c:2442
#18 0x0000000010354f1c in h_logical_store (cpu=0x1fffff0f0010, spapr=0x10fe9510, opcode=0x40,

    args=0x1ffffffd0030) at

/home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_hcall.c:570

This is without this patch (i.e. git checkout b40acf9^ ):

#0 virtqueue_init (vq=0x11014ac0) at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:90
#1 0x00000000103720e4 in virtio_queue_set_addr (vdev=0x11019dd0, n=0x0, addr=0xffe2000)
at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:662
#2 0x00000000102027f0 in virtio_ioport_write (opaque=0x11019580, addr=0x8, val=0xffe2)
at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:278
#3 0x0000000010202f08 in virtio_pci_config_write (opaque=0x11019580, addr=0x8, val=0xffe2, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:416
#4 0x000000001037dca8 in memory_region_write_accessor (opaque=0x11019c78, addr=0x8, value=0x1fffff0edca8,

    size=0x4, shift=0x0, mask=0xffffffff) at
/home/alexey/pcipassthru/qemu-impreza/memory.c:364
#5  0x000000001037ddf4 in access_with_adjusted_size (addr=0x8,
value=0x1fffff0edca8, size=0x4,
    access_size_min=0x1, access_size_max=0x4, access=
    @0x1069def8: 0x1037dbec <memory_region_write_accessor>, opaque=0x11019c78)
    at /home/alexey/pcipassthru/qemu-impreza/memory.c:396
#6  0x000000001037e474 in memory_region_iorange_write
(iorange=0x1ffff0005430, offset=0x8, width=0x4,
    data=0xffe2) at /home/alexey/pcipassthru/qemu-impreza/memory.c:475

#7 0x00000000103750d4 in ioport_writel_thunk (opaque=0x1ffff0005430, addr=0x48, data=0xffe2)
at /home/alexey/pcipassthru/qemu-impreza/ioport.c:226
#8 0x0000000010374728 in ioport_write (index=0x2, address=0x48, data=0xffe2)
at /home/alexey/pcipassthru/qemu-impreza/ioport.c:83
#9 0x0000000010375688 in cpu_outl (addr=0x48, val=0xffe2)
at /home/alexey/pcipassthru/qemu-impreza/ioport.c:296
#10 0x00000000103583fc in spapr_io_write (opaque=0x11016a00, addr=0x48, data=0xffe2, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_pci.c:468
#11 0x000000001037dca8 in memory_region_write_accessor (opaque=0x110191f8, addr=0x48, value=0x1fffff0ee060,

    size=0x4, shift=0x0, mask=0xffffffff) at
/home/alexey/pcipassthru/qemu-impreza/memory.c:364
#12 0x000000001037ddf4 in access_with_adjusted_size (addr=0x48,
value=0x1fffff0ee060, size=0x4,
    access_size_min=0x1, access_size_max=0x4, access=
    @0x1069def8: 0x1037dbec <memory_region_write_accessor>, opaque=0x110191f8)
    at /home/alexey/pcipassthru/qemu-impreza/memory.c:396

#13 0x0000000010380c90 in memory_region_dispatch_write (mr=0x110191f8, addr=0x48, data=0xffe2, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/memory.c:993
#14 0x00000000103840d8 in io_mem_write (mr=0x110191f8, addr=0x48, val=0xe2ff0000, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/memory.c:1696
#15 0x00000000102e4968 in stl_phys_internal (addr=0x10080000048, val=0xe2ff0000, endian=
DEVICE_NATIVE_ENDIAN) at /home/alexey/pcipassthru/qemu-impreza/exec.c:2447
#16 0x00000000102e4a64 in stl_phys (addr=0x10080000048, val=0xe2ff0000)
at /home/alexey/pcipassthru/qemu-impreza/exec.c:2469
#17 0x00000000103550d8 in h_logical_store (cpu=0x1fffff0f0010, spapr=0x10fe9510, opcode=0x40,

    args=0x1ffffffd0030) at

/home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_hcall.c:570 #18 0x0000000010355698 in spapr_hypercall (cpu=0x1fffff0f0010, opcode=0x40, args=0x1ffffffd0030)
at /home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_hcall.c:689

Quoted 110 lines · Paolo Bonzini

Il 19/07/2013 13:09, Alexey Kardashevskiy ha scritto:

Quoted 5 lines

Hi!

This patch also breaks virtio on powerpc. I thought it was fixed (reverted?) in the master branch from qemu.org but it is still there. What did I miss?

It was not reverted, only the "DEVICE_LITTLE_ENDIAN" marking was.

Let me check if I can reproduce this, it looks like a endianness problems reading virtio-blk config space.

Paolo

Quoted 95 lines

Trying to load: from: disk ... virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
[many of those]

Quoted 76 lines · Alexander Graf

Quoted 55 lines · Jan Kiszka

Quoted 40 lines · Hervé Poussineau

Quoted 20 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

The current ioport dispatcher is a complex beast, mostly due to the need to deal with old portio interface users. But we can overcome it without converting all portio users by embedding the required base address of a MemoryRegionPortio access into that data structure. That removes the need to have the additional MemoryRegionIORange structure in the loop on every access.

To handle old portio memory ops, we simply install dispatching handlers for portio memory regions when registering them with the memory core. This removes the need for the old_portio field.

We can drop the additional aliasing of ioport regions and also the special address space listener. cpu_in and cpu_out now simply call address_space_read/write. And we can concentrate portio handling in a single source file.

Signed-off-by Jan Kiszka <[email protected]>

---

...

Quoted 16 lines

(quoted diff) +6 -0

+
+static void portio_write(void *opaque, hwaddr addr, uint64_t data,
+                         unsigned size)
+{
+    MemoryRegionPortioList *mrpio = opaque;
+    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size,

true);

(quoted diff) +8 -0

+
+    if (mrp) {
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
+    } else if (size == 2) {
+        mrp = find_portio(mrpio, addr, 1, true);
+        assert(mrp);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data

Quoted 1 line

Quoted 1 line

8);

Quoted 13 lines

Review

Quoted 10 lines

(quoted diff) +10 -0

+    }
+}
+
+static const MemoryRegionOps portio_ops = {
+    .read = portio_read,
+    .write = portio_write,
+    .valid.unaligned = true,
+    .impl.unaligned = true,
+};
+

You need to mark these operations as DEVICE_LITTLE_ENDIAN. In portio_write above, you clearly assume that data is in LE format.

Quoted 1 line

Anything behind PIO is little endian, of course. Will add this.

This patch breaks VGA on PPC as it is in master today.

Alex

Quoted 9 lines

Quoted 4 lines

This fixes PPC PReP emulation, which would otherwise be broken with this patchset.

Thanks,
Jan

Quoted 9 lines · Alexey Kardashevskiy

Ok. So.

What broke is...
I could try explaining but backtraces are lot better :)

Shortly - virtio_pci_config_ops.endianness was ignored before (was bad but we had a workaround in spapr_io_ops), now it works so double swap happens and everything gets broken.

Quoted 5 lines

If we talk about VGA (in powerpc, it is all about powerpc), I guess memory_region_iorange_write() will go through mr->ops->old_portio branch and won't do any byte swapping (so spapr_io_ops will do the job), so we are fine here. I do not understand yet why it works on mac99 though, too late here :)

I understood. VGA does not work for mac99 either with this command line:
./qemu-system-ppc64 -m "1024" -M "mac99" -vga "std"
So it works for pseries only because of parity bug in spapr_io_ops.

So the right fix is to get rid of spapr_io_ops and every other hack like that and to add byte swapping to every "if (mr->ops->old_portio)" branch (should fix VGA and all other old_portio users). Current byte swapping in memory regions seems to be right.

I would try fixing it but since all my patches were terrible shit so far, I won't risk :)

Quoted 254 lines

h_logical_store is a hypercall for system firmware to do cache inhibited read/write.

This is with the patch applied (git checkout b40acf9):

#0 virtqueue_init (vq=0x11014ac0) at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:90
#1 0x0000000010371f28 in virtio_queue_set_addr (vdev=0x11019dd0, n=0x0, addr=0xd0fb0000000)
at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:662
#2 0x00000000102027f0 in virtio_ioport_write (opaque=0x11019580, addr=0x8, val=0xd0fb0000)
at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:278
#3 0x0000000010202f08 in virtio_pci_config_write (opaque=0x11019580, addr=0x8, val=0xd0fb0000, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:416
#4 0x000000001037e220 in memory_region_write_accessor (opaque=0x11019c78, addr=0x8, value=0x1fffff0edc00,

    size=0x4, shift=0x0, mask=0xffffffff) at
/home/alexey/pcipassthru/qemu-impreza/memory.c:364
#5  0x000000001037e36c in access_with_adjusted_size (addr=0x8,
value=0x1fffff0edc00, size=0x4,
    access_size_min=0x1, access_size_max=0x4, access=
    @0x1069df40: 0x1037e164 <memory_region_write_accessor>, opaque=0x11019c78)
    at /home/alexey/pcipassthru/qemu-impreza/memory.c:396

#6 0x0000000010380b5c in memory_region_dispatch_write (mr=0x11019c78, addr=0x8, data=0xd0fb0000, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/memory.c:905
#7 0x0000000010383fa4 in io_mem_write (mr=0x11019c78, addr=0x8, val=0xfbd0, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/memory.c:1608
#8 0x00000000102e2fdc in address_space_rw (as=0x10ef4350 <address_space_io>, addr=0x48,

    buf=0x1fffff0edde0 "", len=0x4, is_write=0x1) at
/home/alexey/pcipassthru/qemu-impreza/exec.c:1918
#9  0x00000000102e33c8 in address_space_write (as=0x10ef4350
<address_space_io>, addr=0x48,
    buf=0x1fffff0edde0 "", len=0x4) at

/home/alexey/pcipassthru/qemu-impreza/exec.c:1969
#10 0x0000000010375754 in cpu_outl (addr=0x48, val=0xfbd0)
at /home/alexey/pcipassthru/qemu-impreza/ioport.c:309
#11 0x0000000010358240 in spapr_io_write (opaque=0x11016a00, addr=0x48, data=0xfbd0, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_pci.c:468
#12 0x000000001037e220 in memory_region_write_accessor (opaque=0x110191f8, addr=0x48, value=0x1fffff0ee060,

    size=0x4, shift=0x0, mask=0xffffffff) at
/home/alexey/pcipassthru/qemu-impreza/memory.c:364
#13 0x000000001037e36c in access_with_adjusted_size (addr=0x48,
value=0x1fffff0ee060, size=0x4,
    access_size_min=0x1, access_size_max=0x4, access=
    @0x1069df40: 0x1037e164 <memory_region_write_accessor>, opaque=0x110191f8)
    at /home/alexey/pcipassthru/qemu-impreza/memory.c:396

#14 0x0000000010380b5c in memory_region_dispatch_write (mr=0x110191f8, addr=0x48, data=0xfbd0, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/memory.c:905
#15 0x0000000010383fa4 in io_mem_write (mr=0x110191f8, addr=0x48, val=0xd0fb0000, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/memory.c:1608
#16 0x00000000102e47ac in stl_phys_internal (addr=0x10080000048, val=0xd0fb0000, endian=
DEVICE_NATIVE_ENDIAN) at /home/alexey/pcipassthru/qemu-impreza/exec.c:2420
#17 0x00000000102e48a8 in stl_phys (addr=0x10080000048, val=0xd0fb0000)
at /home/alexey/pcipassthru/qemu-impreza/exec.c:2442
#18 0x0000000010354f1c in h_logical_store (cpu=0x1fffff0f0010, spapr=0x10fe9510, opcode=0x40,

    args=0x1ffffffd0030) at

/home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_hcall.c:570

This is without this patch (i.e. git checkout b40acf9^ ):

#0 virtqueue_init (vq=0x11014ac0) at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:90
#1 0x00000000103720e4 in virtio_queue_set_addr (vdev=0x11019dd0, n=0x0, addr=0xffe2000)
at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:662
#2 0x00000000102027f0 in virtio_ioport_write (opaque=0x11019580, addr=0x8, val=0xffe2)
at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:278
#3 0x0000000010202f08 in virtio_pci_config_write (opaque=0x11019580, addr=0x8, val=0xffe2, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:416
#4 0x000000001037dca8 in memory_region_write_accessor (opaque=0x11019c78, addr=0x8, value=0x1fffff0edca8,

    size=0x4, shift=0x0, mask=0xffffffff) at
/home/alexey/pcipassthru/qemu-impreza/memory.c:364
#5  0x000000001037ddf4 in access_with_adjusted_size (addr=0x8,
value=0x1fffff0edca8, size=0x4,
    access_size_min=0x1, access_size_max=0x4, access=
    @0x1069def8: 0x1037dbec <memory_region_write_accessor>, opaque=0x11019c78)
    at /home/alexey/pcipassthru/qemu-impreza/memory.c:396
#6  0x000000001037e474 in memory_region_iorange_write
(iorange=0x1ffff0005430, offset=0x8, width=0x4,
    data=0xffe2) at /home/alexey/pcipassthru/qemu-impreza/memory.c:475

#7 0x00000000103750d4 in ioport_writel_thunk (opaque=0x1ffff0005430, addr=0x48, data=0xffe2)
at /home/alexey/pcipassthru/qemu-impreza/ioport.c:226
#8 0x0000000010374728 in ioport_write (index=0x2, address=0x48, data=0xffe2)
at /home/alexey/pcipassthru/qemu-impreza/ioport.c:83
#9 0x0000000010375688 in cpu_outl (addr=0x48, val=0xffe2)
at /home/alexey/pcipassthru/qemu-impreza/ioport.c:296
#10 0x00000000103583fc in spapr_io_write (opaque=0x11016a00, addr=0x48, data=0xffe2, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_pci.c:468
#11 0x000000001037dca8 in memory_region_write_accessor (opaque=0x110191f8, addr=0x48, value=0x1fffff0ee060,

    size=0x4, shift=0x0, mask=0xffffffff) at
/home/alexey/pcipassthru/qemu-impreza/memory.c:364
#12 0x000000001037ddf4 in access_with_adjusted_size (addr=0x48,
value=0x1fffff0ee060, size=0x4,
    access_size_min=0x1, access_size_max=0x4, access=
    @0x1069def8: 0x1037dbec <memory_region_write_accessor>, opaque=0x110191f8)
    at /home/alexey/pcipassthru/qemu-impreza/memory.c:396

#13 0x0000000010380c90 in memory_region_dispatch_write (mr=0x110191f8, addr=0x48, data=0xffe2, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/memory.c:993
#14 0x00000000103840d8 in io_mem_write (mr=0x110191f8, addr=0x48, val=0xe2ff0000, size=0x4)
at /home/alexey/pcipassthru/qemu-impreza/memory.c:1696
#15 0x00000000102e4968 in stl_phys_internal (addr=0x10080000048, val=0xe2ff0000, endian=
DEVICE_NATIVE_ENDIAN) at /home/alexey/pcipassthru/qemu-impreza/exec.c:2447
#16 0x00000000102e4a64 in stl_phys (addr=0x10080000048, val=0xe2ff0000)
at /home/alexey/pcipassthru/qemu-impreza/exec.c:2469
#17 0x00000000103550d8 in h_logical_store (cpu=0x1fffff0f0010, spapr=0x10fe9510, opcode=0x40,

    args=0x1ffffffd0030) at

/home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_hcall.c:570 #18 0x0000000010355698 in spapr_hypercall (cpu=0x1fffff0f0010, opcode=0x40, args=0x1ffffffd0030)
at /home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_hcall.c:689

Quoted 110 lines · Paolo Bonzini

Il 19/07/2013 13:09, Alexey Kardashevskiy ha scritto:

Quoted 5 lines

Hi!

This patch also breaks virtio on powerpc. I thought it was fixed (reverted?) in the master branch from qemu.org but it is still there. What did I miss?

It was not reverted, only the "DEVICE_LITTLE_ENDIAN" marking was.

Let me check if I can reproduce this, it looks like a endianness problems reading virtio-blk config space.

Paolo

Quoted 95 lines

Trying to load: from: disk ... virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
[many of those]

Quoted 76 lines · Alexander Graf

Quoted 55 lines · Jan Kiszka

Quoted 40 lines · Hervé Poussineau

Quoted 20 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

The current ioport dispatcher is a complex beast, mostly due to the need to deal with old portio interface users. But we can overcome it without converting all portio users by embedding the required base address of a MemoryRegionPortio access into that data structure. That removes the need to have the additional MemoryRegionIORange structure in the loop on every access.

To handle old portio memory ops, we simply install dispatching handlers for portio memory regions when registering them with the memory core. This removes the need for the old_portio field.

We can drop the additional aliasing of ioport regions and also the special address space listener. cpu_in and cpu_out now simply call address_space_read/write. And we can concentrate portio handling in a single source file.

Signed-off-by Jan Kiszka <[email protected]>

---

...

Quoted 16 lines

(quoted diff) +6 -0

+
+static void portio_write(void *opaque, hwaddr addr, uint64_t data,
+                         unsigned size)
+{
+    MemoryRegionPortioList *mrpio = opaque;
+    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size,

true);

(quoted diff) +8 -0

+
+    if (mrp) {
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
+    } else if (size == 2) {
+        mrp = find_portio(mrpio, addr, 1, true);
+        assert(mrp);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data

Quoted 1 line

Quoted 1 line

8);

Quoted 13 lines

Review

Quoted 10 lines

(quoted diff) +10 -0

+    }
+}
+
+static const MemoryRegionOps portio_ops = {
+    .read = portio_read,
+    .write = portio_write,
+    .valid.unaligned = true,
+    .impl.unaligned = true,
+};
+

You need to mark these operations as DEVICE_LITTLE_ENDIAN. In portio_write above, you clearly assume that data is in LE format.

Quoted 1 line

Anything behind PIO is little endian, of course. Will add this.

This patch breaks VGA on PPC as it is in master today.

Alex

Quoted 9 lines

Quoted 4 lines

This fixes PPC PReP emulation, which would otherwise be broken with this patchset.

Thanks,
Jan

Quoted 16 lines · Alexey Kardashevskiy

Quoted 14 lines

Ok. So.

What broke is...
I could try explaining but backtraces are lot better :)

Shortly - virtio_pci_config_ops.endianness was ignored before (was bad but we had a workaround in spapr_io_ops), now it works so double swap happens and everything gets broken.

If we talk about VGA (in powerpc, it is all about powerpc), I guess memory_region_iorange_write() will go through mr->ops->old_portio branch and won't do any byte swapping (so spapr_io_ops will do the job), so we are fine here. I do not understand yet why it works on mac99 though, too late here :)

Quoted 3 lines

I understood. VGA does not work for mac99 either with this command line:
./qemu-system-ppc64 -m "1024" -M "mac99" -vga "std"
So it works for pseries only because of parity bug in spapr_io_ops.

oops. I am wrong and VGA works on mac99 in upstream because isa_mmio_ops does the swapping in this case and portio_ops does not swap (in upstream).

Il 20/07/2013 03:11, Alexey Kardashevskiy ha scritto:

Quoted 17 lines · Alexey Kardashevskiy

Quoted 16 lines · Alexey Kardashevskiy

Quoted 14 lines

Ok. So.

What broke is...
I could try explaining but backtraces are lot better :)

Shortly - virtio_pci_config_ops.endianness was ignored before (was bad but we had a workaround in spapr_io_ops), now it works so double swap happens and everything gets broken.

If we talk about VGA (in powerpc, it is all about powerpc), I guess memory_region_iorange_write() will go through mr->ops->old_portio branch and won't do any byte swapping (so spapr_io_ops will do the job), so we are fine here. I do not understand yet why it works on mac99 though, too late here :)

Quoted 3 lines

Quoted 3 lines

I understood. VGA does not work for mac99 either with this command line:
./qemu-system-ppc64 -m "1024" -M "mac99" -vga "std"
So it works for pseries only because of parity bug in spapr_io_ops.

Yes, this is what Herve was fixing for PREP too.

Quoted 2 lines

oops. I am wrong and VGA works on mac99 in upstream because isa_mmio_ops does the swapping in this case and portio_ops does not swap (in upstream).

Uff... I guess we have to look at all cases for big-endian machines, and make sure there is an odd number of exchanges. Aurelien/Alex/Herve, where can I find images for mips malta, mips jazz, ppc4xx and prep? Edgar, what about sh4eb (IIRC one of the two machine types supported PCI)?

Quoted 1 line

oops. I am wrong and VGA works on mac99 in upstream because

Quoted 1 line

does the swapping in this case and portio_ops does not swap (in upstream).

Uff... I guess we have to look at all cases for big-endian machines, and make sure there is an odd number of exchanges. Aurelien/Alex/Herve, where can I find images for mips malta, mips jazz, ppc4xx and prep? Edgar, what about sh4eb (IIRC one of the two machine types supported PCI)?

Quoted 1 line · Paolo Bonzini

Quoted 1 line

oops. I am wrong and VGA works on mac99 in upstream because

isa_mmio_ops

Quoted 8 lines

Quoted 2 lines

does the swapping in this case and portio_ops does not swap (in upstream).

Uff... I guess we have to look at all cases for big-endian machines, and make sure there is an odd number of exchanges. Aurelien/Alex/Herve, where can I find images for mips malta, mips jazz, ppc4xx and prep? Edgar, what about sh4eb (IIRC one of the two machine types supported PCI)?

For PReP, you can find a kernel at
http://www.juneau-lug.org/zImage.initrd.sandalfoot
Run it with qemu-system-ppc -M prep -kernel zImage.initrd.sandalfoot

Quoted 20 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

The current ioport dispatcher is a complex beast, mostly due to the need to deal with old portio interface users. But we can overcome it without converting all portio users by embedding the required base address of a MemoryRegionPortio access into that data structure. That removes the need to have the additional MemoryRegionIORange structure in the loop on every access.

To handle old portio memory ops, we simply install dispatching handlers for portio memory regions when registering them with the memory core. This removes the need for the old_portio field.

We can drop the additional aliasing of ioport regions and also the special address space listener. cpu_in and cpu_out now simply call address_space_read/write. And we can concentrate portio handling in a single source file.

Signed-off-by Jan Kiszka <[email protected]>

---

...

Quoted 14 lines

ioport.c +14 -0

+
+static void portio_write(void *opaque, hwaddr addr, uint64_t data,
+                         unsigned size)
+{
+    MemoryRegionPortioList *mrpio = opaque;
+    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size, true);
+
+    if (mrp) {
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
+    } else if (size == 2) {
+        mrp = find_portio(mrpio, addr, 1, true);
+        assert(mrp);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data >> 8);

ioport.c void portio_list_init(PortioList *piolist, · line 425

Quoted 10 lines

ioport.c +10 -0

+    }
+}
+
+static const MemoryRegionOps portio_ops = {
+    .read = portio_read,
+    .write = portio_write,
+    .valid.unaligned = true,
+    .impl.unaligned = true,
+};
+

You need to mark these operations as DEVICE_LITTLE_ENDIAN. In portio_write above, you clearly assume that data is in LE format.

This fixes PPC PReP emulation, which would otherwise be broken with this patchset.

Hervé

Changes in v3:
- decouple vmport from portio types
- removed portio traces from memory.h, consolidating it in ioport.h

CC Jan Kiszka <[email protected]>

CC malc <[email protected]>

Jan Kiszka (14):
adlib: replace register_ioport*
applesmc: replace register_ioport*
wdt_ib700: replace register_ioport*
i82374: replace register_ioport*
prep: replace register_ioport*
vt82c686: replace register_ioport*
Privatize register_ioport_read/write
isa: implement isa_is_ioport_assigned via memory_region_find
vmware-vga: Accept unaligned I/O accesses
xen: Mark fixed platform I/O as unaligned
ioport: Switch dispatching to memory core layer
ioport: Remove unused old dispatching services
vmport: Disentangle read handler type from portio
ioport: Move portio types to ioport.h