memory: Provide separate handling of unassigned io ports accesses

From: Jan Kiszka <[email protected]>

Accesses to unassigned io ports shall return -1 on read and be ignored on write. Ensure these properties via dedicated ops, decoupling us from the memory core's handling of unassigned accesses.

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

---
 exec.c                |    3 ++-
 include/exec/ioport.h |    2 ++
 ioport.c              |   16 ++++++++++++++++
 3 files changed, 20 insertions(+), 1 deletions(-)

exec.c +0 -0

index 3ca9381..9ed598f 100644

exec.c +0 -0

     address_space_init(&address_space_io, system_io, "I/O");
 
     memory_listener_register(&core_memory_listener, &address_space_memory);

include/exec/ioport.h +2 -0

index bdd4e96..84f7f85 100644
@@ -45,6 +45,8 @@ typedef struct MemoryRegionPortio {
 
 #define PORTIO_END_OF_LIST() { }
 
+extern const MemoryRegionOps unassigned_io_ops;
+
 void cpu_outb(pio_addr_t addr, uint8_t val);
 void cpu_outw(pio_addr_t addr, uint16_t val);
 void cpu_outl(pio_addr_t addr, uint32_t val);

ioport.c +16 -0

index 79b7f1a..9765588 100644
@@ -44,6 +44,22 @@ typedef struct MemoryRegionPortioList {
     MemoryRegionPortio ports[];
 } MemoryRegionPortioList;
 
+static uint64_t unassigned_io_read(void *opaque, hwaddr addr, unsigned size)
+{
+    return -1UL;
+}
+
+static void unassigned_io_write(void *opaque, hwaddr addr, uint64_t val,
+                                unsigned size)
+{
+}
+
+const MemoryRegionOps unassigned_io_ops = {
+    .read = unassigned_io_read,
+    .write = unassigned_io_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
 void cpu_outb(pio_addr_t addr, uint8_t val)
 {
     LOG_IOPORT("outb: %04"FMT_pioaddr" %02"PRIx8"\n", addr, val);

Am 03.08.2013 10:31, schrieb Jan Kiszka:

Quoted 20 lines

From: Jan Kiszka <[email protected]>

Accesses to unassigned io ports shall return -1 on read and be ignored on write. Ensure these properties via dedicated ops, decoupling us from the memory core's handling of unassigned accesses.

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

---
 exec.c                |    3 ++-
 include/exec/ioport.h |    2 ++
 ioport.c              |   16 ++++++++++++++++
 3 files changed, 20 insertions(+), 1 deletions(-)

exec.c +0 -0

index 3ca9381..9ed598f 100644
@@ -1820,7 +1820,8 @@ static void memory_map_init(void)
     address_space_init(&address_space_memory, system_memory, "memory");
 

exec.c static void memory_map_init(void) · line 1824

Quoted 4 lines

exec.c +2 -1

     system_io = g_malloc(sizeof(*system_io));
-    memory_region_init(system_io, NULL, "io", 65536);
+    memory_region_init_io(system_io, NULL, &unassigned_io_ops, NULL, "io",
+                          65536);

It was reported that there may be some machines/PHBs that have overlapping PIO/MMIO. Unless we use priorities, this ..._io MemoryRegion will shadow or conflict with any ..._io MemoryRegion added to the memory address space, wouldn't it?

Am 03.08.2013 10:31, schrieb Jan Kiszka:

Quoted 20 lines

From: Jan Kiszka <[email protected]>

Accesses to unassigned io ports shall return -1 on read and be ignored on write. Ensure these properties via dedicated ops, decoupling us from the memory core's handling of unassigned accesses.

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

---
 exec.c                |    3 ++-
 include/exec/ioport.h |    2 ++
 ioport.c              |   16 ++++++++++++++++
 3 files changed, 20 insertions(+), 1 deletions(-)

exec.c +0 -0

index 3ca9381..9ed598f 100644
@@ -1820,7 +1820,8 @@ static void memory_map_init(void)
     address_space_init(&address_space_memory, system_memory, "memory");
 

exec.c static void memory_map_init(void) · line 1824

Quoted 4 lines

exec.c +2 -1

     system_io = g_malloc(sizeof(*system_io));
-    memory_region_init(system_io, NULL, "io", 65536);
+    memory_region_init_io(system_io, NULL, &unassigned_io_ops, NULL, "io",
+                          65536);

It was reported that there may be some machines/PHBs that have overlapping PIO/MMIO. Unless we use priorities, this ..._io MemoryRegion will shadow or conflict with any ..._io MemoryRegion added to the memory address space, wouldn't it?

Quoted 21 lines

Am 03.08.2013 10:31, schrieb Jan Kiszka:

Quoted 20 lines

From: Jan Kiszka <[email protected]>

Accesses to unassigned io ports shall return -1 on read and be ignored on write. Ensure these properties via dedicated ops, decoupling us from the memory core's handling of unassigned accesses.

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

---
 exec.c                |    3 ++-
 include/exec/ioport.h |    2 ++
 ioport.c              |   16 ++++++++++++++++
 3 files changed, 20 insertions(+), 1 deletions(-)

exec.c +0 -0

index 3ca9381..9ed598f 100644
@@ -1820,7 +1820,8 @@ static void memory_map_init(void)
     address_space_init(&address_space_memory, system_memory, "memory");
 

Quoted 9 lines

exec.c static void memory_map_init(void) · line 1824

Quoted 4 lines

exec.c +2 -1

     system_io = g_malloc(sizeof(*system_io));
-    memory_region_init(system_io, NULL, "io", 65536);
+    memory_region_init_io(system_io, NULL, &unassigned_io_ops, NULL, "io",
+                          65536);

It was reported that there may be some machines/PHBs that have overlapping PIO/MMIO. Unless we use priorities, this ..._io MemoryRegion will shadow or conflict with any ..._io MemoryRegion added to the memory address space, wouldn't it?

Priorities only apply between different subregions within a container. This is adding IO operations to the container itself, so there's no priority issue here: the container's operations always come last, behind any subregions it has.

(Do we have any existing examples of container regions with their own default IO operations? The memory.c code clearly expects them to be OK, though - eg render_memory_region() specifically does "render subregions; then render the region itself into any gaps".)

Or do you mean that if we had:

[ system memory region, with its own default read/write ops ]
[ io region mapped into it ]
[ io ] [ io ][io]

that now if you access the bit of system memory corresponding to the I/O region at some address with no specific IO port, you'll get the IO region's defaults, rather than the system memory region's defaults? I think that's true and possibly a change in behaviour.

Do we have any boards that do that?

Quoted 21 lines · Andreas Färber

Am 03.08.2013 10:31, schrieb Jan Kiszka:

Quoted 20 lines

From: Jan Kiszka <[email protected]>

Accesses to unassigned io ports shall return -1 on read and be ignored on write. Ensure these properties via dedicated ops, decoupling us from the memory core's handling of unassigned accesses.

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

---
 exec.c                |    3 ++-
 include/exec/ioport.h |    2 ++
 ioport.c              |   16 ++++++++++++++++
 3 files changed, 20 insertions(+), 1 deletions(-)

exec.c +0 -0

index 3ca9381..9ed598f 100644
@@ -1820,7 +1820,8 @@ static void memory_map_init(void)
     address_space_init(&address_space_memory, system_memory, "memory");
 

Quoted 9 lines

exec.c static void memory_map_init(void) · line 1824

Quoted 4 lines

exec.c +2 -1

     system_io = g_malloc(sizeof(*system_io));
-    memory_region_init(system_io, NULL, "io", 65536);
+    memory_region_init_io(system_io, NULL, &unassigned_io_ops, NULL, "io",
+                          65536);

It was reported that there may be some machines/PHBs that have overlapping PIO/MMIO. Unless we use priorities, this ..._io MemoryRegion will shadow or conflict with any ..._io MemoryRegion added to the memory address space, wouldn't it?

Priorities only apply between different subregions within a container. This is adding IO operations to the container itself, so there's no priority issue here: the container's operations always come last, behind any subregions it has.

(Do we have any existing examples of container regions with their own default IO operations? The memory.c code clearly expects them to be OK, though - eg render_memory_region() specifically does "render subregions; then render the region itself into any gaps".)

Or do you mean that if we had:

[ system memory region, with its own default read/write ops ]

[ io region mapped into it ]
[ io ] [ io ][io]

that now if you access the bit of system memory corresponding to the I/O region at some address with no specific IO port, you'll get the IO region's defaults, rather than the system memory region's defaults? I think that's true and possibly a change in behaviour. Do we have any boards that do that?

-- PMM

Quoted 3 lines · Peter Maydell

Or do you mean that if we had:

[ system memory region, with its own default read/write ops ]

I cannot imagine how this could work. The system memory region has no clue about what the regions below it can handle and what not. So it has to pass through the io window.

Quoted 8 lines · Jan Kiszka

Quoted 3 lines · Peter Maydell

Or do you mean that if we had:

[ system memory region, with its own default read/write ops ]

I cannot imagine how this could work. The system memory region has no clue about what the regions below it can handle and what not. So it has to pass through the io window.

The system memory region's just a container, you can add a background region to it at lowest-possible-priority, which then takes accesses which are either (a) not in any subregion or (b) in a subregion but that container doesn't specify its own io ops and nothing in that container handles the access. (Or you could create the system memory region with its own IO ops, which would have the same effect.)

Quoted 10 lines · Peter Maydell

Quoted 8 lines · Jan Kiszka

Quoted 3 lines · Peter Maydell

Or do you mean that if we had:

[ system memory region, with its own default read/write ops ]

I cannot imagine how this could work. The system memory region has no clue about what the regions below it can handle and what not. So it has to pass through the io window.

Quoted 7 lines

The system memory region's just a container, you can add a background region to it at lowest-possible-priority, which then takes accesses which are either (a) not in any subregion or (b) in a subregion but that container doesn't specify its own io ops and nothing in that container handles the access. (Or you could create the system memory region with its own IO ops, which would have the same effect.)

First, we do not render MMIO and IO within the same address space so far.

But even if we would, the IO container now catches all accesses, so the system memory region will never have its default handler run for that window.

Quoted 19 lines · Jan Kiszka

Quoted 17 lines · Peter Maydell

Quoted 8 lines · Jan Kiszka

Quoted 3 lines · Peter Maydell

Or do you mean that if we had:

[ system memory region, with its own default read/write ops ]

I cannot imagine how this could work. The system memory region has no clue about what the regions below it can handle and what not. So it has to pass through the io window.

The system memory region's just a container, you can add a background region to it at lowest-possible-priority, which then takes accesses which are either (a) not in any subregion or (b) in a subregion but that container doesn't specify its own io ops and nothing in that container handles the access. (Or you could create the system memory region with its own IO ops, which would have the same effect.)

Quoted 2 lines

First, we do not render MMIO and IO within the same address space so far.

Is this a statement made because you've checked all the boards and know that nobody's mapping the system-io memory region into the system-memory region? (It is technically trivial, you just need to call memory_region_add_subregion() directly or indirectly...)

Quoted 10 lines · Peter Maydell

Quoted 8 lines · Jan Kiszka

Quoted 3 lines · Peter Maydell

Or do you mean that if we had:

[ system memory region, with its own default read/write ops ]

I cannot imagine how this could work. The system memory region has no clue about what the regions below it can handle and what not. So it has to pass through the io window.

Quoted 7 lines

The system memory region's just a container, you can add a background region to it at lowest-possible-priority, which then takes accesses which are either (a) not in any subregion or (b) in a subregion but that container doesn't specify its own io ops and nothing in that container handles the access. (Or you could create the system memory region with its own IO ops, which would have the same effect.)

First, we do not render MMIO and IO within the same address space so far.

But even if we would, the IO container now catches all accesses, so the system memory region will never have its default handler run for that window.

Quoted 3 lines

But even if we would, the IO container now catches all accesses, so the system memory region will never have its default handler run for that window.

Yes, that is the point -- before the IO container caught all accesses, the default handler that would run would be the system memory region, but now that the IO container has default read/write ops they will take precedence. So this would be a behaviour change if there are any boards set up like this. (I'm not sure there are any, though.)

Quoted 23 lines · Jan Kiszka

Quoted 22 lines · Peter Maydell

Quoted 20 lines

Quoted 10 lines · Jan Kiszka

Quoted 4 lines · Peter Maydell

Or do you mean that if we had:

[ system memory region, with its own default read/write ops ]

I cannot imagine how this could work. The system memory region has no clue about what the regions below it can handle and what not. So it has to pass through the io window.

The system memory region's just a container, you can add a background region to it at lowest-possible-priority, which then takes accesses which are either (a) not in any subregion or (b) in a subregion but that container doesn't specify its own io ops and nothing in that container handles the access. (Or you could create the system memory region with its own IO ops, which would have the same effect.)

Quoted 8 lines

Quoted 2 lines

First, we do not render MMIO and IO within the same address space so far.

Is this a statement made because you've checked all the boards and know that nobody's mapping the system-io memory region into the system-memory region? (It is technically trivial, you just need to call memory_region_add_subregion() directly or indirectly...)

I know this because I just recently wrote the patch that enables this trivial step, i.e. converted PIO dispatching to the memory subsystem.

Am 05.08.2013 13:03, schrieb Jan Kiszka:

Quoted 33 lines · Peter Maydell

Quoted 23 lines · Jan Kiszka

Quoted 22 lines · Peter Maydell

Quoted 20 lines

Quoted 10 lines · Jan Kiszka

Quoted 4 lines · Peter Maydell

Or do you mean that if we had:

[ system memory region, with its own default read/write ops ]

I cannot imagine how this could work. The system memory region has no clue about what the regions below it can handle and what not. So it has to pass through the io window.

The system memory region's just a container, you can add a background region to it at lowest-possible-priority, which then takes accesses which are either (a) not in any subregion or (b) in a subregion but that container doesn't specify its own io ops and nothing in that container handles the access. (Or you could create the system memory region with its own IO ops, which would have the same effect.)

Quoted 8 lines

Quoted 2 lines

First, we do not render MMIO and IO within the same address space so far.

Is this a statement made because you've checked all the boards and know that nobody's mapping the system-io memory region into the system-memory region? (It is technically trivial, you just need to call memory_region_add_subregion() directly or indirectly...)

Quoted 3 lines

I know this because I just recently wrote the patch that enables this trivial step, i.e. converted PIO dispatching to the memory subsystem.

Several patches have been applied since, e.g.

sPAPR PHB:
http://git.qemu.org/?p=qemu.git;a=commit;h=66aab867cedd2a2d81b4d64eff7c3e0f6f272bbf
-> aliases system_io()

PReP i82378 PCI-ISA bridge:
http://git.qemu.org/?p=qemu.git;a=commit;h=5c9736789b79ea49cd236ac326f0a414f63b1015
-> uses pci_address_space_io()

Alpha Typhoon:
http://git.qemu.org/?p=qemu.git;a=commit;h=056e6bae1c91f47165d962564f82f5176bae47f0
http://git.qemu.org/?p=qemu.git;a=commit;h=3661049fec64ffd7ab008e57e396881c6a4b53a4

[For those joining late, this discussion is about whether making PIO MemoryRegion ..._io rather than just container might hurt some use case. If you have a concrete test case that would be appreciated; a we-don't-care-about-such-a-fringe-case would help as well.]

(Do we have any existing examples of container regions with their own default IO operations? The memory.c code clearly expects them to be OK, though - eg render_memory_region() specifically does "render subregions; then render the region itself into any gaps".)

Quoted 6 lines · Jan Kiszka

ioport.c +0 -0

@@ -44,6 +44,22 @@ typedef struct MemoryRegionPortioList {
     MemoryRegionPortio ports[];
 } MemoryRegionPortioList;
 

ioport.c typedef struct MemoryRegionPortioList { · line 49

Quoted 3 lines

ioport.c +3 -0

+static uint64_t unassigned_io_read(void *opaque, hwaddr addr, unsigned size)
+{
+    return -1UL;

This should probably be "-1ULL", otherwise we'll return different values on 32 bit and 64 bit hosts. (Actually managing a 64 bit read of the i/o space is pretty unlikely, though possibly alpha memory-mapped via the PCI space might let you do it.)

PS: something about the way these patches were submitted has confused Anthony's patches tool -- it reports them as two separate patches rather than a single series. (No cover letter, maybe?)

Quoted 7 lines · Jan Kiszka

Quoted 6 lines

ioport.c +0 -0

@@ -44,6 +44,22 @@ typedef struct MemoryRegionPortioList {
     MemoryRegionPortio ports[];
 } MemoryRegionPortioList;
 

Quoted 9 lines

ioport.c typedef struct MemoryRegionPortioList { · line 49

Quoted 3 lines

ioport.c +3 -0

+static uint64_t unassigned_io_read(void *opaque, hwaddr addr, unsigned size)
+{
+    return -1UL;

This should probably be "-1ULL", otherwise we'll return different values on 32 bit and 64 bit hosts. (Actually managing a 64 bit read of the i/o space is pretty unlikely, though possibly alpha memory-mapped via the PCI space might let you do it.)

No problem with changing this - but wouldn't 64-bit i/o accesses be a bug? It's not allowed according to PCI, no device can handle it (officially), so no arch should forward such requests from mmio, rather break them up first.

Il 08/08/2013 17:43, Jan Kiszka ha scritto:

Quoted 18 lines · Peter Maydell

Quoted 16 lines · Jan Kiszka

ioport.c typedef struct MemoryRegionPortioList { · line 49

Quoted 9 lines

ioport.c +3 -0

@@ -44,6 +44,22 @@ typedef struct MemoryRegionPortioList {
     MemoryRegionPortio ports[];
 } MemoryRegionPortioList;
 
+static uint64_t unassigned_io_read(void *opaque, hwaddr addr, unsigned size)
+{
+    return -1UL;

This should probably be "-1ULL", otherwise we'll return different values on 32 bit and 64 bit hosts. (Actually managing a 64 bit read of the i/o space is pretty unlikely, though possibly alpha memory-mapped via the PCI space might let you do it.)

Quoted 4 lines

No problem with changing this - but wouldn't 64-bit i/o accesses be a bug? It's not allowed according to PCI, no device can handle it (officially), so no arch should forward such requests from mmio, rather break them up first.

Yes, the impl.max_access_size should never be 8. Though 1ULL would be clearer perhaps.

Am 09.08.2013 09:41, schrieb Paolo Bonzini:

Quoted 24 lines

Il 08/08/2013 17:43, Jan Kiszka ha scritto:

Quoted 22 lines · Peter Maydell

Quoted 7 lines · Jan Kiszka

Quoted 6 lines

ioport.c +0 -0

@@ -44,6 +44,22 @@ typedef struct MemoryRegionPortioList {
     MemoryRegionPortio ports[];
 } MemoryRegionPortioList;
 

Quoted 9 lines

ioport.c typedef struct MemoryRegionPortioList { · line 49

Quoted 3 lines

ioport.c +3 -0

+static uint64_t unassigned_io_read(void *opaque, hwaddr addr, unsigned size)
+{
+    return -1UL;

This should probably be "-1ULL", otherwise we'll return different values on 32 bit and 64 bit hosts. (Actually managing a 64 bit read of the i/o space is pretty unlikely, though possibly alpha memory-mapped via the PCI space might let you do it.)

No problem with changing this - but wouldn't 64-bit i/o accesses be a bug? It's not allowed according to PCI, no device can handle it (officially), so no arch should forward such requests from mmio, rather break them up first.

Quoted 2 lines

Yes, the impl.max_access_size should never be 8. Though 1ULL would be clearer perhaps.

Let's CC rth.