isa: Avoid using obsolete memory_region_set_offset for old portio

From: Jan Kiszka<[email protected]>

We can express the offset of old portio completely via MemoryRegionPortio::offset by splitting up regions of different offsets and adjusting those offsets appropriately.

memory.c +1 -1

@@ -396,12 +395,12 @@ static void memory_region_iorange_read(IORange *iorange,
 
          *data = ((uint64_t)1<<  (width * 8)) - 1;
          if (mrp) {
-            *data = mrp->read(mr->opaque, offset + mr->offset);
+            *data = mrp->read(mr->opaque, offset + mrp->offset);

Quoted 5 lines · Jan Kiszka

From: Jan Kiszka<[email protected]>

We can express the offset of old portio completely via MemoryRegionPortio::offset by splitting up regions of different offsets and adjusting those offsets appropriately.

Please split into two patches - core and isa.

hw/isa-bus.c static void isa_register_portio_1(ISADevice *dev, · line 113

Quoted 5 lines

hw/isa-bus.c +4 -0

+    /* Copy the sub-list and null-terminate it.  */
+    pio = g_new(MemoryRegionPortio, count + 1);
+    memcpy(pio, pio_init, sizeof(MemoryRegionPortio) * count);
+    memset(pio + count, 0, sizeof(MemoryRegionPortio));

Wish: g_copy(pio, pio_init, count); // aka std::copy()

Quoted 13 lines

memory.c +1 -3

@@ -396,12 +395,12 @@ static void memory_region_iorange_read(IORange
 *iorange,
 
          *data = ((uint64_t)1<<  (width * 8)) - 1;
          if (mrp) {
-            *data = mrp->read(mr->opaque, offset + mr->offset);
+            *data = mrp->read(mr->opaque, offset + mrp->offset);
          } else if (width == 2) {
              mrp = find_portio(mr, offset, 1, false);
              assert(mrp);
-            *data = mrp->read(mr->opaque, offset + mr->offset) |
-                    (mrp->read(mr->opaque, offset + mr->offset +
 1)<<  8);

Quoted 6 lines

(quoted diff) +2 -0

+            *data = mrp->read(mr->opaque, offset + mrp->offset) |
+                    (mrp->read(mr->opaque, offset + mrp->offset +
 1)<<  8);

          }
          return;
      }

So long as mr->offset exists, you need to take it into account.

And I
don't want to remove memory_region_set_offset() until everything (that can potentially use it, at least) has been converted.

Quoted 8 lines · Avi Kivity

Quoted 5 lines · Jan Kiszka

From: Jan Kiszka<[email protected]>

We can express the offset of old portio completely via MemoryRegionPortio::offset by splitting up regions of different offsets and adjusting those offsets appropriately.

Please split into two patches - core and isa.

They depend on each other.

Quoted 3 lines

And I
don't want to remove memory_region_set_offset() until everything (that can potentially use it, at least) has been converted.

IMO it's easier to fix those potential users before converting them. You need to review them anyway to decide if an offset might be needed, and which one precisely.

Are you aware of any candidates? For PIO, there should be none now.

Quoted 11 lines · Jan Kiszka

Quoted 5 lines · Avi Kivity

Quoted 4 lines · Jan Kiszka

From: Jan Kiszka<[email protected]>

We can express the offset of old portio completely via MemoryRegionPortio::offset by splitting up regions of different

offsets

Quoted 3 lines

Quoted 1 line

and adjusting those offsets appropriately.

Please split into two patches - core and isa.

Quoted 1 line

They depend on each other.

How can memory.c depend on isa.c?

If you make the core patch add both mr->offset and mrp->offset, then change isa to drop memory_region_set_offset(), instead adding the delta to mrp->offset, does that not work out?

Quoted 10 lines

Quoted 4 lines

And I
don't want to remove memory_region_set_offset() until everything (that can potentially use it, at least) has been converted.

IMO it's easier to fix those potential users before converting them. You need to review them anyway to decide if an offset might be needed, and which one precisely.

Are you aware of any candidates? For PIO, there should be none now.

For pio, none, but mmio has some:

hw/sh7750.c:    cpu_register_physical_memory_offset(0x1f000000, 0x1000,
hw/sh7750.c:    cpu_register_physical_memory_offset(0xff000000, 0x1000,
hw/sh7750.c:    cpu_register_physical_memory_offset(0x1f800000, 0x1000,
hw/sh7750.c:    cpu_register_physical_memory_offset(0xff800000, 0x1000,
hw/sh7750.c:    cpu_register_physical_memory_offset(0x1fc00000, 0x1000,
hw/sh7750.c:    cpu_register_physical_memory_offset(0xffc00000, 0x1000,

hw/sh_intc.c:
cpu_register_physical_memory_offset(P4ADDR(address), 4, hw/sh_intc.c:
cpu_register_physical_memory_offset(A7ADDR(address), 4,

Quoted 4 lines

If you make the core patch add both mr->offset and mrp->offset, then change isa to drop memory_region_set_offset(), instead adding the delta to mrp->offset, does that not work out?

Nope. The old API accepted arbitrary portio lists per memory region, the new requires one region with a consistent offset per range. I should have documented it...

Quoted 20 lines

Quoted 11 lines

Quoted 5 lines

And I
don't want to remove memory_region_set_offset() until everything (that can potentially use it, at least) has been converted.

IMO it's easier to fix those potential users before converting them. You need to review them anyway to decide if an offset might be needed, and which one precisely.

Are you aware of any candidates? For PIO, there should be none now.

For pio, none, but mmio has some:

 hw/sh7750.c:    cpu_register_physical_memory_offset(0x1f000000, 0x1000,
 hw/sh7750.c:    cpu_register_physical_memory_offset(0xff000000, 0x1000,
 hw/sh7750.c:    cpu_register_physical_memory_offset(0x1f800000, 0x1000,
 hw/sh7750.c:    cpu_register_physical_memory_offset(0xff800000, 0x1000,
 hw/sh7750.c:    cpu_register_physical_memory_offset(0x1fc00000, 0x1000,
 hw/sh7750.c:    cpu_register_physical_memory_offset(0xffc00000, 0x1000,

Quoted 4 lines

hw/sh_intc.c:
cpu_register_physical_memory_offset(P4ADDR(address), 4, hw/sh_intc.c:
cpu_register_physical_memory_offset(A7ADDR(address), 4,

Cool, that's all. Trivial to fix, just push the offset math into those few handler. Then we can drop cpu_register_physical_memory_offset as well.

Quoted 4 lines · Jan Kiszka

Quoted 3 lines

If you make the core patch add both mr->offset and mrp->offset, then change isa to drop memory_region_set_offset(), instead adding the

delta

Quoted 5 lines

Quoted 1 line

to mrp->offset, does that not work out?

Nope. The old API accepted arbitrary portio lists per memory region, the new requires one region with a consistent offset per range. I should have documented it...

What does "a consistent offset per range" mean? You aren't actually changing the caller's ranges.

Quoted 36 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

And I
don't want to remove memory_region_set_offset() until

everything (that

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

can potentially use it, at least) has been converted.

IMO it's easier to fix those potential users before converting

them. You

Quoted 1 line

Quoted 1 line

need to review them anyway to decide if an offset might be needed,

and

Quoted 7 lines

Quoted 3 lines

which one precisely.

Are you aware of any candidates? For PIO, there should be none now.

For pio, none, but mmio has some:

hw/sh7750.c: cpu_register_physical_memory_offset(0x1f000000,

0x1000,

Quoted 1 line

hw/sh7750.c: cpu_register_physical_memory_offset(0xff000000,

0x1000,

Quoted 1 line

hw/sh7750.c: cpu_register_physical_memory_offset(0x1f800000,

0x1000,

Quoted 1 line

hw/sh7750.c: cpu_register_physical_memory_offset(0xff800000,

0x1000,

Quoted 1 line

hw/sh7750.c: cpu_register_physical_memory_offset(0x1fc00000,

0x1000,

Quoted 1 line

hw/sh7750.c: cpu_register_physical_memory_offset(0xffc00000,

0x1000,

Quoted 4 lines

hw/sh_intc.c:
cpu_register_physical_memory_offset(P4ADDR(address), 4, hw/sh_intc.c:
cpu_register_physical_memory_offset(A7ADDR(address), 4,

Quoted 3 lines

Cool, that's all. Trivial to fix, just push the offset math into those few handler. Then we can drop cpu_register_physical_memory_offset as well.

They all use the same handler, so you need to split e.g. sh7750_io_memory into six MemoryRegionsOps. Or use tricks with aliases - have one giant 4G region with one handler, and map six 4k aliases into the system address space.

Quoted 7 lines · Avi Kivity

Quoted 6 lines · Jan Kiszka

Quoted 3 lines

If you make the core patch add both mr->offset and mrp->offset, then change isa to drop memory_region_set_offset(), instead adding the

delta

Quoted 1 line

to mrp->offset, does that not work out?

Quoted 6 lines

Quoted 3 lines

Nope. The old API accepted arbitrary portio lists per memory region, the new requires one region with a consistent offset per range. I should have documented it...

What does "a consistent offset per range" mean? You aren't actually changing the caller's ranges.

I'm changing the way isa_register_portio_1 registers portios with the
core: only one per offset. The new commit log says:

"This implies that MemoryRegionPortio::offset is no longer used as offset within the memory region but just as a correction value for the offset passed to legacy handlers that expect absolute port addresses."

Quoted 5 lines

They all use the same handler, so you need to split e.g. sh7750_io_memory into six MemoryRegionsOps. Or use tricks with aliases - have one giant 4G region with one handler, and map six 4k aliases into the system address space.

Looks more like 3 regions with one alias each. But we likely need to disentangle all that logic first. I would be surprised if there wasn't a more readable way to express it via the memory API.

Quoted 17 lines · Jan Kiszka

Quoted 3 lines · Avi Kivity

Quoted 2 lines · Jan Kiszka

Quoted 2 lines

If you make the core patch add both mr->offset and

mrp->offset, then

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

change isa to drop memory_region_set_offset(), instead adding the

delta

Quoted 1 line

to mrp->offset, does that not work out?

Nope. The old API accepted arbitrary portio lists per memory

region, the

Quoted 5 lines

Quoted 2 lines

new requires one region with a consistent offset per range. I should have documented it...

What does "a consistent offset per range" mean? You aren't actually changing the caller's ranges.

Quoted 6 lines

I'm changing the way isa_register_portio_1 registers portios with the
core: only one per offset. The new commit log says:

"This implies that MemoryRegionPortio::offset is no longer used as offset within the memory region but just as a correction value for the offset passed to legacy handlers that expect absolute port addresses."

Ah:

(quoted diff) +1 -1

-        /* If we see a hole, break the region.  */
+        /* If we see a new offset, break the region. */

But, sorry for being slow, I don't see why it requires a core update (other for adding mrp->offset).

Quoted 8 lines

Quoted 4 lines

They all use the same handler, so you need to split e.g. sh7750_io_memory into six MemoryRegionsOps. Or use tricks with

aliases -

Quoted 2 lines

have one giant 4G region with one handler, and map six 4k aliases into the system address space.

Quoted 4 lines

Looks more like 3 regions with one alias each. But we likely need to disentangle all that logic first. I would be surprised if there wasn't a more readable way to express it via the memory API.

Depends if you subscribe to the "blindly make it work exactly the same way" or "understand the details and rewrite it cleanly" brands of masochism.

Quoted 9 lines

Ah:

- /* If we see a hole, break the region. */
+ /* If we see a new offset, break the region. */

But, sorry for being slow, I don't see why it requires a core update (other for adding mrp->offset).

So far we matched accesses in find_portio by considering the portio offset as well. If we want to replace the region offset with the portio one (which confines legacy to a legacy-only place), we need to make the portio offset a pure correction value on handler invocation and exclude it from any range matching. And that means an old_portio memory region can only describe one range starting exactly at MemoryRegion::addr.

Quoted 14 lines

Quoted 13 lines

Quoted 5 lines

They all use the same handler, so you need to split e.g. sh7750_io_memory into six MemoryRegionsOps. Or use tricks with

aliases -

Quoted 2 lines

have one giant 4G region with one handler, and map six 4k aliases into the system address space.

Looks more like 3 regions with one alias each. But we likely need to disentangle all that logic first. I would be surprised if there wasn't a more readable way to express it via the memory API.

Quoted 3 lines

Depends if you subscribe to the "blindly make it work exactly the same way" or "understand the details and rewrite it cleanly" brands of masochism.

We generally used to convert from APIv<n-1> to APIv<n> by adding legacy wrappers, rarely removing any of them. This doesn't scale, but - granted
- it requires some masochism to make progress.

memory.c static const MemoryRegionPortio *find_portio(MemoryRegion *mr, uint64_t offset, · line 378

Quoted 7 lines · Jan Kiszka

memory.c +1 -2

@@ -375,8 +375,7 @@ static const MemoryRegionPortio *find_portio(MemoryRegion *mr, uint64_t offset,
     const MemoryRegionPortio *mrp;
 
     for (mrp = mr->ops->old_portio; mrp->size; ++mrp) {
-        if (offset >= mrp->offset && offset < mrp->offset + mrp->len
-            && width == mrp->size
+        if (offset < mrp->len && width == mrp->size

This change looks broken to me. How, exactly, are you disambiguating different entries?

Quoted 9 lines · Richard Henderson

Quoted 7 lines · Jan Kiszka

@@ -375,8 +375,7 @@ static const MemoryRegionPortio *find_portio(MemoryRegion *mr, uint64_t offset,

      const MemoryRegionPortio *mrp;

      for (mrp = mr->ops->old_portio; mrp->size; ++mrp) {

- if (offset>= mrp->offset&& offset< mrp->offset + mrp->len
-&& width == mrp->size
+ if (offset< mrp->len&& width == mrp->size

Quoted 2 lines

This change looks broken to me. How, exactly, are you disambiguating different entries?

See my reply to Avi: all offsets of an portio region must be the same.

They should actually only differ in access width, but there is still at least one counter example (of course IDE...). Given that this is just a portability helper, all this will likely be reviewed and cleaned up when getting rid of old portio.