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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.07.2022 12:44, Andrew Cooper wrote:
> On 06/07/2022 11:32, Luca Fancellu wrote:
>>> On 24 Jun 2022, at 19:17, Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx> 
>>> wrote:
>>>
>>> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
>>> index a25e1d29f1..f6f34aa816 100644
>>> --- a/xen/common/efi/boot.c
>>> +++ b/xen/common/efi/boot.c
>>> @@ -567,6 +587,41 @@ static int __init efi_check_dt_boot(const 
>>> EFI_LOADED_IMAGE *loaded_image)
>>> }
>>> #endif
>>>
>>> +static UINTN __initdata esrt = EFI_INVALID_TABLE_ADDR;
>>> +
>>> +static size_t __init get_esrt_size(const EFI_MEMORY_DESCRIPTOR *desc)
>>> +{
>>> +    size_t available_len, len;
>>> +    const UINTN physical_start = desc->PhysicalStart;
>> Hi,
>>
>> From my tests on an arm64 machine so far I can tell that desc is NULL here,
>> for this reason we have the problem.
>>
>> I’ll have a further look on the cause of this, but if you are faster than me 
>> you are
>> welcome to give your opinion on that.
> 
> Given this observation, there's clearly ...
> 
>> @@ -1051,6 +1110,70 @@ static void __init 
>> efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop
>> #define INVALID_VIRTUAL_ADDRESS (0xBAAADUL << \
>>                                  (EFI_PAGE_SHIFT + BITS_PER_LONG - 32))
>>
>> +static void __init efi_relocate_esrt(EFI_SYSTEM_TABLE *SystemTable)
>> +{
>> +    EFI_STATUS status;
>> +    UINTN info_size = 0, map_key, mdesc_size;
>> +    void *memory_map = NULL;
>> +    UINT32 ver;
>> +    unsigned int i;
>> +
>> +    for ( ; ; ) {
>> +        status = efi_bs->GetMemoryMap(&info_size, memory_map, &map_key,
>> +                                      &mdesc_size, &ver);
>> +        if ( status == EFI_SUCCESS && memory_map != NULL )
>> +            break;
>> +        if ( status == EFI_BUFFER_TOO_SMALL || memory_map == NULL )
>> +        {
>> +            info_size += 8 * efi_mdesc_size;
>> +            if ( memory_map != NULL )
>> +                efi_bs->FreePool(memory_map);
>> +            memory_map = NULL;
>> +            status = efi_bs->AllocatePool(EfiLoaderData, info_size, 
>> &memory_map);
>> +            if ( status == EFI_SUCCESS )
>> +                continue;
>> +            PrintErr(L"Cannot allocate memory to relocate ESRT\r\n");
>> +        }
>> +        else
>> +            PrintErr(L"Cannot obtain memory map to relocate ESRT\r\n");
>> +        return;
>> +    }
>> +
>> +    /* Try to obtain the ESRT.  Errors are not fatal. */
>> +    for ( i = 0; i < info_size; i += efi_mdesc_size )
>> +    {
>> +        /*
>> +         * ESRT needs to be moved to memory of type EfiRuntimeServicesData
>> +         * so that the memory it is in will not be used for other purposes.
>> +         */
>> +        void *new_esrt = NULL;
>> +        size_t esrt_size = get_esrt_size(efi_memmap + i);
> 
> ... a NULL pointer here.  And the only way that could happen is if
> efi_memmap is NULL.

Incomplete refactoring - this needs to be memory_map, not efi_memmap.
Of course efi_mdesc_size also needs to be mdesc_size. Didn't check
for further leftover from the earlier patch version. I'm going to
revert the commit.

Jan