From: Tom Lane <tgl@sss.pgh.pa.us>
To: Thomas Munro <thomas.munro@gmail.com>
cc: Heikki Linnakangas <hlinnaka@iki.fi>,
        =?UTF-8?Q?Bastien_Roucari=C3=A8s?= <rouca@debian.org>,
        pgsql-bugs@lists.postgresql.org
Subject: Re: Detection of hadware feature => please do not use signal
In-reply-to: <736756.1732341046@sss.pgh.pa.us>
References: <4496616.iHFcN1HehY@portable-bastien>
 <af84e934-4e9c-4380-95ee-8921dab1254b@iki.fi>
 <1156374.1730396928@sss.pgh.pa.us>
 <7ec1c65c-1a28-49a7-af43-f77c260a3df6@iki.fi>
 <1292593.1730399134@sss.pgh.pa.us>
 <CA+hUKGLRXq0-s0n2=dy_A5iY-Omg628osyJEJMjPQ=6m566UgA@mail.gmail.com>
 <707008.1732325995@sss.pgh.pa.us>
 <CA+hUKGJPrRow2aiUSK=aep5nyXUaB0gBoDJQKfyCsP=-ivfapg@mail.gmail.com>
 <736756.1732341046@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Sat, 23 Nov 2024 00:50:46 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <865679.1732399281.0@sss.pgh.pa.us>
Date: Sat, 23 Nov 2024 17:02:34 -0500
Message-ID: <865773.1732399354@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/865773.1732399354%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <865679.1732399281.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable

I wrote:
> But that developer.arm.com page seems pretty definitive.  Tomorrow
> I'll have a go at spinning up a 32-bit NetBSD installation and test
> it.  Thanks for the research!

Conclusions:

1. Unsurprisingly, a 64-bit kernel exposes machdep.cpuN.cpu_id but
not machdep.id_isar; a 32-bit kernel the reverse.

2. 32-bit userland does not provide <aarch64/armreg.h>.  (So I did
not try to use the ID_AA64ISAR0_EL1_CRC32 macro: we'd have had to
hard-code the field position anyway for 32-bit.)

3. It doesn't look to me like NetBSD really supports 32-bit userland
under a 64-bit kernel.  Maybe it'd kind of work, but utilities like
cpuctl seem to be built for only one of the kernel APIs.

So I end up proposing the attached.  If we do somehow find ourselves
running 32-bit with a 64-bit kernel, this will report "no CRC"
because the sysctl call will fail.  I don't think it's worth
sweating over that case.

BTW, I found out that by default we'll build a software-CRC-only
implementation on 32-bit ARM NetBSD, at least in the "generic"
eabihf build:

checking for __crc32cb, __crc32ch, __crc32cw, and __crc32cd with CFLAGS=3D=
... no
checking for __crc32cb, __crc32ch, __crc32cw, and __crc32cd with CFLAGS=3D=
-march=3Darmv8-a+crc... no
checking which CRC-32C implementation to use... slicing-by-8

It turns out that -march=3Darmv8-a+crc would work, except:

configure:17624: checking for __crc32cb, __crc32ch, __crc32cw, and __crc32=
cd with CFLAGS=3D-march=3Darmv8-a+crc
configure:17648: ccache gcc -o conftest -Wall -Wmissing-prototypes -Wpoint=
er-arith -Wdeclaration-after-statement -Werror=3Dvla -Wendif-labels -Wmiss=
ing-format-attribute -Wimplicit-fallthrough=3D3 -Wcast-function-type -Wsha=
dow=3Dcompatible-local -Wformat-security -fno-strict-aliasing -fwrapv -fex=
cess-precision=3Dstandard -Wno-format-truncation -Wno-stringop-truncation =
-g -O2 -march=3Darmv8-a+crc    -Wl,-z,now -Wl,-z,relro  conftest.c -lz -le=
dit -lcurses -lpthread -lexecinfo -lrt -lm  >&5
cc1: error: '-mfloat-abi=3Dhard': selected processor lacks an FPU

