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

Add gdb xml register support. #160

Closed
wants to merge 44 commits into from

Conversation

jim-wilson
Copy link
Contributor

This allows gdb to read misa and set breakpoints. This is a work in progress,
and has a number of obvious problems, incomplete csr support, wrong int reg
size for rv32, etc.

Michael Clark and others added 30 commits July 25, 2018 13:41
Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Alistair Francis <[email protected]>
Signed-off-by: Palmer Dabbelt <[email protected]>
- Inline PTE_TABLE check for better readability
- Change access checks from ternary operator to if
- Improve readibility of User page U mode and SUM test
- Disallow non U mode from fetching from User pages
- Add reserved PTE flag check: W or W|X
- Add misaligned PPN check
- Set READ protection for PTE X flag and mstatus.mxr
- Use memory_region_is_ram in pte update

Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Alistair Francis <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
Reviewed-by: Alistair Francis <[email protected]>
The PLIC previously used a mutex to protect against concurrent
access to the claimed and pending bitfields. Instead of using
a mutex, we update the bitfields using atomic_cmpxchg.

Rename sifive_plic_num_irqs_pending to sifive_plic_irqs_pending
and add an early out if any interrupts are pending as the
count of pending interrupts is not used.

Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Alistair Francis <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
Reviewed-by: Richard Henderson <[email protected]>
This commit is intended to improve readability.
There is no change to the logic.

Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Alistair Francis <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
Reviewed-by: Alistair Francis <[email protected]>
Change the API of riscv_set_local_interrupt to take a
write mask and value to allow setting and clearing of
multiple local interrupts atomically in a single call.
Rename the new function to riscv_cpu_update_mip.

Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Alistair Francis <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
This patch makes op_helper.c contain only instruction
operation helpers used by translate.c and moves any
unrelated cpu helpers into cpu_helper.c. No logic is
changed by this patch.

Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Alistair Francis <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
Reviewed-by: Alistair Francis <[email protected]>
* Add user-mode CSR defininitions.
* Reorder CSR definitions to match the specification.
* Change H mode interrupt comment to 'reserved'.
* Remove unused X_COP interrupt.
* Add user-mode interrupts.
* Remove erroneous until comments on machine mode interrupts.
* Move together paging mode and page table bit definitions.
* Move together interrupt and exception cause definitions.

Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Alistair Francis <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
Reviewed-by: Alistair Francis <[email protected]>
Previous CSR code uses csr_read_helper and csr_write_helper
to update CSR registers however this interface prevents
atomic read/modify/write CSR operations; in addition
there is no trap-free method to access to CSRs due
to the monolithic CSR functions call longjmp.

The current iCSR interface is not safe to be called by
target/riscv/gdbstub.c as privilege checks or missing CSRs
may call longjmp to generate exceptions. It needs to
indicate existence so traps can be generated in the
CSR instruction helpers.

This commit moves CSR access from the monolithic switch
statements in target/riscv/op_helper.c into modular
read/write functions in target/riscv/csr.c using a new
function pointer table for dispatch (which can later
be used to allow CPUs to hook up model specific CSRs).

A read/modify/write interface is added to support atomic
CSR operations and a non-trapping interface is added
to allow exception-free access to CSRs by the debugger.

The CSR functions and CSR dispatch table are ordered
to match The RISC-V Instruction Set Manual, Volume II:
Privileged Architecture Version 1.10, 2.2 CSR Listing.

Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Alistair Francis <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
Use the new CSR read/modify/write interface to implement
atomic updates to mip/sip.

Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Alistair Francis <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
CSR predicate functions are added to the CSR table.
mstatus.FS and counter enable checks are moved
to predicate functions and two new predicates are
added to check misa.S for s* CSRs and a new PMP
CPU feature for pmp* CSRs.

Processors that don't implement S-mode will trap
on access to s* CSRs and processors that don't
implement PMP will trap on accesses to pmp* CSRs.

