[v8,11/35] RISC-V: Mark ROM read-only after copying in code

Message ID	1524699938-6764-12-git-send-email-mjc@sifive.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org> From: Michael Clark <mjc@sifive.com> To: qemu-devel@nongnu.org Date: Thu, 26 Apr 2018 11:45:14 +1200 Message-Id: <1524699938-6764-12-git-send-email-mjc@sifive.com> In-Reply-To: <1524699938-6764-1-git-send-email-mjc@sifive.com> References: <1524699938-6764-1-git-send-email-mjc@sifive.com> Subject: [Qemu-devel] [PATCH v8 11/35] RISC-V: Mark ROM read-only after copying in code Precedence: list Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>, Bastian Koppelmann <kbastian@mail.uni-paderborn.de>, Palmer Dabbelt <palmer@sifive.com>, Michael Clark <mjc@sifive.com>, Alistair Francis <Alistair.Francis@wdc.com>, patches@groups.riscv.org Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" <qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org>

Michael Clark April 25, 2018, 11:45 p.m. UTC

The sifive_u machine already marks its ROM readonly. This fixes
the remaining boards. This commit also makes all boards use
mask_rom as the variable name for the ROM. This change also
makes space for the maximum device tree size size and adds
an explicit bounds check and error message.

Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Cc: Palmer Dabbelt <palmer@sifive.com>
Cc: Alistair Francis <Alistair.Francis@wdc.com>
Signed-off-by: Michael Clark <mjc@sifive.com>
---
 hw/riscv/sifive_e.c     | 20 +++++++---------
 hw/riscv/sifive_u.c     | 46 ++++++++++++++++++-----------------
 hw/riscv/spike.c        | 64 ++++++++++++++++++++++++++++---------------------
 hw/riscv/virt.c         | 38 +++++++++++++++--------------
 include/hw/riscv/virt.h |  4 ++++
 5 files changed, 93 insertions(+), 79 deletions(-)

Alistair Francis April 26, 2018, 4:48 p.m. UTC | #1

On Wed, Apr 25, 2018 at 5:03 PM Michael Clark <mjc@sifive.com> wrote:

> The sifive_u machine already marks its ROM readonly. This fixes
> the remaining boards. This commit also makes all boards use
> mask_rom as the variable name for the ROM. This change also
> makes space for the maximum device tree size size and adds
> an explicit bounds check and error message.

> Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
> Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> Cc: Palmer Dabbelt <palmer@sifive.com>
> Cc: Alistair Francis <Alistair.Francis@wdc.com>
> Signed-off-by: Michael Clark <mjc@sifive.com>
> ---
>   hw/riscv/sifive_e.c     | 20 +++++++---------
>   hw/riscv/sifive_u.c     | 46 ++++++++++++++++++-----------------
>   hw/riscv/spike.c        | 64
++++++++++++++++++++++++++++---------------------
>   hw/riscv/virt.c         | 38 +++++++++++++++--------------
>   include/hw/riscv/virt.h |  4 ++++
>   5 files changed, 93 insertions(+), 79 deletions(-)

> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> index 39e4cb4..0c8b8e9 100644
> --- a/hw/riscv/sifive_e.c
> +++ b/hw/riscv/sifive_e.c
> @@ -74,14 +74,6 @@ static const struct MemmapEntry {
>       [SIFIVE_E_DTIM] =     { 0x80000000,     0x4000 }
>   };

> -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t len)
> -{
> -    int i;
> -    for (i = 0; i < (len >> 2); i++) {
> -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
> -    }
> -}
> -
>   static uint64_t load_kernel(const char *kernel_filename)
>   {
>       uint64_t kernel_entry, kernel_high;
> @@ -112,6 +104,7 @@ static void riscv_sifive_e_init(MachineState *machine)
>       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
>       MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
>       MemoryRegion *xip_mem = g_new(MemoryRegion, 1);
> +    int i;

>       /* Initialize SOC */
>       object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY);
> @@ -131,7 +124,7 @@ static void riscv_sifive_e_init(MachineState *machine)
>           memmap[SIFIVE_E_DTIM].base, main_mem);

>       /* Mask ROM */
> -    memory_region_init_ram(mask_rom, NULL, "riscv.sifive.e.mrom",
> +    memory_region_init_rom(mask_rom, NULL, "riscv.sifive.e.mrom",
>           memmap[SIFIVE_E_MROM].size, &error_fatal);
>       memory_region_add_subregion(sys_mem,
>           memmap[SIFIVE_E_MROM].base, mask_rom);
> @@ -185,9 +178,12 @@ static void riscv_sifive_e_init(MachineState
*machine)
>           0x00028067,        /* 0x1004: jr      t0 */
>       };

> -    /* copy in the reset vector */
> -    copy_le32_to_phys(memmap[SIFIVE_E_MROM].base, reset_vec,
sizeof(reset_vec));
> -    memory_region_set_readonly(mask_rom, true);
> +    /* copy in the reset vector in little_endian byte order */
> +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> +    }
> +    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
> +                          memmap[SIFIVE_E_MROM].base,
&address_space_memory);

>       if (machine->kernel_filename) {
>           load_kernel(machine->kernel_filename);
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 115618b..11ba4c3 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -52,7 +52,7 @@ static const struct MemmapEntry {
>       hwaddr size;
>   } sifive_u_memmap[] = {
>       [SIFIVE_U_DEBUG] =    {        0x0,      0x100 },
> -    [SIFIVE_U_MROM] =     {     0x1000,     0x2000 },
> +    [SIFIVE_U_MROM] =     {     0x1000,    0x11000 },
>       [SIFIVE_U_CLINT] =    {  0x2000000,    0x10000 },
>       [SIFIVE_U_PLIC] =     {  0xc000000,  0x4000000 },
>       [SIFIVE_U_UART0] =    { 0x10013000,     0x1000 },
> @@ -60,14 +60,6 @@ static const struct MemmapEntry {
>       [SIFIVE_U_DRAM] =     { 0x80000000,        0x0 },
>   };

> -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t len)
> -{
> -    int i;
> -    for (i = 0; i < (len >> 2); i++) {
> -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
> -    }
> -}
> -
>   static uint64_t load_kernel(const char *kernel_filename)
>   {
>       uint64_t kernel_entry, kernel_high;
> @@ -221,9 +213,10 @@ static void riscv_sifive_u_init(MachineState
*machine)
>       const struct MemmapEntry *memmap = sifive_u_memmap;

>       SiFiveUState *s = g_new0(SiFiveUState, 1);
> -    MemoryRegion *sys_memory = get_system_memory();
> +    MemoryRegion *system_memory = get_system_memory();
>       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
> +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> +    int i;

>       /* Initialize SOC */
>       object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY);
> @@ -239,17 +232,17 @@ static void riscv_sifive_u_init(MachineState
*machine)
>       /* register RAM */
>       memory_region_init_ram(main_mem, NULL, "riscv.sifive.u.ram",
>                              machine->ram_size, &error_fatal);
> -    memory_region_add_subregion(sys_memory, memmap[SIFIVE_U_DRAM].base,
> +    memory_region_add_subregion(system_memory,
memmap[SIFIVE_U_DRAM].base,
>           main_mem);

>       /* create device tree */
>       create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline);

>       /* boot rom */
> -    memory_region_init_ram(boot_rom, NULL, "riscv.sifive.u.mrom",
> -                           memmap[SIFIVE_U_MROM].base, &error_fatal);
> -    memory_region_set_readonly(boot_rom, true);
> -    memory_region_add_subregion(sys_memory, 0x0, boot_rom);
> +    memory_region_init_rom(mask_rom, NULL, "riscv.sifive.u.mrom",
> +                           memmap[SIFIVE_U_MROM].size, &error_fatal);
> +    memory_region_add_subregion(system_memory,
memmap[SIFIVE_U_MROM].base,
> +                                mask_rom);

>       if (machine->kernel_filename) {
>           load_kernel(machine->kernel_filename);
> @@ -272,13 +265,22 @@ static void riscv_sifive_u_init(MachineState
*machine)
>                                          /* dtb: */
>       };

> -    /* copy in the reset vector */
> -    copy_le32_to_phys(memmap[SIFIVE_U_MROM].base, reset_vec,
sizeof(reset_vec));
> +    /* copy in the reset vector in little_endian byte order */
> +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> +    }
> +    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
> +                          memmap[SIFIVE_U_MROM].base,
&address_space_memory);

>       /* copy in the device tree */
> +    if (s->fdt_size >= memmap[SIFIVE_U_MROM].size - sizeof(reset_vec)) {
> +        error_report("qemu: not enough space to store device-tree");
> +        exit(1);
> +    }
>       qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
> -    cpu_physical_memory_write(memmap[SIFIVE_U_MROM].base +
> -        sizeof(reset_vec), s->fdt, s->fdt_size);
> +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, s->fdt_size,
> +                          memmap[SIFIVE_U_MROM].base + sizeof(reset_vec),
> +                          &address_space_memory);

>       /* MMIO */
>       s->plic = sifive_plic_create(memmap[SIFIVE_U_PLIC].base,
> @@ -292,9 +294,9 @@ static void riscv_sifive_u_init(MachineState *machine)
>           SIFIVE_U_PLIC_CONTEXT_BASE,
>           SIFIVE_U_PLIC_CONTEXT_STRIDE,
>           memmap[SIFIVE_U_PLIC].size);
> -    sifive_uart_create(sys_memory, memmap[SIFIVE_U_UART0].base,
> +    sifive_uart_create(system_memory, memmap[SIFIVE_U_UART0].base,
>           serial_hds[0], SIFIVE_PLIC(s->plic)->irqs[SIFIVE_U_UART0_IRQ]);
> -    /* sifive_uart_create(sys_memory, memmap[SIFIVE_U_UART1].base,
> +    /* sifive_uart_create(system_memory, memmap[SIFIVE_U_UART1].base,
>           serial_hds[1], SIFIVE_PLIC(s->plic)->irqs[SIFIVE_U_UART1_IRQ]);
*/
>       sifive_clint_create(memmap[SIFIVE_U_CLINT].base,
>           memmap[SIFIVE_U_CLINT].size, smp_cpus,
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 3f6bd0a..d1dbe6b 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -46,19 +46,11 @@ static const struct MemmapEntry {
>       hwaddr base;
>       hwaddr size;
>   } spike_memmap[] = {
> -    [SPIKE_MROM] =     {     0x1000,     0x2000 },
> +    [SPIKE_MROM] =     {     0x1000,    0x11000 },
>       [SPIKE_CLINT] =    {  0x2000000,    0x10000 },
>       [SPIKE_DRAM] =     { 0x80000000,        0x0 },
>   };

> -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t len)
> -{
> -    int i;
> -    for (i = 0; i < (len >> 2); i++) {
> -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
> -    }
> -}
> -
>   static uint64_t load_kernel(const char *kernel_filename)
>   {
>       uint64_t kernel_entry, kernel_high;
> @@ -173,7 +165,8 @@ static void spike_v1_10_0_board_init(MachineState
*machine)
>       SpikeState *s = g_new0(SpikeState, 1);
>       MemoryRegion *system_memory = get_system_memory();
>       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
> +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> +    int i;

>       /* Initialize SOC */
>       object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY);
> @@ -196,9 +189,10 @@ static void spike_v1_10_0_board_init(MachineState
*machine)
>       create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline);

>       /* boot rom */
> -    memory_region_init_ram(boot_rom, NULL, "riscv.spike.bootrom",
> -                           s->fdt_size + 0x2000, &error_fatal);
> -    memory_region_add_subregion(system_memory, 0x0, boot_rom);
> +    memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
> +                           memmap[SPIKE_MROM].size, &error_fatal);
> +    memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
> +                                mask_rom);

>       if (machine->kernel_filename) {
>           load_kernel(machine->kernel_filename);
> @@ -221,16 +215,25 @@ static void spike_v1_10_0_board_init(MachineState
*machine)
>                                        /* dtb: */
>       };

> -    /* copy in the reset vector */
> -    copy_le32_to_phys(memmap[SPIKE_MROM].base, reset_vec,
sizeof(reset_vec));
> +    /* copy in the reset vector in little_endian byte order */
> +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> +    }
> +    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
> +                          memmap[SPIKE_MROM].base,
&address_space_memory);

>       /* copy in the device tree */
> +    if (s->fdt_size >= memmap[SPIKE_MROM].size - sizeof(reset_vec)) {
> +        error_report("qemu: not enough space to store device-tree");
> +        exit(1);
> +    }
>       qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
> -    cpu_physical_memory_write(memmap[SPIKE_MROM].base +
sizeof(reset_vec),
> -        s->fdt, s->fdt_size);
> +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, s->fdt_size,
> +                          memmap[SPIKE_MROM].base + sizeof(reset_vec),
> +                          &address_space_memory);

>       /* initialize HTIF using symbols found in load_kernel */
> -    htif_mm_init(system_memory, boot_rom, &s->soc.harts[0].env,
serial_hds[0]);
> +    htif_mm_init(system_memory, mask_rom, &s->soc.harts[0].env,
serial_hds[0]);

>       /* Core Local Interruptor (timer and IPI) */
>       sifive_clint_create(memmap[SPIKE_CLINT].base,
memmap[SPIKE_CLINT].size,
> @@ -244,7 +247,8 @@ static void spike_v1_09_1_board_init(MachineState
*machine)
>       SpikeState *s = g_new0(SpikeState, 1);
>       MemoryRegion *system_memory = get_system_memory();
>       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
> +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> +    int i;

>       /* Initialize SOC */
>       object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY);
> @@ -264,9 +268,10 @@ static void spike_v1_09_1_board_init(MachineState
*machine)
>           main_mem);

>       /* boot rom */
> -    memory_region_init_ram(boot_rom, NULL, "riscv.spike.bootrom",
> -                           0x40000, &error_fatal);
> -    memory_region_add_subregion(system_memory, 0x0, boot_rom);
> +    memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
> +                           memmap[SPIKE_MROM].size, &error_fatal);
> +    memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
> +                                mask_rom);

>       if (machine->kernel_filename) {
>           load_kernel(machine->kernel_filename);
> @@ -319,15 +324,20 @@ static void spike_v1_09_1_board_init(MachineState
*machine)
>       g_free(isa);
>       size_t config_string_len = strlen(config_string);

> -    /* copy in the reset vector */
> -    copy_le32_to_phys(memmap[SPIKE_MROM].base, reset_vec,
sizeof(reset_vec));
> +    /* copy in the reset vector in little_endian byte order */
> +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> +    }
> +    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
> +                          memmap[SPIKE_MROM].base,
&address_space_memory);

>       /* copy in the config string */
> -    cpu_physical_memory_write(memmap[SPIKE_MROM].base +
sizeof(reset_vec),
> -        config_string, config_string_len);
> +    rom_add_blob_fixed_as("mrom.reset", config_string, config_string_len,
> +                          memmap[SPIKE_MROM].base + sizeof(reset_vec),
> +                          &address_space_memory);

>       /* initialize HTIF using symbols found in load_kernel */
> -    htif_mm_init(system_memory, boot_rom, &s->soc.harts[0].env,
serial_hds[0]);
> +    htif_mm_init(system_memory, mask_rom, &s->soc.harts[0].env,
serial_hds[0]);

>       /* Core Local Interruptor (timer and IPI) */
>       sifive_clint_create(memmap[SPIKE_CLINT].base,
memmap[SPIKE_CLINT].size,
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 090befe..20c509d 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -45,8 +45,8 @@ static const struct MemmapEntry {
>       hwaddr size;
>   } virt_memmap[] = {
>       [VIRT_DEBUG] =    {        0x0,      0x100 },
> -    [VIRT_MROM] =     {     0x1000,     0x2000 },
> -    [VIRT_TEST] =     {     0x4000,     0x1000 },
> +    [VIRT_MROM] =     {     0x1000,    0x11000 },
> +    [VIRT_TEST] =     {   0x100000,     0x1000 },
>       [VIRT_CLINT] =    {  0x2000000,    0x10000 },
>       [VIRT_PLIC] =     {  0xc000000,  0x4000000 },
>       [VIRT_UART0] =    { 0x10000000,      0x100 },
> @@ -54,14 +54,6 @@ static const struct MemmapEntry {
>       [VIRT_DRAM] =     { 0x80000000,        0x0 },
>   };

> -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t len)
> -{
> -    int i;
> -    for (i = 0; i < (len >> 2); i++) {
> -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
> -    }
> -}
> -
>   static uint64_t load_kernel(const char *kernel_filename)
>   {
>       uint64_t kernel_entry, kernel_high;
> @@ -272,7 +264,7 @@ static void riscv_virt_board_init(MachineState
*machine)
>       RISCVVirtState *s = g_new0(RISCVVirtState, 1);
>       MemoryRegion *system_memory = get_system_memory();
>       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
> +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
>       char *plic_hart_config;
>       size_t plic_hart_config_len;
>       int i;
> @@ -299,9 +291,10 @@ static void riscv_virt_board_init(MachineState
*machine)
>       fdt = create_fdt(s, memmap, machine->ram_size,
machine->kernel_cmdline);

>       /* boot rom */
> -    memory_region_init_ram(boot_rom, NULL, "riscv_virt_board.bootrom",
> -                           s->fdt_size + 0x2000, &error_fatal);
> -    memory_region_add_subregion(system_memory, 0x0, boot_rom);
> +    memory_region_init_rom(mask_rom, NULL, "riscv_virt_board.mrom",
> +                           memmap[VIRT_MROM].size, &error_fatal);
> +    memory_region_add_subregion(system_memory, memmap[VIRT_MROM].base,
> +                                mask_rom);

>       if (machine->kernel_filename) {
>           uint64_t kernel_entry = load_kernel(machine->kernel_filename);
> @@ -335,13 +328,22 @@ static void riscv_virt_board_init(MachineState
*machine)
>                                        /* dtb: */
>       };

> -    /* copy in the reset vector */
> -    copy_le32_to_phys(memmap[VIRT_MROM].base, reset_vec,
sizeof(reset_vec));
> +    /* copy in the reset vector in little_endian byte order */
> +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> +    }
> +    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
> +                          memmap[VIRT_MROM].base, &address_space_memory);

>       /* copy in the device tree */
> +    if (s->fdt_size >= memmap[VIRT_MROM].size - sizeof(reset_vec)) {
> +        error_report("qemu: not enough space to store device-tree");
> +        exit(1);
> +    }
>       qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
> -    cpu_physical_memory_write(memmap[VIRT_MROM].base + sizeof(reset_vec),
> -        s->fdt, s->fdt_size);
> +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, s->fdt_size,
> +                          memmap[VIRT_MROM].base + sizeof(reset_vec),
> +                          &address_space_memory);

>       /* create PLIC hart topology configuration string */
>       plic_hart_config_len = (strlen(VIRT_PLIC_HART_CONFIG) + 1) *
smp_cpus;
> diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> index 91163d6..6f2668e 100644
> --- a/include/hw/riscv/virt.h
> +++ b/include/hw/riscv/virt.h
> @@ -19,6 +19,10 @@
>   #ifndef HW_RISCV_VIRT_H
>   #define HW_RISCV_VIRT_H

> +#define TYPE_RISCV_VIRT_BOARD "riscv.virt"
> +#define VIRT(obj) \
> +    OBJECT_CHECK(RISCVVirtState, (obj), TYPE_RISCV_VIRT_BOARD)
> +

This should be in a seperate patch.

Alistair

