[Qemu-devel] [PATCH v3 00/24] RISC-V Post-merge spec conformance and cleanup

[Qemu-devel] [PATCH v3 00/24] RISC-V Post-merge spec conformance and cleanup

[Michael Clark]

This is a series of spec conformance bug fixes and code cleanups
that we would like to get in before the QEMU 2.12 release. This
series does not contain the fix to riscv_isa_string. Previous
versions of this series have been included in the riscv.org QEMU
repository and these changes have had extensive testing running
Fedora for RISC-V, including building QEMU inside of RISC-V QEMU
running SMP Linux.

* 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
* Sets 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

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

Michael Clark (24):
  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: Hold rcu_read_lock when accessing memory
  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

 disas/riscv.c                   |  39 +++++++------
 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           |  83 +++++++++++++++++++-------
 target/riscv/op_helper.c        |  52 ++++++++---------
 17 files changed, 279 insertions(+), 335 deletions(-)

Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>

-- 
2.7.0

[Qemu-devel] [PATCH v3 01/24] RISC-V: Make virt create_fdt interface consistent

[Michael Clark]

create_fdt sets the fdt variable on RISCVVirtState and this is
used to access the fdt. This reverts a change introduced in
https://github.com/riscv/riscv-qemu/pull/109 which introduced
a redundant return value, overlooking the RISCVVirtState
structure member that made create_fdt inconsistent with the
other RISC-V machines. The other alternative is to change
the other boards to return the fdt. Note: the RISCVVirtState
also contains fdt_size.

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>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/riscv/virt.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index e2c214e..37968d2 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -108,7 +108,7 @@ static hwaddr load_initrd(const char *filename, uint64_t mem_size,
     return *start + size;
 }
 
-static void *create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
+static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
     uint64_t mem_size, const char *cmdline)
 {
     void *fdt;
@@ -264,8 +264,6 @@ static void *create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
     qemu_fdt_setprop_string(fdt, "/chosen", "stdout-path", nodename);
     qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
     g_free(nodename);
-
-    return fdt;
 }
 
 static void riscv_virt_board_init(MachineState *machine)
@@ -279,7 +277,6 @@ static void riscv_virt_board_init(MachineState *machine)
     char *plic_hart_config;
     size_t plic_hart_config_len;
     int i;
-    void *fdt;
 
     /* Initialize SOC */
     object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY);
@@ -299,7 +296,7 @@ static void riscv_virt_board_init(MachineState *machine)
         main_mem);
 
     /* create device tree */
-    fdt = create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline);
+    create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline);
 
     /* boot rom */
     memory_region_init_ram(boot_rom, NULL, "riscv_virt_board.bootrom",
@@ -314,9 +311,9 @@ static void riscv_virt_board_init(MachineState *machine)
             hwaddr end = load_initrd(machine->initrd_filename,
                                      machine->ram_size, kernel_entry,
                                      &start);
-            qemu_fdt_setprop_cell(fdt, "/chosen",
-                                  "linux,initrd-start", start);
-            qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-end",
+            qemu_fdt_setprop_cell(s->fdt, "/chosen", "linux,initrd-start",
+                                  start);
+            qemu_fdt_setprop_cell(s->fdt, "/chosen", "linux,initrd-end",
                                   end);
         }
     }
-- 
2.7.0

[Qemu-devel] [PATCH v3 02/24] RISC-V: Replace hardcoded constants with enum values

[Michael Clark]

The RISC-V device-tree code has a number of hard-coded
constants and this change moves them into header enums.

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>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/riscv/sifive_clint.c         | 9 +++------
 hw/riscv/sifive_u.c             | 6 ++++--
 hw/riscv/spike.c                | 6 ++++--
 hw/riscv/virt.c                 | 6 ++++--
 include/hw/riscv/sifive_clint.h | 4 ++++
 include/hw/riscv/sifive_u.h     | 4 ++++
 include/hw/riscv/spike.h        | 4 ++++
 include/hw/riscv/virt.h         | 4 ++++
 8 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/hw/riscv/sifive_clint.c b/hw/riscv/sifive_clint.c
index 4893453..7cc606e 100644
--- a/hw/riscv/sifive_clint.c
+++ b/hw/riscv/sifive_clint.c
@@ -26,13 +26,10 @@
 #include "hw/riscv/sifive_clint.h"
 #include "qemu/timer.h"
 
-/* See: riscv-pk/machine/sbi_entry.S and arch/riscv/kernel/time.c */
-#define TIMER_FREQ (10 * 1000 * 1000)
-
 static uint64_t cpu_riscv_read_rtc(void)
 {
-    return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), TIMER_FREQ,
-                    NANOSECONDS_PER_SECOND);
+    return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+        SIFIVE_CLINT_TIMEBASE_FREQ, NANOSECONDS_PER_SECOND);
 }
 
 /*
@@ -59,7 +56,7 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu, uint64_t value)
     diff = cpu->env.timecmp - rtc_r;
     /* back to ns (note args switched in muldiv64) */
     next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
-        muldiv64(diff, NANOSECONDS_PER_SECOND, TIMER_FREQ);
+        muldiv64(diff, NANOSECONDS_PER_SECOND, SIFIVE_CLINT_TIMEBASE_FREQ);
     timer_mod(cpu->env.timer, next);
 }
 
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 1c2deef..f3f7615 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -122,7 +122,8 @@ static void create_fdt(SiFiveUState *s, const struct MemmapEntry *memmap,
     g_free(nodename);
 
     qemu_fdt_add_subnode(fdt, "/cpus");
-    qemu_fdt_setprop_cell(fdt, "/cpus", "timebase-frequency", 10000000);
+    qemu_fdt_setprop_cell(fdt, "/cpus", "timebase-frequency",
+        SIFIVE_CLINT_TIMEBASE_FREQ);
     qemu_fdt_setprop_cell(fdt, "/cpus", "#size-cells", 0x0);
     qemu_fdt_setprop_cell(fdt, "/cpus", "#address-cells", 0x1);
 
@@ -131,7 +132,8 @@ static void create_fdt(SiFiveUState *s, const struct MemmapEntry *memmap,
         char *intc = g_strdup_printf("/cpus/cpu@%d/interrupt-controller", cpu);
         char *isa = riscv_isa_string(&s->soc.harts[cpu]);
         qemu_fdt_add_subnode(fdt, nodename);
-        qemu_fdt_setprop_cell(fdt, nodename, "clock-frequency", 1000000000);
+        qemu_fdt_setprop_cell(fdt, nodename, "clock-frequency",
+                              SIFIVE_U_CLOCK_FREQ);
         qemu_fdt_setprop_string(fdt, nodename, "mmu-type", "riscv,sv48");
         qemu_fdt_setprop_string(fdt, nodename, "riscv,isa", isa);
         qemu_fdt_setprop_string(fdt, nodename, "compatible", "riscv");
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 2d1f114..4c233ec 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -115,7 +115,8 @@ static void create_fdt(SpikeState *s, const struct MemmapEntry *memmap,
     g_free(nodename);
 
     qemu_fdt_add_subnode(fdt, "/cpus");
-    qemu_fdt_setprop_cell(fdt, "/cpus", "timebase-frequency", 10000000);
+    qemu_fdt_setprop_cell(fdt, "/cpus", "timebase-frequency",
+        SIFIVE_CLINT_TIMEBASE_FREQ);
     qemu_fdt_setprop_cell(fdt, "/cpus", "#size-cells", 0x0);
     qemu_fdt_setprop_cell(fdt, "/cpus", "#address-cells", 0x1);
 
@@ -124,7 +125,8 @@ static void create_fdt(SpikeState *s, const struct MemmapEntry *memmap,
         char *intc = g_strdup_printf("/cpus/cpu@%d/interrupt-controller", cpu);
         char *isa = riscv_isa_string(&s->soc.harts[cpu]);
         qemu_fdt_add_subnode(fdt, nodename);
-        qemu_fdt_setprop_cell(fdt, nodename, "clock-frequency", 1000000000);
+        qemu_fdt_setprop_cell(fdt, nodename, "clock-frequency",
+                              SPIKE_CLOCK_FREQ);
         qemu_fdt_setprop_string(fdt, nodename, "mmu-type", "riscv,sv48");
         qemu_fdt_setprop_string(fdt, nodename, "riscv,isa", isa);
         qemu_fdt_setprop_string(fdt, nodename, "compatible", "riscv");
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 37968d2..a402856 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -145,7 +145,8 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
     g_free(nodename);
 
     qemu_fdt_add_subnode(fdt, "/cpus");
-    qemu_fdt_setprop_cell(fdt, "/cpus", "timebase-frequency", 10000000);
+    qemu_fdt_setprop_cell(fdt, "/cpus", "timebase-frequency",
+                          SIFIVE_CLINT_TIMEBASE_FREQ);
     qemu_fdt_setprop_cell(fdt, "/cpus", "#size-cells", 0x0);
     qemu_fdt_setprop_cell(fdt, "/cpus", "#address-cells", 0x1);
 
@@ -155,7 +156,8 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
         char *intc = g_strdup_printf("/cpus/cpu@%d/interrupt-controller", cpu);
         char *isa = riscv_isa_string(&s->soc.harts[cpu]);
         qemu_fdt_add_subnode(fdt, nodename);
-        qemu_fdt_setprop_cell(fdt, nodename, "clock-frequency", 1000000000);
+        qemu_fdt_setprop_cell(fdt, nodename, "clock-frequency",
+                              VIRT_CLOCK_FREQ);
         qemu_fdt_setprop_string(fdt, nodename, "mmu-type", "riscv,sv48");
         qemu_fdt_setprop_string(fdt, nodename, "riscv,isa", isa);
         qemu_fdt_setprop_string(fdt, nodename, "compatible", "riscv");
diff --git a/include/hw/riscv/sifive_clint.h b/include/hw/riscv/sifive_clint.h
index aaa2a58..e2865be 100644
--- a/include/hw/riscv/sifive_clint.h
+++ b/include/hw/riscv/sifive_clint.h
@@ -47,4 +47,8 @@ enum {
     SIFIVE_TIME_BASE    = 0xBFF8
 };
 
+enum {
+    SIFIVE_CLINT_TIMEBASE_FREQ = 10000000
+};
+
 #endif
diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
index 662e8a1..be38aa0 100644
--- a/include/hw/riscv/sifive_u.h
+++ b/include/hw/riscv/sifive_u.h
@@ -50,6 +50,10 @@ enum {
     SIFIVE_U_UART1_IRQ = 4
 };
 
+enum {
+    SIFIVE_U_CLOCK_FREQ = 1000000000
+};
+
 #define SIFIVE_U_PLIC_HART_CONFIG "MS"
 #define SIFIVE_U_PLIC_NUM_SOURCES 127
 #define SIFIVE_U_PLIC_NUM_PRIORITIES 7
diff --git a/include/hw/riscv/spike.h b/include/hw/riscv/spike.h
index cb55a14..d85a64e 100644
--- a/include/hw/riscv/spike.h
+++ b/include/hw/riscv/spike.h
@@ -42,6 +42,10 @@ enum {
     SPIKE_DRAM
 };
 
+enum {
+    SPIKE_CLOCK_FREQ = 1000000000
+};
+
 #if defined(TARGET_RISCV32)
 #define SPIKE_V1_09_1_CPU TYPE_RISCV_CPU_RV32GCSU_V1_09_1
 #define SPIKE_V1_10_0_CPU TYPE_RISCV_CPU_RV32GCSU_V1_10_0
diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
index 7525647..2fbe808 100644
--- a/include/hw/riscv/virt.h
+++ b/include/hw/riscv/virt.h
@@ -55,6 +55,10 @@ enum {
     VIRTIO_NDEV = 10
 };
 
+enum {
+    VIRT_CLOCK_FREQ = 1000000000
+};
+
 #define VIRT_PLIC_HART_CONFIG "MS"
 #define VIRT_PLIC_NUM_SOURCES 127
 #define VIRT_PLIC_NUM_PRIORITIES 7
-- 
2.7.0

[Qemu-devel] [PATCH v3 03/24] RISC-V: Make virt board description match spike

[Michael Clark]

This makes 'qemu-system-riscv64 -machine help' output more tidy
and consistent.

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>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/riscv/virt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index a402856..0055439 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -404,7 +404,7 @@ static const TypeInfo riscv_virt_board_device = {
 
 static void riscv_virt_board_machine_init(MachineClass *mc)
 {
-    mc->desc = "RISC-V VirtIO Board (Privileged spec v1.10)";
+    mc->desc = "RISC-V VirtIO Board (Privileged ISA v1.10)";
     mc->init = riscv_virt_board_init;
     mc->max_cpus = 8; /* hardcoded limit in BBL */
 }
-- 
2.7.0

[Qemu-devel] [PATCH v3 04/24] RISC-V: Use ROM base address and size from memmap

[Michael Clark]

Another case of replacing hard coded constants, this time
referring to the definition in the virt machine's memmap.

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>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/riscv/virt.c         | 4 ++--
 include/hw/riscv/virt.h | 2 --
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 0055439..0d101fc 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -338,11 +338,11 @@ static void riscv_virt_board_init(MachineState *machine)
     };
 
     /* copy in the reset vector */
-    copy_le32_to_phys(ROM_BASE, reset_vec, sizeof(reset_vec));
+    copy_le32_to_phys(memmap[VIRT_MROM].base, reset_vec, sizeof(reset_vec));
 
     /* copy in the device tree */
     qemu_fdt_dumpdtb(s->fdt, s->fdt_size);
-    cpu_physical_memory_write(ROM_BASE + sizeof(reset_vec),
+    cpu_physical_memory_write(memmap[VIRT_MROM].base + sizeof(reset_vec),
         s->fdt, s->fdt_size);
 
     /* create PLIC hart topology configuration string */
diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
index 2fbe808..655e85d 100644
--- a/include/hw/riscv/virt.h
+++ b/include/hw/riscv/virt.h
@@ -23,8 +23,6 @@
 #define VIRT(obj) \
     OBJECT_CHECK(RISCVVirtState, (obj), TYPE_RISCV_VIRT_BOARD)
 
-enum { ROM_BASE = 0x1000 };
-
 typedef struct {
     /*< private >*/
     SysBusDevice parent_obj;
-- 
2.7.0

[Qemu-devel] [PATCH v3 05/24] RISC-V: Remove identity_translate from load_elf

[Michael Clark]

When load_elf is called with NULL as an argument to the
address translate callback, it does an identity translation.
This commit removes the redundant identity_translate callback.

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>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/riscv/sifive_e.c | 7 +------
 hw/riscv/sifive_u.c | 7 +------
 hw/riscv/spike.c    | 7 +------
 hw/riscv/virt.c     | 7 +------
 4 files changed, 4 insertions(+), 24 deletions(-)

diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
index 19eca36..09c9d49 100644
--- a/hw/riscv/sifive_e.c
+++ b/hw/riscv/sifive_e.c
@@ -82,16 +82,11 @@ static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t len)
     }
 }
 
-static uint64_t identity_translate(void *opaque, uint64_t addr)
-{
-    return addr;
-}
-
 static uint64_t load_kernel(const char *kernel_filename)
 {
     uint64_t kernel_entry, kernel_high;
 
-    if (load_elf(kernel_filename, identity_translate, NULL,
+    if (load_elf(kernel_filename, NULL, NULL,
                  &kernel_entry, NULL, &kernel_high,
                  0, ELF_MACHINE, 1, 0) < 0) {
         error_report("qemu: could not load kernel '%s'", kernel_filename);
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index f3f7615..6116c38 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -68,16 +68,11 @@ static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t len)
     }
 }
 
-static uint64_t identity_translate(void *opaque, uint64_t addr)
-{
-    return addr;
-}
-
 static uint64_t load_kernel(const char *kernel_filename)
 {
     uint64_t kernel_entry, kernel_high;
 
-    if (load_elf(kernel_filename, identity_translate, NULL,
+    if (load_elf(kernel_filename, NULL, NULL,
                  &kernel_entry, NULL, &kernel_high,
                  0, ELF_MACHINE, 1, 0) < 0) {
         error_report("qemu: could not load kernel '%s'", kernel_filename);
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 4c233ec..7710333 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -59,16 +59,11 @@ static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t len)
     }
 }
 
-static uint64_t identity_translate(void *opaque, uint64_t addr)
-{
-    return addr;
-}
-
 static uint64_t load_kernel(const char *kernel_filename)
 {
     uint64_t kernel_entry, kernel_high;
 
-    if (load_elf_ram_sym(kernel_filename, identity_translate, NULL,
+    if (load_elf_ram_sym(kernel_filename, NULL, NULL,
             &kernel_entry, NULL, &kernel_high, 0, ELF_MACHINE, 1, 0,
             NULL, true, htif_symbol_callback) < 0) {
         error_report("qemu: could not load kernel '%s'", kernel_filename);
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 0d101fc..f8c19b4 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -62,16 +62,11 @@ static void copy_le32_to_phys(hwaddr pa, uint32_t *rom, size_t len)
     }
 }
 
-static uint64_t identity_translate(void *opaque, uint64_t addr)
-{
-    return addr;
-}
-
 static uint64_t load_kernel(const char *kernel_filename)
 {
     uint64_t kernel_entry, kernel_high;
 
-    if (load_elf(kernel_filename, identity_translate, NULL,
+    if (load_elf(kernel_filename, NULL, NULL,
                  &kernel_entry, NULL, &kernel_high,
                  0, ELF_MACHINE, 1, 0) < 0) {
         error_report("qemu: could not load kernel '%s'", kernel_filename);
-- 
2.7.0

[Qemu-devel] [PATCH v3 06/24] 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 v3 07/24] RISC-V: Remove unused class definitions

[Michael Clark]

Removes a whole lot of unnecessary boilerplate code. Machines
don't need to be objects. The expansion of the SOC object model
for the RISC-V machines will happen in the future as SiFive
plans to add their FE310 and FU540 SOCs to QEMU. However, it
seems that this present boilerplate is complete unnecessary.

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>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/riscv/riscv_hart.c       |  6 ------
 hw/riscv/sifive_e.c         | 25 -------------------------
 hw/riscv/sifive_u.c         | 25 -------------------------
 hw/riscv/spike.c            | 20 --------------------
 hw/riscv/virt.c             | 25 -------------------------
 include/hw/riscv/sifive_e.h |  5 -----
 include/hw/riscv/sifive_u.h |  5 -----
 include/hw/riscv/spike.h    |  7 ++++---
 include/hw/riscv/virt.h     |  5 -----
 9 files changed, 4 insertions(+), 119 deletions(-)

diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c
index 14e3c18..75ba7ed 100644
--- a/hw/riscv/riscv_hart.c
+++ b/hw/riscv/riscv_hart.c
@@ -68,16 +68,10 @@ static void riscv_harts_class_init(ObjectClass *klass, void *data)
     dc->realize = riscv_harts_realize;
 }
 
-static void riscv_harts_init(Object *obj)
-{
-    /* RISCVHartArrayState *s = SIFIVE_COREPLEX(obj); */
-}
-
 static const TypeInfo riscv_harts_info = {
     .name          = TYPE_RISCV_HART_ARRAY,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(RISCVHartArrayState),
-    .instance_init = riscv_harts_init,
     .class_init    = riscv_harts_class_init,
 };
 
diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
index 09c9d49..4872b68 100644
--- a/hw/riscv/sifive_e.c
+++ b/hw/riscv/sifive_e.c
@@ -194,24 +194,6 @@ static void riscv_sifive_e_init(MachineState *machine)
     }
 }
 
-static int riscv_sifive_e_sysbus_device_init(SysBusDevice *sysbusdev)
-{
-    return 0;
-}
-
-static void riscv_sifive_e_class_init(ObjectClass *klass, void *data)
-{
-    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
-    k->init = riscv_sifive_e_sysbus_device_init;
-}
-
-static const TypeInfo riscv_sifive_e_device = {
-    .name          = TYPE_SIFIVE_E,
-    .parent        = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(SiFiveEState),
-    .class_init    = riscv_sifive_e_class_init,
-};
-
 static void riscv_sifive_e_machine_init(MachineClass *mc)
 {
     mc->desc = "RISC-V Board compatible with SiFive E SDK";
@@ -220,10 +202,3 @@ static void riscv_sifive_e_machine_init(MachineClass *mc)
 }
 
 DEFINE_MACHINE("sifive_e", riscv_sifive_e_machine_init)
-
-static void riscv_sifive_e_register_types(void)
-{
-    type_register_static(&riscv_sifive_e_device);
-}
-
-type_init(riscv_sifive_e_register_types);
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 25df16c..083043a 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -302,31 +302,6 @@ static void riscv_sifive_u_init(MachineState *machine)
         SIFIVE_SIP_BASE, SIFIVE_TIMECMP_BASE, SIFIVE_TIME_BASE);
 }
 
-static int riscv_sifive_u_sysbus_device_init(SysBusDevice *sysbusdev)
-{
-    return 0;
-}
-
-static void riscv_sifive_u_class_init(ObjectClass *klass, void *data)
-{
-    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
-    k->init = riscv_sifive_u_sysbus_device_init;
-}
-
-static const TypeInfo riscv_sifive_u_device = {
-    .name          = TYPE_SIFIVE_U,
-    .parent        = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(SiFiveUState),
-    .class_init    = riscv_sifive_u_class_init,
-};
-
-static void riscv_sifive_u_register_types(void)
-{
-    type_register_static(&riscv_sifive_u_device);
-}
-
-type_init(riscv_sifive_u_register_types);
-
 static void riscv_sifive_u_machine_init(MachineClass *mc)
 {
     mc->desc = "RISC-V Board compatible with SiFive U SDK";
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 74edf33..64e585e 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -336,18 +336,6 @@ static void spike_v1_09_1_board_init(MachineState *machine)
         smp_cpus, SIFIVE_SIP_BASE, SIFIVE_TIMECMP_BASE, SIFIVE_TIME_BASE);
 }
 
-static const TypeInfo spike_v_1_09_1_device = {
-    .name          = TYPE_RISCV_SPIKE_V1_09_1_BOARD,
-    .parent        = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(SpikeState),
-};
-
-static const TypeInfo spike_v_1_10_0_device = {
-    .name          = TYPE_RISCV_SPIKE_V1_10_0_BOARD,
-    .parent        = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(SpikeState),
-};
-
 static void spike_v1_09_1_machine_init(MachineClass *mc)
 {
     mc->desc = "RISC-V Spike Board (Privileged ISA v1.9.1)";
@@ -365,11 +353,3 @@ static void spike_v1_10_0_machine_init(MachineClass *mc)
 
 DEFINE_MACHINE("spike_v1.9.1", spike_v1_09_1_machine_init)
 DEFINE_MACHINE("spike_v1.10", spike_v1_10_0_machine_init)
-
-static void riscv_spike_board_register_types(void)
-{
-    type_register_static(&spike_v_1_09_1_device);
-    type_register_static(&spike_v_1_10_0_device);
-}
-
-type_init(riscv_spike_board_register_types);
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index f1e3641..5913100 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -380,24 +380,6 @@ static void riscv_virt_board_init(MachineState *machine)
         serial_hds[0], DEVICE_LITTLE_ENDIAN);
 }
 
-static int riscv_virt_board_sysbus_device_init(SysBusDevice *sysbusdev)
-{
-    return 0;
-}
-
-static void riscv_virt_board_class_init(ObjectClass *klass, void *data)
-{
-    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
-    k->init = riscv_virt_board_sysbus_device_init;
-}
-
-static const TypeInfo riscv_virt_board_device = {
-    .name          = TYPE_RISCV_VIRT_BOARD,
-    .parent        = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(RISCVVirtState),
-    .class_init    = riscv_virt_board_class_init,
-};
-
 static void riscv_virt_board_machine_init(MachineClass *mc)
 {
     mc->desc = "RISC-V VirtIO Board (Privileged ISA v1.10)";
@@ -406,10 +388,3 @@ static void riscv_virt_board_machine_init(MachineClass *mc)
 }
 
 DEFINE_MACHINE("virt", riscv_virt_board_machine_init)
-
-static void riscv_virt_board_register_types(void)
-{
-    type_register_static(&riscv_virt_board_device);
-}
-
-type_init(riscv_virt_board_register_types);
diff --git a/include/hw/riscv/sifive_e.h b/include/hw/riscv/sifive_e.h
index 0aebc57..12ad6d2 100644
--- a/include/hw/riscv/sifive_e.h
+++ b/include/hw/riscv/sifive_e.h
@@ -19,11 +19,6 @@
 #ifndef HW_SIFIVE_E_H
 #define HW_SIFIVE_E_H
 
-#define TYPE_SIFIVE_E "riscv.sifive_e"
-
-#define SIFIVE_E(obj) \
-    OBJECT_CHECK(SiFiveEState, (obj), TYPE_SIFIVE_E)
-
 typedef struct SiFiveEState {
     /*< private >*/
     SysBusDevice parent_obj;
diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
index be38aa0..94a3905 100644
--- a/include/hw/riscv/sifive_u.h
+++ b/include/hw/riscv/sifive_u.h
@@ -19,11 +19,6 @@
 #ifndef HW_SIFIVE_U_H
 #define HW_SIFIVE_U_H
 
-#define TYPE_SIFIVE_U "riscv.sifive_u"
-
-#define SIFIVE_U(obj) \
-    OBJECT_CHECK(SiFiveUState, (obj), TYPE_SIFIVE_U)
-
 typedef struct SiFiveUState {
     /*< private >*/
     SysBusDevice parent_obj;
diff --git a/include/hw/riscv/spike.h b/include/hw/riscv/spike.h
index 179b6cf..8410430 100644
--- a/include/hw/riscv/spike.h
+++ b/include/hw/riscv/spike.h
@@ -19,10 +19,11 @@
 #ifndef HW_SPIKE_H
 #define HW_SPIKE_H
 
-#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"
-
 typedef struct {
+    /*< private >*/
+    SysBusDevice parent_obj;
+
+    /*< public >*/
     RISCVHartArrayState soc;
     void *fdt;
     int fdt_size;
diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
index 655e85d..b91a412 100644
--- a/include/hw/riscv/virt.h
+++ b/include/hw/riscv/virt.h
@@ -19,10 +19,6 @@
 #ifndef HW_VIRT_H
 #define HW_VIRT_H
 
-#define TYPE_RISCV_VIRT_BOARD "riscv.virt"
-#define VIRT(obj) \
-    OBJECT_CHECK(RISCVVirtState, (obj), TYPE_RISCV_VIRT_BOARD)
-
 typedef struct {
     /*< private >*/
     SysBusDevice parent_obj;
@@ -45,7 +41,6 @@ enum {
     VIRT_DRAM
 };
 
-
 enum {
     UART0_IRQ = 10,
     VIRTIO_IRQ = 1, /* 1 to 8 */
-- 
2.7.0

[Qemu-devel] [PATCH v3 08/24] 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 v3 09/24] RISC-V: Include intruction hex in disassembly

[Michael Clark]

This was added to help debug issues using -d in_asm. It is
useful to see the instruction bytes, as one can detect if
one is trying to execute ASCII or device-tree magic.

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>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 disas/riscv.c | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/disas/riscv.c b/disas/riscv.c
index 3c17501..4580308 100644
--- a/disas/riscv.c
+++ b/disas/riscv.c
@@ -2769,25 +2769,6 @@ static void format_inst(char *buf, size_t buflen, size_t tab, rv_decode *dec)
     char tmp[64];
     const char *fmt;
 
-    if (dec->op == rv_op_illegal) {
-        size_t len = inst_length(dec->inst);
-        switch (len) {
-        case 2:
-            snprintf(buf, buflen, "(0x%04" PRIx64 ")", dec->inst);
-            break;
-        case 4:
-            snprintf(buf, buflen, "(0x%08" PRIx64 ")", dec->inst);
-            break;
-        case 6:
-            snprintf(buf, buflen, "(0x%012" PRIx64 ")", dec->inst);
-            break;
-        default:
-            snprintf(buf, buflen, "(0x%016" PRIx64 ")", dec->inst);
-            break;
-        }
-        return;
-    }
-
     fmt = opcode_data[dec->op].format;
     while (*fmt) {
         switch (*fmt) {
@@ -3004,6 +2985,11 @@ disasm_inst(char *buf, size_t buflen, rv_isa isa, uint64_t pc, rv_inst inst)
     format_inst(buf, buflen, 16, &dec);
 }
 
+#define INST_FMT_2 "%04" PRIx64 "              "
+#define INST_FMT_4 "%08" PRIx64 "          "
+#define INST_FMT_6 "%012" PRIx64 "      "
+#define INST_FMT_8 "%016" PRIx64 "  "
+
 static int
 print_insn_riscv(bfd_vma memaddr, struct disassemble_info *info, rv_isa isa)
 {
@@ -3031,6 +3017,21 @@ print_insn_riscv(bfd_vma memaddr, struct disassemble_info *info, rv_isa isa)
         }
     }
 
+    switch (len) {
+    case 2:
+        (*info->fprintf_func)(info->stream, INST_FMT_2, inst);
+        break;
+    case 4:
+        (*info->fprintf_func)(info->stream, INST_FMT_4, inst);
+        break;
+    case 6:
+        (*info->fprintf_func)(info->stream, INST_FMT_6, inst);
+        break;
+    default:
+        (*info->fprintf_func)(info->stream, INST_FMT_8, inst);
+        break;
+    }
+
     disasm_inst(buf, sizeof(buf), isa, memaddr, inst);
     (*info->fprintf_func)(info->stream, "%s", buf);
 
-- 
2.7.0

[Qemu-devel] [PATCH v3 10/24] RISC-V: Hold rcu_read_lock when accessing memory

[Michael Clark]

>From reading other code that accesses memory regions directly,
it appears that the rcu_read_lock needs to be held. Note: the
original code for accessing RAM directly was added because
there is no other way to use atomic_cmpxchg on guest physical
address space.

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 | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/target/riscv/helper.c b/target/riscv/helper.c
index 02cbcea..e71633a 100644
--- a/target/riscv/helper.c
+++ b/target/riscv/helper.c
@@ -209,6 +209,9 @@ restart:
                    as the PTE is no longer valid */
                 MemoryRegion *mr;
                 hwaddr l = sizeof(target_ulong), addr1;
+                enum { success, translate_fail, restart_walk} action = success;
+
+                rcu_read_lock();
                 mr = address_space_translate(cs->as, pte_addr,
                     &addr1, &l, false);
                 if (memory_access_is_direct(mr, true)) {
@@ -222,7 +225,7 @@ restart:
                     target_ulong old_pte =
                         atomic_cmpxchg(pte_pa, pte, updated_pte);
                     if (old_pte != pte) {
-                        goto restart;
+                        action = restart_walk;
                     } else {
                         pte = updated_pte;
                     }
@@ -230,7 +233,14 @@ restart:
                 } else {
                     /* misconfigured PTE in ROM (AD bits are not preset) or
                      * PTE is in IO space and can't be updated atomically */
-                    return TRANSLATE_FAIL;
+                    action = translate_fail;
+                }
+                rcu_read_unlock();
+
+                switch (action) {
+                    case success: break;
+                    case translate_fail: return TRANSLATE_FAIL;
+                    case restart_walk: goto restart;
                 }
             }
 
-- 
2.7.0

[Qemu-devel] [PATCH v3 11/24] 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 e71633a..523a275 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;
                 enum { success, translate_fail, restart_walk} action = success;
@@ -249,15 +272,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 v3 12/24] 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 4851890..d2ae56a 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 cff02a2..3a0ca2f 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -71,6 +71,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 v3 13/24] RISC-V: Make some header guards more specific

[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>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/riscv/spike.h | 4 ++--
 include/hw/riscv/virt.h  | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/hw/riscv/spike.h b/include/hw/riscv/spike.h
index 8410430..641b70d 100644
--- a/include/hw/riscv/spike.h
+++ b/include/hw/riscv/spike.h
@@ -16,8 +16,8 @@
  * this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifndef HW_SPIKE_H
-#define HW_SPIKE_H
+#ifndef HW_RISCV_SPIKE_H
+#define HW_RISCV_SPIKE_H
 
 typedef struct {
     /*< private >*/
diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
index b91a412..3a4f23e 100644
--- a/include/hw/riscv/virt.h
+++ b/include/hw/riscv/virt.h
@@ -16,8 +16,8 @@
  * this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifndef HW_VIRT_H
-#define HW_VIRT_H
+#ifndef HW_RISCV_VIRT_H
+#define HW_RISCV_VIRT_H
 
 typedef struct {
     /*< private >*/
-- 
2.7.0

[Qemu-devel] [PATCH v3 14/24] RISC-V: Make virt header comment title consistent

[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>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/riscv/virt.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
index 3a4f23e..91163d6 100644
--- a/include/hw/riscv/virt.h
+++ b/include/hw/riscv/virt.h
@@ -1,5 +1,5 @@
 /*
- * SiFive VirtIO Board
+ * QEMU RISC-V VirtIO machine interface
  *
  * Copyright (c) 2017 SiFive, Inc.
  *
-- 
2.7.0

[Qemu-devel] [PATCH v3 15/24] 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 523a275..c430e95 100644
--- a/target/riscv/helper.c
+++ b/target/riscv/helper.c
@@ -237,7 +237,7 @@ restart:
                 rcu_read_lock();
                 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 v3 16/24] RISC-V: Remove EM_RISCV ELF_MACHINE indirection

[Michael Clark]

Pointless indirection. Other ports use EM_ constants directly.

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>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/riscv/sifive_e.c | 2 +-
 hw/riscv/sifive_u.c | 2 +-
 hw/riscv/spike.c    | 2 +-
 hw/riscv/virt.c     | 2 +-
 target/riscv/cpu.h  | 1 -
 5 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
index 4872b68..39e4cb4 100644
--- a/hw/riscv/sifive_e.c
+++ b/hw/riscv/sifive_e.c
@@ -88,7 +88,7 @@ static uint64_t load_kernel(const char *kernel_filename)
 
     if (load_elf(kernel_filename, NULL, NULL,
                  &kernel_entry, NULL, &kernel_high,
-                 0, ELF_MACHINE, 1, 0) < 0) {
+                 0, EM_RISCV, 1, 0) < 0) {
         error_report("qemu: could not load kernel '%s'", kernel_filename);
         exit(1);
     }
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 57b4f4f..0e633a0 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -74,7 +74,7 @@ static uint64_t load_kernel(const char *kernel_filename)
 
     if (load_elf(kernel_filename, NULL, NULL,
                  &kernel_entry, NULL, &kernel_high,
-                 0, ELF_MACHINE, 1, 0) < 0) {
+                 0, EM_RISCV, 1, 0) < 0) {
         error_report("qemu: could not load kernel '%s'", kernel_filename);
         exit(1);
     }
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index c7d937b..70e697c 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -64,7 +64,7 @@ static uint64_t load_kernel(const char *kernel_filename)
     uint64_t kernel_entry, kernel_high;
 
     if (load_elf_ram_sym(kernel_filename, NULL, NULL,
-            &kernel_entry, NULL, &kernel_high, 0, ELF_MACHINE, 1, 0,
+            &kernel_entry, NULL, &kernel_high, 0, EM_RISCV, 1, 0,
             NULL, true, htif_symbol_callback) < 0) {
         error_report("qemu: could not load kernel '%s'", kernel_filename);
         exit(1);
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index d680cbd..e3f8bb7 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -68,7 +68,7 @@ static uint64_t load_kernel(const char *kernel_filename)
 
     if (load_elf(kernel_filename, NULL, NULL,
                  &kernel_entry, NULL, &kernel_high,
-                 0, ELF_MACHINE, 1, 0) < 0) {
+                 0, EM_RISCV, 1, 0) < 0) {
         error_report("qemu: could not load kernel '%s'", kernel_filename);
         exit(1);
     }
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 3a0ca2f..7c4482b 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -34,7 +34,6 @@
 
 #define TCG_GUEST_DEFAULT_MO 0
 
-#define ELF_MACHINE EM_RISCV
 #define CPUArchState struct CPURISCVState
 
 #include "qemu-common.h"
-- 
2.7.0

[Qemu-devel] [PATCH v3 17/24] 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 v3 18/24] RISC-V: Remove braces from satp case statement

[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>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/riscv/op_helper.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index dd3e417..f79716a 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -240,7 +240,7 @@ void csr_write_helper(CPURISCVState *env, target_ulong val_to_write,
         csr_write_helper(env, next_mie, CSR_MIE);
         break;
     }
-    case CSR_SATP: /* CSR_SPTBR */ {
+    case CSR_SATP: /* CSR_SPTBR */
         if (!riscv_feature(env, RISCV_FEATURE_MMU)) {
             break;
         }
@@ -258,7 +258,6 @@ void csr_write_helper(CPURISCVState *env, target_ulong val_to_write,
             env->satp = val_to_write;
         }
         break;
-    }
     case CSR_SEPC:
         env->sepc = val_to_write;
         break;
-- 
2.7.0

[Qemu-devel] [PATCH v3 19/24] 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 7c4482b..f47fc9c 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 v3 20/24] 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..aa101cc 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 v3 21/24] 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 aa101cc..f8595a6 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 v3 22/24] 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 f8595a6..f543e61 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 v3 23/24] RISC-V: Convert cpu definition towards future model

[Michael Clark]

- Model borrowed from target/sh4/cpu.c
- Rewrote riscv_cpu_list to use object_class_get_list
- Dropped 'struct RISCVCPUInfo' and used TypeInfo array
- Replaced riscv_cpu_register_types with DEFINE_TYPES
- Marked base class as abstract

Cc: Igor Mammedov <imammedo@redhat.com>
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>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/riscv/cpu.c | 123 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 69 insertions(+), 54 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index d2ae56a..1f25968 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -115,6 +115,8 @@ static void riscv_any_cpu_init(Object *obj)
     set_resetvec(env, DEFAULT_RSTVEC);
 }
 
+#if defined(TARGET_RISCV32)
+
 static void rv32gcsu_priv1_09_1_cpu_init(Object *obj)
 {
     CPURISCVState *env = &RISCV_CPU(obj)->env;
@@ -141,6 +143,8 @@ static void rv32imacu_nommu_cpu_init(Object *obj)
     set_resetvec(env, DEFAULT_RSTVEC);
 }
 
+#elif defined(TARGET_RISCV64)
+
 static void rv64gcsu_priv1_09_1_cpu_init(Object *obj)
 {
     CPURISCVState *env = &RISCV_CPU(obj)->env;
@@ -167,20 +171,7 @@ static void rv64imacu_nommu_cpu_init(Object *obj)
     set_resetvec(env, DEFAULT_RSTVEC);
 }
 
-static const RISCVCPUInfo riscv_cpus[] = {
-    { 96, TYPE_RISCV_CPU_ANY,              riscv_any_cpu_init },
-    { 32, TYPE_RISCV_CPU_RV32GCSU_V1_09_1, rv32gcsu_priv1_09_1_cpu_init },
-    { 32, TYPE_RISCV_CPU_RV32GCSU_V1_10_0, rv32gcsu_priv1_10_0_cpu_init },
-    { 32, TYPE_RISCV_CPU_RV32IMACU_NOMMU,  rv32imacu_nommu_cpu_init },
-    { 32, TYPE_RISCV_CPU_SIFIVE_E31,       rv32imacu_nommu_cpu_init },
-    { 32, TYPE_RISCV_CPU_SIFIVE_U34,       rv32gcsu_priv1_10_0_cpu_init },
-    { 64, TYPE_RISCV_CPU_RV64GCSU_V1_09_1, rv64gcsu_priv1_09_1_cpu_init },
-    { 64, TYPE_RISCV_CPU_RV64GCSU_V1_10_0, rv64gcsu_priv1_10_0_cpu_init },
-    { 64, TYPE_RISCV_CPU_RV64IMACU_NOMMU,  rv64imacu_nommu_cpu_init },
-    { 64, TYPE_RISCV_CPU_SIFIVE_E51,       rv64imacu_nommu_cpu_init },
-    { 64, TYPE_RISCV_CPU_SIFIVE_U54,       rv64gcsu_priv1_10_0_cpu_init },
-    { 0, NULL, NULL }
-};
+#endif
 
 static ObjectClass *riscv_cpu_class_by_name(const char *cpu_model)
 {
@@ -366,28 +357,6 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
     cc->vmsd = &vmstate_riscv_cpu;
 }
 
-static void cpu_register(const RISCVCPUInfo *info)
-{
-    TypeInfo type_info = {
-        .name = info->name,
-        .parent = TYPE_RISCV_CPU,
-        .instance_size = sizeof(RISCVCPU),
-        .instance_init = info->initfn,
-    };
-
-    type_register(&type_info);
-}
-
-static const TypeInfo riscv_cpu_type_info = {
-    .name = TYPE_RISCV_CPU,
-    .parent = TYPE_CPU,
-    .instance_size = sizeof(RISCVCPU),
-    .instance_init = riscv_cpu_init,
-    .abstract = false,
-    .class_size = sizeof(RISCVCPUClass),
-    .class_init = riscv_cpu_class_init,
-};
-
 char *riscv_isa_string(RISCVCPU *cpu)
 {
     int i;
@@ -403,30 +372,76 @@ char *riscv_isa_string(RISCVCPU *cpu)
     return isa_string;
 }
 
-void riscv_cpu_list(FILE *f, fprintf_function cpu_fprintf)
+typedef struct RISCVCPUListState {
+    fprintf_function cpu_fprintf;
+    FILE *file;
+} RISCVCPUListState;
+
+static gint riscv_cpu_list_compare(gconstpointer a, gconstpointer b)
 {
-    const RISCVCPUInfo *info = riscv_cpus;
+    ObjectClass *class_a = (ObjectClass *)a;
+    ObjectClass *class_b = (ObjectClass *)b;
+    const char *name_a, *name_b;
 
-    while (info->name) {
-        if (info->bit_widths & TARGET_LONG_BITS) {
-            (*cpu_fprintf)(f, "%s\n", info->name);
-        }
-        info++;
-    }
+    name_a = object_class_get_name(class_a);
+    name_b = object_class_get_name(class_b);
+    return strcmp(name_a, name_b);
 }
 
-static void riscv_cpu_register_types(void)
+static void riscv_cpu_list_entry(gpointer data, gpointer user_data)
 {
-    const RISCVCPUInfo *info = riscv_cpus;
+    RISCVCPUListState *s = user_data;
+    const char *typename = object_class_get_name(OBJECT_CLASS(data));
+    int len = strlen(typename) - strlen(RISCV_CPU_TYPE_SUFFIX);
 
-    type_register_static(&riscv_cpu_type_info);
+    (*s->cpu_fprintf)(s->file, "%.*s\n", len, typename);
+}
 
-    while (info->name) {
-        if (info->bit_widths & TARGET_LONG_BITS) {
-            cpu_register(info);
-        }
-        info++;
-    }
+void riscv_cpu_list(FILE *f, fprintf_function cpu_fprintf)
+{
+    RISCVCPUListState s = {
+        .cpu_fprintf = cpu_fprintf,
+        .file = f,
+    };
+    GSList *list;
+
+    list = object_class_get_list(TYPE_RISCV_CPU, false);
+    list = g_slist_sort(list, riscv_cpu_list_compare);
+    g_slist_foreach(list, riscv_cpu_list_entry, &s);
+    g_slist_free(list);
 }
 
-type_init(riscv_cpu_register_types)
+#define DEFINE_CPU(type_name, initfn)      \
+    {                                      \
+        .name = type_name,                 \
+        .parent = TYPE_RISCV_CPU,          \
+        .instance_init = initfn            \
+    }
+
+static const TypeInfo riscv_cpu_type_infos[] = {
+    {
+        .name = TYPE_RISCV_CPU,
+        .parent = TYPE_CPU,
+        .instance_size = sizeof(RISCVCPU),
+        .instance_init = riscv_cpu_init,
+        .abstract = true,
+        .class_size = sizeof(RISCVCPUClass),
+        .class_init = riscv_cpu_class_init,
+    },
+    DEFINE_CPU(TYPE_RISCV_CPU_ANY,              riscv_any_cpu_init),
+#if defined(TARGET_RISCV32)
+    DEFINE_CPU(TYPE_RISCV_CPU_RV32GCSU_V1_09_1, rv32gcsu_priv1_09_1_cpu_init),
+    DEFINE_CPU(TYPE_RISCV_CPU_RV32GCSU_V1_10_0, rv32gcsu_priv1_10_0_cpu_init),
+    DEFINE_CPU(TYPE_RISCV_CPU_RV32IMACU_NOMMU,  rv32imacu_nommu_cpu_init),
+    DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_E31,       rv32imacu_nommu_cpu_init),
+    DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_U34,       rv32gcsu_priv1_10_0_cpu_init)
+#elif defined(TARGET_RISCV64)
+    DEFINE_CPU(TYPE_RISCV_CPU_RV64GCSU_V1_09_1, rv64gcsu_priv1_09_1_cpu_init),
+    DEFINE_CPU(TYPE_RISCV_CPU_RV64GCSU_V1_10_0, rv64gcsu_priv1_10_0_cpu_init),
+    DEFINE_CPU(TYPE_RISCV_CPU_RV64IMACU_NOMMU,  rv64imacu_nommu_cpu_init),
+    DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_E51,       rv64imacu_nommu_cpu_init),
+    DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_U54,       rv64gcsu_priv1_10_0_cpu_init)
+#endif
+};
+
+DEFINE_TYPES(riscv_cpu_type_infos)
-- 
2.7.0

