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 <dpsmith@apertussolutions.com>
---
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
---
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)
{
+ 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) )
+
#endif /* X86_BOOTINFO_H */
/*
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index fac08b6242e9..92dbdfe12a3d 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1034,7 +1034,7 @@ void asmlinkage __init noreturn __start_xen(void)
struct cpu_info *info = get_cpu_info(), *bsp_info;
unsigned int initrdidx, num_parked = 0;
struct boot_info *bi;
- unsigned long nr_pages, raw_max_page, modules_headroom, module_map[1];
+ unsigned long nr_pages, raw_max_page, modules_headroom;
int i, j, e820_warn = 0, bytes = 0;
unsigned long eb_start, eb_end;
bool acpi_boot_table_init_done = false, relocated = false;
@@ -1097,8 +1097,6 @@ void asmlinkage __init noreturn __start_xen(void)
ASSERT(multiboot_ptr < MB(1) || xen_phys_start);
}
- bi->module_map = module_map; /* Temporary */
-
/* Parse the command-line options. */
if ( (kextra = strstr(bi->cmdline, " -- ")) != NULL )
{
@@ -1216,8 +1214,7 @@ void asmlinkage __init noreturn __start_xen(void)
bi->nr_modules);
}
- bitmap_fill(module_map, bi->nr_modules);
- __clear_bit(0, module_map); /* Dom0 kernel is always first */
+ /* Dom0 kernel is always first */
bi->mods[0].type = BOOTMOD_KERNEL;
if ( pvh_boot )
@@ -2098,13 +2095,22 @@ void asmlinkage __init noreturn __start_xen(void)
cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ",
cpu_has_nx ? "" : "not ");
- initrdidx = find_first_bit(module_map, bi->nr_modules);
- if ( initrdidx < bi->nr_modules )
+ /*
+ * At this point all capabilities that consume boot modules should have
+ * claimed their boot modules. Find the first unclaimed boot module and
+ * claim it as the initrd ramdisk. Do a second search to see if there are
+ * any remaining unclaimed boot modules, and report them as unusued initrd
+ * candidates.
+ */
+ initrdidx = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
+ if ( initrdidx < MAX_NR_BOOTMODS )
+ {
bi->mods[initrdidx].type = BOOTMOD_RAMDISK;
- if ( bitmap_weight(module_map, bi->nr_modules) > 1 )
- printk(XENLOG_WARNING
- "Multiple initrd candidates, picking module #%u\n",
- initrdidx);
+ if ( first_boot_module_index(bi, BOOTMOD_UNKNOWN) < MAX_NR_BOOTMODS )
+ printk(XENLOG_WARNING
+ "Multiple initrd candidates, picking module #%u\n",
+ initrdidx);
+ }
/*
* We're going to setup domain0 using the module(s) that we stashed safely
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;
if ( (xsm_magic_t)(*_policy_start) == XSM_MAGIC )
{
@@ -58,8 +54,7 @@ int __init xsm_multiboot_policy_init(
printk("Policy len %#lx, start at %p.\n",
_policy_len,_policy_start);
- __clear_bit(i, bi->module_map);
- bi->mods[i].type = BOOTMOD_XSM_POLICY;
+ bm->type = BOOTMOD_XSM_POLICY;
break;
}
--
2.30.2On 02.11.2024 18:25, Daniel P. Smith wrote:
> --- 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)
> {
> + struct boot_module *bm = &bi->mods[idx];
pointer-to-const? You really want to get used to applying const to pointed-to
types whenever possible. IOW ...
> --- 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];
... same here (and likely elsewhere in the series).
Jan
On 11/7/24 05:05, Jan Beulich wrote:
> On 02.11.2024 18:25, Daniel P. Smith wrote:
>> --- 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)
>> {
>> + struct boot_module *bm = &bi->mods[idx];
>
> pointer-to-const? You really want to get used to applying const to pointed-to
> types whenever possible. IOW ...
Yes, I have been trying to be more diligent to add them when possible.
To the point that I have had to unwind some due to writes that must be
done in later function calls they get passed. Obviously this is not one
of those case and can fix this one.
>> --- 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];
>
> ... same here (and likely elsewhere in the series).
Nope, you can't const this one as that will cause this is at the tail
end of the loop to fail:
+ bm->type = BOOTMOD_XSM_POLICY;
v/r,
dps
On 07.11.2024 16:08, Daniel P. Smith wrote:
> On 11/7/24 05:05, Jan Beulich wrote:
>> On 02.11.2024 18:25, Daniel P. Smith wrote:
>>> --- 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];
>>
>> ... same here (and likely elsewhere in the series).
>
> Nope, you can't const this one as that will cause this is at the tail
> end of the loop to fail:
>
> + bm->type = BOOTMOD_XSM_POLICY;
Oh, I had managed to not spot that.
Jan
On 11/7/24 10:15, Jan Beulich wrote:
> On 07.11.2024 16:08, Daniel P. Smith wrote:
>> On 11/7/24 05:05, Jan Beulich wrote:
>>> On 02.11.2024 18:25, Daniel P. Smith wrote:
>>>> --- 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];
>>>
>>> ... same here (and likely elsewhere in the series).
>>
>> Nope, you can't const this one as that will cause this is at the tail
>> end of the loop to fail:
>>
>> + bm->type = BOOTMOD_XSM_POLICY;
>
> Oh, I had managed to not spot that.
No worries. As I said, I have been trying to develop the habit to check
if write usage will be needed, const-ing and then finding myself often
greeted with gcc error about write to const. Wish gcc could warn about
pointers that have no writes in scope to help catch viable candidates.
v/r,
dps
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 <dpsmith@apertussolutions.com>
> ---
> 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.
> ---
> 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.
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?
> {
> + 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_...
> 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.
~Andrew
On 06.11.2024 15: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 <dpsmith@apertussolutions.com>
>> ---
>> 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.
>
>> ---
>> 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.
Just to clarify - no, the latter form you suggest is not okay to use.
Daniel's form is, as is the first one you suggest. The choice is between
"like a for() loop" (your 1st form) and "just another macro invocation"
(Daniel's form).
Jan
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 <dpsmith@apertussolutions.com>
>> ---
>> 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
On 06/11/2024 2:50 pm, Daniel P. Smith wrote: > On 11/6/24 09:34, Andrew Cooper wrote: >> 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. This is simple enough, and I'm happy to fix this all up on commit. Save it going around the loop yet again. ~Andrew
On 11/6/24 09:55, Andrew Cooper wrote: > On 06/11/2024 2:50 pm, Daniel P. Smith wrote: >> On 11/6/24 09:34, Andrew Cooper wrote: >>> 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. > > This is simple enough, and I'm happy to fix this all up on commit. Save > it going around the loop yet again. No objection on my part, as I was just going to make the changes as you suggested. v/r, dps
© 2016 - 2024 Red Hat, Inc.