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

[Philippe Mathieu-Daudé]

On 7/20/20 6:07 PM, Cédric Le Goater wrote:
> 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é <f4bug@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. 

As you wish, at this point I'm completely lost with error reporting.
Per the manual
(https://www.mail-archive.com/qemu-devel@nongnu.org/msg723217.html):

 "Many, many devices neglect to clean up properly on error, and get away
  with it only because all callers treat errors as fatal.

  If you decide to take cleanup shortcuts, say because the cleanup is
  untestable, consider adding a comment at least."

So I'll go for address + comment:

  assert(s->max_ram_size < 4 * GiB); /* 32-bit address bus */

> 
> 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;
>>
> 
>