Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

abstract register command and aarsize restrictions #929

Open
algrobman opened this issue Dec 20, 2023 · 21 comments
Open

abstract register command and aarsize restrictions #929

algrobman opened this issue Dec 20, 2023 · 21 comments

Comments

@algrobman
Copy link

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Collaborator

all CSRs are 64 bit in 64bit implementations

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

@algrobman
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Author

algrobman commented Dec 21, 2023

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
Copy link
Collaborator

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.

Footnotes

  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
Copy link
Author

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

@algrobman
Copy link
Author

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
Copy link
Contributor

en-sc commented Dec 21, 2023

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
Copy link
Contributor

en-sc commented Dec 21, 2023

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
Copy link
Author

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
Copy link
Contributor

en-sc commented Jan 12, 2024

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
Copy link
Author

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
Copy link
Contributor

en-sc commented Jan 12, 2024

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
Copy link
Author

algrobman commented Jan 12, 2024

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
Copy link
Contributor

en-sc commented Jan 15, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants