EFI Boot Menu not showing other partitions

[beaglejoe]

The <SpaceBar> EFI Boot Menu does not show any bootable partitions after the one it selects to boot.

[beaglejoe]

To duplicate:

qemu-system-x86_64 -m 4096 -enable-kvm -machine q35 -smp 3 -net nic -net user \
	-hda disk1.img \
	-hdb disk2.img \
	-cdrom /haiku/haiku/generated.x86_64/haiku-nightly-anyboot.iso \
	-bios /usr/share/qemu/OVMF.fd

Hit <SpaceBar> to bring up the Boot Menu

Partition layout: ( all Haiku partitions have installs)

[beaglejoe]

When the user invokes the menu mount_file_systems() (in src/system/boot/loader/vfs.cpp) to mount any remaining partitions.

This function gets short-circuited at this block:

	// add all block devices the platform has for us

	status_t status = platform_add_block_devices(args, &gBootDevices);
	if (status < B_OK)
		return status;

(In src/system/boot/platform/efi/devices.cpp)

status_t
platform_add_block_devices(struct stage2_args *args, NodeList *devicesList)
{
	TRACE("%s: called\n", __func__);

	//TODO: Currently we add all in platform_add_boot_device
	return B_ENTRY_NOT_FOUND;
}

Changing this to return B_OK fixes some of the problem. Presumably affects other architectures.

[beaglejoe]

Now, I am only missing Haiku2 partition from the menu. This is because it is on the same drive as the selected partition. In get_boot_file_system() (vfs.cpp) the iteration (and thus the partition mounting) stops when the bootable partition is discovered.

---

[beaglejoe]

I am thinking that the best way to fix this is to add a mounted flag to the partition. Then mount_file_systems() could be simplified to just iterating over gPartitions and mount any without the flag set.

[kallisti5]

Which hrev is this @beaglejoe?

Any opinions here @jessicah?

This behavior is still an improvement over having to re-select the partition on every startup. (though obviously not ideal)

[jessicah]

Likely fallout from the too simplistic device scan. I would personally recommend going back to my code, and fix the bugs. These were all the sorts of complex cases that it used to handle.

As for platform_add_block_devices, this should look something like devicesList->Count() > 0 ? B_OK : B_ENTRY_NOT_FOUND, as if there's none, then obviously it's still an error. From what I understand of tqh's code without looking, it adds all disks to the list.

[beaglejoe]

Replying to kallisti5:

Which hrev is this @beaglejoe?

hrev55217

[beaglejoe]

Replying to jessicah:

Likely fallout from the too simplistic device scan. I would personally recommend going back to my code, and fix the bugs. These were all the sorts of complex cases that it used to handle.

As for platform_add_block_devices, this should look something like devicesList->Count() > 0 ? B_OK : B_ENTRY_NOT_FOUND, as if there's none, then obviously it's still an error. From what I understand of tqh's code without looking, it adds all disks to the list.

It seems that currently 'platform_add_boot_device()' does the work of platform_add_block_devices so returning an error from platform_add_block_devices seems a mistake??

Also, in this code:

status_t
platform_get_boot_partitions(struct stage2_args *args, Node *bootDevice,
		NodeList *partitions, NodeList *bootPartitions)
{
	NodeIterator iterator = partitions->GetIterator();
	boot::Partition *partition = NULL;
	while ((partition = (boot::Partition*)iterator.Next()) != NULL) {
		if (device_contains_partition((EfiDevice*)bootDevice, partition)) {
			iterator.Remove();
			bootPartitions->Insert(partition);
		}
	}

	return bootPartitions->Count() > 0 ? B_OK : B_ENTRY_NOT_FOUND;
}

Why the call to 'iterator.Remove();' ?
If I comment that out (see attached patch). The partitions show up in the menu. (My previous thought about the flag are unneeded)

[jessicah]

I just said it should return B_OK if the passed in list is non-empty. If it's empty, then it should return an error, because it has no bootable devices to pass to the generic loader code.

Regarding iterator.Remove(), that may have been dependent on the old scanning behaviour, and may not be required in the new code. What used to happen is that an entry would end up duplicated when using the select a boot volume in the menu.

If you can confirm that removing iterator.Remove() doesn't result in duplicate entries, then it is safe to remove.

This is all why I had marked my patch as WIP on Gerrit, as it does need extensive testing, which I haven't done.

[jessicah]

please test ​https://review.haiku-os.org/c/haiku/+/4160

[beaglejoe]

Replying to jessicah:

I just said it should return B_OK if the passed in list is non-empty. If it's empty, then it should return an error, because it has no bootable devices to pass to the generic loader code.

Thanks, that makes perfect sense.

Regarding iterator.Remove(), that may have been dependent on the old scanning behaviour, and may not be required in the new code. What used to happen is that an entry would end up duplicated when using the select a boot volume in the menu.

I have seen that also.

If you can confirm that removing iterator.Remove() doesn't result in duplicate entries, then it is safe to remove.

This is all why I had marked my patch as WIP on Gerrit, as it does need extensive testing, which I haven't done.

Indeed. Testing the EFI stuff is difficult (and time consuming!). I only have one hardware system to test on. (plus of course qemu).

[beaglejoe]

Replying to jessicah:

please test ​https://review.haiku-os.org/c/haiku/+/4160

No duplicate entries and all partitions listed. Tested on Intel NUC (with nvme drive and sata drive) Tested on qemu with two GPT drives. Tested on qemu with one GPT drive and one MBR drive. All bootable partitions were listed. Non-bootable partitions were not listed.

[jessicah]

Fixed in hrev55221.