linux-efi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Martin Fernandez <martin.fernandez@eclypsium.com>
To: linux-kernel@vger.kernel.org, linux-efi@vger.kernel.org,
	platform-driver-x86@vger.kernel.org, linux-mm@kvack.org
Cc: tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
	dave.hansen@linux.intel.com, x86@kernel.org, hpa@zytor.com,
	ardb@kernel.org, dvhart@infradead.org, andy@infradead.org,
	gregkh@linuxfoundation.org, rafael@kernel.org, rppt@kernel.org,
	akpm@linux-foundation.org, daniel.gutson@eclypsium.com,
	hughsient@gmail.com, alex.bazhaniuk@eclypsium.com,
	alison.schofield@intel.com, keescook@chromium.org,
	Martin Fernandez <martin.fernandez@eclypsium.com>
Subject: [PATCH v8 3/8] x86/e820: Add infrastructure to refactor e820__range_{update,remove}
Date: Fri, 29 Apr 2022 17:17:12 -0300	[thread overview]
Message-ID: <20220429201717.1946178-4-martin.fernandez@eclypsium.com> (raw)
In-Reply-To: <20220429201717.1946178-1-martin.fernandez@eclypsium.com>

__e820__range_update and e820__range_remove had a very similar flow in
its 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.

Since I need to add a third one, similar to this two, in this and the
following patches I'll propose a refactor of these functions.

In this patch I introduce:

- A new type e820_entry_updater that will carry three callbacks, those
callbacks will decide WHEN to perform actions over the e820_table and
WHAY actions are going to be performed.

  Note that there is a void pointer "data". This pointer will carry
  useful information for the callbacks, like the type that we want to
  update in e820__range_update or if we want to check the type in
  e820__range_remove. Check it out in the next patches where I do the
  rework of __e820__range_update and e820__range_remove.

- A new function __e820__handle_range_update that has the flow of the
original two functions to refactor. Together with e820_entry_updater
will perform the desired update on the input table.

On version 6 of this patch some people pointed out that this solution
was over-complicated. Mike Rapoport suggested a another solution [1].

I took a look at that, and although it is indeed simpler it's more
confusing at the same time. I think is manageable to have a single
function to update or remove sections of the table (what Mike did),
but when I added the functionality to also update the crypto_capable
it became really hard to manage.

I think that the approach presented in this patch it's complex but is
easier to read, to extend and to test.

[1] https://git.kernel.org/rppt/h/x86/e820-update-range

Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com>

--------------------------------------------------

Changes from v7 to v8:

(Some) Callbacks of e820_entry_updater can now be NULL to avoid
defining empty functions

Remove kerneldocs in favor of plain comments just to explain what the
functions do.
---
 arch/x86/kernel/e820.c | 127 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 127 insertions(+)

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index f267205f2d5a..e0fa3ab755a5 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -459,6 +459,133 @@ static int __init append_e820_table(struct boot_e820_entry *entries, u32 nr_entr
 	return __append_e820_table(entries, nr_entries);
 }
 
+/**
+ * struct 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. Can be set to NULL to avoid empty functions.
+ * @new: Create new entry in the table with information gathered from
+ * @original and @data. Can be set to NULL to avoid empty functions.
+ *
+ * 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);
+};
+
+/*
+ * 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.
+ */
+static u64 __init
+__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;
+	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;
+	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;
+		}
+		if (updater->new != NULL)
+			/* Create new entry with intersected region */
+			updater->new(table, inner_start, inner_end - inner_start, entry, data);
+
+		updated_size += inner_end - inner_start;
+	} /* Else: [start, end) doesn't cover entry */
+
+	return updated_size;
+}
+
+/*
+ * Update the table @table in [@start, @start + @size) doing the
+ * actions given in @updater.
+ */
+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;
+
+	if (size > (ULLONG_MAX - start))
+		size = ULLONG_MAX - start;
+
+	end = start + size;
+
+	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(entry, data)) {
+			/* Range completely covers entry */
+			if (entry->addr >= start && entry_end <= end) {
+				updated_size += entry->size;
+				if (updater->update != NULL)
+					updater->update(entry, data);
+			/* Entry completely covers range */
+			} else if (start > entry->addr && end < entry_end) {
+				/* Resize current entry */
+				entry->size = start - entry->addr;
+
+				if (updater->new != NULL)
+					/* 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);
+			}
+		}
+	}
+
+	return 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)
 {
-- 
2.30.2


  parent reply	other threads:[~2022-04-29 20:18 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-29 20:17 [PATCH v8 0/8] x86: Show in sysfs if a memory node is able to do encryption Martin Fernandez
2022-04-29 20:17 ` [PATCH v8 1/8] mm/memblock: Tag memblocks with crypto capabilities Martin Fernandez
2022-04-29 20:17 ` [PATCH v8 2/8] mm/mmzone: Tag pg_data_t " Martin Fernandez
2022-04-29 20:17 ` Martin Fernandez [this message]
2022-04-29 20:17 ` [PATCH v8 4/8] x86/e820: Refactor __e820__range_update Martin Fernandez
2022-04-29 20:17 ` [PATCH v8 5/8] x86/e820: Refactor e820__range_remove Martin Fernandez
2022-04-29 20:17 ` [PATCH v8 6/8] x86/e820: Tag e820_entry with crypto capabilities Martin Fernandez
2022-04-29 20:17 ` [PATCH v8 7/8] x86/efi: Mark e820_entries as crypto capable from EFI memmap Martin Fernandez
2022-04-29 20:17 ` [PATCH v8 8/8] drivers/node: Show in sysfs node's crypto capabilities Martin Fernandez
2022-05-04 16:38 ` [PATCH v8 0/8] x86: Show in sysfs if a memory node is able to do encryption Borislav Petkov
2022-05-04 17:18   ` Martin Fernandez
2022-05-06 12:44     ` Borislav Petkov
2022-05-06 14:18       ` Limonciello, Mario
2022-05-06 15:32       ` Dave Hansen
2022-05-06 16:00         ` Dan Williams
2022-05-06 17:55           ` Boris Petkov
2022-05-06 18:14             ` Dave Hansen
2022-05-06 18:25               ` Boris Petkov
2022-05-06 18:43                 ` Dave Hansen
2022-05-06 19:02                   ` Boris Petkov
2022-05-09 18:47                     ` Dave Hansen
2022-05-09 22:17                       ` Borislav Petkov
2022-05-09 22:56                         ` Dave Hansen
2022-05-16  8:39                     ` Richard Hughes
2022-05-18  7:52                       ` Borislav Petkov
2022-05-18 18:28                         ` Dan Williams
2022-05-18 20:23                           ` Borislav Petkov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220429201717.1946178-4-martin.fernandez@eclypsium.com \
    --to=martin.fernandez@eclypsium.com \
    --cc=akpm@linux-foundation.org \
    --cc=alex.bazhaniuk@eclypsium.com \
    --cc=alison.schofield@intel.com \
    --cc=andy@infradead.org \
    --cc=ardb@kernel.org \
    --cc=bp@alien8.de \
    --cc=daniel.gutson@eclypsium.com \
    --cc=dave.hansen@linux.intel.com \
    --cc=dvhart@infradead.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=hpa@zytor.com \
    --cc=hughsient@gmail.com \
    --cc=keescook@chromium.org \
    --cc=linux-efi@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mingo@redhat.com \
    --cc=platform-driver-x86@vger.kernel.org \
    --cc=rafael@kernel.org \
    --cc=rppt@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).