Re: [Xen-devel] [PATCH] efi: Reallocate memory map if ExitBootServices() fails

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.05.15 at 11:57, <ross.lagerwall@xxxxxxxxxx> wrote:
> On 05/29/2015 10:45 AM, Jan Beulich wrote:
>>>>> On 29.05.15 at 09:48, <ross.lagerwall@xxxxxxxxxx> wrote:
>>> --- a/xen/common/efi/boot.c
>>> +++ b/xen/common/efi/boot.c
>>> @@ -1053,14 +1053,14 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> *SystemTable)
>>>               efi_arch_video_init(gop, info_size, mode_info);
>>>       }
>>>
>>> -    efi_bs->GetMemoryMap(&efi_memmap_size, NULL, &map_key,
>>> -                         &efi_mdesc_size, &mdesc_ver);
>>> -    efi_memmap = efi_arch_allocate_mmap_buffer(&efi_memmap_size);
>>> -    if ( !efi_memmap )
>>> -        blexit(L"Unable to allocate memory for EFI memory map");
>>> -
>>>       for ( retry = 0; ; retry = 1 )
>>>       {
>>> +        efi_bs->GetMemoryMap(&efi_memmap_size, NULL, &map_key,
>>> +                             &efi_mdesc_size, &mdesc_ver);
>>> +        efi_memmap = efi_arch_allocate_mmap_buffer(&efi_memmap_size);
>>> +        if ( !efi_memmap )
>>> +            blexit(L"Unable to allocate memory for EFI memory map");
>>
>> You can't blexit() anymore after having called ExitBootServices() once.
>> Admittedly even the PrintErrMesg() used for "error handling" a few
>> lines down is on the edge of being invalid (but this is a best effort
>> thing anyway).
> 
> OK, I'll convert it into a PrintErrMesg.

Conditionally upon being in the second iteration.

>> Since, finally, this is only a workaround, as the spec clearly says:
>> "After an Operating System calls ExitBootServices(), firmware boot
>> services are no longer available and it is illegal to call any boot
>> service." Even our re-invocation of GetMemoryMap() is bending
>> that (and I only hesitantly agreed for it to be added).
> 
> The spec kinda disagrees with that:
> "It is suggested that GetMemoryMap()be called immediately before calling 
> ExitBootServices(). If MapKey value is incorrect, ExitBootServices() 
> returns EFI_INVALID_PARAMETER and GetMemoryMap() with ExitBootServices() 
> must be called again. Firmware implementation may choose to do a partial 
> shutdown of the boot services during the first call to 
> ExitBootServices(). EFI OS loader should not make calls to any boot 
> service function other then GetMemoryMap() after the first call to 
> ExitBootServices()."

This was the grounds on which the current retry loop was added.

> I think the patch does the right thing in the regard.

For x86 yes, since efi_arch_allocate_mmap_buffer() doesn't use
boot services there. For ARM no, due to its use of AllocatePool().
Iirc Daniel was also planning on changing the x86 side too, and
such a change would break things in non-obvious ways. Hence I
think depending on the ability to do another allocation after the
first ExitBootServices() call is not really a good idea.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel