[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 07/84] hw/arm/aspeed: actually check RAM size
From: |
Cédric Le Goater |
Subject: |
Re: [PATCH v3 07/84] hw/arm/aspeed: actually check RAM size |
Date: |
Mon, 20 Jan 2020 16:33:46 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 |
On 1/20/20 3:21 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.
> 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.
>
> 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.ram-size
> is set 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 <address@hidden>
LGTM, some comments below.
> ---
> v3:
> * replace
> [PATCH v2 07/86] arm:aspeed: convert valid RAM sizes to data
> [PATCH v2 08/86] arm:aspeed: actually check RAM size
> with a simplified variant that adds ram_size check to sdmc.ram-size
> property
>
> CC: address@hidden
> CC: address@hidden
> CC: address@hidden
> CC: address@hidden
> CC: address@hidden
>
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
> include/hw/misc/aspeed_sdmc.h | 1 +
> hw/arm/aspeed.c | 13 +++-----
> hw/misc/aspeed_sdmc.c | 77
> +++++++++++++++++++++++++++++++++++--------
> 3 files changed, 70 insertions(+), 21 deletions(-)
>
> diff --git a/include/hw/misc/aspeed_sdmc.h b/include/hw/misc/aspeed_sdmc.h
> index 5dbde59..cea1e67 100644
> --- a/include/hw/misc/aspeed_sdmc.h
> +++ b/include/hw/misc/aspeed_sdmc.h
> @@ -40,6 +40,7 @@ typedef struct AspeedSDMCClass {
> SysBusDeviceClass parent_class;
>
> uint64_t max_ram_size;
> + const uint64_t *valid_ram_sizes;
> uint32_t (*compute_conf)(AspeedSDMCState *s, uint32_t data);
> void (*write)(AspeedSDMCState *s, uint32_t reg, uint32_t data);
> } AspeedSDMCClass;
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index cc06af4..c8573e5 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -191,8 +191,12 @@ static void aspeed_machine_init(MachineState *machine)
>
> sc = ASPEED_SOC_GET_CLASS(&bmc->soc);
>
> + /*
> + * This will error out if isize is not supported by memory controller.
> + */
> object_property_set_uint(OBJECT(&bmc->soc), ram_size, "ram-size",
> - &error_abort);
> + &error_fatal);
> +
> object_property_set_int(OBJECT(&bmc->soc), amc->hw_strap1, "hw-strap1",
> &error_abort);
> object_property_set_int(OBJECT(&bmc->soc), amc->hw_strap2, "hw-strap2",
> @@ -215,13 +219,6 @@ static void aspeed_machine_init(MachineState *machine)
> 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);
> -
> memory_region_allocate_system_memory(&bmc->ram, NULL, "ram", ram_size);
> memory_region_add_subregion(&bmc->ram_container, 0, &bmc->ram);
> memory_region_add_subregion(get_system_memory(),
> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
> index 2df3244..b36b362 100644
> --- a/hw/misc/aspeed_sdmc.c
> +++ b/hw/misc/aspeed_sdmc.c
> @@ -17,6 +17,9 @@
> #include "migration/vmstate.h"
> #include "qapi/error.h"
> #include "trace.h"
> +#include "qemu/units.h"
> +#include "qemu/cutils.h"
> +#include "qapi/visitor.h"
>
> /* Protection Key Register */
> #define R_PROT (0x00 / 4)
> @@ -163,10 +166,7 @@ static int ast2400_rambits(AspeedSDMCState *s)
> break;
> }
Now that the ramsizes are being checked, I think we can add in the default
statement :
g_assert_not_reached();
>
> - /* use a common default */
> - warn_report("Invalid RAM size 0x%" PRIx64 ". Using default 256M",
> - s->ram_size);
> - s->ram_size = 256 << 20;
> + assert(0);
> return ASPEED_SDMC_DRAM_256MB;
and skip the default return value.
A part from that,
Reviewed-by: Cédric Le Goater <address@hidden>
Thanks,
C.
> }
>
> @@ -185,10 +185,7 @@ static int ast2500_rambits(AspeedSDMCState *s)
> break;
> }
>
> - /* use a common default */
> - warn_report("Invalid RAM size 0x%" PRIx64 ". Using default 512M",
> - s->ram_size);
> - s->ram_size = 512 << 20;
> + assert(0);
> return ASPEED_SDMC_AST2500_512MB;
> }
>
> @@ -207,10 +204,7 @@ static int ast2600_rambits(AspeedSDMCState *s)
> break;
> }
>
> - /* use a common default */
> - warn_report("Invalid RAM size 0x%" PRIx64 ". Using default 1024M",
> - s->ram_size);
> - s->ram_size = 1024 << 20;
> + assert(0);
> return ASPEED_SDMC_AST2600_1024MB;
> }
>
> @@ -225,6 +219,51 @@ static void aspeed_sdmc_reset(DeviceState *dev)
> s->regs[R_CONF] = asc->compute_conf(s, 0);
> }
>
> +static void aspeed_sdmc_get_ram_size(Object *obj, Visitor *v, const char
> *name,
> + void *opaque, Error **errp)
> +{
> + AspeedSDMCState *s = ASPEED_SDMC(obj);
> + int64_t value = s->ram_size;
> +
> + visit_type_int(v, name, &value, errp);
> +}
> +
> +static void aspeed_sdmc_set_ram_size(Object *obj, Visitor *v, const char
> *name,
> + void *opaque, Error **errp)
> +{
> + int i;
> + char *sz;
> + int64_t value;
> + Error *local_err = NULL;
> + AspeedSDMCState *s = ASPEED_SDMC(obj);
> + AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s);
> +
> + visit_type_int(v, name, &value, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
> +
> + for (i = 0; asc->valid_ram_sizes[i]; i++) {
> + if (value == asc->valid_ram_sizes[i]) {
> + s->ram_size = value;
> + return;
> + }
> + }
> +
> + sz = size_to_str(value);
> + error_setg(&local_err, "Invalid RAM size %s", sz);
> + g_free(sz);
> + error_propagate(errp, local_err);
> +}
> +
> +static void aspeed_sdmc_initfn(Object *obj)
> +{
> + object_property_add(obj, "ram-size", "int",
> + aspeed_sdmc_get_ram_size, aspeed_sdmc_set_ram_size,
> + NULL, NULL, NULL);
> +}
> +
> static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)
> {
> SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> @@ -249,7 +288,6 @@ static const VMStateDescription vmstate_aspeed_sdmc = {
> };
>
> static Property aspeed_sdmc_properties[] = {
> - DEFINE_PROP_UINT64("ram-size", AspeedSDMCState, ram_size, 0),
> DEFINE_PROP_UINT64("max-ram-size", AspeedSDMCState, max_ram_size, 0),
> DEFINE_PROP_END_OF_LIST(),
> };
> @@ -268,6 +306,7 @@ static const TypeInfo aspeed_sdmc_info = {
> .name = TYPE_ASPEED_SDMC,
> .parent = TYPE_SYS_BUS_DEVICE,
> .instance_size = sizeof(AspeedSDMCState),
> + .instance_init = aspeed_sdmc_initfn,
> .class_init = aspeed_sdmc_class_init,
> .class_size = sizeof(AspeedSDMCClass),
> .abstract = true,
> @@ -298,6 +337,9 @@ static void aspeed_2400_sdmc_write(AspeedSDMCState *s,
> uint32_t reg,
> s->regs[reg] = data;
> }
>
> +static const uint64_t
> +aspeed_2400_ram_sizes[] = { 64 * MiB, 128 * MiB, 256 * MiB, 512 * MiB, 0};
> +
> static void aspeed_2400_sdmc_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -307,6 +349,7 @@ static void aspeed_2400_sdmc_class_init(ObjectClass
> *klass, void *data)
> asc->max_ram_size = 512 << 20;
> asc->compute_conf = aspeed_2400_sdmc_compute_conf;
> asc->write = aspeed_2400_sdmc_write;
> + asc->valid_ram_sizes = aspeed_2400_ram_sizes;
> }
>
> static const TypeInfo aspeed_2400_sdmc_info = {
> @@ -351,6 +394,9 @@ static void aspeed_2500_sdmc_write(AspeedSDMCState *s,
> uint32_t reg,
> s->regs[reg] = data;
> }
>
> +static const uint64_t
> +aspeed_2500_ram_sizes[] = { 128 * MiB, 256 * MiB, 512 * MiB, 1024 * MiB, 0};
> +
> static void aspeed_2500_sdmc_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -360,6 +406,7 @@ static void aspeed_2500_sdmc_class_init(ObjectClass
> *klass, void *data)
> asc->max_ram_size = 1024 << 20;
> asc->compute_conf = aspeed_2500_sdmc_compute_conf;
> asc->write = aspeed_2500_sdmc_write;
> + asc->valid_ram_sizes = aspeed_2500_ram_sizes;
> }
>
> static const TypeInfo aspeed_2500_sdmc_info = {
> @@ -404,6 +451,9 @@ static void aspeed_2600_sdmc_write(AspeedSDMCState *s,
> uint32_t reg,
> s->regs[reg] = data;
> }
>
> +static const uint64_t
> +aspeed_2600_ram_sizes[] = { 256 * MiB, 512 * MiB, 1024 * MiB, 2048 * MiB, 0};
> +
> static void aspeed_2600_sdmc_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -413,6 +463,7 @@ static void aspeed_2600_sdmc_class_init(ObjectClass
> *klass, void *data)
> asc->max_ram_size = 2048 << 20;
> asc->compute_conf = aspeed_2600_sdmc_compute_conf;
> asc->write = aspeed_2600_sdmc_write;
> + asc->valid_ram_sizes = aspeed_2600_ram_sizes;
> }
>
> static const TypeInfo aspeed_2600_sdmc_info = {
>
- Re: [PATCH v2 03/86] machine: alias -mem-path and -mem-prealloc into memory-foo backend, (continued)
[PATCH v2 09/86] hw:aspeed: drop warning and bogus ram_size fixup, Igor Mammedov, 2020/01/15
[PATCH v2 07/86] arm:aspeed: convert valid RAM sizes to data, Igor Mammedov, 2020/01/15
[PATCH v2 10/86] arm:aspeed: use memdev for RAM, Igor Mammedov, 2020/01/15
[PATCH v2 11/86] arm:collie: use memdev for RAM, Igor Mammedov, 2020/01/15
[PATCH v2 12/86] arm:cubieboard: use memdev for RAM, Igor Mammedov, 2020/01/15
[PATCH v2 13/86] arm:digic_boards: use memdev for RAM, Igor Mammedov, 2020/01/15