[PATCH v4 0/5] x86: Show in sysfs if a memory node is able to do encryption

[PATCH v4 0/5] x86: Show in sysfs if a memory node is able to do encryption

[Martin Fernandez]

Show for each node if every memory descriptor in that node has the
EFI_MEMORY_CPU_CRYPTO attribute.

fwupd project plans to use it as part of a check to see if the users
have properly configured memory hardware encryption
capabilities. fwupd's people have seen cases where it seems like there
is memory encryption because all the hardware is capable of doing it,
but on a closer look there is not, either because of system firmware
or because some component requires updating to enable the feature.

It's planned to make it part of a specification that can be passed to
people purchasing hardware

These checks will run at every boot. The specification is called Host
Security ID: https://fwupd.github.io/libfwupdplugin/hsi.html.

We choosed to do it a per-node basis because although an ABI that
shows that the whole system memory is capable of encryption would be
useful for the fwupd usecase, doing it in a per-node basis gives also
the capability to the user to target allocations from applications to
NUMA nodes which have encryption capabilities.


Changes since v3:
Update date in Doc/ABI file.

More information about the fwupd usecase and the rationale behind
doing it in a per-NUMA-node.


Changes since v2:

e820__range_mark_crypto -> e820__range_mark_crypto_capable.

In e820__range_remove: Create a region with crypto capabilities
instead of creating one without it and then mark it.


Changes since v1:

Modify __e820__range_update to update the crypto capabilities of a
range; now this function will change the crypto capability of a range
if it's called with the same old_type and new_type. Rework
efi_mark_e820_regions_as_crypto_capable based on this.

Update do_add_efi_memmap to mark the regions as it creates them.

Change the type of crypto_capable in e820_entry from bool to u8.

Fix e820__update_table changes.

Remove memblock_add_crypto_capable. Now you have to add the region and
mark it then.

Better place for crypto_capable in pglist_data.

Martin Fernandez (5):
  mm/memblock: Tag memblocks with crypto capabilities
  mm/mmzone: Tag pg_data_t with crypto capabilities
  x86/e820: Tag e820_entry with crypto capabilities
  x86/efi: Tag e820_entries as crypto capable from EFI memmap
  drivers/node: Show in sysfs node's crypto capabilities

 Documentation/ABI/testing/sysfs-devices-node | 10 ++++
 arch/x86/include/asm/e820/api.h              |  1 +
 arch/x86/include/asm/e820/types.h            |  1 +
 arch/x86/kernel/e820.c                       | 59 ++++++++++++++++----
 arch/x86/platform/efi/efi.c                  | 26 +++++++++
 drivers/base/node.c                          | 10 ++++
 include/linux/memblock.h                     |  5 ++
 include/linux/mmzone.h                       |  3 +
 mm/memblock.c                                | 49 ++++++++++++++++
 mm/page_alloc.c                              |  1 +
 10 files changed, 153 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-devices-node

-- 
2.30.2

[PATCH v4 1/5] mm/memblock: Tag memblocks with crypto capabilities

[Martin Fernandez]

Add the capability to mark regions of the memory memory_type able of
hardware memory encryption.

Also add the capability to query if all regions of a memory node are
able to do hardware memory encryption to call it when initializing the
nodes.

Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com>
---
 include/linux/memblock.h |  5 ++++
 mm/memblock.c            | 49 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/include/linux/memblock.h b/include/linux/memblock.h
index 8adcf1fa8096..ec808ad93693 100644
--- a/include/linux/memblock.h
+++ b/include/linux/memblock.h
@@ -41,6 +41,7 @@ extern unsigned long long max_possible_pfn;
  * via a driver, and never indicated in the firmware-provided memory map as
  * system RAM. This corresponds to IORESOURCE_SYSRAM_DRIVER_MANAGED in the
  * kernel resource tree.