>   typedef struct {
>       /*< private >*/
>       SysBusDevice parent_obj;
> --
> 2.7.0

Michael Clark April 27, 2018, 5:22 a.m. UTC | #2

On Fri, Apr 27, 2018 at 4:48 AM, Alistair Francis <alistair23@gmail.com>
wrote:

> On Wed, Apr 25, 2018 at 5:03 PM Michael Clark <mjc@sifive.com> wrote:
>
> > The sifive_u machine already marks its ROM readonly. This fixes
> > the remaining boards. This commit also makes all boards use
> > mask_rom as the variable name for the ROM. This change also
> > makes space for the maximum device tree size size and adds
> > an explicit bounds check and error message.
>
> > Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
> > Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> > Cc: Palmer Dabbelt <palmer@sifive.com>
> > Cc: Alistair Francis <Alistair.Francis@wdc.com>
> > Signed-off-by: Michael Clark <mjc@sifive.com>
> > ---
> >   hw/riscv/sifive_e.c     | 20 +++++++---------
> >   hw/riscv/sifive_u.c     | 46 ++++++++++++++++++-----------------
> >   hw/riscv/spike.c        | 64
> ++++++++++++++++++++++++++++---------------------
> >   hw/riscv/virt.c         | 38 +++++++++++++++--------------
> >   include/hw/riscv/virt.h |  4 ++++
> >   5 files changed, 93 insertions(+), 79 deletions(-)
>
> > diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> > index 39e4cb4..0c8b8e9 100644
> > --- a/hw/riscv/sifive_e.c
> > +++ b/hw/riscv/sifive_e.c
> > @@ -74,14 +74,6 @@ static const struct MemmapEntry {
> >       [SIFIVE_E_DTIM] =     { 0x80000000,     0x4000 }
> >   };
>
> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t len)
> > -{
> > -    int i;
> > -    for (i = 0; i < (len >> 2); i++) {
> > -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
> > -    }
> > -}
> > -
> >   static uint64_t load_kernel(const char *kernel_filename)
> >   {
> >       uint64_t kernel_entry, kernel_high;
> > @@ -112,6 +104,7 @@ static void riscv_sifive_e_init(MachineState
> *machine)
> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> >       MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> >       MemoryRegion *xip_mem = g_new(MemoryRegion, 1);
> > +    int i;
>
> >       /* Initialize SOC */
> >       object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY);
> > @@ -131,7 +124,7 @@ static void riscv_sifive_e_init(MachineState
> *machine)
> >           memmap[SIFIVE_E_DTIM].base, main_mem);
>
> >       /* Mask ROM */
> > -    memory_region_init_ram(mask_rom, NULL, "riscv.sifive.e.mrom",
> > +    memory_region_init_rom(mask_rom, NULL, "riscv.sifive.e.mrom",
> >           memmap[SIFIVE_E_MROM].size, &error_fatal);
> >       memory_region_add_subregion(sys_mem,
> >           memmap[SIFIVE_E_MROM].base, mask_rom);
> > @@ -185,9 +178,12 @@ static void riscv_sifive_e_init(MachineState
> *machine)
> >           0x00028067,        /* 0x1004: jr      t0 */
> >       };
>
> > -    /* copy in the reset vector */
> > -    copy_le32_to_phys(memmap[SIFIVE_E_MROM].base, reset_vec,
> sizeof(reset_vec));
> > -    memory_region_set_readonly(mask_rom, true);
> > +    /* copy in the reset vector in little_endian byte order */
> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> > +    }
> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
> > +                          memmap[SIFIVE_E_MROM].base,
> &address_space_memory);
>
> >       if (machine->kernel_filename) {
> >           load_kernel(machine->kernel_filename);
> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> > index 115618b..11ba4c3 100644
> > --- a/hw/riscv/sifive_u.c
> > +++ b/hw/riscv/sifive_u.c
> > @@ -52,7 +52,7 @@ static const struct MemmapEntry {
> >       hwaddr size;
> >   } sifive_u_memmap[] = {
> >       [SIFIVE_U_DEBUG] =    {        0x0,      0x100 },
> > -    [SIFIVE_U_MROM] =     {     0x1000,     0x2000 },
> > +    [SIFIVE_U_MROM] =     {     0x1000,    0x11000 },
> >       [SIFIVE_U_CLINT] =    {  0x2000000,    0x10000 },
> >       [SIFIVE_U_PLIC] =     {  0xc000000,  0x4000000 },
> >       [SIFIVE_U_UART0] =    { 0x10013000,     0x1000 },
> > @@ -60,14 +60,6 @@ static const struct MemmapEntry {
> >       [SIFIVE_U_DRAM] =     { 0x80000000,        0x0 },
> >   };
>
> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t len)
> > -{
> > -    int i;
> > -    for (i = 0; i < (len >> 2); i++) {
> > -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
> > -    }
> > -}
> > -
> >   static uint64_t load_kernel(const char *kernel_filename)
> >   {
> >       uint64_t kernel_entry, kernel_high;
> > @@ -221,9 +213,10 @@ static void riscv_sifive_u_init(MachineState
> *machine)
> >       const struct MemmapEntry *memmap = sifive_u_memmap;
>
> >       SiFiveUState *s = g_new0(SiFiveUState, 1);
> > -    MemoryRegion *sys_memory = get_system_memory();
> > +    MemoryRegion *system_memory = get_system_memory();
> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> > +    int i;
>
> >       /* Initialize SOC */
> >       object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY);
> > @@ -239,17 +232,17 @@ static void riscv_sifive_u_init(MachineState
> *machine)
> >       /* register RAM */
> >       memory_region_init_ram(main_mem, NULL, "riscv.sifive.u.ram",
> >                              machine->ram_size, &error_fatal);
> > -    memory_region_add_subregion(sys_memory, memmap[SIFIVE_U_DRAM].base,
> > +    memory_region_add_subregion(system_memory,
> memmap[SIFIVE_U_DRAM].base,
> >           main_mem);
>
> >       /* create device tree */
> >       create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline);
>
> >       /* boot rom */
> > -    memory_region_init_ram(boot_rom, NULL, "riscv.sifive.u.mrom",
> > -                           memmap[SIFIVE_U_MROM].base, &error_fatal);
> > -    memory_region_set_readonly(boot_rom, true);
> > -    memory_region_add_subregion(sys_memory, 0x0, boot_rom);
> > +    memory_region_init_rom(mask_rom, NULL, "riscv.sifive.u.mrom",
> > +                           memmap[SIFIVE_U_MROM].size, &error_fatal);
> > +    memory_region_add_subregion(system_memory,
> memmap[SIFIVE_U_MROM].base,
> > +                                mask_rom);
>
> >       if (machine->kernel_filename) {
> >           load_kernel(machine->kernel_filename);
> > @@ -272,13 +265,22 @@ static void riscv_sifive_u_init(MachineState
> *machine)
> >                                          /* dtb: */
> >       };
>
> > -    /* copy in the reset vector */
> > -    copy_le32_to_phys(memmap[SIFIVE_U_MROM].base, reset_vec,
> sizeof(reset_vec));
> > +    /* copy in the reset vector in little_endian byte order */
> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> > +    }
> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
> > +                          memmap[SIFIVE_U_MROM].base,
> &address_space_memory);
>
> >       /* copy in the device tree */
> > +    if (s->fdt_size >= memmap[SIFIVE_U_MROM].size - sizeof(reset_vec)) {
> > +        error_report("qemu: not enough space to store device-tree");
> > +        exit(1);
> > +    }
> >       qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
> > -    cpu_physical_memory_write(memmap[SIFIVE_U_MROM].base +
> > -        sizeof(reset_vec), s->fdt, s->fdt_size);
> > +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, s->fdt_size,
> > +                          memmap[SIFIVE_U_MROM].base +
> sizeof(reset_vec),
> > +                          &address_space_memory);
>
> >       /* MMIO */
> >       s->plic = sifive_plic_create(memmap[SIFIVE_U_PLIC].base,
> > @@ -292,9 +294,9 @@ static void riscv_sifive_u_init(MachineState
> *machine)
> >           SIFIVE_U_PLIC_CONTEXT_BASE,
> >           SIFIVE_U_PLIC_CONTEXT_STRIDE,
> >           memmap[SIFIVE_U_PLIC].size);
> > -    sifive_uart_create(sys_memory, memmap[SIFIVE_U_UART0].base,
> > +    sifive_uart_create(system_memory, memmap[SIFIVE_U_UART0].base,
> >           serial_hds[0], SIFIVE_PLIC(s->plic)->irqs[
> SIFIVE_U_UART0_IRQ]);
> > -    /* sifive_uart_create(sys_memory, memmap[SIFIVE_U_UART1].base,
> > +    /* sifive_uart_create(system_memory, memmap[SIFIVE_U_UART1].base,
> >           serial_hds[1], SIFIVE_PLIC(s->plic)->irqs[
> SIFIVE_U_UART1_IRQ]);
> */
> >       sifive_clint_create(memmap[SIFIVE_U_CLINT].base,
> >           memmap[SIFIVE_U_CLINT].size, smp_cpus,
> > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> > index 3f6bd0a..d1dbe6b 100644
> > --- a/hw/riscv/spike.c
> > +++ b/hw/riscv/spike.c
> > @@ -46,19 +46,11 @@ static const struct MemmapEntry {
> >       hwaddr base;
> >       hwaddr size;
> >   } spike_memmap[] = {
> > -    [SPIKE_MROM] =     {     0x1000,     0x2000 },
> > +    [SPIKE_MROM] =     {     0x1000,    0x11000 },
> >       [SPIKE_CLINT] =    {  0x2000000,    0x10000 },
> >       [SPIKE_DRAM] =     { 0x80000000,        0x0 },
> >   };
>
> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t len)
> > -{
> > -    int i;
> > -    for (i = 0; i < (len >> 2); i++) {
> > -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
> > -    }
> > -}
> > -
> >   static uint64_t load_kernel(const char *kernel_filename)
> >   {
> >       uint64_t kernel_entry, kernel_high;
> > @@ -173,7 +165,8 @@ static void spike_v1_10_0_board_init(MachineState
> *machine)
> >       SpikeState *s = g_new0(SpikeState, 1);
> >       MemoryRegion *system_memory = get_system_memory();
> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> > +    int i;
>
> >       /* Initialize SOC */
> >       object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY);
> > @@ -196,9 +189,10 @@ static void spike_v1_10_0_board_init(MachineState
> *machine)
> >       create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline);
>
> >       /* boot rom */
> > -    memory_region_init_ram(boot_rom, NULL, "riscv.spike.bootrom",
> > -                           s->fdt_size + 0x2000, &error_fatal);
> > -    memory_region_add_subregion(system_memory, 0x0, boot_rom);
> > +    memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
> > +                           memmap[SPIKE_MROM].size, &error_fatal);
> > +    memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
> > +                                mask_rom);
>
> >       if (machine->kernel_filename) {
> >           load_kernel(machine->kernel_filename);
> > @@ -221,16 +215,25 @@ static void spike_v1_10_0_board_init(MachineState
> *machine)
> >                                        /* dtb: */
> >       };
>
> > -    /* copy in the reset vector */
> > -    copy_le32_to_phys(memmap[SPIKE_MROM].base, reset_vec,
> sizeof(reset_vec));
> > +    /* copy in the reset vector in little_endian byte order */
> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> > +    }
> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
> > +                          memmap[SPIKE_MROM].base,
> &address_space_memory);
>
> >       /* copy in the device tree */
> > +    if (s->fdt_size >= memmap[SPIKE_MROM].size - sizeof(reset_vec)) {
> > +        error_report("qemu: not enough space to store device-tree");
> > +        exit(1);
> > +    }
> >       qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
> > -    cpu_physical_memory_write(memmap[SPIKE_MROM].base +
> sizeof(reset_vec),
> > -        s->fdt, s->fdt_size);
> > +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, s->fdt_size,
> > +                          memmap[SPIKE_MROM].base + sizeof(reset_vec),
> > +                          &address_space_memory);
>
> >       /* initialize HTIF using symbols found in load_kernel */
> > -    htif_mm_init(system_memory, boot_rom, &s->soc.harts[0].env,
> serial_hds[0]);
> > +    htif_mm_init(system_memory, mask_rom, &s->soc.harts[0].env,
> serial_hds[0]);
>
> >       /* Core Local Interruptor (timer and IPI) */
> >       sifive_clint_create(memmap[SPIKE_CLINT].base,
> memmap[SPIKE_CLINT].size,
> > @@ -244,7 +247,8 @@ static void spike_v1_09_1_board_init(MachineState
> *machine)
> >       SpikeState *s = g_new0(SpikeState, 1);
> >       MemoryRegion *system_memory = get_system_memory();
> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> > +    int i;
>
> >       /* Initialize SOC */
> >       object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY);
> > @@ -264,9 +268,10 @@ static void spike_v1_09_1_board_init(MachineState
> *machine)
> >           main_mem);
>
> >       /* boot rom */
> > -    memory_region_init_ram(boot_rom, NULL, "riscv.spike.bootrom",
> > -                           0x40000, &error_fatal);
> > -    memory_region_add_subregion(system_memory, 0x0, boot_rom);
> > +    memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
> > +                           memmap[SPIKE_MROM].size, &error_fatal);
> > +    memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
> > +                                mask_rom);
>
> >       if (machine->kernel_filename) {
> >           load_kernel(machine->kernel_filename);
> > @@ -319,15 +324,20 @@ static void spike_v1_09_1_board_init(MachineState
> *machine)
> >       g_free(isa);
> >       size_t config_string_len = strlen(config_string);
>
> > -    /* copy in the reset vector */
> > -    copy_le32_to_phys(memmap[SPIKE_MROM].base, reset_vec,
> sizeof(reset_vec));
> > +    /* copy in the reset vector in little_endian byte order */
> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> > +    }
> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
> > +                          memmap[SPIKE_MROM].base,
> &address_space_memory);
>
> >       /* copy in the config string */
> > -    cpu_physical_memory_write(memmap[SPIKE_MROM].base +
> sizeof(reset_vec),
> > -        config_string, config_string_len);
> > +    rom_add_blob_fixed_as("mrom.reset", config_string,
> config_string_len,
> > +                          memmap[SPIKE_MROM].base + sizeof(reset_vec),
> > +                          &address_space_memory);
>
> >       /* initialize HTIF using symbols found in load_kernel */
> > -    htif_mm_init(system_memory, boot_rom, &s->soc.harts[0].env,
> serial_hds[0]);
> > +    htif_mm_init(system_memory, mask_rom, &s->soc.harts[0].env,
> serial_hds[0]);
>
> >       /* Core Local Interruptor (timer and IPI) */
> >       sifive_clint_create(memmap[SPIKE_CLINT].base,
> memmap[SPIKE_CLINT].size,
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index 090befe..20c509d 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -45,8 +45,8 @@ static const struct MemmapEntry {
> >       hwaddr size;
> >   } virt_memmap[] = {
> >       [VIRT_DEBUG] =    {        0x0,      0x100 },
> > -    [VIRT_MROM] =     {     0x1000,     0x2000 },
> > -    [VIRT_TEST] =     {     0x4000,     0x1000 },
> > +    [VIRT_MROM] =     {     0x1000,    0x11000 },
> > +    [VIRT_TEST] =     {   0x100000,     0x1000 },
> >       [VIRT_CLINT] =    {  0x2000000,    0x10000 },
> >       [VIRT_PLIC] =     {  0xc000000,  0x4000000 },
> >       [VIRT_UART0] =    { 0x10000000,      0x100 },
> > @@ -54,14 +54,6 @@ static const struct MemmapEntry {
> >       [VIRT_DRAM] =     { 0x80000000,        0x0 },
> >   };
>
> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t len)
> > -{
> > -    int i;
> > -    for (i = 0; i < (len >> 2); i++) {
> > -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
> > -    }
> > -}
> > -
> >   static uint64_t load_kernel(const char *kernel_filename)
> >   {
> >       uint64_t kernel_entry, kernel_high;
> > @@ -272,7 +264,7 @@ static void riscv_virt_board_init(MachineState
> *machine)
> >       RISCVVirtState *s = g_new0(RISCVVirtState, 1);
> >       MemoryRegion *system_memory = get_system_memory();
> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> >       char *plic_hart_config;
> >       size_t plic_hart_config_len;
> >       int i;
> > @@ -299,9 +291,10 @@ static void riscv_virt_board_init(MachineState
> *machine)
> >       fdt = create_fdt(s, memmap, machine->ram_size,
> machine->kernel_cmdline);
>
> >       /* boot rom */
> > -    memory_region_init_ram(boot_rom, NULL, "riscv_virt_board.bootrom",
> > -                           s->fdt_size + 0x2000, &error_fatal);
> > -    memory_region_add_subregion(system_memory, 0x0, boot_rom);
> > +    memory_region_init_rom(mask_rom, NULL, "riscv_virt_board.mrom",
> > +                           memmap[VIRT_MROM].size, &error_fatal);
> > +    memory_region_add_subregion(system_memory, memmap[VIRT_MROM].base,
> > +                                mask_rom);
>
> >       if (machine->kernel_filename) {
> >           uint64_t kernel_entry = load_kernel(machine->kernel_filename);
> > @@ -335,13 +328,22 @@ static void riscv_virt_board_init(MachineState
> *machine)
> >                                        /* dtb: */
> >       };
>
> > -    /* copy in the reset vector */
> > -    copy_le32_to_phys(memmap[VIRT_MROM].base, reset_vec,
> sizeof(reset_vec));
> > +    /* copy in the reset vector in little_endian byte order */
> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> > +    }
> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
> > +                          memmap[VIRT_MROM].base,
> &address_space_memory);
>
> >       /* copy in the device tree */
> > +    if (s->fdt_size >= memmap[VIRT_MROM].size - sizeof(reset_vec)) {
> > +        error_report("qemu: not enough space to store device-tree");
> > +        exit(1);
> > +    }
> >       qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
> > -    cpu_physical_memory_write(memmap[VIRT_MROM].base +
> sizeof(reset_vec),
> > -        s->fdt, s->fdt_size);
> > +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, s->fdt_size,
> > +                          memmap[VIRT_MROM].base + sizeof(reset_vec),
> > +                          &address_space_memory);
>
> >       /* create PLIC hart topology configuration string */
> >       plic_hart_config_len = (strlen(VIRT_PLIC_HART_CONFIG) + 1) *
> smp_cpus;
> > diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> > index 91163d6..6f2668e 100644
> > --- a/include/hw/riscv/virt.h
> > +++ b/include/hw/riscv/virt.h
> > @@ -19,6 +19,10 @@
> >   #ifndef HW_RISCV_VIRT_H
> >   #define HW_RISCV_VIRT_H
>
> > +#define TYPE_RISCV_VIRT_BOARD "riscv.virt"
> > +#define VIRT(obj) \
> > +    OBJECT_CHECK(RISCVVirtState, (obj), TYPE_RISCV_VIRT_BOARD)
> > +
>
> This should be in a seperate patch.


I'll shift that chunk into "Remove unused class definitions".


> Alistair
>
> >   typedef struct {
> >       /*< private >*/
> >       SysBusDevice parent_obj;
> > --
> > 2.7.0
>

Michael Clark April 27, 2018, 5:34 a.m. UTC | #3

On Fri, Apr 27, 2018 at 5:22 PM, Michael Clark <mjc@sifive.com> wrote:

>
>
> On Fri, Apr 27, 2018 at 4:48 AM, Alistair Francis <alistair23@gmail.com>
> wrote:
>
>> On Wed, Apr 25, 2018 at 5:03 PM Michael Clark <mjc@sifive.com> wrote:
>>
>> > The sifive_u machine already marks its ROM readonly. This fixes
>> > the remaining boards. This commit also makes all boards use
>> > mask_rom as the variable name for the ROM. This change also
>> > makes space for the maximum device tree size size and adds
>> > an explicit bounds check and error message.
>>
>> > Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
>> > Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
>> > Cc: Palmer Dabbelt <palmer@sifive.com>
>> > Cc: Alistair Francis <Alistair.Francis@wdc.com>
>> > Signed-off-by: Michael Clark <mjc@sifive.com>
>> > ---
>> >   hw/riscv/sifive_e.c     | 20 +++++++---------
>> >   hw/riscv/sifive_u.c     | 46 ++++++++++++++++++-----------------
>> >   hw/riscv/spike.c        | 64
>> ++++++++++++++++++++++++++++---------------------
>> >   hw/riscv/virt.c         | 38 +++++++++++++++--------------
>> >   include/hw/riscv/virt.h |  4 ++++
>> >   5 files changed, 93 insertions(+), 79 deletions(-)
>>
>> > diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
>> > index 39e4cb4..0c8b8e9 100644
>> > --- a/hw/riscv/sifive_e.c
>> > +++ b/hw/riscv/sifive_e.c
>> > @@ -74,14 +74,6 @@ static const struct MemmapEntry {
>> >       [SIFIVE_E_DTIM] =     { 0x80000000,     0x4000 }
>> >   };
>>
>> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t len)
>> > -{
>> > -    int i;
>> > -    for (i = 0; i < (len >> 2); i++) {
>> > -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
>> > -    }
>> > -}
>> > -
>> >   static uint64_t load_kernel(const char *kernel_filename)
>> >   {
>> >       uint64_t kernel_entry, kernel_high;
>> > @@ -112,6 +104,7 @@ static void riscv_sifive_e_init(MachineState
>> *machine)
>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
>> >       MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
>> >       MemoryRegion *xip_mem = g_new(MemoryRegion, 1);
>> > +    int i;
>>
>> >       /* Initialize SOC */
>> >       object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY);
>> > @@ -131,7 +124,7 @@ static void riscv_sifive_e_init(MachineState
>> *machine)
>> >           memmap[SIFIVE_E_DTIM].base, main_mem);
>>
>> >       /* Mask ROM */
>> > -    memory_region_init_ram(mask_rom, NULL, "riscv.sifive.e.mrom",
>> > +    memory_region_init_rom(mask_rom, NULL, "riscv.sifive.e.mrom",
>> >           memmap[SIFIVE_E_MROM].size, &error_fatal);
>> >       memory_region_add_subregion(sys_mem,
>> >           memmap[SIFIVE_E_MROM].base, mask_rom);
>> > @@ -185,9 +178,12 @@ static void riscv_sifive_e_init(MachineState
>> *machine)
>> >           0x00028067,        /* 0x1004: jr      t0 */
>> >       };
>>
>> > -    /* copy in the reset vector */
>> > -    copy_le32_to_phys(memmap[SIFIVE_E_MROM].base, reset_vec,
>> sizeof(reset_vec));
>> > -    memory_region_set_readonly(mask_rom, true);
>> > +    /* copy in the reset vector in little_endian byte order */
>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
>> > +    }
>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
>> > +                          memmap[SIFIVE_E_MROM].base,
>> &address_space_memory);
>>
>> >       if (machine->kernel_filename) {
>> >           load_kernel(machine->kernel_filename);
>> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
>> > index 115618b..11ba4c3 100644
>> > --- a/hw/riscv/sifive_u.c
>> > +++ b/hw/riscv/sifive_u.c
>> > @@ -52,7 +52,7 @@ static const struct MemmapEntry {
>> >       hwaddr size;
>> >   } sifive_u_memmap[] = {
>> >       [SIFIVE_U_DEBUG] =    {        0x0,      0x100 },
>> > -    [SIFIVE_U_MROM] =     {     0x1000,     0x2000 },
>> > +    [SIFIVE_U_MROM] =     {     0x1000,    0x11000 },
>> >       [SIFIVE_U_CLINT] =    {  0x2000000,    0x10000 },
>> >       [SIFIVE_U_PLIC] =     {  0xc000000,  0x4000000 },
>> >       [SIFIVE_U_UART0] =    { 0x10013000,     0x1000 },
>> > @@ -60,14 +60,6 @@ static const struct MemmapEntry {
>> >       [SIFIVE_U_DRAM] =     { 0x80000000,        0x0 },
>> >   };
>>
>> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t len)
>> > -{
>> > -    int i;
>> > -    for (i = 0; i < (len >> 2); i++) {
>> > -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
>> > -    }
>> > -}
>> > -
>> >   static uint64_t load_kernel(const char *kernel_filename)
>> >   {
>> >       uint64_t kernel_entry, kernel_high;
>> > @@ -221,9 +213,10 @@ static void riscv_sifive_u_init(MachineState
>> *machine)
>> >       const struct MemmapEntry *memmap = sifive_u_memmap;
>>
>> >       SiFiveUState *s = g_new0(SiFiveUState, 1);
>> > -    MemoryRegion *sys_memory = get_system_memory();
>> > +    MemoryRegion *system_memory = get_system_memory();
>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
>> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
>> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
>> > +    int i;
>>
>> >       /* Initialize SOC */
>> >       object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY);
>> > @@ -239,17 +232,17 @@ static void riscv_sifive_u_init(MachineState
>> *machine)
>> >       /* register RAM */
>> >       memory_region_init_ram(main_mem, NULL, "riscv.sifive.u.ram",
>> >                              machine->ram_size, &error_fatal);
>> > -    memory_region_add_subregion(sys_memory,
>> memmap[SIFIVE_U_DRAM].base,
>> > +    memory_region_add_subregion(system_memory,
>> memmap[SIFIVE_U_DRAM].base,
>> >           main_mem);
>>
>> >       /* create device tree */
>> >       create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline);
>>
>> >       /* boot rom */
>> > -    memory_region_init_ram(boot_rom, NULL, "riscv.sifive.u.mrom",
>> > -                           memmap[SIFIVE_U_MROM].base, &error_fatal);
>> > -    memory_region_set_readonly(boot_rom, true);
>> > -    memory_region_add_subregion(sys_memory, 0x0, boot_rom);
>> > +    memory_region_init_rom(mask_rom, NULL, "riscv.sifive.u.mrom",
>> > +                           memmap[SIFIVE_U_MROM].size, &error_fatal);
>> > +    memory_region_add_subregion(system_memory,
>> memmap[SIFIVE_U_MROM].base,
>> > +                                mask_rom);
>>
>> >       if (machine->kernel_filename) {
>> >           load_kernel(machine->kernel_filename);
>> > @@ -272,13 +265,22 @@ static void riscv_sifive_u_init(MachineState
>> *machine)
>> >                                          /* dtb: */
>> >       };
>>
>> > -    /* copy in the reset vector */
>> > -    copy_le32_to_phys(memmap[SIFIVE_U_MROM].base, reset_vec,
>> sizeof(reset_vec));
>> > +    /* copy in the reset vector in little_endian byte order */
>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
>> > +    }
>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
>> > +                          memmap[SIFIVE_U_MROM].base,
>> &address_space_memory);
>>
>> >       /* copy in the device tree */
>> > +    if (s->fdt_size >= memmap[SIFIVE_U_MROM].size - sizeof(reset_vec))
>> {
>> > +        error_report("qemu: not enough space to store device-tree");
>> > +        exit(1);
>> > +    }
>> >       qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
>> > -    cpu_physical_memory_write(memmap[SIFIVE_U_MROM].base +
>> > -        sizeof(reset_vec), s->fdt, s->fdt_size);
>> > +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, s->fdt_size,
>> > +                          memmap[SIFIVE_U_MROM].base +
>> sizeof(reset_vec),
>> > +                          &address_space_memory);
>>
>> >       /* MMIO */
>> >       s->plic = sifive_plic_create(memmap[SIFIVE_U_PLIC].base,
>> > @@ -292,9 +294,9 @@ static void riscv_sifive_u_init(MachineState
>> *machine)
>> >           SIFIVE_U_PLIC_CONTEXT_BASE,
>> >           SIFIVE_U_PLIC_CONTEXT_STRIDE,
>> >           memmap[SIFIVE_U_PLIC].size);
>> > -    sifive_uart_create(sys_memory, memmap[SIFIVE_U_UART0].base,
>> > +    sifive_uart_create(system_memory, memmap[SIFIVE_U_UART0].base,
>> >           serial_hds[0], SIFIVE_PLIC(s->plic)->irqs[SIF
>> IVE_U_UART0_IRQ]);
>> > -    /* sifive_uart_create(sys_memory, memmap[SIFIVE_U_UART1].base,
>> > +    /* sifive_uart_create(system_memory, memmap[SIFIVE_U_UART1].base,
>> >           serial_hds[1], SIFIVE_PLIC(s->plic)->irqs[SIF
>> IVE_U_UART1_IRQ]);
>> */
>> >       sifive_clint_create(memmap[SIFIVE_U_CLINT].base,
>> >           memmap[SIFIVE_U_CLINT].size, smp_cpus,
>> > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
>> > index 3f6bd0a..d1dbe6b 100644
>> > --- a/hw/riscv/spike.c
>> > +++ b/hw/riscv/spike.c
>> > @@ -46,19 +46,11 @@ static const struct MemmapEntry {
>> >       hwaddr base;
>> >       hwaddr size;
>> >   } spike_memmap[] = {
>> > -    [SPIKE_MROM] =     {     0x1000,     0x2000 },
>> > +    [SPIKE_MROM] =     {     0x1000,    0x11000 },
>> >       [SPIKE_CLINT] =    {  0x2000000,    0x10000 },
>> >       [SPIKE_DRAM] =     { 0x80000000,        0x0 },
>> >   };
>>
>> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t len)
>> > -{
>> > -    int i;
>> > -    for (i = 0; i < (len >> 2); i++) {
>> > -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
>> > -    }
>> > -}
>> > -
>> >   static uint64_t load_kernel(const char *kernel_filename)
>> >   {
>> >       uint64_t kernel_entry, kernel_high;
>> > @@ -173,7 +165,8 @@ static void spike_v1_10_0_board_init(MachineState
>> *machine)
>> >       SpikeState *s = g_new0(SpikeState, 1);
>> >       MemoryRegion *system_memory = get_system_memory();
>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
>> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
>> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
>> > +    int i;
>>
>> >       /* Initialize SOC */
>> >       object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY);
>> > @@ -196,9 +189,10 @@ static void spike_v1_10_0_board_init(MachineState
>> *machine)
>> >       create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline);
>>
>> >       /* boot rom */
>> > -    memory_region_init_ram(boot_rom, NULL, "riscv.spike.bootrom",
>> > -                           s->fdt_size + 0x2000, &error_fatal);
>> > -    memory_region_add_subregion(system_memory, 0x0, boot_rom);
>> > +    memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
>> > +                           memmap[SPIKE_MROM].size, &error_fatal);
>> > +    memory_region_add_subregion(system_memory,
>> memmap[SPIKE_MROM].base,
>> > +                                mask_rom);
>>
>> >       if (machine->kernel_filename) {
>> >           load_kernel(machine->kernel_filename);
>> > @@ -221,16 +215,25 @@ static void spike_v1_10_0_board_init(MachineState
>> *machine)
>> >                                        /* dtb: */
>> >       };
>>
>> > -    /* copy in the reset vector */
>> > -    copy_le32_to_phys(memmap[SPIKE_MROM].base, reset_vec,
>> sizeof(reset_vec));
>> > +    /* copy in the reset vector in little_endian byte order */
>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
>> > +    }
>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
>> > +                          memmap[SPIKE_MROM].base,
>> &address_space_memory);
>>
>> >       /* copy in the device tree */
>> > +    if (s->fdt_size >= memmap[SPIKE_MROM].size - sizeof(reset_vec)) {
>> > +        error_report("qemu: not enough space to store device-tree");
>> > +        exit(1);
>> > +    }
>> >       qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
>> > -    cpu_physical_memory_write(memmap[SPIKE_MROM].base +
>> sizeof(reset_vec),
>> > -        s->fdt, s->fdt_size);
>> > +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, s->fdt_size,
>> > +                          memmap[SPIKE_MROM].base + sizeof(reset_vec),
>> > +                          &address_space_memory);
>>
>> >       /* initialize HTIF using symbols found in load_kernel */
>> > -    htif_mm_init(system_memory, boot_rom, &s->soc.harts[0].env,
>> serial_hds[0]);
>> > +    htif_mm_init(system_memory, mask_rom, &s->soc.harts[0].env,
>> serial_hds[0]);
>>
>> >       /* Core Local Interruptor (timer and IPI) */
>> >       sifive_clint_create(memmap[SPIKE_CLINT].base,
>> memmap[SPIKE_CLINT].size,
>> > @@ -244,7 +247,8 @@ static void spike_v1_09_1_board_init(MachineState
>> *machine)
>> >       SpikeState *s = g_new0(SpikeState, 1);
>> >       MemoryRegion *system_memory = get_system_memory();
>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
>> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
>> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
>> > +    int i;
>>
>> >       /* Initialize SOC */
>> >       object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY);
>> > @@ -264,9 +268,10 @@ static void spike_v1_09_1_board_init(MachineState
>> *machine)
>> >           main_mem);
>>
>> >       /* boot rom */
>> > -    memory_region_init_ram(boot_rom, NULL, "riscv.spike.bootrom",
>> > -                           0x40000, &error_fatal);
>> > -    memory_region_add_subregion(system_memory, 0x0, boot_rom);
>> > +    memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
>> > +                           memmap[SPIKE_MROM].size, &error_fatal);
>> > +    memory_region_add_subregion(system_memory,
>> memmap[SPIKE_MROM].base,
>> > +                                mask_rom);
>>
>> >       if (machine->kernel_filename) {
>> >           load_kernel(machine->kernel_filename);
>> > @@ -319,15 +324,20 @@ static void spike_v1_09_1_board_init(MachineState
>> *machine)
>> >       g_free(isa);
>> >       size_t config_string_len = strlen(config_string);
>>
>> > -    /* copy in the reset vector */
>> > -    copy_le32_to_phys(memmap[SPIKE_MROM].base, reset_vec,
>> sizeof(reset_vec));
>> > +    /* copy in the reset vector in little_endian byte order */
>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
>> > +    }
>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
>> > +                          memmap[SPIKE_MROM].base,
>> &address_space_memory);
>>
>> >       /* copy in the config string */
>> > -    cpu_physical_memory_write(memmap[SPIKE_MROM].base +
>> sizeof(reset_vec),
>> > -        config_string, config_string_len);
>> > +    rom_add_blob_fixed_as("mrom.reset", config_string,
>> config_string_len,
>> > +                          memmap[SPIKE_MROM].base + sizeof(reset_vec),
>> > +                          &address_space_memory);
>>
>> >       /* initialize HTIF using symbols found in load_kernel */
>> > -    htif_mm_init(system_memory, boot_rom, &s->soc.harts[0].env,
>> serial_hds[0]);
>> > +    htif_mm_init(system_memory, mask_rom, &s->soc.harts[0].env,
>> serial_hds[0]);
>>
>> >       /* Core Local Interruptor (timer and IPI) */
>> >       sifive_clint_create(memmap[SPIKE_CLINT].base,
>> memmap[SPIKE_CLINT].size,
>> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> > index 090befe..20c509d 100644
>> > --- a/hw/riscv/virt.c
>> > +++ b/hw/riscv/virt.c
>> > @@ -45,8 +45,8 @@ static const struct MemmapEntry {
>> >       hwaddr size;
>> >   } virt_memmap[] = {
>> >       [VIRT_DEBUG] =    {        0x0,      0x100 },
>> > -    [VIRT_MROM] =     {     0x1000,     0x2000 },
>> > -    [VIRT_TEST] =     {     0x4000,     0x1000 },
>> > +    [VIRT_MROM] =     {     0x1000,    0x11000 },
>> > +    [VIRT_TEST] =     {   0x100000,     0x1000 },
>> >       [VIRT_CLINT] =    {  0x2000000,    0x10000 },
>> >       [VIRT_PLIC] =     {  0xc000000,  0x4000000 },
>> >       [VIRT_UART0] =    { 0x10000000,      0x100 },
>> > @@ -54,14 +54,6 @@ static const struct MemmapEntry {
>> >       [VIRT_DRAM] =     { 0x80000000,        0x0 },
>> >   };
>>
>> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t len)
>> > -{
>> > -    int i;
>> > -    for (i = 0; i < (len >> 2); i++) {
>> > -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
>> > -    }
>> > -}
>> > -
>> >   static uint64_t load_kernel(const char *kernel_filename)
>> >   {
>> >       uint64_t kernel_entry, kernel_high;
>> > @@ -272,7 +264,7 @@ static void riscv_virt_board_init(MachineState
>> *machine)
>> >       RISCVVirtState *s = g_new0(RISCVVirtState, 1);
>> >       MemoryRegion *system_memory = get_system_memory();
>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
>> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
>> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
>> >       char *plic_hart_config;
>> >       size_t plic_hart_config_len;
>> >       int i;
>> > @@ -299,9 +291,10 @@ static void riscv_virt_board_init(MachineState
>> *machine)
>> >       fdt = create_fdt(s, memmap, machine->ram_size,
>> machine->kernel_cmdline);
>>
>> >       /* boot rom */
>> > -    memory_region_init_ram(boot_rom, NULL, "riscv_virt_board.bootrom",
>> > -                           s->fdt_size + 0x2000, &error_fatal);
>> > -    memory_region_add_subregion(system_memory, 0x0, boot_rom);
>> > +    memory_region_init_rom(mask_rom, NULL, "riscv_virt_board.mrom",
>> > +                           memmap[VIRT_MROM].size, &error_fatal);
>> > +    memory_region_add_subregion(system_memory, memmap[VIRT_MROM].base,
>> > +                                mask_rom);
>>
>> >       if (machine->kernel_filename) {
>> >           uint64_t kernel_entry = load_kernel(machine->kernel_fi
>> lename);
>> > @@ -335,13 +328,22 @@ static void riscv_virt_board_init(MachineState
>> *machine)
>> >                                        /* dtb: */
>> >       };
>>
>> > -    /* copy in the reset vector */
>> > -    copy_le32_to_phys(memmap[VIRT_MROM].base, reset_vec,
>> sizeof(reset_vec));
>> > +    /* copy in the reset vector in little_endian byte order */
>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
>> > +    }
>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
>> > +                          memmap[VIRT_MROM].base,
>> &address_space_memory);
>>
>> >       /* copy in the device tree */
>> > +    if (s->fdt_size >= memmap[VIRT_MROM].size - sizeof(reset_vec)) {
>> > +        error_report("qemu: not enough space to store device-tree");
>> > +        exit(1);
>> > +    }
>> >       qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
>> > -    cpu_physical_memory_write(memmap[VIRT_MROM].base +
>> sizeof(reset_vec),
>> > -        s->fdt, s->fdt_size);
>> > +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, s->fdt_size,
>> > +                          memmap[VIRT_MROM].base + sizeof(reset_vec),
>> > +                          &address_space_memory);
>>
>> >       /* create PLIC hart topology configuration string */
>> >       plic_hart_config_len = (strlen(VIRT_PLIC_HART_CONFIG) + 1) *
>> smp_cpus;
>> > diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
>> > index 91163d6..6f2668e 100644
>> > --- a/include/hw/riscv/virt.h
>> > +++ b/include/hw/riscv/virt.h
>> > @@ -19,6 +19,10 @@
>> >   #ifndef HW_RISCV_VIRT_H
>> >   #define HW_RISCV_VIRT_H
>>
>> > +#define TYPE_RISCV_VIRT_BOARD "riscv.virt"
>> > +#define VIRT(obj) \
>> > +    OBJECT_CHECK(RISCVVirtState, (obj), TYPE_RISCV_VIRT_BOARD)
>> > +
>>
>> This should be in a seperate patch.
>
>
> I'll shift that chunk into "Remove unused class definitions".
>

