TL;DR I am annoyed at your desire to insert hacks into a core driver for all modern arm64 platforms and do not want to see good engineering practice thrown out the window in the name of making a specific piece of hardware "work" in the quickest way possible

Your unwillingness to try and actually understand the problem is your own choice, not mine. Adding a platform-specific quirk to ignore the inability to set something that the specification does not give implementations license to restrict is one thing, but doing it unilaterally for every platform is quite another thing and I wholeheartedly object to just disabling checks that get in the way with our current implementation on your hardware. We could quite easily have a real bug in our driver here, and in doing so we're just ignoring that. I also have absolutely no desire to encounter cache coherency bugs in future because of a change like this which just decides that cache coherency is something that can be ignored and everything will be fine, don't worry about it. I do not know what the implications of picking up a different firmware-chosen value here is and whether FreeBSD can in fact cope with it. Do you?

I also wrote the following on IRC a couple of weeks back:

The first hunk should not be necessary according to the specification, but if it helps your platform, does not regress others and is believed to be better anyway according to Andy then that's fine. The second hunk I do not believe should be committed.

In D45409#1112416, @dim wrote:

In D45409#1111300, @pho wrote:

FYI: I came across this i386 problem after commit 0078df5f0258.

Trying to mount root from ufs:/dev/vtbd0p1 [rw]...
WARNING: WITNESS option enabled, expect reduced performance.
WARNING: DIAGNOSTIC option enabled, expect reduced performance.
WARNING: 32-bit kernels are deprecated and may be removed in FreeBSD 15.0.
panic: vm_page_t 0x5d49be0 phys_addr mismatch ffffffffb8779000 00000000b8779405
cpuid = 3
time = 1738177579
KDB: stack backtrace:
db_trace_self_wrapper(7,114beb40,5d49be0,ffffffff,ffffffff,...) at db_trace_self_wrapper+0x28/frame 0x113f6744
vpanic(148badd,113f6780,113f6780,113f67f0,13a54e9,...) at vpanic+0xf4/frame 0x113f6760
panic(148badd,5d49be0,b8779000,ffffffff,b8779405,...) at panic+0x14/frame 0x113f6774
pmap_pae_remove_pages(114128e8) at pmap_pae_remove_pages+0x599/frame 0x113f67f0
exec_new_vmspace(113f6994,188247c) at exec_new_vmspace+0x1cb/frame 0x113f6810
exec_elf32_imgact(113f6994,16d4801,113f6984,0,0,...) at exec_elf32_imgact+0x5e4/frame 0x113f6874
kern_execve(114beb40,113f6a8c,0,1141284c) at kern_execve+0x72c/frame 0x113f6a74
sys_execve(114beb40,114bedf0,114beb40,13ae913,114bede4,...) at sys_execve+0x4e/frame 0x113f6ac8
syscall(113f6ba8,3b,3b,3b,402aaf,...) at syscall+0x1e6/frame 0x113f6b9c
Xint0x80_syscall() at 0xffc03479/frame 0x113f6b9c
--- syscall (59, FreeBSD ELF32, execve), eip = 0x47e53f, esp = 0xffbfe848, ebp = 0xffbfe858 ---
KDB: enter: panic
[ thread pid 17 tid 100080 ]
Stopped at      kdb_enter+0x34: movl    $0,kdb_why
db> x/s version
version:        FreeBSD 15.0-CURRENT #0 main-n275068-0078df5f0258-dirty: Wed Jan 29 18:44:45 CET 2025\012    pho@mercat1.netperf.freebsd.org:/mnt25/obj/usr/src/i386.i386/sys/PHO\012
db>

Yeah I have almost exactly the same:

Kernel page fault with the following non-sleepable locks held:
exclusive sleep mutex SYSMAPS (SYSMAPS) r = 0 (0x1af1b3c) locked @ /usr/src/sys/i386/i386/pmap.c:4597
stack backtrace:
#0 0xfd9123 at ??+0
#1 0xfda008 at ??+0
#2 0x13a5ca7 at ??+0
#3 0x13a5276 at ??+0
#4 0x1382f3f at ??+0
#5 0x28 at ??+0


