[v2,08/86] arm:aspeed: actually check RAM size
diff mbox series

Message ID 1579100861-73692-9-git-send-email-imammedo@redhat.com
State New
Headers show
Series
  • refactor main RAM allocation to use hostmem backend
Related show

Commit Message

Igor Mammedov Jan. 15, 2020, 3:06 p.m. UTC
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.
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.

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 <imammedo@redhat.com>
---
CC: clg@kaod.org
CC: peter.maydell@linaro.org
CC: andrew@aj.id.au
CC: joel@jms.id.au
CC: qemu-arm@nongnu.org
---
 hw/arm/aspeed.c       | 9 +--------
 hw/misc/aspeed_sdmc.c | 5 +++++
 2 files changed, 6 insertions(+), 8 deletions(-)

Comments

Cédric Le Goater Jan. 16, 2020, 8:41 a.m. UTC | #1
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.  

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. 

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 <imammedo@redhat.com>
> ---
> CC: clg@kaod.org
> CC: peter.maydell@linaro.org
> CC: andrew@aj.id.au
> CC: joel@jms.id.au
> CC: qemu-arm@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,
>
Igor Mammedov Jan. 16, 2020, 5:35 p.m. UTC | #2
On Thu, 16 Jan 2020 09:41:03 +0100
Cédric Le Goater <clg@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. 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.

> 
> 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 <imammedo@redhat.com>
> > ---
> > CC: clg@kaod.org
> > CC: peter.maydell@linaro.org
> > CC: andrew@aj.id.au
> > CC: joel@jms.id.au
> > CC: qemu-arm@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,
> >   
> 
>
Cédric Le Goater Jan. 17, 2020, 7:56 a.m. UTC | #3
On 1/16/20 6:35 PM, Igor Mammedov wrote:
> On Thu, 16 Jan 2020 09:41:03 +0100
> Cédric Le Goater <clg@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 <imammedo@redhat.com>
>>> ---
>>> CC: clg@kaod.org
>>> CC: peter.maydell@linaro.org
>>> CC: andrew@aj.id.au
>>> CC: joel@jms.id.au
>>> CC: qemu-arm@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,
>>>   
>>
>>
>

Patch
diff mbox series

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,