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

[Cédric Le Goater]

On 1/16/20 6:35 PM, Igor Mammedov wrote:
> On Thu, 16 Jan 2020 09:41:03 +0100
> Cédric Le Goater <address@hidden> wrote:
> 
>> 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.  
> 
> That's what I'm after, i.e. board either accepts user asked size or exits
> with error. 

We should be able to catch bogus values with :

    object_property_set_uint(OBJECT(&bmc->soc), ram_size, "ram-size",
                             &error_abort);

in aspeed_machine_init() and let QEMU exit.

> So as far as there aren't any fix-ups it should work for
> the purpose of this series
>
>>
>> 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. 
> 
> I'll rework aspeed parts taking in account feedback.

OK. We will give them a try. I don't think you have to bother with 
bmc->max_ram for the moment. It doesn't seem to be in your way. 

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