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.web11.10786.1673001329852722455 for ; Fri, 06 Jan 2023 02:35:30 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=bRJPPfB0; 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=1673001328; 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=/LG+jlV75XNkymyFaU4LjwLK5mIo17rpj02RjvIKIfk=; b=bRJPPfB03DPuENoqDkHEHf/GIK1H3tFRT6JtPqYGY1JbavkZHR7N+E9i2pso2chLfEsE0m 7VUGh0F747UF/+AxgnZiYqsCenSjEmciputKP2Ntze14Aw94WIkKbASECQEvKuW9+cARCE JssEEkdTjaQ9ZceX+AeOi2SGvNcpqSo= 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-657-b2CTEyRyPTeQPO30tgSsCA-1; Fri, 06 Jan 2023 05:35:27 -0500 X-MC-Unique: b2CTEyRyPTeQPO30tgSsCA-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 6FE6E857D0D; Fri, 6 Jan 2023 10:35:27 +0000 (UTC) Received: from [10.39.192.26] (unknown [10.39.192.26]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 2E6191121314; Fri, 6 Jan 2023 10:35:25 +0000 (UTC) Message-ID: <236acf95-68b9-16c7-f34f-4a57c73a8137@redhat.com> Date: Fri, 6 Jan 2023 11:35: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> <28b1ecd3-631e-8f00-6495-acd9ec76037b@redhat.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.3 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 10:45, Ni, Ray wrote: >> 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.) >> > > But the following cases still require the XD bit be removed from the active page table: > *. 32bit mode > In fact, old 32bit logic contains a bug that paging might still be enabled when AP is in the idle loop. > That's a long-exist bug. QEMU doesn't enable the memory protection feature resulting the paging is disabled in 32bit mode. > Hence the bug doesn't appear in QEMU 32bit image. > Yuanhao's logic tries to not create page table for 32bit mode. I agree with this because it simplifies the 32bit flow. > But considering the paging might be enabled, XD removal logic should be kept. > > *. 64bit AMD > Today Yuanhao's logic tries to not modify AMD 64bit flow no matter SEV is enabled or not. > But if the BSP page table is used by AP when running the code in the reserved memory, the XD bit removal logic should be > kept for 64bit AMD. > So, that means only 64bit Intel flow doesn't require the XD bit removal logic. > Then, for code simplicity (reducing the branches), I recommend keeping the XD bit removal logic always. That sounds OK to me. But then, can we go back to the original purpose here? What is achieved by simplifying the current MpFuncs stuff, for 64-bit Intel processors? What is achieved? Commit 73ccde8f6d04 says things like "avoiding switching modes", and "reclaiming memory by OS". I don't think I understand the importance of those. First, "reclaiming memory by OS" seems questionable, as both before and after, we need reserved memory. In fact, with the modification, we might need even more reserved memory (which the OS cannot reclaim): we still need the portion for the loop func, we still need the portion for the stacks, and we also need a new page table. So that actually seems *worse*. The edk2 codebase seen as a whole gets more complicated, that's a negative too. There are some savings in X64/MpFuncs.nasm for 64-bit Intel processors, but we need to preserve (and maintain) the preexistent code too, so it's a pure code growth, from the codebase perspective. All this against the alleged benefit of "avoiding switching modes". What is wrong with switching modes? The OS needs to boot up the APs *once*, when it starts. The mode in which the firmware parked the APs is effectively irrelevant. Is this change worth the effort and code complications at all? Now, there *is* one benefit I can imagine. For Intel maintainers, it may be difficult to maintain and to "route around" the SEV-related stuff in "X64/MpFuncs.nasm", in the long term. I can wholly accept that. So splitting and duplicating the assembly code for that purpose is justified. But then the commit message should state this, and not present "staying in 64-bit" as a benefit per se. Then the purpose is to ease the assembly code maintenance for Intel developers. Entirely justified goal in my view; nobody likes to work with complicated code they can't regression-test (and I presume Intel developers can't easily test the various SEV enablement levels in-house, on a range of AMD processors). Thanks Laszlo > Imaging in a long future that 32bit x86 support is removed from edk2 and the legacy SEV is removed as well, we can cleanup > the unnecessary XD bit removal then. > > > > > >