[RFC 1/4] RISC-V: Hypervisor ext: Treat as "Standard" extension
Palmer Dabbelt
palmer@dabbelt.com
Fri Dec 17 22:50:22 GMT 2021
On Fri, 17 Dec 2021 08:10:42 PST (-0800), Nelson Chu wrote:
> On Fri, Dec 17, 2021 at 12:10 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>
>> On Thu, 16 Dec 2021 09:33:25 PST (-0800), Vineet Gupta wrote:
>> > gas currently fails for single-letter 'h' prefix in arch string.
>> >
>> > | riscv64-unknown-elf-as -march=rv64gh empty.s
>> > | Assembler messages:
>> > | Error: rv64gh: unknown prefixed ISA extension `h'
>> >
>> > It implements the "multi-letter" prefix for H-ext as recommended in
>> > ISA manual subsection "ISA Extension Naming Conventions". However given
>> > the discussions at [1] there seems to be no pressing need to keep version
>> > for the current H-ext since
>> > - it will remain at 1.0
>> > - any future Hypervisor extensions wull called "Sh*" or some such
>> > - H was the original standard extension with its own MISA bit
>> >
>> > This patch thus relaxes the constraint and allows single letter prefix
>> > 'h'
>> >
>> > Note: To keep the binutils dejagnu failures same, I've fixed the
>> > testsuite files within the same patch. I can break it up if reviewers
>> > feel as such.
>> >
>> > [1] https://github.com/riscv/riscv-isa-manual/issues/781#issuecomment-983233088
>> >
>> > bfd/
>> > * elfxx-riscv.c (riscv_supported_std_ext): Add "h" entry.
>> > (riscv_supported_std_h_ext): Add "h" entry.
>> > (riscv_get_default_ext_version): Handle PRIV_SPEC_CLASS_DRAFT.
>> > (riscv_multi_subset_supports): Handle INSN_CLASS_H.
>> >
>> > gas/
>> > * testsuite/gas/riscv/march-fail-single-prefix-h.d: Deleted as
>> > single-letter prefix 'h' is now valid.
>> > * testsuite/gas/riscv/march-fail-single-prefix-l: Removed 'h'.
>> >
>> > include/
>> > * opcode/riscv.h (enum riscv_insn_class): Add INSN_CLASS_H.
>> >
>> > Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
>> > ---
>> > bfd/elfxx-riscv.c | 5 +++++
>> > gas/testsuite/gas/riscv/march-fail-single-prefix-h.d | 3 ---
>> > gas/testsuite/gas/riscv/march-fail-single-prefix.l | 2 +-
>> > include/opcode/riscv.h | 1 +
>> > 4 files changed, 7 insertions(+), 4 deletions(-)
>> > delete mode 100644 gas/testsuite/gas/riscv/march-fail-single-prefix-h.d
>> >
>> > diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
>> > index 3bd41ff2b551..7d5ae5a4657d 100644
>> > --- a/bfd/elfxx-riscv.c
>> > +++ b/bfd/elfxx-riscv.c
>> > @@ -1173,6 +1173,7 @@ static struct riscv_supported_ext riscv_supported_std_ext[] =
>> > {"t", ISA_SPEC_CLASS_NONE, RISCV_UNKNOWN_VERSION, RISCV_UNKNOWN_VERSION, 0 },
>> > {"p", ISA_SPEC_CLASS_NONE, RISCV_UNKNOWN_VERSION, RISCV_UNKNOWN_VERSION, 0 },
>> > {"v", ISA_SPEC_CLASS_DRAFT, 1, 0, 0 },
>> > + {"h", PRIV_SPEC_CLASS_DRAFT, 1, 0, 0 },
>>
>> According to
>> <https://wiki.riscv.org/display/TECH/Recently+Ratified+Extensions>, this
>> was ratified in November so we should be able to update this to
>> something like PRIV_SPEC_CLASS_1P12 (the V stuff was also ratified, but
>> that's a different issue).
>
> I can understand that why Vineet are doing these changes, but I had a
> quick talk with Kito this morning, and I think we also have three
> issues for now,
>
> 1. The current draft ISA spec doesn't have any related changes, so if
> we change the `h' as a single standard extension, then this will
> conflict with the spec.
Can you be specific about which specification you're talking about? By
my count there's more than a dozen now and it gets kind of tricky to
figure out which is which.
I'm assuming you're talking about the user ISA spec, the last ratified
version I can find is
<https://github.com/riscv/riscv-isa-manual/releases/download/Ratified-IMAFDQC/riscv-spec-20191213.pdf>,
which definately lists H as a prefix.
> 2. Even if we decide to change the `h' as a single extension rather
> than the multiple prefixed keyword, then in what order should we place
> h. I expect the table 27.1 in the ISA spec will also mention the
> order of the single h.
I guess I'd just assumed this was called "H", given that's what we've
all been calling it for a long time, but after actually reading the
ratified specs I don't see that anywhere.
The Hypervisor extension itself is ratified, via priv-1.12, but I don't
see anything in a ratified spec that describes what it's called in ISA
strings. It's called "Hypervisor ISA, Version 1.0" in the preface and
"Hypervisor Extension, Version 1.0.0-rc" in the chapter that defines it
(both in the ratified priv-1.12). Both of those claim they're frozen,
in contrast to the ratified user spec which calls out extensions as
ratified -- not sure if that's relevant, though, as I'm generally pretty
lost WRT the state of the specifications/extensions/versions right now.
The rules in the ratified user spec make it sound like this would either
be called "S*" or "H*" (depending on whether this is a supervisor-mode
extension or an ISA), but I can't find anything pointing to where the
second letter should be. Also no idea what to do with the version,
assuming that RC suffix is canonical.
Maybe I'm missing something?
> 3. I never considered that an extension version may be controlled by
> the privileged spec before, so in the riscv_get_default_ext_version,
>
>> > if (strcmp (table[i].name, name) == 0
>> > && (table[i].isa_spec_class == ISA_SPEC_CLASS_DRAFT
>> > + || table[i].isa_spec_class == PRIV_SPEC_CLASS_DRAFT
>> > || table[i].isa_spec_class == *default_isa_spec))
>
> We will need to rewrite the related code, since the isa_spec_class may
> be one of the PRIV_SPEC_CLASS_XXX in the future, so we will never
> match it to the default_isa_spec...
More that that: it now sounds like the hypervisor extension (and/or ISA?)
encoding for ISA strings changes based on the user specification in
play, despite it being defined in the priv specification.
Certainly sounds like we should hold off until we get something concrete
on the naming scheme, as making something up on our own is going to be a
mess in the long run.
>> > {"n", ISA_SPEC_CLASS_NONE, RISCV_UNKNOWN_VERSION, RISCV_UNKNOWN_VERSION, 0 },
>> > {NULL, 0, 0, 0, 0}
>> > };
>> > @@ -1232,6 +1233,7 @@ static struct riscv_supported_ext riscv_supported_std_s_ext[] =
>> >
>> > static struct riscv_supported_ext riscv_supported_std_h_ext[] =
>> > {
>> > + {"h", PRIV_SPEC_CLASS_DRAFT, 1, 0, 0 },
>> > {NULL, 0, 0, 0, 0}
>> > };
>> >
>> > @@ -1531,6 +1533,7 @@ riscv_get_default_ext_version (enum riscv_spec_class *default_isa_spec,
>> > {
>> > if (strcmp (table[i].name, name) == 0
>> > && (table[i].isa_spec_class == ISA_SPEC_CLASS_DRAFT
>> > + || table[i].isa_spec_class == PRIV_SPEC_CLASS_DRAFT
>> > || table[i].isa_spec_class == *default_isa_spec))
>> > {
>> > *major_version = table[i].major_version;
>> > @@ -2412,6 +2415,8 @@ riscv_multi_subset_supports (riscv_parse_subset_t *rps,
>> > || riscv_subset_supports (rps, "zve64d")
>> > || riscv_subset_supports (rps, "zve64f")
>> > || riscv_subset_supports (rps, "zve32f"));
>> > + case INSN_CLASS_H:
>> > + return riscv_subset_supports (rps, "h");
>> > default:
>> > rps->error_handler
>> > (_("internal: unreachable INSN_CLASS_*"));
>> > diff --git a/gas/testsuite/gas/riscv/march-fail-single-prefix-h.d b/gas/testsuite/gas/riscv/march-fail-single-prefix-h.d
>> > deleted file mode 100644
>> > index eb101bd71353..000000000000
>> > --- a/gas/testsuite/gas/riscv/march-fail-single-prefix-h.d
>> > +++ /dev/null
>> > @@ -1,3 +0,0 @@
>> > -#as: -march=rv32ih
>> > -#source: empty.s
>> > -#error_output: march-fail-single-prefix.l
>> > diff --git a/gas/testsuite/gas/riscv/march-fail-single-prefix.l b/gas/testsuite/gas/riscv/march-fail-single-prefix.l
>> > index 13942ed66642..e4418de69acb 100644
>> > --- a/gas/testsuite/gas/riscv/march-fail-single-prefix.l
>> > +++ b/gas/testsuite/gas/riscv/march-fail-single-prefix.l
>> > @@ -1,2 +1,2 @@
>> > .*Assembler messages:
>> > -.*Error: .*unknown prefixed ISA extension `(s|h|z|x|zxm)'
>> > +.*Error: .*unknown prefixed ISA extension `(s|z|x|zxm)'
>> > diff --git a/include/opcode/riscv.h b/include/opcode/riscv.h
>> > index 14889972abce..2783bdecaae5 100644
>> > --- a/include/opcode/riscv.h
>> > +++ b/include/opcode/riscv.h
>> > @@ -387,6 +387,7 @@ enum riscv_insn_class
>> > INSN_CLASS_ZKND_OR_ZKNE,
>> > INSN_CLASS_V,
>> > INSN_CLASS_ZVEF,
>> > + INSN_CLASS_H,
>> > };
>> >
>> > /* This structure holds information for a particular instruction. */
>>
>> Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>
>>
>> We can always deal with the draft/ratified stuff later, as there's a
>> bunch of them.
More information about the Binutils
mailing list