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

[Jan Beulich <jbeulich@suse.com>]

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