From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web12.6928.1578485904098548812 for ; Wed, 08 Jan 2020 04:18:24 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=jFF6Co36; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1578485903; 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=5DIP2qPY7+N21FRd0XZyKnG42YjsicdNqUhkDBlUd3E=; b=jFF6Co36EjznxWNvzDfPlDy0KUX8NScSOQhKgQwrMZOnrCVVllYs7MxPvt2EES0LqQsamF TxzhOxWO6McMEjzVYi3egV39nX0dby/8fcQUOzdK5N73ENov6QTX2v9ENMjiUqEvpjAAMv 0019uJbOgh3TbKONg7xZro6RYbU+FYA= 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-375-0juSehxoNj6TdiMWQeX_OA-1; Wed, 08 Jan 2020 07:18:19 -0500 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 9837C911EE; Wed, 8 Jan 2020 12:18:17 +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 91E9D5D9E2; Wed, 8 Jan 2020 12:18:15 +0000 (UTC) Subject: Re: [Patch 0/2] Shadow microcode patch according to FIT microcode table. To: "Fu, Siyuan" , "devel@edk2.groups.io" Cc: "Kinney, Michael D" , "Gao, Liming" , "Dong, Eric" , "Ni, Ray" , Tom Lendacky References: <20200108042525.408-1-siyuan.fu@intel.com> <27dad7c9-0b89-4edb-6b5b-a47820068fe4@redhat.com> From: "Laszlo Ersek" Message-ID: <9eee2e4d-3e42-9203-522f-5c0c790271a0@redhat.com> Date: Wed, 8 Jan 2020 13:18:14 +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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-MC-Unique: 0juSehxoNj6TdiMWQeX_OA-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 01/08/20 11:58, Fu, Siyuan wrote: > Hi, Laszlo >=20 >> -----Original Message----- >> From: Laszlo Ersek >> Sent: 2020=E5=B9=B41=E6=9C=888=E6=97=A5 17:43 >> To: Fu, Siyuan ; devel@edk2.groups.io >> Cc: Kinney, Michael D ; Gao, Liming >> ; Dong, Eric ; Ni, Ray >> ; Tom Lendacky >> Subject: Re: [Patch 0/2] Shadow microcode patch according to FIT microco= de >> table. >> >> (+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=3D2449#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 fo= r >> 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: >=20 > I think a Feature PCD should be enough for this since whether using > the FIT based booting flow is decided by platform owner and not expected > to be changed once decided. >=20 > And considering that the FIT table is very critical to processor init flo= w,=20 > I propose to add a DEBUG ASSERT if the Feature PCD is true, but couldn't > find a valid FIT table structure. This could reminder platform developer= =20 > that the FIT address may have been incorrectly override by someone else, > like the extensible structure as you mentioned above. >=20 > So the checking in ShadowMicrocodePatchByFit() should be as follows: > =20 > if (!FeaturePcdGet (PcdFirmwareInterfaceTableSupported)) { > return EFI_UNSUPPORTED; > } > =20 > if (FIRMWARE_INTERFACE_TABLE_ENTRY doesn't look right) { > ASSERT(FALSE); > } >=20 > What's your opinion on this? Works fine for me. Thanks! Laszlo