In D47478#1097638, @leres wrote:

In D47478#1097625, @jhb wrote:

So I wonder what the dialog looks like now by default on an 80x25 console? Does bsddialog do something sane to truncate the option list if it is too long or does it render oddly?

I created some frame grabs with a 80x34 xterm (my default since the 80's)

zfsboot2 is with the installation default.
zfsboot3 is with some extra (bogus) flags.
zfsboot4 is the same as zfsboot3 but with a 121x34 window.

Wait so you put up a patch to disable Sstc before an alternative was provided?

From grepping the tree this doesn't actually have any consequences currently, right, other than possible prints? The only real use I see is cpufreq, which we don't currently have for RISC-V.

In D48058#1096826, @mhorne wrote:

I have no objection to the current revision, although we can say a few things about how the code may look/change in the future.

Obviously, the feature-to-ISA-string code will continue to expand/balloon, as we will want to advertise the availability of the many unprivileged ISA extensions to the guest as well.

The problem is that we have no mechanism for reporting extension presence from the kernel to userspace, therefore you are using vm_cap_type here. I am working on something "official" for this, most likely it will be a sysarch exporting a bitmap, with bit definitions compatible to what can be found in Linux. Therefore one call can obtain the full set of supported extensions (sanitized), and we will be able to use this in bhyve to construct the ISA string for the guest.

In D48057#1095862, @cy wrote:

In D48057#1095687, @manu wrote:

In D48057#1095665, @lwhsu wrote:

Will we (or how about directly) move them to wifi-firmware- ports?

That's not my plan no, since we're moving toward pkgbase users will have a way to install them or not, moving them to ports gives us nothing.

How will buildworld users install the firmware then?

Alternative proposal: https://reviews.freebsd.org/D48064

Also, please upload with full context

Do we really want one vm_cap_type entry per extension?

Is there a reason to not do the right thing automatically via ROOT=... freebsd-version?

Honestly we should instead be removing the use of intptr_t for hwreset, it's not a sensible type to use for an ID.

Ping @andrew

Perhaps worth mentioning in the commit message that RISC-V is unusual in allowing the TLB to cache invalid PTEs

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 (beyond taking the time to test, but there's been plenty of that in the past 2 weeks), 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.