From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:44240) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S0Cgm-0007Az-SG for qemu-devel@nongnu.org; Wed, 22 Feb 2012 08:57:10 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S0Cgg-0002sM-UV for qemu-devel@nongnu.org; Wed, 22 Feb 2012 08:57:00 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37471) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S0Cgg-0002sE-Ib for qemu-devel@nongnu.org; Wed, 22 Feb 2012 08:56:54 -0500 From: Juan Quintela In-Reply-To: (andrzej zaborowski's message of "Wed, 22 Feb 2012 13:13:21 +0100") References: <1329905754-11873-1-git-send-email-i.mitsyanko@samsung.com> <1329905754-11873-4-git-send-email-i.mitsyanko@samsung.com> Date: Wed, 22 Feb 2012 14:56:48 +0100 Message-ID: <87linvug2n.fsf@elfo.elfo> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 3/5] hw/pxa2xx_lcd.c: drop VMSTATE_UINTTL usage Reply-To: quintela@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: andrzej zaborowski Cc: Peter Maydell , Igor Mitsyanko , e.voevodin@samsung.com, qemu-devel@nongnu.org, kyungmin.park@samsung.com, d.solodkiy@samsung.com, m.kozlov@samsung.com, afaerber@suse.de andrzej zaborowski wrote: > On 22 February 2012 13:00, Peter Maydell wrote: >> On 22 February 2012 11:36, andrzej zaborowski wrote: >>> On 22 February 2012 11:15, Igor Mitsyanko wro= te: >>>> Convert three variables in DMAChannel state from type >>>> target_phys_addr_t to uint32_t, >>>> use VMSTATE_UINT32 instead of VMSTATE_UINTTL for these variables. >>>> We can do it safely because: >>>> 1) pxa2xx has 32-bit physical address; >>>> 2) rest of the code in this file treats these variables as uint32_t; >>> >>> Why's uint32_t more correct though? =C2=A0The purpose of using a named = type >>> across qemu is to mark fields as memory addresses (similar to size_t >>> being used for sizes, etc.), uint32_t conveys less information -- only >>> the size. >>> >>> It's a safe hack, but I don't see the rationale. >> >> Because we might change target_phys_addr_t to 64 bits globally >> some day (it's certainly been mooted) and that shouldn't suddenly >> change the register width and certainly shouldn't change the >> migration state. >> >> Basically VMSTATE_UINTTL in hw/ is always a bug, because its >> behaviour depends on the size of target_ulong, which is a >> property of the CPU, which is a completely separate device. > > I'm not really discussing that, my question is unrelated to > migration/savevm because the patch touches parts that shouldn't be > concerned with migration. If a particular function (like migration) > needs the type converted to something then that's why C has type > conversions. A type conversion that compiles to no code is still a > type conversion. For migration, UINTTL is _always_ wrong, we need to handle it that way for backward compatibility. #if TARGET_LONG_BITS =3D=3D 64 #define VMSTATE_UINTTL_V(_f, _s, _v) \ VMSTATE_UINT64_V(_f, _s, _v) #define VMSTATE_UINTTL_ARRAY_V(_f, _s, _n, _v) \ VMSTATE_UINT64_ARRAY_V(_f, _s, _n, _v) #else #define VMSTATE_UINTTL_V(_f, _s, _v) \ VMSTATE_UINT32_V(_f, _s, _v) #define VMSTATE_UINTTL_ARRAY_V(_f, _s, _n, _v) \ VMSTATE_UINT32_ARRAY_V(_f, _s, _n, _v) #endif this was done for CPU's, not devices. If we use this, we can't access a device state without knowing the "long-iness" (don't you like the word) of the cpu that it is running. IMHO, something that always sent 64bit, and on reception if target_long is 32bit and value don't fit -> just break migration makes much more sense. But that can only be done for new code :-( On the other hand, I understand andrzej, we are missig a=20 target_phys_addr32_t or whatever we want to call it, that is for devices that _only_ support 32bit addressing. That is something that is valuable to document, independently of how migration is done. Later, Juan.