[PATCH 0/8] efi_loader: Complete the bootflow_efi() test

[Simon Glass]

Hi Caleb,

On Wed, 8 Jan 2025 at 06:39, Caleb Connolly <caleb.connolly at linaro.org> wrote:
>
>
>
> On 07/01/2025 16:47, Heinrich Schuchardt wrote:
> > On 07.01.25 16:11, Tom Rini wrote:
> >> On Tue, Jan 07, 2025 at 06:57:50AM -0700, Simon Glass wrote:
> >>> Hi Heinrich,
> >>>
> >>> On Tue, 7 Jan 2025 at 06:11, Heinrich Schuchardt <xypron.glpk at gmx.de>
> >>> wrote:
> >>>>
> >>>> On 07.01.25 13:15, Simon Glass wrote:
> >>>>> Hi Heinrich,
> >>>>>
> >>>>> On Mon, 6 Jan 2025 at 10:00, Heinrich Schuchardt
> >>>>> <xypron.glpk at gmx.de> wrote:
> >>>>>>
> >>>>>> On 06.01.25 15:47, Simon Glass wrote:
> >>>>>>> This test was hamstrung in code review so this series is an
> >>>>>>> attempt to
> >>>>>>> complete the intended functionality:
> >>>>>>>
> >>>>>>> - Check memory allocations look correct
> >>>>>>> - Check that exit-boot-services removes active-DMA devices
> >>>>>>> - Check that the bootflow is still present after testapp finishes
> >>>>>>>
> >>>>>>> The EFI functionality duplicates bootm_announce_and_cleanup() and
> >>>>>>> still
> >>>>>>> uses the defunct board_quiesce_devices() so a nice cleanup would
> >>>>>>> be to
> >>>>>>> call the bootm function instead, with suitable modifications.
> >>>>>>> That would
> >>>>>>> allow bootstage to work too.
> >>>>>>>
> >>>>>>> This series is based on sjg/master since the EFI logging was
> >>>>>>> rejected so
> >>>>>>> far.
> >>>>>>
> >>>>>> Yes, it was rejected because a solution at the lib/log.c level
> >>>>>> would be
> >>>>>> more generic.
> >>>>>
> >>>>> As I mentioned, that idea isn't suitable for programmatic use.
> >>>>
> >>>> What can be done with show_addr("mem", rec->memory); that log_debug()
> >>>> does not offer or which you could not do with a new log function in
> >>>> lib/log.c that takes variadic arguments?
> >>>
> >>> There are asserts in [1], for example. How do you propose to handle
> >>> that? See [2] for my previous explanation, quoted here:
> >>>
> >>>> CONFIG_LOG with a bloblist option would be a great idea, but it's hard
> >>>> to programmatically scan text...plus only the external call sites are
> >>>> actually logged.
> >>>
> >>> Also see the discussion on the original patch [3]. There was also your
> >>> reply at [4], but I think you missed that this is intended for use in
> >>> unit tests (i.e. with ut_assert()).
> >>>
> >>> You also requested that this be generalised, rather than being
> >>> EFI-loader-specific. I have no objection to that, but don't have a use
> >>> case for it yet, so have deferred that to later. It's a fairly simple
> >>> change, if/when needed. If the series was not NAKed, I'd be happy to
> >>> do it now.
> >>>
> >>>>>
> >>>>>>
> >>>>>> Tom suggested not to send patches that are for private enjoyment
> >>>>>> to the
> >>>>>> mailing list.
> >>>>>
> >>>>> My contributions to U-Boot are only ever about private enjoyment :-)
> >>>>>
> >>>>> Do you have any comments on the patches?
> >>>
> >>> Regards,
> >>> Simon
> >>>
> >>> [1] https://patchwork.ozlabs.org/project/uboot/
> >>> patch/20250106144755.3054780-6-sjg at chromium.org/
> >>> [2] https://lore.kernel.org/u-boot/
> >>> CAFLszTjxOE_037+kR0jgdax80sBombYo_k0YgiuVnP=KZCOvuA at mail.gmail.com/
> >>> [3] https://lore.kernel.org/u-boot/
> >>> CAC_iWjKtaN54B98OKbkoXkC_GmKJ=x+M4=UY_O6roSOpZaDxag at mail.gmail.com/
> >>> [4] https://lore.kernel.org/u-boot/D513D326-41A6-425E-
> >>> B11F-85958065BCD2 at gmx.de/
> >>
> >> Looking at the logging portions of the original series again, especially
> >> if this was made generic, we probably don't want to print to actual
> >> console every time we're making a note of some memory allocation for
> >> example, that would be unreadable outside of a debug context. The point
> >> of this really seems to be "log things for verifying in tests later".
> >> Does that end up being useful? I don't know. Heinrich or Ilias, do the
> >> tests in [1] look generally useful?
> >>
> >
> > The tests in [1] are not documented, not even in the commit message. So
> > the reasoning behind the tests remains Simon's secret.
> >
> > At first sight the tests in [1] don't make much sense. E.g. that only a
> > subset of memory types have been used does not tell that the right
> > memory type has been used for the right object.
> >
> > Implementing a specific tracing functionality for EFI is definitively
> > the wrong way forward as it will lead to code duplication.
> >
> > We already have function _log() which is variadic.
> >
> > Simon could write a new log driver that parses the `format` parameter
> > and saves the binary data in an appropriate format for analysis by the
> > unit tests:
>
> Isn't this precisely what monkeypatching is for in unit tests? imho this
> does not make sense as a logging API but expanding FTRACE to have more
> capabilities (like Linux trace has) so that it can save arguments and
> then having some nice interface to assert that certain functions were
> called in a certain order with certain arguments would be a scalable way
> to go (and surely useful in other cases too).

Yes, that would be nice.

>
> Honestly though it seems quite wrong for the bootflow test to inspect
> EFI memory allocations, these are completely different levels of API and
> any tests like this are going to be prone to breaking over time just
> because they're making assumptions across so many layers.

It certainly is a bit odd. My goal (not necessarily shared with
others) is to ensure that EFI memory-allocations are only done if an
EFI app is booted.

But if you look at the checks here, they do make sense. If someone
allocates memory of a different type, we would want to know about it.
If an allocation was outside the range of DRAM, ditto. I actually
would *not* expect these tests to break with future changes. The
bootflow_efi test is specifically targetting the EFI layer. I suppose
you could say that it is also testing bootstd in general, but there
are many other tests which are much better at that. So this test is
taking a bit of a look of what EFI is doing under the hood.

Anyway, I'll keep developing the idea, on and off, and we'll see where it goes.

>
> Kind regards,
> >
> > * For %s the driver should save the string and not the address of the
> > string.
> > * For %pD the driver should save the device path instead of the pointer.
> > * ...
> >
> > Some changes to the log driver interface will be needed to pass the
> > variadic arguments instead of the formatted message.

Regards,
Simon