Re: [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature

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

On 09/26/19 10:46, Yao, Jiewen wrote:
> Hi Laszlo
> May I know if you want to support legacy BIOS?

No, thanks.

The end-goal is to implement VCPU hotplug at OS runtime, when the OS is
booted with OVMF, Secure Boot is enabled, and the firmware is protected
with SMM.

In other words, the goal is to support VCPU hotplug when there is a
privilege barrier between the firmware and the OS. (The whole complexity
of the feature arises because a hotplugged VCPU is a risk for that barrier.)

With legacy BIOS, there is no such barrier, and VCPU hotplug already
works with SeaBIOS.

Furthermore, if you build OVMF *without* "-D SMM_REQUIRE", then there is
no such barrier again, and VCPU hotplug already works well, in that case
too. (I had tested it extensively, in RHBZ#1454803.)

In effect, I consider "-D SMM_REQUIRE" and "-D CSM_ENABLE" mutually
exclusive, for OVMF. And the present work is only relevant for "-D
SMM_REQUIRE".

We could perhaps add an "!error" directive to the OVMF DSC files,
conditional on both flags being enabled (SMM_REQUIRE and CSM_ENABLE). I
might propose such a patch in the future, but until it becomes a
practical problem, I don't want to spend cycles on it.

Thanks
Laszlo

> The legacy BIOS memory map is reported in E820, which is different with UEFI memory map.
> 
> In OvmfPkg\Csm\LegacyBiosDxe\LegacyBootSupport.c, LegacyBiosBuildE820() will hardcode below 640K address to be OS usage without consulting UEFI memory map.
> 
>   //
>   // First entry is 0 to (640k - EBDA)
>   //
>   ACCESS_PAGE0_CODE (
>     E820Table[0].BaseAddr  = 0;
>     E820Table[0].Length    = (UINT64) ((*(UINT16 *) (UINTN)0x40E) << 4);
>     E820Table[0].Type      = EfiAcpiAddressRangeMemory;
>   );
> 
> If we want to support this feature in legacy BIOS, we also need reserve the black hole here.
> 
> Thank you
> Yao Jiewen
> 
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
>> Sent: Tuesday, September 24, 2019 7:35 PM
>> To: edk2-devel-groups-io <devel@edk2.groups.io>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Boris Ostrovsky
>> <boris.ostrovsky@oracle.com>; Brijesh Singh <brijesh.singh@amd.com>; Igor
>> Mammedov <imammedo@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>;
>> Joao M Martins <joao.m.martins@oracle.com>; Justen, Jordan L
>> <jordan.l.justen@intel.com>; Nakajima, Jun <jun.nakajima@intel.com>; Kinney,
>> Michael D <michael.d.kinney@intel.com>; Paolo Bonzini
>> <pbonzini@redhat.com>; Phillip Goerl <phillip.goerl@oracle.com>; Chen,
>> Yingwen <yingwen.chen@intel.com>
>> Subject: [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at
>> default SMBASE" feature
>>
>> Ref:    https://bugzilla.tianocore.org/show_bug.cgi?id=1512
>> Repo:   https://github.com/lersek/edk2.git
>> Branch: smram_at_default_smbase_bz_1512_wave_1
>>
>> This is the firmware-side patch series for the QEMU feature that Igor is
>> proposing in
>>
>>   [Qemu-devel] [PATCH 0/2]
>>   q35: mch: allow to lock down 128K RAM at default SMBASE address
>>
>> posted at
>>
>>   http://mid.mail-archive.com/20190917130708.10281-1-
>> imammedo@redhat.com
>>   https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03538.html
>>   https://edk2.groups.io/g/devel/message/47364
>>
>> This OVMF patch series is supposed to be the first "wave" of patch sets,
>> working towards VCPU hotplug with SMM enabled.
>>
>> We want the hot-plugged VCPU to execute its very first instruction from
>> SMRAM, running in SMM, for protection from OS and PCI DMA interference.
>> In the (long!) discussion at
>>
>>   http://mid.mail-archive.com/20190905174503.2acaa46a@redhat.com
>>   https://edk2.groups.io/g/devel/message/46910
>>   https://bugzilla.tianocore.org/show_bug.cgi?id=1512#c9
>>
>> we considered diverging from the specs and changing the default SMBASE
>> to an address different from 0x3_0000, so that it would point into
>> SMRAM. However, Igor had the idea to keep the default SMBASE intact, and
>> replace the underlying system RAM (128 KB of it) with lockable SMRAM.
>> This conforms to the language of the Intel SDM, namely, "[t]he actual
>> physical location of the SMRAM can be in system memory or in a separate
>> RAM memory". When firmware locks this QEMU-specific SMRAM, the latter
>> completely disappears from the address space of code that runs outside
>> of SMM (including PCI devices doing DMA). We call the remaining MMIO
>> area a "black hole".
>>
>> The present patch set detects the QEMU feature, locks the SMRAM at the
>> right times, and informs firmware and OS components to stay away form
>> the SMRAM at the default SMBASE.
>>
>> I've tested the set extensively:
>>
>>   http://mid.mail-archive.com/c18f1ada-3eca-d5f1-da4f-
>> e74181c5a862@redhat.com
>>   https://edk2.groups.io/g/devel/message/47864
>>
>> Going forward, if I understand correctly, the plan is to populate the
>> new SMRAM with the hotplug SMI handler. (This would likely be put in
>> place by OvmfPkg/PlatformPei, from NASM source code. The
>> SmmRelocateBases() function in UefiCpuPkg/PiSmmCpuDxeSmm backs up the
>> original contents before, and restores them after, the initial SMBASE
>> relocation of the cold-plugged VCPUs. Thus the hotplug SMI handler would
>> survive the initial relocation of the cold-plugged VCPUs.)
>>
>> Further, when a VCPU is hot-plugged, we seem to intend to raise an SMI
>> for it (perhaps internally to QEMU?), which will remain pending until
>> the VCPU is launched with INIT-SIPI-SIPI. Once the INIT-SIPI-SIPI is
>> sent, the VCPU will immediately enter SMM, and run the SMI handler in
>> the locked SMRAM at the default SMBASE. At RSM, it may continue at the
>> vector requested in the INIT-SIPI-SIPI.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Joao M Martins <joao.m.martins@oracle.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Jun Nakajima <jun.nakajima@intel.com>
>> Cc: Michael Kinney <michael.d.kinney@intel.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Phillip Goerl <phillip.goerl@oracle.com>
>> Cc: Yingwen Chen <yingwen.chen@intel.com>
>>
>> Thanks
>> Laszlo
>>
>> Laszlo Ersek (10):
>>   OvmfPkg: introduce PcdQ35SmramAtDefaultSmbase
>>   OvmfPkg/IndustryStandard: increase vertical whitespace in Q35 macro
>>     defs
>>   OvmfPkg/IndustryStandard: add MCH_DEFAULT_SMBASE* register macros
>>   OvmfPkg/PlatformPei: factor out Q35BoardVerification()
>>   OvmfPkg/PlatformPei: detect SMRAM at default SMBASE (skeleton)
>>   OvmfPkg/PlatformPei: assert there's no permanent PEI RAM at default
>>     SMBASE
>>   OvmfPkg/PlatformPei: reserve the SMRAM at the default SMBASE, if it
>>     exists
>>   OvmfPkg/SEV: don't manage the lifecycle of the SMRAM at the default
>>     SMBASE
>>   OvmfPkg/SmmAccess: close and lock SMRAM at default SMBASE
>>   OvmfPkg/PlatformPei: detect SMRAM at default SMBASE (for real)
>>
>>  OvmfPkg/Include/IndustryStandard/Q35MchIch9.h           | 106 +++++++++++----
>> -----
>>  OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c   |  21 +++-
>>  OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf |   4 +
>>  OvmfPkg/OvmfPkg.dec                                     |   6 ++
>>  OvmfPkg/OvmfPkgIa32.dsc                                 |   1 +
>>  OvmfPkg/OvmfPkgIa32X64.dsc                              |   1 +
>>  OvmfPkg/OvmfPkgX64.dsc                                  |   1 +
>>  OvmfPkg/PlatformPei/AmdSev.c                            |  24 ++++-
>>  OvmfPkg/PlatformPei/MemDetect.c                         |  87 +++++++++++++---
>>  OvmfPkg/PlatformPei/Platform.c                          |  24 +++++
>>  OvmfPkg/PlatformPei/Platform.h                          |   7 ++
>>  OvmfPkg/PlatformPei/PlatformPei.inf                     |   1 +
>>  OvmfPkg/SmmAccess/SmmAccess2Dxe.c                       |   7 ++
>>  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf                     |   1 +
>>  OvmfPkg/SmmAccess/SmmAccessPei.c                        |   6 ++
>>  OvmfPkg/SmmAccess/SmmAccessPei.inf                      |   1 +
>>  OvmfPkg/SmmAccess/SmramInternal.c                       |  25 +++++
>>  OvmfPkg/SmmAccess/SmramInternal.h                       |   8 ++
>>  18 files changed, 260 insertions(+), 71 deletions(-)
>>
>> --
>> 2.19.1.3.g30247aa5d201
>>
>>
>> -=-=-=-=-=-=
>> Groups.io Links: You receive all messages sent to this group.
>>
>> View/Reply Online (#47924): https://edk2.groups.io/g/devel/message/47924
>> Mute This Topic: https://groups.io/mt/34274934/1772286
>> Group Owner: devel+owner@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [jiewen.yao@intel.com]
>> -=-=-=-=-=-=
>