riscv / riscv-debug-spec

Working Draft of the RISC-V Debug Specification Standard
https://jira.riscv.org/browse/RVG-94
Other
454 stars 92 forks source link

abstract register command and aarsize restrictions #929

Open algrobman opened 9 months ago

algrobman commented 9 months ago

Why is it required to fail register abstract command with size higher than the register size?

For ex, DCSR is defined as 32 bit register even for 64 bit CPU. All CSRs are accessed by the csr* instructions, transferring data value between GPR and CSR regardless of CSR size, including DCSR in normal (not debug) CPU modes.

Why does aarsize even matter for register abstract commands, especially if misa.MXL is fixed?

I understand that debuggers may save some time if only low 32 bits of a register are needed, skipping data1 register accesses, but operations on part of GPRs or CSRs are not defined for RISCV architecture, as far as I know ..

timsifive commented 9 months ago

Why is it required to fail register abstract command with size higher than the register size?

So that a debugger can discover XLEN by attempting a 64-bit access and seeing whether it succeeds or fails.

I understand that debuggers may save some time if only low 32 bits of a register are needed, skipping data1 register accesses, but operations on part of GPRs or CSRs are not defined for RISCV architecture, as far as I know ..

The debug spec requires support for reading <XLEN bits from a register, but does not require it for writing. I think in that case the behavior is obvious. I don't remember why we made that decision, but maybe the github history is enlightening.

algrobman commented 9 months ago

Tim,

So that a debugger can discover XLEN by attempting a 64-bit access and seeing whether it succeeds or fails.

Do we still really need to implement specific to DCSR logic to fail 32 bit DCSR read access with aarsize=3 for 64 bit machine? Also why is DCSR defined as 32 bit CSR even for 64 bit machine? Above requirement seems defeats the the purpose, you mentioned, if somebody tries 64 bit read of DCSR and we implement the "access size check " logic.

If the CPU has variable/programmable XLEN, should misa.MXL field modulate this aarsize check in DM?

BTW, as far as I know openOCD reads misa to detect XLEN.

timsifive commented 9 months ago

Do we still really need to implement specific to DCSR logic to fail 32 bit DCSR read access with aarsize=3 for 64 bit machine?

That is the intent of the spec.

Also why is DCSR defined as 32 bit CSR even for 64 bit machine?

Probably to keep the specification simpler.

Above requirement seems defeats the the purpose, you mentioned, if somebody tries 64 bit read of DCSR and we implement the "access size check " logic.

Yes, for DCSR it doesn't help the debugger. They should choose a different a different register to determine XLEN. OpenOCD uses s0: https://github.com/riscv/riscv-openocd/blob/feff8e005482ce3a070131e62a14b3bc0b9fbdf9/src/target/riscv/riscv-013.c#L2045

If the CPU has variable/programmable XLEN, should misa.MXL field modulate this aarsize check in DM?

That's a good question. I think the answer should be "yes" because users need registers to behave according to the current mode. Making the debugger know how to convert every 64-bit CSR to 32-bit seems complex, especially since the hardware has already implemented it all already.

algrobman commented 9 months ago

Also why is DCSR defined as 32 bit CSR even for 64 bit machine?

Probably to keep the specification simpler.

How does it make it simpler? it makes it inconsistent as all CSRs are 64 bit in 64bit implementations. (and DCSR is accessible by the processor with CSR instructions, where the CSR size does not make any sense )...

You could provide more flexibility, consistency and keep the same access speed by debugger with 32 bit reads and writes , but saying that it is XLEN CSR as any other CSRs with reserved bits ...

pdonahue-ventana commented 9 months ago

all CSRs are 64 bit in 64bit implementations

Not exactly. fcsr is defined to be 32b regardless of XLEN. There might be others.

algrobman commented 9 months ago

