Consider adding mbadinst CSR

[riscv]

Consider adding mbadinst CSR

[aswaterman]

Possible alternative: use mbadaddr for this purpose, since it is redundant with mepc on illegal-instruction traps.

[aswaterman]

Need to be careful about what shows up in MSBs; simplest thing to do is to zero them.

[David-Horner]

How precious are these CSRs, really?
The extra real-estate for flip-flops in the core for 32 (or 64 bits) is indeed significant.
However, reserving and allocating the register number is not a significant burden, is it?

Re:

Possible alternative: use mbadaddr for this purpose, since it is redundant with mepc on illegal instruction traps.

I'd suggest making mbadaddr undefined for trapped instructions that do not have an addressing component.
And that mbadinst contain the trapping instruction in the LSB with any MSB undefined.
As an implementation optimization mbadinst is alias with mbadaddr and the instruction be injected into mbadinst if that overall reduces complexity/cost/etc.

I suggest MSB be undefined as it could allow a future extension to, say inject the actual compressed instruction there - or to indicate a fused instruction, etc.. I only foresee this as significant on RV64/RV128.

Further, how is an illegal compressed instruction shown?
Indeed, if an instruction has the low order bit 00,01 or 10 and it is not a valid Compressed Instruction, is it an illegal Compressed instruction, or "just" an illegal instruction.
e.g. is x"00000000" one or two illegal instructions - does it depend upon C extension being implemented?

This does raise the question of how to handle an illegal exception raise by the hart before the remainder of the instruction is read (for example at the end of a cache line).

I would recommend that at least the 4 LSBytes as available to the decoder be written to mbadinst, whether the format would otherwise be 16bit on a Compressed Extension implementation or not, and whether all the instruction had been loaded for the decode yet.
Imposing a further fetch of the remainder of an illegal instruction is too heavy handed.
And in the case of x"FFFFFF..." completely unnecessary.

The trap handler must be able to parse the instruction from LSB forward and detect early illegal formats in the lower 2 bytes. Otherwise, the trap handler will have to read the instruction from memory.

The mbadinst will potentially help with race conditions that have modified the code before the trap handler gets to processing it, likely a very rare occurrence, but without this register it likely will be extremely difficult to identify.

[kasanovic]

I don't like design that allows badaddr be an alias of badinst, as this doesn't provide any real advantage over just defining a common "bad" CSR that holds the offending bit pattern, and is confusing.
I think you want to make any unwritten upper bits be zero to avoid propagating information between traps, and to avoid explicit masking in some code paths.
I'd expect trap handler to always parse one parcel (2 bytes) a time when C extension supported, so should find trap cause in first two bytes of longer instruction if straddling cache lines, and this will disambiguate 0x00000000 case. I agree don't want to force fetch unit to fetch past first trap, or past one instruction if faulting instruction less than XLEN bits long.
For instructions>XLEN, if handler can't determine what to do from first XLEN bits, will have to refetch from memory - no worse than current scheme.
Alternatlvely, could require multiple CSRs sufficient to hold max instruction bytes. Even then, a system with hardware supporting up to X-byte instructions might want to emulate system supporting >X-byte instructions, so would still have to read from memory.
Macro-op fusing implementations should behave transparently the same as non-fusing implementations.

[aswaterman]

Agreed.

[sorear]

The main subtle point here IMO is that M-mode needs to be able to know the number of valid parcels in mbadinst.

This could be done by saying that mbadinst always holds either 0 or a complete instruction, or by using a second CSR to indicate the length.

In the former case hardware and M-mode need to agree on what "a complete instruction" is. I'd rather not leave that implementation-defined, but §10.3 clearly indicates that implementations are allowed to differ on whether 0x00010001 is 1 instruction or 2. One option would be to use the figure 1.1 table, but modify it so that the first row says "16-bit if MISA.C is 1, 32-bit otherwise".

[aswaterman]

Yeah, I think misa.C should disambiguate this for the 16-vs-32 case, and 48-bit+ extensions will similarly be disambiguated by additional bits in misa.

[David-Horner]

I don't like design that allows badaddr be an alias of badinst, as this doesn't provide any real advantage over just defining a common "bad" CSR that holds the offending bit pattern, and is confusing.
agreed. If we can still redefine CSRs at this juncture - and I assume from your response, we can.
And if we are going to do it, we need to do it in v1.10.
Which means that on an illegal instruction, this register must written; Cleared if the implimentation does not provide any illegal instruction bytes.
In terms of naming is instbad sufficient? As with pseudo-op, badaddr and badinst could be different names for the same csr address.

I'd expect trap handler to always parse one parcel (2 bytes) a time when C extension supported
Or emulated (even if C extension is not (fully) implemented in hardware).

I think you want to make any unwritten upper bits be zero to avoid propagating information between traps, and to avoid explicit masking in some code paths.

