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

["Andreas Färber" <afaerber@suse.de>]

Am 09.03.2012 14:50, schrieb 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 <mark.langsdorf@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.langsdorf@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.
> 
> 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.

No, please. We're describing sizes, not addresses. target_phys_addr_t
thus is semantically wrong here. The RAM size is unsigned long IIRC (it
is limited by the host's available memory). If you subtract something
from a size it remains a size. I had therefore suggested size_t before.
I expect sizeof(size_t) >= sizeof(unsigned long).

In the previous discussion someone suggested off_t due to some function
used internally returning it. I don't know the exact difference between
the two, but off_t still sounds wrong to me since we want a size, not a
file offset.

Andreas

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg