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

[Bruce Evans]

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 removed almost all long long constants ~20 years ago, but there are
now thousands more than when I started.

> Modified: head/sys/x86/x86/x86_mem.c
> ==============================================================================
> --- head/sys/x86/x86/x86_mem.c	Wed Feb 22 06:43:49 2017	(r314086)
> +++ head/sys/x86/x86/x86_mem.c	Wed Feb 22 07:07:05 2017	(r314087)
> @@ -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 | 0xfffL)) + 1;
> +		    (mtrr_physmask | 0xfffLL)) + 1;

Not needed here.  The old i386 version did spell it like this.

> 		if (!mrvalid(mrd->mr_base, mrd->mr_len))
> 			mrd->mr_flags |= MDF_BOGUS;
>
> @@ -638,7 +638,7 @@ 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) & ~0xfffUL;
> +	mtrr_physmask = (((uint64_t)1 << cpu_maxphyaddr) - 1) & ~0xfffULL;

A 64-bit constant is needed here, but spelling it with ULL is a larger
style bug than usual, since the other 64-bit constant on the same line
is spelled without ULL.

The old i386 version spelled both of the constants on this line with ULL,
and the old amd64 version spelled them both with UL, but someone named kib
fixed the style bug for the first and added the type error for the second
when merging them.

>
> 	/* If fixed MTRRs supported and enabled. */
> 	if ((mtrrcap & MTRR_CAP_FIXED) && (mtrrdef & MTRR_DEF_FIXED_ENABLE)) {

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.  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.

x86/x86 only has 17 lines using u_long, and all are wrong:
- most are for counters.  Some counters should be 64 bits, but changing
   them on i386 would cause portability problems.
- ones for lapic timer divisors and frequency should be just int or
   possibly u_register_t
- ones for 16-bit segment registers should be just int or possibly uint16_t
- ones for cr0 and cr4 should be u_register_t.

x86/x86 has 40 lines using long.  Many of the other 23 are wronger:
- some in comments are not about the long type and are not wrong
- many are in comments which say that the resource type is long, but the
   resource type is now rman_res_t = uintmax_t.  It never was signed and
   is now larger than u_long on i386.  Some nearby types are wrong to
   match.  E.g., in nexus_add_irq(), the irq number should be int but
   is u_long.  This u_long matched the old rman type exactly, but now
   gets converted by a prototype.  There is a non-style bug here: smap
   handling above 4GB is turned off for i386 and PAE, with a comment
   saying that this is because resources use long's (sic).  There are
   2 copies of the code for this, with the type suffix spelled as ul
   instead of UL.  ~0ul is a magic i386 way of spelling 4GB-1.  It
   only works because it is under i386 ifdefs.  This is in nexus.c.
   nexus.c otherwise doesn't use 0ul or 0UL.
- some for lapics are for small integers and should be just int
- many in mca.c are the long long abomination used for printf()s and
   should be [u]intmax_t
- 1 in pvclock.c is u_long spelled verbosely as unsigned long
- many in stack_machdep.c are in bogus casts of pointers.  These should
   use uintptr_t.  Casting pointers to access them using atomic ops is
   bogus using (uintptr_t *) too.  uintptr_t is only valid for casting
   pointers directly.  Of course it works indirectly since everything has
   the same width as register_t.

Bruce