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

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

[Simon Glass]

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.


Simon Glass (8):
  sandbox: Make USB controller as having active DMA
  efi_loader: Fix display of addresses in log
  efi_loader: Return the memory map in pointer format
  efi_loader: Correct bounce-buffer setup
  efi_loader: Check memory allocations in bootflow_efi test()
  efi_loader: Update testapp to get memory map correctly
  efi_loader: Check that the bootflow is not removed by app
  efi_loader: Test that active-DMA devices are removed

 drivers/usb/host/usb-sandbox.c |  1 +
 lib/efi_loader/efi_log.c       |  5 +--
 lib/efi_loader/efi_memory.c    |  9 ++--
 lib/efi_loader/testapp.c       | 37 ++++++++++++++++
 test/boot/bootflow.c           | 78 +++++++++++++++++++++++++++++++++-
 5 files changed, 122 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH 1/8] sandbox: Make USB controller as having active DMA

[Simon Glass]

Set the DM_FLAG_ACTIVE_DMA flag on this device so that it is removed
before booting.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 drivers/usb/host/usb-sandbox.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/host/usb-sandbox.c b/drivers/usb/host/usb-sandbox.c
index f687fe2c430..6d75ce80dcd 100644
--- a/drivers/usb/host/usb-sandbox.c
+++ b/drivers/usb/host/usb-sandbox.c
@@ -180,4 +180,5 @@ U_BOOT_DRIVER(usb_sandbox) = {
 	.probe = sandbox_usb_probe,
 	.ops	= &sandbox_usb_ops,
 	.priv_auto	= sizeof(struct sandbox_usb_ctrl),
+	.flags	= DM_FLAG_ACTIVE_DMA,
 };
-- 
2.34.1

[PATCH 2/8] efi_loader: Fix display of addresses in log

[Simon Glass]

The allocate/free-pages functions return an address, so there is no need
to convert it. Fix this.

Signed-off-by: Simon Glass <sjg@chromium.org>
Fixes: c824a96d76d ("efi_loader: Use the log with memory-related...")
---

 lib/efi_loader/efi_log.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/efi_loader/efi_log.c b/lib/efi_loader/efi_log.c
index 4b9c6727fe7..ad490bb431c 100644
--- a/lib/efi_loader/efi_log.c
+++ b/lib/efi_loader/efi_log.c
@@ -340,8 +340,7 @@ void show_rec(int seq, struct efil_rec_hdr *rec_hdr)
 		show_ulong("pgs", (ulong)rec->pages);
 		show_addr("mem", (ulong)rec->memory);
 		if (rec_hdr->ended) {
-			show_addr("*mem",
-				  (ulong)map_to_sysmem((void *)rec->e_memory));
+			show_addr("*mem", rec->e_memory);
 			show_ret(rec_hdr->e_ret);
 		}
 		break;
