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

["Laszlo Ersek" <lersek@redhat.com>]

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