From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45426) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S60nn-0007sJ-11 for qemu-devel@nongnu.org; Fri, 09 Mar 2012 09:28:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S60nk-0002CD-QO for qemu-devel@nongnu.org; Fri, 09 Mar 2012 09:28:14 -0500 Received: from cantor2.suse.de ([195.135.220.15]:38273 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S60nk-0002C4-GK for qemu-devel@nongnu.org; Fri, 09 Mar 2012 09:28:12 -0500 Message-ID: <4F5A1378.5040909@suse.de> Date: Fri, 09 Mar 2012 15:28:08 +0100 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1331225951-31306-1-git-send-email-mark.langsdorf@calxeda.com> <4F5A0288.6090809@calxeda.com> <9A556F7F-2721-4D51-80D2-8B9571E4EC0F@suse.de> <4F5A06C8.7020903@calxeda.com> <88D21EA9-1000-48DB-9D7A-437622BE8745@suse.de> In-Reply-To: <88D21EA9-1000-48DB-9D7A-437622BE8745@suse.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] use an unsigned long for the max_sz parameter in load_image_targphys List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf , Mark Langsdorf Cc: "qemu-devel@nongnu.org" , Peter Maydell , Markus Armbruster , "david@gibson.dropbear.id.au" Am 09.03.2012 14:50, schrieb Alexander Graf: >=20 > On 09.03.2012, at 14:34, Mark Langsdorf wrote: >=20 >> 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 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 >>>>>> --- >>>>>> 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 ma= x_sz) >>>>>> { >>>>>> - int size; >>>>>> + unsigned long size; >>>>>> >>>>>> size =3D get_image_size(filename); >>>>>> if (size > max_sz) { >>>>> >>>>> get_image_size() returns int. How does widening size and max_sz he= re >>>>> improve things? >>>> >>>> If max_sz is greater than 2GB, then: >>>> int max_sz =3D 0xc0000000; >>>> int size =3D 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 =3D 0xc0000000; >>>> unsigned long size =3D 0x300; >>>> if (size > max_sz) >>>> return -1; >>>> >>>> does not return -1. >>> >>> So why change it to long then? Unsigned int would have the same effec= t. >> >> 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. >=20 > 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 bi= tness. >=20 >> >>> 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 retur= n >>> 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. >=20 > 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 qui= te grasped the difference. >=20 >> >>> Also, why are you hitting this in the first place? How are you callin= g >>> 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. >=20 > Ah, that makes sense :). >=20 >> 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) >=3D 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. >=20 >=20 > Alex --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg