| Message ID | 20220203164328.203629-4-martin.fernandez@eclypsium.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86: Show in sysfs if a memory node is able to do encryption | expand |
On Thu, Feb 03, 2022 at 01:43:25PM -0300, Martin Fernandez wrote: > __e820__range_update and e820__range_remove had a very similar > implementation with a few lines different from each other, the lines > that actually perform the modification over the e820_table. The > similiraties were found in the checks for the different cases on how > each entry intersects with the given range (if it does at all). These > checks were very presice and error prone so it was not a good idea to > have them in both places. Yay removing copy/paste code! :) > > I propose a refactor of those functions, given that I need to create a > similar one for this patchset. The diff here is pretty hard (for me) to review; I'll need more time to check it. What might make review easier (at least for me), is to incrementally change these routines. i.e. separate patches to: - add the new infrastructure - replace e820__range_remove - replace __e820__range_update If that's not actually useful, no worries. I'll just stare at it a bit more. :) > > Add a function to modify a E820 table in a given range. This > modification is done backed up by two helper structs: > e820_entry_updater and e820_*_data. > > The first one, e820_entry_updater, carries 3 callbacks which function > as the actions to take on the table. > > The other one, e820_*_data carries information needed by the > callbacks, for example in the case of range_update it will carry the > type that we are targeting. Something I think would be really amazing here is if you could add KUnit tests here to exercise the corner cases and validate the changes. It should be pretty easy to add. Here's a quick example for the boilerplate and testing a bit of __e820__range_add(): #ifdef CONFIG_E820_KUNIT_TEST #include <kunit/test.h> static void __init test_e820_range_add(struct kunit *context) { struct e820_table table; u32 full; full = ARRAY_SIZE(table.entries); /* Add last entry. */ table->nr_entries = full - 1; __e820__range_add(&table, 0, 15, 0); KUNIT_EXPECT_EQ(table->nr_entries, full) /* Skip new entry when full. */ __e820__range_add(&table, 0, 15, 0); KUNIT_EXPECT_EQ(table->nr_entries, full) } static void __init test_e820_update(struct kunit *context) { ... } static struct kunit_case __refdata e820_test_cases[] = { KUNIT_CASE(test_e820_range_add), KUNIT_CASE(test_e820_update), ... {} }; static struct kunit_suite e820_test_suite = { .name = "e820", .test_cases = e820_test_cases, }; kunit_test_suites(&e820_test_suite); #endif
On Mon, Feb 07, 2022 at 01:45:40PM -0800, Kees Cook wrote: > On Thu, Feb 03, 2022 at 01:43:25PM -0300, Martin Fernandez wrote: > > __e820__range_update and e820__range_remove had a very similar > > implementation with a few lines different from each other, the lines > > that actually perform the modification over the e820_table. The > > similiraties were found in the checks for the different cases on how > > each entry intersects with the given range (if it does at all). These > > checks were very presice and error prone so it was not a good idea to > > have them in both places. > > Yay removing copy/paste code! :) Removing copy/paste is nice but diffstat of arch/x86/kernel/e820.c | 383 ++++++++++++++++++++++++++++++----------- 1 file changed, 283 insertions(+), 100 deletions(-) does not look nice even accounting for lots of comments :( I didn't look closely, but diffstat clues that the refactoring making things much more complex. > > > > I propose a refactor of those functions, given that I need to create a > > similar one for this patchset. > > The diff here is pretty hard (for me) to review; I'll need more time > to check it. What might make review easier (at least for me), is to > incrementally change these routines. i.e. separate patches to: > > - add the new infrastructure > - replace e820__range_remove > - replace __e820__range_update > > If that's not actually useful, no worries. I'll just stare at it a bit > more. :)
On 2/8/22, Mike Rapoport <rppt@kernel.org> wrote: > On Mon, Feb 07, 2022 at 01:45:40PM -0800, Kees Cook wrote: >> On Thu, Feb 03, 2022 at 01:43:25PM -0300, Martin Fernandez wrote: >> > __e820__range_update and e820__range_remove had a very similar >> > implementation with a few lines different from each other, the lines >> > that actually perform the modification over the e820_table. The >> > similiraties were found in the checks for the different cases on how >> > each entry intersects with the given range (if it does at all). These >> > checks were very presice and error prone so it was not a good idea to >> > have them in both places. >> >> Yay removing copy/paste code! :) > > Removing copy/paste is nice but diffstat of > > arch/x86/kernel/e820.c | 383 ++++++++++++++++++++++++++++++----------- > 1 file changed, 283 insertions(+), 100 deletions(-) > > does not look nice even accounting for lots of comments :( > > I didn't look closely, but diffstat clues that the refactoring making > things much more complex. > Yes, that diffstat surprised me as well. I have to mention that 110 of those lines are kerneldocs and blank lines, which is quite a lot. Also you have to take into account that I expanded most of the function definitions for better formatting, which also took some space. And as I was able to focus the "hard" part of the problem into a single function, testing can be done easily as Kees suggested and I'm planning to do so in the next patch.
On Thu, Feb 3, 2022 at 1:44 PM Martin Fernandez <martin.fernandez@eclypsium.com> wrote: > > __e820__range_update and e820__range_remove had a very similar > implementation with a few lines different from each other, the lines > that actually perform the modification over the e820_table. The > similiraties were found in the checks for the different cases on how > each entry intersects with the given range (if it does at all). These > checks were very presice and error prone so it was not a good idea to > have them in both places. > > I propose a refactor of those functions, given that I need to create a > similar one for this patchset. > > Add a function to modify a E820 table in a given range. This > modification is done backed up by two helper structs: > e820_entry_updater and e820_*_data. > > The first one, e820_entry_updater, carries 3 callbacks which function > as the actions to take on the table. > > The other one, e820_*_data carries information needed by the > callbacks, for example in the case of range_update it will carry the > type that we are targeting. > > Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com> > --- > arch/x86/kernel/e820.c | 383 ++++++++++++++++++++++++++++++----------- > 1 file changed, 283 insertions(+), 100 deletions(-) > > diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c > index bc0657f0deed..89b78c6b345b 100644 > --- a/arch/x86/kernel/e820.c > +++ b/arch/x86/kernel/e820.c > @@ -459,144 +459,327 @@ static int __init append_e820_table(struct boot_e820_entry *entries, u32 nr_entr > return __append_e820_table(entries, nr_entries); > } > > +/** > + * e820_entry_updater - Helper type for __e820__handle_range_update(). > + * @should_update: Return true if @entry needs to be updated, false > + * otherwise. > + * @update: Apply desired actions to an @entry that is inside the > + * range and satisfies @should_update. > + * @new: Create new entry in the table with information gathered from > + * @original and @data. > + * > + * Each function corresponds to an action that > + * __e820__handle_range_update() does. Callbacks need to cast @data back > + * to the corresponding type. > + */ > +struct e820_entry_updater { > + bool (*should_update)(const struct e820_entry *entry, const void *data); > + void (*update)(struct e820_entry *entry, const void *data); > + void (*new)(struct e820_table *table, u64 new_start, u64 new_size, > + const struct e820_entry *original, const void *data); > +}; > + > +/** > + * e820_remove_data - Helper type for e820__range_remove(). > + * @old_type: old_type parameter of e820__range_remove(). > + * @check_type: check_type parameter of e820__range_remove(). > + * > + * This is intended to be used as the @data argument for the > + * e820_entry_updater callbacks. > + */ > +struct e820_remover_data { > + enum e820_type old_type; > + bool check_type; > +}; > + > +/** > + * e820_type_updater_data - Helper type for __e820__range_update(). > + * @old_type: old_type parameter of __e820__range_update(). > + * @new_type: new_type parameter of __e820__range_update(). > + * > + * This is intended to be used as the @data argument for the > + * e820_entry_updater callbacks. > + */ > +struct e820_type_updater_data { > + enum e820_type old_type; > + enum e820_type new_type; > +}; > + > +/** > + * __e820__handle_intersected_range_update() - Helper function for > + * __e820__handle_range_update(). > + * @table: Target e820_table. > + * @start: Start of the range. > + * @size: Size of the range. > + * @entry: Current entry that __e820__handle_range_update() was > + * looking into. > + * @updater: updater parameter of __e820__handle_range_update(). > + * @data: data parameter of __e820__handle_range_update(). > + * > + * Helper for __e820__handle_range_update to handle the case where > + * neither the entry completely covers the range nor the range > + * completely covers the entry. > + * > + * Return: The updated size. > + */ > static u64 __init > -__e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_type old_type, enum e820_type new_type) > +__e820__handle_intersected_range_update(struct e820_table *table, > + u64 start, > + u64 size, > + struct e820_entry *entry, > + const struct e820_entry_updater *updater, > + const void *data) > { > u64 end; > - unsigned int i; > - u64 real_updated_size = 0; > - > - BUG_ON(old_type == new_type); > + u64 entry_end = entry->addr + entry->size; > + u64 inner_start; > + u64 inner_end; > + u64 updated_size = 0; > > if (size > (ULLONG_MAX - start)) > size = ULLONG_MAX - start; > > end = start + size; > - printk(KERN_DEBUG "e820: update [mem %#010Lx-%#010Lx] ", start, end - 1); > - e820_print_type(old_type); > - pr_cont(" ==> "); > - e820_print_type(new_type); > - pr_cont("\n"); > - > - for (i = 0; i < table->nr_entries; i++) { > - struct e820_entry *entry = &table->entries[i]; > - u64 final_start, final_end; > - u64 entry_end; > + inner_start = max(start, entry->addr); > + inner_end = min(end, entry_end); > + > + /* Range and entry do intersect and... */ > + if (inner_start < inner_end) { > + /* Entry is on the left */ > + if (entry->addr < inner_start) { > + /* Resize current entry */ > + entry->size = inner_start - entry->addr; > + /* Entry is on the right */ > + } else { > + /* Resize and move current section */ > + entry->addr = inner_end; > + entry->size = entry_end - inner_end; > + } > + /* Create new entry with intersected region */ > + updater->new(table, inner_start, inner_end - inner_start, entry, data); > > - if (entry->type != old_type) > - continue; > + updated_size += inner_end - inner_start; > + } /* Else: [start, end) doesn't cover entry */ > > - entry_end = entry->addr + entry->size; > + return updated_size; > +} > > - /* Completely covered by new range? */ > - if (entry->addr >= start && entry_end <= end) { > - entry->type = new_type; > - real_updated_size += entry->size; > - continue; > - } > +/** __e820__handle_range_update(): Helper function to update a address > + * range in a e820_table > + * @table: e820_table that we want to modify. > + * @start: Start of the range. > + * @size: Size of the range. > + * @updater: Callbacks to modify the table. > + * @data: Information to modify the table. > + * > + * Update the table @table in [@start, @start + @size) doing the > + * actions given in @updater. > + * > + * Return: The updated size. > + */ > +static u64 __init > +__e820__handle_range_update(struct e820_table *table, > + u64 start, > + u64 size, > + const struct e820_entry_updater *updater, > + const void *data) > +{ > + u64 updated_size = 0; > + u64 end; > + unsigned int i; > > - /* New range is completely covered? */ > - if (entry->addr < start && entry_end > end) { > - __e820__range_add(table, start, size, new_type); > - __e820__range_add(table, end, entry_end - end, entry->type); > - entry->size = start - entry->addr; > - real_updated_size += size; > - continue; > - } > + if (size > (ULLONG_MAX - start)) > + size = ULLONG_MAX - start; > > - /* Partially covered: */ > - final_start = max(start, entry->addr); > - final_end = min(end, entry_end); > - if (final_start >= final_end) > - continue; > + end = start + size; > > - __e820__range_add(table, final_start, final_end - final_start, new_type); > + for (i = 0; i < table->nr_entries; i++) { > + struct e820_entry *entry = &table->entries[i]; > + u64 entry_end = entry->addr + entry->size; > + > + if (updater->should_update(data, entry)) { > + /* Range completely covers entry */ > + if (entry->addr >= start && entry_end <= end) { > + updater->update(entry, data); > + updated_size += entry->size; > + /* Entry completely covers range */ > + } else if (start > entry->addr && end < entry_end) { > + /* Resize current entry */ > + entry->size = start - entry->addr; > + > + /* Create new entry with intersection region */ > + updater->new(table, start, size, entry, data); > + > + /* > + * Create a new entry for the leftover > + * of the current entry > + */ > + __e820__range_add(table, end, entry_end - end, > + entry->type); > + > + updated_size += size; > + } else { > + updated_size = > + __e820__handle_intersected_range_update(table, start, size, > + entry, updater, data); > + } > + } > + } > > - real_updated_size += final_end - final_start; > + return updated_size; > +} > > - /* > - * Left range could be head or tail, so need to update > - * its size first: > - */ > - entry->size -= final_end - final_start; > - if (entry->addr < final_start) > - continue; > +static bool __init type_updater__should_update(const struct e820_entry *entry, > + const void *data) > +{ > + struct e820_type_updater_data *type_updater_data = > + (struct e820_type_updater_data *)data; Please preserve const correctness. You are removing the const qualifier. > > - entry->addr = final_end; > - } > - return real_updated_size; > + return entry->type == type_updater_data->old_type; > } > > -u64 __init e820__range_update(u64 start, u64 size, enum e820_type old_type, enum e820_type new_type) > +static void __init type_updater__update(struct e820_entry *entry, > + const void *data) > { > - return __e820__range_update(e820_table, start, size, old_type, new_type); > + struct e820_type_updater_data *type_updater_data = > + (struct e820_type_updater_data *)data; > + > + entry->type = type_updater_data->new_type; > } > > -static u64 __init e820__range_update_kexec(u64 start, u64 size, enum e820_type old_type, enum e820_type new_type) > +static void __init type_updater__new(struct e820_table *table, u64 new_start, > + u64 new_size, > + const struct e820_entry *original, > + const void *data) > { > - return __e820__range_update(e820_table_kexec, start, size, old_type, new_type); > + struct e820_type_updater_data *type_updater_data = > + (struct e820_type_updater_data *)data; > + > + __e820__range_add(table, new_start, new_size, > + type_updater_data->new_type); > } > > -/* Remove a range of memory from the E820 table: */ > -u64 __init e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool check_type) > +static u64 __init __e820__range_update(struct e820_table *table, u64 start, > + u64 size, enum e820_type old_type, > + enum e820_type new_type) > { > - int i; > - u64 end; > - u64 real_removed_size = 0; > + struct e820_entry_updater updater = { > + .should_update = type_updater__should_update, > + .update = type_updater__update, > + .new = type_updater__new > + }; > > - if (size > (ULLONG_MAX - start)) > - size = ULLONG_MAX - start; > + struct e820_type_updater_data data = { > + .old_type = old_type, > + .new_type = new_type > + }; > > - end = start + size; > - printk(KERN_DEBUG "e820: remove [mem %#010Lx-%#010Lx] ", start, end - 1); > - if (check_type) > - e820_print_type(old_type); > + BUG_ON(old_type == new_type); > + > + printk(KERN_DEBUG "e820: update [mem %#018Lx-%#018Lx] ", start, > + start + size - 1); > + e820_print_type(old_type); > + pr_cont(" ==> "); > + e820_print_type(new_type); > pr_cont("\n"); > > - for (i = 0; i < e820_table->nr_entries; i++) { > - struct e820_entry *entry = &e820_table->entries[i]; > - u64 final_start, final_end; > - u64 entry_end; > + return __e820__handle_range_update(table, start, size, &updater, &data); > +} > > - if (check_type && entry->type != old_type) > - continue; > +static bool __init remover__should_update(const struct e820_entry *entry, > + const void *data) > +{ > + struct e820_remover_data *remover_data = > + (struct e820_remover_data *)data; > > - entry_end = entry->addr + entry->size; > + return !remover_data->check_type || > + entry->type == remover_data->old_type; > +} > > - /* Completely covered? */ > - if (entry->addr >= start && entry_end <= end) { > - real_removed_size += entry->size; > - memset(entry, 0, sizeof(*entry)); > - continue; > - } > +static void __init remover__update(struct e820_entry *entry, const void *data) > +{ > + memset(entry, 0, sizeof(*entry)); > +} > > - /* Is the new range completely covered? */ > - if (entry->addr < start && entry_end > end) { > - e820__range_add(end, entry_end - end, entry->type); > - entry->size = start - entry->addr; > - real_removed_size += size; > - continue; > - } > +static void __init remover__new(struct e820_table *table, u64 new_start, > + u64 new_size, const struct e820_entry *original, > + const void *data) > +{ > +} > > - /* Partially covered: */ > - final_start = max(start, entry->addr); > - final_end = min(end, entry_end); > - if (final_start >= final_end) > - continue; > +/** > + * e820__range_remove() - Remove an address range from e820_table. > + * @start: Start of the address range. > + * @size: Size of the address range. > + * @old_type: Type of the entries that we want to remove. > + * @check_type: Bool to decide if ignore @old_type or not. > + * > + * Remove [@start, @start + @size) from e820_table. If @check_type is > + * true remove only entries with type @old_type. > + * > + * Return: The size removed. > + */ > +u64 __init e820__range_remove(u64 start, u64 size, enum e820_type old_type, > + bool check_type) > +{ > + struct e820_entry_updater updater = { > + .should_update = remover__should_update, > + .update = remover__update, > + .new = remover__new > + }; > + > + struct e820_remover_data data = { > + .check_type = check_type, > + .old_type = old_type > + }; > + > + printk(KERN_DEBUG "e820: remove [mem %#018Lx-%#018Lx] ", start, > + start + size - 1); > + if (check_type) > + e820_print_type(old_type); > + pr_cont("\n"); > > - real_removed_size += final_end - final_start; > + return __e820__handle_range_update(e820_table, start, size, &updater, > + &data); > +} > > - /* > - * Left range could be head or tail, so need to update > - * the size first: > - */ > - entry->size -= final_end - final_start; > - if (entry->addr < final_start) > - continue; > +/** > + * e820__range_update() - Update the type of a given address range in > + * e820_table. > + * @start: Start of the range. > + * @size: Size of the range. > + * @old_type: Type that we want to change. > + * @new_type: New type to replace @old_type. > + * > + * Update type of addresses in [@start, @start + @size) from @old_type > + * to @new_type in e820_table. > + * > + * Return: The size updated. > + */ > +u64 __init e820__range_update(u64 start, u64 size, enum e820_type old_type, > + enum e820_type new_type) > +{ > + return __e820__range_update(e820_table, start, size, old_type, new_type); > +} > > - entry->addr = final_end; > - } > - return real_removed_size; > +/** > + * e820__range_update_kexec() - Update the type of a given address > + * range in e820_table_kexec. > + * @start: Start of the range. > + * @size: Size of the range. > + * @old_type: Type that we want to change. > + * @new_type: New type to replace @old_type. > + * > + * Update type of addresses in [@start, @start + @size) from @old_type > + * to @new_type in e820_table_kexec. > + * > + * Return: The size updated. > + */ > +static u64 __init e820__range_update_kexec(u64 start, u64 size, > + enum e820_type old_type, > + enum e820_type new_type) > +{ > + return __e820__range_update(e820_table_kexec, start, size, old_type, new_type); > } > > void __init e820__update_table_print(void) > -- > 2.30.2 >
On 2/7/22, Kees Cook <keescook@chromium.org> wrote: > On Thu, Feb 03, 2022 at 01:43:25PM -0300, Martin Fernandez wrote: >> __e820__range_update and e820__range_remove had a very similar >> I propose a refactor of those functions, given that I need to create a >> similar one for this patchset. > > The diff here is pretty hard (for me) to review; I'll need more time > to check it. What might make review easier (at least for me), is to > incrementally change these routines. i.e. separate patches to: > > - add the new infrastructure > - replace e820__range_remove > - replace __e820__range_update > > If that's not actually useful, no worries. I'll just stare at it a bit > more. :) > Yep, that's a good idea. I'll keep that in mind for the next patch. >> >> Add a function to modify a E820 table in a given range. This >> modification is done backed up by two helper structs: >> e820_entry_updater and e820_*_data. >> >> The first one, e820_entry_updater, carries 3 callbacks which function >> as the actions to take on the table. >> >> The other one, e820_*_data carries information needed by the >> callbacks, for example in the case of range_update it will carry the >> type that we are targeting. > > Something I think would be really amazing here is if you could add KUnit > tests here to exercise the corner cases and validate the changes. It > should be pretty easy to add. Here's a quick example for the boilerplate > and testing a bit of __e820__range_add(): > > #ifdef CONFIG_E820_KUNIT_TEST > #include <kunit/test.h> > > static void __init test_e820_range_add(struct kunit *context) > { > struct e820_table table; > u32 full; > > full = ARRAY_SIZE(table.entries); > /* Add last entry. */ > table->nr_entries = full - 1; > __e820__range_add(&table, 0, 15, 0); > KUNIT_EXPECT_EQ(table->nr_entries, full) > /* Skip new entry when full. */ > __e820__range_add(&table, 0, 15, 0); > KUNIT_EXPECT_EQ(table->nr_entries, full) > } > > static void __init test_e820_update(struct kunit *context) > { > ... > } > > static struct kunit_case __refdata e820_test_cases[] = { > KUNIT_CASE(test_e820_range_add), > KUNIT_CASE(test_e820_update), > ... > {} > }; > > static struct kunit_suite e820_test_suite = { > .name = "e820", > .test_cases = e820_test_cases, > }; > > kunit_test_suites(&e820_test_suite); > #endif > Oh that's awesome! I'll definitely take a look into KUnit and integrate it to this patch. Thanks for the code snippet!
Hi Martin, On Tue, Feb 08, 2022 at 06:01:21PM -0300, Martin Fernandez wrote: > On 2/8/22, Mike Rapoport <rppt@kernel.org> wrote: > > On Mon, Feb 07, 2022 at 01:45:40PM -0800, Kees Cook wrote: > >> On Thu, Feb 03, 2022 at 01:43:25PM -0300, Martin Fernandez wrote: > >> > __e820__range_update and e820__range_remove had a very similar > >> > implementation with a few lines different from each other, the lines > >> > that actually perform the modification over the e820_table. The > >> > similiraties were found in the checks for the different cases on how > >> > each entry intersects with the given range (if it does at all). These > >> > checks were very presice and error prone so it was not a good idea to > >> > have them in both places. > >> > >> Yay removing copy/paste code! :) > > > > Removing copy/paste is nice but diffstat of > > > > arch/x86/kernel/e820.c | 383 ++++++++++++++++++++++++++++++----------- > > 1 file changed, 283 insertions(+), 100 deletions(-) > > > > does not look nice even accounting for lots of comments :( > > > > I didn't look closely, but diffstat clues that the refactoring making > > things much more complex. > > > > Yes, that diffstat surprised me as well. > > I have to mention that 110 of those lines are kerneldocs and blank > lines, which is quite a lot. Also you have to take into account that I > expanded most of the function definitions for better formatting, which > also took some space. At last I had time to look more closely and I think that using a set of callbacks is over-complicated. I think this can be done way simpler, e.g like this (untested) draft: https://git.kernel.org/rppt/h/x86/e820-update-range > And as I was able to focus the "hard" part of the problem into a > single function, testing can be done easily as Kees suggested and I'm > planning to do so in the next patch.
On 2/15/22, Mike Rapoport <rppt@kernel.org> wrote: > Hi Martin, > > On Tue, Feb 08, 2022 at 06:01:21PM -0300, Martin Fernandez wrote: >> On 2/8/22, Mike Rapoport <rppt@kernel.org> wrote: >> > On Mon, Feb 07, 2022 at 01:45:40PM -0800, Kees Cook wrote: >> >> On Thu, Feb 03, 2022 at 01:43:25PM -0300, Martin Fernandez wrote: >> >> > __e820__range_update and e820__range_remove had a very similar >> >> > implementation with a few lines different from each other, the lines >> >> > that actually perform the modification over the e820_table. The >> >> > similiraties were found in the checks for the different cases on how >> >> > each entry intersects with the given range (if it does at all). >> >> > These >> >> > checks were very presice and error prone so it was not a good idea >> >> > to >> >> > have them in both places. >> >> >> >> Yay removing copy/paste code! :) >> > >> > Removing copy/paste is nice but diffstat of >> > >> > arch/x86/kernel/e820.c | 383 ++++++++++++++++++++++++++++++----------- >> > 1 file changed, 283 insertions(+), 100 deletions(-) >> > >> > does not look nice even accounting for lots of comments :( >> > >> > I didn't look closely, but diffstat clues that the refactoring making >> > things much more complex. >> > >> >> Yes, that diffstat surprised me as well. >> >> I have to mention that 110 of those lines are kerneldocs and blank >> lines, which is quite a lot. Also you have to take into account that I >> expanded most of the function definitions for better formatting, which >> also took some space. > > At last I had time to look more closely and I think that using a set of > callbacks is over-complicated. > > I think this can be done way simpler, e.g like this (untested) draft: > > https://git.kernel.org/rppt/h/x86/e820-update-range > Thanks for taking the time to reviewing it. Yeah, I did something like that in a previous version. Altough I wasn't really happy with that. https://lore.kernel.org/linux-efi/20220113213027.457282-4-martin.fernandez@eclypsium.com/ I think that with the struct with the function arguments looks more clear than what I did, but you have to take into account that I need to create yet another function similar to those and another parameter to the struct, and with that I think that __e820__range_update will look scary. I'll give it a try anyway!
On 2/7/22, Kees Cook <keescook@chromium.org> wrote: > Something I think would be really amazing here is if you could add KUnit > tests here to exercise the corner cases and validate the changes. It > should be pretty easy to add. Here's a quick example for the boilerplate > and testing a bit of __e820__range_add(): > > #ifdef CONFIG_E820_KUNIT_TEST > #include <kunit/test.h> > > static void __init test_e820_range_add(struct kunit *context) > { > struct e820_table table; > u32 full; > > full = ARRAY_SIZE(table.entries); > /* Add last entry. */ > table->nr_entries = full - 1; > __e820__range_add(&table, 0, 15, 0); > KUNIT_EXPECT_EQ(table->nr_entries, full) > /* Skip new entry when full. */ > __e820__range_add(&table, 0, 15, 0); > KUNIT_EXPECT_EQ(table->nr_entries, full) > } > > static void __init test_e820_update(struct kunit *context) > { > ... > } > > static struct kunit_case __refdata e820_test_cases[] = { > KUNIT_CASE(test_e820_range_add), > KUNIT_CASE(test_e820_update), > ... > {} > }; > > static struct kunit_suite e820_test_suite = { > .name = "e820", > .test_cases = e820_test_cases, > }; > > kunit_test_suites(&e820_test_suite); > #endif I almost got it. Although when added the tests I have a warning when compiling, because KUnit doens't want to deal with __init things: WARNING: modpost: vmlinux.o(.data+0x26800): Section mismatch in reference from the variable __UNIQUE_ID_array286 to the variable .init.data:e820_test_suite The variable __UNIQUE_ID_array286 references the variable __initdata e820_test_suite If the reference is valid then annotate the variable with __init* or __refdata (see linux/init.h) or name the variable: *_template, *_timer, *_sht, *_ops, *_probe, *_probe_one, *_console I need to test __init functions. I couldn't find any other similar cases in existant code. Is there a nice way to solve this? I'm adding the file that contains the tests just in case.. #include <kunit/test.h> #include <asm/e820/api.h> #include <asm/setup.h> #define KUNIT_EXPECT_E820_ENTRY_EQ(test, entry, _addr, _size, _type, \ _crypto_capable) \ do { \ KUNIT_EXPECT_EQ((test), (entry).addr, (_addr)); \ KUNIT_EXPECT_EQ((test), (entry).size, (_size)); \ KUNIT_EXPECT_EQ((test), (entry).type, (_type)); \ KUNIT_EXPECT_EQ((test), (entry).crypto_capable, \ (_crypto_capable)); \ } while (0) struct e820_table test_table __initdata; static void __init test_e820_range_add(struct kunit *test) { u32 full; full = ARRAY_SIZE(test_table.entries); /* Add last entry. */ test_table.nr_entries = full - 1; __e820__range_add(&test_table, 0, 15, 0, 0); KUNIT_EXPECT_EQ(test, test_table.nr_entries, full); /* Skip new entry when full. */ __e820__range_add(&test_table, 0, 15, 0, 0); KUNIT_EXPECT_EQ(test, test_table.nr_entries, full); } static void __init test_e820_range_update(struct kunit *test) { u64 entry_size = 15; u64 updated_size = 0; /* Initialize table */ test_table.nr_entries = 0; __e820__range_add(&test_table, 0, entry_size, E820_TYPE_RAM, E820_NOT_CRYPTO_CAPABLE); __e820__range_add(&test_table, entry_size, entry_size, E820_TYPE_RAM, E820_NOT_CRYPTO_CAPABLE); __e820__range_add(&test_table, entry_size * 2, entry_size, E820_TYPE_ACPI, E820_NOT_CRYPTO_CAPABLE); updated_size = __e820__range_update(&test_table, 0, entry_size * 2, E820_TYPE_RAM, E820_TYPE_RESERVED); /* The first 2 regions were updated */ KUNIT_EXPECT_EQ(test, updated_size, entry_size * 2); KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[0], 0, entry_size, E820_TYPE_RESERVED, E820_NOT_CRYPTO_CAPABLE); KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[1], entry_size, entry_size, E820_TYPE_RESERVED, E820_NOT_CRYPTO_CAPABLE); KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[2], entry_size * 2, entry_size, E820_TYPE_ACPI, E820_NOT_CRYPTO_CAPABLE); updated_size = __e820__range_update(&test_table, 0, entry_size * 3, E820_TYPE_RESERVED, E820_TYPE_RAM); /* Only the first 2 regions were updated */ KUNIT_EXPECT_EQ(test, updated_size, entry_size * 2); KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[0], 0, entry_size, E820_TYPE_RAM, E820_NOT_CRYPTO_CAPABLE); KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[1], entry_size, entry_size, E820_TYPE_RAM, E820_NOT_CRYPTO_CAPABLE); KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[2], entry_size * 2, entry_size, E820_TYPE_ACPI, E820_NOT_CRYPTO_CAPABLE); } static void __init test_e820_range_remove(struct kunit *test) { u64 entry_size = 15; u64 removed_size = 0; struct e820_entry_updater updater = { .should_update = remover__should_update, .update = remover__update, .new = remover__new }; struct e820_remover_data data = { .check_type = true, .old_type = E820_TYPE_RAM }; /* Initialize table */ test_table.nr_entries = 0; __e820__range_add(&test_table, 0, entry_size, E820_TYPE_RAM, E820_NOT_CRYPTO_CAPABLE); __e820__range_add(&test_table, entry_size, entry_size, E820_TYPE_RAM, E820_NOT_CRYPTO_CAPABLE); __e820__range_add(&test_table, entry_size * 2, entry_size, E820_TYPE_ACPI, E820_NOT_CRYPTO_CAPABLE); /* * Need to use __e820__handle_range_update because * e820__range_remove doesn't ask for the table */ removed_size = __e820__handle_range_update(&test_table, 0, entry_size * 2, &updater, &data); /* The first two regions were removed */ KUNIT_EXPECT_EQ(test, removed_size, entry_size * 2); KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[0], 0, 0, 0, 0); KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[1], 0, 0, 0, 0); removed_size = __e820__handle_range_update(&test_table, 0, entry_size * 3, &updater, &data); /* Nothing was removed */ KUNIT_EXPECT_EQ(test, removed_size, 0); KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[0], 0, 0, 0, 0); KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[1], 0, 0, 0, 0); KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[2], entry_size * 2, entry_size, E820_TYPE_ACPI, E820_NOT_CRYPTO_CAPABLE); } static void __init test_e820_range_crypto_update(struct kunit *test) { u64 entry_size = 15; u64 updated_size = 0; /* Initialize table */ test_table.nr_entries = 0; __e820__range_add(&test_table, 0, entry_size, E820_TYPE_RAM, E820_CRYPTO_CAPABLE); __e820__range_add(&test_table, entry_size, entry_size, E820_TYPE_RAM, E820_NOT_CRYPTO_CAPABLE); __e820__range_add(&test_table, entry_size * 2, entry_size, E820_TYPE_RAM, E820_CRYPTO_CAPABLE); updated_size = __e820__range_update_crypto(&test_table, 0, entry_size * 3, E820_CRYPTO_CAPABLE); /* Only the region in the middle was updated */ KUNIT_EXPECT_EQ(test, updated_size, entry_size); KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[0], 0, entry_size, E820_TYPE_RAM, E820_CRYPTO_CAPABLE); KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[1], entry_size, entry_size, E820_TYPE_RAM, E820_CRYPTO_CAPABLE); KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[2], entry_size * 2, entry_size, E820_TYPE_RAM, E820_CRYPTO_CAPABLE); } static void __init test_e820_handle_range_update_intersection(struct kunit *test) { struct e820_entry_updater updater = { .should_update = type_updater__should_update, .update = type_updater__update, .new = type_updater__new }; struct e820_type_updater_data data = { .old_type = E820_TYPE_RAM, .new_type = E820_TYPE_RESERVED }; u64 entry_size = 15; u64 updated_size = 0; /* Initialize table */ test_table.nr_entries = 0; __e820__range_add(&test_table, 0, entry_size, E820_TYPE_RAM, E820_NOT_CRYPTO_CAPABLE); updated_size = __e820__handle_range_update(&test_table, 0, entry_size - 2, &updater, &data); KUNIT_EXPECT_EQ(test, updated_size, entry_size - 2); /* There is a new entry */ KUNIT_EXPECT_EQ(test, test_table.nr_entries, 2); /* The original entry now is moved */ KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[0], entry_size - 2, 2, E820_TYPE_RAM, E820_NOT_CRYPTO_CAPABLE); /* The new entry has the correct values */ KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[1], 0, 13, E820_TYPE_RESERVED, E820_NOT_CRYPTO_CAPABLE); } static void __init test_e820_handle_range_update_inside(struct kunit *test) { struct e820_entry_updater updater = { .should_update = type_updater__should_update, .update = type_updater__update, .new = type_updater__new }; struct e820_type_updater_data data = { .old_type = E820_TYPE_RAM, .new_type = E820_TYPE_RESERVED }; u64 entry_size = 15; u64 updated_size = 0; /* Initialize table */ test_table.nr_entries = 0; __e820__range_add(&test_table, 0, entry_size, E820_TYPE_RAM, E820_NOT_CRYPTO_CAPABLE); updated_size = __e820__handle_range_update(&test_table, 5, entry_size - 10, &updater, &data); KUNIT_EXPECT_EQ(test, updated_size, entry_size - 10); /* There are two new entrie */ KUNIT_EXPECT_EQ(test, test_table.nr_entries, 3); /* The original entry now shrinked */ KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[0], 0, 5, E820_TYPE_RAM, E820_NOT_CRYPTO_CAPABLE); /* The new entries have the correct values */ KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[1], 5, entry_size - 10, E820_TYPE_RESERVED, E820_NOT_CRYPTO_CAPABLE); /* Left over of the original region */ KUNIT_EXPECT_E820_ENTRY_EQ(test, test_table.entries[2], entry_size - 5, 5, E820_TYPE_RAM, E820_NOT_CRYPTO_CAPABLE); } static struct kunit_case e820_test_cases[] __initdata = { KUNIT_CASE(test_e820_range_add), KUNIT_CASE(test_e820_range_update), KUNIT_CASE(test_e820_range_remove), KUNIT_CASE(test_e820_range_crypto_update), KUNIT_CASE(test_e820_handle_range_update_intersection), KUNIT_CASE(test_e820_handle_range_update_inside), {} }; static struct kunit_suite e820_test_suite __initdata = { .name = "e820", .test_cases = e820_test_cases, }; kunit_test_suite(e820_test_suite);
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c index bc0657f0deed..89b78c6b345b 100644 --- a/arch/x86/kernel/e820.c +++ b/arch/x86/kernel/e820.c @@ -459,144 +459,327 @@ static int __init append_e820_table(struct boot_e820_entry *entries, u32 nr_entr return __append_e820_table(entries, nr_entries); } +/** + * e820_entry_updater - Helper type for __e820__handle_range_update(). + * @should_update: Return true if @entry needs to be updated, false + * otherwise. + * @update: Apply desired actions to an @entry that is inside the + * range and satisfies @should_update. + * @new: Create new entry in the table with information gathered from + * @original and @data. + * + * Each function corresponds to an action that + * __e820__handle_range_update() does. Callbacks need to cast @data back + * to the corresponding type. + */ +struct e820_entry_updater { + bool (*should_update)(const struct e820_entry *entry, const void *data); + void (*update)(struct e820_entry *entry, const void *data); + void (*new)(struct e820_table *table, u64 new_start, u64 new_size, + const struct e820_entry *original, const void *data); +}; + +/** + * e820_remove_data - Helper type for e820__range_remove(). + * @old_type: old_type parameter of e820__range_remove(). + * @check_type: check_type parameter of e820__range_remove(). + * + * This is intended to be used as the @data argument for the + * e820_entry_updater callbacks. + */ +struct e820_remover_data { + enum e820_type old_type; + bool check_type; +}; + +/** + * e820_type_updater_data - Helper type for __e820__range_update(). + * @old_type: old_type parameter of __e820__range_update(). + * @new_type: new_type parameter of __e820__range_update(). + * + * This is intended to be used as the @data argument for the + * e820_entry_updater callbacks. + */ +struct e820_type_updater_data { + enum e820_type old_type; + enum e820_type new_type; +}; + +/** + * __e820__handle_intersected_range_update() - Helper function for + * __e820__handle_range_update(). + * @table: Target e820_table. + * @start: Start of the range. + * @size: Size of the range. + * @entry: Current entry that __e820__handle_range_update() was + * looking into. + * @updater: updater parameter of __e820__handle_range_update(). + * @data: data parameter of __e820__handle_range_update(). + * + * Helper for __e820__handle_range_update to handle the case where + * neither the entry completely covers the range nor the range + * completely covers the entry. + * + * Return: The updated size. + */ static u64 __init -__e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_type old_type, enum e820_type new_type) +__e820__handle_intersected_range_update(struct e820_table *table, + u64 start, + u64 size, + struct e820_entry *entry, + const struct e820_entry_updater *updater, + const void *data) { u64 end; - unsigned int i; - u64 real_updated_size = 0; - - BUG_ON(old_type == new_type); + u64 entry_end = entry->addr + entry->size; + u64 inner_start; + u64 inner_end; + u64 updated_size = 0; if (size > (ULLONG_MAX - start)) size = ULLONG_MAX - start; end = start + size; - printk(KERN_DEBUG "e820: update [mem %#010Lx-%#010Lx] ", start, end - 1); - e820_print_type(old_type); - pr_cont(" ==> "); - e820_print_type(new_type); - pr_cont("\n"); - - for (i = 0; i < table->nr_entries; i++) { - struct e820_entry *entry = &table->entries[i]; - u64 final_start, final_end; - u64 entry_end; + inner_start = max(start, entry->addr); + inner_end = min(end, entry_end); + + /* Range and entry do intersect and... */ + if (inner_start < inner_end) { + /* Entry is on the left */ + if (entry->addr < inner_start) { + /* Resize current entry */ + entry->size = inner_start - entry->addr; + /* Entry is on the right */ + } else { + /* Resize and move current section */ + entry->addr = inner_end; + entry->size = entry_end - inner_end; + } + /* Create new entry with intersected region */ + updater->new(table, inner_start, inner_end - inner_start, entry, data); - if (entry->type != old_type) - continue; + updated_size += inner_end - inner_start; + } /* Else: [start, end) doesn't cover entry */ - entry_end = entry->addr + entry->size; + return updated_size; +} - /* Completely covered by new range? */ - if (entry->addr >= start && entry_end <= end) { - entry->type = new_type; - real_updated_size += entry->size; - continue; - } +/** __e820__handle_range_update(): Helper function to update a address + * range in a e820_table + * @table: e820_table that we want to modify. + * @start: Start of the range. + * @size: Size of the range. + * @updater: Callbacks to modify the table. + * @data: Information to modify the table. + * + * Update the table @table in [@start, @start + @size) doing the + * actions given in @updater. + * + * Return: The updated size. + */ +static u64 __init +__e820__handle_range_update(struct e820_table *table, + u64 start, + u64 size, + const struct e820_entry_updater *updater, + const void *data) +{ + u64 updated_size = 0; + u64 end; + unsigned int i; - /* New range is completely covered? */ - if (entry->addr < start && entry_end > end) { - __e820__range_add(table, start, size, new_type); - __e820__range_add(table, end, entry_end - end, entry->type); - entry->size = start - entry->addr; - real_updated_size += size; - continue; - } + if (size > (ULLONG_MAX - start)) + size = ULLONG_MAX - start; - /* Partially covered: */ - final_start = max(start, entry->addr); - final_end = min(end, entry_end); - if (final_start >= final_end) - continue; + end = start + size; - __e820__range_add(table, final_start, final_end - final_start, new_type); + for (i = 0; i < table->nr_entries; i++) { + struct e820_entry *entry = &table->entries[i]; + u64 entry_end = entry->addr + entry->size; + + if (updater->should_update(data, entry)) { + /* Range completely covers entry */ + if (entry->addr >= start && entry_end <= end) { + updater->update(entry, data); + updated_size += entry->size; + /* Entry completely covers range */ + } else if (start > entry->addr && end < entry_end) { + /* Resize current entry */ + entry->size = start - entry->addr; + + /* Create new entry with intersection region */ + updater->new(table, start, size, entry, data); + + /* + * Create a new entry for the leftover + * of the current entry + */ + __e820__range_add(table, end, entry_end - end, + entry->type); + + updated_size += size; + } else { + updated_size = + __e820__handle_intersected_range_update(table, start, size, + entry, updater, data); + } + } + } - real_updated_size += final_end - final_start; + return updated_size; +} - /* - * Left range could be head or tail, so need to update - * its size first: - */ - entry->size -= final_end - final_start; - if (entry->addr < final_start) - continue; +static bool __init type_updater__should_update(const struct e820_entry *entry, + const void *data) +{ + struct e820_type_updater_data *type_updater_data = + (struct e820_type_updater_data *)data; - entry->addr = final_end; - } - return real_updated_size; + return entry->type == type_updater_data->old_type; } -u64 __init e820__range_update(u64 start, u64 size, enum e820_type old_type, enum e820_type new_type) +static void __init type_updater__update(struct e820_entry *entry, + const void *data) { - return __e820__range_update(e820_table, start, size, old_type, new_type); + struct e820_type_updater_data *type_updater_data = + (struct e820_type_updater_data *)data; + + entry->type = type_updater_data->new_type; } -static u64 __init e820__range_update_kexec(u64 start, u64 size, enum e820_type old_type, enum e820_type new_type) +static void __init type_updater__new(struct e820_table *table, u64 new_start, + u64 new_size, + const struct e820_entry *original, + const void *data) { - return __e820__range_update(e820_table_kexec, start, size, old_type, new_type); + struct e820_type_updater_data *type_updater_data = + (struct e820_type_updater_data *)data; + + __e820__range_add(table, new_start, new_size, + type_updater_data->new_type); } -/* Remove a range of memory from the E820 table: */ -u64 __init e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool check_type) +static u64 __init __e820__range_update(struct e820_table *table, u64 start, + u64 size, enum e820_type old_type, + enum e820_type new_type) { - int i; - u64 end; - u64 real_removed_size = 0; + struct e820_entry_updater updater = { + .should_update = type_updater__should_update, + .update = type_updater__update, + .new = type_updater__new + }; - if (size > (ULLONG_MAX - start)) - size = ULLONG_MAX - start; + struct e820_type_updater_data data = { + .old_type = old_type, + .new_type = new_type + }; - end = start + size; - printk(KERN_DEBUG "e820: remove [mem %#010Lx-%#010Lx] ", start, end - 1); - if (check_type) - e820_print_type(old_type); + BUG_ON(old_type == new_type); + + printk(KERN_DEBUG "e820: update [mem %#018Lx-%#018Lx] ", start, + start + size - 1); + e820_print_type(old_type); + pr_cont(" ==> "); + e820_print_type(new_type); pr_cont("\n"); - for (i = 0; i < e820_table->nr_entries; i++) { - struct e820_entry *entry = &e820_table->entries[i]; - u64 final_start, final_end; - u64 entry_end; + return __e820__handle_range_update(table, start, size, &updater, &data); +} - if (check_type && entry->type != old_type) - continue; +static bool __init remover__should_update(const struct e820_entry *entry, + const void *data) +{ + struct e820_remover_data *remover_data = + (struct e820_remover_data *)data; - entry_end = entry->addr + entry->size; + return !remover_data->check_type || + entry->type == remover_data->old_type; +} - /* Completely covered? */ - if (entry->addr >= start && entry_end <= end) { - real_removed_size += entry->size; - memset(entry, 0, sizeof(*entry)); - continue; - } +static void __init remover__update(struct e820_entry *entry, const void *data) +{ + memset(entry, 0, sizeof(*entry)); +} - /* Is the new range completely covered? */ - if (entry->addr < start && entry_end > end) { - e820__range_add(end, entry_end - end, entry->type); - entry->size = start - entry->addr; - real_removed_size += size; - continue; - } +static void __init remover__new(struct e820_table *table, u64 new_start, + u64 new_size, const struct e820_entry *original, + const void *data) +{ +} - /* Partially covered: */ - final_start = max(start, entry->addr); - final_end = min(end, entry_end); - if (final_start >= final_end) - continue; +/** + * e820__range_remove() - Remove an address range from e820_table. + * @start: Start of the address range. + * @size: Size of the address range. + * @old_type: Type of the entries that we want to remove. + * @check_type: Bool to decide if ignore @old_type or not. + * + * Remove [@start, @start + @size) from e820_table. If @check_type is + * true remove only entries with type @old_type. + * + * Return: The size removed. + */ +u64 __init e820__range_remove(u64 start, u64 size, enum e820_type old_type, + bool check_type) +{ + struct e820_entry_updater updater = { + .should_update = remover__should_update, + .update = remover__update, + .new = remover__new + }; + + struct e820_remover_data data = { + .check_type = check_type, + .old_type = old_type + }; + + printk(KERN_DEBUG "e820: remove [mem %#018Lx-%#018Lx] ", start, + start + size - 1); + if (check_type) + e820_print_type(old_type); + pr_cont("\n"); - real_removed_size += final_end - final_start; + return __e820__handle_range_update(e820_table, start, size, &updater, + &data); +} - /* - * Left range could be head or tail, so need to update - * the size first: - */ - entry->size -= final_end - final_start; - if (entry->addr < final_start) - continue; +/** + * e820__range_update() - Update the type of a given address range in + * e820_table. + * @start: Start of the range. + * @size: Size of the range. + * @old_type: Type that we want to change. + * @new_type: New type to replace @old_type. + * + * Update type of addresses in [@start, @start + @size) from @old_type + * to @new_type in e820_table. + * + * Return: The size updated. + */ +u64 __init e820__range_update(u64 start, u64 size, enum e820_type old_type, + enum e820_type new_type) +{ + return __e820__range_update(e820_table, start, size, old_type, new_type); +} - entry->addr = final_end; - } - return real_removed_size; +/** + * e820__range_update_kexec() - Update the type of a given address + * range in e820_table_kexec. + * @start: Start of the range. + * @size: Size of the range. + * @old_type: Type that we want to change. + * @new_type: New type to replace @old_type. + * + * Update type of addresses in [@start, @start + @size) from @old_type + * to @new_type in e820_table_kexec. + * + * Return: The size updated. + */ +static u64 __init e820__range_update_kexec(u64 start, u64 size, + enum e820_type old_type, + enum e820_type new_type) +{ + return __e820__range_update(e820_table_kexec, start, size, old_type, new_type); } void __init e820__update_table_print(void)
__e820__range_update and e820__range_remove had a very similar implementation with a few lines different from each other, the lines that actually perform the modification over the e820_table. The similiraties were found in the checks for the different cases on how each entry intersects with the given range (if it does at all). These checks were very presice and error prone so it was not a good idea to have them in both places. I propose a refactor of those functions, given that I need to create a similar one for this patchset. Add a function to modify a E820 table in a given range. This modification is done backed up by two helper structs: e820_entry_updater and e820_*_data. The first one, e820_entry_updater, carries 3 callbacks which function as the actions to take on the table. The other one, e820_*_data carries information needed by the callbacks, for example in the case of range_update it will carry the type that we are targeting. Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com> --- arch/x86/kernel/e820.c | 383 ++++++++++++++++++++++++++++++----------- 1 file changed, 283 insertions(+), 100 deletions(-)