From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 827C02095D206 for ; Mon, 26 Jun 2017 16:03:15 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C6D9F80F9F; Mon, 26 Jun 2017 23:04:44 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com C6D9F80F9F Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com C6D9F80F9F Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-99.phx2.redhat.com [10.3.116.99]) by smtp.corp.redhat.com (Postfix) with ESMTP id BB4DF7E9D9; Mon, 26 Jun 2017 23:04:42 +0000 (UTC) To: Jordan Justen Cc: Jeff Fan , Jiewen Yao , Michael Kinney , edk2-devel-01 References: <20170608171333.17937-1-lersek@redhat.com> <20170608171333.17937-2-lersek@redhat.com> <149789342556.32751.17592475673245441129@jljusten-skl> <384c7b21-dc46-25ba-c90e-75ae07ab5921@redhat.com> <149849999779.24605.15084992650352143991@jljusten-skl> <04147371-25a3-12d2-0cbd-6e07c816a880@redhat.com> <149851656823.26353.11207512663550762369@jljusten-skl> From: Laszlo Ersek Message-ID: Date: Tue, 27 Jun 2017 01:04:41 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 MIME-Version: 1.0 In-Reply-To: <149851656823.26353.11207512663550762369@jljusten-skl> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Mon, 26 Jun 2017 23:04:45 +0000 (UTC) Subject: Re: [PATCH 1/5] OvmfPkg: introduce Q35TsegSizeLib (class header and sole lib instance) 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: Mon, 26 Jun 2017 23:03:15 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 06/27/17 00:36, Jordan Justen wrote: > On 2017-06-26 12:12:45, Laszlo Ersek wrote: >> On 06/26/17 19:59, Jordan Justen wrote: >>> I looked at this series a number of times. My thought is that: >>> >>> * PcdQ35TsegMbytes should become dynamic >>> >>> * No PCD for tseg mask >>> >>> * PlatformPei adds knowledge of qemu ext tseg to set PcdQ35TsegMbytes >>> >>> * SmmAccessPei adds dep on gEfiPeiMemoryDiscoveredPpiGuid to make sure >>> PcdQ35TsegMbytes is set. >> >> Arbitrary and quite ugly, but I guess I can live with it. > > I'm assuming you mean that the gEfiPeiMemoryDiscoveredPpiGuid > dependency is arbitrary and quite ugly. > > I guess it doesn't seem arbitrary and ugly to me, since on most > firmware you'd need to detect memory before considering anything SMM > related. OK. > We should probably still minimize memory usage until Platform > PEI 'installs' it, even though no configuration is required. > >>> * SmmAccessPei adds some duplicated code to know about qemu ext tseg >>> for mask. (But, I think a reasonably small amount of dup code.) >>> >>> * SmramInternal.c just reads the size from the PCD >> >> This last bullet is a functional change for SmramAccessGetCapabilities() >> that I wanted to avoid -- I sought to preserve the logic of working off >> of the currently set (and possibly locked) contents of ESMRAMC.TSEG_SZ. >> >> I guess the idea is still doable if SmramInternal.c introduces a global >> variable for caching the PCD at module startup -- setting the variable >> from the PCD would be the responsibility of each separate module, both >> PEIM and DXE driver --, and then SmramAccessGetCapabilities() could use >> the global variable if it read "11" binary from the TSEG_SZ bit-field >> register. > > Why can't SmramInternal.c read the size from the PCD directly? If I remember correctly, I wanted to base SmramAccessGetCapabilities() directly on hardware configuration. At that point (regardless of whether the function would be called via PEI_SMM_ACCESS_PPI or EFI_SMM_ACCESS2_PROTOCOL), the ESMRAMC.TSEG_SZ bits used as source for the calculation would already be locked, by the SmmAccessPei entry point function. I don't have a clear threat model in my mind, but I feel unsafe basing SmramAccessGetCapabilities() on a dynamic PcdGet() call. Dynamic PCDs can be changed well into BDS (for example by a UEFI app or driver launched / loaded from the UEFI shell prompt), without breaking any rules. I don't think EFI_SMM_ACCESS2_PROTOCOL.GetCapabilities() should return a different output based on that. I just feel unsafe about adding a functional change like this to the series. Most drivers that I can recall from under MdeModulePkg that consume dynamic PCDs tend to save the values to global variables in their entry points. Thanks Laszlo