Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sleep/hibernate-resume: pass hibernate location through efivar for resume without kernel cmdline #27330

Merged
merged 5 commits into from
Jun 23, 2023

Conversation

YHNdnzj
Copy link
Member

@YHNdnzj YHNdnzj commented Apr 18, 2023

This implements the efivar part of #27247.

@github-actions github-actions bot added sleep please-review PR is ready for (re-)review by a maintainer labels Apr 18, 2023
@YHNdnzj YHNdnzj changed the title sleep/hibernate-resume: pass HibernateInfo through efivar for further resume sleep/hibernate-resume: pass HibernateInfo through efivar for resume without kernel cmdline Apr 18, 2023
@YHNdnzj YHNdnzj marked this pull request as draft April 19, 2023 06:16
@YHNdnzj YHNdnzj force-pushed the hibernate-resume-auto branch 5 times, most recently from 2d27b19 to 477ef39 Compare April 19, 2023 14:48
@YHNdnzj YHNdnzj marked this pull request as ready for review April 19, 2023 14:48
src/sleep/sleep.c Outdated Show resolved Hide resolved
src/sleep/sleep.c Outdated Show resolved Hide resolved
src/sleep/sleep.c Outdated Show resolved Hide resolved
src/sleep/sleep.c Outdated Show resolved Hide resolved
src/hibernate-resume/hibernate-resume-generator.c Outdated Show resolved Hide resolved
src/hibernate-resume/hibernate-resume-generator.c Outdated Show resolved Hide resolved
src/hibernate-resume/hibernate-resume-generator.c Outdated Show resolved Hide resolved
src/hibernate-resume/hibernate-resume.c Outdated Show resolved Hide resolved
src/hibernate-resume/hibernate-resume.c Outdated Show resolved Hide resolved
src/hibernate-resume/hibernate-resume.c Outdated Show resolved Hide resolved
@poettering poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Apr 21, 2023
@github-actions github-actions bot added meson units util-lib please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Apr 21, 2023
@YHNdnzj YHNdnzj force-pushed the hibernate-resume-auto branch 6 times, most recently from dcdc0c2 to c4329eb Compare April 21, 2023 18:16
@poettering poettering added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed please-review PR is ready for (re-)review by a maintainer labels Jun 23, 2023
@poettering
Copy link
Member

lgtm! thanks!

@poettering poettering merged commit d39cdbb into systemd:main Jun 23, 2023
@github-actions github-actions bot removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Jun 23, 2023
@YHNdnzj YHNdnzj deleted the hibernate-resume-auto branch June 23, 2023 21:25
fbuihuu added a commit to fbuihuu/systemd that referenced this pull request Jul 26, 2023
…d EFI is disabled

Otherwise in such case a first `systemctl hibernate` would fail but would still
initialize /sys/power/resume fooling a second `systemctl hibernate` into
believing that 'resume=' is correctly set and can be used by the resume process
to find the swap device to resume from.

Follow-up for systemd#27330.
fbuihuu added a commit to fbuihuu/systemd that referenced this pull request Jul 27, 2023
…d EFI is disabled

Otherwise in such case a first `systemctl hibernate` would fail but would still
initialize /sys/power/resume fooling a second `systemctl hibernate` into
believing that 'resume=' is correctly set and can be used by the resume process
to find the swap device to resume from.

Follow-up for systemd#27330.
fbuihuu added a commit to fbuihuu/systemd that referenced this pull request Jul 27, 2023
…d EFI is disabled

Otherwise in such case a first `systemctl hibernate` would fail but would still
initialize /sys/power/resume fooling a second `systemctl hibernate` into
believing that 'resume=' is correctly set and can be used by the resume process
to find the swap device to resume from.

Follow-up for systemd#27330.
bluca pushed a commit that referenced this pull request Jul 27, 2023
…d EFI is disabled

Otherwise in such case a first `systemctl hibernate` would fail but would still
initialize /sys/power/resume fooling a second `systemctl hibernate` into
believing that 'resume=' is correctly set and can be used by the resume process
to find the swap device to resume from.

