Re: [PATCH v2 08/86] arm:aspeed: actually check RAM size

[Cédric Le Goater]

On 1/15/20 4:06 PM, Igor Mammedov wrote:
> It's supposed that SOC will check if "-m" provided
> RAM size is valid by setting "ram-size" property and
> then board would read back valid (possibly corrected
> value) to map RAM MemoryReging with valid size.
> Well it isn't doing so, since check is called only
> indirectly from
>   aspeed_sdmc_reset()->asc->compute_conf()
> or much later when guest writes to configuration
> register.
> 
> So depending on "-m" value QEMU end-ups with a warning
> and an invalid MemoryRegion size allocated and mapped.
> (examples:
>  -M ast2500-evb -m 1M
>     0000000080000000-000000017ffffffe (prio 0, i/o): aspeed-ram-container
>       0000000080000000-00000000800fffff (prio 0, ram): ram
>       0000000080100000-00000000bfffffff (prio 0, i/o): max_ram
>  -M ast2500-evb -m 3G
>     0000000080000000-000000017ffffffe (prio 0, i/o): aspeed-ram-container
>       0000000080000000-000000013fffffff (prio 0, ram): ram
>       [DETECTED OVERFLOW!] 0000000140000000-00000000bfffffff (prio 0, i/o): 
> max_ram
> )
> On top of that sdmc falls back and reports to guest
> "default" size, it thinks machine should have.>
> I don't know how hardware is supposed to work so
> I've kept it as is.

The HW is hardwired and the modeling is trying to accommodate with
the '-m' option, the machine definition and the SDRAM controller
limits and register definitions for a given SoC. The result is not 
that good it seems :/ 

> But as for CLI side machine should honor whatever
> user configured or error out to make user fix CLI.
> 
> This patch makes ram-size check actually work and
> changes behavior from a warning later on during
> machine reset to error_fatal at the moment SOC is
> realized so user will have to fix RAM size on CLI
> to start machine.

commit 8e00d1a97d1d ("aspeed/sdmc: Introduce an object class per SoC") 
moved some calls from the realize handler to reset handler and it
broke the checks on the RAM size.

I think we should introduce get/set handlers on the "ram-size" property
that would look for a matching size in an AspeedSDMCClass array of valid
RAM sizes. The default size of the machine would be a valid default and
bogus user defined sizes would be fatal to QEMU.  

We could get rid of the code :

    /* use a common default */
    warn_report("Invalid RAM size 0x%" PRIx64 ". Using default 512M",
                s->ram_size);
    s->ram_size = 512 << 20;
    return ASPEED_SDMC_AST2500_512MB;


'max_ram_size' would be the last entry of the AspeedSDMCClass array
and, anyhow, we need to check bmc->max_ram is really needed. I am not 
sure anymore. 

Thanks,

C. 

> It also gets out of the way mutable ram-size logic,
> so we could consolidate RAM allocation logic around
> pre-allocated hostmem backend (supplied by user or
> auto created by generic machine code depending on
> supplied -m/mem-path/mem-prealloc options.
> 
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
> CC: address@hidden
> CC: address@hidden
> CC: address@hidden
> CC: address@hidden
> CC: address@hidden
> ---
>  hw/arm/aspeed.c       | 9 +--------
>  hw/misc/aspeed_sdmc.c | 5 +++++
>  2 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index cc06af4..525c547 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -213,14 +213,7 @@ static void aspeed_machine_init(MachineState *machine)
>                                  "hw-prot-key", &error_abort);
>      }
>      object_property_set_bool(OBJECT(&bmc->soc), true, "realized",
> -                             &error_abort);
> -
> -    /*
> -     * Allocate RAM after the memory controller has checked the size
> -     * was valid. If not, a default value is used.
> -     */
> -    ram_size = object_property_get_uint(OBJECT(&bmc->soc), "ram-size",
> -                                        &error_abort);
> +                             &error_fatal);
>  
>      memory_region_allocate_system_memory(&bmc->ram, NULL, "ram", ram_size);
>      memory_region_add_subregion(&bmc->ram_container, 0, &bmc->ram);
> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
> index 3fc80f0..b398e36 100644
> --- a/hw/misc/aspeed_sdmc.c
> +++ b/hw/misc/aspeed_sdmc.c
> @@ -165,6 +165,11 @@ static void aspeed_sdmc_realize(DeviceState *dev, Error 
> **errp)
>      AspeedSDMCState *s = ASPEED_SDMC(dev);
>      AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s);
>  
> +    if (!g_hash_table_contains(asc->ram2feat,
> +                               GINT_TO_POINTER(s->ram_size >> 20))) {
> +        error_setg(errp, "Invalid RAM size 0x%" PRIx64, s->ram_size);
> +        return;
> +    }
>      s->max_ram_size = asc->max_ram_size;
>  
>      memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_sdmc_ops, s,
>