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

[Peter Maydell]

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.

So we need to change the type, and we also need to change the
VMSTATE macro for reasons explained earlier, and we need to make
sure the macro and the struct agree about the type they are
dealing with, so it's simplest to do it all in one patch.

If you're complaining that the VMSTATE macros don't provide
a way for you to say "do this cast" then I agree that that is
a slightly irritating omission but that's the infrastructure
we have...

-- PMM