Re: [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io()

[Philippe Mathieu-Daudé]

On 3/10/21 9:29 PM, Peter Xu wrote:
> On Wed, Mar 10, 2021 at 09:11:40PM +0100, Philippe Mathieu-Daudé wrote:
>>>>      /* ISA bus: IO space at 0x90000000, mem space at 0x91000000 */
>>>> -    memory_region_init(isa_io, NULL, "isa-io", 0x00010000);
>>>>      memory_region_init(isa_mem, NULL, "isa-mem", 0x01000000);
>>>> -    memory_region_add_subregion(address_space, 0x90000000, isa_io);
>>>> +    memory_region_add_subregion(address_space, 0x90000000, 
>>>> get_system_io());
>>>
>>> The old code has an alias created just for adding subregion into 
>>> address_space:
>>>
>>> -    /* ISA IO space at 0x90000000 */
>>> -    memory_region_init_alias(isa, NULL, "isa_mmio",
>>> -                             get_system_io(), 0, 0x01000000);
>>> -    memory_region_add_subregion(address_space, 0x90000000, isa);
>>> -    isa_mem_base = 0x11000000;
>>>
>>> While you didn't revert that part.  Maybe that's the issue?
>>
>> Hmm I'll have a look. This is not the series I'm working on, which
>> is much bigger and not ready for posting yet. I simply looked for
>> something similar (a bus mapped into sysbus) and remembered the
>> ISA bus from Jazz machines. I'll see if I can find a better PoC.
> 
> Yeah no worry - it's just that I feel one memory_region_init_alias() call is
> probably missing in your huge series somewhere, so that you'll take that alias
> MR as subregion rather than the real MR (which is the root of one AS).

OK, with your earlier comments start + Mark other comment I start
to understand better.

So far:

(1a) AddressSpace is a physical view, its base address must be zero

(1b) AddressSpace aperture is fixed (depends on hardware design,
not changeable at runtime

Therefore due to (1a):
(2) AddressSpace root MemoryRegion is a container and must not be
mmio-mapped anywhere (in particular not on SysBus).

(3) If hardware has a MMIO view of an AddressSpace, it has to be
via a MemoryRegion alias. That way the alias handles paddr offset
adjustment to the zero-based AddressSpace root container MR.
Aliasing allows resizing the alias size without modifying the AS
aperture size (1b).

I'll start adding assertions for (1a) and (2) in the code base and
see if (3) adjustments are required.

Thanks both!

Phil.