diff mbox series

[v3] hw/riscv: microchip_pfsoc: Correct DDR memory map

Message ID 20201101163753.15059-1-bmeng.cn@gmail.com
State New
Headers show
Series [v3] hw/riscv: microchip_pfsoc: Correct DDR memory map | expand

Commit Message

Bin Meng Nov. 1, 2020, 4:37 p.m. UTC
From: Bin Meng <bin.meng@windriver.com>

When system memory is larger than 1 GiB (high memory), PolarFire SoC
maps it at address 0x10_0000_0000. Address 0xC000_0000 and above is
aliased to the same 1 GiB low memory with different cache attributes.

At present QEMU maps the system memory contiguously from 0x8000_0000.
This corrects the wrong QEMU logic. Note address 0x14_0000_0000 is
the alias to the high memory, and even physical memory is only 1 GiB,
the HSS codes still tries to probe the high memory alias address.
It seems there is no issue on the real hardware, so we will have to
take that into the consideration in our emulation. Due to this, we
we increase the default system memory size to 2047 MiB (the largest
ram size allowed when running on a 32-bit host) so that user gets
notified an error when less than 2047 MiB is specified.

Signed-off-by: Bin Meng <bin.meng@windriver.com>

---
This patch should replace the following commit in Alistair's
riscv-to-apply.next tree:
https://github.com/alistair23/qemu/commit/8c47c1e9df850a928b4b230240a950feabe6152f

Changes in v3:
- Change default ram size to 2047 MiB for 32-bit host

 hw/riscv/microchip_pfsoc.c         | 48 ++++++++++++++++++++++++++----
 include/hw/riscv/microchip_pfsoc.h |  5 +++-
 2 files changed, 46 insertions(+), 7 deletions(-)

Comments

Alistair Francis Nov. 1, 2020, 4:46 p.m. UTC | #1
On Sun, Nov 1, 2020 at 8:42 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>

> From: Bin Meng <bin.meng@windriver.com>

>

> When system memory is larger than 1 GiB (high memory), PolarFire SoC

> maps it at address 0x10_0000_0000. Address 0xC000_0000 and above is

> aliased to the same 1 GiB low memory with different cache attributes.

>

> At present QEMU maps the system memory contiguously from 0x8000_0000.

> This corrects the wrong QEMU logic. Note address 0x14_0000_0000 is

> the alias to the high memory, and even physical memory is only 1 GiB,

> the HSS codes still tries to probe the high memory alias address.

> It seems there is no issue on the real hardware, so we will have to

> take that into the consideration in our emulation. Due to this, we

> we increase the default system memory size to 2047 MiB (the largest

> ram size allowed when running on a 32-bit host) so that user gets

> notified an error when less than 2047 MiB is specified.


Is this better than just not supporting 32-bit hosts? Or could we make
this number even lower (as low as possible that still works with HSS)?

Alistair

>

> Signed-off-by: Bin Meng <bin.meng@windriver.com>

>

> ---

> This patch should replace the following commit in Alistair's

> riscv-to-apply.next tree:

> https://github.com/alistair23/qemu/commit/8c47c1e9df850a928b4b230240a950feabe6152f

>

> Changes in v3:

> - Change default ram size to 2047 MiB for 32-bit host

>

>  hw/riscv/microchip_pfsoc.c         | 48 ++++++++++++++++++++++++++----

>  include/hw/riscv/microchip_pfsoc.h |  5 +++-

>  2 files changed, 46 insertions(+), 7 deletions(-)

>

> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c

> index 44a84732ac..0bc17b3955 100644

> --- a/hw/riscv/microchip_pfsoc.c

> +++ b/hw/riscv/microchip_pfsoc.c

> @@ -121,7 +121,10 @@ static const struct MemmapEntry {

>      [MICROCHIP_PFSOC_ENVM_CFG] =        { 0x20200000,     0x1000 },

>      [MICROCHIP_PFSOC_ENVM_DATA] =       { 0x20220000,    0x20000 },

>      [MICROCHIP_PFSOC_IOSCB] =           { 0x30000000, 0x10000000 },

> -    [MICROCHIP_PFSOC_DRAM] =            { 0x80000000,        0x0 },

> +    [MICROCHIP_PFSOC_DRAM_LO] =         { 0x80000000, 0x40000000 },

> +    [MICROCHIP_PFSOC_DRAM_LO_ALIAS] =   { 0xc0000000, 0x40000000 },

> +    [MICROCHIP_PFSOC_DRAM_HI] =       { 0x1000000000,        0x0 },

> +    [MICROCHIP_PFSOC_DRAM_HI_ALIAS] = { 0x1400000000,        0x0 },

>  };

>