[Qemu-devel] [PATCH v3 24/24] 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 c430e95..54d4ff7 100644
--- a/target/riscv/helper.c
+++ b/target/riscv/helper.c
@@ -499,6 +499,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;
@@ -520,6 +524,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

Re: [Qemu-devel] [PATCH v3 00/24] RISC-V Post-merge spec conformance and cleanup

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1521229281-73637-1-git-send-email-mjc@sifive.com
Subject: [Qemu-devel] [PATCH v3 00/24] RISC-V Post-merge spec conformance and cleanup

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/1521229281-73637-1-git-send-email-mjc@sifive.com -> patchew/1521229281-73637-1-git-send-email-mjc@sifive.com
Switched to a new branch 'test'
cf9ad8471c RISC-V: Clear mtval/stval on exceptions without info
e0eb5ad03f RISC-V: Convert cpu definition towards future model
cb986bd661 RISC-V: Remove support for adhoc X_COP interrupt
c1bea999fe RISC-V: No traps on writes to misa, minstret, mcycle
c45005f71d RISC-V: vectored traps are optional
23dccf73aa RISC-V: riscv-qemu port supports sv39 and sv48
9beecddea0 RISC-V: Remove braces from satp case statement
db362a77b9 RISC-V: Hardwire satp to 0 for no-mmu case
c696060959 RISC-V: Remove EM_RISCV ELF_MACHINE indirection
e48c09d25a RISC-V: Use memory_region_is_ram in pte update
eaa01d4e61 RISC-V: Make virt header comment title consistent
9435045fd8 RISC-V: Make some header guards more specific
23973a84f7 RISC-V: Update E order and I extension order
6efbb781e4 RISC-V: Improve page table walker spec compliance
924e46b0b8 RISC-V: Hold rcu_read_lock when accessing memory
eef95b067f RISC-V: Include intruction hex in disassembly
3884a14086 RISC-V: Make sure rom has space for fdt
49ad774612 RISC-V: Remove unused class definitions
4b771876d8 RISC-V: Mark ROM read-only after copying in code
646a8d2508 RISC-V: Remove identity_translate from load_elf
c582b7bbf0 RISC-V: Use ROM base address and size from memmap
8a3ef54b0d RISC-V: Make virt board description match spike
ba0cbace2d RISC-V: Replace hardcoded constants with enum values
ec0c0eea8c RISC-V: Make virt create_fdt interface consistent

