Re: [Qemu-devel] [PATCH v6 06/26] RISC-V: Mark ROM read-only after copying in code

[Peter Maydell]

On 25 March 2018 at 00:23, Michael Clark <address@hidden> wrote:
>
>
> On Sat, Mar 24, 2018 at 2:23 PM, Peter Maydell <address@hidden>
> wrote:
>>
>> On 24 March 2018 at 18:13, Michael Clark <address@hidden> wrote:
>> > The sifive_u machine already marks its ROM readonly. This fixes
>> > the remaining boards.
>> >
>> > Cc: Sagar Karandikar <address@hidden>
>> > Cc: Bastian Koppelmann <address@hidden>
>> > Signed-off-by: Michael Clark <address@hidden>
>> > Signed-off-by: Palmer Dabbelt <address@hidden>
>> > ---
>> >  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