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

[Michael Clark]

On Sat, Mar 24, 2018 at 12:45 PM, Michael Clark <address@hidden> wrote:

>
>
> On Sat, Mar 24, 2018 at 11:13 AM, 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);
>>
>>      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
>>
>>
>