[Qemu-devel] [PATCH v1 00/22] Spec conformance bug fixes and cleanups

[Qemu-devel] [PATCH v1 00/22] Spec conformance bug fixes and cleanups

[Michael Clark]

This is the series of spec conformance bug fixes and code cleanups.
We would like to get this series in after our core changes in v8.2.

- 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
  - 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.
- Adds bounds checks when writing device-tree to ROM

Michael Clark (22):
  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 memory map
  RISC-V: Remove redundant identity_translate from load_elf
  RISC-V: Mark ROM read-only after copying in code and config
  RISC-V: Remove unused class definitions from machines
  RISC-V: Make sure the emulated rom has space for device-tree
  RISC-V: Include hexidecimal instruction in disassembly
  RISC-V: Hold rcu_read_lock when accessing memory directly
  RISC-V: Improve page table walker spec compliance
  RISC-V: Update E order and I extension order
  RISC-V: Make spike and virt header guards more specific
  RISC-V: Make virt header comment title consistent
  RISC-V: Use memory_region_is_ram in atomic pte update
  RISC-V: Remove EM_RISCV ELF_MACHINE indirection from load_elf
  RISC-V: Ingore satp writes and return 0 for reads when no-mmu
  RISC-V: Remove braces from satp case statement with no locals
  RISC-V: riscv-qemu port supports sv39 and sv48
  RISC-V: vectored traps are optional
  RISC-V: No traps on writes to misa,minstret[h],mcycle[h]
  RISC-V: Remove support for adhoc X_COP local-interrupt

 disas/riscv.c                   | 39 +++++++++++----------
 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     |  9 -----
 include/hw/riscv/sifive_u.h     | 13 +++----
 include/hw/riscv/spike.h        | 16 ++++-----
 include/hw/riscv/virt.h         | 21 ++++-------
 target/riscv/cpu.c              |  2 +-
 target/riscv/cpu.h              |  6 ++--
 target/riscv/cpu_bits.h         |  3 --
 target/riscv/helper.c           | 63 +++++++++++++++++++++++----------
 target/riscv/op_helper.c        | 52 ++++++++++++++--------------
 16 files changed, 193 insertions(+), 285 deletions(-)

-- 
2.7.0

[Qemu-devel] [PATCH v1 01/22] 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.

Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 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 v1 02/22] 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.

Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 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 v1 03/22] RISC-V: Make virt board description match spike

[Michael Clark]

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

Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 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 v1 04/22] RISC-V: Use ROM base address and size from memory

[Michael Clark]

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

Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 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 v1 05/22] RISC-V: Remove redundant identity_translate from

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

Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 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 v1 06/22] RISC-V: Mark ROM read-only after copying in code and

[Michael Clark]

The sifive_u machine already marks its ROM readonly. This fixes
the remaining boards.

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 v1 07/22] RISC-V: Remove unused class definitions from

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

Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 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 |  9 ---------
 include/hw/riscv/sifive_u.h |  9 ---------
 include/hw/riscv/virt.h     |  9 ---------
 7 files changed, 122 deletions(-)

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..818fbdc 100644
--- a/include/hw/riscv/sifive_e.h
+++ b/include/hw/riscv/sifive_e.h
@@ -19,16 +19,7 @@
 #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;
-
-    /*< public >*/
     RISCVHartArrayState soc;
     DeviceState *plic;
 } SiFiveEState;
diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
index be38aa0..8ebd545 100644
--- a/include/hw/riscv/sifive_u.h
+++ b/include/hw/riscv/sifive_u.h
@@ -19,16 +19,7 @@
 #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;
-
-    /*< public >*/
     RISCVHartArrayState soc;
     DeviceState *plic;
     void *fdt;
diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
index 655e85d..9588909 100644
--- a/include/hw/riscv/virt.h
+++ b/include/hw/riscv/virt.h
@@ -19,15 +19,7 @@
 #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;
-
-    /*< public >*/
     RISCVHartArrayState soc;
     DeviceState *plic;
     void *fdt;
@@ -45,7 +37,6 @@ enum {
     VIRT_DRAM
 };
 
-
 enum {
     UART0_IRQ = 10,
     VIRTIO_IRQ = 1, /* 1 to 8 */
-- 
2.7.0

[Qemu-devel] [PATCH v1 08/22] RISC-V: Make sure the emulated rom has space for

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

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 v1 09/22] RISC-V: Include hexidecimal instruction in

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

Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 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 v1 10/22] 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 easily.

Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Stefan O'Rear <sorear2@gmail.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 target/riscv/helper.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/riscv/helper.c b/target/riscv/helper.c
index 02cbcea..228933c 100644
--- a/target/riscv/helper.c
+++ b/target/riscv/helper.c
@@ -209,6 +209,7 @@ restart:
                    as the PTE is no longer valid */
                 MemoryRegion *mr;
                 hwaddr l = sizeof(target_ulong), addr1;
+                rcu_read_lock();
                 mr = address_space_translate(cs->as, pte_addr,
                     &addr1, &l, false);
                 if (memory_access_is_direct(mr, true)) {
@@ -222,16 +223,19 @@ restart:
                     target_ulong old_pte =
                         atomic_cmpxchg(pte_pa, pte, updated_pte);
                     if (old_pte != pte) {
+                        rcu_read_unlock();
                         goto restart;
                     } else {
                         pte = updated_pte;
                     }
 #endif
                 } else {
+                    rcu_read_unlock();
                     /* misconfigured PTE in ROM (AD bits are not preset) or
                      * PTE is in IO space and can't be updated atomically */
                     return TRANSLATE_FAIL;
                 }
+                rcu_read_unlock();
             }
 
             /* for superpage mappings, make a fake leaf PTE for the TLB's
-- 
2.7.0

[Qemu-devel] [PATCH v1 11/22] 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
- Change access checks from ternary operator to if statements
- Improves page walker comments
- No measurable performance impact on dd test

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   | 57 ++++++++++++++++++++++++++++++++++---------------
 2 files changed, 40 insertions(+), 19 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 228933c..2165ecb 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 mstats.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;
                 rcu_read_lock();
@@ -243,15 +266,15 @@ restart:
             target_ulong vpn = addr >> PGSHIFT;
             *physical = (ppn | (vpn & ((1L << ptshift) - 1))) << PGSHIFT;
 
+            /* set permissions on the TLB entry */
             if ((pte & PTE_R)) {
                 *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 v1 12/22] 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.

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 v1 13/22] RISC-V: Make spike and virt header guards more

[Michael Clark]

Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 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 179b6cf..ed9d1db 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
 
 #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"
diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
index 9588909..d22f184 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 {
     RISCVHartArrayState soc;
-- 
2.7.0

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

[Michael Clark]

Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 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 d22f184..dcb697f 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 v1 15/22] RISC-V: Use memory_region_is_ram in atomic pte

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

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 2165ecb..88551be 100644
--- a/target/riscv/helper.c
+++ b/target/riscv/helper.c
@@ -235,7 +235,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 v1 16/22] RISC-V: Remove EM_RISCV ELF_MACHINE indirection from

[Michael Clark]

Pointless indirection. Other ports use EM_ constants directly.

Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 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 v1 17/22] RISC-V: Ingore satp writes and return 0 for reads

[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
bear trap overhead, versus simply reading back the value and
checking if the write took (saves hundreds of cycles and much
more complex trap handling code).

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 v1 18/22] RISC-V: Remove braces from satp case statement with

[Michael Clark]

Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 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 v1 19/22] RISC-V: riscv-qemu port supports sv39 and sv48

[Michael Clark]

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 v1 20/22] 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.

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

Re: [Qemu-devel] [PATCH v1 02/22] RISC-V: Replace hardcoded constants with enum values

[Philippe Mathieu-Daudé]

On 03/06/2018 05:43 PM, Michael Clark wrote:
> The RISC-V device-tree code has a number of hard-coded
> constants and this change moves them into header enums.
> 
> 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
> 

Re: [Qemu-devel] [PATCH v1 05/22] RISC-V: Remove redundant identity_translate from

[Philippe Mathieu-Daudé]

On 03/06/2018 05:43 PM, Michael Clark wrote:
> 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.
> 
> 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);
> 

Re: [Qemu-devel] [PATCH v1 00/22] Spec conformance bug fixes and cleanups

[Michael Clark]

On Wed, Mar 7, 2018 at 9:43 AM, Michael Clark <mjc@sifive.com> wrote:

> This is the series of spec conformance bug fixes and code cleanups.
> We would like to get this series in after our core changes in v8.2.
>
> - 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
>   - 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.
> - Adds bounds checks when writing device-tree to ROM
>
> Michael Clark (22):
>   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 memory map
>   RISC-V: Remove redundant identity_translate from load_elf
>   RISC-V: Mark ROM read-only after copying in code and config
>   RISC-V: Remove unused class definitions from machines
>   RISC-V: Make sure the emulated rom has space for device-tree
>   RISC-V: Include hexidecimal instruction in disassembly
>   RISC-V: Hold rcu_read_lock when accessing memory directly
>   RISC-V: Improve page table walker spec compliance
>   RISC-V: Update E order and I extension order
>   RISC-V: Make spike and virt header guards more specific
>   RISC-V: Make virt header comment title consistent
>   RISC-V: Use memory_region_is_ram in atomic pte update
>   RISC-V: Remove EM_RISCV ELF_MACHINE indirection from load_elf
>   RISC-V: Ingore satp writes and return 0 for reads when no-mmu
>   RISC-V: Remove braces from satp case statement with no locals
>   RISC-V: riscv-qemu port supports sv39 and sv48
>   RISC-V: vectored traps are optional
>   RISC-V: No traps on writes to misa,minstret[h],mcycle[h]
>   RISC-V: Remove support for adhoc X_COP local-interrupt
>
>  disas/riscv.c                   | 39 +++++++++++----------
>  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     |  9 -----
>  include/hw/riscv/sifive_u.h     | 13 +++----
>  include/hw/riscv/spike.h        | 16 ++++-----
>  include/hw/riscv/virt.h         | 21 ++++-------
>  target/riscv/cpu.c              |  2 +-
>  target/riscv/cpu.h              |  6 ++--
>  target/riscv/cpu_bits.h         |  3 --
>  target/riscv/helper.c           | 63 +++++++++++++++++++++++----------
>  target/riscv/op_helper.c        | 52 ++++++++++++++--------------
>  16 files changed, 193 insertions(+), 285 deletions(-)
>

BTW Apologies for the duplicate emails. I'm still getting to grips with the
git-sendemail workflow and was using a sed script to Add Cc's which munged
the headers as it didn't take into account Subject lines flowing to two
lines. I guess I can just include Cc: in the commit message? and
git-format-patch will handle it for me? or I just should how to use
git-publish...

I still have to work on Igor's requested change to CPU initializer
declarations, and i'd also like to get the patch in that fixes fmin/fmax
test failures using the riscv-tests testsuite by implementing IEEE-754
minimumNumber/maximumNumber. I'll try to get a patch onto the list before
soft-freeze. I dropped the patch which was in an earlier version of our
port patch series due to a conflict with changes in softfloat (conversion
of minmax from a macro to a static function iirc).

Michael.

Re: [Qemu-devel] [PATCH v1 09/22] RISC-V: Include hexidecimal instruction in

[Philippe Mathieu-Daudé]

On 03/06/2018 05:43 PM, Michael Clark wrote:
> 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.

clean :)

> 
> 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);
>  
> 

Re: [Qemu-devel] [PATCH v1 18/22] RISC-V: Remove braces from satp case statement with

[Philippe Mathieu-Daudé]

On 03/06/2018 05:43 PM, Michael Clark wrote:
> 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;
> 

Re: [Qemu-devel] [PATCH v1 00/22] Spec conformance bug fixes and cleanups

[Emilio G. Cota]

On Wed, Mar 07, 2018 at 12:07:18 +1300, Michael Clark wrote:
> BTW Apologies for the duplicate emails. I'm still getting to grips with the
> git-sendemail workflow and was using a sed script to Add Cc's which munged
> the headers as it didn't take into account Subject lines flowing to two
> lines. I guess I can just include Cc: in the commit message? and
> git-format-patch will handle it for me? or I just should how to use
> git-publish...

I don't have experience with git-publish. The following two suggestions
might help though:

- Yes, add Cc's to individual patches -- those are picked up by send-email.
  That also applies to the cover letter, although note that if
  you use --compose then Cc's won't be picked up. Instead, just write
  the cover letter into a 0000-$cover.patch (with Cc's in there)
  and do not use --compose.

- Use send-email's --dry-run option to make sure everything looks good
  without actually sending any emails.

> I still have to work on Igor's requested change to CPU initializer
> declarations, and i'd also like to get the patch in that fixes fmin/fmax
> test failures using the riscv-tests testsuite by implementing IEEE-754
> minimumNumber/maximumNumber. I'll try to get a patch onto the list before
> soft-freeze. I dropped the patch which was in an earlier version of our
> port patch series due to a conflict with changes in softfloat (conversion
> of minmax from a macro to a static function iirc).

Note that if you're fixing a bug (and the fmin/fmax patch qualifies),
you can definitely get it merged post-soft-freeze.

		E.

Re: [Qemu-devel] [PATCH v1 00/22] Spec conformance bug fixes and cleanups

[Michael Clark]

On Wed, Mar 7, 2018 at 12:47 PM, Emilio G. Cota <cota@braap.org> wrote:

> On Wed, Mar 07, 2018 at 12:07:18 +1300, Michael Clark wrote:
> > BTW Apologies for the duplicate emails. I'm still getting to grips with
> the
> > git-sendemail workflow and was using a sed script to Add Cc's which
> munged
> > the headers as it didn't take into account Subject lines flowing to two
> > lines. I guess I can just include Cc: in the commit message? and
> > git-format-patch will handle it for me? or I just should how to use
> > git-publish...
>
> I don't have experience with git-publish. The following two suggestions
> might help though:
>
> - Yes, add Cc's to individual patches -- those are picked up by send-email.
>   That also applies to the cover letter, although note that if
>   you use --compose then Cc's won't be picked up. Instead, just write
>   the cover letter into a 0000-$cover.patch (with Cc's in there)
>   and do not use --compose.
>
> - Use send-email's --dry-run option to make sure everything looks good
>   without actually sending any emails.


I'll use dry run next time and reword the patches to add 'Cc. The previous
patch series all had subject lines that didn't flow to the next line and I
had script automation to carve up the port. I'm trying to get into a new
habit of making the first line of the patch (Subject) as short as
reasonably possible and instead putting a longer description in the body.
Short subjects and short first lines of commits are better.


> > I still have to work on Igor's requested change to CPU initializer
> > declarations, and i'd also like to get the patch in that fixes fmin/fmax
> > test failures using the riscv-tests testsuite by implementing IEEE-754
> > minimumNumber/maximumNumber. I'll try to get a patch onto the list before
> > soft-freeze. I dropped the patch which was in an earlier version of our
> > port patch series due to a conflict with changes in softfloat (conversion
> > of minmax from a macro to a static function iirc).
>
> Note that if you're fixing a bug (and the fmin/fmax patch qualifies),
> you can definitely get it merged post-soft-freeze.


 Okay. good.

Re: [Qemu-devel] [PATCH v1 07/22] RISC-V: Remove unused class definitions from

[Michael Clark]

On Wed, Mar 7, 2018 at 12:27 PM, Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> Hi Michael,
>
> On 03/06/2018 05:43 PM, Michael Clark wrote:
> > 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.
> >
> > Signed-off-by: Michael Clark <mjc@sifive.com>
> > Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
> > ---
> >  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 |  9 ---------
> >  include/hw/riscv/sifive_u.h |  9 ---------
> >  include/hw/riscv/virt.h     |  9 ---------
> >  7 files changed, 122 deletions(-)
> >
> > 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);
>
> Ok until here.
>
> > diff --git a/include/hw/riscv/sifive_e.h b/include/hw/riscv/sifive_e.h
> > index 0aebc57..818fbdc 100644
> > --- a/include/hw/riscv/sifive_e.h
> > +++ b/include/hw/riscv/sifive_e.h
> > @@ -19,16 +19,7 @@
> >  #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;
>
> I'd keep however a 'Object parent_obj' here, to stay QOM; but your patch
> is valid.
>

Okay I'll keep parent_obj when I respin.

Thanks,
Michael.

> -
> > -    /*< public >*/
> >      RISCVHartArrayState soc;
> >      DeviceState *plic;
> >  } SiFiveEState;
> > diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
> > index be38aa0..8ebd545 100644
> > --- a/include/hw/riscv/sifive_u.h
> > +++ b/include/hw/riscv/sifive_u.h
> > @@ -19,16 +19,7 @@
> >  #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;
> > -
> > -    /*< public >*/
> >      RISCVHartArrayState soc;
> >      DeviceState *plic;
> >      void *fdt;
> > diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> > index 655e85d..9588909 100644
> > --- a/include/hw/riscv/virt.h
> > +++ b/include/hw/riscv/virt.h
> > @@ -19,15 +19,7 @@
> >  #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;
> > -
> > -    /*< public >*/
> >      RISCVHartArrayState soc;
> >      DeviceState *plic;
> >      void *fdt;
> > @@ -45,7 +37,6 @@ enum {
> >      VIRT_DRAM
> >  };
> >
> > -
> >  enum {
> >      UART0_IRQ = 10,
> >      VIRTIO_IRQ = 1, /* 1 to 8 */
> >
>

Re: [Qemu-devel] [PATCH v1 09/22] RISC-V: Include hexidecimal instruction in

[Michael Clark]

On Wed, Mar 7, 2018 at 12:09 PM, Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> On 03/06/2018 05:43 PM, Michael Clark wrote:
> > 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.
>
> clean :)


