• Centralize MI CTRT1OBJS.

I think this is probably consistent with a fair bit of code in the tree which does use patterns like

The OCF interface looks fine to me. Supporting separate output and separate AAD is optional. For IPsec separate AAD will be more meaningful once the semi half folks land ESN support. (ESN uses a 64-bit sequence number where the upper 32-bits are implicit and never sent on the wire, so not in the mbuf.)

FWIW, the general strategy on x86 and other platforms is to mask the interrupt in the PIC in the "pre_ithread" hook and later re-enable it in the PIC again in the "post_ithread" hook. Both "pre_ithread" and "post_filter" send an EOI as they are the only ones guaranteed to run synchronously on the CPU which received the interrupt. MSI alleviates the need for masking on most platforms since it is effectively edge triggered so actual level-triggered interrupts are rare on many modern systems. I'm not quite sure how that model translates to the PLIC, but on the surface reading the description of this commit seems to imply it is not doing the same thing at all. The reason we did this on x86 is that the EOI was global and could not be deferred to post_ithread. If you don't have a global EOI but instead are free to get additional interrupts (including additional lower priority interrupts) while the interrupt is not claimed, then I think your commit will work fine. However, if a high priority interrupt (PLIC priority) blocks other interrupts then you are letting that ithread starve the filters, etc. for other devices which is probably not ideal.

I got tired of reworking the crt changes with lots of conflicts as I moved the ABI tag note around, so wanted to pause and simplify the build glue first so that the diffs would be smaller and more obvious.

As far as I understand it, this looks ok to me.

