[v4,21/21] efi_loader: Expose U-Boot addresses in memory map for sandbox

Message ID 20180618152315.34233-22-agraf@suse.de
State New
Headers show
Series
  • sandbox: efi_loader support
Related show

Commit Message

Alexander Graf June 18, 2018, 3:23 p.m.
We currently expose host addresses in the EFI memory map. That can be
bad if we ever want to use sandbox to boot strap a real kernel, because
then the kernel would fetch its memory table from our host virtual address
map. But to make that use case work, we would need to have full control
over the address space the EFI application sees.

So let's expose only U-Boot addresses to the guest until we get to the
point of allocation. EFI's allocation functions are fun - they can take
U-Boot addresses as input values for hints and return host addresses as
allocation results through the same uint64_t * parameter. So we need to
be extra careful on what to pass in when.

With this patch I am successfully able to run the efi selftest suite as
well as grub.efi on aarch64.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/sandbox/cpu/cpu.c      | 19 -------------------
 lib/efi_loader/efi_memory.c | 12 ++++++------
 2 files changed, 6 insertions(+), 25 deletions(-)

Comments

Alexander Graf June 21, 2018, 3:13 p.m. | #1
> We currently expose host addresses in the EFI memory map. That can be
> bad if we ever want to use sandbox to boot strap a real kernel, because
> then the kernel would fetch its memory table from our host virtual address
> map. But to make that use case work, we would need to have full control
> over the address space the EFI application sees.
> 
> So let's expose only U-Boot addresses to the guest until we get to the
> point of allocation. EFI's allocation functions are fun - they can take
> U-Boot addresses as input values for hints and return host addresses as
> allocation results through the same uint64_t * parameter. So we need to
> be extra careful on what to pass in when.
> 
> With this patch I am successfully able to run the efi selftest suite as
> well as grub.efi on aarch64.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>

Thanks, applied to efi-next

Alex
Simon Glass June 23, 2018, 4:01 a.m. | #2
Hi Alex,

On 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
> We currently expose host addresses in the EFI memory map. That can be
> bad if we ever want to use sandbox to boot strap a real kernel, because
> then the kernel would fetch its memory table from our host virtual address
> map. But to make that use case work, we would need to have full control
> over the address space the EFI application sees.
>
> So let's expose only U-Boot addresses to the guest until we get to the
> point of allocation. EFI's allocation functions are fun - they can take
> U-Boot addresses as input values for hints and return host addresses as
> allocation results through the same uint64_t * parameter. So we need to
> be extra careful on what to pass in when.
>
> With this patch I am successfully able to run the efi selftest suite as
> well as grub.efi on aarch64.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/sandbox/cpu/cpu.c      | 19 -------------------
>  lib/efi_loader/efi_memory.c | 12 ++++++------
>  2 files changed, 6 insertions(+), 25 deletions(-)

Can you please point me to your latest series? I think you have
decided to work on this yourself and pick bits from my series that you
like. This I consider unpleasant behaviour for a maintainer, but
ultimately I'm more interested in getting things resolved than any
procedural issues. Please don't do this to anyone else, though, in the
U-Boot community.

Anyway, at present I'm not sure what state it is in, so please point
me to your latest tree so I can take a look and figure out what has
actually changed from my v9 series.

Regards,
Simon

Regards,
Simon
Alexander Graf June 23, 2018, 6:57 a.m. | #3
Hi Simon,

