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

[PATCH 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. It's
planned to make it part of a specification that can be passed to
people purchasing hardware. It's called Host Security ID:
https://fwupd.github.io/libfwupdplugin/hsi.html

This also can be useful in the future if NUMA decides to prioritize
nodes that are able to do encryption.

Martin Fernandez (5):
  Extend memblock to support memory encryption
  Extend pg_data_t to hold information about memory encryption
  Extend e820_table to hold information about memory encryption
  Mark e820_entries as crypto capable from EFI memmap
  Show in sysfs if a memory node is able to do encryption

 Documentation/ABI/testing/sysfs-devices-node |  10 ++
 arch/x86/include/asm/e820/api.h              |   2 +
 arch/x86/include/asm/e820/types.h            |   1 +
 arch/x86/kernel/e820.c                       |  32 +++++-
 arch/x86/platform/efi/efi.c                  | 109 +++++++++++++++++++
 drivers/base/node.c                          |  10 ++
 include/linux/memblock.h                     |   6 +
 include/linux/mmzone.h                       |   2 +
 mm/memblock.c                                |  74 +++++++++++++
 mm/page_alloc.c                              |   1 +
 10 files changed, 245 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-devices-node


base-commit: 3906fe9bb7f1a2c8667ae54e967dc8690824f4ea
--
2.30.2

[PATCH 1/5] Extend memblock to support memory encryption

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

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

diff --git a/include/linux/memblock.h b/include/linux/memblock.h
index 34de69b3b8ba..945af2cc7966 100644
--- a/include/linux/memblock.h
+++ b/include/linux/memblock.h
@@ -31,6 +31,7 @@ extern unsigned long long max_possible_pfn;
  * @MEMBLOCK_HOTPLUG: hotpluggable region
  * @MEMBLOCK_MIRROR: mirrored region
  * @MEMBLOCK_NOMAP: don't add to kernel direct mapping and treat as
+ * @MEMBLOCK_CRYPTO_CAPABLE: capable of hardware encryption
  * reserved in the memory map; refer to memblock_mark_nomap() description
  * for further details
  */
@@ -39,6 +40,7 @@ enum memblock_flags {
 	MEMBLOCK_HOTPLUG	= 0x1,	/* hotpluggable region */
 	MEMBLOCK_MIRROR		= 0x2,	/* mirrored region */
 	MEMBLOCK_NOMAP		= 0x4,	/* don't add to kernel direct mapping */
+	MEMBLOCK_CRYPTO_CAPABLE = 0x8,  /* capable of hardware encryption */
 };
 
 /**
@@ -102,6 +104,7 @@ static inline void memblock_discard(void) {}
 void memblock_allow_resize(void);
 int memblock_add_node(phys_addr_t base, phys_addr_t size, int nid);
 int memblock_add(phys_addr_t base, phys_addr_t size);
+int memblock_add_crypto_capable(phys_addr_t base, phys_addr_t size);
 int memblock_remove(phys_addr_t base, phys_addr_t size);
 int memblock_free(phys_addr_t base, phys_addr_t size);
 int memblock_reserve(phys_addr_t base, phys_addr_t size);
@@ -111,6 +114,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 5096500b2647..805e0e43ec66 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
@@ -694,6 +715,31 @@ int __init_memblock memblock_add(phys_addr_t base, phys_addr_t size)
 	return memblock_add_range(&memblock.memory, base, size, MAX_NUMNODES, 0);
 }
 
+/**
+ * memblock_add_crypto_capable - add new memblock region capable of
+ * hardware encryption
+ * @base: base address of the new region
+ * @size: size of the new region
+ *
+ * Add new memblock region [@base, @base + @size) to the "memory" type
+ * and set the MEMBLOCK_CRYPTO_CAPABLE flag. See memblock_add_range()
+ * description for mode details
+ *
+ * Return:
+ * 0 on success, -errno on failure.
+ */
+int __init_memblock memblock_add_crypto_capable(phys_addr_t base,
+						phys_addr_t size)
+{
+	const phys_addr_t end = base + size - 1;
+
+	memblock_dbg("%s: [%pa-%pa] %pS\n", __func__,
+		     &base, &end, (void *)_RET_IP_);
+
+	return memblock_add_range(&memblock.memory, base, size, MAX_NUMNODES,
+				  MEMBLOCK_CRYPTO_CAPABLE);
+}
+
 /**
  * memblock_isolate_range - isolate given range into disjoint memblocks
  * @type: memblock type to isolate range for
@@ -884,6 +930,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 2/5] Extend pg_data_t to hold information about memory encryption

[Martin Fernandez]

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

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

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 6a1d79d84675..998fbe371a81 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -869,6 +869,8 @@ typedef struct pglist_data {
 	unsigned long		min_slab_pages;
 #endif /* CONFIG_NUMA */

+	bool crypto_capable;
+
 	/* Write-intensive fields used by page reclaim */
 	ZONE_PADDING(_pad1_)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index b37435c274cf..a19d95bb5c0f 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -7575,6 +7575,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 3/5] Extend e820_table to hold information about memory encryption

[Martin Fernandez]

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

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

diff --git a/arch/x86/include/asm/e820/api.h b/arch/x86/include/asm/e820/api.h
index e8f58ddd06d9..f3a09b6afca1 100644
--- a/arch/x86/include/asm/e820/api.h
+++ b/arch/x86/include/asm/e820/api.h
@@ -18,6 +18,8 @@ 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 void e820__mark_regions_as_crypto_capable(u64 start, u64 size);
+
 extern void e820__print_table(char *who);
 extern int  e820__update_table(struct e820_table *table);
 extern void e820__update_table_print(void);
diff --git a/arch/x86/include/asm/e820/types.h b/arch/x86/include/asm/e820/types.h
index 314f75d886d0..231c9ad9a9c3 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;
+	bool			crypto_capable;
 } __attribute__((packed));
 
 /*
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index bc0657f0deed..3e0aaa5525e0 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -176,6 +176,7 @@ 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 = false;
 	table->nr_entries++;
 }
 
@@ -184,6 +185,19 @@ void __init e820__range_add(u64 start, u64 size, enum e820_type type)
 	__e820__range_add(e820_table, start, size, type);
 }
 
+void __init e820__mark_regions_as_crypto_capable(u64 start, u64 size)
+{
+	int i;
+	u64 end = start + size;
+
+	for (i = 0; i < e820_table->nr_entries; i++) {
+		struct e820_entry *const entry = &e820_table->entries[i];
+
+		if (entry->addr >= start && entry->addr + entry->size <= end)
+			entry->crypto_capable = true;
+	}
+}
+
 static void __init e820_print_type(enum e820_type type)
 {
 	switch (type) {
@@ -211,6 +225,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);
+		pr_cont("%s",
+			e820_table->entries[i].crypto_capable ? "; crypto-capable" : "");
 		pr_cont("\n");
 	}
 }
@@ -327,6 +343,8 @@ 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;
+	bool current_crypto;
+	bool last_crypto = false;
 
 	/* If there's only one memory region, don't bother: */
 	if (table->nr_entries < 2)
@@ -388,13 +406,17 @@ int __init e820__update_table(struct e820_table *table)
 		 * 1=usable, 2,3,4,4+=unusable)
 		 */
 		current_type = 0;
+		current_crypto = false;
 		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,6 +428,9 @@ 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_crypto = current_crypto;
 				last_addr = change_point[chg_idx]->addr;
 			}
 			last_type = current_type;
@@ -1321,7 +1346,10 @@ void __init e820__memblock_setup(void)
 		if (entry->type != E820_TYPE_RAM && entry->type != E820_TYPE_RESERVED_KERN)
 			continue;
 
-		memblock_add(entry->addr, entry->size);
+		if (entry->crypto_capable)
+			memblock_add_crypto_capable(entry->addr, entry->size);
+		else
+			memblock_add(entry->addr, entry->size);
 	}
 
 	/* Throw away partial pages: */
-- 
2.30.2

[PATCH 4/5] Mark e820_entries as crypto capable from EFI memmap

[Martin Fernandez]

Iterate over the EFI memmap finding the contiguous regions that are
able to do hardware encryption (ie, those who have the
EFI_MEMORY_CPU_CRYPTO enabled) and mark those in the e820_table.

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

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 147c30a81f15..6cd1c11dbdad 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -441,6 +441,113 @@ static int __init efi_config_init(const efi_config_table_type_t *arch_tables)
 	return ret;
 }
 