+ * @MEMBLOCK_CRYPTO_CAPABLE: capable of hardware encryption
  */
 enum memblock_flags {
 	MEMBLOCK_NONE		= 0x0,	/* No special request */
@@ -48,6 +49,7 @@ enum memblock_flags {
 	MEMBLOCK_MIRROR		= 0x2,	/* mirrored region */
 	MEMBLOCK_NOMAP		= 0x4,	/* don't add to kernel direct mapping */
 	MEMBLOCK_DRIVER_MANAGED = 0x8,	/* always detected via a driver */
+	MEMBLOCK_CRYPTO_CAPABLE = 0x10,  /* capable of hardware encryption */
 };
 
 /**
@@ -121,6 +123,9 @@ int memblock_physmem_add(phys_addr_t base, phys_addr_t size);
 void memblock_trim_memory(phys_addr_t align);
 bool memblock_overlaps_region(struct memblock_type *type,
 			      phys_addr_t base, phys_addr_t size);
+bool memblock_node_is_crypto_capable(int nid);
+int memblock_mark_crypto_capable(phys_addr_t base, phys_addr_t size);
+int memblock_clear_crypto_capable(phys_addr_t base, phys_addr_t size);
 int memblock_mark_hotplug(phys_addr_t base, phys_addr_t size);
 int memblock_clear_hotplug(phys_addr_t base, phys_addr_t size);
 int memblock_mark_mirror(phys_addr_t base, phys_addr_t size);
diff --git a/mm/memblock.c b/mm/memblock.c
index 1018e50566f3..61ec50647469 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -191,6 +191,27 @@ bool __init_memblock memblock_overlaps_region(struct memblock_type *type,
 	return i < type->cnt;
 }
 
+/**
+ * memblock_node_is_crypto_capable - get if whole node is capable
+ * of encryption
+ * @nid: number of node
+ *
+ * Iterate over all memory memblock_type and find if all regions under
+ * node @nid are capable of hardware encryption.
+ */
+bool __init_memblock memblock_node_is_crypto_capable(int nid)
+{
+	struct memblock_region *region;
+
+	for_each_mem_region(region) {
+		if ((memblock_get_region_node(region) == nid) &&
+		    !(region->flags & MEMBLOCK_CRYPTO_CAPABLE))
+			return false;
+	}
+
+	return true;
+}
+
 /**
  * __memblock_find_range_bottom_up - find free area utility in bottom-up
  * @start: start of candidate range
@@ -885,6 +906,34 @@ static int __init_memblock memblock_setclr_flag(phys_addr_t base,
 	return 0;
 }
 
+/**
+ * memblock_mark_crypto_capable - Mark memory regions capable of hardware
+ * encryption with flag MEMBLOCK_CRYPTO_CAPABLE.
+ * @base: the base phys addr of the region
+ * @size: the size of the region
+ *
+ * Return: 0 on success, -errno on failure.
+ */
+int __init_memblock memblock_mark_crypto_capable(phys_addr_t base,
+						 phys_addr_t size)
+{
+	return memblock_setclr_flag(base, size, 1, MEMBLOCK_CRYPTO_CAPABLE);
+}
+
+/**
+ * memblock_clear_crypto_capable - Clear flag MEMBLOCK_CRYPTO for a
+ * specified region.
+ * @base: the base phys addr of the region
+ * @size: the size of the region
+ *
+ * Return: 0 on success, -errno on failure.
+ */
+int __init_memblock memblock_clear_crypto_capable(phys_addr_t base,
+						  phys_addr_t size)
+{
+	return memblock_setclr_flag(base, size, 0, MEMBLOCK_CRYPTO_CAPABLE);
+}
+
 /**
  * memblock_mark_hotplug - Mark hotpluggable memory with flag MEMBLOCK_HOTPLUG.
  * @base: the base phys addr of the region
-- 
2.30.2

[PATCH v4 2/5] mm/mmzone: Tag pg_data_t with crypto capabilities

[Martin Fernandez]

Add a new member in the pg_data_t struct tell whether the node
corresponding to that pg_data_t is able to do hardware memory encryption.

This will be read from sysfs.

Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com>
---
 include/linux/mmzone.h | 3 +++
 mm/page_alloc.c        | 1 +
 2 files changed, 4 insertions(+)

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 58e744b78c2c..1e4f76a19c62 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -870,6 +870,9 @@ typedef struct pglist_data {
 	struct task_struct *kcompactd;
 	bool proactive_compact_trigger;
 #endif
+
+	bool crypto_capable;
+
 	/*
 	 * This is a per-node reserve of pages that are not available
 	 * to userspace allocations.
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index c5952749ad40..8bcbd6fa0089 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -7592,6 +7592,7 @@ static void __init free_area_init_node(int nid)
 	pgdat->node_id = nid;
 	pgdat->node_start_pfn = start_pfn;
 	pgdat->per_cpu_nodestats = NULL;
+	pgdat->crypto_capable = memblock_node_is_crypto_capable(nid);
 
 	pr_info("Initmem setup node %d [mem %#018Lx-%#018Lx]\n", nid,
 		(u64)start_pfn << PAGE_SHIFT,
-- 
2.30.2

[PATCH v4 3/5] x86/e820: Tag e820_entry with crypto capabilities

[Martin Fernandez]

Add a new member in e820_entry to hold whether an entry is able to do
hardware memory encryption or not.

Add a new argument to __e820__range_add to accept this new
crypto_capable.

Add a new argument to __e820__update_range to be able to change a
region's crypto_capable member. Also, change its behavior a little,
before if you wanted to update a region with its same type it was a
BUG_ON; now if you call it with both old_type and new_type equals,
then the function won't change the types, just crypto_capable.

Change e820__update_table to handle merging and overlap problems
taking into account crypto_capable.

Add a function to mark a range as crypto, using __e820__range_update
in the background. This will be called when initializing EFI.

Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com>
---
 arch/x86/include/asm/e820/api.h   |  1 +
 arch/x86/include/asm/e820/types.h |  1 +
 arch/x86/kernel/e820.c            | 59 ++++++++++++++++++++++++-------
 3 files changed, 49 insertions(+), 12 deletions(-)

diff --git a/arch/x86/include/asm/e820/api.h b/arch/x86/include/asm/e820/api.h
index e8f58ddd06d9..677dcbabcc8b 100644
--- a/arch/x86/include/asm/e820/api.h
+++ b/arch/x86/include/asm/e820/api.h
@@ -17,6 +17,7 @@ extern bool e820__mapped_all(u64 start, u64 end, enum e820_type type);
 extern void e820__range_add   (u64 start, u64 size, enum e820_type type);
 extern u64  e820__range_update(u64 start, u64 size, enum e820_type old_type, enum e820_type new_type);
 extern u64  e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool check_type);
+extern u64  e820__range_mark_as_crypto_capable(u64 start, u64 size);
 
 extern void e820__print_table(char *who);
 extern int  e820__update_table(struct e820_table *table);
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;
 } __attribute__((packed));
 
 /*
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)
 {
 	int x = table->nr_entries;
 
@@ -176,12 +176,13 @@ static void __init __e820__range_add(struct e820_table *table, u64 start, u64 si
 	table->entries[x].addr = start;
 	table->entries[x].size = size;
 	table->entries[x].type = type;
+	table->entries[x].crypto_capable = crypto_capable;
 	table->nr_entries++;
 }
 
 void __init e820__range_add(u64 start, u64 size, enum e820_type type)
 {
-	__e820__range_add(e820_table, start, size, type);
+	__e820__range_add(e820_table, start, size, type, 0);
 }
 
 static void __init e820_print_type(enum e820_type type)
@@ -211,6 +212,8 @@ void __init e820__print_table(char *who)
 			e820_table->entries[i].addr + e820_table->entries[i].size - 1);
 
 		e820_print_type(e820_table->entries[i].type);
+		if (e820_table->entries[i].crypto_capable)
+			pr_cont("; crypto-capable");
 		pr_cont("\n");
 	}
 }
@@ -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;
 			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)) {
 			if (last_type != 0)	 {
 				new_entries[new_nr_entries].size = change_point[chg_idx]->addr - last_addr;
 				/* Move forward only if the new size was non-zero: */
@@ -406,9 +415,12 @@ int __init e820__update_table(struct e820_table *table)
 			if (current_type != 0)	{
 				new_entries[new_nr_entries].addr = change_point[chg_idx]->addr;
 				new_entries[new_nr_entries].type = current_type;
+				new_entries[new_nr_entries].crypto_capable = current_crypto;
+
 				last_addr = change_point[chg_idx]->addr;
 			}
 			last_type = current_type;
+			last_crypto = current_crypto;
 		}
 	}
 
@@ -459,14 +471,20 @@ static int __init append_e820_table(struct boot_e820_entry *entries, u32 nr_entr
 	return __append_e820_table(entries, nr_entries);
 }
 
+/*
+ * Update a memory range.
+ *
+ * If old_type and new_type are the same then ignore the types and
+ * just change crypto_capable.
+ */
 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__range_update(struct e820_table *table, u64 start, u64 size, enum e820_type old_type, enum e820_type new_type, u8 crypto_capable)
 {
 	u64 end;
 	unsigned int i;
 	u64 real_updated_size = 0;
 
-	BUG_ON(old_type == new_type);
+	bool update_crypto = new_type == old_type;
 
 	if (size > (ULLONG_MAX - start))
 		size = ULLONG_MAX - start;
@@ -476,6 +494,8 @@ __e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_ty
 	e820_print_type(old_type);
 	pr_cont(" ==> ");
 	e820_print_type(new_type);
+	if (crypto_capable)
+		pr_cont("; crypto-capable");
 	pr_cont("\n");
 
 	for (i = 0; i < table->nr_entries; i++) {
@@ -483,22 +503,27 @@ __e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_ty
 		u64 final_start, final_end;
 		u64 entry_end;
 
-		if (entry->type != old_type)
+		if (entry->type != old_type && !update_crypto)
 			continue;
 
+		if (update_crypto)
+			new_type = entry->type;
+
 		entry_end = entry->addr + entry->size;
 
 		/* Completely covered by new range? */
 		if (entry->addr >= start && entry_end <= end) {
 			entry->type = new_type;
+			entry->crypto_capable = crypto_capable;
 			real_updated_size += entry->size;
 			continue;
 		}
 
 		/* 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);
+			__e820__range_add(table, start, size, new_type, crypto_capable);
+			__e820__range_add(table, end, entry_end - end,
+					  entry->type, entry->crypto_capable);
 			entry->size = start - entry->addr;
 			real_updated_size += size;
 			continue;
@@ -510,7 +535,8 @@ __e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_ty
 		if (final_start >= final_end)
 			continue;
 
-		__e820__range_add(table, final_start, final_end - final_start, new_type);
+		__e820__range_add(table, final_start, final_end - final_start,
+				  new_type, crypto_capable);
 
 		real_updated_size += final_end - final_start;
 
@@ -527,14 +553,19 @@ __e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_ty
 	return real_updated_size;
 }
 
+u64 __init e820__range_mark_as_crypto_capable(u64 start, u64 size)
+{
+	return __e820__range_update(e820_table, start, size, 0, 0, true);
+}
+
 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);
+	return __e820__range_update(e820_table, start, size, old_type, new_type, false);
 }
 
 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);
+	return __e820__range_update(e820_table_kexec, start, size, old_type, new_type, false);
 }
 
 /* Remove a range of memory from the E820 table: */
@@ -572,7 +603,9 @@ u64 __init e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool
 
 		/* Is the new range completely covered? */
 		if (entry->addr < start && entry_end > end) {
-			e820__range_add(end, entry_end - end, entry->type);
+			__e820__range_add(e820_table, end, entry_end - end,
+					  entry->type, entry->crypto_capable);
+
 			entry->size = start - entry->addr;
 			real_removed_size += size;
 			continue;
@@ -1322,6 +1355,8 @@ void __init e820__memblock_setup(void)
 			continue;
 
 		memblock_add(entry->addr, entry->size);
+		if (entry->crypto_capable)
+			memblock_mark_crypto_capable(entry->addr, entry->size);
 	}
 
 	/* Throw away partial pages: */
-- 
2.30.2

[PATCH v4 4/5] x86/efi: Tag e820_entries as crypto capable from EFI memmap

[Martin Fernandez]

Add a function to iterate over the EFI Memory Map and mark the regions
tagged with EFI_MEMORY_CPU_CRYPTO in the e820_table; and call it from
efi_init if add_efi_memmap is disabled.

Also modify do_add_efi_memmap to mark the regions there.

Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com>
---
 arch/x86/platform/efi/efi.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 147c30a81f15..ce2f9d38fe36 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -184,6 +184,8 @@ static void __init do_add_efi_memmap(void)
 		}
 
 		e820__range_add(start, size, e820_type);
+		if (md->attribute & EFI_MEMORY_CPU_CRYPTO)
+			e820__range_mark_as_crypto_capable(start, size);
 	}
 	e820__update_table(e820_table);
 }
