Re: [edk2-devel] [PATCH 5/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation

["Leif Lindholm" <leif@nuviainc.com>]

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 <leif@nuviainc.com>