=== OUTPUT BEGIN ===
Checking PATCH 1/24: RISC-V: Make virt create_fdt interface consistent...
Checking PATCH 2/24: RISC-V: Replace hardcoded constants with enum values...
Checking PATCH 3/24: RISC-V: Make virt board description match spike...
Checking PATCH 4/24: RISC-V: Use ROM base address and size from memmap...
Checking PATCH 5/24: RISC-V: Remove identity_translate from load_elf...
Checking PATCH 6/24: RISC-V: Mark ROM read-only after copying in code...
Checking PATCH 7/24: RISC-V: Remove unused class definitions...
Checking PATCH 8/24: RISC-V: Make sure rom has space for fdt...
Checking PATCH 9/24: RISC-V: Include intruction hex in disassembly...
Checking PATCH 10/24: RISC-V: Hold rcu_read_lock when accessing memory...
ERROR: switch and case should be at the same indent
#50: FILE: target/riscv/helper.c:240:
+                switch (action) {
+                    case success: break;
+                    case translate_fail: return TRANSLATE_FAIL;
+                    case restart_walk: goto restart;

ERROR: trailing statements should be on next line
#51: FILE: target/riscv/helper.c:241:
+                    case success: break;

ERROR: trailing statements should be on next line
#53: FILE: target/riscv/helper.c:243:
+                    case restart_walk: goto restart;

total: 3 errors, 0 warnings, 32 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 11/24: RISC-V: Improve page table walker spec compliance...
Checking PATCH 12/24: RISC-V: Update E order and I extension order...
Checking PATCH 13/24: RISC-V: Make some header guards more specific...
Checking PATCH 14/24: RISC-V: Make virt header comment title consistent...
Checking PATCH 15/24: RISC-V: Use memory_region_is_ram in pte update...
Checking PATCH 16/24: RISC-V: Remove EM_RISCV ELF_MACHINE indirection...
Checking PATCH 17/24: RISC-V: Hardwire satp to 0 for no-mmu case...
Checking PATCH 18/24: RISC-V: Remove braces from satp case statement...
Checking PATCH 19/24: RISC-V: riscv-qemu port supports sv39 and sv48...
Checking PATCH 20/24: RISC-V: vectored traps are optional...
ERROR: trailing whitespace
#27: FILE: target/riscv/op_helper.c:265:
+        /* we do not support vectored traps for asynchrounous interrupts */ $

ERROR: trailing whitespace
#42: FILE: target/riscv/op_helper.c:286:
+        /* we do not support vectored traps for asynchrounous interrupts */ $

total: 2 errors, 0 warnings, 28 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 21/24: RISC-V: No traps on writes to misa, minstret, mcycle...
Checking PATCH 22/24: RISC-V: Remove support for adhoc X_COP interrupt...
Checking PATCH 23/24: RISC-V: Convert cpu definition towards future model...
Checking PATCH 24/24: RISC-V: Clear mtval/stval on exceptions without info...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [patches] Re: [PATCH v3 00/24] RISC-V Post-merge spec conformance and cleanup

[Michael Clark]

On Fri, Mar 16, 2018 at 1:06 PM, <no-reply@patchew.org> wrote:

> Hi,
>
> This series seems to have some coding style problems. See output below for
> more information:
>
> Type: series
> Message-id: 1521229281-73637-1-git-send-email-mjc@sifive.com
> Subject: [Qemu-devel] [PATCH v3 00/24] RISC-V Post-merge spec conformance
> and cleanup
>
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
>
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
>
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> git config --local diff.algorithm histogram
>
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
>     echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback
> -; then
>         failed=1
>         echo
>     fi
>     n=$((n+1))
> done
>
> exit $failed
> === TEST SCRIPT END ===
>
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> From https://github.com/patchew-project/qemu
>  * [new tag]               patchew/1521229281-73637-1-
> git-send-email-mjc@sifive.com -> patchew/1521229281-73637-1-
> git-send-email-mjc@sifive.com
> Switched to a new branch 'test'
> cf9ad8471c RISC-V: Clear mtval/stval on exceptions without info
> e0eb5ad03f RISC-V: Convert cpu definition towards future model
> cb986bd661 RISC-V: Remove support for adhoc X_COP interrupt
> c1bea999fe RISC-V: No traps on writes to misa, minstret, mcycle
> c45005f71d RISC-V: vectored traps are optional
> 23dccf73aa RISC-V: riscv-qemu port supports sv39 and sv48
> 9beecddea0 RISC-V: Remove braces from satp case statement
> db362a77b9 RISC-V: Hardwire satp to 0 for no-mmu case
> c696060959 RISC-V: Remove EM_RISCV ELF_MACHINE indirection
> e48c09d25a RISC-V: Use memory_region_is_ram in pte update
> eaa01d4e61 RISC-V: Make virt header comment title consistent
> 9435045fd8 RISC-V: Make some header guards more specific
> 23973a84f7 RISC-V: Update E order and I extension order
> 6efbb781e4 RISC-V: Improve page table walker spec compliance
> 924e46b0b8 RISC-V: Hold rcu_read_lock when accessing memory
> eef95b067f RISC-V: Include intruction hex in disassembly
> 3884a14086 RISC-V: Make sure rom has space for fdt
> 49ad774612 RISC-V: Remove unused class definitions
> 4b771876d8 RISC-V: Mark ROM read-only after copying in code
> 646a8d2508 RISC-V: Remove identity_translate from load_elf
> c582b7bbf0 RISC-V: Use ROM base address and size from memmap
> 8a3ef54b0d RISC-V: Make virt board description match spike
> ba0cbace2d RISC-V: Replace hardcoded constants with enum values
> ec0c0eea8c RISC-V: Make virt create_fdt interface consistent
>
> === OUTPUT BEGIN ===
> Checking PATCH 1/24: RISC-V: Make virt create_fdt interface consistent...
> Checking PATCH 2/24: RISC-V: Replace hardcoded constants with enum
> values...
> Checking PATCH 3/24: RISC-V: Make virt board description match spike...
> Checking PATCH 4/24: RISC-V: Use ROM base address and size from memmap...
> Checking PATCH 5/24: RISC-V: Remove identity_translate from load_elf...
> Checking PATCH 6/24: RISC-V: Mark ROM read-only after copying in code...
> Checking PATCH 7/24: RISC-V: Remove unused class definitions...
> Checking PATCH 8/24: RISC-V: Make sure rom has space for fdt...
> Checking PATCH 9/24: RISC-V: Include intruction hex in disassembly...
> Checking PATCH 10/24: RISC-V: Hold rcu_read_lock when accessing memory...
> ERROR: switch and case should be at the same indent
> #50: FILE: target/riscv/helper.c:240:
> +                switch (action) {
> +                    case success: break;
> +                    case translate_fail: return TRANSLATE_FAIL;
> +                    case restart_walk: goto restart;
>
> ERROR: trailing statements should be on next line
> #51: FILE: target/riscv/helper.c:241:
> +                    case success: break;
>
> ERROR: trailing statements should be on next line
> #53: FILE: target/riscv/helper.c:243:
> +                    case restart_walk: goto restart;
>
> total: 3 errors, 0 warnings, 32 lines checked
>
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> Checking PATCH 11/24: RISC-V: Improve page table walker spec compliance...
> Checking PATCH 12/24: RISC-V: Update E order and I extension order...
> Checking PATCH 13/24: RISC-V: Make some header guards more specific...
> Checking PATCH 14/24: RISC-V: Make virt header comment title consistent...
> Checking PATCH 15/24: RISC-V: Use memory_region_is_ram in pte update...
> Checking PATCH 16/24: RISC-V: Remove EM_RISCV ELF_MACHINE indirection...
> Checking PATCH 17/24: RISC-V: Hardwire satp to 0 for no-mmu case...
> Checking PATCH 18/24: RISC-V: Remove braces from satp case statement...
> Checking PATCH 19/24: RISC-V: riscv-qemu port supports sv39 and sv48...
> Checking PATCH 20/24: RISC-V: vectored traps are optional...
> ERROR: trailing whitespace
> #27: FILE: target/riscv/op_helper.c:265:
> +        /* we do not support vectored traps for asynchrounous interrupts
> */ $
>
> ERROR: trailing whitespace
> #42: FILE: target/riscv/op_helper.c:286:
> +        /* we do not support vectored traps for asynchrounous interrupts
> */ $
>
> total: 2 errors, 0 warnings, 28 lines checked
>
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> Checking PATCH 21/24: RISC-V: No traps on writes to misa, minstret,
> mcycle...
> Checking PATCH 22/24: RISC-V: Remove support for adhoc X_COP interrupt...
> Checking PATCH 23/24: RISC-V: Convert cpu definition towards future
> model...
> Checking PATCH 24/24: RISC-V: Clear mtval/stval on exceptions without
> info...
> === OUTPUT END ===
>
> Test command exited with code: 1



BTW: I've just fixed the checkpatch warnings and they are in my tree here:

- https://github.com/michaeljclark/riscv-qemu/tree/qemu-devel

Re: [Qemu-devel] [PATCH v3 10/24] RISC-V: Hold rcu_read_lock when accessing memory

[Paolo Bonzini]

On 16/03/2018 20:41, Michael Clark wrote:
> From reading other code that accesses memory regions directly,
> it appears that the rcu_read_lock needs to be held. Note: the
> original code for accessing RAM directly was added because
> there is no other way to use atomic_cmpxchg on guest physical
> address space.
> 
> 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 | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)

I think the bug here is that rcu_read_lock/unlock is missing in
cpu_memory_rw_debug.  Are there any other callers you had in mind?

Paolo

> diff --git a/target/riscv/helper.c b/target/riscv/helper.c
> index 02cbcea..e71633a 100644
> --- a/target/riscv/helper.c
> +++ b/target/riscv/helper.c
> @@ -209,6 +209,9 @@ restart:
>                     as the PTE is no longer valid */
>                  MemoryRegion *mr;
>                  hwaddr l = sizeof(target_ulong), addr1;
> +                enum { success, translate_fail, restart_walk} action = success;
> +
> +                rcu_read_lock();
>                  mr = address_space_translate(cs->as, pte_addr,
>                      &addr1, &l, false);
>                  if (memory_access_is_direct(mr, true)) {
> @@ -222,7 +225,7 @@ restart:
>                      target_ulong old_pte =
>                          atomic_cmpxchg(pte_pa, pte, updated_pte);
>                      if (old_pte != pte) {
> -                        goto restart;
> +                        action = restart_walk;
>                      } else {
>                          pte = updated_pte;
>                      }
> @@ -230,7 +233,14 @@ restart:
>                  } else {
>                      /* misconfigured PTE in ROM (AD bits are not preset) or
>                       * PTE is in IO space and can't be updated atomically */
> -                    return TRANSLATE_FAIL;
> +                    action = translate_fail;
> +                }
> +                rcu_read_unlock();
> +
> +                switch (action) {
> +                    case success: break;
> +                    case translate_fail: return TRANSLATE_FAIL;
> +                    case restart_walk: goto restart;
>                  }
>              }
>  
> 

