On Thu, May 19, 2022 at 04:55:10PM +0200, Jan Beulich wrote:
> On 19.05.2022 16:45, Demi Marie Obenour wrote:
> > On Thu, May 19, 2022 at 12:32:33PM +0200, Jan Beulich wrote:
> >> On 18.05.2022 19:32, Demi Marie Obenour wrote:
> >>> +    /*
> >>> +     * 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.
> 
> There's no good place to split; the only possible (imo) place is after
> the != .

Will do in v7, along with parentheses to avoid any visual confusion.

> >>> @@ -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?
> 
> Well, there are certainly multiple options. I was thinking that you'd
> add a new call to size the memory map, add a few (again 8?) extra
> entries there as well for the allocation, and leave the present sizing
> call effectively alone (and sitting after all of your additions).

How should I allocate memory for the new memory map?  Getting the size
is easy; allocating the memory is the tricky part.  That’s where the
idea of calling AllocatePool() and GetMemoryMap() in a loop came from.

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