@@ -441,6 +443,23 @@ static int __init efi_config_init(const efi_config_table_type_t *arch_tables)
 	return ret;
 }
 
+static void __init efi_mark_e820_regions_as_crypto_capable(void)
+{
+	efi_memory_desc_t *md;
+
+	for_each_efi_memory_desc(md) {
+		if (md->attribute & EFI_MEMORY_CPU_CRYPTO)
+			e820__range_mark_as_crypto_capable(md->phys_addr,
+							   md->num_pages << EFI_PAGE_SHIFT);
+	}
+
+	/*
+	 * We added and modified regions so it's good to update the
+	 * table to merge/sort
+	 */
+	e820__update_table(e820_table);
+}
+
 void __init efi_init(void)
 {
 	if (IS_ENABLED(CONFIG_X86_32) &&
@@ -494,6 +513,13 @@ void __init efi_init(void)
 	set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
 	efi_clean_memmap();
 
+	/*
+	 * If add_efi_memmap then there is no need to mark the regions
+	 * again
+	 */
+	if (!add_efi_memmap)
+		efi_mark_e820_regions_as_crypto_capable();
+
 	if (efi_enabled(EFI_DBG))
 		efi_print_memmap();
 }
-- 
2.30.2

[PATCH v4 5/5] drivers/node: Show in sysfs node's crypto capabilities

[Martin Fernandez]

Show in each node in sysfs if its memory is able to do be encrypted by
the CPU, ie. if all its memory is marked with EFI_MEMORY_CPU_CRYPTO in
the EFI memory map.

Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com>
---
 Documentation/ABI/testing/sysfs-devices-node | 10 ++++++++++
 drivers/base/node.c                          | 10 ++++++++++
 2 files changed, 20 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-devices-node

diff --git a/Documentation/ABI/testing/sysfs-devices-node b/Documentation/ABI/testing/sysfs-devices-node
new file mode 100644
index 000000000000..93581b31225e
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-devices-node
@@ -0,0 +1,10 @@
+What:		/sys/devices/system/node/nodeX/crypto_capable
+Date:		December 2021
+Contact:	Martin Fernandez <martin.fernandez@eclypsium.com>
+Users:		fwupd (https://fwupd.org)
+Description:
+		This value is 1 if all system memory in this node is
+		marked with EFI_MEMORY_CPU_CRYPTO, indicating that the
+		system memory is capable of being protected with the
+		CPU’s memory cryptographic capabilities. It is 0
+		otherwise.
\ No newline at end of file
diff --git a/drivers/base/node.c b/drivers/base/node.c
index b5a4ba18f9f9..67b0e2fa93b1 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -560,11 +560,21 @@ static ssize_t node_read_distance(struct device *dev,
 }
 static DEVICE_ATTR(distance, 0444, node_read_distance, NULL);
 
+static ssize_t crypto_capable_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct pglist_data *pgdat = NODE_DATA(dev->id);
+
+	return sysfs_emit(buf, "%d\n", pgdat->crypto_capable);
+}
+static DEVICE_ATTR_RO(crypto_capable);
+
 static struct attribute *node_dev_attrs[] = {
 	&dev_attr_meminfo.attr,
 	&dev_attr_numastat.attr,
 	&dev_attr_distance.attr,
 	&dev_attr_vmstat.attr,
+	&dev_attr_crypto_capable.attr,
 	NULL
 };
 
-- 
2.30.2

Re: [PATCH v4 3/5] x86/e820: Tag e820_entry with crypto capabilities

[Greg KH]

On Thu, Dec 16, 2021 at 04:22:20PM -0300, Martin Fernandez wrote:
> Add a new member in e820_entry to hold whether an entry is able to do
> hardware memory encryption or not.
> 
> Add a new argument to __e820__range_add to accept this new
> crypto_capable.
> 
> Add a new argument to __e820__update_range to be able to change a
> region's crypto_capable member. Also, change its behavior a little,
> before if you wanted to update a region with its same type it was a
> BUG_ON; now if you call it with both old_type and new_type equals,
> then the function won't change the types, just crypto_capable.
> 
> Change e820__update_table to handle merging and overlap problems
> taking into account crypto_capable.
> 
> Add a function to mark a range as crypto, using __e820__range_update
> in the background. This will be called when initializing EFI.
> 
> Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com>
> ---
>  arch/x86/include/asm/e820/api.h   |  1 +
>  arch/x86/include/asm/e820/types.h |  1 +
>  arch/x86/kernel/e820.c            | 59 ++++++++++++++++++++++++-------
>  3 files changed, 49 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/include/asm/e820/api.h b/arch/x86/include/asm/e820/api.h
> index e8f58ddd06d9..677dcbabcc8b 100644
> --- a/arch/x86/include/asm/e820/api.h
> +++ b/arch/x86/include/asm/e820/api.h
> @@ -17,6 +17,7 @@ extern bool e820__mapped_all(u64 start, u64 end, enum e820_type type);
>  extern void e820__range_add   (u64 start, u64 size, enum e820_type type);
>  extern u64  e820__range_update(u64 start, u64 size, enum e820_type old_type, enum e820_type new_type);
>  extern u64  e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool check_type);
> +extern u64  e820__range_mark_as_crypto_capable(u64 start, u64 size);
>  
>  extern void e820__print_table(char *who);
>  extern int  e820__update_table(struct e820_table *table);
> 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?