I believe this is correct. I think you only put things in Symbol.map if you want them public for when you link new binaries. We use __sym_compat without any corresponding Symbol.map entries for other compat shims in libc (e.g. fts* symbols in lib/libc/gen/Symbol.map are only the latest FBSD1.5 version and don't include the legacy versions for FBSD1.0 and FBSD1.1.)

This might get some more testing of the driver before it should be committed, but this isn't a bad place to coordinate testing if others are interested in testing I suppose. On my wimpy little 4-core Haswell box KTLS with aesni was able to push about 35 Gbps in a simple https benchmark vs 41-42 Gbps with this module (and 42-43 Gbps with the KTLS-specific ktls-isa_l-crypto-kmod port).

It's probably worth seeing what other distros do. I'd probably be inclined to just return NULL as well. Some old software might call all of these methods even if it ends up using TLS 1.x instead, so the abort might be overkill. (I ran into things like this with the deprecation warnings for old crypto since OpenSSL in stable/10 would just open /dev/crypto and try to create sessions for all algos even if it didn't end up using any of them).

In D25304#562386, @kib wrote:

In D25304#562372, @jhb wrote:

It also affects what OSABI GDB assigns if you do 'file /path/to/dso' or 'add-symbol-file /path/to/dso'. Mostly this would matter in the context of GDB used on a non-FreeBSD host (on FreeBSD hosts the fallback default OSABI is FreeBSD, so when you fail to recognize a shared library, it ends up still working due to the accident).

Removing the EI_OSABI check on aarch64 and riscv64 means ldd would try to dlopen() Linux shared libraries on those platforms instead of erroring out.

I would say that this is all cosmetics, but I see that you do not agree.
Then I suggest trying the comdat idea to put the ABI note into both crt1.o and crti.o, to keep crt1.o self-contained ABI marker.

I suspect it is less deliberate and it's probably worth looking into to see if we can reverse them. To be honest, what most other subsystems would do is use a single SYSINIT that initialized the object and then added it to the list (e.g. the module handler for device driver modules). If domains were per-VNET that would be trivial. As it is, you probably want to init on all VNETs first via the existing separate SYSINITs before the domain_add.

It also affects what OSABI GDB assigns if you do 'file /path/to/dso' or 'add-symbol-file /path/to/dso'. Mostly this would matter in the context of GDB used on a non-FreeBSD host (on FreeBSD hosts the fallback default OSABI is FreeBSD, so when you fail to recognize a shared library, it ends up still working due to the accident).

We already use a slightly different pattern (ALLSRC:N:*.h:[1]) in csu/Makefile.inc without a comment, and also not sure what the comment would say? I am hopeful if I can get some of the other reviews in this train all landed that I can move most of this duplicated logic up into csu/Makefile.inc anyway at which point there would be a single instance of this command.

Technically timespec32 != timespec for amd64 since i386 uses uint32_t for time_t? However, it seems no one bothered to add the 32-bit compat support shims anyway, so fine to remove.

Actually, why do we add the domain to the list via domain_add() before calling domain_init()? If we do domain_init() before domain_add() then doesn't that fix the issue? It would mean swapping the order of the SYSINIT's in DOMAIN_SET and VNET_DOMAIN_SET, but presumably none of the domain init routines really care that they are on the global list yet?

• Rebase.
• Update for separate output buffer changes.
• Use separate AAD support to remove duplicate iovec.
• Pass the mbuf chain down to the decryption backends.
• Compile.

• Restore an empty tcpsignature_cleanup.
• Restore some NULL pointer assignments.

In D25443#562018, @jhb wrote:

In D25443#562009, @jhb wrote:

In D25443#561680, @delphij wrote:

I'm not 100% comfortable with the removal of these = NULL's but it appears that the code didn't cared about their value being non-zero, so the change as-is seemed Okay.

I'd probably change key_cleansav() to perform a bzero on the whole 'sav' structure and remove all sav->* = NULL, but that's only a personal preference.

I'd be fine doing a single zero of the sav. I'd much rather do that centralized than the current approach of partial clearing scattered about. I do think that in all these cases we are doing a zero just before freeing, and that none of these pointers being cleared currently are sensitive in the sense of requiring explicit_bzero().

Hmm, this is not quite trivial. Some fields like sav->lock and sav->lftc_c probably need to not be zeroed in key_cleansav. Some of that I can fix by moving key_cleansav down in key_delsav just before the free and in that case we could even use zfree. This would potentially permit removing many of the NULL assignments from key_cleansav. However, key_cleansav is also called for the error case for key_setsaval. Hmm, looking at that more though, if that errors the key always gets freed, and in one of those cases we effectively duplicate key_freeval. Maybe I can clean this up.

In D25443#562009, @jhb wrote:

In D25443#561680, @delphij wrote:

I'm not 100% comfortable with the removal of these = NULL's but it appears that the code didn't cared about their value being non-zero, so the change as-is seemed Okay.

I'd probably change key_cleansav() to perform a bzero on the whole 'sav' structure and remove all sav->* = NULL, but that's only a personal preference.

I'd be fine doing a single zero of the sav. I'd much rather do that centralized than the current approach of partial clearing scattered about. I do think that in all these cases we are doing a zero just before freeing, and that none of these pointers being cleared currently are sensitive in the sense of requiring explicit_bzero().

In D25443#561680, @delphij wrote:

I'm not 100% comfortable with the removal of these = NULL's but it appears that the code didn't cared about their value being non-zero, so the change as-is seemed Okay.

I'd probably change key_cleansav() to perform a bzero on the whole 'sav' structure and remove all sav->* = NULL, but that's only a personal preference.

In D25442#561674, @delphij wrote:

I think setting tdb_xform to NULL is no longer needed (as they are about to be done by the caller anyway).

I was hoping this was going to fix the bluetooth issue of adding domains post-boot from the initial mail, but it doesn't seem so? (I get a warning every time I boot on a laptop with bluetooth since the BT domain is pulled in via devmatch.) It seems like solving that problem cleanly would be a prerequisite for kicking this out of GENERIC and into a module as well.

• Rebase.

Using explicit_bzero vs bzero is largely for readability, though it can also prevent "helpful" compiler optimizations if bzero is mapped to a memset the compiler knows it can elide. The compiler probably won't elide the bzero before calling kmem_free but might elide ones at the end of functions.

If you do 'make toolchain' first to get the updated headers into the WORLDTMP sysroot you don't need this workaround. In general in the tree we assume you are building against an up-to-date sysroot rather than using this sort of approach. Note that to be fully correct you'd need to replace every machine/ include with "amd64/include" above which would then also break for arm64 support, etc.

*shrug* Given the function already defines variables in scope I'm not sure it really matters to pull it out to the top-level (esp in light of the recent style(9) change Warner is working on). Presumably this is all the same for the compiler anyway?

My only real question is do you want to do a single target? Since you are using mv instead of cp make is always going to invoke both targets anyway. The single target version would just join the two bodies.