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

[andrzej zaborowski]

On 22 February 2012 13:48, Peter Maydell <address@hidden> wrote:
> On 22 February 2012 12:13, andrzej zaborowski <address@hidden> wrote:
>> On 22 February 2012 13:00, Peter Maydell <address@hidden> wrote:
>>> On 22 February 2012 11:36, andrzej zaborowski <address@hidden> wrote:
>>>> On 22 February 2012 11:15, Igor Mitsyanko <address@hidden> 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