| Message ID | 20200507181937.8923-1-xypron.glpk@gmx.de |
|---|---|
| State | Accepted |
| Commit | 42a426e027df472714e8d6209cafac291935c331 |
| Headers | show |
| Series | [v2,1/1] efi_loader: put device tree into EfiACPIReclaimMemory | expand |
On 07/05/2020 19:19, Heinrich Schuchardt wrote: > According to the UEFI spec ACPI tables should be placed in > EfiACPIReclaimMemory. Let's do the same with the device tree. > > Suggested-by: Ard Biesheuvel <ardb at kernel.org> > Cc: Grant Likely <grant.likely at arm.com> > Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de> > --- > v2: > adjust the unit test Is there any impact to changing the memory type for current users? Does the kernel currently expect the EFI_BOOT_SERVICES_DATA memory type? What happens if Grub or another EFI application replaces the DTB table? Will it try to use a different memory type, and will that matter? g. > --- > cmd/bootefi.c | 4 ++-- > lib/efi_selftest/efi_selftest_memory.c | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c > index 54b4b8f984..06573b14e9 100644 > --- a/cmd/bootefi.c > +++ b/cmd/bootefi.c > @@ -127,13 +127,13 @@ static efi_status_t copy_fdt(void **fdtp) > new_fdt_addr = (uintptr_t)map_sysmem(fdt_ram_start + 0x7f00000 + > fdt_size, 0); > ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, > - EFI_BOOT_SERVICES_DATA, fdt_pages, > + EFI_ACPI_RECLAIM_MEMORY, fdt_pages, > &new_fdt_addr); > if (ret != EFI_SUCCESS) { > /* If we can't put it there, put it somewhere */ > new_fdt_addr = (ulong)memalign(EFI_PAGE_SIZE, fdt_size); > ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, > - EFI_BOOT_SERVICES_DATA, fdt_pages, > + EFI_ACPI_RECLAIM_MEMORY, fdt_pages, > &new_fdt_addr); > if (ret != EFI_SUCCESS) { > printf("ERROR: Failed to reserve space for FDT\n"); > diff --git a/lib/efi_selftest/efi_selftest_memory.c b/lib/efi_selftest/efi_selftest_memory.c > index e71732dc6d..4d32a28006 100644 > --- a/lib/efi_selftest/efi_selftest_memory.c > +++ b/lib/efi_selftest/efi_selftest_memory.c > @@ -176,9 +176,9 @@ static int execute(void) > /* Check memory reservation for the device tree */ > if (fdt_addr && > find_in_memory_map(map_size, memory_map, desc_size, fdt_addr, > - EFI_BOOT_SERVICES_DATA) != EFI_ST_SUCCESS) { > + EFI_ACPI_RECLAIM_MEMORY) != EFI_ST_SUCCESS) { > efi_st_error > - ("Device tree not marked as boot services data\n"); > + ("Device tree not marked as ACPI reclaim memory\n"); > return EFI_ST_FAILURE; > } > return EFI_ST_SUCCESS; > -- > 2.26.2 >
On 11.05.20 10:48, Grant Likely wrote: > On 07/05/2020 19:19, Heinrich Schuchardt wrote: >> According to the UEFI spec ACPI tables should be placed in >> EfiACPIReclaimMemory. Let's do the same with the device tree. >> >> Suggested-by: Ard Biesheuvel <ardb at kernel.org> >> Cc: Grant Likely <grant.likely at arm.com> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de> >> --- >> v2: >> ????adjust the unit test > > Is there any impact to changing the memory type for current users? Does > the kernel currently expect the EFI_BOOT_SERVICES_DATA memory type? What > happens if Grub or another EFI application replaces the DTB table? Will > it try to use a different memory type, and will that matter? If we are using GRUB, linux_boot() allocates EFI_LOADER_DATA memory and copies the FDT to it. When the devicetree command is used GRUB also allocates EFI_LOADER_DATA. So changes in U-Boot have no effect on what Linux sees if booted via GRUB. Both EFI_BOOT_SERVICES_DATA and EFI_LOADER_DATA are meant to be gone past ExitBootServices(). EfiACPIReclaimMemory is were the UEFI payload is adviced to consider if keeping the memory at runtime is needed. The current patch therefore makes no difference to Linux before ExitBootServices(). A side effect could be that Linux unnecessarily keeps the EfiACPIReclaimMemory instead of reusing it. But Linux's is_usable_memory() in drivers/firmware/efi/arm-init.c treats EFI_ACPI_RECLAIM_MEMORY, EFI_ACPI_RECLAIM_MEMORY, EFI_ACPI_RECLAIM_MEMORY just the same. @Daniel, Leif A patch for the EBBR specification suggests to use EFI_ACPI_RECLAIM_MEMORY for the devicetree passed to Linux. Please, consider if you would apply this in GRUB too. Best regards Heinrich > > g. > >> --- >> ? cmd/bootefi.c????????????????????????? | 4 ++-- >> ? lib/efi_selftest/efi_selftest_memory.c | 4 ++-- >> ? 2 files changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/cmd/bootefi.c b/cmd/bootefi.c >> index 54b4b8f984..06573b14e9 100644 >> --- a/cmd/bootefi.c >> +++ b/cmd/bootefi.c >> @@ -127,13 +127,13 @@ static efi_status_t copy_fdt(void **fdtp) >> ????? new_fdt_addr = (uintptr_t)map_sysmem(fdt_ram_start + 0x7f00000 + >> ?????????????????????????? fdt_size, 0); >> ????? ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, >> -???????????????? EFI_BOOT_SERVICES_DATA, fdt_pages, >> +???????????????? EFI_ACPI_RECLAIM_MEMORY, fdt_pages, >> ?????????????????? &new_fdt_addr); >> ????? if (ret != EFI_SUCCESS) { >> ????????? /* If we can't put it there, put it somewhere */ >> ????????? new_fdt_addr = (ulong)memalign(EFI_PAGE_SIZE, fdt_size); >> ????????? ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, >> -???????????????????? EFI_BOOT_SERVICES_DATA, fdt_pages, >> +???????????????????? EFI_ACPI_RECLAIM_MEMORY, fdt_pages, >> ?????????????????????? &new_fdt_addr); >> ????????? if (ret != EFI_SUCCESS) { >> ????????????? printf("ERROR: Failed to reserve space for FDT\n"); >> diff --git a/lib/efi_selftest/efi_selftest_memory.c >> b/lib/efi_selftest/efi_selftest_memory.c >> index e71732dc6d..4d32a28006 100644 >> --- a/lib/efi_selftest/efi_selftest_memory.c >> +++ b/lib/efi_selftest/efi_selftest_memory.c >> @@ -176,9 +176,9 @@ static int execute(void) >> ????? /* Check memory reservation for the device tree */ >> ????? if (fdt_addr && >> ????????? find_in_memory_map(map_size, memory_map, desc_size, fdt_addr, >> -?????????????????? EFI_BOOT_SERVICES_DATA) != EFI_ST_SUCCESS) { >> +?????????????????? EFI_ACPI_RECLAIM_MEMORY) != EFI_ST_SUCCESS) { >> ????????? efi_st_error >> -??????????? ("Device tree not marked as boot services data\n"); >> +??????????? ("Device tree not marked as ACPI reclaim memory\n"); >> ????????? return EFI_ST_FAILURE; >> ????? } >> ????? return EFI_ST_SUCCESS; >> -- >> 2.26.2 >>
On 11/05/2020 12:55, Heinrich Schuchardt wrote: > On 11.05.20 10:48, Grant Likely wrote: >> On 07/05/2020 19:19, Heinrich Schuchardt wrote: >>> According to the UEFI spec ACPI tables should be placed in >>> EfiACPIReclaimMemory. Let's do the same with the device tree. >>> >>> Suggested-by: Ard Biesheuvel <ardb at kernel.org> >>> Cc: Grant Likely <grant.likely at arm.com> >>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de> >>> --- >>> v2: >>> ????adjust the unit test >> >> Is there any impact to changing the memory type for current users? Does >> the kernel currently expect the EFI_BOOT_SERVICES_DATA memory type? What >> happens if Grub or another EFI application replaces the DTB table? Will >> it try to use a different memory type, and will that matter? > > If we are using GRUB, linux_boot() allocates EFI_LOADER_DATA memory and > copies the FDT to it. When the devicetree command is used GRUB also > allocates EFI_LOADER_DATA. > > So changes in U-Boot have no effect on what Linux sees if booted via GRUB. > > Both EFI_BOOT_SERVICES_DATA and EFI_LOADER_DATA are meant to be gone > past ExitBootServices(). EfiACPIReclaimMemory is were the UEFI payload > is adviced to consider if keeping the memory at runtime is needed. > > The current patch therefore makes no difference to Linux before > ExitBootServices(). A side effect could be that Linux unnecessarily > keeps the EfiACPIReclaimMemory instead of reusing it. But Linux's > is_usable_memory() in drivers/firmware/efi/arm-init.c treats > EFI_ACPI_RECLAIM_MEMORY, EFI_ACPI_RECLAIM_MEMORY, > EFI_ACPI_RECLAIM_MEMORY just the same. Okay. Works for me. g. > @Daniel, Leif > A patch for the EBBR specification suggests to use > EFI_ACPI_RECLAIM_MEMORY for the devicetree passed to Linux. Please, > consider if you would apply this in GRUB too. > > Best regards > > Heinrich > >> >> g. >> >>> --- >>> ? cmd/bootefi.c????????????????????????? | 4 ++-- >>> ? lib/efi_selftest/efi_selftest_memory.c | 4 ++-- >>> ? 2 files changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c >>> index 54b4b8f984..06573b14e9 100644 >>> --- a/cmd/bootefi.c >>> +++ b/cmd/bootefi.c >>> @@ -127,13 +127,13 @@ static efi_status_t copy_fdt(void **fdtp) >>> ????? new_fdt_addr = (uintptr_t)map_sysmem(fdt_ram_start + 0x7f00000 + >>> ?????????????????????????? fdt_size, 0); >>> ????? ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, >>> -???????????????? EFI_BOOT_SERVICES_DATA, fdt_pages, >>> +???????????????? EFI_ACPI_RECLAIM_MEMORY, fdt_pages, >>> ?????????????????? &new_fdt_addr); >>> ????? if (ret != EFI_SUCCESS) { >>> ????????? /* If we can't put it there, put it somewhere */ >>> ????????? new_fdt_addr = (ulong)memalign(EFI_PAGE_SIZE, fdt_size); >>> ????????? ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, >>> -???????????????????? EFI_BOOT_SERVICES_DATA, fdt_pages, >>> +???????????????????? EFI_ACPI_RECLAIM_MEMORY, fdt_pages, >>> ?????????????????????? &new_fdt_addr); >>> ????????? if (ret != EFI_SUCCESS) { >>> ????????????? printf("ERROR: Failed to reserve space for FDT\n"); >>> diff --git a/lib/efi_selftest/efi_selftest_memory.c >>> b/lib/efi_selftest/efi_selftest_memory.c >>> index e71732dc6d..4d32a28006 100644 >>> --- a/lib/efi_selftest/efi_selftest_memory.c >>> +++ b/lib/efi_selftest/efi_selftest_memory.c >>> @@ -176,9 +176,9 @@ static int execute(void) >>> ????? /* Check memory reservation for the device tree */ >>> ????? if (fdt_addr && >>> ????????? find_in_memory_map(map_size, memory_map, desc_size, fdt_addr, >>> -?????????????????? EFI_BOOT_SERVICES_DATA) != EFI_ST_SUCCESS) { >>> +?????????????????? EFI_ACPI_RECLAIM_MEMORY) != EFI_ST_SUCCESS) { >>> ????????? efi_st_error >>> -??????????? ("Device tree not marked as boot services data\n"); >>> +??????????? ("Device tree not marked as ACPI reclaim memory\n"); >>> ????????? return EFI_ST_FAILURE; >>> ????? } >>> ????? return EFI_ST_SUCCESS; >>> -- >>> 2.26.2 >>> >
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 54b4b8f984..06573b14e9 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -127,13 +127,13 @@ static efi_status_t copy_fdt(void **fdtp) new_fdt_addr = (uintptr_t)map_sysmem(fdt_ram_start + 0x7f00000 + fdt_size, 0); ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, - EFI_BOOT_SERVICES_DATA, fdt_pages, + EFI_ACPI_RECLAIM_MEMORY, fdt_pages, &new_fdt_addr); if (ret != EFI_SUCCESS) { /* If we can't put it there, put it somewhere */ new_fdt_addr = (ulong)memalign(EFI_PAGE_SIZE, fdt_size); ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, - EFI_BOOT_SERVICES_DATA, fdt_pages, + EFI_ACPI_RECLAIM_MEMORY, fdt_pages, &new_fdt_addr); if (ret != EFI_SUCCESS) { printf("ERROR: Failed to reserve space for FDT\n"); diff --git a/lib/efi_selftest/efi_selftest_memory.c b/lib/efi_selftest/efi_selftest_memory.c index e71732dc6d..4d32a28006 100644 --- a/lib/efi_selftest/efi_selftest_memory.c +++ b/lib/efi_selftest/efi_selftest_memory.c @@ -176,9 +176,9 @@ static int execute(void) /* Check memory reservation for the device tree */ if (fdt_addr && find_in_memory_map(map_size, memory_map, desc_size, fdt_addr, - EFI_BOOT_SERVICES_DATA) != EFI_ST_SUCCESS) { + EFI_ACPI_RECLAIM_MEMORY) != EFI_ST_SUCCESS) { efi_st_error - ("Device tree not marked as boot services data\n"); + ("Device tree not marked as ACPI reclaim memory\n"); return EFI_ST_FAILURE; } return EFI_ST_SUCCESS;
According to the UEFI spec ACPI tables should be placed in EfiACPIReclaimMemory. Let's do the same with the device tree. Suggested-by: Ard Biesheuvel <ardb at kernel.org> Cc: Grant Likely <grant.likely at arm.com> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de> --- v2: adjust the unit test --- cmd/bootefi.c | 4 ++-- lib/efi_selftest/efi_selftest_memory.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) -- 2.26.2