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

[Jan Kiszka <jan.kiszka@web.de>]

[-- 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 --]