And amd64 has:

Also, perhaps worth pointing out arm64 does an unconditional intr_enable for the kernel too...

Presumably in practice the td_critnest check meant the interrupt enabling didn't matter, as spinlock_enter increments it? I assume there aren't many places where we disable interrupts without also being inside a critical section, so it would only be a problem if that sequence of code itself took a spurious fault.

Thanks, no more objections from me

I'd prefer some of the other cleanups I suggested to have been done as I see no downside to doing things more properly, but I'm not going to block the review on those. There is one other point I do have to make though that I really do object to being here.

boolean_t -> bool

In D47458#1082363, @mhorne wrote:

In D47458#1082358, @jrtc27 wrote:

Running C code without translation enabled is fragile and could break at any time, since it's not running at right address. You're relying on PC-relative addressing being used for everything (i.e. not absolute and not GOT-indirect), which happens to be true today with -mcmodel=medany for this code and all it uses, but there's no guarantee of that. This should really be in assembly (which shouldn't be hard to do, given sbi_get_mvendorid is just a wrapper around ecall?).

I did find it was quite fragile what could be done this early, reads/writes to globals failed, for example. I am surprised that Linux is able to do as much as they do in this way.

Sure, I will convert it to asm.

As with D47458, C code should not be run prior to enabling translation so we're running at the right address, so IMO this patch is a sign of going down the wrong road. Yes, Linux does horrendously fragile things like this, but that doesn't mean we should do the same.

Running C code without translation enabled is fragile and could break at any time, since it's not running at right address. You're relying on PC-relative addressing being used for everything (i.e. not absolute and not GOT-indirect), which happens to be true today with -mcmodel=medany for this code and all it uses, but there's no guarantee of that. This should really be in assembly (which shouldn't be hard to do, given sbi_get_mvendorid is just a wrapper around ecall?).

In D47407#1081914, @bz wrote:

Yeah, we do all our CheriBSD CI builds on Linux hosts and also support building release media on macOS for ease of local testing. That’s only (mini-)memstick/bootonly/disc1/ftp though, no dvd1 (due to pkg), and no VM images.

Are you happy with the added guards?

In D47407#1081787, @cperciva wrote:

In D47407#1081562, @jrtc27 wrote:

In D47407#1081558, @cperciva wrote:

@jrtc27 We should make this conditional on "is x86 or arm64" since there's no guarantee that the FreeBSD project will have package sets for any other platforms. Would that address your concern about the availability of a pkg binary?

Well, x86 and arm64 macOS/Linux will still fall under that. But yes, making sure it doesn't fail for architectures without package sets is also a good point.

Oh, people are building FreeBSD release images on non-FreeBSD? I didn't realize that. This sounds like even more reason to have pkg in the base system (and as a cross-tool).

In D47407#1081558, @cperciva wrote:

@jrtc27 We should make this conditional on "is x86 or arm64" since there's no guarantee that the FreeBSD project will have package sets for any other platforms. Would that address your concern about the availability of a pkg binary?

Please don't insert unconditional use of pkg for disc1, it will break the build for non-FreeBSD systems as they normally don't have a pkg binary. Whilst eventually we'll need to figure out a story there for pkgbase, I suggest just guarding it by NO_ROOT for now. It really exists to support non-FreeBSD where NO_ROOT is required, though eventually would ideally be used for the real release builds on FreeBSD rather than requiring root.

(Alternatively, one could use the ugly PRIxPTR macro in place of the zx)

In D47331#1079376, @jrtc27 wrote:

In D47331#1079374, @cperciva wrote:

Hmm, I don't actually see anything in the ACPI spec saying that IoRestriction only applies for ACPI_RESOURCE_GPIO_TYPE_IO. So I think we have three options here:

1. Go ahead with this patch, on the basis of "this is what Linux does",
2. Force GPIO_PIN_INPUT for ACPI_RESOURCE_GPIO_TYPE_INT on the basis of "if we're generating interrupts obviously it's an input", or
3. Just parse the values we're given and hope they make sense.

@andrew, @jrtc27, any preference?

This table in the link I posted above:

In D47331#1079374, @cperciva wrote:

Hmm, I don't actually see anything in the ACPI spec saying that IoRestriction only applies for ACPI_RESOURCE_GPIO_TYPE_IO. So I think we have three options here:

1. Go ahead with this patch, on the basis of "this is what Linux does",
2. Force GPIO_PIN_INPUT for ACPI_RESOURCE_GPIO_TYPE_INT on the basis of "if we're generating interrupts obviously it's an input", or
3. Just parse the values we're given and hope they make sense.

@andrew, @jrtc27, any preference?

In D47331#1079365, @andrew wrote:

Don't we need to set GPIO_PIN_INPUT for interrupts?

Technically should have a blank line between the ifs, though that was true before.

In D47279#1078373, @ehem_freebsd_m5p.com wrote:

@mmel I also find it odd that you were the first to suggest an enum.

In D47279#1078373, @ehem_freebsd_m5p.com wrote:

Wow, so friendly to revert something immediately after the person gets a rebase done and without informing them about discussion which was occurring.

Yeah, I mean, I don't really care which way we go, so long as we commit fully to one. As it stands we're using enums in name only, we're not getting any of the benefits that they bring due to this halfway house approach.

In D47279#1078240, @mmel wrote:

In D47279#1078231, @jrtc27 wrote:

In D47279#1078226, @mmel wrote:

And we also don't compile the kernel with -fno-short-enums. So the actual behavior/variant depends only on the compiler (for which both variants are equally valid).
What happens when someone uses this enum as a member of KABI compliant structure?

They get the same as any other enum in a structure? There are plenty already out there, and unless you start to reach 128 enum values there is no risk of the type changing by adding one.

???? The enum (with less than 128 values) size changes with the default compiler settings.

In D47279#1078226, @mmel wrote:

And we also don't compile the kernel with -fno-short-enums. So the actual behavior/variant depends only on the compiler (for which both variants are equally valid).
What happens when someone uses this enum as a member of KABI compliant structure?

So long as the assembly adheres to the ABI it works, and each assembly file is already correct for the corresponding ABI. The numbers are small positive integers, so whether they need sign-extending or zero-extending (or nothing at all) doesn’t matter.

[18:20:04] <@jrtc27> one review for the new C implementation
[18:20:10] <@jrtc27> one review for each new asm implementation on top of that
[18:20:58] <@jrtc27> and this screams missing depend-cleanup.sh changes to me
[18:23:07] <@jrtc27> and if USE_ASM_SOURCES is an interface into Makefile.md5.inc that's way too global a namespace to be using for it
[18:23:16] <@jrtc27> previously it was fine because it didn't leak outside lib/libmd
[18:23:39] <@jrtc27> but USE_ASM_SOURCES=0 in stand/libsa should be screaming that the name is wrong

Was I really that stupid? Oops. Please MFC this fix.

In D47002#1075850, @ehem_freebsd_m5p.com wrote:

I had thought that it might be better, if you were going to push this through quickly, to simply push past and deal with the aftermath. Issue is you've now taken long enough for me to start organizing my thoughts better.

I hope this is merely meant as a representation of a commit and not the full commit. The reason is the actual commit will be much larger. >95% of #include of the INTRNG headers is by #include <machine/intr.h>, hence this will need to change hundreds of files. The reason Github #1431, commit 2 could be so small, was it was addressing deviations from an existing consensus, rather than overturning a previous consensus.

#include <sys/intr.h> is inherently architecture-dependent. As including from sys is always the same no matter the architecture, that will always get the same header, which breaks on PowerPC and x86. Unless/until INTRNG manages to replace the interrupt infrastructures on PowerPC and x86, #include <sys/intr.h> will require a surrounding #ifdef INTRNG for architecture-independent files.

Presently, #include <machine/intr.h> could very readily be made architecture-independent. All that is needed is to rename the PowerPC and x86 intr_machdep.h headers to intr.h and modifying source files to match.

Presently, #include <machine/intr_machdep.h> could very readily be made architecture-independent. All that is needed is to rename the INTRNG-architecture intr.h headers to intr_machdep.h and modifying source files to match.

Presently, #include <machine/interrupt.h> could very readily be made architecture-independent. All that is needed is to rename the intr.h and intr_machdep.h headers to interrupt.h and modifying source files to match.

