isa: Avoid using obsolete memory_region_set_offset for old portio

PATCH In review Jan Kiszka 3 participants 14 messages Sep 18, 2011 – Sep 19, 2011

lore

Share & tools

lore

Jan Kiszka

author

2011-09-18 12:54 UTC #0 lore raw

PATCH 1/1 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.

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

---

Will write a patch to remove MemoryRegion::offset now.

 hw/isa-bus.c |   28 ++++++++++------------------
 memory.c     |   15 +++++++--------
 2 files changed, 17 insertions(+), 26 deletions(-)

hw/isa-bus.c +10 ~~-18~~

index 27c76b4..558312d 100644
@@ -107,19 +107,14 @@ static void isa_register_portio_1(ISADevice *dev,
     MemoryRegion *region;
     unsigned i;
 
-    if (off_low == 0 && pio_init[count].size == 0) {
-        /* Special case simple adjustments.  */
-        pio = (MemoryRegionPortio *) pio_init;
-    } else {
-        /* 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));
-
-        /* Adjust the offsets to all be zero-based for the region.  */
-        for (i = 0; i < count; ++i) {
-            pio[i].offset -= off_low;
-        }
+    /* 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));
+
+    /* Adjust the offsets to be absolute.  */
+    for (i = 0; i < count; ++i) {
+        pio[i].offset += start;
     }
 
     ops = g_new0(MemoryRegionOps, 1);

@@ -127,7 +122,6 @@ static void isa_register_portio_1(ISADevice *dev,
 
     region = g_new(MemoryRegion, 1);
     memory_region_init_io(region, ops, opaque, name, off_high - off_low);
-    memory_region_set_offset(region, start + off_low);
     memory_region_add_subregion(isabus->address_space_io,
                                 start + off_low, region);
 }

@@ -154,8 +148,8 @@ void isa_register_portio_list(ISADevice *dev, uint16_t start,
         assert(pio->offset >= off_last);
         off_last = pio->offset;
 
-        /* If we see a hole, break the region.  */
-        if (off_last > off_high) {
+        /* If we see a new offset, break the region. */
+        if (off_last > off_low) {
             isa_register_portio_1(dev, pio_start, count, start, off_low,
                                   off_high, opaque, name);
             /* ... and start collecting anew.  */

@@ -163,8 +157,6 @@ void isa_register_portio_list(ISADevice *dev, uint16_t start,
             off_low = off_last;
             off_high = off_low + pio->len;
             count = 0;
-        } else if (off_last + pio->len > off_high) {
-            off_high = off_last + pio->len;
         }
     }

memory.c +7 -8

index aef4702..51f0297 100644
@@ -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
             && (write ? (bool)mrp->write : (bool)mrp->read)) {
             return mrp;
         }

@@ -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);
+            *data = mrp->read(mr->opaque, offset + mrp->offset) |
+                    (mrp->read(mr->opaque, offset + mrp->offset + 1) << 8);
         }
         return;
     }

@@ -423,12 +422,12 @@ static void memory_region_iorange_write(IORange *iorange,
         const MemoryRegionPortio *mrp = find_portio(mr, offset, width, true);
 
         if (mrp) {
-            mrp->write(mr->opaque, offset + mr->offset, data);
+            mrp->write(mr->opaque, offset + mrp->offset, data);
         } else if (width == 2) {
             mrp = find_portio(mr, offset, 1, false);
             assert(mrp);
-            mrp->write(mr->opaque, offset + mr->offset, data & 0xff);
-            mrp->write(mr->opaque, offset + mr->offset + 1, data >> 8);
+            mrp->write(mr->opaque, offset + mrp->offset, data & 0xff);
+            mrp->write(mr->opaque, offset + mrp->offset + 1, data >> 8);
         }
         return;
     }

Signature · 1 line

1.7.3.4

Jan Kiszka

author

2011-09-18 13:09 UTC #1 lore raw

PATCH 1/1 memory: Eliminate region offset

From: Jan Kiszka <[email protected]>

Before anything makes use of this legacy mechanism again, remove it. This will enforce proper conversion of device models while they are ported over the memory API.

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

---
 memory.c |   14 ++++----------
 memory.h |    9 ---------
 2 files changed, 4 insertions(+), 19 deletions(-)

memory.c +4 ~~-10~~

index 51f0297..4dd63cc 100644
@@ -405,7 +405,7 @@ static void memory_region_iorange_read(IORange *iorange,
         return;
     }
     *data = 0;
-    access_with_adjusted_size(offset + mr->offset, data, width,
+    access_with_adjusted_size(offset, data, width,
                               mr->ops->impl.min_access_size,
                               mr->ops->impl.max_access_size,
                               memory_region_read_accessor, mr);

@@ -431,7 +431,7 @@ static void memory_region_iorange_write(IORange *iorange,
         }
         return;
     }
