[PATCH] efi_loader: consider no-map property of reserved memory
If a reserved memory node in the device tree has the property no-map, remove it from the UEFI memory map provided by GetMemoryMap().
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- cmd/bootefi.c | 34 ++++++++++++++++++++++++++++------ include/efi.h | 3 +++ lib/efi_loader/efi_memory.c | 7 +++++-- 3 files changed, 36 insertions(+), 8 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 40d5ef2b3a..f173105251 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -135,12 +135,29 @@ done: return ret; }
-static void efi_reserve_memory(u64 addr, u64 size) +/** + * efi_reserve_memory() - add reserved memory to memory map + * + * @addr: start address of the reserved memory range + * @size: size of the reserved memory range + * @nomap: indicates that the memory range shall be hidden from the memory + * map + */ +static void efi_reserve_memory(u64 addr, u64 size, bool nomap) { + int type; + efi_uintn_t ret; + /* Convert from sandbox address space. */ addr = (uintptr_t)map_sysmem(addr, 0); - if (efi_add_memory_map(addr, size, - EFI_RESERVED_MEMORY_TYPE) != EFI_SUCCESS) + + if (nomap) + type = EFI_NO_MAP_MEMORY; + else + type = EFI_RESERVED_MEMORY_TYPE; + + ret = efi_add_memory_map(addr, size, type); + if (ret != EFI_SUCCESS) log_err("Reserved memory mapping failed addr %llx size %llx\n", addr, size); } @@ -166,7 +183,7 @@ static void efi_carve_out_dt_rsv(void *fdt) for (i = 0; i < nr_rsv; i++) { if (fdt_get_mem_rsv(fdt, i, &addr, &size) != 0) continue; - efi_reserve_memory(addr, size); + efi_reserve_memory(addr, size, false); }
/* process reserved-memory */ @@ -186,8 +203,13 @@ static void efi_carve_out_dt_rsv(void *fdt) * a size instead of a reg property. */ if (fdt_addr != FDT_ADDR_T_NONE && - fdtdec_get_is_enabled(fdt, subnode)) - efi_reserve_memory(fdt_addr, fdt_size); + fdtdec_get_is_enabled(fdt, subnode)) { + bool nomap; + + nomap = !!fdt_getprop(fdt, subnode, "no-map", + NULL); + efi_reserve_memory(fdt_addr, fdt_size, nomap); + } subnode = fdt_next_subnode(fdt, subnode); } } diff --git a/include/efi.h b/include/efi.h index f986aad877..50190021ef 100644 --- a/include/efi.h +++ b/include/efi.h @@ -181,6 +181,9 @@ enum efi_mem_type {
EFI_MAX_MEMORY_TYPE, EFI_TABLE_END, /* For efi_build_mem_table() */ + + /* Memory that shall not be mapped */ + EFI_NO_MAP_MEMORY, };
/* Attribute values */ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 7be756e370..d156b9533c 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -251,7 +251,7 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__, start, pages, memory_type, overlap_only_ram ? "yes" : "no");
- if (memory_type >= EFI_MAX_MEMORY_TYPE) + if (memory_type >= EFI_MAX_MEMORY_TYPE && memory_type != EFI_NO_MAP_MEMORY) return EFI_INVALID_PARAMETER;
if (!pages) @@ -327,7 +327,10 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, }
/* Add our new map */ - list_add_tail(&newlist->link, &efi_mem); + if (memory_type == EFI_NO_MAP_MEMORY) + free(newlist); + else + list_add_tail(&newlist->link, &efi_mem);
/* And make sure memory is listed in descending order */ efi_mem_sort(); -- 2.28.0
On Thu, Aug 27, 2020 at 9:16 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
If a reserved memory node in the device tree has the property no-map, remove it from the UEFI memory map provided by GetMemoryMap().
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
cmd/bootefi.c | 34 ++++++++++++++++++++++++++++------ include/efi.h | 3 +++ lib/efi_loader/efi_memory.c | 7 +++++-- 3 files changed, 36 insertions(+), 8 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 40d5ef2b3a..f173105251 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -135,12 +135,29 @@ done: return ret; }
-static void efi_reserve_memory(u64 addr, u64 size) +/**
- efi_reserve_memory() - add reserved memory to memory map
- @addr: start address of the reserved memory range
- @size: size of the reserved memory range
- @nomap: indicates that the memory range shall be hidden from the memory
map- */
+static void efi_reserve_memory(u64 addr, u64 size, bool nomap) {
int type;efi_uintn_t ret;/* Convert from sandbox address space. */ addr = (uintptr_t)map_sysmem(addr, 0);
if (efi_add_memory_map(addr, size,EFI_RESERVED_MEMORY_TYPE) != EFI_SUCCESS)
if (nomap)type = EFI_NO_MAP_MEMORY;elsetype = EFI_RESERVED_MEMORY_TYPE;ret = efi_add_memory_map(addr, size, type);if (ret != EFI_SUCCESS) log_err("Reserved memory mapping failed addr %llx size %llx\n", addr, size);} @@ -166,7 +183,7 @@ static void efi_carve_out_dt_rsv(void *fdt) for (i = 0; i < nr_rsv; i++) { if (fdt_get_mem_rsv(fdt, i, &addr, &size) != 0) continue;
efi_reserve_memory(addr, size);
efi_reserve_memory(addr, size, false); } /* process reserved-memory */@@ -186,8 +203,13 @@ static void efi_carve_out_dt_rsv(void *fdt) * a size instead of a reg property. */ if (fdt_addr != FDT_ADDR_T_NONE &&
fdtdec_get_is_enabled(fdt, subnode))efi_reserve_memory(fdt_addr, fdt_size);
fdtdec_get_is_enabled(fdt, subnode)) {bool nomap;nomap = !!fdt_getprop(fdt, subnode, "no-map",NULL);efi_reserve_memory(fdt_addr, fdt_size, nomap);} subnode = fdt_next_subnode(fdt, subnode); } }diff --git a/include/efi.h b/include/efi.h index f986aad877..50190021ef 100644 --- a/include/efi.h +++ b/include/efi.h @@ -181,6 +181,9 @@ enum efi_mem_type {
EFI_MAX_MEMORY_TYPE, EFI_TABLE_END, /* For efi_build_mem_table() */
/* Memory that shall not be mapped */EFI_NO_MAP_MEMORY,
Should it be named as EFI_NO_MAP_RSVD_MEMORY to avoid ambiguity ?
};
/* Attribute values */ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 7be756e370..d156b9533c 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -251,7 +251,7 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__, start, pages, memory_type, overlap_only_ram ? "yes" : "no");
if (memory_type >= EFI_MAX_MEMORY_TYPE)
if (memory_type >= EFI_MAX_MEMORY_TYPE && memory_type != EFI_NO_MAP_MEMORY) return EFI_INVALID_PARAMETER; if (!pages)@@ -327,7 +327,10 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, }
/* Add our new map */
list_add_tail(&newlist->link, &efi_mem);
if (memory_type == EFI_NO_MAP_MEMORY)free(newlist);elselist_add_tail(&newlist->link, &efi_mem); /* And make sure memory is listed in descending order */ efi_mem_sort();-- 2.28.0
On 31.08.20 20:08, Atish Patra wrote:
On Thu, Aug 27, 2020 at 9:16 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
If a reserved memory node in the device tree has the property no-map, remove it from the UEFI memory map provided by GetMemoryMap().
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
In the mail-thread starting a
[PATCH 1/1] EBBR: GetMemoryMap(), handling of no-map DT property https://lists.linaro.org/pipermail/boot-architecture/2020-September/001389.h...
the issue has been discussed. The conclusion was that we should not change the current behavior.
Best regards
Heinrich
cmd/bootefi.c | 34 ++++++++++++++++++++++++++++------ include/efi.h | 3 +++ lib/efi_loader/efi_memory.c | 7 +++++-- 3 files changed, 36 insertions(+), 8 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 40d5ef2b3a..f173105251 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -135,12 +135,29 @@ done: return ret; }
-static void efi_reserve_memory(u64 addr, u64 size) +/**
- efi_reserve_memory() - add reserved memory to memory map
- @addr: start address of the reserved memory range
- @size: size of the reserved memory range
- @nomap: indicates that the memory range shall be hidden from the memory
map- */
+static void efi_reserve_memory(u64 addr, u64 size, bool nomap) {
int type;efi_uintn_t ret;/* Convert from sandbox address space. */ addr = (uintptr_t)map_sysmem(addr, 0);
if (efi_add_memory_map(addr, size,EFI_RESERVED_MEMORY_TYPE) != EFI_SUCCESS)
if (nomap)type = EFI_NO_MAP_MEMORY;elsetype = EFI_RESERVED_MEMORY_TYPE;ret = efi_add_memory_map(addr, size, type);if (ret != EFI_SUCCESS) log_err("Reserved memory mapping failed addr %llx size %llx\n", addr, size);} @@ -166,7 +183,7 @@ static void efi_carve_out_dt_rsv(void *fdt) for (i = 0; i < nr_rsv; i++) { if (fdt_get_mem_rsv(fdt, i, &addr, &size) != 0) continue;
efi_reserve_memory(addr, size);
efi_reserve_memory(addr, size, false); } /* process reserved-memory */@@ -186,8 +203,13 @@ static void efi_carve_out_dt_rsv(void *fdt) * a size instead of a reg property. */ if (fdt_addr != FDT_ADDR_T_NONE &&
fdtdec_get_is_enabled(fdt, subnode))efi_reserve_memory(fdt_addr, fdt_size);
fdtdec_get_is_enabled(fdt, subnode)) {bool nomap;nomap = !!fdt_getprop(fdt, subnode, "no-map",NULL);efi_reserve_memory(fdt_addr, fdt_size, nomap);} subnode = fdt_next_subnode(fdt, subnode); } }diff --git a/include/efi.h b/include/efi.h index f986aad877..50190021ef 100644 --- a/include/efi.h +++ b/include/efi.h @@ -181,6 +181,9 @@ enum efi_mem_type {
EFI_MAX_MEMORY_TYPE, EFI_TABLE_END, /* For efi_build_mem_table() */
/* Memory that shall not be mapped */EFI_NO_MAP_MEMORY,Should it be named as EFI_NO_MAP_RSVD_MEMORY to avoid ambiguity ?
};
/* Attribute values */ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 7be756e370..d156b9533c 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -251,7 +251,7 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__, start, pages, memory_type, overlap_only_ram ? "yes" : "no");
if (memory_type >= EFI_MAX_MEMORY_TYPE)
if (memory_type >= EFI_MAX_MEMORY_TYPE && memory_type != EFI_NO_MAP_MEMORY) return EFI_INVALID_PARAMETER; if (!pages)@@ -327,7 +327,10 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, }
/* Add our new map */
list_add_tail(&newlist->link, &efi_mem);
if (memory_type == EFI_NO_MAP_MEMORY)free(newlist);elselist_add_tail(&newlist->link, &efi_mem); /* And make sure memory is listed in descending order */ efi_mem_sort();-- 2.28.0
On Wed, Sep 2, 2020 at 12:10 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 31.08.20 20:08, Atish Patra wrote:
On Thu, Aug 27, 2020 at 9:16 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
If a reserved memory node in the device tree has the property no-map, remove it from the UEFI memory map provided by GetMemoryMap().
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
In the mail-thread starting a
[PATCH 1/1] EBBR: GetMemoryMap(), handling of no-map DT property https://lists.linaro.org/pipermail/boot-architecture/2020-September/001389.h...
the issue has been discussed. The conclusion was that we should not change the current behavior.
Digging some old threads :)
The merged version of the patch marks any reserved memory as EFI_BOOT_SERVICES_DATA. AFAIK, these memory regions will be available after ExitBootservice. If the operating system chooses to map these region and access them, it violates the purpose of the reserved memory region specified by the firmware.
Did I miss something ?
Best regards
Heinrich
cmd/bootefi.c | 34 ++++++++++++++++++++++++++++------ include/efi.h | 3 +++ lib/efi_loader/efi_memory.c | 7 +++++-- 3 files changed, 36 insertions(+), 8 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 40d5ef2b3a..f173105251 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -135,12 +135,29 @@ done: return ret; }
-static void efi_reserve_memory(u64 addr, u64 size) +/**
- efi_reserve_memory() - add reserved memory to memory map
- @addr: start address of the reserved memory range
- @size: size of the reserved memory range
- @nomap: indicates that the memory range shall be hidden from the memory
map- */
+static void efi_reserve_memory(u64 addr, u64 size, bool nomap) {
int type;efi_uintn_t ret;/* Convert from sandbox address space. */ addr = (uintptr_t)map_sysmem(addr, 0);
if (efi_add_memory_map(addr, size,EFI_RESERVED_MEMORY_TYPE) != EFI_SUCCESS)
if (nomap)type = EFI_NO_MAP_MEMORY;elsetype = EFI_RESERVED_MEMORY_TYPE;ret = efi_add_memory_map(addr, size, type);if (ret != EFI_SUCCESS) log_err("Reserved memory mapping failed addr %llx size %llx\n", addr, size);} @@ -166,7 +183,7 @@ static void efi_carve_out_dt_rsv(void *fdt) for (i = 0; i < nr_rsv; i++) { if (fdt_get_mem_rsv(fdt, i, &addr, &size) != 0) continue;
efi_reserve_memory(addr, size);
efi_reserve_memory(addr, size, false); } /* process reserved-memory */@@ -186,8 +203,13 @@ static void efi_carve_out_dt_rsv(void *fdt) * a size instead of a reg property. */ if (fdt_addr != FDT_ADDR_T_NONE &&
fdtdec_get_is_enabled(fdt, subnode))efi_reserve_memory(fdt_addr, fdt_size);
fdtdec_get_is_enabled(fdt, subnode)) {bool nomap;nomap = !!fdt_getprop(fdt, subnode, "no-map",NULL);efi_reserve_memory(fdt_addr, fdt_size, nomap);} subnode = fdt_next_subnode(fdt, subnode); } }diff --git a/include/efi.h b/include/efi.h index f986aad877..50190021ef 100644 --- a/include/efi.h +++ b/include/efi.h @@ -181,6 +181,9 @@ enum efi_mem_type {
EFI_MAX_MEMORY_TYPE, EFI_TABLE_END, /* For efi_build_mem_table() */
/* Memory that shall not be mapped */EFI_NO_MAP_MEMORY,Should it be named as EFI_NO_MAP_RSVD_MEMORY to avoid ambiguity ?
};
/* Attribute values */ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 7be756e370..d156b9533c 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -251,7 +251,7 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__, start, pages, memory_type, overlap_only_ram ? "yes" : "no");
if (memory_type >= EFI_MAX_MEMORY_TYPE)
if (memory_type >= EFI_MAX_MEMORY_TYPE && memory_type != EFI_NO_MAP_MEMORY) return EFI_INVALID_PARAMETER; if (!pages)@@ -327,7 +327,10 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, }
/* Add our new map */
list_add_tail(&newlist->link, &efi_mem);
if (memory_type == EFI_NO_MAP_MEMORY)free(newlist);elselist_add_tail(&newlist->link, &efi_mem); /* And make sure memory is listed in descending order */ efi_mem_sort();-- 2.28.0
On Mon, May 10, 2021 at 4:47 PM Atish Patra atishp@atishpatra.org wrote:
On Wed, Sep 2, 2020 at 12:10 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 31.08.20 20:08, Atish Patra wrote:
On Thu, Aug 27, 2020 at 9:16 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
If a reserved memory node in the device tree has the property no-map, remove it from the UEFI memory map provided by GetMemoryMap().
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
In the mail-thread starting a
[PATCH 1/1] EBBR: GetMemoryMap(), handling of no-map DT property https://lists.linaro.org/pipermail/boot-architecture/2020-September/001389.h...
the issue has been discussed. The conclusion was that we should not change the current behavior.
Digging some old threads :)
The merged version of the patch marks any reserved memory as EFI_BOOT_SERVICES_DATA. AFAIK, these memory regions will be available after ExitBootservice. If the operating system chooses to map these region and access them, it violates the purpose of the reserved memory region specified by the firmware.
Did I miss something ?
ping ?
Best regards
Heinrich
cmd/bootefi.c | 34 ++++++++++++++++++++++++++++------ include/efi.h | 3 +++ lib/efi_loader/efi_memory.c | 7 +++++-- 3 files changed, 36 insertions(+), 8 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 40d5ef2b3a..f173105251 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -135,12 +135,29 @@ done: return ret; }
-static void efi_reserve_memory(u64 addr, u64 size) +/**
- efi_reserve_memory() - add reserved memory to memory map
- @addr: start address of the reserved memory range
- @size: size of the reserved memory range
- @nomap: indicates that the memory range shall be hidden from the memory
map- */
+static void efi_reserve_memory(u64 addr, u64 size, bool nomap) {
int type;efi_uintn_t ret;/* Convert from sandbox address space. */ addr = (uintptr_t)map_sysmem(addr, 0);
if (efi_add_memory_map(addr, size,EFI_RESERVED_MEMORY_TYPE) != EFI_SUCCESS)
if (nomap)type = EFI_NO_MAP_MEMORY;elsetype = EFI_RESERVED_MEMORY_TYPE;ret = efi_add_memory_map(addr, size, type);if (ret != EFI_SUCCESS) log_err("Reserved memory mapping failed addr %llx size %llx\n", addr, size);} @@ -166,7 +183,7 @@ static void efi_carve_out_dt_rsv(void *fdt) for (i = 0; i < nr_rsv; i++) { if (fdt_get_mem_rsv(fdt, i, &addr, &size) != 0) continue;
efi_reserve_memory(addr, size);
efi_reserve_memory(addr, size, false); } /* process reserved-memory */@@ -186,8 +203,13 @@ static void efi_carve_out_dt_rsv(void *fdt) * a size instead of a reg property. */ if (fdt_addr != FDT_ADDR_T_NONE &&
fdtdec_get_is_enabled(fdt, subnode))efi_reserve_memory(fdt_addr, fdt_size);
fdtdec_get_is_enabled(fdt, subnode)) {bool nomap;nomap = !!fdt_getprop(fdt, subnode, "no-map",NULL);efi_reserve_memory(fdt_addr, fdt_size, nomap);} subnode = fdt_next_subnode(fdt, subnode); } }diff --git a/include/efi.h b/include/efi.h index f986aad877..50190021ef 100644 --- a/include/efi.h +++ b/include/efi.h @@ -181,6 +181,9 @@ enum efi_mem_type {
EFI_MAX_MEMORY_TYPE, EFI_TABLE_END, /* For efi_build_mem_table() */
/* Memory that shall not be mapped */EFI_NO_MAP_MEMORY,Should it be named as EFI_NO_MAP_RSVD_MEMORY to avoid ambiguity ?
};
/* Attribute values */ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 7be756e370..d156b9533c 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -251,7 +251,7 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__, start, pages, memory_type, overlap_only_ram ? "yes" : "no");
if (memory_type >= EFI_MAX_MEMORY_TYPE)
if (memory_type >= EFI_MAX_MEMORY_TYPE && memory_type != EFI_NO_MAP_MEMORY) return EFI_INVALID_PARAMETER; if (!pages)@@ -327,7 +327,10 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, }
/* Add our new map */
list_add_tail(&newlist->link, &efi_mem);
if (memory_type == EFI_NO_MAP_MEMORY)free(newlist);elselist_add_tail(&newlist->link, &efi_mem); /* And make sure memory is listed in descending order */ efi_mem_sort();-- 2.28.0
-- Regards, Atish
On 5/11/21 1:47 AM, Atish Patra wrote:
On Wed, Sep 2, 2020 at 12:10 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 31.08.20 20:08, Atish Patra wrote:
On Thu, Aug 27, 2020 at 9:16 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
If a reserved memory node in the device tree has the property no-map, remove it from the UEFI memory map provided by GetMemoryMap().
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
In the mail-thread starting a
[PATCH 1/1] EBBR: GetMemoryMap(), handling of no-map DT property https://lists.linaro.org/pipermail/boot-architecture/2020-September/001389.h...
the issue has been discussed. The conclusion was that we should not change the current behavior.
Digging some old threads :)
The merged version of the patch marks any reserved memory as EFI_BOOT_SERVICES_DATA. AFAIK, these memory regions will be available after ExitBootservice. If the operating system chooses to map these region and access them, it violates the purpose of the reserved memory region specified by the firmware.
Did I miss something ?
The no-map property is neither described in the EBBR nor in the Devicetree specification v0.3 but only in Linux' reserved-memory.txt.
In https://lists.linaro.org/pipermail/boot-architecture/2020-September/001418.h... Ard requested that no-map memory shall be marked EfiReservedMemory because Linux will not map EfiReservedMemory, see Linux function is_usable_memory().
All reserved memory that is not marked as no-map shall be mapped by Linux. It may for instance serve as DMA pool or as video memory. Or it may even be reusable. See reserved-memory.txt.
Only drivers will access their own reserved memory if the region is not marked as reusable.
Best regards
Heinrich
Best regards
Heinrich
cmd/bootefi.c | 34 ++++++++++++++++++++++++++++------ include/efi.h | 3 +++ lib/efi_loader/efi_memory.c | 7 +++++-- 3 files changed, 36 insertions(+), 8 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 40d5ef2b3a..f173105251 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -135,12 +135,29 @@ done: return ret; }
-static void efi_reserve_memory(u64 addr, u64 size) +/**
- efi_reserve_memory() - add reserved memory to memory map
- @addr: start address of the reserved memory range
- @size: size of the reserved memory range
- @nomap: indicates that the memory range shall be hidden from the memory
map- */
+static void efi_reserve_memory(u64 addr, u64 size, bool nomap) {
int type;efi_uintn_t ret;/* Convert from sandbox address space. */ addr = (uintptr_t)map_sysmem(addr, 0);
if (efi_add_memory_map(addr, size,EFI_RESERVED_MEMORY_TYPE) != EFI_SUCCESS)
if (nomap)type = EFI_NO_MAP_MEMORY;elsetype = EFI_RESERVED_MEMORY_TYPE;ret = efi_add_memory_map(addr, size, type);if (ret != EFI_SUCCESS) log_err("Reserved memory mapping failed addr %llx size %llx\n", addr, size);@@ -166,7 +183,7 @@ static void efi_carve_out_dt_rsv(void *fdt) for (i = 0; i < nr_rsv; i++) { if (fdt_get_mem_rsv(fdt, i, &addr, &size) != 0) continue;
efi_reserve_memory(addr, size);
efi_reserve_memory(addr, size, false); } /* process reserved-memory */@@ -186,8 +203,13 @@ static void efi_carve_out_dt_rsv(void *fdt) * a size instead of a reg property. */ if (fdt_addr != FDT_ADDR_T_NONE &&
fdtdec_get_is_enabled(fdt, subnode))efi_reserve_memory(fdt_addr, fdt_size);
fdtdec_get_is_enabled(fdt, subnode)) {bool nomap;nomap = !!fdt_getprop(fdt, subnode, "no-map",NULL);efi_reserve_memory(fdt_addr, fdt_size, nomap);} subnode = fdt_next_subnode(fdt, subnode); } }diff --git a/include/efi.h b/include/efi.h index f986aad877..50190021ef 100644 --- a/include/efi.h +++ b/include/efi.h @@ -181,6 +181,9 @@ enum efi_mem_type {
EFI_MAX_MEMORY_TYPE, EFI_TABLE_END, /* For efi_build_mem_table() */
/* Memory that shall not be mapped */EFI_NO_MAP_MEMORY,Should it be named as EFI_NO_MAP_RSVD_MEMORY to avoid ambiguity ?
};
/* Attribute values */ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 7be756e370..d156b9533c 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -251,7 +251,7 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__, start, pages, memory_type, overlap_only_ram ? "yes" : "no");
if (memory_type >= EFI_MAX_MEMORY_TYPE)
if (memory_type >= EFI_MAX_MEMORY_TYPE && memory_type != EFI_NO_MAP_MEMORY) return EFI_INVALID_PARAMETER; if (!pages)@@ -327,7 +327,10 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, }
/* Add our new map */
list_add_tail(&newlist->link, &efi_mem);
if (memory_type == EFI_NO_MAP_MEMORY)free(newlist);elselist_add_tail(&newlist->link, &efi_mem); /* And make sure memory is listed in descending order */ efi_mem_sort();-- 2.28.0
From: Heinrich Schuchardt xypron.glpk@gmx.de Date: Thu, 13 May 2021 21:41:40 +0200
Hi Heinrich & Atish,
On 5/11/21 1:47 AM, Atish Patra wrote:
On Wed, Sep 2, 2020 at 12:10 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 31.08.20 20:08, Atish Patra wrote:
On Thu, Aug 27, 2020 at 9:16 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
If a reserved memory node in the device tree has the property no-map, remove it from the UEFI memory map provided by GetMemoryMap().
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
In the mail-thread starting a
[PATCH 1/1] EBBR: GetMemoryMap(), handling of no-map DT property https://lists.linaro.org/pipermail/boot-architecture/2020-September/001389.h...
the issue has been discussed. The conclusion was that we should not change the current behavior.
Digging some old threads :)
The merged version of the patch marks any reserved memory as EFI_BOOT_SERVICES_DATA. AFAIK, these memory regions will be available after ExitBootservice. If the operating system chooses to map these region and access them, it violates the purpose of the reserved memory region specified by the firmware.
Did I miss something ?
The no-map property is neither described in the EBBR nor in the Devicetree specification v0.3 but only in Linux' reserved-memory.txt.
It is described (quite explicitly) in the current devicetree specification draft.
In https://lists.linaro.org/pipermail/boot-architecture/2020-September/001418.h... Ard requested that no-map memory shall be marked EfiReservedMemory because Linux will not map EfiReservedMemory, see Linux function is_usable_memory().
All reserved memory that is not marked as no-map shall be mapped by Linux. It may for instance serve as DMA pool or as video memory. Or it may even be reusable. See reserved-memory.txt.
Only drivers will access their own reserved memory if the region is not marked as reusable.
I suspect Atish is asking because of the issue I opened for opensbi:
https://github.com/riscv/opensbi/issues/210
On many RISC-V platforms, opensbi runs in "machine mode" (somewhat equivalent to EL3 on ARMv8) and allocates some memory for itself. It sets up protections for this memory such that "supervisor mode" (somewhat equivalent to EL1 on ARMv8) can't access this memory.
Older versions of opensbi marked this memory area as "no-map", resulting in that memory area being marked as EfiReservedMemoryType, and evrything is fine.
However, according to reserved-memory.txt, "no-map" means the the area isn't supposed to be mapped by the OS at all. That is deemed undesirable since it prevents the OS from using a large 2MB or 1G page table entry to map the memory range that happens to include the memory reserved for opensbi. That is sub-optimal as it means the OS has to allocated more levels of page tables for this memory block and has to use 4K pages which will increase TLB pressure. So newer versions of opensbi dropped the "no-map" property resulting in the area being marked as EfiBootServicesData. But that is obviously wrong since the OS can't actually access that memory range.
Now somewhat by accident the Linux kernel didn't actually attempt to use this memory area so the issue wasn't noticed. But we're working on OpenBSD/riscv64 and when I added code to use the EFI memory map the OpenBSD kernel tried to use that inaccessable memory, which obviously didn't end well.
I suspect differences between the ARMv8 and RISC-V architecture are at play here. On ARMv8 "secure" memory areas like this are not supposed to be mapped as speculative access might trigger a fault. But on RISC-V mapping a "protected" memory area is fine as long as you don't try to actually access it.
So the question really is how opensbi can express that a certain memory area is reserved (and should end up as EfiReservedMemoryType in the EFI memory map) but that it is ok to map it. Possible solution include:
* Change the interpretation of the "no-map" property for RISC-V such that it is clear that the region in question can be mapped but can not be accessed. This only makes sense if the RISC-V architecture guarantees that creating a mapping for physical memory will never cause problems even if those areas can't be accessed.
* Invent a new property that conveys the desired semantics.
* Always flag memory ranges under /reserved-memory as EfiReserbedMemoryType unless that memory range is marked as "reusable".
Cheers,
Mark
On 5/13/21 11:53 PM, Mark Kettenis wrote:
From: Heinrich Schuchardt xypron.glpk@gmx.de Date: Thu, 13 May 2021 21:41:40 +0200
Hi Heinrich & Atish,
On 5/11/21 1:47 AM, Atish Patra wrote:
On Wed, Sep 2, 2020 at 12:10 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 31.08.20 20:08, Atish Patra wrote:
On Thu, Aug 27, 2020 at 9:16 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
If a reserved memory node in the device tree has the property no-map, remove it from the UEFI memory map provided by GetMemoryMap().
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
In the mail-thread starting a
[PATCH 1/1] EBBR: GetMemoryMap(), handling of no-map DT property https://lists.linaro.org/pipermail/boot-architecture/2020-September/001389.h...
the issue has been discussed. The conclusion was that we should not change the current behavior.
Digging some old threads :)
The merged version of the patch marks any reserved memory as EFI_BOOT_SERVICES_DATA. AFAIK, these memory regions will be available after ExitBootservice. If the operating system chooses to map these region and access them, it violates the purpose of the reserved memory region specified by the firmware.
Did I miss something ?
The no-map property is neither described in the EBBR nor in the Devicetree specification v0.3 but only in Linux' reserved-memory.txt.
It is described (quite explicitly) in the current devicetree specification draft.
In https://lists.linaro.org/pipermail/boot-architecture/2020-September/001418.h... Ard requested that no-map memory shall be marked EfiReservedMemory because Linux will not map EfiReservedMemory, see Linux function is_usable_memory().
All reserved memory that is not marked as no-map shall be mapped by Linux. It may for instance serve as DMA pool or as video memory. Or it may even be reusable. See reserved-memory.txt.
Only drivers will access their own reserved memory if the region is not marked as reusable.
I suspect Atish is asking because of the issue I opened for opensbi:
https://github.com/riscv/opensbi/issues/210
On many RISC-V platforms, opensbi runs in "machine mode" (somewhat equivalent to EL3 on ARMv8) and allocates some memory for itself. It sets up protections for this memory such that "supervisor mode" (somewhat equivalent to EL1 on ARMv8) can't access this memory.
Older versions of opensbi marked this memory area as "no-map", resulting in that memory area being marked as EfiReservedMemoryType, and evrything is fine.
However, according to reserved-memory.txt, "no-map" means the the area isn't supposed to be mapped by the OS at all. That is deemed undesirable since it prevents the OS from using a large 2MB or 1G page table entry to map the memory range that happens to include the memory reserved for opensbi. That is sub-optimal as it means the OS has to allocated more levels of page tables for this memory block and has to use 4K pages which will increase TLB pressure. So newer versions of opensbi dropped the "no-map" property resulting in the area being marked as EfiBootServicesData. But that is obviously wrong since the OS can't actually access that memory range.
Now somewhat by accident the Linux kernel didn't actually attempt to use this memory area so the issue wasn't noticed. But we're working on OpenBSD/riscv64 and when I added code to use the EFI memory map the OpenBSD kernel tried to use that inaccessable memory, which obviously didn't end well.
It is not by accident. The idea of reserved memory is that only a driver responsible for this reserved memory is allowed to access it.
Why should OpenBSD ever try to access reserved memory if there is no driver for it? Are you describing an OpenBSD bug?
Take this example from the MacchiatoBin device tree:
reserved-memory { #address-cells = <2>; #size-cells = <2>; ranges;
psci-area@4000000 { reg = <0x0 0x4000000 0x0 0x200000>; no-map; }; };
Any access in the reserved area will lead to a Linux crash. But this node is enough to keep Linux out.
I suspect differences between the ARMv8 and RISC-V architecture are at play here. On ARMv8 "secure" memory areas like this are not supposed to be mapped as speculative access might trigger a fault. But on RISC-V mapping a "protected" memory area is fine as long as you don't try to actually access it.
It might have been wiser to use no-map in the example above to rule out the possibility of speculative access.
So the question really is how opensbi can express that a certain memory area is reserved (and should end up as EfiReservedMemoryType in the EFI memory map) but that it is ok to map it. Possible solution include:
Just follow the draft device-tree spec.
If a reserved memory area can be mapped don't use EfiReservedMemoryType. Just mark the area as reserved in the device-tree and use EfiBootServicesData.
Make sure that OpenBSD follows the DT spec.
Best regards
Heinrich
Change the interpretation of the "no-map" property for RISC-V such that it is clear that the region in question can be mapped but can not be accessed. This only makes sense if the RISC-V architecture guarantees that creating a mapping for physical memory will never cause problems even if those areas can't be accessed.
Invent a new property that conveys the desired semantics.
Always flag memory ranges under /reserved-memory as EfiReserbedMemoryType unless that memory range is marked as "reusable".
Cheers,
Mark
On Fri, May 14, 2021 at 12:59 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 5/13/21 11:53 PM, Mark Kettenis wrote:
From: Heinrich Schuchardt xypron.glpk@gmx.de Date: Thu, 13 May 2021 21:41:40 +0200
Hi Heinrich & Atish,
On 5/11/21 1:47 AM, Atish Patra wrote:
On Wed, Sep 2, 2020 at 12:10 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 31.08.20 20:08, Atish Patra wrote:
On Thu, Aug 27, 2020 at 9:16 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote: > > If a reserved memory node in the device tree has the property no-map, > remove it from the UEFI memory map provided by GetMemoryMap(). > > Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
In the mail-thread starting a
[PATCH 1/1] EBBR: GetMemoryMap(), handling of no-map DT property https://lists.linaro.org/pipermail/boot-architecture/2020-September/001389.h...
the issue has been discussed. The conclusion was that we should not change the current behavior.
Digging some old threads :)
The merged version of the patch marks any reserved memory as EFI_BOOT_SERVICES_DATA. AFAIK, these memory regions will be available after ExitBootservice. If the operating system chooses to map these region and access them, it violates the purpose of the reserved memory region specified by the firmware.
Did I miss something ?
The no-map property is neither described in the EBBR nor in the Devicetree specification v0.3 but only in Linux' reserved-memory.txt.
It is described (quite explicitly) in the current devicetree specification draft.
In https://lists.linaro.org/pipermail/boot-architecture/2020-September/001418.h... Ard requested that no-map memory shall be marked EfiReservedMemory because Linux will not map EfiReservedMemory, see Linux function is_usable_memory().
All reserved memory that is not marked as no-map shall be mapped by Linux. It may for instance serve as DMA pool or as video memory. Or it may even be reusable. See reserved-memory.txt.
Only drivers will access their own reserved memory if the region is not marked as reusable.
I suspect Atish is asking because of the issue I opened for opensbi:
https://github.com/riscv/opensbi/issues/210
On many RISC-V platforms, opensbi runs in "machine mode" (somewhat equivalent to EL3 on ARMv8) and allocates some memory for itself. It sets up protections for this memory such that "supervisor mode" (somewhat equivalent to EL1 on ARMv8) can't access this memory.
Older versions of opensbi marked this memory area as "no-map", resulting in that memory area being marked as EfiReservedMemoryType, and evrything is fine.
However, according to reserved-memory.txt, "no-map" means the the area isn't supposed to be mapped by the OS at all. That is deemed undesirable since it prevents the OS from using a large 2MB or 1G page table entry to map the memory range that happens to include the memory reserved for opensbi. That is sub-optimal as it means the OS has to allocated more levels of page tables for this memory block and has to use 4K pages which will increase TLB pressure. So newer versions of opensbi dropped the "no-map" property resulting in the area being marked as EfiBootServicesData. But that is obviously wrong since the OS can't actually access that memory range.
Now somewhat by accident the Linux kernel didn't actually attempt to use this memory area so the issue wasn't noticed. But we're working on OpenBSD/riscv64 and when I added code to use the EFI memory map the OpenBSD kernel tried to use that inaccessable memory, which obviously didn't end well.
It is not by accident. The idea of reserved memory is that only a driver responsible for this reserved memory is allowed to access it.
What is the expected behavior if the firmware is adding reserved memory ? In this case, OpenSBI firmware adds the reserved memory node for the regions protected by PMP.
Once the proper kernel boots, it is not aware of the reserved memory anymore as it follows the efi memory mappings only. Any reserved regions without no-map property(by firmware) are marked as EfiBootServicesData. Linux kernel currently doesn't try to access those regions but can access anytime during the proper kernel boot. Isn't it ?
The obvious solution is to mark those regions as no-map however that degrades the performance as described by Mark. +Alexandre Ghiti (who pointed out the huge page mapping issue)
Is there a way to ensure that any reserved memory regions are mapped but not accessed ?
Why should OpenBSD ever try to access reserved memory if there is no driver for it? Are you describing an OpenBSD bug?
Take this example from the MacchiatoBin device tree:
reserved-memory { #address-cells = <2>; #size-cells = <2>; ranges; psci-area@4000000 { reg = <0x0 0x4000000 0x0 0x200000>; no-map; }; };Any access in the reserved area will lead to a Linux crash. But this node is enough to keep Linux out.
I suspect differences between the ARMv8 and RISC-V architecture are at play here. On ARMv8 "secure" memory areas like this are not supposed to be mapped as speculative access might trigger a fault. But on RISC-V mapping a "protected" memory area is fine as long as you don't try to actually access it.
It might have been wiser to use no-map in the example above to rule out the possibility of speculative access.
So the question really is how opensbi can express that a certain memory area is reserved (and should end up as EfiReservedMemoryType in the EFI memory map) but that it is ok to map it. Possible solution include:
Just follow the draft device-tree spec.
If a reserved memory area can be mapped don't use EfiReservedMemoryType. Just mark the area as reserved in the device-tree and use EfiBootServicesData.
Make sure that OpenBSD follows the DT spec.
Best regards
Heinrich
Change the interpretation of the "no-map" property for RISC-V such that it is clear that the region in question can be mapped but can not be accessed. This only makes sense if the RISC-V architecture guarantees that creating a mapping for physical memory will never cause problems even if those areas can't be accessed.
Invent a new property that conveys the desired semantics.
Always flag memory ranges under /reserved-memory as EfiReserbedMemoryType unless that memory range is marked as "reusable".
Cheers,
Mark
On 5/14/21 9:26 PM, Atish Patra wrote:
On Fri, May 14, 2021 at 12:59 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 5/13/21 11:53 PM, Mark Kettenis wrote:
From: Heinrich Schuchardt xypron.glpk@gmx.de Date: Thu, 13 May 2021 21:41:40 +0200
Hi Heinrich & Atish,
On 5/11/21 1:47 AM, Atish Patra wrote:
On Wed, Sep 2, 2020 at 12:10 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 31.08.20 20:08, Atish Patra wrote: > On Thu, Aug 27, 2020 at 9:16 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote: >> >> If a reserved memory node in the device tree has the property no-map, >> remove it from the UEFI memory map provided by GetMemoryMap(). >> >> Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
In the mail-thread starting a
[PATCH 1/1] EBBR: GetMemoryMap(), handling of no-map DT property https://lists.linaro.org/pipermail/boot-architecture/2020-September/001389.h...
the issue has been discussed. The conclusion was that we should not change the current behavior.
Digging some old threads :)
The merged version of the patch marks any reserved memory as EFI_BOOT_SERVICES_DATA. AFAIK, these memory regions will be available after ExitBootservice. If the operating system chooses to map these region and access them, it violates the purpose of the reserved memory region specified by the firmware.
Did I miss something ?
The no-map property is neither described in the EBBR nor in the Devicetree specification v0.3 but only in Linux' reserved-memory.txt.
It is described (quite explicitly) in the current devicetree specification draft.
In https://lists.linaro.org/pipermail/boot-architecture/2020-September/001418.h... Ard requested that no-map memory shall be marked EfiReservedMemory because Linux will not map EfiReservedMemory, see Linux function is_usable_memory().
All reserved memory that is not marked as no-map shall be mapped by Linux. It may for instance serve as DMA pool or as video memory. Or it may even be reusable. See reserved-memory.txt.
Only drivers will access their own reserved memory if the region is not marked as reusable.
I suspect Atish is asking because of the issue I opened for opensbi:
https://github.com/riscv/opensbi/issues/210On many RISC-V platforms, opensbi runs in "machine mode" (somewhat equivalent to EL3 on ARMv8) and allocates some memory for itself. It sets up protections for this memory such that "supervisor mode" (somewhat equivalent to EL1 on ARMv8) can't access this memory.
Older versions of opensbi marked this memory area as "no-map", resulting in that memory area being marked as EfiReservedMemoryType, and evrything is fine.
However, according to reserved-memory.txt, "no-map" means the the area isn't supposed to be mapped by the OS at all. That is deemed undesirable since it prevents the OS from using a large 2MB or 1G page table entry to map the memory range that happens to include the memory reserved for opensbi. That is sub-optimal as it means the OS has to allocated more levels of page tables for this memory block and has to use 4K pages which will increase TLB pressure. So newer versions of opensbi dropped the "no-map" property resulting in the area being marked as EfiBootServicesData. But that is obviously wrong since the OS can't actually access that memory range.
Now somewhat by accident the Linux kernel didn't actually attempt to use this memory area so the issue wasn't noticed. But we're working on OpenBSD/riscv64 and when I added code to use the EFI memory map the OpenBSD kernel tried to use that inaccessable memory, which obviously didn't end well.
It is not by accident. The idea of reserved memory is that only a driver responsible for this reserved memory is allowed to access it.
What is the expected behavior if the firmware is adding reserved memory ? In this case, OpenSBI firmware adds the reserved memory node for the regions protected by PMP.
The expected behavior is defined by the device-tree specification.
Once the proper kernel boots, it is not aware of the reserved memory anymore as it follows the efi memory mappings only.
No, it still has the device-tree to know what is reserved.
Any reserved regions without no-map property(by firmware) are marked as EfiBootServicesData. Linux kernel currently doesn't try to access those regions but can access anytime during the proper kernel boot. Isn't it ?
Should Linux ignore the device-tree you need to fix it there. This is nothing OpenSBI or U-Boot should care about.
Do you have any evidence of such a bug?
Best regards
Heinrich
The obvious solution is to mark those regions as no-map however that degrades the performance as described by Mark. +Alexandre Ghiti (who pointed out the huge page mapping issue)
Is there a way to ensure that any reserved memory regions are mapped but not accessed ?
Why should OpenBSD ever try to access reserved memory if there is no driver for it? Are you describing an OpenBSD bug?
Take this example from the MacchiatoBin device tree:
reserved-memory { #address-cells = <2>; #size-cells = <2>; ranges; psci-area@4000000 { reg = <0x0 0x4000000 0x0 0x200000>; no-map; }; };Any access in the reserved area will lead to a Linux crash. But this node is enough to keep Linux out.
I suspect differences between the ARMv8 and RISC-V architecture are at play here. On ARMv8 "secure" memory areas like this are not supposed to be mapped as speculative access might trigger a fault. But on RISC-V mapping a "protected" memory area is fine as long as you don't try to actually access it.
It might have been wiser to use no-map in the example above to rule out the possibility of speculative access.
So the question really is how opensbi can express that a certain memory area is reserved (and should end up as EfiReservedMemoryType in the EFI memory map) but that it is ok to map it. Possible solution include:
Just follow the draft device-tree spec.
If a reserved memory area can be mapped don't use EfiReservedMemoryType. Just mark the area as reserved in the device-tree and use EfiBootServicesData.
Make sure that OpenBSD follows the DT spec.
Best regards
Heinrich
Change the interpretation of the "no-map" property for RISC-V such that it is clear that the region in question can be mapped but can not be accessed. This only makes sense if the RISC-V architecture guarantees that creating a mapping for physical memory will never cause problems even if those areas can't be accessed.
Invent a new property that conveys the desired semantics.
Always flag memory ranges under /reserved-memory as EfiReserbedMemoryType unless that memory range is marked as "reusable".
Cheers,
Mark
participants (3)
-
Atish Patra -
Heinrich Schuchardt -
Mark Kettenis