Fatal trap 12: page fault while in kernel mode
cpuid = 0; apic id = 00
fault virtual address   = 0x1f01000
fault code              = supervisor write data, reserved bits in PTE
instruction pointer     = 0x20:0x13ab070
stack pointer           = 0x28:0x1ef9ab4
frame pointer           = 0x28:0x1ef9ac0
code segment            = base 0x0, limit 0xfffff, type 0x1b
                        = DPL 0, pres 1, def32 1, gran 1
processor eflags        = resume, IOPL = 0
current process         = 0 ()
trap number             = 12
panic: page fault
cpuid = 0
time = 1
KDB: stack backtrace:
db_trace_self_wrapper(0,1a4d3e0,1ef9a74,1f01000,c,...) at db_trace_self_wrapper+0x28/frame 0x1ef9900
vpanic(1442402,1ef993c,1ef993c,1ef9968,13a5c44,...) at vpanic+0xf4/frame 0x1ef991c
panic(1442402,149cbab,0,fffff,1aac59b,...) at panic+0x14/frame 0x1ef9930
trap_fatal(8,1470e0e,5e0,1aa05e0,46,...) at trap_fatal+0x344/frame 0x1ef9968
trap_pfault(1f01000,0,0) at trap_pfault+0x6f/frame 0x1ef999c
trap(1ef9a74) at trap+0x326/frame 0x1ef9a68
calltrap() at calltrap+0x8/frame 0x1ef9a68
--- trap 0xc, eip = 0x13ab070, esp = 0x1ef9ab4, ebp = 0x1ef9ac0 ---
memset(1f01000,0,1000) at memset+0x50/frame 0x1ef9ac0
pmap_pae_zero_page(5d74994) at pmap_pae_zero_page+0xb2/frame 0x1ef9aec
vm_page_alloc_noobj_contig_domain(0,8062,1,0,0,ffffffff,ffffffff,1,0,0,6) at vm_page_alloc_noobj_contig_domain+0x20a/frame 0x1ef9b1c
uma_startup1(5f5e000) at uma_startup1+0xc5/frame 0x1ef9bcc
vm_mem_init(0) at vm_mem_init+0x30/frame 0x1ef9bd8
mi_startup() at mi_startup+0x1a4/frame 0x1ef9bf8
btext() at btext+0x5f
KDB: enter: panic

Probably best to land this, since it's actually important for reproducibility, then we can update both to something else later that gives extra visibility into the build less urgently

Emit an error if MYMAKE is being messed with in any way

It doesn't need clocks, it needs one of clocks or clock-frequency. QEMU's virt board lists the latter, which is far simpler and preferable in a virtualisation setting. It also uses 3686400 as the frequency, which I believe lets the guest go up to 230400 baud. We should align with QEMU where possible in order to avoid surprises.

One could conceivably imagine the handles being small positive integers (nexus0 is 1, and so on) rather than using kernel addresses, so -1 seems more flexible IMO, and is generally a clear sentinel.

In D48501#1110074, @jhb wrote:

In D48501#1109394, @jrtc27 wrote:

In D48501#1109382, @jhb wrote:

Booting a RISC-V image under qemu with a virtio-rng-pci device seems to have exercised this as it has a pci_host_generic as a child of a simplebus:

pcib0: <Generic PCI host controller> mem 0x30000000-0x3fffffff on simplebus1
pcib0: parsing FDT for ECAM0:
pcib0: Bus is cache-coherent
pcib0: PCI addr: 0x0, CPU addr: 0x3000000, Size: 0x10000, Type: I/O port
pcib0: PCI addr: 0x40000000, CPU addr: 0x40000000, Size: 0x40000000, Type: memory
pcib0: PCI addr: 0x400000000, CPU addr: 0x400000000, Size: 0x400000000, Type: memory
pci0: <PCI bus> on pcib0
pci0: domain=0, physical bus=0
found-> vendor=0x1b36, dev=0x0008, revid=0x00
        domain=0, bus=0, slot=0, func=0
        class=06-00-00, hdrtype=0x00, mfdev=0
        cmdreg=0x0000, statreg=0x0000, cachelnsz=0 (dwords)
        lattimer=0x00 (0 ns), mingnt=0x00 (0 ns), maxlat=0x00 (0 ns)
found-> vendor=0x1af4, dev=0x1005, revid=0x00
        domain=0, bus=0, slot=1, func=0
        class=00-ff-00, hdrtype=0x00, mfdev=0
        cmdreg=0x0000, statreg=0x0010, cachelnsz=0 (dwords)
        lattimer=0x00 (0 ns), mingnt=0x00 (0 ns), maxlat=0x00 (0 ns)
        intpin=a, irq=0
        MSI-X supports 2 messages in map 0x14
        map[10]: type I/O Port, range 32, base 0, size  5, port disabled
        map[14]: type Memory, range 32, base 0, size 12, memory disabled
        map[20]: type Prefetchable Memory, range 64, base 0, size 14, memory disabled
virtio_pci0: <VirtIO PCI (legacy) Entropy adapter> irq 16 at device 1.0 on pci0
virtio_pci0: Lazy allocation of 0x20 bytes rid 0x10 type 4 at 0
virtio_pci0: Lazy allocation of 0x1000 bytes rid 0x14 type 3 at 0x40000000
`

Some command output:

root@riscv64:~ # pciconf -lb
hostb0@pci0:0:0:0:      class=0x060000 rev=0x00 hdr=0x00 vendor=0x1b36 device=0x0008 subvendor=0x1af4 subdevice=0x1100
virtio_pci0@pci0:0:1:0: class=0x00ff00 rev=0x00 hdr=0x00 vendor=0x1af4 device=0x1005 subvendor=0x1af4 subdevice=0x0004
    bar   [10] = type I/O Port, range 32, base 0, size 32, enabled
    bar   [14] = type Memory, range 32, base 0x40000000, size 4096, enabled
    bar   [20] = type Prefetchable Memory, range 64, base 0, size 16384, enabled
root@riscv64:~ # devinfo -u
I/O memory addresses:
    0x0-0xfffff (root0)
    0x100000-0x100fff (riscv_syscon0)
    0x101000-0x101fff (goldfish_rtc0)
    0x102000-0x2ffffff (root0)
    0x3000000-0x300ffff (pcib0)
    0x3010000-0xbffffff (root0)
....
pcib0 I/O port window:
    0x0-0x1f (virtio_pci0)
    0x20-0xffff (root0)
....

Not with D44207 though, right? Even absent this change it should show up as I/O memory rather than I/O port, right? Or is that just showing that it's still an I/O port from pcib0 downwards; if so, that's not so relevant because that's not changed, what's relevant is how simplebus0 and above see it?

This is after D44207 was merged, so it has it included. The point is that it only looks like an I/O port for pcib0 downwards. For simplebus0 it instead sees the SYS_RES_MEMORY resource allocated to pcib0 (0x3000000-0x300ffff (pcib0)) corresponding to the I/O window.

In D48501#1109382, @jhb wrote:

Booting a RISC-V image under qemu with a virtio-rng-pci device seems to have exercised this as it has a pci_host_generic as a child of a simplebus:

pcib0: <Generic PCI host controller> mem 0x30000000-0x3fffffff on simplebus1
pcib0: parsing FDT for ECAM0:
pcib0: Bus is cache-coherent
pcib0: PCI addr: 0x0, CPU addr: 0x3000000, Size: 0x10000, Type: I/O port
pcib0: PCI addr: 0x40000000, CPU addr: 0x40000000, Size: 0x40000000, Type: memory
pcib0: PCI addr: 0x400000000, CPU addr: 0x400000000, Size: 0x400000000, Type: memory
pci0: <PCI bus> on pcib0
pci0: domain=0, physical bus=0
found-> vendor=0x1b36, dev=0x0008, revid=0x00
        domain=0, bus=0, slot=0, func=0
        class=06-00-00, hdrtype=0x00, mfdev=0
        cmdreg=0x0000, statreg=0x0000, cachelnsz=0 (dwords)
        lattimer=0x00 (0 ns), mingnt=0x00 (0 ns), maxlat=0x00 (0 ns)
found-> vendor=0x1af4, dev=0x1005, revid=0x00
        domain=0, bus=0, slot=1, func=0
        class=00-ff-00, hdrtype=0x00, mfdev=0
        cmdreg=0x0000, statreg=0x0010, cachelnsz=0 (dwords)
        lattimer=0x00 (0 ns), mingnt=0x00 (0 ns), maxlat=0x00 (0 ns)
        intpin=a, irq=0
        MSI-X supports 2 messages in map 0x14
        map[10]: type I/O Port, range 32, base 0, size  5, port disabled
        map[14]: type Memory, range 32, base 0, size 12, memory disabled
        map[20]: type Prefetchable Memory, range 64, base 0, size 14, memory disabled
virtio_pci0: <VirtIO PCI (legacy) Entropy adapter> irq 16 at device 1.0 on pci0
virtio_pci0: Lazy allocation of 0x20 bytes rid 0x10 type 4 at 0
virtio_pci0: Lazy allocation of 0x1000 bytes rid 0x14 type 3 at 0x40000000
`

