From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Jan Beulich" <JBeulich@suse.com>
Subject: Re: [PATCH 17/18] x86/efi: create new early memory
	allocator
Date: Fri, 27 Mar 2015 13:35:11 +0000
Message-ID: <55156A9F020000780006E928__8550.3707560294$1427463418$gmane$org@mail.emea.novell.com>
References: <1422640462-28103-1-git-send-email-daniel.kiper@oracle.com>
	<1422640462-28103-18-git-send-email-daniel.kiper@oracle.com>
	<54F4AAB502000078000654ED@mail.emea.novell.com>
	<20150327125701.GL8294@olila.local.net-space.pl>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta3.messagelabs.com ([195.245.230.39])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <JBeulich@suse.com>) id 1YbUPr-0006Cy-50
	for xen-devel@lists.xenproject.org; Fri, 27 Mar 2015 13:35:15 +0000
In-Reply-To: <20150327125701.GL8294@olila.local.net-space.pl>
Content-Disposition: inline
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Daniel Kiper <daniel.kiper@oracle.com>
Cc: Juergen Gross <JGross@suse.com>, grub-devel@gnu.org, keir@xen.org, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, roy.franz@linaro.org, ning.sun@intel.com, david.vrabel@citrix.com, phcoder@gmail.com, xen-devel@lists.xenproject.org, qiaowei.ren@intel.com, richard.l.maliszewski@intel.com, gang.wei@intel.com, fu.wei@linaro.org
List-Id: xen-devel@lists.xenproject.org

>>> On 27.03.15 at 13:57, <daniel.kiper@oracle.com> wrote:
> On Mon, Mar 02, 2015 at 05:23:49PM +0000, Jan Beulich wrote:
>> >>> On 30.01.15 at 18:54, <daniel.kiper@oracle.com> wrote:
>> > +{
>> > +    void *ptr;
>> > +
>> > +    /*
>> > +     * Init __malloc_free on runtime. Static initialization
>> > +     * will not work because it puts virtual address there.
>> > +     */
>> > +    if ( __malloc_free == NULL )
>> > +        __malloc_free = __malloc_mem;
>> > +
>> > +    ptr = __malloc_free;
>> > +
>> > +    __malloc_free += size;
>> > +
>> > +    if ( __malloc_free - __malloc_mem > sizeof(__malloc_mem) )
>> > +        blexit(L"Out of static memory\r\n");
>> > +
>> > +    return ptr;
>> > +}
>>
>> You're ignoring alignment requirements here altogether.
> 
> I can understand why __malloc_mem should be let's say page aligned
> but I am not sure why we should care here about alignment inside
> of __malloc_mem array like...
> 
>> > @@ -192,12 +218,7 @@ static void __init
>> > efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
>> >
>> >  static void *__init efi_arch_allocate_mmap_buffer(UINTN *map_size)
>> >  {
>> > -    place_string(&mbi.mem_upper, NULL);
>> > -    mbi.mem_upper -= *map_size;
>> > -    mbi.mem_upper &= -__alignof__(EFI_MEMORY_DESCRIPTOR);
> 
> ...here...

Specifically with the later mentioned potential for sharing this with
ARM I think you have to make sure that things are suitably aligned,
or else you cause aborts.

>> > -    if ( mbi.mem_upper < xen_phys_start )
>> > -        return NULL;
>> > -    return (void *)(long)mbi.mem_upper;
>> > +    return __malloc(*map_size);
>> >  }
>>
>> Which then even suggests that _if_ we go this route, this could be
>> shared with ARM (and hence become common code again).
> 
> So, go or no go this route?

As long as it's being done properly, I'm not wildly opposed.

Jan