From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web12.16.1583973019041047364 for ; Wed, 11 Mar 2020 17:30:19 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=cccotUys; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1583973017; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=k1op4vgZgsJxhP8a6ua8I2kor6Z9RT2PK2YoEyancqw=; b=cccotUyszcxoPcFixNosxQ8jF9Ove7v9l4va5PUy+qB6RkIQBmpd1F76b0ly/+Ufb+/o+A x6cUokFWSwJPcGHK1oTlF7yk6LseQvymnj7nhIOVt2zA7pzcoauRH4lHqniTXCrkE2HFFQ nsTu8XtkdCfSBzJ6Jf86QOdLkl78Tvo= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-408-A3AOjlUPMOGvGDFhpMz0Eg-1; Wed, 11 Mar 2020 20:30:14 -0400 X-MC-Unique: A3AOjlUPMOGvGDFhpMz0Eg-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id B4CFE13F6; Thu, 12 Mar 2020 00:30:12 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-23.ams2.redhat.com [10.36.116.23]) by smtp.corp.redhat.com (Postfix) with ESMTP id 37E028D553; Thu, 12 Mar 2020 00:30:11 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 5/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation To: Leif Lindholm Cc: devel@edk2.groups.io, Ard Biesheuvel , Jordan Justen , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= References: <20200310222739.26717-1-lersek@redhat.com> <20200310222739.26717-6-lersek@redhat.com> <20200311160040.GS23627@bivouac.eciton.net> <20200311193646.GY23627@bivouac.eciton.net> From: "Laszlo Ersek" Message-ID: <7e836a5b-f7ab-9595-0464-a4918524f057@redhat.com> Date: Thu, 12 Mar 2020 01:30:10 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200311193646.GY23627@bivouac.eciton.net> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 03/11/20 20:36, Leif Lindholm wrote: > On Wed, Mar 11, 2020 at 17:22:47 +0100, Laszlo Ersek wrote: >>>> +STATIC EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] = { >>>> + { EfiACPIMemoryNVS, 0x004 }, >>>> + { EfiACPIReclaimMemory, 0x008 }, >>>> + { EfiReservedMemoryType, 0x004 }, >>>> + { EfiRuntimeServicesData, 0x024 }, >>>> + { EfiRuntimeServicesCode, 0x030 }, >>>> + { EfiBootServicesCode, 0x180 }, >>>> + { EfiBootServicesData, 0xF00 }, >>>> + { EfiMaxMemoryType, 0x000 } >>>> +}; >>> >>> Could you add a comment as to where these page counts come from? >>> Oh, right, they're just moved across from OvmfPkg/PlatformPei/Platform.c. >>> >>> It *would* be nice if that could be cleaned up at the same time... >> >> Sorry, I don't understand -- what kind of cleanup do you have in mind >> precisely? The table is not copied, but moved from the original place, >> so I'm unsure what's left in "Platform.c" to clean up. > > Not left to clean up there, but something to consider addressing when > moving it here. Yes, it's just a move, so we could argue it doesn't > need fixing - but it's a struct with a bunch of live-coded numerical > values completely without explanation. > > "I'd rather not" is an acceptable answer, but I figured I should point > it out. Good point! Yet, I'd rather not :) Long read ahead: This table is used for priming the memory type BINs in the DXE Core. After this set, in non-SMM builds, the functionality will remain the same (the table stays in effect for every boot); in SMM builds, the table is only a starting point for the feedback loop. What's important is that the numbers in the table are entirely ad-hoc. They were last updated in commit 991d95636264, in 2010. They capture a set of BIN hints that made sense at the time, for *some* (now unknown) workloads / boot paths. That's the important trait of this table: it made sense at some point in time, for some use case. That's the property we should not regress. So let's consider the possible ways to improve the table. (1) Given that in SMM builds, the table will function only as a starting point for the feedback loop, start using two tables. A new one (for the SMM build) with nice numbers (everything 0x1, or everyting 0x1000, whatever), and keep the old one for the non-SMM build. Unfortunately, this "improvement" is a net negative, because then we'd have *more* stuff, on top of the *current* dump-of-obscure-numbers. (2) Keep the one table, but replace the values with nicer looking numbers (see examples above). Unfortunately, larger numbers could waste memory (stuck in BINs and hence in the UEFI memmap) for some boots, and smaller page counts would increase memmap fragmentation. We might cause some (at best, superficial) regressions. And we'd lose the property "this table made sense at some point in time" -- because the new ad-hoc numbers wouldn't even be rooted in measurements. (3) Actually measure various boots and try to derive new page counts from those. This is something I'm not prepared to do. The memory needs (considering the various memory types too), depend on a bunch of stuff: - ACPI tables generated by QEMU (influences AcpiNVS, AcpiReclaim, and even Reserved -- some opcodes in QEMU's ACPI linker/loader script require the production of S3 boot script opcodes). For example if the QEMU command line specifies the vmgenid device, then the S3 boot script stuff applies. - S3 support enabled/disabled in general on the QEMU cmdline. - OS bootloader footprint. This approach would uphold the property "has been useful at some point in time, for some workloads" -- but it's too much time to research, and it's anyway obviated by the dynamic / adaptive approach that this series enables for OVMF (in the SMM build). (4) OK, so let's not touch the numeric values, but move them out of the table? (4a) Introduce macros. Not a good idea, as these numbers are never referenced anywhere else. The following: #define MTI_RT_DATA_PAGE_COUNT 0x024 ... { EfiRuntimeServicesData, MTI_RT_DATA_PAGE_COUNT }, is not one bit more readable or expressive, but is more wasteful, than the current { EfiRuntimeServicesData, 0x024 }, (4b) Introduce PCDs. Even worse: it elevates these ad-hoc numbers to the OvmfPkg.dec file, without any plan or intent whatsoever to ever update the constants, or to reference them in any other modules, or to override them in any of the locations where PCDs can be overridden (DSC file, FDF file, and for dynamic access PCDs: C code). These numbers are basically a state-dump from a time when they had been found somewhat useful. They still work acceptably, and without an interest in (3), I wouldn't like to touch them with a ten foot pole. :) Thanks, Laszlo