From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f67.google.com (mail-wm1-f67.google.com [209.85.128.67]) by mx.groups.io with SMTP id smtpd.web10.8504.1586264910830859419 for ; Tue, 07 Apr 2020 06:08:31 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=UeyEEryA; spf=pass (domain: nuviainc.com, ip: 209.85.128.67, mailfrom: leif@nuviainc.com) Received: by mail-wm1-f67.google.com with SMTP id a81so1745564wmf.5 for ; Tue, 07 Apr 2020 06:08:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=iKGwOycZX8TwmduLAAwCWchPVFwa9Jm2OQqoW/wg5/c=; b=UeyEEryAWpGl3oq255yN+rjEhGJ1YtBsshiXv/hkRmrn1ufej01kB7YgcXhrMC4aj9 PaMNfwtliCOucJuG8XHawd2r3ZOvICM/5DTduS8pmcSJuHL2ovJHiaUUAHk5xd75FsSl R/FFm9jLVSZpy9rcqnmrMpp78G+P37nVHMNxKLeHGii42rWrCBhGQdicyo6uLcW9ZAX0 SIoEqaExb/4i7V0BAoDJBGXpvrW9CG+YMe6XyHBBQWDAIpK8lHUAIp+GaI1yuwviGGvP /Rs+hmL6dziL6bnnXmA/8Eg8S71gYYot5ktBJLHepMwDzkXI/lb8eyqUeyvgF78Kf6+j RyjA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=iKGwOycZX8TwmduLAAwCWchPVFwa9Jm2OQqoW/wg5/c=; b=YrFkPzmdXOnAxRPnQPeMIZSq2pNtQ2ZxOxcZWgUlKDtrXY+3v/IAiWDrvGRPa/MZIL gEJqYsjPjulP0Desc2RQiGrtdmF5W6PuE6FJMZwW9f9gUxRFg73ZxkIzNGiEIgvAnJQU EVB4sxDC315/7nC7lrolZmBnaJXsv4t+0xfZibWYr3kmWqpPWhRB/BMfZZc+2KsiSrUM 0hlKj4l8Za8eAs9RxKKj98j7dsTu/4xwNJAVVmOMCvta+nEDRD3m2HtWg+kSt0bn36bw ZGFWCJaVVUhAiHvuVsg6cyvafRBfIscydmjzTBhY3RxGcl/T4Mz4YeEX88S8xJsGnmRW o74g== X-Gm-Message-State: AGi0PuaZKFSozJoTAwGn0IM/tslXjpEfESa5Mx4BPUFfEef/Oe/KJ1gO j4qnysr0OFb1dIGgOa4PPROWjQ== X-Google-Smtp-Source: APiQypIWb/ZbBLvJUrTVALtFJAAfOqewps9j7Mb6VP0l79qQE/4y4RXrLZty8EnllXzEvMMLZaiKZA== X-Received: by 2002:a1c:750f:: with SMTP id o15mr2424721wmc.110.1586264909279; Tue, 07 Apr 2020 06:08:29 -0700 (PDT) Return-Path: Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8]) by smtp.gmail.com with ESMTPSA id k84sm2344452wmk.2.2020.04.07.06.08.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 Apr 2020 06:08:28 -0700 (PDT) Date: Tue, 7 Apr 2020 14:08:26 +0100 From: "Leif Lindholm" To: "Pankaj Bansal (OSS)" Cc: Meenakshi Aggarwal , Michael D Kinney , "devel@edk2.groups.io" , Varun Sethi , Samer El-Haj-Mahmoud , Jon Nettleton , ard.biesheuvel@arm.com Subject: Re: [PATCH v2 23/28] NXP/LS1043aRdbPkg/ArmPlatformLib: Use Allocate pool Message-ID: <20200407130826.GP14075@vanye> References: <20200320143543.18615-1-pankaj.bansal@oss.nxp.com> <20200320143543.18615-24-pankaj.bansal@oss.nxp.com> <20200401180337.GF7468@vanye> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline +Ard On Mon, Apr 06, 2020 at 15:26:45 +0000, Pankaj Bansal (OSS) wrote: > > -----Original Message----- > > From: Leif Lindholm > > Sent: Wednesday, April 1, 2020 11:34 PM > > To: Pankaj Bansal (OSS) > > Cc: Meenakshi Aggarwal ; Michael D Kinney > > ; devel@edk2.groups.io; Varun Sethi > > ; Samer El-Haj-Mahmoud > Mahmoud@arm.com>; Jon Nettleton > > Subject: Re: [PATCH v2 23/28] NXP/LS1043aRdbPkg/ArmPlatformLib: Use > > Allocate pool > > > > On Fri, Mar 20, 2020 at 20:05:38 +0530, Pankaj Bansal wrote: > > > From: Pankaj Bansal > > > > > > Allocate Pages may allocate more memory than required for > > > VirtualMemoryTable. > > > There is no special requirement that VirtualMemoryTable size should be > > > page size aligned. > > > > > > Therefore, replace AllocatePages with AllocatePool. > > > > > > Signed-off-by: Pankaj Bansal > > > > I don't object to this as such (although one comment), but what is the > > purpose of this change? > > > > My comment is that most other platforms use AllocatePages for this. So > > this is diverging from the norm. > > I referred ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c Sure. This one was converted to AllocatePool because the QEMU virt machine is very simple (because it does not emulate much real hardware) and the port rarely changes. Ard's what's your opinion - do you think this worth it even for a platform that has #define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 25 ? Clearly there is still wasteage going on, but it's 16% less bad. > > Secondly, while I don't necessarily > > *like* the current design (copied across most ARM platforms), it's > > somewhat mitigated by the AllocatePages giving you a minimum of 128 > > entries before you start overwriting things. I don't know what you'll > > overwrite if you do go too far, but you will overwrite it *before* the > > ASSERT ever gets evaluated. > > > > We can improve this by evaluating ASSERT after each entry like this : > VirtualMemoryTable[Index].PhysicalBase = 0xXXXXXXXX; > VirtualMemoryTable[Index].VirtualBase = 0xXXXXXXXX; > VirtualMemoryTable[Index].Length = 0xXXXXXXXX; > VirtualMemoryTable[Index++].Attributes = 0xXXXXXXXX; > > ASSERT (Index < MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS); Whilst functionally preferable, I think that would make for very tedious reading. I'll let Ard call this one. / Leif > > / > > Leif > > > > > --- > > > .../LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLib.inf | 1 + > > > .../LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLibMem.c | 5 +++-- > > > 2 files changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git > > a/Platform/NXP/LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLib.inf > > b/Platform/NXP/LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLib.inf > > > index 1faf99b99c54..c64032f32772 100644 > > > --- > > a/Platform/NXP/LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLib.inf > > > +++ > > b/Platform/NXP/LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLib.inf > > > @@ -25,6 +25,7 @@ [Packages] > > > > > > [LibraryClasses] > > > ArmLib > > > + DebugLib > > > SocLib > > > > > > [Sources.common] > > > diff --git > > a/Platform/NXP/LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLibMem. > > c > > b/Platform/NXP/LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLibMem. > > c > > > index f5fa308551aa..f8dd642e3cff 100644 > > > --- > > a/Platform/NXP/LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLibMem. > > c > > > +++ > > b/Platform/NXP/LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLibMem. > > c > > > @@ -43,10 +43,11 @@ ArmPlatformGetVirtualMemoryMap ( > > > > > > ASSERT (VirtualMemoryMap != NULL); > > > > > > - VirtualMemoryTable = > > (ARM_MEMORY_REGION_DESCRIPTOR*)AllocatePages ( > > > - EFI_SIZE_TO_PAGES (sizeof (ARM_MEMORY_REGION_DESCRIPTOR) * > > MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS)); > > > + VirtualMemoryTable = AllocatePool (sizeof > > (ARM_MEMORY_REGION_DESCRIPTOR) * > > > + MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS); > > > > > > if (VirtualMemoryTable == NULL) { > > > + DEBUG ((DEBUG_ERROR, "%a: Error: Failed AllocatePool()\n", > > __FUNCTION__)); > > > return; > > > } > > > > > > -- > > > 2.17.1 > > >