Patchwork [3/5] hw/pxa2xx_lcd.c: drop VMSTATE_UINTTL usage

login
register
mail settings
Submitter Mitsyanko Igor
Date Feb. 22, 2012, 10:15 a.m.
Message ID <1329905754-11873-4-git-send-email-i.mitsyanko@samsung.com>
Download mbox | patch
Permalink /patch/142431/
State New
Headers show

Comments

Mitsyanko Igor - Feb. 22, 2012, 10:15 a.m.
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;
3) we shouldn't have used VMSTATE_UINTTL in the first place because this macro
is for target_ulong type (which can be different from target_phys_addr_t).

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
---
 hw/pxa2xx_lcd.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)
Peter Maydell - Feb. 22, 2012, 11:07 a.m.
On 22 February 2012 10: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;
> 3) we shouldn't have used VMSTATE_UINTTL in the first place because this macro
> is for target_ulong type (which can be different from target_phys_addr_t).
>
> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
> ---
>  hw/pxa2xx_lcd.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/pxa2xx_lcd.c b/hw/pxa2xx_lcd.c
> index 9495226..4b712bb 100644
> --- a/hw/pxa2xx_lcd.c
> +++ b/hw/pxa2xx_lcd.c
> @@ -19,15 +19,15 @@
>  #include "framebuffer.h"
>
>  struct DMAChannel {
> -    target_phys_addr_t branch;
> +    uint32_t branch;
>     uint8_t up;
>     uint8_t palette[1024];
>     uint8_t pbuffer[1024];
>     void (*redraw)(PXA2xxLCDState *s, target_phys_addr_t addr,
>                    int *miny, int *maxy);
>
> -    target_phys_addr_t descriptor;
> -    target_phys_addr_t source;
> +    uint32_t descriptor;
> +    uint32_t source;
>     uint32_t id;
>     uint32_t command;
>  };
> @@ -934,11 +934,11 @@ static const VMStateDescription vmstate_dma_channel = {
>     .minimum_version_id = 0,
>     .minimum_version_id_old = 0,
>     .fields      = (VMStateField[]) {
> -        VMSTATE_UINTTL(branch, struct DMAChannel),
> +        VMSTATE_UINT32(branch, struct DMAChannel),
>         VMSTATE_UINT8(up, struct DMAChannel),
>         VMSTATE_BUFFER(pbuffer, struct DMAChannel),
> -        VMSTATE_UINTTL(descriptor, struct DMAChannel),
> -        VMSTATE_UINTTL(source, struct DMAChannel),
> +        VMSTATE_UINT32(descriptor, struct DMAChannel),
> +        VMSTATE_UINT32(source, struct DMAChannel),
>         VMSTATE_UINT32(id, struct DMAChannel),
>         VMSTATE_UINT32(command, struct DMAChannel),
>         VMSTATE_END_OF_LIST()
> --
> 1.7.4.1
>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM
andrzej zaborowski - Feb. 22, 2012, 11:36 a.m.
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.

If it's because VMSTATE_UINT32 requires that specific type than a less
ugly hack would be to make a pxa specific memory address type.

Cheers
Peter Maydell - Feb. 22, 2012, noon
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.

> If it's because VMSTATE_UINT32 requires that specific type than a less
> ugly hack would be to make a pxa specific memory address type.

Yuck.

-- PMM
andrzej zaborowski - Feb. 22, 2012, 12:13 p.m.
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.

>
>> If it's because VMSTATE_UINT32 requires that specific type than a less
>> ugly hack would be to make a pxa specific memory address type.
>
> Yuck.

Or a general 32-bit memory address type, but as I said uint32_t
conveys less information.  Do you disagree?

Cheers
Mitsyanko Igor - Feb. 22, 2012, 12:26 p.m.
On 02/22/2012 03:36 PM, andrzej zaborowski 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 obviously more informative, but I thought it's main purpose is to 
be used with code that could be executed for a different targets (with 
different address bus width).


> It's a safe hack, but I don't see the rationale.

I don't consider this a hack, we are trying to emulate real hardware, 
and pxa lcd and dma controllers are intended to work with 32-bit bus. We 
should not have a possibility to use them with 64-bit targets.

> If it's because VMSTATE_UINT32 requires that specific type than a less
> ugly hack would be to make a pxa specific memory address type.
>

Introducing new type doesn't look pretty to me, maybe just rename 
variables to source_addr, dest_addr e.t.c?
Peter Maydell - Feb. 22, 2012, 12:48 p.m.
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.

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
andrzej zaborowski - Feb. 22, 2012, 12:48 p.m.
On 22 February 2012 13:26, Mitsyanko Igor <i.mitsyanko@samsung.com> wrote:
> On 02/22/2012 03:36 PM, andrzej zaborowski wrote:
>> 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 obviously more informative, but I thought it's main purpose is to be
> used with code that could be executed for a different targets (with
> different address bus width).

In some case for sure, but I believe not in most cases.

>
>
>
>> It's a safe hack, but I don't see the rationale.
>
>
> I don't consider this a hack, we are trying to emulate real hardware, and
> pxa lcd and dma controllers are intended to work with 32-bit bus. We should
> not have a possibility to use them with 64-bit targets.
>
>
>> If it's because VMSTATE_UINT32 requires that specific type than a less
>> ugly hack would be to make a pxa specific memory address type.
>>
>
> Introducing new type doesn't look pretty to me,

Why?

> maybe just rename variables
> to source_addr, dest_addr e.t.c?

Wouldn't it be analogous to changing pointer typed variables to void *
and adding the actual type in their names?  The result is that at
language level they'll all be the same type even though they are not.

(or changing le32 and be32 to uint32 in Linux)

Cheers
andrzej zaborowski - Feb. 22, 2012, 12:56 p.m.
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
Mitsyanko Igor - Feb. 22, 2012, 1:30 p.m.
On 02/22/2012 04:48 PM, andrzej zaborowski wrote:
> On 22 February 2012 13:26, Mitsyanko Igor<i.mitsyanko@samsung.com>  wrote:
>> On 02/22/2012 03:36 PM, andrzej zaborowski wrote:
>>> 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 obviously more informative, but I thought it's main purpose is to be
>> used with code that could be executed for a different targets (with
>> different address bus width).
>
> In some case for sure, but I believe not in most cases.
>
>>
>>
>>
>>> It's a safe hack, but I don't see the rationale.
>>
>>
>> I don't consider this a hack, we are trying to emulate real hardware, and
>> pxa lcd and dma controllers are intended to work with 32-bit bus. We should
>> not have a possibility to use them with 64-bit targets.
>>
>>
>>> If it's because VMSTATE_UINT32 requires that specific type than a less
>>> ugly hack would be to make a pxa specific memory address type.
>>>
>>
>> Introducing new type doesn't look pretty to me,
>
> Why?

Peter already answered, this fields should be exactly 32-bit wide 
(hardware is implemented this way) and we already have a type that is 
exactly 32-bit wide. Implementing each device without any assumptions 
about other parts of emulated system seems like a right approach to me.
Doing something like typedef uint32_t pxalcd_phys_addr_t is fun, but 
then we could end up introducing typedefs like this for every device in hw/.
Also, currently we can't save custom types in VMSTATE.

>> maybe just rename variables
>> to source_addr, dest_addr e.t.c?
>
> Wouldn't it be analogous to changing pointer typed variables to void *
> and adding the actual type in their names?  The result is that at
> language level they'll all be the same type even though they are not.
>
> (or changing le32 and be32 to uint32 in Linux)
>

Yes, but you got to admit they would be more informative for human :)
Mitsyanko Igor - Feb. 22, 2012, 1:32 p.m.
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.
Juan Quintela - Feb. 22, 2012, 1:56 p.m.
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.

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.

Patch

diff --git a/hw/pxa2xx_lcd.c b/hw/pxa2xx_lcd.c
index 9495226..4b712bb 100644
--- a/hw/pxa2xx_lcd.c
+++ b/hw/pxa2xx_lcd.c
@@ -19,15 +19,15 @@ 
 #include "framebuffer.h"
 
 struct DMAChannel {
-    target_phys_addr_t branch;
+    uint32_t branch;
     uint8_t up;
     uint8_t palette[1024];
     uint8_t pbuffer[1024];
     void (*redraw)(PXA2xxLCDState *s, target_phys_addr_t addr,
                    int *miny, int *maxy);
 
-    target_phys_addr_t descriptor;
-    target_phys_addr_t source;
+    uint32_t descriptor;
+    uint32_t source;
     uint32_t id;
     uint32_t command;
 };
@@ -934,11 +934,11 @@  static const VMStateDescription vmstate_dma_channel = {
     .minimum_version_id = 0,
     .minimum_version_id_old = 0,
     .fields      = (VMStateField[]) {
-        VMSTATE_UINTTL(branch, struct DMAChannel),
+        VMSTATE_UINT32(branch, struct DMAChannel),
         VMSTATE_UINT8(up, struct DMAChannel),
         VMSTATE_BUFFER(pbuffer, struct DMAChannel),
-        VMSTATE_UINTTL(descriptor, struct DMAChannel),
-        VMSTATE_UINTTL(source, struct DMAChannel),
+        VMSTATE_UINT32(descriptor, struct DMAChannel),
+        VMSTATE_UINT32(source, struct DMAChannel),
         VMSTATE_UINT32(id, struct DMAChannel),
         VMSTATE_UINT32(command, struct DMAChannel),
         VMSTATE_END_OF_LIST()