Re: [Xen-devel] [PATCH V4 03/15] create arch functions to get and process EFI memory map.

[Roy Franz <roy.franz@xxxxxxxxxx>]

On Thu, Sep 11, 2014 at 7:11 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 10.09.14 at 02:51, <roy.franz@xxxxxxxxxx> wrote:
>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -56,8 +56,6 @@ static EFI_HANDLE __initdata efi_ih;
>>  static SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdOut;
>>  static SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdErr;
>>
>> -static UINT32 __initdata mdesc_ver;
>
> Is this correct for the USE_SET_VIRTUAL_ADDRESS_MAP case?
I'm not sure.  That is more of a runtime services issue, and along
with the unconditional disabling
of using set virtual address map, I haven't really considered that case.

>
>> @@ -1171,67 +1169,12 @@ 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);
>> -    if ( EFI_ERROR(status) )
>> -        PrintErrMesg(L"Cannot obtain memory map", status);
>> +    efi_arch_get_memory_map(&mmap_size, &mmap, &mmap_key,
>> +                                  &mmap_desc_size, &mmap_desc_ver);
>
> The only arch-specific bit here is where to put the map.
Yes, but the ARM method of allocating the space can make the map
bigger.  I can re-arrange
this so that only the memory allocation is in the arch code, but I
think the current implementation
is also reasonable.


>
>> --- a/xen/include/asm-x86/efi-boot.h
>> +++ b/xen/include/asm-x86/efi-boot.h
>> @@ -449,3 +449,89 @@ static void __init place_string(u32 *addr, const char 
>> *s)
>>      }
>>      *addr = (long)alloc;
>>  }
>> +
>> +static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE 
>> *SystemTable,
>> +                                               void *map,
>> +                                               UINTN map_size,
>> +                                               UINTN desc_size,
>> +                                               UINT32 desc_ver)
>> +{
>> +    struct e820entry *e;
>> +    int i;
>
> This was "unsigned int" originally, and should remain so. Please
> avoid type changes, or spell them out in the description if you
> find ones which indeed need adjustment.
This was unintentional - I'll fix this.


>
>> +static void __init efi_arch_get_memory_map(UINTN *map_size,
>> +                                             void **map,
>> +                                             UINTN *map_key, UINTN 
>> *desc_size,
>> +                                             UINT32 *desc_ver)
>> +{
>> +    EFI_STATUS status;
>
> Blank line after declarations please.

Noted - I'll review this patch-wide.
>
> Jan
>

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