Follow-up for #27330.
YHNdnzj added a commit to YHNdnzj/systemd that referenced this pull request Aug 4, 2023
YHNdnzj added a commit to YHNdnzj/systemd that referenced this pull request Aug 4, 2023
keszybz added a commit to keszybz/systemd that referenced this pull request Sep 8, 2023
We already had a similar check that was removed, see
8340b76 (*). The kernel supports loading of a
resume image from a different kernel version. This makes sense, because the
goal of "resume" is to replace the running system by a saved memory image, so
it doesn't really matter that the short-lived kernel is different.

By removing the check, we make the process more reliable: for example, the user
may select a different kernel from a list, or not have the previously running
kernel in /boot at all, etc. Requiring the exact same kernel version makes the
process more fragile for no benefit.

Similar reasoning holds for the image version: the image may be updated, and
for example an older kernel+initrd might be used, with an embedded VERSION_ID
that is not the latest. This is fine, and the check is not useful.

I left the check for ID/IMAGE_ID: we probably don't want to use the resume
image if the hibernation was done from a different installation.

OTOH, I think it is useful to *write* all the details to the EFI var. As
discussed in systemd#29037, we may want to
show this information in the boot loader. It may also be useful for debugging.

(*) Also again discussed and verified in
systemd#27330 (comment).
keszybz added a commit to keszybz/systemd that referenced this pull request Sep 8, 2023
We already had a similar check that was removed, see
8340b76 (*). The kernel supports loading of a
resume image from a different kernel version. This makes sense, because the
goal of "resume" is to replace the running system by a saved memory image, so
it doesn't really matter that the short-lived kernel is different.

By removing the check, we make the process more reliable: for example, the user
may select a different kernel from a list, or not have the previously running
kernel in /boot at all, etc. Requiring the exact same kernel version makes the
process more fragile for no benefit.

Similar reasoning holds for the image version: the image may be updated, and
for example an older kernel+initrd might be used, with an embedded VERSION_ID
that is not the latest. This is fine, and the check is not useful.

I left the check for ID/IMAGE_ID: we probably don't want to use the resume
image if the hibernation was done from a different installation.

OTOH, I think it is useful to *write* all the details to the EFI var. As
discussed in systemd#29037, we may want to
show this information in the boot loader. It may also be useful for debugging.

(*) Also again discussed and verified in
systemd#27330 (comment).

", ignored" is dropped, since this failure is likely to cause the following
check to fail. Better not to say anything then to say the misleading thing.
keszybz added a commit to keszybz/systemd that referenced this pull request Sep 11, 2023
We already had a similar check that was removed, see
8340b76 (*). The kernel supports loading of a
resume image from a different kernel version. This makes sense, because the
goal of "resume" is to replace the running system by a saved memory image, so
it doesn't really matter that the short-lived kernel is different.

By removing the check, we make the process more reliable: for example, the user
may select a different kernel from a list, or not have the previously running
kernel in /boot at all, etc. Requiring the exact same kernel version makes the
process more fragile for no benefit.

Similar reasoning holds for the image version: the image may be updated, and
for example an older kernel+initrd might be used, with an embedded VERSION_ID
that is not the latest. This is fine, and the check is not useful.

I left the check for ID/IMAGE_ID: we probably don't want to use the resume
image if the hibernation was done from a different installation.

(Note: why not check VERSION_ID/IMAGE_VERSION? Because of the following
scenario: a user has an installation of Fedora 35, and they upgrade to Fedora
36, which means that the os-release file on disk gets replaced and now
specifies VERSION_ID=36. But the running kernel is not replaced, and its
package is not removed because the running kernel version is never removed, so
we still have a boot entry that in initrd-release says VERSION_ID=35. Without
rebooting, the user does hibernation. When resuming, we want to resume, no
matter if one of the new entries with VERSION_ID=36 or one of the old entries
with VERSION_ID=35 is picked in the boot loader menu.

If the installation is image-based, i.e. it has IMAGE_ID+IMAGE_VERSION, the
situation is similar: after an upgrade, we may still have an boot entry from
before the upgrade. Using an older kernel+initrd to boot and switch-root into a
newer installation is supported and is rather common.

In fact, it is a rather common situation that the version reported by the boot
entry (or stored internally in the initrd-release in the initrd) does not match
the actual system on disk. Generally, this metadata is saved when the boot menu
entry is written and does not reflect subsequent upgrades. Various
distributions generally keep at least 3 kernels after a upgrade, and during an
upgrade only install one new, which means that after a major upgrade, generally
there will be at least two kernels which have mismatched version information.)

