Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 9 Nov 2012 03:45:15 +1100 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        Marius Strobl <marius@alchemy.franken.de>
Cc:        svn-src-head@FreeBSD.org, Marius Strobl <marius@FreeBSD.org>, src-committers@FreeBSD.org, svn-src-all@FreeBSD.org, Bruce Evans <brde@optusnet.com.au>
Subject:   Re: svn commit: r242747 - head/sys/kern
Message-ID:  <20121109031318.J5271@besplex.bde.org>
In-Reply-To: <20121108135048.GQ80490@alchemy.franken.de>
References:  <201211080810.qA88AW8Y027373@svn.freebsd.org> <20121108205636.O3941@besplex.bde.org> <20121108135048.GQ80490@alchemy.franken.de>

next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 8 Nov 2012, Marius Strobl wrote:

> On Thu, Nov 08, 2012 at 09:41:29PM +1100, Bruce Evans wrote:
>> On Thu, 8 Nov 2012, Marius Strobl wrote:
>>
>>> Log:
>>> Make r242655 build on sparc64. While at it, make
>>> vm_{max,min}_kernel_address
>>> vm_offset_t as they should be.
>>
>> Er, they shouldn't be vm_offset_t.
>>
>>> Modified: head/sys/kern/kern_malloc.c
>>> ==============================================================================
>>> --- head/sys/kern/kern_malloc.c	Thu Nov  8 04:02:36 2012 (r242746)
>>> +++ head/sys/kern/kern_malloc.c	Thu Nov  8 08:10:32 2012 (r242747)
>>> @@ -186,11 +186,13 @@ struct {
>>> */
>>> static uma_zone_t mt_zone;
>>>
>>> -static u_long vm_min_kernel_address = VM_MIN_KERNEL_ADDRESS;
>>> +static vm_offset_t vm_min_kernel_address = VM_MIN_KERNEL_ADDRESS;
>>> SYSCTL_ULONG(_vm, OID_AUTO, min_kernel_address, CTLFLAG_RD,
>>>    &vm_min_kernel_address, 0, "Min kernel address");
>>
>> SYSCTL_ULONG takes a u_long, not a vm_offset_t.  A cast in
>> SYSCTL_ULONG() prevents possible detection of the type mismatch from
>> this.
>>
>> This is most broken on i386's with correctly-sized longs (or almost
>> any arch with correctly-sized longs).  There, vm_offset_t is 1
>> register wide and longs are 2 registers wide.
>
> Eh, FreeBSD/i386 is ILP32, so longs are 32-bit there, as is its
> __vm_offset_t.

Yes, it has incorrectly sized longs.  It used to have a _LARGE_LONG
option to give 64-bit longs, and I used this to find and fix some
type errors before there were any 64-bit arches (most things compiled
and parts of userland ran).

>> The bugs could be moved using SYSCTL_QUAD().  Correctly-sized quads
>> would be 4 registers wide, except quad has come to mean just a bad
>> way of spelling long long or int64_t.
>>
>>> -static u_long vm_max_kernel_address = VM_MAX_KERNEL_ADDRESS;
>>> +#ifndef __sparc64__
>>> +static vm_offset_t vm_max_kernel_address = VM_MAX_KERNEL_ADDRESS;
>>> +#endif
>>> SYSCTL_ULONG(_vm, OID_AUTO, max_kernel_address, CTLFLAG_RD,
>>>    &vm_max_kernel_address, 0, "Max kernel address");
>>
>> Since the value is a compile-time constant, u_long has a chance of
>> holding its value even if u_long != vm_offset_t, and the old code is
>> closer to being correct than first appeared, and the correct fix is
>> relatively easy -- just use a uintmax_t vatiable and SYSCTL_UINTMAX()
>> (*).  Unfortunately SYSCT_UINTMAX() doesn't exist, and the bogus
>> SYSCTL_UQUAD() does exist.
>
> Right, SYSCTL_UINTMAX() unfortunately doesn't exist. SYSCTL_UQUAD()
> seemed inappropriate as it is 64-bit and we want 32-bit for ILP32
> for an exact match. Using uintmax_t with SYSCTL_UQUAD() also just
> happens to be of the same width.
> From the available combinations, using vm_offset_t with SYSCTL_ULONG()
> just seemed to suck the least.

This is backwards, since it u_long that the API supports, so to use the
API it must be assumed that vm_offset_t can be punned to a u_long, not
vice versa although the difference is only logical.

On sparc64, as on most or all 64-bit arches, vm_offset_t is identical to
u_long, so this doesn't even involve type puns.  On i386's with
incorrectly-sized longs (ILP32), there is a type pun from vm_offset_t =
u_int to u_long.  On i386's with correctly-sized longs (I32L64P32),
there is a type mismatch.  In this case, making the variable u_long to
match the API works correctly (when the type error is not detected),
since the top 32 bits are just always zero because the sysctl is r/o
so no one can can change them from their initial zero values.  The
reverse dosen't work, since it reads 32 bits beyond the end of the
variable, giving garbage results.  The overrun would be a more harmful
for a r/w sysctl.

MD code mostly hard-codes assumptions that vm_offset_t is u_int or u_long
for printing it -- it doesn't laboriously convert vm_offset_t to uintmax_t
as is strictly necessary for printing all MI typedefs.  This corresponds
to punning vm_offset_t to u_long here, except the assumptions are much
larger in MI code.

>> (*) Except a temporary variable is just a style bug in the above and
>> for this.  SYSCTL_UNLONG(), like all integer SYSCTL()'s, has the feature
>> of supporting either a variable or a constant arg.  The above should
>> use a constant arg and not need a variable.  IIRC, the syntax for this is:
>>
>> SYSCTL_ULONG(_vm, OID_AUTO, max_kernel_address, CTLFLAG_RD,
>>     NULL, VM_MAX_KERNEL_ADDRESS, "Max kernel address");
>
> Actually, for sparc64 VM_MAX_KERNEL_ADDRESS isn't constant (and can't
> be).

I didn't notice that when I replied before.  The declaration would have
to be ifdefed, as you did but with the correct type for the MI variable.
The MD variable can't have the correct type for this since it needs to
remain vm_offset_t to match other APIs.  Also, the ifdefs are simpler
if a variable (with the same name) is used for all cases.

Bruce



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121109031318.J5271>