Actually we to need to drop this chunk as the unused check macros were
removed from machine state structs in "Remove unused class definitions".
Somehow the chunk made it into this patch. Likely a rebase issue.

It's probably best that we add what we need back in the QOM SOC refactor
on-top of the this series, or at least after the first set of patches are
merged...

I think that's what you were planning to do anyway.

Thanks.
Michael.

Alistair Francis April 27, 2018, 4:17 p.m. UTC | #4

On Thu, Apr 26, 2018 at 10:34 PM Michael Clark <mjc@sifive.com> wrote:



> On Fri, Apr 27, 2018 at 5:22 PM, Michael Clark <mjc@sifive.com> wrote:



>> On Fri, Apr 27, 2018 at 4:48 AM, Alistair Francis <alistair23@gmail.com>
wrote:

>>> On Wed, Apr 25, 2018 at 5:03 PM Michael Clark <mjc@sifive.com> wrote:

>>> > The sifive_u machine already marks its ROM readonly. This fixes
>>> > the remaining boards. This commit also makes all boards use
>>> > mask_rom as the variable name for the ROM. This change also
>>> > makes space for the maximum device tree size size and adds
>>> > an explicit bounds check and error message.

>>> > Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
>>> > Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
>>> > Cc: Palmer Dabbelt <palmer@sifive.com>
>>> > Cc: Alistair Francis <Alistair.Francis@wdc.com>
>>> > Signed-off-by: Michael Clark <mjc@sifive.com>
>>> > ---
>>> >   hw/riscv/sifive_e.c     | 20 +++++++---------
>>> >   hw/riscv/sifive_u.c     | 46 ++++++++++++++++++-----------------
>>> >   hw/riscv/spike.c        | 64
>>> ++++++++++++++++++++++++++++---------------------
>>> >   hw/riscv/virt.c         | 38 +++++++++++++++--------------
>>> >   include/hw/riscv/virt.h |  4 ++++
>>> >   5 files changed, 93 insertions(+), 79 deletions(-)

>>> > diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
>>> > index 39e4cb4..0c8b8e9 100644
>>> > --- a/hw/riscv/sifive_e.c
>>> > +++ b/hw/riscv/sifive_e.c
>>> > @@ -74,14 +74,6 @@ static const struct MemmapEntry {
>>> >       [SIFIVE_E_DTIM] =     { 0x80000000,     0x4000 }
>>> >   };

>>> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t len)
>>> > -{
>>> > -    int i;
>>> > -    for (i = 0; i < (len >> 2); i++) {
>>> > -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
>>> > -    }
>>> > -}
>>> > -
>>> >   static uint64_t load_kernel(const char *kernel_filename)
>>> >   {
>>> >       uint64_t kernel_entry, kernel_high;
>>> > @@ -112,6 +104,7 @@ static void riscv_sifive_e_init(MachineState
*machine)
>>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
>>> >       MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
>>> >       MemoryRegion *xip_mem = g_new(MemoryRegion, 1);
>>> > +    int i;

>>> >       /* Initialize SOC */
>>> >       object_initialize(&s->soc, sizeof(s->soc),
TYPE_RISCV_HART_ARRAY);
>>> > @@ -131,7 +124,7 @@ static void riscv_sifive_e_init(MachineState
*machine)
>>> >           memmap[SIFIVE_E_DTIM].base, main_mem);

>>> >       /* Mask ROM */
>>> > -    memory_region_init_ram(mask_rom, NULL, "riscv.sifive.e.mrom",
>>> > +    memory_region_init_rom(mask_rom, NULL, "riscv.sifive.e.mrom",
>>> >           memmap[SIFIVE_E_MROM].size, &error_fatal);
>>> >       memory_region_add_subregion(sys_mem,
>>> >           memmap[SIFIVE_E_MROM].base, mask_rom);
>>> > @@ -185,9 +178,12 @@ static void riscv_sifive_e_init(MachineState
>>> *machine)
>>> >           0x00028067,        /* 0x1004: jr      t0 */
>>> >       };

>>> > -    /* copy in the reset vector */
>>> > -    copy_le32_to_phys(memmap[SIFIVE_E_MROM].base, reset_vec,
>>> sizeof(reset_vec));
>>> > -    memory_region_set_readonly(mask_rom, true);
>>> > +    /* copy in the reset vector in little_endian byte order */
>>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
>>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
>>> > +    }
>>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
>>> > +                          memmap[SIFIVE_E_MROM].base,
>>> &address_space_memory);

>>> >       if (machine->kernel_filename) {
>>> >           load_kernel(machine->kernel_filename);
>>> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
>>> > index 115618b..11ba4c3 100644
>>> > --- a/hw/riscv/sifive_u.c
>>> > +++ b/hw/riscv/sifive_u.c
>>> > @@ -52,7 +52,7 @@ static const struct MemmapEntry {
>>> >       hwaddr size;
>>> >   } sifive_u_memmap[] = {
>>> >       [SIFIVE_U_DEBUG] =    {        0x0,      0x100 },
>>> > -    [SIFIVE_U_MROM] =     {     0x1000,     0x2000 },
>>> > +    [SIFIVE_U_MROM] =     {     0x1000,    0x11000 },
>>> >       [SIFIVE_U_CLINT] =    {  0x2000000,    0x10000 },
>>> >       [SIFIVE_U_PLIC] =     {  0xc000000,  0x4000000 },
>>> >       [SIFIVE_U_UART0] =    { 0x10013000,     0x1000 },
>>> > @@ -60,14 +60,6 @@ static const struct MemmapEntry {
>>> >       [SIFIVE_U_DRAM] =     { 0x80000000,        0x0 },
>>> >   };

>>> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t len)
>>> > -{
>>> > -    int i;
>>> > -    for (i = 0; i < (len >> 2); i++) {
>>> > -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
>>> > -    }
>>> > -}
>>> > -
>>> >   static uint64_t load_kernel(const char *kernel_filename)
>>> >   {
>>> >       uint64_t kernel_entry, kernel_high;
>>> > @@ -221,9 +213,10 @@ static void riscv_sifive_u_init(MachineState
>>> *machine)
>>> >       const struct MemmapEntry *memmap = sifive_u_memmap;

>>> >       SiFiveUState *s = g_new0(SiFiveUState, 1);
>>> > -    MemoryRegion *sys_memory = get_system_memory();
>>> > +    MemoryRegion *system_memory = get_system_memory();
>>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
>>> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
>>> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
>>> > +    int i;

>>> >       /* Initialize SOC */
>>> >       object_initialize(&s->soc, sizeof(s->soc),
TYPE_RISCV_HART_ARRAY);
>>> > @@ -239,17 +232,17 @@ static void riscv_sifive_u_init(MachineState
>>> *machine)
>>> >       /* register RAM */
>>> >       memory_region_init_ram(main_mem, NULL, "riscv.sifive.u.ram",
>>> >                              machine->ram_size, &error_fatal);
>>> > -    memory_region_add_subregion(sys_memory,
memmap[SIFIVE_U_DRAM].base,
>>> > +    memory_region_add_subregion(system_memory,
>>> memmap[SIFIVE_U_DRAM].base,
>>> >           main_mem);

>>> >       /* create device tree */
>>> >       create_fdt(s, memmap, machine->ram_size,
machine->kernel_cmdline);

>>> >       /* boot rom */
>>> > -    memory_region_init_ram(boot_rom, NULL, "riscv.sifive.u.mrom",
>>> > -                           memmap[SIFIVE_U_MROM].base, &error_fatal);
>>> > -    memory_region_set_readonly(boot_rom, true);
>>> > -    memory_region_add_subregion(sys_memory, 0x0, boot_rom);
>>> > +    memory_region_init_rom(mask_rom, NULL, "riscv.sifive.u.mrom",
>>> > +                           memmap[SIFIVE_U_MROM].size, &error_fatal);
>>> > +    memory_region_add_subregion(system_memory,
>>> memmap[SIFIVE_U_MROM].base,
>>> > +                                mask_rom);

>>> >       if (machine->kernel_filename) {
>>> >           load_kernel(machine->kernel_filename);
>>> > @@ -272,13 +265,22 @@ static void riscv_sifive_u_init(MachineState
>>> *machine)
>>> >                                          /* dtb: */
>>> >       };

>>> > -    /* copy in the reset vector */
>>> > -    copy_le32_to_phys(memmap[SIFIVE_U_MROM].base, reset_vec,
>>> sizeof(reset_vec));
>>> > +    /* copy in the reset vector in little_endian byte order */
>>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
>>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
>>> > +    }
>>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
>>> > +                          memmap[SIFIVE_U_MROM].base,
>>> &address_space_memory);

>>> >       /* copy in the device tree */
>>> > +    if (s->fdt_size >= memmap[SIFIVE_U_MROM].size -
sizeof(reset_vec)) {
>>> > +        error_report("qemu: not enough space to store device-tree");
>>> > +        exit(1);
>>> > +    }
>>> >       qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
>>> > -    cpu_physical_memory_write(memmap[SIFIVE_U_MROM].base +
>>> > -        sizeof(reset_vec), s->fdt, s->fdt_size);
>>> > +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, s->fdt_size,
>>> > +                          memmap[SIFIVE_U_MROM].base +
sizeof(reset_vec),
>>> > +                          &address_space_memory);

>>> >       /* MMIO */
>>> >       s->plic = sifive_plic_create(memmap[SIFIVE_U_PLIC].base,
>>> > @@ -292,9 +294,9 @@ static void riscv_sifive_u_init(MachineState
*machine)
>>> >           SIFIVE_U_PLIC_CONTEXT_BASE,
>>> >           SIFIVE_U_PLIC_CONTEXT_STRIDE,
>>> >           memmap[SIFIVE_U_PLIC].size);
>>> > -    sifive_uart_create(sys_memory, memmap[SIFIVE_U_UART0].base,
>>> > +    sifive_uart_create(system_memory, memmap[SIFIVE_U_UART0].base,
>>> >           serial_hds[0],
SIFIVE_PLIC(s->plic)->irqs[SIFIVE_U_UART0_IRQ]);
>>> > -    /* sifive_uart_create(sys_memory, memmap[SIFIVE_U_UART1].base,
>>> > +    /* sifive_uart_create(system_memory, memmap[SIFIVE_U_UART1].base,
>>> >           serial_hds[1],
SIFIVE_PLIC(s->plic)->irqs[SIFIVE_U_UART1_IRQ]);
>>> */
>>> >       sifive_clint_create(memmap[SIFIVE_U_CLINT].base,
>>> >           memmap[SIFIVE_U_CLINT].size, smp_cpus,
>>> > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
>>> > index 3f6bd0a..d1dbe6b 100644
>>> > --- a/hw/riscv/spike.c
>>> > +++ b/hw/riscv/spike.c
>>> > @@ -46,19 +46,11 @@ static const struct MemmapEntry {
>>> >       hwaddr base;
>>> >       hwaddr size;
>>> >   } spike_memmap[] = {
>>> > -    [SPIKE_MROM] =     {     0x1000,     0x2000 },
>>> > +    [SPIKE_MROM] =     {     0x1000,    0x11000 },
>>> >       [SPIKE_CLINT] =    {  0x2000000,    0x10000 },
>>> >       [SPIKE_DRAM] =     { 0x80000000,        0x0 },
>>> >   };

>>> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t len)
>>> > -{
>>> > -    int i;
>>> > -    for (i = 0; i < (len >> 2); i++) {
>>> > -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
>>> > -    }
>>> > -}
>>> > -
>>> >   static uint64_t load_kernel(const char *kernel_filename)
>>> >   {
>>> >       uint64_t kernel_entry, kernel_high;
>>> > @@ -173,7 +165,8 @@ static void spike_v1_10_0_board_init(MachineState
>>> *machine)
>>> >       SpikeState *s = g_new0(SpikeState, 1);
>>> >       MemoryRegion *system_memory = get_system_memory();
>>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
>>> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
>>> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
>>> > +    int i;

>>> >       /* Initialize SOC */
>>> >       object_initialize(&s->soc, sizeof(s->soc),
TYPE_RISCV_HART_ARRAY);
>>> > @@ -196,9 +189,10 @@ static void spike_v1_10_0_board_init(MachineState
>>> *machine)
>>> >       create_fdt(s, memmap, machine->ram_size,
machine->kernel_cmdline);

>>> >       /* boot rom */
>>> > -    memory_region_init_ram(boot_rom, NULL, "riscv.spike.bootrom",
>>> > -                           s->fdt_size + 0x2000, &error_fatal);
>>> > -    memory_region_add_subregion(system_memory, 0x0, boot_rom);
>>> > +    memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
>>> > +                           memmap[SPIKE_MROM].size, &error_fatal);
>>> > +    memory_region_add_subregion(system_memory,
memmap[SPIKE_MROM].base,
>>> > +                                mask_rom);

>>> >       if (machine->kernel_filename) {
>>> >           load_kernel(machine->kernel_filename);
>>> > @@ -221,16 +215,25 @@ static void
spike_v1_10_0_board_init(MachineState
>>> *machine)
>>> >                                        /* dtb: */
>>> >       };

>>> > -    /* copy in the reset vector */
>>> > -    copy_le32_to_phys(memmap[SPIKE_MROM].base, reset_vec,
>>> sizeof(reset_vec));
>>> > +    /* copy in the reset vector in little_endian byte order */
>>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
>>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
>>> > +    }
>>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
>>> > +                          memmap[SPIKE_MROM].base,
>>> &address_space_memory);

>>> >       /* copy in the device tree */
>>> > +    if (s->fdt_size >= memmap[SPIKE_MROM].size - sizeof(reset_vec)) {
>>> > +        error_report("qemu: not enough space to store device-tree");
>>> > +        exit(1);
>>> > +    }
>>> >       qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
>>> > -    cpu_physical_memory_write(memmap[SPIKE_MROM].base +
>>> sizeof(reset_vec),
>>> > -        s->fdt, s->fdt_size);
>>> > +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, s->fdt_size,
>>> > +                          memmap[SPIKE_MROM].base +
sizeof(reset_vec),
>>> > +                          &address_space_memory);

>>> >       /* initialize HTIF using symbols found in load_kernel */
>>> > -    htif_mm_init(system_memory, boot_rom, &s->soc.harts[0].env,
>>> serial_hds[0]);
>>> > +    htif_mm_init(system_memory, mask_rom, &s->soc.harts[0].env,
>>> serial_hds[0]);

>>> >       /* Core Local Interruptor (timer and IPI) */
>>> >       sifive_clint_create(memmap[SPIKE_CLINT].base,
>>> memmap[SPIKE_CLINT].size,
>>> > @@ -244,7 +247,8 @@ static void spike_v1_09_1_board_init(MachineState
>>> *machine)
>>> >       SpikeState *s = g_new0(SpikeState, 1);
>>> >       MemoryRegion *system_memory = get_system_memory();
>>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
>>> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
>>> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
>>> > +    int i;

>>> >       /* Initialize SOC */
>>> >       object_initialize(&s->soc, sizeof(s->soc),
TYPE_RISCV_HART_ARRAY);
>>> > @@ -264,9 +268,10 @@ static void spike_v1_09_1_board_init(MachineState
>>> *machine)
>>> >           main_mem);