Re: [Qemu-devel] [PATCH v3 23/24] RISC-V: Convert cpu definition towards future model

[Igor Mammedov]

On Fri, 16 Mar 2018 12:41:20 -0700
Michael Clark <mjc@sifive.com> wrote:

> - Model borrowed from target/sh4/cpu.c
> - Rewrote riscv_cpu_list to use object_class_get_list
> - Dropped 'struct RISCVCPUInfo' and used TypeInfo array
> - Replaced riscv_cpu_register_types with DEFINE_TYPES
> - Marked base class as abstract
> 
> Cc: Igor Mammedov <imammedo@redhat.com>
> 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>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
[...]

> -static void riscv_cpu_register_types(void)
> +static void riscv_cpu_list_entry(gpointer data, gpointer user_data)
>  {
> -    const RISCVCPUInfo *info = riscv_cpus;
> +    RISCVCPUListState *s = user_data;
> +    const char *typename = object_class_get_name(OBJECT_CLASS(data));
> +    int len = strlen(typename) - strlen(RISCV_CPU_TYPE_SUFFIX);
This also fixes "-cpu" output,
before this patch:

# qemu-system-riscv32 -cpu help 
any-riscv-cpu
rv32gcsu-v1.9.1-riscv-cpu
rv32gcsu-v1.10.0-riscv-cpu
rv32imacu-nommu-riscv-cpu
sifive-e31-riscv-cpu
sifive-u34-riscv-cpu

# qemu-system-riscv32 -cpu rv32gcsu-v1.9.1-riscv-cpu 
qemu-system-riscv32: unable to find CPU model 'rv32gcsu-v1.9.1-riscv-cpu'

after this patch:

# qemu-system-riscv32 -cpu help
any
rv32gcsu-v1.10.0
rv32gcsu-v1.9.1
rv32imacu-nommu
sifive-e31
sifive-u34

which cpu model matches conversion rules of riscv_cpu_class_by_name()
and matching cpu type is found as expected.

> -    type_register_static(&riscv_cpu_type_info);
> +    (*s->cpu_fprintf)(s->file, "%.*s\n", len, typename);
> +}
[...]