+/*
+ * The contiguous_region type is used to help
+ * efi_mark_e820_regions_as_crypto_capable to pick all the contiguous
+ * regions that have the EFI_MEMORY_CPU_CRYPTO attribute, and call a
+ * function of the e820 module to mark those regions as being able to
+ * do hardware encryption.
+ *
+ * To use this properly the memory map must not have any overlapped
+ * regions and the regions should be sorted.
+ *
+ * cr in the function names stands for contiguous_region
+ */
+struct contiguous_region {
+	u64 start;
+	u64 end;
+};
+
+static void __init cr_init(struct contiguous_region *region)
+{
+	region->start = 0;
+	region->end = 0;
+}
+
+static void __init efi_md_to_cr(const efi_memory_desc_t *md,
+				struct contiguous_region *region)
+{
+	region->start = md->phys_addr;
+	region->end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1;
+}
+
+static u64 __init cr_size(const struct contiguous_region *r)
+{
+	return r->end - r->start + 1;
+}
+
+static bool __init cr_is_empty(const struct contiguous_region *r)
+{
+	/*
+	 * Since contiguous regions are built upon efi_memory_desc_t
+	 * it is safe to say that a region is empty if its size is
+	 * lower than the size of one EFI page.
+	 */
+	return cr_size(r) < (1 << EFI_PAGE_SHIFT);
+}
+
+static bool __init cr_merge_regions(struct contiguous_region *region1,
+				    const struct contiguous_region *region2)
+{
+	bool merged_result;
+
+	if (cr_is_empty(region1)) {
+		*region1 = *region2;
+		merged_result = true;
+	} else if (region1->end + 1 == region2->start) {
+		/* Extend region1 */
+		region1->end = region2->end;
+		merged_result = true;
+	} else {
+		merged_result = false;
+	}
+
+	return merged_result;
+}
+
+static void __init cr_mark_e820_as_crypto_capable(const struct contiguous_region *r)
+{
+	e820__mark_regions_as_crypto_capable(r->start, cr_size(r));
+}
+
+/*
+ * This assumes that there'll be no overlaps in the memory map
+ * (otherwise we'd have a deeper problem going on). It also assumes
+ * that the system DRAM regions are already sorted; in EDK2 based UEFI
+ * firmware the entries covering system DRAM are usually sorted, with
+ * additional MMIO entries appearing unordered. This is because the
+ * UEFI memory map is constructed from the GCD memory map, which is
+ * seeded with the DRAM regions at boot, and allocations are created
+ * by splitting them up.
+ */
+static void __init efi_mark_e820_regions_as_crypto_capable(void)
+{
+	efi_memory_desc_t *md;
+	struct contiguous_region prev_region;
+
+	cr_init(&prev_region);
+
+	for_each_efi_memory_desc(md) {
+		if (md->attribute & EFI_MEMORY_CPU_CRYPTO) {
+			struct contiguous_region cur_region;
+
+			efi_md_to_cr(md, &cur_region);
+
+			if (!cr_merge_regions(&prev_region, &cur_region)) {
+				cr_mark_e820_as_crypto_capable(&prev_region);
+				prev_region = cur_region;
+			} /* else: Merge succeeded, don't mark yet */
+		} else if (!cr_is_empty(&prev_region)) {
+			cr_mark_e820_as_crypto_capable(&prev_region);
+			cr_init(&prev_region);
+		} /* else: All previous regions are already marked */
+	}
+
+	/* Mark last region (if any) */
+	if (!cr_is_empty(&prev_region))
+		cr_mark_e820_as_crypto_capable(&prev_region);
+}
+
 void __init efi_init(void)
 {
 	if (IS_ENABLED(CONFIG_X86_32) &&
@@ -494,6 +601,8 @@ void __init efi_init(void)
 	set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
 	efi_clean_memmap();
 
+	efi_mark_e820_regions_as_crypto_capable();
+
 	if (efi_enabled(EFI_DBG))
 		efi_print_memmap();
 }
-- 
2.30.2

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

[Martin Fernandez]

Show in each node in sysfs if its memory is able to do hardware memory
encryption, 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..ab46fdd3f6a8
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-devices-node
@@ -0,0 +1,10 @@
+What:		/sys/devices/system/node/nodeX/crypto_capable
+Date:		October 2021
+Contact:	Martin Fernandez <martin.fernandez@eclypsium.com>
+Users:		fwupd
+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 c56d34f8158f..4e6ef86f4523 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 1/5] Extend memblock to support memory encryption

[Dave Hansen]

On 11/5/21 2:27 PM, Martin Fernandez wrote:
> 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.

This is looking quite a bit nicer than the last post.  One nit on the
changelogs: please make an effort to connect the patches.  If you
introduce a function here but don't use it, tell us where it _will_ be used.

As it stands, this series _looks_ like it is touching 3 disjoint things:
 * pg_data_t
 * e820
 * memblocks

It would be valuable to spend a few sentences to connect the dots.

Subsystem prefixes are also highly preferred.  Perhaps:

	mm: Extend memblock to support memory encryption

or

	mm/memblock: Tag memblocks with crypto capabilities

It also helps make it obvious that this series touches both core mm/ and
arch/x86/ code.

Re: [PATCH 2/5] Extend pg_data_t to hold information about memory encryption

