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

[Cédric Le Goater]

On 3/11/21 6:27 PM, Peter Xu wrote:
> On Thu, Mar 11, 2021 at 05:21:49PM +0100, Philippe Mathieu-Daudé wrote:
>> +Aspeed team
>>
>> On 3/11/21 1:18 PM, Philippe Mathieu-Daudé wrote:
>>> On 3/10/21 9:29 PM, Peter Xu wrote:
>>
>>>> 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.
>>
>> So using:
>>
>> -- >8 --
>> diff --git a/softmmu/memory.c b/softmmu/memory.c
>> index 874a8fccdee..8ce2d7f83b9 100644
>> --- a/softmmu/memory.c
>> +++ b/softmmu/memory.c
>> @@ -713,6 +713,12 @@ static MemoryRegion
>> *memory_region_get_flatview_root(MemoryRegion *mr)
>>                  continue;
>>              }
>>          }
>> +        if (mr && mr->addr) {
>> +            error_report("Detected flatview root memory region '%s' with"
>> +                         " non-zero base address (0x%"HWADDR_PRIx"):
>> aborting",
>> +                         memory_region_name(mr), mr->addr);
>> +            abort();
>> +        }
>>
>>          return mr;
>>      }
>> ---
> 
> Maybe it works, but it looks a bit odd to test here.  What I meant was
> something like attached.
> 
>>
>> I get:
>>
>> $ ./qemu-system-arm -M ast2600-evb
>> qemu-system-arm: Detected flatview root memory region
>> 'aspeed.fmc-ast2600.flash' with non-zero base address (0x20000000): aborting
>> Aborted (core dumped)
>>
>> Indeed:
>>
>> $ ./qemu-system-arm -M ast2600-evb -S -monitor stdio
>> QEMU 5.2.50 monitor - type 'help' for more information
>> (qemu) info mtree
>> address-space: dma-dram
>>   0000000080000000-000000017fffffff (prio 0, i/o): aspeed-ram-container
>>     0000000080000000-00000000bfffffff (prio 0, ram): ram
>>     00000000c0000000-00000000ffffffff (prio 0, i/o): max_ram
>>
>> address-space: aspeed.fmc-ast2600-dma-flash
>>   0000000020000000-000000002fffffff (prio 0, i/o): aspeed.fmc-ast2600.flash
>>     0000000020000000-0000000027ffffff (prio 0, i/o): aspeed.fmc-ast2600.0
>>     0000000028000000-000000002fffffff (prio 0, i/o): aspeed.fmc-ast2600.1
>>
>> address-space: aspeed.fmc-ast2600-dma-dram
>>   0000000080000000-000000017fffffff (prio 0, i/o): aspeed-ram-container
>>     0000000080000000-00000000bfffffff (prio 0, ram): ram
>>     00000000c0000000-00000000ffffffff (prio 0, i/o): max_ram
>>
>> address-space: aspeed.spi1-ast2600-dma-flash
>>   0000000030000000-000000003fffffff (prio 0, i/o): aspeed.spi1-ast2600.flash
>>     0000000030000000-0000000037ffffff (prio 0, i/o): aspeed.spi1-ast2600.0
>>
>> address-space: aspeed.spi1-ast2600-dma-dram
>>   0000000080000000-000000017fffffff (prio 0, i/o): aspeed-ram-container
>>     0000000080000000-00000000bfffffff (prio 0, ram): ram
>>     00000000c0000000-00000000ffffffff (prio 0, i/o): max_ram
>>
>> address-space: aspeed.spi2-ast2600-dma-flash
>>   0000000050000000-000000005fffffff (prio 0, i/o): aspeed.spi2-ast2600.flash
>>     0000000050000000-0000000057ffffff (prio 0, i/o): aspeed.spi2-ast2600.0
>>
>> address-space: aspeed.spi2-ast2600-dma-dram
>>   0000000080000000-000000017fffffff (prio 0, i/o): aspeed-ram-container
>>     0000000080000000-00000000bfffffff (prio 0, ram): ram
>>     00000000c0000000-00000000ffffffff (prio 0, i/o): max_ram
>>
>> Many address spaces not zero-based...
> 
> Maybe it's still legal to make the root mr a subregion of another, so maybe 
> I'm
> completely wrong... 

what is the problem you are trying to solve ? 

C. 




> then the patch attached won't make any sense either.  It's
> just that in my mind each MR should have a "parent" - for normal MR it's the
> container MR, then for root MR it's easier to see the AS as its "parent".
> 
> Maybe Paolo could clarify this..
> 
> Thanks,
>