If you force it with a compatible -mfpu switch (I used -mfpu=3Dneon)
then you get a build with runtime CRC check, and all seems to work.
I did not try very hard to determine what the default -mfpu is here
or why -march=3Darmv8-a+crc overrides whatever the default is.  This
probably means that no end user is going to see any benefit from this
exercise, because they are not likely to try adding -mfpu by itself.
If they add "-march=3Darmv8-a+crc -mfpu=3Dwhatever" to CFLAGS manually
then configure will deem that a runtime check is not required, so this
code still doesn't get exercised.

tl;dr: a lot of time spent on a probably-useless exercise.  Still,
now that we've got the code we may as well commit it.  The configure
issue was there all along.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="v2-netbsd-arm-crc-detection.patch";
	charset="us-ascii"
Content-ID: <865679.1732399281.2@sss.pgh.pa.us>
Content-Description: v2-netbsd-arm-crc-detection.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/port/pg_crc32c_armv8_choose.c b/src/port/pg_crc32c_armv8_=
choose.c
index 306500154e..11e27978fd 100644
--- a/src/port/pg_crc32c_armv8_choose.c
+++ b/src/port/pg_crc32c_armv8_choose.c
@@ -31,6 +31,13 @@
 #endif
 #endif
 =

+#if defined(__NetBSD__)
+#include <sys/sysctl.h>
+#if defined(__aarch64__)
+#include <aarch64/armreg.h>
+#endif
+#endif
+
 #include "port/pg_crc32c.h"
 =

 static bool
@@ -52,6 +59,47 @@ pg_crc32c_armv8_available(void)
 #else
 	return (getauxval(AT_HWCAP2) & HWCAP2_CRC32) !=3D 0;
 #endif
+#elif defined(__NetBSD__)
+	/*
+	 * On NetBSD we can read the Instruction Set Attribute Registers via
+	 * sysctl.  For doubtless-historical reasons the sysctl interface is
+	 * completely different on 64-bit than 32-bit, but the underlying
+	 * registers contain the same fields.
+	 */
+#define ISAR0_CRC32_BITPOS 16
+#define ISAR0_CRC32_BITWIDTH 4
+#define WIDTHMASK(w)	((1 << (w)) - 1)
+#define SYSCTL_CPU_ID_MAXSIZE 64
+
+	size_t		len;
+	uint64		sysctlbuf[SYSCTL_CPU_ID_MAXSIZE];
+#if defined(__aarch64__)
+	/* We assume cpu0 is representative of all the machine's CPUs. */
+	const char *path =3D "machdep.cpu0.cpu_id";
+	size_t		expected_len =3D sizeof(struct aarch64_sysctl_cpu_id);
+#define ISAR0 ((struct aarch64_sysctl_cpu_id *) sysctlbuf)->ac_aa64isar0
+#else
+	const char *path =3D "machdep.id_isar";
+	size_t		expected_len =3D 6 * sizeof(int);
+#define ISAR0 ((int *) sysctlbuf)[5]
+#endif
+	uint64		fld;
+
+	len =3D sizeof(sysctlbuf);
+	memset(sysctlbuf, 0, len);
+	if (sysctlbyname(path, sysctlbuf, &len, NULL, 0) !=3D 0)
+		return false;			/* perhaps kernel is 64-bit and we aren't? */
+	if (len !=3D expected_len)
+		return false;			/* kernel API change? */
+
+	fld =3D (ISAR0 >> ISAR0_CRC32_BITPOS) & WIDTHMASK(ISAR0_CRC32_BITWIDTH);
+
+	/*
+	 * Current documentation defines only the field values 0 (No CRC32) and =
1
+	 * (CRC32B/CRC32H/CRC32W/CRC32X/CRC32CB/CRC32CH/CRC32CW/CRC32CX).  Assum=
e
+	 * that any future nonzero value will be a superset of 1.
+	 */
+	return (fld !=3D 0);
 #else
 	return false;
 #endif

------- =_aaaaaaaaaa0--