>  } __attribute__((packed));
>  
>  /*
> 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.

>  {
>  	int x = table->nr_entries;
>  
> @@ -176,12 +176,13 @@ static void __init __e820__range_add(struct e820_table *table, u64 start, u64 si
>  	table->entries[x].addr = start;
>  	table->entries[x].size = size;
>  	table->entries[x].type = type;
> +	table->entries[x].crypto_capable = crypto_capable;
>  	table->nr_entries++;
>  }
>  
>  void __init e820__range_add(u64 start, u64 size, enum e820_type type)
>  {
> -	__e820__range_add(e820_table, start, size, type);
> +	__e820__range_add(e820_table, start, size, type, 0);
>  }
>  
>  static void __init e820_print_type(enum e820_type type)
> @@ -211,6 +212,8 @@ void __init e820__print_table(char *who)
>  			e820_table->entries[i].addr + e820_table->entries[i].size - 1);
>  
>  		e820_print_type(e820_table->entries[i].type);
> +		if (e820_table->entries[i].crypto_capable)
> +			pr_cont("; crypto-capable");
>  		pr_cont("\n");
>  	}
>  }
> @@ -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 :(

>  			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?

>  			if (last_type != 0)	 {
>  				new_entries[new_nr_entries].size = change_point[chg_idx]->addr - last_addr;
>  				/* Move forward only if the new size was non-zero: */
> @@ -406,9 +415,12 @@ int __init e820__update_table(struct e820_table *table)
>  			if (current_type != 0)	{
>  				new_entries[new_nr_entries].addr = change_point[chg_idx]->addr;
>  				new_entries[new_nr_entries].type = current_type;
> +				new_entries[new_nr_entries].crypto_capable = current_crypto;
> +
>  				last_addr = change_point[chg_idx]->addr;
>  			}
>  			last_type = current_type;
> +			last_crypto = current_crypto;
>  		}
>  	}
>  
> @@ -459,14 +471,20 @@ static int __init append_e820_table(struct boot_e820_entry *entries, u32 nr_entr
>  	return __append_e820_table(entries, nr_entries);
>  }
>  
> +/*
> + * Update a memory range.
> + *
> + * If old_type and new_type are the same then ignore the types and
> + * just change crypto_capable.
> + */
>  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__range_update(struct e820_table *table, u64 start, u64 size, enum e820_type old_type, enum e820_type new_type, u8 crypto_capable)

