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

[Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>]

On Wed, Jul 06, 2022 at 10:44:46AM +0000, 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.
> 
> Which perhaps isn't surprising because presumably ARM gets memory
> information from the DT, not EFI?
> 
> ~Andrew

Which in turn would explain why the problem is ARM-specific, and would
mean that disabling this on ARM would solve the problem.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature