From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id BF45BC433F5 for ; Tue, 8 Feb 2022 21:04:53 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 271436B0074; Tue, 8 Feb 2022 16:04:53 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 220026B0075; Tue, 8 Feb 2022 16:04:53 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 099B56B0078; Tue, 8 Feb 2022 16:04:53 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0254.hostedemail.com [216.40.44.254]) by kanga.kvack.org (Postfix) with ESMTP id EE1FB6B0074 for ; Tue, 8 Feb 2022 16:04:52 -0500 (EST) Received: from smtpin17.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id A9DD7181D3026 for ; Tue, 8 Feb 2022 21:04:52 +0000 (UTC) X-FDA: 79120842024.17.8F4249D Received: from mail-pj1-f54.google.com (mail-pj1-f54.google.com [209.85.216.54]) by imf23.hostedemail.com (Postfix) with ESMTP id 42F65140005 for ; Tue, 8 Feb 2022 21:04:52 +0000 (UTC) Received: by mail-pj1-f54.google.com with SMTP id c8-20020a17090a674800b001b91184b732so668362pjm.5 for ; Tue, 08 Feb 2022 13:04:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=eclypsium.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=afzJpCERAmk3yFBzidPNdDx8rS5idfCgJ4X6kw/EPaM=; b=eEUrW4OF40G7F+WuRB6qBYML3NnN8jyRLUuDZQ36/XwjKQrosY7EncGqdoZ56goO11 xq22cEWlVcV/nfTZEgwOl4cltBYzFo5qBDhHsxZIXIfbl/ev31PA89aerwPLPjquVt41 bY0xhOiHnke5u4XhoMvSuQatKLPM9ope1sJvXd2uTxspzkQiDP8VZ4s1wrfo6tk5IrOU A6r85p6t3+zI13d14ceOdn91Vz20yvAhdXYKsJQULVcHMNuj5RsQfYF+P5tX1QutEYJE 6qCcVAOXut7WXPhiIObuboj20J0WcrVEUKTbTC8X33BIepw3pq1fHPDyOd0Q5SaN7CQI k9Nw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=afzJpCERAmk3yFBzidPNdDx8rS5idfCgJ4X6kw/EPaM=; b=sCDQhbyr5nyUdHxBfsAC2D+BzUGONhyccCTjPki1dfX7DTGfZ4OUIOB4Bza02KRDV1 JyYq5MQW7p1aOk9tmePcRKpKvk1yeLlCEjpNpom8i5vbNk/Gbks786SAnMJ/2AF6tFZx WkhhluEwaZzjjKrgatZWc/uDYgsWPrFgH6hpEnAKmTo/smwN+xoCaxBFhPUqANmNtSVB mkcpP5DWUtw7Vxxh8T3XwFdSxcs20EQmAb96OezlZb7Uc8Kj611+YuXmwX7oaQyrG3z/ Sn4rgqAuQtJUc2MXdHqHQx/N3IocINX4uqYFvBR5NiHIR92xumK5HXB9ZOZkRkQr5uNi dAuQ== X-Gm-Message-State: AOAM533VKt18EAC3Og3+/Vo3CqPEIMjm+cmZFKbaULty9wa8ru5xabev fMKGig5TM4xWI/ri21qg6okTBjLpH8jCemmzYroI+w== X-Google-Smtp-Source: ABdhPJzhPgIonUJWKRLCbon/hP7emWLkYkN+Dx+b87pFcCzMzBNuU1ZCEMqTrJ+4QIVkDxz93NlatPENKIsESM0QRxo= X-Received: by 2002:a17:902:ab53:: with SMTP id ij19mr6335813plb.45.1644354290468; Tue, 08 Feb 2022 13:04:50 -0800 (PST) MIME-Version: 1.0 References: <20220203164328.203629-1-martin.fernandez@eclypsium.com> <20220203164328.203629-4-martin.fernandez@eclypsium.com> In-Reply-To: <20220203164328.203629-4-martin.fernandez@eclypsium.com> From: Daniel Gutson Date: Tue, 8 Feb 2022 18:04:39 -0300 Message-ID: Subject: Re: [PATCH v6 3/6] x86/e820: Refactor range_update and range_remove To: Martin Fernandez Cc: linux-kernel , linux-efi@vger.kernel.org, platform-driver-x86@vger.kernel.org, linux-mm@kvack.org, Thomas Gleixner , Ingo Molnar , Borislav Petkov , Dave Hansen , x86@kernel.org, "H. Peter Anvin" , ardb@kernel.org, dvhart@infradead.org, andy@infradead.org, Greg Kroah-Hartman , rafael@kernel.org, rppt@kernel.org, akpm@linux-foundation.org, Richard Hughes , Alex Bazhaniuk , Alison Schofield , Kees Cook Content-Type: text/plain; charset="UTF-8" Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=eclypsium.com header.s=google header.b=eEUrW4OF; dmarc=pass (policy=quarantine) header.from=eclypsium.com; spf=pass (imf23.hostedemail.com: domain of daniel.gutson@eclypsium.com designates 209.85.216.54 as permitted sender) smtp.mailfrom=daniel.gutson@eclypsium.com X-Rspam-User: X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: 42F65140005 X-Stat-Signature: z3d4t5ob7hfq8d5fp6je9gp7fhkwqqis X-HE-Tag: 1644354292-853363 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu, Feb 3, 2022 at 1:44 PM 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. > > 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 > --- > 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 >