On Thu, Mar 28, 2024 at 10:21:10AM -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. Keep freeing
> the old memory map before obtaining new memory map via
> efi_get_memory_map() in case of exit boot services failure.
>
> Signed-off-by: Pavan Kumar Paluri <papaluri@xxxxxxx>
> ---
> lib/efi.c | 34 ++++++++++++++++++++--------------
> 1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/lib/efi.c b/lib/efi.c
> index 8a74a22834a4..d2569b22b4f2 100644
> --- a/lib/efi.c
> +++ b/lib/efi.c
> @@ -406,8 +406,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 */
> @@ -446,13 +446,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) {
> @@ -461,11 +454,24 @@ 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);
> + status = EFI_INVALID_PARAMETER;
> + while (status == EFI_INVALID_PARAMETER) {
> + /* Get EFI memory map */
I tried to get rid of this comment since it states the exact same thing
as the function name below does.
> + status = efi_get_memory_map(&efi_bootinfo.mem_map);
> + if (status != EFI_SUCCESS) {
> + printf("Failed to get memory map\n");
> + goto efi_main_error;
> + }
> + /*
> + * Exit EFI boot services, let kvm-unit-tests take full
> + * control of the guest.
> + */
> + status = efi_exit_boot_services(handle,
> + &efi_bootinfo.mem_map);
We have 100 char lines (and that's just a soft limit) so this would look
better sticking out.
> + 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;
> --
> 2.34.1
>
Besides the nits,
Reviewed-by: Andrew Jones <andrew.jones@xxxxxxxxx>
(A general comment for the series is that we're on v3 but there's no
changelog anywhere. Please use cover letters for a series and then
put the changelog in the cover letter.)
Thanks,
drew
