From owner-svn-src-head@freebsd.org Sun Jan 3 01:24:53 2016 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 1F0D1A5F10F; Sun, 3 Jan 2016 01:24:53 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail105.syd.optusnet.com.au (mail105.syd.optusnet.com.au [211.29.132.249]) by mx1.freebsd.org (Postfix) with ESMTP id 955C7147B; Sun, 3 Jan 2016 01:24:52 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c211-30-166-197.carlnfd1.nsw.optusnet.com.au (c211-30-166-197.carlnfd1.nsw.optusnet.com.au [211.30.166.197]) by mail105.syd.optusnet.com.au (Postfix) with ESMTPS id 24C931045F9D; Sun, 3 Jan 2016 12:24:45 +1100 (AEDT) Date: Sun, 3 Jan 2016 12:24:45 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Ian Lepore cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r293053 - head/sys/boot/uboot/lib In-Reply-To: <201601021816.u02IGOXQ060620@repo.freebsd.org> Message-ID: <20160103112530.Q1220@besplex.bde.org> References: <201601021816.u02IGOXQ060620@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=PfoC/XVd c=1 sm=1 tr=0 a=KA6XNC2GZCFrdESI5ZmdjQ==:117 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=kj9zAlcOel0A:10 a=uh6MPLOrl7Xpo6US5dgA:9 a=9R64MTe1H_08cVun:21 a=cwHEBaPuE8GakeBG:21 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 03 Jan 2016 01:24:53 -0000 On Sat, 2 Jan 2016, Ian Lepore wrote: > Log: > Use 64-bit math when finding a block of ram to hold the kernel. This fixes > a problem on 32-bit systems which have ram occupying the end of the physical > address space -- for example, a block of ram at 0x80000000 with a size of > 0x80000000 was overflowing 32 bit math and ending up with a calculated size > of zero. > > This is a fix for one of the two problems mentioned in the PR. Something > similar will need to be done on the kernel side before the PR is closed. I don't like this. It gives a type morass, and has no effect if uintptr_t has 64 bits, and is just broken when uintptr_t has more that 64-bits. The corresponding hack for 64-bit arches is to use uint128_t, and for 128-bit arches to use uint256_t, and that is more obviously excessive and more likely to expose type errors (for example, __uint128_t compiles, but is unprintable since printf() hasn't dreamed of its existence yet, and its existence breaks uintmax_t since it is larger than uintmax_t). I see that you committed related changes in many places. Only the one in arm/physmem.c looks right -- don't change types, but handle the top page specially. I once looked at vm to see how it handles this. It seemed to use (offset, size) pairs a lot where it would simpler to use offset+size except for the problem that this would overflow. This would avoid most problems with the top page. Eventually I decided that it didn't really care about the top page but was just doing this because it was over-engineered. > Modified: head/sys/boot/uboot/lib/copy.c > ============================================================================== > --- head/sys/boot/uboot/lib/copy.c Sat Jan 2 18:15:10 2016 (r293052) > +++ head/sys/boot/uboot/lib/copy.c Sat Jan 2 18:16:24 2016 (r293053) > @@ -69,11 +69,11 @@ uint64_t > uboot_loadaddr(u_int type, void *data, uint64_t addr) > { > struct sys_info *si; > - uintptr_t sblock, eblock, subldr, eubldr; > - uintptr_t biggest_block, this_block; > - size_t biggest_size, this_size; > + uint64_t sblock, eblock, subldr, eubldr; > + uint64_t biggest_block, this_block; > + uint64_t biggest_size, this_size; This obviously breaks the 128-bit case. I replied out of order about a fixup for the type morass created by this, and said that subldr and euldr have type uintptr_t. They only had this correct (less incorrect) type before this change. Making everything 64-bit mostly avoids the type morass but gives a lot of bloat in the 32-bit case. It doesn't completely avoid the type morass since sizeof() still gives type uint32_t in the 32-bit case. > @@ -100,14 +100,15 @@ uboot_loadaddr(u_int type, void *data, u > > biggest_block = 0; > biggest_size = 0; > - subldr = rounddown2((uintptr_t)_start, KERN_ALIGN); My previous reply was about this. This was correct except for using uintptr_t instead of uintfptr_t. > - eubldr = roundup2(uboot_heap_end, KERN_ALIGN); > + subldr = rounddown2((uint64_t)_start, KERN_ALIGN); This has various type errors -- see previous reply. > + eubldr = roundup2((uint64_t)uboot_heap_end, KERN_ALIGN); Most of the uintptr_t types were also wrong. vm_offset_t or vm_addr_t or vm_paddr_t or vm_size_t would be better (I think the boot code is low enough level to use these instead of Standard "portable" types). Except vm_addr_t doesn't exist. vm has its own type morass, but not enough complications to express the difference beteen offsets and addresses in the type (virtual addresses have to abuse vm_offset_t and physical offsets have to abuse vm_physaddr_t). OTOH, all these types except vm_paddr_t are the same, so it is too easy to misuse vm_size_t for an address of offset. Using uintptr_t was wrong since it is just a C type large enough to represent pointers. It has little relation to virtual addresses and no relation to physical addresses. It tends to be large enough to represent virtual addresses since it has to be large enough to represent all C pointers uniquely. But this is not required. Perhaps a general pointer has 36 bits but C pointers have only 32 bits, or C pointers have 36 bits but can be encoded uniquely in 32 bits and casting them to uint32_t does the encoding. Boot code and vm should operate at a lower level than C pointers and use raw offsets (probably represented as raw bits). Even casting pointers to uintptr_t is technically wrong. If this cast does special encoding, then it is probably not what is wanted. A recalcitrant implement of C could use the simple encoding of reversing all the bits. The resulting uintptr_t's would be useless for most existing (ab)uses of the result. Boot and vm code is low enough level to be honestly unportable and convert the bits using a type pun, or better memcpy(), or better still, portable using a conversion function that usually just casts through uintptr_t for virtual addresses. Bruce