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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 6 Jul 2022, at 11:59, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> 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.

I will test it on arm64, so please ping me if I miss it, I got submerged by ML 
mails
after my holidays and I didn’t test (on arm) this one like the previous version 
that
you sent before

Cheers,
Luca

> 
> Jan