Hi Alex,

On 14 June 2018 at 04:12, Alexander Graf agraf@suse.de wrote:

On 06/13/2018 04:37 AM, Simon Glass wrote:

...

With sandbox the U-Boot code is not mapped into the sandbox memory range so does not need to be excluded when allocating EFI memory. Update the EFI memory init code to take account of that.

Also use mapmem instead of a cast to convert a memory address to a pointer.

Signed-off-by: Simon Glass sjg@chromium.org

Changes in v6: None Changes in v5: None Changes in v4: None Changes in v3: None Changes in v2:

• Update to use mapmem instead of a cast

  lib/efi_loader/efi_memory.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index ec66af98ea..210e49ee8b 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -9,6 +9,7 @@ #include <efi_loader.h> #include <inttypes.h> #include <malloc.h> +#include <mapmem.h> #include <watchdog.h> #include <linux/list_sort.h> @@ -393,7 +394,7 @@ efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer) &t); if (r == EFI_SUCCESS) {

•           struct efi_pool_allocation *alloc = (void *)(uintptr_t)t;
  

•           struct efi_pool_allocation *alloc = map_sysmem(t, size);
  

This is still the wrong spot. We don't want the conversion to happen when going from an EFI internal address to an allocation, but when determining which addresses are usable in the first place.

There seem to be two ways to do this:

1. Record addresses (ulong) in the EFI tables and use map_sysmem() before returning an address in the allocator 2. Store pointers in the EFI tables using map_sysmem(), then do no mapping in the allocator

I've gone with option 1 since:

- the EFI addresses are not pointers - it makes sandbox consistent with other architectures which use an address rather than a pointer in EFI tables - we don't normally do pointer arithmetic on the results of map_sysmem() - we normally map the memory when it is used rather than when it is set up

I think you are asking for option 2. I think that would get very confusing. The addresses where things actually end up in sandbox are best kept to sandbox.

Overall I feel that you are either missing the point of sandbox addressing, or don't agree with how it is done. But it does work pretty well and we don't get a lot of confusion with sandbox pointers since we typically use the address until the last moment.

...

            alloc->num_pages = num_pages;
            *buffer = alloc->data;
    }

@@ -504,18 +505,22 @@ int efi_memory_init(void) efi_add_known_memory();

• /* Add U-Boot */
  

•   uboot_start = (gd->start_addr_sp - uboot_stack_size) &
  

~EFI_PAGE_MASK;

•   uboot_pages = (gd->ram_top - uboot_start) >> EFI_PAGE_SHIFT;
  

•   efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA,
  

false);

•   /* Add Runtime Services */
  

•   runtime_start = (ulong)&__efi_runtime_start & ~EFI_PAGE_MASK;
  

•   runtime_end = (ulong)&__efi_runtime_stop;
  

•   runtime_end = (runtime_end + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;
  

•   runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;
  

•   efi_add_memory_map(runtime_start, runtime_pages,
  

•                      EFI_RUNTIME_SERVICES_CODE, false);
  

•   if (!IS_ENABLED(CONFIG_SANDBOX)) {
  

•           /* Add U-Boot */
  

•           uboot_start = (gd->start_addr_sp - uboot_stack_size) &
  

•                           ~EFI_PAGE_MASK;
  

•           uboot_pages = (gd->ram_top - uboot_start) >>
  

EFI_PAGE_SHIFT;

•           efi_add_memory_map(uboot_start, uboot_pages,
  

EFI_LOADER_DATA,

•                              false);
  

Didn't we want to have these in a function?

Yes but I forgot. Will do for v7.

Regards, Simon

Hi Alex,

On 14 June 2018 at 07:41, Alexander Graf agraf@suse.de wrote:

On 06/14/2018 02:58 PM, Simon Glass wrote:

...

Hi Alex,

On 14 June 2018 at 04:12, Alexander Graf agraf@suse.de wrote:

...

On 06/13/2018 04:37 AM, Simon Glass wrote:

...

With sandbox the U-Boot code is not mapped into the sandbox memory range so does not need to be excluded when allocating EFI memory. Update the EFI memory init code to take account of that.

Also use mapmem instead of a cast to convert a memory address to a pointer.

Signed-off-by: Simon Glass sjg@chromium.org

Changes in v6: None Changes in v5: None Changes in v4: None Changes in v3: None Changes in v2:

• Update to use mapmem instead of a cast

  lib/efi_loader/efi_memory.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index ec66af98ea..210e49ee8b 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -9,6 +9,7 @@ #include <efi_loader.h> #include <inttypes.h> #include <malloc.h> +#include <mapmem.h> #include <watchdog.h> #include <linux/list_sort.h> @@ -393,7 +394,7 @@ efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer) &t); if (r == EFI_SUCCESS) {

•           struct efi_pool_allocation *alloc = (void
  

*)(uintptr_t)t;

•           struct efi_pool_allocation *alloc = map_sysmem(t, size);
  

This is still the wrong spot. We don't want the conversion to happen when going from an EFI internal address to an allocation, but when determining which addresses are usable in the first place.

There seem to be two ways to do this:

1. Record addresses (ulong) in the EFI tables and use map_sysmem()

before returning an address in the allocator 2. Store pointers in the EFI tables using map_sysmem(), then do no mapping in the allocator

I've gone with option 1 since:

• the EFI addresses are not pointers
• it makes sandbox consistent with other architectures which use an

address rather than a pointer in EFI tables

• we don't normally do pointer arithmetic on the results of map_sysmem()
• we normally map the memory when it is used rather than when it is set up

I think you are asking for option 2. I think that would get very confusing. The addresses where things actually end up in sandbox are best kept to sandbox.

Overall I feel that you are either missing the point of sandbox addressing, or don't agree with how it is done. But it does work pretty well and we don't get a lot of confusion with sandbox pointers since we typically use the address until the last moment.

I've assembled a quick tree for version 2. With that I'm able to run a simple hello world efi application. Grub refuses to start because it wants memory in the low 32bit and also emits random PIO accessing functions, which obviously don't work work from user space.

But overall, I think this is the right path to tackle this:

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

What do you mean by version 2? It looks like you've added one patch, so will you send that to the list?

Anyway, I hope I can convince you of the above, the way sandbox memory works.

Regards, Simon

Hi Alex,

On 14 June 2018 at 08:22, Alexander Graf agraf@suse.de wrote:

...

Am 14.06.2018 um 16:12 schrieb Simon Glass sjg@chromium.org:

Hi Alex,

...

On 14 June 2018 at 07:41, Alexander Graf agraf@suse.de wrote:

...

On 06/14/2018 02:58 PM, Simon Glass wrote:

Hi Alex,

...

On 14 June 2018 at 04:12, Alexander Graf agraf@suse.de wrote:

...

On 06/13/2018 04:37 AM, Simon Glass wrote:

With sandbox the U-Boot code is not mapped into the sandbox memory range so does not need to be excluded when allocating EFI memory. Update the EFI memory init code to take account of that.

Also use mapmem instead of a cast to convert a memory address to a pointer.

Signed-off-by: Simon Glass sjg@chromium.org

Changes in v6: None Changes in v5: None Changes in v4: None Changes in v3: None Changes in v2:

• Update to use mapmem instead of a cast

  lib/efi_loader/efi_memory.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index ec66af98ea..210e49ee8b 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -9,6 +9,7 @@ #include <efi_loader.h> #include <inttypes.h> #include <malloc.h> +#include <mapmem.h> #include <watchdog.h> #include <linux/list_sort.h> @@ -393,7 +394,7 @@ efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer) &t); if (r == EFI_SUCCESS) {

•           struct efi_pool_allocation *alloc = (void
  

*)(uintptr_t)t;

•           struct efi_pool_allocation *alloc = map_sysmem(t, size);
  

This is still the wrong spot. We don't want the conversion to happen when going from an EFI internal address to an allocation, but when determining which addresses are usable in the first place.

There seem to be two ways to do this:

1. Record addresses (ulong) in the EFI tables and use map_sysmem()

before returning an address in the allocator 2. Store pointers in the EFI tables using map_sysmem(), then do no mapping in the allocator

I've gone with option 1 since:

• the EFI addresses are not pointers
• it makes sandbox consistent with other architectures which use an

address rather than a pointer in EFI tables

• we don't normally do pointer arithmetic on the results of map_sysmem()
• we normally map the memory when it is used rather than when it is set up

I think you are asking for option 2. I think that would get very confusing. The addresses where things actually end up in sandbox are best kept to sandbox.

Overall I feel that you are either missing the point of sandbox addressing, or don't agree with how it is done. But it does work pretty well and we don't get a lot of confusion with sandbox pointers since we typically use the address until the last moment.

I've assembled a quick tree for version 2. With that I'm able to run a simple hello world efi application. Grub refuses to start because it wants memory in the low 32bit and also emits random PIO accessing functions, which obviously don't work work from user space.

But overall, I think this is the right path to tackle this:

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

What do you mean by version 2?

Option 2 is what you called it. It's the only option we have to make efi binaries work.

...

It looks like you've added one patch, so will you send that to the list?

It's more than 1 patch. And yes, I'll send them.

...

Anyway, I hope I can convince you of the above, the way sandbox memory works.

I still dislike option 1 :)

The reason is simple: The efi memory map is available to efi payloads. It's perfectly legal for them to do a static allocation at a particular address. We also share a lot of (host) pointers for callbacks and structs already with efi applications, so there is no real point to have a split brain situation between u-boot and host pointers.

OK so you mean they don't have to allocate memory before using it? How then do you make sure that there is no conflict? I thought EFI was an API?

Regards, Simon

Hi Alex,

On 14 June 2018 at 09:47, Alexander Graf agraf@suse.de wrote:

...

Am 14.06.2018 um 17:43 schrieb Simon Glass sjg@chromium.org:

Hi Alex,

...

On 14 June 2018 at 08:22, Alexander Graf agraf@suse.de wrote:

...

Am 14.06.2018 um 16:12 schrieb Simon Glass sjg@chromium.org:

Hi Alex,

...

...

On 14 June 2018 at 07:41, Alexander Graf agraf@suse.de wrote: On 06/14/2018 02:58 PM, Simon Glass wrote:

Hi Alex,

>> On 14 June 2018 at 04:12, Alexander Graf agraf@suse.de wrote: >> >> On 06/13/2018 04:37 AM, Simon Glass wrote: >> >> With sandbox the U-Boot code is not mapped into the sandbox memory range >> so does not need to be excluded when allocating EFI memory. Update the >> EFI >> memory init code to take account of that. >> >> Also use mapmem instead of a cast to convert a memory address to a >> pointer. >> >> Signed-off-by: Simon Glass sjg@chromium.org >> --- >> >> Changes in v6: None >> Changes in v5: None >> Changes in v4: None >> Changes in v3: None >> Changes in v2: >> - Update to use mapmem instead of a cast >> >> lib/efi_loader/efi_memory.c | 31 ++++++++++++++++++------------- >> 1 file changed, 18 insertions(+), 13 deletions(-) >> >> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c >> index ec66af98ea..210e49ee8b 100644 >> --- a/lib/efi_loader/efi_memory.c >> +++ b/lib/efi_loader/efi_memory.c >> @@ -9,6 +9,7 @@ >> #include <efi_loader.h> >> #include <inttypes.h> >> #include <malloc.h> >> +#include <mapmem.h> >> #include <watchdog.h> >> #include <linux/list_sort.h> >> @@ -393,7 +394,7 @@ efi_status_t efi_allocate_pool(int pool_type, >> efi_uintn_t size, void **buffer) >> &t); >> if (r == EFI_SUCCESS) { >> - struct efi_pool_allocation *alloc = (void >> *)(uintptr_t)t; >> + struct efi_pool_allocation *alloc = map_sysmem(t, size); > > > This is still the wrong spot. We don't want the conversion to happen when > going from an EFI internal address to an allocation, but when determining > which addresses are usable in the first place.

There seem to be two ways to do this:

1. Record addresses (ulong) in the EFI tables and use map_sysmem()

before returning an address in the allocator 2. Store pointers in the EFI tables using map_sysmem(), then do no mapping in the allocator

I've gone with option 1 since:

• the EFI addresses are not pointers
• it makes sandbox consistent with other architectures which use an

address rather than a pointer in EFI tables

• we don't normally do pointer arithmetic on the results of map_sysmem()
• we normally map the memory when it is used rather than when it is set up

I think you are asking for option 2. I think that would get very confusing. The addresses where things actually end up in sandbox are best kept to sandbox.

Overall I feel that you are either missing the point of sandbox addressing, or don't agree with how it is done. But it does work pretty well and we don't get a lot of confusion with sandbox pointers since we typically use the address until the last moment.

I've assembled a quick tree for version 2. With that I'm able to run a simple hello world efi application. Grub refuses to start because it wants memory in the low 32bit and also emits random PIO accessing functions, which obviously don't work work from user space.

But overall, I think this is the right path to tackle this:

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

What do you mean by version 2?

Option 2 is what you called it. It's the only option we have to make efi binaries work.

...

It looks like you've added one patch, so will you send that to the list?

It's more than 1 patch. And yes, I'll send them.

...

Anyway, I hope I can convince you of the above, the way sandbox memory works.

I still dislike option 1 :)

