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 30E8A21A07A8B for ; Mon, 19 Jun 2017 12:38:32 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5CB64811A7; Mon, 19 Jun 2017 19:39:53 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 5CB64811A7 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 5CB64811A7 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-113.phx2.redhat.com [10.3.116.113]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1046717108; Mon, 19 Jun 2017 19:39:51 +0000 (UTC) To: Jordan Justen , Jeff Fan , Michael Kinney , Jiewen Yao , edk2-devel-01 References: <20170608171333.17937-1-lersek@redhat.com> <20170608171333.17937-2-lersek@redhat.com> <149789342556.32751.17592475673245441129@jljusten-skl> From: Laszlo Ersek Message-ID: <384c7b21-dc46-25ba-c90e-75ae07ab5921@redhat.com> Date: Mon, 19 Jun 2017 21:39:51 +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: <149789342556.32751.17592475673245441129@jljusten-skl> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Mon, 19 Jun 2017 19:39:53 +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, 19 Jun 2017 19:38:32 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 06/19/17 19:30, Jordan Justen wrote: > On 2017-06-08 10:13:29, Laszlo Ersek wrote: >> OvmfPkg contains three modules that work with the TSEG (SMRAM) size: >> PlatformPei (PEIM), SmmAccessPei (PEIM), and SmmAccess2Dxe (DXE_DRIVER). >> These modules open-code the interpretation of the ESMRAMC register's >> TSEG_SZ bit-field. That is OK as long as we stick with the Q35 hardware >> spec and nothing more, but it makes it difficult to benefit from an >> upcoming QEMU feature, namely extended TSEG sizes. >> >> Introduce the Q35TsegSizeLib class, and its sole lib instance, for >> extracting / centralizing TSEG size querying and interpretation. This >> library instance is self contained and does not consume dynamic PCDs (for >> example, it doesn't consume PcdOvmfHostBridgePciDevId), because such PCDs >> tend to be set in PlatformPei, but the dispatch order between PlatformPei >> and SmmAccessPei is unspecified (both have TRUE for DEPEX). > > I think that on 'real' platforms there would be an enforced ordering > of Platform PEI before the PEI SMM drivers. I'm not sure what the > mechanism is, and whether it is applicable to OVMF. > > Jeff, Mike, Jiewen, Do you know? Do the chipset SMM drivers depend on > gEfiPeiMemoryDiscoveredPpiGuid that is triggered by Platform PEI? In OVMF's Ia32X64 build report file (with SMM enabled), the only PEIM with a DEPEX on gEfiPeiMemoryDiscoveredPpiGuid is CpuMpPei. The final DEPEX of SmmAccessPei is ((TRUE) AND (gEfiPeiPcdPpiGuid)); the latter is inherited from the PcdLib instance that SmmAccessPei uses. (I think.) > Does > gEfiPeiMemoryDiscoveredPpiGuid also install on S3 resume? Yes. That PPI GUID is produced as a consequence of our call to PublishSystemMemory(), which we (must) do on the S3 resume path as well (using a pre-reserved area); see PublishPeiMemory() in "MemDetect.c". And, we actively use CpuMpPei to set the Feature Control MSR on all processors, on both normal boot and S3 resume. (See "FeatureControl.c".) > > Laszlo, It sounded like you'd be open to using a PCD, but the PEI > driver ordering issue prevented it. Is that right? * If we can turn the following two variables into dynamic PCDs: - mPreferredEsmramcTsegSzMask (UINT8) - mExtendedTsegMbytes (UINT16) such that PlatformPei sets them "early enough" on both normal boot and S3 resume, incorporating the logic of the Q35TsegSizeGetPreferences() function, then the library instance itself can drop Q35TsegSizeGetPreferences(), and the rest of the library functions can be rebased to these PCDs. * The library interfaces should stay as they are; they provide precisely what the client code needs. 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. Thanks, Laszlo