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

[Michael Rolnik]

Hi Richard,

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

How it can be done? how can I modify a value passed to VMSTATE_UINT32?


On Mon, Jun 6, 2016 at 11:15 PM, Richard Henderson <address@hidden> wrote:

> 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~
>



-- 
Best Regards,
Michael Rolnik