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

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


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

 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       | 25 +++++++++++++
 drivers/base/node.c               | 10 ++++++
 include/linux/memblock.h          |  5 +++
 include/linux/mmzone.h            |  3 ++
 mm/memblock.c                     | 49 +++++++++++++++++++++++++
 mm/page_alloc.c                   |  1 +
 9 files changed, 142 insertions(+), 12 deletions(-)

-- 
2.30.2

[PATCH v3 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 v3 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 v3 3/5] 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 v3 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 | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 147c30a81f15..38744fadcb9c 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,22 @@ 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 +512,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 v3 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>
---
 drivers/base/node.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

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 v3 3/5] Tag e820_entry with crypto capabilities

[Greg KH]

On Fri, Dec 03, 2021 at 04:21:46PM -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.

Shouldn't this Subject have the same prefix as patch 4/5?

thanks,

greg k-h

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

[Greg KH]

On Fri, Dec 03, 2021 at 04:21:48PM -0300, Martin Fernandez wrote:
> 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>
> ---
>  drivers/base/node.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> 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
>  };

You forgot a Documentation/ABI/ update for this new sysfs file you
added :(

Re: [PATCH v3 3/5] Tag e820_entry with crypto capabilities

[Mike Rapoport]

On Sat, Dec 04, 2021 at 09:21:51AM +0100, Greg KH wrote:
> On Fri, Dec 03, 2021 at 04:21:46PM -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.
> 
> Shouldn't this Subject have the same prefix as patch 4/5?

I'd say it should be x86/e820
 
> thanks,
> 
> greg k-h

-- 
Sincerely yours,
Mike.

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

[Martin Fernandez]

On 12/4/21, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> You forgot a Documentation/ABI/ update for this new sysfs file you
> added :(
>

Damn, I forgot to add it to the patch. It will be in my next patch,
this is what it looks like:

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

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

[Greg KH]

On Sat, Dec 04, 2021 at 01:35:15PM -0300, Martin Fernandez wrote:
> On 12/4/21, Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > You forgot a Documentation/ABI/ update for this new sysfs file you
> > added :(
> >
> 
> Damn, I forgot to add it to the patch. It will be in my next patch,
> this is what it looks like:
> 
> 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

October is long gone :(

> +Contact:	Martin Fernandez <martin.fernandez@eclypsium.com>
> +Users:		fwupd

Maybe a link to what 'fwupd' is?

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

thanks,

greg k-h

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

[Martin Fernandez]

On 12/4/21, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Sat, Dec 04, 2021 at 01:35:15PM -0300, Martin Fernandez wrote:
>> +Date:		October 2021
>
> October is long gone :(
>

:(

>> +Contact:	Martin Fernandez <martin.fernandez@eclypsium.com>
>> +Users:		fwupd
>
> Maybe a link to what 'fwupd' is?
>

Will add.

Thanks.

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

[Mike Rapoport]

Hi Martin,

On Fri, Dec 03, 2021 at 04:21:43PM -0300, Martin Fernandez wrote:
> 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.
 
I'm missing a description about *how* the new APIs/ABIs are going to be
used. This comment also applies to the changelogs of the patches that
mostly describe what the patch does and do not describe why is it needed.
 
> 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
>   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
> 
>  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       | 25 +++++++++++++
>  drivers/base/node.c               | 10 ++++++
>  include/linux/memblock.h          |  5 +++
>  include/linux/mmzone.h            |  3 ++
>  mm/memblock.c                     | 49 +++++++++++++++++++++++++
>  mm/page_alloc.c                   |  1 +
>  9 files changed, 142 insertions(+), 12 deletions(-)
> 
> -- 
> 2.30.2
> 

-- 
Sincerely yours,
Mike.

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

[Richard Hughes]

On Sun, 5 Dec 2021 at 06:04, Mike Rapoport <rppt@kernel.org> wrote:
> On Fri, Dec 03, 2021 at 04:21:43PM -0300, Martin Fernandez wrote:
> > fwupd project plans to use it as part of a check to see if the users
> > have properly configured memory hardware encryption capabilities.
> I'm missing a description about *how* the new APIs/ABIs are going to be
> used.

We're planning to use this feature in the Host Security ID checks done
at every boot. Please see
https://fwupd.github.io/libfwupdplugin/hsi.html for details. I'm happy
to answer questions or concerns. Thanks!

Richard

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

[Mike Rapoport]

Hi Richard,

On Mon, Dec 06, 2021 at 07:58:10PM +0000, Richard Hughes wrote:
> On Sun, 5 Dec 2021 at 06:04, Mike Rapoport <rppt@kernel.org> wrote:
> > On Fri, Dec 03, 2021 at 04:21:43PM -0300, Martin Fernandez wrote:
> > > fwupd project plans to use it as part of a check to see if the users
> > > have properly configured memory hardware encryption capabilities.
> > I'm missing a description about *how* the new APIs/ABIs are going to be
> > used.
> 
> We're planning to use this feature in the Host Security ID checks done
> at every boot. Please see
> https://fwupd.github.io/libfwupdplugin/hsi.html for details. I'm happy
> to answer questions or concerns. Thanks!

Can you please describe the actual check for the memory encryption and how
it would impact the HSI rating?

I wonder, for example, why did you choose per-node reporting rather than
per-region as described in UEFI spec.

-- 
Sincerely yours,
Mike.

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

[Martin Fernandez]

On 12/7/21, Mike Rapoport <rppt@kernel.org> wrote:
> Hi Richard,
>
> On Mon, Dec 06, 2021 at 07:58:10PM +0000, Richard Hughes wrote:
>> On Sun, 5 Dec 2021 at 06:04, Mike Rapoport <rppt@kernel.org> wrote:
>> > On Fri, Dec 03, 2021 at 04:21:43PM -0300, Martin Fernandez wrote:
>> > > fwupd project plans to use it as part of a check to see if the users
>> > > have properly configured memory hardware encryption capabilities.
>> > I'm missing a description about *how* the new APIs/ABIs are going to be
>> > used.
>>
>> We're planning to use this feature in the Host Security ID checks done
>> at every boot. Please see
>> https://fwupd.github.io/libfwupdplugin/hsi.html for details. I'm happy
>> to answer questions or concerns. Thanks!
>
> Can you please describe the actual check for the memory encryption and how
> it would impact the HSI rating?
>
> I wonder, for example, why did you choose per-node reporting rather than
> per-region as described in UEFI spec.

Some time ago we discussed about this and concluded with Dave Hansen
that it was better to do it in this per-node way.

This is the archive of the relevant discussion:
http://lkml.iu.edu/hypermail/linux/kernel/2006.2/06753.html

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

[Dave Hansen]

On 12/7/21 11:45 AM, Martin Fernandez wrote:
>> I wonder, for example, why did you choose per-node reporting rather than
>> per-region as described in UEFI spec.
> Some time ago we discussed about this and concluded with Dave Hansen
> that it was better to do it in this per-node way.

Physical memory regions aren't exposed to userspace in any meaningful way.

An ABI that says "everything is encrypted" is pretty meaningless and
only useful for this one, special case.

A per-node ABI is useful for this case and is also useful going forward
if folks want to target allocations from applications to NUMA nodes
which have encryption capabilities.  The ABI in this set is useful for
the immediate case and is useful to other folks.

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

[Mike Rapoport]

On Tue, Dec 07, 2021 at 11:52:54AM -0800, Dave Hansen wrote:
> On 12/7/21 11:45 AM, Martin Fernandez wrote:
> >> I wonder, for example, why did you choose per-node reporting rather than
> >> per-region as described in UEFI spec.
> > Some time ago we discussed about this and concluded with Dave Hansen
> > that it was better to do it in this per-node way.
> 
> Physical memory regions aren't exposed to userspace in any meaningful way.

Well, we have /sys/firmware/memory that exposes e820...
 
> An ABI that says "everything is encrypted" is pretty meaningless and
> only useful for this one, special case.
> 
> A per-node ABI is useful for this case and is also useful going forward
> if folks want to target allocations from applications to NUMA nodes
> which have encryption capabilities.  The ABI in this set is useful for
> the immediate case and is useful to other folks.

I don't mind per-node ABI, I'm just concerned that having a small region
without the encryption flag set will render the entire node "not
encryptable". This may happen because a bug in firmware, a user that shoot
themself in a leg with weird memmap= or some hidden gem in interaction
between e820, EFI and memblock that we still didn't discover.

I agree that per-node flag is useful, but maybe we should also have better
granularity as well.

-- 
Sincerely yours,
Mike.

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

[Dave Hansen]

On 12/7/21 12:06 PM, Mike Rapoport wrote:
>> An ABI that says "everything is encrypted" is pretty meaningless and
>> only useful for this one, special case.
>>
>> A per-node ABI is useful for this case and is also useful going forward
>> if folks want to target allocations from applications to NUMA nodes
>> which have encryption capabilities.  The ABI in this set is useful for
>> the immediate case and is useful to other folks.
> I don't mind per-node ABI, I'm just concerned that having a small region
> without the encryption flag set will render the entire node "not
> encryptable". This may happen because a bug in firmware, a user that shoot
> themself in a leg with weird memmap= or some hidden gem in interaction
> between e820, EFI and memblock that we still didn't discover.

That's a good point.  But, that seems more in the realm of a
pr_{info,warn}_once() than something deserving of its own specific ABI.

If we have a 100GB of a node that supports encryption, and 4k that
causes the whole thing to be considered un-encryptable, a warning is be
appropriate and feasible.

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

[Richard Hughes]

On Tue, 7 Dec 2021 at 07:25, Mike Rapoport <rppt@kernel.org> wrote:
> Can you please describe the actual check for the memory encryption and how
> it would impact the HSI rating?

The problem HSI is trying to solve is that customers are buying
systems where the CPU supports memory encryption, where the
motherboard and dram controller support memory encryption and where
the vendor says it's supported. But in some cases it's not working,
either because the system firmware is not working properly, or some
component requires updating to enable the feature. We're found quite a
few cases where people assumed this was all working fine, but on
looking closer, finding out that it's not working at all. The higher
HSI rating would only be available where most of the system RAM is
encrypted, although we've not worked out a heuristic number for "good
enough" yet.

> I wonder, for example, why did you choose per-node reporting rather than
> per-region as described in UEFI spec.

I think Dave is better to answer this question.

Richard.