PMP checks are disabled in riscv_cpu_handle_mmu_fault
when the PMP CPU feature is not present.

Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Alistair Francis <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Alistair Francis <[email protected]>
Cc: Richard Henderson <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
Reviewed-by: Michael Clark <[email protected]>
Modifed from Richard Henderson's patch [1] to integrate
with the new control and status register implementation.

[1] https://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg07034.html

Note: the f* CSRs already mark mstatus.FS dirty using
env->mstatus |= mstatus.FS so the bug in the first
spin of this patch has been fixed in a prior commit.

Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Alistair Francis <[email protected]>
Cc: Richard Henderson <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
Reviewed-by: Michael Clark <[email protected]>

Co-authored-by: Richard Henderson <[email protected]>
Co-authored-by: Michael Clark <[email protected]>
This adds the necessary minimum to support S-mode
virtualization for priv ISA >= v1.10

Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Alistair Francis <[email protected]>
Cc: Matthew Suozzo <[email protected]>
Signed-off-by: Michael Clark <[email protected]>

Co-authored-by: Matthew Suozzo <[email protected]>
Co-authored-by: Michael Clark <[email protected]>
This allows hardware and/or derived cpu instances
to override or implement new CSR operations.

Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Alistair Francis <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
Add carriage return that was erroneously removed
when converting to qemu_log. Change hard coded
core number to the actual hartid.

Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Alistair Francis <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
* Add riscv prefix to raise_exception function
* Add riscv prefix to CSR read/write functions
* Add riscv prefix to signal handler function
* Add riscv prefix to get fflags function
* Remove redundant declaration of riscv_cpu_init
  and rename cpu_riscv_init to riscv_cpu_init
* rename riscv_set_mode to riscv_cpu_set_mode

Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Alistair Francis <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
The mode variable only uses the lower 4-bits (M,H,S,U) so
replace the GCC specific __builtin_popcount with ctpop8.

Cc: Palmer Dabbelt <[email protected]>
Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Alistair Francis <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Alistair Francis <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
We can't allow the supervisor to control SEIP as this would allow the
supervisor to clear a pending external interrupt which will result in
lost a interrupt in the case a PLIC is attached. The SEIP bit must be
hardware controlled when a PLIC is attached.

This logic was previously hard-coded so SEIP was always masked even
if no PLIC was attached. This patch adds riscv_cpu_claim_interrupts
so that the PLIC can register control of SEIP. In the case of models
without a PLIC (spike), the SEIP bit remains software controlled.

This interface allows for hardware control of supervisor timer and
software interrupts by other interrupt controller models.

Cc: Palmer Dabbelt <[email protected]>
Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Alistair Francis <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
gen methods should access state from DisasContext. Add misa
field to the DisasContext struct and remove CPURISCVState
argument from all gen methods.

Cc: Palmer Dabbelt <[email protected]>
Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Alistair Francis <[email protected]>
Cc: Emilio G. Cota <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
Reviewed-by: Richard Henderson <[email protected]>
Add misa checks for M, A, F and D extensions and if they are
not present generate illegal instructions. This improves
emulation accurary for harts with a limited set of extensions.

Cc: Palmer Dabbelt <[email protected]>
Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Alistair Francis <[email protected]>
Cc: Emilio G. Cota <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
This patch adds support for writing misa. misa is validated based
on rules in the ISA specification. 'E' is mutually exclusive with
all other extensions. 'D' depends on 'F' so 'D' bit is dropped
if 'F' is not present. A conservative approach to consistency is
taken by flushing the translation cache on misa writes. misa_mask
is added to the CPU struct to store the original set of extensions.

Cc: Palmer Dabbelt <[email protected]>
Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Alistair Francis <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
A missing shift made updates to the low order bits
of timecmp erroneously copy the old low order bits
into the high order bits of the 64-bit timecmp
register. Add the missing shift and rename timecmp
local variables to timecmp_hi and timecmp_lo.

This bug didn't show up as the low order bits are
usually written first followed by the high order
bits meaning the high order bits contained an invalid
value between the timecmp_lo and timecmp_hi update.

