[PATCH] efi_loader: consider no-map property of reserved memory

[PATCH] efi_loader: consider no-map property of reserved memory

[Heinrich Schuchardt]

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

[PATCH] efi_loader: consider no-map property of reserved memory

[Atish Patra]

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;
> +       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,

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);
> +       else
> +               list_add_tail(&newlist->link, &efi_mem);
>
>         /* And make sure memory is listed in descending order */
>         efi_mem_sort();
> --
> 2.28.0
>


-- 
Regards,
Atish

[PATCH] efi_loader: consider no-map property of reserved memory

[Heinrich Schuchardt]

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

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;
>> +       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,
>
> 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);
>> +       else
>> +               list_add_tail(&newlist->link, &efi_mem);
>>
>>         /* And make sure memory is listed in descending order */
>>         efi_mem_sort();
>> --
>> 2.28.0
>>
>
>

[PATCH] efi_loader: consider no-map property of reserved memory

[Atish Patra]

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.html
>
> 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;
> >> +       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,
> >
> > 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);
> >> +       else
> >> +               list_add_tail(&newlist->link, &efi_mem);
> >>
> >>         /* And make sure memory is listed in descending order */
> >>         efi_mem_sort();
> >> --
> >> 2.28.0
> >>
> >
> >
>


-- 
Regards,
Atish

[PATCH] efi_loader: consider no-map property of reserved memory

[Atish Patra]

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.html
> >
> > 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;
> > >> +       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,
> > >
> > > 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);
> > >> +       else
> > >> +               list_add_tail(&newlist->link, &efi_mem);
> > >>
> > >>         /* And make sure memory is listed in descending order */
> > >>         efi_mem_sort();
> > >> --
> > >> 2.28.0
> > >>
> > >
> > >
> >
>
>
> --
> Regards,
> Atish



-- 
Regards,
Atish

[PATCH] efi_loader: consider no-map property of reserved memory

[Heinrich Schuchardt]

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.html
>>
>> 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.html
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;
>>>> +       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,
>>>
>>> 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);
>>>> +       else
>>>> +               list_add_tail(&newlist->link, &efi_mem);
>>>>
>>>>          /* And make sure memory is listed in descending order */
>>>>          efi_mem_sort();
>>>> --
>>>> 2.28.0
>>>>
>>>
>>>
>>
>
>

[PATCH] efi_loader: consider no-map property of reserved memory

[Mark Kettenis]

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

[PATCH] efi_loader: consider no-map property of reserved memory

[Heinrich Schuchardt]

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

[PATCH] efi_loader: consider no-map property of reserved memory

[Atish Patra]

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


-- 
Regards,
Atish

[PATCH] efi_loader: consider no-map property of reserved memory

[Heinrich Schuchardt]

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

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