diff mbox series

[v2,01/11] hw/arm/aspeed: Do not directly map ram container onto main address bus

Message ID 20210417103028.601124-2-f4bug@amsat.org
State New
Headers show
Series memory: Forbid mapping AddressSpace root MemoryRegion | expand

Commit Message

Philippe Mathieu-Daudé April 17, 2021, 10:30 a.m. UTC 
The RAM container is exposed as an AddressSpace.
AddressSpaces root MemoryRegion must not be mapped into other
MemoryRegion, therefore map the RAM container using an alias.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/aspeed.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Peter Xu April 20, 2021, 6:28 p.m. UTC | #1 
On Sat, Apr 17, 2021 at 12:30:18PM +0200, Philippe Mathieu-Daudé wrote:
> The RAM container is exposed as an AddressSpace.

I didn't see where did ram_container got exposed as an address space.

I see it's added as one subregion of get_system_memory(), which looks okay?
Cédric Le Goater April 21, 2021, 5:53 a.m. UTC | #2 
On 4/20/21 8:28 PM, Peter Xu wrote:
> On Sat, Apr 17, 2021 at 12:30:18PM +0200, Philippe Mathieu-Daudé wrote:
>> The RAM container is exposed as an AddressSpace.
> 
> I didn't see where did ram_container got exposed as an address space.
> 
> I see it's added as one subregion of get_system_memory(), which looks okay? 
my version of this patch took a simpler approach. See below.

Thanks,

C.

--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -327,7 +327,7 @@ static void aspeed_machine_init(MachineState *machine)
     object_property_set_int(OBJECT(&bmc->soc), "num-cs", amc->num_cs,
                             &error_abort);
     object_property_set_link(OBJECT(&bmc->soc), "dram",
-                             OBJECT(&bmc->ram_container), &error_abort);
+                             OBJECT(machine->ram), &error_abort);
     if (machine->kernel_filename) {
         /*
          * When booting with a -kernel command line there is no u-boot
Philippe Mathieu-Daudé April 21, 2021, 1:02 p.m. UTC | #3 
On 4/21/21 7:53 AM, Cédric Le Goater wrote:
> On 4/20/21 8:28 PM, Peter Xu wrote:
>> On Sat, Apr 17, 2021 at 12:30:18PM +0200, Philippe Mathieu-Daudé wrote:
>>> The RAM container is exposed as an AddressSpace.
>>
>> I didn't see where did ram_container got exposed as an address space.

I guess I used the wrong base to git-publish and skipped the first patch =)

>> I see it's added as one subregion of get_system_memory(), which looks okay? 
> my version of this patch took a simpler approach. See below.
> 
> Thanks,
> 
> C.
> 
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -327,7 +327,7 @@ static void aspeed_machine_init(MachineState *machine)
>      object_property_set_int(OBJECT(&bmc->soc), "num-cs", amc->num_cs,
>                              &error_abort);
>      object_property_set_link(OBJECT(&bmc->soc), "dram",
> -                             OBJECT(&bmc->ram_container), &error_abort);
> +                             OBJECT(machine->ram), &error_abort);

This will work as long as no board maps the main memory elsewhere than
0x0. Using the alias make it more robust (and also is good API example
for the usual "use API via copy/pasting" style when adding new board)
IMHO.

>      if (machine->kernel_filename) {
>          /*
>           * When booting with a -kernel command line there is no u-boot
> 
>
diff mbox series

Patch

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index a17b75f4940..daeef5b32a2 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -40,6 +40,7 @@  struct AspeedMachineState {
 
     AspeedSoCState soc;
     MemoryRegion ram_container;
+    MemoryRegion ram_container_alias;
     MemoryRegion max_ram;
     bool mmio_exec;
     char *fmc_model;
@@ -339,9 +340,12 @@  static void aspeed_machine_init(MachineState *machine)
     }
     qdev_realize(DEVICE(&bmc->soc), NULL, &error_abort);
 
+    memory_region_init_alias(&bmc->ram_container_alias, NULL,
+                             "ram-container-alias", &bmc->ram_container, 0,
+                             memory_region_size(&bmc->ram_container));
     memory_region_add_subregion(get_system_memory(),
                                 sc->memmap[ASPEED_DEV_SDRAM],
-                                &bmc->ram_container);
+                                &bmc->ram_container_alias);
 
     max_ram_size = object_property_get_uint(OBJECT(&bmc->soc), "max-ram-size",
                                             &error_abort);