I believe it's also legal to go and map two BARs to overlap so long as you don't try to access the overlapping range when that's the case? QEMU just maintains a list of PCI BARs and goes for the first one in the list that matches. Allocating the big chunk of MMIO memory statically at the start and just maintaining the list layered on top in PCI code as you mess with BARs seems like it would be a simple, more general fix, and should perform just fine? (Though passthrough may well be "fun" as you mention)

In D45049#1027045, @jrtc27 wrote:

I believe it's also legal to go and map two BARs to overlap so long as you don't try to access the overlapping range when that's the case? QEMU just maintains a list of PCI BARs and goes for the first one in the list that matches. Allocating the big chunk of MMIO memory statically at the start and just maintaining the list layered on top in PCI code as you mess with BARs seems like it would be a simple, more general fix, and should perform just fine? (Though passthrough may well be "fun" as you mention)

Yes, we could also permit overlapping BARs since u-boot (empirically) won't try to access them in the window where there's some ambiguity. It's not too hard to implement and I thought about that, but this approach is simpler and is closer to what u-boot actually expects, which is that it needs to program BARs itself. It feels a bit questionable to make bhyve more complicated to support what u-boot's doing when we can instead make bhyve do less. If there are further reasons to support overlapping MMIO regions then it's easy to revert this patch.

My mental model of what is safe is that it's allowed to move BARs around so long as you disable decoding while you do so, and I'm pretty sure we already do that now to handle FreeBSD kernels (and other OS kernels) that rewrite BARs to size them during boot. We have to avoid trying to register/unregister them while rewriting, and I thought that was driven by if the I/O space was enabled. (See how update_bar_address makes the register_bar call conditional on encoding being enabled.) It sounds like u-boot is just buggy here in that it isn't disabling decoding while it messes with the BARs. This idea is ok though. I wonder if FreeBSD/amd64 boots with this set to true. :)

In D45049#1027191, @jhb wrote:

My mental model of what is safe is that it's allowed to move BARs around so long as you disable decoding while you do so, and I'm pretty sure we already do that now to handle FreeBSD kernels (and other OS kernels) that rewrite BARs to size them during boot. We have to avoid trying to register/unregister them while rewriting, and I thought that was driven by if the I/O space was enabled. (See how update_bar_address makes the register_bar call conditional on encoding being enabled.) It sounds like u-boot is just buggy here in that it isn't disabling decoding while it messes with the BARs. This idea is ok though. I wonder if FreeBSD/amd64 boots with this set to true. :)

Aha. So yes, U-Boot indeed isn't disabling decoding whilst it probes and allocates the BARs. It *is* only enabling decoding for the types of BARs it allocates once it's done, but it doesn't write the initial value back first. https://source.denx.de/u-boot/u-boot/-/blob/ff0de1f0557ed7d2dab47ba976a37347a1fdc432/drivers/pci/pci_auto.c#L42

But that still doesn't help you if you move the BAR for A to overlap one of B's before you've probed it? bhyve will still get in a mess in that scenario.

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.

IIRC, the default value of the busmasteren, porten and memen bits is 0 according to PCI specification. So, as jhb already mentioned, it should be valid to unset these values when booting with a bootrom.

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.

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#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.

Thank you! I think this means though that @jhb 's suggestion isn't viable, as we generally expect newer bhyve to work properly with older EDK2. In other words, we need some config flag which lets a particular platform say, "I want to enable BARs before executing any guest code". Or is there something more clever we can do?

In D45049#1040000, @markj wrote:

Thank you! I think this means though that @jhb 's suggestion isn't viable, as we generally expect newer bhyve to work properly with older EDK2. In other words, we need some config flag which lets a particular platform say, "I want to enable BARs before executing any guest code". Or is there something more clever we can do?

Commonly a bootrom enumerates BARs on it's own and we can skip it in bhyve if a bootrom is given. Unfortunately, that's not working for older EDK2. I'm not sure if it's possible to read a PCD value from a given EDK2 image. Nevertheless, it wouldn't be trivial to detect an EDK2 image and scan it for a specific PCD value.

Btw. is this an issue on arm64? We don't have an bhyve EDK2 for arm64 yet. So, we could use latest EDK2 for our first arm64 image and assume that EDK2 on arm64 always supports BARs enumeration.

In D45049#1040114, @corvink wrote:

In D45049#1040000, @markj wrote:

Thank you! I think this means though that @jhb 's suggestion isn't viable, as we generally expect newer bhyve to work properly with older EDK2. In other words, we need some config flag which lets a particular platform say, "I want to enable BARs before executing any guest code". Or is there something more clever we can do?

Commonly a bootrom enumerates BARs on it's own and we can skip it in bhyve if a bootrom is given. Unfortunately, that's not working for older EDK2. I'm not sure if it's possible to read a PCD value from a given EDK2 image. Nevertheless, it wouldn't be trivial to detect an EDK2 image and scan it for a specific PCD value.

Btw. is this an issue on arm64? We don't have an bhyve EDK2 for arm64 yet. So, we could use latest EDK2 for our first arm64 image and assume that EDK2 on arm64 always supports BARs enumeration.

Right now there is only one usable bootrom for arm64 that I'm aware of, u-boot-bhyve-arm64. So if someone works on EDK2 for arm64, I think we'd prefer for it to have the same behaviour as u-boot, i.e., it should have PcdPciDisableBusEnumeration=TRUE.

On amd64 I think it's more important to preserve compatibility with existing EDK2. With this patch, if someone wants to use an alternate bootrom, they have the option to set pci.enable_bars=false as well. The only other alternative I can imagine is to have some kind of "bootrom profile", wherein one can specify a collection of variables associated with a particular bootrom, not just the path to the image.

Thinking about this more, I wonder if we want to just leave BARs all set to 0 in this case and not even bother reserving resource ranges? I'll have to see how EDK2 copes with that I guess.

In D45049#1052826, @jhb wrote:

Thinking about this more, I wonder if we want to just leave BARs all set to 0 in this case and not even bother reserving resource ranges? I'll have to see how EDK2 copes with that I guess.

Devices don't need host resources backing them. Writing all 1s with the io and mem bits disable lets firmware proble the size. The original value on x86 is the space the BIOS assigned. On arm it's been 0 for the ancient uboot i messed with. Not sure about EDK2 but seems like a good place to start.

Do you plan to do this as one commit? I'd be tempted to suggest doing all the bootrom reorganization as a separate commit before doing the PCI changes.

In D45049#1053202, @jhb wrote:

Do you plan to do this as one commit? I'd be tempted to suggest doing all the bootrom reorganization as a separate commit before doing the PCI changes.

Yes, I should split it.