Re: [PATCH 2/3] sim: Add nanoMIPS port

[Mike Frysinger <vapier@gentoo.org>]

[-- Attachment #1: Type: text/plain, Size: 3474 bytes --]

On 29 Apr 2022 17:58, Aleksandar Rikalo wrote:
> Co-Authored-By: Jaydeep Patil <jaydeep.patil@imgtec.com>
> Co-Authored-By: Matthew Fortune <matthew.fortune@imgtec.com>
> Co-Authored-By: Maciej W. Rozycki <macro@mips.com>
> Co-Authored-By: Stefan Markovic <stefan.markovic@mips.com>
> Co-Authored-By: Sara Graovac <sara.graovac@syrmia.com>
> Co-Authored-By: Dragan Mladjenovic <dragan.mladjenovic@syrmia.com>

is everyone's copyright papers in place ?

> --- a/sim/configure
> +++ b/sim/configure

you seem to be missing the change to the source of this file.
this is generated, so you shouldn't be editing this.

> --- a/sim/common/sim-bits.h
> +++ b/sim/common/sim-bits.h

looks fine

> --- a/sim/mips/Makefile.in
> +++ b/sim/mips/Makefile.in
> @@ -79,6 +80,20 @@ SIM_EXTRA_DEPS = itable.h
>  
>  ## COMMON_POST_CONFIG_FRAG
>  
> +interp.o: $(srcdir)/interp.c sim-main.h itable.h
> +
> +m16run.o: sim-main.h m16_idecode.h m32_idecode.h m16run.c $(SIM_EXTRA_DEPS)
> +
> +micromipsrun.o: sim-main.h micromips16_idecode.h micromips32_idecode.h \
> +		micromips_m32_idecode.h micromipsrun.c $(SIM_EXTRA_DEPS)
> +
> +nms.o: $(srcdir)/nms.c $(srcdir)/sim-main.h
> +
> +multi-run.o: multi-include.h tmp-mach-multi
> +
> +../igen/igen:
> +	cd ../igen && $(MAKE)

we deleted all this stuff.  don't add it back.

> --- a/sim/mips/configure.ac
> +++ b/sim/mips/configure.ac
> @@ -106,6 +106,12 @@ case "${target}" in
>  			  mipsisa64r6:mips64r6:32,64,f:mipsisa32r6,mipsisa64r6"
>  			sim_multi_default=mipsisa64r2
>  			;;
> +  nanomips*-*-elf*)
> +			sim_gen=MULTI
> +			sim_multi_configs="\
> +			  nanor6sim:nanomips64r6,nanomipsdsp:32,64,f:nanomipsisa64r6,nanomipsisa32r6"
> +			sim_multi_default=nanomipsisa64r6
> +			;;

seems odd that you're using a new "nanomips*" tuple instead of existing
"mips*" space.  but i guess if you've convinced everyone else that this
isn't wrong, then sim/ will just follow.

> --- a/sim/mips/interp.c
> +++ b/sim/mips/interp.c
>  
> +int is_nanomips = 0;

do not declare globals.  state goes in sim_cpu.

> @@ -1531,21 +1535,26 @@ store_word (SIM_DESC sd,
>  	    uword64 vaddr,
>  	    signed_word val)
>  {
> -  address_word paddr = vaddr;
> +  address_word paddr;
> +  int uncached;
>  
>    if ((vaddr & 3) != 0)
>      SignalExceptionAddressStore ();
>    else
>      {
> -      const uword64 mask = 7;
> -      uword64 memval;
> -      unsigned int byte;
> -
> -      paddr = (paddr & ~mask) | ((paddr & mask) ^ (ReverseEndian << 2));
> -      byte = (vaddr & mask) ^ (BigEndianCPU << 2);
> -      memval = ((uword64) val) << (8 * byte);
> -      StoreMemory (AccessLength_WORD, memval, 0, paddr, vaddr,
> -		   isREAL);
> +      if (AddressTranslation (vaddr, isDATA, isSTORE, &paddr, &uncached,
> +			      isTARGET, isREAL))
> +	{
> +	  const uword64 mask = 7;
> +	  uword64 memval;
> +	  unsigned int byte;
> +
> +	  paddr = (paddr & ~mask) | ((paddr & mask) ^ (ReverseEndian << 2));
> +	  byte = (vaddr & mask) ^ (BigEndianCPU << 2);
> +	  memval = ((uword64) val) << (8 * byte);
> +	  StoreMemory (uncached, AccessLength_WORD, memval, 0, paddr, vaddr,
> +		       isREAL);
> +	}

you seem to be reverting changes in mips/ that we made a while ago with
no explanation.  i'm giving up on reviewing at this point.  you need to
update your port to the current state of the world, not roll it back to
code from 7 years ago.
-mike

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]