On 11/6/24 09:34, Andrew Cooper wrote:
On 02/11/2024 5:25 pm, Daniel P. Smith wrote:
With all boot modules now labeled by type, it is no longer necessary to
track whether a boot module was identified via the module_map bitmap.
Introduce a set of helpers to search the list of boot modules based on type and
the reference type, pointer or array index, desired. Then drop all uses of
setting a bit in module_map and replace its use for looping with the helpers.
Signed-off-by: Daniel P. Smith <[email protected]>
---
Changes since v7:
- collapse the three module_map patches into one,
- x86/boot: remove module_map usage from microcode loading
- x86/boot: remove module_map usage from xsm policy loading
- x86/boot: remove module_map usage by ramdisk loading
Definitely nicer for having been collapsed together.
Most definitely.
---
xen/arch/x86/cpu/microcode/core.c | 12 ++++-----
xen/arch/x86/include/asm/bootinfo.h | 42 ++++++++++++++++++++++++++++-
xen/arch/x86/setup.c | 28 +++++++++++--------
xen/xsm/xsm_policy.c | 19 +++++--------
4 files changed, 70 insertions(+), 31 deletions(-)
diff --git a/xen/arch/x86/cpu/microcode/core.c
b/xen/arch/x86/cpu/microcode/core.c
index f46464241557..b09cf83249f6 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -790,15 +790,13 @@ static int __init early_microcode_load(struct boot_info
*bi)
if ( opt_scan ) /* Scan for a CPIO archive */
{
- for ( idx = 1; idx < bi->nr_modules; ++idx )
+ for_each_boot_module_by_type(idx, bi, BOOTMOD_UNKNOWN)
Minor, but we treat for_each_* as if they were for loops, so this either
wants to be
for_each_boot_module_by_type ( idx, bi, BOOTMOD_UNKNOWN )
or
for_each_boot_module_by_type (idx, bi, BOOTMOD_UNKNOWN)
spacing wise. There's no agreement between maintainers on the extra
spaces inside brackets or not.
If it is considered a for loop, then I would feel obliged to add the
spacing per the coding style.
However, despite looking at this many times, I've only just realised...
This semantically changes things in a direction that we won't want.
Today, BOOTMOD_RAMDISK only happens a side effect of being "first
BOOTMOD_UNKNOWN standing at the end".
But the EFI boot code ought to set bi->type=RAMDISK explicitly from the
ramdisk= argument (it can probably set type=MICROCODE too), and future
plans with a large HL config probably will be similar.
Anything which sets type=, and type=RAMDISK in particular, prior to
early_microcode_load() excludes it from the search. This is definitely
not what we want.
It's a latent bug for now, but I'd suggest keeping the plain for loop, with
/* Search anything unclaimed or likely to be a CPIO archive. */
if ( bm->type != BOOTMOD_UNKNOWN &&
bm->type != BOOTMOD_RAMDISK )
continue;
as the selection criteria. Probably also want to start from idx=0 to
remove assumptions about the dom0 kernel.
Thoughts?
Yah, as much as it would be nice to use the helper, this is the
exception where there is a complex match condition to be handled. This
will be switched over to an explicit for loop.
{
+ struct boot_module *bm = &bi->mods[idx];
struct cpio_data cd;
- if ( !test_bit(idx, bi->module_map) )
- continue;
-
- size = bi->mods[idx].mod->mod_end;
- data = bootstrap_map_bm(&bi->mods[idx]);
+ size = bm->mod->mod_end;
+ data = bootstrap_map_bm(bm);
if ( !data )
{
printk(XENLOG_WARNING "Microcode: Could not map module %d, size
%zu\n",
@@ -840,7 +838,7 @@ static int __init early_microcode_load(struct boot_info *bi)
return -ENODEV;
}
- if ( !__test_and_clear_bit(idx, bi->module_map) )
+ if ( bi->mods[idx].type != BOOTMOD_UNKNOWN )
{
printk(XENLOG_WARNING "Microcode: Chosen module %d already
used\n", idx);
return -ENODEV;
diff --git a/xen/arch/x86/include/asm/bootinfo.h
b/xen/arch/x86/include/asm/bootinfo.h
index fc74e3b224e7..37dfcc14fa7d 100644
--- a/xen/arch/x86/include/asm/bootinfo.h
+++ b/xen/arch/x86/include/asm/bootinfo.h
@@ -43,10 +43,50 @@ struct boot_info {
size_t memmap_length;
unsigned int nr_modules;
- unsigned long *module_map; /* Temporary */
struct boot_module mods[MAX_NR_BOOTMODS + 1];
};
+/*
+ * next_boot_module_index:
+ * Finds the next boot module of type t, starting at array index start.
+ *
+ * Returns:
+ * Success - index in boot_module array
+ * Failure - a value greater than MAX_NR_BOOTMODS
+ */
+static inline unsigned int __init next_boot_module_index(
+ const struct boot_info *bi, enum bootmod_type t, unsigned int start)
+{
+ unsigned int i;
+
+ if ( t == BOOTMOD_XEN )
+ return bi->nr_modules;
+
+ for ( i = start; i < bi->nr_modules; i++ )
+ {
+ if ( bi->mods[i].type == t )
+ return i;
+ }
+
+ return MAX_NR_BOOTMODS + 1;
+}
+
+/*
+ * first_boot_module_index:
+ * Finds the first boot module of type t.
+ *
+ * Returns:
+ * Success - index in boot_module array
+ * Failure - a value greater than MAX_NR_BOOTMODS
+ */
+#define first_boot_module_index(bi, t) \
+ next_boot_module_index(bi, t, 0)
+
+#define for_each_boot_module_by_type(i, b, t) \
+ for ( i = first_boot_module_index(b, t); \
+ i <= (b)->nr_modules; \
+ i = next_boot_module_index(b, t, i + 1) )
(i) = first_...
Ack.
diff --git a/xen/xsm/xsm_policy.c b/xen/xsm/xsm_policy.c
index 4c195411d05b..12c9de5a1fbf 100644
--- a/xen/xsm/xsm_policy.c
+++ b/xen/xsm/xsm_policy.c
@@ -33,22 +33,18 @@
int __init xsm_multiboot_policy_init(
struct boot_info *bi, void **policy_buffer, size_t *policy_size)
{
- int i;
+ unsigned int i;
int rc = 0;
u32 *_policy_start;
unsigned long _policy_len;
- /*
- * Try all modules and see whichever could be the binary policy.
- * Adjust module_map for the module that is the binary policy.
- */
- for ( i = bi->nr_modules - 1; i >= 1; i-- )
+ /* Try all unknown modules and see whichever could be the binary policy. */
+ for_each_boot_module_by_type(i, bi, BOOTMOD_UNKNOWN)
{
- if ( !test_bit(i, bi->module_map) )
- continue;
+ struct boot_module *bm = &bi->mods[i];
- _policy_start = bootstrap_map(bi->mods[i].mod);
- _policy_len = bi->mods[i].mod->mod_end;
+ _policy_start = bootstrap_map(bm->mod);
+ _policy_len = bm->mod->mod_end;
Minor, but you ought to switch to bootstrap_map_bm() here straight away,
which reduces the churn in patch 9.
Ack.
v/r,
dps
