stand/boot1.efi: use the bootonce dataset as root dataset
ClosedPublic
Actions

Authored by glebius on Jun 2 2023, 5:22 AM.

Details

Reviewers

imp
tsoome
manu

Commits

rGbbc64cf66cdf: stand/boot1.efi: use the bootonce dataset as root dataset

Summary

Before this change we would only pass the bootonce dataset name
to the environment for the next loader, while actually reading
the next stage loader from the 'bootfs' dataset, not the bootonce
dataset.

Another problem fixed by this change is a boot from a configuration
when bootonce attribute is present, but 'bootfs' property is not set.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

glebius created this revision.Jun 2 2023, 5:22 AM

Herald added a reviewer: manu. · View Herald TranscriptJun 2 2023, 5:22 AM

Herald added subscribers: dab, bcran. · View Herald Transcript

glebius requested review of this revision.Jun 2 2023, 5:22 AM

Harbormaster completed remote builds in B51855: Diff 122747.Jun 2 2023, 5:22 AM

glebius added a parent revision: D40388: stand/loader.efi: read zfs bootonce attribute before checking currdev.Jun 2 2023, 5:22 AM

The trick question there is, the bootonce property is supposed to have generic boot descriptor, it does not have to be just zfs dataset name.... of course, if you have pools, then most likely you only do use zfs datasets.

In D40389#919527, @tsoome wrote:

The trick question there is, the bootonce property is supposed to have generic boot descriptor, it does not have to be just zfs dataset name.... of course, if you have pools, then most likely you only do use zfs datasets.

But all this code belongs to the ZFS module of the boot1.

I think we have a bug with this, which I think we can fix as I suggested. I don't think that spa_name is ever change once it's cerated, so I think at least the error message is OK, though it doesn't specify which dataset failed.

But maybe we need a loop to try again for all the error cases when we've started with boot once and that fails.

stand/efi/boot1/zfs_module.c
208	this has the same issue... If we try the boot once property and it can't be mounted, we should try again with what we would have done had that boot once not been there. In this case I think it's just having if ((err = zfs_mount_impl(spa, rootobj, &zmount)) != 0) { printf("Failed to mount pool '%s' (%d)\n", spa->spa_name, err); if (rootobj != 0) { err = zfs_mount_impl(spa, 0, &zmount); if (err != 0) { printf("Failed to mount ... return (EFI_NOT_FOUND); } } } but I can't recall if zfs_lookup_dataset updates the spa to point to the looked up image or not...

Though thinking about it, one could likely make the same argument as '88 and say this is existing behavior.
If you'd agree with that, then this is fine and we'll look at making things better.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 8 2023, 6:15 PM

Closed by commit rGbbc64cf66cdf: stand/boot1.efi: use the bootonce dataset as root dataset (authored by glebius). · Explain Why

This revision was automatically updated to reflect the committed changes.

glebius added a commit: rGbbc64cf66cdf: stand/boot1.efi: use the bootonce dataset as root dataset.