From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web11.8461.1584048021783772879 for ; Thu, 12 Mar 2020 14:20:21 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=ix8NuuRQ; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1584048020; 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=AUMy9ZWgGfvt0GLHs/iSu+QfQAKOkefGxBZyE5NJ1Tw=; b=ix8NuuRQFoYnLRIphmkPsb9VPXvnrDawceWtuwH7fAFC1N2NL9gGOS6gk1YxQ6wz8SjAo7 JzBEdnM0LIEZkj4KMFeb8Z5iq/VdCx3ZZpms3wSIVhhRgyPKmc7sMCeWiBbAknAQwjl88U tqG25vz6RSKwDDFSP/PI9eYeEmm/ojU= 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-15-ZGSvrgF5MISHPtmnbGzKxg-1; Thu, 12 Mar 2020 17:19:44 -0400 X-MC-Unique: ZGSvrgF5MISHPtmnbGzKxg-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 325128018A1; Thu, 12 Mar 2020 21:19:43 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-74.ams2.redhat.com [10.36.117.74]) by smtp.corp.redhat.com (Postfix) with ESMTP id BB5FB5C1C3; Thu, 12 Mar 2020 21:19:41 +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> <7e836a5b-f7ab-9595-0464-a4918524f057@redhat.com> <20200312104006.GB23627@bivouac.eciton.net> From: "Laszlo Ersek" Message-ID: <6c580a37-75aa-7897-f214-60be4cee539b@redhat.com> Date: Thu, 12 Mar 2020 22:19:40 +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: <20200312104006.GB23627@bivouac.eciton.net> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 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/12/20 11:40, Leif Lindholm wrote: > On Thu, Mar 12, 2020 at 01:30:10 +0100, Laszlo Ersek wrote: >> 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. > > There is fixing and there is improving. > > The preceding paragraph as a comment block would prevent the next > person who comes across it going "Where the $EXPLETIVE did these > numbers come from?". > > (Adding the preceding paragraph as well would have saved me another > minute of grepping, but that is more down to the fact that this is a > repeating pattern implemented differently for different platforms - > for most ARM platforms partly hidden away in EmbeddedPkg: > https://github.com/tianocore/edk2/blob/master/EmbeddedPkg/EmbeddedPkg.dec#L104) > >> (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. > > - Separately loaded drivers (including Option ROMs?), making these > numers impossible to precisely determine statically. > >> 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). > > See EmbeddedPkg. > > Basically, all of the variants you enumerate exist in the tree(s) > today. > >> 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. :) > > Sure. > > So what I'm *actually* after is. > > (5) *Somehow* standardise how platforms build up the HOB > > I think this means *hiding* BuildGuidDataHob() behind some > higher-level functions, backed by some market-segment (or > market-segment:architecture tuple) specific defaults. > > > But for this patch, if you could add the archaeology bit in a comment > block, I think that would be useful for whatever next lost soul > stumbles upon it. > > With or without that, for the series: > Acked-by: Leif Lindholm > Merged as-is, in commit range 7d325f93e190..d42fdd6f8384, via . I am going to submit a separate patch with the suggested comment. Thank you! Laszlo