[Qemu-devel] [PATCH v6 00/26] RISC-V: Fixes and cleanups for QEMU 2.12

[Qemu-devel] [PATCH v6 00/26] RISC-V: Fixes and cleanups for QEMU 2.12

[Michael Clark]

This is a series of bug fixes and code cleanups that we would
like to get in before the QEMU 2.12 release. We are respinning
v6 of this series to include two new bug fixes. These changes 
are present in the downstream riscv.org riscv-all branch:

- https://github.com/riscv/riscv-qemu/commits/riscv-all

This series also addresses post-merge feedback such as updating
the cpu initialization model to conform with other architectures
as requested by Igor Mammedov.

The riscv_isa_string patch has been dropped as it was merged
independently. The patch to hold rcu_read_lock when accessing
physical memory has been dropped as requested by Paolo Bonzini.

* Implements WARL behavior for CSRs that don't support writes
* Improves specification conformance of the page table walker
  * Change access checks from ternary operator to if statements
  * Checks for misaligned PPNs
  * Disallow M-mode or S-mode from fetching from User pages
  * Adds reserved PTE flag check: W or W|X
  * Set READ flag for PTE X flag if mstatus.mxr is in effect
  * Improves page walker comments and general readability 
* Several trivial code cleanups to hw/riscv
  * Replacing hard coded constants with reference to enums
    or the machine memory maps.
  * Remove unnecessary class initialization boilerplate
* Adds bounds checks when writing device-tree to ROM
* Updates the cpu model to use a more modern interface
* Adds hexidecimal instruction bytes to disassembly output
* Sets mtval/stval to zero on exceptions without addresses
* Critical fix for an mstatus.FS bug when MTTCG is enabled
* Fix for incorrect disassembly of addiw instructions

v6

* added workaround for critical mstatus.FS MTTCG bug
* added fix for incorrect disassembly of addiw

v5

* dropped fix for memory allocation bug in riscv_isa_string
* dropped Hold rcu_read_lock when accessing memory

v4

* added fix for memory allocation bug in riscv_isa_string
* trivial fix to remove erroneous comment from translate.c

v3

* refactor rcu_read_lock in PTE update to use single unlock
* mstatus.mxr is in effect regardless of privilege mode
* remove unnecessary class init from riscv_hart
* set mtval/stval to zero on exceptions without addresses

v2

* remove unused class boilerplate retains qom parent_obj
* convert cpu definition towards future model
* honor mstatus.mxr flag in page table walker

v1

* initial post merge cleanup patch series

Michael Clark (26):
  RISC-V: Make virt create_fdt interface consistent
  RISC-V: Replace hardcoded constants with enum values
  RISC-V: Make virt board description match spike
  RISC-V: Use ROM base address and size from memmap
  RISC-V: Remove identity_translate from load_elf
  RISC-V: Mark ROM read-only after copying in code
  RISC-V: Remove unused class definitions
  RISC-V: Make sure rom has space for fdt
  RISC-V: Include intruction hex in disassembly
  RISC-V: Improve page table walker spec compliance
  RISC-V: Update E order and I extension order
  RISC-V: Make some header guards more specific
  RISC-V: Make virt header comment title consistent
  RISC-V: Use memory_region_is_ram in pte update
  RISC-V: Remove EM_RISCV ELF_MACHINE indirection
  RISC-V: Hardwire satp to 0 for no-mmu case
  RISC-V: Remove braces from satp case statement
  RISC-V: riscv-qemu port supports sv39 and sv48
  RISC-V: vectored traps are optional
  RISC-V: No traps on writes to misa,minstret,mcycle
  RISC-V: Remove support for adhoc X_COP interrupt
  RISC-V: Convert cpu definition towards future model
  RISC-V: Clear mtval/stval on exceptions without info
  RISC-V: Remove erroneous comment from translate.c
  RISC-V: Fix incorrect disassembly for addiw
  RISC-V: Workaround for critical mstatus.FS MTTCG bug

 disas/riscv.c                   |  41 ++++++-------
 hw/riscv/riscv_hart.c           |   6 --
 hw/riscv/sifive_clint.c         |   9 +--
 hw/riscv/sifive_e.c             |  34 +----------
 hw/riscv/sifive_u.c             |  65 +++++++--------------
 hw/riscv/spike.c                |  65 ++++++++-------------
 hw/riscv/virt.c                 |  77 +++++++++----------------
 include/hw/riscv/sifive_clint.h |   4 ++
 include/hw/riscv/sifive_e.h     |   5 --
 include/hw/riscv/sifive_u.h     |   9 ++-
 include/hw/riscv/spike.h        |  15 ++---
 include/hw/riscv/virt.h         |  17 +++---
 target/riscv/cpu.c              | 125 ++++++++++++++++++++++------------------
 target/riscv/cpu.h              |   6 +-
 target/riscv/cpu_bits.h         |   3 -
 target/riscv/helper.c           |  69 ++++++++++++++++------
 target/riscv/op_helper.c        |  71 ++++++++++++++---------
 target/riscv/translate.c        |   1 -
 18 files changed, 285 insertions(+), 337 deletions(-)

-- 
2.7.0

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

[Michael Clark]

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);
 
     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 >*/
     RISCVHartArrayState soc;
     void *fdt;
     int fdt_size;
 } SpikeState;
 
