svn commit: r314087 - head/sys/x86/x86

[Bruce Evans]

On Fri, 24 Feb 2017, Konstantin Belousov wrote:

> On Thu, Feb 23, 2017 at 06:33:43AM +1100, Bruce Evans wrote:
>> On Wed, 22 Feb 2017, Konstantin Belousov wrote:
>>
>>> Log:
>>>  More fixes for regression in r313898 on i386.
>>>  Use long long constants where needed.
>>
>> The long long abomination is never needed, and is always a style bug.
> I never saw any explanation behind this claim.  Esp. the first part
> of it, WRT 'never needed'.

I hope I wrote enough about this in log messages when I cleaned up the
long longs 20 years ago :-).

long long was a hack to work around intmax_t not existing and long being
unexpandable in practice because it was used in ABIs.  It should have gone
away when intmax_t was standardized.  Unfortunately, long long was
standardised too.

It is "never needed" since anything that can be done with it can be done
better using intmax_t or intN_t or int_fastN_T or int_leastN_t.  Except,
there is no suffix for explicit intmax_t constants, so you would have to
write such constants using INTMAX_C() or better a cast to intmax_t if
the constant is not needed in a cpp expression.

>> I don't like using explicit long constants either.  Here the number of bits
>> in the register is fixed by the hardware at 64.  The number of bits in a
>> long on amd64 and a long on i386 is only fixed by ABI because the ABI is
>> broken for historical reasons.
> I really cannot make any sense of this statement.

To know that the ULL suffix is correct for 64-bit types, you have to now
that long longs are 64 bits on all arches supported by the code.  Then
to use this suffix, you have to hard-code this knowledge.  Then to read
the code, the reader has to translate back to 64 bits.  The translations
were easier 1 arch at a time.  Casting to uint64_t is clearer, but doesn't
work in cpp expressions.  In cpp expressions, use UINT64_C().  Almost no
one knows about it uses this.  There are 5 examples of using it in /sys
(3 in arm64 pte.h, 1 in powerpc pte.h, and 1 in mips xlr_machdep.c,
where the use is unnecessary but interesting: it is ~UINT64_C(0).  We
used to have squillions of magic ~0's for the rman max limit.  This was
spelled ~0U, ~0UL and perhaps even plain ~0.  Plain ~0 worked best except
on unsupported 1's complement machines, since it normally gets sign extended
to as many bits as necessary.  Now this is spelled RM_MAX_END, which is
implemented non-magically using a cast: (~(rman_res_t)0).  Grepping for
~0[uU] in dev/* shows only 1 obvious unconverted place.

>>  Only very MD code can safely assume the
>> size of long and long long.  This code was MD enough before it was merged,
>> but now it shouldn't use long since that varies between amd64 and i386,
>> and it shouldn't use long long since that is a style bug.
>
> Well, I do not see anything wrong with long long, at least until
> explained.
>
> Anyway, below is the patch to use uint64_t cast in important place,
> and removal of LL suffix in unimportant expression.

This is correct.

> diff --git a/sys/x86/x86/x86_mem.c b/sys/x86/x86/x86_mem.c
> index d639224f840..8bc4d3917a0 100644
> --- a/sys/x86/x86/x86_mem.c
> +++ b/sys/x86/x86/x86_mem.c
> @@ -260,7 +260,7 @@ x86_mrfetch(struct mem_range_softc *sc)
>
> 		/* Compute the range from the mask. Ick. */
> 		mrd->mr_len = (~(msrv & mtrr_physmask) &
> -		    (mtrr_physmask | 0xfffLL)) + 1;
> +		    (mtrr_physmask | 0xfff)) + 1;
> 		if (!mrvalid(mrd->mr_base, mrd->mr_len))
> 			mrd->mr_flags |= MDF_BOGUS;
>
> @@ -638,7 +638,8 @@ x86_mrinit(struct mem_range_softc *sc)
> 	 * Determine the size of the PhysMask and PhysBase fields in
> 	 * the variable range MTRRs.
> 	 */
> -	mtrr_physmask = (((uint64_t)1 << cpu_maxphyaddr) - 1) & ~0xfffULL;
> +	mtrr_physmask = (((uint64_t)1 << cpu_maxphyaddr) - 1) &
> +	    ~(uint64_t)0xfff;
>
> 	/* If fixed MTRRs supported and enabled. */
> 	if ((mtrrcap & MTRR_CAP_FIXED) && (mtrrdef & MTRR_DEF_FIXED_ENABLE)) {

Now I wonder what these magic 0xfff's are.  Are they PAGE_MASK, where the
register is encoded like a page table to put metadata in the low bits?

There is already a macro MTRR_PHYSBASE_PHYSMASK which looks like it should
be used here, except it is zero in the top 12 bits too.

There is no macro for 0xfff, but you get that by ORing the bits for other
macros.  0xfff is just as readable.

The MTRR_* macros are in x86/specialreg.h, and are spelled without ULL
suffixes.  I prefer the latter, but seem to rememeber bugs elsewhere
caused by using expressions like ~FOO where FOO happens to be small.
Actually the problems are mostly when FOO happens to be between
INT_MAX+1U and UINT_MAX.  When FOO is small and has no suffix, e.g.,
if it is 0, then its type is int and ~FOO has type int and sign-extends
to 64 bits if necessary.  But if FOO is say 0x80000000, it has type u_int
so ~FOO doesn't sign-extend.  (Decimal constants without a suffix never
have an unsigned type and the hex constant here lets me write this number
and automatically give it an unsigned type.  Normally this type is best.)

Explicit type suffixes mainly hide these problems.  If FOO is 0x80000000ULL,
then it has the correct type for ~FOO to work in expressions where everything
has type unsigned long long, but in other expressions a cast might still
be needed.

Bruce