Re: [PATCH, FT32] gdb and sim support

[Mike Frysinger <vapier@gentoo.org>]

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

it mostly looks good.  i made some fixes you should apply (see attached).

there's some outstanding issues related to the memory handling.  i think the 
disconnect here is how the sim works.  the core sim logic just processes the
ISA and manages the core of the cpu -- it executes insns and handles the regs
and sends requests to the memory.  there are devices you can attach to the main 
memory bus (see common/dv-*.c for examples) so that when requests are made to 
addresses that a device is attached to, they automatically get routed to the 
right code for you.

this is where the model layers come into play.  you declare specific cpu models 
that have devices attached to known addresses.  at runtime, you can select from 
the different models, and the sim will build that up.

the power of the sim comes into play when you want to try out ideas for new cpu 
models.  you can easily move device address attachments around, add more/less 
memory, and create models for hardware that doesn't yet exist.  then run code in 
that simulation and see how things work.

> +#define RAM_BIAS  0x800000  /* Bias added to RAM addresses.  */

what's with this ?  if the code requests address 0, it should go to address 0.

> +/* Align EA according to its size DW.  */
> +static uint32_t ft32_align (uint32_t dw, uint32_t ea)

what's with this ?  is this how the hardware works ?  e.g. if the PC is set to 
0x101, it'll mask off the low bit ?  or will it throw an exception ?  same for 
if it reads/writes address 0x101.

> +/* Read an item from memory address EA, sized DW.  */
> +static uint32_t
> +ft32_read_item (SIM_DESC sd, int dw, uint32_t ea)
> +{
> +  sim_cpu *cpu = STATE_CPU (sd, 0);
> +  uint8_t byte[4];
> +  uint32_t r;
> +
> +  ea = ft32_align (dw, ea);
> +  sim_core_read_buffer (sd, cpu, read_map, byte, ea, 1 << dw);
> +  r = byte[0];
> +  if (1 <= dw)
> +    {
> +      r += (byte[1] << 8);
> +      if (2 <= dw)
> +	{
> +	  r += byte[2] << 16;
> +	  r += byte[3] << 24;
> +	}
> +    }
> +  return r;
> +}

you should be able to use sim_core_read_aligned_{1,2,4} here instead of reading 
and unpacking the byte array yourself

you should check the return value of the sim core read function

> +/* Write item V to memory address EA, sized DW.  */
> +static void
> +ft32_write_item (SIM_DESC sd, int dw, uint32_t ea, uint32_t v)
> +{
> +  sim_cpu *cpu = STATE_CPU (sd, 0);
> +  uint8_t byte[4];
> +
> +  ea = ft32_align (dw, ea);
> +
> +  byte[0] = v & 0xff;
> +  byte[1] = (v >> 8) & 0xff;
> +  byte[2] = (v >> 16) & 0xff;
> +  byte[3] = (v >> 24) & 0xff;
> +  sim_core_write_buffer (sd, cpu, write_map, byte, ea, 1 << dw);
> +}

this looks like assumes the memory is little endian.  should it be using 
sim_core_write_unaligned_{1,2,4} instead ?

you should check the return value of the sim core write function

> +static uint32_t cpu_mem_read (SIM_DESC sd, uint32_t dw, uint32_t ea)
> +{
> +  sim_cpu *cpu = STATE_CPU (sd, 0);
> +  uint32_t insnpc = cpu->state.pc;
> +  uint32_t r;
> +  uint8_t byte[4];
> +
> +  ea &= 0x1ffff;
> +  if ((ea & ~0xffff))
> +    {
> +      switch (ea)
> +	{
> +	case 0x1fff4:
> +	  /* Read the simulator cycle timer.  */
> +	  return cpu->state.cycles / 100;
> +	default:
> +	  sim_io_eprintf (sd,
> +			  "Illegal IO read address %08x, pc %#x\n",
> +			  ea,
> +			  insnpc);
> +	  ILLEGAL ();
> +	}
> +    }
> +  return ft32_read_item (sd, dw, RAM_BIAS + ea);
> +}
> +
> +static void cpu_mem_write (SIM_DESC sd, uint32_t dw, uint32_t ea, uint32_t d)
> +{
> +  sim_cpu *cpu = STATE_CPU (sd, 0);
> +  ea &= 0x1ffff;
> +  if (ea & 0x10000)
> +    {
> +      switch (ea)
> +	{
> +	case 0x10000:
> +	  putchar (d & 0xff);
> +	  break;
> +	case 0x1fc80:
> +	  cpu->state.pm_unlock = (d == 0x1337f7d1);
> +	  break;
> +	case 0x1fc84:
> +	  cpu->state.pm_addr = d;
> +	  break;
> +	case 0x1fc88:
> +	  ft32_write_item (sd, dw, cpu->state.pm_addr, d);
> +	  break;
> +	case 0x10024:
> +	  break;
> +	case 0x1fffc:
> +	  /* Normal exit.  */
> +	  sim_engine_halt (sd, cpu, NULL, cpu->state.pc, sim_exited, cpu->state.regs[0]);
> +	  break;
> +	case 0x1fff8:
> +	case 0x19470:
> +	  sim_io_printf (sd, "Debug write %08x\n", d);
> +	  break;
> +	default:
> +	  sim_io_eprintf (sd, "Unknown IO write %08x to to %08x\n", d, ea);
> +	}
> +    }
> +  else
> +    ft32_write_item (sd, dw, RAM_BIAS + ea, d);
> +}

what's with these addresses ?  are you trying to simulate attached hardware 
devices ?

> +  /* CPU specific initialization.  */
> +  for (i = 0; i < MAX_NR_PROCESSORS; ++i)
> +    {
> +      SIM_CPU *cpu = STATE_CPU (sd, i);
> +
> +      CPU_REG_FETCH (cpu) = ft32_reg_fetch;
> +      CPU_REG_STORE (cpu) = ft32_reg_store;
> +    }

still should implement the CPU_PC_FETCH/etc... stuff i suggested earlier.
that way the builtin profile functions will work automatically.
$ ./run -p -v ./some-elf
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]