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

From: "Jan Beulich" <JBeulich@suse.com>
To: Julien Grall <julien.grall@arm.com>
Cc: Juergen Gross <JGross@suse.com>,
	sstabellini@kernel.org, andrew.cooper3@citrix.com,
	Daniel Kiper <daniel.kiper@oracle.com>,
	cardoe@cardoe.com, pgnet.dev@gmail.com, ning.sun@intel.com,
	david.vrabel@citrix.com, xen-devel@lists.xenproject.org,
	qiaowei.ren@intel.com, gang.wei@intel.com, fu.wei@linaro.org
Subject: Re: [PATCH v9 06/13] efi: create new early memory allocator
Date: Wed, 12 Oct 2016 06:59:52 -0600	[thread overview]
Message-ID: <57FE4FE80200007800116B15@prv-mh.provo.novell.com> (raw)
In-Reply-To: <7bec7259-85cd-6ca6-b577-aeb5a7463c0f@arm.com>

>>> On 12.10.16 at 14:51, <julien.grall@arm.com> wrote:
> Hello Jan,
> 
> On 12/10/2016 12:45, Jan Beulich wrote:
>>>>> On 11.10.16 at 15:39, <julien.grall@arm.com> wrote:
>>> On 06/10/16 13:21, Jan Beulich wrote:
>>>>>>> On 05.10.16 at 20:30, <julien.grall@arm.com> wrote:
>>>>> On 30/09/2016 02:46, Jan Beulich wrote:
>>>>>>>>> On 29.09.16 at 23:42, <daniel.kiper@oracle.com> 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@lists.xen.org
https://lists.xen.org/xen-devel