>>> >       /* boot rom */
>>> > -    memory_region_init_ram(boot_rom, NULL, "riscv.spike.bootrom",
>>> > -                           0x40000, &error_fatal);
>>> > -    memory_region_add_subregion(system_memory, 0x0, boot_rom);
>>> > +    memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
>>> > +                           memmap[SPIKE_MROM].size, &error_fatal);
>>> > +    memory_region_add_subregion(system_memory,
memmap[SPIKE_MROM].base,
>>> > +                                mask_rom);

>>> >       if (machine->kernel_filename) {
>>> >           load_kernel(machine->kernel_filename);
>>> > @@ -319,15 +324,20 @@ static void
spike_v1_09_1_board_init(MachineState
>>> *machine)
>>> >       g_free(isa);
>>> >       size_t config_string_len = strlen(config_string);

>>> > -    /* copy in the reset vector */
>>> > -    copy_le32_to_phys(memmap[SPIKE_MROM].base, reset_vec,
>>> sizeof(reset_vec));
>>> > +    /* copy in the reset vector in little_endian byte order */
>>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
>>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
>>> > +    }
>>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
>>> > +                          memmap[SPIKE_MROM].base,
>>> &address_space_memory);

>>> >       /* copy in the config string */
>>> > -    cpu_physical_memory_write(memmap[SPIKE_MROM].base +
>>> sizeof(reset_vec),
>>> > -        config_string, config_string_len);
>>> > +    rom_add_blob_fixed_as("mrom.reset", config_string,
config_string_len,
>>> > +                          memmap[SPIKE_MROM].base +
sizeof(reset_vec),
>>> > +                          &address_space_memory);

>>> >       /* initialize HTIF using symbols found in load_kernel */
>>> > -    htif_mm_init(system_memory, boot_rom, &s->soc.harts[0].env,
>>> serial_hds[0]);
>>> > +    htif_mm_init(system_memory, mask_rom, &s->soc.harts[0].env,
>>> serial_hds[0]);

>>> >       /* Core Local Interruptor (timer and IPI) */
>>> >       sifive_clint_create(memmap[SPIKE_CLINT].base,
>>> memmap[SPIKE_CLINT].size,
>>> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>>> > index 090befe..20c509d 100644
>>> > --- a/hw/riscv/virt.c
>>> > +++ b/hw/riscv/virt.c
>>> > @@ -45,8 +45,8 @@ static const struct MemmapEntry {
>>> >       hwaddr size;
>>> >   } virt_memmap[] = {
>>> >       [VIRT_DEBUG] =    {        0x0,      0x100 },
>>> > -    [VIRT_MROM] =     {     0x1000,     0x2000 },
>>> > -    [VIRT_TEST] =     {     0x4000,     0x1000 },
>>> > +    [VIRT_MROM] =     {     0x1000,    0x11000 },
>>> > +    [VIRT_TEST] =     {   0x100000,     0x1000 },
>>> >       [VIRT_CLINT] =    {  0x2000000,    0x10000 },
>>> >       [VIRT_PLIC] =     {  0xc000000,  0x4000000 },
>>> >       [VIRT_UART0] =    { 0x10000000,      0x100 },
>>> > @@ -54,14 +54,6 @@ static const struct MemmapEntry {
>>> >       [VIRT_DRAM] =     { 0x80000000,        0x0 },
>>> >   };

>>> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t len)
>>> > -{
>>> > -    int i;
>>> > -    for (i = 0; i < (len >> 2); i++) {
>>> > -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
>>> > -    }
>>> > -}
>>> > -
>>> >   static uint64_t load_kernel(const char *kernel_filename)
>>> >   {
>>> >       uint64_t kernel_entry, kernel_high;
>>> > @@ -272,7 +264,7 @@ static void riscv_virt_board_init(MachineState
>>> *machine)
>>> >       RISCVVirtState *s = g_new0(RISCVVirtState, 1);
>>> >       MemoryRegion *system_memory = get_system_memory();
>>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
>>> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
>>> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
>>> >       char *plic_hart_config;
>>> >       size_t plic_hart_config_len;
>>> >       int i;
>>> > @@ -299,9 +291,10 @@ static void riscv_virt_board_init(MachineState
>>> *machine)
>>> >       fdt = create_fdt(s, memmap, machine->ram_size,
>>> machine->kernel_cmdline);

>>> >       /* boot rom */
>>> > -    memory_region_init_ram(boot_rom, NULL,
"riscv_virt_board.bootrom",
>>> > -                           s->fdt_size + 0x2000, &error_fatal);
>>> > -    memory_region_add_subregion(system_memory, 0x0, boot_rom);
>>> > +    memory_region_init_rom(mask_rom, NULL, "riscv_virt_board.mrom",
>>> > +                           memmap[VIRT_MROM].size, &error_fatal);
>>> > +    memory_region_add_subregion(system_memory,
memmap[VIRT_MROM].base,
>>> > +                                mask_rom);

>>> >       if (machine->kernel_filename) {
>>> >           uint64_t kernel_entry =
load_kernel(machine->kernel_filename);
>>> > @@ -335,13 +328,22 @@ static void riscv_virt_board_init(MachineState
>>> *machine)
>>> >                                        /* dtb: */
>>> >       };

>>> > -    /* copy in the reset vector */
>>> > -    copy_le32_to_phys(memmap[VIRT_MROM].base, reset_vec,
>>> sizeof(reset_vec));
>>> > +    /* copy in the reset vector in little_endian byte order */
>>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
>>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
>>> > +    }
>>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
>>> > +                          memmap[VIRT_MROM].base,
&address_space_memory);

>>> >       /* copy in the device tree */
>>> > +    if (s->fdt_size >= memmap[VIRT_MROM].size - sizeof(reset_vec)) {
>>> > +        error_report("qemu: not enough space to store device-tree");
>>> > +        exit(1);
>>> > +    }
>>> >       qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
>>> > -    cpu_physical_memory_write(memmap[VIRT_MROM].base +
sizeof(reset_vec),
>>> > -        s->fdt, s->fdt_size);
>>> > +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, s->fdt_size,
>>> > +                          memmap[VIRT_MROM].base + sizeof(reset_vec),
>>> > +                          &address_space_memory);

>>> >       /* create PLIC hart topology configuration string */
>>> >       plic_hart_config_len = (strlen(VIRT_PLIC_HART_CONFIG) + 1) *
>>> smp_cpus;
>>> > diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
>>> > index 91163d6..6f2668e 100644
>>> > --- a/include/hw/riscv/virt.h
>>> > +++ b/include/hw/riscv/virt.h
>>> > @@ -19,6 +19,10 @@
>>> >   #ifndef HW_RISCV_VIRT_H
>>> >   #define HW_RISCV_VIRT_H

>>> > +#define TYPE_RISCV_VIRT_BOARD "riscv.virt"
>>> > +#define VIRT(obj) \
>>> > +    OBJECT_CHECK(RISCVVirtState, (obj), TYPE_RISCV_VIRT_BOARD)
>>> > +

>>> This should be in a seperate patch.


>> I'll shift that chunk into "Remove unused class definitions".


> Actually we to need to drop this chunk as the unused check macros were
removed from machine state structs in "Remove unused class definitions".
Somehow the chunk made it into this patch. Likely a rebase issue.

> It's probably best that we add what we need back in the QOM SOC refactor
on-top of the this series, or at least after the first set of patches are
merged...

> I think that's what you were planning to do anyway.

Yeah, that works. So just remove it from this series.

Alistair


> Thanks.
> Michael.

Michael Clark May 4, 2018, 1:45 a.m. UTC | #5

On Sat, Apr 28, 2018 at 4:17 AM, Alistair Francis <alistair23@gmail.com>
wrote:

> On Thu, Apr 26, 2018 at 10:34 PM Michael Clark <mjc@sifive.com> wrote:
>
>
>
> > On Fri, Apr 27, 2018 at 5:22 PM, Michael Clark <mjc@sifive.com> wrote:
>
>
>
> >> On Fri, Apr 27, 2018 at 4:48 AM, Alistair Francis <alistair23@gmail.com
> >
> wrote:
>
> >>> On Wed, Apr 25, 2018 at 5:03 PM Michael Clark <mjc@sifive.com> wrote:
>
> >>> > The sifive_u machine already marks its ROM readonly. This fixes
> >>> > the remaining boards. This commit also makes all boards use
> >>> > mask_rom as the variable name for the ROM. This change also
> >>> > makes space for the maximum device tree size size and adds
> >>> > an explicit bounds check and error message.
>
> >>> > Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
> >>> > Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> >>> > Cc: Palmer Dabbelt <palmer@sifive.com>
> >>> > Cc: Alistair Francis <Alistair.Francis@wdc.com>
> >>> > Signed-off-by: Michael Clark <mjc@sifive.com>
> >>> > ---
> >>> >   hw/riscv/sifive_e.c     | 20 +++++++---------
> >>> >   hw/riscv/sifive_u.c     | 46 ++++++++++++++++++-----------------
> >>> >   hw/riscv/spike.c        | 64
> >>> ++++++++++++++++++++++++++++---------------------
> >>> >   hw/riscv/virt.c         | 38 +++++++++++++++--------------
> >>> >   include/hw/riscv/virt.h |  4 ++++
> >>> >   5 files changed, 93 insertions(+), 79 deletions(-)
>
> >>> > diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> >>> > index 39e4cb4..0c8b8e9 100644
> >>> > --- a/hw/riscv/sifive_e.c
> >>> > +++ b/hw/riscv/sifive_e.c
> >>> > @@ -74,14 +74,6 @@ static const struct MemmapEntry {
> >>> >       [SIFIVE_E_DTIM] =     { 0x80000000,     0x4000 }
> >>> >   };
>
> >>> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t len)
> >>> > -{
> >>> > -    int i;
> >>> > -    for (i = 0; i < (len >> 2); i++) {
> >>> > -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
> >>> > -    }
> >>> > -}
> >>> > -
> >>> >   static uint64_t load_kernel(const char *kernel_filename)
> >>> >   {
> >>> >       uint64_t kernel_entry, kernel_high;
> >>> > @@ -112,6 +104,7 @@ static void riscv_sifive_e_init(MachineState
> *machine)
> >>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> >>> >       MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> >>> >       MemoryRegion *xip_mem = g_new(MemoryRegion, 1);
> >>> > +    int i;
>
> >>> >       /* Initialize SOC */
> >>> >       object_initialize(&s->soc, sizeof(s->soc),
> TYPE_RISCV_HART_ARRAY);
> >>> > @@ -131,7 +124,7 @@ static void riscv_sifive_e_init(MachineState
> *machine)
> >>> >           memmap[SIFIVE_E_DTIM].base, main_mem);
>
> >>> >       /* Mask ROM */
> >>> > -    memory_region_init_ram(mask_rom, NULL, "riscv.sifive.e.mrom",
> >>> > +    memory_region_init_rom(mask_rom, NULL, "riscv.sifive.e.mrom",
> >>> >           memmap[SIFIVE_E_MROM].size, &error_fatal);
> >>> >       memory_region_add_subregion(sys_mem,
> >>> >           memmap[SIFIVE_E_MROM].base, mask_rom);
> >>> > @@ -185,9 +178,12 @@ static void riscv_sifive_e_init(MachineState
> >>> *machine)
> >>> >           0x00028067,        /* 0x1004: jr      t0 */
> >>> >       };
>
> >>> > -    /* copy in the reset vector */
> >>> > -    copy_le32_to_phys(memmap[SIFIVE_E_MROM].base, reset_vec,
> >>> sizeof(reset_vec));
> >>> > -    memory_region_set_readonly(mask_rom, true);
> >>> > +    /* copy in the reset vector in little_endian byte order */
> >>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> >>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> >>> > +    }
> >>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec,
> sizeof(reset_vec),
> >>> > +                          memmap[SIFIVE_E_MROM].base,
> >>> &address_space_memory);
>
> >>> >       if (machine->kernel_filename) {
> >>> >           load_kernel(machine->kernel_filename);
> >>> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> >>> > index 115618b..11ba4c3 100644
> >>> > --- a/hw/riscv/sifive_u.c
> >>> > +++ b/hw/riscv/sifive_u.c
> >>> > @@ -52,7 +52,7 @@ static const struct MemmapEntry {
> >>> >       hwaddr size;
> >>> >   } sifive_u_memmap[] = {
> >>> >       [SIFIVE_U_DEBUG] =    {        0x0,      0x100 },
> >>> > -    [SIFIVE_U_MROM] =     {     0x1000,     0x2000 },
> >>> > +    [SIFIVE_U_MROM] =     {     0x1000,    0x11000 },
> >>> >       [SIFIVE_U_CLINT] =    {  0x2000000,    0x10000 },
> >>> >       [SIFIVE_U_PLIC] =     {  0xc000000,  0x4000000 },
> >>> >       [SIFIVE_U_UART0] =    { 0x10013000,     0x1000 },
> >>> > @@ -60,14 +60,6 @@ static const struct MemmapEntry {
> >>> >       [SIFIVE_U_DRAM] =     { 0x80000000,        0x0 },
> >>> >   };
>
> >>> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t len)
> >>> > -{
> >>> > -    int i;
> >>> > -    for (i = 0; i < (len >> 2); i++) {
> >>> > -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
> >>> > -    }
> >>> > -}
> >>> > -
> >>> >   static uint64_t load_kernel(const char *kernel_filename)
> >>> >   {
> >>> >       uint64_t kernel_entry, kernel_high;
> >>> > @@ -221,9 +213,10 @@ static void riscv_sifive_u_init(MachineState
> >>> *machine)
> >>> >       const struct MemmapEntry *memmap = sifive_u_memmap;
>
> >>> >       SiFiveUState *s = g_new0(SiFiveUState, 1);
> >>> > -    MemoryRegion *sys_memory = get_system_memory();
> >>> > +    MemoryRegion *system_memory = get_system_memory();
> >>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> >>> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
> >>> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> >>> > +    int i;
>
> >>> >       /* Initialize SOC */
> >>> >       object_initialize(&s->soc, sizeof(s->soc),
> TYPE_RISCV_HART_ARRAY);
> >>> > @@ -239,17 +232,17 @@ static void riscv_sifive_u_init(MachineState
> >>> *machine)
> >>> >       /* register RAM */
> >>> >       memory_region_init_ram(main_mem, NULL, "riscv.sifive.u.ram",
> >>> >                              machine->ram_size, &error_fatal);
> >>> > -    memory_region_add_subregion(sys_memory,
> memmap[SIFIVE_U_DRAM].base,
> >>> > +    memory_region_add_subregion(system_memory,
> >>> memmap[SIFIVE_U_DRAM].base,
> >>> >           main_mem);
>
> >>> >       /* create device tree */
> >>> >       create_fdt(s, memmap, machine->ram_size,
> machine->kernel_cmdline);
>
> >>> >       /* boot rom */
> >>> > -    memory_region_init_ram(boot_rom, NULL, "riscv.sifive.u.mrom",
> >>> > -                           memmap[SIFIVE_U_MROM].base,
> &error_fatal);
> >>> > -    memory_region_set_readonly(boot_rom, true);
> >>> > -    memory_region_add_subregion(sys_memory, 0x0, boot_rom);
> >>> > +    memory_region_init_rom(mask_rom, NULL, "riscv.sifive.u.mrom",
> >>> > +                           memmap[SIFIVE_U_MROM].size,
> &error_fatal);
> >>> > +    memory_region_add_subregion(system_memory,
> >>> memmap[SIFIVE_U_MROM].base,
> >>> > +                                mask_rom);
>
> >>> >       if (machine->kernel_filename) {
> >>> >           load_kernel(machine->kernel_filename);
> >>> > @@ -272,13 +265,22 @@ static void riscv_sifive_u_init(MachineState
> >>> *machine)
> >>> >                                          /* dtb: */
> >>> >       };
>
> >>> > -    /* copy in the reset vector */
> >>> > -    copy_le32_to_phys(memmap[SIFIVE_U_MROM].base, reset_vec,
> >>> sizeof(reset_vec));
> >>> > +    /* copy in the reset vector in little_endian byte order */
> >>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> >>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> >>> > +    }
> >>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec,
> sizeof(reset_vec),
> >>> > +                          memmap[SIFIVE_U_MROM].base,
> >>> &address_space_memory);
>
> >>> >       /* copy in the device tree */
> >>> > +    if (s->fdt_size >= memmap[SIFIVE_U_MROM].size -
> sizeof(reset_vec)) {
> >>> > +        error_report("qemu: not enough space to store device-tree");
> >>> > +        exit(1);
> >>> > +    }
> >>> >       qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
> >>> > -    cpu_physical_memory_write(memmap[SIFIVE_U_MROM].base +
> >>> > -        sizeof(reset_vec), s->fdt, s->fdt_size);
> >>> > +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, s->fdt_size,
> >>> > +                          memmap[SIFIVE_U_MROM].base +
> sizeof(reset_vec),
> >>> > +                          &address_space_memory);
>
> >>> >       /* MMIO */
> >>> >       s->plic = sifive_plic_create(memmap[SIFIVE_U_PLIC].base,
> >>> > @@ -292,9 +294,9 @@ static void riscv_sifive_u_init(MachineState
> *machine)
> >>> >           SIFIVE_U_PLIC_CONTEXT_BASE,
> >>> >           SIFIVE_U_PLIC_CONTEXT_STRIDE,
> >>> >           memmap[SIFIVE_U_PLIC].size);
> >>> > -    sifive_uart_create(sys_memory, memmap[SIFIVE_U_UART0].base,
> >>> > +    sifive_uart_create(system_memory, memmap[SIFIVE_U_UART0].base,
> >>> >           serial_hds[0],
> SIFIVE_PLIC(s->plic)->irqs[SIFIVE_U_UART0_IRQ]);
> >>> > -    /* sifive_uart_create(sys_memory, memmap[SIFIVE_U_UART1].base,
> >>> > +    /* sifive_uart_create(system_memory,
> memmap[SIFIVE_U_UART1].base,
> >>> >           serial_hds[1],
> SIFIVE_PLIC(s->plic)->irqs[SIFIVE_U_UART1_IRQ]);
> >>> */
> >>> >       sifive_clint_create(memmap[SIFIVE_U_CLINT].base,
> >>> >           memmap[SIFIVE_U_CLINT].size, smp_cpus,
> >>> > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> >>> > index 3f6bd0a..d1dbe6b 100644
> >>> > --- a/hw/riscv/spike.c
> >>> > +++ b/hw/riscv/spike.c
> >>> > @@ -46,19 +46,11 @@ static const struct MemmapEntry {
> >>> >       hwaddr base;
> >>> >       hwaddr size;
> >>> >   } spike_memmap[] = {
> >>> > -    [SPIKE_MROM] =     {     0x1000,     0x2000 },
> >>> > +    [SPIKE_MROM] =     {     0x1000,    0x11000 },
> >>> >       [SPIKE_CLINT] =    {  0x2000000,    0x10000 },
> >>> >       [SPIKE_DRAM] =     { 0x80000000,        0x0 },
> >>> >   };
>
> >>> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t len)
> >>> > -{
> >>> > -    int i;
> >>> > -    for (i = 0; i < (len >> 2); i++) {
> >>> > -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
> >>> > -    }
> >>> > -}
> >>> > -
> >>> >   static uint64_t load_kernel(const char *kernel_filename)
> >>> >   {
> >>> >       uint64_t kernel_entry, kernel_high;
> >>> > @@ -173,7 +165,8 @@ static void spike_v1_10_0_board_init(
> MachineState
> >>> *machine)
> >>> >       SpikeState *s = g_new0(SpikeState, 1);
> >>> >       MemoryRegion *system_memory = get_system_memory();
> >>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> >>> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
> >>> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> >>> > +    int i;
>
> >>> >       /* Initialize SOC */
> >>> >       object_initialize(&s->soc, sizeof(s->soc),
> TYPE_RISCV_HART_ARRAY);
> >>> > @@ -196,9 +189,10 @@ static void spike_v1_10_0_board_init(
> MachineState
> >>> *machine)
> >>> >       create_fdt(s, memmap, machine->ram_size,
> machine->kernel_cmdline);
>
> >>> >       /* boot rom */
> >>> > -    memory_region_init_ram(boot_rom, NULL, "riscv.spike.bootrom",
> >>> > -                           s->fdt_size + 0x2000, &error_fatal);
> >>> > -    memory_region_add_subregion(system_memory, 0x0, boot_rom);
> >>> > +    memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
> >>> > +                           memmap[SPIKE_MROM].size, &error_fatal);
> >>> > +    memory_region_add_subregion(system_memory,
> memmap[SPIKE_MROM].base,
> >>> > +                                mask_rom);
>
> >>> >       if (machine->kernel_filename) {
> >>> >           load_kernel(machine->kernel_filename);
> >>> > @@ -221,16 +215,25 @@ static void
> spike_v1_10_0_board_init(MachineState
> >>> *machine)
> >>> >                                        /* dtb: */
> >>> >       };
>
> >>> > -    /* copy in the reset vector */
> >>> > -    copy_le32_to_phys(memmap[SPIKE_MROM].base, reset_vec,
> >>> sizeof(reset_vec));
> >>> > +    /* copy in the reset vector in little_endian byte order */
> >>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> >>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> >>> > +    }
> >>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec,
> sizeof(reset_vec),
> >>> > +                          memmap[SPIKE_MROM].base,
> >>> &address_space_memory);
>
> >>> >       /* copy in the device tree */
> >>> > +    if (s->fdt_size >= memmap[SPIKE_MROM].size - sizeof(reset_vec))
> {
> >>> > +        error_report("qemu: not enough space to store device-tree");
> >>> > +        exit(1);
> >>> > +    }
> >>> >       qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
> >>> > -    cpu_physical_memory_write(memmap[SPIKE_MROM].base +
> >>> sizeof(reset_vec),
> >>> > -        s->fdt, s->fdt_size);
> >>> > +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, s->fdt_size,
> >>> > +                          memmap[SPIKE_MROM].base +
> sizeof(reset_vec),
> >>> > +                          &address_space_memory);
>
> >>> >       /* initialize HTIF using symbols found in load_kernel */
> >>> > -    htif_mm_init(system_memory, boot_rom, &s->soc.harts[0].env,
> >>> serial_hds[0]);
> >>> > +    htif_mm_init(system_memory, mask_rom, &s->soc.harts[0].env,
> >>> serial_hds[0]);
>
> >>> >       /* Core Local Interruptor (timer and IPI) */
> >>> >       sifive_clint_create(memmap[SPIKE_CLINT].base,
> >>> memmap[SPIKE_CLINT].size,
> >>> > @@ -244,7 +247,8 @@ static void spike_v1_09_1_board_init(
> MachineState
> >>> *machine)
> >>> >       SpikeState *s = g_new0(SpikeState, 1);
> >>> >       MemoryRegion *system_memory = get_system_memory();
> >>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> >>> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
> >>> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> >>> > +    int i;
>
> >>> >       /* Initialize SOC */
> >>> >       object_initialize(&s->soc, sizeof(s->soc),
> TYPE_RISCV_HART_ARRAY);
> >>> > @@ -264,9 +268,10 @@ static void spike_v1_09_1_board_init(
> MachineState
> >>> *machine)
> >>> >           main_mem);
>
> >>> >       /* boot rom */
> >>> > -    memory_region_init_ram(boot_rom, NULL, "riscv.spike.bootrom",
> >>> > -                           0x40000, &error_fatal);
> >>> > -    memory_region_add_subregion(system_memory, 0x0, boot_rom);
> >>> > +    memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
> >>> > +                           memmap[SPIKE_MROM].size, &error_fatal);
> >>> > +    memory_region_add_subregion(system_memory,
> memmap[SPIKE_MROM].base,
> >>> > +                                mask_rom);
>
> >>> >       if (machine->kernel_filename) {
> >>> >           load_kernel(machine->kernel_filename);
> >>> > @@ -319,15 +324,20 @@ static void
> spike_v1_09_1_board_init(MachineState
> >>> *machine)
> >>> >       g_free(isa);
> >>> >       size_t config_string_len = strlen(config_string);
>
> >>> > -    /* copy in the reset vector */
> >>> > -    copy_le32_to_phys(memmap[SPIKE_MROM].base, reset_vec,
> >>> sizeof(reset_vec));
> >>> > +    /* copy in the reset vector in little_endian byte order */
> >>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> >>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> >>> > +    }
> >>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec,
> sizeof(reset_vec),
> >>> > +                          memmap[SPIKE_MROM].base,
> >>> &address_space_memory);
>
> >>> >       /* copy in the config string */
> >>> > -    cpu_physical_memory_write(memmap[SPIKE_MROM].base +
> >>> sizeof(reset_vec),
> >>> > -        config_string, config_string_len);
> >>> > +    rom_add_blob_fixed_as("mrom.reset", config_string,
> config_string_len,
> >>> > +                          memmap[SPIKE_MROM].base +
> sizeof(reset_vec),
> >>> > +                          &address_space_memory);
>
> >>> >       /* initialize HTIF using symbols found in load_kernel */
> >>> > -    htif_mm_init(system_memory, boot_rom, &s->soc.harts[0].env,
> >>> serial_hds[0]);
> >>> > +    htif_mm_init(system_memory, mask_rom, &s->soc.harts[0].env,
> >>> serial_hds[0]);
>
> >>> >       /* Core Local Interruptor (timer and IPI) */
> >>> >       sifive_clint_create(memmap[SPIKE_CLINT].base,
> >>> memmap[SPIKE_CLINT].size,
> >>> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> >>> > index 090befe..20c509d 100644
> >>> > --- a/hw/riscv/virt.c
> >>> > +++ b/hw/riscv/virt.c
> >>> > @@ -45,8 +45,8 @@ static const struct MemmapEntry {
> >>> >       hwaddr size;
> >>> >   } virt_memmap[] = {
> >>> >       [VIRT_DEBUG] =    {        0x0,      0x100 },
> >>> > -    [VIRT_MROM] =     {     0x1000,     0x2000 },
> >>> > -    [VIRT_TEST] =     {     0x4000,     0x1000 },
> >>> > +    [VIRT_MROM] =     {     0x1000,    0x11000 },
> >>> > +    [VIRT_TEST] =     {   0x100000,     0x1000 },
> >>> >       [VIRT_CLINT] =    {  0x2000000,    0x10000 },
> >>> >       [VIRT_PLIC] =     {  0xc000000,  0x4000000 },
> >>> >       [VIRT_UART0] =    { 0x10000000,      0x100 },
> >>> > @@ -54,14 +54,6 @@ static const struct MemmapEntry {
> >>> >       [VIRT_DRAM] =     { 0x80000000,        0x0 },
> >>> >   };
>
> >>> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t len)
> >>> > -{
> >>> > -    int i;
> >>> > -    for (i = 0; i < (len >> 2); i++) {
> >>> > -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
> >>> > -    }
> >>> > -}
> >>> > -
> >>> >   static uint64_t load_kernel(const char *kernel_filename)
> >>> >   {
> >>> >       uint64_t kernel_entry, kernel_high;
> >>> > @@ -272,7 +264,7 @@ static void riscv_virt_board_init(MachineState
> >>> *machine)
> >>> >       RISCVVirtState *s = g_new0(RISCVVirtState, 1);
> >>> >       MemoryRegion *system_memory = get_system_memory();
> >>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> >>> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
> >>> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> >>> >       char *plic_hart_config;
> >>> >       size_t plic_hart_config_len;
> >>> >       int i;
> >>> > @@ -299,9 +291,10 @@ static void riscv_virt_board_init(MachineState
> >>> *machine)
> >>> >       fdt = create_fdt(s, memmap, machine->ram_size,
> >>> machine->kernel_cmdline);
>
> >>> >       /* boot rom */
> >>> > -    memory_region_init_ram(boot_rom, NULL,
> "riscv_virt_board.bootrom",
> >>> > -                           s->fdt_size + 0x2000, &error_fatal);
> >>> > -    memory_region_add_subregion(system_memory, 0x0, boot_rom);
> >>> > +    memory_region_init_rom(mask_rom, NULL, "riscv_virt_board.mrom",
> >>> > +                           memmap[VIRT_MROM].size, &error_fatal);
> >>> > +    memory_region_add_subregion(system_memory,
> memmap[VIRT_MROM].base,
> >>> > +                                mask_rom);
>
> >>> >       if (machine->kernel_filename) {
> >>> >           uint64_t kernel_entry =
> load_kernel(machine->kernel_filename);
> >>> > @@ -335,13 +328,22 @@ static void riscv_virt_board_init(MachineState
> >>> *machine)
> >>> >                                        /* dtb: */
> >>> >       };
>
> >>> > -    /* copy in the reset vector */
> >>> > -    copy_le32_to_phys(memmap[VIRT_MROM].base, reset_vec,
> >>> sizeof(reset_vec));
> >>> > +    /* copy in the reset vector in little_endian byte order */
> >>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> >>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> >>> > +    }
> >>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec,
> sizeof(reset_vec),
> >>> > +                          memmap[VIRT_MROM].base,
> &address_space_memory);
>
> >>> >       /* copy in the device tree */
> >>> > +    if (s->fdt_size >= memmap[VIRT_MROM].size - sizeof(reset_vec)) {
> >>> > +        error_report("qemu: not enough space to store device-tree");
> >>> > +        exit(1);
> >>> > +    }
> >>> >       qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
> >>> > -    cpu_physical_memory_write(memmap[VIRT_MROM].base +
> sizeof(reset_vec),
> >>> > -        s->fdt, s->fdt_size);
> >>> > +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, s->fdt_size,
> >>> > +                          memmap[VIRT_MROM].base +
> sizeof(reset_vec),
> >>> > +                          &address_space_memory);
>
> >>> >       /* create PLIC hart topology configuration string */
> >>> >       plic_hart_config_len = (strlen(VIRT_PLIC_HART_CONFIG) + 1) *
> >>> smp_cpus;
> >>> > diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> >>> > index 91163d6..6f2668e 100644
> >>> > --- a/include/hw/riscv/virt.h
> >>> > +++ b/include/hw/riscv/virt.h
> >>> > @@ -19,6 +19,10 @@
> >>> >   #ifndef HW_RISCV_VIRT_H
> >>> >   #define HW_RISCV_VIRT_H
>
> >>> > +#define TYPE_RISCV_VIRT_BOARD "riscv.virt"
> >>> > +#define VIRT(obj) \
> >>> > +    OBJECT_CHECK(RISCVVirtState, (obj), TYPE_RISCV_VIRT_BOARD)
> >>> > +
>
> >>> This should be in a seperate patch.
>
>
> >> I'll shift that chunk into "Remove unused class definitions".
>
>
> > Actually we to need to drop this chunk as the unused check macros were
> removed from machine state structs in "Remove unused class definitions".
> Somehow the chunk made it into this patch. Likely a rebase issue.
>
> > It's probably best that we add what we need back in the QOM SOC refactor
> on-top of the this series, or at least after the first set of patches are
> merged...
>
> > I think that's what you were planning to do anyway.
>
> Yeah, that works. So just remove it from this series.
>

