diff mbox series

[v6,1/2] opcodes: microblaze: Add new bit-field instructions

Message ID 20231013072856.638728-1-neal.frager@amd.com
State New
Headers show
Series [v6,1/2] opcodes: microblaze: Add new bit-field instructions | expand

Commit Message

Neal Frager Oct. 13, 2023, 7:28 a.m. UTC
This patches adds new bsefi and bsifi instructions.
BSEFI- The instruction shall extract a bit field from a
register and place it right-adjusted in the destination register.
The other bits in the destination register shall be set to zero.
BSIFI- The instruction shall insert a right-adjusted bit field
from a register at another position in the destination register.
The rest of the bits in the destination register shall be unchanged.

Further documentation of these instructions can be found here:
https://docs.xilinx.com/v/u/en-US/ug984-vivado-microblaze-ref

With version 6 of the patch, no new relocation types are added as
this was unnecessary for adding the bsefi and bsifi instructions.

By removing any changes to relocation, there are no longer any
regressions to the binutils testsuite.  Both before this patch
and after lead to the following testsuite results with the
following binutils configure command:

./configure --disable-nls --disable-gdb --disable-gdbserver \
            --disable-gprofng --disable-libbacktrace \
            --disable-libdecnumber --disable-readline \
            --disable-sim --enable-obsolete --enable-plugins \
            --build=powerpc64le-linux --target=microblaze-xilinx-elf

		=== binutils Summary ===

 # of expected passes		220
 # of expected failures		2
 # of untested testcases		17
 # of unsupported tests		14

		=== gas Summary ===

 # of expected passes		272
 # of unexpected failures	1
 # of expected failures		1
 # of unsupported tests		8

		=== ld Summary ===

 # of expected passes		379
 # of unexpected failures	4
 # of expected failures		13
 # of untested testcases		26
 # of unsupported tests		217

		=== libctf Summary ===

 # of expected passes		5
 # of unsupported tests		3

		=== libsframe Summary ===

 # of expected passes		57

Signed-off-by: nagaraju <nagaraju.mekala@amd.com>
Signed-off-by: Ibai Erkiaga <ibai.erkiaga-elorza@amd.com>
Signed-off-by: Neal Frager <neal.frager@amd.com>
---
V1->V2:
 -corrected relocation values for the linker
V2->V3:
 - fixed build issue for 32-bit hosts
 - added test cases for bsefi and bsifi instructions
V3->V4:
 - fixed GNU coding standard issues
V4->V5:
 - fixed a remaining line of code > 80 chars
V5->V6:
 - removed unnecessary relocation changes
---
 gas/config/tc-microblaze.c | 83 +++++++++++++++++++++++++++++++++++++-
 opcodes/microblaze-dis.c   | 23 +++++++++++
 opcodes/microblaze-opc.h   | 11 ++++-
 opcodes/microblaze-opcm.h  |  6 ++-
 4 files changed, 120 insertions(+), 3 deletions(-)

Comments

Michael Eager Oct. 15, 2023, 4:30 p.m. UTC | #1
Neal --

When I run the test suite I see many segfaults.

Take a look at
gas/config/tc-microblaze.c +418:
md_begin() ...
   /* Insert unique names into hash table.  */
   for (opcode = microblaze_opcodes; opcode->name; opcode ++)
     str_hash_insert (opcode_hash_control, opcode->name, opcode, 0);

compare the end of the microblaze_opcodes table:
binutils/opcodes/microblaze-opc.h +427
microblaze_opcodes[] ...
   {"", 0, 0, 0, 0, 0, 0, 0, 0},

It looks like there is a mismatch on termination condition.

I changed this to
   {NULL, 0, 0, 0, 0, 0, 0, 0, 0},

I'm at a loss to explain why this did not fail earlier.