OTOH, I think it is useful to *write* all the details to the EFI var. As
discussed in systemd#29037, we may want to
show this information in the boot loader. It is also useful for debugging.

(*) Also again discussed and verified in
systemd#27330 (comment).

", ignored" is dropped, since this failure is likely to cause the following
check to fail. Better not to say anything then to say the misleading thing.
keszybz added a commit to keszybz/systemd that referenced this pull request Oct 11, 2023
We already had a similar check that was removed, see
8340b76 (*). The kernel supports loading of a
resume image from a different kernel version. This makes sense, because the
goal of "resume" is to replace the running system by a saved memory image, so
it doesn't really matter that the short-lived kernel is different.

By removing the check, we make the process more reliable: for example, the user
may select a different kernel from a list, or not have the previously running
kernel in /boot at all, etc. Requiring the exact same kernel version makes the
process more fragile for no benefit.

Similar reasoning holds for the image version: the image may be updated, and
for example an older kernel+initrd might be used, with an embedded VERSION_ID
that is not the latest. This is fine, and the check is not useful.

I left the check for ID/IMAGE_ID: we probably don't want to use the resume
image if the hibernation was done from a different installation.

(Note: why not check VERSION_ID/IMAGE_VERSION? Because of the following
scenario: a user has an installation of Fedora 35, and they upgrade to Fedora
36, which means that the os-release file on disk gets replaced and now
specifies VERSION_ID=36. But the running kernel is not replaced, and its
package is not removed because the running kernel version is never removed, so
we still have a boot entry that in initrd-release says VERSION_ID=35. Without
rebooting, the user does hibernation. When resuming, we want to resume, no
matter if one of the new entries with VERSION_ID=36 or one of the old entries
with VERSION_ID=35 is picked in the boot loader menu.

If the installation is image-based, i.e. it has IMAGE_ID+IMAGE_VERSION, the
situation is similar: after an upgrade, we may still have an boot entry from
before the upgrade. Using an older kernel+initrd to boot and switch-root into a
newer installation is supported and is rather common.

In fact, it is a rather common situation that the version reported by the boot
entry (or stored internally in the initrd-release in the initrd) does not match
the actual system on disk. Generally, this metadata is saved when the boot menu
entry is written and does not reflect subsequent upgrades. Various
distributions generally keep at least 3 kernels after a upgrade, and during an
upgrade only install one new, which means that after a major upgrade, generally
there will be at least two kernels which have mismatched version information.)

OTOH, I think it is useful to *write* all the details to the EFI var. As
discussed in systemd#29037, we may want to
show this information in the boot loader. It is also useful for debugging.

(*) Also again discussed and verified in
systemd#27330 (comment).

", ignored" is dropped, since this failure is likely to cause the following
check to fail. Better not to say anything then to say the misleading thing.
poettering pushed a commit that referenced this pull request Oct 12, 2023
We already had a similar check that was removed, see
8340b76 (*). The kernel supports loading of a
resume image from a different kernel version. This makes sense, because the
goal of "resume" is to replace the running system by a saved memory image, so
it doesn't really matter that the short-lived kernel is different.

By removing the check, we make the process more reliable: for example, the user
may select a different kernel from a list, or not have the previously running
kernel in /boot at all, etc. Requiring the exact same kernel version makes the
process more fragile for no benefit.

Similar reasoning holds for the image version: the image may be updated, and
for example an older kernel+initrd might be used, with an embedded VERSION_ID
that is not the latest. This is fine, and the check is not useful.

I left the check for ID/IMAGE_ID: we probably don't want to use the resume
image if the hibernation was done from a different installation.

