Re: [PATCH 11/19] efi: mm: Extract function to add memory regions

[Daniel Axtens <dja@axtens.net>]

>>    /* Prepare a memory region to store two memory maps.  */
>>    memory_map = grub_efi_allocate_any_pages (2 * BYTES_TO_PAGES (MEMORY_MAP_SIZE));
>>    if (! memory_map)
>> -    grub_fatal ("cannot allocate memory");
>> +    return grub_error (GRUB_ERR_OUT_OF_MEMORY, "cannot allocate memory");
>
> Something more descriptive would be nice, may be even "cannot allocate
> memory for memory map"

Yes.

>>  
>>    /* Obtain descriptors for available memory.  */
>>    map_size = MEMORY_MAP_SIZE;
>> @@ -588,14 +588,14 @@ grub_efi_mm_init (void)
>>  
>>        memory_map = grub_efi_allocate_any_pages (2 * BYTES_TO_PAGES (map_size));
>>        if (! memory_map)
>> -	grub_fatal ("cannot allocate memory");
>> +	return grub_error (GRUB_ERR_OUT_OF_MEMORY, "cannot allocate memory");
>
> Ditto. And maybe nice to have it slightly different, perhaps "cannot
> re-allocate memory map"

Yes.

>>  
>>        mm_status = grub_efi_get_memory_map (&map_size, memory_map, 0,
>>  					   &desc_size, 0);
>>      }
>>  
>>    if (mm_status < 0)
>> -    grub_fatal ("cannot get memory map");
>> +    return grub_error (GRUB_ERR_OUT_OF_MEMORY, "cannot get memory map");
>
> What are the range of values that mm_status could be? Would it be
> useful to include the status code in the error message? IOW, could a
> failure here be affected by a configuration that the user could change
> to make this work? Also I like the message "failed to retrieve memory
> map (status=%d)".
>

The function says:
/* Get the memory map as defined in the EFI spec. Return 1 if successful,
   return 0 if partial, or return -1 if an error occurs.  */

So no, I don't think we could print anything more interesting without
reworking the grub_efi_get_memory_map function.

>>  
>>    memory_map_end = NEXT_MEMORY_DESCRIPTOR (memory_map, map_size);
>>  
>> @@ -610,7 +610,7 @@ grub_efi_mm_init (void)
>>  
>>    /* Allocate memory regions for GRUB's memory management.  */
>>    add_memory_regions (filtered_memory_map, desc_size,
>> -		      filtered_memory_map_end, BYTES_TO_PAGES (DEFAULT_HEAP_SIZE));
>> +		      filtered_memory_map_end, BYTES_TO_PAGES (required_bytes));
>>  
>>  #if 0
>>    /* For debug.  */
>
> What about turning this on when MM_DEBUG is on? Seems like it could be
> a useful (would been to get rid of/change the grub_fatal calls).
>

Perhaps, but I'm not sufficiently across EFI to be confident making that
sort of change, and I'm not well set up to test EFI systems or evaluate
the correctness or usefulness of any output. I think this would be
better deferred to another patch from someone with more experience in
the field.

Kind regards,
Daniel