-    access_with_adjusted_size(offset + mr->offset, &data, width,
+    access_with_adjusted_size(offset, &data, width,
                               mr->ops->impl.min_access_size,
                               mr->ops->impl.max_access_size,
                               memory_region_write_accessor, mr);

@@ -778,7 +778,6 @@ void memory_region_init(MemoryRegion *mr,
     mr->parent = NULL;
     mr->size = size;
     mr->addr = 0;
-    mr->offset = 0;
     mr->terminates = false;
     mr->readable = true;
     mr->destructor = memory_region_destructor_none;

@@ -830,7 +829,7 @@ static uint32_t memory_region_read_thunk_n(void *_mr,
     }
 
     /* FIXME: support unaligned access */
-    access_with_adjusted_size(addr + mr->offset, &data, size,
+    access_with_adjusted_size(addr, &data, size,
                               mr->ops->impl.min_access_size,
                               mr->ops->impl.max_access_size,
                               memory_region_read_accessor, mr);

@@ -855,7 +854,7 @@ static void memory_region_write_thunk_n(void *_mr,
     }
 
     /* FIXME: support unaligned access */
-    access_with_adjusted_size(addr + mr->offset, &data, size,
+    access_with_adjusted_size(addr, &data, size,
                               mr->ops->impl.min_access_size,
                               mr->ops->impl.max_access_size,
                               memory_region_write_accessor, mr);

@@ -1004,11 +1003,6 @@ uint64_t memory_region_size(MemoryRegion *mr)
     return mr->size;
 }
 
-void memory_region_set_offset(MemoryRegion *mr, target_phys_addr_t offset)
-{
-    mr->offset = offset;
-}
-
 void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
 {
     uint8_t mask = 1 << client;

memory.h +0 -9

index 06b83ae..b07cd55 100644
@@ -107,7 +107,6 @@ struct MemoryRegion {
     MemoryRegion *parent;
     uint64_t size;
     target_phys_addr_t addr;
-    target_phys_addr_t offset;
     bool backend_registered;
     void (*destructor)(MemoryRegion *mr);
     ram_addr_t ram_addr;

@@ -268,14 +267,6 @@ uint64_t memory_region_size(MemoryRegion *mr);
 void *memory_region_get_ram_ptr(MemoryRegion *mr);
 
 /**
- * memory_region_set_offset: Sets an offset to be added to MemoryRegionOps
- *                           callbacks.
- *
- * This function is deprecated and should not be used in new code.
- */
-void memory_region_set_offset(MemoryRegion *mr, target_phys_addr_t offset);
-
-/**
  * memory_region_set_log: Turn dirty logging on or off for a region.
  *
  * Turns dirty logging on or off for a specified client (display, migration).

Signature · 1 line

1.7.3.4

Avi Kivity

2011-09-18 15:57 UTC #2 lore raw

In reply to Jan Kiszka

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 4 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 6 lines

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);

memory.c static void memory_region_iorange_read(IORange *iorange, · line 406

Quoted 10 lines

memory.c +2 -2

          } 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);
+            *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.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2011-09-18 16:29 UTC #3 lore raw

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 21 lines

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 8 lines

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.

Only fair.

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.

Jan

Avi Kivity

2011-09-18 16:46 UTC #4 lore raw

Quoted 11 lines · Jan Kiszka

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.

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 9 lines

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.

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,

Signature · 1 line

error compiling committee.c: too many arguments to function

Richard Henderson

2011-09-18 16:49 UTC #11 lore raw

In reply to Jan Kiszka

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?

Jan Kiszka

author

2011-09-18 19:05 UTC #5 lore raw

In reply to Avi Kivity

Quoted 12 lines · Avi Kivity

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 7 lines

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?

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 19 lines

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,

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.

Jan

Jan Kiszka

author

2011-09-18 19:16 UTC #12 lore raw

In reply to Richard Henderson

Quoted 11 lines · Richard Henderson

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?

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.

Jan

Avi Kivity

2011-09-19 12:14 UTC #6 lore raw

In reply to Jan Kiszka

Quoted 8 lines · Jan Kiszka

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...

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

Quoted 25 lines

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,

Quoted 2 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.

Signature · 1 line

error compiling committee.c: too many arguments to function

Avi Kivity

2011-09-19 12:15 UTC #13 lore raw

In reply to Jan Kiszka

Quoted 10 lines · Jan Kiszka

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 4 lines

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.

Said Avi doesn't understand. VGA for example has many ports.

Or are you saying, split the input into sets of discontinuous ports, within each set you can use only one offset?

Quoted 5 lines

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.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2011-09-19 12:29 UTC #7 lore raw

In reply to Avi Kivity

Quoted 5 lines · Avi Kivity

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 8 lines

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.

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 36 lines

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 8 lines

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.

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.

Jan

Avi Kivity

2011-09-19 12:37 UTC #8 lore raw

Quoted 15 lines · Jan Kiszka

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.

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:

- /* 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 10 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 - 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.

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.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2011-09-19 12:48 UTC #9 lore raw

Quoted 32 lines · Avi Kivity

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. */

Quoted 2 lines

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 8 lines

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 8 lines

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.

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.

Jan

Avi Kivity

2011-09-19 12:59 UTC #10 lore raw

Quoted 10 lines · Jan Kiszka

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).

Quoted 6 lines

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.

Thanks for the explanation. But I think you're trying to remove ->offset by moving it somewhere else. That's not removal, that's renaming, and it reduces functionality for other old_portio users.

If users need absolute addresses, then we should provide them via set_offset(), not pretend the need doesn't exist.

(btw, another way to emulate set_offset() is via aliases, as detailed in the other thread).

Quoted 14 lines

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 8 lines

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.

Wrappers reduce the risk of regression from a bad conversion by a tired coder. But yes, they increase the amount of cruft immensely.

Signature · 1 line

error compiling committee.c: too many arguments to function