[v2] hw/misc/aspeed_sdmc: Fix incorrect memory size

[PATCH-for-5.1 v2] hw/misc/aspeed_sdmc: Fix incorrect memory size

Posted by Philippe Mathieu-Daudé 5 years, 3 months ago

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
an assertion check to verify the memory is valid. This would have
catched this bug:

  $ qemu-system-arm -M ast2600-evb
  qemu-system-arm: hw/misc/aspeed_sdmc.c:258: aspeed_sdmc_realize: Assertion `asc->max_ram_size < 4 * GiB' failed.
  Aborted (core dumped)

Fixes: 1550d72679 ("aspeed/sdmc: Add AST2600 support")
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
v2: Assert (Cédric)
---
 hw/misc/aspeed_sdmc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
index 0737d8de81..855848b7d2 100644
--- a/hw/misc/aspeed_sdmc.c
+++ b/hw/misc/aspeed_sdmc.c
@@ -255,6 +255,7 @@ static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)
     AspeedSDMCState *s = ASPEED_SDMC(dev);
     AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s);
 
+    assert(asc->max_ram_size < 4 * GiB); /* 32-bit address bus */
     s->max_ram_size = asc->max_ram_size;
 
     memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_sdmc_ops, s,
@@ -341,7 +342,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 +409,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 +486,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;
-- 
2.21.3

Re: [PATCH-for-5.1 v2] hw/misc/aspeed_sdmc: Fix incorrect memory size

Posted by Cédric Le Goater 5 years, 3 months ago

On 7/20/20 7:49 PM, 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
> an assertion check to verify the memory is valid. This would have
> catched this bug:
> 
>   $ qemu-system-arm -M ast2600-evb
>   qemu-system-arm: hw/misc/aspeed_sdmc.c:258: aspeed_sdmc_realize: Assertion `asc->max_ram_size < 4 * GiB' failed.
>   Aborted (core dumped)
> 
> Fixes: 1550d72679 ("aspeed/sdmc: Add AST2600 support")
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
> v2: Assert (Cédric)
> ---
>  hw/misc/aspeed_sdmc.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
> index 0737d8de81..855848b7d2 100644
> --- a/hw/misc/aspeed_sdmc.c
> +++ b/hw/misc/aspeed_sdmc.c
> @@ -255,6 +255,7 @@ static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)
>      AspeedSDMCState *s = ASPEED_SDMC(dev);
>      AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s);
>  
> +    assert(asc->max_ram_size < 4 * GiB); /* 32-bit address bus */
>      s->max_ram_size = asc->max_ram_size;
>  
>      memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_sdmc_ops, s,
> @@ -341,7 +342,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 +409,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 +486,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;
>