On 07.05.2022 06:26, Demi Marie Obenour wrote:
> On Fri, May 06, 2022 at 12:59:05PM +0200, 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?
>
> Will fix in v5.
>
>>> + 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.
>
> Will fix in v5.
>
>>> + 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).
>
> Will fix in v5.
>
>>> + 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().
>
> Whoops! I did not realized PrintErrMsg() was fatal.
>
>>> + }
>>> + }
>>> + 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().
>
> This would mean the allocation would need to be unconditional. Right
> now, I avoid the allocation if it is not necessary.
Hmm, I guess I don't see (taking into account also my own reply to that
comment of mine) why it would need to become unconditional then.
Jan
