Date: Sun, 3 Jan 2016 12:24:45 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Ian Lepore <ian@freebsd.org> 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 Message-ID: <20160103112530.Q1220@besplex.bde.org> In-Reply-To: <201601021816.u02IGOXQ060620@repo.freebsd.org> References: <201601021816.u02IGOXQ060620@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160103112530.Q1220>