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:     Qian Cai <cai@lca.pw>,
        Chris Packham <Chris.Packham@alliedtelesis.co.nz>
Cc:     "christophe.leroy\@c-s.fr" <christophe.leroy@c-s.fr>,
        "paulus\@samba.org" <paulus@samba.org>,
        "benh\@kernel.crashing.org" <benh@kernel.crashing.org>,
        "tglx\@linutronix.de" <tglx@linutronix.de>,
        "linuxppc-dev\@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
        "linux-kernel\@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Hamish Martin <Hamish.Martin@alliedtelesis.co.nz>
Subject: Re: Argh, can't find dcache properties !
In-Reply-To: <48F1D8CF-13A1-4348-8973-81503782A451@lca.pw>
References: <be8c123a90f6d1664a902b6ad6c754b9f3d9e567.camel@alliedtelesis.co.nz> <87tv2exst1.fsf@mpe.ellerman.id.au> <876a5938fbad9d9e176e5f22f12e6b472d0dc4f7.camel@alliedtelesis.co.nz> <48F1D8CF-13A1-4348-8973-81503782A451@lca.pw>
Date:   Wed, 25 Mar 2020 17:01:49 +1100
Message-ID: <87369xx99u.fsf@mpe.ellerman.id.au>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Qian Cai <cai@lca.pw> writes:
>> On Mar 24, 2020, at 4:06 PM, Chris Packham <Chris.Packham@alliedtelesis.co.nz> wrote:
>> On Tue, 2020-03-24 at 15:47 +1100, Michael Ellerman wrote:
>>> Chris Packham <Chris.Packham@alliedtelesis.co.nz> writes:
>>>> Hi All,
>>>> 
>>>> Just booting up v5.5.11 on a Freescale T2080RDB and I'm seeing the
>>>> following mesage.
>>>> 
>>>> kern.warning linuxbox kernel: Argh, can't find dcache properties !
>>>> kern.warning linuxbox kernel: Argh, can't find icache properties !
...
>
> BTW, POWER9 PowerNV would have the same thing. 

Ugh, you're right.

Because we're missing the cache-line-size properties, even though they
are optional when the block & line size are the same.

  # find /proc/device-tree/cpus/PowerPC\,POWER9@0/ -name '*cache*'
  /proc/device-tree/cpus/PowerPC,POWER9@0/l2-cache
  /proc/device-tree/cpus/PowerPC,POWER9@0/d-cache-block-size
  /proc/device-tree/cpus/PowerPC,POWER9@0/d-cache-size
  /proc/device-tree/cpus/PowerPC,POWER9@0/i-cache-size
  /proc/device-tree/cpus/PowerPC,POWER9@0/d-cache-sets
  /proc/device-tree/cpus/PowerPC,POWER9@0/i-cache-block-size
  /proc/device-tree/cpus/PowerPC,POWER9@0/i-cache-sets

skiboot even explicitly omits them:

	if (cache->icache_line_size != cache->icache_block_size)
		dt_add_property_cells(cpu, "i-cache-line-size",
				      be32_to_cpu(cache->icache_line_size));
	if (cache->l1_dcache_line_size != cache->dcache_block_size)
		dt_add_property_cells(cpu, "d-cache-line-size",
				      be32_to_cpu(cache->l1_dcache_line_size));


Looks like it was broken ~3 years ago, in:
  bd067f83b084 ("powerpc/64: Fix naming of cache block vs. cache line")


Previously we did:
			lsizep = of_get_property(np, "d-cache-block-size",
						 NULL);
			/* fallback if block size missing */
			if (lsizep == NULL)
				lsizep = of_get_property(np,
							 "d-cache-line-size",
							 NULL);
			if (lsizep != NULL)
				lsize = be32_to_cpu(*lsizep);
			if (sizep == NULL || lsizep == NULL)
				DBG("Argh, can't find dcache properties ! "
				    "sizep: %p, lsizep: %p\n", sizep, lsizep);

ie. fallback from block size to line size, and only print if both are missing.

That commit changed the names and the logic, but not in a consistent
fashion, making "d-cache-line-size" required to avoid the Argh:

			bsizep = of_get_property(np, "d-cache-block-size",
						 NULL);
			lsizep = of_get_property(np, "d-cache-line-size",
						 NULL);
			if (bsizep == NULL)
				bsizep = lsizep;
			if (lsizep != NULL)
				lsize = be32_to_cpu(*lsizep);
			if (bsizep != NULL)
				bsize = be32_to_cpu(*bsizep);
			if (sizep == NULL || bsizep == NULL || lsizep == NULL)
				DBG("Argh, can't find dcache properties ! "
				    "sizep: %p, bsizep: %p, lsizep: %p\n",
				    sizep, bsizep, lsizep);

Back then we fell back to cur_cpu_spec->dcache_bsize, which should be
correct. But since then we introduced the device tree CPU features
parsing, which does:

static struct cpu_spec __initdata base_cpu_spec = {
	...
	.icache_bsize		= 32, /* minimum block size, fixed by */
	.dcache_bsize		= 32, /* cache info init.             */

So on systems with new enough skiboot we now default to 32, which is
wrong on Power9.


Luckily this info is not used by the sysfs cache files, because that
code doesn't use the values we parse here, it goes and looks at the
device tree itself. Which is pretty gross but actually saves us in this
case.

These values do end up in the vdso_data, and I can see the wrong values
in the vdso_data:

1c:mon> d4 c000000002390000
c000000002390000 54535953 46434d45 50503a47 00343643	eyecatcher
c000000002390010 00000001 00000001 00000100 004e1202	major		minor	platform	processor
c000000002390020 000000b0 00000000 00000000 0000003c	processorCount		physicalMemorySize
c000000002390030 57b7623f 0000ac10 1e848000 00000000	tb_orig_stamp		tb_ticks_per_sec
c000000002390040 de6d9e42 008637af 8de66bca 0005e7ae	tb_to_xs		stamp_xsec
c000000002390050 000ff730 00000000 00000000 00000000	tb_update_count		tz_minuteswest	tz_dsttime
c000000002390060 00008000 00000020 00008000 00000020	dcache_size	line_size	icache_size	icache_line_size
                                ^
                                32 != 128

And that appears to flow through to glibc, ie. I see:

_SC_LEVEL1_ICACHE_LINESIZE = 32
_SC_LEVEL1_DCACHE_LINESIZE = 32
_SC_LEVEL2_CACHE_LINESIZE = 32
_SC_LEVEL3_CACHE_LINESIZE = 32


So excuse me while I go and swear at something.

cheers