(+David) On 09/27/19 03:14, Yao, Jiewen wrote: > Sounds good. > > Maybe you can write that info in the commit message. A simple sentence such > as : > No CSM support is needed because legacy BIOS boot don't use SMM. > > So other people won't have same question in the future.
I've given this more thought. I can't decide what's the best approach, without David's input. David, here's the problem statement, stripped down: with a new QEMU feature that's dynamically detected and enabled by the firmware, the 128KB memory area at 0x3_0000 becomes inaccessible to the OS. (This subfeature is necessary for enabling the larger "VCPU hotplug with SMM" feature). Right now, this patch series communcates that fact to the UEFI OS through the UEFI memory map, but it's not told to a legacy OS through the E820 memmap. Which option do you prefer? (1) The feature depends on building OVMF with -D SMM_REQUIRE. We can say that -D SMM_REQUIRE and -D CSM_ENABLE are mutually exclusive, and add an !error directive to the DSC files to catch a build that is passed both -D flags. This option is the most convenient for me, but it's likely not flexible enough for users that have no access to hypervisor configuration (such as QEMU cmdline options and/or firmware binary choice). They might want to pick UEFI boot vs. CSM boot "unpredictably". (2) Jiewen suggested a patch for LegacyBiosBuildE820() [OvmfPkg/Csm/LegacyBiosDxe/LegacyBootSupport.c], punching a hole into E820Table[0]. While this would make the CSM honest about the memory map, to legacy OSes, I strongly doubt legacy OSes would actually *work* with such a memory map (i.e. with 128 KB missing in the middle of the <=640KB RAM). It would require a bunch of testing anyway, and it could break with any newly tried legacy OS. (3) Modify OvmfPkg/SmmAccess/SmmAccess2Dxe to set up an event notification function for the "gEfiEventLegacyBootGuid" event group, in case the feature has been detected in PlatformPei. In the notification function, log an error message, call ASSERT (FALSE), and call CpuDeadLoop (). Advantages: - when OVMF is built without -D SMM_REQUIRE, SmmAccess2Dxe is not included - when OVMF is built without -D CSM_REQUIRE, then the event group is never signaled -- the only EfiSignalEventLegacyBoot() call is in "OvmfPkg/Csm/LegacyBiosDxe/LegacyBootSupport.c". - when OVMF is built with both options, but no legacy boot is performed, the callback does nothing - the callback catches the invalid constellation and prevents the boot cleanly. The error message in the OVMF debug log would instruct users to disable the dynamically detectable feature on the QEMU command line, and to boot again. Disadvantages: - users have to look at the OVMF log - users may not have the power to change the QEMU configuration -- if they had that power, they'd simply use SeaBIOS for booting the legacy OS, not OVMF+CSM. (4) Identify a CSM build in time for the feature detection (in PlatformPei), and force-disable the feature (don't even attempt detecting it from QEMU) if this is a CSM build. In the DXE phase (and especially in the BDS phase), we can detect a CSM build, from the presence of gEfiLegacyBiosProtocolGuid in the UEFI protocol database. However, the protocol is not available in the PEI phase, which is when this particular QEMU feature has to be enabled. Therefore we could introduce a dedicated FeaturePCD, which would reflect -D CSM_ENABLE. When set, the PCD would short-circuit the dynamic detection in PlatformPei, and pretend that the feature doesn't exist. This option would allow -D SMM_REQUIRE and -D CSM_ENABLE to co-exist (like they do now); the latter would only disable the new feature (regardless of QEMU offering it), not all of SMM. With -D CSM_ENABLE, users would not lose the protection they get from SMM_REQUIRE when booting a UEFI OS; they'd only lose VCPU hotplug capability. My preference is (1), but I never use -D CSM_ENABLE. David, do you see (or do you foresee) OVMF binaries that are built with *both* of -D CSM_ENABLE and -D SMM_REQUIRE? > With the commit message change, the series reviewed-by: jiewen....@intel.com Thank you Jiewen -- it's possible that I'll post a v2, based on David's answer, and then I'll pick up your R-b for unchanged patches from v1. Thanks Laszlo > >> -----Original Message----- >> From: Laszlo Ersek <ler...@redhat.com> >> Sent: Thursday, September 26, 2019 10:52 PM >> To: Yao, Jiewen <jiewen....@intel.com>; devel@edk2.groups.io >> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>; Boris Ostrovsky >> <boris.ostrov...@oracle.com>; Brijesh Singh <brijesh.si...@amd.com>; Igor >> Mammedov <imamm...@redhat.com>; Joao M Martins >> <joao.m.mart...@oracle.com>; Justen, Jordan L <jordan.l.jus...@intel.com>; >> Nakajima, Jun <jun.nakaj...@intel.com>; Kinney, Michael D >> <michael.d.kin...@intel.com>; Paolo Bonzini <pbonz...@redhat.com>; Phillip >> Goerl <phillip.go...@oracle.com>; Chen, Yingwen <yingwen.c...@intel.com> >> Subject: Re: [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at >> default SMBASE" feature >> >> 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.biesheu...@linaro.org>; Boris Ostrovsky >>>> <boris.ostrov...@oracle.com>; Brijesh Singh <brijesh.si...@amd.com>; Igor >>>> Mammedov <imamm...@redhat.com>; Yao, Jiewen >> <jiewen....@intel.com>; >>>> Joao M Martins <joao.m.mart...@oracle.com>; Justen, Jordan L >>>> <jordan.l.jus...@intel.com>; Nakajima, Jun <jun.nakaj...@intel.com>; >> Kinney, >>>> Michael D <michael.d.kin...@intel.com>; Paolo Bonzini >>>> <pbonz...@redhat.com>; Phillip Goerl <phillip.go...@oracle.com>; Chen, >>>> Yingwen <yingwen.c...@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- >>>> imamm...@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 >>>> >>>> 20190905174503.2acaa46a@redhat.com">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- >>>> e74181c5a...@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.biesheu...@linaro.org> >>>> Cc: Boris Ostrovsky <boris.ostrov...@oracle.com> >>>> Cc: Brijesh Singh <brijesh.si...@amd.com> >>>> Cc: Igor Mammedov <imamm...@redhat.com> >>>> Cc: Jiewen Yao <jiewen....@intel.com> >>>> Cc: Joao M Martins <joao.m.mart...@oracle.com> >>>> Cc: Jordan Justen <jordan.l.jus...@intel.com> >>>> Cc: Jun Nakajima <jun.nakaj...@intel.com> >>>> Cc: Michael Kinney <michael.d.kin...@intel.com> >>>> Cc: Paolo Bonzini <pbonz...@redhat.com> >>>> Cc: Phillip Goerl <phillip.go...@oracle.com> >>>> Cc: Yingwen Chen <yingwen.c...@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+ow...@edk2.groups.io >>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [jiewen....@intel.com] >>>> -=-=-=-=-=-= >>> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48348): https://edk2.groups.io/g/devel/message/48348 Mute This Topic: https://groups.io/mt/34274934/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
