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.129.124]) by mx.groups.io with SMTP id smtpd.web10.10125.1672996779639573840 for ; Fri, 06 Jan 2023 01:19:39 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=egt7sOts; spf=pass (domain: redhat.com, ip: 170.10.129.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1672996778; 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=hq1ud7Vlj/DHrtanYKLAERsdu75aPsUosqpJ+nhIzAs=; b=egt7sOtsPRw7p8erAjfBcrYJogZ0LGxu5hNJS6EV0cnt1bCGOFnerRzYcKwzRUKVSNrhvl Yjij1nBzqZBan0NkNrjq9o3e1/nUyLSOtJFJ2V4uAJP7j+M0aQPkShhBg2vhq8g9RePDw7 34PWQ9wrggg2anoE+JewMRTg2GsbFDI= 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-613-fX7dDk8tPUe3ujUiV0CeMw-1; Fri, 06 Jan 2023 04:19:33 -0500 X-MC-Unique: fX7dDk8tPUe3ujUiV0CeMw-1 Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id B198E100F902; Fri, 6 Jan 2023 09:19:32 +0000 (UTC) Received: from [10.39.192.26] (unknown [10.39.192.26]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 9877A492B06; Fri, 6 Jan 2023 09:19:25 +0000 (UTC) Message-ID: <28b1ecd3-631e-8f00-6495-acd9ec76037b@redhat.com> Date: Fri, 6 Jan 2023 10:19:24 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg:Fixed AsmRelocateApLoopStart and ensure allocated memory <4GB To: devel@edk2.groups.io, ray.ni@intel.com, Gerd Hoffmann Cc: "ardb@kernel.org" , "Xie, Yuanhao" , "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.9 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:39, Ni, Ray wrote: > > >> -----Original Message----- >> From: Laszlo Ersek >> Sent: Friday, January 6, 2023 4:31 PM >> To: Gerd Hoffmann >> Cc: devel@edk2.groups.io; Ni, Ray ; ardb@kernel.org; Xie, Yuanhao ; >> thomas.lendacky@amd.com >> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg:Fixed AsmRelocateApLoopStart and ensure allocated memory <4GB >> >> On 1/6/23 09:03, Gerd Hoffmann wrote: >>> On Fri, Jan 06, 2023 at 07:42:20AM +0100, Laszlo Ersek wrote: >>>> On 1/6/23 05:12, Ni, Ray wrote: >>>>> >>>>> Ard, >>>>> >>>>> Only AMD X64 (including SEV and without SEV) runs the code that >>>>> switches to 32bit paging disabled mode. >>>>> Intel X64 runs the code that stays at 64bit paging mode. So no need >>>>> for <4G memory. >>>>> All IA32 CPUs (including intel and AMD) stays at 32bit paging disabled >>>>> mode. The AllocateReservedPages() call should not return a memory >>>>> above 4GB in 32bit env. >>>> >>>> This argument about the allocations sounds valid, thanks. >>>> >>>> The code still remains incredibly hard to read. It needs serious >>>> cleanup. >>>> >>>> (1) Wherever we have "Amd" in an identifier, let's rename it to "Amd64", >>>> to better reflect the revised check. >>> >>> Maybe even better: Use PcdConfidentialComputingGuestAttr to figure >>> whenever SEV is active, if so branch into Amd assembler code. Rename >>> "Amd" to "AmdSev". >>> >>> Otherwise just call normal X64 / Ia32 code. >>> >>> Amd assembler code can subsequently be simplified, the checks for SEV >>> are not needed any more (but should not harm either). >>> >>> [ Adding Tom to CC ] >>> >>>> Commit 73ccde8f6d04 ("UefiCpuPkg: Has APs in 64 bit long-mode before >>>> booting to OS.", 2022-12-20) *removed* the executable marking. >>>> >>>> (4a) Is that not a problem? >>> >>> I think so. >> >> Ah... OK, my fault: one should never ask questions in English the >> negative! :) >> >> So, based on your next paragraph, I think you agree that this *is* a >> problem. (I first thought you agreed with the lack of executable marking >> *not* being a problem -- again, my mistake for formulating the question >> in the negative!) > > I agree it's a problem. Original thought was since AP is using a brand-new page table > that doesn't have the XD bit set. There is no need for removing the XD bit in > existing page table. This makes sense, but, again, even disregarding the problem that the code forgot to switch to the new page table, the idea should be spelled out in the commit message and/or in code comments. Preferably: both. (In fact if the idea had been documented, Yuanhao might not have forgotten to implement the switch.) Laszlo > But the final code change forgot to switch to the new page table before calling to > code in the reserved memory. > > > > > >