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.3008.1588794822691416098 for ; Wed, 06 May 2020 12:53:42 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=M50gbr6u; 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=1588794821; 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=IxHT716HzCp1yUlKCnKXMwcrp9amIvO8lpMgPjUTxj4=; b=M50gbr6uRSN07n/kxR9uKCqb1M3inwh6PEzSJSfocVatQkaD1C8fitFX8eLDbOswtyxlbx kjncE60+PvS1yTmMUYmQhAZUpuZbTiq1Pof24y/EHy4uYNpW+nG0wJ6Q5LAuKYdaZAy1vZ hXNI4YwH0rlweJEUQD6xvYRPDg/+8kg= 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-288-iBLm7hiwPsqsM593FY738w-1; Wed, 06 May 2020 15:53:38 -0400 X-MC-Unique: iBLm7hiwPsqsM593FY738w-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 46569107ACF4; Wed, 6 May 2020 19:53:36 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-227.ams2.redhat.com [10.36.112.227]) by smtp.corp.redhat.com (Postfix) with ESMTP id 654C960CCC; Wed, 6 May 2020 19:53:33 +0000 (UTC) Subject: Re: [PATCH v2 0/3] XCODE5 toolchain binary patching fix To: Tom Lendacky , devel@edk2.groups.io Cc: Jordan Justen , Ard Biesheuvel , Liming Gao , Eric Dong , Ray Ni , Anthony Perard , Julien Grall , Brijesh Singh , Andrew Fish References: From: "Laszlo Ersek" Message-ID: <9ad8bce8-e90a-a429-bb42-bd53a690ebcc@redhat.com> Date: Wed, 6 May 2020 21:53:32 +0200 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.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 05/06/20 18:32, Tom Lendacky wrote: > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2340 > > Commit 2db0ccc2d7fe ("UefiCpuPkg: Update CpuExceptionHandlerLib pass > XCODE5 tool chain") introduced binary patching in the > ExceptionHandlerAsm.nasm in order to support the XCODE5 toolchain. > However, the CpuExceptionHandlerLib can be used during SEC phase which > would result in binary patching of flash. > > This series creates a new CpuExceptionHandlerLib file to support the > required binary patching for the XCODE5 toolchain, while reverting the > changes from commit 2db0ccc2d7fe in the standard file. As the Pei, Dxe > and SMM versions of the library operate in memory (as opposed to > flash), only the SEC/PEI version is of the library is updated to use > the version of the ExceptionHandlerAsm.nasm that does not perform > binary patching. > > This is accomplished in phases: > - Create a new XCODE5 specific version of the > ExceptionHandlerAsm.nasm file and update all CpuExceptionHandler > INF files to use it while also creating a new SEC/PEI > CpuExceptionHandler INF file specifically for the XCODE5 > toolchain. > - Update all package DSC files that use the > SecPeiCpuExceptionHandlerLib version of the library to use the > XCODE5 version of the library, Xcode5SecPeiCpuExceptionHandlerLib, > when the XCODE5 toolchain is used. > - Revert the changes made by commit 2db0ccc2d7fe in the standard > file and update the SecPeiCpuExceptionHandlerLib.inf file to use > the standard file. > > I don't have access to an XCODE5 toolchain setup, so I have not tested > this with XCODE5. I would like to request that someone who does please > test this. > > Also, will this change have an impact on any of the platform builds > outside of this tree? This series takes care of all the "SecPeiCpuExceptionHandlerLib.inf" occurrences in edk2. Regarding edk2-platforms, with master being at 644e223bb371 ("Platform/RaspberryPi: create DXE phase SerialPortLib version for RPi3", 2020-05-06): $ git grep -l \ 'UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf' Platform/Intel/MinPlatformPkg/Include/Dsc/CorePeiLib.dsc Platform/Intel/QuarkPlatformPkg/Quark.dsc Platform/Intel/QuarkPlatformPkg/QuarkMin.dsc Platform/Intel/Vlv2TbltDevicePkg/PlatformPkgIA32.dsc Platform/Intel/Vlv2TbltDevicePkg/PlatformPkgX64.dsc I don't know what's best for these platforms: an XCODE5-compatible source code that does the wrong thing when run from flash, or an XCODE5-incompatible source code that does the right thing. If all of these platforms lived in edk2 proper, I would suggest inserting patches for them just before patch#3 in this series -- similarly to your OvmfPkg patch. But, these platforms live outside of edk2, and I don't know if they are ever built with XCODE5... ... You could propose a separate edk2-platforms series: Platform/Intel/MinPlatformPkg/Include/Dsc/CorePeiLib.dsc Chasel Chiu Nate DeSimone Liming Gao Platform/Intel/QuarkPlatformPkg/Quark.dsc Michael D Kinney Kelly Steele Platform/Intel/QuarkPlatformPkg/QuarkMin.dsc Michael D Kinney Kelly Steele Platform/Intel/Vlv2TbltDevicePkg/PlatformPkgIA32.dsc Zailiang Sun Yi Qian Platform/Intel/Vlv2TbltDevicePkg/PlatformPkgX64.dsc Zailiang Sun Yi Qian and we could then delay just the pushing of patch#3 in this series until the edk2-platforms patches have been merged too. > In other words, should the new INF be the one that uses the reverted > ExceptionHandlerAsm.nasm file and it be called something like > BaseSecPeiCpuExceptionHandlerLib.inf? Keeping the current (= self-patching) logic associated with the current filename ("SecPeiCpuExceptionHandlerLib.inf") would certainly eliminate the headache about out-of-tree platforms. But it would also mean we'd have to use a special prefix for the *non-broken* SEC INF file. And that irks me quite a bit. The quirk is in the patching variant, and IMO that should be reflected by the file name. Thanks Laszlo