[PATCH v5 18/23] efi_loader: Don't try to add sandbox runtime code

[Simon Glass]

Hi Ilias,

On Wed, 11 Dec 2024 at 23:16, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> On Wed, 11 Dec 2024 at 17:54, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Wed, 11 Dec 2024 at 08:19, Ilias Apalodimas
> > <ilias.apalodimas at linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Wed, 11 Dec 2024 at 15:54, Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > This cannot work since the code is not present in the emulated memory.
> > > > In any case, sandbox cannot make use of the runtime code.
> > > >
> > > > For now, just drop it from sandbox. We can always adjust things to copy
> > > > it into memory, if needed.
> > > >
> > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > ---
> > > >
> > > > (no changes since v1)
> > > >
> > > >  lib/efi_loader/efi_memory.c | 26 ++++++++++++++++----------
> > > >  1 file changed, 16 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> > > > index 0f149f99c7d..6475b94f951 100644
> > > > --- a/lib/efi_loader/efi_memory.c
> > > > +++ b/lib/efi_loader/efi_memory.c
> > > > @@ -767,16 +767,22 @@ static void add_u_boot_and_runtime(void)
> > > >         runtime_mask = SZ_64K - 1;
> > > >  #endif
> > > >
> > > > -       /*
> > > > -        * Add Runtime Services. We mark surrounding boottime code as runtime as
> > > > -        * well to fulfill the runtime alignment constraints but avoid padding.
> > > > -        */
> > > > -       runtime_start = (uintptr_t)__efi_runtime_start & ~runtime_mask;
> > > > -       runtime_end = (uintptr_t)__efi_runtime_stop;
> > > > -       runtime_end = (runtime_end + runtime_mask) & ~runtime_mask;
> > > > -       runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;
> > > > -       efi_add_memory_map_pg(runtime_start, runtime_pages,
> > > > -                             EFI_RUNTIME_SERVICES_CODE, false);
> > >
> > >
> > > Can sandbox boot an OS? If not there's no point adding this. But if
> > > you insist, I prefer
> >
> > No sandbox can't really boot an OS. The primary reason for this change
> > is to avoid strange things appearing in the EFI memory map (and
> > therefore the EFI log). I found it very confusing.
> >
> > > if (IS_ENABLED(CONFIG_SANDBOX)
> > >     return
> > >
> > > at the top of the function
> >
> > OK
>
> But can't we call ExitBootServices  from sandbox and test some of those?

Yes I'm pretty sure that could be made to work, although I would need
to try it. But we don't have such a test today, so we can worry about
it then.

>
> Thanks
> /Ilias
> >
> > >
> > > Thanks
> > > /Ilias
> > >
> > > > +       if (!IS_ENABLED(CONFIG_SANDBOX)) {
> > > > +               /*
> > > > +                * Add Runtime Services. We mark surrounding boottime code as
> > > > +                * runtime as well to fulfill the runtime alignment constraints
> > > > +                * but avoid padding.
> > > > +                *
> > > > +                * This is not enabled for sandbox, since we cannot map the
> > > > +                * sandbox code into emulated SDRAM
> > > > +                */
> > > > +               runtime_start = (uintptr_t)__efi_runtime_start & ~runtime_mask;
> > > > +               runtime_end = (uintptr_t)__efi_runtime_stop;
> > > > +               runtime_end = (runtime_end + runtime_mask) & ~runtime_mask;
> > > > +               runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;
> > > > +               efi_add_memory_map_pg(runtime_start, runtime_pages,
> > > > +                                     EFI_RUNTIME_SERVICES_CODE, false);
> > > > +       }
> > > >  }
> > > >
> > > >  int efi_memory_init(void)
> > > > --
> > > > 2.34.1
> > > >
> >

Regards,
SImon