[PATCH v4 04/25] efi_loader: Add comments where incorrect addresses are used

[Simon Glass]

Hi Tom,

On Tue, 3 Dec 2024 at 07:09, Tom Rini <trini at konsulko.com> wrote:
>
> On Tue, Dec 03, 2024 at 06:45:58AM -0700, Simon Glass wrote:
> > Hi Ilias,
> >
> > On Tue, 3 Dec 2024 at 01:49, Ilias Apalodimas
> > <ilias.apalodimas at linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Tue, 3 Dec 2024 at 02:22, Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Tom,
> > > >
> > > > On Mon, 2 Dec 2024 at 13:18, Tom Rini <trini at konsulko.com> wrote:
> > > > >
> > > > > On Sun, Dec 01, 2024 at 08:24:23AM -0700, Simon Glass 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.
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > >
> > > > > Please just resolve these rather than introducing a patch to then fix
> > > > > them later. This is something that should have been fixup'd before
> > > > > posting. Thanks.
> > > >
> > > > That was deliberate, as I wanted people to see the problems. It will
> > > > save discussion on later patches where the problems are fixed, if we
> > > > can agree that these are actual problems. If people are happy to add
> > > > review tags to the later patches then I'm happy to redo it.
> > >
> > > I am pretty sure Heinrich has repeated this in the past. Why do we
> > > have to sprinkle around map_sysmem/unmap sysmem for sandbox?
> > > Polluting the entire u-boot to support a special platform is less than
> > > ideal. Why can't sandbox limit this internally and do whatever
> > > mappings it needs when it receives an address?
> >
> > Tom, I suppose I have made my point.
> >
> > Ilias, this is documented at [1] and has been the same for 10 years.
> >
> > See for example the 'md' command, do_mem_md(), which shows how 'md 0'
> > is implemented in sandbox.
> >
> > As I have mentioned before, the nice thing is that you can easily make
> > code work with sandbox just by converting casts from
> > address-to-pointer into map_sysmem().
> >
> > Back to this series, if you look at efi_allocate_pages() you'll
> > currently see three calls (two map_to_sysmem() one map_sysmem(), but
> > now, with this series, there is none. It also makes it a lot easier to
> > understand what is going on, IMO.
> >
> > Regards,
> > Simon
> >
> > [1] https://docs.u-boot.org/en/latest/arch/sandbox/sandbox.html#memory-emulation
>
> This is something I was wondering about yesterday in fact. Especially
> these days when "everyone" is developing on machines with 8GB or more of
> memory, why does sandbox not just malloc 512MB/1GB/2GB/4GB (make it a
> switch?) and pretend that area is memory starting at some address.
> Especially if it's less than 4GB and starts at 0x80000000 more "memory
> starts at 0" bugs would be found and fixed. I swear we did something
> like that in my OS class 20+ years ago. I'm not sure what your link is
> meant for, as it doesn't say why sandbox works the way it does, merely
> that it works this way. Not that it can't work some other way.

It could work some other way, it's just that this seems to be the best way.

It is easier to have memory starting at address zero (many platforms
do), For example, tests can assume that and are easier to write,
particularly tests which check cmdline output.

But note that the actual pointer address is elsewhere, so we do get a
segfault if a null pointer is used. That has indeed found a lot of
bugs.

Using a mapping does make it easy to see when code has been set up for sandbox.

Some tests rely on memory being preserved across phases, so if memory
moves around in each phase, it gets pretty confusing. Also things like
kernel_addr_r become hard, and printing out addresses becomes very
confusing when every address is 0x555555599fbc ...

In my opinion, sandbox is an important and useful feature of U-Boot.
It is where I do almost all of my development and testing. We have >1k
tests. So I believe it is worth the cost of a little discipline around
address/pointer casting. What do you think?

Regards,
Simon