Re: [Qemu-arm] [PATCH 6/7] aspeed: add a max_ram_size property to the memory controller

[Peter Maydell]

On 7 August 2018 at 08:57, Joel Stanley <address@hidden> wrote:
> From: Cédric Le Goater <address@hidden>
>
> This will be used to construct a memory region beyond the RAM region
> to let firmwares scan the address space with load/store to guess how
> much RAM the SoC has.
>
> Signed-off-by: Cédric Le Goater <address@hidden>
> Signed-off-by: Joel Stanley <address@hidden>
> ---
>  hw/arm/aspeed.c               | 31 +++++++++++++++++++++++++++++++
>  hw/arm/aspeed_soc.c           |  2 ++
>  hw/misc/aspeed_sdmc.c         |  3 +++
>  include/hw/misc/aspeed_sdmc.h |  1 +
>  4 files changed, 37 insertions(+)
>
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index bb9d33848d3f..e078269266bc 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -31,6 +31,7 @@ static struct arm_boot_info aspeed_board_binfo = {
>  typedef struct AspeedBoardState {
>      AspeedSoCState soc;
>      MemoryRegion ram;
> +    MemoryRegion max_ram;
>  } AspeedBoardState;
>
>  typedef struct AspeedBoardConfig {
> @@ -127,6 +128,27 @@ static const AspeedBoardConfig aspeed_boards[] = {
>      },
>  };
>
> +/*
> + * The max ram region is for firmwares that scan the address space
> + * with load/store to guess how much RAM the SoC has.
> + */
> +static uint64_t max_ram_read(void *opaque, hwaddr offset, unsigned size)
> +{
> +    return 0;
> +}
> +
> +static void max_ram_write(void *opaque, hwaddr offset, uint64_t value,
> +                           unsigned size)
> +{
> +    /* Disacard writes */
> +}
> +
> +static const MemoryRegionOps max_ram_ops = {
> +    .read = max_ram_read,
> +    .write = max_ram_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
>  #define FIRMWARE_ADDR 0x0
>
>  static void write_boot_rom(DriveInfo *dinfo, hwaddr addr, size_t rom_size,
> @@ -187,6 +209,7 @@ static void aspeed_board_init(MachineState *machine,
>      AspeedBoardState *bmc;
>      AspeedSoCClass *sc;
>      DriveInfo *drive0 = drive_get(IF_MTD, 0, 0);
> +    ram_addr_t max_ram_size;
>
>      bmc = g_new0(AspeedBoardState, 1);
>      object_initialize(&bmc->soc, (sizeof(bmc->soc)), cfg->soc_name);
> @@ -226,6 +249,14 @@ static void aspeed_board_init(MachineState *machine,
>      object_property_add_const_link(OBJECT(&bmc->soc), "ram", 
> OBJECT(&bmc->ram),
>                                     &error_abort);
>
> +    max_ram_size = object_property_get_uint(OBJECT(&bmc->soc), 
> "max-ram-size",
> +                                            &error_abort);
> +    memory_region_init_io(&bmc->max_ram, NULL, &max_ram_ops, NULL,
> +                          "max_ram", max_ram_size  - ram_size);
> +    memory_region_add_subregion(get_system_memory(),
> +                                sc->info->sdram_base + ram_size,
> +                                &bmc->max_ram);

I'm surprised that you need the IO ops, ie that it doesn't work
just to define an empty container region with memory_region_init().

thanks
-- PMM