@@ -349,7 +348,7 @@ void show_rec(int seq, struct efil_rec_hdr *rec_hdr)
 	case EFILT_FREE_PAGES: {
 		struct efil_free_pages *rec = start;
 
-		show_addr("mem", map_to_sysmem((void *)rec->memory));
+		show_addr("mem", rec->memory);
 		show_ulong("pag", (ulong)rec->pages);
 		if (rec_hdr->ended)
 			show_ret(rec_hdr->e_ret);
-- 
2.34.1

Re: [PATCH 2/8] efi_loader: Fix display of addresses in log

[Ilias Apalodimas]

Hi Simon,

On Mon, 6 Jan 2025 at 16:48, Simon Glass <sjg@chromium.org> wrote:
>
> The allocate/free-pages functions return an address, so there is no need
> to convert it. Fix this.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Fixes: c824a96d76d ("efi_loader: Use the log with memory-related...")

I can't find that commit in -master. Is there anything I am missing?

Thanks
/Ilias
> ---
>
>  lib/efi_loader/efi_log.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/lib/efi_loader/efi_log.c b/lib/efi_loader/efi_log.c
> index 4b9c6727fe7..ad490bb431c 100644
> --- a/lib/efi_loader/efi_log.c
> +++ b/lib/efi_loader/efi_log.c
> @@ -340,8 +340,7 @@ void show_rec(int seq, struct efil_rec_hdr *rec_hdr)
>                 show_ulong("pgs", (ulong)rec->pages);
>                 show_addr("mem", (ulong)rec->memory);
>                 if (rec_hdr->ended) {
> -                       show_addr("*mem",
> -                                 (ulong)map_to_sysmem((void *)rec->e_memory));
> +                       show_addr("*mem", rec->e_memory);
>                         show_ret(rec_hdr->e_ret);
>                 }
>                 break;
> @@ -349,7 +348,7 @@ void show_rec(int seq, struct efil_rec_hdr *rec_hdr)
>         case EFILT_FREE_PAGES: {
>                 struct efil_free_pages *rec = start;
>
> -               show_addr("mem", map_to_sysmem((void *)rec->memory));
> +               show_addr("mem", rec->memory);
>                 show_ulong("pag", (ulong)rec->pages);
>                 if (rec_hdr->ended)
>                         show_ret(rec_hdr->e_ret);
> --
> 2.34.1
>

Re: [PATCH 2/8] efi_loader: Fix display of addresses in log

[Simon Glass]

Hi Ilias,

On Tue, 7 Jan 2025 at 06:37, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> On Mon, 6 Jan 2025 at 16:48, Simon Glass <sjg@chromium.org> wrote:
> >
> > The allocate/free-pages functions return an address, so there is no need
> > to convert it. Fix this.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > Fixes: c824a96d76d ("efi_loader: Use the log with memory-related...")
>
> I can't find that commit in -master. Is there anything I am missing?

Sort-of. You, Tom and Heinrich NAKed this series. See [1]

> >  lib/efi_loader/efi_log.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_log.c b/lib/efi_loader/efi_log.c
> > index 4b9c6727fe7..ad490bb431c 100644
> > --- a/lib/efi_loader/efi_log.c
> > +++ b/lib/efi_loader/efi_log.c
> > @@ -340,8 +340,7 @@ void show_rec(int seq, struct efil_rec_hdr *rec_hdr)
> >                 show_ulong("pgs", (ulong)rec->pages);
> >                 show_addr("mem", (ulong)rec->memory);
> >                 if (rec_hdr->ended) {
> > -                       show_addr("*mem",
> > -                                 (ulong)map_to_sysmem((void *)rec->e_memory));
> > +                       show_addr("*mem", rec->e_memory);
> >                         show_ret(rec_hdr->e_ret);
> >                 }
> >                 break;
> > @@ -349,7 +348,7 @@ void show_rec(int seq, struct efil_rec_hdr *rec_hdr)
> >         case EFILT_FREE_PAGES: {
> >                 struct efil_free_pages *rec = start;
> >
> > -               show_addr("mem", map_to_sysmem((void *)rec->memory));
> > +               show_addr("mem", rec->memory);
> >                 show_ulong("pag", (ulong)rec->pages);
> >                 if (rec_hdr->ended)
> >                         show_ret(rec_hdr->e_ret);
> > --
> > 2.34.1
> >

Regards,
Simon

[1] https://lore.kernel.org/u-boot/CAC_iWjKtaN54B98OKbkoXkC_GmKJ=x+M4=UY_O6roSOpZaDxag@mail.gmail.com/

Re: [PATCH 2/8] efi_loader: Fix display of addresses in log

[Ilias Apalodimas]

On Tue, 7 Jan 2025 at 15:58, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Tue, 7 Jan 2025 at 06:37, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Mon, 6 Jan 2025 at 16:48, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > The allocate/free-pages functions return an address, so there is no need
> > > to convert it. Fix this.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > Fixes: c824a96d76d ("efi_loader: Use the log with memory-related...")
> >
> > I can't find that commit in -master. Is there anything I am missing?
>
> Sort-of. You, Tom and Heinrich NAKed this series. See [1]

Ok, please don't cc me on patches that are not destined for master or
next. I have no interest in out-of-tree work


Thanks
/Ilias
>
> > >  lib/efi_loader/efi_log.c | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/lib/efi_loader/efi_log.c b/lib/efi_loader/efi_log.c
> > > index 4b9c6727fe7..ad490bb431c 100644
> > > --- a/lib/efi_loader/efi_log.c
> > > +++ b/lib/efi_loader/efi_log.c
> > > @@ -340,8 +340,7 @@ void show_rec(int seq, struct efil_rec_hdr *rec_hdr)
> > >                 show_ulong("pgs", (ulong)rec->pages);
> > >                 show_addr("mem", (ulong)rec->memory);
> > >                 if (rec_hdr->ended) {
> > > -                       show_addr("*mem",
> > > -                                 (ulong)map_to_sysmem((void *)rec->e_memory));
> > > +                       show_addr("*mem", rec->e_memory);
> > >                         show_ret(rec_hdr->e_ret);
> > >                 }
> > >                 break;
> > > @@ -349,7 +348,7 @@ void show_rec(int seq, struct efil_rec_hdr *rec_hdr)
> > >         case EFILT_FREE_PAGES: {
> > >                 struct efil_free_pages *rec = start;
> > >
> > > -               show_addr("mem", map_to_sysmem((void *)rec->memory));
> > > +               show_addr("mem", rec->memory);
> > >                 show_ulong("pag", (ulong)rec->pages);
> > >                 if (rec_hdr->ended)
> > >                         show_ret(rec_hdr->e_ret);
> > > --
> > > 2.34.1
> > >
>
> Regards,
> Simon
>
> [1] https://lore.kernel.org/u-boot/CAC_iWjKtaN54B98OKbkoXkC_GmKJ=x+M4=UY_O6roSOpZaDxag@mail.gmail.com/

Re: [PATCH 2/8] efi_loader: Fix display of addresses in log

[Simon Glass]

Hi Ilias,

On Wed, 8 Jan 2025 at 00:12, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Tue, 7 Jan 2025 at 15:58, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Tue, 7 Jan 2025 at 06:37, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Mon, 6 Jan 2025 at 16:48, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > The allocate/free-pages functions return an address, so there is no need
> > > > to convert it. Fix this.
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > Fixes: c824a96d76d ("efi_loader: Use the log with memory-related...")
> > >
> > > I can't find that commit in -master. Is there anything I am missing?
> >
> > Sort-of. You, Tom and Heinrich NAKed this series. See [1]
>
> Ok, please don't cc me on patches that are not destined for master or
> next. I have no interest in out-of-tree work

Everything is out-of-tree until it is applied.

Yes, I'll try to drop you from future patches.

Regards,
Simon

[PATCH 3/8] efi_loader: Return the memory map in pointer format

[Simon Glass]

An EFI app expects pointers to be returned, cast to u64. The conversion
to use addresses missed this, so fix the call.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 lib/efi_loader/efi_memory.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 321a8c3cf31..2e197e94edd 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -735,10 +735,11 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
 	list_for_each_entry(lmem, &efi_mem, link) {
 		memory_map->type = lmem->type;
 		memory_map->reserved = 0;
-		memory_map->physical_start = lmem->base;
+		memory_map->physical_start = (u64)(ulong)map_sysmem(lmem->base,
+					lmem->num_pages * EFI_PAGE_SIZE);
 
 		/* virtual and physical are always the same */
-		memory_map->virtual_start = lmem->base;
+		memory_map->virtual_start = memory_map->physical_start;
 		memory_map->num_pages = lmem->num_pages;
 		memory_map->attribute = lmem->attribute;
 		memory_map--;
-- 
2.34.1

[PATCH 4/8] efi_loader: Correct bounce-buffer setup

[Simon Glass]

This should set the bounce buffer to a pointer, not an address. Fix it.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 lib/efi_loader/efi_memory.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 2e197e94edd..9374f77d3b7 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -837,11 +837,11 @@ int efi_memory_init(void)
 	uint64_t efi_bounce_buffer_addr = 0xffffffff;
 
 	if (efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_BOOT_SERVICES_DATA,
-			       (64 * 1024 * 1024) >> EFI_PAGE_SHIFT,
+			       SZ_64M >> EFI_PAGE_SHIFT,
 			       &efi_bounce_buffer_addr) != EFI_SUCCESS)
 		return -1;
 
-	efi_bounce_buffer = (void*)(uintptr_t)efi_bounce_buffer_addr;
+	efi_bounce_buffer = map_sysmem(efi_bounce_buffer_addr, SZ_64M);
 #endif
 
 	return 0;
-- 
2.34.1

Re: [PATCH 4/8] efi_loader: Correct bounce-buffer setup

[Ilias Apalodimas]

Hi Simon,

Is this patch for -master of your tree?

On Mon, 6 Jan 2025 at 16:48, Simon Glass <sjg@chromium.org> wrote:
>
> This should set the bounce buffer to a pointer, not an address. Fix it.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  lib/efi_loader/efi_memory.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 2e197e94edd..9374f77d3b7 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -837,11 +837,11 @@ int efi_memory_init(void)
>         uint64_t efi_bounce_buffer_addr = 0xffffffff;
>
>         if (efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_BOOT_SERVICES_DATA,
> -                              (64 * 1024 * 1024) >> EFI_PAGE_SHIFT,
> +                              SZ_64M >> EFI_PAGE_SHIFT,
>                                &efi_bounce_buffer_addr) != EFI_SUCCESS)
>                 return -1;
>
> -       efi_bounce_buffer = (void*)(uintptr_t)efi_bounce_buffer_addr;
> +       efi_bounce_buffer = map_sysmem(efi_bounce_buffer_addr, SZ_64M);

map_sysmem does *exactly* the same thing so you need to adjust your
commit message.
Does this solve any sandbox crashes?

/Ilias
>  #endif
>
>         return 0;
> --
> 2.34.1
>

Re: [PATCH 4/8] efi_loader: Correct bounce-buffer setup

[Simon Glass]

Hi Ilias,

On Fri, 17 Jan 2025 at 03:34, Ilias Apalodimas <ilias.apalodimas@linaro.org>
wrote:
>
> Hi Simon,
>
> Is this patch for -master of your tree?

Yes, but I haven't applied it yet. The updated bootflow_efi() test ([1]
which this is part of) is throwing up problems with how testing is done. We
cannot today reinit EFI so only get to run one test which does an EFI boot.
I'd like to improve that at some point.

The address/pointer tidy-up was rejected as well, I think by you, but
perhaps Heinrich?

>
> On Mon, 6 Jan 2025 at 16:48, Simon Glass <sjg@chromium.org> wrote:
> >
> > This should set the bounce buffer to a pointer, not an address. Fix it.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  lib/efi_loader/efi_memory.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> > index 2e197e94edd..9374f77d3b7 100644
> > --- a/lib/efi_loader/efi_memory.c
> > +++ b/lib/efi_loader/efi_memory.c
> > @@ -837,11 +837,11 @@ int efi_memory_init(void)
> >         uint64_t efi_bounce_buffer_addr = 0xffffffff;
> >
> >         if (efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS,
EFI_BOOT_SERVICES_DATA,
> > -                              (64 * 1024 * 1024) >> EFI_PAGE_SHIFT,
> > +                              SZ_64M >> EFI_PAGE_SHIFT,
> >                                &efi_bounce_buffer_addr) != EFI_SUCCESS)
> >                 return -1;
> >
> > -       efi_bounce_buffer = (void*)(uintptr_t)efi_bounce_buffer_addr;
> > +       efi_bounce_buffer = map_sysmem(efi_bounce_buffer_addr, SZ_64M);
>
> map_sysmem does *exactly* the same thing so you need to adjust your
> commit message.
>
> Does this solve any sandbox crashes?

No, sandbox doesn't test this feature yet, but the new code is more
correct. Whenever you see an address, use map_sysmem() to turn it into a
pointer.

>
> /Ilias
> >  #endif
> >
> >         return 0;
> > --
> > 2.34.1
> >

Regards,
Simon

[1] https://patchwork.ozlabs.org/project/uboot/list/?series=439021
[2] https://patchwork.ozlabs.org/project/uboot/list/?series=436241

[PATCH 5/8] efi_loader: Check memory allocations in bootflow_efi test()

[Simon Glass]

Add checks that all memory allocations complete successfully and that
only expected parameters are provided. Check that memory addresses are
within range.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 test/boot/bootflow.c | 64 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 63 insertions(+), 1 deletion(-)

diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
index 35580cee90c..8b1bc9f10ab 100644
--- a/test/boot/bootflow.c
+++ b/test/boot/bootflow.c
@@ -6,6 +6,7 @@
  * Written by Simon Glass <sjg@chromium.org>
  */
 
+#include <bloblist.h>
 #include <bootdev.h>
 #include <bootflow.h>
 #include <bootmeth.h>
@@ -14,6 +15,7 @@
 #include <dm.h>
 #include <efi.h>
 #include <efi_loader.h>
+#include <efi_log.h>
 #include <expo.h>
 #include <mapmem.h>
 #ifdef CONFIG_SANDBOX
@@ -1271,10 +1273,13 @@ BOOTSTD_TEST(bootflow_android_image_v2, UTF_CONSOLE | UTF_DM | UTF_SCAN_FDT);
 /* Test EFI bootmeth */
 static int bootflow_efi(struct unit_test_state *uts)
 {
+	struct efil_hdr *hdr = bloblist_find(BLOBLISTT_EFI_LOG, 0);
 	static const char *order[] = {"mmc1", "usb", NULL};
+	struct efil_rec_hdr *rec_hdr;
 	struct bootstd_priv *std;
 	struct udevice *bootstd;
 	const char **old_order;
+	int i;
 
 	ut_assertok(uclass_first_device_err(UCLASS_BOOTSTD, &bootstd));
 	std = dev_get_priv(bootstd);
@@ -1325,7 +1330,64 @@ static int bootflow_efi(struct unit_test_state *uts)
 
 	ut_assert_console_end();
 
-	ut_assertok(bootstd_test_drop_bootdev_order(uts));
+	/* check memory allocations are as expected */
+	if (!hdr)
+		return 0;
+
+	for (i = 0, rec_hdr = (void *)hdr + sizeof(*hdr);
+	     (void *)rec_hdr - (void *)hdr < hdr->upto;
+	     i++, rec_hdr = (void *)rec_hdr + rec_hdr->size) {
+		void *start = (void *)rec_hdr + sizeof(struct efil_rec_hdr);
+
+		switch (rec_hdr->tag) {
+		case EFILT_ALLOCATE_PAGES: {
+			struct efil_allocate_pages *rec = start;
+
+			ut_assert(rec_hdr->ended);
+			ut_assertok(rec_hdr->e_ret);
+			ut_asserteq(EFI_ALLOCATE_ANY_PAGES, rec->type);
+			log_debug("rec->memory_type %d\n", rec->memory_type);
+			ut_assert(rec->memory_type == EFI_RUNTIME_SERVICES_DATA ||
+				  rec->memory_type == EFI_BOOT_SERVICES_DATA ||
+				  rec->memory_type == EFI_ACPI_MEMORY_NVS ||
+				  rec->memory_type == EFI_LOADER_CODE);
+			ut_assert(rec->e_memory < gd->ram_size);
+			break;
+		}
+		case EFILT_FREE_PAGES: {
+			struct efil_free_pages *rec = start;
+
+			ut_assert(rec_hdr->ended);
+			ut_assertok(rec_hdr->e_ret);
+			ut_assert(rec->memory < gd->ram_size);
+			break;
+		}
+		case EFILT_ALLOCATE_POOL: {
+			struct efil_allocate_pool *rec = start;
+
+			ut_assert(rec_hdr->ended);
+			ut_assertok(rec_hdr->e_ret);
+			log_debug("rec->pool_type %d\n", rec->pool_type);
+			ut_assert(rec->pool_type == EFI_RUNTIME_SERVICES_DATA ||
+				  rec->pool_type == EFI_BOOT_SERVICES_DATA ||
+				  rec->pool_type == EFI_ACPI_MEMORY_NVS);
+			ut_assert(map_to_sysmem((void *)rec->e_buffer) <
+				  gd->ram_size);
+			break;
+		}
+		case EFILT_FREE_POOL: {
+			struct efil_free_pool *rec = start;
+
+			ut_assert(rec_hdr->ended);
+			ut_assertok(rec_hdr->e_ret);
+			ut_assert(map_to_sysmem((void *)rec->buffer) <
+				  gd->ram_size);
+			break;
+		}
+		default:
+			break;
+		}
+	}
 
 	return 0;
 }
-- 
2.34.1

[PATCH 6/8] efi_loader: Update testapp to get memory map correctly

[Simon Glass]

Allocate enough memory for the memory map so that it can be received
successfully.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 lib/efi_loader/testapp.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/lib/efi_loader/testapp.c b/lib/efi_loader/testapp.c
index 804ca7e4679..171ecdab043 100644
--- a/lib/efi_loader/testapp.c
+++ b/lib/efi_loader/testapp.c
@@ -28,7 +28,12 @@ efi_status_t EFIAPI efi_main(efi_handle_t handle,
 			     struct efi_system_table *systab)
 {
 	struct efi_loaded_image *loaded_image;
+	struct efi_mem_desc *map;
 	efi_status_t ret;
+	efi_uintn_t map_size;
+	efi_uintn_t map_key;
+	efi_uintn_t desc_size;
+	u32 desc_version;
 
 	systable = systab;
 	boottime = systable->boottime;
@@ -48,6 +53,29 @@ efi_status_t EFIAPI efi_main(efi_handle_t handle,
 	con_out->output_string(con_out, u"U-Boot test app for EFI_LOADER\r\n");
 
 out:
+	map_size = 0;
+	ret = boottime->get_memory_map(&map_size, NULL, &map_key, &desc_size,
+				       &desc_version);
+	if (ret != EFI_BUFFER_TOO_SMALL) {
+		con_out->output_string(con_out, u"map error A\n");
+		return ret;
+	}
+
+	ret = boottime->allocate_pool(EFI_BOOT_SERVICES_DATA, map_size,
+				      (void **)&map);
+	if (ret) {
+		con_out->output_string(con_out, u"map error B\n");
+		return ret;
+	}
+	/* Allocate extra space for newly allocated memory */
+	map_size += sizeof(struct efi_mem_desc);
+	ret = boottime->get_memory_map(&map_size, map, &map_key, &desc_size,
+				       &desc_version);
+	if (ret) {
+		con_out->output_string(con_out, u"map error C\n");
+		return ret;
+	}
+
 	con_out->output_string(con_out, u"Exiting test app\n");
 	ret = boottime->exit(handle, ret, 0, NULL);
 
-- 
2.34.1

[PATCH 7/8] efi_loader: Check that the bootflow is not removed by app

[Simon Glass]

The bootflow should not be affected by the app running and returning.
Add a check that it is still valid after the app completes execution.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 test/boot/bootflow.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
index 8b1bc9f10ab..81e81f3cfbf 100644
--- a/test/boot/bootflow.c
+++ b/test/boot/bootflow.c
@@ -1330,6 +1330,9 @@ static int bootflow_efi(struct unit_test_state *uts)
 
 	ut_assert_console_end();
 
+	/* make sure the bootflow is still present */
+	ut_assertnonnull(std->cur_bootflow);
+
 	/* check memory allocations are as expected */
 	if (!hdr)
 		return 0;
-- 
2.34.1

[PATCH 8/8] efi_loader: Test that active-DMA devices are removed

[Simon Glass]

When exit-boot-services is called, active devices should be removed.

Update testapp to call this method and check that the USB controller is
removed as expected.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 lib/efi_loader/testapp.c |  9 +++++++++
 test/boot/bootflow.c     | 11 +++++++++++
 2 files changed, 20 insertions(+)

diff --git a/lib/efi_loader/testapp.c b/lib/efi_loader/testapp.c
index 171ecdab043..2c89e14bc5d 100644
--- a/lib/efi_loader/testapp.c
+++ b/lib/efi_loader/testapp.c
@@ -76,6 +76,15 @@ out:
 		return ret;
 	}
 
+	/* exit boot services so that this part of U-Boot can be tested */
+	con_out->output_string(con_out, u"Exiting boot services\n");
+	ret = boottime->exit_boot_services(handle, map_key);
+	if (ret) {
+		con_out->output_string(con_out, u"Failed exit-boot-services\n");
+		return ret;
+	}
+
+	/* now exit for real */
 	con_out->output_string(con_out, u"Exiting test app\n");
 	ret = boottime->exit(handle, ret, 0, NULL);
 
diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
index 81e81f3cfbf..bd96564c915 100644
--- a/test/boot/bootflow.c
+++ b/test/boot/bootflow.c
@@ -23,6 +23,7 @@
 #endif
 #include <dm/device-internal.h>
 #include <dm/lists.h>
+#include <dm/uclass-internal.h>
 #include <linux/libfdt.h>
 #include <test/suites.h>
 #include <test/ut.h>
@@ -1279,6 +1280,7 @@ static int bootflow_efi(struct unit_test_state *uts)
 	struct bootstd_priv *std;
 	struct udevice *bootstd;
 	const char **old_order;
+	struct udevice *usb;
 	int i;
 
 	ut_assertok(uclass_first_device_err(UCLASS_BOOTSTD, &bootstd));
@@ -1315,6 +1317,10 @@ static int bootflow_efi(struct unit_test_state *uts)
 
 	systab.fw_vendor = test_vendor;
 
+	/* the USB block-device should have beeen probed */
+	ut_assertok(uclass_find_device_by_seq(UCLASS_USB, 1, &usb));
+	ut_assert(device_active(usb));
+
 	ut_asserteq(1, run_command("bootflow boot", 0));
 	ut_assert_nextline(
 		"** Booting bootflow 'usb_mass_storage.lun0.bootdev.part_1' with efi");
@@ -1325,6 +1331,7 @@ static int bootflow_efi(struct unit_test_state *uts)
 
 	/* TODO: Why the \r ? */
 	ut_assert_nextline("U-Boot test app for EFI_LOADER\r");
+	ut_assert_nextline("Exiting boot services");
 	ut_assert_nextline("Exiting test app");
 	ut_assert_nextline("Boot failed (err=-14)");
 
@@ -1333,6 +1340,10 @@ static int bootflow_efi(struct unit_test_state *uts)
 	/* make sure the bootflow is still present */
 	ut_assertnonnull(std->cur_bootflow);
 
+	/* the USB block-device should have beeen removed */
+	ut_assertok(uclass_find_device_by_seq(UCLASS_USB, 1, &usb));
+	ut_assert(!device_active(usb));
+
 	/* check memory allocations are as expected */
 	if (!hdr)
 		return 0;
-- 
2.34.1

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

[Heinrich Schuchardt]

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.

Tom suggested not to send patches that are for private enjoyment to the
mailing list.

Best regards

Heinrich

>
>
> Simon Glass (8):
>    sandbox: Make USB controller as having active DMA
>    efi_loader: Fix display of addresses in log
>    efi_loader: Return the memory map in pointer format
>    efi_loader: Correct bounce-buffer setup
>    efi_loader: Check memory allocations in bootflow_efi test()
>    efi_loader: Update testapp to get memory map correctly
>    efi_loader: Check that the bootflow is not removed by app
>    efi_loader: Test that active-DMA devices are removed
>
>   drivers/usb/host/usb-sandbox.c |  1 +
>   lib/efi_loader/efi_log.c       |  5 +--
>   lib/efi_loader/efi_memory.c    |  9 ++--
>   lib/efi_loader/testapp.c       | 37 ++++++++++++++++
>   test/boot/bootflow.c           | 78 +++++++++++++++++++++++++++++++++-
>   5 files changed, 122 insertions(+), 8 deletions(-)
>

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

[Simon Glass]

Hi Heinrich,

On Mon, 6 Jan 2025 at 10:00, Heinrich Schuchardt <xypron.glpk@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.

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

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

[Heinrich Schuchardt]

On 07.01.25 13:15, Simon Glass wrote:
> Hi Heinrich,
>
> On Mon, 6 Jan 2025 at 10:00, Heinrich Schuchardt <xypron.glpk@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?

Best regards

Heinrich

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

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

[Simon Glass]

Hi Heinrich,

On Tue, 7 Jan 2025 at 06:11, Heinrich Schuchardt <xypron.glpk@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@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/

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

[Tom Rini]

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

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@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@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?

-- 
Tom

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

Re: [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@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@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

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

[Caleb Connolly]

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@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@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:

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

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.

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.
> 
> Best regards
> 
> Heinrich

-- 
// Caleb (they/them)

Re: [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@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@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@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:
>
> 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

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

[Simon Glass]

Hi Heinrich, Tom,

On Tue, 7 Jan 2025 at 08:47, Heinrich Schuchardt <xypron.glpk@gmx.de> 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@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@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.

Are you asking for code comments in the test? If so, I can add some.

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

It is a pretty good start, though. It makes sure that the memory types
are sane, checks addresses are within DRAM, etc. With [5] it makes
sure that devices are removed.

>
> Implementing a specific tracing functionality for EFI is definitively
> the wrong way forward as it will lead to code duplication.

We can cross that bridge when we come to it.

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

Perhaps the word 'log' is confusing people. But the above suggestion
is quite a complicated way of handling things. We have no way to
decode printf() strings in this way. See log_dispatch() for how this
is handled today. It uses sprintf(). Trying to test based on text
output would be very clumsy (lots of regexes and sscan() calls?) and
result in a huge amount of parsing code, highly dependent on the
printf() format, etc.

I very-much doubt that would produce a useful implementation, but if
you would like to try it out then I would be happy to look at it.

I mentioned this several times, but even if we did go that way, we
only have logging on the external calls, so much of the EFI-memory
allocation in U-Boot would not be logged.

Regards,
Simon

[5] https://patchwork.ozlabs.org/project/uboot/patch/20250106144755.3054780-9-sjg@chromium.org/

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

[Tom Rini]

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

On Wed, Jan 08, 2025 at 10:02:52AM -0700, Simon Glass wrote:
> Hi Heinrich, Tom,
> 
> On Tue, 7 Jan 2025 at 08:47, Heinrich Schuchardt <xypron.glpk@gmx.de> 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@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@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.
> 
> Are you asking for code comments in the test? If so, I can add some.
> 
> >
> > 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.
> 
> It is a pretty good start, though. It makes sure that the memory types
> are sane, checks addresses are within DRAM, etc. With [5] it makes
> sure that devices are removed.
> 
> >
> > Implementing a specific tracing functionality for EFI is definitively
> > the wrong way forward as it will lead to code duplication.
> 
> We can cross that bridge when we come to it.

Well, no. It's backwards to make a bridge in one place when everyone
agrees it needs to be moved somewhere else. I mean [5] is a generic
issue and test/py/tests/test_net_boot.py or some other test we already
have which tests booting an OS should confirm that we've quiesced
devices before moving on. And as a bonus it's in python where dealing
with strings doesn't suck.

> 
> >
> > 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.
> 
> Perhaps the word 'log' is confusing people. But the above suggestion
> is quite a complicated way of handling things. We have no way to
> decode printf() strings in this way. See log_dispatch() for how this
> is handled today. It uses sprintf(). Trying to test based on text
> output would be very clumsy (lots of regexes and sscan() calls?) and
> result in a huge amount of parsing code, highly dependent on the
> printf() format, etc.
> 
> I very-much doubt that would produce a useful implementation, but if
> you would like to try it out then I would be happy to look at it.
> 
> I mentioned this several times, but even if we did go that way, we
> only have logging on the external calls, so much of the EFI-memory
> allocation in U-Boot would not be logged.
> 
> Regards,
> Simon
> 
> [5] https://patchwork.ozlabs.org/project/uboot/patch/20250106144755.3054780-9-sjg@chromium.org/

Yes, calling this a "log" when it's intended for capturing information
for tests got some of this off on the wrong track. But that also helps
explain now that this is still on the wrong track and should instead be
following normal design practices for testing and expanding existing
infrastructure and not inventing a new everything. So if you don't like
Heinrich's suggestion, take a look at Caleb's suggestion. And if you
don't like Caleb's suggestion, go put this in a topic branch you can
merge when you need to debug some problem that seemingly nothing else
will catch.

-- 
Tom

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

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

[Simon Glass]

Hi Tom,

On Wed, 8 Jan 2025 at 12:15, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Jan 08, 2025 at 10:02:52AM -0700, Simon Glass wrote:
> > Hi Heinrich, Tom,
> >
> > On Tue, 7 Jan 2025 at 08:47, Heinrich Schuchardt <xypron.glpk@gmx.de> 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@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@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.
> >
> > Are you asking for code comments in the test? If so, I can add some.
> >
> > >
> > > 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.
> >
> > It is a pretty good start, though. It makes sure that the memory types
> > are sane, checks addresses are within DRAM, etc. With [5] it makes
> > sure that devices are removed.
> >
> > >
> > > Implementing a specific tracing functionality for EFI is definitively
> > > the wrong way forward as it will lead to code duplication.
> >
> > We can cross that bridge when we come to it.
>
> Well, no. It's backwards to make a bridge in one place when everyone
> agrees it needs to be moved somewhere else. I mean [5] is a generic
> issue and test/py/tests/test_net_boot.py or some other test we already
> have which tests booting an OS should confirm that we've quiesced
> devices before moving on. And as a bonus it's in python where dealing
> with strings doesn't suck.

I really don't want to write C tests in Python. CI is slow enough as
it is, something realy want to fix. I'm also not sure how you can tell
if a device has been removed. Run 'dm tree' and look for the missing
'star' in the resulting 300 lines of text?

But actually [5] is not generic, since EFI uses its own code to remove
devices. This test is solely focussed on EFI.

If you want the logging to be renamed and placed centrally I don't
mind doing it now. But note that only EFI will use it for now.

>
> >
> > >
> > > 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.
> >
> > Perhaps the word 'log' is confusing people. But the above suggestion
> > is quite a complicated way of handling things. We have no way to
> > decode printf() strings in this way. See log_dispatch() for how this
> > is handled today. It uses sprintf(). Trying to test based on text
> > output would be very clumsy (lots of regexes and sscan() calls?) and
> > result in a huge amount of parsing code, highly dependent on the
> > printf() format, etc.
> >
> > I very-much doubt that would produce a useful implementation, but if
> > you would like to try it out then I would be happy to look at it.
> >
> > I mentioned this several times, but even if we did go that way, we
> > only have logging on the external calls, so much of the EFI-memory
> > allocation in U-Boot would not be logged.
> >
> > Regards,
> > Simon
> >
> > [5] https://patchwork.ozlabs.org/project/uboot/patch/20250106144755.3054780-9-sjg@chromium.org/
>
> Yes, calling this a "log" when it's intended for capturing information
> for tests got some of this off on the wrong track. But that also helps
> explain now that this is still on the wrong track and should instead be
> following normal design practices for testing and expanding existing
> infrastructure and not inventing a new everything. So if you don't like
> Heinrich's suggestion, take a look at Caleb's suggestion.

I don't have the energy to port the tracing framework from Linux to
U-Boot, although I agree it would be useful. Still, function tracing
is quite fragile and confusing to work with when refactoring code. I
don't like that idea much for this use case, although if function
tracing did exist in U-Boot I would likely have used it.

> And if you
> don't like Caleb's suggestion, go put this in a topic branch you can
> merge when you need to debug some problem that seemingly nothing else
> will catch.

Here we are over a year after I reported the bug and we still don't
have a test to cover it. This series is better than the available
alternatives, IMO.

Regards,
Simon

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

[Tom Rini]

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

On Thu, Jan 09, 2025 at 08:02:01AM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 8 Jan 2025 at 12:15, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Jan 08, 2025 at 10:02:52AM -0700, Simon Glass wrote:
> > > Hi Heinrich, Tom,
> > >
> > > On Tue, 7 Jan 2025 at 08:47, Heinrich Schuchardt <xypron.glpk@gmx.de> 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@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@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.
> > >
> > > Are you asking for code comments in the test? If so, I can add some.
> > >
> > > >
> > > > 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.
> > >
> > > It is a pretty good start, though. It makes sure that the memory types
> > > are sane, checks addresses are within DRAM, etc. With [5] it makes
> > > sure that devices are removed.
> > >
> > > >
> > > > Implementing a specific tracing functionality for EFI is definitively
> > > > the wrong way forward as it will lead to code duplication.
> > >
> > > We can cross that bridge when we come to it.
> >
> > Well, no. It's backwards to make a bridge in one place when everyone
> > agrees it needs to be moved somewhere else. I mean [5] is a generic
> > issue and test/py/tests/test_net_boot.py or some other test we already
> > have which tests booting an OS should confirm that we've quiesced
> > devices before moving on. And as a bonus it's in python where dealing
> > with strings doesn't suck.
> 
> I really don't want to write C tests in Python. CI is slow enough as
> it is, something realy want to fix. I'm also not sure how you can tell
> if a device has been removed. Run 'dm tree' and look for the missing
> 'star' in the resulting 300 lines of text?

As I'm in a bisect-hell in our C tests you'll have to forgive me for not
thinking the C tests are noticeably faster than python tests. Or that
they aren't their own potential source of corner-case bugs. But I
digress..

And yes, taking a bunch of text and parsing it, is what python is fast
at. And easier to write.

> But actually [5] is not generic, since EFI uses its own code to remove
> devices. This test is solely focussed on EFI.

Yes, you're testing the EFI version of the code in
arch/$(ARCH)/lib/bootm.c. The remove devices functions being called in
both cases are generic.

> If you want the logging to be renamed and placed centrally I don't
> mind doing it now. But note that only EFI will use it for now.
> 
> >
> > >
> > > >
> > > > 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.
> > >
> > > Perhaps the word 'log' is confusing people. But the above suggestion
> > > is quite a complicated way of handling things. We have no way to
> > > decode printf() strings in this way. See log_dispatch() for how this
> > > is handled today. It uses sprintf(). Trying to test based on text
> > > output would be very clumsy (lots of regexes and sscan() calls?) and
> > > result in a huge amount of parsing code, highly dependent on the
> > > printf() format, etc.
> > >
> > > I very-much doubt that would produce a useful implementation, but if
> > > you would like to try it out then I would be happy to look at it.
> > >
> > > I mentioned this several times, but even if we did go that way, we
> > > only have logging on the external calls, so much of the EFI-memory
> > > allocation in U-Boot would not be logged.
> > >
> > > Regards,
> > > Simon
> > >
> > > [5] https://patchwork.ozlabs.org/project/uboot/patch/20250106144755.3054780-9-sjg@chromium.org/
> >
> > Yes, calling this a "log" when it's intended for capturing information
> > for tests got some of this off on the wrong track. But that also helps
> > explain now that this is still on the wrong track and should instead be
> > following normal design practices for testing and expanding existing
> > infrastructure and not inventing a new everything. So if you don't like
> > Heinrich's suggestion, take a look at Caleb's suggestion.
> 
> I don't have the energy to port the tracing framework from Linux to
> U-Boot, although I agree it would be useful. Still, function tracing
> is quite fragile and confusing to work with when refactoring code. I
> don't like that idea much for this use case, although if function
> tracing did exist in U-Boot I would likely have used it.

I mean yes, it would be good if you went back and expanded on the trace
functionality you did before.

> > And if you
> > don't like Caleb's suggestion, go put this in a topic branch you can
> > merge when you need to debug some problem that seemingly nothing else
> > will catch.
> 
> Here we are over a year after I reported the bug and we still don't
> have a test to cover it. This series is better than the available
> alternatives, IMO.

Well, no. We have commit dabaa4ae3206 ("dm: Add
dm_remove_devices_active() for ordered device removal") we have a test
for the underlying problem. We need more functional boot tests, but we
need those to be in python too, and not more C code.

And you're not just coming up with a test, you're refactoring a bunch of
code and introducing new subsystems in order to do that. When as I keep
pointing out, we don't need that. We could easily extend the existing OS
boot tests we have to script booting an ISO. And we only run those when
say "ENABLE_SLOW_TESTS" is set, and only do that on tagged releases.

-- 
Tom

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

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

[Simon Glass]

Hi Tom,

On Thu, 9 Jan 2025 at 09:51, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Jan 09, 2025 at 08:02:01AM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Wed, 8 Jan 2025 at 12:15, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Jan 08, 2025 at 10:02:52AM -0700, Simon Glass wrote:
> > > > Hi Heinrich, Tom,
> > > >
> > > > On Tue, 7 Jan 2025 at 08:47, Heinrich Schuchardt <xypron.glpk@gmx.de> 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@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@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.
> > > >
> > > > Are you asking for code comments in the test? If so, I can add some.
> > > >
> > > > >
> > > > > 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.
> > > >
> > > > It is a pretty good start, though. It makes sure that the memory types
> > > > are sane, checks addresses are within DRAM, etc. With [5] it makes
> > > > sure that devices are removed.
> > > >
> > > > >
> > > > > Implementing a specific tracing functionality for EFI is definitively
> > > > > the wrong way forward as it will lead to code duplication.
> > > >
> > > > We can cross that bridge when we come to it.
> > >
> > > Well, no. It's backwards to make a bridge in one place when everyone
> > > agrees it needs to be moved somewhere else. I mean [5] is a generic
> > > issue and test/py/tests/test_net_boot.py or some other test we already
> > > have which tests booting an OS should confirm that we've quiesced
> > > devices before moving on. And as a bonus it's in python where dealing
> > > with strings doesn't suck.
> >
> > I really don't want to write C tests in Python. CI is slow enough as
> > it is, something realy want to fix. I'm also not sure how you can tell
> > if a device has been removed. Run 'dm tree' and look for the missing
> > 'star' in the resulting 300 lines of text?
>
> As I'm in a bisect-hell in our C tests you'll have to forgive me for not
> thinking the C tests are noticeably faster than python tests. Or that
> they aren't their own potential source of corner-case bugs. But I
> digress..

Welcome to my world. I bisected my lab devices so many times to try to
isolate all the breakages that have crept in. What is the problem,
maybe I can help?

>
> And yes, taking a bunch of text and parsing it, is what python is fast
> at. And easier to write.
>
> > But actually [5] is not generic, since EFI uses its own code to remove
> > devices. This test is solely focussed on EFI.
>
> Yes, you're testing the EFI version of the code in
> arch/$(ARCH)/lib/bootm.c. The remove devices functions being called in
> both cases are generic.

The code in EFI is:

if (!efi_st_keep_devices) {
bootm_disable_interrupts();
if (IS_ENABLED(CONFIG_USB_DEVICE))
udc_disconnect();
board_quiesce_devices();
dm_remove_devices_active();
}

It does call somewhat the same functions, but is doing its own thing,
not even using the arch-specific code. As I mentioned, a nice clean-up
would be to make bootm_announce_and_cleanup() common.

Actually, now that I see efi_st_keep_devices, I wonder why Heinrich
didn't want my ANSI patch[6] which serves a similar function.

>
> > If you want the logging to be renamed and placed centrally I don't
> > mind doing it now. But note that only EFI will use it for now.
> >
> > >
> > > >
> > > > >
> > > > > 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.
> > > >
> > > > Perhaps the word 'log' is confusing people. But the above suggestion
> > > > is quite a complicated way of handling things. We have no way to
> > > > decode printf() strings in this way. See log_dispatch() for how this
> > > > is handled today. It uses sprintf(). Trying to test based on text
> > > > output would be very clumsy (lots of regexes and sscan() calls?) and
> > > > result in a huge amount of parsing code, highly dependent on the
> > > > printf() format, etc.
> > > >
> > > > I very-much doubt that would produce a useful implementation, but if
> > > > you would like to try it out then I would be happy to look at it.
> > > >
> > > > I mentioned this several times, but even if we did go that way, we
> > > > only have logging on the external calls, so much of the EFI-memory
> > > > allocation in U-Boot would not be logged.
> > > >
> > > > Regards,
> > > > Simon
> > > >
> > > > [5] https://patchwork.ozlabs.org/project/uboot/patch/20250106144755.3054780-9-sjg@chromium.org/
> > >
> > > Yes, calling this a "log" when it's intended for capturing information
> > > for tests got some of this off on the wrong track. But that also helps
> > > explain now that this is still on the wrong track and should instead be
> > > following normal design practices for testing and expanding existing
> > > infrastructure and not inventing a new everything. So if you don't like
> > > Heinrich's suggestion, take a look at Caleb's suggestion.
> >
> > I don't have the energy to port the tracing framework from Linux to
> > U-Boot, although I agree it would be useful. Still, function tracing
> > is quite fragile and confusing to work with when refactoring code. I
> > don't like that idea much for this use case, although if function
> > tracing did exist in U-Boot I would likely have used it.
>
> I mean yes, it would be good if you went back and expanded on the trace
> functionality you did before.

I still don't believe it is the best solution and seems like yet
another ocean I should avoid sticking my heater into.

>
> > > And if you
> > > don't like Caleb's suggestion, go put this in a topic branch you can
> > > merge when you need to debug some problem that seemingly nothing else
> > > will catch.
> >
> > Here we are over a year after I reported the bug and we still don't
> > have a test to cover it. This series is better than the available
> > alternatives, IMO.
>
> Well, no. We have commit dabaa4ae3206 ("dm: Add
> dm_remove_devices_active() for ordered device removal") we have a test
> for the underlying problem. We need more functional boot tests, but we
> need those to be in python too, and not more C code.

That is a nice improvement, but did not fix the underlying problem.
The underlying problem was that EFI was calling exit-boot-services,
causing U-Boot to free up data structures which were needed to boot.
This was on x86_64. I never quite figured out which one (very hard
when you cannot get back to U-Boot to check).

There were quite a lot of problems, actually. There v2 series is at [7]

Only a C test can check what actually happens inside U-Boot.

>
> And you're not just coming up with a test, you're refactoring a bunch of
> code and introducing new subsystems in order to do that. When as I keep
> pointing out, we don't need that. We could easily extend the existing OS
> boot tests we have to script booting an ISO. And we only run those when
> say "ENABLE_SLOW_TESTS" is set, and only do that on tagged releases.

Yes of course we need to refactor to make tests work. This is not
necessarily a bad thing, as it helps us break code down into testable
chunks. We cannot rely only on large functional-tests, not that you
are suggesting that. See [8], but they are too slow, too hard to debug
when they fail. They also tend to devolve into chaos as people get
lazy and stop writing unit/smaller tests.

Regards,
Simon

>
> --
> Tom

[6] https://patchwork.ozlabs.org/project/uboot/patch/20231121113557.800353-5-sjg@chromium.org/
[7] https://patchwork.ozlabs.org/project/uboot/cover/20240806125850.2316956-1-sjg@chromium.org/
[8] https://circleci.com/blog/testing-pyramid/

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

[Tom Rini]

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

On Fri, Jan 10, 2025 at 06:40:37AM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 9 Jan 2025 at 09:51, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Jan 09, 2025 at 08:02:01AM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Wed, 8 Jan 2025 at 12:15, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Wed, Jan 08, 2025 at 10:02:52AM -0700, Simon Glass wrote:
> > > > > Hi Heinrich, Tom,
> > > > >
> > > > > On Tue, 7 Jan 2025 at 08:47, Heinrich Schuchardt <xypron.glpk@gmx.de> 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@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@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.
> > > > >
> > > > > Are you asking for code comments in the test? If so, I can add some.
> > > > >
> > > > > >
> > > > > > 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.
> > > > >
> > > > > It is a pretty good start, though. It makes sure that the memory types
> > > > > are sane, checks addresses are within DRAM, etc. With [5] it makes
> > > > > sure that devices are removed.
> > > > >
> > > > > >
> > > > > > Implementing a specific tracing functionality for EFI is definitively
> > > > > > the wrong way forward as it will lead to code duplication.
> > > > >
> > > > > We can cross that bridge when we come to it.
> > > >
> > > > Well, no. It's backwards to make a bridge in one place when everyone
> > > > agrees it needs to be moved somewhere else. I mean [5] is a generic
> > > > issue and test/py/tests/test_net_boot.py or some other test we already
> > > > have which tests booting an OS should confirm that we've quiesced
> > > > devices before moving on. And as a bonus it's in python where dealing
> > > > with strings doesn't suck.
> > >
> > > I really don't want to write C tests in Python. CI is slow enough as
> > > it is, something realy want to fix. I'm also not sure how you can tell
> > > if a device has been removed. Run 'dm tree' and look for the missing
> > > 'star' in the resulting 300 lines of text?
> >
> > As I'm in a bisect-hell in our C tests you'll have to forgive me for not
> > thinking the C tests are noticeably faster than python tests. Or that
> > they aren't their own potential source of corner-case bugs. But I
> > digress..
> 
> Welcome to my world. I bisected my lab devices so many times to try to
> isolate all the breakages that have crept in. What is the problem,
> maybe I can help?

Sure. test/cmd/hash.c::dm_test_cmd_hash_md5 fails randomly, in maybe 1
out of 100 runs, via pytest, in sandbox. Not via "./u-boot -T -c 'ut dm
dm_test_cmd_hash_md5'" however (I stopped checking after 1000
iterations). I was iterating over "and built with clang" but I think it
happens with gcc too, from the actual failures in CI. And you can use
"-k ut" to limit to just what's matched there, so it's a quicker
iteration.

> > And yes, taking a bunch of text and parsing it, is what python is fast
> > at. And easier to write.
> >
> > > But actually [5] is not generic, since EFI uses its own code to remove
> > > devices. This test is solely focussed on EFI.
> >
> > Yes, you're testing the EFI version of the code in
> > arch/$(ARCH)/lib/bootm.c. The remove devices functions being called in
> > both cases are generic.
> 
> The code in EFI is:
> 
> if (!efi_st_keep_devices) {
> bootm_disable_interrupts();
> if (IS_ENABLED(CONFIG_USB_DEVICE))
> udc_disconnect();
> board_quiesce_devices();
> dm_remove_devices_active();
> }
> 
> It does call somewhat the same functions, but is doing its own thing,
> not even using the arch-specific code. As I mentioned, a nice clean-up
> would be to make bootm_announce_and_cleanup() common.

Yes, we almost agree? Both the EFI code, and arch/$(ARCH)/lib/bootm.c
have functions that make the above calls. A nice clean-up would be to
have something common.

> 
> Actually, now that I see efi_st_keep_devices, I wonder why Heinrich
> didn't want my ANSI patch[6] which serves a similar function.

No? Your patch disables ANSI output in those tests, that variable is for
making sure those tests can accomplish (if I skim things right) similar
kinds of tests you've asked for before, but with an EFI app instead? But
perhaps better to not start yet another tangent here...

> > > If you want the logging to be renamed and placed centrally I don't
> > > mind doing it now. But note that only EFI will use it for now.
> > >
> > > >
> > > > >
> > > > > >
> > > > > > 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.
> > > > >
> > > > > Perhaps the word 'log' is confusing people. But the above suggestion
> > > > > is quite a complicated way of handling things. We have no way to
> > > > > decode printf() strings in this way. See log_dispatch() for how this
> > > > > is handled today. It uses sprintf(). Trying to test based on text
> > > > > output would be very clumsy (lots of regexes and sscan() calls?) and
> > > > > result in a huge amount of parsing code, highly dependent on the
> > > > > printf() format, etc.
> > > > >
> > > > > I very-much doubt that would produce a useful implementation, but if
> > > > > you would like to try it out then I would be happy to look at it.
> > > > >
> > > > > I mentioned this several times, but even if we did go that way, we
> > > > > only have logging on the external calls, so much of the EFI-memory
> > > > > allocation in U-Boot would not be logged.
> > > > >
> > > > > Regards,
> > > > > Simon
> > > > >
> > > > > [5] https://patchwork.ozlabs.org/project/uboot/patch/20250106144755.3054780-9-sjg@chromium.org/
> > > >
> > > > Yes, calling this a "log" when it's intended for capturing information
> > > > for tests got some of this off on the wrong track. But that also helps
> > > > explain now that this is still on the wrong track and should instead be
> > > > following normal design practices for testing and expanding existing
> > > > infrastructure and not inventing a new everything. So if you don't like
> > > > Heinrich's suggestion, take a look at Caleb's suggestion.
> > >
> > > I don't have the energy to port the tracing framework from Linux to
> > > U-Boot, although I agree it would be useful. Still, function tracing
> > > is quite fragile and confusing to work with when refactoring code. I
> > > don't like that idea much for this use case, although if function
> > > tracing did exist in U-Boot I would likely have used it.
> >
> > I mean yes, it would be good if you went back and expanded on the trace
> > functionality you did before.
> 
> I still don't believe it is the best solution and seems like yet
> another ocean I should avoid sticking my heater into.

I strongly disagree. If you go back to the trace code you brought in to
start with and make it more useful / include newer features existing
elsewhere you're not going to end up in conflict with everyone asking
why you're doing something subsystem specific.

> > > > And if you
> > > > don't like Caleb's suggestion, go put this in a topic branch you can
> > > > merge when you need to debug some problem that seemingly nothing else
> > > > will catch.
> > >
> > > Here we are over a year after I reported the bug and we still don't
> > > have a test to cover it. This series is better than the available
> > > alternatives, IMO.
> >
> > Well, no. We have commit dabaa4ae3206 ("dm: Add
> > dm_remove_devices_active() for ordered device removal") we have a test
> > for the underlying problem. We need more functional boot tests, but we
> > need those to be in python too, and not more C code.
> 
> That is a nice improvement, but did not fix the underlying problem.
> The underlying problem was that EFI was calling exit-boot-services,
> causing U-Boot to free up data structures which were needed to boot.
> This was on x86_64. I never quite figured out which one (very hard
> when you cannot get back to U-Boot to check).
> 
> There were quite a lot of problems, actually. There v2 series is at [7]
> 
> Only a C test can check what actually happens inside U-Boot.

Yes, I think now we get back to disagreeing on which symptoms lead to
which code problems and then what to do about them.

> > And you're not just coming up with a test, you're refactoring a bunch of
> > code and introducing new subsystems in order to do that. When as I keep
> > pointing out, we don't need that. We could easily extend the existing OS
> > boot tests we have to script booting an ISO. And we only run those when
> > say "ENABLE_SLOW_TESTS" is set, and only do that on tagged releases.
> 
> Yes of course we need to refactor to make tests work. This is not
> necessarily a bad thing, as it helps us break code down into testable
> chunks. We cannot rely only on large functional-tests, not that you
> are suggesting that. See [8], but they are too slow, too hard to debug
> when they fail. They also tend to devolve into chaos as people get
> lazy and stop writing unit/smaller tests.

I'll just note that I don't ever even think to use "make tests" or
"qcheck" or any of the others since they never work for me. With only a
little bit of wrappering I can however run pytest like in CI.

-- 
Tom

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

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

[Simon Glass]

Hi Tom,

On Fri, 10 Jan 2025 at 09:48, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Jan 10, 2025 at 06:40:37AM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 9 Jan 2025 at 09:51, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Thu, Jan 09, 2025 at 08:02:01AM -0700, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Wed, 8 Jan 2025 at 12:15, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Wed, Jan 08, 2025 at 10:02:52AM -0700, Simon Glass wrote:
> > > > > > Hi Heinrich, Tom,
> > > > > >
> > > > > > On Tue, 7 Jan 2025 at 08:47, Heinrich Schuchardt <xypron.glpk@gmx.de> 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@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@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.
> > > > > >
> > > > > > Are you asking for code comments in the test? If so, I can add some.
> > > > > >
> > > > > > >
> > > > > > > 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.
> > > > > >
> > > > > > It is a pretty good start, though. It makes sure that the memory types
> > > > > > are sane, checks addresses are within DRAM, etc. With [5] it makes
> > > > > > sure that devices are removed.
> > > > > >
> > > > > > >
> > > > > > > Implementing a specific tracing functionality for EFI is definitively
> > > > > > > the wrong way forward as it will lead to code duplication.
> > > > > >
> > > > > > We can cross that bridge when we come to it.
> > > > >
> > > > > Well, no. It's backwards to make a bridge in one place when everyone
> > > > > agrees it needs to be moved somewhere else. I mean [5] is a generic
> > > > > issue and test/py/tests/test_net_boot.py or some other test we already
> > > > > have which tests booting an OS should confirm that we've quiesced
> > > > > devices before moving on. And as a bonus it's in python where dealing
> > > > > with strings doesn't suck.
> > > >
> > > > I really don't want to write C tests in Python. CI is slow enough as
> > > > it is, something realy want to fix. I'm also not sure how you can tell
> > > > if a device has been removed. Run 'dm tree' and look for the missing
> > > > 'star' in the resulting 300 lines of text?
> > >
> > > As I'm in a bisect-hell in our C tests you'll have to forgive me for not
> > > thinking the C tests are noticeably faster than python tests. Or that
> > > they aren't their own potential source of corner-case bugs. But I
> > > digress..
> >
> > Welcome to my world. I bisected my lab devices so many times to try to
> > isolate all the breakages that have crept in. What is the problem,
> > maybe I can help?
>
> Sure. test/cmd/hash.c::dm_test_cmd_hash_md5 fails randomly, in maybe 1
> out of 100 runs, via pytest, in sandbox. Not via "./u-boot -T -c 'ut dm
> dm_test_cmd_hash_md5'" however (I stopped checking after 1000
> iterations). I was iterating over "and built with clang" but I think it
> happens with gcc too, from the actual failures in CI. And you can use
> "-k ut" to limit to just what's matched there, so it's a quicker
> iteration.

Hmmm do you have a link? It's hard to imagine what it is, but perhaps
a dependency on a previous test.

At present 'ut all' fails so I am going to take a look at that. Quite
a bit of clean-up needed in test system, though. Ideally we could run
the tests in random order so we can find and fix the dependencies. For
driver model we reinit as needed, but that's not the case for EFI, for
example.

>
> > > And yes, taking a bunch of text and parsing it, is what python is fast
> > > at. And easier to write.
> > >
> > > > But actually [5] is not generic, since EFI uses its own code to remove
> > > > devices. This test is solely focussed on EFI.
> > >
> > > Yes, you're testing the EFI version of the code in
> > > arch/$(ARCH)/lib/bootm.c. The remove devices functions being called in
> > > both cases are generic.
> >
> > The code in EFI is:
> >
> > if (!efi_st_keep_devices) {
> > bootm_disable_interrupts();
> > if (IS_ENABLED(CONFIG_USB_DEVICE))
> > udc_disconnect();
> > board_quiesce_devices();
> > dm_remove_devices_active();
> > }
> >
> > It does call somewhat the same functions, but is doing its own thing,
> > not even using the arch-specific code. As I mentioned, a nice clean-up
> > would be to make bootm_announce_and_cleanup() common.
>
> Yes, we almost agree? Both the EFI code, and arch/$(ARCH)/lib/bootm.c
> have functions that make the above calls. A nice clean-up would be to
> have something common.

Yes indeed. It still does not provide a test for the EFI bootmeth,
though, where I found half a dozen bugs.

>
> >
> > Actually, now that I see efi_st_keep_devices, I wonder why Heinrich
> > didn't want my ANSI patch[6] which serves a similar function.
>
> No? Your patch disables ANSI output in those tests, that variable is for
> making sure those tests can accomplish (if I skim things right) similar
> kinds of tests you've asked for before, but with an EFI app instead? But
> perhaps better to not start yet another tangent here...

I wouldn't know where to start, anyway...

> > > > If you want the logging to be renamed and placed centrally I don't
> > > > mind doing it now. But note that only EFI will use it for now.
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > 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.
> > > > > >
> > > > > > Perhaps the word 'log' is confusing people. But the above suggestion
> > > > > > is quite a complicated way of handling things. We have no way to
> > > > > > decode printf() strings in this way. See log_dispatch() for how this
> > > > > > is handled today. It uses sprintf(). Trying to test based on text
> > > > > > output would be very clumsy (lots of regexes and sscan() calls?) and
> > > > > > result in a huge amount of parsing code, highly dependent on the
> > > > > > printf() format, etc.
> > > > > >
> > > > > > I very-much doubt that would produce a useful implementation, but if
> > > > > > you would like to try it out then I would be happy to look at it.
> > > > > >
> > > > > > I mentioned this several times, but even if we did go that way, we
> > > > > > only have logging on the external calls, so much of the EFI-memory
> > > > > > allocation in U-Boot would not be logged.
> > > > > >
> > > > > > Regards,
> > > > > > Simon
> > > > > >
> > > > > > [5] https://patchwork.ozlabs.org/project/uboot/patch/20250106144755.3054780-9-sjg@chromium.org/
> > > > >
> > > > > Yes, calling this a "log" when it's intended for capturing information
> > > > > for tests got some of this off on the wrong track. But that also helps
> > > > > explain now that this is still on the wrong track and should instead be
> > > > > following normal design practices for testing and expanding existing
> > > > > infrastructure and not inventing a new everything. So if you don't like
> > > > > Heinrich's suggestion, take a look at Caleb's suggestion.
> > > >
> > > > I don't have the energy to port the tracing framework from Linux to
> > > > U-Boot, although I agree it would be useful. Still, function tracing
> > > > is quite fragile and confusing to work with when refactoring code. I
> > > > don't like that idea much for this use case, although if function
> > > > tracing did exist in U-Boot I would likely have used it.
> > >
> > > I mean yes, it would be good if you went back and expanded on the trace
> > > functionality you did before.
> >
> > I still don't believe it is the best solution and seems like yet
> > another ocean I should avoid sticking my heater into.
>
> I strongly disagree. If you go back to the trace code you brought in to
> start with and make it more useful / include newer features existing
> elsewhere you're not going to end up in conflict with everyone asking
> why you're doing something subsystem specific.

Perhaps someone else could do this? It would be a substantial amount
of work to bring runtime tooling into U-Boot, bpf and the like. It
would be quite a pain to use, I suspect, and certainly not possible to
write a simple C test as I have done here.

>
> > > > > And if you
> > > > > don't like Caleb's suggestion, go put this in a topic branch you can
> > > > > merge when you need to debug some problem that seemingly nothing else
> > > > > will catch.
> > > >
> > > > Here we are over a year after I reported the bug and we still don't
> > > > have a test to cover it. This series is better than the available
> > > > alternatives, IMO.
> > >
> > > Well, no. We have commit dabaa4ae3206 ("dm: Add
> > > dm_remove_devices_active() for ordered device removal") we have a test
> > > for the underlying problem. We need more functional boot tests, but we
> > > need those to be in python too, and not more C code.
> >
> > That is a nice improvement, but did not fix the underlying problem.
> > The underlying problem was that EFI was calling exit-boot-services,
> > causing U-Boot to free up data structures which were needed to boot.
> > This was on x86_64. I never quite figured out which one (very hard
> > when you cannot get back to U-Boot to check).
> >
> > There were quite a lot of problems, actually. There v2 series is at [7]
> >
> > Only a C test can check what actually happens inside U-Boot.
>
> Yes, I think now we get back to disagreeing on which symptoms lead to
> which code problems and then what to do about them.

OK

>
> > > And you're not just coming up with a test, you're refactoring a bunch of
> > > code and introducing new subsystems in order to do that. When as I keep
> > > pointing out, we don't need that. We could easily extend the existing OS
> > > boot tests we have to script booting an ISO. And we only run those when
> > > say "ENABLE_SLOW_TESTS" is set, and only do that on tagged releases.
> >
> > Yes of course we need to refactor to make tests work. This is not
> > necessarily a bad thing, as it helps us break code down into testable
> > chunks. We cannot rely only on large functional-tests, not that you
> > are suggesting that. See [8], but they are too slow, too hard to debug
> > when they fail. They also tend to devolve into chaos as people get
> > lazy and stop writing unit/smaller tests.
>
> I'll just note that I don't ever even think to use "make tests" or
> "qcheck" or any of the others since they never work for me.

Would you mind filing an issue on that? I use 'make pcheck' all the time.

> With only a
> little bit of wrappering I can however run pytest like in CI.

Yes, I use that too.

Regards,
Simon

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

[Tom Rini]

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

On Mon, Jan 13, 2025 at 12:01:36PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, 10 Jan 2025 at 09:48, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Jan 10, 2025 at 06:40:37AM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Thu, 9 Jan 2025 at 09:51, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Thu, Jan 09, 2025 at 08:02:01AM -0700, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Wed, 8 Jan 2025 at 12:15, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Wed, Jan 08, 2025 at 10:02:52AM -0700, Simon Glass wrote:
> > > > > > > Hi Heinrich, Tom,
> > > > > > >
> > > > > > > On Tue, 7 Jan 2025 at 08:47, Heinrich Schuchardt <xypron.glpk@gmx.de> 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@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@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.
> > > > > > >
> > > > > > > Are you asking for code comments in the test? If so, I can add some.
> > > > > > >
> > > > > > > >
> > > > > > > > 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.
> > > > > > >
> > > > > > > It is a pretty good start, though. It makes sure that the memory types
> > > > > > > are sane, checks addresses are within DRAM, etc. With [5] it makes
> > > > > > > sure that devices are removed.
> > > > > > >
> > > > > > > >
> > > > > > > > Implementing a specific tracing functionality for EFI is definitively
> > > > > > > > the wrong way forward as it will lead to code duplication.
> > > > > > >
> > > > > > > We can cross that bridge when we come to it.
> > > > > >
> > > > > > Well, no. It's backwards to make a bridge in one place when everyone
> > > > > > agrees it needs to be moved somewhere else. I mean [5] is a generic
> > > > > > issue and test/py/tests/test_net_boot.py or some other test we already
> > > > > > have which tests booting an OS should confirm that we've quiesced
> > > > > > devices before moving on. And as a bonus it's in python where dealing
> > > > > > with strings doesn't suck.
> > > > >
> > > > > I really don't want to write C tests in Python. CI is slow enough as
> > > > > it is, something realy want to fix. I'm also not sure how you can tell
> > > > > if a device has been removed. Run 'dm tree' and look for the missing
> > > > > 'star' in the resulting 300 lines of text?
> > > >
> > > > As I'm in a bisect-hell in our C tests you'll have to forgive me for not
> > > > thinking the C tests are noticeably faster than python tests. Or that
> > > > they aren't their own potential source of corner-case bugs. But I
> > > > digress..
> > >
> > > Welcome to my world. I bisected my lab devices so many times to try to
> > > isolate all the breakages that have crept in. What is the problem,
> > > maybe I can help?
> >
> > Sure. test/cmd/hash.c::dm_test_cmd_hash_md5 fails randomly, in maybe 1
> > out of 100 runs, via pytest, in sandbox. Not via "./u-boot -T -c 'ut dm
> > dm_test_cmd_hash_md5'" however (I stopped checking after 1000
> > iterations). I was iterating over "and built with clang" but I think it
> > happens with gcc too, from the actual failures in CI. And you can use
> > "-k ut" to limit to just what's matched there, so it's a quicker
> > iteration.
> 
> Hmmm do you have a link? It's hard to imagine what it is, but perhaps
> a dependency on a previous test.

Sure:
https://source.denx.de/u-boot/u-boot/-/jobs/993200#L286

My current gut feeling is that we've got some section overlap issue
somewhere. And, FWIW, if I turn off EFI_LOADER I still see it. And if
you use the same binary it won't always fail.

> At present 'ut all' fails so I am going to take a look at that. Quite
> a bit of clean-up needed in test system, though. Ideally we could run
> the tests in random order so we can find and fix the dependencies. For
> driver model we reinit as needed, but that's not the case for EFI, for
> example.

Personally, for making pytest faster I'd look at the general
recommendations various blog posts about "make your pytest run faster"
and then go from there.

> > > > And yes, taking a bunch of text and parsing it, is what python is fast
> > > > at. And easier to write.
> > > >
> > > > > But actually [5] is not generic, since EFI uses its own code to remove
> > > > > devices. This test is solely focussed on EFI.
> > > >
> > > > Yes, you're testing the EFI version of the code in
> > > > arch/$(ARCH)/lib/bootm.c. The remove devices functions being called in
> > > > both cases are generic.
> > >
> > > The code in EFI is:
> > >
> > > if (!efi_st_keep_devices) {
> > > bootm_disable_interrupts();
> > > if (IS_ENABLED(CONFIG_USB_DEVICE))
> > > udc_disconnect();
> > > board_quiesce_devices();
> > > dm_remove_devices_active();
> > > }
> > >
> > > It does call somewhat the same functions, but is doing its own thing,
> > > not even using the arch-specific code. As I mentioned, a nice clean-up
> > > would be to make bootm_announce_and_cleanup() common.
> >
> > Yes, we almost agree? Both the EFI code, and arch/$(ARCH)/lib/bootm.c
> > have functions that make the above calls. A nice clean-up would be to
> > have something common.
> 
> Yes indeed. It still does not provide a test for the EFI bootmeth,
> though, where I found half a dozen bugs.

Yes, the bootmeth code is it's own thing.

> > > Actually, now that I see efi_st_keep_devices, I wonder why Heinrich
> > > didn't want my ANSI patch[6] which serves a similar function.
> >
> > No? Your patch disables ANSI output in those tests, that variable is for
> > making sure those tests can accomplish (if I skim things right) similar
> > kinds of tests you've asked for before, but with an EFI app instead? But
> > perhaps better to not start yet another tangent here...
> 
> I wouldn't know where to start, anyway...
> 
> > > > > If you want the logging to be renamed and placed centrally I don't
> > > > > mind doing it now. But note that only EFI will use it for now.
> > > > >
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > 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.
> > > > > > >
> > > > > > > Perhaps the word 'log' is confusing people. But the above suggestion
> > > > > > > is quite a complicated way of handling things. We have no way to
> > > > > > > decode printf() strings in this way. See log_dispatch() for how this
> > > > > > > is handled today. It uses sprintf(). Trying to test based on text
> > > > > > > output would be very clumsy (lots of regexes and sscan() calls?) and
> > > > > > > result in a huge amount of parsing code, highly dependent on the
> > > > > > > printf() format, etc.
> > > > > > >
> > > > > > > I very-much doubt that would produce a useful implementation, but if
> > > > > > > you would like to try it out then I would be happy to look at it.
> > > > > > >
> > > > > > > I mentioned this several times, but even if we did go that way, we
> > > > > > > only have logging on the external calls, so much of the EFI-memory
> > > > > > > allocation in U-Boot would not be logged.
> > > > > > >
> > > > > > > Regards,
> > > > > > > Simon
> > > > > > >
> > > > > > > [5] https://patchwork.ozlabs.org/project/uboot/patch/20250106144755.3054780-9-sjg@chromium.org/
> > > > > >
> > > > > > Yes, calling this a "log" when it's intended for capturing information
> > > > > > for tests got some of this off on the wrong track. But that also helps
> > > > > > explain now that this is still on the wrong track and should instead be
> > > > > > following normal design practices for testing and expanding existing
> > > > > > infrastructure and not inventing a new everything. So if you don't like
> > > > > > Heinrich's suggestion, take a look at Caleb's suggestion.
> > > > >
> > > > > I don't have the energy to port the tracing framework from Linux to
> > > > > U-Boot, although I agree it would be useful. Still, function tracing
> > > > > is quite fragile and confusing to work with when refactoring code. I
> > > > > don't like that idea much for this use case, although if function
> > > > > tracing did exist in U-Boot I would likely have used it.
> > > >
> > > > I mean yes, it would be good if you went back and expanded on the trace
> > > > functionality you did before.
> > >
> > > I still don't believe it is the best solution and seems like yet
> > > another ocean I should avoid sticking my heater into.
> >
> > I strongly disagree. If you go back to the trace code you brought in to
> > start with and make it more useful / include newer features existing
> > elsewhere you're not going to end up in conflict with everyone asking
> > why you're doing something subsystem specific.
> 
> Perhaps someone else could do this? It would be a substantial amount
> of work to bring runtime tooling into U-Boot, bpf and the like. It
> would be quite a pain to use, I suspect, and certainly not possible to
> write a simple C test as I have done here.

I'm not sure where bpf and the like comes from in what I said here. But
again, if you find that logging thing you wrote handy, keep it in a
topic branch somewhere and then later on you can "Told You So" the rest
of us later on.

> > > > > > And if you
> > > > > > don't like Caleb's suggestion, go put this in a topic branch you can
> > > > > > merge when you need to debug some problem that seemingly nothing else
> > > > > > will catch.
> > > > >
> > > > > Here we are over a year after I reported the bug and we still don't
> > > > > have a test to cover it. This series is better than the available
> > > > > alternatives, IMO.
> > > >
> > > > Well, no. We have commit dabaa4ae3206 ("dm: Add
> > > > dm_remove_devices_active() for ordered device removal") we have a test
> > > > for the underlying problem. We need more functional boot tests, but we
> > > > need those to be in python too, and not more C code.
> > >
> > > That is a nice improvement, but did not fix the underlying problem.
> > > The underlying problem was that EFI was calling exit-boot-services,
> > > causing U-Boot to free up data structures which were needed to boot.
> > > This was on x86_64. I never quite figured out which one (very hard
> > > when you cannot get back to U-Boot to check).
> > >
> > > There were quite a lot of problems, actually. There v2 series is at [7]
> > >
> > > Only a C test can check what actually happens inside U-Boot.
> >
> > Yes, I think now we get back to disagreeing on which symptoms lead to
> > which code problems and then what to do about them.
> 
> OK
> 
> >
> > > > And you're not just coming up with a test, you're refactoring a bunch of
> > > > code and introducing new subsystems in order to do that. When as I keep
> > > > pointing out, we don't need that. We could easily extend the existing OS
> > > > boot tests we have to script booting an ISO. And we only run those when
> > > > say "ENABLE_SLOW_TESTS" is set, and only do that on tagged releases.
> > >
> > > Yes of course we need to refactor to make tests work. This is not
> > > necessarily a bad thing, as it helps us break code down into testable
> > > chunks. We cannot rely only on large functional-tests, not that you
> > > are suggesting that. See [8], but they are too slow, too hard to debug
> > > when they fail. They also tend to devolve into chaos as people get
> > > lazy and stop writing unit/smaller tests.
> >
> > I'll just note that I don't ever even think to use "make tests" or
> > "qcheck" or any of the others since they never work for me.
> 
> Would you mind filing an issue on that? I use 'make pcheck' all the time.

Done:
https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/33

I didn't file one for "qcheck" which fails differently in that same
environment.

-- 
Tom

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

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

[Simon Glass]

Hi Tom,

On Mon, 13 Jan 2025 at 13:20, Tom Rini <trini@konsulko.com> wrote:
>
> On Mon, Jan 13, 2025 at 12:01:36PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Fri, 10 Jan 2025 at 09:48, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Fri, Jan 10, 2025 at 06:40:37AM -0700, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Thu, 9 Jan 2025 at 09:51, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Thu, Jan 09, 2025 at 08:02:01AM -0700, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Wed, 8 Jan 2025 at 12:15, Tom Rini <trini@konsulko.com>
wrote:
> > > > > > >
> > > > > > > On Wed, Jan 08, 2025 at 10:02:52AM -0700, Simon Glass wrote:
> > > > > > > > Hi Heinrich, Tom,
> > > > > > > >
> > > > > > > > On Tue, 7 Jan 2025 at 08:47, Heinrich Schuchardt <
xypron.glpk@gmx.de> 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@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@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.
> > > > > > > >
> > > > > > > > Are you asking for code comments in the test? If so, I can
add some.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > 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.
> > > > > > > >
> > > > > > > > It is a pretty good start, though. It makes sure that the
memory types
> > > > > > > > are sane, checks addresses are within DRAM, etc. With [5]
it makes
> > > > > > > > sure that devices are removed.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Implementing a specific tracing functionality for EFI is
definitively
> > > > > > > > > the wrong way forward as it will lead to code duplication.
> > > > > > > >
> > > > > > > > We can cross that bridge when we come to it.
> > > > > > >
> > > > > > > Well, no. It's backwards to make a bridge in one place when
everyone
> > > > > > > agrees it needs to be moved somewhere else. I mean [5] is a
generic
> > > > > > > issue and test/py/tests/test_net_boot.py or some other test
we already
> > > > > > > have which tests booting an OS should confirm that we've
quiesced
> > > > > > > devices before moving on. And as a bonus it's in python where
dealing
> > > > > > > with strings doesn't suck.
> > > > > >
> > > > > > I really don't want to write C tests in Python. CI is slow
enough as
> > > > > > it is, something realy want to fix. I'm also not sure how you
can tell
> > > > > > if a device has been removed. Run 'dm tree' and look for the
missing
> > > > > > 'star' in the resulting 300 lines of text?
> > > > >
> > > > > As I'm in a bisect-hell in our C tests you'll have to forgive me
for not
> > > > > thinking the C tests are noticeably faster than python tests. Or
that
> > > > > they aren't their own potential source of corner-case bugs. But I
> > > > > digress..
> > > >
> > > > Welcome to my world. I bisected my lab devices so many times to try
to
> > > > isolate all the breakages that have crept in. What is the problem,
> > > > maybe I can help?
> > >
> > > Sure. test/cmd/hash.c::dm_test_cmd_hash_md5 fails randomly, in maybe 1
> > > out of 100 runs, via pytest, in sandbox. Not via "./u-boot -T -c 'ut
dm
> > > dm_test_cmd_hash_md5'" however (I stopped checking after 1000
> > > iterations). I was iterating over "and built with clang" but I think
it
> > > happens with gcc too, from the actual failures in CI. And you can use
> > > "-k ut" to limit to just what's matched there, so it's a quicker
> > > iteration.
> >
> > Hmmm do you have a link? It's hard to imagine what it is, but perhaps
> > a dependency on a previous test.
>
> Sure:
> https://source.denx.de/u-boot/u-boot/-/jobs/993200#L286
>
> My current gut feeling is that we've got some section overlap issue
> somewhere. And, FWIW, if I turn off EFI_LOADER I still see it. And if
> you use the same binary it won't always fail.

The only way I think the hash command can return CMD_RET_USAGE is if there
are not enough arguments.

The only way that can happen with this code:

ut_assertok(run_command("hash md5 $loadaddr 0", 0));

is if loadaddr is undefined.

But (like you, I suspect) I cannot find how that could be.

In fact, I can't even build with clang:

$ buildman -O clang-17 --bo sandbox -w -o /tmp/b/sandbox-clang
Building current source for 1 boards (1 thread, 32 jobs per thread)
   sandbox:  +   sandbox
+/usr/bin/llvm-ar: error: arch/sandbox/cpu/built-in.o: Opaque pointers are
only supported in -opaque-pointers mode (Producer: 'LLVM17.0.6' Reader:
'LLVM 14.0.0')
+make[2]: *** [scripts/Makefile.build:333: arch/sandbox/cpu/built-in.o]
Error 1
+make[1]: *** [Makefile:1906: arch/sandbox/cpu] Error 2
+make: *** [Makefile:177: sub-make] Error 2
    0    0    1 /1              sandbox


>
> > At present 'ut all' fails so I am going to take a look at that. Quite
> > a bit of clean-up needed in test system, though. Ideally we could run
> > the tests in random order so we can find and fix the dependencies. For
> > driver model we reinit as needed, but that's not the case for EFI, for
> > example.
>
> Personally, for making pytest faster I'd look at the general
> recommendations various blog posts about "make your pytest run faster"
> and then go from there.

I think the problem is that you are looking at the C tests through a Python
lens, so everything seems a bit slow.

'ut all' takes about 18 seconds for me.

>
> > > > > And yes, taking a bunch of text and parsing it, is what python is
fast
> > > > > at. And easier to write.
> > > > >
> > > > > > But actually [5] is not generic, since EFI uses its own code to
remove
> > > > > > devices. This test is solely focussed on EFI.
> > > > >
> > > > > Yes, you're testing the EFI version of the code in
> > > > > arch/$(ARCH)/lib/bootm.c. The remove devices functions being
called in
> > > > > both cases are generic.
> > > >
> > > > The code in EFI is:
> > > >
> > > > if (!efi_st_keep_devices) {
> > > > bootm_disable_interrupts();
> > > > if (IS_ENABLED(CONFIG_USB_DEVICE))
> > > > udc_disconnect();
> > > > board_quiesce_devices();
> > > > dm_remove_devices_active();
> > > > }
> > > >
> > > > It does call somewhat the same functions, but is doing its own
thing,
> > > > not even using the arch-specific code. As I mentioned, a nice
clean-up
> > > > would be to make bootm_announce_and_cleanup() common.
> > >
> > > Yes, we almost agree? Both the EFI code, and arch/$(ARCH)/lib/bootm.c
> > > have functions that make the above calls. A nice clean-up would be to
> > > have something common.
> >
> > Yes indeed. It still does not provide a test for the EFI bootmeth,
> > though, where I found half a dozen bugs.
>
> Yes, the bootmeth code is it's own thing.

No I mean there were bugs in the EFI implementation.

>
> > > > Actually, now that I see efi_st_keep_devices, I wonder why Heinrich
> > > > didn't want my ANSI patch[6] which serves a similar function.
> > >
> > > No? Your patch disables ANSI output in those tests, that variable is
for
> > > making sure those tests can accomplish (if I skim things right)
similar
> > > kinds of tests you've asked for before, but with an EFI app instead?
But
> > > perhaps better to not start yet another tangent here...
> >
> > I wouldn't know where to start, anyway...
> >
> > > > > > If you want the logging to be renamed and placed centrally I
don't
> > > > > > mind doing it now. But note that only EFI will use it for now.
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > 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.
> > > > > > > >
> > > > > > > > Perhaps the word 'log' is confusing people. But the above
suggestion
> > > > > > > > is quite a complicated way of handling things. We have no
way to
> > > > > > > > decode printf() strings in this way. See log_dispatch() for
how this
> > > > > > > > is handled today. It uses sprintf(). Trying to test based
on text
> > > > > > > > output would be very clumsy (lots of regexes and sscan()
calls?) and
> > > > > > > > result in a huge amount of parsing code, highly dependent
on the
> > > > > > > > printf() format, etc.
> > > > > > > >
> > > > > > > > I very-much doubt that would produce a useful
implementation, but if
> > > > > > > > you would like to try it out then I would be happy to look
at it.
> > > > > > > >
> > > > > > > > I mentioned this several times, but even if we did go that
way, we
> > > > > > > > only have logging on the external calls, so much of the
EFI-memory
> > > > > > > > allocation in U-Boot would not be logged.
> > > > > > > >
> > > > > > > > Regards,
> > > > > > > > Simon
> > > > > > > >
> > > > > > > > [5]
https://patchwork.ozlabs.org/project/uboot/patch/20250106144755.3054780-9-sjg@chromium.org/
> > > > > > >
> > > > > > > Yes, calling this a "log" when it's intended for capturing
information
> > > > > > > for tests got some of this off on the wrong track. But that
also helps
> > > > > > > explain now that this is still on the wrong track and should
instead be
> > > > > > > following normal design practices for testing and expanding
existing
> > > > > > > infrastructure and not inventing a new everything. So if you
don't like
> > > > > > > Heinrich's suggestion, take a look at Caleb's suggestion.
> > > > > >
> > > > > > I don't have the energy to port the tracing framework from
Linux to
> > > > > > U-Boot, although I agree it would be useful. Still, function
tracing
> > > > > > is quite fragile and confusing to work with when refactoring
code. I
> > > > > > don't like that idea much for this use case, although if
function
> > > > > > tracing did exist in U-Boot I would likely have used it.
> > > > >
> > > > > I mean yes, it would be good if you went back and expanded on the
trace
> > > > > functionality you did before.
> > > >
> > > > I still don't believe it is the best solution and seems like yet
> > > > another ocean I should avoid sticking my heater into.
> > >
> > > I strongly disagree. If you go back to the trace code you brought in
to
> > > start with and make it more useful / include newer features existing
> > > elsewhere you're not going to end up in conflict with everyone asking
> > > why you're doing something subsystem specific.
> >
> > Perhaps someone else could do this? It would be a substantial amount
> > of work to bring runtime tooling into U-Boot, bpf and the like. It
> > would be quite a pain to use, I suspect, and certainly not possible to
> > write a simple C test as I have done here.
>
> I'm not sure where bpf and the like comes from in what I said here. But
> again, if you find that logging thing you wrote handy, keep it in a
> topic branch somewhere and then later on you can "Told You So" the rest
> of us later on.

I'm not sure what you are suggesting re tracing, but I assumed it was that
we patch the code at runtime and set tracepoints to check for particular
calls in a test. I'm not sure how else we would get access to the function
arguments. Then, presumably we need a bpf rule to collect the function
args, etc.? It wasn't my idea, so perhaps we should let Caleb explain the
intent.

>
> > > > > > > And if you
> > > > > > > don't like Caleb's suggestion, go put this in a topic branch
you can
> > > > > > > merge when you need to debug some problem that seemingly
nothing else
> > > > > > > will catch.
> > > > > >
> > > > > > Here we are over a year after I reported the bug and we still
don't
> > > > > > have a test to cover it. This series is better than the
available
> > > > > > alternatives, IMO.
> > > > >
> > > > > Well, no. We have commit dabaa4ae3206 ("dm: Add
> > > > > dm_remove_devices_active() for ordered device removal") we have a
test
> > > > > for the underlying problem. We need more functional boot tests,
but we
> > > > > need those to be in python too, and not more C code.
> > > >
> > > > That is a nice improvement, but did not fix the underlying problem.
> > > > The underlying problem was that EFI was calling exit-boot-services,
> > > > causing U-Boot to free up data structures which were needed to boot.
> > > > This was on x86_64. I never quite figured out which one (very hard
> > > > when you cannot get back to U-Boot to check).
> > > >
> > > > There were quite a lot of problems, actually. There v2 series is at
[7]
> > > >
> > > > Only a C test can check what actually happens inside U-Boot.
> > >
> > > Yes, I think now we get back to disagreeing on which symptoms lead to
> > > which code problems and then what to do about them.
> >
> > OK
> >
> > >
> > > > > And you're not just coming up with a test, you're refactoring a
bunch of
> > > > > code and introducing new subsystems in order to do that. When as
I keep
> > > > > pointing out, we don't need that. We could easily extend the
existing OS
> > > > > boot tests we have to script booting an ISO. And we only run
those when
> > > > > say "ENABLE_SLOW_TESTS" is set, and only do that on tagged
releases.
> > > >
> > > > Yes of course we need to refactor to make tests work. This is not
> > > > necessarily a bad thing, as it helps us break code down into
testable
> > > > chunks. We cannot rely only on large functional-tests, not that you
> > > > are suggesting that. See [8], but they are too slow, too hard to
debug
> > > > when they fail. They also tend to devolve into chaos as people get
> > > > lazy and stop writing unit/smaller tests.
> > >
> > > I'll just note that I don't ever even think to use "make tests" or
> > > "qcheck" or any of the others since they never work for me.
> >
> > Would you mind filing an issue on that? I use 'make pcheck' all the
time.
>
> Done:
> https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/33
>
> I didn't file one for "qcheck" which fails differently in that same
> environment.

OK, I'll reply there, thanks.

Regards,
Simon

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

[Tom Rini]

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

On Fri, Jan 17, 2025 at 08:35:42PM -0800, Simon Glass wrote:
> Hi Tom,
> 
> On Mon, 13 Jan 2025 at 13:20, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Mon, Jan 13, 2025 at 12:01:36PM -0700, Simon Glass wrote:
[snip]
> > > > Sure. test/cmd/hash.c::dm_test_cmd_hash_md5 fails randomly, in maybe 1
> > > > out of 100 runs, via pytest, in sandbox. Not via "./u-boot -T -c 'ut
> dm
> > > > dm_test_cmd_hash_md5'" however (I stopped checking after 1000
> > > > iterations). I was iterating over "and built with clang" but I think
> it
> > > > happens with gcc too, from the actual failures in CI. And you can use
> > > > "-k ut" to limit to just what's matched there, so it's a quicker
> > > > iteration.
> > >
> > > Hmmm do you have a link? It's hard to imagine what it is, but perhaps
> > > a dependency on a previous test.
> >
> > Sure:
> > https://source.denx.de/u-boot/u-boot/-/jobs/993200#L286
> >
> > My current gut feeling is that we've got some section overlap issue
> > somewhere. And, FWIW, if I turn off EFI_LOADER I still see it. And if
> > you use the same binary it won't always fail.
> 
> The only way I think the hash command can return CMD_RET_USAGE is if there
> are not enough arguments.
> 
> The only way that can happen with this code:
> 
> ut_assertok(run_command("hash md5 $loadaddr 0", 0));
> 
> is if loadaddr is undefined.
> 
> But (like you, I suspect) I cannot find how that could be.
> 
> In fact, I can't even build with clang:
> 
> $ buildman -O clang-17 --bo sandbox -w -o /tmp/b/sandbox-clang
> Building current source for 1 boards (1 thread, 32 jobs per thread)
>    sandbox:  +   sandbox
> +/usr/bin/llvm-ar: error: arch/sandbox/cpu/built-in.o: Opaque pointers are
> only supported in -opaque-pointers mode (Producer: 'LLVM17.0.6' Reader:
> 'LLVM 14.0.0')
> +make[2]: *** [scripts/Makefile.build:333: arch/sandbox/cpu/built-in.o]
> Error 1
> +make[1]: *** [Makefile:1906: arch/sandbox/cpu] Error 2
> +make: *** [Makefile:177: sub-make] Error 2
>     0    0    1 /1              sandbox

This command works fine for me in the CI container, so I don't know
what's going on with your local installation. I've also seen CI fail
enough times now with just regular sandbox that I don't _think_ it's a
compiler specific issue. Thanks for looking in to this more!

-- 
Tom

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

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

[Tom Rini]

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

On Fri, Jan 17, 2025 at 08:35:42PM -0800, Simon Glass wrote:

[snip]
> > > At present 'ut all' fails so I am going to take a look at that. Quite
> > > a bit of clean-up needed in test system, though. Ideally we could run
> > > the tests in random order so we can find and fix the dependencies. For
> > > driver model we reinit as needed, but that's not the case for EFI, for
> > > example.
> >
> > Personally, for making pytest faster I'd look at the general
> > recommendations various blog posts about "make your pytest run faster"
> > and then go from there.
> 
> I think the problem is that you are looking at the C tests through a Python
> lens, so everything seems a bit slow.
> 
> 'ut all' takes about 18 seconds for me.

Yes, and the "ut" tests just via pytest do take longer. And it would be
good to have more testing in CI, and for tests to be as fast as
possible.

Looking at the run I just did on HW:
====================== 202 passed, 426 skipped in 48.76s =======================
================= 202 passed, 426 skipped in 80.04s (0:01:20) ==================
================= 206 passed, 422 skipped in 128.74s (0:02:08) =================
================= 206 passed, 422 skipped in 118.15s (0:01:58) =================
================= 206 passed, 422 skipped in 128.65s (0:02:08) =================
================= 206 passed, 422 skipped in 123.76s (0:02:03) =================
================= 205 passed, 423 skipped in 128.22s (0:02:08) =================
================= 198 passed, 429 skipped in 106.92s (0:01:46) =================
================= 192 passed, 431 skipped in 87.95s (0:01:27) ==================

That could be better, but it's not unreasonable and part of that is
tftp'ing an 83MB kernel image.

I think if I had a point around here before, it was this. C is a
terrible language for processing strings. Python is a reasonable
language for processing strings. Neither of these statements should be
controversial. The controversy is that I'm saying tests that parse
output for results shouldn't be written in C.

-- 
Tom

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

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

[Simon Glass]

-Ilias

On Mon, 6 Jan 2025 at 07:48, Simon Glass <sjg@chromium.org> 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.
>
>
> Simon Glass (8):
>   sandbox: Make USB controller as having active DMA
>   efi_loader: Fix display of addresses in log
>   efi_loader: Return the memory map in pointer format
>   efi_loader: Correct bounce-buffer setup
>   efi_loader: Check memory allocations in bootflow_efi test()
>   efi_loader: Update testapp to get memory map correctly
>   efi_loader: Check that the bootflow is not removed by app
>   efi_loader: Test that active-DMA devices are removed
>
>  drivers/usb/host/usb-sandbox.c |  1 +
>  lib/efi_loader/efi_log.c       |  5 +--
>  lib/efi_loader/efi_memory.c    |  9 ++--
>  lib/efi_loader/testapp.c       | 37 ++++++++++++++++
>  test/boot/bootflow.c           | 78 +++++++++++++++++++++++++++++++++-
>  5 files changed, 122 insertions(+), 8 deletions(-)
>
> --
> 2.34.1
>

Series applied to sjg/master, thanks!