On Thu, May 19, 2022 at 12:32:33PM +0200, Jan Beulich wrote:
> On 18.05.2022 19:32, Demi Marie Obenour wrote:
> > @@ -567,6 +587,39 @@ static int __init efi_check_dt_boot(const EFI_LOADED_IMAGE *loaded_image)
> >  }
> >  #endif
> >  
> > +static UINTN __initdata esrt = EFI_INVALID_TABLE_ADDR;
> 
> Just out of curiosity: It's an arbitrary choice to use this initializer,
> i.e. no initializer (and hence zero) would do as well (with ...

That is correct.  I chose EFI_INVALID_TABLE_ADDR because it seemed meant
for this purpose.

> > +static size_t __init get_esrt_size(const EFI_MEMORY_DESCRIPTOR *desc)
> > +{
> > +    size_t available_len, len;
> > +    const UINTN physical_start = desc->PhysicalStart;
> > +    const EFI_SYSTEM_RESOURCE_TABLE *esrt_ptr;
> > +
> > +    len = desc->NumberOfPages << EFI_PAGE_SHIFT;
> > +    if ( esrt == EFI_INVALID_TABLE_ADDR )
> 
> ... an adjustment here, of course)?
> 
> > +        return 0;
> > +    if ( physical_start > esrt || esrt - physical_start >= len )
> > +        return 0;
> > +    /*
> > +     * The specification requires EfiBootServicesData, but accept
> > +     * EfiRuntimeServicesData, which is a more logical choice.
> > +     */
> > +    if ( (desc->Type != EfiRuntimeServicesData) &&
> > +         (desc->Type != EfiBootServicesData) )
> > +        return 0;
> > +    available_len = len - (esrt - physical_start);
> > +    if ( available_len <= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries) )
> > +        return 0;
> > +    available_len -= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries);
> > +    esrt_ptr = (const EFI_SYSTEM_RESOURCE_TABLE *)esrt;
> > +    if ( esrt_ptr->FwResourceVersion != EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION ||
> 
> Nit (style): Overlong line.

Where is the best place to split this?
EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION is a rather long
identifier.

> > +         !esrt_ptr->FwResourceCount )
> > +        return 0;
> > +    if ( esrt_ptr->FwResourceCount > available_len / sizeof(esrt_ptr->Entries[0]) )
> > +        return 0;
> > +    return esrt_ptr->FwResourceCount * sizeof(esrt_ptr->Entries[0]);
> > +}
> 
> Nit (style again): We generally put a blank line ahead of a function's
> main return statement.

Will fix in v7.

> > @@ -1067,6 +1122,46 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
> >      if ( !efi_memmap )
> >          blexit(L"Unable to allocate memory for EFI memory map");
> >  
> > +    efi_memmap_size = info_size;
> 
> I don't think this global needs setting here, yet? The local will
> do just fine here, likely yielding smaller code. But I realize that's
> connected to how you did your change vs what I was expecting you to
> do (see below).
> 
> > +    status = SystemTable->BootServices->GetMemoryMap(&efi_memmap_size,
> > +                                                     efi_memmap, &map_key,
> > +                                                     &efi_mdesc_size,
> > +                                                     &mdesc_ver);
> > +    if ( EFI_ERROR(status) )
> > +        PrintErrMesg(L"Cannot obtain memory map", status);
> > +
> > +    /* Try to obtain the ESRT.  Errors are not fatal. */
> > +    for ( i = 0; i < efi_memmap_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);
> > +
> > +        if ( !esrt_size )
> > +            continue;
> > +        if ( ((EFI_MEMORY_DESCRIPTOR *)(efi_memmap + i))->Type ==
> > +             EfiRuntimeServicesData )
> > +            break; /* ESRT already safe from reuse */
> > +        status = efi_bs->AllocatePool(EfiRuntimeServicesData, esrt_size,
> > +                                      &new_esrt);
> 
> I should have re-raised the earlier voiced concern when reviewing v5 (or
> maybe already v4), and I'm sorry for not having paid close enough
> attention: This may add up to two more entries in the memory map (if an
> entry is split and its middle part is used; of course with an unusual
> implementation there could be even more of a growth). Yet below your
> addition, before obtaining the final memory map, you don't re- obtain
> (and re-increase) the size needed. As to (re-)increase: In fact, prior
> to the allocation you do there shouldn't be a need to bump the space by
> 8 extra entries. That's a safety measure only for possible allocations
> happening across ExitBootServices().
> 
> And yes, in earlier versions you had
> 
> -    info_size += 8 * efi_mdesc_size;
> +    info_size += 8 * (efi_mdesc_size + 1);
> 
> there, but that's not what would be needed anyway (if trying to avoid
> a 2nd pass of establishing the needed size). Instead in such an event
> you need to bump 8 to 10 (or at least 9, when assuming that normally it
> wouldn't be the middle part of a new range which would be used, but
> rather the leading or trailing one).
> 
> While I'd be okay with addressing the two nits above while committing,
> this allocation size aspect first wants settling on. Personally I'd
> prefer the more involved solution, but I'd be okay with merely
> bumping the 8 (plus the addition of a suitable comment, explaining
> the now multiple [two] constituent parts of a seemingly arbitrary
> number). If you want to go this easier route, I guess I could also
> make that adjustment while committing (and adding my R-b).

I would prefer the more involved solution too, but I am not quite sure
how to implement it.  Should Xen call GetMemoryMap() in a loop, retrying
as long as it returns EFI_BUFFER_TOO_SMALL?  If I do get
EFI_BUFFER_TOO_SMALL, how should I allocate memory for the new buffer?
Should I ask ebmalloc() to provide all remaining memory, and then tell
it how much was actually used?

Once I understand how to allocate the memory for the new memory map, and
where to split the long line mentioned above, I will send a v7.

-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab