[v2] RISC-V: Generate ELF priv attributes if priv instruction are explicited used.

Message ID 1592813478-19385-1-git-send-email-nelson.chu@sifive.com
State New
Headers show
Series
  • [v2] RISC-V: Generate ELF priv attributes if priv instruction are explicited used.
Related show

Commit Message

Nelson Chu June 22, 2020, 8:11 a.m.
We should generate the ELF priv attributes only if,

1. The priv attributes are aleady set in the assembly file.
2. The CSR are explicited used.
3. The privileged instruction are explicited used.

* There are four privileged instruction defined in the v1.11 priv spec:
`mret`, `sret`, `wfi` and `sfence.vma`.

* `sfence.vm` is dropped in the v1.10 priv spec.

* `uret` is actually a N-ext instruction.  So it is better to regard it as
an user instruction rather than the priv instruction.

* `hret` is used to return from traps in H-mode.  H-mode is removed since
the v1.10 priv spec, but probably be added in the new hypervisor spec.
Therefore, `hret` should be controlled by the hypervisor spec rather than
priv spec in the future.

* `dret` is a debug instruction.  We should record the debug spec versions
once it is explicited used in the future.

	gas/
	* config/tc-riscv.c (explicit_priv_attr): Rename explicit_csr to
	explicit_priv_attr.  It used to indicate CSR or priv instructions are
	explictly used.
	(riscv_is_priv_insn): Return True if it is a privileged instruction.
	(riscv_ip): Call riscv_is_priv_insn to check whether the instruction
	is privileged or not.  If it is, then set explicit_priv_attr to TRUE.
	(riscv_write_out_attrs): Clarification of when to generate the elf
	priv spec attributes.

	* testsuite/gas/riscv/attribute-11.s: Add comments.
	* testsuite/gas/riscv/attribute-14.s: New testcase.  Use symbol
	`priv_insn_<n>` to decide which priv instruction is expected to used.
	(<n> is a to g.)
	* testsuite/gas/riscv/attribute-14a.d: Likewise.
	* testsuite/gas/riscv/attribute-14b.d: Likewise.
	* testsuite/gas/riscv/attribute-14c.d: Likewise.
	* testsuite/gas/riscv/attribute-14d.d: Likewise.
	* testsuite/gas/riscv/attribute-14e.d: Likewise.
---
 gas/config/tc-riscv.c                   | 48 ++++++++++++++++++++++++++++-----
 gas/testsuite/gas/riscv/attribute-11.s  |  2 ++
 gas/testsuite/gas/riscv/attribute-14.s  | 19 +++++++++++++
 gas/testsuite/gas/riscv/attribute-14a.d |  8 ++++++
 gas/testsuite/gas/riscv/attribute-14b.d |  8 ++++++
 gas/testsuite/gas/riscv/attribute-14c.d |  8 ++++++
 gas/testsuite/gas/riscv/attribute-14d.d |  8 ++++++
 gas/testsuite/gas/riscv/attribute-14e.d |  8 ++++++
 8 files changed, 103 insertions(+), 6 deletions(-)
 create mode 100644 gas/testsuite/gas/riscv/attribute-14.s
 create mode 100644 gas/testsuite/gas/riscv/attribute-14a.d
 create mode 100644 gas/testsuite/gas/riscv/attribute-14b.d
 create mode 100644 gas/testsuite/gas/riscv/attribute-14c.d
 create mode 100644 gas/testsuite/gas/riscv/attribute-14d.d
 create mode 100644 gas/testsuite/gas/riscv/attribute-14e.d

-- 
2.7.4

Comments

Nelson Chu June 22, 2020, 8:29 a.m. | #1
Thanks for all your feedback, I update what you suggested in this v2 patch.

> +/* Return True if it is a privileged instruction.  Otherwise, return FALSE.

> +

> +   uret is actually a N-ext instruction.  So it is better to regard it as

> +   an user instruction rather than the priv instruction.

> +

> +   hret is used to return from traps in H-mode.  H-mode is removed since

> +   the v1.10 priv spec, but probably be added in the new hypervisor spec.

> +   Therefore, hret should be controlled by the hypervisor spec rather than

> +   priv spec in the future.

> +

> +   dret is defined in the debug spec, so it should be checked in the future,

> +   too.  */

