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

[Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>]

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?
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature