| Message ID | 20180614182232.78201-8-agraf@suse.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | sandbox: efi_loader support | expand |
Hi Alex, On 14 June 2018 at 12:22, Alexander Graf <agraf@suse.de> wrote: > With efi_loader we do not control payload applications, so we can not > teach them about the difference between virtual and physical addresses. > > Instead, let's just always map host virtual addresses in the efi memory > map. That way we can be sure that all memory allocation functions always > return consumable pointers. > > Signed-off-by: Alexander Graf <agraf@suse.de> > > --- > > v1 -> v2: > > - only compile efi_add_known_memory if efi_loader is enabled > --- > arch/sandbox/cpu/cpu.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) NAK. You should not point sandbox pointers into the EFI tables. I know it looks like a clever shortcut, but it is not correct. It will mess up logging and debugging, since those pointers bear no easily accessible relationship to U-Boot address. Please start from my v7 patch. I'm happy to help do this correctly. But, again, I think it should come after we have basic sandbox EFI support applied. Regards, Simon
On 14.06.18 21:02, Simon Glass wrote: > Hi Alex, > > On 14 June 2018 at 12:22, Alexander Graf <agraf@suse.de> wrote: >> With efi_loader we do not control payload applications, so we can not >> teach them about the difference between virtual and physical addresses. >> >> Instead, let's just always map host virtual addresses in the efi memory >> map. That way we can be sure that all memory allocation functions always >> return consumable pointers. >> >> Signed-off-by: Alexander Graf <agraf@suse.de> >> >> --- >> >> v1 -> v2: >> >> - only compile efi_add_known_memory if efi_loader is enabled >> --- >> arch/sandbox/cpu/cpu.c | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) > > NAK. > > You should not point sandbox pointers into the EFI tables. I know it > looks like a clever shortcut, but it is not correct. It will mess up > logging and debugging, since those pointers bear no easily accessible > relationship to U-Boot address. > > Please start from my v7 patch. I'm happy to help do this correctly. > But, again, I think it should come after we have basic sandbox EFI > support applied. I don't want to play ping pong with you here. NAK on your approach until I see it properly executing selftest. So either we drive this forward or we don't. Your choice. Alex
On 06/14/2018 09:02 PM, Simon Glass wrote: > Hi Alex, > > On 14 June 2018 at 12:22, Alexander Graf <agraf@suse.de> wrote: >> With efi_loader we do not control payload applications, so we can not >> teach them about the difference between virtual and physical addresses. >> >> Instead, let's just always map host virtual addresses in the efi memory >> map. That way we can be sure that all memory allocation functions always >> return consumable pointers. >> >> Signed-off-by: Alexander Graf <agraf@suse.de> >> >> --- >> >> v1 -> v2: >> >> - only compile efi_add_known_memory if efi_loader is enabled >> --- >> arch/sandbox/cpu/cpu.c | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) > > NAK. > > You should not point sandbox pointers into the EFI tables. I know it > looks like a clever shortcut, but it is not correct. It will mess up > logging and debugging, since those pointers bear no easily accessible > relationship to U-Boot address. Hello Simon, why do we need this Babylonic confusion of addresses which do not point to valid memory and pointers that are not valid addresses where everybody is getting lost? Simply use only addresses with there mmap'ed values and get rid of all conversions. Use your board file to adjust kernel_addr_r and the like to the address range that Linux offered you. Best regards Heinrich > > Please start from my v7 patch. I'm happy to help do this correctly. > But, again, I think it should come after we have basic sandbox EFI > support applied. > > Regards, > Simon >
Hi Heinrich, On 14 June 2018 at 13:21, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > On 06/14/2018 09:02 PM, Simon Glass wrote: >> Hi Alex, >> >> On 14 June 2018 at 12:22, Alexander Graf <agraf@suse.de> wrote: >>> With efi_loader we do not control payload applications, so we can not >>> teach them about the difference between virtual and physical addresses. >>> >>> Instead, let's just always map host virtual addresses in the efi memory >>> map. That way we can be sure that all memory allocation functions always >>> return consumable pointers. >>> >>> Signed-off-by: Alexander Graf <agraf@suse.de> >>> >>> --- >>> >>> v1 -> v2: >>> >>> - only compile efi_add_known_memory if efi_loader is enabled >>> --- >>> arch/sandbox/cpu/cpu.c | 20 ++++++++++++++++++++ >>> 1 file changed, 20 insertions(+) >> >> NAK. >> >> You should not point sandbox pointers into the EFI tables. I know it >> looks like a clever shortcut, but it is not correct. It will mess up >> logging and debugging, since those pointers bear no easily accessible >> relationship to U-Boot address. > > Hello Simon, > > why do we need this Babylonic confusion of addresses which do not point > to valid memory and pointers that are not valid addresses where > everybody is getting lost? > > Simply use only addresses with there mmap'ed values and get rid of all > conversions. Use your board file to adjust kernel_addr_r and the like to > the address range that Linux offered you. No one is getting lost. It has been working fine for a long time. Sandbox was introduced almost 7 years ago. It has been a huge benefit in terms of productivity and testing. We are not necessarily running on Linux, but in any case, we get ugly addresses which expose the underlying machine architecture, which is not relevant for tests, and just introduces confusion. Sandbox is intended to be pure U-Boot, just like you might boot up on a 32-bit ARM or x86 machine. The conversions existing in any case, since we must case between addresses and pointers. This just makes it explicit in a convenient, type-safe manner. Regards, Simon
Hi Alex, On 14 June 2018 at 13:15, Alexander Graf <agraf@suse.de> wrote: > > > On 14.06.18 21:02, Simon Glass wrote: >> Hi Alex, >> >> On 14 June 2018 at 12:22, Alexander Graf <agraf@suse.de> wrote: >>> With efi_loader we do not control payload applications, so we can not >>> teach them about the difference between virtual and physical addresses. >>> >>> Instead, let's just always map host virtual addresses in the efi memory >>> map. That way we can be sure that all memory allocation functions always >>> return consumable pointers. >>> >>> Signed-off-by: Alexander Graf <agraf@suse.de> >>> >>> --- >>> >>> v1 -> v2: >>> >>> - only compile efi_add_known_memory if efi_loader is enabled >>> --- >>> arch/sandbox/cpu/cpu.c | 20 ++++++++++++++++++++ >>> 1 file changed, 20 insertions(+) >> >> NAK. >> >> You should not point sandbox pointers into the EFI tables. I know it >> looks like a clever shortcut, but it is not correct. It will mess up >> logging and debugging, since those pointers bear no easily accessible >> relationship to U-Boot address. >> >> Please start from my v7 patch. I'm happy to help do this correctly. >> But, again, I think it should come after we have basic sandbox EFI >> support applied. > > I don't want to play ping pong with you here. NAK on your approach until > I see it properly executing selftest. I think you just did :-) But if you are asking for me to pull together a patch that gets that far, then OK. I can see that you are not convinced it would work, or be easy to follow, and I have not proven that yet. I was just hoping to take things in incremental steps since this has been outstanding for so long. > > So either we drive this forward or we don't. Your choice. I have long wanted EFI to fall into the sandbox testing framework, so that e.g. 'make tests' will quickly run the EFI tests. I don't think we are too far away. It doesn't have to happen immediately, but I predict that when we get it working, we won't look back. It will be much more convenient than running a separate app or testing on 'real hardware' and you can set up quite complicated things with it. Regards, Simon
diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c index cde0b055a6..23d8b70648 100644 --- a/arch/sandbox/cpu/cpu.c +++ b/arch/sandbox/cpu/cpu.c @@ -5,6 +5,7 @@ #define DEBUG #include <common.h> #include <dm.h> +#include <efi_loader.h> #include <errno.h> #include <linux/libfdt.h> #include <os.h> @@ -177,3 +178,22 @@ void longjmp(jmp_buf jmp, int ret) while (1) ; } + +#ifdef CONFIG_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
With efi_loader we do not control payload applications, so we can not teach them about the difference between virtual and physical addresses. Instead, let's just always map host virtual addresses in the efi memory map. That way we can be sure that all memory allocation functions always return consumable pointers. Signed-off-by: Alexander Graf <agraf@suse.de> --- v1 -> v2: - only compile efi_add_known_memory if efi_loader is enabled --- arch/sandbox/cpu/cpu.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)