Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Michael Ellerman <mpe@ellerman.id.au>
To:     Christophe Leroy <christophe.leroy@c-s.fr>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Paul Mackerras <paulus@samba.org>
Cc:     linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v3] powerpc: implement CONFIG_DEBUG_VIRTUAL
In-Reply-To: <87y38nrkto.fsf@concordia.ellerman.id.au>
References: <a42260e77d609a10667872c2e7bf14ceabcce050.1544562518.git.christophe.leroy@c-s.fr> <87y38nrkto.fsf@concordia.ellerman.id.au>
Date:   Wed, 19 Dec 2018 11:26:00 +1100
Message-ID: <87a7l2s6p3.fsf@concordia.ellerman.id.au>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Michael Ellerman <mpe@ellerman.id.au> writes:
> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>
>> This patch implements CONFIG_DEBUG_VIRTUAL to warn about
>> incorrect use of virt_to_phys() and page_to_phys()
>
> This commit is breaking my p5020ds booting a 32-bit kernel with:
>
>   smp: Bringing up secondary CPUs ...
>   __ioremap(): phys addr 0x7fef5000 is RAM lr ioremap_coherent
>   Unable to handle kernel paging request for data at address 0x00000000
>   Faulting instruction address: 0xc002e950
>   Oops: Kernel access of bad area, sig: 11 [#1]
>   BE SMP NR_CPUS=24 CoreNet Generic
>   Modules linked in:
>   CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.20.0-rc2-gcc-7.0.1-00138-g9a0380d299e9 #148
>   NIP:  c002e950 LR: c002eb20 CTR: 00000001
>   REGS: e804bd20 TRAP: 0300   Not tainted  (4.20.0-rc2-gcc-7.0.1-00138-g9a0380d299e9)
>   MSR:  00021002 <CE,ME>  CR: 28004222  XER: 00000000
>   DEAR: 00000000 ESR: 00000000 
>   GPR00: c002eb20 e804bdd0 e8050000 00000000 00021002 00000000 00000050 00021002 
>   GPR08: 2d3f0000 00000001 00000000 00000004 24000842 00000000 c00026d0 00000000 
>   GPR16: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000001 
>   GPR24: 00029002 7fef5140 30000000 00000000 00000000 00000040 00000001 00000000 
>   NIP [c002e950] smp_85xx_kick_cpu+0x120/0x410
>   LR [c002eb20] smp_85xx_kick_cpu+0x2f0/0x410
>   Call Trace:
>   [e804bdd0] [c002eb20] smp_85xx_kick_cpu+0x2f0/0x410 (unreliable)
>   [e804be20] [c0012e38] __cpu_up+0xc8/0x230
>   [e804be50] [c0040b34] bringup_cpu+0x34/0x110
>   [e804be70] [c00418a8] cpu_up+0x128/0x250
>   [e804beb0] [c0b84b14] smp_init+0xc4/0x10c
>   [e804bee0] [c0b75c1c] kernel_init_freeable+0xc8/0x250
>   [e804bf20] [c00026e8] kernel_init+0x18/0x120
>   [e804bf40] [c0011298] ret_from_kernel_thread+0x14/0x1c
>   Instruction dump:
>   7fb3e850 57bdd1be 2e1d0000 41d20250 57bd3032 393dffc0 7e6a9b78 5529d1be 
>   39290001 7d2903a6 60000000 60000000 <7c0050ac> 394a0040 4200fff8 7c0004ac 
>   ---[ end trace edcab2a1dfd5b38c ]---
>
>
> Which is obviously this hunk:
>
>> diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
>> index 4fc77a99c9bf..68d204a45cd0 100644
>> --- a/arch/powerpc/mm/pgtable_32.c
>> +++ b/arch/powerpc/mm/pgtable_32.c
>> @@ -143,7 +143,7 @@ __ioremap_caller(phys_addr_t addr, unsigned long size, pgprot_t prot, void *call
>>  	 * Don't allow anybody to remap normal RAM that we're using.
>>  	 * mem_init() sets high_memory so only do the check after that.
>>  	 */
>> -	if (slab_is_available() && (p < virt_to_phys(high_memory)) &&
>> +	if (slab_is_available() && virt_addr_valid(p) &&
>>  	    page_is_ram(__phys_to_pfn(p))) {
>>  		printk("__ioremap(): phys addr 0x%llx is RAM lr %ps\n",
>>  		       (unsigned long long)p, __builtin_return_address(0));
>
>
> I'll try and come up with a fix tomorrow.

Actually I think that change is just wrong. virt_addr_valid() takes a
virtual address, but p is a physical address.

So I'll drop this hunk for now, which makes the patch a no-op when
DEBUG_VIRTUAL is n which is probably the way it should be.

cheers