Re: [Qemu-devel] [PATCH] use an unsigned long for the max_sz parameter in load_image_targphys

[Alexander Graf]

On 09.03.2012, at 14:34, Mark Langsdorf wrote:

> 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 <address@hidden> 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 <address@hidden>
>>>>> ---
>>>>> 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.

Then use uint64_t or target_phys_addr_t really. Longs are almost always wrong 
in qemu code, because the guest shouldn't care about the host's bitness.

> 
>> 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.

Semantically, I would rather go with target_phys_addr_t. You're trying to 
describe addresses. If anything, ram_addr_t might work too - never quite 
grasped the difference.

> 
>> 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.

Ah, that makes sense :).

> I'll resubmit the patch with target_phys_addr_t.

Thanks! And please check for the other loaders too if they suffer from the same 
badness.


Alex