From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web10.9897.1672995900857352616 for ; Fri, 06 Jan 2023 01:05:01 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=K4EouVqj; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1672995899; 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=6pTXFcyLTMUspu9Uy3hBpnSsZhDNWNc12uCPZ0hWkZc=; b=K4EouVqjPNW0FY9Z8tlq9rlRvimw852GEB5tjngVGCkgDbfuL9S9l4bJ9lxymM4cc0laaF uvZVLGDs48afM0LDyJVK1ZouRIvqt2KUxoZwzDQUTr3aQx7coKACK9a/d435oMYbf5lDAw 01mKRizY3+xUXVq2bDL00Z1O9QjJOo0= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-646-ojxgwMLGMWC86s0GH45WVw-1; Fri, 06 Jan 2023 04:04:56 -0500 X-MC-Unique: ojxgwMLGMWC86s0GH45WVw-1 Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 3AD40811E6E; Fri, 6 Jan 2023 09:04:56 +0000 (UTC) Received: from [10.39.192.26] (unknown [10.39.192.26]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 2103A492B00; Fri, 6 Jan 2023 09:04:54 +0000 (UTC) Message-ID: <19f11bca-c6ea-df05-1ddd-e76b0b4d31ac@redhat.com> Date: Fri, 6 Jan 2023 10:04:53 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg:Fixed AsmRelocateApLoopStart and ensure allocated memory <4GB To: "Xie, Yuanhao" , "devel@edk2.groups.io" , "kraxel@redhat.com" Cc: "Ni, Ray" , "ardb@kernel.org" , "thomas.lendacky@amd.com" References: <20230105062108.1796-1-yuanhao.xie@intel.com> <20230106080300.tsohpx24ddxjo5x4@sirius.home.kraxel.org> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.10 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 1/6/23 09:43, Xie, Yuanhao wrote: > Hi all, > > Thanks for feedbacks. I will revert the original ones, and send the > new version. OK, thanks. Please pay attention the ordering of the reverts. The original series was merged in the following order: (a) 1 7bda8c648192 UefiCpuPkg: Duplicated AsmRelocateApLoop as AsmRelocateApLoopAmd 2 73ccde8f6d04 UefiCpuPkg: Has APs in 64 bit long-mode before booting to OS. 3 4a8642422460 OvmfPkg: Add CpuPageTableLib required by MpInitLib. 4 3f378450dfaf UefiPayloadPkg: Add CpuPageTableLib required by MpInitLib. Commit #2 introduced a new lib class dependency. The dependency was resolved for OvmfPkg and UefiPayloadPkg only in patches #3 and #4, respectively. This means that, if someone checks out the tree at #2 or #3, then at #2, neither the OvmfPkg nor UefiPayloadPkg platforms build, and at #3, the UefiPayloadPkg platforms don't build: > $ git checkout 73ccde8f6d04 > > $ build -a X64 -b NOOPT -p OvmfPkg/OvmfPkgX64.dsc -t GCC5 > > [...] > > .../OvmfPkg/OvmfPkgX64.dsc(...): error 4000: Instance of library class [CpuPageTableLib] is not found > in [.../UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf] [X64] > consumed by module [.../UefiCpuPkg/CpuDxe/CpuDxe.inf] This is a problem. It's a problem because a broken build interferes with the "git bisect" command's ability to narrow down a functional (runtime) issue to a specific patch. If you can't build the tree at a particular commit, then you cannot test whether that commit already contains the regression, or not. Usually when a series is reverted, the revert commits are ordered in reverse to the original commits. However, in this case, we shouldn't do that, because then we'd introduce two more commits into the git history at which the tree doesn't build. The proper original order (for keeping the tree buildable at all times) would have been the following (moving (a)/#2 to the end, so that by the time the CpuPageTableLib dependency is introduced to CpuDxe, all CpuDxe-dependent DSC files in the tree have a CpuPageTableLib resolution): (b) 1 7bda8c648192 UefiCpuPkg: Duplicated AsmRelocateApLoop as AsmRelocateApLoopAmd 2 4a8642422460 OvmfPkg: Add CpuPageTableLib required by MpInitLib. 3 3f378450dfaf UefiPayloadPkg: Add CpuPageTableLib required by MpInitLib. 4 73ccde8f6d04 UefiCpuPkg: Has APs in 64 bit long-mode before booting to OS. Therefore the right order to revert is the inverse of (b), and not the inverse of (a): git revert 73ccde8f6d04 # UefiCpuPkg: Has APs in 64 bit long-mode before booting to OS. git revert 3f378450dfaf # UefiPayloadPkg: Add CpuPageTableLib required by MpInitLib. git revert 4a8642422460 # OvmfPkg: Add CpuPageTableLib required by MpInitLib. git revert 7bda8c648192 # UefiCpuPkg: Duplicated AsmRelocateApLoop as AsmRelocateApLoopAmd This keeps the tree buildable at every stage of the revert. The importance of this cannot be over-emphasized. If someone investigates a completely unrelated problem in edk2 in a year from now, and they don't even know what package the issue could be in, so they can't restrict "git-bisect" to a particular package, then their git-bisect run could very well land on one of the non-building commits, at some point. The present UefiCpuPkg commits may be totally irrelevant for their problem, but they won't be able to tell or test that, because the tree will simply not build for them. "git-bisect" supports a command called "git bisect skip", which more or less deals with such situations, by picking a "nearby" commit to try, but that's really just a kludge. Laszlo