From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by mx.groups.io with SMTP id smtpd.web10.6880.1584009610663230276 for ; Thu, 12 Mar 2020 03:40:11 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=JMjVGFGg; spf=pass (domain: nuviainc.com, ip: 209.85.221.68, mailfrom: leif@nuviainc.com) Received: by mail-wr1-f68.google.com with SMTP id z15so6765533wrl.1 for ; Thu, 12 Mar 2020 03:40:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=6vnV7wSvP/Evj+gfaH/4/ij9eG9B594rtw+jK2c7ew8=; b=JMjVGFGg5Av/l6r4Oz1bG8HldFCJyC2KIc5DBeYhfnZ9NRrow7NyatbzWT4sAQ6WMB /zSR71Ww5IV2OStspw2dPHKij4+UQVbYsouob1F2oHosXB+zJXay4/ZU5RFiVkWLAWQH fhzZ7QGiR4iT6z/ZJT6XDsMwOnWszZ+RJ5jQGVagXoYZj5+g47lXGYpWyqYWsLDyAxdV dzoEuNBCg0769ohYgfN7Od5U7yNul7LhNVelCu0Y6egWO7XFmj5ro9BbngVT8so7roQa 1bIpIZL4Ggo/nb3+D6RkbyaCcc6JDzpjIEJSfz3M+1VAK8jbFcRhgiDW9Ll+bgYWSFD7 H/qg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=6vnV7wSvP/Evj+gfaH/4/ij9eG9B594rtw+jK2c7ew8=; b=TuIsxJy2n5eWA2PKPKdRBsg6j/I5Sna7U+LOJBVCAsUrEemh9OB/oe1t6epkB8tyjt 2JPanjCKkEJt5ZQYoTz8fTW/+SzL2bX5KXr9oatRCn5XdmMS6awrO6V4ciySE8zGCukx ttHBaSrUYVCdem8LkepepOvSGK5pNDBATJZgObHZxfbrFLkYVpCYeto4ctRSe0ea5MqR MBybyvI3LpPmNby40wOx+XNOrsxr0UQxkQ7YaE2bb3ECfT34V4EZXFusIZ6GDKJMbJXg xIvG8W0sqizJJjGfkzFZqJdZAkA2BAqVWxa+AGUTHo5GK1ZQLDrIL6e2Z+5rL1BfIBdI 8j3g== X-Gm-Message-State: ANhLgQ2xa+LukSxRfdAqHzwPEFY5AmYRJwWBTMzsDgoFjp0N4xBNgBn9 SQRBs4vgH0VWTSkUSNEznumBmQ== X-Google-Smtp-Source: ADFU+vvGCyy8OLHrRAEhGbl5MgHBWPQuYFdR4FdciimiM4vhHrcWUNfZaql0qtWkBrs++s6yDZZ5tA== X-Received: by 2002:adf:f2cf:: with SMTP id d15mr10162496wrp.397.1584009609040; Thu, 12 Mar 2020 03:40:09 -0700 (PDT) Return-Path: Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id q72sm12485499wme.31.2020.03.12.03.40.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 12 Mar 2020 03:40:08 -0700 (PDT) Date: Thu, 12 Mar 2020 10:40:06 +0000 From: "Leif Lindholm" To: Laszlo Ersek Cc: devel@edk2.groups.io, Ard Biesheuvel , Jordan Justen , Philippe =?iso-8859-1?Q?Mathieu-Daud=E9?= Subject: Re: [edk2-devel] [PATCH 5/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation Message-ID: <20200312104006.GB23627@bivouac.eciton.net> 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> MIME-Version: 1.0 In-Reply-To: <7e836a5b-f7ab-9595-0464-a4918524f057@redhat.com> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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