Re: [kvm-unit-tests PATCH v3 2/4] x86/efi: Retry call to efi exit boot services

["Paluri, PavanKumar" <papaluri@amd.com>]

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