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 <imamm...@redhat.com>
> ---
> CC: c...@kaod.org
> CC: peter.mayd...@linaro.org
> CC: and...@aj.id.au
> CC: j...@jms.id.au
> CC: qemu-...@nongnu.org
> ---
> 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,
>
