From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web12.5725.1578476577732184178 for ; Wed, 08 Jan 2020 01:42:58 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=gS0RWJX9; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1578476576; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=6mhXru6yl9+AcSONL1vrjsX8oWqF0b80HF8JdEmDEqs=; b=gS0RWJX9WkHkSHxWxQ0Eb9SRtLl4eKKlxjd7pgn3cbWDPMhxqm9ooWydWBeKBCXcrjoCJz Teb17Oc+9N7Lb5E0zQVgS2PKpPAXn9C30F168WYJYnSYjFe6zMzwNl5Mlbz7cSOzVY8SNc kOna2xUsYRNGGXpAQ6RgZ/fw1HtwEQA= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-414-4b0nyLkcOvmMuPkTPunErg-1; Wed, 08 Jan 2020 04:42:53 -0500 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 23FD2184B1EC; Wed, 8 Jan 2020 09:42:52 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-37.ams2.redhat.com [10.36.117.37]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5812D5C241; Wed, 8 Jan 2020 09:42:50 +0000 (UTC) Subject: Re: [Patch 0/2] Shadow microcode patch according to FIT microcode table. To: Siyuan Fu , devel@edk2.groups.io Cc: michael.d.kinney@intel.com, liming.gao@intel.com, eric.dong@intel.com, ray.ni@intel.com, Tom Lendacky References: <20200108042525.408-1-siyuan.fu@intel.com> From: "Laszlo Ersek" Message-ID: <27dad7c9-0b89-4edb-6b5b-a47820068fe4@redhat.com> Date: Wed, 8 Jan 2020 10:42:49 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200108042525.408-1-siyuan.fu@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-MC-Unique: 4b0nyLkcOvmMuPkTPunErg-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit (+Tom) On 01/08/20 05:25, Siyuan Fu wrote: > The existing MpInitLib will shadow the microcode update patches from > flash to memory and this is done by searching microcode region > specified by PCD PcdCpuMicrocodePatchAddress and > PcdCpuMicrocodePatchRegionSize. > This brings a limition to platform FW that all the microcode patches > must be placed in one continuous flash space. > > This patch set shadows microcode update according to FIT microcode > entries if it's present, otherwise it will fallback to original logic > (by PCD). > > Patch 1/2: Add FIT header file to MdePkg. > Patch 2/2: Update microcode loader to shadow microcode according to FIT. > > Cc: Michael D Kinney > Cc: Liming Gao > Cc: Eric Dong > Cc: Ray Ni > Cc: Laszlo Ersek > > Siyuan Fu (2): > MdePkg: Add header file for Firmware Interface Table specification. > UefiCpuPkg: Shadow microcode patch according to FIT microcode entry. > > .../IndustryStandard/FirmwareInterfaceTable.h | 76 ++++++ > UefiCpuPkg/Library/MpInitLib/Microcode.c | 255 +++++++++++++----- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 4 +- > UefiCpuPkg/Library/MpInitLib/MpLib.h | 7 +- > 4 files changed, 276 insertions(+), 66 deletions(-) > create mode 100644 MdePkg/Include/IndustryStandard/FirmwareInterfaceTable.h > I have now checked https://www.intel.com/content/dam/www/public/us/en/documents/guides/fit-bios-specification.pdf that is referenced in https://tianocore.acgmultimedia.com/show_bug.cgi?id=2449#c0 I think it is wrong for MpInitLib to deduce anything from the flash contents at fixed physical address 0xFFFFFFC0 *unless* the platform advertizes up-front adherence to the FIT specification. The proposed logic in patch#2 goes like this: (1) read the UINT64 at physical address 0xFFFFFFC0 (2) If the value read is zero, all-nibles-F, or all-nibbles-E, then assume "no FIT" (3) otherwise, dereference the value read, as a pointer-to-FIRMWARE_INTERFACE_TABLE_ENTRY (4) if the assumed FIRMWARE_INTERFACE_TABLE_ENTRY doesn't look right, assume "no FIT" (5) if "no FIT", then fall back to previous PCDs (which describe if there is a microcode update in the flash, and if so, where) This approach is wrong. If a platform has never heard of the FIT specification, it may have any value at all at address 0xFFFFFFC0. The value there may easily differ from the three values that step (2) equates to "no FIT". Therefore the whole procedure above needs to be gated with a new FeaturePCD (default value FALSE). This is not a theoretical risk. Please see the following patches: * [edk2-devel] [RFC PATCH v3 36/43] UefiCpuPkg: Allow AP booting under SEV-ES https://edk2.groups.io/g/devel/message/50976 http://mid.mail-archive.com/ddc10e014822c9a87a7dc9a44ee854751ffcf442.1574280425.git.thomas.lendacky@amd.com * [edk2-devel] [RFC PATCH v3 37/43] OvmfPkg: Reserve a page in memory for the SEV-ES AP reset vector https://edk2.groups.io/g/devel/message/50977 http://mid.mail-archive.com/706f17aa0ea3ed3f57c1e933e93164a94ba1a0cf.1574280425.git.thomas.lendacky@amd.com Those patches describe a structure whose *last* field, and EFI_GUID, is placed ath 0xffffffd0. This structure is extensible, and new fields are supposed to be *prepended* to it. The currently defined first field starts at 0xffffffca. Whereas the (exclusive) end of the FIT pointer is at 0xffffffc8. This means there is a 2 (two) bytes gap between them. If more than two bytes are prepended to the extensible structure in the future, that may easily lead step (2) above to incorrectly determine "yes FIT". So please introduce a new FeaturePCD in MdePkg (or even a dynamic boolean PCD, if you think that's more flexible), and add logic similar to: > VOID > ShadowMicrocodeUpdatePatch ( > IN OUT CPU_MP_DATA *CpuMpData > ) > { > EFI_STATUS Status; > > if (FeaturePcdGet (PcdFirmwareInterfaceTableSupported)) { > Status = ShadowMicrocodePatchByFit (CpuMpData); > } else { > Status = EFI_UNSUPPORTED; > } > > if (EFI_ERROR (Status)) { > ShadowMicrocodePatchByPcd (CpuMpData); > } > } Alternatively, leave ShadowMicrocodeUpdatePatch() as it is currently proposed, but start ShadowMicrocodePatchByFit() as follows: > if (!FeaturePcdGet (PcdFirmwareInterfaceTableSupported)) { > return EFI_UNSUPPORTED; > } In closing: it seems short-sighted that the FIT specification placed a "naked" pointer at a fixed offset in flash, rather than a three-field structure consisting of: - a GUID, - preceded by a structure size, - preceded by the FIT pointer. Because, using a GUID-ed approach, the chance to *incorrectly* deduce "yes FIT" would be 1 in (2^128) -- all 128-bit values except one magic value would indicate "no FIT". That's good. Whereas, with the spec's current "naked pointer" approach, the chance to *correctly* deduce "yes FIT" is 3 in (2^64) -- all 64-bit values except three magic values indicate "yes FIT". Not good. Thanks Laszlo