andrzej zaborowski <balr...@gmail.com> wrote:
> On 22 February 2012 13:00, Peter Maydell <peter.mayd...@linaro.org> wrote:
>> On 22 February 2012 11:36, andrzej zaborowski <bal...@zabor.org> wrote:
>>> On 22 February 2012 11:15, Igor Mitsyanko <i.mitsya...@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.
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.
