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 D126C2095A6B2 for ; Mon, 26 Jun 2017 12:11:18 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DA73C12B6; Mon, 26 Jun 2017 19:12:47 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com DA73C12B6 Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com DA73C12B6 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 ACE3477D53; Mon, 26 Jun 2017 19:12:46 +0000 (UTC) To: Jordan Justen 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> From: Laszlo Ersek Cc: Jeff Fan , Jiewen Yao , Michael Kinney , edk2-devel-01 Message-ID: <04147371-25a3-12d2-0cbd-6e07c816a880@redhat.com> Date: Mon, 26 Jun 2017 21:12:45 +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: <149849999779.24605.15084992650352143991@jljusten-skl> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Mon, 26 Jun 2017 19:12:48 +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 19:11:19 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 06/26/17 19:59, Jordan Justen wrote: > On 2017-06-19 12:39:51, Laszlo Ersek wrote: >> >> Even if the PCDs were technically doable, I think I'd slightly prefer >> the current approach. The PCI config space accesses at module startup >> are minimal, and they affect only three modules. OTOH, introducing two >> PCDs feels, in this particular case, a bit like "polluting" OvmfPkg.dec. >> (Well, "PcdPreferredEsmramcTsegSzMask" would *replace* >> "PcdQ35TsegMbytes", so it would mean the addition of one PCD.) >> >> Anyway, this is just a slight preference. What I feel much more strongly >> about are the library APIs. > > The library is the part that seems odd to me. A library just to deal > with Q35 tseg seems a bit specific. Maybe so, but that's only because edk2 requires all libraries (even one-off utility ones) that are used by at least two modules to have full-blown library classes. (NULL class library resolution is different, but those in turn require constructors.) This makes the overhead for small utility libraries worse. Specifically, the first four patches in the series can be considered a refactoring / improvement per se, because they centralize the TSEG_SZ<->megabytes mapping to one module. Extracting the mapping is also useful because the last patch can then add the feature to all affected modules at once. If we proceed piecemeal, then there's going to be a stage where the PEI phase modules know how to handle extended TSEG, but the DXE phase doesn't. Not tragic, but not optimal for bisection. > 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. > * 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. This would be in effect the same as the current patch series, except the bit-field<->MB mappings would remain scattered over the tree. Please confirm if you are OK with the above and I'll spin a v2. Thanks Laszlo > > Now, as usual, I expect you'll point out some trivially obvious issue > that I missed. :) > > -Jordan >