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 05:45:27 -0600	[thread overview]
Message-ID: <57FE3E770200007800116A9C@prv-mh.provo.novell.com> (raw)
In-Reply-To: <99c6b4e2-e9fb-d545-c134-435ae685016c@arm.com>

>>> On 11.10.16 at 15:39, <julien.grall@arm.com> wrote:
> Hello Jan,
> 
> 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,
- as we've already figured there's no need for the stub
  free_ebmalloc_unused_mem() right now anyway.

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.

> I would be happy to review any patches addressing the TODOs.

This, I'm sorry, gets me to raise another question: When is this
finally going to happen? Shared EFI code was introduced for 4.5,
and we're now looking to release 4.8 still with runtime support
unimplemented on ARM. How much longer is this going to take?

Jan

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