Unwritten bytes of trapped instruction(s)- Agreed for 32/64. Perhaps RV128 could benefit from meta data in the higher byte(s).
I'd like us to reserve them in RV128 for now. This does align with the potential plans for address
The trap handler has to determine if the whole instruction is one which it intends to emulate. In the event that the higher bytes are truly zero in the instruction, the trap handler will have to reread those bytes to ensure they are so in memory. Again, no worse than without the csr.

However, I proposed that as many bytes as are seen by the decoder be written (and minimally Even if the trap parsing could determine a possible reason for invalid opcode in fewer bytes. because many an available to theseen by the

[David-Horner]

oops. the quotes extended more than I intended.

I don't like design that allows badaddr be an alias of badinst, as this doesn't provide any real advantage over just defining a common "bad" CSR that holds the offending bit pattern, and is confusing.

agreed. If we can still redefine CSRs at this juncture - and I assume from your response, we can.
And if we are going to do it, we need to do it in v1.10.
Which means that on an illegal instruction, this register must written; Cleared if the implimentation does not provide any illegal instruction bytes.
In terms of naming is instbad sufficient? As with pseudo-op, badaddr and badinst could be different names for the same csr address.

I'd expect trap handler to always parse one parcel (2 bytes) a time when C extension supported

Or emulated (even if C extension is not (fully) implemented in hardware).

[sorear]

@aswaterman would it be a pain for 64-bit rocket-ish implementations today to write 0 to mbadinst if (insn & 31) == 31? I'm assuming we won't have a single "48-bit extension" with a dedicated bit in misa, the 48-bit opcode space will be used by a large number of standard and nonstandard extension?

[David-Horner]

The main subtle point here IMO is that M-mode needs to be able to know the number of valid parcels in mbadinst.

As I suggested - the zero bytes are sufficient to delimit all the definitively known bytes of the instruction the decoder has. The high order zero bytes will need to be checked.

This could be done by saying that mbadinst always holds either 0 or a complete instruction, or by using a second CSR to indicate the length.

An additional CSR is overkill for a non-essential helper fucntion.

In the former case hardware and M-mode need to agree on what "a complete instruction" is. I'd rather not leave that implementation-defined. but §10.3 clearly indicates that implementations are allowed to differ on whether 0x00010001 is 1 instruction or 2. One option would be to use the figure 1.1 table, but modify it so that the first row says "16-bit if MISA.C is 1, 32-bit otherwise".

And The MISA.C bit does not help if the M-Mode is intending on emulating the mode on machines that (honestly and transparently) clear C bit because they do not provide the hardware support in the decoder. .

[sorear]

@David-Horner my point is that if you clear the top 16 bits of a valid instruction, say "lui a0, 17", you wind up with a different valid instruction, in this case "lui a0, 1". CLZ is not sufficient.

[David-Horner]

And The MISA.C bit does not help if the M-Mode is intending on emulating

ditto for 48 bit opcode emulation. What hardware registers indicate of there support , apparently, can be what every they want if they emulate it.

According to - casinovic

At the first meeting at the last workshop, the consensus was that there was no need/ impossible to distinguish between hardware and software implementations of a feature.

[asb]

@aswaterman: doesn't misa.C indicate that that the standard C extension is supported, which is slightly different to the question of whether 16-bit instructions are supported. An implementation might use the C opcode space for non-standard 16-bit instructions, but they couldn't advertise misa.C in that case.

[David-Horner]

@sorear - what I am suggesting is that the CSR are a help, and will not disambiguate all situations.

In the case you suggest, neither opcode is illegal (regardless of the literal value, lui is non likely to trap

So, in the typical case a zero upper half word will have to reread the instruction.

And, for clarity, the granularity should always be 2 bytes provided at a time.
All non-provided be zero'd.

This reduces to the "non-implemented" case that write the register to zero.

[David-Horner]

Alternatlvely, could require multiple CSRs sufficient to hold max instruction bytes. Even then, a system with hardware supporting up to X-byte instructions might want to emulate system supporting >X-byte instructions, so would still have to read from memory.

It doesn't have to be mandated, but certainly more instrbad CSR could be defined (instrbad2, etc).
And for the instruction decode (illegal trap case), the same convention of high order zero half words need further checking. The RV128 intension of meta data in the high order bytes of the auxiliary csr (instrbad2) could be implemented with the additional registers in RV32/RV64

Macro-op fusing implementations should behave transparently the same as non-fusing implementations.

Agreed - it should fall back to discrete instructions. But an optimistic decoding implementation may cause a illegal instruction trap (which is really an unsupported instruction trap, possibly with a further cause in the mcause CSR) and allow the M-mode to handle some fringe cases.

I hope we can also provide support to this model with a clean instrbad design.

[aswaterman]

@asb IMO, in the event of conflicting non-standard extensions, the M-mode software needs to be aware of them (hence won't need to interrogate misa.C)

[David-Horner]

I believe the other traps that store an address value in mbadaddr (breakpoint , address-misaligned or access exception) would also substantially benefit from the mbadinstr information.
This means that separate CSR registers is required.

@sorear

or by using a second CSR to indicate the length.

In the general case, the occurrence of high order half-word zero values is low (1 of 65536).
Even so, a convention for WLRL can be used to determine the half words of the faulting instruction that have been written to the register vs default filler:

CSRRS individual half words that are zeros in the register to ones would set the bits iff the value is not part of the trapped instruction.
This is the classic WLRL that occurs when the "unconstrained" parts of the legal value are changed.
The special case of CSRW of -1 needs to be allowed to set all the bits to differentiate between implementations that do not support the WLRL convention.
If it cannot be determined that this convention is implemented, then fall back to reread of the opcode is required.

[aswaterman]

Let's not have mission creep here. The goal is a good spec, not a perfect one. mbadinst is meant to improve the performance of illegal instruction emulation--not for any other reason.

…

On Sat, Mar 4, 2017 at 9:07 AM David-Horner ***@***.***> wrote: I believe the other traps that store an address value in mbadaddr (breakpoint , address-misaligned or access exception) would also substantially benefit from the mbadinstr information. This means that separate CSR registers is required. @sorear <https://github.com/sorear> or by using a second CSR to indicate the length. In the general case, the occurrence of high order half-word zero values is low (1 of 65536). Even so, a convention for WLRL can be used to determine the half words of the faulting instruction that have been written to the register vs default filler: CSRRS individual half words that are zeros in the register to ones would set the bits iff the value is not part of the trapped instruction. This is the classic WLRL that occurs when the "unconstrained" parts of the legal value are changed. The special case of CSRW of -1 needs to be allowed to set all the bits to differentiate between implementations that do not support the WLRL convention. If it cannot be determined that this convention is implemented, then fall back to reread of the opcode is required. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA-7wnoJFcfTHlm8zgMe_pRjIFuY57dcks5riZo6gaJpZM4L-WFY> .

[aswaterman]

I don't think we're going to do the potentially-zero-parcel thing for 32-bit instructions.

If the instruction is 32 bits long, then either (a) the whole instruction is fetched and reported as an illegal instruction, then placed into mbadinst, with bits 32 and above zero-filled, or the (b) an access or misalignment exception occurs, and mbadaddr is written with the address. Partial fetches of 32-bit instructions never cause illegal instruction traps. This keeps emulation of standard 16-bit and 32-bit instructions simple (which was the whole point of adding this register in the first place!).

If 16-bit instructions are supported, then either (a) the system supports 16-bit instructions, an illegal instruction trap occurs, and 16 bits are written to misa with the upper bits zero-filled, or (b) the system doesn't support 16-bit instructions, an illegal instruction trap occurs, and 32 bits are written to misa, with bits 32 and above zero-filled; or (c) an access or misalignment exception occurs, and mbadaddr is written with the bad address.

These both keep emulation of 16-bit and 32-bit standard instructions simple, which was the whole point of adding mbadinst!

For >32-bit instructions, here are two options that are both simple to implement and compatible with the above:

• Unconditionally zero-fill bits 32 and above. This has the advantage of being simple to spec, and has similar behavior for RV32 and RV64+.
• If the machine supports N-bit instructions, fetch up to N bits and put min(N, XLEN) of those bits into mbadinst. This is also easy to spec, and easy to use, if there is a way of determining N. (N can vary with misa.)

[DSHorner]

@aswaterman

If 16-bit instructions are supported, then either (a) the system supports 16-bit instructions, an illegal instruction trap occurs, and 16 bits are written to misa with the upper bits zero-filled, or (b) the system doesn't support 16-bit instructions, an illegal instruction trap occurs, and 32 bits are written to misa..

I expect mbadinst was intended here.

With the above, If mbadinst is a distinct CSR from mbadaddr, then it should be read-only.

Or are all these registers that could be accessible in lower privilege levels read-write to allow virtualisation?

Are mcause and mbadaddr implicitly readable in lower privilege levels if x-edeleg illegal instruction
is enabled for that level?
Should mbadinst be explicitly added as implicitly accessible in the same situation?
Or should we be adding a mechanism like that for the hardware performance counters?

[aswaterman]

(I'm still assuming the design that mbadaddr is used for this purpose, possibly with a new name.)

No, mbadaddr is not accessible to less-privileged code. Supervisor has its own sbadaddr register. If an exception is delegated to S-mode, and that exception would've written mbadaddr, sbadaddr is written instead.

[kasanovic]

Going to write this up with mbadbits being the single CSR that contains the offending bit pattern, either address or instruction depending on trap type (and I guess can be used for bad data values in future) I'm assuming we can alias mbadaddr and mbadinst to this same CSR in the assembler.

I'm also assuming whole instruction is fetched and placed into mbadbits, or bottom XLEN bits if >XLEN bits, with any unused upper bits zeroed.

[kasanovic]

Writeup pushed.