svn commit: r341103 - head/sys/powerpc/include

[Justin Hibbits]

On Wed, 28 Nov 2018 16:31:33 +1100 (EST)
Bruce Evans <brde at optusnet.com.au> wrote:

> On Wed, 28 Nov 2018, Justin Hibbits wrote:
> 
> > Log:
> >  powerpc: Fix the powerpc64 build post-r341102
> >
> >  VM_MIN_KERNEL_ADDRESS is now used in locore.S, but the UL suffix
> > isn't permitted in .S files.  
> 
> The UL suffix is arguably a style bug in .c files too.  It was not
> even wrong (it had no effect) this case, but nearby code seems to be
> more broken.
> 
> A ULL suffix would be unarguably a style bug everywhere.

I'll take a closer look at this eventually.  I'm in the process of
overhauling a lot of the Book-E bits to play nicer with loader, etc.
This does involve touching vmparam.h a lot, so I'll think about
cleaning it up for these cases as well.

> 
> > Modified: head/sys/powerpc/include/vmparam.h
> > ==============================================================================
> > --- head/sys/powerpc/include/vmparam.h	Wed Nov 28 02:00:27
> > 2018	(r341102) +++ head/sys/powerpc/include/vmparam.h
> > Wed Nov 28 02:48:43 2018	(r341103) @@ -106,8 +106,13 @@
> > #define	FREEBSD32_USRSTACK	FREEBSD32_SHAREDPAGE
> >
> > #ifdef __powerpc64__
> > +#ifndef LOCORE
> > #define	VM_MIN_KERNEL_ADDRESS
> > 0xe000000000000000UL #define
> > VM_MAX_KERNEL_ADDRESS		0xe0000007ffffffffUL +#else
> > +#define	VM_MIN_KERNEL_ADDRESS
> > 0xe000000000000000 +#define
> > VM_MAX_KERNEL_ADDRESS		0xe0000007ffffffff +#endif  
> 
> These constants automatically have type unsigned long, since they are
> larger that LONG_MAX and smaller than ULONG_MAX.  Thus the UL suffix
> had no effect except to break in asm files.
> 
> This would not be true for smaller constants.  Smaller constants often
> need to be combined or shifted back and forth, and then it may be
> necessary to use them as unsigned int or unsigned long constants
> (signed constants are better handled by implicit conversions between
> int and long unless they are mixed with unsigned constants).  This is
> sometimes done using U or UL or suffixes.  I don't like this.  Cases
> where the natural type doesn't work are delicate and it is better to
> not hide the unnatural conversions by implicitly converting using a
> suffix on some constants.
> 
> The correct fix is to remove the bogus UL suffixes and not add any
> ifdefs.
> 
> On 32-bit arches, the above constants would have natural type
> [(long long abomination; should be deleted].  The abominatation is
> unavoidable, but badly written code increases it using explicit
> [abomination deleted] suffixes.
> 
> > #define	VM_MAX_SAFE_KERNEL_ADDRESS
> > VM_MAX_KERNEL_ADDRESS #endif  
> 
> Nearby code has mounds of unportabilities and style bugs.  E.g.,
> - VM_MIN_ADDRESS for the !LOCORE && 64-bit case has bogus UL suffixes
> and bogus parentheses around single tokens.  It is 0, so it has
> natural type int, so  the suffix might be needed to obfuscate
> conversions to unsigned long in some cases.
> - VM_MIN_ADDRESS for the !LOCORE && 32-bit case casts to vm_offset_t
> instead of hard-coding the same conversion using a suffix.  The cast
> is more technically correct, but is an even larger syntax error -- it
> breaks both asm uses and uses in cpp expressions.

Indeed, the cast is probably redundant.

> - VM_MIN_ADDRESS already had the bogus ifdefs to support LOCORE.
> These are expressed in the opposite order (LOCORE is in the outer
> ifdef instead of the inner ifdef), which makes them more unreadable.
> - similarly for VM_MAXUSER_ADDRESS, plus an extra convolution to
> define this in terms of VM_MAXUSER_ADDRESS32 in the 32-bit case, but
> only for the !LOCORE case (since VM_MAXUSER_ADDRESS32 doesn't have a
> LOCORE ifdef).

Agreed.  Removing the LOCORE differentiation would certainly clean up
the file.

> 
> I used to like casts on constants and macros, like the ones here for
> vm_offset_t here, but jake@ convinced me that this was wrong in
> connection with PAE and sparc64 work.  The wrongness is most obvious
> for PAE.  vm_paddr_t is 64 bits for PAE, but most vm types for PAE
> (especially vm_offset_t) are only 32 bits.  If you sprinkle
> [abomination deleted] suffixes or casts to uint64_t or vm_paddr_t on
> constants or macros, then this is too pessimal and/or confusing for
> most vm expressions.  Sprinkling UL suffixes and vm_offset_t casts is
> relatively harmless only because it usually has no effect, especially
> on 32-bit non-PAE arches where even more types are naturally 32 bits.
> 
> The i386 vmparam.h has the following style bugs in this area:
> - UL suffixes only for MAXTSIZ and friends
> - cast to vm_offset_t only for VM_MIN_KERNEL_ADDRESS
> - VM_MAX_KERNEL_ADDRESS and related values are encrypted through about
>    10 layers of macros depending on configuration variables like PAE.
>    Most of the encryption is no longer really used, since it was
> mostly to make addresses depend on the user/kernel split.
> 
>    The encryptions mostly use signed constants and expressions, but a
>    critical one is NPDEPTD defined in another file using sizeof().
>    Unsigned poisoning from this probably leaks into most macros, but
>    the resulting types are even less clear than the resulting values
>    since the resulting values are now documented for the only
> supported user/kernel split.
> 
> - bogus extra parentheses only for VM_KMEM_SIZE_SCALE.
> 
>    The existence and default value of this macro are also wrong.  They
>    were last almost correct when the default user/kernel split was 3/1
>    and the physical memory size was 3 or 4 GB.
> 
>    The default for the macro is 3, which is closely related to the
> split ratio.  This value works to prevent overflow in kva size
> calculations when the physical memory size is larger than the total
> kva size but less than 3 or 4 GB.  Overflow still occurs for PAE
> since then the memory size can be larger than 4GB.  Other magic
> properties of 3 result in the overflows giving reasonable values for
> the kva sizes when the memory size is 8GB or 16GB, but not when it is
> 12GB (because 12GB / 3 = 4GB = 0GB after overflow, and 0GB is not a
> useful kva size).

Thanks for the x86 lesson with this.

> 
> Bruce

- Justin