> +

> +static bfd_boolean

> +riscv_is_priv_insn (insn_t insn)

> +{

> +  return (((insn ^ MATCH_SRET) & MASK_SRET) == 0

> +         || ((insn ^ MATCH_MRET) & MASK_MRET) == 0

> +         || ((insn ^ MATCH_SFENCE_VMA) & MASK_SFENCE_VMA) == 0

> +         || ((insn ^ MATCH_WFI) & MASK_WFI) == 0

> +  /* The sfence.vm is dropped in the v1.10 priv specs, but we still need to

> +     check it here to keep the compatible.  Maybe we should issue warning

> +     if sfence.vm is used, but the priv spec newer than v1.10 is choosen.

> +     We aleady have a similar check for CSR, but not yet for instructions.

> +     It would be good if we could check the spec versions both for CSR and

> +     instructions, but not here.  */

> +         || ((insn ^ MATCH_SFENCE_VM) & MASK_SFENCE_VM) == 0);

> +}


* Some typo I will fix when committing. "aleady -> already". "choosen
-> chosen".

* hret is actually a hypervisor instruction, so I think it should be
controlled by the new hypervisor specs rather than the priv spec.

* I also remove uret instruction in the riscv_is_priv_insn since it
should be a N-ext instruction.

* Two options when handling sfence.vm in the future,
1. Encode the max/min priv specs in the elf attributes.
2. Check the spec versions, including ISA , priv and debug specs, both
for CSR and instruction.  We already have the similar check for CSR on
upstream, so maybe we could add the check for instruction in the
future.


>             case 'E':           /* Control register.  */

>               insn_with_csr = TRUE;

> -             explicit_csr = TRUE;

> +             explicit_priv_attr = TRUE;

>               if (reg_lookup (&s, RCLASS_CSR, &regno))

>                 INSERT_OPERAND (CSR, *ip, regno);

>               else


This will be updated when I support the unprivileged CSR in the future
patches.  The unprivileged CSR should be controlled by the other specs
rather than the priv spec, so the explicit_priv_attr shouldn't be set.
Besides, we haven't checked the CSR_CLASS when the CSR is used by the
CSR address directly, rather than the CSR name.

Thanks
Nelson
Jim Wilson June 22, 2020, 6:09 p.m. | #2
On Mon, Jun 22, 2020 at 1:11 AM Nelson Chu <nelson.chu@sifive.com> wrote:
>         gas/

>         * config/tc-riscv.c (explicit_priv_attr): Rename explicit_csr to

>         explicit_priv_attr.  It used to indicate CSR or priv instructions are

>         explictly used.

>         ...


OK.

Jim
Nelson Chu June 23, 2020, 1:40 a.m. | #3
Committed.  Thanks.
Nelson

On Tue, Jun 23, 2020 at 2:09 AM Jim Wilson <jimw@sifive.com> wrote:
>

> On Mon, Jun 22, 2020 at 1:11 AM Nelson Chu <nelson.chu@sifive.com> wrote:

> >         gas/

> >         * config/tc-riscv.c (explicit_priv_attr): Rename explicit_csr to

> >         explicit_priv_attr.  It used to indicate CSR or priv instructions are

> >         explictly used.

> >         ...

>

> OK.

>

> Jim

Patch

diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
index b6c8c4e..9ac31d0 100644
--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -356,8 +356,8 @@  static bfd_boolean start_assemble = FALSE;
 /* Indicate ELF attributes are explictly set.  */
 static bfd_boolean explicit_attr = FALSE;
 
-/* Indicate CSR are explictly used.  */
-static bfd_boolean explicit_csr = FALSE;
+/* Indicate CSR or priv instructions are explictly used.  */
+static bfd_boolean explicit_priv_attr = FALSE;
 
 /* Macros for encoding relaxation state for RVC branches and far jumps.  */
 #define RELAX_BRANCH_ENCODE(uncond, rvc, length)	\
