| Message ID | 1329905754-11873-4-git-send-email-i.mitsyanko@samsung.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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?
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
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
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
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 :)
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.
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.
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()
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(-)