From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 7D9967803CF for ; Tue, 16 Jan 2024 05:07:14 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=cmTe45Lw0VcuBO5n/iFTUfYgwfnV+Qyj5I4euXwcUXU=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1705381633; v=1; b=Edzp2vARRP7ftKNJ5agOqbjaUdciNJ4unzamZK81kuREhpXlUr2oAf2Z3YaWX2zCATloJPiE mPyxVq+zUa8QyEL+Q9nElEpHkRxiAmNLcBtr4jmLXMNfVQ3YYihg4HE4hWNNGlL3/mCNzQqztzn O9JKM7Klzsz4pA2mCzymwjt0= X-Received: by 127.0.0.2 with SMTP id LLpKYY7687511xnXv8dWBQ8m; Mon, 15 Jan 2024 21:07:13 -0800 X-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.89878.1705341632766661802 for ; Mon, 15 Jan 2024 10:00:32 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-532-gICHVlCOPcmUFrqB0oMG7Q-1; Mon, 15 Jan 2024 13:00:28 -0500 X-MC-Unique: gICHVlCOPcmUFrqB0oMG7Q-1 X-Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 1AAF3868A04; Mon, 15 Jan 2024 18:00:28 +0000 (UTC) X-Received: from [10.39.193.170] (unknown [10.39.193.170]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 1566DC15E6C; Mon, 15 Jan 2024 18:00:26 +0000 (UTC) Message-ID: Date: Mon, 15 Jan 2024 19:00:26 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix issue that IsModified is wrongly set in PageTableMap To: Pedro Falcato , devel@edk2.groups.io Cc: ray.ni@intel.com, "Liu, Zhiguang" , "Kumar, Rahul R" , Gerd Hoffmann , "Lee, Crystal" References: <20240110053828.1473-1-zhiguang.liu@intel.com> <95163437-75ea-9265-a5e1-35cf01f186eb@redhat.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.8 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: Wul7axuyiP4BdxRCym6P3o4hx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=Edzp2vAR; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 1/15/24 10:43, Pedro Falcato wrote: > On Thu, Jan 11, 2024 at 8:56 AM Laszlo Ersek wrote: >> >> On 1/11/24 03:03, Ni, Ray wrote: >>>> This function is incredibly complicated, so reviewing this patch is >>>> hard, even after reading the bugzilla ticket. >>>> >>>> The commit message is useless. It should contain a brief description of >>>> the problem, and how the fix resolves the problem. >>>> >>>> The documentation of the PageTableLibMapInLevel() function is wrong, >>>> even before this patch. It documents the "IsModified" output-only >>>> parameter as follows: >>>> >>>> "TRUE means page table is modified. FALSE means page table is not >>>> modified." >>>> >>>> This states that "IsModified" is always set on output, to either FALSE >>>> or TRUE. Which is an incorrect statement; in reality the caller is >>>> expected to pre-set (*IsModified) to FALSE, and PageTableLibMapInLevel() >>>> will (conditionally!) perform a FALSE->TRUE transition only. >>>> >>>> Now, this patch may fix a bug, but it makes the above-described >>>> documentation issue worse, by further restricting the condition for said >>>> FALSE->TRUE transition. >>> >>> Laszlo, thanks for the comments! >>> Though the fixing looks simple, Zhiguang and I did have several rounds of offline discussions >>> regarding how to fix it. >>> >>> When the lib accesses the page table content, CPU would set the "Access" bit in the page entry >>> that points to the page table memory being accessed by the lib. >>> >>> So, even when the "Modify" is FALSE (indicating caller doesn't want the lib to modify the page table), >>> lib code should not modify the page table but CPU still sets the "Access" bit in some of the entries due to >>> the reasons above. >> >> Huh, tricky! >> >> Should the comparison explicitly mask out the Accessed bit from each of >> the "before" page table entry and the "after" one, perhaps? > > FWIW, clearing the A and D bits off of PTEs requires a TLB flush and, > as such, that change would break them. I didn't mean to clear the A/D bits inside the actual live PTEs, only in those temporary / helper variables (or even just expressions) that we use for comparing the before/after states. > > In general: > - You need a TLB flush when unmapping a page > - You need a TLB flush when changing an already-mapped PTE (unless > you tolerate a stale TLB and want to eat a spurious page fault, which > is a valid technique) > - You don't need a TLB flush when freshly mapping a page (unmapped -> > mapped) as x86 doesn't cache non-present PTEs > > so you shouldn't need to inspect the PTE before and after; this seems to invite further discussion wrt. what the function is supposed to do at all... > in fact, > that's erroneous as Intel CPUs can speculatively set the A and D bits > (they're slightly more careful since CET rolled around, but as far as > I've heard older Intel used to wildly set those bits speculatively) > and AMD ones can too (although they cannot speculatively set D). What is the error behavior here? Assuming we consider a speculative A/D setting by the hardware, the worst that can happen is that we spuriously flush the TLB. Is that right? Doesn't seem extremely harmful. > > I'd love to give out more feedback on this patch, but I *really* don't > understand what any of that function is doing :/ > Yup. (In general I'm just acknowledging that right now this is quite out of my league...) Thanks, Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113839): https://edk2.groups.io/g/devel/message/113839 Mute This Topic: https://groups.io/mt/103636407/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-