From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.31; helo=mga06.intel.com; envelope-from=jordan.l.justen@intel.com; receiver=edk2-devel@lists.01.org Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 9D0982034BBD4 for ; Wed, 8 Nov 2017 17:49:57 -0800 (PST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga104.jf.intel.com with ESMTP; 08 Nov 2017 17:53:58 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,367,1505804400"; d="scan'208";a="2769866" Received: from jwjarre1-mobl1.amr.corp.intel.com (HELO localhost) ([10.255.76.183]) by orsmga001.jf.intel.com with ESMTP; 08 Nov 2017 17:53:58 -0800 MIME-Version: 1.0 To: Laszlo Ersek , Ruiyu Ni Message-ID: <151019243761.10467.634318081879242382@jljusten-skl> From: Jordan Justen In-Reply-To: <03e369bb-77c4-0134-258f-bdae62cbc8c5@redhat.com> Cc: Eric Dong , Ard Biesheuvel , edk2-devel@lists.01.org, Jiewen Yao , Michael D Kinney References: <20171012084810.148196-1-ruiyu.ni@intel.com> <20171012084810.148196-4-ruiyu.ni@intel.com> <03e369bb-77c4-0134-258f-bdae62cbc8c5@redhat.com> User-Agent: alot/0.6 Date: Wed, 08 Nov 2017 17:53:57 -0800 Subject: Re: [PATCH 3/4] UefiCpuPkg/MtrrLib: Update algorithm to calculate optimal settings X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Nov 2017 01:49:57 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On 2017-11-08 17:36:01, Laszlo Ersek wrote: > Hi Ray, > = > On 10/12/17 10:48, Ruiyu Ni wrote: > > The new algorithm converts the problem calculating optimal > > MTRR settings (using least MTRR registers) to the problem finding > > the shortest path in a graph. > > The memory required in extreme but rare case can be up to 256KB, > > so using local stack buffer is impossible considering current > > DxeIpl only allocates 128KB stack. > > = > > The patch changes existing MtrrSetMemoryAttributeInMtrrSettings() and > > MtrrSetMemoryAttribute() to use the 4-page stack buffer for > > calculation. The two APIs return BUFFER_TOO_SMALL when the buffer > > is too small for calculation. > = > [snip] > = > > +#define SCRATCH_BUFFER_SIZE (4 * SIZE_4KB) > = > [snip] > = > > RETURN_STATUS > > EFIAPI > > -MtrrSetMemoryAttribute ( > > +MtrrSetMemoryAttributeInMtrrSettings ( > > + IN OUT MTRR_SETTINGS *MtrrSetting, > > IN PHYSICAL_ADDRESS BaseAddress, > > IN UINT64 Length, > > IN MTRR_MEMORY_CACHE_TYPE Attribute > > ) > > { > > RETURN_STATUS Status; > > + UINT8 Scratch[SCRATCH_BUFFER_SIZE]; > = > [snip] > = > (This patch is now commit 2bbd7e2fbd4b.) > = > Today I managed to spend time on > = > https://bugzilla.tianocore.org/show_bug.cgi?id=3D747 > = > (which is in turn based on the earlier mailing list thread > = > [edk2] dynamic PCD impact on temporary PEI memory > https://lists.01.org/pipermail/edk2-devel/2017-October/016213.html > ). > = > While writing the patches, I found the root cause of BZ#747: > "OvmfPkg/PlatformPei" calls MtrrLib APIs, and due to the above 16KB > stack allocation, MtrrLib overflow's OVMF's 16KB (total) temp SEC/PEI > stack. I thought it was considered bad form to use a significant portion of the stack (> ~100 bytes) via local variables. This used to occasionally break MSVC builds as MS would insert a stack check call if the locals size exceeded some threshold. For a BASE library, I think this should go beyond "bad form" and into not allowed. -Jordan