Re: [PATCH V5 03/15] create arch functions to allocate memory for and process EFI memory map.

[Roy Franz <roy.franz@linaro.org>]

On Fri, Sep 19, 2014 at 1:47 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 19.09.14 at 00:49, <roy.franz@linaro.org> wrote:
>> @@ -657,16 +655,18 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>>      EFI_STATUS status;
>>      unsigned int i, argc;
>>      CHAR16 **argv, *file_name, *cfg_file_name = NULL;
>> -    UINTN cols, rows, depth, size, map_key, info_size, gop_mode = ~0;
>> +    UINTN cols, rows, depth, size, info_size, gop_mode = ~0;
>>      EFI_HANDLE *handles = NULL;
>>      EFI_SHIM_LOCK_PROTOCOL *shim_lock;
>>      EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
>>      EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info;
>>      EFI_FILE_HANDLE dir_handle;
>>      union string section = { NULL }, name;
>> -    struct e820entry *e;
>>      u64 efer;
>>      bool_t base_video = 0;
>> +    UINT32 mmap_desc_ver = 0;
>> +    UINTN mmap_size, mmap_desc_size, mmap_key = 0;
>
> Are these initializers really needed?
The base_video is, some of the others aren't in the current form.  I will
audit the initialized local variables here.
>
> Also I previously commented about mdesc_ver having been static
> rather than automatic for a reason, yet you still don't retain that.

Sorry, I had missed that bit in the virtual mapping issues.  I'll return this
to being static.
>
> Similarly for efi_memmap_size, efi_mdesc_size, and efi_memmap:
> With runtime.c moved and (presumably later in the series) also
> built for ARM, there's no point in moving the setting of these into
> efi_arch_process_memory_map().

Yes, I missed undoing this as part of pulling the in the global
runtime variables.  These should
all be able to be reverted.

>
>> @@ -1262,67 +1260,20 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>>          }
>>      }
>>
>> -    efi_bs->GetMemoryMap(&efi_memmap_size, NULL, &map_key,
>> -                         &efi_mdesc_size, &mdesc_ver);
>> -    mbi.mem_upper -= efi_memmap_size;
>> -    mbi.mem_upper &= -__alignof__(EFI_MEMORY_DESCRIPTOR);
>> -    if ( mbi.mem_upper < xen_phys_start )
>> -        blexit(L"Out of static memory");
>> -    efi_memmap = (void *)(long)mbi.mem_upper;
>> -    status = efi_bs->GetMemoryMap(&efi_memmap_size, efi_memmap, &map_key,
>> -                                  &efi_mdesc_size, &mdesc_ver);
>> +    efi_bs->GetMemoryMap(&mmap_size, NULL, &mmap_key,
>> +                         &mmap_desc_size, &mmap_desc_ver);
>> +    mmap = efi_arch_allocate_mmap_buffer(mmap_size);
>> +    if ( !mmap )
>> +        blexit(L"ERROR Unable to allocate memory for EFI memory map\r\n");
>
> blexit() appends a newline itself.

I'll fix this and review the other blexit() calls.
>
> Jan
>