>  static void microchip_pfsoc_soc_instance_init(Object *obj)

> @@ -424,7 +427,11 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)

>      const struct MemmapEntry *memmap = microchip_pfsoc_memmap;

>      MicrochipIcicleKitState *s = MICROCHIP_ICICLE_KIT_MACHINE(machine);

>      MemoryRegion *system_memory = get_system_memory();

> -    MemoryRegion *main_mem = g_new(MemoryRegion, 1);

> +    MemoryRegion *mem_low = g_new(MemoryRegion, 1);

> +    MemoryRegion *mem_low_alias = g_new(MemoryRegion, 1);

> +    MemoryRegion *mem_high = g_new(MemoryRegion, 1);

> +    MemoryRegion *mem_high_alias = g_new(MemoryRegion, 1);

> +    uint64_t mem_high_size;

>      DriveInfo *dinfo = drive_get_next(IF_SD);

>

>      /* Sanity check on RAM size */

> @@ -441,10 +448,39 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)

>      qdev_realize(DEVICE(&s->soc), NULL, &error_abort);

>

>      /* Register RAM */

> -    memory_region_init_ram(main_mem, NULL, "microchip.icicle.kit.ram",

> -                           machine->ram_size, &error_fatal);

> +    memory_region_init_ram(mem_low, NULL, "microchip.icicle.kit.ram_low",

> +                           memmap[MICROCHIP_PFSOC_DRAM_LO].size,

> +                           &error_fatal);

> +    memory_region_init_alias(mem_low_alias, NULL,

> +                             "microchip.icicle.kit.ram_low.alias",

> +                             mem_low, 0,

> +                             memmap[MICROCHIP_PFSOC_DRAM_LO_ALIAS].size);

> +    memory_region_add_subregion(system_memory,

> +                                memmap[MICROCHIP_PFSOC_DRAM_LO].base,

> +                                mem_low);

> +    memory_region_add_subregion(system_memory,

> +                                memmap[MICROCHIP_PFSOC_DRAM_LO_ALIAS].base,

> +                                mem_low_alias);

> +

> +    /*

> +     * Map 1 GiB high memory because HSS will do memory test against the high

> +     * memory address range regardless of physical memory installed.

> +     *

> +     * See memory_tests() in mss_ddr.c in the HSS source code.

> +     */

> +    mem_high_size = machine->ram_size - 1 * GiB;

> +

> +    memory_region_init_ram(mem_high, NULL, "microchip.icicle.kit.ram_high",

> +                           mem_high_size, &error_fatal);

> +    memory_region_init_alias(mem_high_alias, NULL,

> +                             "microchip.icicle.kit.ram_high.alias",

> +                             mem_high, 0, mem_high_size);

> +    memory_region_add_subregion(system_memory,

> +                                memmap[MICROCHIP_PFSOC_DRAM_HI].base,

> +                                mem_high);

>      memory_region_add_subregion(system_memory,

> -                                memmap[MICROCHIP_PFSOC_DRAM].base, main_mem);

> +                                memmap[MICROCHIP_PFSOC_DRAM_HI_ALIAS].base,

> +                                mem_high_alias);

>

>      /* Load the firmware */

>      riscv_find_and_load_firmware(machine, BIOS_FILENAME, RESET_VECTOR, NULL);

> @@ -470,7 +506,7 @@ static void microchip_icicle_kit_machine_class_init(ObjectClass *oc, void *data)

>                     MICROCHIP_PFSOC_COMPUTE_CPU_COUNT;

>      mc->min_cpus = MICROCHIP_PFSOC_MANAGEMENT_CPU_COUNT + 1;

>      mc->default_cpus = mc->min_cpus;

> -    mc->default_ram_size = 1 * GiB;

> +    mc->default_ram_size = 2047 * MiB;

>  }

>

>  static const TypeInfo microchip_icicle_kit_machine_typeinfo = {

> diff --git a/include/hw/riscv/microchip_pfsoc.h b/include/hw/riscv/microchip_pfsoc.h

> index f34a6b3fd7..db77e9c84a 100644

> --- a/include/hw/riscv/microchip_pfsoc.h

> +++ b/include/hw/riscv/microchip_pfsoc.h

> @@ -105,7 +105,10 @@ enum {

>      MICROCHIP_PFSOC_ENVM_CFG,

>      MICROCHIP_PFSOC_ENVM_DATA,

>      MICROCHIP_PFSOC_IOSCB,

> -    MICROCHIP_PFSOC_DRAM,

> +    MICROCHIP_PFSOC_DRAM_LO,

> +    MICROCHIP_PFSOC_DRAM_LO_ALIAS,

> +    MICROCHIP_PFSOC_DRAM_HI,

> +    MICROCHIP_PFSOC_DRAM_HI_ALIAS

>  };

>

>  enum {

> --

> 2.25.1

>

>
Bin Meng Nov. 1, 2020, 4:50 p.m. UTC | #2
Hi Alistair,

On Mon, Nov 2, 2020 at 12:46 AM Alistair Francis <alistair23@gmail.com> wrote:
>

> On Sun, Nov 1, 2020 at 8:42 AM Bin Meng <bmeng.cn@gmail.com> wrote:

> >

> > From: Bin Meng <bin.meng@windriver.com>

> >

> > When system memory is larger than 1 GiB (high memory), PolarFire SoC

> > maps it at address 0x10_0000_0000. Address 0xC000_0000 and above is

> > aliased to the same 1 GiB low memory with different cache attributes.

> >

> > At present QEMU maps the system memory contiguously from 0x8000_0000.

> > This corrects the wrong QEMU logic. Note address 0x14_0000_0000 is

> > the alias to the high memory, and even physical memory is only 1 GiB,

> > the HSS codes still tries to probe the high memory alias address.

> > It seems there is no issue on the real hardware, so we will have to

> > take that into the consideration in our emulation. Due to this, we

> > we increase the default system memory size to 2047 MiB (the largest

> > ram size allowed when running on a 32-bit host) so that user gets

> > notified an error when less than 2047 MiB is specified.

>

> Is this better than just not supporting 32-bit hosts? Or could we make


I am not sure if we have a general rule about discontinuing 32-bit
hosts support, i.e.: deprecating 32-bit hosts at some time?

> this number even lower (as low as possible that still works with HSS)?

>


Sure I will figure this out and set this number to meet the minium
requirement of HSS.

Regards,
Bin
Alistair Francis Nov. 1, 2020, 4:53 p.m. UTC | #3
On Sun, Nov 1, 2020 at 8:51 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>

> Hi Alistair,

>

> On Mon, Nov 2, 2020 at 12:46 AM Alistair Francis <alistair23@gmail.com> wrote:

> >

> > On Sun, Nov 1, 2020 at 8:42 AM Bin Meng <bmeng.cn@gmail.com> wrote:

> > >

> > > From: Bin Meng <bin.meng@windriver.com>

> > >

> > > When system memory is larger than 1 GiB (high memory), PolarFire SoC

> > > maps it at address 0x10_0000_0000. Address 0xC000_0000 and above is

> > > aliased to the same 1 GiB low memory with different cache attributes.

> > >

> > > At present QEMU maps the system memory contiguously from 0x8000_0000.

> > > This corrects the wrong QEMU logic. Note address 0x14_0000_0000 is

> > > the alias to the high memory, and even physical memory is only 1 GiB,

> > > the HSS codes still tries to probe the high memory alias address.

> > > It seems there is no issue on the real hardware, so we will have to

> > > take that into the consideration in our emulation. Due to this, we

> > > we increase the default system memory size to 2047 MiB (the largest

> > > ram size allowed when running on a 32-bit host) so that user gets

> > > notified an error when less than 2047 MiB is specified.

> >

> > Is this better than just not supporting 32-bit hosts? Or could we make

>

> I am not sure if we have a general rule about discontinuing 32-bit

> hosts support, i.e.: deprecating 32-bit hosts at some time?

>

> > this number even lower (as low as possible that still works with HSS)?

> >

>

> Sure I will figure this out and set this number to meet the minium

> requirement of HSS.


Thanks, that's probably the best bet for both 32 and 64-bit hosts then.

Alistair

>

> Regards,

> Bin
diff mbox series

Patch

diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
index 44a84732ac..0bc17b3955 100644
--- a/hw/riscv/microchip_pfsoc.c
+++ b/hw/riscv/microchip_pfsoc.c
@@ -121,7 +121,10 @@  static const struct MemmapEntry {
     [MICROCHIP_PFSOC_ENVM_CFG] =        { 0x20200000,     0x1000 },
     [MICROCHIP_PFSOC_ENVM_DATA] =       { 0x20220000,    0x20000 },
     [MICROCHIP_PFSOC_IOSCB] =           { 0x30000000, 0x10000000 },
-    [MICROCHIP_PFSOC_DRAM] =            { 0x80000000,        0x0 },
+    [MICROCHIP_PFSOC_DRAM_LO] =         { 0x80000000, 0x40000000 },
+    [MICROCHIP_PFSOC_DRAM_LO_ALIAS] =   { 0xc0000000, 0x40000000 },
+    [MICROCHIP_PFSOC_DRAM_HI] =       { 0x1000000000,        0x0 },
+    [MICROCHIP_PFSOC_DRAM_HI_ALIAS] = { 0x1400000000,        0x0 },
 };
 
 static void microchip_pfsoc_soc_instance_init(Object *obj)
@@ -424,7 +427,11 @@  static void microchip_icicle_kit_machine_init(MachineState *machine)
     const struct MemmapEntry *memmap = microchip_pfsoc_memmap;
     MicrochipIcicleKitState *s = MICROCHIP_ICICLE_KIT_MACHINE(machine);
     MemoryRegion *system_memory = get_system_memory();
-    MemoryRegion *main_mem = g_new(MemoryRegion, 1);
+    MemoryRegion *mem_low = g_new(MemoryRegion, 1);
+    MemoryRegion *mem_low_alias = g_new(MemoryRegion, 1);
+    MemoryRegion *mem_high = g_new(MemoryRegion, 1);
+    MemoryRegion *mem_high_alias = g_new(MemoryRegion, 1);
+    uint64_t mem_high_size;
     DriveInfo *dinfo = drive_get_next(IF_SD);
 
     /* Sanity check on RAM size */
@@ -441,10 +448,39 @@  static void microchip_icicle_kit_machine_init(MachineState *machine)
     qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
 
     /* Register RAM */
-    memory_region_init_ram(main_mem, NULL, "microchip.icicle.kit.ram",
-                           machine->ram_size, &error_fatal);
+    memory_region_init_ram(mem_low, NULL, "microchip.icicle.kit.ram_low",
+                           memmap[MICROCHIP_PFSOC_DRAM_LO].size,
+                           &error_fatal);
+    memory_region_init_alias(mem_low_alias, NULL,
+                             "microchip.icicle.kit.ram_low.alias",
+                             mem_low, 0,
+                             memmap[MICROCHIP_PFSOC_DRAM_LO_ALIAS].size);
+    memory_region_add_subregion(system_memory,
+                                memmap[MICROCHIP_PFSOC_DRAM_LO].base,
+                                mem_low);
+    memory_region_add_subregion(system_memory,
+                                memmap[MICROCHIP_PFSOC_DRAM_LO_ALIAS].base,
+                                mem_low_alias);
+
+    /*
+     * Map 1 GiB high memory because HSS will do memory test against the high
+     * memory address range regardless of physical memory installed.
+     *
+     * See memory_tests() in mss_ddr.c in the HSS source code.
+     */
+    mem_high_size = machine->ram_size - 1 * GiB;
+
+    memory_region_init_ram(mem_high, NULL, "microchip.icicle.kit.ram_high",
+                           mem_high_size, &error_fatal);
+    memory_region_init_alias(mem_high_alias, NULL,
+                             "microchip.icicle.kit.ram_high.alias",
+                             mem_high, 0, mem_high_size);
+    memory_region_add_subregion(system_memory,
+                                memmap[MICROCHIP_PFSOC_DRAM_HI].base,
+                                mem_high);
     memory_region_add_subregion(system_memory,
-                                memmap[MICROCHIP_PFSOC_DRAM].base, main_mem);
+                                memmap[MICROCHIP_PFSOC_DRAM_HI_ALIAS].base,
+                                mem_high_alias);
 
     /* Load the firmware */
     riscv_find_and_load_firmware(machine, BIOS_FILENAME, RESET_VECTOR, NULL);
@@ -470,7 +506,7 @@  static void microchip_icicle_kit_machine_class_init(ObjectClass *oc, void *data)
                    MICROCHIP_PFSOC_COMPUTE_CPU_COUNT;
     mc->min_cpus = MICROCHIP_PFSOC_MANAGEMENT_CPU_COUNT + 1;
     mc->default_cpus = mc->min_cpus;
-    mc->default_ram_size = 1 * GiB;
+    mc->default_ram_size = 2047 * MiB;
 }
 
 static const TypeInfo microchip_icicle_kit_machine_typeinfo = {
diff --git a/include/hw/riscv/microchip_pfsoc.h b/include/hw/riscv/microchip_pfsoc.h
index f34a6b3fd7..db77e9c84a 100644
--- a/include/hw/riscv/microchip_pfsoc.h
+++ b/include/hw/riscv/microchip_pfsoc.h
@@ -105,7 +105,10 @@  enum {
     MICROCHIP_PFSOC_ENVM_CFG,
     MICROCHIP_PFSOC_ENVM_DATA,
     MICROCHIP_PFSOC_IOSCB,
-    MICROCHIP_PFSOC_DRAM,
+    MICROCHIP_PFSOC_DRAM_LO,
+    MICROCHIP_PFSOC_DRAM_LO_ALIAS,
+    MICROCHIP_PFSOC_DRAM_HI,
+    MICROCHIP_PFSOC_DRAM_HI_ALIAS
 };
 
 enum {