On 7/20/20 11:58 AM, Philippe Mathieu-Daudé wrote:
> The SDRAM Memory Controller has a 32-bit address bus, thus
> supports up to 4 GiB of DRAM. There is a signed to unsigned
> conversion error with the AST2600 maximum memory size:
>
> (uint64_t)(2048 << 20) = (uint64_t)(-2147483648)
> = 0xffffffff40000000
> = 16 EiB - 2 GiB
>
> Fix by using the IEC suffixes which are usually safer, and add
> a check to verify the memory is valid. This would have catched
> this bug:
>
> Unexpected error in aspeed_sdmc_realize() at hw/misc/aspeed_sdmc.c:261:
> qemu-system-arm: Invalid RAM size 16 EiB
Indeed :/
>
> Fixes: 1550d72679 ("aspeed/sdmc: Add AST2600 support")
> Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org>
> ---
> hw/misc/aspeed_sdmc.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
> index 0737d8de81..76dd7e6a20 100644
> --- a/hw/misc/aspeed_sdmc.c
> +++ b/hw/misc/aspeed_sdmc.c
> @@ -256,6 +256,12 @@ static void aspeed_sdmc_realize(DeviceState *dev, Error
> **errp)
> AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s);
>
> s->max_ram_size = asc->max_ram_size;
> + if (s->max_ram_size >= 4 * GiB) {
> + char *szstr = size_to_str(s->max_ram_size);
> + error_setg(errp, "Invalid RAM size %s", szstr);
> + g_free(szstr);
> + return;
> + }
>
I would put an assert() since the max RAM size is not user configurable.
C.
> memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_sdmc_ops, s,
> TYPE_ASPEED_SDMC, 0x1000);
> @@ -341,7 +347,7 @@ static void aspeed_2400_sdmc_class_init(ObjectClass
> *klass, void *data)
> AspeedSDMCClass *asc = ASPEED_SDMC_CLASS(klass);
>
> dc->desc = "ASPEED 2400 SDRAM Memory Controller";
> - asc->max_ram_size = 512 << 20;
> + asc->max_ram_size = 512 * MiB;
> asc->compute_conf = aspeed_2400_sdmc_compute_conf;
> asc->write = aspeed_2400_sdmc_write;
> asc->valid_ram_sizes = aspeed_2400_ram_sizes;
> @@ -408,7 +414,7 @@ static void aspeed_2500_sdmc_class_init(ObjectClass
> *klass, void *data)
> AspeedSDMCClass *asc = ASPEED_SDMC_CLASS(klass);
>
> dc->desc = "ASPEED 2500 SDRAM Memory Controller";
> - asc->max_ram_size = 1024 << 20;
> + asc->max_ram_size = 1 * GiB;
> asc->compute_conf = aspeed_2500_sdmc_compute_conf;
> asc->write = aspeed_2500_sdmc_write;
> asc->valid_ram_sizes = aspeed_2500_ram_sizes;
> @@ -485,7 +491,7 @@ static void aspeed_2600_sdmc_class_init(ObjectClass
> *klass, void *data)
> AspeedSDMCClass *asc = ASPEED_SDMC_CLASS(klass);
>
> dc->desc = "ASPEED 2600 SDRAM Memory Controller";
> - asc->max_ram_size = 2048 << 20;
> + asc->max_ram_size = 2 * GiB;
> asc->compute_conf = aspeed_2600_sdmc_compute_conf;
> asc->write = aspeed_2600_sdmc_write;
> asc->valid_ram_sizes = aspeed_2600_ram_sizes;
>
