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

[Juan Quintela]

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.

For migration, UINTTL is _always_ wrong, we need to handle it that way
for backward compatibility.

#if TARGET_LONG_BITS == 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 

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.