| Message ID | 4E75EA08.4090809@web.de |
|---|---|
| State | New |
| Headers | show |
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.
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
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,
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~
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
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
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.
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.
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
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.
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
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.
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; }