Skip to content
This repository has been archived by the owner on Apr 13, 2019. It is now read-only.

RISC-V Privileged ISA spec conformance fixes and cleanup #115

Merged
merged 23 commits into from
Mar 9, 2018
Merged

RISC-V Privileged ISA spec conformance fixes and cleanup #115

merged 23 commits into from
Mar 9, 2018

Conversation

michaeljclark
Copy link
Collaborator

@michaeljclark michaeljclark commented Mar 6, 2018 •
edited
Loading

This is a 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
  • Updates the cpu model to use a more modern interface

@michaeljclark michaeljclark self-assigned this Mar 6, 2018
@michaeljclark michaeljclark mentioned this pull request Mar 6, 2018
Closed
@michaeljclark michaeljclark changed the base branch from qemu-upstream-v8.1 to qemu-upstream-v8.2 March 6, 2018 19:36
@michaeljclark michaeljclark force-pushed the qemu-devel branch 3 times, most recently from 42ec46a to 477e766 Compare March 7, 2018 01:37
@michaeljclark michaeljclark changed the title RISC-V - Privileged ISA specification conformance fixes and code cleanup RISC-V - Privileged ISA spec conformance fixes and cleanup Mar 7, 2018
@michaeljclark michaeljclark changed the title RISC-V - Privileged ISA spec conformance fixes and cleanup RISC-V Privileged ISA spec conformance fixes and cleanup Mar 7, 2018
palmer-dabbelt
palmer-dabbelt approved these changes Mar 7, 2018
View reviewed changes
hw/riscv/sifive_u.c
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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, there are two UARTs on the U540-C000

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second UART is disabled until we can figure out how to plug them both in without getting "make check" failures. That's why i've left the code commented. I should probably add a further comment. The serial frontend/backend has changed a lot since we rebased to upstream QEMU so it may be possible to fix the issue. We also have to figure out how to plug hardware from a config file or the command line given 'sifive_u' and 'sifive_e' are going to be somewhat generic.

hw/riscv/spike.c
@@ -115,7 +110,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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, odd -- I guess I expected Spike not to emulate a CLINT as that's kind of a SiFive thing, but it's what real spike does so it's correct to do it here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yap. The CLINT models exactly the same behaviour as the old riscv_rtc.c so we eliminated the duplicate code when we added the CLINT. It's spike compatible 👍

@michaeljclark michaeljclark force-pushed the qemu-devel branch 3 times, most recently from 47aef06 to f611e65 Compare March 9, 2018 03:42
Michael Clark added 16 commits March 10, 2018 02:59
RISC-V: Make virt create_fdt interface consistent
5cfe29a 
create_fdt sets the fdt variable on RISCVVirtState and this is
used to access the fdt. This reverts a change introduced in
riscvarchive#109 which introduced
a redundant return value, overlooking the RISCVVirtState
structure member that made create_fdt inconsistent with the
other RISC-V machines. The other alternative is to change
the other boards to return the fdt. Note: the RISCVVirtState
also contains fdt_size.

Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
RISC-V: Replace hardcoded constants with enum values
60274fb 
The RISC-V device-tree code has a number of hard-coded
constants and this change moves them into header enums.

Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
RISC-V: Make virt board description match spike
0284ca0 
This makes 'qemu-system-riscv64 -machine help' output more tidy
and consistent.

Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
RISC-V: Use ROM base address and size from memmap
8736852 
Another case of replaceing hard coded constants, this time
referring to the definition in the virt machine's memmap.

Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
RISC-V: Remove identity_translate from load_elf
f456c03 
When load_elf is called with NULL as an argument to the
address translate callback, it does an identity translation.
This commit removes the redundant identity_translate callback.

Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
RISC-V: Mark ROM read-only after copying in code
f514dfa 
The sifive_u machine already marks its ROM readonly. This fixes
the remaining boards.

Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
RISC-V: Remove unused class definitions
451bc44 
Removes a whole lot of unnecessary boilerplate code. Machines
don't need to be objects. The expansion of the SOC object model
for the RISC-V machines will happen in the future as SiFive
plans to add their FE310 and FU540 SOCs to QEMU. However, it
seems that this present boilerplate is complete unnecessary.

Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
RISC-V: Make sure rom has space for fdt
e17d9f8 
Remove a potential buffer overflow (not seen in practice).
Perhaps cpu_physical_memory_write already has bound checks.
This change however makes space for the maximum device tree
size and adds an explicit bounds check and error message.
It doesn't trigger, but it may help in the future if the
device-tree size is exceeded. e.g. large bootargs.

Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
RISC-V: Include intruction hex in disassembly
92f5664 
This was added to help debug issues using -d in_asm. It is
useful to see the instruction bytes, as one can detect if
one is trying to execute ASCII or device-tree magic.

Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
RISC-V: Hold rcu_read_lock when accessing memory
eef9a47 
From reading other code that accesses memory regions directly,
it appears that the rcu_read_lock needs to be held. Note: the
original code for accessing RAM directly was added because
there is no other way to use atomic_cmpxchg on guest physical
address space.

Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
RISC-V: Improve page table walker spec compliance
e137da9 
- 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

Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
RISC-V: Update E order and I extension order
f17c97a 
Section 22.8 Subset Naming Convention of the RISC-V ISA Specification
defines the canonical order for extensions in the ISA string. It is
silent on the position of the E extension however E is a substitute
for I so it must come early in the extension list order. A comment
is added to state E and I are mutually exclusive, as the E extension
will be added to the RISC-V port in the future.

Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
RISC-V: Make some header guards more specific
b2593c5 
Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
RISC-V: Make virt header comment title consistent
5201799 
Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
RISC-V: Use memory_region_is_ram in pte update
fdb8b87 
After reading cpu_physical_memory_write and friends, it seems
that memory_region_is_ram is a more appropriate interface,
and matches the intent of the code that is calling it.

Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
RISC-V: Remove EM_RISCV ELF_MACHINE indirection
18fa302 
Pointless indirection. Other ports use EM_ constants directly.

Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
Michael Clark added 7 commits March 10, 2018 03:01
RISC-V: Hardwire satp to 0 for no-mmu case
e805df2 
satp is WARL so it should not trap on illegal writes, rather
it can be hardwired to zero and silently ignore illegal writes.

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

Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
RISC-V: Remove braces from satp case statement
fd744c2 
Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
RISC-V: riscv-qemu port supports sv39 and sv48
1e295e6 
Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
RISC-V: vectored traps are optional
ae1a88d 
Vectored traps for asynchrounous interrupts are optional.
The mtvec/stvec mode field is WARL and hence does not trap
if an illegal value is written. Illegal values are ignored.

Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
RISC-V: No traps on writes to misa,minstret,mcycle
9fd0020 
These fields are marked WARL in the specification so illegal
writes are silently dropped.

Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
RISC-V: Remove support for adhoc X_COP interrupt
c96d0c2 
This is essentially dead-code elimination. Support for more
local interrupts will be added in a future revision, as they
will be defined in a future version of the Privileged ISA
specification.

Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
RISC-V: Convert cpu definition towards future model
b31d4af 
- Model borrowed from target/sh4/cpu.c
- Rewrote riscv_cpu_list to use object_class_get_list
- Dropped 'struct RISCVCPUInfo' and used TypeInfo array
- Replaced riscv_cpu_register_types with DEFINE_TYPES
- Marked base class as abstract

Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Palmer Dabbelt <palmer@sifive.com>
Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by Michael Clark <mjc@sifive.com>
@michaeljclark michaeljclark changed the base branch from qemu-upstream-v8.2 to riscv-all March 9, 2018 14:20
@michaeljclark
Copy link
Collaborator Author