The reason is simple: The efi memory map is available to efi payloads. It's perfectly legal for them to do a static allocation at a particular address. We also share a lot of (host) pointers for callbacks and structs already with efi applications, so there is no real point to have a split brain situation between u-boot and host pointers.

OK so you mean they don't have to allocate memory before using it? How then do you make sure that there is no conflict? I thought EFI was an API?

You allocate it, but payloads expect that the address you pass in as address you allocate at is the pointer/address that gets returned.

Can you please point me to that? Are you referring to this call?

static efi_status_t EFIAPI efi_get_memory_map_ext( efi_uintn_t *memory_map_size, struct efi_mem_desc *memory_map, efi_uintn_t *map_key, efi_uintn_t *descriptor_size, uint32_t *descriptor_version)

If so, we can still support that.

In a nutshell, what you're proposing is that the efi quivalent of mmap(addr, MAP_FIXED) returns something != addr. That will break things.

I see this function:

efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer)

So there appears to be no way for people to specify the address they want. Is there another call somewhere?

NOTE: I am not proposing that the address is made available to the EFI application - that would always see the sandbox pointer.

Regards, Simon

Hi Alex,

On 14 June 2018 at 10:07, Alexander Graf agraf@suse.de wrote:

On 06/14/2018 05:53 PM, Simon Glass wrote:

...

Hi Alex,

On 14 June 2018 at 09:47, Alexander Graf agraf@suse.de wrote:

...

...

Am 14.06.2018 um 17:43 schrieb Simon Glass sjg@chromium.org:

Hi Alex,

...

On 14 June 2018 at 08:22, Alexander Graf agraf@suse.de wrote:

...

Am 14.06.2018 um 16:12 schrieb Simon Glass sjg@chromium.org:

Hi Alex,

>> On 14 June 2018 at 07:41, Alexander Graf agraf@suse.de wrote: >> On 06/14/2018 02:58 PM, Simon Glass wrote: >> >> Hi Alex, >> >>>> On 14 June 2018 at 04:12, Alexander Graf agraf@suse.de wrote: >>>> >>>> On 06/13/2018 04:37 AM, Simon Glass wrote: >>>> >>>> With sandbox the U-Boot code is not mapped into the sandbox memory >>>> range >>>> so does not need to be excluded when allocating EFI memory. Update >>>> the >>>> EFI >>>> memory init code to take account of that. >>>> >>>> Also use mapmem instead of a cast to convert a memory address to a >>>> pointer. >>>> >>>> Signed-off-by: Simon Glass sjg@chromium.org >>>> --- >>>> >>>> Changes in v6: None >>>> Changes in v5: None >>>> Changes in v4: None >>>> Changes in v3: None >>>> Changes in v2: >>>> - Update to use mapmem instead of a cast >>>> >>>> lib/efi_loader/efi_memory.c | 31 ++++++++++++++++++------------- >>>> 1 file changed, 18 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/lib/efi_loader/efi_memory.c >>>> b/lib/efi_loader/efi_memory.c >>>> index ec66af98ea..210e49ee8b 100644 >>>> --- a/lib/efi_loader/efi_memory.c >>>> +++ b/lib/efi_loader/efi_memory.c >>>> @@ -9,6 +9,7 @@ >>>> #include <efi_loader.h> >>>> #include <inttypes.h> >>>> #include <malloc.h> >>>> +#include <mapmem.h> >>>> #include <watchdog.h> >>>> #include <linux/list_sort.h> >>>> @@ -393,7 +394,7 @@ efi_status_t efi_allocate_pool(int >>>> pool_type, >>>> efi_uintn_t size, void **buffer) >>>> &t); >>>> if (r == EFI_SUCCESS) { >>>> - struct efi_pool_allocation *alloc = (void >>>> *)(uintptr_t)t; >>>> + struct efi_pool_allocation *alloc = map_sysmem(t, >>>> size); >>> >>> >>> This is still the wrong spot. We don't want the conversion to >>> happen when >>> going from an EFI internal address to an allocation, but when >>> determining >>> which addresses are usable in the first place. >> >> There seem to be two ways to do this: >> >> 1. Record addresses (ulong) in the EFI tables and use map_sysmem() >> before returning an address in the allocator >> 2. Store pointers in the EFI tables using map_sysmem(), then do no >> mapping in the allocator >> >> I've gone with option 1 since: >> >> - the EFI addresses are not pointers >> - it makes sandbox consistent with other architectures which use an >> address rather than a pointer in EFI tables >> - we don't normally do pointer arithmetic on the results of >> map_sysmem() >> - we normally map the memory when it is used rather than when it is >> set up >> >> I think you are asking for option 2. I think that would get very >> confusing. The addresses where things actually end up in sandbox are >> best kept to sandbox. >> >> Overall I feel that you are either missing the point of sandbox >> addressing, or don't agree with how it is done. But it does work >> pretty well and we don't get a lot of confusion with sandbox >> pointers >> since we typically use the address until the last moment. > > > I've assembled a quick tree for version 2. With that I'm able to run > a > simple hello world efi application. Grub refuses to start because it > wants > memory in the low 32bit and also emits random PIO accessing > functions, which > obviously don't work work from user space. > > But overall, I think this is the right path to tackle this: > > https://github.com/agraf/u-boot/tree/efi-sandbox

What do you mean by version 2?

Option 2 is what you called it. It's the only option we have to make efi binaries work.

...

It looks like you've added one patch, so will you send that to the list?

It's more than 1 patch. And yes, I'll send them.

...

Anyway, I hope I can convince you of the above, the way sandbox memory works.

I still dislike option 1 :)

The reason is simple: The efi memory map is available to efi payloads. It's perfectly legal for them to do a static allocation at a particular address. We also share a lot of (host) pointers for callbacks and structs already with efi applications, so there is no real point to have a split brain situation between u-boot and host pointers.

OK so you mean they don't have to allocate memory before using it? How then do you make sure that there is no conflict? I thought EFI was an API?

You allocate it, but payloads expect that the address you pass in as address you allocate at is the pointer/address that gets returned.

Can you please point me to that? Are you referring to this call?

static efi_status_t EFIAPI efi_get_memory_map_ext( efi_uintn_t *memory_map_size, struct efi_mem_desc *memory_map, efi_uintn_t *map_key, efi_uintn_t *descriptor_size, uint32_t *descriptor_version)

If so, we can still support that.

...

In a nutshell, what you're proposing is that the efi quivalent of mmap(addr, MAP_FIXED) returns something != addr. That will break things.

I see this function:

efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer)

So there appears to be no way for people to specify the address they want. Is there another call somewhere?

You can call efi_allocate_pages() with EFI_ALLOCATE_ADDRESS or even EFI_ALLOCATE_MAX_ADDRESS which you usually base on information you receive from efi_get_memory_map().

That's fine though, I'm not worried about that. It will work correctly, right?

...

NOTE: I am not proposing that the address is made available to the EFI application - that would always see the sandbox pointer.

In that case it's *much* easier to just always store host pointers in the efi memory map. Everything else will be very intrusive.

Yes there will be map_sysmem() and map_to_sysmem() calls at each boundary. That 'intrusion' is what we have everywhere in sandbox and it is how things work. But the nice thing is that the tables don't end up with 'private' sandbox addresses in them. Also see me comments above.

Regards, Simon

Hi Alex,

On 14 June 2018 at 10:26, Alexander Graf agraf@suse.de wrote:

On 06/14/2018 06:13 PM, Simon Glass wrote:

...

Hi Alex,

On 14 June 2018 at 10:07, Alexander Graf agraf@suse.de wrote:

...

On 06/14/2018 05:53 PM, Simon Glass wrote:

...

Hi Alex,

On 14 June 2018 at 09:47, Alexander Graf agraf@suse.de wrote:

...

...

Am 14.06.2018 um 17:43 schrieb Simon Glass sjg@chromium.org:

Hi Alex,

> On 14 June 2018 at 08:22, Alexander Graf agraf@suse.de wrote: > > > >> Am 14.06.2018 um 16:12 schrieb Simon Glass sjg@chromium.org: >> >> Hi Alex, >> >>>> On 14 June 2018 at 07:41, Alexander Graf agraf@suse.de wrote: >>>> On 06/14/2018 02:58 PM, Simon Glass wrote: >>>> >>>> Hi Alex, >>>> >>>>>> On 14 June 2018 at 04:12, Alexander Graf agraf@suse.de wrote: >>>>>> >>>>>> On 06/13/2018 04:37 AM, Simon Glass wrote: >>>>>> >>>>>> With sandbox the U-Boot code is not mapped into the sandbox >>>>>> memory >>>>>> range >>>>>> so does not need to be excluded when allocating EFI memory. >>>>>> Update >>>>>> the >>>>>> EFI >>>>>> memory init code to take account of that. >>>>>> >>>>>> Also use mapmem instead of a cast to convert a memory address to >>>>>> a >>>>>> pointer. >>>>>> >>>>>> Signed-off-by: Simon Glass sjg@chromium.org >>>>>> --- >>>>>> >>>>>> Changes in v6: None >>>>>> Changes in v5: None >>>>>> Changes in v4: None >>>>>> Changes in v3: None >>>>>> Changes in v2: >>>>>> - Update to use mapmem instead of a cast >>>>>> >>>>>> lib/efi_loader/efi_memory.c | 31 >>>>>> ++++++++++++++++++------------- >>>>>> 1 file changed, 18 insertions(+), 13 deletions(-) >>>>>> >>>>>> diff --git a/lib/efi_loader/efi_memory.c >>>>>> b/lib/efi_loader/efi_memory.c >>>>>> index ec66af98ea..210e49ee8b 100644 >>>>>> --- a/lib/efi_loader/efi_memory.c >>>>>> +++ b/lib/efi_loader/efi_memory.c >>>>>> @@ -9,6 +9,7 @@ >>>>>> #include <efi_loader.h> >>>>>> #include <inttypes.h> >>>>>> #include <malloc.h> >>>>>> +#include <mapmem.h> >>>>>> #include <watchdog.h> >>>>>> #include <linux/list_sort.h> >>>>>> @@ -393,7 +394,7 @@ efi_status_t efi_allocate_pool(int >>>>>> pool_type, >>>>>> efi_uintn_t size, void **buffer) >>>>>> &t); >>>>>> if (r == EFI_SUCCESS) { >>>>>> - struct efi_pool_allocation *alloc = (void >>>>>> *)(uintptr_t)t; >>>>>> + struct efi_pool_allocation *alloc = >>>>>> map_sysmem(t, >>>>>> size); >>>>> >>>>> >>>>> This is still the wrong spot. We don't want the conversion to >>>>> happen when >>>>> going from an EFI internal address to an allocation, but when >>>>> determining >>>>> which addresses are usable in the first place. >>>> >>>> There seem to be two ways to do this: >>>> >>>> 1. Record addresses (ulong) in the EFI tables and use map_sysmem() >>>> before returning an address in the allocator >>>> 2. Store pointers in the EFI tables using map_sysmem(), then do no >>>> mapping in the allocator >>>> >>>> I've gone with option 1 since: >>>> >>>> - the EFI addresses are not pointers >>>> - it makes sandbox consistent with other architectures which use >>>> an >>>> address rather than a pointer in EFI tables >>>> - we don't normally do pointer arithmetic on the results of >>>> map_sysmem() >>>> - we normally map the memory when it is used rather than when it >>>> is >>>> set up >>>> >>>> I think you are asking for option 2. I think that would get very >>>> confusing. The addresses where things actually end up in sandbox >>>> are >>>> best kept to sandbox. >>>> >>>> Overall I feel that you are either missing the point of sandbox >>>> addressing, or don't agree with how it is done. But it does work >>>> pretty well and we don't get a lot of confusion with sandbox >>>> pointers >>>> since we typically use the address until the last moment. >>> >>> >>> I've assembled a quick tree for version 2. With that I'm able to >>> run >>> a >>> simple hello world efi application. Grub refuses to start because >>> it >>> wants >>> memory in the low 32bit and also emits random PIO accessing >>> functions, which >>> obviously don't work work from user space. >>> >>> But overall, I think this is the right path to tackle this: >>> >>> https://github.com/agraf/u-boot/tree/efi-sandbox >> >> What do you mean by version 2? > > Option 2 is what you called it. It's the only option we have to make > efi binaries work. > >> It looks like you've added one patch, >> so will you send that to the list? > > It's more than 1 patch. And yes, I'll send them. > >> Anyway, I hope I can convince you of the above, the way sandbox >> memory >> works. > > I still dislike option 1 :) > > The reason is simple: The efi memory map is available to efi > payloads. > It's perfectly legal for them to do a static allocation at a > particular > address. We also share a lot of (host) pointers for callbacks and > structs > already with efi applications, so there is no real point to have a > split > brain situation between u-boot and host pointers.

OK so you mean they don't have to allocate memory before using it? How then do you make sure that there is no conflict? I thought EFI was an API?

You allocate it, but payloads expect that the address you pass in as address you allocate at is the pointer/address that gets returned.

Can you please point me to that? Are you referring to this call?

static efi_status_t EFIAPI efi_get_memory_map_ext( efi_uintn_t *memory_map_size, struct efi_mem_desc *memory_map, efi_uintn_t *map_key, efi_uintn_t *descriptor_size, uint32_t *descriptor_version)

If so, we can still support that.

...

In a nutshell, what you're proposing is that the efi quivalent of mmap(addr, MAP_FIXED) returns something != addr. That will break things.

I see this function:

efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer)

So there appears to be no way for people to specify the address they want. Is there another call somewhere?

You can call efi_allocate_pages() with EFI_ALLOCATE_ADDRESS or even EFI_ALLOCATE_MAX_ADDRESS which you usually base on information you receive from efi_get_memory_map().

That's fine though, I'm not worried about that. It will work correctly, right?

...

...

NOTE: I am not proposing that the address is made available to the EFI application - that would always see the sandbox pointer.

In that case it's *much* easier to just always store host pointers in the efi memory map. Everything else will be very intrusive.

Yes there will be map_sysmem() and map_to_sysmem() calls at each boundary. That 'intrusion' is what we have everywhere in sandbox and it is how things work. But the nice thing is that the tables don't end up with 'private' sandbox addresses in them. Also see me comments above.

I seem to be missing the point. efi_loader is just a tiny shim layer whose only purpose it is to interface between the binary efi interface and U-Boot internal code. So why shouldn't efi_loader treat addresses as pointers internally?

So how about this: I'll send out a patch set that works the way I think is the right way forward. With this I can at least run binaries generated by gnu-efi. Then you can send a patch on top with a proposal how you think the mapping should happen. My guess is that your patch will just clutter the code, but I'll be happy to be persuaded otherwise.

I'm just not keen on putting sandbox pointers in internal data structures, sorry. It's not the way it is supposed to work and it makes sandbox diffferent from all the other archs. Trying to minimise the number of calls to map_sysmem() is not really the goal here.

Regards, Simon

Hi Alex,

On 14 June 2018 at 10:42, Alexander Graf agraf@suse.de wrote:

On 06/14/2018 06:33 PM, Simon Glass wrote:

...

Hi Alex,

On 14 June 2018 at 10:26, Alexander Graf agraf@suse.de wrote:

...

On 06/14/2018 06:13 PM, Simon Glass wrote:

...

Hi Alex,

On 14 June 2018 at 10:07, Alexander Graf agraf@suse.de wrote:

...

On 06/14/2018 05:53 PM, Simon Glass wrote:

...

Hi Alex,

On 14 June 2018 at 09:47, Alexander Graf agraf@suse.de wrote: > > >> Am 14.06.2018 um 17:43 schrieb Simon Glass sjg@chromium.org: >> >> Hi Alex, >> >>> On 14 June 2018 at 08:22, Alexander Graf agraf@suse.de wrote: >>> >>> >>> >>>> Am 14.06.2018 um 16:12 schrieb Simon Glass sjg@chromium.org: >>>> >>>> Hi Alex, >>>> >>>>>> On 14 June 2018 at 07:41, Alexander Graf agraf@suse.de wrote: >>>>>> On 06/14/2018 02:58 PM, Simon Glass wrote: >>>>>> >>>>>> Hi Alex, >>>>>> >>>>>>>> On 14 June 2018 at 04:12, Alexander Graf agraf@suse.de >>>>>>>> wrote: >>>>>>>> >>>>>>>> On 06/13/2018 04:37 AM, Simon Glass wrote: >>>>>>>> >>>>>>>> With sandbox the U-Boot code is not mapped into the sandbox >>>>>>>> memory >>>>>>>> range >>>>>>>> so does not need to be excluded when allocating EFI memory. >>>>>>>> Update >>>>>>>> the >>>>>>>> EFI >>>>>>>> memory init code to take account of that. >>>>>>>> >>>>>>>> Also use mapmem instead of a cast to convert a memory address >>>>>>>> to >>>>>>>> a >>>>>>>> pointer. >>>>>>>> >>>>>>>> Signed-off-by: Simon Glass sjg@chromium.org >>>>>>>> --- >>>>>>>> >>>>>>>> Changes in v6: None >>>>>>>> Changes in v5: None >>>>>>>> Changes in v4: None >>>>>>>> Changes in v3: None >>>>>>>> Changes in v2: >>>>>>>> - Update to use mapmem instead of a cast >>>>>>>> >>>>>>>> lib/efi_loader/efi_memory.c | 31 >>>>>>>> ++++++++++++++++++------------- >>>>>>>> 1 file changed, 18 insertions(+), 13 deletions(-) >>>>>>>> >>>>>>>> diff --git a/lib/efi_loader/efi_memory.c >>>>>>>> b/lib/efi_loader/efi_memory.c >>>>>>>> index ec66af98ea..210e49ee8b 100644 >>>>>>>> --- a/lib/efi_loader/efi_memory.c >>>>>>>> +++ b/lib/efi_loader/efi_memory.c >>>>>>>> @@ -9,6 +9,7 @@ >>>>>>>> #include <efi_loader.h> >>>>>>>> #include <inttypes.h> >>>>>>>> #include <malloc.h> >>>>>>>> +#include <mapmem.h> >>>>>>>> #include <watchdog.h> >>>>>>>> #include <linux/list_sort.h> >>>>>>>> @@ -393,7 +394,7 @@ efi_status_t efi_allocate_pool(int >>>>>>>> pool_type, >>>>>>>> efi_uintn_t size, void **buffer) >>>>>>>> &t); >>>>>>>> if (r == EFI_SUCCESS) { >>>>>>>> - struct efi_pool_allocation *alloc = (void >>>>>>>> *)(uintptr_t)t; >>>>>>>> + struct efi_pool_allocation *alloc = >>>>>>>> map_sysmem(t, >>>>>>>> size); >>>>>>> >>>>>>> >>>>>>> This is still the wrong spot. We don't want the conversion to >>>>>>> happen when >>>>>>> going from an EFI internal address to an allocation, but when >>>>>>> determining >>>>>>> which addresses are usable in the first place. >>>>>> >>>>>> There seem to be two ways to do this: >>>>>> >>>>>> 1. Record addresses (ulong) in the EFI tables and use >>>>>> map_sysmem() >>>>>> before returning an address in the allocator >>>>>> 2. Store pointers in the EFI tables using map_sysmem(), then do >>>>>> no >>>>>> mapping in the allocator >>>>>> >>>>>> I've gone with option 1 since: >>>>>> >>>>>> - the EFI addresses are not pointers >>>>>> - it makes sandbox consistent with other architectures which use >>>>>> an >>>>>> address rather than a pointer in EFI tables >>>>>> - we don't normally do pointer arithmetic on the results of >>>>>> map_sysmem() >>>>>> - we normally map the memory when it is used rather than when it >>>>>> is >>>>>> set up >>>>>> >>>>>> I think you are asking for option 2. I think that would get very >>>>>> confusing. The addresses where things actually end up in sandbox >>>>>> are >>>>>> best kept to sandbox. >>>>>> >>>>>> Overall I feel that you are either missing the point of sandbox >>>>>> addressing, or don't agree with how it is done. But it does work >>>>>> pretty well and we don't get a lot of confusion with sandbox >>>>>> pointers >>>>>> since we typically use the address until the last moment. >>>>> >>>>> >>>>> I've assembled a quick tree for version 2. With that I'm able to >>>>> run >>>>> a >>>>> simple hello world efi application. Grub refuses to start because >>>>> it >>>>> wants >>>>> memory in the low 32bit and also emits random PIO accessing >>>>> functions, which >>>>> obviously don't work work from user space. >>>>> >>>>> But overall, I think this is the right path to tackle this: >>>>> >>>>> https://github.com/agraf/u-boot/tree/efi-sandbox >>>> >>>> What do you mean by version 2? >>> >>> Option 2 is what you called it. It's the only option we have to >>> make >>> efi binaries work. >>> >>>> It looks like you've added one patch, >>>> so will you send that to the list? >>> >>> It's more than 1 patch. And yes, I'll send them. >>> >>>> Anyway, I hope I can convince you of the above, the way sandbox >>>> memory >>>> works. >>> >>> I still dislike option 1 :) >>> >>> The reason is simple: The efi memory map is available to efi >>> payloads. >>> It's perfectly legal for them to do a static allocation at a >>> particular >>> address. We also share a lot of (host) pointers for callbacks and >>> structs >>> already with efi applications, so there is no real point to have a >>> split >>> brain situation between u-boot and host pointers. >> >> OK so you mean they don't have to allocate memory before using it? >> How >> then do you make sure that there is no conflict? I thought EFI was >> an >> API? > > You allocate it, but payloads expect that the address you pass in as > address you allocate at is the pointer/address that gets returned. > Can you please point me to that? Are you referring to this call?

static efi_status_t EFIAPI efi_get_memory_map_ext( efi_uintn_t *memory_map_size, struct efi_mem_desc *memory_map, efi_uintn_t *map_key, efi_uintn_t *descriptor_size, uint32_t *descriptor_version)

If so, we can still support that.

> In a nutshell, what you're proposing is that the efi quivalent of > mmap(addr, MAP_FIXED) returns something != addr. That will break > things.

I see this function:

efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer)

So there appears to be no way for people to specify the address they want. Is there another call somewhere?

You can call efi_allocate_pages() with EFI_ALLOCATE_ADDRESS or even EFI_ALLOCATE_MAX_ADDRESS which you usually base on information you receive from efi_get_memory_map().

That's fine though, I'm not worried about that. It will work correctly, right?

...

...

NOTE: I am not proposing that the address is made available to the EFI application - that would always see the sandbox pointer.

In that case it's *much* easier to just always store host pointers in the efi memory map. Everything else will be very intrusive.

Yes there will be map_sysmem() and map_to_sysmem() calls at each boundary. That 'intrusion' is what we have everywhere in sandbox and it is how things work. But the nice thing is that the tables don't end up with 'private' sandbox addresses in them. Also see me comments above.

I seem to be missing the point. efi_loader is just a tiny shim layer whose only purpose it is to interface between the binary efi interface and U-Boot internal code. So why shouldn't efi_loader treat addresses as pointers internally?

So how about this: I'll send out a patch set that works the way I think is the right way forward. With this I can at least run binaries generated by gnu-efi. Then you can send a patch on top with a proposal how you think the mapping should happen. My guess is that your patch will just clutter the code, but I'll be happy to be persuaded otherwise.

I'm just not keen on putting sandbox pointers in internal data structures, sorry. It's not the way it is supposed to work and it makes sandbox diffferent from all the other archs. Trying to minimise the number of calls to map_sysmem() is not really the goal here.

Well, you're not the one who needs to maintain efi_loader code after every 2nd line in it ends up being some random map/unmap call ;).

But my proposal was serious. Putting virtual addresses in is trivial and gives us proper API interfaces. We can based on that still decide whether it's better to not have pointers in the map. I am not convinced yet, but open to the idea.

And yes, code maintainability is my uttermost goal of this. Things have to be simple for sandbox, otherwise they will break.

A virtual address has a particular meaning. This is not what is going on with sandbox...

Making sandbox store its internal addresses in an EFI table is not a good idea. I've tried to explain why. It is not more maintainable - it just adds confusion. At present all ulong addresses are normal U-Boot addresses and pointers point into the emulated RAM. It's pretty clear for people. Please don't change that for EFI.

Can we get my patches applied and then add yours on top? I will send v7 which I think addresses all comments.

Regards, Simon

Hi Alex,

On 14 June 2018 at 11:08, Alexander Graf agraf@suse.de wrote:

On 06/14/2018 06:55 PM, Simon Glass wrote:

...

Hi Alex,

On 14 June 2018 at 10:42, Alexander Graf agraf@suse.de wrote:

...

On 06/14/2018 06:33 PM, Simon Glass wrote:

...

Hi Alex,

On 14 June 2018 at 10:26, Alexander Graf agraf@suse.de wrote:

...

On 06/14/2018 06:13 PM, Simon Glass wrote:

...

Hi Alex,

On 14 June 2018 at 10:07, Alexander Graf agraf@suse.de wrote: > > On 06/14/2018 05:53 PM, Simon Glass wrote: >> >> Hi Alex, >> >> On 14 June 2018 at 09:47, Alexander Graf agraf@suse.de wrote: >>> >>> >>>> Am 14.06.2018 um 17:43 schrieb Simon Glass sjg@chromium.org: >>>> >>>> Hi Alex, >>>> >>>>> On 14 June 2018 at 08:22, Alexander Graf agraf@suse.de wrote: >>>>> >>>>> >>>>> >>>>>> Am 14.06.2018 um 16:12 schrieb Simon Glass sjg@chromium.org: >>>>>> >>>>>> Hi Alex, >>>>>> >>>>>>>> On 14 June 2018 at 07:41, Alexander Graf agraf@suse.de wrote: >>>>>>>> On 06/14/2018 02:58 PM, Simon Glass wrote: >>>>>>>> >>>>>>>> Hi Alex, >>>>>>>> >>>>>>>>>> On 14 June 2018 at 04:12, Alexander Graf agraf@suse.de >>>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> On 06/13/2018 04:37 AM, Simon Glass wrote: >>>>>>>>>> >>>>>>>>>> With sandbox the U-Boot code is not mapped into the sandbox >>>>>>>>>> memory >>>>>>>>>> range >>>>>>>>>> so does not need to be excluded when allocating EFI memory. >>>>>>>>>> Update >>>>>>>>>> the >>>>>>>>>> EFI >>>>>>>>>> memory init code to take account of that. >>>>>>>>>> >>>>>>>>>> Also use mapmem instead of a cast to convert a memory address >>>>>>>>>> to >>>>>>>>>> a >>>>>>>>>> pointer. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Simon Glass sjg@chromium.org >>>>>>>>>> --- >>>>>>>>>> >>>>>>>>>> Changes in v6: None >>>>>>>>>> Changes in v5: None >>>>>>>>>> Changes in v4: None >>>>>>>>>> Changes in v3: None >>>>>>>>>> Changes in v2: >>>>>>>>>> - Update to use mapmem instead of a cast >>>>>>>>>> >>>>>>>>>> lib/efi_loader/efi_memory.c | 31 >>>>>>>>>> ++++++++++++++++++------------- >>>>>>>>>> 1 file changed, 18 insertions(+), 13 deletions(-) >>>>>>>>>> >>>>>>>>>> diff --git a/lib/efi_loader/efi_memory.c >>>>>>>>>> b/lib/efi_loader/efi_memory.c >>>>>>>>>> index ec66af98ea..210e49ee8b 100644 >>>>>>>>>> --- a/lib/efi_loader/efi_memory.c >>>>>>>>>> +++ b/lib/efi_loader/efi_memory.c >>>>>>>>>> @@ -9,6 +9,7 @@ >>>>>>>>>> #include <efi_loader.h> >>>>>>>>>> #include <inttypes.h> >>>>>>>>>> #include <malloc.h> >>>>>>>>>> +#include <mapmem.h> >>>>>>>>>> #include <watchdog.h> >>>>>>>>>> #include <linux/list_sort.h> >>>>>>>>>> @@ -393,7 +394,7 @@ efi_status_t efi_allocate_pool(int >>>>>>>>>> pool_type, >>>>>>>>>> efi_uintn_t size, void **buffer) >>>>>>>>>> &t); >>>>>>>>>> if (r == EFI_SUCCESS) { >>>>>>>>>> - struct efi_pool_allocation *alloc = (void >>>>>>>>>> *)(uintptr_t)t; >>>>>>>>>> + struct efi_pool_allocation *alloc = >>>>>>>>>> map_sysmem(t, >>>>>>>>>> size); >>>>>>>>> >>>>>>>>> >>>>>>>>> This is still the wrong spot. We don't want the conversion to >>>>>>>>> happen when >>>>>>>>> going from an EFI internal address to an allocation, but when >>>>>>>>> determining >>>>>>>>> which addresses are usable in the first place. >>>>>>>> >>>>>>>> There seem to be two ways to do this: >>>>>>>> >>>>>>>> 1. Record addresses (ulong) in the EFI tables and use >>>>>>>> map_sysmem() >>>>>>>> before returning an address in the allocator >>>>>>>> 2. Store pointers in the EFI tables using map_sysmem(), then do >>>>>>>> no >>>>>>>> mapping in the allocator >>>>>>>> >>>>>>>> I've gone with option 1 since: >>>>>>>> >>>>>>>> - the EFI addresses are not pointers >>>>>>>> - it makes sandbox consistent with other architectures which use >>>>>>>> an >>>>>>>> address rather than a pointer in EFI tables >>>>>>>> - we don't normally do pointer arithmetic on the results of >>>>>>>> map_sysmem() >>>>>>>> - we normally map the memory when it is used rather than when it >>>>>>>> is >>>>>>>> set up >>>>>>>> >>>>>>>> I think you are asking for option 2. I think that would get very >>>>>>>> confusing. The addresses where things actually end up in sandbox >>>>>>>> are >>>>>>>> best kept to sandbox. >>>>>>>> >>>>>>>> Overall I feel that you are either missing the point of sandbox >>>>>>>> addressing, or don't agree with how it is done. But it does work >>>>>>>> pretty well and we don't get a lot of confusion with sandbox >>>>>>>> pointers >>>>>>>> since we typically use the address until the last moment. >>>>>>> >>>>>>> >>>>>>> I've assembled a quick tree for version 2. With that I'm able to >>>>>>> run >>>>>>> a >>>>>>> simple hello world efi application. Grub refuses to start because >>>>>>> it >>>>>>> wants >>>>>>> memory in the low 32bit and also emits random PIO accessing >>>>>>> functions, which >>>>>>> obviously don't work work from user space. >>>>>>> >>>>>>> But overall, I think this is the right path to tackle this: >>>>>>> >>>>>>> https://github.com/agraf/u-boot/tree/efi-sandbox >>>>>> >>>>>> What do you mean by version 2? >>>>> >>>>> Option 2 is what you called it. It's the only option we have to >>>>> make >>>>> efi binaries work. >>>>> >>>>>> It looks like you've added one patch, >>>>>> so will you send that to the list? >>>>> >>>>> It's more than 1 patch. And yes, I'll send them. >>>>> >>>>>> Anyway, I hope I can convince you of the above, the way sandbox >>>>>> memory >>>>>> works. >>>>> >>>>> I still dislike option 1 :) >>>>> >>>>> The reason is simple: The efi memory map is available to efi >>>>> payloads. >>>>> It's perfectly legal for them to do a static allocation at a >>>>> particular >>>>> address. We also share a lot of (host) pointers for callbacks and >>>>> structs >>>>> already with efi applications, so there is no real point to have a >>>>> split >>>>> brain situation between u-boot and host pointers. >>>> >>>> OK so you mean they don't have to allocate memory before using it? >>>> How >>>> then do you make sure that there is no conflict? I thought EFI was >>>> an >>>> API? >>> >>> You allocate it, but payloads expect that the address you pass in as >>> address you allocate at is the pointer/address that gets returned. >>> >> Can you please point me to that? Are you referring to this call? >> >> static efi_status_t EFIAPI efi_get_memory_map_ext( >> efi_uintn_t *memory_map_size, >> struct efi_mem_desc *memory_map, >> efi_uintn_t *map_key, >> efi_uintn_t *descriptor_size, >> uint32_t *descriptor_version) >> >> If so, we can still support that. >> >>> In a nutshell, what you're proposing is that the efi quivalent of >>> mmap(addr, MAP_FIXED) returns something != addr. That will break >>> things. >> >> I see this function: >> >> efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void >> **buffer) >> >> So there appears to be no way for people to specify the address they >> want. Is there another call somewhere? > > > You can call efi_allocate_pages() with EFI_ALLOCATE_ADDRESS or even > EFI_ALLOCATE_MAX_ADDRESS which you usually base on information you > receive > from efi_get_memory_map().

That's fine though, I'm not worried about that. It will work correctly, right?

>> NOTE: I am not proposing that the address is made available to the EFI >> application - that would always see the sandbox pointer. > > > In that case it's *much* easier to just always store host pointers in > the > efi memory map. Everything else will be very intrusive.

Yes there will be map_sysmem() and map_to_sysmem() calls at each boundary. That 'intrusion' is what we have everywhere in sandbox and it is how things work. But the nice thing is that the tables don't end up with 'private' sandbox addresses in them. Also see me comments above.

I seem to be missing the point. efi_loader is just a tiny shim layer whose only purpose it is to interface between the binary efi interface and U-Boot internal code. So why shouldn't efi_loader treat addresses as pointers internally?

So how about this: I'll send out a patch set that works the way I think is the right way forward. With this I can at least run binaries generated by gnu-efi. Then you can send a patch on top with a proposal how you think the mapping should happen. My guess is that your patch will just clutter the code, but I'll be happy to be persuaded otherwise.

I'm just not keen on putting sandbox pointers in internal data structures, sorry. It's not the way it is supposed to work and it makes sandbox diffferent from all the other archs. Trying to minimise the number of calls to map_sysmem() is not really the goal here.

Well, you're not the one who needs to maintain efi_loader code after every 2nd line in it ends up being some random map/unmap call ;).

But my proposal was serious. Putting virtual addresses in is trivial and gives us proper API interfaces. We can based on that still decide whether it's better to not have pointers in the map. I am not convinced yet, but open to the idea.

And yes, code maintainability is my uttermost goal of this. Things have to be simple for sandbox, otherwise they will break.

A virtual address has a particular meaning. This is not what is going on with sandbox...

Making sandbox store its internal addresses in an EFI table is not a good idea. I've tried to explain why. It is not more maintainable - it just adds confusion. At present all ulong addresses are normal U-Boot addresses and pointers point into the emulated RAM. It's pretty clear for people. Please don't change that for EFI.

If that's your concern, let's have the efi memory map contain pointers; because that's what it really does.

...

Can we get my patches applied and then add yours on top? I will send v7 which I think addresses all comments.

The patch set I just sent contains all patches from your set that I think we need, no?

I don't think so, you are missing several. I would like to resolve comments and get those applied and then move forward with expanding the feature set. At least that's how it normally works in U-Boot - we don't normally have the maintainer modify the patch series to his liking :-)

Regards, Simon

Hi Alex,

On 14 June 2018 at 12:05, Alexander Graf agraf@suse.de wrote:

On 14.06.18 19:15, Simon Glass wrote:

...

Hi Alex,

On 14 June 2018 at 11:08, Alexander Graf agraf@suse.de wrote:

...

On 06/14/2018 06:55 PM, Simon Glass wrote:

...

Hi Alex,

On 14 June 2018 at 10:42, Alexander Graf agraf@suse.de wrote:

...

On 06/14/2018 06:33 PM, Simon Glass wrote:

...

Hi Alex,

On 14 June 2018 at 10:26, Alexander Graf agraf@suse.de wrote: > > On 06/14/2018 06:13 PM, Simon Glass wrote: >> >> Hi Alex, >> >> On 14 June 2018 at 10:07, Alexander Graf agraf@suse.de wrote: >>> >>> On 06/14/2018 05:53 PM, Simon Glass wrote: >>>> >>>> Hi Alex, >>>> >>>> On 14 June 2018 at 09:47, Alexander Graf agraf@suse.de wrote: >>>>> >>>>> >>>>>> Am 14.06.2018 um 17:43 schrieb Simon Glass sjg@chromium.org: >>>>>> >>>>>> Hi Alex, >>>>>> >>>>>>> On 14 June 2018 at 08:22, Alexander Graf agraf@suse.de wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>>> Am 14.06.2018 um 16:12 schrieb Simon Glass sjg@chromium.org: >>>>>>>> >>>>>>>> Hi Alex, >>>>>>>> >>>>>>>>>> On 14 June 2018 at 07:41, Alexander Graf agraf@suse.de wrote: >>>>>>>>>> On 06/14/2018 02:58 PM, Simon Glass wrote: >>>>>>>>>> >>>>>>>>>> Hi Alex, >>>>>>>>>> >>>>>>>>>>>> On 14 June 2018 at 04:12, Alexander Graf agraf@suse.de >>>>>>>>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>> On 06/13/2018 04:37 AM, Simon Glass wrote: >>>>>>>>>>>> >>>>>>>>>>>> With sandbox the U-Boot code is not mapped into the sandbox >>>>>>>>>>>> memory >>>>>>>>>>>> range >>>>>>>>>>>> so does not need to be excluded when allocating EFI memory. >>>>>>>>>>>> Update >>>>>>>>>>>> the >>>>>>>>>>>> EFI >>>>>>>>>>>> memory init code to take account of that. >>>>>>>>>>>> >>>>>>>>>>>> Also use mapmem instead of a cast to convert a memory address >>>>>>>>>>>> to >>>>>>>>>>>> a >>>>>>>>>>>> pointer. >>>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: Simon Glass sjg@chromium.org >>>>>>>>>>>> --- >>>>>>>>>>>> >>>>>>>>>>>> Changes in v6: None >>>>>>>>>>>> Changes in v5: None >>>>>>>>>>>> Changes in v4: None >>>>>>>>>>>> Changes in v3: None >>>>>>>>>>>> Changes in v2: >>>>>>>>>>>> - Update to use mapmem instead of a cast >>>>>>>>>>>> >>>>>>>>>>>> lib/efi_loader/efi_memory.c | 31 >>>>>>>>>>>> ++++++++++++++++++------------- >>>>>>>>>>>> 1 file changed, 18 insertions(+), 13 deletions(-) >>>>>>>>>>>> >>>>>>>>>>>> diff --git a/lib/efi_loader/efi_memory.c >>>>>>>>>>>> b/lib/efi_loader/efi_memory.c >>>>>>>>>>>> index ec66af98ea..210e49ee8b 100644 >>>>>>>>>>>> --- a/lib/efi_loader/efi_memory.c >>>>>>>>>>>> +++ b/lib/efi_loader/efi_memory.c >>>>>>>>>>>> @@ -9,6 +9,7 @@ >>>>>>>>>>>> #include <efi_loader.h> >>>>>>>>>>>> #include <inttypes.h> >>>>>>>>>>>> #include <malloc.h> >>>>>>>>>>>> +#include <mapmem.h> >>>>>>>>>>>> #include <watchdog.h> >>>>>>>>>>>> #include <linux/list_sort.h> >>>>>>>>>>>> @@ -393,7 +394,7 @@ efi_status_t efi_allocate_pool(int >>>>>>>>>>>> pool_type, >>>>>>>>>>>> efi_uintn_t size, void **buffer) >>>>>>>>>>>> &t); >>>>>>>>>>>> if (r == EFI_SUCCESS) { >>>>>>>>>>>> - struct efi_pool_allocation *alloc = (void >>>>>>>>>>>> *)(uintptr_t)t; >>>>>>>>>>>> + struct efi_pool_allocation *alloc = >>>>>>>>>>>> map_sysmem(t, >>>>>>>>>>>> size); >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> This is still the wrong spot. We don't want the conversion to >>>>>>>>>>> happen when >>>>>>>>>>> going from an EFI internal address to an allocation, but when >>>>>>>>>>> determining >>>>>>>>>>> which addresses are usable in the first place. >>>>>>>>>> >>>>>>>>>> There seem to be two ways to do this: >>>>>>>>>> >>>>>>>>>> 1. Record addresses (ulong) in the EFI tables and use >>>>>>>>>> map_sysmem() >>>>>>>>>> before returning an address in the allocator >>>>>>>>>> 2. Store pointers in the EFI tables using map_sysmem(), then do >>>>>>>>>> no >>>>>>>>>> mapping in the allocator >>>>>>>>>> >>>>>>>>>> I've gone with option 1 since: >>>>>>>>>> >>>>>>>>>> - the EFI addresses are not pointers >>>>>>>>>> - it makes sandbox consistent with other architectures which use >>>>>>>>>> an >>>>>>>>>> address rather than a pointer in EFI tables >>>>>>>>>> - we don't normally do pointer arithmetic on the results of >>>>>>>>>> map_sysmem() >>>>>>>>>> - we normally map the memory when it is used rather than when it >>>>>>>>>> is >>>>>>>>>> set up >>>>>>>>>> >>>>>>>>>> I think you are asking for option 2. I think that would get very >>>>>>>>>> confusing. The addresses where things actually end up in sandbox >>>>>>>>>> are >>>>>>>>>> best kept to sandbox. >>>>>>>>>> >>>>>>>>>> Overall I feel that you are either missing the point of sandbox >>>>>>>>>> addressing, or don't agree with how it is done. But it does work >>>>>>>>>> pretty well and we don't get a lot of confusion with sandbox >>>>>>>>>> pointers >>>>>>>>>> since we typically use the address until the last moment. >>>>>>>>> >>>>>>>>> >>>>>>>>> I've assembled a quick tree for version 2. With that I'm able to >>>>>>>>> run >>>>>>>>> a >>>>>>>>> simple hello world efi application. Grub refuses to start because >>>>>>>>> it >>>>>>>>> wants >>>>>>>>> memory in the low 32bit and also emits random PIO accessing >>>>>>>>> functions, which >>>>>>>>> obviously don't work work from user space. >>>>>>>>> >>>>>>>>> But overall, I think this is the right path to tackle this: >>>>>>>>> >>>>>>>>> https://github.com/agraf/u-boot/tree/efi-sandbox >>>>>>>> >>>>>>>> What do you mean by version 2? >>>>>>> >>>>>>> Option 2 is what you called it. It's the only option we have to >>>>>>> make >>>>>>> efi binaries work. >>>>>>> >>>>>>>> It looks like you've added one patch, >>>>>>>> so will you send that to the list? >>>>>>> >>>>>>> It's more than 1 patch. And yes, I'll send them. >>>>>>> >>>>>>>> Anyway, I hope I can convince you of the above, the way sandbox >>>>>>>> memory >>>>>>>> works. >>>>>>> >>>>>>> I still dislike option 1 :) >>>>>>> >>>>>>> The reason is simple: The efi memory map is available to efi >>>>>>> payloads. >>>>>>> It's perfectly legal for them to do a static allocation at a >>>>>>> particular >>>>>>> address. We also share a lot of (host) pointers for callbacks and >>>>>>> structs >>>>>>> already with efi applications, so there is no real point to have a >>>>>>> split >>>>>>> brain situation between u-boot and host pointers. >>>>>> >>>>>> OK so you mean they don't have to allocate memory before using it? >>>>>> How >>>>>> then do you make sure that there is no conflict? I thought EFI was >>>>>> an >>>>>> API? >>>>> >>>>> You allocate it, but payloads expect that the address you pass in as >>>>> address you allocate at is the pointer/address that gets returned. >>>>> >>>> Can you please point me to that? Are you referring to this call? >>>> >>>> static efi_status_t EFIAPI efi_get_memory_map_ext( >>>> efi_uintn_t *memory_map_size, >>>> struct efi_mem_desc *memory_map, >>>> efi_uintn_t *map_key, >>>> efi_uintn_t *descriptor_size, >>>> uint32_t *descriptor_version) >>>> >>>> If so, we can still support that. >>>> >>>>> In a nutshell, what you're proposing is that the efi quivalent of >>>>> mmap(addr, MAP_FIXED) returns something != addr. That will break >>>>> things. >>>> >>>> I see this function: >>>> >>>> efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void >>>> **buffer) >>>> >>>> So there appears to be no way for people to specify the address they >>>> want. Is there another call somewhere? >>> >>> >>> You can call efi_allocate_pages() with EFI_ALLOCATE_ADDRESS or even >>> EFI_ALLOCATE_MAX_ADDRESS which you usually base on information you >>> receive >>> from efi_get_memory_map(). >> >> That's fine though, I'm not worried about that. It will work correctly, >> right? >> >>>> NOTE: I am not proposing that the address is made available to the EFI >>>> application - that would always see the sandbox pointer. >>> >>> >>> In that case it's *much* easier to just always store host pointers in >>> the >>> efi memory map. Everything else will be very intrusive. >> >> Yes there will be map_sysmem() and map_to_sysmem() calls at each >> boundary. That 'intrusion' is what we have everywhere in sandbox and >> it is how things work. But the nice thing is that the tables don't end >> up with 'private' sandbox addresses in them. Also see me comments >> above. > > > I seem to be missing the point. efi_loader is just a tiny shim layer > whose > only purpose it is to interface between the binary efi interface and > U-Boot > internal code. So why shouldn't efi_loader treat addresses as pointers > internally? > > So how about this: I'll send out a patch set that works the way I think > is > the right way forward. With this I can at least run binaries generated by > gnu-efi. Then you can send a patch on top with a proposal how you think > the > mapping should happen. My guess is that your patch will just clutter the > code, but I'll be happy to be persuaded otherwise.

