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 <r...@twiddle.net> 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