| Message ID | 1485954693-25750-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | c8f325a59cfc718d13a50fbc746ed9b415c25e92 |
| Headers | show |
On 1 February 2017 at 13:11, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > Some AArch64 UEFI implementations disable the MMU in ExitBootServices(), > after which unaligned accesses to RAM are no longer supported. > > Commit abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the > kernel") fixed an issue in the memory map handling of the stub FDT code, > but inadvertently created an issue with such firmwares, by moving some > of the FDT manipulation to after the invocation of ExitBootServices(). > However, the stub's libfdt implementation uses the ordinary, accelerated > string functions, which rely on hardware handling of unaligned accesses, > resulting in alignment faults when executed with the MMU off. > > Whether disabling the MMU in ExitBootServices() complies with the UEFI > spec is unclear, but it is a reality we have to deal with, given that > it wasn't a problem before commit abfb7b686a3e was applied. I'm not sure it's unclear, but someone not reading the specification and instead looking at what the unfortunate LinuxLoader did might incorrectly deduce that such is the case. > So fix the situation by moving the update_fdt_memmap() into the callback > invoked by efi_exit_boot_services() right before it hits the button. > > Fixes: abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the kernel") I would sort argue against the Fixes: tag, given that I don't consider that code to be incorrect. I agree that this is a good change regardless. / Leif > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > drivers/firmware/efi/libstub/fdt.c | 14 +++----------- > 1 file changed, 3 insertions(+), 11 deletions(-) > > diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c > index 921dfa047202..260c4b4b492e 100644 > --- a/drivers/firmware/efi/libstub/fdt.c > +++ b/drivers/firmware/efi/libstub/fdt.c > @@ -187,6 +187,7 @@ static efi_status_t update_fdt_memmap(void *fdt, struct efi_boot_memmap *map) > struct exit_boot_struct { > efi_memory_desc_t *runtime_map; > int *runtime_entry_count; > + void *new_fdt_addr; > }; > > static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg, > @@ -202,7 +203,7 @@ static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg, > efi_get_virtmap(*map->map, *map->map_size, *map->desc_size, > p->runtime_map, p->runtime_entry_count); > > - return EFI_SUCCESS; > + return update_fdt_memmap(p->new_fdt_addr, map); > } > > /* > @@ -300,22 +301,13 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table, > > priv.runtime_map = runtime_map; > priv.runtime_entry_count = &runtime_entry_count; > + priv.new_fdt_addr = (void *)*new_fdt_addr; > status = efi_exit_boot_services(sys_table, handle, &map, &priv, > exit_boot_func); > > if (status == EFI_SUCCESS) { > efi_set_virtual_address_map_t *svam; > > - status = update_fdt_memmap((void *)*new_fdt_addr, &map); > - if (status != EFI_SUCCESS) { > - /* > - * The kernel won't get far without the memory map, but > - * may still be able to print something meaningful so > - * return success here. > - */ > - return EFI_SUCCESS; > - } > - > /* Install the new virtual address map */ > svam = sys_table->runtime->set_virtual_address_map; > status = svam(runtime_entry_count * desc_size, desc_size, > -- > 2.7.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 1 February 2017 at 13:39, Leif Lindholm <leif.lindholm@linaro.org> wrote: > On 1 February 2017 at 13:11, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> Some AArch64 UEFI implementations disable the MMU in ExitBootServices(), >> after which unaligned accesses to RAM are no longer supported. >> >> Commit abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the >> kernel") fixed an issue in the memory map handling of the stub FDT code, >> but inadvertently created an issue with such firmwares, by moving some >> of the FDT manipulation to after the invocation of ExitBootServices(). >> However, the stub's libfdt implementation uses the ordinary, accelerated >> string functions, which rely on hardware handling of unaligned accesses, >> resulting in alignment faults when executed with the MMU off. >> >> Whether disabling the MMU in ExitBootServices() complies with the UEFI >> spec is unclear, but it is a reality we have to deal with, given that >> it wasn't a problem before commit abfb7b686a3e was applied. > > I'm not sure it's unclear, but someone not reading the specification > and instead looking > at what the unfortunate LinuxLoader did might incorrectly deduce that > such is the case. > >> So fix the situation by moving the update_fdt_memmap() into the callback >> invoked by efi_exit_boot_services() right before it hits the button. >> >> Fixes: abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the kernel") > > I would sort argue against the Fixes: tag, given that I don't consider > that code to be > incorrect. > Well, that patch ended up in -stable, and is breaking actual systems that worked 'fine' before. > I agree that this is a good change regardless. > Cheers. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 01, 2017 at 01:11:33PM +0000, Ard Biesheuvel wrote: > Some AArch64 UEFI implementations disable the MMU in ExitBootServices(), > after which unaligned accesses to RAM are no longer supported. Urrgh. That's a pretty horrible bug. :( > Commit abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the > kernel") fixed an issue in the memory map handling of the stub FDT code, > but inadvertently created an issue with such firmwares, by moving some > of the FDT manipulation to after the invocation of ExitBootServices(). > However, the stub's libfdt implementation uses the ordinary, accelerated > string functions, which rely on hardware handling of unaligned accesses, > resulting in alignment faults when executed with the MMU off. > > Whether disabling the MMU in ExitBootServices() complies with the UEFI > spec is unclear, but it is a reality we have to deal with, given that > it wasn't a problem before commit abfb7b686a3e was applied. I'd argue that this quite clearly violates the calling convention requiremments in 2.3.6 (e.g. SCTLR_EL1 configuration, which permits unaligned accesses). If ExitBootServices() changes the SCTLR_EL1 value, it clearly violates the calling convention. This would also mean we're calling SetVirtualAddressMap() in violation of said convention. For the timebeing, can we drop this statement regarding the spec from the commit message? Regardless, I agree that we need to handle this somehow. > So fix the situation by moving the update_fdt_memmap() into the callback > invoked by efi_exit_boot_services() right before it hits the button. I think this is a sensible change, but I do suspect that we're likely to have more fun with this broken platform in future as the stub changes (or compilers decide to generate unaligned accesses elsewhere). Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 1 February 2017 at 15:11, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > Some AArch64 UEFI implementations disable the MMU in ExitBootServices(), > after which unaligned accesses to RAM are no longer supported. > > Commit abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the > kernel") fixed an issue in the memory map handling of the stub FDT code, > but inadvertently created an issue with such firmwares, by moving some > of the FDT manipulation to after the invocation of ExitBootServices(). > However, the stub's libfdt implementation uses the ordinary, accelerated > string functions, which rely on hardware handling of unaligned accesses, > resulting in alignment faults when executed with the MMU off. > > Whether disabling the MMU in ExitBootServices() complies with the UEFI > spec is unclear, but it is a reality we have to deal with, given that > it wasn't a problem before commit abfb7b686a3e was applied. > > So fix the situation by moving the update_fdt_memmap() into the callback > invoked by efi_exit_boot_services() right before it hits the button. > > Fixes: abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the kernel") > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> Tested-by: Riku Voipio <riku.voipio@linaro.org> > --- > drivers/firmware/efi/libstub/fdt.c | 14 +++----------- > 1 file changed, 3 insertions(+), 11 deletions(-) > > diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c > index 921dfa047202..260c4b4b492e 100644 > --- a/drivers/firmware/efi/libstub/fdt.c > +++ b/drivers/firmware/efi/libstub/fdt.c > @@ -187,6 +187,7 @@ static efi_status_t update_fdt_memmap(void *fdt, struct efi_boot_memmap *map) > struct exit_boot_struct { > efi_memory_desc_t *runtime_map; > int *runtime_entry_count; > + void *new_fdt_addr; > }; > > static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg, > @@ -202,7 +203,7 @@ static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg, > efi_get_virtmap(*map->map, *map->map_size, *map->desc_size, > p->runtime_map, p->runtime_entry_count); > > - return EFI_SUCCESS; > + return update_fdt_memmap(p->new_fdt_addr, map); > } > > /* > @@ -300,22 +301,13 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table, > > priv.runtime_map = runtime_map; > priv.runtime_entry_count = &runtime_entry_count; > + priv.new_fdt_addr = (void *)*new_fdt_addr; > status = efi_exit_boot_services(sys_table, handle, &map, &priv, > exit_boot_func); > > if (status == EFI_SUCCESS) { > efi_set_virtual_address_map_t *svam; > > - status = update_fdt_memmap((void *)*new_fdt_addr, &map); > - if (status != EFI_SUCCESS) { > - /* > - * The kernel won't get far without the memory map, but > - * may still be able to print something meaningful so > - * return success here. > - */ > - return EFI_SUCCESS; > - } > - > /* Install the new virtual address map */ > svam = sys_table->runtime->set_virtual_address_map; > status = svam(runtime_entry_count * desc_size, desc_size, > -- > 2.7.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c index 921dfa047202..260c4b4b492e 100644 --- a/drivers/firmware/efi/libstub/fdt.c +++ b/drivers/firmware/efi/libstub/fdt.c @@ -187,6 +187,7 @@ static efi_status_t update_fdt_memmap(void *fdt, struct efi_boot_memmap *map) struct exit_boot_struct { efi_memory_desc_t *runtime_map; int *runtime_entry_count; + void *new_fdt_addr; }; static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg, @@ -202,7 +203,7 @@ static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg, efi_get_virtmap(*map->map, *map->map_size, *map->desc_size, p->runtime_map, p->runtime_entry_count); - return EFI_SUCCESS; + return update_fdt_memmap(p->new_fdt_addr, map); } /* @@ -300,22 +301,13 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table, priv.runtime_map = runtime_map; priv.runtime_entry_count = &runtime_entry_count; + priv.new_fdt_addr = (void *)*new_fdt_addr; status = efi_exit_boot_services(sys_table, handle, &map, &priv, exit_boot_func); if (status == EFI_SUCCESS) { efi_set_virtual_address_map_t *svam; - status = update_fdt_memmap((void *)*new_fdt_addr, &map); - if (status != EFI_SUCCESS) { - /* - * The kernel won't get far without the memory map, but - * may still be able to print something meaningful so - * return success here. - */ - return EFI_SUCCESS; - } - /* Install the new virtual address map */ svam = sys_table->runtime->set_virtual_address_map; status = svam(runtime_entry_count * desc_size, desc_size,
Some AArch64 UEFI implementations disable the MMU in ExitBootServices(), after which unaligned accesses to RAM are no longer supported. Commit abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the kernel") fixed an issue in the memory map handling of the stub FDT code, but inadvertently created an issue with such firmwares, by moving some of the FDT manipulation to after the invocation of ExitBootServices(). However, the stub's libfdt implementation uses the ordinary, accelerated string functions, which rely on hardware handling of unaligned accesses, resulting in alignment faults when executed with the MMU off. Whether disabling the MMU in ExitBootServices() complies with the UEFI spec is unclear, but it is a reality we have to deal with, given that it wasn't a problem before commit abfb7b686a3e was applied. So fix the situation by moving the update_fdt_memmap() into the callback invoked by efi_exit_boot_services() right before it hits the button. Fixes: abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the kernel") Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- drivers/firmware/efi/libstub/fdt.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html