Re: MIPS simulator is broken

["Maciej W. Rozycki" <macro at imgtec dot com>]

Mike,

> > mips-core: 4 byte read to unmapped address 0xffffffff80020004 at 0xffffffff80020004
> > program stopped with signal 10 (User defined signal 1).
> 
> the mips address logic was fundamentally broken.  my changes merely
> uncovered that braindeadness rather than caused it.  if you look at
> that commit and the AddressTranslation definition, you'll see:
>   /* For a simple (flat) memory model, we simply pass virtual
>      addressess through (mostly) unchanged. */
>   vAddr &= 0xFFFFFFFF;
> 
> this is forcing all addresses to be 32-bit.  when i saw this, i assumed
> it was just a dummy function someone didn't get around to finishing.

 Not unreasonable unless you want to simulate gigabytes of memory (or a 
TLB -- does the MIPS port of sim support it?).  Most programs run under 
sim won't need that though.  There is a mistake there however, you really 
want the mask to be 0x1FFFFFFF, to handle the architecture's 512MB KSEG0 
and KSEG1 unmapped segments and their aliasing correctly -- some programs 
rely on that.  Beyond that you need some more intelligence.

>  i
> didn't notice that the sim explicitly sets the address space to be 64-bit
> in configure.ac for many targets:
> mips_addr_bitsize=
> case "${target}" in
>   mips*-sde-elf*)       mips_bitsize=64 ; mips_msb=63 ;;
> ...
>   mips*-*-*)            mips_bitsize=32 ; mips_msb=31 ;;
> ...
> SIM_AC_OPTION_BITSIZE($mips_bitsize,$mips_msb,$mips_addr_bitsize)
> 
> it passes for me because mips-elf is a 32-bit target where that mask is
> actually a nop.  but for targets w/64-bit address space, it's getting a
> bad 32-bit sign extended address.

 The masking is still right, even for 64-bit targets.  It does shrink the 
size of XKPHYS segments to an unusually small size (a typical is at least 
36 bits), but this is actually permitted by the architecture, there's no 
explicit low limit set on the number of the physical address bits (PABITS) 
supported, though there is no means provided for software to detect it if 
it was below 12 (if such a low number was practical in the first place, 
that is)[1].

>  the sim is behaving correctly here:
> it should be throwing an error because 0xffffffff80010000 is not mapped.

 The mapping of the 0xffffffff80010000 virtual address is well defined in 
the architecture.  As this is within the KSEG0 unmapped segment, to map it  
to a physical address of you need to AND it with 0x1ffffff.  Therefore the
resulting bus address is 0x10000.

> digging further, it looks like the bug is in bfd, not the sim.  the pc
> is set to 0xffffffff80010000 during init:
> sim-main.c:sim_create_inferior:
>   CPU_PC_SET (cpu, (unsigned64) bfd_get_start_address (abfd));
> 
> when we look at the bfd init, bfd_set_start_address is called with that
> value as well:
> elfcode.h:elf_object_p:
>   bfd_set_start_address (abfd, i_ehdrp->e_entry);
> (gdb) p *i_ehdrp
> $2 = {
>   e_ident = "\177ELF\001\002\001\000\000\000\000\000\000\000\000",
>   e_entry = 0xffffffff80010000,
> 
> e_ehdrp is supposed to be representing an Elf32_Ehdr (by way of the
> bfd Elf_Internal_Ehdr structure), and e_entry is supposed to be an
> Elf32_Addr, and that's supposed to be uint32_t.
> 
> looking at include/elf/internal.h, Elf_Internal_Ehdr is:
>   bfd_vma e_entry;    /* Entry point virtual address */
> and bfd_vma is set to:
>   bfd-in.h:typedef bfd_vma bfd_uint64_t;
> 
> so that all looks correct.  poking elf_object_p further, the x_ehdr
> (Elf_External_Ehdr) is read in correctly too.
> 
> the badness starts here:
>   elf_swap_ehdr_in (abfd, &x_ehdr, i_ehdrp);
> which does:
>   int signed_vma = get_elf_backend_data (abfd)->sign_extend_vma;
>   if (signed_vma)
>     dst->e_entry = H_GET_SIGNED_WORD (abfd, src->e_entry);
>   else
>     dst->e_entry = H_GET_WORD (abfd, src->e_entry);
> 
> and indeed, the mips bfds (for whatever reason) do:
> elf32-mips.c:#define elf_backend_sign_extend_vma    TRUE
> elf64-mips.c:#define elf_backend_sign_extend_vma    TRUE
> elfn32-mips.c:#define elf_backend_sign_extend_vma   TRUE
> 
> i don't know why mips wants to do this, and considering literally no
> other target does it (ok, sh64 does it, but let's ignore them since
> that target is dead & being removed), it's probably not an accident.
> so we have to hack around it in the sim:

 This is how the 64-bit MIPS architecture has been defined for backwards 
compatibility.  All 32-bit operations previously defined operate such that 
results are sign-extended from bit 31 in processor registers.  For example 
a 32-bit load from memory of a value that has bit 31 set will result in 
bits 63:31 set in the destination register.  It's only 64-bit operations 
that may produce results that are not sign-extended from bit 31[2].

 So e.g. a 32-bit virtual address of 0xbfc00000 becomes 0xffffffffbfc00000 
in a 64-bit MIPS processor (and maps to a physical address of 0x1fc00000); 
as this is the reset vector, this is how the architectural notion of the 
PC register will be set at the exit from the reset state in 32-bit and 
64-bit MIPS processors, respectively.  Any simulator has to reproduce this 
behaviour.

 Similarly if a 32-bit program has an entry point of 0x80010000, then 
execution has to start from 0xffffffff80010000 on a 64-bit processor.

> --- a/sim/mips/interp.c
> +++ b/sim/mips/interp.c
> @@ -55,6 +55,7 @@ code on the hardware.
>  #include "getopt.h"
>  #include "libiberty.h"
>  #include "bfd.h"
> +#include "elf-bfd.h"
>  #include "gdb/callback.h"   /* GDB simulator callback interface */
>  #include "gdb/remote-sim.h" /* GDB simulator interface */
>  
> @@ -1020,7 +1021,17 @@ 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);
> -	  CPU_PC_SET (cpu, (unsigned64) bfd_get_start_address (abfd));
> +	  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);
>  	}
>      }

 So this change is wrong, at least as far as the comment is concerned, if 
not the actual calculation.  I suggest that you read the chapter on 
virtual addressing in the MIPS architecture[3].  This may help you to come 
up with the correct calculation here.

> that allows most tests to pass, but 6 (out of 69) are still failing.
> fpu64-ps.s for example has:
> mips-core: 8 byte read to unmapped address 0xffffffff80010b18 at 0x80010070
> 
> poking this a bit, i'm not sure if the code is bad (it's actually
> doing a load from that address), or the mips decode logic has a bad
> sign extension in there.  i'll let someone else figure it out.

 No idea offhand, that just ought to map to 0x10b18.  The test case uses 
LUXC1, unusually, so maybe that has something to do with the instruction?

References:

[1] "MIPS Architecture For Programmers, Volume III: The MIPS64 and
    microMIPS64 Privileged Resource Architecture", MIPS Technologies, 
    Inc., Document Number: MD00091, Revision 5.04, January 15, 2014, 
    Section 4.2.3 "Physical Address Size (PABITS)", p. 28
    <https://imgtec.com/mips/architectures/mips64/>

[2] "MIPS Architecture For Programmers, Volume I-A: Introduction to the 
    MIPS64 Architecture", MIPS Technologies, Inc., Document Number: 
    MD00083, Revision 5.04, November 20, 2013, Section 2.8.4 "CPU 
    Registers", p. 32
    <https://imgtec.com/mips/architectures/mips64/>

[3] same as [1], Section 4.3 "Virtual Address Spaces", pp. 28-31

 HTH,

  Maciej