@@ -1766,6 +1766,35 @@  riscv_csr_read_only_check (insn_t insn)
   return TRUE;
 }
 
+/* Return True if it is a privileged instruction.  Otherwise, return FALSE.
+
+   uret is actually a N-ext instruction.  So it is better to regard it as
+   an user instruction rather than the priv instruction.
+
+   hret is used to return from traps in H-mode.  H-mode is removed since
+   the v1.10 priv spec, but probably be added in the new hypervisor spec.
+   Therefore, hret should be controlled by the hypervisor spec rather than
+   priv spec in the future.
+
+   dret is defined in the debug spec, so it should be checked in the future,
+   too.  */
+
+static bfd_boolean
+riscv_is_priv_insn (insn_t insn)
+{
+  return (((insn ^ MATCH_SRET) & MASK_SRET) == 0
+	  || ((insn ^ MATCH_MRET) & MASK_MRET) == 0
+	  || ((insn ^ MATCH_SFENCE_VMA) & MASK_SFENCE_VMA) == 0
+	  || ((insn ^ MATCH_WFI) & MASK_WFI) == 0
+  /* The sfence.vm is dropped in the v1.10 priv specs, but we still need to
+     check it here to keep the compatible.  Maybe we should issue warning
+     if sfence.vm is used, but the priv spec newer than v1.10 is choosen.
+     We aleady have a similar check for CSR, but not yet for instructions.
+     It would be good if we could check the spec versions both for CSR and
+     instructions, but not here.  */
+	  || ((insn ^ MATCH_SFENCE_VM) & MASK_SFENCE_VM) == 0);
+}
+
 /* This routine assembles an instruction into its binary format.  As a
    side effect, it sets the global variable imm_reloc to the type of
    relocation to do if one of the operands is an address expression.  */
@@ -1833,6 +1862,9 @@  riscv_ip (char *str, struct riscv_cl_insn *ip, expressionS *imm_expr,
 		      && !riscv_opts.rvc)
 		    break;
 
+		  if (riscv_is_priv_insn (ip->insn_opcode))
+		    explicit_priv_attr = TRUE;
+
 		  /* Check if we write a read-only CSR by the CSR
 		     instruction.  */
 		  if (insn_with_csr
@@ -2197,7 +2229,7 @@  riscv_ip (char *str, struct riscv_cl_insn *ip, expressionS *imm_expr,
 
 	    case 'E':		/* Control register.  */
 	      insn_with_csr = TRUE;
-	      explicit_csr = TRUE;
+	      explicit_priv_attr = TRUE;
 	      if (reg_lookup (&s, RCLASS_CSR, &regno))
 		INSERT_OPERAND (CSR, *ip, regno);
 	      else
@@ -3591,9 +3623,13 @@  riscv_write_out_attrs (void)
       && !riscv_set_default_priv_spec (NULL))
     return;
 
-  /* If we already have set elf priv attributes, then generate them.
-     Otherwise, don't generate them when no CSR are used.  */
-  if (!explicit_csr)
+  /* If we already have set elf priv attributes, then no need to do anything,
+     assembler will generate them according to what you set.  Otherwise, don't
+     generate or update them when no CSR and priv instructions are used.
+     Generate the priv attributes according to default_priv_spec, which can be
+     set by -mpriv-spec and --with-priv-spec, and be updated by the original
+     priv attribute sets.  */
+  if (!explicit_priv_attr)
     return;
 
   /* Re-write priv attributes by default_priv_spec.  */
diff --git a/gas/testsuite/gas/riscv/attribute-11.s b/gas/testsuite/gas/riscv/attribute-11.s
index 81099b2..81367da 100644
--- a/gas/testsuite/gas/riscv/attribute-11.s
+++ b/gas/testsuite/gas/riscv/attribute-11.s
@@ -1 +1,3 @@ 
+	# The csr is explicit used, so we need to generate the priv spec
+	# attributes even if the attributes are not set.
 	csrr a0, 0x0
diff --git a/gas/testsuite/gas/riscv/attribute-14.s b/gas/testsuite/gas/riscv/attribute-14.s
new file mode 100644
index 0000000..ddda6b9
--- /dev/null
+++ b/gas/testsuite/gas/riscv/attribute-14.s
@@ -0,0 +1,19 @@ 
+	# The priv instruction is explicit used, so we need to generate
+	# the priv spec attributes even if the attributes are not set.
+.ifdef priv_insn_a
+	mret
+.endif
+.ifdef priv_insn_b
+	sret
+.endif
+.ifdef priv_insn_c
+	wfi
+.endif
+.ifdef priv_insn_d
+	sfence.vma
+.endif
+
+	# Obselete priv instruction.
+.ifdef priv_insn_e
+	sfence.vm
+.endif
diff --git a/gas/testsuite/gas/riscv/attribute-14a.d b/gas/testsuite/gas/riscv/attribute-14a.d
new file mode 100644
index 0000000..7b66f0f
--- /dev/null
+++ b/gas/testsuite/gas/riscv/attribute-14a.d
@@ -0,0 +1,8 @@ 
+#as: -march-attr --defsym priv_insn_a=1
+#readelf: -A
+#source: attribute-14.s
+Attribute Section: riscv
+File Attributes
+  Tag_RISCV_arch: [a-zA-Z0-9_\"].*
+  Tag_RISCV_priv_spec: [0-9_\"].*
+#...
diff --git a/gas/testsuite/gas/riscv/attribute-14b.d b/gas/testsuite/gas/riscv/attribute-14b.d
new file mode 100644
index 0000000..e044f4f
--- /dev/null
+++ b/gas/testsuite/gas/riscv/attribute-14b.d
@@ -0,0 +1,8 @@ 
+#as: -march-attr --defsym priv_insn_b=1
+#readelf: -A
+#source: attribute-14.s
+Attribute Section: riscv
+File Attributes
+  Tag_RISCV_arch: [a-zA-Z0-9_\"].*
+  Tag_RISCV_priv_spec: [0-9_\"].*
+#...
diff --git a/gas/testsuite/gas/riscv/attribute-14c.d b/gas/testsuite/gas/riscv/attribute-14c.d
new file mode 100644
index 0000000..6311a9d
--- /dev/null
+++ b/gas/testsuite/gas/riscv/attribute-14c.d
@@ -0,0 +1,8 @@ 
+#as: -march-attr --defsym priv_insn_c=1
+#readelf: -A
+#source: attribute-14.s
+Attribute Section: riscv
+File Attributes
+  Tag_RISCV_arch: [a-zA-Z0-9_\"].*
+  Tag_RISCV_priv_spec: [0-9_\"].*
+#...
diff --git a/gas/testsuite/gas/riscv/attribute-14d.d b/gas/testsuite/gas/riscv/attribute-14d.d
new file mode 100644
index 0000000..f0f2d63
--- /dev/null
+++ b/gas/testsuite/gas/riscv/attribute-14d.d
@@ -0,0 +1,8 @@ 
+#as: -march-attr --defsym priv_insn_d=1
+#readelf: -A
+#source: attribute-14.s
+Attribute Section: riscv
+File Attributes
+  Tag_RISCV_arch: [a-zA-Z0-9_\"].*
+  Tag_RISCV_priv_spec: [0-9_\"].*
+#...
diff --git a/gas/testsuite/gas/riscv/attribute-14e.d b/gas/testsuite/gas/riscv/attribute-14e.d
new file mode 100644
index 0000000..47fdc2e
--- /dev/null
+++ b/gas/testsuite/gas/riscv/attribute-14e.d
@@ -0,0 +1,8 @@ 
+#as: -march-attr --defsym priv_insn_e=1
+#readelf: -A
+#source: attribute-14.s
+Attribute Section: riscv
+File Attributes
+  Tag_RISCV_arch: [a-zA-Z0-9_\"].*
+  Tag_RISCV_priv_spec: [0-9_\"].*
+#...