Re: [Qemu-devel] [PATCH v4 1/9] target-avr: AVR cores support is added. 1. basic CPU structure 2. registers 3. no instructions

[Richard Henderson]

On 06/06/2016 03:37 AM, Michael Rolnik wrote:
> +int print_insn_avr(bfd_vma addr, disassemble_info *info)
> +{
> +    int length  = 0;;
> +    /*  TODO    */
> +    return length;
> +}

Again, delete this file.  This prohibits the default implementation from 
working.

> +static void avr_cpu_reset(CPUState *s)
> +{
> +    AVRCPU *cpu = AVR_CPU(s);
> +    AVRCPUClass *mcc = AVR_CPU_GET_CLASS(cpu);
> +    CPUAVRState *env = &cpu->env;
> +
> +    mcc->parent_reset(s);
> +
> +    memset(env, 0, sizeof(CPUAVRState));

You didn't fix the memset issue I pointed out.

> +}
> +AVRCPU *cpu_avr_init(const char *cpu_model)

Spacing.

> +/*
> +    NOTE:   all registers are assumed to hold 8 bit values.
> +            so all operations done on registers should preseve this property
> +*/
> +
> +/*
> +    NOTE:   the flags C,H,V,N,V have either 0 or 1 values
> +    NOTE:   the flag Z has inverse logic, when the value of Zf is 0 the flag 
> is assumed to be set, non zero - not set
> +*/

Don't put these at the top, put them next to the declarations of the actual 
variables, where they're useful.

> +#define TARGET_IS_LIENDIAN 1

Didn't fix the typo, or really just delete this.

In case you're wondering, this *doesn't* mean little-endian.

TARGET_IS_BIENDIAN means that the target changes between big- and little-endian 
at runtime.

One selects the endian-ness of the target in configure.  The default is little; 
set target_bigendian to select big.


> +    uint32_t        sregC;  /*   0x00000001 1 bits         */
> +    uint32_t        sregZ;  /*   0x000000ff 8 bits         */

This isn't quite true; sregZ may contain 0xffff after adwi.

> +static inline int cpu_mmu_index(CPUAVRState *env, bool ifetch)
> +{
> +    return  0;
> +}

This should be

  return ifetch ? CODE_INDEX : DATA_INDEX;

> +        sreg =   (env->sregC & 0x01) << 0
> +                |   (env->sregZ & 0x01) << 1
> +                |   (env->sregN & 0x01) << 2
> +                |   (env->sregV & 0x01) << 3
> +                |   (env->sregS & 0x01) << 4
> +                |   (env->sregH & 0x01) << 5
> +                |   (env->sregT & 0x01) << 6
> +                |   (env->sregI & 0x01) << 7;

Didn't add the function I requested.
Plus, there's an error in handling of sregZ.

> +        env->sregC = (tmp >> 0) & 0x01;
> +        env->sregZ = (tmp >> 1) & 0x01;
> +        env->sregN = (tmp >> 2) & 0x01;
> +        env->sregV = (tmp >> 3) & 0x01;
> +        env->sregS = (tmp >> 4) & 0x01;
> +        env->sregH = (tmp >> 5) & 0x01;
> +        env->sregT = (tmp >> 6) & 0x01;
> +        env->sregI = (tmp >> 7) & 0x01;

Likewise.

> +        VMSTATE_UINT32_ARRAY(r, CPUAVRState, 32),
> +
> +        VMSTATE_UINT32(sregC,   CPUAVRState),
> +        VMSTATE_UINT32(sregZ,   CPUAVRState),
> +        VMSTATE_UINT32(sregN,   CPUAVRState),
> +        VMSTATE_UINT32(sregV,   CPUAVRState),
> +        VMSTATE_UINT32(sregS,   CPUAVRState),
> +        VMSTATE_UINT32(sregH,   CPUAVRState),
> +        VMSTATE_UINT32(sregT,   CPUAVRState),
> +        VMSTATE_UINT32(sregI,   CPUAVRState),
> +
> +        VMSTATE_UINT32(rampD,   CPUAVRState),
> +        VMSTATE_UINT32(rampX,   CPUAVRState),
> +        VMSTATE_UINT32(rampY,   CPUAVRState),
> +        VMSTATE_UINT32(rampZ,   CPUAVRState),
> +
> +        VMSTATE_UINT32(eind,    CPUAVRState),
> +        VMSTATE_UINT32(sp,      CPUAVRState),
> +        VMSTATE_UINT32(pc,      CPUAVRState),

Consider making the vm save state reflect the actual hardware format.  That way 
you can change the qemu internal format while retaining migration compatibility.

> +    cpu_fprintf(f, "rampX:     %02x\n", env->rampX);
> +    cpu_fprintf(f, "rampY:     %02x\n", env->rampY);
> +    cpu_fprintf(f, "rampZ:     %02x\n", env->rampZ);
> +    cpu_fprintf(f, "rampD:     %02x\n", env->rampD);
> +    cpu_fprintf(f, "EIND:      %02x\n", env->eind);

This format probably isn't what you intended after shifting these values.


r~