From: Tom Lane <tgl@sss.pgh.pa.us>
To: Thomas Munro <thomas.munro@gmail.com>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Bastien Roucariès <rouca@debian.org>
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: Detection of hadware feature => please do not use signal
Date: Sat, 23 Nov 2024 17:02:34 -0500
Message-ID: <865773.1732399354@sss.pgh.pa.us> (raw)
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>
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=... no
checking for __crc32cb, __crc32ch, __crc32cw, and __crc32cd with CFLAGS=-march=armv8-a+crc... no
checking which CRC-32C implementation to use... slicing-by-8
It turns out that -march=armv8-a+crc would work, except:
configure:17624: checking for __crc32cb, __crc32ch, __crc32cw, and __crc32cd with CFLAGS=-march=armv8-a+crc
configure:17648: ccache gcc -o conftest -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type -Wshadow=compatible-local -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -Wno-format-truncation -Wno-stringop-truncation -g -O2 -march=armv8-a+crc -Wl,-z,now -Wl,-z,relro conftest.c -lz -ledit -lcurses -lpthread -lexecinfo -lrt -lm >&5
cc1: error: '-mfloat-abi=hard': selected processor lacks an FPU
If you force it with a compatible -mfpu switch (I used -mfpu=neon)
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=armv8-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=armv8-a+crc -mfpu=whatever" 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
Attachments:
[text/x-diff] v2-netbsd-arm-crc-detection.patch (2.0K, ../865773.1732399354@sss.pgh.pa.us/2-v2-netbsd-arm-crc-detection.patch)
download | inline diff:diff --git a/src/port/pg_crc32c_armv8_choose.c b/src/port/pg_crc32c_armv8_choose.cindex 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) != 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 = "machdep.cpu0.cpu_id";+ size_t expected_len = sizeof(struct aarch64_sysctl_cpu_id);+#define ISAR0 ((struct aarch64_sysctl_cpu_id *) sysctlbuf)->ac_aa64isar0+#else+ const char *path = "machdep.id_isar";+ size_t expected_len = 6 * sizeof(int);+#define ISAR0 ((int *) sysctlbuf)[5]+#endif+ uint64 fld;++ len = sizeof(sysctlbuf);+ memset(sysctlbuf, 0, len);+ if (sysctlbyname(path, sysctlbuf, &len, NULL, 0) != 0)+ return false; /* perhaps kernel is 64-bit and we aren't? */+ if (len != expected_len)+ return false; /* kernel API change? */++ fld = (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). Assume+ * that any future nonzero value will be a superset of 1.+ */+ return (fld != 0);
#else
return false;
#endif
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, thomas.munro@gmail.com, hlinnaka@iki.fi, rouca@debian.org, pgsql-bugs@lists.postgresql.org
Subject: Re: Detection of hadware feature => please do not use signal
In-Reply-To: <865773.1732399354@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox