[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 06/26] RISC-V: Mark ROM read-only after copyi
From: |
Michael Clark |
Subject: |
Re: [Qemu-devel] [PATCH v6 06/26] RISC-V: Mark ROM read-only after copying in code |
Date: |
Sat, 24 Mar 2018 17:23:39 -0700 |
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.
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
- [Qemu-devel] [PATCH v6 24/26] RISC-V: Remove erroneous comment from translate.c, (continued)
- [Qemu-devel] [PATCH v6 24/26] RISC-V: Remove erroneous comment from translate.c, Michael Clark, 2018/03/24
- [Qemu-devel] [PATCH v6 25/26] RISC-V: Fix incorrect disassembly for addiw, Michael Clark, 2018/03/24
- [Qemu-devel] [PATCH v6 11/26] RISC-V: Update E order and I extension order, Michael Clark, 2018/03/24
- [Qemu-devel] [PATCH v6 14/26] RISC-V: Use memory_region_is_ram in pte update, Michael Clark, 2018/03/24
- [Qemu-devel] [PATCH v6 16/26] RISC-V: Hardwire satp to 0 for no-mmu case, Michael Clark, 2018/03/24
- [Qemu-devel] [PATCH v6 23/26] RISC-V: Clear mtval/stval on exceptions without info, Michael Clark, 2018/03/24
- [Qemu-devel] [PATCH v6 06/26] RISC-V: Mark ROM read-only after copying in code, Michael Clark, 2018/03/24
[Qemu-devel] [PATCH v6 19/26] RISC-V: vectored traps are optional, Michael Clark, 2018/03/24
[Qemu-devel] [PATCH v6 10/26] RISC-V: Improve page table walker spec compliance, Michael Clark, 2018/03/24
[Qemu-devel] [PATCH v6 08/26] RISC-V: Make sure rom has space for fdt, Michael Clark, 2018/03/24
[Qemu-devel] [PATCH v6 26/26] RISC-V: Workaround for critical mstatus.FS MTTCG bug, Michael Clark, 2018/03/24