[PATCH 0/6] Prioritize volumes on boot media, code simplifications

[Jan Kiszka]

Resolve the issue that config environments from the boot medium were
already preferred but not kernel artifacts if there were identically
labeled partitions on both boot and non-boot media.

Along that, several simplifications of the code were implemented.

Please review/test carefully!

Jan

Jan Kiszka (6):
Add dependency on generated config.h to efi build
Cache boot medium state of volumes
Prioritize volumes on boot media
Avoid enumerating and filtering config partitions on save
Drop ignored return code from save_current_config
Fold filter_cfg_parts into enumerate_cfg_parts

Makefile.am | 2 +-
env/fatvars.c | 41 ++++++--------------------------
env/syspart.c | 60 +++++++++++------------------------------------
include/syspart.h | 1 -
include/utils.h | 2 +-
utils.c | 47 +++++++++++++++++++++++--------------
6 files changed, 52 insertions(+), 101 deletions(-)

--
2.43.0

[Jan Kiszka]

From: Jan Kiszka <jan.k...@siemens.com>

The caller ignores it, and we can simplify the code when only printing
but not returning an error.

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---
env/fatvars.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/env/fatvars.c b/env/fatvars.c
index 079595c..eacd72b 100644
--- a/env/fatvars.c
+++ b/env/fatvars.c
@@ -23,16 +23,16 @@
static int current_partition = 0;
static BG_ENVDATA *env;

-static BG_STATUS save_current_config(const UINTN *config_volumes, UINTN numHandles)
+static VOID save_current_config(const UINTN *config_volumes, UINTN numHandles)
{
EFI_STATUS efistatus;

if (numHandles != ENV_NUM_CONFIG_PARTS) {
- ERROR(L"Unexpected number of config partitions: found %d, but expected %d.\n",
- numHandles, ENV_NUM_CONFIG_PARTS);
/* In case of saving, this must be treated as error, to not
* overwrite another partition's config file. */
- return BG_CONFIG_ERROR;
+ ERROR(L"Unexpected number of config partitions: found %d, but expected %d.\n",
+ numHandles, ENV_NUM_CONFIG_PARTS);
+ return;
}

VOLUME_DESC *v = &volumes[config_volumes[current_partition]];
@@ -42,7 +42,7 @@ static BG_STATUS save_current_config(const UINTN *config_volumes, UINTN numHandl
if (EFI_ERROR(efistatus)) {
ERROR(L"Could not open environment file on system partition %d: %r\n",
current_partition, efistatus);
- return BG_CONFIG_ERROR;
+ return;
}

UINTN writelen = sizeof(BG_ENVDATA);
@@ -55,16 +55,11 @@ static BG_STATUS save_current_config(const UINTN *config_volumes, UINTN numHandl
efistatus = fh->Write(fh, &writelen, (VOID *)&env[current_partition]);
if (EFI_ERROR(efistatus)) {
ERROR(L"Cannot write environment to file: %r\n", efistatus);
- (VOID) close_cfg_file(v->root, fh);
- return BG_CONFIG_ERROR;
}

if (EFI_ERROR(close_cfg_file(v->root, fh))) {
ERROR(L"Could not close environment config file.\n");
- return BG_CONFIG_ERROR;
}
-
- return BG_SUCCESS;
}

BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
--
2.43.0

[Jan Kiszka]

From: Jan Kiszka <jan.k...@siemens.com>

We already did that in the caller load_config. Just pass the result, and
can save a lot of effort and code.

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---

env/fatvars.c | 35 ++++++++---------------------------
1 file changed, 8 insertions(+), 27 deletions(-)

diff --git a/env/fatvars.c b/env/fatvars.c
index 56ff65a..079595c 100644
--- a/env/fatvars.c
+++ b/env/fatvars.c
@@ -23,32 +23,16 @@

static int current_partition = 0;
static BG_ENVDATA *env;

-static BG_STATUS save_current_config(VOID)
+static BG_STATUS save_current_config(const UINTN *config_volumes, UINTN numHandles)
{
- BG_STATUS result = BG_CONFIG_ERROR;
EFI_STATUS efistatus;
- UINTN numHandles = volume_count;
- UINTN *config_volumes;
-
- config_volumes = (UINTN *)AllocatePool(sizeof(UINTN) * volume_count);
- if (!config_volumes) {
- ERROR(L"Could not allocate memory for config partition mapping.\n");
- return result;
- }
-
- if (EFI_ERROR(enumerate_cfg_parts(config_volumes, &numHandles))) {
- ERROR(L"Could not enumerate config partitions.\n");
- goto scc_cleanup;
- }
-
- numHandles = filter_cfg_parts(config_volumes, numHandles);

if (numHandles != ENV_NUM_CONFIG_PARTS) {

ERROR(L"Unexpected number of config partitions: found %d, but expected %d.\n",

numHandles, ENV_NUM_CONFIG_PARTS);
/* In case of saving, this must be treated as error, to not
* overwrite another partition's config file. */

- goto scc_cleanup;
+ return BG_CONFIG_ERROR;

}

VOLUME_DESC *v = &volumes[config_volumes[current_partition]];

@@ -58,7 +42,7 @@ static BG_STATUS save_current_config(VOID)

if (EFI_ERROR(efistatus)) {
ERROR(L"Could not open environment file on system partition %d: %r\n",
current_partition, efistatus);

- goto scc_cleanup;
+ return BG_CONFIG_ERROR;
}

UINTN writelen = sizeof(BG_ENVDATA);
@@ -72,18 +56,15 @@ static BG_STATUS save_current_config(VOID)

if (EFI_ERROR(efistatus)) {
ERROR(L"Cannot write environment to file: %r\n", efistatus);

(VOID) close_cfg_file(v->root, fh);
- goto scc_cleanup;
+ return BG_CONFIG_ERROR;

}

if (EFI_ERROR(close_cfg_file(v->root, fh))) {
ERROR(L"Could not close environment config file.\n");

- goto scc_cleanup;
+ return BG_CONFIG_ERROR;
}

- result = BG_SUCCESS;
-scc_cleanup:
- FreePool(config_volumes);
- return result;
+ return BG_SUCCESS;
}

BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
@@ -214,7 +195,7 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
* zero-revision */
env[latest_idx].ustate = USTATE_FAILED;
env[latest_idx].revision = REVISION_FAILED;
- save_current_config();
+ save_current_config(config_volumes, numHandles);
/* We must boot with the configuration that was active before
*/
current_partition = pre_latest_idx;
@@ -222,7 +203,7 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
/* If this configuration has never been booted with, set ustate
* to indicate that this configuration is now being tested */
env[latest_idx].ustate = USTATE_TESTING;
- save_current_config();
+ save_current_config(config_volumes, numHandles);
}

bglp->payload_path = StrDuplicate(env[current_partition].kernelfile);
--
2.43.0

[Jan Kiszka]

From: Jan Kiszka <jan.k...@siemens.com>

This ensures that kernels are preferably loaded from the boot medium in
case there are multiple partitions with identical label names. For that
purpose, move all volumes on the boot medium to the front of the list.

As we are potentially reordering the list while building it, we need to
postpone its printing to the end when it is fully constructed.

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---

utils.c | 42 +++++++++++++++++++++++++-----------------
1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/utils.c b/utils.c
index 52bf034..6bc32a0 100644
--- a/utils.c
+++ b/utils.c
@@ -108,7 +108,7 @@ EFI_STATUS get_volumes(VOLUME_DESC **volumes, UINTN *count)
EFI_HANDLE *handles = NULL;
EFI_GUID sfspGuid = SIMPLE_FILE_SYSTEM_PROTOCOL;
UINTN handleCount = 0;
- UINTN index, rootCount = 0;
+ UINTN index, rootCount = 0, bootCount = 0;

EFI_FILE_HANDLE tmp;

@@ -133,7 +133,6 @@ EFI_STATUS get_volumes(VOLUME_DESC **volumes, UINTN *count)

for (index = 0; index < handleCount; index++) {
EFI_FILE_IO_INTERFACE *fs = NULL;
- CHAR16 *devpathstr;

status = BS->HandleProtocol(
handles[index], &sfspGuid, (VOID **)&fs);
@@ -155,29 +154,38 @@ EFI_STATUS get_volumes(VOLUME_DESC **volumes, UINTN *count)
ERROR(L"Could not get device path for config partition, skipping.\n");
continue;
}
- devpathstr = DevicePathToStr(devpath);

+ UINTN target = rootCount;
BOOLEAN onbootmedium = IsOnBootMedium(devpath);
-
- (*volumes)[rootCount].root = tmp;
- (*volumes)[rootCount].devpath = devpath;
- (*volumes)[rootCount].onbootmedium = onbootmedium;
- (*volumes)[rootCount].fslabel =
- get_volume_label((*volumes)[rootCount].root);
- (*volumes)[rootCount].fscustomlabel =
- get_volume_custom_label((*volumes)[rootCount].root);
- INFO(L"Volume %d: ", rootCount);
if (onbootmedium) {
- INFO(L"(On boot medium) ");
+ CopyMem(&(*volumes)[bootCount + 1], &(*volumes)[bootCount],
+ (rootCount - bootCount) * sizeof(VOLUME_DESC));
+ target = bootCount++;
}
- INFO(L"%s, LABEL=%s, CLABEL=%s\n",
- devpathstr, (*volumes)[rootCount].fslabel,
- (*volumes)[rootCount].fscustomlabel);

- FreePool(devpathstr);
+ (*volumes)[target].root = tmp;
+ (*volumes)[target].devpath = devpath;
+ (*volumes)[target].onbootmedium = onbootmedium;
+ (*volumes)[target].fslabel =
+ get_volume_label((*volumes)[target].root);
+ (*volumes)[target].fscustomlabel =
+ get_volume_custom_label((*volumes)[target].root);

rootCount++;
}
+
+ for (index = 0; index < rootCount; index++) {
+ INFO(L"Volume %d: ", index);
+ if ((*volumes)[index].onbootmedium) {
+ INFO(L"(On boot medium) ");
+ }
+ CHAR16 *devpathstr = DevicePathToStr((*volumes)[index].devpath);
+ INFO(L"%s, LABEL=%s, CLABEL=%s\n",
+ devpathstr, (*volumes)[index].fslabel,
+ (*volumes)[index].fscustomlabel);
+ FreePool(devpathstr);
+ }
+
*count = rootCount;
return EFI_SUCCESS;
}
--
2.43.0

[MOESSBAUER, Felix]

On Mon, 2024-12-09 at 14:39 +0100, Jan Kiszka wrote:
> Resolve the issue that config environments from the boot medium were
> already preferred but not kernel artifacts if there were identically
> labeled partitions on both boot and non-boot media.
>
> Along that, several simplifications of the code were implemented.
>
> Please review/test carefully!

Hi,

I successfully tested this on a platform with multiple EBG devices
where previously the wrong partition was booted. Now the EFI part also
behaves as expected.

Tested-by: Felix Moessbauer <felix.mo...@siemens.com>

Felix

>
> Jan
>
> Jan Kiszka (6):
>   Add dependency on generated config.h to efi build
>   Cache boot medium state of volumes
>   Prioritize volumes on boot media
>   Avoid enumerating and filtering config partitions on save
>   Drop ignored return code from save_current_config
>   Fold filter_cfg_parts into enumerate_cfg_parts
>
>  Makefile.am       |  2 +-
>  env/fatvars.c     | 41 ++++++--------------------------
>  env/syspart.c     | 60 +++++++++++----------------------------------
> --
>  include/syspart.h |  1 -
>  include/utils.h   |  2 +-
>  utils.c           | 47 +++++++++++++++++++++++--------------
>  6 files changed, 52 insertions(+), 101 deletions(-)
>

--

Siemens AG, Technology
Linux Expert Center