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

Add option to specify version for debug spec and priv spec #16

Closed
wants to merge 1 commit into from

Conversation

kito-cheng
Copy link
Collaborator

RISC-V having ISA string to represent enabled extensions and corresponding
versions, ideally we can use that as an universal scheme, but not all
specification are encoded in ISA string, like debug specification and privilege
specification, so we have defined following flags to control individual
versions:

  • -mpriv-spec=: Specify version for privilege specification.
  • -mdebug-spec=: Specify version for debug specification.

The argument of the option is the version number which is composed by number
and dot, e.g. -mpriv-spec=1.9, -mpriv-spec=1.10 or -mdebug-spec=0.13

NOTE: Toolchain may not support every version.

RISC-V having ISA string to represent enabled extensions and corresponding
versions, ideally we can use that as an universal scheme, but not all
specification are encoded in ISA string, like debug specification and privilege
specification, so we have defined following flags to control individual
versions:

- `-mpriv-spec=`: Specify version for privilege specification.
- `-mdebug-spec=`: Specify version for debug specification.

The argument of the option is the version number which is composed by number
and dot, e.g. `-mpriv-spec=1.9`, `-mpriv-spec=1.10` or `-mdebug-spec=0.13`

NOTE: Toolchain may not support every version.
@nick-knight
Copy link

I thought that the privileged spec working group was working on fixing their ISA string. I'm not opposed to instituting a shorter-term workaround (like this), especially since this issue has been outstanding for years. Just wondering what your plan is for the long-term, once they do formalize their ISA string: do we have to support this new flag in addition to the ISA string? (How are contradictions resolved? Etc.)

@kito-cheng
Copy link
Collaborator Author

My understanding is different privilege spec version will provide different set of CSR and instruction, and ideally it's only addition without incompatible: rename, no remove or re-map, although it's already happened several times before...like h* are dropped from privilege spec , sptbr renamed to satp...

So the assumption is new privileged extension, add new CSR and new instruction without changing the base-privileged spec. e.g. Sfoo add sxxx csr and syyy instruction, but existing CSR and instruction won't be changed or re-group...in theory? But I guess zicsr and zifence it's a bad example, they split CSR stuff and ifence instruction from I/E extension...in case there is some CSR or instruction are split out from (base?) privilege spec again, having an option to control the behavior would be better.

What's the interaction between -mpriv-spec and other privileged extension with ISA string?

  • -mpriv-spec=1.12 + -march=rv64gc: base line privilege spec 1.12, you can use any CSR/instruction defined in privilege spec 1.12
  • -mpriv-spec=1.12 + -march=rv64gc_sfoo: base line privilege spec 1.12, you can use any CSR/instruction defined in privilege spec 1.12 and sfoo extension.

That's basically what binutils doing now.

And what if we always adding new privilege stuffs by adding new extension in future? -mpriv-spec would become kind of backward compatible option for who want to using older privilege spec. once toolchain default to priv-spec 1.12, the option would be less useful I think (and I hope so).

@nick-knight
Copy link

I'm not going to stand in the way of progress, especially since I am not putting in the hard work to maintain our toolchain. I was objecting to what I view as a shortsighted workaround for a longstanding issue. Realistically, since we don't have control over the privileged spec task group, this workaround might just be the best solution.

I am also not an expert on the privileged spec. My understanding is that an implementation can support different parts of it. So my concern is that your simple version number is going to evolve into a separate, ad hoc ISA string, basically what the privileged spec task group is supposed to come up with.

@kito-cheng
Copy link
Collaborator Author

@nick-knight thanks your input, I guess I should consult with @aswaterman, I didn't spend too much effort on monitoring privileged spec task group, so really appropriate you :)

@aswaterman
Copy link

I don't view this proposed solution as problematic, but you could seek guidance from the tech-privileged mailing list.

@cmuellner
Copy link
Collaborator

I think it would make sense to define a default behavior if this flag is not given (e.g. if this flag is absent, then the latest implemented version will be in effect).

Additionally, there should be a way to retrieve the spec version (e.g. macro symbols) so that the code can test for the availability of certain features (e.g. CSRs). Otherwise, it is not possible to write code that works for several (potentially incompatible spec versions).

@kito-cheng
Copy link
Collaborator Author

@cmuellner sorry for late reply, I guess I miss this on my mailbox..

I think it would make sense to define a default behavior if this flag is not given (e.g. if this flag is absent, then the latest implemented version will be in effect).

Currently binutils implementation (already on trunk) have a configure time option, --with-priv-spec= which can specify the default version for binutils, but I guess this should not write into riscv-toolchain-conventions since clang/LLVM isn't work on that way.

Additionally, there should be a way to retrieve the spec version (e.g. macro symbols) so that the code can test for the availability of certain features (e.g. CSRs). Otherwise, it is not possible to write code that works for several (potentially incompatible spec versions).

Yeah, that's missing now, I should write another proposal for that :)

@kito-cheng
Copy link
Collaborator Author

Got +1 from @palmer-dabbelt at RISC-V GNU sync-up call.

@jrtc27
Copy link

jrtc27 commented Aug 29, 2021

Does enabling a newer privileged spec version disable CSRs+instructions from older versions? If so, do you have a way to ensure I can still write an OS kernel that works on older and newer spec versions with a single kernel binary rather than falling into the trap Arm did by having one kernel per board? Or does it just add the new CSRs+instructions, i.e. attempting to provide a means to catch some of the more obvious cases of people writing incompatible code?

Your examples are also all for pre-ratification CSRs and instructions, which neither toolchain makes any claims about supporting, although GNU as does support old CSR names due to inertia and LLVM recently gained support for them as code out there still uses the pre-ratified names (but will unconditionally warn about it), but that's done for practical reasons and per our policies could be removed at any time. I believe it's highly unlikely post-ratification that we will see such significant incompatible changes to the privileged spec, just additions of new features and occasional clarification/additional constraints for existing features?

@jim-wilson
Copy link
Collaborator

In GNU as, csr checks are warnings, not errors, and can be disabled, via either command line option or via the .option directive.

There is shipping hardware that supports old versions of the priv spec. We need to support multiple priv spec versions so people can build code for all available hardware.

We set up a hash table that has entries that associate a csr name with a range of valid priv spec versions. There can be multiple entries for a single csr name. If csr checking is enabled, then we look for an entry where the specified priv spec version is within the range that entry is valid for. If no such entry is found, then we emit a warning and use the default entry. If csr checking is disabled, then we just use the default entry. The default entry is presumably the one for the more recent version of the priv spec.

This allows us to give warnings if you use the old name of a csr and asked for a current priv spec version. We can give warnings if you asked for an old priv spec and used a new csr name. Etc.

The debug spec option will work the same way but isn't implemented yet.

@asb
Copy link
Contributor

asb commented Sep 2, 2021

Got +1 from @palmer-dabbelt at RISC-V GNU sync-up call.

So just to confirm - is there expected to be an upcoming proposal that adds functionally similar versioning of the privileged (and maybe debug specs) to the standard ISA string? This mechanism seems perfectly sensible, but per @nick-knight's comment above I'd just be concerned if we ended up implementing this and then having a second way to do it in the ISA string in a few month's time. If that's not a direction the privilege spec group are likely to go in, or is going to be much much further in the future, then obviously it's not such a concern.

@kito-cheng
Copy link
Collaborator Author

Got +1 from @palmer-dabbelt at RISC-V GNU sync-up call.

So just to confirm - is there expected to be an upcoming proposal that adds functionally similar versioning of the privileged (and maybe debug specs) to the standard ISA string? This mechanism seems perfectly sensible, but per @nick-knight's comment above I'd just be concerned if we ended up implementing this and then having a second way to do it in the ISA string in a few month's time. If that's not a direction the privilege spec group are likely to go in, or is going to be much much further in the future, then obviously it's not such a concern.

I got confirmation from RVIA[1], all privileged spec will have unique extension names, once that really happen, this option would be used for compatible way for existing hardware, program and build scripts.

[1] https://lists.riscv.org/g/tech-privileged/message/828

@jrtc27
Copy link

jrtc27 commented Sep 2, 2021

Got +1 from @palmer-dabbelt at RISC-V GNU sync-up call.

So just to confirm - is there expected to be an upcoming proposal that adds functionally similar versioning of the privileged (and maybe debug specs) to the standard ISA string? This mechanism seems perfectly sensible, but per @nick-knight's comment above I'd just be concerned if we ended up implementing this and then having a second way to do it in the ISA string in a few month's time. If that's not a direction the privilege spec group are likely to go in, or is going to be much much further in the future, then obviously it's not such a concern.

I got confirmation from RVIA[1], all privileged spec will have unique extension names, once that really happen, this option would be used for compatible way for existing hardware, program and build scripts.

[1] https://lists.riscv.org/g/tech-privileged/message/828

Isn't that precisely what Alex is worried about? We want to ensure we have a single way of doing things, rather than introducing -mpriv-spec= now for existing privileged specs and then adding -march= extensions for future things. It seems the right thing to do would be to retroactively define extension names for older privileged spec versions that toolchains need to support?

@kito-cheng
Copy link
Collaborator Author

Potential solution for this issue:

  • Add -mpriv-spec= option to control the version, and also using -march= after priv 1.12 ratified.
    • Don't need to specify any -march if -mpriv-spec < 1.12
    • Need specify all priv extension to -march if -mpriv-spec >= 1.12
  • Add -mpriv-spec= option to control the version, but user don't need to specify any -march.
  • NO new option, use -march as unify interface, but once priv 1.12 ratified, force user must specify all used extension to -march.
  • NO new option, always allow user to use all priv CSR/instruction.
  • -mprofile[1] implied priv spec version, implied to 1.11 if not given, and require user to explicitly priv ext in -march
  • -mprofile implied priv spec version, implied to 1.11 if not given, and NO priv ext in -march

Any further proposal or better solution?

[1] This option isn't present yet, but we need similar one in future to handle architecture profile, see also https://github.com/riscv/riscv-profiles

@rjiejie
Copy link

rjiejie commented Sep 3, 2021

I thought that the privileged spec working group was working on fixing their ISA string. I'm not opposed to instituting a shorter-term workaround (like this), especially since this issue has been outstanding for years. Just wondering what your plan is for the long-term, once they do formalize their ISA string: do we have to support this new flag in addition to the ISA string? (How are contradictions resolved? Etc.)

+1

I think it's better consistency if we add new flag in addition to the ISA string,
specially for variant ISA specs :)

@kito-cheng
Copy link
Collaborator Author

Close due to RVI has added extension for those stuffs, so I think option is not really a prefer way :)

@kito-cheng kito-cheng closed this Mar 13, 2024
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

Successfully merging this pull request may close these issues.

None yet

8 participants