Merging into riscv-all

@michaeljclark michaeljclark merged commit 3fe8f03 into riscvarchive:riscv-all Mar 9, 2018
@NonerKao
Copy link

NonerKao commented Mar 26, 2018 •
edited
Loading

Sorry for bringing this up again, but it seems that in target/riscv/op_helper.c's csr_read_helper function, the logic around ctr_en is still out-of-date: it depends on m[s|u]counteren registers.
The branch is the riscv-all after #123, so it should be fairly new. I'll keep a side note to this thread in my upcoming perf patch.

palmer-dabbelt pushed a commit to riscvarchive/riscv-linux that referenced this pull request May 14, 2018
@NonerKao @palmer-dabbelt
perf: riscv: preliminary RISC-V support
d969510 
This patch provide a basic PMU, riscv_base_pmu, which supports two
general hardware event, instructions and cycles.  Furthermore, this
PMU serves as a reference implementation to ease the portings in
the future.

riscv_base_pmu should be able to run on any RISC-V machine that
conforms to the Priv-Spec.  Note that the latest qemu model hasn't
fully support a proper behavior of Priv-Spec 1.10 yet, but work
around should be easy with very small fixes.  Please check
riscvarchive/riscv-qemu#115 for future updates.

Cc: Nick Hu <nickhu@andestech.com>
Cc: Greentime Hu <greentime@andestech.com>
Signed-off-by: Alan Kao <alankao@andestech.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
palmer-dabbelt pushed a commit to riscvarchive/riscv-linux that referenced this pull request Jun 11, 2018
@NonerKao @palmer-dabbelt
perf: riscv: preliminary RISC-V support
178e9fc 
This patch provide a basic PMU, riscv_base_pmu, which supports two
general hardware event, instructions and cycles.  Furthermore, this
PMU serves as a reference implementation to ease the portings in
the future.

riscv_base_pmu should be able to run on any RISC-V machine that
conforms to the Priv-Spec.  Note that the latest qemu model hasn't
fully support a proper behavior of Priv-Spec 1.10 yet, but work
around should be easy with very small fixes.  Please check
riscvarchive/riscv-qemu#115 for future updates.

Cc: Nick Hu <nickhu@andestech.com>
Cc: Greentime Hu <greentime@andestech.com>
Signed-off-by: Alan Kao <alankao@andestech.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@palmer-dabbelt palmer-dabbelt palmer-dabbelt approved these changes

Assignees

@michaeljclark michaeljclark

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@michaeljclark @NonerKao @palmer-dabbelt