| Message ID | 20240328152112.800177-2-papaluri@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests,v3,1/4] x86 EFI: Bypass call to fdt_check_header() | expand |
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@amd.com> > --- > 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@linux.dev> (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
On 3/28/2024 11:33 AM, Andrew Jones wrote: > 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@amd.com> >> --- >> 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. > Will make the change. >> + 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. > Will do. >> + 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@linux.dev> > > (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.) > Ah, I missed out on that. I will include a cover-letter for v4 and also plan to drop patches 3 & 4 and send them separately as they aren't very relevant to UEFI fixes and for easier upstreaming. Thanks, Pavan > Thanks, > drew
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 */ + 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); + 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;
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@amd.com> --- lib/efi.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-)