-
 enum {
     SPIKE_MROM,
     SPIKE_CLINT,
-- 
2.7.0

[Qemu-devel] [PATCH v6 08/26] RISC-V: Make sure rom has space for fdt

[Michael Clark]

Remove a potential buffer overflow (not seen in practice).
Perhaps cpu_physical_memory_write already has bound checks.
This change however makes space for the maximum device tree
size and adds an explicit bounds check and error message.
It doesn't trigger, but it may help in the future if the
device-tree size is exceeded. e.g. large bootargs.

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 | 20 ++++++++++++--------
 hw/riscv/spike.c    | 16 +++++++++++-----
 hw/riscv/virt.c     | 13 +++++++++----
 3 files changed, 32 insertions(+), 17 deletions(-)

diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 083043a..57b4f4f 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -52,7 +52,7 @@ static const struct MemmapEntry {
     hwaddr size;
 } sifive_u_memmap[] = {
     [SIFIVE_U_DEBUG] =    {        0x0,      0x100 },
-    [SIFIVE_U_MROM] =     {     0x1000,     0x2000 },
+    [SIFIVE_U_MROM] =     {     0x1000,    0x11000 },
     [SIFIVE_U_CLINT] =    {  0x2000000,    0x10000 },
     [SIFIVE_U_PLIC] =     {  0xc000000,  0x4000000 },
     [SIFIVE_U_UART0] =    { 0x10013000,     0x1000 },
@@ -221,7 +221,7 @@ static void riscv_sifive_u_init(MachineState *machine)
     const struct MemmapEntry *memmap = sifive_u_memmap;
 
     SiFiveUState *s = g_new0(SiFiveUState, 1);
-    MemoryRegion *sys_memory = get_system_memory();
+    MemoryRegion *system_memory = get_system_memory();
     MemoryRegion *main_mem = g_new(MemoryRegion, 1);
     MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
 
@@ -239,7 +239,7 @@ static void riscv_sifive_u_init(MachineState *machine)
     /* register RAM */
     memory_region_init_ram(main_mem, NULL, "riscv.sifive.u.ram",
                            machine->ram_size, &error_fatal);
-    memory_region_add_subregion(sys_memory, memmap[SIFIVE_U_DRAM].base,
+    memory_region_add_subregion(system_memory, memmap[SIFIVE_U_DRAM].base,
         main_mem);
 
     /* create device tree */
@@ -247,9 +247,9 @@ static void riscv_sifive_u_init(MachineState *machine)
 
     /* boot rom */
     memory_region_init_ram(mask_rom, NULL, "riscv.sifive.u.mrom",
-                           memmap[SIFIVE_U_MROM].base, &error_fatal);
-    memory_region_set_readonly(mask_rom, true);
-    memory_region_add_subregion(sys_memory, 0x0, mask_rom);
+                           memmap[SIFIVE_U_MROM].size, &error_fatal);
+    memory_region_add_subregion(system_memory, memmap[SIFIVE_U_MROM].base,
+                                mask_rom);
 
     if (machine->kernel_filename) {
         load_kernel(machine->kernel_filename);
@@ -276,6 +276,10 @@ static void riscv_sifive_u_init(MachineState *machine)
     copy_le32_to_phys(memmap[SIFIVE_U_MROM].base, reset_vec, sizeof(reset_vec));
 
     /* copy in the device tree */
+    if (s->fdt_size >= memmap[SIFIVE_U_MROM].size - sizeof(reset_vec)) {
+        error_report("qemu: not enough space to store device-tree");
+        exit(1);
+    }
     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);
@@ -293,9 +297,9 @@ static void riscv_sifive_u_init(MachineState *machine)
         SIFIVE_U_PLIC_CONTEXT_BASE,
         SIFIVE_U_PLIC_CONTEXT_STRIDE,
         memmap[SIFIVE_U_PLIC].size);
-    sifive_uart_create(sys_memory, memmap[SIFIVE_U_UART0].base,
+    sifive_uart_create(system_memory, memmap[SIFIVE_U_UART0].base,
         serial_hds[0], SIFIVE_PLIC(s->plic)->irqs[SIFIVE_U_UART0_IRQ]);
-    /* sifive_uart_create(sys_memory, memmap[SIFIVE_U_UART1].base,
+    /* sifive_uart_create(system_memory, memmap[SIFIVE_U_UART1].base,
         serial_hds[1], SIFIVE_PLIC(s->plic)->irqs[SIFIVE_U_UART1_IRQ]); */
     sifive_clint_create(memmap[SIFIVE_U_CLINT].base,
         memmap[SIFIVE_U_CLINT].size, smp_cpus,
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 64e585e..c7d937b 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -46,7 +46,7 @@ static const struct MemmapEntry {
     hwaddr base;
     hwaddr size;
 } spike_memmap[] = {
-    [SPIKE_MROM] =     {     0x1000,     0x2000 },
+    [SPIKE_MROM] =     {     0x1000,    0x11000 },
     [SPIKE_CLINT] =    {  0x2000000,    0x10000 },
     [SPIKE_DRAM] =     { 0x80000000,        0x0 },
 };
@@ -197,8 +197,9 @@ static void spike_v1_10_0_board_init(MachineState *machine)
 
     /* boot rom */
     memory_region_init_ram(mask_rom, NULL, "riscv.spike.mrom",
-                           s->fdt_size + 0x2000, &error_fatal);
-    memory_region_add_subregion(system_memory, 0x0, mask_rom);
+                           memmap[SPIKE_MROM].size, &error_fatal);
+    memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
+                                mask_rom);
 
     if (machine->kernel_filename) {
         load_kernel(machine->kernel_filename);
@@ -225,6 +226,10 @@ static void spike_v1_10_0_board_init(MachineState *machine)
     copy_le32_to_phys(memmap[SPIKE_MROM].base, reset_vec, sizeof(reset_vec));
 
     /* copy in the device tree */
+    if (s->fdt_size >= memmap[SPIKE_MROM].size - sizeof(reset_vec)) {
+        error_report("qemu: not enough space to store device-tree");
+        exit(1);
+    }
     qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
     cpu_physical_memory_write(memmap[SPIKE_MROM].base + sizeof(reset_vec),
         s->fdt, s->fdt_size);
@@ -266,8 +271,9 @@ static void spike_v1_09_1_board_init(MachineState *machine)
 
     /* boot rom */
     memory_region_init_ram(mask_rom, NULL, "riscv.spike.mrom",
-                           0x40000, &error_fatal);
-    memory_region_add_subregion(system_memory, 0x0, mask_rom);
+                           memmap[SPIKE_MROM].size, &error_fatal);
+    memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
+                                mask_rom);
 
     if (machine->kernel_filename) {
         load_kernel(machine->kernel_filename);
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 5913100..d680cbd 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -45,8 +45,8 @@ static const struct MemmapEntry {
     hwaddr size;
 } virt_memmap[] = {
     [VIRT_DEBUG] =    {        0x0,      0x100 },
-    [VIRT_MROM] =     {     0x1000,     0x2000 },
-    [VIRT_TEST] =     {     0x4000,     0x1000 },
+    [VIRT_MROM] =     {     0x1000,    0x11000 },
+    [VIRT_TEST] =     {   0x100000,     0x1000 },
     [VIRT_CLINT] =    {  0x2000000,    0x10000 },
     [VIRT_PLIC] =     {  0xc000000,  0x4000000 },
     [VIRT_UART0] =    { 0x10000000,      0x100 },
@@ -297,8 +297,9 @@ static void riscv_virt_board_init(MachineState *machine)
 
     /* boot rom */
     memory_region_init_ram(mask_rom, NULL, "riscv_virt_board.mrom",
-                           s->fdt_size + 0x2000, &error_fatal);
-    memory_region_add_subregion(system_memory, 0x0, mask_rom);
+                           memmap[VIRT_MROM].size, &error_fatal);
+    memory_region_add_subregion(system_memory, memmap[VIRT_MROM].base,
+                                mask_rom);
 
     if (machine->kernel_filename) {
         uint64_t kernel_entry = load_kernel(machine->kernel_filename);
@@ -336,6 +337,10 @@ static void riscv_virt_board_init(MachineState *machine)
     copy_le32_to_phys(memmap[VIRT_MROM].base, reset_vec, sizeof(reset_vec));
 
     /* copy in the device tree */
+    if (s->fdt_size >= memmap[VIRT_MROM].size - sizeof(reset_vec)) {
+        error_report("qemu: not enough space to store device-tree");
+        exit(1);
+    }
     qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
     cpu_physical_memory_write(memmap[VIRT_MROM].base + sizeof(reset_vec),
         s->fdt, s->fdt_size);
-- 
2.7.0

[Qemu-devel] [PATCH v6 10/26] RISC-V: Improve page table walker spec compliance

[Michael Clark]

- Inline PTE_TABLE check for better readability
- Improve readibility of User page U mode and SUM test
- Disallow non U mode from fetching from User pages
- Add reserved PTE flag check: W or W|X
- Add misaligned PPN check
- Set READ flag for PTE X flag if mstatus.mxr is in effect
- Change access checks from ternary operator to if statements
- Improves page walker comments
- No measurable performance impact on dd test

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>
---
 target/riscv/cpu_bits.h |  2 --
 target/riscv/helper.c   | 59 ++++++++++++++++++++++++++++++++++---------------
 2 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 64aa097..12b4757 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -407,5 +407,3 @@
 #define PTE_SOFT  0x300 /* Reserved for Software */
 
 #define PTE_PPN_SHIFT 10
-
-#define PTE_TABLE(PTE) (((PTE) & (PTE_V | PTE_R | PTE_W | PTE_X)) == PTE_V)
diff --git a/target/riscv/helper.c b/target/riscv/helper.c
index 02cbcea..9010620 100644
--- a/target/riscv/helper.c
+++ b/target/riscv/helper.c
@@ -185,16 +185,36 @@ restart:
 #endif
         target_ulong ppn = pte >> PTE_PPN_SHIFT;
 
-        if (PTE_TABLE(pte)) { /* next level of page table */
+        if (!(pte & PTE_V)) {
+            /* Invalid PTE */
+            return TRANSLATE_FAIL;
+        } else if (!(pte & (PTE_R | PTE_W | PTE_X))) {
+            /* Inner PTE, continue walking */
             base = ppn << PGSHIFT;
-        } else if ((pte & PTE_U) ? (mode == PRV_S) && !sum : !(mode == PRV_S)) {
-            break;
-        } else if (!(pte & PTE_V) || (!(pte & PTE_R) && (pte & PTE_W))) {
-            break;
-        } else if (access_type == MMU_INST_FETCH ? !(pte & PTE_X) :
-                  access_type == MMU_DATA_LOAD ?  !(pte & PTE_R) &&
-                  !(mxr && (pte & PTE_X)) : !((pte & PTE_R) && (pte & PTE_W))) {
-            break;
+        } else if ((pte & (PTE_R | PTE_W | PTE_X)) == PTE_W) {
+            /* Reserved leaf PTE flags: PTE_W */
+            return TRANSLATE_FAIL;
+        } else if ((pte & (PTE_R | PTE_W | PTE_X)) == (PTE_W | PTE_X)) {
+            /* Reserved leaf PTE flags: PTE_W + PTE_X */
+            return TRANSLATE_FAIL;
+        } else if ((pte & PTE_U) && ((mode != PRV_U) &&
+                   (!sum || access_type == MMU_INST_FETCH))) {
+            /* User PTE flags when not U mode and mstatus.SUM is not set,
+               or the access type is an instruction fetch */
+            return TRANSLATE_FAIL;
+        } else if (ppn & ((1ULL << ptshift) - 1)) {
+            /* Misasligned PPN */
+            return TRANSLATE_FAIL;
+        } else if (access_type == MMU_DATA_LOAD && !((pte & PTE_R) ||
+                   ((pte & PTE_X) && mxr))) {
+            /* Read access check failed */
+            return TRANSLATE_FAIL;
+        } else if (access_type == MMU_DATA_STORE && !(pte & PTE_W)) {
+            /* Write access check failed */
+            return TRANSLATE_FAIL;
+        } else if (access_type == MMU_INST_FETCH && !(pte & PTE_X)) {
+            /* Fetch access check failed */
+            return TRANSLATE_FAIL;
         } else {
             /* if necessary, set accessed and dirty bits. */
             target_ulong updated_pte = pte | PTE_A |
@@ -202,11 +222,14 @@ restart:
 
             /* Page table updates need to be atomic with MTTCG enabled */
             if (updated_pte != pte) {
-                /* if accessed or dirty bits need updating, and the PTE is
-                 * in RAM, then we do so atomically with a compare and swap.
-                 * if the PTE is in IO space, then it can't be updated.
-                 * if the PTE changed, then we must re-walk the page table
-                   as the PTE is no longer valid */
+                /*
+                 * - if accessed or dirty bits need updating, and the PTE is
+                 *   in RAM, then we do so atomically with a compare and swap.
+                 * - if the PTE is in IO space or ROM, then it can't be updated
+                 *   and we return TRANSLATE_FAIL.
+                 * - if the PTE changed by the time we went to update it, then
+                 *   it is no longer valid and we must re-walk the page table.
+                 */
                 MemoryRegion *mr;
                 hwaddr l = sizeof(target_ulong), addr1;
                 mr = address_space_translate(cs->as, pte_addr,
@@ -239,15 +262,15 @@ restart:
             target_ulong vpn = addr >> PGSHIFT;
             *physical = (ppn | (vpn & ((1L << ptshift) - 1))) << PGSHIFT;
 
-            if ((pte & PTE_R)) {
+            /* set permissions on the TLB entry */
+            if ((pte & PTE_R) || (mode != PRV_U && (pte & PTE_X) && mxr)) {
                 *prot |= PAGE_READ;
             }
             if ((pte & PTE_X)) {
                 *prot |= PAGE_EXEC;
             }
-           /* only add write permission on stores or if the page
-              is already dirty, so that we don't miss further
-              page table walks to update the dirty bit */
+            /* add write permission on stores or if the page is already dirty,
+               so that we TLB miss on later writes to update the dirty bit */
             if ((pte & PTE_W) &&
                     (access_type == MMU_DATA_STORE || (pte & PTE_D))) {
                 *prot |= PAGE_WRITE;
-- 
2.7.0

[Qemu-devel] [PATCH v6 11/26] RISC-V: Update E order and I extension order

[Michael Clark]

Section 22.8 Subset Naming Convention of the RISC-V ISA Specification
defines the canonical order for extensions in the ISA string. It is
silent on the position of the E extension however E is a substitute
for I so it must come early in the extension list order. A comment
is added to state E and I are mutually exclusive, as the E extension
will be added to the RISC-V port in the future.

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>
---
 target/riscv/cpu.c | 2 +-
 target/riscv/cpu.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 9de34d7..ad65b39 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -26,7 +26,7 @@
 
 /* RISC-V CPU definitions */
 
-static const char riscv_exts[26] = "IMAFDQECLBJTPVNSUHKORWXYZG";
+static const char riscv_exts[26] = "IEMAFDQCLBJTPVNSUHKORWXYZG";
 
 const char * const riscv_int_regnames[] = {
   "zero", "ra  ", "sp  ", "gp  ", "tp  ", "t0  ", "t1  ", "t2  ",
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 41e06ac..1fdcd75 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -72,6 +72,7 @@
 #define RV(x) ((target_ulong)1 << (x - 'A'))
 
 #define RVI RV('I')
+#define RVE RV('E') /* E and I are mutually exclusive */
 #define RVM RV('M')
 #define RVA RV('A')
 #define RVF RV('F')
-- 
2.7.0

[Qemu-devel] [PATCH v6 14/26] RISC-V: Use memory_region_is_ram in pte update

[Michael Clark]

After reading cpu_physical_memory_write and friends, it seems
that memory_region_is_ram is a more appropriate interface,
and matches the intent of the code that is calling it.

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>
---
 target/riscv/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/helper.c b/target/riscv/helper.c
index 9010620..b2e3f45 100644
--- a/target/riscv/helper.c
+++ b/target/riscv/helper.c
@@ -234,7 +234,7 @@ restart:
                 hwaddr l = sizeof(target_ulong), addr1;
                 mr = address_space_translate(cs->as, pte_addr,
                     &addr1, &l, false);
-                if (memory_access_is_direct(mr, true)) {
+                if (memory_region_is_ram(mr)) {
                     target_ulong *pte_pa =
                         qemu_map_ram_ptr(mr->ram_block, addr1);
 #if TCG_OVERSIZED_GUEST
-- 
2.7.0

[Qemu-devel] [PATCH v6 16/26] RISC-V: Hardwire satp to 0 for no-mmu case

[Michael Clark]

satp is WARL so it should not trap on illegal writes, rather
it can be hardwired to zero and silently ignore illegal writes.

It seems the RISC-V WARL behaviour is preferred to having to
trap overhead versus simply reading back the value and checking
if the write took (saves hundreds of cycles and more complex
trap handling code).

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>
---
 target/riscv/op_helper.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index e34715d..dd3e417 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -242,7 +242,7 @@ void csr_write_helper(CPURISCVState *env, target_ulong val_to_write,
     }
     case CSR_SATP: /* CSR_SPTBR */ {
         if (!riscv_feature(env, RISCV_FEATURE_MMU)) {
-            goto do_illegal;
+            break;
         }
         if (env->priv_ver <= PRIV_VERSION_1_09_1 && (val_to_write ^ env->sptbr))
         {
@@ -452,7 +452,10 @@ target_ulong csr_read_helper(CPURISCVState *env, target_ulong csrno)
         return env->scounteren;
     case CSR_SCAUSE:
         return env->scause;
-    case CSR_SPTBR:
+    case CSR_SATP: /* CSR_SPTBR */
+        if (!riscv_feature(env, RISCV_FEATURE_MMU)) {
+            return 0;
+        }
         if (env->priv_ver >= PRIV_VERSION_1_10_0) {
             return env->satp;
         } else {
-- 
2.7.0

[Qemu-devel] [PATCH v6 18/26] RISC-V: riscv-qemu port supports sv39 and sv48

[Michael Clark]

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>
---
 target/riscv/cpu.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 1dcbdbe..cd337ab 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -24,8 +24,8 @@
 #define TARGET_PAGE_BITS 12 /* 4 KiB Pages */
 #if defined(TARGET_RISCV64)
 #define TARGET_LONG_BITS 64
-#define TARGET_PHYS_ADDR_SPACE_BITS 50
-#define TARGET_VIRT_ADDR_SPACE_BITS 39
+#define TARGET_PHYS_ADDR_SPACE_BITS 52
+#define TARGET_VIRT_ADDR_SPACE_BITS 48
 #elif defined(TARGET_RISCV32)
 #define TARGET_LONG_BITS 32
 #define TARGET_PHYS_ADDR_SPACE_BITS 34
-- 
2.7.0

[Qemu-devel] [PATCH v6 19/26] RISC-V: vectored traps are optional

[Michael Clark]

Vectored traps for asynchrounous interrupts are optional.
The mtvec/stvec mode field is WARL and hence does not trap
if an illegal value is written. Illegal values are ignored.

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>
---
 target/riscv/op_helper.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index f79716a..36b9e8e 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -262,11 +262,10 @@ void csr_write_helper(CPURISCVState *env, target_ulong val_to_write,
         env->sepc = val_to_write;
         break;
     case CSR_STVEC:
-        if (val_to_write & 1) {
-            qemu_log_mask(LOG_UNIMP, "CSR_STVEC: vectored traps not supported");
-            goto do_illegal;
+        /* we do not support vectored traps for asynchrounous interrupts */
+        if ((val_to_write & 3) == 0) {
+            env->stvec = val_to_write >> 2 << 2;
         }
-        env->stvec = val_to_write >> 2 << 2;
         break;
     case CSR_SCOUNTEREN:
         env->scounteren = val_to_write;
@@ -284,11 +283,10 @@ void csr_write_helper(CPURISCVState *env, target_ulong val_to_write,
         env->mepc = val_to_write;
         break;
     case CSR_MTVEC:
-        if (val_to_write & 1) {
-            qemu_log_mask(LOG_UNIMP, "CSR_MTVEC: vectored traps not supported");
-            goto do_illegal;
+        /* we do not support vectored traps for asynchrounous interrupts */
+        if ((val_to_write & 3) == 0) {
+            env->mtvec = val_to_write >> 2 << 2;
         }
-        env->mtvec = val_to_write >> 2 << 2;
         break;
     case CSR_MCOUNTEREN:
         env->mcounteren = val_to_write;
-- 
2.7.0

[Qemu-devel] [PATCH v6 20/26] RISC-V: No traps on writes to misa, minstret, mcycle

[Michael Clark]

These fields are marked WARL in the specification so illegal
writes are silently dropped.

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>
---
 target/riscv/op_helper.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index 36b9e8e..ba3639d 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -200,17 +200,19 @@ void csr_write_helper(CPURISCVState *env, target_ulong val_to_write,
         break;
     }
     case CSR_MINSTRET:
-        qemu_log_mask(LOG_UNIMP, "CSR_MINSTRET: write not implemented");
-        goto do_illegal;
+        /* minstret is WARL so unsupported writes are ignored */
+        break;
     case CSR_MCYCLE:
-        qemu_log_mask(LOG_UNIMP, "CSR_MCYCLE: write not implemented");
-        goto do_illegal;
+        /* mcycle is WARL so unsupported writes are ignored */
+        break;
+#if defined(TARGET_RISCV32)
     case CSR_MINSTRETH:
-        qemu_log_mask(LOG_UNIMP, "CSR_MINSTRETH: write not implemented");
-        goto do_illegal;
+        /* minstreth is WARL so unsupported writes are ignored */
+        break;
     case CSR_MCYCLEH:
-        qemu_log_mask(LOG_UNIMP, "CSR_MCYCLEH: write not implemented");
-        goto do_illegal;
+        /* mcycleh is WARL so unsupported writes are ignored */
+        break;
+#endif
     case CSR_MUCOUNTEREN:
         env->mucounteren = val_to_write;
         break;
@@ -300,10 +302,9 @@ void csr_write_helper(CPURISCVState *env, target_ulong val_to_write,
     case CSR_MBADADDR:
         env->mbadaddr = val_to_write;
         break;
-    case CSR_MISA: {
-        qemu_log_mask(LOG_UNIMP, "CSR_MISA: misa writes not supported");
-        goto do_illegal;
-    }
+    case CSR_MISA:
+        /* misa is WARL so unsupported writes are ignored */
+        break;
     case CSR_PMPCFG0:
     case CSR_PMPCFG1:
     case CSR_PMPCFG2:
@@ -328,7 +329,6 @@ void csr_write_helper(CPURISCVState *env, target_ulong val_to_write,
     case CSR_PMPADDR15:
        pmpaddr_csr_write(env, csrno - CSR_PMPADDR0, val_to_write);
        break;
-    do_illegal:
 #endif
     default:
         do_raise_exception_err(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
-- 
2.7.0

[Qemu-devel] [PATCH v6 21/26] RISC-V: Remove support for adhoc X_COP interrupt

[Michael Clark]

This is essentially dead-code elimination. Support for more
local interrupts will be added in a future revision, as they
will be defined in a future version of the Privileged ISA
specification.

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>
---
 target/riscv/cpu_bits.h  | 1 -
 target/riscv/op_helper.c | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 12b4757..133e070 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -346,7 +346,6 @@
 #define IRQ_S_EXT       9
 #define IRQ_H_EXT       10 /* until: priv-1.9.1 */
 #define IRQ_M_EXT       11 /* until: priv-1.9.1 */
-#define IRQ_X_COP       12 /* non-standard */
 
 /* Default addresses */
 #define DEFAULT_RSTVEC     0x00001000
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index ba3639d..1fdde90 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -90,7 +90,7 @@ void csr_write_helper(CPURISCVState *env, target_ulong val_to_write,
         target_ulong csrno)
 {
 #ifndef CONFIG_USER_ONLY
-    uint64_t delegable_ints = MIP_SSIP | MIP_STIP | MIP_SEIP | (1 << IRQ_X_COP);
+    uint64_t delegable_ints = MIP_SSIP | MIP_STIP | MIP_SEIP;
     uint64_t all_ints = delegable_ints | MIP_MSIP | MIP_MTIP;
 #endif
 
-- 
2.7.0

[Qemu-devel] [PATCH v6 23/26] RISC-V: Clear mtval/stval on exceptions without info

[Michael Clark]

mtval/stval must be set on all exceptions but zero is
a legal value if there is no exception specific info.
Placing the instruction bytes for illegal instruction
exceptions in mtval/stval is an optional feature and
is currently not supported by QEMU RISC-V.

Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
Signed-off-by: Michael Clark <mjc@sifive.com>
---
 target/riscv/helper.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/target/riscv/helper.c b/target/riscv/helper.c
index b2e3f45..0d802a8 100644
--- a/target/riscv/helper.c
+++ b/target/riscv/helper.c
@@ -489,6 +489,10 @@ void riscv_cpu_do_interrupt(CPUState *cs)
                     ": badaddr 0x" TARGET_FMT_lx, env->mhartid, env->badaddr);
             }
             env->sbadaddr = env->badaddr;
+        } else {
+            /* otherwise we must clear sbadaddr/stval
+             * todo: support populating stval on illegal instructions */
+            env->sbadaddr = 0;
         }
 
         target_ulong s = env->mstatus;
@@ -510,6 +514,10 @@ void riscv_cpu_do_interrupt(CPUState *cs)
                     ": badaddr 0x" TARGET_FMT_lx, env->mhartid, env->badaddr);
             }
             env->mbadaddr = env->badaddr;
+        } else {
+            /* otherwise we must clear mbadaddr/mtval
+             * todo: support populating mtval on illegal instructions */
+            env->mbadaddr = 0;
         }
 
         target_ulong s = env->mstatus;
-- 
2.7.0

[Qemu-devel] [PATCH v6 24/26] RISC-V: Remove erroneous comment from translate.c

[Michael Clark]

Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
Signed-off-by: Michael Clark <mjc@sifive.com>
---
 target/riscv/translate.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 808eab7..c3a029a 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -280,7 +280,6 @@ static void gen_arith(DisasContext *ctx, uint32_t opc, int rd, int rs1,
         tcg_gen_andi_tl(source2, source2, 0x1F);
         tcg_gen_sar_tl(source1, source1, source2);
         break;
-        /* fall through to SRA */
 #endif
     case OPC_RISC_SRA:
         tcg_gen_andi_tl(source2, source2, TARGET_LONG_BITS - 1);
-- 
2.7.0

[Qemu-devel] [PATCH v6 25/26] RISC-V: Fix incorrect disassembly for addiw

[Michael Clark]

This fixes a bug in the disassembler constraints used
to lift instructions into pseudo-instructions, whereby
addiw instructions are always lifted to sext.w instead
of just lifting addiw with a zero immediate.

An associated fix has been made to the metadata used to
machine generate the disseasembler:

https://github.com/michaeljclark/riscv-meta/
commit/4a6b2f3898430768acfe201405224d2ea31e1477

Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Cc: Palmer Dabbelt <palmer@sifive.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Michael Clark <mjc@sifive.com>
---
 disas/riscv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/disas/riscv.c b/disas/riscv.c
index 4580308..2cecf0d 100644
--- a/disas/riscv.c
+++ b/disas/riscv.c
@@ -600,7 +600,7 @@ static const rvc_constraint rvcc_mv[] = { rvc_imm_eq_zero, rvc_end };
 static const rvc_constraint rvcc_not[] = { rvc_imm_eq_n1, rvc_end };
 static const rvc_constraint rvcc_neg[] = { rvc_rs1_eq_x0, rvc_end };
 static const rvc_constraint rvcc_negw[] = { rvc_rs1_eq_x0, rvc_end };
-static const rvc_constraint rvcc_sext_w[] = { rvc_rs2_eq_x0, rvc_end };
+static const rvc_constraint rvcc_sext_w[] = { rvc_imm_eq_zero, rvc_end };
 static const rvc_constraint rvcc_seqz[] = { rvc_imm_eq_p1, rvc_end };
 static const rvc_constraint rvcc_snez[] = { rvc_rs1_eq_x0, rvc_end };
 static const rvc_constraint rvcc_sltz[] = { rvc_rs2_eq_x0, rvc_end };
-- 
2.7.0

[Qemu-devel] [PATCH v6 26/26] RISC-V: Workaround for critical mstatus.FS MTTCG bug

[Michael Clark]

This change is a workaround for a bug where mstatus.FS
is not correctly reporting dirty when MTTCG and SMP are
enabled which results in the floating point register file
not being saved during context switches. This a critical
bug for RISC-V in QEMU as it results in floating point
register file corruption when running SMP Linux in the
RISC-V 'virt' machine.

This workaround will return dirty if mstatus.FS is
switched from off to initial or clean. We have checked
the specification and it is legal for an implementation
to return either off, or dirty, if set to initial or clean.

This workaround will result in unnecessary floating point
save restore. When mstatus.FS is off, floating point
instruction trap to indicate the process is using the FPU.
The OS can then save floating-point state of the previous
process using the FPU and set mstatus.FS to initial or
clean. With this workaround, mstatus.FS will always return
dirty if set to a non-zero value, indicating floating point
save restore is necessary, versus misreporting mstatus.FS
resulting in floating point register file corruption.

Cc: Palmer Dabbelt <palmer@sifive.com>
Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Cc: Richard W.M. Jones <rjones@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Michael Clark <mjc@sifive.com>
---
 target/riscv/op_helper.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index 1fdde90..d345688 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -144,8 +144,23 @@ void csr_write_helper(CPURISCVState *env, target_ulong val_to_write,
         }
 
         mstatus = (mstatus & ~mask) | (val_to_write & mask);
-        int dirty = (mstatus & MSTATUS_FS) == MSTATUS_FS;
-        dirty |= (mstatus & MSTATUS_XS) == MSTATUS_XS;
+
+        /* Note: this is a workaround for an issue where mstatus.FS
+           does not report dirty when SMP and MTTCG is enabled. This
+           workaround is technically compliant with the RISC-V Privileged
+           specification as it is legal to return only off, or dirty,
+           however this may cause unnecessary saves of floating point state.
+           Without this workaround, floating point state is not saved and
+           restored coorectly when SMP and MTTCG is enabled, */
+        if (qemu_tcg_mttcg_enabled()) {
+            /* FP is always dirty or off */
+            if (mstatus & MSTATUS_FS) {
+                mstatus |= MSTATUS_FS;
+            }
+        }
+
+        int dirty = ((mstatus & MSTATUS_FS) == MSTATUS_FS) |
+                    ((mstatus & MSTATUS_XS) == MSTATUS_XS);
         mstatus = set_field(mstatus, MSTATUS_SD, dirty);
         env->mstatus = mstatus;
         break;
-- 
2.7.0

Re: [Qemu-devel] [PATCH v6 26/26] RISC-V: Workaround for critical mstatus.FS MTTCG bug

[Michael Clark]

On Sat, Mar 24, 2018 at 11:13 AM, Michael Clark <mjc@sifive.com> wrote:

> This change is a workaround for a bug where mstatus.FS
> is not correctly reporting dirty when MTTCG and SMP are
> enabled which results in the floating point register file
> not being saved during context switches. This a critical
> bug for RISC-V in QEMU as it results in floating point
> register file corruption when running SMP Linux in the
> RISC-V 'virt' machine.
>
> This workaround will return dirty if mstatus.FS is
> switched from off to initial or clean. We have checked
> the specification and it is legal for an implementation
> to return either off, or dirty, if set to initial or clean.
>
> This workaround will result in unnecessary floating point
> save restore. When mstatus.FS is off, floating point
> instruction trap to indicate the process is using the FPU.
> The OS can then save floating-point state of the previous
> process using the FPU and set mstatus.FS to initial or
> clean. With this workaround, mstatus.FS will always return
> dirty if set to a non-zero value, indicating floating point
> save restore is necessary, versus misreporting mstatus.FS
> resulting in floating point register file corruption.
>
> Cc: Palmer Dabbelt <palmer@sifive.com>
> Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
> Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> Cc: Richard W.M. Jones <rjones@redhat.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Michael Clark <mjc@sifive.com>
> ---
>  target/riscv/op_helper.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index 1fdde90..d345688 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -144,8 +144,23 @@ void csr_write_helper(CPURISCVState *env,
> target_ulong val_to_write,
>          }
>
>          mstatus = (mstatus & ~mask) | (val_to_write & mask);
> -        int dirty = (mstatus & MSTATUS_FS) == MSTATUS_FS;
> -        dirty |= (mstatus & MSTATUS_XS) == MSTATUS_XS;
> +
> +        /* Note: this is a workaround for an issue where mstatus.FS
> +           does not report dirty when SMP and MTTCG is enabled. This
> +           workaround is technically compliant with the RISC-V Privileged
> +           specification as it is legal to return only off, or dirty,
> +           however this may cause unnecessary saves of floating point
> state.
> +           Without this workaround, floating point state is not saved and
> +           restored coorectly when SMP and MTTCG is enabled, */
>

typo in "correctly". I will fix this...


> +        if (qemu_tcg_mttcg_enabled()) {
> +            /* FP is always dirty or off */
> +            if (mstatus & MSTATUS_FS) {
> +                mstatus |= MSTATUS_FS;
> +            }
> +        }
> +
> +        int dirty = ((mstatus & MSTATUS_FS) == MSTATUS_FS) |
> +                    ((mstatus & MSTATUS_XS) == MSTATUS_XS);
>          mstatus = set_field(mstatus, MSTATUS_SD, dirty);
>          env->mstatus = mstatus;
>          break;
> --
> 2.7.0
>
>

Re: [Qemu-devel] [PATCH v6 10/26] RISC-V: Improve page table walker spec compliance

[Michael Clark]

On Sat, Mar 24, 2018 at 11:13 AM, Michael Clark <mjc@sifive.com> wrote:

> - Inline PTE_TABLE check for better readability
> - Improve readibility of User page U mode and SUM test
> - Disallow non U mode from fetching from User pages
> - Add reserved PTE flag check: W or W|X
> - Add misaligned PPN check
> - Set READ flag for PTE X flag if mstatus.mxr is in effect
> - Change access checks from ternary operator to if statements
> - Improves page walker comments
> - No measurable performance impact on dd test
>
> 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>
> ---
>  target/riscv/cpu_bits.h |  2 --
>  target/riscv/helper.c   | 59 ++++++++++++++++++++++++++++++
> ++++---------------
>  2 files changed, 41 insertions(+), 20 deletions(-)
>
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index 64aa097..12b4757 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -407,5 +407,3 @@
>  #define PTE_SOFT  0x300 /* Reserved for Software */
>
>  #define PTE_PPN_SHIFT 10
> -
> -#define PTE_TABLE(PTE) (((PTE) & (PTE_V | PTE_R | PTE_W | PTE_X)) ==
> PTE_V)
> diff --git a/target/riscv/helper.c b/target/riscv/helper.c
> index 02cbcea..9010620 100644
> --- a/target/riscv/helper.c
> +++ b/target/riscv/helper.c
> @@ -185,16 +185,36 @@ restart:
>  #endif
>          target_ulong ppn = pte >> PTE_PPN_SHIFT;
>
> -        if (PTE_TABLE(pte)) { /* next level of page table */
> +        if (!(pte & PTE_V)) {
> +            /* Invalid PTE */
> +            return TRANSLATE_FAIL;
> +        } else if (!(pte & (PTE_R | PTE_W | PTE_X))) {
> +            /* Inner PTE, continue walking */
>              base = ppn << PGSHIFT;
> -        } else if ((pte & PTE_U) ? (mode == PRV_S) && !sum : !(mode ==
> PRV_S)) {
> -            break;
> -        } else if (!(pte & PTE_V) || (!(pte & PTE_R) && (pte & PTE_W))) {
> -            break;
> -        } else if (access_type == MMU_INST_FETCH ? !(pte & PTE_X) :
> -                  access_type == MMU_DATA_LOAD ?  !(pte & PTE_R) &&
> -                  !(mxr && (pte & PTE_X)) : !((pte & PTE_R) && (pte &
> PTE_W))) {
> -            break;
> +        } else if ((pte & (PTE_R | PTE_W | PTE_X)) == PTE_W) {
> +            /* Reserved leaf PTE flags: PTE_W */
> +            return TRANSLATE_FAIL;
> +        } else if ((pte & (PTE_R | PTE_W | PTE_X)) == (PTE_W | PTE_X)) {
> +            /* Reserved leaf PTE flags: PTE_W + PTE_X */
> +            return TRANSLATE_FAIL;
> +        } else if ((pte & PTE_U) && ((mode != PRV_U) &&
> +                   (!sum || access_type == MMU_INST_FETCH))) {
> +            /* User PTE flags when not U mode and mstatus.SUM is not set,
> +               or the access type is an instruction fetch */
> +            return TRANSLATE_FAIL;
> +        } else if (ppn & ((1ULL << ptshift) - 1)) {
> +            /* Misasligned PPN */
> +            return TRANSLATE_FAIL;
> +        } else if (access_type == MMU_DATA_LOAD && !((pte & PTE_R) ||
> +                   ((pte & PTE_X) && mxr))) {
> +            /* Read access check failed */
> +            return TRANSLATE_FAIL;
> +        } else if (access_type == MMU_DATA_STORE && !(pte & PTE_W)) {
> +            /* Write access check failed */
> +            return TRANSLATE_FAIL;
> +        } else if (access_type == MMU_INST_FETCH && !(pte & PTE_X)) {
> +            /* Fetch access check failed */
> +            return TRANSLATE_FAIL;
>          } else {
>              /* if necessary, set accessed and dirty bits. */
>              target_ulong updated_pte = pte | PTE_A |
> @@ -202,11 +222,14 @@ restart:
>
>              /* Page table updates need to be atomic with MTTCG enabled */
>              if (updated_pte != pte) {
> -                /* if accessed or dirty bits need updating, and the PTE is
> -                 * in RAM, then we do so atomically with a compare and
> swap.
> -                 * if the PTE is in IO space, then it can't be updated.
> -                 * if the PTE changed, then we must re-walk the page table
> -                   as the PTE is no longer valid */
> +                /*
> +                 * - if accessed or dirty bits need updating, and the PTE
> is
> +                 *   in RAM, then we do so atomically with a compare and
> swap.
> +                 * - if the PTE is in IO space or ROM, then it can't be
> updated
> +                 *   and we return TRANSLATE_FAIL.
> +                 * - if the PTE changed by the time we went to update it,
> then
> +                 *   it is no longer valid and we must re-walk the page
> table.
> +                 */
>                  MemoryRegion *mr;
>                  hwaddr l = sizeof(target_ulong), addr1;
>                  mr = address_space_translate(cs->as, pte_addr,
> @@ -239,15 +262,15 @@ restart:
>              target_ulong vpn = addr >> PGSHIFT;
>              *physical = (ppn | (vpn & ((1L << ptshift) - 1))) << PGSHIFT;
>
> -            if ((pte & PTE_R)) {
> +            /* set permissions on the TLB entry */
> +            if ((pte & PTE_R) || (mode != PRV_U && (pte & PTE_X) && mxr))
> {
>

The mode check for mxr is incorrect. This should be:

        if ((pte & PTE_R) || ((pte & PTE_X) && mxr)) {

Page 20 Volume II: RISC-V Privileged Architectures V1.10

3.1.9 Memory Privilege in mstatus Register

"For simplicity, MPRV and MXR are in e ect regardless of privilege mode,
but in normal
use will only be enabled for short sequences in machine mode."

The bug is harmless because no Supervisor never would enable MXR for user
mode. This patch in fact makes mstatus.MXR work, however the check for mode
was overconstrained. A previous update to this patch to ignore the
privilege mode missed removing the mode check in the subsequent check for
mstatus.MXR when setting page protections.

Fixed in the riscv.org branch here:

- https://github.com/riscv/riscv-qemu/tree/qemu-2.12-fixes

                 *prot |= PAGE_READ;
>              }
>              if ((pte & PTE_X)) {
>                  *prot |= PAGE_EXEC;
>              }
> -           /* only add write permission on stores or if the page
> -              is already dirty, so that we don't miss further
> -              page table walks to update the dirty bit */
> +            /* add write permission on stores or if the page is already
> dirty,
> +               so that we TLB miss on later writes to update the dirty
> bit */
>              if ((pte & PTE_W) &&
>                      (access_type == MMU_DATA_STORE || (pte & PTE_D))) {
>                  *prot |= PAGE_WRITE;
> --
> 2.7.0
>
>

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 11:13 AM, 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);
>
>      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.


>      RISCVHartArrayState soc;
>      void *fdt;
>      int fdt_size;
>  } SpikeState;
>
> -
>  enum {
>      SPIKE_MROM,
>      SPIKE_CLINT,
> --
> 2.7.0
>
>

Re: [Qemu-devel] [PATCH v6 26/26] RISC-V: Workaround for critical mstatus.FS MTTCG bug

[Richard W.M. Jones]

On Sat, Mar 24, 2018 at 11:13:40AM -0700, Michael Clark wrote:
> This change is a workaround for a bug where mstatus.FS
> is not correctly reporting dirty when MTTCG and SMP are
> enabled which results in the floating point register file
> not being saved during context switches. This a critical
> bug for RISC-V in QEMU as it results in floating point
> register file corruption when running SMP Linux in the
> RISC-V 'virt' machine.
> 
> This workaround will return dirty if mstatus.FS is
> switched from off to initial or clean. We have checked
> the specification and it is legal for an implementation
> to return either off, or dirty, if set to initial or clean.
> 
> This workaround will result in unnecessary floating point
> save restore. When mstatus.FS is off, floating point
> instruction trap to indicate the process is using the FPU.
> The OS can then save floating-point state of the previous
> process using the FPU and set mstatus.FS to initial or
> clean. With this workaround, mstatus.FS will always return
> dirty if set to a non-zero value, indicating floating point
> save restore is necessary, versus misreporting mstatus.FS
> resulting in floating point register file corruption.
> 
> Cc: Palmer Dabbelt <palmer@sifive.com>
> Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
> Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> Cc: Richard W.M. Jones <rjones@redhat.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Michael Clark <mjc@sifive.com>

I tested this by running qemu from git with and without this patch,
both times compiling and running the “sched.c” test program:

http://oirase.annexia.org/tmp/sched.c

In my tests it fixes the problem, and therefore:

Tested-by: Richard W.M. Jones <rjones@redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

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 <mjc@sifive.com> wrote:

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

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

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH v6 08/26] RISC-V: Make sure rom has space for fdt

[Peter Maydell]

On 24 March 2018 at 18:13, Michael Clark <mjc@sifive.com> wrote:
> Remove a potential buffer overflow (not seen in practice).
> Perhaps cpu_physical_memory_write already has bound checks.

cpu_physical_memory_write() writes to the guest address
space, so it won't overflow. If you ask it to write
off the end of a ROM then it will correctly write into
an unassigned part of the guest memory space (which does
nothing) or into whatever device or other ram is there.
You probably don't want to do that, but it is not a buffer
overflow.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v6 08/26] RISC-V: Make sure rom has space for fdt

[Michael Clark]

On Sat, Mar 24, 2018 at 2:25 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 24 March 2018 at 18:13, Michael Clark <mjc@sifive.com> wrote:
> > Remove a potential buffer overflow (not seen in practice).
> > Perhaps cpu_physical_memory_write already has bound checks.
>
> cpu_physical_memory_write() writes to the guest address
> space, so it won't overflow. If you ask it to write
> off the end of a ROM then it will correctly write into
> an unassigned part of the guest memory space (which does
> nothing) or into whatever device or other ram is there.
> You probably don't want to do that, but it is not a buffer
> overflow.
>

I assumed that was the case but it is still probably good discipline to
have the bounds check.

We have also expanded the ROM regions to account for the default FDT size
which was larger than the previous ROM region sizes. I discovered this
while debugging another issue, where I had a debug statement to print the
fdt_size and noticed it was larger than the ROM region reserved for it.

It's belts and braces change. I'd prefer we at least make sure our ROM
regions are large enough for the default FDT size. It could be overflowed
on the virt board eventually if we enable many CPUs and add more devices.
The error message is a nice to have, as we'll know if the FDT size is too
large rather than have a subtle failure due to the boot loader parsing
truncated device tree.

This problem is not seen in practice... yet... but I still think it is
worth fixing.

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 2:23 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> 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.
>

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

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 <mjc@sifive.com> wrote:
>
>
> On Sat, Mar 24, 2018 at 2:23 PM, Peter Maydell <peter.maydell@linaro.org>
> wrote:
>>
>> 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.
>
>
> 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

Re: [Qemu-devel] [PATCH v6 00/26] RISC-V: Fixes and cleanups for QEMU 2.12

[Peter Maydell]

On 24 March 2018 at 18:13, Michael Clark <mjc@sifive.com> wrote:
> This is a series of bug fixes and code cleanups that we would
> like to get in before the QEMU 2.12 release. We are respinning
> v6 of this series to include two new bug fixes. These changes
> are present in the downstream riscv.org riscv-all branch:
>
> - https://github.com/riscv/riscv-qemu/commits/riscv-all
>
> This series also addresses post-merge feedback such as updating
> the cpu initialization model to conform with other architectures
> as requested by Igor Mammedov.

Hi. It looks to me like a fair number of these patches
are already reviewed, so we don't need to wait on the
rest being reviewed to get those into master.

My suggestion is that you send a pullrequest now for the
reviewed patches, and send a patchset for review for the
new ones or the ones that still need review. (If there
are patches that are reviewed but depend on earlier ones
that need to go in set 2 then they go in set 2 as well.)

26 patches is a lot to still be carrying around much
beyond rc1, so I would like to see the size of this set
reducing rather than increasing. As the release process
moves forward the bar for "can this still go in" gradually
goes up -- by about rc3 it is at about "is this a
really critical bug or regression from the previous
release".

(Also something seems to have unhelpfully decided to eat
or delay about half of your emails in this patchset :-(
Patchew only sees 14 of the 26. Our mailing list server
does seem to do that occasionally so that would be my
first guess at the culprit, but it's possible it's
something at your end.)

thanks
-- PMM

Re: [Qemu-devel] [patches] Re: [PATCH v6 00/26] RISC-V: Fixes and cleanups for QEMU 2.12

[Michael Clark]

On Sun, Mar 25, 2018 at 8:03 AM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 24 March 2018 at 18:13, Michael Clark <mjc@sifive.com> wrote:
> > This is a series of bug fixes and code cleanups that we would
> > like to get in before the QEMU 2.12 release. We are respinning
> > v6 of this series to include two new bug fixes. These changes
> > are present in the downstream riscv.org riscv-all branch:
> >
> > - https://github.com/riscv/riscv-qemu/commits/riscv-all
> >
> > This series also addresses post-merge feedback such as updating
> > the cpu initialization model to conform with other architectures
> > as requested by Igor Mammedov.
>
> Hi. It looks to me like a fair number of these patches
> are already reviewed, so we don't need to wait on the
> rest being reviewed to get those into master.
>
> My suggestion is that you send a pullrequest now for the
> reviewed patches, and send a patchset for review for the
> new ones or the ones that still need review. (If there
> are patches that are reviewed but depend on earlier ones
> that need to go in set 2 then they go in set 2 as well.)
>

Unfortunately the reviewed patches are mostly just minor cleanups. It's
almost not worth making a PR for them as *none* of the reviewed patches are
actually bug fixes. They are things like removing unused definitions or
replacing hardcoded constants with enums, removing unnesscary braces, etc,
etc

$ grep Reviewed outgoing/v6-00*
outgoing/v6-0001-RISC-V-Make-virt-create_fdt-interface-consistent.patch:Reviewed-by:
Philippe Mathieu-Daudé <f4bug@amsat.org>
outgoing/v6-0002-RISC-V-Replace-hardcoded-constants-with-enum-valu.patch:Reviewed-by:
Philippe Mathieu-Daudé <f4bug@amsat.org>
outgoing/v6-0003-RISC-V-Make-virt-board-description-match-spike.patch:Reviewed-by:
Philippe Mathieu-Daudé <f4bug@amsat.org>
outgoing/v6-0004-RISC-V-Use-ROM-base-address-and-size-from-memmap.patch:Reviewed-by:
Philippe Mathieu-Daudé <f4bug@amsat.org>
outgoing/v6-0005-RISC-V-Remove-identity_translate-from-load_elf.patch:Reviewed-by:
Philippe Mathieu-Daudé <f4bug@amsat.org>
outgoing/v6-0007-RISC-V-Remove-unused-class-definitions.patch:Reviewed-by:
Philippe Mathieu-Daudé <f4bug@amsat.org>
outgoing/v6-0009-RISC-V-Include-intruction-hex-in-disassembly.patch:Reviewed-by:
Philippe Mathieu-Daudé <f4bug@amsat.org>
outgoing/v6-0012-RISC-V-Make-some-header-guards-more-specific.patch:Reviewed-by:
Philippe Mathieu-Daudé <f4bug@amsat.org>
outgoing/v6-0013-RISC-V-Make-virt-header-comment-title-consistent.patch:Reviewed-by:
Philippe Mathieu-Daudé <f4bug@amsat.org>
outgoing/v6-0015-RISC-V-Remove-EM_RISCV-ELF_MACHINE-indirection.patch:Reviewed-by:
Philippe Mathieu-Daudé <f4bug@amsat.org>
outgoing/v6-0017-RISC-V-Remove-braces-from-satp-case-statement.patch:Reviewed-by:
Philippe Mathieu-Daudé <f4bug@amsat.org>
outgoing/v6-0022-RISC-V-Convert-cpu-definition-towards-future-mode.patch:Reviewed-by:
Philippe Mathieu-Daudé <f4bug@amsat.org>
outgoing/v6-0022-RISC-V-Convert-cpu-definition-towards-future-mode.patch:Reviewed-by:
Igor Mammedov <imammedo@redhat.com>

The unreviewed patches, as I've mentioned many times are the ones that
require reading the RISC-V Privileged ISA Specification or are actual bug
fixes and hence are harder to review. They are in the maintainer's tree and
are what folk who are interested in using RISC-V in QEMU are actually
running.

I can drop the fix to make ROM read-only it is not critical, however it is
a bug fix. I went through with a critical eye and reviewed them myself and
picked up a few minor issues, but I believe the patchset as a whole should
be fine as long as I can find someone to Ack them. Otherwise we're sort of
in a Catch-22 situation.

26 patches is a lot to still be carrying around much
> beyond rc1, so I would like to see the size of this set
> reducing rather than increasing. As the release process
> moves forward the bar for "can this still go in" gradually
> goes up -- by about rc3 it is at about "is this a
> really critical bug or regression from the previous
> release".
>
> (Also something seems to have unhelpfully decided to eat
> or delay about half of your emails in this patchset :-(
> Patchew only sees 14 of the 26. Our mailing list server
> does seem to do that occasionally so that would be my
> first guess at the culprit, but it's possible it's
> something at your end.)
>

Phil asked that I send out only the patches that don't have review, so
that's what I did.

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

[Michael Clark]

On Sun, Mar 25, 2018 at 5:47 AM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 25 March 2018 at 00:23, Michael Clark <mjc@sifive.com> wrote:
> >
> >
> > On Sat, Mar 24, 2018 at 2:23 PM, Peter Maydell <peter.maydell@linaro.org
> >
> > wrote:
> >>
> >> 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.
> >
> >
> > 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.
>

Okay. It's on my list...

I will be replying to the other thread with a list of the oustanding
patches, whether they are bug fixes vs cleanups, and whether they are
reviewed.

The mstatus.FS fix is relatively critical and Igor's cpu init actually
fixes a bug with -cpu list. In any case I'll send a list shortly...

Re: [Qemu-devel] [patches] Re: [PATCH v6 00/26] RISC-V: Fixes and cleanups for QEMU 2.12

[Michael Clark]

On Mon, Mar 26, 2018 at 11:07 AM, Michael Clark <mjc@sifive.com> wrote:

>
>
> On Sun, Mar 25, 2018 at 8:03 AM, Peter Maydell <peter.maydell@linaro.org>
> wrote:
>
>> On 24 March 2018 at 18:13, Michael Clark <mjc@sifive.com> wrote:
>> > This is a series of bug fixes and code cleanups that we would
>> > like to get in before the QEMU 2.12 release. We are respinning
>> > v6 of this series to include two new bug fixes. These changes
>> > are present in the downstream riscv.org riscv-all branch:
>> >
>> > - https://github.com/riscv/riscv-qemu/commits/riscv-all
>> >
>> > This series also addresses post-merge feedback such as updating
>> > the cpu initialization model to conform with other architectures
>> > as requested by Igor Mammedov.
>>
>> Hi. It looks to me like a fair number of these patches
>> are already reviewed, so we don't need to wait on the
>> rest being reviewed to get those into master.
>>
>> My suggestion is that you send a pullrequest now for the
>> reviewed patches, and send a patchset for review for the
>> new ones or the ones that still need review. (If there
>> are patches that are reviewed but depend on earlier ones
>> that need to go in set 2 then they go in set 2 as well.)
>>
>
> Unfortunately the reviewed patches are mostly just minor cleanups. It's
> almost not worth making a PR for them as *none* of the reviewed patches are
> actually bug fixes. They are things like removing unused definitions or
> replacing hardcoded constants with enums, removing unnesscary braces, etc,
> etc
>

Apologies. There is one bug fix in the subset of reviewed patches. the -cpu
model changes.


> $ grep Reviewed outgoing/v6-00*
> outgoing/v6-0001-RISC-V-Make-virt-create_fdt-interface-consistent.patch:Reviewed-by:
> Philippe Mathieu-Daudé <f4bug@amsat.org>
> outgoing/v6-0002-RISC-V-Replace-hardcoded-constants-
> with-enum-valu.patch:Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> outgoing/v6-0003-RISC-V-Make-virt-board-description-match-spike.patch:Reviewed-by:
> Philippe Mathieu-Daudé <f4bug@amsat.org>
> outgoing/v6-0004-RISC-V-Use-ROM-base-address-and-size-from-memmap.patch:Reviewed-by:
> Philippe Mathieu-Daudé <f4bug@amsat.org>
> outgoing/v6-0005-RISC-V-Remove-identity_translate-
> from-load_elf.patch:Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> outgoing/v6-0007-RISC-V-Remove-unused-class-definitions.patch:Reviewed-by:
> Philippe Mathieu-Daudé <f4bug@amsat.org>
> outgoing/v6-0009-RISC-V-Include-intruction-hex-in-disassembly.patch:Reviewed-by:
> Philippe Mathieu-Daudé <f4bug@amsat.org>
> outgoing/v6-0012-RISC-V-Make-some-header-guards-more-specific.patch:Reviewed-by:
> Philippe Mathieu-Daudé <f4bug@amsat.org>
> outgoing/v6-0013-RISC-V-Make-virt-header-comment-title-consistent.patch:Reviewed-by:
> Philippe Mathieu-Daudé <f4bug@amsat.org>
> outgoing/v6-0015-RISC-V-Remove-EM_RISCV-ELF_MACHINE-indirection.patch:Reviewed-by:
> Philippe Mathieu-Daudé <f4bug@amsat.org>
> outgoing/v6-0017-RISC-V-Remove-braces-from-satp-case-statement.patch:Reviewed-by:
> Philippe Mathieu-Daudé <f4bug@amsat.org>
> outgoing/v6-0022-RISC-V-Convert-cpu-definition-towards-future-mode.patch:Reviewed-by:
> Philippe Mathieu-Daudé <f4bug@amsat.org>
> outgoing/v6-0022-RISC-V-Convert-cpu-definition-towards-future-mode.patch:Reviewed-by:
> Igor Mammedov <imammedo@redhat.com>
>
> The unreviewed patches, as I've mentioned many times are the ones that
> require reading the RISC-V Privileged ISA Specification or are actual bug
> fixes and hence are harder to review. They are in the maintainer's tree and
> are what folk who are interested in using RISC-V in QEMU are actually
> running.
>
> I can drop the fix to make ROM read-only it is not critical, however it is
> a bug fix. I went through with a critical eye and reviewed them myself and
> picked up a few minor issues, but I believe the patchset as a whole should
> be fine as long as I can find someone to Ack them. Otherwise we're sort of
> in a Catch-22 situation.
>

Most of the spec compliance bug fixes are innocuous when it comes to
running Linux, nevertheless, they are bugs and will be exposed by future
verification and compliance tests. The only really critical bug fix is
26/26 mstatus.FS. Igor's fix for the cpu model 22/26 is important as it
addresses remaining review feedback with the initial port submission. 25/26
is a user-visible bugfix for the disassembler which is important for anyone
using -d in_asm. I'm pretty comfortable with the amount of testing this
series has had despite the lack of review. Testing is also important.

The FDT ROM truncation issue fixed by 8/26 isn't seen in practice but it
was seen during development as i've been working on patches to separate the
bbl bootloader from the linux kernel (currently the kernel is embedded as a
payload in embedded in the monitor firmware). I would like to change the
ports to use -bios to load firmware and -kernel can then point to a regular
linux kernel and we can indicate to the firmware where the kernel is loaded
using a device-tree chosen entry (as some other ports do). That work is not
included in this series.

C=cleanup, B=bugfix, I=important bugfix, S=spec compliance bugfix, X=critical
bugfix, R=reviewed

C R [PATCH v6 01/26] RISC-V: Make virt create_fdt interface consistent
C R [PATCH v6 02/26] RISC-V: Replace hardcoded constants with enum values
C R [PATCH v6 03/26] RISC-V: Make virt board description match spike
C R [PATCH v6 04/26] RISC-V: Use ROM base address and size from memmap
C R [PATCH v6 05/26] RISC-V: Remove identity_translate from load_elf
B - [PATCH v6 06/26] RISC-V: Mark ROM read-only after copying in code
C R [PATCH v6 07/26] RISC-V: Remove unused class definitions
B - [PATCH v6 08/26] RISC-V: Make sure rom has space for fdt
C R [PATCH v6 09/26] RISC-V: Include instruction hex in disassembly
S - [PATCH v6 10/26] RISC-V: Improve page table walker spec compliance
S - [PATCH v6 11/26] RISC-V: Update E order and I extension order
C R [PATCH v6 12/26] RISC-V: Make some header guards more specific
C R [PATCH v6 13/26] RISC-V: Make virt header comment title consistent
B - [PATCH v6 14/26] RISC-V: Use memory_region_is_ram in pte update
C R [PATCH v6 15/26] RISC-V: Remove EM_RISCV ELF_MACHINE indirection
S - [PATCH v6 16/26] RISC-V: Hardwire satp to 0 for no-mmu case
C R [PATCH v6 17/26] RISC-V: Remove braces from satp case statement
B - [PATCH v6 18/26] RISC-V: riscv-qemu port supports sv39 and sv48
S - [PATCH v6 19/26] RISC-V: vectored traps are optional
S - [PATCH v6 20/26] RISC-V: No traps on writes to misa,minstret,mcycle
C - [PATCH v6 21/26] RISC-V: Remove support for adhoc X_COP interrupt
I R [PATCH v6 22/26] RISC-V: Convert cpu definition towards future model
B - [PATCH v6 23/26] RISC-V: Clear mtval/stval on exceptions without info
C - [PATCH v6 24/26] RISC-V: Remove erroneous comment from translate.c
I - [PATCH v6 25/26] RISC-V: Fix incorrect disassembly for addiw
X - [PATCH v6 26/26] RISC-V: Workaround for critical mstatus.FS MTTCG bug



> 26 patches is a lot to still be carrying around much
>> beyond rc1, so I would like to see the size of this set
>> reducing rather than increasing. As the release process
>> moves forward the bar for "can this still go in" gradually
>> goes up -- by about rc3 it is at about "is this a
>> really critical bug or regression from the previous
>> release".
>>
>> (Also something seems to have unhelpfully decided to eat
>> or delay about half of your emails in this patchset :-(
>> Patchew only sees 14 of the 26. Our mailing list server
>> does seem to do that occasionally so that would be my
>> first guess at the culprit, but it's possible it's
>> something at your end.)
>>
>
> Phil asked that I send out only the patches that don't have review, so
> that's what I did.
>

Re: [Qemu-devel] [patches] Re: [PATCH v6 00/26] RISC-V: Fixes and cleanups for QEMU 2.12

[Michael Clark]

On Mon, Mar 26, 2018 at 4:14 PM, Michael Clark <mjc@sifive.com> wrote:

>
>
> On Mon, Mar 26, 2018 at 11:07 AM, Michael Clark <mjc@sifive.com> wrote:
>
>>
>>
>> On Sun, Mar 25, 2018 at 8:03 AM, Peter Maydell <peter.maydell@linaro.org>
>> wrote:
>>
>>> On 24 March 2018 at 18:13, Michael Clark <mjc@sifive.com> wrote:
>>> > This is a series of bug fixes and code cleanups that we would
>>> > like to get in before the QEMU 2.12 release. We are respinning
>>> > v6 of this series to include two new bug fixes. These changes
>>> > are present in the downstream riscv.org riscv-all branch:
>>> >
>>> > - https://github.com/riscv/riscv-qemu/commits/riscv-all
>>> >
>>> > This series also addresses post-merge feedback such as updating
>>> > the cpu initialization model to conform with other architectures
>>> > as requested by Igor Mammedov.
>>>
>>> Hi. It looks to me like a fair number of these patches
>>> are already reviewed, so we don't need to wait on the
>>> rest being reviewed to get those into master.
>>>
>>> My suggestion is that you send a pullrequest now for the
>>> reviewed patches, and send a patchset for review for the
>>> new ones or the ones that still need review. (If there
>>> are patches that are reviewed but depend on earlier ones
>>> that need to go in set 2 then they go in set 2 as well.)
>>>
>>
>> Unfortunately the reviewed patches are mostly just minor cleanups. It's
>> almost not worth making a PR for them as *none* of the reviewed patches are
>> actually bug fixes. They are things like removing unused definitions or
>> replacing hardcoded constants with enums, removing unnesscary braces, etc,
>> etc
>>
>
> Apologies. There is one bug fix in the subset of reviewed patches. the
> -cpu model changes.
>
>
>> $ grep Reviewed outgoing/v6-00*
>> outgoing/v6-0001-RISC-V-Make-virt-create_fdt-interface-consistent.patch:Reviewed-by:
>> Philippe Mathieu-Daudé <f4bug@amsat.org>
>> outgoing/v6-0002-RISC-V-Replace-hardcoded-constants-with-enum-valu.patch:Reviewed-by:
>> Philippe Mathieu-Daudé <f4bug@amsat.org>
>> outgoing/v6-0003-RISC-V-Make-virt-board-description-match-spike.patch:Reviewed-by:
>> Philippe Mathieu-Daudé <f4bug@amsat.org>
>> outgoing/v6-0004-RISC-V-Use-ROM-base-address-and-size-from-memmap.patch:Reviewed-by:
>> Philippe Mathieu-Daudé <f4bug@amsat.org>
>> outgoing/v6-0005-RISC-V-Remove-identity_translate-from-load_elf.patch:Reviewed-by:
>> Philippe Mathieu-Daudé <f4bug@amsat.org>
>> outgoing/v6-0007-RISC-V-Remove-unused-class-definitions.patch:Reviewed-by:
>> Philippe Mathieu-Daudé <f4bug@amsat.org>
>> outgoing/v6-0009-RISC-V-Include-intruction-hex-in-disassembly.patch:Reviewed-by:
>> Philippe Mathieu-Daudé <f4bug@amsat.org>
>> outgoing/v6-0012-RISC-V-Make-some-header-guards-more-specific.patch:Reviewed-by:
>> Philippe Mathieu-Daudé <f4bug@amsat.org>
>> outgoing/v6-0013-RISC-V-Make-virt-header-comment-title-consistent.patch:Reviewed-by:
>> Philippe Mathieu-Daudé <f4bug@amsat.org>
>> outgoing/v6-0015-RISC-V-Remove-EM_RISCV-ELF_MACHINE-indirection.patch:Reviewed-by:
>> Philippe Mathieu-Daudé <f4bug@amsat.org>
>> outgoing/v6-0017-RISC-V-Remove-braces-from-satp-case-statement.patch:Reviewed-by:
>> Philippe Mathieu-Daudé <f4bug@amsat.org>
>> outgoing/v6-0022-RISC-V-Convert-cpu-definition-towards-future-mode.patch:Reviewed-by:
>> Philippe Mathieu-Daudé <f4bug@amsat.org>
>> outgoing/v6-0022-RISC-V-Convert-cpu-definition-towards-future-mode.patch:Reviewed-by:
>> Igor Mammedov <imammedo@redhat.com>
>>
>> The unreviewed patches, as I've mentioned many times are the ones that
>> require reading the RISC-V Privileged ISA Specification or are actual bug
>> fixes and hence are harder to review. They are in the maintainer's tree and
>> are what folk who are interested in using RISC-V in QEMU are actually
>> running.
>>
>> I can drop the fix to make ROM read-only it is not critical, however it
>> is a bug fix. I went through with a critical eye and reviewed them myself
>> and picked up a few minor issues, but I believe the patchset as a whole
>> should be fine as long as I can find someone to Ack them. Otherwise we're
>> sort of in a Catch-22 situation.
>>
>
> Most of the spec compliance bug fixes are innocuous when it comes to
> running Linux, nevertheless, they are bugs and will be exposed by future
> verification and compliance tests. The only really critical bug fix is
> 26/26 mstatus.FS. Igor's fix for the cpu model 22/26 is important as it
> addresses remaining review feedback with the initial port submission. 25/26
> is a user-visible bugfix for the disassembler which is important for anyone
> using -d in_asm. I'm pretty comfortable with the amount of testing this
> series has had despite the lack of review. Testing is also important.
>
> The FDT ROM truncation issue fixed by 8/26 isn't seen in practice but it
> was seen during development as i've been working on patches to separate the
> bbl bootloader from the linux kernel (currently the kernel is embedded as a
> payload in embedded in the monitor firmware). I would like to change the
> ports to use -bios to load firmware and -kernel can then point to a regular
> linux kernel and we can indicate to the firmware where the kernel is loaded
> using a device-tree chosen entry (as some other ports do). That work is not
> included in this series.
>
> C=cleanup, B=bugfix, I=important bugfix, S=spec compliance bugfix, X=critical
> bugfix, R=reviewed
>
> C R [PATCH v6 01/26] RISC-V: Make virt create_fdt interface consistent
> C R [PATCH v6 02/26] RISC-V: Replace hardcoded constants with enum values
> C R [PATCH v6 03/26] RISC-V: Make virt board description match spike
> C R [PATCH v6 04/26] RISC-V: Use ROM base address and size from memmap
> C R [PATCH v6 05/26] RISC-V: Remove identity_translate from load_elf
> B - [PATCH v6 06/26] RISC-V: Mark ROM read-only after copying in code
> C R [PATCH v6 07/26] RISC-V: Remove unused class definitions
> B - [PATCH v6 08/26] RISC-V: Make sure rom has space for fdt
> C R [PATCH v6 09/26] RISC-V: Include instruction hex in disassembly
> S - [PATCH v6 10/26] RISC-V: Improve page table walker spec compliance
> S - [PATCH v6 11/26] RISC-V: Update E order and I extension order
> C R [PATCH v6 12/26] RISC-V: Make some header guards more specific
> C R [PATCH v6 13/26] RISC-V: Make virt header comment title consistent
> B - [PATCH v6 14/26] RISC-V: Use memory_region_is_ram in pte update
> C R [PATCH v6 15/26] RISC-V: Remove EM_RISCV ELF_MACHINE indirection
> S - [PATCH v6 16/26] RISC-V: Hardwire satp to 0 for no-mmu case
> C R [PATCH v6 17/26] RISC-V: Remove braces from satp case statement
> B - [PATCH v6 18/26] RISC-V: riscv-qemu port supports sv39 and sv48
> S - [PATCH v6 19/26] RISC-V: vectored traps are optional
> S - [PATCH v6 20/26] RISC-V: No traps on writes to misa,minstret,mcycle
> C - [PATCH v6 21/26] RISC-V: Remove support for adhoc X_COP interrupt
> I R [PATCH v6 22/26] RISC-V: Convert cpu definition towards future model
> B - [PATCH v6 23/26] RISC-V: Clear mtval/stval on exceptions without info
> C - [PATCH v6 24/26] RISC-V: Remove erroneous comment from translate.c
> I - [PATCH v6 25/26] RISC-V: Fix incorrect disassembly for addiw
> X - [PATCH v6 26/26] RISC-V: Workaround for critical mstatus.FS MTTCG bug
>
>
>
>> 26 patches is a lot to still be carrying around much
>>> beyond rc1, so I would like to see the size of this set
>>> reducing rather than increasing. As the release process
>>> moves forward the bar for "can this still go in" gradually
>>> goes up -- by about rc3 it is at about "is this a
>>> really critical bug or regression from the previous
>>> release".
>>>
>>
I've made a tag for the series including the fixes from my own review
during the weekend (one logic fix and 2 comment of commit log typos, and a
patch hunk in the wrong commit):

- https://github.com/riscv/riscv-qemu/releases/tag/riscv-qemu-2.12-fixes-v7

That said, if you want important or critical fixes only, then i'd suggest
these at least these 3:

I R [PATCH v6 22/26] RISC-V: Convert cpu definition towards future model
I - [PATCH v6 25/26] RISC-V: Fix incorrect disassembly for addiw
X - [PATCH v6 26/26] RISC-V: Workaround for critical mstatus.FS MTTCG bug

The other patches are the 23 patches which were accidentially included in
the initial pull on March 8, now with Signed-off-by, less the one that
Paolo asked me to drop.

Most of the bugs are relatively innocuous except for the mstatus.FS fix.
Linux boots on upstream QEMU but has FPU register file corruption when SMP
is enabled.

I need advice as to which fixes you want to have included in QEMU 2.12. I'm
not sure if the reviewed set is the right set as it excludes 25/26 and more
importantly 26/26.

(Also something seems to have unhelpfully decided to eat
>>> or delay about half of your emails in this patchset :-(
>>> Patchew only sees 14 of the 26. Our mailing list server
>>> does seem to do that occasionally so that would be my
>>> first guess at the culprit, but it's possible it's
>>> something at your end.)
>>>
>>
>> Phil asked that I send out only the patches that don't have review, so
>> that's what I did.
>>
>
>

Re: [Qemu-devel] [patches] Re: [PATCH v6 00/26] RISC-V: Fixes and cleanups for QEMU 2.12

[Peter Maydell]

On 26 March 2018 at 19:07, Michael Clark <mjc@sifive.com> wrote:
> On Sun, Mar 25, 2018 at 8:03 AM, Peter Maydell <peter.maydell@linaro.org>
> wrote:
>> Hi. It looks to me like a fair number of these patches
>> are already reviewed, so we don't need to wait on the
>> rest being reviewed to get those into master.
>>
>> My suggestion is that you send a pullrequest now for the
>> reviewed patches, and send a patchset for review for the
>> new ones or the ones that still need review. (If there
>> are patches that are reviewed but depend on earlier ones
>> that need to go in set 2 then they go in set 2 as well.)
>>
>
> Unfortunately the reviewed patches are mostly just minor cleanups. It's
> almost not worth making a PR for them as *none* of the reviewed patches are
> actually bug fixes. They are things like removing unused definitions or
> replacing hardcoded constants with enums, removing unnesscary braces, etc,
> etc

No, what I'm saying is that it is very much worth it. You
want to shorten the size of your set of uncommitted patches.
Large pull requests increase the chances that some
random thing in there hits a compile issue or other minor
problem that means I have to bounce the whole thing and
you need to respin it. Smaller ones are more likely to
go in. This is especially true during the freeze part
of the release cycle, when we do an RC every week -- having
patches in earlier RCs reduces the risk. I do not want
to still see a 26 patch set unapplied by the time we get
to RC3 or RC4.

Or if you don't think the minor cleanups are worth putting
into 2.12, that's fine too (it's a submaintainer judgement
you can make). In that case you can put those to one side
and trim down the size of the patchset you're sending out
(ie make it an 01/11...11/11 patchset or whatever).

> 26 patches is a lot to still be carrying around much
>> beyond rc1, so I would like to see the size of this set
>> reducing rather than increasing. As the release process
>> moves forward the bar for "can this still go in" gradually
>> goes up -- by about rc3 it is at about "is this a
>> really critical bug or regression from the previous
>> release".
>>
>> (Also something seems to have unhelpfully decided to eat
>> or delay about half of your emails in this patchset :-(
>> Patchew only sees 14 of the 26. Our mailing list server
>> does seem to do that occasionally so that would be my
>> first guess at the culprit, but it's possible it's
>> something at your end.)
>>
>
> Phil asked that I send out only the patches that don't have review, so
> that's what I did.

I think that was a miscommunication. You should always
send out entire patchsets, not just parts of one.
Philippe said:
https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06038.html
"You could have sent a PR of the reviewed patches, and
respin the unreviewed patches separately.", which is the
same thing I'm suggesting here.

thanks
-- PMM

Re: [Qemu-devel] [patches] Re: [PATCH v6 00/26] RISC-V: Fixes and cleanups for QEMU 2.12

[Daniel P. Berrangé]

On Mon, Mar 26, 2018 at 04:45:34PM -0700, Michael Clark wrote:
> I've made a tag for the series including the fixes from my own review
> during the weekend (one logic fix and 2 comment of commit log typos, and a
> patch hunk in the wrong commit):
> 
> - https://github.com/riscv/riscv-qemu/releases/tag/riscv-qemu-2.12-fixes-v7
> 
> That said, if you want important or critical fixes only, then i'd suggest
> these at least these 3:
> 
> I R [PATCH v6 22/26] RISC-V: Convert cpu definition towards future model
> I - [PATCH v6 25/26] RISC-V: Fix incorrect disassembly for addiw
> X - [PATCH v6 26/26] RISC-V: Workaround for critical mstatus.FS MTTCG bug
> 
> The other patches are the 23 patches which were accidentially included in
> the initial pull on March 8, now with Signed-off-by, less the one that
> Paolo asked me to drop.
> 
> Most of the bugs are relatively innocuous except for the mstatus.FS fix.
> Linux boots on upstream QEMU but has FPU register file corruption when SMP
> is enabled.

Even innocuous bug fixes are still welcome before rc2 is released.

> I need advice as to which fixes you want to have included in QEMU 2.12. I'm
> not sure if the reviewed set is the right set as it excludes 25/26 and more
> importantly 26/26.

We've got rc1 now, but you can still send pull requests for pretty much
any kind of bug fix to get into the rc2 release, assuming the patches have
a Reviewed-by, and are not excessively complicated or likely to cause
regressions.

After rc2 is out you want to only be sending important bug fixes, that
users cannot wait for.

After rc3 is out, ideally we won't find any more important bugs that
need fixing. A bug would have to be really severe to justify including
at that stage.

Based on your description, patches 22/25/26 definitely still welcome
in a pull request before rc2 or rc3. The more innocuous patches can
also still be sent if you can do send a pull request for them pretty
much straightaway before rc2. You'll obviously need to chase reviews
for the few that miss them still.

We're slightly unlucky in this release that the time between rc1 and
rc2 falls over Easter weekend and Peter is on holiday, so has pretty
limited time for dealing with pull requests. So you want to do what
you can to minimize the work that Peter has to do, and minimize risk
that a patch will fail build or automated testing, as that risks
having the PR rejected. This is why it is worth sending patches in
smaller groups. If you sent 20 patches at once and the last one
fails, all 20 patches get rejected. If you send two batches of 10
you would at least get some of them off your plate, even if they
are innocuous fixes.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [patches] Re: [PATCH v6 00/26] RISC-V: Fixes and cleanups for QEMU 2.12

[Michael Clark]

On Tue, Mar 27, 2018 at 2:42 AM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 26 March 2018 at 19:07, Michael Clark <mjc@sifive.com> wrote:
> > On Sun, Mar 25, 2018 at 8:03 AM, Peter Maydell <peter.maydell@linaro.org
> >
> > wrote:
> >> Hi. It looks to me like a fair number of these patches
> >> are already reviewed, so we don't need to wait on the
> >> rest being reviewed to get those into master.
> >>
> >> My suggestion is that you send a pullrequest now for the
> >> reviewed patches, and send a patchset for review for the
> >> new ones or the ones that still need review. (If there
> >> are patches that are reviewed but depend on earlier ones
> >> that need to go in set 2 then they go in set 2 as well.)
> >>
> >
> > Unfortunately the reviewed patches are mostly just minor cleanups. It's
> > almost not worth making a PR for them as *none* of the reviewed patches
> are
> > actually bug fixes. They are things like removing unused definitions or
> > replacing hardcoded constants with enums, removing unnesscary braces,
> etc,
> > etc
>
> No, what I'm saying is that it is very much worth it. You
> want to shorten the size of your set of uncommitted patches.
> Large pull requests increase the chances that some
> random thing in there hits a compile issue or other minor
> problem that means I have to bounce the whole thing and
> you need to respin it. Smaller ones are more likely to
> go in. This is especially true during the freeze part
> of the release cycle, when we do an RC every week -- having
> patches in earlier RCs reduces the risk. I do not want
> to still see a 26 patch set unapplied by the time we get
> to RC3 or RC4.
>
> Or if you don't think the minor cleanups are worth putting
> into 2.12, that's fine too (it's a submaintainer judgement
> you can make). In that case you can put those to one side
> and trim down the size of the patchset you're sending out
> (ie make it an 01/11...11/11 patchset or whatever).


I'm not sure whether maintainer or submaintainer is really that relavant.
Active maintainership is probably more relevant. i.e. responding to RISC-V
related emails, PRs, issues on the riscv.org qemu repo, testing PRs before
merging them, etc, etc.

I'm going to focus on getting the critical bug fixes in for QEMU 2.12 i.e.
the ones that break RISC-V Linux in QEMU 2.12 e.g. the mstatus.FS fix. User
visible bugs like the disassembler bug and the -cpu list bug. I'm going to
make a 3 patch series... possibly a 2 patch series... we can leave the
disassembler bug there and just include Igor's change and the mstatus.FS
workaround. I don't think writable ROM is really that critical, and bounds
checks for potential device-tree truncation are just nice-to-haves. Spec
conformance is nice-to-have if we are triaging against critical issues.

Once we can ensure we have a working RISC-V port for QEMU 2.12 we can then
worry about spec conformance bug fixes and tidy ups, perhaps for QEMU 2.13.

My pesonal opinion is that Tested-by: should carry more weight than
Reviewed-by: assuming Reviewed-by: only means someone has reviewed the code
versus checking out and testing that a critical bug has been resolved by
said patch. That said, if all QEMU patches need Reviewed-by: then there is
not much we can do. In GCC and Linux, subsystem maintainers are allowed to
make judgements over the inclusion of critical bug fixes. i.e. Reviewed-by:
is not mandatory if the change is a critical fix.

I will divide the series up into 3 branches, and move through them in order
of priority, with correctness ahead of tidyness:

1). riscv-qemu-2.12-critical-fixes
2). riscv-qemu-2.13-bug-fixes
3). riscv-qemu-2.13-tidy-ups

Expect to see riscv-qemu-2.12-critical-fixes very soon...

> 26 patches is a lot to still be carrying around much
> >> beyond rc1, so I would like to see the size of this set
> >> reducing rather than increasing. As the release process
> >> moves forward the bar for "can this still go in" gradually
> >> goes up -- by about rc3 it is at about "is this a
> >> really critical bug or regression from the previous
> >> release".
> >>
> >> (Also something seems to have unhelpfully decided to eat
> >> or delay about half of your emails in this patchset :-(
> >> Patchew only sees 14 of the 26. Our mailing list server
> >> does seem to do that occasionally so that would be my
> >> first guess at the culprit, but it's possible it's
> >> something at your end.)
> >>
> >
> > Phil asked that I send out only the patches that don't have review, so
> > that's what I did.
>
> I think that was a miscommunication. You should always
> send out entire patchsets, not just parts of one.
> Philippe said:
> https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06038.html
> "You could have sent a PR of the reviewed patches, and
> respin the unreviewed patches separately.", which is the
> same thing I'm suggesting here.


My mistake.

Re: [Qemu-devel] [patches] Re: [PATCH v6 00/26] RISC-V: Fixes and cleanups for QEMU 2.12

[Michael Clark]

On Tue, Mar 27, 2018 at 11:39 AM, Michael Clark <mjc@sifive.com> wrote:

>
> I will divide the series up into 3 branches, and move through them in
> order of priority, with correctness ahead of tidyness:
>
> 1). riscv-qemu-2.12-critical-fixes
> 2). riscv-qemu-2.13-bug-fixes
> 3). riscv-qemu-2.13-tidy-ups
>

I think we need 4 categories:

1). riscv-qemu-2.12-critical-fixes
- floating point register file corruption mstatus.FS

2). riscv-qemu-2.12-important-fixes
- user visible bugs, e.g. Igor's -cpu list bug, wrong dissassembly for
sext.w/addiw bug

3). riscv-qemu-2.13-bug-fixes
- spec bugs and other innocuous bug fixes that are not /yet/ user visible.
i.e. not exercised by RISC-V Linux

4). riscv-qemu-2.13-tidy-ups
- code cleanups