On 15.05.23 10:16, 'Christian Storm' via EFI Boot Guard wrote:
> 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 <[email protected]>
>>> ---
>>> 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 :)
>
>
>>> +
>>> #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?
>
> 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.
>
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.
>
>>> }
>>> -
>>> - 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
>
Jan
--
Siemens AG, Technology
Competence Center Embedded Linux
--
You received this message because you are subscribed to the Google Groups "EFI
Boot Guard" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/efibootguard-dev/a9381118-4508-a406-b012-61656e1120fc%40siemens.com.
