From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:36390) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S0CJK-0001Ha-Tr for qemu-devel@nongnu.org; Wed, 22 Feb 2012 08:32:51 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S0CJF-0006Wc-UC for qemu-devel@nongnu.org; Wed, 22 Feb 2012 08:32:46 -0500 Received: from mailout2.w1.samsung.com ([210.118.77.12]:37757) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S0CJF-0006WY-Ly for qemu-devel@nongnu.org; Wed, 22 Feb 2012 08:32:41 -0500 Received: from euspt2 (mailout2.w1.samsung.com [210.118.77.12]) by mailout2.w1.samsung.com (iPlanet Messaging Server 5.2 Patch 2 (built Jul 14 2004)) with ESMTP id <0LZS003MXQYGSU@mailout2.w1.samsung.com> for qemu-devel@nongnu.org; Wed, 22 Feb 2012 13:32:40 +0000 (GMT) Received: from [106.109.8.162] by spt2.w1.samsung.com (iPlanet Messaging Server 5.2 Patch 2 (built Jul 14 2004)) with ESMTPA id <0LZS000E8QYFX3@spt2.w1.samsung.com> for qemu-devel@nongnu.org; Wed, 22 Feb 2012 13:32:40 +0000 (GMT) Date: Wed, 22 Feb 2012 17:32:39 +0400 From: Mitsyanko Igor In-reply-to: Message-id: <4F44EE77.7080505@samsung.com> MIME-version: 1.0 Content-type: text/plain; charset=UTF-8; format=flowed Content-transfer-encoding: 7BIT References: <1329905754-11873-1-git-send-email-i.mitsyanko@samsung.com> <1329905754-11873-4-git-send-email-i.mitsyanko@samsung.com> Subject: Re: [Qemu-devel] [PATCH 3/5] hw/pxa2xx_lcd.c: drop VMSTATE_UINTTL usage Reply-To: i.mitsyanko@samsung.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: andrzej zaborowski Cc: Peter Maydell , e.voevodin@samsung.com, quintela@redhat.com, qemu-devel@nongnu.org, kyungmin.park@samsung.com, d.solodkiy@samsung.com, m.kozlov@samsung.com, afaerber@suse.de On 02/22/2012 04:56 PM, andrzej zaborowski wrote: > On 22 February 2012 13:48, Peter Maydell wrote: >> On 22 February 2012 12:13, 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 wrote: >>>>>> 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? The 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. >> Well, target_phys_addr_t is the wrong type here because it's >> really "at least as large as the widest address type in the >> system" (cf proposals to make it 64 bits), so using it for >> a register that must be exactly 32 bits wide is wrong. So we >> need to change it to something, and customarily what we use >> for "I am modelling a physical register which is 32 bits wide" >> is uint32_t. Introducing extra device-specific typedefs to >> try to label the semantics of device registers seems a bit >> unnecessary to me. > If we treat the struct as a representation of the register values > rather than state of the emulated device then I guess you're right. > The reason it rings an alarm is that the change is not an improvement > (other than for migration, but again the change is in code that is not > related to savevm) > > Cheers > It's an improvement in a way that it fixes a (style) bug in code, VMSTATE_UINTTL* macro are not intended for target_phys_addr_t. -- Mitsyanko Igor ASWG, Moscow R&D center, Samsung Electronics email: i.mitsyanko@samsung.com