On Tue, Mar 26, 2024 at 12:33:58PM -0500, Pavan Kumar Paluri wrote:
> In some cases, KUT guest might fail to exit boot services due to a
> possible memory map update that might have taken place between
> efi_get_memory_map() and efi_exit_boot_services() calls. As per UEFI
> spec 2.10 (Section 7.4.6 EFI_BOOT_SERVICES.ExitBootServices()), we need
> to keep trying to update the memory map and calls to exit boot
> services as long as case status is EFI_INVALID_PARAMETER.
>
> Signed-off-by: Pavan Kumar Paluri <papaluri@xxxxxxx>
> ---
> lib/efi.c | 23 +++++++++++++++++++++--
> 1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/lib/efi.c b/lib/efi.c
> index 8a74a22834a4..c98bc5c0a022 100644
> --- a/lib/efi.c
> +++ b/lib/efi.c
> @@ -461,16 +461,35 @@ efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
> }
> #endif
>
> - /*
> + /*
> * Exit EFI boot services, let kvm-unit-tests take full control of the
> * guest
> */
> status = efi_exit_boot_services(handle, &efi_bootinfo.mem_map);
> - if (status != EFI_SUCCESS) {
> + if (status != EFI_SUCCESS && status != EFI_INVALID_PARAMETER) {
> printf("Failed to exit boot services\n");
> goto efi_main_error;
> }
>
> + /*
> + * There is a possibility that memory map might have changed
> + * between efi_get_memory_map() and efi_exit_boot_services in
nit: Either add the () to all function names or to none.
> + * which case status is EFI_INVALID_PARAMETER. As per UEFI spec
> + * 2.10, we need to get the updated memory map and keep trying
Even older specs have this, but calling out 2.10 makes it sound like it's
something from 2.10. It's probably not worth looking for when it first
appeared, but normally we'd want to reference the oldest version. If you
don't want to dig that up, then I'm fine with just dropping the number,
or even the whole comment.
> + * until status is not EFI_INVALID_PARAMETER.
> + */
> + while (status == EFI_INVALID_PARAMETER) {
> + efi_get_memory_map(&efi_bootinfo.mem_map);
> +
> + status = efi_exit_boot_services(handle,
> + &efi_bootinfo.mem_map);
> + if (status != EFI_SUCCESS &&
> + status != EFI_INVALID_PARAMETER) {
> + printf("Failed to exit boot services\n");
> + goto efi_main_error;
> + }
> + }
> +
> /* Set up arch-specific resources */
> status = setup_efi(&efi_bootinfo);
> if (status != EFI_SUCCESS) {
> --
> 2.34.1
>
We're leaking the old maps when we need to try again. Also, I see
another issue (which I introduced with efi_get_boot_hartid()) which is
to have a call between getting the memory map and exiting boot services,
which the spec recommends not doing. I think for this patch we should
do something like below (which is untested).
Thanks,
drew
diff --git a/lib/efi.c b/lib/efi.c
index dfbadea60411..d876afefc614 100644
--- a/lib/efi.c
+++ b/lib/efi.c
@@ -404,8 +404,8 @@ efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
efi_system_table = sys_tab;
/* Memory map struct values */
- efi_memory_desc_t *map = NULL;
- unsigned long map_size = 0, desc_size = 0, key = 0, buff_size = 0;
+ efi_memory_desc_t *map;
+ unsigned long map_size, desc_size, key, buff_size;
u32 desc_ver;
/* Helper variables needed to get the cmdline */
@@ -444,13 +444,6 @@ efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
efi_bootinfo.mem_map.key_ptr = &key;
efi_bootinfo.mem_map.buff_size = &buff_size;
- /* Get EFI memory map */
- status = efi_get_memory_map(&efi_bootinfo.mem_map);
- if (status != EFI_SUCCESS) {
- printf("Failed to get memory map\n");
- goto efi_main_error;
- }
-
#ifdef __riscv
status = efi_get_boot_hartid();
if (status != EFI_SUCCESS) {
@@ -463,7 +456,18 @@ efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
* Exit EFI boot services, let kvm-unit-tests take full control of the
* guest
*/
- status = efi_exit_boot_services(handle, &efi_bootinfo.mem_map);
+ status = EFI_INVALID_PARAMETER;
+ while (status == EFI_INVALID_PARAMETER) {
+ status = efi_get_memory_map(&efi_bootinfo.mem_map);
+ if (status != EFI_SUCCESS) {
+ printf("Failed to get memory map\n");
+ goto efi_main_error;
+ }
+
+ status = efi_exit_boot_services(handle, &efi_bootinfo.mem_map);
+ if (status == EFI_INVALID_PARAMETER)
+ efi_free_pool(*efi_bootinfo.mem_map.map);
+ }
if (status != EFI_SUCCESS) {
printf("Failed to exit boot services\n");
goto efi_main_error;
