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

[Heinrich Schuchardt]

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@chromium.org/
>> [2] https://lore.kernel.org/u-boot/CAFLszTjxOE_037+kR0jgdax80sBombYo_k0YgiuVnP=KZCOvuA@mail.gmail.com/
>> [3] https://lore.kernel.org/u-boot/CAC_iWjKtaN54B98OKbkoXkC_GmKJ=x+M4=UY_O6roSOpZaDxag@mail.gmail.com/
>> [4] https://lore.kernel.org/u-boot/D513D326-41A6-425E-B11F-85958065BCD2@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:

* 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.

Best regards

Heinrich