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

[Jan Beulich <jbeulich@xxxxxxxx>]

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 != .

>>> @@ -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).

Jan