[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8 11/35] RISC-V: Mark ROM read-only after copyi
|
From: |
Michael Clark |
|
Subject: |
Re: [Qemu-devel] [PATCH v8 11/35] RISC-V: Mark ROM read-only after copying in code |
|
Date: |
Sat, 5 May 2018 14:02:04 +1200 |
On Sat, May 5, 2018 at 11:54 AM, Alistair Francis <address@hidden>
wrote:
> On Fri, May 4, 2018 at 4:44 PM Alistair Francis <address@hidden>
> wrote:
>
> > On Thu, May 3, 2018 at 6:45 PM Michael Clark <address@hidden> wrote:
>
>
>
> > > On Sat, Apr 28, 2018 at 4:17 AM, Alistair Francis <
> address@hidden>
> > wrote:
>
> > >> On Thu, Apr 26, 2018 at 10:34 PM Michael Clark <address@hidden>
> wrote:
>
>
>
> > >> > On Fri, Apr 27, 2018 at 5:22 PM, Michael Clark <address@hidden>
> wrote:
>
>
>
> > >> >> On Fri, Apr 27, 2018 at 4:48 AM, Alistair Francis <
> > address@hidden>
> > >> wrote:
>
> > >> >>> On Wed, Apr 25, 2018 at 5:03 PM Michael Clark <address@hidden>
> 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 <address@hidden>
> > >> >>> > Cc: Bastian Koppelmann <address@hidden>
> > >> >>> > Cc: Palmer Dabbelt <address@hidden>
> > >> >>> > Cc: Alistair Francis <address@hidden>
> > >> >>> > Signed-off-by: Michael Clark <address@hidden>
> > >> >>> > ---
> > >> >>> > 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);