I'm just not keen on putting sandbox pointers in internal data structures, sorry. It's not the way it is supposed to work and it makes sandbox diffferent from all the other archs. Trying to minimise the number of calls to map_sysmem() is not really the goal here.

Well, you're not the one who needs to maintain efi_loader code after every 2nd line in it ends up being some random map/unmap call ;).

But my proposal was serious. Putting virtual addresses in is trivial and gives us proper API interfaces. We can based on that still decide whether it's better to not have pointers in the map. I am not convinced yet, but open to the idea.

And yes, code maintainability is my uttermost goal of this. Things have to be simple for sandbox, otherwise they will break.

A virtual address has a particular meaning. This is not what is going on with sandbox...

Making sandbox store its internal addresses in an EFI table is not a good idea. I've tried to explain why. It is not more maintainable - it just adds confusion. At present all ulong addresses are normal U-Boot addresses and pointers point into the emulated RAM. It's pretty clear for people. Please don't change that for EFI.

If that's your concern, let's have the efi memory map contain pointers; because that's what it really does.

...

Can we get my patches applied and then add yours on top? I will send v7 which I think addresses all comments.

The patch set I just sent contains all patches from your set that I think we need, no?

I don't think so, you are missing several. I would like to resolve comments and get those applied and then move forward with expanding the feature set. At least that's how it normally works in U-Boot - we don't normally have the maintainer modify the patch series to his liking :-)

True, usually I would just ignore patches after my comments weren't address 5 times in a row ;).

What are you talking about? I only missed one comment on v5, for one patch, which was creating the function, now done in v7. Please explain.

In all seriousness, I want us to move this forward properly. So what patches from your set are missing?

[U-Boot,v7,10/10] efi: Rename bootefi_test_finish() to bootefi_run_finish() [U-Boot,v7,09/10] efi: Create a function to set up for running EFI code [U-Boot,v7,08/10] efi: sandbox: Add a simple 'bootefi test' command [U-Boot,v7,07/10] efi: Split out test init/uninit into functions

I don't want to introduce any new commands or do refactoring at this point. Let's make the selftest work first.

[U-Boot,v7,06/10] efi: sandbox: Enable EFI loader build for sandbox

Included

[U-Boot,v7,05/10] efi: sandbox: Add relocation constants

Included

[U-Boot,v7,04/10] efi: sandbox: Add distroboot support

Included

[U-Boot,v7,03/10] sandbox: smbios: Update to support sandbox

Dropped because it's no longer needed.

Actually this one should be applied. These are addresses so we should not be passing sandbox pointers in here.

[U-Boot,v7,02/10] efi: sandbox: Adjust memory usage for sandbox

Dropped because it's no longer needed.

Same here

[U-Boot,v7,01/10] efi: Don't allow CMD_BOOTEFI_SELFTEST on sandbox

Replaced with Heinrich's other proposal

So I think all patches that should be in are in?

I'd like to get 7-10 in as well. The 'bootefi test' thing is useful as a sanity check. Or are you saying you don't want that simple test? Otherwise you should not be changing the ordering as a maintainer.

The main issue is the sandbox memory mapping stuff, which we still disagree on.

Regards, Simon

Hi Alex,

On 14 June 2018 at 13:32, Alexander Graf agraf@suse.de wrote:

On 14.06.18 21:02, Simon Glass wrote:

...

Hi Alex,

On 14 June 2018 at 12:05, Alexander Graf agraf@suse.de wrote:

...

On 14.06.18 19:15, Simon Glass wrote:

...

Hi Alex,

On 14 June 2018 at 11:08, Alexander Graf agraf@suse.de wrote:

...

On 06/14/2018 06:55 PM, Simon Glass wrote:

...

Hi Alex,

On 14 June 2018 at 10:42, Alexander Graf agraf@suse.de wrote: > > On 06/14/2018 06:33 PM, Simon Glass wrote: >> >> Hi Alex, >> >> On 14 June 2018 at 10:26, Alexander Graf agraf@suse.de wrote: >>> >>> On 06/14/2018 06:13 PM, Simon Glass wrote: >>>> >>>> Hi Alex, >>>> >>>> On 14 June 2018 at 10:07, Alexander Graf agraf@suse.de wrote: >>>>> >>>>> On 06/14/2018 05:53 PM, Simon Glass wrote: >>>>>> >>>>>> Hi Alex, >>>>>> >>>>>> On 14 June 2018 at 09:47, Alexander Graf agraf@suse.de wrote: >>>>>>> >>>>>>> >>>>>>>> Am 14.06.2018 um 17:43 schrieb Simon Glass sjg@chromium.org: >>>>>>>> >>>>>>>> Hi Alex, >>>>>>>> >>>>>>>>> On 14 June 2018 at 08:22, Alexander Graf agraf@suse.de wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>>> Am 14.06.2018 um 16:12 schrieb Simon Glass sjg@chromium.org: >>>>>>>>>> >>>>>>>>>> Hi Alex, >>>>>>>>>> >>>>>>>>>>>> On 14 June 2018 at 07:41, Alexander Graf agraf@suse.de wrote: >>>>>>>>>>>> On 06/14/2018 02:58 PM, Simon Glass wrote: >>>>>>>>>>>> >>>>>>>>>>>> Hi Alex, >>>>>>>>>>>> >>>>>>>>>>>>>> On 14 June 2018 at 04:12, Alexander Graf agraf@suse.de >>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>> On 06/13/2018 04:37 AM, Simon Glass wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>> With sandbox the U-Boot code is not mapped into the sandbox >>>>>>>>>>>>>> memory >>>>>>>>>>>>>> range >>>>>>>>>>>>>> so does not need to be excluded when allocating EFI memory. >>>>>>>>>>>>>> Update >>>>>>>>>>>>>> the >>>>>>>>>>>>>> EFI >>>>>>>>>>>>>> memory init code to take account of that. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Also use mapmem instead of a cast to convert a memory address >>>>>>>>>>>>>> to >>>>>>>>>>>>>> a >>>>>>>>>>>>>> pointer. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Signed-off-by: Simon Glass sjg@chromium.org >>>>>>>>>>>>>> --- >>>>>>>>>>>>>> >>>>>>>>>>>>>> Changes in v6: None >>>>>>>>>>>>>> Changes in v5: None >>>>>>>>>>>>>> Changes in v4: None >>>>>>>>>>>>>> Changes in v3: None >>>>>>>>>>>>>> Changes in v2: >>>>>>>>>>>>>> - Update to use mapmem instead of a cast >>>>>>>>>>>>>> >>>>>>>>>>>>>> lib/efi_loader/efi_memory.c | 31 >>>>>>>>>>>>>> ++++++++++++++++++------------- >>>>>>>>>>>>>> 1 file changed, 18 insertions(+), 13 deletions(-) >>>>>>>>>>>>>> >>>>>>>>>>>>>> diff --git a/lib/efi_loader/efi_memory.c >>>>>>>>>>>>>> b/lib/efi_loader/efi_memory.c >>>>>>>>>>>>>> index ec66af98ea..210e49ee8b 100644 >>>>>>>>>>>>>> --- a/lib/efi_loader/efi_memory.c >>>>>>>>>>>>>> +++ b/lib/efi_loader/efi_memory.c >>>>>>>>>>>>>> @@ -9,6 +9,7 @@ >>>>>>>>>>>>>> #include <efi_loader.h> >>>>>>>>>>>>>> #include <inttypes.h> >>>>>>>>>>>>>> #include <malloc.h> >>>>>>>>>>>>>> +#include <mapmem.h> >>>>>>>>>>>>>> #include <watchdog.h> >>>>>>>>>>>>>> #include <linux/list_sort.h> >>>>>>>>>>>>>> @@ -393,7 +394,7 @@ efi_status_t efi_allocate_pool(int >>>>>>>>>>>>>> pool_type, >>>>>>>>>>>>>> efi_uintn_t size, void **buffer) >>>>>>>>>>>>>> &t); >>>>>>>>>>>>>> if (r == EFI_SUCCESS) { >>>>>>>>>>>>>> - struct efi_pool_allocation *alloc = (void >>>>>>>>>>>>>> *)(uintptr_t)t; >>>>>>>>>>>>>> + struct efi_pool_allocation *alloc = >>>>>>>>>>>>>> map_sysmem(t, >>>>>>>>>>>>>> size); >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> This is still the wrong spot. We don't want the conversion to >>>>>>>>>>>>> happen when >>>>>>>>>>>>> going from an EFI internal address to an allocation, but when >>>>>>>>>>>>> determining >>>>>>>>>>>>> which addresses are usable in the first place. >>>>>>>>>>>> >>>>>>>>>>>> There seem to be two ways to do this: >>>>>>>>>>>> >>>>>>>>>>>> 1. Record addresses (ulong) in the EFI tables and use >>>>>>>>>>>> map_sysmem() >>>>>>>>>>>> before returning an address in the allocator >>>>>>>>>>>> 2. Store pointers in the EFI tables using map_sysmem(), then do >>>>>>>>>>>> no >>>>>>>>>>>> mapping in the allocator >>>>>>>>>>>> >>>>>>>>>>>> I've gone with option 1 since: >>>>>>>>>>>> >>>>>>>>>>>> - the EFI addresses are not pointers >>>>>>>>>>>> - it makes sandbox consistent with other architectures which use >>>>>>>>>>>> an >>>>>>>>>>>> address rather than a pointer in EFI tables >>>>>>>>>>>> - we don't normally do pointer arithmetic on the results of >>>>>>>>>>>> map_sysmem() >>>>>>>>>>>> - we normally map the memory when it is used rather than when it >>>>>>>>>>>> is >>>>>>>>>>>> set up >>>>>>>>>>>> >>>>>>>>>>>> I think you are asking for option 2. I think that would get very >>>>>>>>>>>> confusing. The addresses where things actually end up in sandbox >>>>>>>>>>>> are >>>>>>>>>>>> best kept to sandbox. >>>>>>>>>>>> >>>>>>>>>>>> Overall I feel that you are either missing the point of sandbox >>>>>>>>>>>> addressing, or don't agree with how it is done. But it does work >>>>>>>>>>>> pretty well and we don't get a lot of confusion with sandbox >>>>>>>>>>>> pointers >>>>>>>>>>>> since we typically use the address until the last moment. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> I've assembled a quick tree for version 2. With that I'm able to >>>>>>>>>>> run >>>>>>>>>>> a >>>>>>>>>>> simple hello world efi application. Grub refuses to start because >>>>>>>>>>> it >>>>>>>>>>> wants >>>>>>>>>>> memory in the low 32bit and also emits random PIO accessing >>>>>>>>>>> functions, which >>>>>>>>>>> obviously don't work work from user space. >>>>>>>>>>> >>>>>>>>>>> But overall, I think this is the right path to tackle this: >>>>>>>>>>> >>>>>>>>>>> https://github.com/agraf/u-boot/tree/efi-sandbox >>>>>>>>>> >>>>>>>>>> What do you mean by version 2? >>>>>>>>> >>>>>>>>> Option 2 is what you called it. It's the only option we have to >>>>>>>>> make >>>>>>>>> efi binaries work. >>>>>>>>> >>>>>>>>>> It looks like you've added one patch, >>>>>>>>>> so will you send that to the list? >>>>>>>>> >>>>>>>>> It's more than 1 patch. And yes, I'll send them. >>>>>>>>> >>>>>>>>>> Anyway, I hope I can convince you of the above, the way sandbox >>>>>>>>>> memory >>>>>>>>>> works. >>>>>>>>> >>>>>>>>> I still dislike option 1 :) >>>>>>>>> >>>>>>>>> The reason is simple: The efi memory map is available to efi >>>>>>>>> payloads. >>>>>>>>> It's perfectly legal for them to do a static allocation at a >>>>>>>>> particular >>>>>>>>> address. We also share a lot of (host) pointers for callbacks and >>>>>>>>> structs >>>>>>>>> already with efi applications, so there is no real point to have a >>>>>>>>> split >>>>>>>>> brain situation between u-boot and host pointers. >>>>>>>> >>>>>>>> OK so you mean they don't have to allocate memory before using it? >>>>>>>> How >>>>>>>> then do you make sure that there is no conflict? I thought EFI was >>>>>>>> an >>>>>>>> API? >>>>>>> >>>>>>> You allocate it, but payloads expect that the address you pass in as >>>>>>> address you allocate at is the pointer/address that gets returned. >>>>>>> >>>>>> Can you please point me to that? Are you referring to this call? >>>>>> >>>>>> static efi_status_t EFIAPI efi_get_memory_map_ext( >>>>>> efi_uintn_t *memory_map_size, >>>>>> struct efi_mem_desc *memory_map, >>>>>> efi_uintn_t *map_key, >>>>>> efi_uintn_t *descriptor_size, >>>>>> uint32_t *descriptor_version) >>>>>> >>>>>> If so, we can still support that. >>>>>> >>>>>>> In a nutshell, what you're proposing is that the efi quivalent of >>>>>>> mmap(addr, MAP_FIXED) returns something != addr. That will break >>>>>>> things. >>>>>> >>>>>> I see this function: >>>>>> >>>>>> efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void >>>>>> **buffer) >>>>>> >>>>>> So there appears to be no way for people to specify the address they >>>>>> want. Is there another call somewhere? >>>>> >>>>> >>>>> You can call efi_allocate_pages() with EFI_ALLOCATE_ADDRESS or even >>>>> EFI_ALLOCATE_MAX_ADDRESS which you usually base on information you >>>>> receive >>>>> from efi_get_memory_map(). >>>> >>>> That's fine though, I'm not worried about that. It will work correctly, >>>> right? >>>> >>>>>> NOTE: I am not proposing that the address is made available to the EFI >>>>>> application - that would always see the sandbox pointer. >>>>> >>>>> >>>>> In that case it's *much* easier to just always store host pointers in >>>>> the >>>>> efi memory map. Everything else will be very intrusive. >>>> >>>> Yes there will be map_sysmem() and map_to_sysmem() calls at each >>>> boundary. That 'intrusion' is what we have everywhere in sandbox and >>>> it is how things work. But the nice thing is that the tables don't end >>>> up with 'private' sandbox addresses in them. Also see me comments >>>> above. >>> >>> >>> I seem to be missing the point. efi_loader is just a tiny shim layer >>> whose >>> only purpose it is to interface between the binary efi interface and >>> U-Boot >>> internal code. So why shouldn't efi_loader treat addresses as pointers >>> internally? >>> >>> So how about this: I'll send out a patch set that works the way I think >>> is >>> the right way forward. With this I can at least run binaries generated by >>> gnu-efi. Then you can send a patch on top with a proposal how you think >>> the >>> mapping should happen. My guess is that your patch will just clutter the >>> code, but I'll be happy to be persuaded otherwise. >> >> I'm just not keen on putting sandbox pointers in internal data >> structures, sorry. It's not the way it is supposed to work and it >> makes sandbox diffferent from all the other archs. Trying to minimise >> the number of calls to map_sysmem() is not really the goal here. > > > Well, you're not the one who needs to maintain efi_loader code after every > 2nd line in it ends up being some random map/unmap call ;). > > But my proposal was serious. Putting virtual addresses in is trivial and > gives us proper API interfaces. We can based on that still decide whether > it's better to not have pointers in the map. I am not convinced yet, but > open to the idea. > > And yes, code maintainability is my uttermost goal of this. Things have to > be simple for sandbox, otherwise they will break.

A virtual address has a particular meaning. This is not what is going on with sandbox...

Making sandbox store its internal addresses in an EFI table is not a good idea. I've tried to explain why. It is not more maintainable - it just adds confusion. At present all ulong addresses are normal U-Boot addresses and pointers point into the emulated RAM. It's pretty clear for people. Please don't change that for EFI.

If that's your concern, let's have the efi memory map contain pointers; because that's what it really does.

...

Can we get my patches applied and then add yours on top? I will send v7 which I think addresses all comments.

The patch set I just sent contains all patches from your set that I think we need, no?

I don't think so, you are missing several. I would like to resolve comments and get those applied and then move forward with expanding the feature set. At least that's how it normally works in U-Boot - we don't normally have the maintainer modify the patch series to his liking :-)

True, usually I would just ignore patches after my comments weren't address 5 times in a row ;).

What are you talking about? I only missed one comment on v5, for one patch, which was creating the function, now done in v7. Please explain.

I thought I had expressed my dislike for the current memory mapping model a few times by now. Maybe I wasn't explicit enough?

I was pretty clear that I don't want to change the sandbox memory model, so that's why I didn't do anything on that point. I don't think I could have been any clearer.

...

...

In all seriousness, I want us to move this forward properly. So what patches from your set are missing?

[U-Boot,v7,10/10] efi: Rename bootefi_test_finish() to bootefi_run_finish() [U-Boot,v7,09/10] efi: Create a function to set up for running EFI code [U-Boot,v7,08/10] efi: sandbox: Add a simple 'bootefi test' command [U-Boot,v7,07/10] efi: Split out test init/uninit into functions

I don't want to introduce any new commands or do refactoring at this point. Let's make the selftest work first.

[U-Boot,v7,06/10] efi: sandbox: Enable EFI loader build for sandbox

Included

[U-Boot,v7,05/10] efi: sandbox: Add relocation constants

Included

[U-Boot,v7,04/10] efi: sandbox: Add distroboot support

Included

[U-Boot,v7,03/10] sandbox: smbios: Update to support sandbox

Dropped because it's no longer needed.

Actually this one should be applied. These are addresses so we should not be passing sandbox pointers in here.

Well, maybe. But then we'd have to adjust for the API the other way around, as now pointers are what allocations return (because they're a payload visible API, so they have to).

Actually the function I am talking about is internal to U-Boot. The _ext() function can do the mapping as needed, as per my latest series. But I feel it can still be improved, since addresses and pointers are still a little mixed together in the EFI code.

...

...

[U-Boot,v7,02/10] efi: sandbox: Adjust memory usage for sandbox

Dropped because it's no longer needed.

Same here

Why does it hurt to expose the U-Boot and RTS memory addresses?

What is an RTS memory address?

...

...

[U-Boot,v7,01/10] efi: Don't allow CMD_BOOTEFI_SELFTEST on sandbox

Replaced with Heinrich's other proposal

So I think all patches that should be in are in?

I'd like to get 7-10 in as well. The 'bootefi test' thing is useful as a sanity check. Or are you saying you don't want that simple test?

I want a simple test, but I think the simple test is bootefi selftest. You can limit its scope to only emit "hello world" by setting an environment variable if you want to :).

I've reviewed selftest as it was built up and again recently. It is certainly not a simple sanity chec, which is what hello word is for.

...

Otherwise you should not be changing the ordering as a maintainer.

The main issue is the sandbox memory mapping stuff, which we still disagree on.

I guess we agree that we disagree :).

Looking at the current code base, we pretty much only call the map/unmap dance every time we go from command line arguments (where you pass in u-boot addresses) to pointers. Anything internal is usually dealing with pointers.

That's just not the case, as I have explained. Images in U-Boot, device tree, frame buffer, all peripheral's register locations, bootm, FIT and many other things are dealt with with addresses. U-Boot uses addresses for addresses, not pointers. The pointers are created from the addresses, but are not the primary means for communicating addresses.

I think that's a reasonable thing to do. And I think we should follow the same logic in the efi_loader parts. Command line parameters can easily be handled via map/unmap, but anything internal - and the efi memory map is what I consider internal - should just contain pointers.

The only reason they're not marked as pointers in the struct is that we want to copy the structs right back into the payload and there they are ABI defined as 64bit fields.

I still strongly disagree with this. I would like to get the sandbox feature in without changing how sandbox works.

Regards, Simon