Some command output:

root@riscv64:~ # pciconf -lb
hostb0@pci0:0:0:0:      class=0x060000 rev=0x00 hdr=0x00 vendor=0x1b36 device=0x0008 subvendor=0x1af4 subdevice=0x1100
virtio_pci0@pci0:0:1:0: class=0x00ff00 rev=0x00 hdr=0x00 vendor=0x1af4 device=0x1005 subvendor=0x1af4 subdevice=0x0004
    bar   [10] = type I/O Port, range 32, base 0, size 32, enabled
    bar   [14] = type Memory, range 32, base 0x40000000, size 4096, enabled
    bar   [20] = type Prefetchable Memory, range 64, base 0, size 16384, enabled
root@riscv64:~ # devinfo -u
I/O memory addresses:
    0x0-0xfffff (root0)
    0x100000-0x100fff (riscv_syscon0)
    0x101000-0x101fff (goldfish_rtc0)
    0x102000-0x2ffffff (root0)
    0x3000000-0x300ffff (pcib0)
    0x3010000-0xbffffff (root0)
....
pcib0 I/O port window:
    0x0-0x1f (virtio_pci0)
    0x20-0xffff (root0)
....

This doesn't seem to conform to the spec.

This always bugged me :)

Since this seems to be a saga that keeps on giving, I'm going to ask the important question to which the answer must have been no for every other commit for this, despite my comments on IRC:

Please put the rationale from your comment in the commit message, and mention that this may be improved in future to use the right DB (I intend to do that if nobody beats me to it).

Uh, why do we need that wrapper instead of just calling memchr itself?

Hm, this used to be required by POSIX but newer versions allow the implementation not to treat that as an error

I was going to suggest using the device_t-taking APIs instead, but then realised that doesn't work for the early case... bit sad but oh well.

(but please mention BSS in the commit message)

In D43237#1103957, @minsoochoo0122_proton.me wrote:

In D43237#1103894, @jhb wrote:

Also, the only contrib things I could find that are using c99 explicitly is ZFS. I wonder if those can use c17?

OpenZFS seems to use gnu99 (source code). I don't think it is necessary, but if it is, it can be done in a separate commit/revision.

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