* [PATCH 0/3] memory: Display AddressSpace zero-based in 'info mtree' @ 2021-03-05 23:54 Philippe Mathieu-Daudé 2021-03-05 23:54 ` [PATCH 1/3] memory: Better name 'offset' argument in mtree_print_mr() Philippe Mathieu-Daudé ` (2 more replies) 0 siblings, 3 replies; 25+ messages in thread From: Philippe Mathieu-Daudé @ 2021-03-05 23:54 UTC (permalink / raw) To: qemu-devel Cc: Paolo Bonzini, Philippe Mathieu-Daudé, Peter Xu, Alexander Bulekov Hi, I have been confused for some years by AddressSpace being displayed based on the physical address of their first MemoryRegion. The actual fix is quite trivial for my needs, but I might not see all the possible side effects. Altough the change are restricted to mtree_info() which is only available on the monitor. Last patch is a minor cleanup. I ended not using it. Regards, Phil. Philippe Mathieu-Daudé (3): memory: Better name 'offset' argument in mtree_print_mr() memory: Provide 'base address' argument to mtree_print_mr() memory: Make memory_region_to_absolute_addr() take a const MemoryRegion softmmu/memory.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) -- 2.26.2 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 1/3] memory: Better name 'offset' argument in mtree_print_mr() 2021-03-05 23:54 [PATCH 0/3] memory: Display AddressSpace zero-based in 'info mtree' Philippe Mathieu-Daudé @ 2021-03-05 23:54 ` Philippe Mathieu-Daudé 2021-03-05 23:54 ` [PATCH 2/3] memory: Provide 'base address' argument to mtree_print_mr() Philippe Mathieu-Daudé 2021-03-05 23:54 ` [PATCH 3/3] memory: Make memory_region_to_absolute_addr() take a const MemoryRegion Philippe Mathieu-Daudé 2 siblings, 0 replies; 25+ messages in thread From: Philippe Mathieu-Daudé @ 2021-03-05 23:54 UTC (permalink / raw) To: qemu-devel Cc: Paolo Bonzini, Philippe Mathieu-Daudé, Peter Xu, Alexander Bulekov The 'base' argument of mtree_print_mr() actually represents an offset, not a base address. Rename it as such. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- softmmu/memory.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/softmmu/memory.c b/softmmu/memory.c index 874a8fccdee..e4d93b2fd6f 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -2925,7 +2925,7 @@ static void mtree_print_mr_owner(const MemoryRegion *mr) } static void mtree_print_mr(const MemoryRegion *mr, unsigned int level, - hwaddr base, + hwaddr offset, MemoryRegionListHead *alias_print_queue, bool owner, bool display_disabled) { @@ -2939,7 +2939,7 @@ static void mtree_print_mr(const MemoryRegion *mr, unsigned int level, return; } - cur_start = base + mr->addr; + cur_start = offset + mr->addr; cur_end = cur_start + MR_SIZE(mr->size); /* @@ -2947,7 +2947,7 @@ static void mtree_print_mr(const MemoryRegion *mr, unsigned int level, * happen normally. When it happens, we dump something to warn the * user who is observing this. */ - if (cur_start < base || cur_end < cur_start) { + if (cur_start < offset || cur_end < cur_start) { qemu_printf("[DETECTED OVERFLOW!] "); } -- 2.26.2 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 2/3] memory: Provide 'base address' argument to mtree_print_mr() 2021-03-05 23:54 [PATCH 0/3] memory: Display AddressSpace zero-based in 'info mtree' Philippe Mathieu-Daudé 2021-03-05 23:54 ` [PATCH 1/3] memory: Better name 'offset' argument in mtree_print_mr() Philippe Mathieu-Daudé @ 2021-03-05 23:54 ` Philippe Mathieu-Daudé 2021-03-08 23:40 ` Peter Xu 2021-03-05 23:54 ` [PATCH 3/3] memory: Make memory_region_to_absolute_addr() take a const MemoryRegion Philippe Mathieu-Daudé 2 siblings, 1 reply; 25+ messages in thread From: Philippe Mathieu-Daudé @ 2021-03-05 23:54 UTC (permalink / raw) To: qemu-devel Cc: Paolo Bonzini, Philippe Mathieu-Daudé, Peter Xu, Alexander Bulekov Currently AdressSpace are display in 'info mtree' based on the physical address of their first MemoryRegion. This is rather confusing. Provide a 'base' address argument to mtree_print_mr() and use it in mtree_info() to display AdressSpace always based at address 0. Display behavior of MemoryRegions and FlatViews is not modified. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- softmmu/memory.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/softmmu/memory.c b/softmmu/memory.c index e4d93b2fd6f..991d9227a88 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -2925,7 +2925,7 @@ static void mtree_print_mr_owner(const MemoryRegion *mr) } static void mtree_print_mr(const MemoryRegion *mr, unsigned int level, - hwaddr offset, + hwaddr offset, hwaddr base, MemoryRegionListHead *alias_print_queue, bool owner, bool display_disabled) { @@ -2974,7 +2974,7 @@ static void mtree_print_mr(const MemoryRegion *mr, unsigned int level, qemu_printf(TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d, %s%s): alias %s @%s " TARGET_FMT_plx "-" TARGET_FMT_plx "%s", - cur_start, cur_end, + cur_start - base, cur_end - base, mr->priority, mr->nonvolatile ? "nv-" : "", memory_region_type((MemoryRegion *)mr), @@ -2995,7 +2995,7 @@ static void mtree_print_mr(const MemoryRegion *mr, unsigned int level, } qemu_printf(TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d, %s%s): %s%s", - cur_start, cur_end, + cur_start - base, cur_end - base, mr->priority, mr->nonvolatile ? "nv-" : "", memory_region_type((MemoryRegion *)mr), @@ -3028,7 +3028,7 @@ static void mtree_print_mr(const MemoryRegion *mr, unsigned int level, } QTAILQ_FOREACH(ml, &submr_print_queue, mrqueue) { - mtree_print_mr(ml->mr, level + 1, cur_start, + mtree_print_mr(ml->mr, level + 1, cur_start, base, alias_print_queue, owner, display_disabled); } @@ -3188,14 +3188,15 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled) QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) { qemu_printf("address-space: %s\n", as->name); - mtree_print_mr(as->root, 1, 0, &ml_head, owner, disabled); + mtree_print_mr(as->root, 1, 0, as->root->addr, + &ml_head, owner, disabled); qemu_printf("\n"); } /* print aliased regions */ QTAILQ_FOREACH(ml, &ml_head, mrqueue) { qemu_printf("memory-region: %s\n", memory_region_name(ml->mr)); - mtree_print_mr(ml->mr, 1, 0, &ml_head, owner, disabled); + mtree_print_mr(ml->mr, 1, 0, 0, &ml_head, owner, disabled); qemu_printf("\n"); } -- 2.26.2 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 2/3] memory: Provide 'base address' argument to mtree_print_mr() 2021-03-05 23:54 ` [PATCH 2/3] memory: Provide 'base address' argument to mtree_print_mr() Philippe Mathieu-Daudé @ 2021-03-08 23:40 ` Peter Xu 2021-03-09 9:39 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 25+ messages in thread From: Peter Xu @ 2021-03-08 23:40 UTC (permalink / raw) To: Philippe Mathieu-Daudé; +Cc: Paolo Bonzini, qemu-devel, Alexander Bulekov Phil, On Sat, Mar 06, 2021 at 12:54:13AM +0100, Philippe Mathieu-Daudé wrote: > @@ -3188,14 +3188,15 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled) > > QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) { > qemu_printf("address-space: %s\n", as->name); > - mtree_print_mr(as->root, 1, 0, &ml_head, owner, disabled); > + mtree_print_mr(as->root, 1, 0, as->root->addr, Root MR of any address space should have mr->addr==0, right? I'm slightly confused on what this patch wanted to do if so, since then "base" will always be zero.. Or am I wrong? Thanks, > + &ml_head, owner, disabled); > qemu_printf("\n"); > } > > /* print aliased regions */ > QTAILQ_FOREACH(ml, &ml_head, mrqueue) { > qemu_printf("memory-region: %s\n", memory_region_name(ml->mr)); > - mtree_print_mr(ml->mr, 1, 0, &ml_head, owner, disabled); > + mtree_print_mr(ml->mr, 1, 0, 0, &ml_head, owner, disabled); > qemu_printf("\n"); > } > > -- > 2.26.2 > -- Peter Xu ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/3] memory: Provide 'base address' argument to mtree_print_mr() 2021-03-08 23:40 ` Peter Xu @ 2021-03-09 9:39 ` Philippe Mathieu-Daudé 2021-03-09 21:54 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 25+ messages in thread From: Philippe Mathieu-Daudé @ 2021-03-09 9:39 UTC (permalink / raw) To: Peter Xu; +Cc: Paolo Bonzini, qemu-devel, Alexander Bulekov Hi Peter, On 3/9/21 12:40 AM, Peter Xu wrote:> On Sat, Mar 06, 2021 at 12:54:13AM +0100, Philippe Mathieu-Daudé wrote: >> @@ -3188,14 +3188,15 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled) >> >> QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) { >> qemu_printf("address-space: %s\n", as->name); >> - mtree_print_mr(as->root, 1, 0, &ml_head, owner, disabled); >> + mtree_print_mr(as->root, 1, 0, as->root->addr, > > Root MR of any address space should have mr->addr==0, right? > > I'm slightly confused on what this patch wanted to do if so, since then "base" > will always be zero.. Or am I wrong? That is what I am expecting too... Maybe the problem is elsewhere when I create the address space... The simpler way to figure it out is add an assertion. I haven't figure out my issue yet, I'll follow up later with a proof-of-concept series which triggers the assertion. FYI I also have to modify: -- >8 -- diff --git a/softmmu/physmem.c b/softmmu/physmem.c index e19bc9f1c19..41a77e15752 100644 --- a/softmmu/physmem.c +++ b/softmmu/physmem.c @@ -2889,7 +2889,7 @@ MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr, if (len > 0) { RCU_READ_LOCK_GUARD(); fv = address_space_to_flatview(as); - result = flatview_read(fv, addr, attrs, buf, len); + result = flatview_read(fv, as->root->addr + addr, attrs, buf, len); } return result; @@ -2905,7 +2905,7 @@ MemTxResult address_space_write(AddressSpace *as, hwaddr addr, if (len > 0) { RCU_READ_LOCK_GUARD(); fv = address_space_to_flatview(as); - result = flatview_write(fv, addr, attrs, buf, len); + result = flatview_write(fv, as->root->addr + addr, attrs, buf, len); } return result; @@ -3117,7 +3117,8 @@ bool address_space_access_valid(AddressSpace *as, hwaddr addr, RCU_READ_LOCK_GUARD(); fv = address_space_to_flatview(as); - result = flatview_access_valid(fv, addr, len, is_write, attrs); + result = flatview_access_valid(fv, as->root->addr + addr, len, + is_write, attrs); return result; } --- ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 2/3] memory: Provide 'base address' argument to mtree_print_mr() 2021-03-09 9:39 ` Philippe Mathieu-Daudé @ 2021-03-09 21:54 ` Philippe Mathieu-Daudé 2021-03-10 17:06 ` Peter Xu 2021-03-10 22:00 ` Mark Cave-Ayland 0 siblings, 2 replies; 25+ messages in thread From: Philippe Mathieu-Daudé @ 2021-03-09 21:54 UTC (permalink / raw) To: Peter Xu, Peter Maydell, Mark Cave-Ayland, Edgar E. Iglesias Cc: Paolo Bonzini, qemu-devel, Alexander Bulekov +Peter/Mark/Edgar for SoC modelling On 3/9/21 10:39 AM, Philippe Mathieu-Daudé wrote: > Hi Peter, > > On 3/9/21 12:40 AM, Peter Xu wrote:> On Sat, Mar 06, 2021 at 12:54:13AM > +0100, Philippe Mathieu-Daudé wrote: >>> @@ -3188,14 +3188,15 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled) >>> >>> QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) { >>> qemu_printf("address-space: %s\n", as->name); >>> - mtree_print_mr(as->root, 1, 0, &ml_head, owner, disabled); >>> + mtree_print_mr(as->root, 1, 0, as->root->addr, >> >> Root MR of any address space should have mr->addr==0, right? >> >> I'm slightly confused on what this patch wanted to do if so, since then "base" >> will always be zero.. Or am I wrong? > > That is what I am expecting too... Maybe the problem is elsewhere > when I create the address space... The simpler way to > figure it out is add an assertion. I haven't figure out my > issue yet, I'll follow up later with a proof-of-concept series > which triggers the assertion. To trigger I simply use: mydevice_realize() { memory_region_init(&mr, obj, name, size); address_space_init(&as, &mr, name); // here we have as.root.addr = 0 sysbus_init_mmio(sbd, &mr); } soc_realize() { ... sysbus_realize(SYS_BUS_DEVICE(&mydevice), &error_abort); sysbus_mmio_map(SYS_BUS_DEVICE(&mydevice), 0, NONZERO_ADDRESS); ... } sysbus_mmio_map -> sysbus_mmio_map_common -> memory_region_add_subregion -> memory_region_add_subregion_common Which does: static void memory_region_add_subregion_common(MemoryRegion *mr, hwaddr offset, MemoryRegion *subregion) { assert(!subregion->container); subregion->container = mr; subregion->addr = offset; // subregion = &as.root // offset = NONZERO_ADDRESS // so here as.root.addr becomes non-zero Is that use case incorrect? If so: - where to add the proper assertions to avoid having address spaces with non-zero base address? - what is the proper design? Else what should we fix? Thanks, Phil. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/3] memory: Provide 'base address' argument to mtree_print_mr() 2021-03-09 21:54 ` Philippe Mathieu-Daudé @ 2021-03-10 17:06 ` Peter Xu 2021-03-10 19:09 ` Philippe Mathieu-Daudé 2021-03-10 22:00 ` Mark Cave-Ayland 1 sibling, 1 reply; 25+ messages in thread From: Peter Xu @ 2021-03-10 17:06 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Peter Maydell, Mark Cave-Ayland, qemu-devel, Alexander Bulekov, Edgar E. Iglesias, Paolo Bonzini Phil, On Tue, Mar 09, 2021 at 10:54:18PM +0100, Philippe Mathieu-Daudé wrote: > +Peter/Mark/Edgar for SoC modelling > > On 3/9/21 10:39 AM, Philippe Mathieu-Daudé wrote: > > Hi Peter, > > > > On 3/9/21 12:40 AM, Peter Xu wrote:> On Sat, Mar 06, 2021 at 12:54:13AM > > +0100, Philippe Mathieu-Daudé wrote: > >>> @@ -3188,14 +3188,15 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled) > >>> > >>> QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) { > >>> qemu_printf("address-space: %s\n", as->name); > >>> - mtree_print_mr(as->root, 1, 0, &ml_head, owner, disabled); > >>> + mtree_print_mr(as->root, 1, 0, as->root->addr, > >> > >> Root MR of any address space should have mr->addr==0, right? > >> > >> I'm slightly confused on what this patch wanted to do if so, since then "base" > >> will always be zero.. Or am I wrong? > > > > That is what I am expecting too... Maybe the problem is elsewhere > > when I create the address space... The simpler way to > > figure it out is add an assertion. I haven't figure out my > > issue yet, I'll follow up later with a proof-of-concept series > > which triggers the assertion. > > To trigger I simply use: > > mydevice_realize() > { > memory_region_init(&mr, obj, name, size); > > address_space_init(&as, &mr, name); Could I ask why you need to set this sysbus mmio region as root MR of as? What's AS used for here? Btw, normally I see these regions should be initialized with memory_region_init_io(), since normally that MR will need a MemoryRegionOps bound to it to trap MMIOs, iiuc. Thanks, > // here we have as.root.addr = 0 > > sysbus_init_mmio(sbd, &mr); > } -- Peter Xu ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/3] memory: Provide 'base address' argument to mtree_print_mr() 2021-03-10 17:06 ` Peter Xu @ 2021-03-10 19:09 ` Philippe Mathieu-Daudé 2021-03-10 19:12 ` [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io() Philippe Mathieu-Daudé 2021-03-10 19:31 ` [PATCH 2/3] memory: Provide 'base address' argument to mtree_print_mr() Peter Xu 0 siblings, 2 replies; 25+ messages in thread From: Philippe Mathieu-Daudé @ 2021-03-10 19:09 UTC (permalink / raw) To: Peter Xu Cc: Peter Maydell, Mark Cave-Ayland, qemu-devel, Alexander Bulekov, Paolo Bonzini, Edgar E. Iglesias On 3/10/21 6:06 PM, Peter Xu wrote: > Phil, > > On Tue, Mar 09, 2021 at 10:54:18PM +0100, Philippe Mathieu-Daudé wrote: >> +Peter/Mark/Edgar for SoC modelling >> >> On 3/9/21 10:39 AM, Philippe Mathieu-Daudé wrote: >>> Hi Peter, >>> >>> On 3/9/21 12:40 AM, Peter Xu wrote:> On Sat, Mar 06, 2021 at 12:54:13AM >>> +0100, Philippe Mathieu-Daudé wrote: >>>>> @@ -3188,14 +3188,15 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled) >>>>> >>>>> QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) { >>>>> qemu_printf("address-space: %s\n", as->name); >>>>> - mtree_print_mr(as->root, 1, 0, &ml_head, owner, disabled); >>>>> + mtree_print_mr(as->root, 1, 0, as->root->addr, >>>> >>>> Root MR of any address space should have mr->addr==0, right? >>>> >>>> I'm slightly confused on what this patch wanted to do if so, since then "base" >>>> will always be zero.. Or am I wrong? >>> >>> That is what I am expecting too... Maybe the problem is elsewhere >>> when I create the address space... The simpler way to >>> figure it out is add an assertion. I haven't figure out my >>> issue yet, I'll follow up later with a proof-of-concept series >>> which triggers the assertion. >> >> To trigger I simply use: >> >> mydevice_realize() >> { >> memory_region_init(&mr, obj, name, size); >> >> address_space_init(&as, &mr, name); > > Could I ask why you need to set this sysbus mmio region as root MR of as? > What's AS used for here? > > Btw, normally I see these regions should be initialized with > memory_region_init_io(), since normally that MR will need a MemoryRegionOps > bound to it to trap MMIOs, iiuc. This is the pattern for buses: static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, const char *name, int devfn, Error **errp) { ... memory_region_init(&pci_dev->bus_master_container_region, OBJECT(pci_dev), "bus master container", UINT64_MAX); address_space_init(&pci_dev->bus_master_as, &pci_dev->bus_master_container_region, pci_dev->name); .... AUXBus *aux_bus_init(DeviceState *parent, const char *name) { AUXBus *bus; Object *auxtoi2c; bus = AUX_BUS(qbus_create(TYPE_AUX_BUS, parent, name)); auxtoi2c = object_new_with_props(TYPE_AUXTOI2C, OBJECT(bus), "i2c", &error_abort, NULL); bus->bridge = AUXTOI2C(auxtoi2c); /* Memory related. */ bus->aux_io = g_malloc(sizeof(*bus->aux_io)); memory_region_init(bus->aux_io, OBJECT(bus), "aux-io", 1 * MiB); address_space_init(&bus->aux_addr_space, bus->aux_io, "aux-io"); return bus; } static void artist_realizefn(DeviceState *dev, Error **errp) { ... memory_region_init(&s->mem_as_root, OBJECT(dev), "artist", ~0ull); address_space_init(&s->as, &s->mem_as_root, "artist"); PCI host: PCIBus *gt64120_register(qemu_irq *pic) { ... memory_region_init(&d->pci0_mem, OBJECT(dev), "pci0-mem", 4 * GiB); address_space_init(&d->pci0_mem_as, &d->pci0_mem, "pci0-mem"); Also: static void pnv_xive_realize(DeviceState *dev, Error **errp) { ... /* * The XiveSource and XiveENDSource objects are realized with the * maximum allowed HW configuration. The ESB MMIO regions will be * resized dynamically when the controller is configured by the FW * to limit accesses to resources not provisioned. */ memory_region_init(&xive->ipi_mmio, OBJECT(xive), "xive-vc-ipi", PNV9_XIVE_VC_SIZE); address_space_init(&xive->ipi_as, &xive->ipi_mmio, "xive-vc-ipi"); memory_region_init(&xive->end_mmio, OBJECT(xive), "xive-vc-end", PNV9_XIVE_VC_SIZE); address_space_init(&xive->end_as, &xive->end_mmio, "xive-vc-end"); And: static void memory_map_init(void) { ... memory_region_init(system_memory, NULL, "system", UINT64_MAX); address_space_init(&address_space_memory, system_memory, "memory"); Or: static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) { ... /* * Memory region relationships looks like (Address range shows * only lower 32 bits to make it short in length...): * * |-----------------+-------------------+----------| * | Name | Address range | Priority | * |-----------------+-------------------+----------+ * | amdvi_root | 00000000-ffffffff | 0 | * | amdvi_iommu | 00000000-ffffffff | 1 | * | amdvi_iommu_ir | fee00000-feefffff | 64 | * |-----------------+-------------------+----------| */ memory_region_init(&amdvi_dev_as->root, OBJECT(s), "amdvi_root", UINT64_MAX); address_space_init(&amdvi_dev_as->as, &amdvi_dev_as->root, name); memory_region_init_io(&amdvi_dev_as->iommu_ir, OBJECT(s), &amdvi_ir_ops, s, "amd_iommu_ir", AMDVI_INT_ADDR_SIZE); memory_region_add_subregion_overlap(&amdvi_dev_as->root, AMDVI_INT_ADDR_FIRST, &amdvi_dev_as->iommu_ir, 64); memory_region_add_subregion_overlap(&amdvi_dev_as->root, 0, MEMORY_REGION(&amdvi_dev_as->iommu), 1); I'll send a reproducer. ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io() 2021-03-10 19:09 ` Philippe Mathieu-Daudé @ 2021-03-10 19:12 ` Philippe Mathieu-Daudé 2021-03-10 19:12 ` [PATCH 2/2] NOTFORMERGE memory: Ensure AddressSpace physical base address is zero Philippe Mathieu-Daudé ` (2 more replies) 2021-03-10 19:31 ` [PATCH 2/3] memory: Provide 'base address' argument to mtree_print_mr() Peter Xu 1 sibling, 3 replies; 25+ messages in thread From: Philippe Mathieu-Daudé @ 2021-03-10 19:12 UTC (permalink / raw) To: Peter Xu, qemu-devel Cc: Peter Maydell, Aleksandar Rikalo, Mark Cave-Ayland, Laurent Vivier, Philippe Mathieu-Daudé, Alexander Bulekov, Hervé Poussineau, Paolo Bonzini, Edgar E . Iglesias, Aurelien Jarno No need to create a local ISA I/O MemoryRegion, use get_system_io(). This partly reverts commit 5c63bcf7501527b844f61624957bdba254d75bfc. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/mips/jazz.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c index 1a0888a0fd5..9ac9361b7eb 100644 --- a/hw/mips/jazz.c +++ b/hw/mips/jazz.c @@ -158,7 +158,6 @@ static void mips_jazz_init(MachineState *machine, rc4030_dma *dmas; IOMMUMemoryRegion *rc4030_dma_mr; MemoryRegion *isa_mem = g_new(MemoryRegion, 1); - MemoryRegion *isa_io = g_new(MemoryRegion, 1); MemoryRegion *rtc = g_new(MemoryRegion, 1); MemoryRegion *i8042 = g_new(MemoryRegion, 1); MemoryRegion *dma_dummy = g_new(MemoryRegion, 1); @@ -259,11 +258,10 @@ static void mips_jazz_init(MachineState *machine, memory_region_add_subregion(address_space, 0x8000d000, dma_dummy); /* 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()); memory_region_add_subregion(address_space, 0x91000000, isa_mem); - isa_bus = isa_bus_new(NULL, isa_mem, isa_io, &error_abort); + isa_bus = isa_bus_new(NULL, isa_mem, get_system_io(), &error_abort); /* ISA devices */ i8259 = i8259_init(isa_bus, env->irq[4]); -- 2.26.2 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 2/2] NOTFORMERGE memory: Ensure AddressSpace physical base address is zero 2021-03-10 19:12 ` [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io() Philippe Mathieu-Daudé @ 2021-03-10 19:12 ` Philippe Mathieu-Daudé 2021-03-10 19:49 ` [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io() Peter Xu 2021-03-11 0:48 ` Jiaxun Yang 2 siblings, 0 replies; 25+ messages in thread From: Philippe Mathieu-Daudé @ 2021-03-10 19:12 UTC (permalink / raw) To: Peter Xu, qemu-devel Cc: Peter Maydell, Aleksandar Rikalo, Mark Cave-Ayland, Laurent Vivier, Philippe Mathieu-Daudé, Alexander Bulekov, Hervé Poussineau, Paolo Bonzini, Edgar E . Iglesias, Aurelien Jarno AddressSpaces aren't suppose to have a physical base address. Add an assertion to demostrate the problem: $ echo info mtree | qemu-system-mips64el -M magnum -S -monitor stdio QEMU 5.2.50 monitor - type 'help' for more information (qemu) info mtree address-space: memory 0000000000000000-ffffffffffffffff (prio 0, i/o): system 0000000000000000-0000000007ffffff (prio 0, ram): mips_jazz.ram 000000001fc00000-000000001fc7dfff (prio 0, rom): mips_jazz.bios 0000000040000000-00000000407fffff (prio 0, ram): vram 0000000060000000-000000006007ffff (prio 0, rom): g364fb.rom 0000000060080000-00000000601fffff (prio 0, i/o): ctrl 0000000080000000-00000000800002ff (prio 0, i/o): rc4030.chipset 0000000080001000-00000000800010ff (prio 0, i/o): dp8393x-regs 0000000080002000-000000008000200f (prio 0, i/o): esp-regs 0000000080003000-0000000080003007 (prio 0, i/o): fdc 0000000080004000-0000000080004fff (prio 0, i/o): rtc 0000000080005000-0000000080005fff (prio 0, i/o): i8042 0000000080006000-0000000080006007 (prio 0, i/o): serial 0000000080008000-0000000080008007 (prio 0, i/o): parallel 0000000080009000-000000008000afff (prio 0, i/o): nvram 000000008000b000-000000008000bfff (prio 0, rom): dp8393x-prom 000000008000d000-000000008000dfff (prio 0, i/o): dummy_dma 000000008000f000-000000008000f000 (prio 0, i/o): led 0000000090000000-000000009000ffff (prio 0, i/o): io 0000000090000000-0000000090000007 (prio 0, i/o): dma-chan 0000000090000008-000000009000000f (prio 0, i/o): dma-cont 0000000090000020-0000000090000021 (prio 0, i/o): pic 0000000090000040-0000000090000043 (prio 0, i/o): pit 0000000090000061-0000000090000061 (prio 0, i/o): pcspk 0000000090000070-0000000090000071 (prio 0, i/o): rtc 0000000090000070-0000000090000070 (prio 0, i/o): rtc-index 0000000090000081-0000000090000083 (prio 0, i/o): dma-page 0000000090000087-0000000090000087 (prio 0, i/o): dma-page 0000000090000089-000000009000008b (prio 0, i/o): dma-page 000000009000008f-000000009000008f (prio 0, i/o): dma-page 00000000900000a0-00000000900000a1 (prio 0, i/o): pic 00000000900000c0-00000000900000cf (prio 0, i/o): dma-chan 00000000900000d0-00000000900000df (prio 0, i/o): dma-cont 00000000900004d0-00000000900004d0 (prio 0, i/o): elcr 00000000900004d1-00000000900004d1 (prio 0, i/o): elcr 0000000091000000-0000000091ffffff (prio 0, i/o): isa-mem 00000000f0000000-00000000f0000fff (prio 0, i/o): rc4030.jazzio 00000000fff00000-00000000fff7dfff (prio 0, rom): alias mips_jazz.bios @mips_jazz.bios 0000000000000000-000000000007dfff address-space: I/O 0000000090000000-000000009000ffff (prio 0, i/o): io 0000000090000000-0000000090000007 (prio 0, i/o): dma-chan 0000000090000008-000000009000000f (prio 0, i/o): dma-cont 0000000090000020-0000000090000021 (prio 0, i/o): pic 0000000090000040-0000000090000043 (prio 0, i/o): pit 0000000090000061-0000000090000061 (prio 0, i/o): pcspk 0000000090000070-0000000090000071 (prio 0, i/o): rtc 0000000090000070-0000000090000070 (prio 0, i/o): rtc-index 0000000090000081-0000000090000083 (prio 0, i/o): dma-page 0000000090000087-0000000090000087 (prio 0, i/o): dma-page 0000000090000089-000000009000008b (prio 0, i/o): dma-page 000000009000008f-000000009000008f (prio 0, i/o): dma-page 00000000900000a0-00000000900000a1 (prio 0, i/o): pic 00000000900000c0-00000000900000cf (prio 0, i/o): dma-chan 00000000900000d0-00000000900000df (prio 0, i/o): dma-cont 00000000900004d0-00000000900004d0 (prio 0, i/o): elcr 00000000900004d1-00000000900004d1 (prio 0, i/o): elcr qemu-system-mips64el: softmmu/memory.c:3193: mtree_info: Assertion `!as->root->addr' failed. Aborted (core dumped) Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- softmmu/memory.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/softmmu/memory.c b/softmmu/memory.c index 874a8fccdee..164e7971e5b 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -3190,6 +3190,8 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled) qemu_printf("address-space: %s\n", as->name); mtree_print_mr(as->root, 1, 0, &ml_head, owner, disabled); qemu_printf("\n"); + /* address spaces aren't suppose to have a physical base address */ + assert(!as->root->addr); } /* print aliased regions */ -- 2.26.2 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io() 2021-03-10 19:12 ` [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io() Philippe Mathieu-Daudé 2021-03-10 19:12 ` [PATCH 2/2] NOTFORMERGE memory: Ensure AddressSpace physical base address is zero Philippe Mathieu-Daudé @ 2021-03-10 19:49 ` Peter Xu 2021-03-10 20:11 ` Philippe Mathieu-Daudé 2021-03-11 0:48 ` Jiaxun Yang 2 siblings, 1 reply; 25+ messages in thread From: Peter Xu @ 2021-03-10 19:49 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Peter Maydell, Aleksandar Rikalo, Mark Cave-Ayland, qemu-devel, Laurent Vivier, Alexander Bulekov, Hervé Poussineau, Paolo Bonzini, Edgar E . Iglesias, Aurelien Jarno On Wed, Mar 10, 2021 at 08:12:54PM +0100, Philippe Mathieu-Daudé wrote: > No need to create a local ISA I/O MemoryRegion, use get_system_io(). > > This partly reverts commit 5c63bcf7501527b844f61624957bdba254d75bfc. I think it's not a clean revert of that, see below. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/mips/jazz.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c > index 1a0888a0fd5..9ac9361b7eb 100644 > --- a/hw/mips/jazz.c > +++ b/hw/mips/jazz.c > @@ -158,7 +158,6 @@ static void mips_jazz_init(MachineState *machine, > rc4030_dma *dmas; > IOMMUMemoryRegion *rc4030_dma_mr; > MemoryRegion *isa_mem = g_new(MemoryRegion, 1); > - MemoryRegion *isa_io = g_new(MemoryRegion, 1); > MemoryRegion *rtc = g_new(MemoryRegion, 1); > MemoryRegion *i8042 = g_new(MemoryRegion, 1); > MemoryRegion *dma_dummy = g_new(MemoryRegion, 1); > @@ -259,11 +258,10 @@ static void mips_jazz_init(MachineState *machine, > memory_region_add_subregion(address_space, 0x8000d000, dma_dummy); > > /* 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? -- Peter Xu ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io() 2021-03-10 19:49 ` [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io() Peter Xu @ 2021-03-10 20:11 ` Philippe Mathieu-Daudé 2021-03-10 20:29 ` Peter Xu 0 siblings, 1 reply; 25+ messages in thread From: Philippe Mathieu-Daudé @ 2021-03-10 20:11 UTC (permalink / raw) To: Peter Xu Cc: Peter Maydell, Aleksandar Rikalo, Mark Cave-Ayland, qemu-devel, Laurent Vivier, Alexander Bulekov, Hervé Poussineau, Edgar E . Iglesias, Paolo Bonzini, Aurelien Jarno On 3/10/21 8:49 PM, Peter Xu wrote: > On Wed, Mar 10, 2021 at 08:12:54PM +0100, Philippe Mathieu-Daudé wrote: >> No need to create a local ISA I/O MemoryRegion, use get_system_io(). >> >> This partly reverts commit 5c63bcf7501527b844f61624957bdba254d75bfc. > > I think it's not a clean revert of that, see below. > >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> hw/mips/jazz.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c >> index 1a0888a0fd5..9ac9361b7eb 100644 >> --- a/hw/mips/jazz.c >> +++ b/hw/mips/jazz.c >> @@ -158,7 +158,6 @@ static void mips_jazz_init(MachineState *machine, >> rc4030_dma *dmas; >> IOMMUMemoryRegion *rc4030_dma_mr; >> MemoryRegion *isa_mem = g_new(MemoryRegion, 1); >> - MemoryRegion *isa_io = g_new(MemoryRegion, 1); >> MemoryRegion *rtc = g_new(MemoryRegion, 1); >> MemoryRegion *i8042 = g_new(MemoryRegion, 1); >> MemoryRegion *dma_dummy = g_new(MemoryRegion, 1); >> @@ -259,11 +258,10 @@ static void mips_jazz_init(MachineState *machine, >> memory_region_add_subregion(address_space, 0x8000d000, dma_dummy); >> >> /* 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. Thanks for having a look at this patch, Phil. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io() 2021-03-10 20:11 ` Philippe Mathieu-Daudé @ 2021-03-10 20:29 ` Peter Xu 2021-03-11 12:18 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 25+ messages in thread From: Peter Xu @ 2021-03-10 20:29 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Peter Maydell, Aleksandar Rikalo, Mark Cave-Ayland, qemu-devel, Laurent Vivier, Alexander Bulekov, Hervé Poussineau, Edgar E . Iglesias, Paolo Bonzini, Aurelien Jarno 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). -- Peter Xu ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io() 2021-03-10 20:29 ` Peter Xu @ 2021-03-11 12:18 ` Philippe Mathieu-Daudé 2021-03-11 16:21 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 25+ messages in thread From: Philippe Mathieu-Daudé @ 2021-03-11 12:18 UTC (permalink / raw) To: Peter Xu, Mark Cave-Ayland Cc: Peter Maydell, Aleksandar Rikalo, qemu-devel, Laurent Vivier, Alexander Bulekov, Hervé Poussineau, Paolo Bonzini, Edgar E . Iglesias, Aurelien Jarno 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io() 2021-03-11 12:18 ` Philippe Mathieu-Daudé @ 2021-03-11 16:21 ` Philippe Mathieu-Daudé 2021-03-11 17:27 ` Peter Xu ` (2 more replies) 0 siblings, 3 replies; 25+ messages in thread From: Philippe Mathieu-Daudé @ 2021-03-11 16:21 UTC (permalink / raw) To: Peter Xu, Mark Cave-Ayland Cc: Peter Maydell, Aleksandar Rikalo, Andrew Jeffery, qemu-devel, Laurent Vivier, Alexander Bulekov, Hervé Poussineau, Cédric Le Goater, Edgar E . Iglesias, Paolo Bonzini, Aurelien Jarno, Joel Stanley +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; } --- 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... ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io() 2021-03-11 16:21 ` Philippe Mathieu-Daudé @ 2021-03-11 17:27 ` Peter Xu 2021-03-11 17:59 ` Philippe Mathieu-Daudé 2021-03-12 9:08 ` Cédric Le Goater 2021-03-11 17:27 ` Peter Xu 2021-03-11 17:41 ` Philippe Mathieu-Daudé 2 siblings, 2 replies; 25+ messages in thread From: Peter Xu @ 2021-03-11 17:27 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Peter Maydell, Aleksandar Rikalo, Andrew Jeffery, Mark Cave-Ayland, qemu-devel, Laurent Vivier, Alexander Bulekov, Hervé Poussineau, Cédric Le Goater, Edgar E . Iglesias, Paolo Bonzini, Aurelien Jarno, Joel Stanley 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... 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, -- Peter Xu ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io() 2021-03-11 17:27 ` Peter Xu @ 2021-03-11 17:59 ` Philippe Mathieu-Daudé 2021-03-12 9:08 ` Cédric Le Goater 1 sibling, 0 replies; 25+ messages in thread From: Philippe Mathieu-Daudé @ 2021-03-11 17:59 UTC (permalink / raw) To: Peter Xu, Paolo Bonzini Cc: Peter Maydell, Aleksandar Rikalo, Andrew Jeffery, Mark Cave-Ayland, qemu-devel, Laurent Vivier, Alexander Bulekov, Hervé Poussineau, Cédric Le Goater, Edgar E . Iglesias, Aurelien Jarno, Joel Stanley 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. > > Maybe it's still legal to make the root mr a subregion of another, so maybe I'm > completely wrong... then the patch attached won't make any sense either. Maybe it does, and we need to rework some boards code. With your patch applied: $ ./qemu-system-ppc -M 40p qemu-system-ppc: softmmu/memory.c:2445: memory_region_add_subregion_common: Assertion `!subregion->is_root_mr' failed. Aborted (core dumped) $ ./qemu-system-arm -M ast2600-evb qemu-system-arm: softmmu/memory.c:2445: memory_region_add_subregion_common: Assertion `!subregion->is_root_mr' failed. Aborted (core dumped) > 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, > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io() 2021-03-11 17:27 ` Peter Xu 2021-03-11 17:59 ` Philippe Mathieu-Daudé @ 2021-03-12 9:08 ` Cédric Le Goater 1 sibling, 0 replies; 25+ messages in thread From: Cédric Le Goater @ 2021-03-12 9:08 UTC (permalink / raw) To: Peter Xu, Philippe Mathieu-Daudé Cc: Peter Maydell, Aleksandar Rikalo, Andrew Jeffery, Mark Cave-Ayland, qemu-devel, Laurent Vivier, Alexander Bulekov, Hervé Poussineau, Joel Stanley, Edgar E . Iglesias, Paolo Bonzini, Aurelien Jarno 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, > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io() 2021-03-11 16:21 ` Philippe Mathieu-Daudé 2021-03-11 17:27 ` Peter Xu @ 2021-03-11 17:27 ` Peter Xu 2021-03-11 17:41 ` Philippe Mathieu-Daudé 2 siblings, 0 replies; 25+ messages in thread From: Peter Xu @ 2021-03-11 17:27 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Peter Maydell, Aleksandar Rikalo, Andrew Jeffery, Mark Cave-Ayland, qemu-devel, Laurent Vivier, Alexander Bulekov, Hervé Poussineau, Cédric Le Goater, Edgar E . Iglesias, Paolo Bonzini, Aurelien Jarno, Joel Stanley [-- Attachment #1: Type: text/plain, Size: 760 bytes --] On Thu, Mar 11, 2021 at 05:21:49PM +0100, Philippe Mathieu-Daudé wrote: > 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; > } (Attaching again...) -- Peter Xu [-- Attachment #2: 0001-memory-Make-sure-root-MR-won-t-be-added-as-subregion.patch --] [-- Type: text/plain, Size: 1658 bytes --] From 4fe7614f2087117aa912fd3d33d43ba02f2b50b1 Mon Sep 17 00:00:00 2001 From: Peter Xu <peterx@redhat.com> Date: Thu, 11 Mar 2021 11:40:21 -0500 Subject: [PATCH] memory: Make sure root MR won't be added as subregion Add a bool for MR to mark whether this MR is a root MR of an AS. We bail out asap if this MR is added as a subregion of another MR. Signed-off-by: Peter Xu <peterx@redhat.com> --- include/exec/memory.h | 1 + softmmu/memory.c | 2 ++ 2 files changed, 3 insertions(+) diff --git a/include/exec/memory.h b/include/exec/memory.h index c6fb714e499..211f10e877e 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -477,6 +477,7 @@ struct MemoryRegion { bool ram_device; bool enabled; bool warning_printed; /* For reservations */ + bool is_root_mr; uint8_t vga_logging_count; MemoryRegion *alias; hwaddr alias_offset; diff --git a/softmmu/memory.c b/softmmu/memory.c index 874a8fccdee..aebaa956258 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -2442,6 +2442,7 @@ static void memory_region_add_subregion_common(MemoryRegion *mr, MemoryRegion *subregion) { assert(!subregion->container); + assert(!subregion->is_root_mr); subregion->container = mr; subregion->addr = offset; memory_region_update_container_subregions(subregion); @@ -2819,6 +2820,7 @@ void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name) { memory_region_ref(root); as->root = root; + root->is_root_mr = true; as->current_map = NULL; as->ioeventfd_nb = 0; as->ioeventfds = NULL; -- 2.26.2 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io() 2021-03-11 16:21 ` Philippe Mathieu-Daudé 2021-03-11 17:27 ` Peter Xu 2021-03-11 17:27 ` Peter Xu @ 2021-03-11 17:41 ` Philippe Mathieu-Daudé 2 siblings, 0 replies; 25+ messages in thread From: Philippe Mathieu-Daudé @ 2021-03-11 17:41 UTC (permalink / raw) To: Peter Xu, Mark Cave-Ayland Cc: Peter Maydell, Aleksandar Rikalo, Andrew Jeffery, qemu-devel, Laurent Vivier, Alexander Bulekov, Hervé Poussineau, Cédric Le Goater, Paolo Bonzini, Edgar E . Iglesias, Aurelien Jarno, Joel Stanley On 3/11/21 5:21 PM, 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; > } > --- > > 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) Another one (PPC): $ ./qemu-system-ppc -S -monitor stdio -M 40p QEMU 5.2.50 monitor - type 'help' for more information (qemu) qemu-system-ppc: Detected flatview root memory region 'pci-io' with non-zero base address (0x80000000): aborting Aborted (core dumped) $ ./qemu-system-ppc -S -monitor stdio -M 40p QEMU 5.2.50 monitor - type 'help' for more information (qemu) info mtree address-space: raven-io 0000000080000000-00000000bf7fffff (prio 0, i/o): pci-io 0000000080000000-0000000080000007 (prio 0, i/o): dma-chan 0000000080000008-000000008000000f (prio 0, i/o): dma-cont 0000000080000020-0000000080000021 (prio 0, i/o): pic 0000000080000040-0000000080000043 (prio 0, i/o): pit 0000000080000060-0000000080000060 (prio 0, i/o): i8042-data 0000000080000061-0000000080000061 (prio 0, i/o): pcspk 0000000080000064-0000000080000064 (prio 0, i/o): i8042-cmd 0000000080000070-0000000080000071 (prio 0, i/o): rtc 0000000080000070-0000000080000070 (prio 0, i/o): rtc-index 0000000080000074-0000000080000077 (prio 0, i/o): m48t59 0000000080000081-0000000080000083 (prio 0, i/o): dma-page 0000000080000087-0000000080000087 (prio 0, i/o): dma-page ... address-space: lsi-pci-io 0000000080000000-00000000bf7fffff (prio 0, i/o): pci-io 0000000080000000-0000000080000007 (prio 0, i/o): dma-chan 0000000080000008-000000008000000f (prio 0, i/o): dma-cont 0000000080000020-0000000080000021 (prio 0, i/o): pic 0000000080000040-0000000080000043 (prio 0, i/o): pit 0000000080000060-0000000080000060 (prio 0, i/o): i8042-data 0000000080000061-0000000080000061 (prio 0, i/o): pcspk 0000000080000064-0000000080000064 (prio 0, i/o): i8042-cmd 0000000080000070-0000000080000071 (prio 0, i/o): rtc 0000000080000070-0000000080000070 (prio 0, i/o): rtc-index ... memory-region: pci-memory 00000000c0000000-00000000feffffff (prio 0, i/o): pci-memory 00000000c00a0000-00000000c00bffff (prio 1, i/o): vga-lowmem ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io() 2021-03-10 19:12 ` [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io() Philippe Mathieu-Daudé 2021-03-10 19:12 ` [PATCH 2/2] NOTFORMERGE memory: Ensure AddressSpace physical base address is zero Philippe Mathieu-Daudé 2021-03-10 19:49 ` [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io() Peter Xu @ 2021-03-11 0:48 ` Jiaxun Yang 2 siblings, 0 replies; 25+ messages in thread From: Jiaxun Yang @ 2021-03-11 0:48 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Peter Xu, BALATON Zoltan via Cc: Peter Maydell, Aleksandar Rikalo, Mark Cave-Ayland, Laurent Vivier, Alexander Bulekov, Hervé Poussineau, Paolo Bonzini, Edgar E . Iglesias, Aurelien Jarno On Thu, Mar 11, 2021, at 3:12 AM, Philippe Mathieu-Daudé wrote: > No need to create a local ISA I/O MemoryRegion, use get_system_io(). > > This partly reverts commit 5c63bcf7501527b844f61624957bdba254d75bfc. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> No luck to boot arcboot with current jazz :-( https://virtuallyfun.com/wordpress/2011/03/06/windows-nt-4-0-mips-revisited/ > --- > hw/mips/jazz.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > -- - Jiaxun ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/3] memory: Provide 'base address' argument to mtree_print_mr() 2021-03-10 19:09 ` Philippe Mathieu-Daudé 2021-03-10 19:12 ` [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io() Philippe Mathieu-Daudé @ 2021-03-10 19:31 ` Peter Xu 1 sibling, 0 replies; 25+ messages in thread From: Peter Xu @ 2021-03-10 19:31 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Peter Maydell, Mark Cave-Ayland, qemu-devel, Alexander Bulekov, Paolo Bonzini, Edgar E. Iglesias On Wed, Mar 10, 2021 at 08:09:59PM +0100, Philippe Mathieu-Daudé wrote: > On 3/10/21 6:06 PM, Peter Xu wrote: > > Phil, > > > > On Tue, Mar 09, 2021 at 10:54:18PM +0100, Philippe Mathieu-Daudé wrote: > >> +Peter/Mark/Edgar for SoC modelling > >> > >> On 3/9/21 10:39 AM, Philippe Mathieu-Daudé wrote: > >>> Hi Peter, > >>> > >>> On 3/9/21 12:40 AM, Peter Xu wrote:> On Sat, Mar 06, 2021 at 12:54:13AM > >>> +0100, Philippe Mathieu-Daudé wrote: > >>>>> @@ -3188,14 +3188,15 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled) > >>>>> > >>>>> QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) { > >>>>> qemu_printf("address-space: %s\n", as->name); > >>>>> - mtree_print_mr(as->root, 1, 0, &ml_head, owner, disabled); > >>>>> + mtree_print_mr(as->root, 1, 0, as->root->addr, > >>>> > >>>> Root MR of any address space should have mr->addr==0, right? > >>>> > >>>> I'm slightly confused on what this patch wanted to do if so, since then "base" > >>>> will always be zero.. Or am I wrong? > >>> > >>> That is what I am expecting too... Maybe the problem is elsewhere > >>> when I create the address space... The simpler way to > >>> figure it out is add an assertion. I haven't figure out my > >>> issue yet, I'll follow up later with a proof-of-concept series > >>> which triggers the assertion. > >> > >> To trigger I simply use: > >> > >> mydevice_realize() > >> { > >> memory_region_init(&mr, obj, name, size); > >> > >> address_space_init(&as, &mr, name); > > > > Could I ask why you need to set this sysbus mmio region as root MR of as? > > What's AS used for here? > > > > Btw, normally I see these regions should be initialized with > > memory_region_init_io(), since normally that MR will need a MemoryRegionOps > > bound to it to trap MMIOs, iiuc. > > This is the pattern for buses: > > static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, > const char *name, int devfn, > Error **errp) > { > ... > memory_region_init(&pci_dev->bus_master_container_region, > OBJECT(pci_dev), > "bus master container", UINT64_MAX); > address_space_init(&pci_dev->bus_master_as, > &pci_dev->bus_master_container_region, > pci_dev->name); > .... This one is the pci bus address space container as its name shows, IMHO it should never be added as subregion of another MR, but only the top parent. > > AUXBus *aux_bus_init(DeviceState *parent, const char *name) > { > AUXBus *bus; > Object *auxtoi2c; > > bus = AUX_BUS(qbus_create(TYPE_AUX_BUS, parent, name)); > auxtoi2c = object_new_with_props(TYPE_AUXTOI2C, OBJECT(bus), "i2c", > &error_abort, NULL); > > bus->bridge = AUXTOI2C(auxtoi2c); > > /* Memory related. */ > bus->aux_io = g_malloc(sizeof(*bus->aux_io)); > memory_region_init(bus->aux_io, OBJECT(bus), "aux-io", 1 * MiB); > address_space_init(&bus->aux_addr_space, bus->aux_io, "aux-io"); > return bus; > } This one seems similar - it's only used in aux_map_slave() as the parent MR, so its mr->addr should still always be zero. > > static void artist_realizefn(DeviceState *dev, Error **errp) > { > ... > memory_region_init(&s->mem_as_root, OBJECT(dev), "artist", ~0ull); > address_space_init(&s->as, &s->mem_as_root, "artist"); This one is similar with above - mem_as_root is the top MR. > > PCI host: > > PCIBus *gt64120_register(qemu_irq *pic) > { > ... > memory_region_init(&d->pci0_mem, OBJECT(dev), "pci0-mem", 4 * GiB); > address_space_init(&d->pci0_mem_as, &d->pci0_mem, "pci0-mem"); For this one, I even think pci0_mem_as can be removed since it's never referenced after initialized.. > > Also: > > static void pnv_xive_realize(DeviceState *dev, Error **errp) > { > ... > /* > * The XiveSource and XiveENDSource objects are realized with the > * maximum allowed HW configuration. The ESB MMIO regions will be > * resized dynamically when the controller is configured by the FW > * to limit accesses to resources not provisioned. > */ > memory_region_init(&xive->ipi_mmio, OBJECT(xive), "xive-vc-ipi", > PNV9_XIVE_VC_SIZE); > address_space_init(&xive->ipi_as, &xive->ipi_mmio, "xive-vc-ipi"); > memory_region_init(&xive->end_mmio, OBJECT(xive), "xive-vc-end", > PNV9_XIVE_VC_SIZE); > address_space_init(&xive->end_as, &xive->end_mmio, "xive-vc-end"); Similar here - both are top MRs. > > And: > > static void memory_map_init(void) > { > ... > memory_region_init(system_memory, NULL, "system", UINT64_MAX); > address_space_init(&address_space_memory, system_memory, "memory"); This is the system address space, system_memory should never be added as a subregion to other memory region, afaiu.. > > Or: > > static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, > int devfn) > { > ... > /* > * Memory region relationships looks like (Address range shows > * only lower 32 bits to make it short in length...): > * > * |-----------------+-------------------+----------| > * | Name | Address range | Priority | > * |-----------------+-------------------+----------+ > * | amdvi_root | 00000000-ffffffff | 0 | > * | amdvi_iommu | 00000000-ffffffff | 1 | > * | amdvi_iommu_ir | fee00000-feefffff | 64 | > * |-----------------+-------------------+----------| > */ > memory_region_init(&amdvi_dev_as->root, OBJECT(s), > "amdvi_root", UINT64_MAX); > address_space_init(&amdvi_dev_as->as, &amdvi_dev_as->root, name); > memory_region_init_io(&amdvi_dev_as->iommu_ir, OBJECT(s), > &amdvi_ir_ops, s, "amd_iommu_ir", > AMDVI_INT_ADDR_SIZE); > memory_region_add_subregion_overlap(&amdvi_dev_as->root, > AMDVI_INT_ADDR_FIRST, > &amdvi_dev_as->iommu_ir, > 64); > memory_region_add_subregion_overlap(&amdvi_dev_as->root, 0, > MEMORY_REGION(&amdvi_dev_as->iommu), 1); I think this is similar too. The thing is in your example you passed over the root MR to sysbus_init_mmio() where it would be further added into the system memory layout as sub-mr. Maybe you should create two MRs, one as root MR, the other one as the one passed into sysbus_init_mmio()? -- Peter Xu ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/3] memory: Provide 'base address' argument to mtree_print_mr() 2021-03-09 21:54 ` Philippe Mathieu-Daudé 2021-03-10 17:06 ` Peter Xu @ 2021-03-10 22:00 ` Mark Cave-Ayland 1 sibling, 0 replies; 25+ messages in thread From: Mark Cave-Ayland @ 2021-03-10 22:00 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Peter Xu, Peter Maydell, Edgar E. Iglesias Cc: Paolo Bonzini, qemu-devel, Alexander Bulekov On 09/03/2021 21:54, Philippe Mathieu-Daudé wrote: > +Peter/Mark/Edgar for SoC modelling > > On 3/9/21 10:39 AM, Philippe Mathieu-Daudé wrote: >> Hi Peter, >> >> On 3/9/21 12:40 AM, Peter Xu wrote:> On Sat, Mar 06, 2021 at 12:54:13AM >> +0100, Philippe Mathieu-Daudé wrote: >>>> @@ -3188,14 +3188,15 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled) >>>> >>>> QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) { >>>> qemu_printf("address-space: %s\n", as->name); >>>> - mtree_print_mr(as->root, 1, 0, &ml_head, owner, disabled); >>>> + mtree_print_mr(as->root, 1, 0, as->root->addr, >>> >>> Root MR of any address space should have mr->addr==0, right? >>> >>> I'm slightly confused on what this patch wanted to do if so, since then "base" >>> will always be zero.. Or am I wrong? >> >> That is what I am expecting too... Maybe the problem is elsewhere >> when I create the address space... The simpler way to >> figure it out is add an assertion. I haven't figure out my >> issue yet, I'll follow up later with a proof-of-concept series >> which triggers the assertion. > > To trigger I simply use: > > mydevice_realize() > { > memory_region_init(&mr, obj, name, size); > > address_space_init(&as, &mr, name); > // here we have as.root.addr = 0 > > sysbus_init_mmio(sbd, &mr); > } > > soc_realize() > { > ... > sysbus_realize(SYS_BUS_DEVICE(&mydevice), &error_abort); > sysbus_mmio_map(SYS_BUS_DEVICE(&mydevice), 0, NONZERO_ADDRESS); > ... > } > > sysbus_mmio_map > -> sysbus_mmio_map_common > -> memory_region_add_subregion > -> memory_region_add_subregion_common > > Which does: > > static void memory_region_add_subregion_common(MemoryRegion *mr, > hwaddr offset, > MemoryRegion *subregion) > { > assert(!subregion->container); > subregion->container = mr; > subregion->addr = offset; > // subregion = &as.root > // offset = NONZERO_ADDRESS > // so here as.root.addr becomes non-zero > > Is that use case incorrect? If so: > - where to add the proper assertions to avoid having address spaces > with non-zero base address? > - what is the proper design? > > Else what should we fix? I think I've seen something like this before with the sun4u IOMMU - I tried to get the offset of the IOMMU address space memory region from its parent in sun4u_translate_iommu() but I ended up getting back an absolute address instead. I think the conclusion I came to was that it wouldn't work with a memory region at the root of an address space, so I'd be interested to know what the behaviour here should be. In terms of soc_realize() I believe there are 2 options for getting the memory region: 1) use object_resolve_path_component() to retrieve the memory region as a QOM property or 2) use sysbus_mmio_get_region(). Once you have a reference to the memory region you can add it to the parent using memory_region_add_subregion() as needed. There is an existing example of 2) in hw/sparc64/sun4u.c in ebus_realize(). ATB, Mark. ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 3/3] memory: Make memory_region_to_absolute_addr() take a const MemoryRegion 2021-03-05 23:54 [PATCH 0/3] memory: Display AddressSpace zero-based in 'info mtree' Philippe Mathieu-Daudé 2021-03-05 23:54 ` [PATCH 1/3] memory: Better name 'offset' argument in mtree_print_mr() Philippe Mathieu-Daudé 2021-03-05 23:54 ` [PATCH 2/3] memory: Provide 'base address' argument to mtree_print_mr() Philippe Mathieu-Daudé @ 2021-03-05 23:54 ` Philippe Mathieu-Daudé 2021-03-11 12:19 ` Philippe Mathieu-Daudé 2 siblings, 1 reply; 25+ messages in thread From: Philippe Mathieu-Daudé @ 2021-03-05 23:54 UTC (permalink / raw) To: qemu-devel Cc: Paolo Bonzini, Philippe Mathieu-Daudé, Peter Xu, Alexander Bulekov There is no reason to not have memory_region_to_absolute_addr() work with a const MemoryRegion. Else we get: softmmu/memory.c: error: passing argument 1 of ‘memory_region_to_absolute_addr’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers] 6666 | myaddr = memory_region_to_absolute_addr(constmr, addr); | ^~ softmmu/memory.c:410:60: note: expected ‘MemoryRegion *’ but argument is of type ‘const MemoryRegion *’ 410 | static hwaddr memory_region_to_absolute_addr(MemoryRegion *mr, hwaddr offset) | ~~~~~~~~~~~~~~^~ Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- softmmu/memory.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/softmmu/memory.c b/softmmu/memory.c index 991d9227a88..6d1e96ba37d 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -407,9 +407,10 @@ static inline uint64_t memory_region_shift_write_access(uint64_t *value, return tmp; } -static hwaddr memory_region_to_absolute_addr(MemoryRegion *mr, hwaddr offset) +static hwaddr memory_region_to_absolute_addr(const MemoryRegion *mr, + hwaddr offset) { - MemoryRegion *root; + const MemoryRegion *root; hwaddr abs_addr = offset; abs_addr += mr->addr; -- 2.26.2 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 3/3] memory: Make memory_region_to_absolute_addr() take a const MemoryRegion 2021-03-05 23:54 ` [PATCH 3/3] memory: Make memory_region_to_absolute_addr() take a const MemoryRegion Philippe Mathieu-Daudé @ 2021-03-11 12:19 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 25+ messages in thread From: Philippe Mathieu-Daudé @ 2021-03-11 12:19 UTC (permalink / raw) To: qemu-devel, QEMU Trivial; +Cc: Paolo Bonzini, Peter Xu, Alexander Bulekov Cc'ing qemu-trivial@ for this single patch. On 3/6/21 12:54 AM, Philippe Mathieu-Daudé wrote: > There is no reason to not have memory_region_to_absolute_addr() > work with a const MemoryRegion. Else we get: > > softmmu/memory.c: error: passing argument 1 of ‘memory_region_to_absolute_addr’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers] > 6666 | myaddr = memory_region_to_absolute_addr(constmr, addr); > | ^~ > softmmu/memory.c:410:60: note: expected ‘MemoryRegion *’ but argument is of type ‘const MemoryRegion *’ > 410 | static hwaddr memory_region_to_absolute_addr(MemoryRegion *mr, hwaddr offset) > | ~~~~~~~~~~~~~~^~ > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > softmmu/memory.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/softmmu/memory.c b/softmmu/memory.c > index 991d9227a88..6d1e96ba37d 100644 > --- a/softmmu/memory.c > +++ b/softmmu/memory.c > @@ -407,9 +407,10 @@ static inline uint64_t memory_region_shift_write_access(uint64_t *value, > return tmp; > } > > -static hwaddr memory_region_to_absolute_addr(MemoryRegion *mr, hwaddr offset) > +static hwaddr memory_region_to_absolute_addr(const MemoryRegion *mr, > + hwaddr offset) > { > - MemoryRegion *root; > + const MemoryRegion *root; > hwaddr abs_addr = offset; > > abs_addr += mr->addr; > ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2021-03-12 9:19 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-05 23:54 [PATCH 0/3] memory: Display AddressSpace zero-based in 'info mtree' Philippe Mathieu-Daudé 2021-03-05 23:54 ` [PATCH 1/3] memory: Better name 'offset' argument in mtree_print_mr() Philippe Mathieu-Daudé 2021-03-05 23:54 ` [PATCH 2/3] memory: Provide 'base address' argument to mtree_print_mr() Philippe Mathieu-Daudé 2021-03-08 23:40 ` Peter Xu 2021-03-09 9:39 ` Philippe Mathieu-Daudé 2021-03-09 21:54 ` Philippe Mathieu-Daudé 2021-03-10 17:06 ` Peter Xu 2021-03-10 19:09 ` Philippe Mathieu-Daudé 2021-03-10 19:12 ` [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io() Philippe Mathieu-Daudé 2021-03-10 19:12 ` [PATCH 2/2] NOTFORMERGE memory: Ensure AddressSpace physical base address is zero Philippe Mathieu-Daudé 2021-03-10 19:49 ` [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io() Peter Xu 2021-03-10 20:11 ` Philippe Mathieu-Daudé 2021-03-10 20:29 ` Peter Xu 2021-03-11 12:18 ` Philippe Mathieu-Daudé 2021-03-11 16:21 ` Philippe Mathieu-Daudé 2021-03-11 17:27 ` Peter Xu 2021-03-11 17:59 ` Philippe Mathieu-Daudé 2021-03-12 9:08 ` Cédric Le Goater 2021-03-11 17:27 ` Peter Xu 2021-03-11 17:41 ` Philippe Mathieu-Daudé 2021-03-11 0:48 ` Jiaxun Yang 2021-03-10 19:31 ` [PATCH 2/3] memory: Provide 'base address' argument to mtree_print_mr() Peter Xu 2021-03-10 22:00 ` Mark Cave-Ayland 2021-03-05 23:54 ` [PATCH 3/3] memory: Make memory_region_to_absolute_addr() take a const MemoryRegion Philippe Mathieu-Daudé 2021-03-11 12:19 ` Philippe Mathieu-Daudé
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.