[PATCH] kernel-stub: Use EfiACPIReclaimMemory for DTB memory

[Christian Storm]

From: Christian Storm <christi...@siemens.com>

DTBs are kind-of ACPI Tables, hence flag this as memory that holds
ACPI tables, see https://arm-software.github.io/ebbr/#devicetree

Signed-off-by: Christian Storm <christi...@siemens.com>
---
kernel-stub/fdt.c | 40 +++++++++++++++++++---------------------
1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/kernel-stub/fdt.c b/kernel-stub/fdt.c
index e6fe410..8b0fb24 100644
--- a/kernel-stub/fdt.c
+++ b/kernel-stub/fdt.c
@@ -25,6 +25,8 @@
#define BE32_TO_HOST(val) (val)
#endif

+#define SIZE_IN_PAGES(size) (size / EFI_PAGE_SIZE + !!(size % EFI_PAGE_SIZE))
+
#define FDT_BEGIN_NODE 0x1
#define FDT_END_NODE 0x2
#define FDT_PROP 0x3
@@ -152,19 +154,16 @@ BOOLEAN match_fdt(const VOID *fdt, const CHAR8 *compatible)
return strcmpa(compatible, alt_compatible) == 0;
}

-static VOID *clone_fdt(const VOID *fdt, UINTN size)
+static EFI_PHYSICAL_ADDRESS clone_fdt(const VOID *fdt, UINTN size)
{
- const FDT_HEADER *header = fdt;
- VOID *clone;
-
- clone = AllocatePool(size);
- if (!clone) {
- error(L"Error allocating device tree buffer",
- EFI_OUT_OF_RESOURCES);
- return NULL;
+ EFI_PHYSICAL_ADDRESS clone;
+ EFI_STATUS status = BS->AllocatePages(AllocateAnyPages, EfiACPIReclaimMemory,
+ SIZE_IN_PAGES(size), &clone);
+ if (EFI_ERROR(status)) {
+ error(L"Error allocating device tree buffer", status);
+ return 0x0;
}
-
- CopyMem(clone, fdt, BE32_TO_HOST(header->TotalSize));
+ CopyMem((VOID *)clone, fdt, BE32_TO_HOST(((FDT_HEADER *)fdt)->TotalSize));
return clone;
}

@@ -172,16 +171,15 @@ EFI_STATUS replace_fdt(const VOID *fdt)
{
EFI_DT_FIXUP_PROTOCOL *protocol;
EFI_STATUS status;
- VOID *fdt_buffer;
+ EFI_PHYSICAL_ADDRESS fdt_buffer;
UINTN size;

- status = LibLocateProtocol(&EfiDtFixupProtocol, (VOID **)&protocol);
- if (EFI_ERROR(status)) {
- const FDT_HEADER *header = fdt;
-
+ if (EFI_ERROR(LibLocateProtocol(&EfiDtFixupProtocol, (VOID **)&protocol))) {
info(L"Firmware does not provide device tree fixup protocol");

- fdt_buffer = clone_fdt(fdt, BE32_TO_HOST(header->TotalSize));
+ size = BE32_TO_HOST(((FDT_HEADER *)fdt)->TotalSize);
+
+ fdt_buffer = clone_fdt(fdt, size);
if (!fdt_buffer) {
return EFI_OUT_OF_RESOURCES;
}
@@ -200,19 +198,19 @@ EFI_STATUS replace_fdt(const VOID *fdt)
return EFI_OUT_OF_RESOURCES;
}

- status = protocol->Fixup(protocol, fdt_buffer, &size,
+ status = protocol->Fixup(protocol, (VOID *)fdt_buffer, &size,
EFI_DT_APPLY_FIXUPS |
EFI_DT_RESERVE_MEMORY);
if (EFI_ERROR(status)) {
- FreePool(fdt_buffer);
+ (VOID) BS->FreePages(fdt_buffer, SIZE_IN_PAGES(size));
error(L"Device tree fixup failed", status);
return status;
}
}

- status = BS->InstallConfigurationTable(&EfiDtbTableGuid, fdt_buffer);
+ status = BS->InstallConfigurationTable(&EfiDtbTableGuid, (VOID *)fdt_buffer);
if (EFI_ERROR(status)) {
- FreePool(fdt_buffer);
+ (VOID) BS->FreePages(fdt_buffer, SIZE_IN_PAGES(size));
error(L"Failed to install alternative device tree", status);
}

--
2.40.1

[Jan Kiszka]

On 12.05.23 16:51, 'Christian Storm' via EFI Boot Guard wrote:
> From: Christian Storm <christi...@siemens.com>
>
> DTBs are kind-of ACPI Tables, hence flag this as memory that holds
> ACPI tables, see https://arm-software.github.io/ebbr/#devicetree
>
> Signed-off-by: Christian Storm <christi...@siemens.com>
> ---
> kernel-stub/fdt.c | 40 +++++++++++++++++++---------------------
> 1 file changed, 19 insertions(+), 21 deletions(-)
>
> diff --git a/kernel-stub/fdt.c b/kernel-stub/fdt.c
> index e6fe410..8b0fb24 100644
> --- a/kernel-stub/fdt.c
> +++ b/kernel-stub/fdt.c
> @@ -25,6 +25,8 @@
> #define BE32_TO_HOST(val) (val)
> #endif
>
> +#define SIZE_IN_PAGES(size) (size / EFI_PAGE_SIZE + !!(size % EFI_PAGE_SIZE))

AKA (more readable and less than 80 chars):

#define SIZE_IN_PAGES(size) ((size + EFI_PAGE_SIZE - 1) / EFI_PAGE_SIZE)

> +
> #define FDT_BEGIN_NODE 0x1
> #define FDT_END_NODE 0x2
> #define FDT_PROP 0x3
> @@ -152,19 +154,16 @@ BOOLEAN match_fdt(const VOID *fdt, const CHAR8 *compatible)
> return strcmpa(compatible, alt_compatible) == 0;
> }
>
> -static VOID *clone_fdt(const VOID *fdt, UINTN size)
> +static EFI_PHYSICAL_ADDRESS clone_fdt(const VOID *fdt, UINTN size)
> {
> - const FDT_HEADER *header = fdt;
> - VOID *clone;
> -
> - clone = AllocatePool(size);
> - if (!clone) {
> - error(L"Error allocating device tree buffer",
> - EFI_OUT_OF_RESOURCES);
> - return NULL;
> + EFI_PHYSICAL_ADDRESS clone;
> + EFI_STATUS status = BS->AllocatePages(AllocateAnyPages, EfiACPIReclaimMemory,

Overlong line.

> + SIZE_IN_PAGES(size), &clone);
> + if (EFI_ERROR(status)) {
> + error(L"Error allocating device tree buffer", status);
> + return 0x0;

Is 0 defined as invalid EFI_PHYSICAL_ADDRESS? Or do we have some
EFI_INVALID_PHYSICAL_ADDRESS?

> }
> -
> - CopyMem(clone, fdt, BE32_TO_HOST(header->TotalSize));
> + CopyMem((VOID *)clone, fdt, BE32_TO_HOST(((FDT_HEADER *)fdt)->TotalSize));

Overlong line.

> return clone;
> }
>
> @@ -172,16 +171,15 @@ EFI_STATUS replace_fdt(const VOID *fdt)
> {
> EFI_DT_FIXUP_PROTOCOL *protocol;
> EFI_STATUS status;
> - VOID *fdt_buffer;
> + EFI_PHYSICAL_ADDRESS fdt_buffer;
> UINTN size;
>
> - status = LibLocateProtocol(&EfiDtFixupProtocol, (VOID **)&protocol);
> - if (EFI_ERROR(status)) {
> - const FDT_HEADER *header = fdt;
> -
> + if (EFI_ERROR(LibLocateProtocol(&EfiDtFixupProtocol, (VOID **)&protocol))) {

Unrelated changes - and then also one which creates an overlong line.

> info(L"Firmware does not provide device tree fixup protocol");
>
> - fdt_buffer = clone_fdt(fdt, BE32_TO_HOST(header->TotalSize));
> + size = BE32_TO_HOST(((FDT_HEADER *)fdt)->TotalSize);

I liked the "header" variant more. Do we need to store "size" now?

> +
> + fdt_buffer = clone_fdt(fdt, size);
> if (!fdt_buffer) {
> return EFI_OUT_OF_RESOURCES;
> }
> @@ -200,19 +198,19 @@ EFI_STATUS replace_fdt(const VOID *fdt)
> return EFI_OUT_OF_RESOURCES;
> }
>
> - status = protocol->Fixup(protocol, fdt_buffer, &size,
> + status = protocol->Fixup(protocol, (VOID *)fdt_buffer, &size,
> EFI_DT_APPLY_FIXUPS |
> EFI_DT_RESERVE_MEMORY);
> if (EFI_ERROR(status)) {
> - FreePool(fdt_buffer);
> + (VOID) BS->FreePages(fdt_buffer, SIZE_IN_PAGES(size));
> error(L"Device tree fixup failed", status);
> return status;
> }
> }
>
> - status = BS->InstallConfigurationTable(&EfiDtbTableGuid, fdt_buffer);
> + status = BS->InstallConfigurationTable(&EfiDtbTableGuid, (VOID *)fdt_buffer);

Overlong line.

> if (EFI_ERROR(status)) {
> - FreePool(fdt_buffer);
> + (VOID) BS->FreePages(fdt_buffer, SIZE_IN_PAGES(size));
> error(L"Failed to install alternative device tree", status);
> }
>

Thanks, already forgot about this topic :)

Jan

--
Siemens AG, Technology
Competence Center Embedded Linux

[Christian Storm]

Hi Jan,

> > DTBs are kind-of ACPI Tables, hence flag this as memory that holds
> > ACPI tables, see https://arm-software.github.io/ebbr/#devicetree
> >
> > Signed-off-by: Christian Storm <christi...@siemens.com>
> > ---
> > kernel-stub/fdt.c | 40 +++++++++++++++++++---------------------
> > 1 file changed, 19 insertions(+), 21 deletions(-)
> >
> > diff --git a/kernel-stub/fdt.c b/kernel-stub/fdt.c
> > index e6fe410..8b0fb24 100644
> > --- a/kernel-stub/fdt.c
> > +++ b/kernel-stub/fdt.c
> > @@ -25,6 +25,8 @@
> > #define BE32_TO_HOST(val) (val)
> > #endif
> >
> > +#define SIZE_IN_PAGES(size) (size / EFI_PAGE_SIZE + !!(size % EFI_PAGE_SIZE))
>
> AKA (more readable and less than 80 chars):
>
> #define SIZE_IN_PAGES(size) ((size + EFI_PAGE_SIZE - 1) / EFI_PAGE_SIZE)

I have to update my snippet collection :)

No, it's not invalid but it's "NULL-like" and takes the error-out path in
if (!fdt_buffer) {
return EFI_OUT_OF_RESOURCES;
}
in case of AllocatePages() failure.

> > }
> > -
> > - CopyMem(clone, fdt, BE32_TO_HOST(header->TotalSize));
> > + CopyMem((VOID *)clone, fdt, BE32_TO_HOST(((FDT_HEADER *)fdt)->TotalSize));
>
> Overlong line.
>
> > return clone;
> > }
> >
> > @@ -172,16 +171,15 @@ EFI_STATUS replace_fdt(const VOID *fdt)
> > {
> > EFI_DT_FIXUP_PROTOCOL *protocol;
> > EFI_STATUS status;
> > - VOID *fdt_buffer;
> > + EFI_PHYSICAL_ADDRESS fdt_buffer;
> > UINTN size;
> >
> > - status = LibLocateProtocol(&EfiDtFixupProtocol, (VOID **)&protocol);
> > - if (EFI_ERROR(status)) {
> > - const FDT_HEADER *header = fdt;
> > -
> > + if (EFI_ERROR(LibLocateProtocol(&EfiDtFixupProtocol, (VOID **)&protocol))) {
>
> Unrelated changes - and then also one which creates an overlong line.
>
> > info(L"Firmware does not provide device tree fixup protocol");
> >
> > - fdt_buffer = clone_fdt(fdt, BE32_TO_HOST(header->TotalSize));
> > + size = BE32_TO_HOST(((FDT_HEADER *)fdt)->TotalSize);
>
> I liked the "header" variant more.

There's no accounting for taste. I don't care too much so I'll revert it.

> Do we need to store "size" now?

Yes, we need to record size now for [scroll down]

> > +
> > + fdt_buffer = clone_fdt(fdt, size);
> > if (!fdt_buffer) {
> > return EFI_OUT_OF_RESOURCES;
> > }
> > @@ -200,19 +198,19 @@ EFI_STATUS replace_fdt(const VOID *fdt)
> > return EFI_OUT_OF_RESOURCES;
> > }
> >
> > - status = protocol->Fixup(protocol, fdt_buffer, &size,
> > + status = protocol->Fixup(protocol, (VOID *)fdt_buffer, &size,
> > EFI_DT_APPLY_FIXUPS |
> > EFI_DT_RESERVE_MEMORY);
> > if (EFI_ERROR(status)) {
> > - FreePool(fdt_buffer);
> > + (VOID) BS->FreePages(fdt_buffer, SIZE_IN_PAGES(size));
> > error(L"Device tree fixup failed", status);
> > return status;
> > }
> > }
> >
> > - status = BS->InstallConfigurationTable(&EfiDtbTableGuid, fdt_buffer);
> > + status = BS->InstallConfigurationTable(&EfiDtbTableGuid, (VOID *)fdt_buffer);
>
> Overlong line.
>
> > if (EFI_ERROR(status)) {
> > - FreePool(fdt_buffer);
> > + (VOID) BS->FreePages(fdt_buffer, SIZE_IN_PAGES(size));

[to here] this.

> > error(L"Failed to install alternative device tree", status);
> > }
> >
>
> Thanks, already forgot about this topic :)

Me as well but not my persistent backlog :)


Kind regards,
Christian

--
Dr. Christian Storm
Siemens AG, Technology, T CED SES-DE
Otto-Hahn-Ring 6, 81739 München, Germany

[Jan Kiszka]

I know where this goes, and it is likely practically safe. But it is
ugly to read because 0 is a valid EFI_PHYSICAL_ADDRESS then. Maybe we
should do it like UEFI itself an return an error code and propagate the
pointer via an additional argument.

[Christian Storm]

Well, it's a static helper function, and it contains just two calls to
AllocatePages() and CopyMem(). We could as well inline those two calls
then, that's less verbose than imposing the EFI return code machinery.
Anyway, as you like....

[Christian Storm]

From: Christian Storm <christi...@siemens.com>

DTBs are kind-of ACPI Tables, hence flag this as memory that holds
ACPI tables, see https://arm-software.github.io/ebbr/#devicetree

Signed-off-by: Christian Storm <christi...@siemens.com>
---

Changes in V2:
* Fix lines to < 80 chars
* Use simpler SIZE_IN_PAGES macro
* Use EFI style return code handling for clone_fdt()
* Revert to use explicit `header` variable

kernel-stub/fdt.c | 46 +++++++++++++++++++++++++---------------------
1 file changed, 25 insertions(+), 21 deletions(-)

diff --git a/kernel-stub/fdt.c b/kernel-stub/fdt.c
index e6fe410..8b1a6de 100644

--- a/kernel-stub/fdt.c
+++ b/kernel-stub/fdt.c
@@ -25,6 +25,8 @@
#define BE32_TO_HOST(val) (val)
#endif

+#define SIZE_IN_PAGES(size) ((size + EFI_PAGE_SIZE - 1) / EFI_PAGE_SIZE)

+
#define FDT_BEGIN_NODE 0x1
#define FDT_END_NODE 0x2
#define FDT_PROP 0x3

@@ -152,27 +154,27 @@ BOOLEAN match_fdt(const VOID *fdt, const CHAR8 *compatible)

return strcmpa(compatible, alt_compatible) == 0;
}

