Re: [PATCH v4] Preserve the EFI System Resource Table for dom0

[Jan Beulich <jbeulich@xxxxxxxx>]

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