On 10/13/23 00:28, Neal Frager wrote:
> This patches adds new bsefi and bsifi instructions.
> BSEFI- The instruction shall extract a bit field from a
> register and place it right-adjusted in the destination register.
> The other bits in the destination register shall be set to zero.
> BSIFI- The instruction shall insert a right-adjusted bit field
> from a register at another position in the destination register.
> The rest of the bits in the destination register shall be unchanged.

...

> diff --git a/opcodes/microblaze-dis.c b/opcodes/microblaze-dis.c
> index 12981abfea1..b2d3f19337c 100644
> --- a/opcodes/microblaze-dis.c
> +++ b/opcodes/microblaze-dis.c
> @@ -427,6 +442,14 @@ print_insn_microblaze (bfd_vma memaddr, struct disassemble_info * info)
>   	  /* For mbar 16 or sleep insn.  */
>   	case INST_TYPE_NONE:
>   	  break;
> +	  /* For bit field insns.  */
> +	case INST_TYPE_RD_R1_IMMW_IMMS:
> +	  print_func (stream, "\t%s, %s, %s, %s",
> +			get_field_rd (&buf, inst),
> +			get_field_r1 (&buf, inst),
> +			get_field_immw (&buf, inst),
> +			get_field_imm5 (&buf, inst));
> +	  break;

Fixed indent.

Trimmed commit message.

Committed.
Neal Frager Oct. 15, 2023, 4:48 p.m. UTC | #2
Hi Michael,

> When I run the test suite I see many segfaults.

> Take a look at
> gas/config/tc-microblaze.c +418:
> md_begin() ...
>   /* Insert unique names into hash table.  */
>   for (opcode = microblaze_opcodes; opcode->name; opcode ++)
>     str_hash_insert (opcode_hash_control, opcode->name, opcode, 0);

> compare the end of the microblaze_opcodes table:
> binutils/opcodes/microblaze-opc.h +427
> microblaze_opcodes[] ...
>   {"", 0, 0, 0, 0, 0, 0, 0, 0},

> It looks like there is a mismatch on termination condition.

> I changed this to
>   {NULL, 0, 0, 0, 0, 0, 0, 0, 0},

> I'm at a loss to explain why this did not fail earlier.

I believe I know why this did not fail earlier.

In the past, the array size was larger than needed with a value of MAX_OPCODES that was larger than the number of elements in the array.

As part of this patch, I reduced the MAX_OPCODES to the exact number of elements in the array.

-#define MAX_OPCODES 300
+#define MAX_OPCODES 291

So in the past, there were extra NULL elements beyond the end of the array.

In any case, thank you for your review, fixing this issue and committing the patch!

Best regards,
Neal Frager
AMD
Alan Modra Oct. 19, 2023, 3:40 a.m. UTC | #3
On Sun, Oct 15, 2023 at 04:48:29PM +0000, Frager, Neal wrote:
> In any case, thank you for your review, fixing this issue and committing the patch!

Commit bb0d05ff74fd caused
FAIL: objdump -S
FAIL: objdump --source-comment
on microblaze-linux-gnu.
Alan Modra Oct. 19, 2023, 3:53 a.m. UTC | #4
On Thu, Oct 19, 2023 at 02:10:41PM +1030, Alan Modra wrote:
> On Sun, Oct 15, 2023 at 04:48:29PM +0000, Frager, Neal wrote:
> > In any case, thank you for your review, fixing this issue and committing the patch!
> 
> Commit bb0d05ff74fd caused
> FAIL: objdump -S
> FAIL: objdump --source-comment
> on microblaze-linux-gnu.

Assembling and attempting to disassemble this
 .text
 .long 0x65005f5f
should give something to look at.  I see

$ binutils/objdump -d xxx.o
xxx.o:     file format elf32-microblaze


Disassembly of section .text:

00000000 <.text>:
Aborted (core dumped)
Neal Frager Oct. 19, 2023, 6:40 a.m. UTC | #5
Hi Alan,