Re: [Qemu-devel] [PATCH v3 10/24] RISC-V: Hold rcu_read_lock when accessing memory

[Michael Clark]

On Mon, Mar 19, 2018 at 2:41 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 16/03/2018 20:41, Michael Clark wrote:
> > From reading other code that accesses memory regions directly,
> > it appears that the rcu_read_lock needs to be held. Note: the
> > original code for accessing RAM directly was added because
> > there is no other way to use atomic_cmpxchg on guest physical
> > address space.
> >
> > 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 | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
>
> I think the bug here is that rcu_read_lock/unlock is missing in
> cpu_memory_rw_debug.  Are there any other callers you had in mind?


So this is not a bug in our patch per se, rather a bug in
cpu_memory_rw_debug?

It seems that most other code that that uses the address_space_* interfaces
(e.g. hw/virtio/virtio.c) holds the rcu_read_lock, presumably to handle
cases where the memory map might change at runtime, e.g. during ballooning.
This would be exhibited if a page walk collided with a balloon.

Ideally we could add a cpu_physical_memory_atomic_cmpxchg API to generic
code, and we could avoid holding the rcu_read_lock in target/riscv i.e.
encapsulate guest physical memory atomic_cmpxchg. The atomic_cmpxchg
wrapper handles multiple word widthes, so we would need
cpu_physical_memory_atomic_cmpxchg32
and cpu_physical_memory_atomic_cmpxchg64. We need to use atomic_cmpxchg in
the PTE update to detect the case where the PTE has changed between reading
it and updating the accessed dirty bits. As far as I can tell, the code is
correct. In fact we are reading a very strong interpretation of the RISC-V
specification.

