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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 3174 bytes --]

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@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Tue, 3 Dec 2024 at 02:22, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Tom,
> > >
> > > On Mon, 2 Dec 2024 at 13:18, Tom Rini <trini@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@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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]