(Note: why not check VERSION_ID/IMAGE_VERSION? Because of the following
scenario: a user has an installation of Fedora 35, and they upgrade to Fedora
36, which means that the os-release file on disk gets replaced and now
specifies VERSION_ID=36. But the running kernel is not replaced, and its
package is not removed because the running kernel version is never removed, so
we still have a boot entry that in initrd-release says VERSION_ID=35. Without
rebooting, the user does hibernation. When resuming, we want to resume, no
matter if one of the new entries with VERSION_ID=36 or one of the old entries
with VERSION_ID=35 is picked in the boot loader menu.

If the installation is image-based, i.e. it has IMAGE_ID+IMAGE_VERSION, the
situation is similar: after an upgrade, we may still have an boot entry from
before the upgrade. Using an older kernel+initrd to boot and switch-root into a
newer installation is supported and is rather common.

In fact, it is a rather common situation that the version reported by the boot
entry (or stored internally in the initrd-release in the initrd) does not match
the actual system on disk. Generally, this metadata is saved when the boot menu
entry is written and does not reflect subsequent upgrades. Various
distributions generally keep at least 3 kernels after a upgrade, and during an
upgrade only install one new, which means that after a major upgrade, generally
there will be at least two kernels which have mismatched version information.)

OTOH, I think it is useful to *write* all the details to the EFI var. As
discussed in #29037, we may want to
show this information in the boot loader. It is also useful for debugging.

(*) Also again discussed and verified in
#27330 (comment).

", ignored" is dropped, since this failure is likely to cause the following
check to fail. Better not to say anything then to say the misleading thing.
ssahani pushed a commit to ssahani/systemd that referenced this pull request Nov 23, 2023
We already had a similar check that was removed, see
8340b76 (*). The kernel supports loading of a
resume image from a different kernel version. This makes sense, because the
goal of "resume" is to replace the running system by a saved memory image, so
it doesn't really matter that the short-lived kernel is different.

By removing the check, we make the process more reliable: for example, the user
may select a different kernel from a list, or not have the previously running
kernel in /boot at all, etc. Requiring the exact same kernel version makes the
process more fragile for no benefit.

Similar reasoning holds for the image version: the image may be updated, and
for example an older kernel+initrd might be used, with an embedded VERSION_ID
that is not the latest. This is fine, and the check is not useful.

I left the check for ID/IMAGE_ID: we probably don't want to use the resume
image if the hibernation was done from a different installation.

(Note: why not check VERSION_ID/IMAGE_VERSION? Because of the following
scenario: a user has an installation of Fedora 35, and they upgrade to Fedora
36, which means that the os-release file on disk gets replaced and now
specifies VERSION_ID=36. But the running kernel is not replaced, and its
package is not removed because the running kernel version is never removed, so
we still have a boot entry that in initrd-release says VERSION_ID=35. Without
rebooting, the user does hibernation. When resuming, we want to resume, no
matter if one of the new entries with VERSION_ID=36 or one of the old entries
with VERSION_ID=35 is picked in the boot loader menu.

If the installation is image-based, i.e. it has IMAGE_ID+IMAGE_VERSION, the
situation is similar: after an upgrade, we may still have an boot entry from
before the upgrade. Using an older kernel+initrd to boot and switch-root into a
newer installation is supported and is rather common.

In fact, it is a rather common situation that the version reported by the boot
entry (or stored internally in the initrd-release in the initrd) does not match
the actual system on disk. Generally, this metadata is saved when the boot menu
entry is written and does not reflect subsequent upgrades. Various
distributions generally keep at least 3 kernels after a upgrade, and during an
upgrade only install one new, which means that after a major upgrade, generally
there will be at least two kernels which have mismatched version information.)

OTOH, I think it is useful to *write* all the details to the EFI var. As
discussed in systemd#29037, we may want to
show this information in the boot loader. It is also useful for debugging.

(*) Also again discussed and verified in
systemd#27330 (comment).

", ignored" is dropped, since this failure is likely to cause the following
check to fail. Better not to say anything then to say the misleading thing.
peckato1 pushed a commit to peckato1/systemd that referenced this pull request Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

sleep/hibernate-resume: acquire generated hibernation info on resume
2 participants