Date: Wed, 28 Nov 2018 16:31:33 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Justin Hibbits <jhibbits@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r341103 - head/sys/powerpc/include Message-ID: <20181128151148.X1660@besplex.bde.org> In-Reply-To: <201811280248.wAS2miqW055485@repo.freebsd.org> References: <201811280248.wAS2miqW055485@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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. > 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. - 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). 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). Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20181128151148.X1660>