From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (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 6782321A00AC1 for ; Mon, 26 Jun 2017 10:58:37 -0700 (PDT) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Jun 2017 10:59:59 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.39,396,1493708400"; d="scan'208";a="278809511" Received: from slexing-mobl.amr.corp.intel.com (HELO localhost) ([10.252.128.225]) by fmsmga004.fm.intel.com with ESMTP; 26 Jun 2017 10:59:58 -0700 MIME-Version: 1.0 To: Jeff Fan , Jiewen Yao , Laszlo Ersek , Michael Kinney , edk2-devel-01 Message-ID: <149849999779.24605.15084992650352143991@jljusten-skl> From: Jordan Justen In-Reply-To: <384c7b21-dc46-25ba-c90e-75ae07ab5921@redhat.com> 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> User-Agent: alot/0.5.1 Date: Mon, 26 Jun 2017 10:59:58 -0700 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 17:58:37 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable 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. 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. * 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 Now, as usual, I expect you'll point out some trivially obvious issue that I missed. :) -Jordan