Re: [Qemu-devel] [PATCH 3/5] hw/pxa2xx_lcd.c: drop VMSTATE_UINTTL usage

[Mitsyanko Igor <i.mitsyanko@samsung.com>]

On 02/22/2012 04:56 PM, andrzej zaborowski wrote:
> On 22 February 2012 13:48, Peter Maydell<peter.maydell@linaro.org>  wrote:
>> On 22 February 2012 12:13, andrzej zaborowski<balrogg@gmail.com>  wrote:
>>> On 22 February 2012 13:00, Peter Maydell<peter.maydell@linaro.org>  wrote:
>>>> On 22 February 2012 11:36, andrzej zaborowski<balrog@zabor.org>  wrote:
>>>>> On 22 February 2012 11:15, Igor Mitsyanko<i.mitsyanko@samsung.com>  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