Same api comment here.

>  {
>  	u64 end;
>  	unsigned int i;
>  	u64 real_updated_size = 0;
>  
> -	BUG_ON(old_type == new_type);

No more bug?

> +	bool update_crypto = new_type == old_type;

if statement?  This looks really odd and is easy to overlook.

>  
>  	if (size > (ULLONG_MAX - start))
>  		size = ULLONG_MAX - start;
> @@ -476,6 +494,8 @@ __e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_ty
>  	e820_print_type(old_type);
>  	pr_cont(" ==> ");
>  	e820_print_type(new_type);
> +	if (crypto_capable)
> +		pr_cont("; crypto-capable");
>  	pr_cont("\n");
>  
>  	for (i = 0; i < table->nr_entries; i++) {
> @@ -483,22 +503,27 @@ __e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_ty
>  		u64 final_start, final_end;
>  		u64 entry_end;
>  
> -		if (entry->type != old_type)
> +		if (entry->type != old_type && !update_crypto)
>  			continue;
>  
> +		if (update_crypto)
> +			new_type = entry->type;
> +
>  		entry_end = entry->addr + entry->size;
>  
>  		/* Completely covered by new range? */
>  		if (entry->addr >= start && entry_end <= end) {
>  			entry->type = new_type;
> +			entry->crypto_capable = crypto_capable;
>  			real_updated_size += entry->size;
>  			continue;
>  		}
>  
>  		/* 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);
> +			__e820__range_add(table, start, size, new_type, crypto_capable);
> +			__e820__range_add(table, end, entry_end - end,
> +					  entry->type, entry->crypto_capable);
>  			entry->size = start - entry->addr;
>  			real_updated_size += size;
>  			continue;
> @@ -510,7 +535,8 @@ __e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_ty
>  		if (final_start >= final_end)
>  			continue;
>  
> -		__e820__range_add(table, final_start, final_end - final_start, new_type);
> +		__e820__range_add(table, final_start, final_end - final_start,
> +				  new_type, crypto_capable);
>  
>  		real_updated_size += final_end - final_start;
>  
> @@ -527,14 +553,19 @@ __e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_ty
>  	return real_updated_size;
>  }
>  
> +u64 __init e820__range_mark_as_crypto_capable(u64 start, u64 size)
> +{
> +	return __e820__range_update(e820_table, start, size, 0, 0, true);
> +}
> +
>  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);
> +	return __e820__range_update(e820_table, start, size, old_type, new_type, false);

See, what does "false" here mean?  You have to now go look that up.

thanks,

greg k-h

Re: [PATCH v4 3/5] x86/e820: Tag e820_entry with crypto capabilities

[Martin Fernandez]

On 12/20/21, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Dec 16, 2021 at 04:22:20PM -0300, Martin Fernandez wrote:
>> 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.

>> 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.

>> @@ -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 :(
>
>>  			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?

>>  			if (last_type != 0)	 {
>>  				new_entries[new_nr_entries].size = change_point[chg_idx]->addr -
>> last_addr;
>>  				/* Move forward only if the new size was non-zero: */
>> @@ -406,9 +415,12 @@ int __init e820__update_table(struct e820_table
>> *table)
>>  			if (current_type != 0)	{
>>  				new_entries[new_nr_entries].addr = change_point[chg_idx]->addr;
>>  				new_entries[new_nr_entries].type = current_type;
>> +				new_entries[new_nr_entries].crypto_capable = current_crypto;
>> +
>>  				last_addr = change_point[chg_idx]->addr;
>>  			}
>>  			last_type = current_type;
>> +			last_crypto = current_crypto;
>>  		}
>>  	}
>>
>> @@ -459,14 +471,20 @@ static int __init append_e820_table(struct
>> boot_e820_entry *entries, u32 nr_entr
>>  	return __append_e820_table(entries, nr_entries);
>>  }
>>
>> +/*
>> + * Update a memory range.
>> + *
>> + * If old_type and new_type are the same then ignore the types and
>> + * just change crypto_capable.
>> + */
>>  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__range_update(struct e820_table *table, u64 start, u64 size, enum
>> e820_type old_type, enum e820_type new_type, u8 crypto_capable)
>
> Same api comment here.
>
>>  {
>>  	u64 end;
>>  	unsigned int i;
>>  	u64 real_updated_size = 0;
>>
>> -	BUG_ON(old_type == new_type);
>
> No more bug?
>

No,

>> +	bool update_crypto = new_type == old_type;
>
> if statement?  This looks really odd and is easy to overlook.
>

because now I'm using those cases where new_type is equal to old_type
to change crypto_capable, because I don't care about the type if I
want to change crypto_capable. This is what I'm checking here, to
ignore the types inside the for and just leave them as they are.

I'm not really happy with this change, maybe a new function will be
better, although it will be pretty similar to this one and its
implementation is quite fragile, for checking the cases and
adding/removing the regions, so I decided to just have only one ugly
fragile function, instead of two fragile functions.

I can do the new function if you prefer.

>>
>>  	if (size > (ULLONG_MAX - start))
>>  		size = ULLONG_MAX - start;
>> @@ -476,6 +494,8 @@ __e820__range_update(struct e820_table *table, u64
>> start, u64 size, enum e820_ty
>>  	e820_print_type(old_type);
>>  	pr_cont(" ==> ");
>>  	e820_print_type(new_type);
>> +	if (crypto_capable)
>> +		pr_cont("; crypto-capable");
>>  	pr_cont("\n");
>>
>>  	for (i = 0; i < table->nr_entries; i++) {
>> @@ -483,22 +503,27 @@ __e820__range_update(struct e820_table *table, u64
>> start, u64 size, enum e820_ty
>>  		u64 final_start, final_end;
>>  		u64 entry_end;
>>
>> -		if (entry->type != old_type)
>> +		if (entry->type != old_type && !update_crypto)
>>  			continue;
>>
>> +		if (update_crypto)
>> +			new_type = entry->type;
>> +
>>  		entry_end = entry->addr + entry->size;
>>
>>  		/* Completely covered by new range? */
>>  		if (entry->addr >= start && entry_end <= end) {
>>  			entry->type = new_type;
>> +			entry->crypto_capable = crypto_capable;
>>  			real_updated_size += entry->size;
>>  			continue;
>>  		}
>>
>>  		/* 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);
>> +			__e820__range_add(table, start, size, new_type, crypto_capable);
>> +			__e820__range_add(table, end, entry_end - end,
>> +					  entry->type, entry->crypto_capable);
>>  			entry->size = start - entry->addr;
>>  			real_updated_size += size;
>>  			continue;
>> @@ -510,7 +535,8 @@ __e820__range_update(struct e820_table *table, u64
>> start, u64 size, enum e820_ty
>>  		if (final_start >= final_end)
>>  			continue;
>>
>> -		__e820__range_add(table, final_start, final_end - final_start,
>> new_type);
>> +		__e820__range_add(table, final_start, final_end - final_start,
>> +				  new_type, crypto_capable);
>>
>>  		real_updated_size += final_end - final_start;
>>
>> @@ -527,14 +553,19 @@ __e820__range_update(struct e820_table *table, u64
>> start, u64 size, enum e820_ty
>>  	return real_updated_size;
>>  }
>>
>> +u64 __init e820__range_mark_as_crypto_capable(u64 start, u64 size)
>> +{
>> +	return __e820__range_update(e820_table, start, size, 0, 0, true);
>> +}
>> +
>>  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);
>> +	return __e820__range_update(e820_table, start, size, old_type, new_type,
>> false);
>
> See, what does "false" here mean?  You have to now go look that up.
>
> thanks,
>
> greg k-h
>

Re: [PATCH v4 3/5] x86/e820: Tag e820_entry with crypto capabilities

[Greg KH]

On Mon, Dec 20, 2021 at 05:27:00PM -0300, Martin Fernandez wrote:
> On 12/20/21, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Thu, Dec 16, 2021 at 04:22:20PM -0300, Martin Fernandez wrote:
> >> 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.

> >> 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.
> 
> >> @@ -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 :(
> >
> >>  			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

Re: [PATCH v4 3/5] x86/e820: Tag e820_entry with crypto capabilities

[Martin Fernandez]

On 12/21/21, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Mon, Dec 20, 2021 at 05:27:00PM -0300, Martin Fernandez wrote:
>> On 12/20/21, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Thu, Dec 16, 2021 at 04:22:20PM -0300, Martin Fernandez wrote:
>> >> 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.
>

I prefer that Andy answers.

Either way, I think that the enum will be the best option.

>> >> 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.
>>
>> >> @@ -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 :(
>> >
>> >>  			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?
>

Oh I see. No, that's not a problem. That if guard is to decide if you
need to start/close a region. e820_nomerge is to prevent merging
certain region types. In any case, the new check will cause less
merging, complying with what e820_nomerge says.