Yap. One feature crept in. I was trying to debug early firmware (to
separate firmware and kernel image) and this helped me discover that I was
jumping into device tree. 0xfeedd00d is the device-tree magic and it is
actually valid RVC code so I was quite puzzled until I modifed the
disassembler to show the instruction bytes.

> 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);
> >
> >
>

Re: [Qemu-devel] [PATCH v1 07/22] RISC-V: Remove unused class definitions from

[Michael Clark]

On Wed, Mar 7, 2018 at 5:14 PM, Michael Clark <mjc@sifive.com> wrote:

>
>
> On Wed, Mar 7, 2018 at 12:27 PM, Philippe Mathieu-Daudé <f4bug@amsat.org>
> wrote:
>
>>
>> Ok until here.
>>
>> > diff --git a/include/hw/riscv/sifive_e.h b/include/hw/riscv/sifive_e.h
>> > index 0aebc57..818fbdc 100644
>> > --- a/include/hw/riscv/sifive_e.h
>> > +++ b/include/hw/riscv/sifive_e.h
>> > @@ -19,16 +19,7 @@
>> >  #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;
>>
>> I'd keep however a 'Object parent_obj' here, to stay QOM; but your patch
>> is valid.
>>
>
> Okay I'll keep parent_obj when I respin.
>

BTW is the string constant in DEFINE_MACHINE a QOM type?

i.e. should I keep the type conversion and type name macros and use the
type name macro in DEFINE_MACHINE?

Re: [Qemu-devel] [PATCH v1 00/22] Spec conformance bug fixes and cleanups

[Emilio G. Cota]

On Wed, Mar 07, 2018 at 13:00:35 +1300, Michael Clark wrote:
> I'll use dry run next time and reword the patches to add 'Cc.

Just to be clear, it's totally OK to add the Cc lines to the commit
message -- much easier than editing the .patch files, especially
for long patch series that go through possibly many iterations.

		E.

Re: [Qemu-devel] [PATCH v1 11/22] RISC-V: Improve page table walker spec compliance

[Michael Clark]

On Wed, Mar 7, 2018 at 9:43 AM, Michael Clark <mjc@sifive.com> wrote:

> - Inline PTE_TABLE check for better readability
> - Improve readibility of User page U mode and SUM test
> - Disallow non U mode from fetching from User pages
> - Add reserved PTE flag check: W or W|X
> - Add misaligned PPN check
> - Change access checks from ternary operator to if statements
> - Improves page walker comments
> - No measurable performance impact on dd test
>
> 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   | 57 ++++++++++++++++++++++++++++++
> ++++---------------
>  2 files changed, 40 insertions(+), 19 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 228933c..2165ecb 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 mstats.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))) {
>

This should only honor the mstatus.MXR flags if mode != PRV_U

+            /* 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;
>                  rcu_read_lock();
> @@ -243,15 +266,15 @@ restart:
>              target_ulong vpn = addr >> PGSHIFT;
>              *physical = (ppn | (vpn & ((1L << ptshift) - 1))) << PGSHIFT;
>
> +            /* set permissions on the TLB entry */
>              if ((pte & PTE_R)) {
>                  *prot |= PAGE_READ;
>              }
>

There is a logic bug here, but it is pre-existing. If mxr and mode is not
U, the X flag needs to make the page readable i.e. (MXR) Make eXecute
Readable.


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