| Message ID | 20180618152315.34233-22-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | sandbox: efi_loader support | expand |
> 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
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
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
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
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
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; }
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(-)