Presently, #include <machine/bikeshed.h> could very readily be made architecture-independent. All that is needed is to rename the intr.h and intr_machdep.h headers to bikeshed.h and modifying source files to match.

I am avoiding expressing a preference on which of these I prefer since I don't want to stir the pot. I simply want them to converge on any name since that is high value to me and expressing a preference is trivial. You might notice, Github #1286, commit 1 (alas, likely needs an update due to 2 months and silence). I've been trying to get progress on this trivial issue for more than 2 years.

D47002 increases the architecture interrupt framework divergence. Please keep an eye on convergence.

3154506a168d8188fe06f252e070b394b6581db3

In D47002#1071781, @ehem_freebsd_m5p.com wrote:

In D47002#1071690, @imp wrote:

This is the more typical pattern.

See sys/endian.h including machine/endian.h for but one of many examples (though that example is a bit convoluted in spots due to slight differences between endian.h, sys/endian.h and machine/endian.h consumer expectations.

This is the more typical pattern since most things in sys are truly machine-independent. sys/intr.h though is not machine-independent, as neither PowerPC nor x86 currently use it.

In D47002#1071777, @ehem_freebsd_m5p.com wrote:

NO ABSOLUTELY NOT. The reason is this makes architecture-independent source impossible without a #ifdef INTRNG.

Take a look at the first two commits of GitHub #1286. The first two create an interrupt_t type definition for things which only handle pointers to the architecture type, but don't directly examine the contents. If you have to #include sys/intr.h, then it is literally impossible to write truly machine-independent beyond having a void *.

An as yet not yet ready patch series proposes adding:

#include <machine/a_bikeshed_string_for_sed_to_target.h>

intr_event_create_device(struct intr_event **event, device_t pic, interrupt_t *source, u_int irq, int flags, const char *fmt, ...);

To <sys/interrupt.h>. If you require #include <sys/intr.h> then this degrades the quality as you must do one of two things:

#ifdef INTRNG
#include <sys/intr.h>
#else
#include <machine/intr_machdep.h>
#endif

intr_event_create_device(struct intr_event **event, device_t pic, interrupt_t *source, u_int irq, int flags, const char *fmt, ...);

Or else:

intr_event_create_device(struct intr_event **event, device_t pic, void *source, u_int irq, int flags, const char *fmt, ...);

The former means it is no longer truly architecture-independent, the latter compromises type-safety. This is a BAD idea!

Could be guarded by !FreeBSD (AFAICT this isn’t?) but I don’t see a problem with doing it there too.

Cc @bdrewery for poudriere things...

In D46502#1060031, @jrtc27 wrote:

In D46502#1059884, @tijl wrote:

bmake calls uname(3) to define MACHINE and that returns the value of environment variable UNAME_m which is defined by poudriere so MACHINE should be correct. MACHINE_ARCH cannot be obtained through uname(3) so bmake has to call sysctl which will indeed return the native architecture. I think it's best to add MACHINE_ARCH=${ARCH} to WRK_ENV in Mk/bsd.port.mk so it applies to all ports not just kmod ports.

The more I think about it the more I'm convinced the original commit was just wrong.

If a user puts things in make.conf, there is the expectation it applies to all builds made with that environment, ports included. If they configure their system in a way that breaks building a port, that's their fault. If they want a sanitised environment, that's what poudriere is for, and sidesteps any such issues. That is, it is a feature not a bug that make.conf applies to ports builds, and it is a feature which poudriere has relied upon for many years to correctly configure ports in the jail, which that commit broke (and doing an exp run of native amd64 isn't going to turn up the interesting failures like cross-building kmods). Quite frankly, I believe any port that disables make.conf is wrong, let alone doing it globally.

In D40161#1063583, @ehem_freebsd_m5p.com wrote:

@jrtc27 my one concern was to alert everyone here that discussion has occurred elsewhere. I was thinking discussion might have ended here, but it is now clear it has not terminated. Now I know I need to be tracking both.

People put up new versions of patches rather than commandeering an existing one regularly. Whether that new version is on GitHub or Phabricator is irrelevant. Please stop complaining all the time about GitHub and Phabricator, your views are well known and it just leads to a bunch of spam in everyone's inboxes and IRC clients.