From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39410) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SQKQv-0005l0-Sv for qemu-devel@nongnu.org; Fri, 04 May 2012 11:28:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SQKQq-0004Ad-JT for qemu-devel@nongnu.org; Fri, 04 May 2012 11:28:37 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37019) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SQKQq-0004AG-Bd for qemu-devel@nongnu.org; Fri, 04 May 2012 11:28:32 -0400 From: Juan Quintela In-Reply-To: (Peter Maydell's message of "Fri, 4 May 2012 14:04:43 +0100") References: <5a3abb34f8a5c286a12d6d067e11b14930757825.1336128412.git.quintela@redhat.com> Date: Fri, 04 May 2012 17:28:27 +0200 Message-ID: <87k40s9d9g.fsf@elfo.elfo> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 23/35] vmstate: port arm cpu Reply-To: quintela@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: qemu-devel@nongnu.org Peter Maydell wrote: > On 4 May 2012 11:54, Juan Quintela wrote: >> Use one subsection for each feature. =C2=A0This means that we don't need= to >> bump the version field each time that a new feature gets introduced. >> >> Introduce cpsr_vmstate field, as I am not sure if I can "use" >> uncached_cpsr for saving state. >> >> Signed-off-by: Juan Quintela >> --- >> =C2=A0target-arm/cpu.h =C2=A0 =C2=A0 | =C2=A0 =C2=A05 +- >> =C2=A0target-arm/machine.c | =C2=A0344 ++++++++++++++++++++++-----------= ----------------- >> =C2=A02 files changed, 156 insertions(+), 193 deletions(-) >> >> diff --git a/target-arm/cpu.h b/target-arm/cpu.h >> index 9434902..37744c6 100644 >> --- a/target-arm/cpu.h >> +++ b/target-arm/cpu.h >> @@ -236,6 +236,9 @@ typedef struct CPUARMState { >> =C2=A0 =C2=A0 } cp[15]; >> =C2=A0 =C2=A0 void *nvic; >> =C2=A0 =C2=A0 const struct arm_boot_info *boot_info; >> + >> + =C2=A0 =C2=A0/* Fields needed as intermediate for vmstate */ >> + =C2=A0 =C2=A0uint32_t cpsr_vmstate; >> =C2=A0} CPUARMState; > > I still think this is the wrong approach. We need to support > "this is how you read/write this field" functions. See also > target-alpha handling of the fpcr. It is easy to change to support that (see alpha example), but then, each time that we change protocol format (and we have a pending change for 1.2), every user that is "like" uint64_t, but with accesor and setter, needs to get a new change. This other way (with pre_save/post_load) instead makes trivial to change protocols. So, we can have "cleaniness" or "flexibility", take one. I still think that alpha should be changed to this other approach, for the same reason. Notice that we _know_ that what we sent is an uint32_t, it is how we get that uint32_t what changes, that is why I preffer a new field and pre_load/post_load functions. The only other sane approach that I can think is changing: #define VMSTATE_INT32_V(_f, _s, _v) \ into something like: VMSTATE_INT32_GENERAL(_f, _s, _v, _getter, _setter) and then _getter/_setter functions for normal ints the obvious copy this uint32_t? Would that work for you? Later, Juan.