Date: Mon, 3 Dec 2018 10:20:07 -0600 From: Justin Hibbits <jhibbits@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> 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: <20181203102007.4021aa32@ralga.knownspace> In-Reply-To: <20181128151148.X1660@besplex.bde.org> References: <201811280248.wAS2miqW055485@repo.freebsd.org> <20181128151148.X1660@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 28 Nov 2018 16:31:33 +1100 (EST) Bruce Evans <brde@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20181203102007.4021aa32>