Re: [Xen-devel] [PATCH v9 06/13] efi: create new early memory allocator

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.10.16 at 14:51, <julien.grall@xxxxxxx> wrote:
> Hello Jan,
> 
> On 12/10/2016 12:45, Jan Beulich wrote:
>>>>> On 11.10.16 at 15:39, <julien.grall@xxxxxxx> wrote:
>>> On 06/10/16 13:21, Jan Beulich wrote:
>>>>>>> On 05.10.16 at 20:30, <julien.grall@xxxxxxx> wrote:
>>>>> On 30/09/2016 02:46, Jan Beulich wrote:
>>>>>>>>> On 29.09.16 at 23:42, <daniel.kiper@xxxxxxxxxx> wrote:
>>>>>>> +#else
>>>>>>> +static void __init free_ebmalloc_unused_mem(void)
>>>>>>> +{
>>>>>>> +}
>>>>>>> +#endif
>>>>>>
>>>>>> Did you build test this for ARM? The function ought to be unused,
>>>>>> as ...
>>>>>>
>>>>>>> @@ -1251,6 +1301,8 @@ void __init efi_init_memory(void)
>>>>>>>      } *extra, *extra_head = NULL;
>>>>>>>  #endif
>>>>>>>
>>>>>>> +    free_ebmalloc_unused_mem();
>>>>>>
>>>>>> ... the whole function here doesn't get built on ARM.
>>>>>>
>>>>>> Julien - we're still awaiting your input on general aspects here.
>>>>>
>>>>> efi_init_memory would need to be called during Xen boot on ARM. I am not
>>>>> sure where as I we don't yet have runtime support on ARM.
>>>>>
>>>>> Other than that, the patch looks good to me.
>>>>
>>>> But that wasn't the question. My goal is to have as little code
>>>> inside #ifndef CONFIG_ARM as possible, and hence I'd like to have
>>>> as much of this new code as possible outside of such conditionals.
>>>> So the question really is whether that alternative approach would
>>>> be fine with you, or what problems you might see.
>>>
>>> I am not sure to get it. The current approach looks good to me, however,
>>> the implementation should not be exposed to ARM until all the TODOs
>>> mentioned by Daniel are fixed.
>>
>> Which is precisely the opposite of what I'm aiming at. Once again:
>> Don't you think it is desirable to keep the #ifndef CONFIG_ARM
>> instances to cover as little code as possible? Not all of the named
>> TODOs really need to be addressed in order to compile most of
>> what comprises this new allocator; in fact none of them really
>> needs addressing:
>> - if the size estimation turns out to low once ARM starts actually
>>   using this, let's just bump it (perhaps by making it a per-arch
>>   constant),
>> - if the section chosen needs to be different (which it really
>>   shouldn't be), let's simply adjust it,
> 
> If we keep the section in BSS, then we really need to move the 
> initialization of BSS earlier.

Right, but that should be simple enough. Or we do ...

> This TODO really needs to be fixed now otherwise it will be a nightmare 
> to debug later on.
> 
>> - as we've already figured there's no need for the stub
>>   free_ebmalloc_unused_mem() right now anyway.
> 
> But we would need to call free_ebmalloc_unused_mem from somewhere. The 
> idea to not expose the early memory allocator on ARM is avoid to have an 
> implementation with may not fully work on ARM because of known missing 
> pieces.
> 
>> And then (as another alternative) we have the option of ARM
>> simply defining EBMALLOC_SIZE to zero for the time being. That
>> would eliminate the need to actually call free_ebmalloc_unused_mem()
>> and turn the other two items into non-issues.

... this, which you didn't comment on at all.

Jan


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