After rebasing I had to change this patch because of this patch which
increases the default device tree size to 1MiB. This is not controllable by
the user and we don't know how big the resultant device-tree is. It could
be < 8KiB in our case.

-
https://git.qemu.org/?p=qemu.git;a=commit;h=14ec3cbd7c1e31dca4d23f028100c8f43e156573


I studied ftd and used public interfaces and a mechanism consistent with
the fdt resize functions to calculate the size. As far as I can tell it is
accurate and covers exactly to the end of the uint32 terminator. I needed
this because our ROMs are currently small.

Peter, this is the patch that changes our ROMs from RAM to ROM
using memory_region_init_rom and rom_add_blob_fixed_as (as per
hw/arm/boot.c), and it also adds a truncation warning, so that we actually
know what size the device-tree is, given our ROMs are currently much
smaller than 1MiB. That is why we needed a method that tells us how big the
device tree actually is.

BTW I'm actually suspicious of 'fdt_resize' here:

-
https://git.qemu.org/?p=dtc.git;a=blob;f=libfdt/fdt_sw.c;h=6d33cc29d0224d9fc6307607ef7563df944da2d3

as it doesn't check that 'bufsize' has enough space for the header and
terminator, although that's potentially a dtc bug. I read dtc to make sure
the method we use to calculate the size was accurate. There probably should
be a method in dtc as we rely on some implementation details, however they
are quite simple and we can get: sizeof(header) + sizeof(structs) +
sizeof(strings)
+ terminator using public APIs and basic knowledge of the serialised
device-tree form.

Anyway, here is the rebased version of this patch using the new
'qemu_fdt_totalsize' method in the patch I just sent.

-
https://github.com/riscv/riscv-qemu/commit/a65f6e0447d6e32d75f64ba31df5f20d529d0489

I have a feeling this is the patch that fixes sifive_u. Did you bisect
which patch in the series fixed sifive_u?

I have to send a pull for the reviewed patches which I can do today, but
this is one of the patches that is early in the series that does not yet
have Reviewed-by. When I split the series this patch would be in a second
series that i'll have to link to the pull in patchew (using the method
Peter mentioned) or wait until the pull is accepted.

Michael.

Alistair Francis May 4, 2018, 11:44 p.m. UTC | #6

On Thu, May 3, 2018 at 6:45 PM Michael Clark <mjc@sifive.com> wrote:



> On Sat, Apr 28, 2018 at 4:17 AM, Alistair Francis <alistair23@gmail.com>
wrote:

>> On Thu, Apr 26, 2018 at 10:34 PM Michael Clark <mjc@sifive.com> wrote:



>> > On Fri, Apr 27, 2018 at 5:22 PM, Michael Clark <mjc@sifive.com> wrote:



>> >> On Fri, Apr 27, 2018 at 4:48 AM, Alistair Francis <
alistair23@gmail.com>
>> wrote:

>> >>> On Wed, Apr 25, 2018 at 5:03 PM Michael Clark <mjc@sifive.com> wrote:

>> >>> > The sifive_u machine already marks its ROM readonly. This fixes
>> >>> > the remaining boards. This commit also makes all boards use
>> >>> > mask_rom as the variable name for the ROM. This change also
>> >>> > makes space for the maximum device tree size size and adds
>> >>> > an explicit bounds check and error message.

>> >>> > Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
>> >>> > Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
>> >>> > Cc: Palmer Dabbelt <palmer@sifive.com>
>> >>> > Cc: Alistair Francis <Alistair.Francis@wdc.com>
>> >>> > Signed-off-by: Michael Clark <mjc@sifive.com>
>> >>> > ---
>> >>> >   hw/riscv/sifive_e.c     | 20 +++++++---------
>> >>> >   hw/riscv/sifive_u.c     | 46 ++++++++++++++++++-----------------
>> >>> >   hw/riscv/spike.c        | 64
>> >>> ++++++++++++++++++++++++++++---------------------
>> >>> >   hw/riscv/virt.c         | 38 +++++++++++++++--------------
>> >>> >   include/hw/riscv/virt.h |  4 ++++
>> >>> >   5 files changed, 93 insertions(+), 79 deletions(-)

>> >>> > diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
>> >>> > index 39e4cb4..0c8b8e9 100644
>> >>> > --- a/hw/riscv/sifive_e.c
>> >>> > +++ b/hw/riscv/sifive_e.c
>> >>> > @@ -74,14 +74,6 @@ static const struct MemmapEntry {
>> >>> >       [SIFIVE_E_DTIM] =     { 0x80000000,     0x4000 }
>> >>> >   };

>> >>> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t
len)
>> >>> > -{
>> >>> > -    int i;
>> >>> > -    for (i = 0; i < (len >> 2); i++) {
>> >>> > -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
>> >>> > -    }
>> >>> > -}
>> >>> > -
>> >>> >   static uint64_t load_kernel(const char *kernel_filename)
>> >>> >   {
>> >>> >       uint64_t kernel_entry, kernel_high;
>> >>> > @@ -112,6 +104,7 @@ static void riscv_sifive_e_init(MachineState
>> *machine)
>> >>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
>> >>> >       MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
>> >>> >       MemoryRegion *xip_mem = g_new(MemoryRegion, 1);
>> >>> > +    int i;

>> >>> >       /* Initialize SOC */
>> >>> >       object_initialize(&s->soc, sizeof(s->soc),
>> TYPE_RISCV_HART_ARRAY);
>> >>> > @@ -131,7 +124,7 @@ static void riscv_sifive_e_init(MachineState
>> *machine)
>> >>> >           memmap[SIFIVE_E_DTIM].base, main_mem);

>> >>> >       /* Mask ROM */
>> >>> > -    memory_region_init_ram(mask_rom, NULL, "riscv.sifive.e.mrom",
>> >>> > +    memory_region_init_rom(mask_rom, NULL, "riscv.sifive.e.mrom",
>> >>> >           memmap[SIFIVE_E_MROM].size, &error_fatal);
>> >>> >       memory_region_add_subregion(sys_mem,
>> >>> >           memmap[SIFIVE_E_MROM].base, mask_rom);
>> >>> > @@ -185,9 +178,12 @@ static void riscv_sifive_e_init(MachineState
>> >>> *machine)
>> >>> >           0x00028067,        /* 0x1004: jr      t0 */
>> >>> >       };

>> >>> > -    /* copy in the reset vector */
>> >>> > -    copy_le32_to_phys(memmap[SIFIVE_E_MROM].base, reset_vec,
>> >>> sizeof(reset_vec));
>> >>> > -    memory_region_set_readonly(mask_rom, true);
>> >>> > +    /* copy in the reset vector in little_endian byte order */
>> >>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
>> >>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
>> >>> > +    }
>> >>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec,
sizeof(reset_vec),
>> >>> > +                          memmap[SIFIVE_E_MROM].base,
>> >>> &address_space_memory);

>> >>> >       if (machine->kernel_filename) {
>> >>> >           load_kernel(machine->kernel_filename);
>> >>> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
>> >>> > index 115618b..11ba4c3 100644
>> >>> > --- a/hw/riscv/sifive_u.c
>> >>> > +++ b/hw/riscv/sifive_u.c
>> >>> > @@ -52,7 +52,7 @@ static const struct MemmapEntry {
>> >>> >       hwaddr size;
>> >>> >   } sifive_u_memmap[] = {
>> >>> >       [SIFIVE_U_DEBUG] =    {        0x0,      0x100 },
>> >>> > -    [SIFIVE_U_MROM] =     {     0x1000,     0x2000 },
>> >>> > +    [SIFIVE_U_MROM] =     {     0x1000,    0x11000 },
>> >>> >       [SIFIVE_U_CLINT] =    {  0x2000000,    0x10000 },
>> >>> >       [SIFIVE_U_PLIC] =     {  0xc000000,  0x4000000 },
>> >>> >       [SIFIVE_U_UART0] =    { 0x10013000,     0x1000 },
>> >>> > @@ -60,14 +60,6 @@ static const struct MemmapEntry {
>> >>> >       [SIFIVE_U_DRAM] =     { 0x80000000,        0x0 },
>> >>> >   };

>> >>> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t
len)
>> >>> > -{
>> >>> > -    int i;
>> >>> > -    for (i = 0; i < (len >> 2); i++) {
>> >>> > -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
>> >>> > -    }
>> >>> > -}
>> >>> > -
>> >>> >   static uint64_t load_kernel(const char *kernel_filename)
>> >>> >   {
>> >>> >       uint64_t kernel_entry, kernel_high;
>> >>> > @@ -221,9 +213,10 @@ static void riscv_sifive_u_init(MachineState
>> >>> *machine)
>> >>> >       const struct MemmapEntry *memmap = sifive_u_memmap;

>> >>> >       SiFiveUState *s = g_new0(SiFiveUState, 1);
>> >>> > -    MemoryRegion *sys_memory = get_system_memory();
>> >>> > +    MemoryRegion *system_memory = get_system_memory();
>> >>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
>> >>> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
>> >>> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
>> >>> > +    int i;

>> >>> >       /* Initialize SOC */
>> >>> >       object_initialize(&s->soc, sizeof(s->soc),
>> TYPE_RISCV_HART_ARRAY);
>> >>> > @@ -239,17 +232,17 @@ static void riscv_sifive_u_init(MachineState
>> >>> *machine)
>> >>> >       /* register RAM */
>> >>> >       memory_region_init_ram(main_mem, NULL, "riscv.sifive.u.ram",
>> >>> >                              machine->ram_size, &error_fatal);
>> >>> > -    memory_region_add_subregion(sys_memory,
>> memmap[SIFIVE_U_DRAM].base,
>> >>> > +    memory_region_add_subregion(system_memory,
>> >>> memmap[SIFIVE_U_DRAM].base,
>> >>> >           main_mem);

>> >>> >       /* create device tree */
>> >>> >       create_fdt(s, memmap, machine->ram_size,
>> machine->kernel_cmdline);

>> >>> >       /* boot rom */
>> >>> > -    memory_region_init_ram(boot_rom, NULL, "riscv.sifive.u.mrom",
>> >>> > -                           memmap[SIFIVE_U_MROM].base,
&error_fatal);
>> >>> > -    memory_region_set_readonly(boot_rom, true);
>> >>> > -    memory_region_add_subregion(sys_memory, 0x0, boot_rom);
>> >>> > +    memory_region_init_rom(mask_rom, NULL, "riscv.sifive.u.mrom",
>> >>> > +                           memmap[SIFIVE_U_MROM].size,
&error_fatal);
>> >>> > +    memory_region_add_subregion(system_memory,
>> >>> memmap[SIFIVE_U_MROM].base,
>> >>> > +                                mask_rom);

>> >>> >       if (machine->kernel_filename) {
>> >>> >           load_kernel(machine->kernel_filename);
>> >>> > @@ -272,13 +265,22 @@ static void riscv_sifive_u_init(MachineState
>> >>> *machine)
>> >>> >                                          /* dtb: */
>> >>> >       };

>> >>> > -    /* copy in the reset vector */
>> >>> > -    copy_le32_to_phys(memmap[SIFIVE_U_MROM].base, reset_vec,
>> >>> sizeof(reset_vec));
>> >>> > +    /* copy in the reset vector in little_endian byte order */
>> >>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
>> >>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
>> >>> > +    }
>> >>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec,
sizeof(reset_vec),
>> >>> > +                          memmap[SIFIVE_U_MROM].base,
>> >>> &address_space_memory);

>> >>> >       /* copy in the device tree */
>> >>> > +    if (s->fdt_size >= memmap[SIFIVE_U_MROM].size -
>> sizeof(reset_vec)) {
>> >>> > +        error_report("qemu: not enough space to store
device-tree");
>> >>> > +        exit(1);
>> >>> > +    }
>> >>> >       qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
>> >>> > -    cpu_physical_memory_write(memmap[SIFIVE_U_MROM].base +
>> >>> > -        sizeof(reset_vec), s->fdt, s->fdt_size);
>> >>> > +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, s->fdt_size,
>> >>> > +                          memmap[SIFIVE_U_MROM].base +
>> sizeof(reset_vec),
>> >>> > +                          &address_space_memory);

>> >>> >       /* MMIO */
>> >>> >       s->plic = sifive_plic_create(memmap[SIFIVE_U_PLIC].base,
>> >>> > @@ -292,9 +294,9 @@ static void riscv_sifive_u_init(MachineState
>> *machine)
>> >>> >           SIFIVE_U_PLIC_CONTEXT_BASE,
>> >>> >           SIFIVE_U_PLIC_CONTEXT_STRIDE,
>> >>> >           memmap[SIFIVE_U_PLIC].size);
>> >>> > -    sifive_uart_create(sys_memory, memmap[SIFIVE_U_UART0].base,
>> >>> > +    sifive_uart_create(system_memory, memmap[SIFIVE_U_UART0].base,
>> >>> >           serial_hds[0],
>> SIFIVE_PLIC(s->plic)->irqs[SIFIVE_U_UART0_IRQ]);
>> >>> > -    /* sifive_uart_create(sys_memory, memmap[SIFIVE_U_UART1].base,
>> >>> > +    /* sifive_uart_create(system_memory,
memmap[SIFIVE_U_UART1].base,
>> >>> >           serial_hds[1],
>> SIFIVE_PLIC(s->plic)->irqs[SIFIVE_U_UART1_IRQ]);
>> >>> */
>> >>> >       sifive_clint_create(memmap[SIFIVE_U_CLINT].base,
>> >>> >           memmap[SIFIVE_U_CLINT].size, smp_cpus,
>> >>> > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
>> >>> > index 3f6bd0a..d1dbe6b 100644
>> >>> > --- a/hw/riscv/spike.c
>> >>> > +++ b/hw/riscv/spike.c
>> >>> > @@ -46,19 +46,11 @@ static const struct MemmapEntry {
>> >>> >       hwaddr base;
>> >>> >       hwaddr size;
>> >>> >   } spike_memmap[] = {
>> >>> > -    [SPIKE_MROM] =     {     0x1000,     0x2000 },
>> >>> > +    [SPIKE_MROM] =     {     0x1000,    0x11000 },
>> >>> >       [SPIKE_CLINT] =    {  0x2000000,    0x10000 },
>> >>> >       [SPIKE_DRAM] =     { 0x80000000,        0x0 },
>> >>> >   };

>> >>> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t
len)
>> >>> > -{
>> >>> > -    int i;
>> >>> > -    for (i = 0; i < (len >> 2); i++) {
>> >>> > -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
>> >>> > -    }
>> >>> > -}
>> >>> > -
>> >>> >   static uint64_t load_kernel(const char *kernel_filename)
>> >>> >   {
>> >>> >       uint64_t kernel_entry, kernel_high;
>> >>> > @@ -173,7 +165,8 @@ static void
spike_v1_10_0_board_init(MachineState
>> >>> *machine)
>> >>> >       SpikeState *s = g_new0(SpikeState, 1);
>> >>> >       MemoryRegion *system_memory = get_system_memory();
>> >>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
>> >>> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
>> >>> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
>> >>> > +    int i;

>> >>> >       /* Initialize SOC */
>> >>> >       object_initialize(&s->soc, sizeof(s->soc),
>> TYPE_RISCV_HART_ARRAY);
>> >>> > @@ -196,9 +189,10 @@ static void
spike_v1_10_0_board_init(MachineState
>> >>> *machine)
>> >>> >       create_fdt(s, memmap, machine->ram_size,
>> machine->kernel_cmdline);

>> >>> >       /* boot rom */
>> >>> > -    memory_region_init_ram(boot_rom, NULL, "riscv.spike.bootrom",
>> >>> > -                           s->fdt_size + 0x2000, &error_fatal);
>> >>> > -    memory_region_add_subregion(system_memory, 0x0, boot_rom);
>> >>> > +    memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
>> >>> > +                           memmap[SPIKE_MROM].size, &error_fatal);
>> >>> > +    memory_region_add_subregion(system_memory,
>> memmap[SPIKE_MROM].base,
>> >>> > +                                mask_rom);

>> >>> >       if (machine->kernel_filename) {
>> >>> >           load_kernel(machine->kernel_filename);
>> >>> > @@ -221,16 +215,25 @@ static void
>> spike_v1_10_0_board_init(MachineState
>> >>> *machine)
>> >>> >                                        /* dtb: */
>> >>> >       };

>> >>> > -    /* copy in the reset vector */
>> >>> > -    copy_le32_to_phys(memmap[SPIKE_MROM].base, reset_vec,
>> >>> sizeof(reset_vec));
>> >>> > +    /* copy in the reset vector in little_endian byte order */
>> >>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
>> >>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
>> >>> > +    }
>> >>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec,
sizeof(reset_vec),
>> >>> > +                          memmap[SPIKE_MROM].base,
>> >>> &address_space_memory);

>> >>> >       /* copy in the device tree */
>> >>> > +    if (s->fdt_size >= memmap[SPIKE_MROM].size -
sizeof(reset_vec)) {
>> >>> > +        error_report("qemu: not enough space to store
device-tree");
>> >>> > +        exit(1);
>> >>> > +    }
>> >>> >       qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
>> >>> > -    cpu_physical_memory_write(memmap[SPIKE_MROM].base +
>> >>> sizeof(reset_vec),
>> >>> > -        s->fdt, s->fdt_size);
>> >>> > +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, s->fdt_size,
>> >>> > +                          memmap[SPIKE_MROM].base +
>> sizeof(reset_vec),
>> >>> > +                          &address_space_memory);

>> >>> >       /* initialize HTIF using symbols found in load_kernel */
>> >>> > -    htif_mm_init(system_memory, boot_rom, &s->soc.harts[0].env,
>> >>> serial_hds[0]);
>> >>> > +    htif_mm_init(system_memory, mask_rom, &s->soc.harts[0].env,
>> >>> serial_hds[0]);