Cc: Palmer Dabbelt <[email protected]>
Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Alistair Francis <[email protected]>
Co-Authored-by: Johannes Haring <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
The address calculation for the pending bitfield had
a copy paste bug. This bug went unnoticed because the Linux
PLIC driver does not read the pending bitfield, rather it
reads pending interrupt numbers from the claim register
and writes acknowledgements back to the claim register.

Cc: Palmer Dabbelt <[email protected]>
Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Alistair Francis <[email protected]>
Reported-by: Vincent Siles <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
Previously the second UARTs on the sifive_e and sifive_u machines
where disabled due to check-qtest-riscv32 and check-qtest-riscv64
failures. Recent changes in the QEMU core serial code have
resolved these failures so the second UARTs can be instantiated.

Cc: Palmer Dabbelt <[email protected]>
Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Alistair Francis <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
Remove machine generated constraints that are not
referenced by the pseudo-instruction constraints.

Cc: Palmer Dabbelt <[email protected]>
Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Alistair Francis <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
Refer to the RISC-V PSABI specification for details:

- https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md

Cc: Michael Tokarev <[email protected]>
Cc: Richard Henderson <[email protected]>
Cc: Alistair Francis <[email protected]>
Reviewed-by: Laurent Vivier <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
This change checks elf_flags for EF_RISCV_RVE and if
present uses the RVE linux syscall ABI which uses t0
for the syscall number instead of a7.

Warn and exit if a non-RVE ABI binary is run on a
cpu with the RVE extension as it is incompatible.

Cc: Palmer Dabbelt <[email protected]>
Cc: Sagar Karandikar <[email protected]>
Cc: Bastian Koppelmann <[email protected]>
Cc: Alistair Francis <[email protected]>
Co-authored-by: Kito Cheng <[email protected]>
Co-authored-by: Michael Clark <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Alistair Francis <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
Support for separate firmware and kernel payload is added
by updating BBL to read optional preloaded kernel address
attributes from device-tree using a similar mechanism to
that used to pass init ramdisk addresses to linux kernel.

    chosen {
        riscv,kernel-start = <0x00000000 0x80200000>;
        riscv,kernel-end = <0x00000000 0x80590634>;
    };

These attributes are added by QEMU and read by BBL when combining
-bios <firmware-image> and -kernel <kernel-image> options. e.g.

$ qemu-system-riscv64 -machine virt -bios bbl -kernel vmlinux

With this change, bbl can be compiled without --with-payload
and the dummy payload alignment is altered to make the memory
footprint of the firmware-only bbl smaller. The dummy payload
message is updated to indicate the alternative load method.

This load method could also be supported by a first stage boot
loader that reads seperate firmware and kernel from SPI flash.
The main advantage of this new mechanism is that it eases kernel
development by avoiding the riscv-pk packaging step after kernel
builds, makes building per repository artefacts for CI simpler,
and mimics bootloaders on other platforms that can load a kernel
image file directly. Ultimately BBL should use an SPI driver to
load the kernel image however this mechanism supports use cases
such such as QEMU's -bios, -kernel and -initrd options following
examples from other platforms that pass kernel entry to firmware
via device-tree.

The board is also changed to use the firmware address from the
loaded firmware or combined firmware+kernel. This is normally
equal to the DRAM base address of 0x8000_0000, however now it
is possible to boot firmware at different load addresses because
the reset code jumps to the actual firmware entry address.

Cc: Palmer Dabbelt <[email protected]>
Cc: Alistair Francis <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
@jim-wilson
Copy link
Contributor Author

This is a proposed solution for issue #156.

@@ -30,7 +30,7 @@ static riscv_csr_operations csr_ops[];
/* CSR function table constants */

