Re: [Qemu-devel] [PATCH 1/6] m68k cpu instantiation comments improvements

[Laurent Vivier]

Le 16/06/2019 à 16:22, Lucien Murray-Pitts a écrit :
> Improvement in comments for the instantiation functions.
> This is to highlight what each cpu class, in the 68000 series, contains
> in terms of instructions/features.
> 
> Signed-off-by: Lucien Murray-Pitts <address@hidden>
> ---
>  target/m68k/cpu.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  target/m68k/cpu.h | 40 +++++++++++++++++++++++++---------------
>  2 files changed, 72 insertions(+), 15 deletions(-)
> 
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index c144278661..cc770a8042 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -104,6 +104,10 @@ static void m5206_cpu_initfn(Object *obj)
>      m68k_set_feature(env, M68K_FEATURE_CF_ISA_A);
>  }
>  

Adding some references can help, for instance:

/* MC68000 FAMILY PROGRAMMER’S REFERENCE MANUAL
 * APPENDIX A
 * PROCESSOR INSTRUCTION SUMMARY
 * Table A-1. M68000 Family Instruction Set And
 *            Processor Cross-Reference
 */

> +
> +/*
> + * Base feature set, including isns. for m68k family
> + */
>  static void m68000_cpu_initfn(Object *obj)
>  {
>      M68kCPU *cpu = M68K_CPU(obj);
> @@ -115,6 +119,12 @@ static void m68000_cpu_initfn(Object *obj)
>      m68k_set_feature(env, M68K_FEATURE_MOVEP);
>  }
>  
> +/*
> + * Adds BFCHG, BFCLR, BFEXTS, BFEXTU, BFFFO, BFINS, BFSET, BFTST, CAS, CAS2,
> + *      CHK2, CMP2, DIVSL, DIVUL, EXTB, PACK, TRAPcc, UNPK.
> + *
> + * 68020/30 Only:CALLM, cpBcc, cpDBcc, cpGEN, cpRESTORE, cpSAVE, cpScc, 
> cpTRAPcc
> + */

Add BKPT (since 68010)
CALLM and RTM are 68020 only


