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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.07.2022 12:58, Demi Marie Obenour wrote:
> On Wed, Jul 06, 2022 at 12:55:50PM +0200, Jan Beulich wrote:
>> 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.
> 
> Sorry about that.  Want me to submit a v9?

Yes please. And please make sure you have tested this at least on x86.

Jan