On 1/16/20 6:35 PM, Igor Mammedov wrote:
> On Thu, 16 Jan 2020 09:41:03 +0100
> Cédric Le Goater <c...@kaod.org> 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 <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,
>>>
>>
>>
>
