Message ID | 1521915220-65389-2-git-send-email-mjc@sifive.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Sat, Mar 24, 2018 at 11:13 AM, Michael Clark <mjc@sifive.com> wrote: > The sifive_u machine already marks its ROM readonly. This fixes > the remaining boards. > > Cc: Sagar Karandikar <sagark@eecs.berkeley.edu> > Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de> > Signed-off-by: Michael Clark <mjc@sifive.com> > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > --- > hw/riscv/sifive_u.c | 9 +++++---- > hw/riscv/spike.c | 18 ++++++++++-------- > hw/riscv/virt.c | 7 ++++--- > include/hw/riscv/spike.h | 8 -------- > 4 files changed, 19 insertions(+), 23 deletions(-) > > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c > index 6116c38..25df16c 100644 > --- a/hw/riscv/sifive_u.c > +++ b/hw/riscv/sifive_u.c > @@ -223,7 +223,7 @@ static void riscv_sifive_u_init(MachineState *machine) > SiFiveUState *s = g_new0(SiFiveUState, 1); > MemoryRegion *sys_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); > > /* Initialize SOC */ > object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY); > @@ -246,10 +246,10 @@ static void riscv_sifive_u_init(MachineState > *machine) > create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline); > > /* boot rom */ > - memory_region_init_ram(boot_rom, NULL, "riscv.sifive.u.mrom", > + memory_region_init_ram(mask_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_set_readonly(mask_rom, true); > + memory_region_add_subregion(sys_memory, 0x0, mask_rom); > > if (machine->kernel_filename) { > load_kernel(machine->kernel_filename); > @@ -279,6 +279,7 @@ static void riscv_sifive_u_init(MachineState *machine) > 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); > + memory_region_set_readonly(mask_rom, true); > > /* MMIO */ > s->plic = sifive_plic_create(memmap[SIFIVE_U_PLIC].base, > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c > index 7710333..74edf33 100644 > --- a/hw/riscv/spike.c > +++ b/hw/riscv/spike.c > @@ -173,7 +173,7 @@ 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); > > /* Initialize SOC */ > object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY); > @@ -196,9 +196,9 @@ 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", > + memory_region_init_ram(mask_rom, NULL, "riscv.spike.mrom", > s->fdt_size + 0x2000, &error_fatal); > - memory_region_add_subregion(system_memory, 0x0, boot_rom); > + memory_region_add_subregion(system_memory, 0x0, mask_rom); > > if (machine->kernel_filename) { > load_kernel(machine->kernel_filename); > @@ -228,9 +228,10 @@ static void spike_v1_10_0_board_init(MachineState > *machine) > qemu_fdt_dumpdtb(s->fdt, s->fdt_size); > cpu_physical_memory_write(memmap[SPIKE_MROM].base + > sizeof(reset_vec), > s->fdt, s->fdt_size); > + memory_region_set_readonly(mask_rom, true); > > /* 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 +245,7 @@ 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); > > /* Initialize SOC */ > object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY); > @@ -264,9 +265,9 @@ static void spike_v1_09_1_board_init(MachineState > *machine) > main_mem); > > /* boot rom */ > - memory_region_init_ram(boot_rom, NULL, "riscv.spike.bootrom", > + memory_region_init_ram(mask_rom, NULL, "riscv.spike.mrom", > 0x40000, &error_fatal); > - memory_region_add_subregion(system_memory, 0x0, boot_rom); > + memory_region_add_subregion(system_memory, 0x0, mask_rom); > > if (machine->kernel_filename) { > load_kernel(machine->kernel_filename); > @@ -325,9 +326,10 @@ static void spike_v1_09_1_board_init(MachineState > *machine) > /* copy in the config string */ > cpu_physical_memory_write(memmap[SPIKE_MROM].base + > sizeof(reset_vec), > config_string, config_string_len); > + memory_region_set_readonly(mask_rom, true); > > /* 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 f8c19b4..f1e3641 100644 > --- a/hw/riscv/virt.c > +++ b/hw/riscv/virt.c > @@ -270,7 +270,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; > @@ -296,9 +296,9 @@ static void riscv_virt_board_init(MachineState > *machine) > create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline); > > /* boot rom */ > - memory_region_init_ram(boot_rom, NULL, "riscv_virt_board.bootrom", > + memory_region_init_ram(mask_rom, NULL, "riscv_virt_board.mrom", > s->fdt_size + 0x2000, &error_fatal); > - memory_region_add_subregion(system_memory, 0x0, boot_rom); > + memory_region_add_subregion(system_memory, 0x0, mask_rom); > > if (machine->kernel_filename) { > uint64_t kernel_entry = load_kernel(machine->kernel_filename); > @@ -339,6 +339,7 @@ static void riscv_virt_board_init(MachineState > *machine) > qemu_fdt_dumpdtb(s->fdt, s->fdt_size); > cpu_physical_memory_write(memmap[VIRT_MROM].base + sizeof(reset_vec), > s->fdt, s->fdt_size); > + memory_region_set_readonly(mask_rom, true); > > /* create PLIC hart topology configuration string */ > plic_hart_config_len = (strlen(VIRT_PLIC_HART_CONFIG) + 1) * smp_cpus; > diff --git a/include/hw/riscv/spike.h b/include/hw/riscv/spike.h > index d85a64e..179b6cf 100644 > --- a/include/hw/riscv/spike.h > +++ b/include/hw/riscv/spike.h > @@ -22,20 +22,12 @@ > #define TYPE_RISCV_SPIKE_V1_09_1_BOARD "riscv.spike_v1_9_1" > #define TYPE_RISCV_SPIKE_V1_10_0_BOARD "riscv.spike_v1_10" > > -#define SPIKE(obj) \ > - OBJECT_CHECK(SpikeState, (obj), TYPE_RISCV_SPIKE_BOARD) > - > typedef struct { > - /*< private >*/ > - SysBusDevice parent_obj; > - > - /*< public >*/ > These should not be removed in patch 6, however they are added back in the subsequent patch 7. The net result is okay, however if we want to be pedantic, we could remove this hunk from patch 6 and alter patch 7 to not add these fields back. > RISCVHartArrayState soc; > void *fdt; > int fdt_size; > } SpikeState; > > - > enum { > SPIKE_MROM, > SPIKE_CLINT, > -- > 2.7.0 > >
On Sat, Mar 24, 2018 at 12:45 PM, Michael Clark <mjc@sifive.com> wrote: > > > On Sat, Mar 24, 2018 at 11:13 AM, Michael Clark <mjc@sifive.com> wrote: > >> The sifive_u machine already marks its ROM readonly. This fixes >> the remaining boards. >> >> Cc: Sagar Karandikar <sagark@eecs.berkeley.edu> >> Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de> >> Signed-off-by: Michael Clark <mjc@sifive.com> >> Signed-off-by: Palmer Dabbelt <palmer@sifive.com> >> --- >> hw/riscv/sifive_u.c | 9 +++++---- >> hw/riscv/spike.c | 18 ++++++++++-------- >> hw/riscv/virt.c | 7 ++++--- >> include/hw/riscv/spike.h | 8 -------- >> 4 files changed, 19 insertions(+), 23 deletions(-) >> >> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c >> index 6116c38..25df16c 100644 >> --- a/hw/riscv/sifive_u.c >> +++ b/hw/riscv/sifive_u.c >> @@ -223,7 +223,7 @@ static void riscv_sifive_u_init(MachineState >> *machine) >> SiFiveUState *s = g_new0(SiFiveUState, 1); >> MemoryRegion *sys_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); >> >> /* Initialize SOC */ >> object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY); >> @@ -246,10 +246,10 @@ static void riscv_sifive_u_init(MachineState >> *machine) >> create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline); >> >> /* boot rom */ >> - memory_region_init_ram(boot_rom, NULL, "riscv.sifive.u.mrom", >> + memory_region_init_ram(mask_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_set_readonly(mask_rom, true); >> + memory_region_add_subregion(sys_memory, 0x0, mask_rom); >> >> if (machine->kernel_filename) { >> load_kernel(machine->kernel_filename); >> @@ -279,6 +279,7 @@ static void riscv_sifive_u_init(MachineState >> *machine) >> 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); >> + memory_region_set_readonly(mask_rom, true); >> >> /* MMIO */ >> s->plic = sifive_plic_create(memmap[SIFIVE_U_PLIC].base, >> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c >> index 7710333..74edf33 100644 >> --- a/hw/riscv/spike.c >> +++ b/hw/riscv/spike.c >> @@ -173,7 +173,7 @@ 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); >> >> /* Initialize SOC */ >> object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY); >> @@ -196,9 +196,9 @@ 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", >> + memory_region_init_ram(mask_rom, NULL, "riscv.spike.mrom", >> s->fdt_size + 0x2000, &error_fatal); >> - memory_region_add_subregion(system_memory, 0x0, boot_rom); >> + memory_region_add_subregion(system_memory, 0x0, mask_rom); >> >> if (machine->kernel_filename) { >> load_kernel(machine->kernel_filename); >> @@ -228,9 +228,10 @@ static void spike_v1_10_0_board_init(MachineState >> *machine) >> qemu_fdt_dumpdtb(s->fdt, s->fdt_size); >> cpu_physical_memory_write(memmap[SPIKE_MROM].base + >> sizeof(reset_vec), >> s->fdt, s->fdt_size); >> + memory_region_set_readonly(mask_rom, true); >> >> /* 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 +245,7 @@ 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); >> >> /* Initialize SOC */ >> object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY); >> @@ -264,9 +265,9 @@ static void spike_v1_09_1_board_init(MachineState >> *machine) >> main_mem); >> >> /* boot rom */ >> - memory_region_init_ram(boot_rom, NULL, "riscv.spike.bootrom", >> + memory_region_init_ram(mask_rom, NULL, "riscv.spike.mrom", >> 0x40000, &error_fatal); >> - memory_region_add_subregion(system_memory, 0x0, boot_rom); >> + memory_region_add_subregion(system_memory, 0x0, mask_rom); >> >> if (machine->kernel_filename) { >> load_kernel(machine->kernel_filename); >> @@ -325,9 +326,10 @@ static void spike_v1_09_1_board_init(MachineState >> *machine) >> /* copy in the config string */ >> cpu_physical_memory_write(memmap[SPIKE_MROM].base + >> sizeof(reset_vec), >> config_string, config_string_len); >> + memory_region_set_readonly(mask_rom, true); >> >> /* 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 f8c19b4..f1e3641 100644 >> --- a/hw/riscv/virt.c >> +++ b/hw/riscv/virt.c >> @@ -270,7 +270,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; >> @@ -296,9 +296,9 @@ static void riscv_virt_board_init(MachineState >> *machine) >> create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline); >> >> /* boot rom */ >> - memory_region_init_ram(boot_rom, NULL, "riscv_virt_board.bootrom", >> + memory_region_init_ram(mask_rom, NULL, "riscv_virt_board.mrom", >> s->fdt_size + 0x2000, &error_fatal); >> - memory_region_add_subregion(system_memory, 0x0, boot_rom); >> + memory_region_add_subregion(system_memory, 0x0, mask_rom); >> >> if (machine->kernel_filename) { >> uint64_t kernel_entry = load_kernel(machine->kernel_filename); >> @@ -339,6 +339,7 @@ static void riscv_virt_board_init(MachineState >> *machine) >> qemu_fdt_dumpdtb(s->fdt, s->fdt_size); >> cpu_physical_memory_write(memmap[VIRT_MROM].base + >> sizeof(reset_vec), >> s->fdt, s->fdt_size); >> + memory_region_set_readonly(mask_rom, true); >> >> /* create PLIC hart topology configuration string */ >> plic_hart_config_len = (strlen(VIRT_PLIC_HART_CONFIG) + 1) * >> smp_cpus; >> diff --git a/include/hw/riscv/spike.h b/include/hw/riscv/spike.h >> index d85a64e..179b6cf 100644 >> --- a/include/hw/riscv/spike.h >> +++ b/include/hw/riscv/spike.h >> @@ -22,20 +22,12 @@ >> #define TYPE_RISCV_SPIKE_V1_09_1_BOARD "riscv.spike_v1_9_1" >> #define TYPE_RISCV_SPIKE_V1_10_0_BOARD "riscv.spike_v1_10" >> >> -#define SPIKE(obj) \ >> - OBJECT_CHECK(SpikeState, (obj), TYPE_RISCV_SPIKE_BOARD) >> - >> typedef struct { >> - /*< private >*/ >> - SysBusDevice parent_obj; >> - >> - /*< public >*/ >> > > These should not be removed in patch 6, however they are added back in the > subsequent patch 7. The net result is okay, however if we want to be > pedantic, we could remove this hunk from patch 6 and alter patch 7 to not > add these fields back. > Fixed in the riscv.org qemu-2.12-fixes branch: - https://github.com/riscv/riscv-qemu/commits/qemu-2.12-fixes This essentially removes a hunk from patch 6 and instead merges it into patch 7. The net result is the same however the patches are now cleaner. RISCVHartArrayState soc; >> void *fdt; >> int fdt_size; >> } SpikeState; >> >> - >> enum { >> SPIKE_MROM, >> SPIKE_CLINT, >> -- >> 2.7.0 >> >> >
On 24 March 2018 at 18:13, Michael Clark <mjc@sifive.com> wrote: > The sifive_u machine already marks its ROM readonly. This fixes > the remaining boards. > > Cc: Sagar Karandikar <sagark@eecs.berkeley.edu> > Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de> > Signed-off-by: Michael Clark <mjc@sifive.com> > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > --- > hw/riscv/sifive_u.c | 9 +++++---- > hw/riscv/spike.c | 18 ++++++++++-------- > hw/riscv/virt.c | 7 ++++--- > include/hw/riscv/spike.h | 8 -------- > 4 files changed, 19 insertions(+), 23 deletions(-) > > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c > index 6116c38..25df16c 100644 > --- a/hw/riscv/sifive_u.c > +++ b/hw/riscv/sifive_u.c > @@ -223,7 +223,7 @@ static void riscv_sifive_u_init(MachineState *machine) > SiFiveUState *s = g_new0(SiFiveUState, 1); > MemoryRegion *sys_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); > > /* Initialize SOC */ > object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY); > @@ -246,10 +246,10 @@ static void riscv_sifive_u_init(MachineState *machine) > create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline); > > /* boot rom */ > - memory_region_init_ram(boot_rom, NULL, "riscv.sifive.u.mrom", > + memory_region_init_ram(mask_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_set_readonly(mask_rom, true); > + memory_region_add_subregion(sys_memory, 0x0, mask_rom); memory_region_init_ram + memory_region_set_readonly is equivalent to memory_region_init_rom. > if (machine->kernel_filename) { > load_kernel(machine->kernel_filename); > @@ -279,6 +279,7 @@ static void riscv_sifive_u_init(MachineState *machine) > 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); > + memory_region_set_readonly(mask_rom, true); Rather than doing this, you should use rom_add_blob_fixed(). That works even on ROMs which means you can just create them as read-only from the start rather than waiting til you've written to them and then marking them read-only. It also means that you get the contents correctly reset on reset, even if the user has been messing with their contents via the debugger or something. hw/arm/boot.c has code which (among a lot of other things) loads initial kernels and dtb images into guest memory. You can also find ppc code doing similar things. thanks -- PMM
On Sat, Mar 24, 2018 at 2:23 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 24 March 2018 at 18:13, Michael Clark <mjc@sifive.com> wrote: > > The sifive_u machine already marks its ROM readonly. This fixes > > the remaining boards. > > > > Cc: Sagar Karandikar <sagark@eecs.berkeley.edu> > > Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de> > > Signed-off-by: Michael Clark <mjc@sifive.com> > > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > > --- > > hw/riscv/sifive_u.c | 9 +++++---- > > hw/riscv/spike.c | 18 ++++++++++-------- > > hw/riscv/virt.c | 7 ++++--- > > include/hw/riscv/spike.h | 8 -------- > > 4 files changed, 19 insertions(+), 23 deletions(-) > > > > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c > > index 6116c38..25df16c 100644 > > --- a/hw/riscv/sifive_u.c > > +++ b/hw/riscv/sifive_u.c > > @@ -223,7 +223,7 @@ static void riscv_sifive_u_init(MachineState > *machine) > > SiFiveUState *s = g_new0(SiFiveUState, 1); > > MemoryRegion *sys_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); > > > > /* Initialize SOC */ > > object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY); > > @@ -246,10 +246,10 @@ static void riscv_sifive_u_init(MachineState > *machine) > > create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline); > > > > /* boot rom */ > > - memory_region_init_ram(boot_rom, NULL, "riscv.sifive.u.mrom", > > + memory_region_init_ram(mask_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_set_readonly(mask_rom, true); > > + memory_region_add_subregion(sys_memory, 0x0, mask_rom); > > memory_region_init_ram + memory_region_set_readonly is > equivalent to memory_region_init_rom. > > > if (machine->kernel_filename) { > > load_kernel(machine->kernel_filename); > > @@ -279,6 +279,7 @@ static void riscv_sifive_u_init(MachineState > *machine) > > 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); > > + memory_region_set_readonly(mask_rom, true); > > Rather than doing this, you should use > rom_add_blob_fixed(). That works even on ROMs which > means you can just create them as read-only from the > start rather than waiting til you've written to them > and then marking them read-only. It also means that > you get the contents correctly reset on reset, even > if the user has been messing with their contents > via the debugger or something. > > hw/arm/boot.c has code which (among a lot of other > things) loads initial kernels and dtb images into > guest memory. You can also find ppc code doing > similar things. > I don't mind to make this change, however it is a case of good vs perfect. Currently we have some machines with writable ROM sections, this change fixes it and has been tested. I can address this early next week. Change in itself introduces risk. The current set of changes against the series have been pretty well tested and the most recent changes have been very small. In any case I'll try to address this in spin 7 early next week as I noticed some issues myself after switching from writing and submitting patches mode to reviewing my own patches mode, so we're going to need to do a respin. Below is the v7 changelog. Only one of the changes is logic and it is a very tiny change. v7 * fix typo in mstatus.FS workaround comment * remove privilege mode from mstatus.mxr page protection check * shift class initialization boilerplate patch hunk to correct patch
On 25 March 2018 at 00:23, Michael Clark <mjc@sifive.com> wrote: > > > On Sat, Mar 24, 2018 at 2:23 PM, Peter Maydell <peter.maydell@linaro.org> > wrote: >> >> On 24 March 2018 at 18:13, Michael Clark <mjc@sifive.com> wrote: >> > The sifive_u machine already marks its ROM readonly. This fixes >> > the remaining boards. >> > >> > Cc: Sagar Karandikar <sagark@eecs.berkeley.edu> >> > Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de> >> > Signed-off-by: Michael Clark <mjc@sifive.com> >> > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> >> > --- >> > hw/riscv/sifive_u.c | 9 +++++---- >> > hw/riscv/spike.c | 18 ++++++++++-------- >> > hw/riscv/virt.c | 7 ++++--- >> > include/hw/riscv/spike.h | 8 -------- >> > 4 files changed, 19 insertions(+), 23 deletions(-) >> > >> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c >> > index 6116c38..25df16c 100644 >> > --- a/hw/riscv/sifive_u.c >> > +++ b/hw/riscv/sifive_u.c >> > @@ -223,7 +223,7 @@ static void riscv_sifive_u_init(MachineState >> > *machine) >> > SiFiveUState *s = g_new0(SiFiveUState, 1); >> > MemoryRegion *sys_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); >> > >> > /* Initialize SOC */ >> > object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY); >> > @@ -246,10 +246,10 @@ static void riscv_sifive_u_init(MachineState >> > *machine) >> > create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline); >> > >> > /* boot rom */ >> > - memory_region_init_ram(boot_rom, NULL, "riscv.sifive.u.mrom", >> > + memory_region_init_ram(mask_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_set_readonly(mask_rom, true); >> > + memory_region_add_subregion(sys_memory, 0x0, mask_rom); >> >> memory_region_init_ram + memory_region_set_readonly is >> equivalent to memory_region_init_rom. >> >> > if (machine->kernel_filename) { >> > load_kernel(machine->kernel_filename); >> > @@ -279,6 +279,7 @@ static void riscv_sifive_u_init(MachineState >> > *machine) >> > 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); >> > + memory_region_set_readonly(mask_rom, true); >> >> Rather than doing this, you should use >> rom_add_blob_fixed(). That works even on ROMs which >> means you can just create them as read-only from the >> start rather than waiting til you've written to them >> and then marking them read-only. It also means that >> you get the contents correctly reset on reset, even >> if the user has been messing with their contents >> via the debugger or something. >> >> hw/arm/boot.c has code which (among a lot of other >> things) loads initial kernels and dtb images into >> guest memory. You can also find ppc code doing >> similar things. > > > I don't mind to make this change, however it is a case of good vs perfect. > Currently we have some machines with writable ROM sections, this change > fixes it and has been tested. Yes, I would put this on your set of things to address for 2.13. thanks -- PMM
On Sun, Mar 25, 2018 at 5:47 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 25 March 2018 at 00:23, Michael Clark <mjc@sifive.com> wrote: > > > > > > On Sat, Mar 24, 2018 at 2:23 PM, Peter Maydell <peter.maydell@linaro.org > > > > wrote: > >> > >> On 24 March 2018 at 18:13, Michael Clark <mjc@sifive.com> wrote: > >> > The sifive_u machine already marks its ROM readonly. This fixes > >> > the remaining boards. > >> > > >> > Cc: Sagar Karandikar <sagark@eecs.berkeley.edu> > >> > Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de> > >> > Signed-off-by: Michael Clark <mjc@sifive.com> > >> > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > >> > --- > >> > hw/riscv/sifive_u.c | 9 +++++---- > >> > hw/riscv/spike.c | 18 ++++++++++-------- > >> > hw/riscv/virt.c | 7 ++++--- > >> > include/hw/riscv/spike.h | 8 -------- > >> > 4 files changed, 19 insertions(+), 23 deletions(-) > >> > > >> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c > >> > index 6116c38..25df16c 100644 > >> > --- a/hw/riscv/sifive_u.c > >> > +++ b/hw/riscv/sifive_u.c > >> > @@ -223,7 +223,7 @@ static void riscv_sifive_u_init(MachineState > >> > *machine) > >> > SiFiveUState *s = g_new0(SiFiveUState, 1); > >> > MemoryRegion *sys_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); > >> > > >> > /* Initialize SOC */ > >> > object_initialize(&s->soc, sizeof(s->soc), > TYPE_RISCV_HART_ARRAY); > >> > @@ -246,10 +246,10 @@ static void riscv_sifive_u_init(MachineState > >> > *machine) > >> > create_fdt(s, memmap, machine->ram_size, > machine->kernel_cmdline); > >> > > >> > /* boot rom */ > >> > - memory_region_init_ram(boot_rom, NULL, "riscv.sifive.u.mrom", > >> > + memory_region_init_ram(mask_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_set_readonly(mask_rom, true); > >> > + memory_region_add_subregion(sys_memory, 0x0, mask_rom); > >> > >> memory_region_init_ram + memory_region_set_readonly is > >> equivalent to memory_region_init_rom. > >> > >> > if (machine->kernel_filename) { > >> > load_kernel(machine->kernel_filename); > >> > @@ -279,6 +279,7 @@ static void riscv_sifive_u_init(MachineState > >> > *machine) > >> > 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); > >> > + memory_region_set_readonly(mask_rom, true); > >> > >> Rather than doing this, you should use > >> rom_add_blob_fixed(). That works even on ROMs which > >> means you can just create them as read-only from the > >> start rather than waiting til you've written to them > >> and then marking them read-only. It also means that > >> you get the contents correctly reset on reset, even > >> if the user has been messing with their contents > >> via the debugger or something. > >> > >> hw/arm/boot.c has code which (among a lot of other > >> things) loads initial kernels and dtb images into > >> guest memory. You can also find ppc code doing > >> similar things. > > > > > > I don't mind to make this change, however it is a case of good vs > perfect. > > Currently we have some machines with writable ROM sections, this change > > fixes it and has been tested. > > Yes, I would put this on your set of things to > address for 2.13. > Okay. It's on my list... I will be replying to the other thread with a list of the oustanding patches, whether they are bug fixes vs cleanups, and whether they are reviewed. The mstatus.FS fix is relatively critical and Igor's cpu init actually fixes a bug with -cpu list. In any case I'll send a list shortly...
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c index 6116c38..25df16c 100644 --- a/hw/riscv/sifive_u.c +++ b/hw/riscv/sifive_u.c @@ -223,7 +223,7 @@ static void riscv_sifive_u_init(MachineState *machine) SiFiveUState *s = g_new0(SiFiveUState, 1); MemoryRegion *sys_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); /* Initialize SOC */ object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY); @@ -246,10 +246,10 @@ static void riscv_sifive_u_init(MachineState *machine) create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline); /* boot rom */ - memory_region_init_ram(boot_rom, NULL, "riscv.sifive.u.mrom", + memory_region_init_ram(mask_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_set_readonly(mask_rom, true); + memory_region_add_subregion(sys_memory, 0x0, mask_rom); if (machine->kernel_filename) { load_kernel(machine->kernel_filename); @@ -279,6 +279,7 @@ static void riscv_sifive_u_init(MachineState *machine) 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); + memory_region_set_readonly(mask_rom, true); /* MMIO */ s->plic = sifive_plic_create(memmap[SIFIVE_U_PLIC].base, diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c index 7710333..74edf33 100644 --- a/hw/riscv/spike.c +++ b/hw/riscv/spike.c @@ -173,7 +173,7 @@ 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); /* Initialize SOC */ object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY); @@ -196,9 +196,9 @@ 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", + memory_region_init_ram(mask_rom, NULL, "riscv.spike.mrom", s->fdt_size + 0x2000, &error_fatal); - memory_region_add_subregion(system_memory, 0x0, boot_rom); + memory_region_add_subregion(system_memory, 0x0, mask_rom); if (machine->kernel_filename) { load_kernel(machine->kernel_filename); @@ -228,9 +228,10 @@ static void spike_v1_10_0_board_init(MachineState *machine) qemu_fdt_dumpdtb(s->fdt, s->fdt_size); cpu_physical_memory_write(memmap[SPIKE_MROM].base + sizeof(reset_vec), s->fdt, s->fdt_size); + memory_region_set_readonly(mask_rom, true); /* 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 +245,7 @@ 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); /* Initialize SOC */ object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY); @@ -264,9 +265,9 @@ static void spike_v1_09_1_board_init(MachineState *machine) main_mem); /* boot rom */ - memory_region_init_ram(boot_rom, NULL, "riscv.spike.bootrom", + memory_region_init_ram(mask_rom, NULL, "riscv.spike.mrom", 0x40000, &error_fatal); - memory_region_add_subregion(system_memory, 0x0, boot_rom); + memory_region_add_subregion(system_memory, 0x0, mask_rom); if (machine->kernel_filename) { load_kernel(machine->kernel_filename); @@ -325,9 +326,10 @@ static void spike_v1_09_1_board_init(MachineState *machine) /* copy in the config string */ cpu_physical_memory_write(memmap[SPIKE_MROM].base + sizeof(reset_vec), config_string, config_string_len); + memory_region_set_readonly(mask_rom, true); /* 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 f8c19b4..f1e3641 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -270,7 +270,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; @@ -296,9 +296,9 @@ static void riscv_virt_board_init(MachineState *machine) create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline); /* boot rom */ - memory_region_init_ram(boot_rom, NULL, "riscv_virt_board.bootrom", + memory_region_init_ram(mask_rom, NULL, "riscv_virt_board.mrom", s->fdt_size + 0x2000, &error_fatal); - memory_region_add_subregion(system_memory, 0x0, boot_rom); + memory_region_add_subregion(system_memory, 0x0, mask_rom); if (machine->kernel_filename) { uint64_t kernel_entry = load_kernel(machine->kernel_filename); @@ -339,6 +339,7 @@ static void riscv_virt_board_init(MachineState *machine) qemu_fdt_dumpdtb(s->fdt, s->fdt_size); cpu_physical_memory_write(memmap[VIRT_MROM].base + sizeof(reset_vec), s->fdt, s->fdt_size); + memory_region_set_readonly(mask_rom, true); /* create PLIC hart topology configuration string */ plic_hart_config_len = (strlen(VIRT_PLIC_HART_CONFIG) + 1) * smp_cpus; diff --git a/include/hw/riscv/spike.h b/include/hw/riscv/spike.h index d85a64e..179b6cf 100644 --- a/include/hw/riscv/spike.h +++ b/include/hw/riscv/spike.h @@ -22,20 +22,12 @@ #define TYPE_RISCV_SPIKE_V1_09_1_BOARD "riscv.spike_v1_9_1" #define TYPE_RISCV_SPIKE_V1_10_0_BOARD "riscv.spike_v1_10" -#define SPIKE(obj) \ - OBJECT_CHECK(SpikeState, (obj), TYPE_RISCV_SPIKE_BOARD) - typedef struct { - /*< private >*/ - SysBusDevice parent_obj; - - /*< public >*/ RISCVHartArrayState soc; void *fdt; int fdt_size; } SpikeState; - enum { SPIKE_MROM, SPIKE_CLINT,