In D45049#1039824, @corvink wrote:

In D45049#1039608, @markj wrote:

In D45049#1027224, @jhb wrote:

So for "bare metal" cases like booting with a bootrom (e.g. UEFI) we should arguably leave bars unregistered since the raw firmware probably does that. Only the bhyveload case probably wants to enable BARs on startup. I would be fine with not using a user-facing config option but basically making the test in this patch be some kind of "is there a bootrom or not" check. Would need to test it on amd64 with UEFI.

I implemented this and EDK2 fails to load our loader. I get the following output before dropped into a shell:

Launching virtual machine "test" ...                                                                                                                          
fbuf frame buffer base: 0x32b086e00000 [sz 33554432]                                                                                                          
BdsDxe: loading Boot0001 "EFI Internal Shell" from Fv(7CB8BDC9-F8EB-4F34-AAEA-3EE4AF6516A1)/FvFile(7C04A583-9E3E-4F1C-AD65-E05268D0B4D1)                      
BdsDxe: starting Boot0001 "EFI Internal Shell" from Fv(7CB8BDC9-F8EB-4F34-AAEA-3EE4AF6516A1)/FvFile(7C04A583-9E3E-4F1C-AD65-E05268D0B4D1)                     
UEFI Interactive Shell v2.2                                                                                                                                   
EDK II                                                                                                                                                        
UEFI v2.70 (BHYVE, 0x00010000)                                                                                                                                
map: No mapping found.

Presumably EDK2 does rely on us having registered BARs. After staring at the EDK2 bhyve platform code for a while, I'm not sure how this is encoded.

It's encoded by PcdPciDisableBusEnumeration. If set to TRUE, EDK2 rely on us. If set to FALSE or unset, EDK2 will register BARs itself. For the X64 platform it was removed some time ago: https://github.com/tianocore/edk2/commit/70f3e62dc73d28962b833373246ef25c865c575e.

You could try building a debug version of EDK2 to get a more detailed log. Therefore, pass -b DEBUG and -DDEBUG_ON_SERIAL_PORT=TRUE to the build command.

In D45049#1027224, @jhb wrote:

So for "bare metal" cases like booting with a bootrom (e.g. UEFI) we should arguably leave bars unregistered since the raw firmware probably does that. Only the bhyveload case probably wants to enable BARs on startup. I would be fine with not using a user-facing config option but basically making the test in this patch be some kind of "is there a bootrom or not" check. Would need to test it on amd64 with UEFI.

I think there's some overlap here with D45498. At a glance this seems fine to me.

This looks fine to me. I don't really like the duplication of fdt.c, but it's not that much code and it's not obvious to me that creating a generic FDT layer is useful at this point.

I like the idea, but I wonder if it'd be better to modify pw(8) to create a directory under /usr/obj/home with the correct ownership?

Apply feedback, don't use fractional times.

In D45518#1038713, @rlibby wrote:

In D45518#1038712, @rlibby wrote:

In D45518#1038621, @markj wrote:

I don't know this GEOM well enough to say whether this is right, but I can't see any reason it's wrong. It's unfortunate that there are no regression tests for this code.

I see there are tests for other geoms in tests/sys/geom, but I haven't figured out how to run them yet. Do you know? I might be able to cobble together something basic at least.

Ah, I got it with

sudo kyua test -k /usr/tests/sys/geom/Kyuafile

They are not hooked up to the sys/Kyuafile. I wonder why.

The diff looks right to me. @np , @kib , and @erj might want to comment on the cxgbe, mlx5 and irdma diffs respectively.

I don't know this GEOM well enough to say whether this is right, but I can't see any reason it's wrong. It's unfortunate that there are no regression tests for this code.

In D45510#1038477, @rlibby wrote:

In D45510#1038444, @rlibby wrote:

In D45510#1038439, @dougm wrote:

I would expect an iterator to consist of an array of maxdepth pointers and a parent index and a leaf index. Initialization means setting the first array element to root and the counter to 1. Searching updates the path, and insert/remove operations can happen in constant time at the bottom of the path, given the leaf index, and iteration happens by incrementing the leaf index to the next non-null child or, if there is none, backing up the path as far as necessary to take a step further right.

Yes I think that would work, and probably has simpler logic. IIUC the maxdepth is about 16 or 21 levels (depending on arch, and I may be off by 1), so 128 or 84 bytes for the array. I guess that's not so bad.

Thinking more, I agree. Two pointers really only saves stack space, which is not so precious. A full depth stack of pointers is simpler and faster, and anyway reasonably bounded.

Seems ok to me.

Sorry for not having done this when I added the -fno-sanitize* pattern.