[Qemu-devel] [PATCH] isa: Avoid using obsolete memory_region_set_offset for old portio

[Qemu-devel] [PATCH] isa: Avoid using obsolete memory_region_set_offset for old portio

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

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 <jan.kiszka@siemens.com>
---

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

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

diff --git a/hw/isa-bus.c b/hw/isa-bus.c
index 27c76b4..558312d 100644
--- a/hw/isa-bus.c
+++ b/hw/isa-bus.c
@@ -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;
         }
     }
 
diff --git a/memory.c b/memory.c
index aef4702..51f0297 100644
--- a/memory.c
+++ b/memory.c
@@ -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;
     }
-- 
1.7.3.4

[Qemu-devel] [PATCH] memory: Eliminate region offset

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

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 <jan.kiszka@siemens.com>
---
 memory.c |   14 ++++----------
 memory.h |    9 ---------
 2 files changed, 4 insertions(+), 19 deletions(-)

diff --git a/memory.c b/memory.c
index 51f0297..4dd63cc 100644
--- a/memory.c
+++ b/memory.c
@@ -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;
diff --git a/memory.h b/memory.h
index 06b83ae..b07cd55 100644
--- a/memory.h
+++ b/memory.h
@@ -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).
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH] isa: Avoid using obsolete memory_region_set_offset for old portio

[Avi Kivity]

On 09/18/2011 03:54 PM, Jan Kiszka wrote:
> From: Jan Kiszka<jan.kiszka@siemens.com>
>
> 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.

> +    /* 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()

> @@ -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;
>       }

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.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] isa: Avoid using obsolete memory_region_set_offset for old portio

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1989 bytes --]

On 2011-09-18 17:57, Avi Kivity wrote:
> On 09/18/2011 03:54 PM, Jan Kiszka wrote:
>> From: Jan Kiszka<jan.kiszka@siemens.com>
>>
>> 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.

> 
>> +    /* 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()
> 
>> @@ -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;
>>       }
> 
> So long as mr->offset exists, you need to take it into account.

Only fair.

>  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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH] isa: Avoid using obsolete memory_region_set_offset for old portio

[Avi Kivity]

On 09/18/2011 07:29 PM, Jan Kiszka wrote:
> On 2011-09-18 17:57, Avi Kivity wrote:
> >  On 09/18/2011 03:54 PM, Jan Kiszka wrote:
> >>  From: Jan Kiszka<jan.kiszka@siemens.com>
> >>
> >>  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?

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



-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] isa: Avoid using obsolete memory_region_set_offset for old portio

[Richard Henderson]

On 09/18/2011 05:54 AM, Jan Kiszka wrote:
> @@ -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?



r~

Re: [Qemu-devel] [PATCH] isa: Avoid using obsolete memory_region_set_offset for old portio

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2165 bytes --]

On 2011-09-18 18:46, Avi Kivity wrote:
> On 09/18/2011 07:29 PM, Jan Kiszka wrote:
>> On 2011-09-18 17:57, Avi Kivity wrote:
>> >  On 09/18/2011 03:54 PM, Jan Kiszka wrote:
>> >>  From: Jan Kiszka<jan.kiszka@siemens.com>
>> >>
>> >>  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?

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

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH] isa: Avoid using obsolete memory_region_set_offset for old portio

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 906 bytes --]

On 2011-09-18 18:49, Richard Henderson wrote:
> On 09/18/2011 05:54 AM, Jan Kiszka wrote:
>> @@ -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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH] isa: Avoid using obsolete memory_region_set_offset for old portio

[Avi Kivity]

On 09/18/2011 10:04 PM, Jan Kiszka wrote:
> >
> >  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.


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

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] isa: Avoid using obsolete memory_region_set_offset for old portio

[Avi Kivity]

On 09/18/2011 10:16 PM, Jan Kiszka wrote:
> On 2011-09-18 18:49, Richard Henderson wrote:
> >  On 09/18/2011 05:54 AM, Jan Kiszka wrote:
> >>  @@ -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.

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?

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

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] isa: Avoid using obsolete memory_region_set_offset for old portio

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2737 bytes --]

On 2011-09-19 14:14, Avi Kivity wrote:
> On 09/18/2011 10:04 PM, Jan Kiszka wrote:
>> >
>> >  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.

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH] isa: Avoid using obsolete memory_region_set_offset for old portio

[Avi Kivity]

On 09/19/2011 03:29 PM, Jan Kiszka wrote:
> On 2011-09-19 14:14, Avi Kivity wrote:
> >  On 09/18/2011 10:04 PM, Jan Kiszka wrote:
> >>  >
> >>  >   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.
>
> 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).

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

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] isa: Avoid using obsolete memory_region_set_offset for old portio

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2623 bytes --]

On 2011-09-19 14:37, Avi Kivity wrote:
> On 09/19/2011 03:29 PM, Jan Kiszka wrote:
>> On 2011-09-19 14:14, Avi Kivity wrote:
>> >  On 09/18/2011 10:04 PM, Jan Kiszka wrote:
>> >>  >
>> >>  >   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.
>>
>> 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).

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.

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH] isa: Avoid using obsolete memory_region_set_offset for old portio

[Avi Kivity]

On 09/19/2011 03:48 PM, Jan Kiszka wrote:
> >
> >  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.

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

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

-- 
error compiling committee.c: too many arguments to function