On Thu, 12 Dec 2024 at 15:44, Simon Glass <[email protected]> wrote: > > Hi Ilias, > > On Wed, 11 Dec 2024 at 23:15, Ilias Apalodimas > <[email protected]> wrote: > > > > On Wed, 11 Dec 2024 at 17:58, Simon Glass <[email protected]> wrote: > > > > > > Hi Ilias, > > > > > > On Wed, 11 Dec 2024 at 07:48, Ilias Apalodimas > > > <[email protected]> wrote: > > > > > > > > On Wed, 11 Dec 2024 at 15:54, Simon Glass <[email protected]> wrote: > > > > > > > > > > Some functions are passing addresses instead of pointers to the > > > > > efi_add_memory_map() function. This confusion is understandable since > > > > > the function arguments indicate an address. > > > > > > > > > > Make a note of the 8 places where there are problems, which would > > > > > break > > > > > usage in sandbox tests. > > > > > > > > > > Future work will resolve these problems. > > > > > > > > NAK, this serves no purpose whatsoever. Just send a patch with > > > > whatever changes you think are needed > > > > > > Please see below > > > > > > > > > > > Thanks > > > > /Ilias > > > > > > > > > > Signed-off-by: Simon Glass <[email protected]> > > > > > --- > > > > > A request was made in v4 to drop this patch, but in the same thread, a > > > > > query was sent about why we need these addresses, so at least until > > > > > the > > > > > series is about to be applied, I've left it in, so we can have that > > > > > discussion in one place and build a shared understanding of why these > > > > > addresses are needed, instead of peppering the rest of the series with > > > > > similar questions. > > > > > > I'm quite happy to drop this patch. But I want to first make sure that > > > you understand that these bugs exist today in the code, albeit they > > > only matter on sandbox. > > > > Thei weird thing is sandbox works perfectly fine in EFI, passes all > > internal tests, EFI SCT tests, loads binaries etc. Any idea why that > > works? > > From what I can tell, the code mentioned is not tested by sandbox yet. > It would just segfault. We do have a lot of self tests, but my > bootflow_efi should allow testing of more features over time. I > noticed the runtime-code issue when looking at an EFI log. > > BTW how are you running that? When I tried it on x86_64 it just > crashed. Are you using the sandbox build on an Arm64 machine?
Yes, and it passes all the SCT tests. Thanks /Ilias > > > > > > You will notice that my future patches don't > > > change the code mentioned in this file. The bugs are simply resolved > > > by reworking the EFI-loader memory map to use addresses, instead of > > > pointers cast to u64 > > > > > > So can you please confirm this makes sense? (if not, let's discuss > > > it). Then I can drop this patch and the later one. > > > > The ptrs to u64 doesn't make too much sense tbh. I do understand how > > sandbox works, but EFI deals with physical addresses which are also > > mapped 1:1 > > Yes, and that's why (in efi_boottime) the conversion is done in one > place (from u64 pointers/addresses to U-Boot addresses). It simplifies > the code and avoids the kind of bugs here. > > Regards, > Simon > > > > > > Thanks > > /Ilias > > > > > > > > > > > > > Link: > > > > > https://lore.kernel.org/u-boot/cac_iwjlex4z41cdpx4u07xecygjb7h+ynd8aqyl-wruh7yr...@mail.gmail.com/ > > > > > > > > > > (no changes since v2) > > > > > > > > > > Changes in v2: > > > > > - Add a new patch with comments where incorrect addresses are used > > > > > > > > > > lib/efi_loader/efi_acpi.c | 2 ++ > > > > > lib/efi_loader/efi_bootmgr.c | 5 +++++ > > > > > lib/efi_loader/efi_helper.c | 1 + > > > > > lib/efi_loader/efi_smbios.c | 6 +++++- > > > > > lib/efi_loader/efi_var_mem.c | 2 ++ > > > > > 5 files changed, 15 insertions(+), 1 deletion(-) > > > > > Applied to sjg/master, thanks!
