qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Mitsyanko Igor
Subject: Re: [Qemu-devel] [PATCH 3/5] hw/pxa2xx_lcd.c: drop VMSTATE_UINTTL usage
Date: Wed, 22 Feb 2012 17:32:39 +0400
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.27) Gecko/20120216 Thunderbird/3.1.19



On 02/22/2012 04:56 PM, andrzej zaborowski wrote:
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

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.

--
Mitsyanko Igor
ASWG, Moscow R&D center, Samsung Electronics
email: address@hidden




reply via email to

[Prev in Thread] Current Thread [Next in Thread]