From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <lersek@redhat.com>
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 <edk2-devel@lists.01.org>; 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 <jordan.l.justen@intel.com>
Cc: Jeff Fan <jeff.fan@intel.com>, Jiewen Yao <jiewen.yao@intel.com>,
 Michael Kinney <michael.d.kinney@intel.com>,
 edk2-devel-01 <edk2-devel@lists.01.org>
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 <lersek@redhat.com>
Message-ID: <ba6fbe72-2639-2cfb-87c3-b30da7f3dc89@redhat.com>
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  <edk2-devel.lists.01.org>
List-Unsubscribe: <https://lists.01.org/mailman/options/edk2-devel>,
 <mailto:edk2-devel-request@lists.01.org?subject=unsubscribe>
List-Archive: <http://lists.01.org/pipermail/edk2-devel/>
List-Post: <mailto:edk2-devel@lists.01.org>
List-Help: <mailto:edk2-devel-request@lists.01.org?subject=help>
List-Subscribe: <https://lists.01.org/mailman/listinfo/edk2-devel>,
 <mailto:edk2-devel-request@lists.01.org?subject=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