> Am 23.06.2018 um 06:01 schrieb Simon Glass <sjg@chromium.org>:
> 
> Hi Alex,
> 
>> On 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
>> We currently expose host addresses in the EFI memory map. That can be
>> bad if we ever want to use sandbox to boot strap a real kernel, because
>> then the kernel would fetch its memory table from our host virtual address
>> map. But to make that use case work, we would need to have full control
>> over the address space the EFI application sees.
>> 
>> So let's expose only U-Boot addresses to the guest until we get to the
>> point of allocation. EFI's allocation functions are fun - they can take
>> U-Boot addresses as input values for hints and return host addresses as
>> allocation results through the same uint64_t * parameter. So we need to
>> be extra careful on what to pass in when.
>> 
>> With this patch I am successfully able to run the efi selftest suite as
>> well as grub.efi on aarch64.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> arch/sandbox/cpu/cpu.c      | 19 -------------------
>> lib/efi_loader/efi_memory.c | 12 ++++++------
>> 2 files changed, 6 insertions(+), 25 deletions(-)
> 
> Can you please point me to your latest series? I think you have
> decided to work on this yourself and pick bits from my series that you
> like.

Believe me, I also picked things that I don't like. But ultimately sandbox is your court while efi_loader is mine. And I'm fairly sure both of us have better things to do than to run in circles.

> This I consider unpleasant behaviour for a maintainer, but
> ultimately I'm more interested in getting things resolved than any
> procedural issues. Please don't do this to anyone else, though, in the
> U-Boot community.

I don't see the problem - it's pretty common in the Linux world. You propose something, I counterpropose, we converge, maintainer decides what to pick up.

> Anyway, at present I'm not sure what state it is in, so please point
> me to your latest tree so I can take a look and figure out what has
> actually changed from my v9 series.

The current tree with v5 applied is here:

https://github.com/agraf/u-boot/tree/efi-sandbox-v5

Branch efi-next at the same location is the base for v5.


Alex
Simon Glass June 25, 2018, 2:58 a.m. | #4
Hi Alex,

On 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
>
> We currently expose host addresses in the EFI memory map. That can be
> bad if we ever want to use sandbox to boot strap a real kernel, because
> then the kernel would fetch its memory table from our host virtual address
> map. But to make that use case work, we would need to have full control
> over the address space the EFI application sees.
>
> So let's expose only U-Boot addresses to the guest until we get to the
> point of allocation. EFI's allocation functions are fun - they can take
> U-Boot addresses as input values for hints and return host addresses as
> allocation results through the same uint64_t * parameter. So we need to
> be extra careful on what to pass in when.
>
> With this patch I am successfully able to run the efi selftest suite as
> well as grub.efi on aarch64.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/sandbox/cpu/cpu.c      | 19 -------------------
>  lib/efi_loader/efi_memory.c | 12 ++++++------
>  2 files changed, 6 insertions(+), 25 deletions(-)

This seems to compete with a patch from my series, but it's not clear
to be what the overlap is.

>
> diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
> index 641b66a0a7..be88ab2f1c 100644
> --- a/arch/sandbox/cpu/cpu.c
> +++ b/arch/sandbox/cpu/cpu.c
> @@ -176,25 +176,6 @@ void longjmp(jmp_buf jmp, int ret)
>
>  #if CONFIG_IS_ENABLED(EFI_LOADER)
>
> -/*
> - * In sandbox, we don't have a 1:1 map, so we need to expose
> - * process addresses instead of U-Boot addresses
> - */
> -void efi_add_known_memory(void)
> -{
> -       u64 ram_start = (uintptr_t)map_sysmem(0, gd->ram_size);
> -       u64 ram_size = gd->ram_size;
> -       u64 start = (ram_start + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;
> -       u64 pages = (ram_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
> -
> -       efi_add_memory_map(start, pages, EFI_CONVENTIONAL_MEMORY,
> -                          false);
> -}
> -
> -#endif
> -
> -#if CONFIG_IS_ENABLED(EFI_LOADER)
> -
>  void allow_unaligned(void)
>  {
>         int r;
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 19492df518..64d2b8f7fa 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -326,7 +326,7 @@ efi_status_t efi_allocate_pages(int type, int memory_type,
>                 /* Reserve that map in our memory maps */
>                 ret = efi_add_memory_map(addr, pages, memory_type, true);
>                 if (ret == addr) {
> -                       *memory = addr;
> +                       *memory = (uintptr_t)map_sysmem(addr, pages * EFI_PAGE_SIZE);

This line does not seem to correspond to the code in your efi-sandbox-v5 tree.

Also if an address is passed in then presumably it needs
map_to_sysmem() before use (e.g. replace the *memory accesses in this
function).  I suspect those code paths have no tests which is why
things work.

Given that you have efi_allocate_pages() takes (and returns) a pointer
(but stored in a uin64_t) I think you are asking for confusion. At the
least this needs a comment to explain what is being returned.

Apart from that I think it looks right.

>                 } else {
>                         /* Map would overlap, bail out */
>                         r = EFI_OUT_OF_RESOURCES;
> @@ -360,11 +360,12 @@ void *efi_alloc(uint64_t len, int memory_type)
>  efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
>  {
>         uint64_t r = 0;
> +       uint64_t addr = map_to_sysmem((void*)(uintptr_t)memory);
>
> -       r = efi_add_memory_map(memory, pages, EFI_CONVENTIONAL_MEMORY, false);
> +       r = efi_add_memory_map(addr, pages, EFI_CONVENTIONAL_MEMORY, false);
>         /* Merging of adjacent free regions is missing */
>
> -       if (r == memory)
> +       if (r == addr)
>                 return EFI_SUCCESS;
>
>         return EFI_NOT_FOUND;
> @@ -381,9 +382,9 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
>  efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer)
>  {
>         efi_status_t r;
> -       efi_physical_addr_t t;
>         u64 num_pages = (size + sizeof(struct efi_pool_allocation) +
>                          EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
> +       struct efi_pool_allocation *alloc;
>
>         if (size == 0) {
>                 *buffer = NULL;
> @@ -391,10 +392,9 @@ efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer)
>         }
>
>         r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, pool_type, num_pages,
> -                              &t);
> +                              (uint64_t*)&alloc);
>
>         if (r == EFI_SUCCESS) {
> -               struct efi_pool_allocation *alloc = (void *)(uintptr_t)t;
>                 alloc->num_pages = num_pages;
>                 *buffer = alloc->data;
>         }
> --
> 2.12.3
>

Regards,
Simon
Simon Glass June 25, 2018, 2:58 a.m. | #5
Hi Alex,

On 23 June 2018 at 00:57, Alexander Graf <agraf@suse.de> wrote:
> Hi Simon,
>
> Am 23.06.2018 um 06:01 schrieb Simon Glass <sjg@chromium.org>:
>
> Hi Alex,
>
> On 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
>
> We currently expose host addresses in the EFI memory map. That can be
>
> bad if we ever want to use sandbox to boot strap a real kernel, because
>
> then the kernel would fetch its memory table from our host virtual address
>
> map. But to make that use case work, we would need to have full control
>
> over the address space the EFI application sees.
>
>
> So let's expose only U-Boot addresses to the guest until we get to the
>
> point of allocation. EFI's allocation functions are fun - they can take
>
> U-Boot addresses as input values for hints and return host addresses as
>
> allocation results through the same uint64_t * parameter. So we need to
>
> be extra careful on what to pass in when.
>
>
> With this patch I am successfully able to run the efi selftest suite as
>
> well as grub.efi on aarch64.
>
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
>
> ---
>
> arch/sandbox/cpu/cpu.c      | 19 -------------------
>
> lib/efi_loader/efi_memory.c | 12 ++++++------
>
> 2 files changed, 6 insertions(+), 25 deletions(-)
>
>
> Can you please point me to your latest series? I think you have
> decided to work on this yourself and pick bits from my series that you
> like.
>
>
> Believe me, I also picked things that I don't like. But ultimately sandbox
> is your court while efi_loader is mine. And I'm fairly sure both of us have
> better things to do than to run in circles.
>
> This I consider unpleasant behaviour for a maintainer, but
> ultimately I'm more interested in getting things resolved than any
> procedural issues. Please don't do this to anyone else, though, in the
> U-Boot community.
>
>
> I don't see the problem - it's pretty common in the Linux world. You propose
> something, I counterpropose, we converge, maintainer decides what to pick
> up.

This is certainly not Linux and I like to think we have a kinder and
more supportive environment here. I very seldom call out people on
this list for language and behaviour.

Also you are a maintainer, not another contributor.

>
> Anyway, at present I'm not sure what state it is in, so please point
> me to your latest tree so I can take a look and figure out what has
> actually changed from my v9 series.
>
>
> The current tree with v5 applied is here:
>
> https://github.com/agraf/u-boot/tree/efi-sandbox-v5
>
> Branch efi-next at the same location is the base for v5.

OK well at this stage I'm going to leave it in your hands as I've lost
track of all the patches and have no desire to send a v10 series.

Once everything lands I'll take a look and see if I think anything is
missing. Hopefully we can get sandbox EFi support into master when the
merge window opens.

Regards,
Simon

Patch

diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
index 641b66a0a7..be88ab2f1c 100644
--- a/arch/sandbox/cpu/cpu.c
+++ b/arch/sandbox/cpu/cpu.c
@@ -176,25 +176,6 @@  void longjmp(jmp_buf jmp, int ret)
 
 #if CONFIG_IS_ENABLED(EFI_LOADER)
 
-/*
- * In sandbox, we don't have a 1:1 map, so we need to expose
- * process addresses instead of U-Boot addresses
- */
-void efi_add_known_memory(void)
-{
-	u64 ram_start = (uintptr_t)map_sysmem(0, gd->ram_size);
-	u64 ram_size = gd->ram_size;
-	u64 start = (ram_start + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;
-	u64 pages = (ram_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
-
-	efi_add_memory_map(start, pages, EFI_CONVENTIONAL_MEMORY,
-			   false);
-}
-
-#endif
-
-#if CONFIG_IS_ENABLED(EFI_LOADER)
-
 void allow_unaligned(void)
 {
 	int r;
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 19492df518..64d2b8f7fa 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -326,7 +326,7 @@  efi_status_t efi_allocate_pages(int type, int memory_type,
 		/* Reserve that map in our memory maps */
 		ret = efi_add_memory_map(addr, pages, memory_type, true);
 		if (ret == addr) {
-			*memory = addr;
+			*memory = (uintptr_t)map_sysmem(addr, pages * EFI_PAGE_SIZE);
 		} else {
 			/* Map would overlap, bail out */
 			r = EFI_OUT_OF_RESOURCES;
@@ -360,11 +360,12 @@  void *efi_alloc(uint64_t len, int memory_type)
 efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
 {
 	uint64_t r = 0;
+	uint64_t addr = map_to_sysmem((void*)(uintptr_t)memory);
 
-	r = efi_add_memory_map(memory, pages, EFI_CONVENTIONAL_MEMORY, false);
+	r = efi_add_memory_map(addr, pages, EFI_CONVENTIONAL_MEMORY, false);
 	/* Merging of adjacent free regions is missing */
 
-	if (r == memory)
+	if (r == addr)
 		return EFI_SUCCESS;
 
 	return EFI_NOT_FOUND;
@@ -381,9 +382,9 @@  efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
 efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer)
 {
 	efi_status_t r;
-	efi_physical_addr_t t;
 	u64 num_pages = (size + sizeof(struct efi_pool_allocation) +
 			 EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
+	struct efi_pool_allocation *alloc;
 
 	if (size == 0) {
 		*buffer = NULL;
@@ -391,10 +392,9 @@  efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer)
 	}
 
 	r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, pool_type, num_pages,
-			       &t);
+			       (uint64_t*)&alloc);
 
 	if (r == EFI_SUCCESS) {
-		struct efi_pool_allocation *alloc = (void *)(uintptr_t)t;
 		alloc->num_pages = num_pages;
 		*buffer = alloc->data;
 	}