[RFC 2/4] RISC-V: Hypervisor ext: CSR and Instructions

[Vineet Gupta]

On 12/17/21 9:30 AM, 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:26 PST (-0800), Vineet Gupta wrote:
>>> +#define MASK_HFENCE       0xfe007fff
>>> +#define MATCH_HFENCE_VVMA 0x22000073
>>> +#define MATCH_HFENCE_GVMA 0x62000073
>>> +
>>> +#define MASK_HINVAL       0xfe007fff
>>> +#define MATCH_HINVAL_VVMA 0x26000073
>>> +#define MATCH_HINVAL_GVMA 0x66000073
> We already have HINVAL in the mainline for now.

Ok I'll rebase for next version.

>>> +#define CSR_HCONTEXT    0x6a8
> The hcontext is one of the hypervisor csr, but it is also a debug csr.
> I have sent a patch to define this csr before, and it is controlled by
> the debug spec,
> https://sourceware.org/pipermail/binutils/2021-August/117568.html
>
> Maybe we should only choose one spec or one extension to control the
> hcontext csr, it should be worth discussing.

Indeed we need to sort out how to handle features that span multiple 
extensions (hypervisor + svinval). I'll send an email to tech-toolchain 
to solicit t

>>> +DECLARE_CSR(hstatus,     CSR_HSTATUS,     CSR_CLASS_H, PRIV_SPEC_CLASS_1P9P1, PRIV_SPEC_CLASS_DRAFT)
> The fourth and fifth fields means which spec starts to define the csr,
> and which spec starts to drop it.  Therefore, I think this should be,
>
> DECLARE_CSR(hstatus, CSR_HSTATUS, CSR_CLASS_H, PRIV_SPEC_CLASS_1P12,
> PRIV_SPEC_CLASS_DRAFT)
>
> Which means the csr hstatus is defined to 0x600 since the privileged
> spec 1.12, and until now (until draft).


[1] 
https://github.com/riscv/riscv-isa-manual/releases/download/draft-20211216-5651528/riscv-privileged.pdf

> Besides, we haven't dropped
> the privileged 1.9.1, so we also need an extra definition,
>
> DECLARE_CSR_ALIAS(hstatus, CSR_VSSTATUS, CSR_CLASS_H,
> PRIV_SPEC_CLASS_1P9P1, PRIV_SPEC_CLASS_1P10)
>
> Which means the csr hstatus is defined to 0x200 since the privileged
> spec 1.9.1, but dropped since the spec 1.10.

I like this approach for keeping backward compatible items and it is 
essential to do this to not break users. However in this specific case I 
wonder if we can clean the house a bit. Is there a way to tell the 
assembler to allow H-extension from 1.9.1 ? If not then it is 
effectively a dead code - until we fix it - in which case we just say 
that we only implement the latest 1.12 priv spec's H-ext 1.0.
What do you think ?

>
>>> +++ b/opcodes/riscv-opc.c
>>> @@ -839,9 +839,7 @@ const struct riscv_opcode riscv_opcodes[] =
>>>   {"csrrc",      0, INSN_CLASS_ZICSR,"d,E,Z",    MATCH_CSRRCI, MASK_CSRRCI, match_opcode, INSN_ALIAS },
>>>   {"uret",       0, INSN_CLASS_I,    "",         MATCH_URET, MASK_URET, match_opcode, 0 },
>>>   {"sret",       0, INSN_CLASS_I,    "",         MATCH_SRET, MASK_SRET, match_opcode, 0 },
>>> -{"hret",       0, INSN_CLASS_I,    "",         MATCH_HRET, MASK_HRET, match_opcode, 0 },
>>>   {"mret",       0, INSN_CLASS_I,    "",         MATCH_MRET, MASK_MRET, match_opcode, 0 },
>>> -{"dret",       0, INSN_CLASS_I,    "",         MATCH_DRET, MASK_DRET, match_opcode, 0 },
>> I don't know where the debug stuff ended up going, but dret was in some
>> earlier specs and was used.  I guess we should split it out into a D
>> spec of some sort.
> They are defined in the debug spec, including the debug instructions and csrs,
> https://github.com/riscv/riscv-debug-spec/blob/master/riscv-debug-stable.pdf
>
> However, we never consider that the instructions may be controlled not
> only by the ISA spec, and I don't know if there is any extension name
> defined for the debug spec or not...  Anyway, yes, eventually these
> debug instructions should be splitted to something like
> INSN_CLASS_DEBUG?  I don't know, we never consider that in fact...
> But I would suggest that we just keep it for now.