this was my point that from CPU pipe, data transfer point of view other size than XLEN for CSRs does not make sense. (ex MIP/MIE have most of the upper bits reserved, but have XLEN width ( ? ) why are DCSR and FCSR different ? What difference does it make would these CSRs be formally defined with XLEN width vs 32 bits? How can a user see difference?

algrobman commented 9 months ago

BTW, can somebody enlighten me what is the size for fflags and frm CSRs? So far I've found following CSRs defined as 32 bits: mvendorid mcounteren scounteren mcountinhibit fcsr dcsr

timsifive commented 9 months ago

I don't have a good answer for you, but this is where things ended up. It's not something we should change at this stage, unless it's a bug.

I can't speak to other RISC-V specs. OpenOCD treats fflags and frm as XLEN bit wide.

pdonahue-ventana commented 9 months ago

As for dcsr specifically, the git history shows that it was defined that way from the very first commit. I don't know why.

This topic came up previously in https://lists.riscv.org/g/tech-debug/topic/84876701 but there's not much of a conclusion. I think that's because relatively few implementations allow abstract access to CSRs so the rest of us forgot about it when the thread died.

algrobman commented 9 months ago

Tim, I would like you to consider to change wording of arrsize constraint in next spec release from:

If aarsize specifies a size larger than the register's actual size, then the access must fail.

to:

If aarsize specifies a size larger than current XLEN for GPRs and SPRs access or FLEN for FPRs access , then the access must fail.

what do you think?

pdonahue-ventana commented 9 months ago

I think that you mean CSRs instead of SPRs. (You must have an ARM background.)

The current XLEN can change dynamically (e.g. running a 32b process under a 64b OS). If the implementation supports abstract commands without halting then there's no way of knowing what the XLEN is at any given time because you don't know what mode you'll be in. If the implementation can only do the reads while halted, this isn't a problem. In that case, XLEN=DXLEN which ought to be known.

This seems better: "If aarsize specifies a size larger than DXLEN for accesses to X registers or CSRs or a size larger than FLEN for accesses to F registers, then the access must fail."

Functionally, this doesn't change anything with respect to X registers (the official name of GPRs) since "the register's actual size" is DXLEN (even if you're doing a non-halted access while XLEN!=DXLEN). Similarly, it doesn't affect F registers since "the register's actual size" is FLEN.[^1] The actual size of almost every CSR is also DXLEN. The only cases where that's definitely not true are fcsr, dcsr, mvendorid, mcounteren, mcountinhibit, scounteren, and hcounteren. And it might or might not be true for fflags, frm, vstart, and vcsr where the size is unspecified (which makes it hard to comply with the existing spec).

Because there are 4 CSRs where it's impossible to determine whether you're compliant with the spec, it seems like something definitely needs to change (when the public review period closes). I can't imagine that existing debuggers would be unhappy if doing a 64b access to the 32b fcsr, dcsr, mvendorid, etc. didn't fail when DXLEN=64 so bringing those 7 CSRs along seems reasonable.

[^1]: I'm intentionally ignoring the case where you have a writable misa.D bit and it's 0. The physical register is 64 bits but FLEN is 32.

algrobman commented 9 months ago

yes, I meant CSRs, BTW, my background is PowerPC ...

algrobman commented 9 months ago

One more problem may exist too - if an implementation does not support some of the "32 bit" CSRs, what error code should register abstract command return if debugger tries to access the CSR with size 64bits? We use CPU pipe /decoder to exercise these commands so we are getting error from the pipe/decoder for unimplemented CSRs. Make DM to know CSR size and respond accordingly we have to build at least partial CSR decoder in the DM which seems to me unreasonable waste of gates.

en-sc commented 9 months ago

BTW, can somebody enlighten me what is the size for fflags and frm CSRs?

This was somewhat covered in #888. Please, take a look.

en-sc commented 9 months ago

BTW, as far as I know openOCD reads misa to detect XLEN.

You are not correct, at least for the most recent RISC-V specific version: https://github.com/riscv/riscv-openocd/blob/riscv/src/target/riscv/riscv-013.c#L2045

algrobman commented 9 months ago

Guys, one more question , how should a debugger handle RV64F ? Does openOCD detects FLEN too by trying a FPR reads with different arrsize arguments?

en-sc commented 9 months ago

Does openOCD detects FLEN too by trying a FPR reads with different arrsize arguments?

The current approach in OpenOCD is to derive this through misa value. As soon as DXLEN of the target have been examined (as well as program buffer and so on), misa can be read and presence of D/F extensions can be determined.

algrobman commented 9 months ago

And how is all this handled if misa is not implemented? 2nd question: How/where is the "aarsize" for CSRs defined in openOCD? (like 64 bit MTVEC vs 32 bit DCSR)

en-sc commented 9 months ago

And how is all this handled if misa is not implemented?

Not sure currently there is any hardware that does support F or D without implementing misa, so this case is not covered in OpenOCD for now.

How/where is the "aarsize" for CSRs defined in openOCD? (like 64 bit MTVEC vs 32 bit DCSR)

I'm not sure I fully understand the question. A size of a register is defined in the corresponding specification (e.g. as you mentioned, DCSR is defined to be 32 bits by the debug spec). The mapping between register size and aarsize value is defined in debug spec.

algrobman commented 9 months ago

I'm not sure I fully understand the question.

I meant, where are the CSRs sizes defined in the openOCD code. (source files). If we define some privet CSRs to be 32 bit , how will the tool "know" about this? From first glance if CSR abstract command fails, (due arrsize mismatch) openOCD tries to use program buffer. But our CPU does not support program buffer.

We are forced to implement CSR size mismatch error check in Debug Module for abstract command ( while our implementation internally ignores the size - all operations to move data to/from CSRs is 64 bit, just upper data bits are set 0 for "32 bit CSRs")

Now I'm wondering if this size mismatch check will actually make more harm to CSR access support due all this logistics in debuggers code than help. (I saw in openOCD that it queries register size from some structure, but couldn't find how it is set up ).

en-sc commented 8 months ago

I meant, where are the CSRs sizes defined in the openOCD code. (source files).

Currently this is done in riscv_init_registers(). Though this function is quite cumbersome and I'm preparing a patch that will change register file examination and access, so it's likely to change in the near future.

If we define some privet CSRs to be 32 bit , how will the tool "know" about this?

This is a valid concern. You are correct: such register will be assumed to have width of XLEN (more precisely, DXLEN), which will result in abstract access failure. Please, file an issue in RISC-V OpenOCD repo.