On 25 March 2018 at 00:23, Michael Clark <m...@sifive.com> wrote:
>
>
> On Sat, Mar 24, 2018 at 2:23 PM, Peter Maydell <peter.mayd...@linaro.org>
> wrote:
>>
>> On 24 March 2018 at 18:13, Michael Clark <m...@sifive.com> wrote:
>> > The sifive_u machine already marks its ROM readonly. This fixes
>> > the remaining boards.
>> >
>> > Cc: Sagar Karandikar <sag...@eecs.berkeley.edu>
>> > Cc: Bastian Koppelmann <kbast...@mail.uni-paderborn.de>
>> > Signed-off-by: Michael Clark <m...@sifive.com>
>> > Signed-off-by: Palmer Dabbelt <pal...@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