[Dave Hansen]

On 11/5/21 2:27 PM, Martin Fernandez wrote:
> @@ -869,6 +869,8 @@ typedef struct pglist_data {
>  	unsigned long		min_slab_pages;
>  #endif /* CONFIG_NUMA */
> 
> +	bool crypto_capable;

When adding to a data structure, please take a moment and familiarize
yourself with it.  For instance, would it make sense to have an
'enum pgdat_flags' for this as opposed to a bool?

Or, if you place it up next to the CONFIG_COMPACTION
proactive_compact_trigger, can it share storage?  Placing it next to a
ulong is probably the worst place it can go.

Re: [PATCH 3/5] Extend e820_table to hold information about memory encryption

[Dave Hansen]

On 11/5/21 2:27 PM, Martin Fernandez wrote:
> Add a new member in e820_entry to hold whether an entry is able to do
> hardware memory encryption or not.

That's a bit sparse for what this is doing.  It covers the first hunk at
best.

> diff --git a/arch/x86/include/asm/e820/api.h b/arch/x86/include/asm/e820/api.h
> index e8f58ddd06d9..f3a09b6afca1 100644
> --- a/arch/x86/include/asm/e820/api.h
> +++ b/arch/x86/include/asm/e820/api.h
> @@ -18,6 +18,8 @@ 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 void e820__mark_regions_as_crypto_capable(u64 start, u64 size);
> +
>  extern void e820__print_table(char *who);
>  extern int  e820__update_table(struct e820_table *table);
>  extern void e820__update_table_print(void);
> diff --git a/arch/x86/include/asm/e820/types.h b/arch/x86/include/asm/e820/types.h
> index 314f75d886d0..231c9ad9a9c3 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;
> +	bool			crypto_capable;
>  } __attribute__((packed));
>  
>  /*
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index bc0657f0deed..3e0aaa5525e0 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -176,6 +176,7 @@ 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 = false;
>  	table->nr_entries++;
>  }
>  
> @@ -184,6 +185,19 @@ void __init e820__range_add(u64 start, u64 size, enum e820_type type)
>  	__e820__range_add(e820_table, start, size, type);
>  }
>  
> +void __init e820__mark_regions_as_crypto_capable(u64 start, u64 size)
> +{
> +	int i;
> +	u64 end = start + size;
> +
> +	for (i = 0; i < e820_table->nr_entries; i++) {
> +		struct e820_entry *const entry = &e820_table->entries[i];
> +
> +		if (entry->addr >= start && entry->addr + entry->size <= end)
> +			entry->crypto_capable = true;
> +	}
> +}

Looking at this in isolation, this is really tricky.  I have no idea
what this is _supposed_ to or expected to be doing.  It also makes me
wonder what happens when start/size don't line up exactly on an e820 entry.

>  static void __init e820_print_type(enum e820_type type)
>  {
>  	switch (type) {
> @@ -211,6 +225,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);
> +		pr_cont("%s",
> +			e820_table->entries[i].crypto_capable ? "; crypto-capable" : "");

Am I missing something or should this just be:

	if (e820_table->entries[i].crypto_capable)
		pr_cont("; crypto-capable");

In general, I find code that retreats to the ternary form is almost
always doing something nasty.

>  		pr_cont("\n");
>  	}
>  }
> @@ -327,6 +343,8 @@ 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;
> +	bool current_crypto;
> +	bool last_crypto = false;
>  
>  	/* If there's only one memory region, don't bother: */
>  	if (table->nr_entries < 2)
> @@ -388,13 +406,17 @@ int __init e820__update_table(struct e820_table *table)
>  		 * 1=usable, 2,3,4,4+=unusable)
>  		 */
>  		current_type = 0;
> +		current_crypto = false;
>  		for (i = 0; i < overlap_entries; i++) {
> +			current_crypto = current_crypto || overlap_list[i]->crypto_capable;

No comment, eh?

This seems backwards to me.  If there are overlapping region and only
one is crypto-capable, shouldn't the whole thing become non-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,6 +428,9 @@ 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_crypto = current_crypto;
>  				last_addr = change_point[chg_idx]->addr;
>  			}
>  			last_type = current_type;

The "current_crypto != last_crypto" checks seem to go with the
current_type/last_type checks.  I'm naively surprised that the
last_crypto assignment wasn't paired with the last_type assignment.

I kinda get the impression this was just quickly hacked in here.  It
seems like "crypto" and "type" are very closely related in how they are
being handled.  It's a shame they're not being managed in a common way.