Make sense

>>> +/* Hypervisor instructions.  */
>>> +{"hlv.b",       0, INSN_CLASS_H, "d,(s)", MATCH_HLVB,   MASK_HLV, match_opcode, INSN_DREF|INSN_1_BYTE },
> Consider that users may write the assembly like this,
> hlv.b   a0, 0(a1)
>
> I would suggest adding the '0' operand into the string like "d,0(s)".
> We also have similar support for atomic instructions (A-ext), so
> keeping the compatibility should be good to users.

Make sense, I'll keep that.

>>> +{"hfence.vvma", 0, INSN_CLASS_H, "t,s", MATCH_HFENCE_VVMA, MASK_HFENCE, match_opcode, 0 },
>>> +{"hfence.gvma", 0, INSN_CLASS_H, "t,s", MATCH_HFENCE_GVMA, MASK_HFENCE, match_opcode, 0 },
> I cannot find the assembly syntax in the ISA spec, maybe they are
> defined somewhere just I don't know.  However, I find this patch,
> https://patchwork.kernel.org/project/linux-riscv/patch/20210115121846.114528-10-anup.patel@wdc.com/
>
> And according to the link, it shows the syntax as follows,
>
> /*
> * Instruction encoding of hfence.gvma is:
> * HFENCE.GVMA rs1, rs2
> * HFENCE.GVMA zero, rs2
> * HFENCE.GVMA rs1
> * HFENCE.GVMA
> *
> * rs1!=zero and rs2!=zero ==> HFENCE.GVMA rs1, rs2
> * rs1==zero and rs2!=zero ==> HFENCE.GVMA zero, rs2
> * rs1!=zero and rs2==zero ==> HFENCE.GVMA rs1
> * rs1==zero and rs2==zero ==> HFENCE.GVMA
> *
> * Instruction encoding of HFENCE.GVMA is:
> * 0110001 rs2(5) rs1(5) 000 00000 1110011
> */
>
> The syntax is compatible with sfence.vma, so this is also what I
> expected.  Maybe we should change the entries to,
>
> {"hfence.gvma", 0, INSN_CLASS_I,  "",     MATCH_HFENCE_GVMA,
> MASK_HFENCE_GVMA|MASK_RS1|MASK_RS2, match_opcode, INSN_ALIAS },
> {"hfence.gvma", 0, INSN_CLASS_I,  "s",    MATCH_HFENCE_GVMA,
> MASK_HFENCE_GVMA|MASK_RS2, match_opcode, INSN_ALIAS },
> {"hfence.gvma", 0, INSN_CLASS_I,  "s,t",  MATCH_HFENCE_GVMA,
> MASK_HFENCE_GVMA, match_opcode, 0 },
>
> And so does the hfence.vvma.

OK will fix.
  I just looked at the table os all encodings and missed the fact that 
some cases could omit an operand.

>>> +{"hinval.vvma", 0, INSN_CLASS_H, "t,s", MATCH_HINVAL_VVMA, MASK_HINVAL, match_opcode, 0 },
>>> +{"hinval.gvma", 0, INSN_CLASS_H, "t,s", MATCH_HINVAL_GVMA, MASK_HINVAL, match_opcode, 0 },
> I have cherry-picked the Svinval extension from the integration branch
> back to here, so I think hinval instruction may be changed to
> something like INSN_CLASS_SVINVAL_AND_H?

And this is after there's consensus on using both extensions for things 
will span multiple extensions.

And I think it would be a mess to define INSN_CLASS_SVINVAL_AND_H - who 
knows in future we have features crossing 3 extensions ?
IMO we should use indiv one and OR them - 
(INSN_CLASS_SVINVAL|INSN_CLASS__H)  -except that these would no longer 
be an enum but a bitmask

>   But we still have to clarify
> and make sure that the single h won't conflict with the ISA spec
> first.

Yeah for now we don't change it.