On 06.05.2022 12:59, Jan Beulich wrote:
> On 05.05.2022 07:38, Demi Marie Obenour wrote:
>> @@ -1077,6 +1110,35 @@ static void __init efi_exit_boot(EFI_HANDLE
>> ImageHandle, EFI_SYSTEM_TABLE *Syste
>> if ( EFI_ERROR(status) )
>> PrintErrMesg(L"Cannot obtain memory map", status);
>>
>> + for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
>> + {
>> + if ( !is_esrt_valid(efi_memmap + i) )
>> + continue;
>
> Instead of repeating the size calculation below, could you make the
> function (with an altered name) simply return the size (and zero if
> not [valid] ESRT), simplifying things below?
>
>> + if ( ((EFI_MEMORY_DESCRIPTOR *)(efi_memmap + i))->Type !=
>> + EfiRuntimeServicesData )
>> + {
>> + /* ESRT needs to be moved to memory of type
>> EfiRuntimeServicesData
>> + * so that the memory it is in will not be used for other
>> purposes */
>
> Nit: Comment style.
>
>> + size_t esrt_size = offsetof(ESRT, Entries) +
>> + ((ESRT *)esrt)->Count * sizeof(ESRT_ENTRY);
>> + void *new_esrt = NULL;
>> + status = efi_bs->AllocatePool(EfiRuntimeServicesData,
>> esrt_size, &new_esrt);
>
> Nit: Please have a blank line between declaration(s) and statement(s).
>
>> + if ( status != EFI_SUCCESS )
>> + {
>> + PrintErrMesg(L"Cannot allocate memory for ESRT",
>> status);
>
> Neither this nor ...
>
>> + break;
>> + }
>> + memcpy(new_esrt, (void *)esrt, esrt_size);
>> + status = efi_bs->InstallConfigurationTable(&esrt_guid,
>> new_esrt);
>> + if ( status != EFI_SUCCESS )
>> + {
>> + PrintErrMesg(L"Cannot install new ESRT", status);
>> + efi_bs->FreePool(new_esrt);
>
> ... this ought to be fatal to the booting of Xen. Yet PrintErrMesg()
> ends in blexit().
>
>> + }
>> + }
>> + break;
>> + }
>> +
>> efi_arch_process_memory_map(SystemTable, efi_memmap,
>> efi_memmap_size,
>> efi_mdesc_size, mdesc_ver);
>
> The allocation may have altered the memory map and hence invalidated what
> was retrieved just before. You'd need to "continue;" without setting
> "retry" to true, but then the question is why you make this allocation
> after retrieving the memory map in the first place. It's not entirely
> clear to me if it can be done _much_ earlier (if it can, doing it earlier
> would of course be better), but since you need to do it before
> ExitBootServices() anyway, and since you will need to call GetMemoryMap()
> afterwards again, you could as well do it before calling GetMemoryMap().
Over lunch I figured that this was partly rubbish. Of course you need to
do the check after GetMemoryMap(). But I still think it would be better if
you moved this out of this function (or at the very least out of the loop)
and not piggy-back on the ExitBootServices() retry mechanism. I'd be
afraid this could end up in a single retry not being sufficient.
Jan
