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