On 03/09/2012 07:21 AM, Alexander Graf wrote:
>
> On 09.03.2012, at 14:15, Mark Langsdorf wrote:
>
>> On 03/09/2012 03:25 AM, Markus Armbruster wrote:
>>> Mark Langsdorf <mark.langsd...@calxeda.com> writes:
>>>
>>>> Allow load_image_targphys to load files on systems with more than 2G of
>>>> emulated memory by changing the max_sz parameter from an int to an
>>>> unsigned long.
>>>>
>>>> Signed-off-by: Mark Langsdorf <mark.langsd...@calxeda.com>
>>>> ---
>>>> hw/loader.c | 4 ++--
>>>> hw/loader.h | 3 ++-
>>>> 2 files changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/loader.c b/hw/loader.c
>>>> index 415cdce..a5333d0 100644
>>>> --- a/hw/loader.c
>>>> +++ b/hw/loader.c
>>>> @@ -103,9 +103,9 @@ ssize_t read_targphys(const char *name,
>>>>
>>>> /* return the size or -1 if error */
>>>> int load_image_targphys(const char *filename,
>>>> - target_phys_addr_t addr, int max_sz)
>>>> + target_phys_addr_t addr, unsigned long max_sz)
>>>> {
>>>> - int size;
>>>> + unsigned long size;
>>>>
>>>> size = get_image_size(filename);
>>>> if (size > max_sz) {
>>>
>>> get_image_size() returns int. How does widening size and max_sz here
>>> improve things?
>>
>> If max_sz is greater than 2GB, then:
>> int max_sz = 0xc0000000;
>> int size = 0x300;
>> if (size > max_sz)
>> return -1;
>>
>> returns -1, even though size is much less than max_sz.
>>
>> doing it my way:
>> unsigned long max_sz = 0xc0000000;
>> unsigned long size = 0x300;
>> if (size > max_sz)
>> return -1;
>>
>> does not return -1.
>
> So why change it to long then? Unsigned int would have the same effect.
Well, I hit this bug because the arm_loader code passes the system's
memory size as the max_sz argument. Currently, we have a 32-bit memory
bus, but I know we'll move to 64-bits in the future, and I wanted to
be type safe.
> But really what this should be changed to is target_phys_addr_t. That
> way it's aligned with the address. I guess we can leave int for return
> values for now though, since we won't get images that big.
Or convert all this stuff to size_t, since that's also appropriate.
> Also, why are you hitting this in the first place? How are you calling
> read_targphys that you end up with such a big max_sz? Using INT_MAX
> as max_sz should work just as well, no? It's probably cleaner to
> change the size type, but I'm curious why nobody else hit this before :).
Well, arm_load_kernel calls read_targphys and passes
(ram_size - KERNEL_LOAD_ADDR) as the max_sz argument. As far as I know,
the highbank model is the only ARM model that uses more than 2 GB as
ram_size. The change that actually tested the max_sz argument didn't
go in until January 2012, and our internal tree was lagging until
quite recently, so our testing didn't catch it either.
I'll resubmit the patch with target_phys_addr_t.
--Mark Langsdorf
Calxeda, Inc.