This is what The RISC-V Instruction Set Manual Volume II: Privileged
Architecture says about accessed and dirty updates, which is the reason for
the code in question:

    When a virtual page is accessed and the A bit is clear, or is written
and the D bit is clear,
    the implementation sets the corresponding bit in the PTE. The PTE
update must be atomic
    with respect to other accesses to the PTE, and must atomically check
that the PTE is valid
    and grants suffi cient permissions. The PTE update must be exact (i.e.,
not speculative), and
    observed in program order by the local hart. The ordering on loads and
stores provided by
    FENCE instructions and the acquire/release bits on atomic instructions
also orders the PTE
    updates associated with those loads and stores as observed by remote
harts.

This is an interesting constraint. I believe the intent is that we just
AMOOR bit A+D bits on the PTE (although we don't have AMOOR, we have
atomic_cmpxchg), however we have read a stronger interpretation (while
stronger, it is not incorrect), and that is that the atomicity guarantee
extends from the page walker reading the PTE entry, checking its
permissions and then updating it, which in hardware would require the page
walker to take load reservations, and I don't think it does. Apparently the
hardware just AMOORs the bits, so the PTE could actually be pointing
somewhere else by the time we go to update the bits, although it is the
same virtual address has been accessed or dirtied (albiet with a different
physical translation). In any case, we have a very strong interpretation of
the specification wording, potentially stronger than hardware may provide.
We had a long discussion about this on the RISC-V QEMU issue tracker.
Stefan O'Rear mentioned he might make a test that hammers a PTE from
another thread while one thread is doing a page walk to try to cause the
page walker to set the A+D bits on a PTE that is different to the one it
used to create the virtual to physical mapping. That's some background
around why the code exists in the first place, which provides the strongest
possible gaurantee on PTE atomicity.