>> >>> >       /* Core Local Interruptor (timer and IPI) */
>> >>> >       sifive_clint_create(memmap[SPIKE_CLINT].base,
>> >>> memmap[SPIKE_CLINT].size,
>> >>> > @@ -244,7 +247,8 @@ static void
spike_v1_09_1_board_init(MachineState
>> >>> *machine)
>> >>> >       SpikeState *s = g_new0(SpikeState, 1);
>> >>> >       MemoryRegion *system_memory = get_system_memory();
>> >>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
>> >>> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
>> >>> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
>> >>> > +    int i;

>> >>> >       /* Initialize SOC */
>> >>> >       object_initialize(&s->soc, sizeof(s->soc),
>> TYPE_RISCV_HART_ARRAY);
>> >>> > @@ -264,9 +268,10 @@ static void
spike_v1_09_1_board_init(MachineState
>> >>> *machine)
>> >>> >           main_mem);

>> >>> >       /* boot rom */
>> >>> > -    memory_region_init_ram(boot_rom, NULL, "riscv.spike.bootrom",
>> >>> > -                           0x40000, &error_fatal);
>> >>> > -    memory_region_add_subregion(system_memory, 0x0, boot_rom);
>> >>> > +    memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
>> >>> > +                           memmap[SPIKE_MROM].size, &error_fatal);
>> >>> > +    memory_region_add_subregion(system_memory,
>> memmap[SPIKE_MROM].base,
>> >>> > +                                mask_rom);

>> >>> >       if (machine->kernel_filename) {
>> >>> >           load_kernel(machine->kernel_filename);
>> >>> > @@ -319,15 +324,20 @@ static void
>> spike_v1_09_1_board_init(MachineState
>> >>> *machine)
>> >>> >       g_free(isa);
>> >>> >       size_t config_string_len = strlen(config_string);

>> >>> > -    /* copy in the reset vector */
>> >>> > -    copy_le32_to_phys(memmap[SPIKE_MROM].base, reset_vec,
>> >>> sizeof(reset_vec));
>> >>> > +    /* copy in the reset vector in little_endian byte order */
>> >>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
>> >>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
>> >>> > +    }
>> >>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec,
sizeof(reset_vec),
>> >>> > +                          memmap[SPIKE_MROM].base,
>> >>> &address_space_memory);

>> >>> >       /* copy in the config string */
>> >>> > -    cpu_physical_memory_write(memmap[SPIKE_MROM].base +
>> >>> sizeof(reset_vec),
>> >>> > -        config_string, config_string_len);
>> >>> > +    rom_add_blob_fixed_as("mrom.reset", config_string,
>> config_string_len,
>> >>> > +                          memmap[SPIKE_MROM].base +
>> sizeof(reset_vec),
>> >>> > +                          &address_space_memory);

>> >>> >       /* initialize HTIF using symbols found in load_kernel */
>> >>> > -    htif_mm_init(system_memory, boot_rom, &s->soc.harts[0].env,
>> >>> serial_hds[0]);
>> >>> > +    htif_mm_init(system_memory, mask_rom, &s->soc.harts[0].env,
>> >>> serial_hds[0]);

>> >>> >       /* Core Local Interruptor (timer and IPI) */
>> >>> >       sifive_clint_create(memmap[SPIKE_CLINT].base,
>> >>> memmap[SPIKE_CLINT].size,
>> >>> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> >>> > index 090befe..20c509d 100644
>> >>> > --- a/hw/riscv/virt.c
>> >>> > +++ b/hw/riscv/virt.c
>> >>> > @@ -45,8 +45,8 @@ static const struct MemmapEntry {
>> >>> >       hwaddr size;
>> >>> >   } virt_memmap[] = {
>> >>> >       [VIRT_DEBUG] =    {        0x0,      0x100 },
>> >>> > -    [VIRT_MROM] =     {     0x1000,     0x2000 },
>> >>> > -    [VIRT_TEST] =     {     0x4000,     0x1000 },
>> >>> > +    [VIRT_MROM] =     {     0x1000,    0x11000 },
>> >>> > +    [VIRT_TEST] =     {   0x100000,     0x1000 },
>> >>> >       [VIRT_CLINT] =    {  0x2000000,    0x10000 },
>> >>> >       [VIRT_PLIC] =     {  0xc000000,  0x4000000 },
>> >>> >       [VIRT_UART0] =    { 0x10000000,      0x100 },
>> >>> > @@ -54,14 +54,6 @@ static const struct MemmapEntry {
>> >>> >       [VIRT_DRAM] =     { 0x80000000,        0x0 },
>> >>> >   };

>> >>> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t
len)
>> >>> > -{
>> >>> > -    int i;
>> >>> > -    for (i = 0; i < (len >> 2); i++) {
>> >>> > -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
>> >>> > -    }
>> >>> > -}
>> >>> > -
>> >>> >   static uint64_t load_kernel(const char *kernel_filename)
>> >>> >   {
>> >>> >       uint64_t kernel_entry, kernel_high;
>> >>> > @@ -272,7 +264,7 @@ static void riscv_virt_board_init(MachineState
>> >>> *machine)
>> >>> >       RISCVVirtState *s = g_new0(RISCVVirtState, 1);
>> >>> >       MemoryRegion *system_memory = get_system_memory();
>> >>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
>> >>> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
>> >>> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
>> >>> >       char *plic_hart_config;
>> >>> >       size_t plic_hart_config_len;
>> >>> >       int i;
>> >>> > @@ -299,9 +291,10 @@ static void riscv_virt_board_init(MachineState
>> >>> *machine)
>> >>> >       fdt = create_fdt(s, memmap, machine->ram_size,
>> >>> machine->kernel_cmdline);

>> >>> >       /* boot rom */
>> >>> > -    memory_region_init_ram(boot_rom, NULL,
>> "riscv_virt_board.bootrom",
>> >>> > -                           s->fdt_size + 0x2000, &error_fatal);
>> >>> > -    memory_region_add_subregion(system_memory, 0x0, boot_rom);
>> >>> > +    memory_region_init_rom(mask_rom, NULL,
"riscv_virt_board.mrom",
>> >>> > +                           memmap[VIRT_MROM].size, &error_fatal);
>> >>> > +    memory_region_add_subregion(system_memory,
>> memmap[VIRT_MROM].base,
>> >>> > +                                mask_rom);

>> >>> >       if (machine->kernel_filename) {
>> >>> >           uint64_t kernel_entry =
>> load_kernel(machine->kernel_filename);
>> >>> > @@ -335,13 +328,22 @@ static void
riscv_virt_board_init(MachineState
>> >>> *machine)
>> >>> >                                        /* dtb: */
>> >>> >       };

>> >>> > -    /* copy in the reset vector */
>> >>> > -    copy_le32_to_phys(memmap[VIRT_MROM].base, reset_vec,
>> >>> sizeof(reset_vec));
>> >>> > +    /* copy in the reset vector in little_endian byte order */
>> >>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
>> >>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
>> >>> > +    }
>> >>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec,
sizeof(reset_vec),
>> >>> > +                          memmap[VIRT_MROM].base,
>> &address_space_memory);

>> >>> >       /* copy in the device tree */
>> >>> > +    if (s->fdt_size >= memmap[VIRT_MROM].size -
sizeof(reset_vec)) {
>> >>> > +        error_report("qemu: not enough space to store
device-tree");
>> >>> > +        exit(1);
>> >>> > +    }
>> >>> >       qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
>> >>> > -    cpu_physical_memory_write(memmap[VIRT_MROM].base +
>> sizeof(reset_vec),
>> >>> > -        s->fdt, s->fdt_size);
>> >>> > +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, s->fdt_size,
>> >>> > +                          memmap[VIRT_MROM].base +
sizeof(reset_vec),
>> >>> > +                          &address_space_memory);

>> >>> >       /* create PLIC hart topology configuration string */
>> >>> >       plic_hart_config_len = (strlen(VIRT_PLIC_HART_CONFIG) + 1) *
>> >>> smp_cpus;
>> >>> > diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
>> >>> > index 91163d6..6f2668e 100644
>> >>> > --- a/include/hw/riscv/virt.h
>> >>> > +++ b/include/hw/riscv/virt.h
>> >>> > @@ -19,6 +19,10 @@
>> >>> >   #ifndef HW_RISCV_VIRT_H
>> >>> >   #define HW_RISCV_VIRT_H

>> >>> > +#define TYPE_RISCV_VIRT_BOARD "riscv.virt"
>> >>> > +#define VIRT(obj) \
>> >>> > +    OBJECT_CHECK(RISCVVirtState, (obj), TYPE_RISCV_VIRT_BOARD)
>> >>> > +

>> >>> This should be in a seperate patch.


>> >> I'll shift that chunk into "Remove unused class definitions".


>> > Actually we to need to drop this chunk as the unused check macros were
>> removed from machine state structs in "Remove unused class definitions".
>> Somehow the chunk made it into this patch. Likely a rebase issue.

>> > It's probably best that we add what we need back in the QOM SOC
refactor
>> on-top of the this series, or at least after the first set of patches are
>> merged...

>> > I think that's what you were planning to do anyway.

>> Yeah, that works. So just remove it from this series.


> After rebasing I had to change this patch because of this patch which
increases the default device tree size to 1MiB. This is not controllable by
the user and we don't know how big the resultant device-tree is. It could
be < 8KiB in our case.

> -
https://git.qemu.org/?p=qemu.git;a=commit;h=14ec3cbd7c1e31dca4d23f028100c8f43e156573

> I studied ftd and used public interfaces and a mechanism consistent with
the fdt resize functions to calculate the size. As far as I can tell it is
accurate and covers exactly to the end of the uint32 terminator. I needed
this because our ROMs are currently small.

> Peter, this is the patch that changes our ROMs from RAM to ROM using
memory_region_init_rom and rom_add_blob_fixed_as (as per hw/arm/boot.c),
and it also adds a truncation warning, so that we actually know what size
the device-tree is, given our ROMs are currently much smaller than 1MiB.
That is why we needed a method that tells us how big the device tree
actually is.

> BTW I'm actually suspicious of 'fdt_resize' here:

> -
https://git.qemu.org/?p=dtc.git;a=blob;f=libfdt/fdt_sw.c;h=6d33cc29d0224d9fc6307607ef7563df944da2d3

> as it doesn't check that 'bufsize' has enough space for the header and
terminator, although that's potentially a dtc bug. I read dtc to make sure
the method we use to calculate the size was accurate. There probably should
be a method in dtc as we rely on some implementation details, however they
are quite simple and we can get: sizeof(header) + sizeof(structs) +
sizeof(strings) + terminator using public APIs and basic knowledge of the
serialised device-tree form.

> Anyway, here is the rebased version of this patch using the new
'qemu_fdt_totalsize' method in the patch I just sent.

> -
https://github.com/riscv/riscv-qemu/commit/a65f6e0447d6e32d75f64ba31df5f20d529d0489

> I have a feeling this is the patch that fixes sifive_u. Did you bisect
which patch in the series fixed sifive_u?

I agree, I think this is the patch.

No, I havent' bisected it. I didn't think there was much point, but if we
want patches backported to stable I guess there is. I'll dig into it.


> I have to send a pull for the reviewed patches which I can do today, but
this is one of the patches that is early in the series that does not yet
have Reviewed-by. When I split the series this patch would be in a second
series that i'll have to link to the pull in patchew (using the method
Peter mentioned) or wait until the pull is accepted.

Great, let's get the first part of the series merged. It'd be nice to send
out the next version of the second part while the PR is being merged.

Alistair


> Michael.

Alistair Francis May 4, 2018, 11:54 p.m. UTC | #7

On Fri, May 4, 2018 at 4:44 PM Alistair Francis <alistair23@gmail.com>
wrote:

> On Thu, May 3, 2018 at 6:45 PM Michael Clark <mjc@sifive.com> wrote:



> > On Sat, Apr 28, 2018 at 4:17 AM, Alistair Francis <alistair23@gmail.com>
> wrote:

> >> On Thu, Apr 26, 2018 at 10:34 PM Michael Clark <mjc@sifive.com> wrote:



> >> > On Fri, Apr 27, 2018 at 5:22 PM, Michael Clark <mjc@sifive.com>
wrote:



> >> >> On Fri, Apr 27, 2018 at 4:48 AM, Alistair Francis <
> alistair23@gmail.com>
> >> wrote:

> >> >>> On Wed, Apr 25, 2018 at 5:03 PM Michael Clark <mjc@sifive.com>
wrote:

> >> >>> > The sifive_u machine already marks its ROM readonly. This fixes
> >> >>> > the remaining boards. This commit also makes all boards use
> >> >>> > mask_rom as the variable name for the ROM. This change also
> >> >>> > makes space for the maximum device tree size size and adds
> >> >>> > an explicit bounds check and error message.

> >> >>> > Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
> >> >>> > Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> >> >>> > Cc: Palmer Dabbelt <palmer@sifive.com>
> >> >>> > Cc: Alistair Francis <Alistair.Francis@wdc.com>
> >> >>> > Signed-off-by: Michael Clark <mjc@sifive.com>
> >> >>> > ---
> >> >>> >   hw/riscv/sifive_e.c     | 20 +++++++---------
> >> >>> >   hw/riscv/sifive_u.c     | 46
++++++++++++++++++-----------------
> >> >>> >   hw/riscv/spike.c        | 64
> >> >>> ++++++++++++++++++++++++++++---------------------
> >> >>> >   hw/riscv/virt.c         | 38 +++++++++++++++--------------
> >> >>> >   include/hw/riscv/virt.h |  4 ++++
> >> >>> >   5 files changed, 93 insertions(+), 79 deletions(-)

> >> >>> > diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> >> >>> > index 39e4cb4..0c8b8e9 100644
> >> >>> > --- a/hw/riscv/sifive_e.c
> >> >>> > +++ b/hw/riscv/sifive_e.c
> >> >>> > @@ -74,14 +74,6 @@ static const struct MemmapEntry {
> >> >>> >       [SIFIVE_E_DTIM] =     { 0x80000000,     0x4000 }
> >> >>> >   };

> >> >>> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t
> len)
> >> >>> > -{
> >> >>> > -    int i;
> >> >>> > -    for (i = 0; i < (len >> 2); i++) {
> >> >>> > -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
> >> >>> > -    }
> >> >>> > -}
> >> >>> > -
> >> >>> >   static uint64_t load_kernel(const char *kernel_filename)
> >> >>> >   {
> >> >>> >       uint64_t kernel_entry, kernel_high;
> >> >>> > @@ -112,6 +104,7 @@ static void riscv_sifive_e_init(MachineState
> >> *machine)
> >> >>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> >> >>> >       MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> >> >>> >       MemoryRegion *xip_mem = g_new(MemoryRegion, 1);
> >> >>> > +    int i;

> >> >>> >       /* Initialize SOC */
> >> >>> >       object_initialize(&s->soc, sizeof(s->soc),
> >> TYPE_RISCV_HART_ARRAY);
> >> >>> > @@ -131,7 +124,7 @@ static void riscv_sifive_e_init(MachineState
> >> *machine)
> >> >>> >           memmap[SIFIVE_E_DTIM].base, main_mem);

> >> >>> >       /* Mask ROM */
> >> >>> > -    memory_region_init_ram(mask_rom, NULL,
"riscv.sifive.e.mrom",
> >> >>> > +    memory_region_init_rom(mask_rom, NULL,
"riscv.sifive.e.mrom",
> >> >>> >           memmap[SIFIVE_E_MROM].size, &error_fatal);
> >> >>> >       memory_region_add_subregion(sys_mem,
> >> >>> >           memmap[SIFIVE_E_MROM].base, mask_rom);
> >> >>> > @@ -185,9 +178,12 @@ static void riscv_sifive_e_init(MachineState
> >> >>> *machine)
> >> >>> >           0x00028067,        /* 0x1004: jr      t0 */
> >> >>> >       };

> >> >>> > -    /* copy in the reset vector */
> >> >>> > -    copy_le32_to_phys(memmap[SIFIVE_E_MROM].base, reset_vec,
> >> >>> sizeof(reset_vec));
> >> >>> > -    memory_region_set_readonly(mask_rom, true);
> >> >>> > +    /* copy in the reset vector in little_endian byte order */
> >> >>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> >> >>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> >> >>> > +    }
> >> >>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec,
> sizeof(reset_vec),
> >> >>> > +                          memmap[SIFIVE_E_MROM].base,
> >> >>> &address_space_memory);

> >> >>> >       if (machine->kernel_filename) {
> >> >>> >           load_kernel(machine->kernel_filename);
> >> >>> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> >> >>> > index 115618b..11ba4c3 100644
> >> >>> > --- a/hw/riscv/sifive_u.c
> >> >>> > +++ b/hw/riscv/sifive_u.c
> >> >>> > @@ -52,7 +52,7 @@ static const struct MemmapEntry {
> >> >>> >       hwaddr size;
> >> >>> >   } sifive_u_memmap[] = {
> >> >>> >       [SIFIVE_U_DEBUG] =    {        0x0,      0x100 },
> >> >>> > -    [SIFIVE_U_MROM] =     {     0x1000,     0x2000 },
> >> >>> > +    [SIFIVE_U_MROM] =     {     0x1000,    0x11000 },
> >> >>> >       [SIFIVE_U_CLINT] =    {  0x2000000,    0x10000 },
> >> >>> >       [SIFIVE_U_PLIC] =     {  0xc000000,  0x4000000 },
> >> >>> >       [SIFIVE_U_UART0] =    { 0x10013000,     0x1000 },
> >> >>> > @@ -60,14 +60,6 @@ static const struct MemmapEntry {
> >> >>> >       [SIFIVE_U_DRAM] =     { 0x80000000,        0x0 },
> >> >>> >   };

> >> >>> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t
> len)
> >> >>> > -{
> >> >>> > -    int i;
> >> >>> > -    for (i = 0; i < (len >> 2); i++) {
> >> >>> > -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
> >> >>> > -    }
> >> >>> > -}
> >> >>> > -
> >> >>> >   static uint64_t load_kernel(const char *kernel_filename)
> >> >>> >   {
> >> >>> >       uint64_t kernel_entry, kernel_high;
> >> >>> > @@ -221,9 +213,10 @@ static void riscv_sifive_u_init(MachineState
> >> >>> *machine)
> >> >>> >       const struct MemmapEntry *memmap = sifive_u_memmap;

> >> >>> >       SiFiveUState *s = g_new0(SiFiveUState, 1);
> >> >>> > -    MemoryRegion *sys_memory = get_system_memory();
> >> >>> > +    MemoryRegion *system_memory = get_system_memory();
> >> >>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> >> >>> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
> >> >>> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> >> >>> > +    int i;

> >> >>> >       /* Initialize SOC */
> >> >>> >       object_initialize(&s->soc, sizeof(s->soc),
> >> TYPE_RISCV_HART_ARRAY);
> >> >>> > @@ -239,17 +232,17 @@ static void
riscv_sifive_u_init(MachineState
> >> >>> *machine)
> >> >>> >       /* register RAM */
> >> >>> >       memory_region_init_ram(main_mem, NULL,
"riscv.sifive.u.ram",
> >> >>> >                              machine->ram_size, &error_fatal);
> >> >>> > -    memory_region_add_subregion(sys_memory,
> >> memmap[SIFIVE_U_DRAM].base,
> >> >>> > +    memory_region_add_subregion(system_memory,
> >> >>> memmap[SIFIVE_U_DRAM].base,
> >> >>> >           main_mem);

> >> >>> >       /* create device tree */
> >> >>> >       create_fdt(s, memmap, machine->ram_size,
> >> machine->kernel_cmdline);

> >> >>> >       /* boot rom */
> >> >>> > -    memory_region_init_ram(boot_rom, NULL,
"riscv.sifive.u.mrom",
> >> >>> > -                           memmap[SIFIVE_U_MROM].base,
> &error_fatal);
> >> >>> > -    memory_region_set_readonly(boot_rom, true);
> >> >>> > -    memory_region_add_subregion(sys_memory, 0x0, boot_rom);
> >> >>> > +    memory_region_init_rom(mask_rom, NULL,
"riscv.sifive.u.mrom",
> >> >>> > +                           memmap[SIFIVE_U_MROM].size,
> &error_fatal);
> >> >>> > +    memory_region_add_subregion(system_memory,
> >> >>> memmap[SIFIVE_U_MROM].base,
> >> >>> > +                                mask_rom);

> >> >>> >       if (machine->kernel_filename) {
> >> >>> >           load_kernel(machine->kernel_filename);
> >> >>> > @@ -272,13 +265,22 @@ static void
riscv_sifive_u_init(MachineState
> >> >>> *machine)
> >> >>> >                                          /* dtb: */
> >> >>> >       };

> >> >>> > -    /* copy in the reset vector */
> >> >>> > -    copy_le32_to_phys(memmap[SIFIVE_U_MROM].base, reset_vec,
> >> >>> sizeof(reset_vec));
> >> >>> > +    /* copy in the reset vector in little_endian byte order */
> >> >>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> >> >>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> >> >>> > +    }
> >> >>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec,
> sizeof(reset_vec),
> >> >>> > +                          memmap[SIFIVE_U_MROM].base,
> >> >>> &address_space_memory);

> >> >>> >       /* copy in the device tree */
> >> >>> > +    if (s->fdt_size >= memmap[SIFIVE_U_MROM].size -
> >> sizeof(reset_vec)) {
> >> >>> > +        error_report("qemu: not enough space to store
> device-tree");
> >> >>> > +        exit(1);
> >> >>> > +    }
> >> >>> >       qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
> >> >>> > -    cpu_physical_memory_write(memmap[SIFIVE_U_MROM].base +
> >> >>> > -        sizeof(reset_vec), s->fdt, s->fdt_size);
> >> >>> > +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, s->fdt_size,
> >> >>> > +                          memmap[SIFIVE_U_MROM].base +
> >> sizeof(reset_vec),
> >> >>> > +                          &address_space_memory);

> >> >>> >       /* MMIO */
> >> >>> >       s->plic = sifive_plic_create(memmap[SIFIVE_U_PLIC].base,
> >> >>> > @@ -292,9 +294,9 @@ static void riscv_sifive_u_init(MachineState
> >> *machine)
> >> >>> >           SIFIVE_U_PLIC_CONTEXT_BASE,
> >> >>> >           SIFIVE_U_PLIC_CONTEXT_STRIDE,
> >> >>> >           memmap[SIFIVE_U_PLIC].size);
> >> >>> > -    sifive_uart_create(sys_memory, memmap[SIFIVE_U_UART0].base,
> >> >>> > +    sifive_uart_create(system_memory,
memmap[SIFIVE_U_UART0].base,
> >> >>> >           serial_hds[0],
> >> SIFIVE_PLIC(s->plic)->irqs[SIFIVE_U_UART0_IRQ]);
> >> >>> > -    /* sifive_uart_create(sys_memory,
memmap[SIFIVE_U_UART1].base,
> >> >>> > +    /* sifive_uart_create(system_memory,
> memmap[SIFIVE_U_UART1].base,
> >> >>> >           serial_hds[1],
> >> SIFIVE_PLIC(s->plic)->irqs[SIFIVE_U_UART1_IRQ]);
> >> >>> */
> >> >>> >       sifive_clint_create(memmap[SIFIVE_U_CLINT].base,
> >> >>> >           memmap[SIFIVE_U_CLINT].size, smp_cpus,
> >> >>> > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> >> >>> > index 3f6bd0a..d1dbe6b 100644
> >> >>> > --- a/hw/riscv/spike.c
> >> >>> > +++ b/hw/riscv/spike.c
> >> >>> > @@ -46,19 +46,11 @@ static const struct MemmapEntry {
> >> >>> >       hwaddr base;
> >> >>> >       hwaddr size;
> >> >>> >   } spike_memmap[] = {
> >> >>> > -    [SPIKE_MROM] =     {     0x1000,     0x2000 },
> >> >>> > +    [SPIKE_MROM] =     {     0x1000,    0x11000 },
> >> >>> >       [SPIKE_CLINT] =    {  0x2000000,    0x10000 },
> >> >>> >       [SPIKE_DRAM] =     { 0x80000000,        0x0 },
> >> >>> >   };

> >> >>> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t
> len)
> >> >>> > -{
> >> >>> > -    int i;
> >> >>> > -    for (i = 0; i < (len >> 2); i++) {
> >> >>> > -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
> >> >>> > -    }
> >> >>> > -}
> >> >>> > -
> >> >>> >   static uint64_t load_kernel(const char *kernel_filename)
> >> >>> >   {
> >> >>> >       uint64_t kernel_entry, kernel_high;
> >> >>> > @@ -173,7 +165,8 @@ static void
> spike_v1_10_0_board_init(MachineState
> >> >>> *machine)
> >> >>> >       SpikeState *s = g_new0(SpikeState, 1);
> >> >>> >       MemoryRegion *system_memory = get_system_memory();
> >> >>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> >> >>> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
> >> >>> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> >> >>> > +    int i;

> >> >>> >       /* Initialize SOC */
> >> >>> >       object_initialize(&s->soc, sizeof(s->soc),
> >> TYPE_RISCV_HART_ARRAY);
> >> >>> > @@ -196,9 +189,10 @@ static void
> spike_v1_10_0_board_init(MachineState
> >> >>> *machine)
> >> >>> >       create_fdt(s, memmap, machine->ram_size,
> >> machine->kernel_cmdline);

> >> >>> >       /* boot rom */
> >> >>> > -    memory_region_init_ram(boot_rom, NULL,
"riscv.spike.bootrom",
> >> >>> > -                           s->fdt_size + 0x2000, &error_fatal);
> >> >>> > -    memory_region_add_subregion(system_memory, 0x0, boot_rom);
> >> >>> > +    memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
> >> >>> > +                           memmap[SPIKE_MROM].size,
&error_fatal);
> >> >>> > +    memory_region_add_subregion(system_memory,
> >> memmap[SPIKE_MROM].base,
> >> >>> > +                                mask_rom);

> >> >>> >       if (machine->kernel_filename) {
> >> >>> >           load_kernel(machine->kernel_filename);
> >> >>> > @@ -221,16 +215,25 @@ static void
> >> spike_v1_10_0_board_init(MachineState
> >> >>> *machine)
> >> >>> >                                        /* dtb: */
> >> >>> >       };

> >> >>> > -    /* copy in the reset vector */
> >> >>> > -    copy_le32_to_phys(memmap[SPIKE_MROM].base, reset_vec,
> >> >>> sizeof(reset_vec));
> >> >>> > +    /* copy in the reset vector in little_endian byte order */
> >> >>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> >> >>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> >> >>> > +    }
> >> >>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec,
> sizeof(reset_vec),
> >> >>> > +                          memmap[SPIKE_MROM].base,
> >> >>> &address_space_memory);