>  static void m68020_cpu_initfn(Object *obj)
>  {
>      M68kCPU *cpu = M68K_CPU(obj);
> @@ -137,8 +147,34 @@ static void m68020_cpu_initfn(Object *obj)
>      m68k_set_feature(env, M68K_FEATURE_CHK2);
>      m68k_set_feature(env, M68K_FEATURE_MOVEP);
>  }
> +
> +/*
> + * Adds: PFLUSH (*5)

PFLUSH is also 68030 only

> + * 68030 Only: PFLUSHA (*5), PLOAD (*5), PMOVE
> + * 68030/40 Only: PTEST

Don't speak about 040 in the 030 section.
You should group PFLUSH and PTEST, as they exist on 030 and 040, but
their syntax and opcodes differ between 030 and 040

> + *
> + * NOTES:
> + *  5. Not valid on MC68EC030
> + */
>  #define m68030_cpu_initfn m68020_cpu_initfn
>  
> +/*
> + * Adds: CINV, CPUSH
> + * Adds all with Note *2: FABS, FSABS, FDABS, FADD, FSADD, FDADD, FBcc, FCMP,
> + *                        FDBcc, FDIV, FSDIV, FDDIV, FMOVE, FSMOVE, FDMOVE,
> + *                        FMOVEM, FMUL, FSMUL, FDMUL, FNEG, FSNEG, FDNEG, 
> FNOP,
> + *                        FRESTORE, FSAVE, FScc, FSQRT, FSSQRT, FDSQRT, FSUB,
> + *                        FSSUB, FDSUB, FTRAPcc, FTST
> + *
> + * Adds with Notes *2, and *3: FACOS, FASIN, FATAN, FATANH, FCOS, FCOSH, 
> FETOX,
> + *                             FETOXM, FGETEXP, FGETMAN, FINT, FINTRZ, 
> FLOG10,
> + *                             FLOG2, FLOGN, FLOGNP1, FMOD, FMOVECR, FREM,
> + *                             FSCALE, FSGLDIV, FSGLMUL, FSIN, FSINCOS, 
> FSINH,
> + *                             FTAN, FTANH, FTENTOX, FTWOTOX

All are FPU operations.
Perhaps you can add details to show which are also supported by 68881/68882.

> + * NOTES:
> + * 2. Not applicable to the MC68EC040, MC68LC040, MC68EC060, and MC68LC060.
> + * 3. These are software-supported instructions on the MC68040 and MC68060.
> + */
>  static void m68040_cpu_initfn(Object *obj)
>  {
>      M68kCPU *cpu = M68K_CPU(obj);
> @@ -148,6 +184,17 @@ static void m68040_cpu_initfn(Object *obj)
>      m68k_set_feature(env, M68K_FEATURE_M68040);
>  }
>  
> +/*
> + * Adds: PLPA
> + * Adds all with Note *2: CAS, CAS2, MULS, MULU, CHK2, CMP2, DIVS, DIVU
> + * All Fxxxx instructions are as per m68040 with exception to; FMOVEM NOTE3
> + *
> + * Does NOT implement MOVEP
> + *
> + * NOTES:
> + * 2. Not applicable to the MC68EC040, MC68LC040, MC68EC060, and MC68LC060.

Don't need to speak about 040 in the 060 section. The comment is already
in the 040 section.

> + * 3. These are software-supported instructions on the MC68040 and MC68060.
> + */
>  static void m68060_cpu_initfn(Object *obj)
>  {
>      M68kCPU *cpu = M68K_CPU(obj);
> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
> index 3c4d7de017..b5b3db01c9 100644
> --- a/target/m68k/cpu.h
> +++ b/target/m68k/cpu.h
> @@ -468,36 +468,46 @@ void m68k_switch_sp(CPUM68KState *env);
>  void do_m68k_semihosting(CPUM68KState *env, int nr);
>  
>  /*
> + * The 68000 family is defined in six main CPU classes, the 680[012346]0.
> + * Generally each successive CPU adds enhanced data/stack/instructions.
> + * However, some features are only common to one, or a few classes.
> + * The features covers those subsets of instructons.
> + *
> + * CPU32/32+ are basically 680010 compatible with some 68020 class 
> instructons,

s/instructons/instructions/

> + * and some additional CPU32 instructions. Mostly Supervisor state 
> differences.
> + *
> + * The ColdFire core ISA is a RISC-style reduction of the 68000 series cpu.
>   * There are 4 ColdFire core ISA revisions: A, A+, B and C.
>   * Each feature covers the subset of instructions common to the
>   * ISA revisions mentioned.
>   */
>  
>  enum m68k_features {
> -    M68K_FEATURE_M68000,
> -    M68K_FEATURE_CF_ISA_A,
> +    M68K_FEATURE_M68000,   /* Base m68k instruction set */
> +    M68K_FEATURE_M68040,   /* Additional insn. specific to MC68040 */
> +    M68K_FEATURE_CF_ISA_A, /* Base Coldfire set Rev A. */
>      M68K_FEATURE_CF_ISA_B, /* (ISA B or C).  */
>      M68K_FEATURE_CF_ISA_APLUSC, /* BIT/BITREV, FF1, STRLDSR (ISA A+ or C).  
> */
> -    M68K_FEATURE_BRAL, /* Long unconditional branch.  (ISA A+ or B).  */
> +    M68K_FEATURE_BRAL, /* BRA with Long branch.  (680[2346]0, ISA A+ or B). 
> */
>      M68K_FEATURE_CF_FPU,
>      M68K_FEATURE_CF_MAC,
>      M68K_FEATURE_CF_EMAC,
>      M68K_FEATURE_CF_EMAC_B, /* Revision B EMAC (dual accumulate).  */
> -    M68K_FEATURE_USP, /* User Stack Pointer.  (ISA A+, B or C).  */
> +    M68K_FEATURE_USP, /* User Stack Pointer. (680[012346]0, ISA A+, B or 
> C).*/
> +    M68K_FEATURE_MSP, /* Master Stack Pointer. (680[234]0) */

Introduce M68K_FEATURE_MSP when you need it: in patch 6.

>      M68K_FEATURE_EXT_FULL, /* 68020+ full extension word.  */
>      M68K_FEATURE_WORD_INDEX, /* word sized address index registers.  */
>      M68K_FEATURE_SCALED_INDEX, /* scaled address index registers.  */
> -    M68K_FEATURE_LONG_MULDIV, /* 32 bit multiply/divide. */
> -    M68K_FEATURE_QUAD_MULDIV, /* 64 bit multiply/divide. */
> -    M68K_FEATURE_BCCL, /* Long conditional branches.  */
> -    M68K_FEATURE_BITFIELD, /* Bit field insns.  */
> -    M68K_FEATURE_FPU,
> -    M68K_FEATURE_CAS,
> -    M68K_FEATURE_BKPT,
> -    M68K_FEATURE_RTD,
> -    M68K_FEATURE_CHK2,
> -    M68K_FEATURE_M68040, /* instructions specific to MC68040 */
> -    M68K_FEATURE_MOVEP,
> +    M68K_FEATURE_LONG_MULDIV, /* 32 bit mul/div. (680[2346]0, and CPU32) */
> +    M68K_FEATURE_QUAD_MULDIV, /* 64 bit mul/div. (680[2346]0, and CPU32) */
> +    M68K_FEATURE_BCCL, /* Bcc with Long branches. (680[2346]0, and CPU32) */
> +    M68K_FEATURE_BITFIELD, /* BFxxx Bit field insns. (680[2346]0) */
> +    M68K_FEATURE_FPU, /* fpu insn. (680[46]0) */
> +    M68K_FEATURE_CAS, /* CAS/CAS2[WL] insns. (680[2346]0) */
> +    M68K_FEATURE_BKPT,/* BKPT insn. (680[12346]0, and CPU32) */
> +    M68K_FEATURE_RTD, /* RTD insn. (680[12346]0, and CPU32) */
> +    M68K_FEATURE_CHK2,/* CHL2 insn. (680[2346]0, and CPU32) */
> +    M68K_FEATURE_MOVEP,/* MOVEP insn. (680[01234]0, and CPU32) */
>  };
>  
>  static inline int m68k_feature(CPUM68KState *env, int feature)
> 

Thanks,
Laurent