Patchwork [1/6] hw/arm_boot.c: Make ram_size a target_phys_addr_t

login
register
mail settings
Submitter Peter Maydell
Date July 5, 2012, 5 p.m.
Message ID <1341507652-22155-2-git-send-email-peter.maydell@linaro.org>
Download mbox | patch
Permalink /patch/169230/
State New
Headers show

Comments

Peter Maydell - July 5, 2012, 5 p.m.
Make the RAM size in arm_boot_info a target_phys_addr_t so
it can express RAM sizes up to the limit imposed by the
physical address size.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm-misc.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Peter A. G. Crosthwaite - July 6, 2012, 1:58 a.m.
On Fri, Jul 6, 2012 at 3:00 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Make the RAM size in arm_boot_info a target_phys_addr_t so
> it can express RAM sizes up to the limit imposed by the
> physical address size.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>

> ---
>  hw/arm-misc.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/hw/arm-misc.h b/hw/arm-misc.h
> index 1f96229..c313027 100644
> --- a/hw/arm-misc.h
> +++ b/hw/arm-misc.h
> @@ -25,7 +25,7 @@ qemu_irq *armv7m_init(MemoryRegion *address_space_mem,
>
>  /* arm_boot.c */
>  struct arm_boot_info {
> -    int ram_size;
> +    target_phys_addr_t ram_size;
>      const char *kernel_filename;
>      const char *kernel_cmdline;
>      const char *initrd_filename;
> --
> 1.7.1
>
Andreas Färber - July 6, 2012, 1:48 p.m.
Am 05.07.2012 19:00, schrieb Peter Maydell:
> Make the RAM size in arm_boot_info a target_phys_addr_t so
> it can express RAM sizes up to the limit imposed by the
> physical address size.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm-misc.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/arm-misc.h b/hw/arm-misc.h
> index 1f96229..c313027 100644
> --- a/hw/arm-misc.h
> +++ b/hw/arm-misc.h
> @@ -25,7 +25,7 @@ qemu_irq *armv7m_init(MemoryRegion *address_space_mem,
>  
>  /* arm_boot.c */
>  struct arm_boot_info {
> -    int ram_size;
> +    target_phys_addr_t ram_size;
>      const char *kernel_filename;
>      const char *kernel_cmdline;
>      const char *initrd_filename;

Didn't we conclude in lengthy and emotional discussions that int64_t was
the best compromise to solve the highbank and pseries image loading issues?

What I still dislike about target_phys_addr_t is that "ram_size" is not
an address but a size.

Andreas
Alexander Graf - July 6, 2012, 1:54 p.m.
On 06.07.2012, at 15:48, Andreas Färber wrote:

> Am 05.07.2012 19:00, schrieb Peter Maydell:
>> Make the RAM size in arm_boot_info a target_phys_addr_t so
>> it can express RAM sizes up to the limit imposed by the
>> physical address size.
>> 
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> hw/arm-misc.h |    2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>> 
>> diff --git a/hw/arm-misc.h b/hw/arm-misc.h
>> index 1f96229..c313027 100644
>> --- a/hw/arm-misc.h
>> +++ b/hw/arm-misc.h
>> @@ -25,7 +25,7 @@ qemu_irq *armv7m_init(MemoryRegion *address_space_mem,
>> 
>> /* arm_boot.c */
>> struct arm_boot_info {
>> -    int ram_size;
>> +    target_phys_addr_t ram_size;
>>     const char *kernel_filename;
>>     const char *kernel_cmdline;
>>     const char *initrd_filename;
> 
> Didn't we conclude in lengthy and emotional discussions that int64_t was
> the best compromise to solve the highbank and pseries image loading issues?
> 
> What I still dislike about target_phys_addr_t is that "ram_size" is not
> an address but a size.

But isn't MAX(size) always defined to be smaller than or equal to MAX(addr)? So target_phys_addr_t is _always_ a type that is big enough to hold the information.


Alex
Andreas Färber - July 6, 2012, 2:02 p.m.
Am 06.07.2012 15:54, schrieb Alexander Graf:
> 
> On 06.07.2012, at 15:48, Andreas Färber wrote:
> 
>> Am 05.07.2012 19:00, schrieb Peter Maydell:
>>> Make the RAM size in arm_boot_info a target_phys_addr_t so
>>> it can express RAM sizes up to the limit imposed by the
>>> physical address size.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>> hw/arm-misc.h |    2 +-
>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/hw/arm-misc.h b/hw/arm-misc.h
>>> index 1f96229..c313027 100644
>>> --- a/hw/arm-misc.h
>>> +++ b/hw/arm-misc.h
>>> @@ -25,7 +25,7 @@ qemu_irq *armv7m_init(MemoryRegion *address_space_mem,
>>>
>>> /* arm_boot.c */
>>> struct arm_boot_info {
>>> -    int ram_size;
>>> +    target_phys_addr_t ram_size;
>>>     const char *kernel_filename;
>>>     const char *kernel_cmdline;
>>>     const char *initrd_filename;
>>
>> Didn't we conclude in lengthy and emotional discussions that int64_t was
>> the best compromise to solve the highbank and pseries image loading issues?
>>
>> What I still dislike about target_phys_addr_t is that "ram_size" is not
>> an address but a size.
> 
> But isn't MAX(size) always defined to be smaller than or equal to MAX(addr)? So target_phys_addr_t is _always_ a type that is big enough to hold the information.

I'm not disputing that. If you do
typedef target_phys_addr_t/* or whatever */ target_phys_size_t;
target_phys_size_t ram_size;
then I'm happy as well, I just dislike writing target_phys_addr_t size.

Andreas
Peter Maydell - July 6, 2012, 2:04 p.m.
On 6 July 2012 14:48, Andreas Färber <afaerber@suse.de> wrote:
> Am 05.07.2012 19:00, schrieb Peter Maydell:
>> Make the RAM size in arm_boot_info a target_phys_addr_t so
>> it can express RAM sizes up to the limit imposed by the
>> physical address size.

> Didn't we conclude in lengthy and emotional discussions that int64_t was
> the best compromise to solve the highbank and pseries image loading issues?

uint64_t, but yes, you're right. Will change.

-- PMM

Patch

diff --git a/hw/arm-misc.h b/hw/arm-misc.h
index 1f96229..c313027 100644
--- a/hw/arm-misc.h
+++ b/hw/arm-misc.h
@@ -25,7 +25,7 @@  qemu_irq *armv7m_init(MemoryRegion *address_space_mem,
 
 /* arm_boot.c */
 struct arm_boot_info {
-    int ram_size;
+    target_phys_addr_t ram_size;
     const char *kernel_filename;
     const char *kernel_cmdline;
     const char *initrd_filename;