-static VOID *clone_fdt(const VOID *fdt, UINTN size)

+static EFI_STATUS clone_fdt(const VOID *fdt, UINTN size,
+ EFI_PHYSICAL_ADDRESS *fdt_buffer)
{

const FDT_HEADER *header = fdt;
- VOID *clone;

+ EFI_STATUS status;

- clone = AllocatePool(size);
- if (!clone) {
- error(L"Error allocating device tree buffer",
- EFI_OUT_OF_RESOURCES);
- return NULL;

+ status = BS->AllocatePages(AllocateAnyPages, EfiACPIReclaimMemory,
+ SIZE_IN_PAGES(size), fdt_buffer);

+ if (EFI_ERROR(status)) {
+ error(L"Error allocating device tree buffer", status);

+ return status;

}
-
- CopyMem(clone, fdt, BE32_TO_HOST(header->TotalSize));

- return clone;
+ CopyMem((VOID *)*fdt_buffer, fdt, BE32_TO_HOST(header->TotalSize));
+ return EFI_SUCCESS;

}

EFI_STATUS replace_fdt(const VOID *fdt)
{
EFI_DT_FIXUP_PROTOCOL *protocol;
EFI_STATUS status;
- VOID *fdt_buffer;
+ EFI_PHYSICAL_ADDRESS fdt_buffer;
UINTN size;

status = LibLocateProtocol(&EfiDtFixupProtocol, (VOID **)&protocol);

@@ -181,9 +183,10 @@ EFI_STATUS replace_fdt(const VOID *fdt)

info(L"Firmware does not provide device tree fixup protocol");

- fdt_buffer = clone_fdt(fdt, BE32_TO_HOST(header->TotalSize));

- if (!fdt_buffer) {
- return EFI_OUT_OF_RESOURCES;
+ size = BE32_TO_HOST(header->TotalSize);
+ status = clone_fdt(fdt, size, &fdt_buffer);
+ if (EFI_ERROR(status)) {
+ return status;
}
} else {
/* Find out which size we need */
@@ -195,24 +198,25 @@ EFI_STATUS replace_fdt(const VOID *fdt)
return status;
}

- fdt_buffer = clone_fdt(fdt, size);
- if (!fdt_buffer) {
- return EFI_OUT_OF_RESOURCES;
+ status = clone_fdt(fdt, size, &fdt_buffer);
+ if (EFI_ERROR(status)) {
+ return status;

}

- status = protocol->Fixup(protocol, fdt_buffer, &size,
+ status = protocol->Fixup(protocol, (VOID *)fdt_buffer, &size,
EFI_DT_APPLY_FIXUPS |
EFI_DT_RESERVE_MEMORY);
if (EFI_ERROR(status)) {
- FreePool(fdt_buffer);
+ (VOID) BS->FreePages(fdt_buffer, SIZE_IN_PAGES(size));
error(L"Device tree fixup failed", status);
return status;
}
}

- status = BS->InstallConfigurationTable(&EfiDtbTableGuid, fdt_buffer);
+ status = BS->InstallConfigurationTable(&EfiDtbTableGuid,

+ (VOID *)fdt_buffer);

if (EFI_ERROR(status)) {
- FreePool(fdt_buffer);
+ (VOID) BS->FreePages(fdt_buffer, SIZE_IN_PAGES(size));

error(L"Failed to install alternative device tree", status);
}

--
2.40.1

[Jan Kiszka]

Thanks, applied.