> Le 19 oct. 2023 à 05:53, Alan Modra <amodra@gmail.com> a écrit :
> 
> On Thu, Oct 19, 2023 at 02:10:41PM +1030, Alan Modra wrote:
>>> On Sun, Oct 15, 2023 at 04:48:29PM +0000, Frager, Neal wrote:
>>> In any case, thank you for your review, fixing this issue and committing the patch!
>> 
>> Commit bb0d05ff74fd caused
>> FAIL: objdump -S
>> FAIL: objdump --source-comment
>> on microblaze-linux-gnu.
> 
> Assembling and attempting to disassemble this
> .text
> .long 0x65005f5f
> should give something to look at.  I see
> 
> $ binutils/objdump -d xxx.o
> xxx.o:     file format elf32-microblaze
> 
> 
> Disassembly of section .text:
> 
> 00000000 <.text>:
> Aborted (core dumped)
> 

Thank you for sharing this.  I believe I know the root cause.  These new instructions start with the most significant byte 0x64.  I think the disassembler code is ignoring bit 24 and treating 0x65 as an instruction with bad parameters.

I am confident I can fix it.  I just need to get in front my a PC.

I will send you a patch that fixes this shortly.

Thanks for sharing this!

Best regards,
Neal Frager
AMD

> 
> --
> Alan Modra
> Australia Development Lab, IBM
Alan Modra Oct. 19, 2023, 10:23 a.m. UTC | #6
On Thu, Oct 19, 2023 at 06:40:31AM +0000, Frager, Neal wrote:
> I am confident I can fix it.  I just need to get in front my a PC.
> 
> I will send you a patch that fixes this shortly.

Yes, it is an easy and obvious patch.  ;-)
Neal Frager Oct. 19, 2023, 1:20 p.m. UTC | #7
Hi Alan,

> > In any case, thank you for your review, fixing this issue and committing the patch!
> 
> Commit bb0d05ff74fd caused
> FAIL: objdump -S
> FAIL: objdump --source-comment
> on microblaze-linux-gnu.

> Assembling and attempting to disassemble this  .text  .long 0x65005f5f should give something to look at.  I see

> $ binutils/objdump -d xxx.o
>xxx.o:     file format elf32-microblaze


> Disassembly of section .text:

> 00000000 <.text>:
> Aborted (core dumped)

Could you please test the following patch?  On my side, it resolves the issue you are reporting.

https://patchwork.sourceware.org/project/binutils/patch/20231019113740.2071556-1-neal.frager@amd.com/

Thank you for finding and reporting this issue!

Best regards,
Neal Frager
AMD
Alan Modra Oct. 19, 2023, 9:30 p.m. UTC | #8
On Thu, Oct 19, 2023 at 01:20:03PM +0000, Frager, Neal wrote:
> Could you please test the following patch?  On my side, it resolves the issue you are reporting.
> 
> https://patchwork.sourceware.org/project/binutils/patch/20231019113740.2071556-1-neal.frager@amd.com/

It does.  I saw the email yesterday and thought, "good, he's fixed the
obvious NUM_STRBUFS bug which was the source of the abort, and also
the .short thing".
Neal Frager Oct. 20, 2023, 6:28 a.m. UTC | #9
Hi Alan,


> Le 19 oct. 2023 à 23:30, Alan Modra <amodra@gmail.com> a écrit :
> 
> On Thu, Oct 19, 2023 at 01:20:03PM +0000, Frager, Neal wrote:
>> Could you please test the following patch?  On my side, it resolves the issue you are reporting.
>> 
>> https://patchwork.sourceware.org/project/binutils/patch/20231019113740.2071556-1-neal.frager@amd.com/
> 
> It does.  I saw the email yesterday and thought, "good, he's fixed the
> obvious NUM_STRBUFS bug which was the source of the abort, and also
> the .short thing".
> 
> --
> Alan Modra
> Australia Development Lab, IBM

The original patch I submitted was just copy pasting from our meta-xilinx patch set.  I am actually quite surprised such a bug exists since AMD has been shipping a microblaze toolchain with this bug in it for a few years now.

And now that I am looking more closely at the microblaze-opc.h file, I think there are probably quite a few issues with disassembly and the objdump utility which need fixing.

If you catch any other issues, please let me know, as I would like to get this cleaned up.

Best regards,
Neal Frager
AMD
diff mbox series

Patch

diff --git a/gas/config/tc-microblaze.c b/gas/config/tc-microblaze.c
index d900a9e1d05..1703e95c452 100644
--- a/gas/config/tc-microblaze.c
+++ b/gas/config/tc-microblaze.c
@@ -915,7 +915,7 @@  md_assemble (char * str)
   unsigned reg2;
   unsigned reg3;
   unsigned isize;
-  unsigned int immed = 0, temp;
+  unsigned int immed = 0, immed2 = 0, temp;
   expressionS exp;
   char name[20];
 
@@ -1177,6 +1177,87 @@  md_assemble (char * str)
       inst |= (immed << IMM_LOW) & IMM5_MASK;
       break;
 
+    case INST_TYPE_RD_R1_IMMW_IMMS:
+      if (strcmp (op_end, ""))
+	op_end = parse_reg (op_end + 1, &reg1);  /* Get rd.  */
+      else
+	{
+	  as_fatal (_("Error in statement syntax"));
+	  reg1 = 0;
+	}
+
+      if (strcmp (op_end, ""))
+	op_end = parse_reg (op_end + 1, &reg2);  /* Get r1.  */
+      else
+	{
+	  as_fatal (_("Error in statement syntax"));
+	  reg2 = 0;
+	}
+
+      /* Check for spl registers.  */
+      if (check_spl_reg (&reg1))
+	as_fatal (_("Cannot use special register with this instruction"));
+      if (check_spl_reg (&reg2))
+	as_fatal (_("Cannot use special register with this instruction"));
+
+      /* Width immediate value.  */
+      if (strcmp (op_end, ""))
+	op_end = parse_imm (op_end + 1, &exp, MIN_IMM_WIDTH, MAX_IMM_WIDTH);
+      else
+	as_fatal (_("Error in statement syntax"));
+
+      if (exp.X_op != O_constant)
+	{
+	  as_warn (_(
+	  "Symbol used as immediate width value for bit field instruction"));
+	  immed = 1;
+	}
+      else
+	immed = exp.X_add_number;
+	
+      if (opcode->instr == bsefi && immed > 31)
+	as_fatal (_("Width value must be less than 32"));
+
+      /* Shift immediate value.  */
+      if (strcmp (op_end, ""))
+	op_end = parse_imm (op_end + 1, &exp, MIN_IMM, MAX_IMM);
+      else
+	as_fatal (_("Error in statement syntax"));
+
+      if (exp.X_op != O_constant)
+	{
+	  as_warn (_(
+	  "Symbol used as immediate shift value for bit field instruction"));
+	  immed2 = 0;
+	}
+      else
+	{
+	  output = frag_more (isize);
+	  immed2 = exp.X_add_number;
+	}
+
+      if (immed2 != (immed2 % 32))
+	{
+	  as_warn (_("Shift value greater than 32. using <value %% 32>"));
+	  immed2 = immed2 % 32;
+	}
+
+      /* Check combined value.  */
+      if (immed + immed2 > 32)
+	as_fatal (_("Width value + shift value must not be greater than 32"));
+
+      inst |= (reg1 << RD_LOW) & RD_MASK;
+      inst |= (reg2 << RA_LOW) & RA_MASK;
+
+      if (opcode->instr == bsefi)
+	inst |= (immed & IMM5_MASK) << IMM_WIDTH_LOW; /* bsefi */
+      else
+	inst |= ((immed + immed2 - 1) & IMM5_MASK)
+		<< IMM_WIDTH_LOW; /* bsifi */
+
+      inst |= (immed2 << IMM_LOW) & IMM5_MASK;
+      break;
+
     case INST_TYPE_R1_R2:
       if (strcmp (op_end, ""))
         op_end = parse_reg (op_end + 1, &reg1);  /* Get r1.  */
diff --git a/opcodes/microblaze-dis.c b/opcodes/microblaze-dis.c
index 12981abfea1..b2d3f19337c 100644
--- a/opcodes/microblaze-dis.c
+++ b/opcodes/microblaze-dis.c
@@ -90,6 +90,21 @@  get_field_imm5_mbar (struct string_buf *buf, long instr)
   return p;
 }
 
+static char *
+get_field_immw (struct string_buf *buf, long instr)
+{
+  char *p = strbuf (buf);
+
+  if (instr & 0x00004000)
+    sprintf (p, "%d", (short)(((instr & IMM5_WIDTH_MASK)
+				>> IMM_WIDTH_LOW))); /* bsefi */
+  else
+    sprintf (p, "%d", (short)(((instr & IMM5_WIDTH_MASK) >>
+				IMM_WIDTH_LOW) - ((instr & IMM5_MASK) >>
+				IMM_LOW) + 1)); /* bsifi */
+  return p;
+}
+
 static char *
 get_field_rfsl (struct string_buf *buf, long instr)
 {
@@ -427,6 +442,14 @@  print_insn_microblaze (bfd_vma memaddr, struct disassemble_info * info)
 	  /* For mbar 16 or sleep insn.  */
 	case INST_TYPE_NONE:
 	  break;
+	  /* For bit field insns.  */
+	case INST_TYPE_RD_R1_IMMW_IMMS:
+	  print_func (stream, "\t%s, %s, %s, %s",
+			get_field_rd (&buf, inst),
+			get_field_r1 (&buf, inst),
+			get_field_immw (&buf, inst),
+			get_field_imm5 (&buf, inst));
+	  break;
 	  /* For tuqula instruction */
 	case INST_TYPE_RD:
 	  print_func (stream, "\t%s", get_field_rd (&buf, inst));
diff --git a/opcodes/microblaze-opc.h b/opcodes/microblaze-opc.h
index 7398e9e246a..19bbbdaf68e 100644
--- a/opcodes/microblaze-opc.h
+++ b/opcodes/microblaze-opc.h
@@ -59,6 +59,9 @@ 
 /* For mbar.  */
 #define INST_TYPE_IMM5 20
 
+/* For bsefi and bsifi */
+#define INST_TYPE_RD_R1_IMMW_IMMS  21
+
 #define INST_TYPE_NONE 25
 
 
@@ -90,6 +93,7 @@ 
 #define OPCODE_MASK_H1234 0xFFFFFFFF /* All 32 bits.  */
 #define OPCODE_MASK_H3    0xFC000600 /* High 6 bits and bits 21, 22.  */
 #define OPCODE_MASK_H32   0xFC00FC00 /* High 6 bits and bit 16-21.  */
+#define OPCODE_MASK_H32B  0xFC00C000 /* High 6 bits and bit 16, 17.  */
 #define OPCODE_MASK_H34B  0xFC0000FF /* High 6 bits and low 8 bits.  */
 #define OPCODE_MASK_H35B  0xFC0004FF /* High 6 bits and low 9 bits.  */
 #define OPCODE_MASK_H34C  0xFC0007E0 /* High 6 bits and bits 21-26.  */
@@ -102,7 +106,7 @@ 
 #define DELAY_SLOT 1
 #define NO_DELAY_SLOT 0
 
-#define MAX_OPCODES 300
+#define MAX_OPCODES 291
 
 const struct op_code_struct
 {
@@ -159,6 +163,8 @@  const struct op_code_struct
   {"bslli", INST_TYPE_RD_R1_IMM5, INST_NO_OFFSET, NO_DELAY_SLOT, IMMVAL_MASK_NON_SPECIAL, 0x64000400, OPCODE_MASK_H3, bslli, barrel_shift_inst },
   {"bsrai", INST_TYPE_RD_R1_IMM5, INST_NO_OFFSET, NO_DELAY_SLOT, IMMVAL_MASK_NON_SPECIAL, 0x64000200, OPCODE_MASK_H3, bsrai, barrel_shift_inst },
   {"bsrli", INST_TYPE_RD_R1_IMM5, INST_NO_OFFSET, NO_DELAY_SLOT, IMMVAL_MASK_NON_SPECIAL, 0x64000000, OPCODE_MASK_H3, bsrli, barrel_shift_inst },
+  {"bsefi", INST_TYPE_RD_R1_IMMW_IMMS, INST_NO_OFFSET, NO_DELAY_SLOT, IMMVAL_MASK_NON_SPECIAL, 0x64004000, OPCODE_MASK_H32B, bsefi, barrel_shift_inst },
+  {"bsifi", INST_TYPE_RD_R1_IMMW_IMMS, INST_NO_OFFSET, NO_DELAY_SLOT, IMMVAL_MASK_NON_SPECIAL, 0x64008000, OPCODE_MASK_H32B, bsifi, barrel_shift_inst },
   {"or",    INST_TYPE_RD_R1_R2, INST_NO_OFFSET, NO_DELAY_SLOT, IMMVAL_MASK_NON_SPECIAL, 0x80000000, OPCODE_MASK_H4, microblaze_or, logical_inst },
   {"and",   INST_TYPE_RD_R1_R2, INST_NO_OFFSET, NO_DELAY_SLOT, IMMVAL_MASK_NON_SPECIAL, 0x84000000, OPCODE_MASK_H4, microblaze_and, logical_inst },
   {"xor",   INST_TYPE_RD_R1_R2, INST_NO_OFFSET, NO_DELAY_SLOT, IMMVAL_MASK_NON_SPECIAL, 0x88000000, OPCODE_MASK_H4, microblaze_xor, logical_inst },
@@ -438,5 +444,8 @@  char pvr_register_prefix[] = "rpvr";
 #define MIN_IMM5  ((int) 0x00000000)
 #define MAX_IMM5  ((int) 0x0000001f)
 
+#define MIN_IMM_WIDTH  ((int) 0x00000001)
+#define MAX_IMM_WIDTH  ((int) 0x00000020)
+
 #endif /* MICROBLAZE_OPC */
 
diff --git a/opcodes/microblaze-opcm.h b/opcodes/microblaze-opcm.h
index c91b002d951..3c4f8948c76 100644
--- a/opcodes/microblaze-opcm.h
+++ b/opcodes/microblaze-opcm.h
@@ -29,7 +29,7 @@  enum microblaze_instr
   addi, rsubi, addic, rsubic, addik, rsubik, addikc, rsubikc, mul,
   mulh, mulhu, mulhsu, swapb, swaph,
   idiv, idivu, bsll, bsra, bsrl, get, put, nget, nput, cget, cput,
-  ncget, ncput, muli, bslli, bsrai, bsrli, mului,
+  ncget, ncput, muli, bslli, bsrai, bsrli, bsefi, bsifi, mului,
   /* 'or/and/xor' are C++ keywords.  */
   microblaze_or, microblaze_and, microblaze_xor,
   andn, pcmpbf, pcmpbc, pcmpeq, pcmpne, sra, src, srl, sext8, sext16,
@@ -130,6 +130,7 @@  enum microblaze_instr_type
 #define RB_LOW  11 /* Low bit for RB.  */
 #define IMM_LOW  0 /* Low bit for immediate.  */
 #define IMM_MBAR 21 /* low bit for mbar instruction.  */
+#define IMM_WIDTH_LOW 6 /* Low bit for immediate width */
 
 #define RD_MASK 0x03E00000
 #define RA_MASK 0x001F0000
@@ -142,6 +143,9 @@  enum microblaze_instr_type
 /* Imm mask for mbar.  */
 #define IMM5_MBAR_MASK 0x03E00000
 
+/* Imm mask for extract/insert width. */
+#define IMM5_WIDTH_MASK 0x000007C0
+
 /* FSL imm mask for get, put instructions.  */
 #define  RFSL_MASK 0x000000F