- https://github.com/riscv/riscv-qemu/pull/103
- https://github.com/riscv/riscv-qemu/pull/105

The get_physical_address bug that this patch fixes does not show up in
practice. i.e. we aren't actually hitting cases where we have page walks
colliding with address space changes, however I think holding rcu_read_lock
seems to be correct and this bug may show up in the future.

> diff --git a/target/riscv/helper.c b/target/riscv/helper.c
> > index 02cbcea..e71633a 100644
> > --- a/target/riscv/helper.c
> > +++ b/target/riscv/helper.c
> > @@ -209,6 +209,9 @@ restart:
> >                     as the PTE is no longer valid */
> >                  MemoryRegion *mr;
> >                  hwaddr l = sizeof(target_ulong), addr1;
> > +                enum { success, translate_fail, restart_walk} action =
> success;
> > +
> > +                rcu_read_lock();
> >                  mr = address_space_translate(cs->as, pte_addr,
> >                      &addr1, &l, false);
> >                  if (memory_access_is_direct(mr, true)) {
> > @@ -222,7 +225,7 @@ restart:
> >                      target_ulong old_pte =
> >                          atomic_cmpxchg(pte_pa, pte, updated_pte);
> >                      if (old_pte != pte) {
> > -                        goto restart;
> > +                        action = restart_walk;
> >                      } else {
> >                          pte = updated_pte;
> >                      }
> > @@ -230,7 +233,14 @@ restart:
> >                  } else {
> >                      /* misconfigured PTE in ROM (AD bits are not
> preset) or
> >                       * PTE is in IO space and can't be updated
> atomically */
> > -                    return TRANSLATE_FAIL;
> > +                    action = translate_fail;
> > +                }
> > +                rcu_read_unlock();
> > +
> > +                switch (action) {
> > +                    case success: break;
> > +                    case translate_fail: return TRANSLATE_FAIL;
> > +                    case restart_walk: goto restart;
> >                  }
> >              }
> >
> >
>
>

Re: [Qemu-devel] [PATCH v3 10/24] RISC-V: Hold rcu_read_lock when accessing memory

[Paolo Bonzini]

On 19/03/2018 22:07, Michael Clark wrote:
> 
> 
> On Mon, Mar 19, 2018 at 2:41 AM, Paolo Bonzini <pbonzini@redhat.com
> <mailto:pbonzini@redhat.com>> wrote:
> 
>     On 16/03/2018 20:41, Michael Clark wrote:
>     > From reading other code that accesses memory regions directly,
>     > it appears that the rcu_read_lock needs to be held. Note: the
>     > original code for accessing RAM directly was added because
>     > there is no other way to use atomic_cmpxchg on guest physical
>     > address space.
>     >
>     > Cc: Sagar Karandikar <sagark@eecs.berkeley.edu <mailto:sagark@eecs.berkeley.edu>>
>     > Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de <mailto:kbastian@mail.uni-paderborn.de>>
>     > Signed-off-by: Michael Clark <mjc@sifive.com <mailto:mjc@sifive.com>>
>     > Signed-off-by: Palmer Dabbelt <palmer@sifive.com <mailto:palmer@sifive.com>>
>     > ---
>     >  target/riscv/helper.c | 14 ++++++++++++--
>     >  1 file changed, 12 insertions(+), 2 deletions(-)
> 
>     I think the bug here is that rcu_read_lock/unlock is missing in
>     cpu_memory_rw_debug.  Are there any other callers you had in mind?
> 
> 
> So this is not a bug in our patch per se, rather a bug in
> cpu_memory_rw_debug?

Yes.

> It seems that most other code that that uses the address_space_*
> interfaces (e.g. hw/virtio/virtio.c) holds the rcu_read_lock, presumably
> to handle cases where the memory map might change at runtime, e.g.
> during ballooning. This would be exhibited if a page walk collided with
> a balloon.

Note that hw/virtio/virtio.c calls rcu_read_lock() not so much to
protect from memory map changes, but rather to protect reads of
vq->vring.caches.

In general, exec.c and memory.c take care of taking the lock.  Functions
that need the caller to take the lock are annotated with something like

    /* Called from RCU critical section.  */

(see for example flatview_write).

> Ideally we could add a cpu_physical_memory_atomic_cmpxchg API to generic
> code, and we could avoid holding the rcu_read_lock in target/riscv i.e.
> encapsulate guest physical memory atomic_cmpxchg. The atomic_cmpxchg
> wrapper handles multiple word widthes, so we would
> need cpu_physical_memory_atomic_cmpxchg32
> and cpu_physical_memory_atomic_cmpxchg64. We need to use atomic_cmpxchg
> in the PTE update to detect the case where the PTE has changed between
> reading it and updating the accessed dirty bits.

Yes, this makes sense.  In fact having such a function (more precisely
address_space_atomic_cmpxchg) would be useful for x86 too.  Right now
x86 is wrong in not using cmpxchg.

Even without such a function, however, I have two more things that I
noticed:

1) you're using a very low-level function, which complicates things a
bit for yourself.  You can use address_space_map/unmap too, which is a
bit simpler.

2) I'm wondering if the page walk needs to use load-acquire instructions
(and I cannot really find the relevant text in the privileged spec)

> This is an interesting constraint. I believe the intent is that we just
> AMOOR bit A+D bits on the PTE (although we don't have AMOOR, we have
> atomic_cmpxchg), however we have read a stronger interpretation (while
> stronger, it is not incorrect),

Stronger is never incorrect. :)

> and that is that the atomicity guarantee
> extends from the page walker reading the PTE entry, checking its
> permissions and then updating it, which in hardware would require the
> page walker to take load reservations, and I don't think it does.

Indeed.  But maybe another possibility could be to do an atomic_cmpxchg
and ignore it if the comparison failed?  This would be susceptible to
ABA problems, but apart from that it would be the same as taking a load
reservation (in assembly language tems, that's
load-link/or/store-conditional).

> Apparently the hardware just AMOORs the bits, so the PTE could actually
> be pointing somewhere else by the time we go to update the bits,
> although it is the same virtual address has been accessed or dirtied
> (albiet with a different physical translation). In any case, we have a
> very strong interpretation of the specification wording, potentially
> stronger than hardware may provide. We had a long discussion about this
> on the RISC-V QEMU issue tracker. Stefan O'Rear mentioned he might make
> a test that hammers a PTE from another thread while one thread is doing
> a page walk to try to cause the page walker to set the A+D bits on a PTE
> that is different to the one it used to create the virtual to physical
> mapping. That's some background around why the code exists in the first
> place, which provides the strongest possible gaurantee on PTE atomicity.
> 
> - https://github.com/riscv/riscv-qemu/pull/103
> - https://github.com/riscv/riscv-qemu/pull/105
> 
> The get_physical_address bug that this patch fixes does not show up in
> practice. i.e. we aren't actually hitting cases where we have page walks
> colliding with address space changes, however I think
> holding rcu_read_lock seems to be correct and this bug may show up in
> the future.

Yes, it is correct, but it shouldn't be your responsibility---the bug is
not in your code.

tlb_fill and thus riscv_cpu_handle_mmu_fault are already called with
rcu_read_lock (translated code always is) and the same ought to be true
for riscv_cpu_get_phys_page_debug.

All the callers of cpu_get_phys_page_attrs_debug must therefore call
rcu_read_lock(), and that leaves us two possibilities:

1) cpu_memory_rw_debug and breakpoint_invalidate (in this case,
tb_invalidate_phys_addr's RCU lock/unlock can also be pushed up to
breakpoint_invalidate)

2) cpu_get_phys_page_attrs_debug itself.

I'll try to set aside some time, and post a patch for this before 2.12.

Paolo

Re: [Qemu-devel] [PATCH v3 10/24] RISC-V: Hold rcu_read_lock when accessing memory

[Peter Maydell]

On 21 March 2018 at 13:55, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 19/03/2018 22:07, Michael Clark wrote:
>> We need to use atomic_cmpxchg
>> in the PTE update to detect the case where the PTE has changed between
>> reading it and updating the accessed dirty bits.
>
> Yes, this makes sense.  In fact having such a function (more precisely
> address_space_atomic_cmpxchg) would be useful for x86 too.  Right now
> x86 is wrong in not using cmpxchg.

Yeah, this is a known missing feature in our APIs for memory
accesses (it only starts to matter with MTTCG, really). We
ought to have functions that guarantee that they do the
access as a single 32/64 bit load/store, as well as
having atomic support. PPC and Arm TLB walk code will need
these. For the moment we just ignore the possibility of
races here, but for the 2.13 timeframe we really ought to
design a solution to this properly.

thanks
-- PMM