[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: |
Fri, 27 Apr 2018 17:22:33 +1200 |
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".
> Alistair
>
> > typedef struct {
> > /*< private >*/
> > SysBusDevice parent_obj;
> > --
> > 2.7.0
>
- [Qemu-devel] [PATCH v8 08/35] RISC-V: Make virt header comment title consistent, (continued)
- [Qemu-devel] [PATCH v8 08/35] RISC-V: Make virt header comment title consistent, Michael Clark, 2018/04/25
- [Qemu-devel] [PATCH v8 09/35] RISC-V: Remove EM_RISCV ELF_MACHINE indirection, Michael Clark, 2018/04/25
- [Qemu-devel] [PATCH v8 10/35] RISC-V: Remove erroneous comment from translate.c, Michael Clark, 2018/04/25
- [Qemu-devel] [PATCH v8 12/35] RISC-V: Update address bits to support sv39 and sv48, Michael Clark, 2018/04/25
- [Qemu-devel] [PATCH v8 11/35] RISC-V: Mark ROM read-only after copying in code, Michael Clark, 2018/04/25
[Qemu-devel] [PATCH v8 13/35] RISC-V: Improve page table walker spec compliance, Michael Clark, 2018/04/25
[Qemu-devel] [PATCH v8 14/35] RISC-V: Update E order and I extension order, Michael Clark, 2018/04/25
[Qemu-devel] [PATCH v8 15/35] RISC-V: Hardwire satp to 0 for no-mmu case, Michael Clark, 2018/04/25
[Qemu-devel] [PATCH v8 16/35] RISC-V: Make mtvec/stvec ignore vectored traps, Michael Clark, 2018/04/25
[Qemu-devel] [PATCH v8 17/35] RISC-V: No traps on writes to misa, minstret, mcycle, Michael Clark, 2018/04/25