> >> >>> >       /* copy in the device tree */
> >> >>> > +    if (s->fdt_size >= memmap[SPIKE_MROM].size -
> sizeof(reset_vec)) {
> >> >>> > +        error_report("qemu: not enough space to store
> device-tree");
> >> >>> > +        exit(1);
> >> >>> > +    }
> >> >>> >       qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
> >> >>> > -    cpu_physical_memory_write(memmap[SPIKE_MROM].base +
> >> >>> sizeof(reset_vec),
> >> >>> > -        s->fdt, s->fdt_size);
> >> >>> > +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, s->fdt_size,
> >> >>> > +                          memmap[SPIKE_MROM].base +
> >> sizeof(reset_vec),
> >> >>> > +                          &address_space_memory);

> >> >>> >       /* initialize HTIF using symbols found in load_kernel */
> >> >>> > -    htif_mm_init(system_memory, boot_rom, &s->soc.harts[0].env,
> >> >>> serial_hds[0]);
> >> >>> > +    htif_mm_init(system_memory, mask_rom, &s->soc.harts[0].env,
> >> >>> serial_hds[0]);

> >> >>> >       /* Core Local Interruptor (timer and IPI) */
> >> >>> >       sifive_clint_create(memmap[SPIKE_CLINT].base,
> >> >>> memmap[SPIKE_CLINT].size,
> >> >>> > @@ -244,7 +247,8 @@ static void
> spike_v1_09_1_board_init(MachineState
> >> >>> *machine)
> >> >>> >       SpikeState *s = g_new0(SpikeState, 1);
> >> >>> >       MemoryRegion *system_memory = get_system_memory();
> >> >>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> >> >>> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
> >> >>> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> >> >>> > +    int i;

> >> >>> >       /* Initialize SOC */
> >> >>> >       object_initialize(&s->soc, sizeof(s->soc),
> >> TYPE_RISCV_HART_ARRAY);
> >> >>> > @@ -264,9 +268,10 @@ static void
> spike_v1_09_1_board_init(MachineState
> >> >>> *machine)
> >> >>> >           main_mem);

> >> >>> >       /* boot rom */
> >> >>> > -    memory_region_init_ram(boot_rom, NULL,
"riscv.spike.bootrom",
> >> >>> > -                           0x40000, &error_fatal);
> >> >>> > -    memory_region_add_subregion(system_memory, 0x0, boot_rom);
> >> >>> > +    memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
> >> >>> > +                           memmap[SPIKE_MROM].size,
&error_fatal);
> >> >>> > +    memory_region_add_subregion(system_memory,
> >> memmap[SPIKE_MROM].base,
> >> >>> > +                                mask_rom);

> >> >>> >       if (machine->kernel_filename) {
> >> >>> >           load_kernel(machine->kernel_filename);
> >> >>> > @@ -319,15 +324,20 @@ static void
> >> spike_v1_09_1_board_init(MachineState
> >> >>> *machine)
> >> >>> >       g_free(isa);
> >> >>> >       size_t config_string_len = strlen(config_string);

> >> >>> > -    /* copy in the reset vector */
> >> >>> > -    copy_le32_to_phys(memmap[SPIKE_MROM].base, reset_vec,
> >> >>> sizeof(reset_vec));
> >> >>> > +    /* copy in the reset vector in little_endian byte order */
> >> >>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> >> >>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> >> >>> > +    }
> >> >>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec,
> sizeof(reset_vec),
> >> >>> > +                          memmap[SPIKE_MROM].base,
> >> >>> &address_space_memory);

> >> >>> >       /* copy in the config string */
> >> >>> > -    cpu_physical_memory_write(memmap[SPIKE_MROM].base +
> >> >>> sizeof(reset_vec),
> >> >>> > -        config_string, config_string_len);
> >> >>> > +    rom_add_blob_fixed_as("mrom.reset", config_string,
> >> config_string_len,
> >> >>> > +                          memmap[SPIKE_MROM].base +
> >> sizeof(reset_vec),
> >> >>> > +                          &address_space_memory);

> >> >>> >       /* initialize HTIF using symbols found in load_kernel */
> >> >>> > -    htif_mm_init(system_memory, boot_rom, &s->soc.harts[0].env,
> >> >>> serial_hds[0]);
> >> >>> > +    htif_mm_init(system_memory, mask_rom, &s->soc.harts[0].env,
> >> >>> serial_hds[0]);

> >> >>> >       /* Core Local Interruptor (timer and IPI) */
> >> >>> >       sifive_clint_create(memmap[SPIKE_CLINT].base,
> >> >>> memmap[SPIKE_CLINT].size,
> >> >>> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> >> >>> > index 090befe..20c509d 100644
> >> >>> > --- a/hw/riscv/virt.c
> >> >>> > +++ b/hw/riscv/virt.c
> >> >>> > @@ -45,8 +45,8 @@ static const struct MemmapEntry {
> >> >>> >       hwaddr size;
> >> >>> >   } virt_memmap[] = {
> >> >>> >       [VIRT_DEBUG] =    {        0x0,      0x100 },
> >> >>> > -    [VIRT_MROM] =     {     0x1000,     0x2000 },
> >> >>> > -    [VIRT_TEST] =     {     0x4000,     0x1000 },
> >> >>> > +    [VIRT_MROM] =     {     0x1000,    0x11000 },
> >> >>> > +    [VIRT_TEST] =     {   0x100000,     0x1000 },
> >> >>> >       [VIRT_CLINT] =    {  0x2000000,    0x10000 },
> >> >>> >       [VIRT_PLIC] =     {  0xc000000,  0x4000000 },
> >> >>> >       [VIRT_UART0] =    { 0x10000000,      0x100 },
> >> >>> > @@ -54,14 +54,6 @@ static const struct MemmapEntry {
> >> >>> >       [VIRT_DRAM] =     { 0x80000000,        0x0 },
> >> >>> >   };

> >> >>> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t
> len)
> >> >>> > -{
> >> >>> > -    int i;
> >> >>> > -    for (i = 0; i < (len >> 2); i++) {
> >> >>> > -        stl_phys(&address_space_memory, pa + (i << 2), rom[i]);
> >> >>> > -    }
> >> >>> > -}
> >> >>> > -
> >> >>> >   static uint64_t load_kernel(const char *kernel_filename)
> >> >>> >   {
> >> >>> >       uint64_t kernel_entry, kernel_high;
> >> >>> > @@ -272,7 +264,7 @@ static void
riscv_virt_board_init(MachineState
> >> >>> *machine)
> >> >>> >       RISCVVirtState *s = g_new0(RISCVVirtState, 1);
> >> >>> >       MemoryRegion *system_memory = get_system_memory();
> >> >>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> >> >>> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
> >> >>> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> >> >>> >       char *plic_hart_config;
> >> >>> >       size_t plic_hart_config_len;
> >> >>> >       int i;
> >> >>> > @@ -299,9 +291,10 @@ static void
riscv_virt_board_init(MachineState
> >> >>> *machine)
> >> >>> >       fdt = create_fdt(s, memmap, machine->ram_size,
> >> >>> machine->kernel_cmdline);

> >> >>> >       /* boot rom */
> >> >>> > -    memory_region_init_ram(boot_rom, NULL,
> >> "riscv_virt_board.bootrom",
> >> >>> > -                           s->fdt_size + 0x2000, &error_fatal);
> >> >>> > -    memory_region_add_subregion(system_memory, 0x0, boot_rom);
> >> >>> > +    memory_region_init_rom(mask_rom, NULL,
> "riscv_virt_board.mrom",
> >> >>> > +                           memmap[VIRT_MROM].size,
&error_fatal);
> >> >>> > +    memory_region_add_subregion(system_memory,
> >> memmap[VIRT_MROM].base,
> >> >>> > +                                mask_rom);

> >> >>> >       if (machine->kernel_filename) {
> >> >>> >           uint64_t kernel_entry =
> >> load_kernel(machine->kernel_filename);
> >> >>> > @@ -335,13 +328,22 @@ static void
> riscv_virt_board_init(MachineState
> >> >>> *machine)
> >> >>> >                                        /* dtb: */
> >> >>> >       };

> >> >>> > -    /* copy in the reset vector */
> >> >>> > -    copy_le32_to_phys(memmap[VIRT_MROM].base, reset_vec,
> >> >>> sizeof(reset_vec));
> >> >>> > +    /* copy in the reset vector in little_endian byte order */
> >> >>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> >> >>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> >> >>> > +    }
> >> >>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec,
> sizeof(reset_vec),
> >> >>> > +                          memmap[VIRT_MROM].base,
> >> &address_space_memory);

> >> >>> >       /* copy in the device tree */
> >> >>> > +    if (s->fdt_size >= memmap[VIRT_MROM].size -
> sizeof(reset_vec)) {
> >> >>> > +        error_report("qemu: not enough space to store
> device-tree");
> >> >>> > +        exit(1);
> >> >>> > +    }
> >> >>> >       qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
> >> >>> > -    cpu_physical_memory_write(memmap[VIRT_MROM].base +
> >> sizeof(reset_vec),
> >> >>> > -        s->fdt, s->fdt_size);
> >> >>> > +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, s->fdt_size,
> >> >>> > +                          memmap[VIRT_MROM].base +
> sizeof(reset_vec),
> >> >>> > +                          &address_space_memory);

> >> >>> >       /* create PLIC hart topology configuration string */
> >> >>> >       plic_hart_config_len = (strlen(VIRT_PLIC_HART_CONFIG) + 1)
*
> >> >>> smp_cpus;
> >> >>> > diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> >> >>> > index 91163d6..6f2668e 100644
> >> >>> > --- a/include/hw/riscv/virt.h
> >> >>> > +++ b/include/hw/riscv/virt.h
> >> >>> > @@ -19,6 +19,10 @@
> >> >>> >   #ifndef HW_RISCV_VIRT_H
> >> >>> >   #define HW_RISCV_VIRT_H

> >> >>> > +#define TYPE_RISCV_VIRT_BOARD "riscv.virt"
> >> >>> > +#define VIRT(obj) \
> >> >>> > +    OBJECT_CHECK(RISCVVirtState, (obj), TYPE_RISCV_VIRT_BOARD)
> >> >>> > +

> >> >>> This should be in a seperate patch.


> >> >> I'll shift that chunk into "Remove unused class definitions".


> >> > Actually we to need to drop this chunk as the unused check macros
were
> >> removed from machine state structs in "Remove unused class
definitions".
> >> Somehow the chunk made it into this patch. Likely a rebase issue.

> >> > It's probably best that we add what we need back in the QOM SOC
> refactor
> >> on-top of the this series, or at least after the first set of patches
are
> >> merged...

> >> > I think that's what you were planning to do anyway.

> >> Yeah, that works. So just remove it from this series.


> > After rebasing I had to change this patch because of this patch which
> increases the default device tree size to 1MiB. This is not controllable
by
> the user and we don't know how big the resultant device-tree is. It could
> be < 8KiB in our case.

> > -

https://git.qemu.org/?p=qemu.git;a=commit;h=14ec3cbd7c1e31dca4d23f028100c8f43e156573

> > I studied ftd and used public interfaces and a mechanism consistent with
> the fdt resize functions to calculate the size. As far as I can tell it is
> accurate and covers exactly to the end of the uint32 terminator. I needed
> this because our ROMs are currently small.

> > Peter, this is the patch that changes our ROMs from RAM to ROM using
> memory_region_init_rom and rom_add_blob_fixed_as (as per hw/arm/boot.c),
> and it also adds a truncation warning, so that we actually know what size
> the device-tree is, given our ROMs are currently much smaller than 1MiB.
> That is why we needed a method that tells us how big the device tree
> actually is.

> > BTW I'm actually suspicious of 'fdt_resize' here:

> > -

https://git.qemu.org/?p=dtc.git;a=blob;f=libfdt/fdt_sw.c;h=6d33cc29d0224d9fc6307607ef7563df944da2d3

> > as it doesn't check that 'bufsize' has enough space for the header and
> terminator, although that's potentially a dtc bug. I read dtc to make sure
> the method we use to calculate the size was accurate. There probably
should
> be a method in dtc as we rely on some implementation details, however they
> are quite simple and we can get: sizeof(header) + sizeof(structs) +
> sizeof(strings) + terminator using public APIs and basic knowledge of the
> serialised device-tree form.

> > Anyway, here is the rebased version of this patch using the new
> 'qemu_fdt_totalsize' method in the patch I just sent.

> > -

https://github.com/riscv/riscv-qemu/commit/a65f6e0447d6e32d75f64ba31df5f20d529d0489

> > I have a feeling this is the patch that fixes sifive_u. Did you bisect
> which patch in the series fixed sifive_u?

> I agree, I think this is the patch.

> No, I havent' bisected it. I didn't think there was much point, but if we
> want patches backported to stable I guess there is. I'll dig into it.

Yep, just checked this patch is the first patch in which the SiFive U
machine works.

If this patch is rebased onto master if also works, so only this patch is
required.

Alistair



> > I have to send a pull for the reviewed patches which I can do today, but
> this is one of the patches that is early in the series that does not yet
> have Reviewed-by. When I split the series this patch would be in a second
> series that i'll have to link to the pull in patchew (using the method
> Peter mentioned) or wait until the pull is accepted.

> Great, let's get the first part of the series merged. It'd be nice to send
> out the next version of the second part while the PR is being merged.

> Alistair


> > Michael.

Michael Clark May 5, 2018, 2:02 a.m. UTC | #8

On Sat, May 5, 2018 at 11:54 AM, Alistair Francis <alistair23@gmail.com>
wrote:

> On Fri, May 4, 2018 at 4:44 PM Alistair Francis <alistair23@gmail.com>
> wrote:
>
> > On Thu, May 3, 2018 at 6:45 PM Michael Clark <mjc@sifive.com> wrote:
>
>
>
> > > On Sat, Apr 28, 2018 at 4:17 AM, Alistair Francis <
> alistair23@gmail.com>
> > wrote:
>
> > >> On Thu, Apr 26, 2018 at 10:34 PM Michael Clark <mjc@sifive.com>
> wrote:
>
>
>
> > >> > On Fri, Apr 27, 2018 at 5:22 PM, Michael Clark <mjc@sifive.com>
> wrote:
>
>
>
> > >> >> On Fri, Apr 27, 2018 at 4:48 AM, Alistair Francis <
> > alistair23@gmail.com>
> > >> wrote:
>
> > >> >>> On Wed, Apr 25, 2018 at 5:03 PM Michael Clark <mjc@sifive.com>
> wrote:
>
> > >> >>> > The sifive_u machine already marks its ROM readonly. This fixes
> > >> >>> > the remaining boards. This commit also makes all boards use
> > >> >>> > mask_rom as the variable name for the ROM. This change also
> > >> >>> > makes space for the maximum device tree size size and adds
> > >> >>> > an explicit bounds check and error message.
>
> > >> >>> > Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
> > >> >>> > Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> > >> >>> > Cc: Palmer Dabbelt <palmer@sifive.com>
> > >> >>> > Cc: Alistair Francis <Alistair.Francis@wdc.com>
> > >> >>> > Signed-off-by: Michael Clark <mjc@sifive.com>
> > >> >>> > ---
> > >> >>> >   hw/riscv/sifive_e.c     | 20 +++++++---------
> > >> >>> >   hw/riscv/sifive_u.c     | 46
> ++++++++++++++++++-----------------
> > >> >>> >   hw/riscv/spike.c        | 64
> > >> >>> ++++++++++++++++++++++++++++---------------------
> > >> >>> >   hw/riscv/virt.c         | 38 +++++++++++++++--------------
> > >> >>> >   include/hw/riscv/virt.h |  4 ++++
> > >> >>> >   5 files changed, 93 insertions(+), 79 deletions(-)
>
> > >> >>> > diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> > >> >>> > index 39e4cb4..0c8b8e9 100644
> > >> >>> > --- a/hw/riscv/sifive_e.c
> > >> >>> > +++ b/hw/riscv/sifive_e.c
> > >> >>> > @@ -74,14 +74,6 @@ static const struct MemmapEntry {
> > >> >>> >       [SIFIVE_E_DTIM] =     { 0x80000000,     0x4000 }
> > >> >>> >   };
>
> > >> >>> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t
> > len)
> > >> >>> > -{
> > >> >>> > -    int i;
> > >> >>> > -    for (i = 0; i < (len >> 2); i++) {
> > >> >>> > -        stl_phys(&address_space_memory, pa + (i << 2),
> rom[i]);
> > >> >>> > -    }
> > >> >>> > -}
> > >> >>> > -
> > >> >>> >   static uint64_t load_kernel(const char *kernel_filename)
> > >> >>> >   {
> > >> >>> >       uint64_t kernel_entry, kernel_high;
> > >> >>> > @@ -112,6 +104,7 @@ static void riscv_sifive_e_init(
> MachineState
> > >> *machine)
> > >> >>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> > >> >>> >       MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> > >> >>> >       MemoryRegion *xip_mem = g_new(MemoryRegion, 1);
> > >> >>> > +    int i;
>
> > >> >>> >       /* Initialize SOC */
> > >> >>> >       object_initialize(&s->soc, sizeof(s->soc),
> > >> TYPE_RISCV_HART_ARRAY);
> > >> >>> > @@ -131,7 +124,7 @@ static void riscv_sifive_e_init(
> MachineState
> > >> *machine)
> > >> >>> >           memmap[SIFIVE_E_DTIM].base, main_mem);
>
> > >> >>> >       /* Mask ROM */
> > >> >>> > -    memory_region_init_ram(mask_rom, NULL,
> "riscv.sifive.e.mrom",
> > >> >>> > +    memory_region_init_rom(mask_rom, NULL,
> "riscv.sifive.e.mrom",
> > >> >>> >           memmap[SIFIVE_E_MROM].size, &error_fatal);
> > >> >>> >       memory_region_add_subregion(sys_mem,
> > >> >>> >           memmap[SIFIVE_E_MROM].base, mask_rom);
> > >> >>> > @@ -185,9 +178,12 @@ static void riscv_sifive_e_init(
> MachineState
> > >> >>> *machine)
> > >> >>> >           0x00028067,        /* 0x1004: jr      t0 */
> > >> >>> >       };
>
> > >> >>> > -    /* copy in the reset vector */
> > >> >>> > -    copy_le32_to_phys(memmap[SIFIVE_E_MROM].base, reset_vec,
> > >> >>> sizeof(reset_vec));
> > >> >>> > -    memory_region_set_readonly(mask_rom, true);
> > >> >>> > +    /* copy in the reset vector in little_endian byte order */
> > >> >>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> > >> >>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> > >> >>> > +    }
> > >> >>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec,
> > sizeof(reset_vec),
> > >> >>> > +                          memmap[SIFIVE_E_MROM].base,
> > >> >>> &address_space_memory);
>
> > >> >>> >       if (machine->kernel_filename) {
> > >> >>> >           load_kernel(machine->kernel_filename);
> > >> >>> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> > >> >>> > index 115618b..11ba4c3 100644
> > >> >>> > --- a/hw/riscv/sifive_u.c
> > >> >>> > +++ b/hw/riscv/sifive_u.c
> > >> >>> > @@ -52,7 +52,7 @@ static const struct MemmapEntry {
> > >> >>> >       hwaddr size;
> > >> >>> >   } sifive_u_memmap[] = {
> > >> >>> >       [SIFIVE_U_DEBUG] =    {        0x0,      0x100 },
> > >> >>> > -    [SIFIVE_U_MROM] =     {     0x1000,     0x2000 },
> > >> >>> > +    [SIFIVE_U_MROM] =     {     0x1000,    0x11000 },
> > >> >>> >       [SIFIVE_U_CLINT] =    {  0x2000000,    0x10000 },
> > >> >>> >       [SIFIVE_U_PLIC] =     {  0xc000000,  0x4000000 },
> > >> >>> >       [SIFIVE_U_UART0] =    { 0x10013000,     0x1000 },
> > >> >>> > @@ -60,14 +60,6 @@ static const struct MemmapEntry {
> > >> >>> >       [SIFIVE_U_DRAM] =     { 0x80000000,        0x0 },
> > >> >>> >   };
>
> > >> >>> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t
> > len)
> > >> >>> > -{
> > >> >>> > -    int i;
> > >> >>> > -    for (i = 0; i < (len >> 2); i++) {
> > >> >>> > -        stl_phys(&address_space_memory, pa + (i << 2),
> rom[i]);
> > >> >>> > -    }
> > >> >>> > -}
> > >> >>> > -
> > >> >>> >   static uint64_t load_kernel(const char *kernel_filename)
> > >> >>> >   {
> > >> >>> >       uint64_t kernel_entry, kernel_high;
> > >> >>> > @@ -221,9 +213,10 @@ static void riscv_sifive_u_init(
> MachineState
> > >> >>> *machine)
> > >> >>> >       const struct MemmapEntry *memmap = sifive_u_memmap;
>
> > >> >>> >       SiFiveUState *s = g_new0(SiFiveUState, 1);
> > >> >>> > -    MemoryRegion *sys_memory = get_system_memory();
> > >> >>> > +    MemoryRegion *system_memory = get_system_memory();
> > >> >>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> > >> >>> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
> > >> >>> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> > >> >>> > +    int i;
>
> > >> >>> >       /* Initialize SOC */
> > >> >>> >       object_initialize(&s->soc, sizeof(s->soc),
> > >> TYPE_RISCV_HART_ARRAY);
> > >> >>> > @@ -239,17 +232,17 @@ static void
> riscv_sifive_u_init(MachineState
> > >> >>> *machine)
> > >> >>> >       /* register RAM */
> > >> >>> >       memory_region_init_ram(main_mem, NULL,
> "riscv.sifive.u.ram",
> > >> >>> >                              machine->ram_size, &error_fatal);
> > >> >>> > -    memory_region_add_subregion(sys_memory,
> > >> memmap[SIFIVE_U_DRAM].base,
> > >> >>> > +    memory_region_add_subregion(system_memory,
> > >> >>> memmap[SIFIVE_U_DRAM].base,
> > >> >>> >           main_mem);
>
> > >> >>> >       /* create device tree */
> > >> >>> >       create_fdt(s, memmap, machine->ram_size,
> > >> machine->kernel_cmdline);
>
> > >> >>> >       /* boot rom */
> > >> >>> > -    memory_region_init_ram(boot_rom, NULL,
> "riscv.sifive.u.mrom",
> > >> >>> > -                           memmap[SIFIVE_U_MROM].base,
> > &error_fatal);
> > >> >>> > -    memory_region_set_readonly(boot_rom, true);
> > >> >>> > -    memory_region_add_subregion(sys_memory, 0x0, boot_rom);
> > >> >>> > +    memory_region_init_rom(mask_rom, NULL,
> "riscv.sifive.u.mrom",
> > >> >>> > +                           memmap[SIFIVE_U_MROM].size,
> > &error_fatal);
> > >> >>> > +    memory_region_add_subregion(system_memory,
> > >> >>> memmap[SIFIVE_U_MROM].base,
> > >> >>> > +                                mask_rom);
>
> > >> >>> >       if (machine->kernel_filename) {
> > >> >>> >           load_kernel(machine->kernel_filename);
> > >> >>> > @@ -272,13 +265,22 @@ static void
> riscv_sifive_u_init(MachineState
> > >> >>> *machine)
> > >> >>> >                                          /* dtb: */
> > >> >>> >       };
>
> > >> >>> > -    /* copy in the reset vector */
> > >> >>> > -    copy_le32_to_phys(memmap[SIFIVE_U_MROM].base, reset_vec,
> > >> >>> sizeof(reset_vec));
> > >> >>> > +    /* copy in the reset vector in little_endian byte order */
> > >> >>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> > >> >>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> > >> >>> > +    }
> > >> >>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec,
> > sizeof(reset_vec),
> > >> >>> > +                          memmap[SIFIVE_U_MROM].base,
> > >> >>> &address_space_memory);
>
> > >> >>> >       /* copy in the device tree */
> > >> >>> > +    if (s->fdt_size >= memmap[SIFIVE_U_MROM].size -
> > >> sizeof(reset_vec)) {
> > >> >>> > +        error_report("qemu: not enough space to store
> > device-tree");
> > >> >>> > +        exit(1);
> > >> >>> > +    }
> > >> >>> >       qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
> > >> >>> > -    cpu_physical_memory_write(memmap[SIFIVE_U_MROM].base +
> > >> >>> > -        sizeof(reset_vec), s->fdt, s->fdt_size);
> > >> >>> > +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, s->fdt_size,
> > >> >>> > +                          memmap[SIFIVE_U_MROM].base +
> > >> sizeof(reset_vec),
> > >> >>> > +                          &address_space_memory);
>
> > >> >>> >       /* MMIO */
> > >> >>> >       s->plic = sifive_plic_create(memmap[SIFIVE_U_PLIC].base,
> > >> >>> > @@ -292,9 +294,9 @@ static void riscv_sifive_u_init(
> MachineState
> > >> *machine)
> > >> >>> >           SIFIVE_U_PLIC_CONTEXT_BASE,
> > >> >>> >           SIFIVE_U_PLIC_CONTEXT_STRIDE,
> > >> >>> >           memmap[SIFIVE_U_PLIC].size);
> > >> >>> > -    sifive_uart_create(sys_memory, memmap[SIFIVE_U_UART0].base,
> > >> >>> > +    sifive_uart_create(system_memory,
> memmap[SIFIVE_U_UART0].base,
> > >> >>> >           serial_hds[0],
> > >> SIFIVE_PLIC(s->plic)->irqs[SIFIVE_U_UART0_IRQ]);
> > >> >>> > -    /* sifive_uart_create(sys_memory,
> memmap[SIFIVE_U_UART1].base,
> > >> >>> > +    /* sifive_uart_create(system_memory,
> > memmap[SIFIVE_U_UART1].base,
> > >> >>> >           serial_hds[1],
> > >> SIFIVE_PLIC(s->plic)->irqs[SIFIVE_U_UART1_IRQ]);
> > >> >>> */
> > >> >>> >       sifive_clint_create(memmap[SIFIVE_U_CLINT].base,
> > >> >>> >           memmap[SIFIVE_U_CLINT].size, smp_cpus,
> > >> >>> > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> > >> >>> > index 3f6bd0a..d1dbe6b 100644
> > >> >>> > --- a/hw/riscv/spike.c
> > >> >>> > +++ b/hw/riscv/spike.c
> > >> >>> > @@ -46,19 +46,11 @@ static const struct MemmapEntry {
> > >> >>> >       hwaddr base;
> > >> >>> >       hwaddr size;
> > >> >>> >   } spike_memmap[] = {
> > >> >>> > -    [SPIKE_MROM] =     {     0x1000,     0x2000 },
> > >> >>> > +    [SPIKE_MROM] =     {     0x1000,    0x11000 },
> > >> >>> >       [SPIKE_CLINT] =    {  0x2000000,    0x10000 },
> > >> >>> >       [SPIKE_DRAM] =     { 0x80000000,        0x0 },
> > >> >>> >   };
>
> > >> >>> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t
> > len)
> > >> >>> > -{
> > >> >>> > -    int i;
> > >> >>> > -    for (i = 0; i < (len >> 2); i++) {
> > >> >>> > -        stl_phys(&address_space_memory, pa + (i << 2),
> rom[i]);
> > >> >>> > -    }
> > >> >>> > -}
> > >> >>> > -
> > >> >>> >   static uint64_t load_kernel(const char *kernel_filename)
> > >> >>> >   {
> > >> >>> >       uint64_t kernel_entry, kernel_high;
> > >> >>> > @@ -173,7 +165,8 @@ static void
> > spike_v1_10_0_board_init(MachineState
> > >> >>> *machine)
> > >> >>> >       SpikeState *s = g_new0(SpikeState, 1);
> > >> >>> >       MemoryRegion *system_memory = get_system_memory();
> > >> >>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> > >> >>> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
> > >> >>> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> > >> >>> > +    int i;
>
> > >> >>> >       /* Initialize SOC */
> > >> >>> >       object_initialize(&s->soc, sizeof(s->soc),
> > >> TYPE_RISCV_HART_ARRAY);
> > >> >>> > @@ -196,9 +189,10 @@ static void
> > spike_v1_10_0_board_init(MachineState
> > >> >>> *machine)
> > >> >>> >       create_fdt(s, memmap, machine->ram_size,
> > >> machine->kernel_cmdline);
>
> > >> >>> >       /* boot rom */
> > >> >>> > -    memory_region_init_ram(boot_rom, NULL,
> "riscv.spike.bootrom",
> > >> >>> > -                           s->fdt_size + 0x2000, &error_fatal);
> > >> >>> > -    memory_region_add_subregion(system_memory, 0x0, boot_rom);
> > >> >>> > +    memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
> > >> >>> > +                           memmap[SPIKE_MROM].size,
> &error_fatal);
> > >> >>> > +    memory_region_add_subregion(system_memory,
> > >> memmap[SPIKE_MROM].base,
> > >> >>> > +                                mask_rom);
>
> > >> >>> >       if (machine->kernel_filename) {
> > >> >>> >           load_kernel(machine->kernel_filename);
> > >> >>> > @@ -221,16 +215,25 @@ static void
> > >> spike_v1_10_0_board_init(MachineState
> > >> >>> *machine)
> > >> >>> >                                        /* dtb: */
> > >> >>> >       };
>
> > >> >>> > -    /* copy in the reset vector */
> > >> >>> > -    copy_le32_to_phys(memmap[SPIKE_MROM].base, reset_vec,
> > >> >>> sizeof(reset_vec));
> > >> >>> > +    /* copy in the reset vector in little_endian byte order */
> > >> >>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> > >> >>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> > >> >>> > +    }
> > >> >>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec,
> > sizeof(reset_vec),
> > >> >>> > +                          memmap[SPIKE_MROM].base,
> > >> >>> &address_space_memory);
>
> > >> >>> >       /* copy in the device tree */
> > >> >>> > +    if (s->fdt_size >= memmap[SPIKE_MROM].size -
> > sizeof(reset_vec)) {
> > >> >>> > +        error_report("qemu: not enough space to store
> > device-tree");
> > >> >>> > +        exit(1);
> > >> >>> > +    }
> > >> >>> >       qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
> > >> >>> > -    cpu_physical_memory_write(memmap[SPIKE_MROM].base +
> > >> >>> sizeof(reset_vec),
> > >> >>> > -        s->fdt, s->fdt_size);
> > >> >>> > +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, s->fdt_size,
> > >> >>> > +                          memmap[SPIKE_MROM].base +
> > >> sizeof(reset_vec),
> > >> >>> > +                          &address_space_memory);
>
> > >> >>> >       /* initialize HTIF using symbols found in load_kernel */
> > >> >>> > -    htif_mm_init(system_memory, boot_rom, &s->soc.harts[0].env,
> > >> >>> serial_hds[0]);
> > >> >>> > +    htif_mm_init(system_memory, mask_rom, &s->soc.harts[0].env,
> > >> >>> serial_hds[0]);
>
> > >> >>> >       /* Core Local Interruptor (timer and IPI) */
> > >> >>> >       sifive_clint_create(memmap[SPIKE_CLINT].base,
> > >> >>> memmap[SPIKE_CLINT].size,
> > >> >>> > @@ -244,7 +247,8 @@ static void
> > spike_v1_09_1_board_init(MachineState
> > >> >>> *machine)
> > >> >>> >       SpikeState *s = g_new0(SpikeState, 1);
> > >> >>> >       MemoryRegion *system_memory = get_system_memory();
> > >> >>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> > >> >>> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
> > >> >>> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> > >> >>> > +    int i;
>
> > >> >>> >       /* Initialize SOC */
> > >> >>> >       object_initialize(&s->soc, sizeof(s->soc),
> > >> TYPE_RISCV_HART_ARRAY);
> > >> >>> > @@ -264,9 +268,10 @@ static void
> > spike_v1_09_1_board_init(MachineState
> > >> >>> *machine)
> > >> >>> >           main_mem);
>
> > >> >>> >       /* boot rom */
> > >> >>> > -    memory_region_init_ram(boot_rom, NULL,
> "riscv.spike.bootrom",
> > >> >>> > -                           0x40000, &error_fatal);
> > >> >>> > -    memory_region_add_subregion(system_memory, 0x0, boot_rom);
> > >> >>> > +    memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
> > >> >>> > +                           memmap[SPIKE_MROM].size,
> &error_fatal);
> > >> >>> > +    memory_region_add_subregion(system_memory,
> > >> memmap[SPIKE_MROM].base,
> > >> >>> > +                                mask_rom);
>
> > >> >>> >       if (machine->kernel_filename) {
> > >> >>> >           load_kernel(machine->kernel_filename);
> > >> >>> > @@ -319,15 +324,20 @@ static void
> > >> spike_v1_09_1_board_init(MachineState
> > >> >>> *machine)
> > >> >>> >       g_free(isa);
> > >> >>> >       size_t config_string_len = strlen(config_string);
>
> > >> >>> > -    /* copy in the reset vector */
> > >> >>> > -    copy_le32_to_phys(memmap[SPIKE_MROM].base, reset_vec,
> > >> >>> sizeof(reset_vec));
> > >> >>> > +    /* copy in the reset vector in little_endian byte order */
> > >> >>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> > >> >>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> > >> >>> > +    }
> > >> >>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec,
> > sizeof(reset_vec),
> > >> >>> > +                          memmap[SPIKE_MROM].base,
> > >> >>> &address_space_memory);
>
> > >> >>> >       /* copy in the config string */
> > >> >>> > -    cpu_physical_memory_write(memmap[SPIKE_MROM].base +
> > >> >>> sizeof(reset_vec),
> > >> >>> > -        config_string, config_string_len);
> > >> >>> > +    rom_add_blob_fixed_as("mrom.reset", config_string,
> > >> config_string_len,
> > >> >>> > +                          memmap[SPIKE_MROM].base +
> > >> sizeof(reset_vec),
> > >> >>> > +                          &address_space_memory);
>
> > >> >>> >       /* initialize HTIF using symbols found in load_kernel */
> > >> >>> > -    htif_mm_init(system_memory, boot_rom, &s->soc.harts[0].env,
> > >> >>> serial_hds[0]);
> > >> >>> > +    htif_mm_init(system_memory, mask_rom, &s->soc.harts[0].env,
> > >> >>> serial_hds[0]);
>
> > >> >>> >       /* Core Local Interruptor (timer and IPI) */
> > >> >>> >       sifive_clint_create(memmap[SPIKE_CLINT].base,
> > >> >>> memmap[SPIKE_CLINT].size,
> > >> >>> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > >> >>> > index 090befe..20c509d 100644
> > >> >>> > --- a/hw/riscv/virt.c
> > >> >>> > +++ b/hw/riscv/virt.c
> > >> >>> > @@ -45,8 +45,8 @@ static const struct MemmapEntry {
> > >> >>> >       hwaddr size;
> > >> >>> >   } virt_memmap[] = {
> > >> >>> >       [VIRT_DEBUG] =    {        0x0,      0x100 },
> > >> >>> > -    [VIRT_MROM] =     {     0x1000,     0x2000 },
> > >> >>> > -    [VIRT_TEST] =     {     0x4000,     0x1000 },
> > >> >>> > +    [VIRT_MROM] =     {     0x1000,    0x11000 },
> > >> >>> > +    [VIRT_TEST] =     {   0x100000,     0x1000 },
> > >> >>> >       [VIRT_CLINT] =    {  0x2000000,    0x10000 },
> > >> >>> >       [VIRT_PLIC] =     {  0xc000000,  0x4000000 },
> > >> >>> >       [VIRT_UART0] =    { 0x10000000,      0x100 },
> > >> >>> > @@ -54,14 +54,6 @@ static const struct MemmapEntry {
> > >> >>> >       [VIRT_DRAM] =     { 0x80000000,        0x0 },
> > >> >>> >   };
>
> > >> >>> > -static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t
> > len)
> > >> >>> > -{
> > >> >>> > -    int i;
> > >> >>> > -    for (i = 0; i < (len >> 2); i++) {
> > >> >>> > -        stl_phys(&address_space_memory, pa + (i << 2),
> rom[i]);
> > >> >>> > -    }
> > >> >>> > -}
> > >> >>> > -
> > >> >>> >   static uint64_t load_kernel(const char *kernel_filename)
> > >> >>> >   {
> > >> >>> >       uint64_t kernel_entry, kernel_high;
> > >> >>> > @@ -272,7 +264,7 @@ static void
> riscv_virt_board_init(MachineState
> > >> >>> *machine)
> > >> >>> >       RISCVVirtState *s = g_new0(RISCVVirtState, 1);
> > >> >>> >       MemoryRegion *system_memory = get_system_memory();
> > >> >>> >       MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> > >> >>> > -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
> > >> >>> > +    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> > >> >>> >       char *plic_hart_config;
> > >> >>> >       size_t plic_hart_config_len;
> > >> >>> >       int i;
> > >> >>> > @@ -299,9 +291,10 @@ static void
> riscv_virt_board_init(MachineState
> > >> >>> *machine)
> > >> >>> >       fdt = create_fdt(s, memmap, machine->ram_size,
> > >> >>> machine->kernel_cmdline);
>
> > >> >>> >       /* boot rom */
> > >> >>> > -    memory_region_init_ram(boot_rom, NULL,
> > >> "riscv_virt_board.bootrom",
> > >> >>> > -                           s->fdt_size + 0x2000, &error_fatal);
> > >> >>> > -    memory_region_add_subregion(system_memory, 0x0, boot_rom);
> > >> >>> > +    memory_region_init_rom(mask_rom, NULL,
> > "riscv_virt_board.mrom",
> > >> >>> > +                           memmap[VIRT_MROM].size,
> &error_fatal);
> > >> >>> > +    memory_region_add_subregion(system_memory,
> > >> memmap[VIRT_MROM].base,
> > >> >>> > +                                mask_rom);
>
> > >> >>> >       if (machine->kernel_filename) {
> > >> >>> >           uint64_t kernel_entry =
> > >> load_kernel(machine->kernel_filename);
> > >> >>> > @@ -335,13 +328,22 @@ static void
> > riscv_virt_board_init(MachineState
> > >> >>> *machine)
> > >> >>> >                                        /* dtb: */
> > >> >>> >       };
>
> > >> >>> > -    /* copy in the reset vector */
> > >> >>> > -    copy_le32_to_phys(memmap[VIRT_MROM].base, reset_vec,
> > >> >>> sizeof(reset_vec));
> > >> >>> > +    /* copy in the reset vector in little_endian byte order */
> > >> >>> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> > >> >>> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> > >> >>> > +    }
> > >> >>> > +    rom_add_blob_fixed_as("mrom.reset", reset_vec,
> > sizeof(reset_vec),
> > >> >>> > +                          memmap[VIRT_MROM].base,
> > >> &address_space_memory);
>
> > >> >>> >       /* copy in the device tree */
> > >> >>> > +    if (s->fdt_size >= memmap[VIRT_MROM].size -
> > sizeof(reset_vec)) {
> > >> >>> > +        error_report("qemu: not enough space to store
> > device-tree");
> > >> >>> > +        exit(1);
> > >> >>> > +    }
> > >> >>> >       qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
> > >> >>> > -    cpu_physical_memory_write(memmap[VIRT_MROM].base +
> > >> sizeof(reset_vec),
> > >> >>> > -        s->fdt, s->fdt_size);
> > >> >>> > +    rom_add_blob_fixed_as("mrom.fdt", s->fdt, s->fdt_size,
> > >> >>> > +                          memmap[VIRT_MROM].base +
> > sizeof(reset_vec),
> > >> >>> > +                          &address_space_memory);
>
> > >> >>> >       /* create PLIC hart topology configuration string */
> > >> >>> >       plic_hart_config_len = (strlen(VIRT_PLIC_HART_CONFIG) + 1)
> *
> > >> >>> smp_cpus;
> > >> >>> > diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> > >> >>> > index 91163d6..6f2668e 100644
> > >> >>> > --- a/include/hw/riscv/virt.h
> > >> >>> > +++ b/include/hw/riscv/virt.h
> > >> >>> > @@ -19,6 +19,10 @@
> > >> >>> >   #ifndef HW_RISCV_VIRT_H
> > >> >>> >   #define HW_RISCV_VIRT_H
>
> > >> >>> > +#define TYPE_RISCV_VIRT_BOARD "riscv.virt"
> > >> >>> > +#define VIRT(obj) \
> > >> >>> > +    OBJECT_CHECK(RISCVVirtState, (obj), TYPE_RISCV_VIRT_BOARD)
> > >> >>> > +
>
> > >> >>> This should be in a seperate patch.
>
>
> > >> >> I'll shift that chunk into "Remove unused class definitions".
>
>
> > >> > Actually we to need to drop this chunk as the unused check macros
> were
> > >> removed from machine state structs in "Remove unused class
> definitions".
> > >> Somehow the chunk made it into this patch. Likely a rebase issue.
>
> > >> > It's probably best that we add what we need back in the QOM SOC
> > refactor
> > >> on-top of the this series, or at least after the first set of patches
> are
> > >> merged...
>
> > >> > I think that's what you were planning to do anyway.
>
> > >> Yeah, that works. So just remove it from this series.
>
>
> > > After rebasing I had to change this patch because of this patch which
> > increases the default device tree size to 1MiB. This is not controllable
> by
> > the user and we don't know how big the resultant device-tree is. It could
> > be < 8KiB in our case.
>
> > > -
>
> https://git.qemu.org/?p=qemu.git;a=commit;h=14ec3cbd7c1e31dca4d23f028100c8
> f43e156573
>
> > > I studied ftd and used public interfaces and a mechanism consistent
> with
> > the fdt resize functions to calculate the size. As far as I can tell it
> is
> > accurate and covers exactly to the end of the uint32 terminator. I needed
> > this because our ROMs are currently small.
>
> > > Peter, this is the patch that changes our ROMs from RAM to ROM using
> > memory_region_init_rom and rom_add_blob_fixed_as (as per hw/arm/boot.c),
> > and it also adds a truncation warning, so that we actually know what size
> > the device-tree is, given our ROMs are currently much smaller than 1MiB.
> > That is why we needed a method that tells us how big the device tree
> > actually is.
>
> > > BTW I'm actually suspicious of 'fdt_resize' here:
>
> > > -
>
> https://git.qemu.org/?p=dtc.git;a=blob;f=libfdt/fdt_sw.c;h=
> 6d33cc29d0224d9fc6307607ef7563df944da2d3
>
> > > as it doesn't check that 'bufsize' has enough space for the header and
> > terminator, although that's potentially a dtc bug. I read dtc to make
> sure
> > the method we use to calculate the size was accurate. There probably
> should
> > be a method in dtc as we rely on some implementation details, however
> they
> > are quite simple and we can get: sizeof(header) + sizeof(structs) +
> > sizeof(strings) + terminator using public APIs and basic knowledge of the
> > serialised device-tree form.
>
> > > Anyway, here is the rebased version of this patch using the new
> > 'qemu_fdt_totalsize' method in the patch I just sent.
>
> > > -
>
> https://github.com/riscv/riscv-qemu/commit/a65f6e0447d6e32d75f64ba31df5f2
> 0d529d0489
>
> > > I have a feeling this is the patch that fixes sifive_u. Did you bisect
> > which patch in the series fixed sifive_u?
>
> > I agree, I think this is the patch.
>
> > No, I havent' bisected it. I didn't think there was much point, but if we
> > want patches backported to stable I guess there is. I'll dig into it.
>
> Yep, just checked this patch is the first patch in which the SiFive U
> machine works.
>
> If this patch is rebased onto master if also works, so only this patch is
> required.
>
> Alistair
>
>
>
> > > I have to send a pull for the reviewed patches which I can do today,
> but
> > this is one of the patches that is early in the series that does not yet
> > have Reviewed-by. When I split the series this patch would be in a second
> > series that i'll have to link to the pull in patchew (using the method
> > Peter mentioned) or wait until the pull is accepted.
>
> > Great, let's get the first part of the series merged. It'd be nice to
> send
> > out the next version of the second part while the PR is being merged.
>

Okay, I assume I can add your "Reviewed-by:" to this patch which fixes a
bubble in the early set of sequential patches.

I need to do the PR... however we now depend on review for this patch due
to a recent device_tree change in upstream:

- https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg00892.html

We have small ROMs on these machines. They are smaller than the new 1MiB
default introduced in 14ec3cbd7c1e31dca4d23f028100c8f43e156573 which is now
in master.

I'm disinclined towards bumping the ROM sizes when it is reasonable to want
to be able to query the size of the  device_tree. 1MiB mask ROM is possibly
too large for some small embedded system mask ROMs. However if we can't
merge the qemu_fdt_totalsize patch then we'll need to bump all of the ROM
sizes.

Relying on cpu_physical_memory_write and rom_add_blob_fixed_as to silently
drop writes doesn't allow us to detect device-tree truncation (which
happened to me). I could remove the check and we'd then be relying on
rom_add_blob_fixed_as
adding a block that is larger than the ROM we created from the memory map.

In fact, the warning triggered when I rebased and tested linux in 'virt',
due to the current ROM size, which resulted in me implementing
qemu_fdt_totalsize. It now means we detecting whether the device tree is
going to be truncated versus checking whether the default buffer length
from create_device_tree fits within the ROM.

This is a snippet from the rebased version of "Mark ROM read-only after
copying in code".  I just added one line with "s->fdt_size =
qemu_fdt_totalsize(s->fdt);" before the device tree ROM size check and call
to rom_add_blob_fixed_as, which means we only copy in the actual. I've
tested 'virt' with 8 CPUs (as that's the current limit for CPU stacks in
the bbl firmware) to make sure qemu_fdt_totalsize is working, which it is.

-    /* copy in the reset vector */
-    copy_le32_to_phys(memmap[SIFIVE_U_MROM].base, reset_vec,
sizeof(reset_vec));
+    /* copy in the reset vector in little_endian byte order */
+    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
+        reset_vec[i] = cpu_to_le32(reset_vec[i]);
+    }
+    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
+                          memmap[SIFIVE_U_MROM].base,
&address_space_memory);

     /* copy in the device tree */
+    s->fdt_size = qemu_fdt_totalsize(s->fdt);
+    if (s->fdt_size > memmap[SIFIVE_U_MROM].size - sizeof(reset_vec)) {
+        error_report("not enough space to store device-tree");
+        exit(1);
+    }
     qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
-    cpu_physical_memory_write(memmap[SIFIVE_U_MROM].base +
-        sizeof(reset_vec), s->fdt, s->fdt_size);
+    rom_add_blob_fixed_as("mrom.fdt", s->fdt, s->fdt_size,
+                          memmap[SIFIVE_U_MROM].base + sizeof(reset_vec),
+                          &address_space_memory);

[v8,11/35] RISC-V: Mark ROM read-only after copying in code

Commit Message

Comments

Patch