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

Unclear size of FWFT Feature Types (Chapter 18) #174

Open
sfiedler0 opened this issue Oct 16, 2024 · 4 comments
Open

Unclear size of FWFT Feature Types (Chapter 18) #174

sfiedler0 opened this issue Oct 16, 2024 · 4 comments

Comments

@sfiedler0
Copy link
Contributor

In the text the FWFT feature types are described as "32-bit identifiers", but in function calls they are described as unsigned long, which will result in 64-bit identifiers.

@atishp04
Copy link
Collaborator

Good catch.
@clementleger @avpatel : We don't need 64 bit feature values. We should change the function arguments to uint32_t ? It would require the fixes in the existing implementation though.

@clementleger
Copy link
Contributor

Yes clearly that should be defined as uint32_t, thanks for catching this. I'll submit fixes for the doc/implementation if you want.

@jrtc27
Copy link
Contributor

jrtc27 commented Oct 17, 2024

The documentation says:

Every SBI function should prefer unsigned long as the data type. It keeps the specification simple and easily adaptable for all RISC-V ISA types. In case the data is defined as 32bit wide, higher privilege software must ensure that it only uses 32 bit data.

This doesn't read very clearly to me. Does that mean the function should be declared as taking an unsigned long but only read 32 bits from it? Or does it mean the function should be declared as taking a 32-bit integer? Also it seems to suggest that the high bits of the register on RV64 can be junk, which is notably not what the psABI says, so you can't just forward the register in assembly to a C function taking a uint32_t.

@jrtc27
Copy link
Contributor

jrtc27 commented Oct 17, 2024

Though it seems there are a number of interfaces already taking uint32_t and hitting that exact case of a lack of adequate documentation...

atishp04 pushed a commit to atishp04/riscv-sbi-doc that referenced this issue Oct 29, 2024
As reported on github [1], the feature types values are described as 32
bits identifiers but declared as unsigned long in function arguments. Use
uint32_t for the parameters types to be coherent.

Link: riscv-non-isa#174 [1]

Reviewed-by: Andrew Jones <[email protected]>
Reviewed-by: Atish Patra <[email protected]>
Signed-off-by: Clément Léger <[email protected]>
atishp04 pushed a commit to atishp04/riscv-sbi-doc that referenced this issue Oct 29, 2024
As reported on github [1], the feature types values are described as 32
bits identifiers but declared as unsigned long in function arguments. Use
uint32_t for the parameters types to be coherent.

Link: riscv-non-isa#174 [1]

Reviewed-by: Andrew Jones <[email protected]>
Reviewed-by: Atish Patra <[email protected]>
Signed-off-by: Clément Léger <[email protected]>
atishp04 pushed a commit that referenced this issue Oct 29, 2024
As reported on github [1], the feature types values are described as 32
bits identifiers but declared as unsigned long in function arguments. Use
uint32_t for the parameters types to be coherent.

Link: #174 [1]

Reviewed-by: Andrew Jones <[email protected]>
Reviewed-by: Atish Patra <[email protected]>
Signed-off-by: Clément Léger <[email protected]>
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