> @@ -1321,7 +1346,10 @@ void __init e820__memblock_setup(void)
>  		if (entry->type != E820_TYPE_RAM && entry->type != E820_TYPE_RESERVED_KERN)
>  			continue;
>  
> -		memblock_add(entry->addr, entry->size);
> +		if (entry->crypto_capable)
> +			memblock_add_crypto_capable(entry->addr, entry->size);
> +		else
> +			memblock_add(entry->addr, entry->size);

Having a different memblock_add_foo() doesn't seem to be the way this is
done.  See:

	memblock_mark_hotplug();
or
	memblock_mark_mirror();

Shouldn't this be: memblock_mark_crypto()

By the way, how was this tested?

Re: [PATCH 4/5] Mark e820_entries as crypto capable from EFI memmap

[Dave Hansen]

On 11/5/21 2:27 PM, Martin Fernandez wrote:
> Iterate over the EFI memmap finding the contiguous regions that are
> able to do hardware encryption (ie, those who have the
> EFI_MEMORY_CPU_CRYPTO enabled) and mark those in the e820_table.

It would also be nice to remind the reader about the connection between
EFI memory maps and the e820.

...
> +/*
> + * This assumes that there'll be no overlaps in the memory map
> + * (otherwise we'd have a deeper problem going on). It also assumes
> + * that the system DRAM regions are already sorted; in EDK2 based UEFI
> + * firmware the entries covering system DRAM are usually sorted, with
> + * additional MMIO entries appearing unordered. This is because the
> + * UEFI memory map is constructed from the GCD memory map, which is
> + * seeded with the DRAM regions at boot, and allocations are created
> + * by splitting them up.
> + */
> +static void __init efi_mark_e820_regions_as_crypto_capable(void)
> +{
> +	efi_memory_desc_t *md;
> +	struct contiguous_region prev_region;
> +
> +	cr_init(&prev_region);

A little theory of operation for this would be nice.  What's collected
in here?  What does this region mean?  Is the *entire* region in here
crypto-capable system RAM?

> +	for_each_efi_memory_desc(md) {
> +		if (md->attribute & EFI_MEMORY_CPU_CRYPTO) {
> +			struct contiguous_region cur_region;
> +
> +			efi_md_to_cr(md, &cur_region);

FWIW, I think that helper obfuscates more than it helps.  I say, just
open-code it.

> +			if (!cr_merge_regions(&prev_region, &cur_region)) {
> +				cr_mark_e820_as_crypto_capable(&prev_region);
> +				prev_region = cur_region;
> +			} /* else: Merge succeeded, don't mark yet */

That's really unusual CodingStyle.  Could you try to move those to a
more normal place: below or above the if() entirely.

> +		} else if (!cr_is_empty(&prev_region)) {
> +			cr_mark_e820_as_crypto_capable(&prev_region);
> +			cr_init(&prev_region);
> +		} /* else: All previous regions are already marked */

There are much nicer ways to write this.  For instance, I'd much prefer:

		/* Bail on empty regions: */
		if (cr_is_empty(&prev_region))
			continue;

		/* Mark the region: */
		cr_mark_e820_as_crypto_capable(&prev_region);
		cr_init(&prev_region);
	
That makes the flow much more understandable.  In general, you want to
keep the "main" flow if your code at the lowest indent and the exception
handling as indented.

> +	}
> +
> +	/* Mark last region (if any) */
> +	if (!cr_is_empty(&prev_region))
> +		cr_mark_e820_as_crypto_capable(&prev_region);
> +}
> +
>  void __init efi_init(void)
>  {
>  	if (IS_ENABLED(CONFIG_X86_32) &&
> @@ -494,6 +601,8 @@ void __init efi_init(void)
>  	set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
>  	efi_clean_memmap();
>  
> +	efi_mark_e820_regions_as_crypto_capable();
> +
>  	if (efi_enabled(EFI_DBG))
>  		efi_print_memmap();
>  }
> 

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

[Dave Hansen]

On 11/5/21 2:27 PM, Martin Fernandez wrote:
> +++ b/Documentation/ABI/testing/sysfs-devices-node
> @@ -0,0 +1,10 @@
> +What:		/sys/devices/system/node/nodeX/crypto_capable
> +Date:		October 2021
> +Contact:	Martin Fernandez <martin.fernandez@eclypsium.com>
> +Users:		fwupd
> +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.

This looks fine.  I do wonder if we want to call this the full on
cpu_crypto_capable, in case we ever get something like CXL devices with
system memory and their *own* crypto capabilities.

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

[Dave Hansen]

On 11/5/21 2:27 PM, Martin Fernandez wrote:
>  Documentation/ABI/testing/sysfs-devices-node |  10 ++
>  arch/x86/include/asm/e820/api.h              |   2 +
>  arch/x86/include/asm/e820/types.h            |   1 +
>  arch/x86/kernel/e820.c                       |  32 +++++-
>  arch/x86/platform/efi/efi.c                  | 109 +++++++++++++++++++
>  drivers/base/node.c                          |  10 ++
>  include/linux/memblock.h                     |   6 +
>  include/linux/mmzone.h                       |   2 +
>  mm/memblock.c                                |  74 +++++++++++++
>  mm/page_alloc.c                              |   1 +

One more high-level comment: While the majority of this is x86 code, the
ABI implications are all arch-generic.  It would be really nice to know
if other architectures have any need for something like this.  Is the
ARM EFI code just a total fork of the x86 stuff?

I'd highly suggest cc'ing linux-mm@kvack.org on future versions.

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

[Williams, Dan J]

On Fri, 2021-11-05 at 18:27 -0300, Martin Fernandez wrote:
> Show for each node if every memory descriptor in that node has the
> EFI_MEMORY_CPU_CRYPTO attribute.

The problem I have with EFI_MEMORY_CPU_CRYPTO is it that is vague what
memory encryption technology is deployed and does not tell you anything
about whether it is in effect or not.

If this is just for basic inventory for determining if one platform
might be more secure than another then maybe it is ok, but I don't know
how well this will dovetail with CXL that can dynamically define memory
ranges. To date I've only seen a specification for CXL Link encryption,
data at rest encryption for CXL PMEM. I imagine one day it will gain
encryption capabilities, but that won't be something the platform
firmware will always be involved estabishing.

> 
> fwupd project plans to use it as part of a check to see if the users
> have properly configured memory hardware encryption capabilities. It's
> planned to make it part of a specification that can be passed to
> people purchasing hardware. It's called Host Security ID:
> https://fwupd.github.io/libfwupdplugin/hsi.html
> 
> This also can be useful in the future if NUMA decides to prioritize
> nodes that are able to do encryption.

I'd feel better if this one one step indirected from the raw EFI
attribute and let architectures indicate whether traffic going over the
memory bus (DDR / DDR-T / CXL etc) is known to be encrypted or not.
EFI_MEMORY_CPU_CRYPTO does not communicate that property.

> 
> Martin Fernandez (5):
>   Extend memblock to support memory encryption
>   Extend pg_data_t to hold information about memory encryption
>   Extend e820_table to hold information about memory encryption
>   Mark e820_entries as crypto capable from EFI memmap
>   Show in sysfs if a memory node is able to do encryption
> 
>  Documentation/ABI/testing/sysfs-devices-node |  10 ++
>  arch/x86/include/asm/e820/api.h              |   2 +
>  arch/x86/include/asm/e820/types.h            |   1 +
>  arch/x86/kernel/e820.c                       |  32 +++++-
>  arch/x86/platform/efi/efi.c                  | 109 +++++++++++++++++++
>  drivers/base/node.c                          |  10 ++
>  include/linux/memblock.h                     |   6 +
>  include/linux/mmzone.h                       |   2 +
>  mm/memblock.c                                |  74 +++++++++++++
>  mm/page_alloc.c                              |   1 +
>  10 files changed, 245 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-devices-node
> 
> 
> base-commit: 3906fe9bb7f1a2c8667ae54e967dc8690824f4ea
> --
> 2.30.2
> 

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

[Dave Hansen]

On 11/6/21 2:35 PM, Williams, Dan J wrote:
> On Fri, 2021-11-05 at 18:27 -0300, Martin Fernandez wrote:
>> Show for each node if every memory descriptor in that node has the
>> EFI_MEMORY_CPU_CRYPTO attribute.
> 
> The problem I have with EFI_MEMORY_CPU_CRYPTO is it that is vague what
> memory encryption technology is deployed and does not tell you anything
> about whether it is in effect or not.

Would this be better if it were more detailed than a binary 0/1 for
being crypto-capable?  We do some pretty detailed descriptions of things
like:

> # cat /sys/devices/system/cpu/vulnerabilities/spectre_v2
> Mitigation: Full generic retpoline, IBPB: conditional, IBRS_FW, STIBP: conditional, RSB filling

We could do something in this case like:

# cat /sys/devices/system/node/node0/crypto_capable
Yes, EFI CPU Crypto Capable, TME active

Re: [PATCH 3/5] Extend e820_table to hold information about memory encryption

[Martin Fernandez]

On 11/5/21, Dave Hansen <dave.hansen@intel.com> wrote:
> On 11/5/21 2:27 PM, Martin Fernandez wrote:
>> +void __init e820__mark_regions_as_crypto_capable(u64 start, u64 size)
>> +{
>> +	int i;
>> +	u64 end = start + size;
>> +
>> +	for (i = 0; i < e820_table->nr_entries; i++) {
>> +		struct e820_entry *const entry = &e820_table->entries[i];
>> +
>> +		if (entry->addr >= start && entry->addr + entry->size <= end)
>> +			entry->crypto_capable = true;
>> +	}
>> +}
>
> Looking at this in isolation, this is really tricky.  I have no idea
> what this is _supposed_ to or expected to be doing.  It also makes me
> wonder what happens when start/size don't line up exactly on an e820 entry.

Do you think it's better to just add new entries, just as they are in
the EFI memmap and then let e820__update_table handle them?

Although, as it is it's faster, the other way would be clearer in the
code (since efi_mark_e820_regions_as_crypto_capable in part 4/5 isn't
also the nicest of the functions and with this change it would be very
straightforward), but it would require one e820__update_table. Also,
it would more accurate, since if you call this with a start and size
that doesn't cover at least one e820_entry then it will do nothing.

>>  static void __init e820_print_type(enum e820_type type)
>>  {
>>  	switch (type) {
>> @@ -211,6 +225,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);
>> +		pr_cont("%s",
>> +			e820_table->entries[i].crypto_capable ? "; crypto-capable" : "");
>
> Am I missing something or should this just be:
>
> 	if (e820_table->entries[i].crypto_capable)
> 		pr_cont("; crypto-capable");
>
> In general, I find code that retreats to the ternary form is almost
> always doing something nasty.

You're right, I'll fix that.

>> @@ -327,6 +343,8 @@ 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;
>> +	bool current_crypto;
>> +	bool last_crypto = false;
>>
>>  	/* If there's only one memory region, don't bother: */
>>  	if (table->nr_entries < 2)
>> @@ -388,13 +406,17 @@ int __init e820__update_table(struct e820_table
>> *table)
>>  		 * 1=usable, 2,3,4,4+=unusable)
>>  		 */
>>  		current_type = 0;
>> +		current_crypto = false;
>>  		for (i = 0; i < overlap_entries; i++) {
>> +			current_crypto = current_crypto || overlap_list[i]->crypto_capable;
>
> No comment, eh?
>
> This seems backwards to me.  If there are overlapping region and only
> one is crypto-capable, shouldn't the whole thing become non-crypto-capable?

The reason for that is that right now, if a region is mark as
crypto_capable is because EFI memmap says so, and again, right now
that's the only source to fill the crypto_capable value, so I have to
"believe" it. Now that I think about it, yes I should have a least put
a comment on it.

>>  		/* 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,6 +428,9 @@ 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_crypto = current_crypto;
>>  				last_addr = change_point[chg_idx]->addr;
>>  			}
>>  			last_type = current_type;
>
> The "current_crypto != last_crypto" checks seem to go with the
> current_type/last_type checks.  I'm naively surprised that the
> last_crypto assignment wasn't paired with the last_type assignment.
>
> I kinda get the impression this was just quickly hacked in here.  It
> seems like "crypto" and "type" are very closely related in how they are
> being handled.  It's a shame they're not being managed in a common way.

Yes, "crypto" and "type" seems really close, but to be honest, this
function has a very weird flow, or something that I couldn't
completely understand. After a while thinking about it I came up with
that.

Again, this function is a pain, but I'll dedicate it some more time to
see if I can come up with something better.

>> @@ -1321,7 +1346,10 @@ void __init e820__memblock_setup(void)
>>  		if (entry->type != E820_TYPE_RAM && entry->type !=
>> E820_TYPE_RESERVED_KERN)
>>  			continue;
>>
>> -		memblock_add(entry->addr, entry->size);
>> +		if (entry->crypto_capable)
>> +			memblock_add_crypto_capable(entry->addr, entry->size);
>> +		else
>> +			memblock_add(entry->addr, entry->size);
>
> Having a different memblock_add_foo() doesn't seem to be the way this is
> done.  See:
>
> 	memblock_mark_hotplug();
> or
> 	memblock_mark_mirror();
>
> Shouldn't this be: memblock_mark_crypto()

I thought it would be good to add it and mark it all together, but I
can add it and then mark it, no problem.

> By the way, how was this tested?

This was tested on my laptop that doesn't have the EFI attribute, so I
faked it, bypassing the check for this attribute in
efi_mark_e820_regions_as_crypto_capable (patch 4/5).

Re: [PATCH 3/5] Extend e820_table to hold information about memory encryption

[Dave Hansen]

On 11/8/21 10:40 AM, Martin Fernandez wrote:
> On 11/5/21, Dave Hansen <dave.hansen@intel.com> wrote:
>> On 11/5/21 2:27 PM, Martin Fernandez wrote:
>>> +void __init e820__mark_regions_as_crypto_capable(u64 start, u64 size)
>>> +{
>>> +	int i;
>>> +	u64 end = start + size;
>>> +
>>> +	for (i = 0; i < e820_table->nr_entries; i++) {
>>> +		struct e820_entry *const entry = &e820_table->entries[i];
>>> +
>>> +		if (entry->addr >= start && entry->addr + entry->size <= end)
>>> +			entry->crypto_capable = true;
>>> +	}
>>> +}
>>
>> Looking at this in isolation, this is really tricky.  I have no idea
>> what this is _supposed_ to or expected to be doing.  It also makes me
>> wonder what happens when start/size don't line up exactly on an e820 entry.
> 
> Do you think it's better to just add new entries, just as they are in
> the EFI memmap and then let e820__update_table handle them?
> 
> Although, as it is it's faster, the other way would be clearer in the
> code (since efi_mark_e820_regions_as_crypto_capable in part 4/5 isn't
> also the nicest of the functions and with this change it would be very
> straightforward), but it would require one e820__update_table. Also,
> it would more accurate, since if you call this with a start and size
> that doesn't cover at least one e820_entry then it will do nothing.

I was actually trying to make a comment about this function's lack of
documentation.

But, you make a good point about the alternate approach.  I don't have a
preference either way.


>>> @@ -327,6 +343,8 @@ 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;
>>> +	bool current_crypto;
>>> +	bool last_crypto = false;
>>>
>>>  	/* If there's only one memory region, don't bother: */
>>>  	if (table->nr_entries < 2)
>>> @@ -388,13 +406,17 @@ int __init e820__update_table(struct e820_table
>>> *table)
>>>  		 * 1=usable, 2,3,4,4+=unusable)
>>>  		 */
>>>  		current_type = 0;
>>> +		current_crypto = false;
>>>  		for (i = 0; i < overlap_entries; i++) {
>>> +			current_crypto = current_crypto || overlap_list[i]->crypto_capable;
>>
>> No comment, eh?
>>
>> This seems backwards to me.  If there are overlapping region and only
>> one is crypto-capable, shouldn't the whole thing become non-crypto-capable?
> 
> The reason for that is that right now, if a region is mark as
> crypto_capable is because EFI memmap says so, and again, right now
> that's the only source to fill the crypto_capable value, so I have to
> "believe" it. Now that I think about it, yes I should have a least put
> a comment on it.

My concern was if:

	current_crypto=0
and
	overlap_list[i]->crypto_capable=1

Doesn't that mean a non-crypto entry is being parsed, but current_crypto
will end up as 1?

>>>  		/* 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,6 +428,9 @@ 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_crypto = current_crypto;
>>>  				last_addr = change_point[chg_idx]->addr;
>>>  			}
>>>  			last_type = current_type;
>>
>> The "current_crypto != last_crypto" checks seem to go with the
>> current_type/last_type checks.  I'm naively surprised that the
>> last_crypto assignment wasn't paired with the last_type assignment.
>>
>> I kinda get the impression this was just quickly hacked in here.  It
>> seems like "crypto" and "type" are very closely related in how they are
>> being handled.  It's a shame they're not being managed in a common way.
> 
> Yes, "crypto" and "type" seems really close, but to be honest, this
> function has a very weird flow, or something that I couldn't
> completely understand. After a while thinking about it I came up with
> that.
> 
> Again, this function is a pain, but I'll dedicate it some more time to
> see if I can come up with something better.

Please do.  Like you said, there's probably more context that it would
be helpful to understand.

Re: [PATCH 3/5] Extend e820_table to hold information about memory encryption

[Martin Fernandez]

On 11/8/21, Dave Hansen <dave.hansen@intel.com> wrote:
> But, you make a good point about the alternate approach.  I don't have a
> preference either way.

I was going to do this, and remembered why I discarded this option.

There is already a function to map EFI memmap entries to the
e820_table called do_add_efi_memmap, but it is only called when you
add the kernel param "add_efi_memmap", or when the system has some
soft reserved entries in the EFI memmap. Do you know why there is such
kernel param? Why would you not want to have also the EFI memmap?
since e820 already has the mechanisms to deal with overlaps.

I first thought that it was a size issue, maybe the e820_table
couldn't hold both memmaps at the same time. But reading the comment
where the e820_table is defined makes think that's not an issue. Am I
missing something?