On Mon, Dec 20, 2021 at 05:27:00PM -0300, Martin Fernandez wrote:

On 12/20/21, Greg KH [off-list ref] wrote:

quoted

On Thu, Dec 16, 2021 at 04:22:20PM -0300, Martin Fernandez wrote:

quoted

diff --git a/arch/x86/include/asm/e820/types.h

b/arch/x86/include/asm/e820/types.h
index 314f75d886d0..7b510dffd3b9 100644

--- a/arch/x86/include/asm/e820/types.h
+++ b/arch/x86/include/asm/e820/types.h

@@ -56,6 +56,7 @@ struct e820_entry {
 	u64			addr;
 	u64			size;
 	enum e820_type		type;
+	u8			crypto_capable;

Why isn't this a bool?

It was a bool initially, but Andy Shevchenko told me that it couldn't
be that way because boolean may not be part of firmware ABIs.

Where does this structure hit an "ABI"?  Looks internal to me.  If not,
then something just broke anyway.

quoted

quoted

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index bc0657f0deed..001d64686938 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c

@@ -163,7 +163,7 @@ int e820__get_entry_type(u64 start, u64 end)
 /*
  * Add a memory region to the kernel E820 map.
  */
-static void __init __e820__range_add(struct e820_table *table, u64 start,

u64 size, enum e820_type type)
+static void __init __e820__range_add(struct e820_table *table, u64 start,
u64 size, enum e820_type type, u8 crypto_capable)

Horrid api change, but it's internal to this file so oh well :(

Hint, don't add flags to functions like this, it forces you to have to
always remember what those flags are when you read the code.  Right now
you stuck "0" and "1" in the function call, which is not instructional
at all.

Heck, why not make it an enum to have it be self-describing?  Like the
type is here.  that would make it much better and easier to understand
and maintain over time.

Yes, an enum will absolutely improve things. I'll do that.

quoted

quoted

@@ -327,6 +330,7 @@ int __init e820__update_table(struct e820_table

*table)
 	unsigned long long last_addr;
 	u32 new_nr_entries, overlap_entries;
 	u32 i, chg_idx, chg_nr;
+	u8 current_crypto, last_crypto;

 	/* If there's only one memory region, don't bother: */
 	if (table->nr_entries < 2)

@@ -367,6 +371,7 @@ int __init e820__update_table(struct e820_table

*table)
 	new_nr_entries = 0;	 /* Index for creating new map entries */
 	last_type = 0;		 /* Start with undefined memory type */
 	last_addr = 0;		 /* Start with 0 as last starting address */
+	last_crypto = 0;

 	/* Loop through change-points, determining effect on the new map: */
 	for (chg_idx = 0; chg_idx < chg_nr; chg_idx++) {

@@ -388,13 +393,17 @@ int __init e820__update_table(struct e820_table

*table)
 		 * 1=usable, 2,3,4,4+=unusable)
 		 */
 		current_type = 0;
+		current_crypto = 1;
 		for (i = 0; i < overlap_entries; i++) {
+			current_crypto = current_crypto && overlap_list[i]->crypto_capable;

Is it a u8 or not?  You treat it as a boolean a lot :(

quoted

 			if (overlap_list[i]->type > current_type)
 				current_type = overlap_list[i]->type;
 		}

 		/* Continue building up new map based on this information: */
-		if (current_type != last_type || e820_nomerge(current_type)) {
+		if (current_type != last_type ||
+		    current_crypto != last_crypto ||
+		    e820_nomerge(current_type)) {

Why check it before calling e820_nomerge()?  Is that required?

I don't see how the order of the checks matter, am I missing something?

It might prevent this function from being called now when it previously
was.  Is that ok?

thanks,

greg k-h