Re: [PATCH v2 1/5] powerpc/mm: Enhance 'slice' for supporting PPC32

From: Segher Boessenkool <segher@kernel.crashing.org>
To: christophe leroy <christophe.leroy@c-s.fr>
Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Paul Mackerras <paulus@samba.org>,
	Michael Ellerman <mpe@ellerman.id.au>,
	Scott Wood <oss@buserror.net>,
	linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/5] powerpc/mm: Enhance 'slice' for supporting PPC32
Date: Sat, 20 Jan 2018 11:56:55 -0600	[thread overview]
Message-ID: <20180120175655.GY21977@gate.crashing.org> (raw)
In-Reply-To: <36e8d873-4021-4266-bf5f-287f396ba9e1@c-s.fr>

Hi!

On Sat, Jan 20, 2018 at 09:22:50AM +0100, christophe leroy wrote:
> >>>>>>>On PPC32, the address space is limited to 4Gbytes, hence only the 
> >>>>>>>low
> >>>>>>>slices will be used. As of today, the code uses
> >>>>>>>SLICE_LOW_TOP (0x100000000ul) and compares it with addr to determine
> >>>>>>>if addr refers to low or high space.
> >>>>>>>On PPC32, such a (addr < SLICE_LOW_TOP) test is always false because
> >>>>>>>0x100000000ul degrades to 0. Therefore, the patch modifies
> >>>>>>>SLICE_LOW_TOP to (0xfffffffful) and modifies the tests to
> >>>>>>>(addr <= SLICE_LOW_TOP) which will then always be true on PPC32
> >>>>>>>as addr has type 'unsigned long' while not modifying the PPC64
> >>>>>>>behaviour.

It should work to define SLICE_LOW_TOP as 0x100000000ull and keep
everything else the same, no?

> >I don't think so. When I had the missing prototype, the compilation goes 
> >ok, including the final link. Which means at the end the code is not 
> >included since radix_enabled() evaluates to 0.
> >
> >Many many parts of the kernel are based on this assumption.
> 
> Segher, what is your opinion on the above ? Can we consider that a ' if 
> (nbits)' will always be compiled out when nbits is a #define constant, 
> or should we duplicate the macros as suggested in order to avoid 
> unneccessary 'if' test on platforms where 'nbits' is always not null by 
> definition ?

Doing things like

	if (nbits)
		some_undeclared_function();

will likely work in practice if the condition evaluates to false at
compile time, but a) it will warn; b) it is just yuck; and c) it will
not always work (for example, you get the wrong prototype in this case,
not lethal here with most ABIs, but ugh).

Just make sure to declare all functions, or define it to some empty
thing, or #ifdeffery if you have to.  There are many options, it is
not hard, and if it means you have to pull code further apart that is
not so bad: you get cleaner, clearer code.

Segher