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

[Peter Maydell <peter.maydell@linaro.org>]

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