enum {
CSR_TABLE_SIZE = 0xfff
CSR_TABLE_SIZE = 0x1000
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good spotting. I vaguely remember this one. CSR 0xfff is out of bounds. We should roll this fragment into 0bf5716 RISC-V: Implement modular CSR helper interface When I merge the commit I can rebase and move this fragment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With an earlier version of the patch, I got a core dump when gdb tried to read CSR4095, because of the out-of-bounds array access.


void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
{
/* ??? Assume all targets have FPU regs for now. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can get look at misa to see if the CPU has F or D. If D is present, F is present, so it suffices to look for F as write_misa checks these constraints. i.e.

    RISCVCPU *cpu = RISCV_CPU(cs);
    CPURISCVState *env = &cpu->env;
    if (riscv_has_ext(env, RVF)) {
        gdb_register_coprocessor(cs, riscv_gdb_get_fpu, riscv_gdb_set_fpu,
                                 32, "riscv-fpu.xml", 0);
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Register numbers have to match gdb, and gdb always makes the FP regs 33 to 64. I think that we will always have to register the FP regs for now, even if they aren't enabled. I will check this when I have a chance. Ideally we only register FP regs if enabled, but I suspect this will require a gdb patch, which in turn will also require a OpenOCD patch. So if we change this, we will need coordinated qemu/gdb/OpenOCD patches which will be inconvenient. It appears that this is how the ARM port works, but then they have 4 different kinds of FP registers to support, so they probably don't have a choice.

@lbmeng
Copy link
Contributor

lbmeng commented Sep 11, 2018

This patch solves my problem with #101. Thanks!

@jim-wilson
Copy link
Contributor Author

One odd thing about the patch is that misa is listed as 0x342 when it is actually 0x301. This is because in gdb, CSRs start at 65, and 0x301+65 is 0x342. This is a little confusing. We can avoid the problem if we put all 4096 registers in the riscv-csr.xml file, in which case we don't need to specify register numbers, but then gdb will actually try to read all 4096 of them, so that may not be the right solution. I want to check the gdb XML syntax, to see if maybe there is a better way to handle this.

@michaeljclark michaeljclark force-pushed the riscv-all branch 2 times, most recently from b00c9d1 to 6e53247 Compare September 17, 2018 00:50
@michaeljclark
Copy link
Collaborator

Can we just put the present subset of the CSRs in the riscv-csr.xml file. We have that information in the csr_ops array in target/riscv/csr.c. Sorry I am not sure how GDB XML works so this might be a dumb idea.

BTW I have backported the CSR_TABLE_SIZE fix to the patch that adds the feature as it is not in upstream QEMU yet. It made sense to collapse the change into the CSR table feature patch in the qemu-for-upstream branch.

Sorry for the rebasing here, but it is a result of us sharing our in-progress patches... the workflow requires us to rebase our pending branch. If you have a patch, then you can just remove the CSR.

The riscv-qemu-3.0 branch is stable and we can backport to it. The riscv-all branch currently just has a merge commit of the qemu-for-upstream branch which gets rebased against upstream.

@jim-wilson
Copy link
Contributor Author

The rebasing is fine. I can deal with that.

The more CSRs we put into the XML file, the more CSRs gdb will read when connecting to the target. I'm not sure if that is OK; I haven't looked at that yet. I just started with misa as gdb requires that one in order to work properly. I haven't had a chance to expand the list yet. Presumably qemu is OK with reading lots of CSRs and will do something reasonable if we try to read a CSR that doesn't exist on the target. On the gdb side the register displays may get a bit messy, e.g. the user types "info all-registers" and then gdb will display hundreds or maybe thousands of lines of info because we have lots of CSRs. But these are manageable problems, and ideally we should put as many CSRs in the list as reasonable, as gdb can only read/write registers mentioned in the XML files, and there may be people that want to see all of the CSR register values from gdb.

@jim-wilson
Copy link
Contributor Author

The gdb support is in upstream qemu now.

@jim-wilson jim-wilson closed this Mar 19, 2019
@jim-wilson jim-wilson deleted the gdb-xml-regs branch March 19, 2019 18:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants