[PATCH 1/2] [pr gdb/19447] sim: mips: Only truncate sign extension bits for 32-bit target models

[PATCH 1/2] [pr gdb/19447] sim: mips: Only truncate sign extension bits for 32-bit target models

[Faraz Shahbazker]

64-bit BFD for MIPS applies a standard sign extension on all addresses
assuming 64-bit target. These bits are required for 64-bit and can only be
safely truncated for 32-bit target models. This partially reverts commit
b36d953bced0a4fecdde1823abac70ed7038ee95

2021-04-23  Faraz Shahbazker  <fshahbazker@wavecomp.com>

sim/mips/ChangeLog:
	* interp.c (sim_create_inferior): Only truncate sign extension
	bits for 32-bit target models.
---

Notes:
    The sign-extension logic modeled by BFD is an integral part of the MIPS64
    architecture spec. It appears in the virtual address map, where sign extension
    allows for 32-bit compatibility segments [1] with 64-bit
    addressing. Truncating these addresses prematurely (commit
    #b36d953bced0a4fecdde1823abac70ed7038ee95) in PC space breaks 64-bit models
    (-DWITH_TARGET_WORD_BITSIZE=64).
    
    In the ISA itself, direct addressing (Load-Upper-Immediate) and indirect
    addressing (Load-Word) both automatically sign-extend their results. These
    instructions regenerate the sign-extended addresses even if we don't start
    with one. That's what causes the failures in pr gdb/19447.
    
    Moreover, some instructions like ADD*/SUB* have unpredictable behaviour when
    an operand is not correctly sign extended [3]. This affects PC-relative
    addressing in particular. So arithmetic on the link-address generated in the
    return address register by a jump-and-link is no longer possible, neither is
    the use of the PC-relative addressing instructions provided by MIPSR6. I am
    preparing upstream submission for R6, which is where I first encounted this
    problem.
    
    [1] "MIPS64 Architecture for Programmers Volume III: The MIPS64
        Privileged Resource Architecture", Document Number: MD00091,
        Revision 6.02, December 10, 2015, Section 4.3 "Virtual Address
        Spaces", pp. 29-31
    https://s3-eu-west-1.amazonaws.com/downloads-mips/documents/MD00091-2B-MIPS64PRA-AFP-06.03.pdf
    
    [2] "MIPS64 Architecture for Programmers Volume II-A: The MIPS64
        Instruction Set Reference Manual", Document Number: MD00087,
        Revision 6.06, December 15, 2016, Section 3.2 "Alphabetical
        List of Instructions", pp. 321
    https://s3-eu-west-1.amazonaws.com/downloads-mips/documents/MD00087-2B-MIPS64BIS-AFP-6.06.pdf
    
    [3] "MIPS64 Architecture for Programmers Volume II-A: The MIPS64
        Instruction Set Reference Manual", Document Number: MD00087,
        Revision 6.06, December 15, 2016, Section 3.2 "Alphabetical
        List of Instructions", pp. 56
    https://s3-eu-west-1.amazonaws.com/downloads-mips/documents/MD00087-2B-MIPS64BIS-AFP-6.06.pdf

 sim/mips/ChangeLog |  5 +++++
 sim/mips/interp.c  | 19 ++++++++-----------
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/sim/mips/ChangeLog b/sim/mips/ChangeLog
index 62b3b4cf2a6..5dd0dc8512f 100644
--- a/sim/mips/ChangeLog
+++ b/sim/mips/ChangeLog
@@ -1,3 +1,8 @@
+2021-05-04  Faraz Shahbazker  <fshahbazker@wavecomp.com>
+
+	* interp.c (sim_create_inferior): Only truncate sign extension
+	bits for 32-bit target models.
+
 2021-05-04  Mike Frysinger  <vapier@gentoo.org>
 
 	* configure: Regenerate.
diff --git a/sim/mips/interp.c b/sim/mips/interp.c
index 64259cc3fed..fc3a953f3b9 100644
--- a/sim/mips/interp.c
+++ b/sim/mips/interp.c
@@ -1012,17 +1012,14 @@ sim_create_inferior (SIM_DESC sd, struct bfd *abfd,
       for (cpu_nr = 0; cpu_nr < sim_engine_nr_cpus (sd); cpu_nr++)
 	{
 	  sim_cpu *cpu = STATE_CPU (sd, cpu_nr);
-	  sim_cia pc = bfd_get_start_address (abfd);
-
-	  /* We need to undo brain-dead bfd behavior where it sign-extends
-	     addresses that are supposed to be unsigned.  See the mips bfd
-	     sign_extend_vma setting.  We have to check the ELF data itself
-	     in order to handle o32 & n32 ABIs.  */
-	  if (abfd->tdata.elf_obj_data->elf_header->e_ident[EI_CLASS] ==
-	      ELFCLASS32)
-	    pc = (unsigned32) pc;
-
-	  CPU_PC_SET (cpu, pc);
+	  /* The 64-bit BFD sign-extends MIPS addresses to model
+	     32-bit compatibility segments with 64-bit addressing.
+	     These addresses work as is on 64-bit targets but
+	     can be truncated for 32-bit targets.  */
+	  if (WITH_TARGET_WORD_BITSIZE == 32)
+	    CPU_PC_SET (cpu, (unsigned32) bfd_get_start_address (abfd));
+	  else
+	    CPU_PC_SET (cpu, bfd_get_start_address (abfd));
 	}
     }
 
-- 
2.25.1

[PATCH 2/2] [pr gdb/19447] sim: mips: Add shadow mappings for 32-bit memory address space

[Faraz Shahbazker]

32-bit MIPS programs run on the 64-bit simulator model in 64-bit
sign-extended space. The mapping from 64-bit sign-extended addresses to
32-bit addresses was removed by commit
26f8bf63bf36f9062a5cc1afacf71462a4abe0c8, breaking the 64-bit simulator
model. Add shadow mappings from 64-bit sign extended address space to
32-bit address spaces, in lieu of the AddressTranslation function.

2021-05-04  Faraz Shahbazker  <fshahbazker@wavecomp.com>

sim/mips/ChangeLog:
	* interp.c (sim_open): Add shadow mappings from 32-bit
	address space to 64-bit sign-extended address space.
---
 sim/mips/ChangeLog |  5 +++++
 sim/mips/interp.c  | 12 ++++++++++--
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/sim/mips/ChangeLog b/sim/mips/ChangeLog
index 5dd0dc8512f..db88d64852e 100644
--- a/sim/mips/ChangeLog
+++ b/sim/mips/ChangeLog
@@ -1,3 +1,8 @@
+2021-05-04  Faraz Shahbazker  <fshahbazker@wavecomp.com>
+
+	* interp.c (sim_open): Add memory aliases from 64-bit
+	 sign-extended address space to 32-bit address space.
+
 2021-05-04  Faraz Shahbazker  <fshahbazker@wavecomp.com>
 
 	* interp.c (sim_create_inferior): Only truncate sign extension
diff --git a/sim/mips/interp.c b/sim/mips/interp.c
index fc3a953f3b9..e190a00ff38 100644
--- a/sim/mips/interp.c
+++ b/sim/mips/interp.c
@@ -438,6 +438,9 @@ sim_open (SIM_OPEN_KIND kind, host_callback *cb,
 	  /* memory alias K1BASE@1,K1SIZE%MEMSIZE,K0BASE */
 	  sim_do_commandf (sd, "memory alias 0x%lx@1,0x%lx%%0x%lx,0x%0x",
 			   K1BASE, K1SIZE, (long)mem_size, K0BASE);
+	  if (WITH_TARGET_WORD_BITSIZE == 64)
+	    sim_do_commandf (sd, "memory alias 0x%lx,0x%lx,0x%lx",
+			     (K0BASE), (long)mem_size, EXTENDED(K0BASE));
 	}
 
       device_init(sd);
@@ -699,8 +702,13 @@ sim_open (SIM_OPEN_KIND kind, host_callback *cb,
       unsigned idt_monitor_size = 1 << 11;
 
       /* the default monitor region */
-      sim_do_commandf (sd, "memory region 0x%x,0x%x",
-		       idt_monitor_base, idt_monitor_size);
+      if (WITH_TARGET_WORD_BITSIZE == 64)
+	sim_do_commandf (sd, "memory alias 0x%lx,0x%lx,0x%lx",
+			 (idt_monitor_base), (long)idt_monitor_size,
+			 EXTENDED (idt_monitor_base));
+      else
+	sim_do_commandf (sd, "memory region 0x%lx,0x%x",
+			 idt_monitor_base, idt_monitor_size);
 
       /* Entry into the IDT monitor is via fixed address vectors, and
 	 not using machine instructions. To avoid clashing with use of
-- 
2.25.1

Re: [PATCH 1/2] [pr gdb/19447] sim: mips: Only truncate sign extension bits for 32-bit target models

[Mike Frysinger]

there were 3 copies of these patches.  was there any diff between them,
or was it just an accident ?
-mike

Re: [EXTERNAL]Re: [PATCH 1/2] [pr gdb/19447] sim: mips: Only truncate sign extension bits for 32-bit target models

[Faraz Shahbazker]

Sorry about that. Just an accident, trying to figure out my mail
configuration.

Regards,
Faraz


On 5/12/21 3:26 AM, Mike Frysinger wrote:
> there were 3 copies of these patches.  was there any diff between them,
> or was it just an accident ?
> -mike
> 

Re: [PATCH 1/2] [pr gdb/19447] sim: mips: Only truncate sign extension bits for 32-bit target models

[Mike Frysinger]

On 05 May 2021 04:51, Faraz Shahbazker wrote:
> 64-bit BFD for MIPS applies a standard sign extension on all addresses
> assuming 64-bit target. These bits are required for 64-bit and can only be
> safely truncated for 32-bit target models. This partially reverts commit
> b36d953bced0a4fecdde1823abac70ed7038ee95

can you verify there are no regressions in `make check` for the various
mips targets & build configs ?

if the tests pass, then i'm fine with the patch.

> ---
> 
> Notes:

these notes should be part of the commit message for posterity

>  	  sim_cpu *cpu = STATE_CPU (sd, cpu_nr);
> -	  sim_cia pc = bfd_get_start_address (abfd);
> -
> -	  /* We need to undo brain-dead bfd behavior where it sign-extends
> -	     addresses that are supposed to be unsigned.  See the mips bfd
> -	     sign_extend_vma setting.  We have to check the ELF data itself
> -	     in order to handle o32 & n32 ABIs.  */
> -	  if (abfd->tdata.elf_obj_data->elf_header->e_ident[EI_CLASS] ==
> -	      ELFCLASS32)
> -	    pc = (unsigned32) pc;
> -
> -	  CPU_PC_SET (cpu, pc);
> +	  /* The 64-bit BFD sign-extends MIPS addresses to model
> +	     32-bit compatibility segments with 64-bit addressing.
> +	     These addresses work as is on 64-bit targets but
> +	     can be truncated for 32-bit targets.  */
> +	  if (WITH_TARGET_WORD_BITSIZE == 32)
> +	    CPU_PC_SET (cpu, (unsigned32) bfd_get_start_address (abfd));
> +	  else
> +	    CPU_PC_SET (cpu, bfd_get_start_address (abfd));

style-wise, it seems like you should have just changed the if() statement
and left the rest of the code alone.  it avoids duplicating the rest of the
logic.
-mike

Re: [PATCH 2/2] [pr gdb/19447] sim: mips: Add shadow mappings for 32-bit memory address space

[Mike Frysinger]

On 05 May 2021 04:51, Faraz Shahbazker wrote:
> --- a/sim/mips/interp.c
> +++ b/sim/mips/interp.c
> @@ -438,6 +438,9 @@ sim_open (SIM_OPEN_KIND kind, host_callback *cb,
>  	  /* memory alias K1BASE@1,K1SIZE%MEMSIZE,K0BASE */
>  	  sim_do_commandf (sd, "memory alias 0x%lx@1,0x%lx%%0x%lx,0x%0x",
>  			   K1BASE, K1SIZE, (long)mem_size, K0BASE);
> +	  if (WITH_TARGET_WORD_BITSIZE == 64)
> +	    sim_do_commandf (sd, "memory alias 0x%lx,0x%lx,0x%lx",
> +			     (K0BASE), (long)mem_size, EXTENDED(K0BASE));

instead of using %lx & casting to (long), use sim's PRIx* for the format.
	sim_do_commandf (sd, "memory alias 0x%x,0x%" PRIxTA ",0x%" PRIxTW,
		K0BASE, mem_size, EXTENDED(K0BASE));
-mike

Re: [PATCH 2/2] [pr gdb/19447] sim: mips: Add shadow mappings for 32-bit memory address space

[Faraz Shahbazker]

Tested on a bunch of MIPS configurations derived from 
sim/mips/configure, improves results accross the board for 64-bit 
builds. The 32-bit build is still broken for 64-bit simulator models.

[PATCH v2 1/2] [pr gdb/19447] sim: mips: Only truncate sign extension bits for 32-bit target models

[Faraz Shahbazker]

64-bit BFD for MIPS applies a standard sign extension on all addresses
assuming 64-bit target.  These bits are required for 64-bit and can only
be safely truncated for 32-bit target models. This partially reverts commit
b36d953bced0a4fecdde1823abac70ed7038ee95

The sign-extension logic modeled by BFD is an integral part of the
MIPS64 architecture spec. It appears in the virtual address map, where
sign extension allows for 32-bit compatibility segments [1] with 64-bit
addressing. Truncating these addresses prematurely (commit
models (-DWITH_TARGET_WORD_BITSIZE=64).

In the ISA itself, direct addressing (Load-Upper-Immediate) and indirect
addressing (Load-Word) both automatically sign-extend their results. These
instructions regenerate the sign-extended addresses even if we don't start
with one (see pr gdb/19447).

Moreover, some instructions like ADD*/SUB* have unpredictable behaviour when
an operand is not correctly sign extended [3]. This affects PC-relative
addressing in particular, so arithmetic on the link-address generated in the
return address register by a jump-and-link is no longer possible, neither is
the use of the PC-relative addressing instructions provided by MIPSR6.

[1] "MIPS64 Architecture for Programmers Volume III: The MIPS64
    Privileged Resource Architecture", Document Number: MD00091,
    Revision 6.02, December 10, 2015, Section 4.3 "Virtual Address
    Spaces", pp. 29-31
https://s3-eu-west-1.amazonaws.com/downloads-mips/documents/MD00091-2B-MIPS64PRA-AFP-06.03.pdf

[2] "MIPS64 Architecture for Programmers Volume II-A: The MIPS64
    Instruction Set Reference Manual", Document Number: MD00087,
    Revision 6.06, December 15, 2016, Section 3.2 "Alphabetical
    List of Instructions", pp. 321
https://s3-eu-west-1.amazonaws.com/downloads-mips/documents/MD00087-2B-MIPS64BIS-AFP-6.06.pdf

[3] "MIPS64 Architecture for Programmers Volume II-A: The MIPS64
    Instruction Set Reference Manual", Document Number: MD00087,
    Revision 6.06, December 15, 2016, Section 3.2 "Alphabetical
    List of Instructions", pp. 56
https://s3-eu-west-1.amazonaws.com/downloads-mips/documents/MD00087-2B-MIPS64BIS-AFP-6.06.pdf

2021-04-23  Faraz Shahbazker  <fshahbazker@wavecomp.com>

sim/mips/ChangeLog:
	* interp.c (sim_create_inferior): Only truncate sign extension
	bits for 32-bit target models.
---
 sim/mips/interp.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/sim/mips/interp.c b/sim/mips/interp.c
index 2839715959c..6e00fd0dbf0 100644
--- a/sim/mips/interp.c
+++ b/sim/mips/interp.c
@@ -1014,12 +1014,11 @@ sim_create_inferior (SIM_DESC sd, struct bfd *abfd,
 	  sim_cpu *cpu = STATE_CPU (sd, cpu_nr);
 	  sim_cia pc = bfd_get_start_address (abfd);
 
-	  /* We need to undo brain-dead bfd behavior where it sign-extends
-	     addresses that are supposed to be unsigned.  See the mips bfd
-	     sign_extend_vma setting.  We have to check the ELF data itself
-	     in order to handle o32 & n32 ABIs.  */
-	  if (abfd->tdata.elf_obj_data->elf_header->e_ident[EI_CLASS] ==
-	      ELFCLASS32)
+	  /* The 64-bit BFD sign-extends MIPS addresses to model
+	     32-bit compatibility segments with 64-bit addressing.
+	     These addresses work as is on 64-bit targets but
+	     can be truncated for 32-bit targets.  */
+	  if (WITH_TARGET_WORD_BITSIZE == 32)
 	    pc = (unsigned32) pc;
 
 	  CPU_PC_SET (cpu, pc);
-- 
2.25.1

[PATCH v2 2/2] [pr gdb/19447] sim: mips: Add shadow mappings for 32-bit memory address space

[Faraz Shahbazker]

32-bit MIPS programs run on the 64-bit simulator model in 64-bit
sign-extended space. The mapping from 64-bit sign-extended addresses to
32-bit addresses was removed by commit
26f8bf63bf36f9062a5cc1afacf71462a4abe0c8, breaking the 64-bit simulator
model. Add shadow mappings from 64-bit sign extended address space to
32-bit address spaces, in lieu of the AddressTranslation function.

2021-05-04  Faraz Shahbazker  <fshahbazker@wavecomp.com>

sim/mips/ChangeLog:
	* interp.c (sim_open): Add shadow mappings from 32-bit
	address space to 64-bit sign-extended address space.
---
 sim/mips/interp.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/sim/mips/interp.c b/sim/mips/interp.c
index 6e00fd0dbf0..00c1b632889 100644
--- a/sim/mips/interp.c
+++ b/sim/mips/interp.c
@@ -438,6 +438,9 @@ sim_open (SIM_OPEN_KIND kind, host_callback *cb,
 	  /* memory alias K1BASE@1,K1SIZE%MEMSIZE,K0BASE */
 	  sim_do_commandf (sd, "memory alias 0x%lx@1,0x%lx%%0x%lx,0x%0x",
 			   K1BASE, K1SIZE, (long)mem_size, K0BASE);
+	  if (WITH_TARGET_WORD_BITSIZE == 64)
+	    sim_do_commandf (sd, "memory alias 0x%x,0x%" PRIxTW ",0x%" PRIxTA,
+			     (K0BASE), mem_size, EXTENDED(K0BASE));
 	}
 
       device_init(sd);
@@ -696,11 +699,16 @@ sim_open (SIM_OPEN_KIND kind, host_callback *cb,
   if (idt_monitor_base != 0)
     {
       unsigned loop;
-      unsigned idt_monitor_size = 1 << 11;
+      address_word idt_monitor_size = 1 << 11;
 
       /* the default monitor region */
-      sim_do_commandf (sd, "memory region 0x%x,0x%x",
-		       idt_monitor_base, idt_monitor_size);
+      if (WITH_TARGET_WORD_BITSIZE == 64)
+	sim_do_commandf (sd, "memory alias 0x%x,0x%" PRIxTW ",0x%" PRIxTA,
+			 idt_monitor_base, idt_monitor_size,
+			 EXTENDED (idt_monitor_base));
+      else
+	sim_do_commandf (sd, "memory region 0x%x,0x%x",
+			 idt_monitor_base, idt_monitor_size);
 
       /* Entry into the IDT monitor is via fixed address vectors, and
 	 not using machine instructions. To avoid clashing with use of
-- 
2.25.1

Re: [PATCH v2 1/2] [pr gdb/19447] sim: mips: Only truncate sign extension bits for 32-bit target models

[Mike Frysinger]

OK, let's merge this for some level of progress
-mike

Re: [PATCH v2 2/2] [pr gdb/19447] sim: mips: Add shadow mappings for 32-bit memory address space

[Mike Frysinger]

looks OK to me, thanks
-mike