[PATCH v5 03/23] efi_loader: Add comments where incorrect addresses are used

[Simon Glass]

On Thu, 12 Dec 2024 at 15:44, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Ilias,
>
> On Wed, 11 Dec 2024 at 23:15, Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
> >
> > On Wed, 11 Dec 2024 at 17:58, Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi Ilias,
> > >
> > > On Wed, 11 Dec 2024 at 07:48, Ilias Apalodimas
> > > <ilias.apalodimas at linaro.org> wrote:
> > > >
> > > > On Wed, 11 Dec 2024 at 15:54, Simon Glass <sjg at chromium.org> 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 <sjg at chromium.org>
> > > > > ---
> > > > > 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-wruH7yRmQA@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!