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 4A45CAC0AEE for ; Tue, 16 Jan 2024 07:30:00 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=x3iORQz14I6nN+RlWM5aFa8qPajarLtO9XE/QhISgWA=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Transfer-Encoding; s=20140610; t=1705390199; v=1; b=MiTeM4SEW+Bp22L/RMO93zDtOWDWLk7Yn356Kl3EmjI+YQDt3kDj+EBhXAmj14+m/wXHyLgK BmhbCGvGTDHQlEi8E3DjqBiQb96e4c8Bg0gn2a19WFKK0L70frpKvMBT7afTKQ9MO9RQjLBW3sV +e0GHuZrttKbIQs2x8/CUcMo= X-Received: by 127.0.0.2 with SMTP id 6ME0YY7687511xyvg7Y9tuWG; Mon, 15 Jan 2024 23:29:59 -0800 X-Received: from mail-ua1-f43.google.com (mail-ua1-f43.google.com [209.85.222.43]) by mx.groups.io with SMTP id smtpd.web10.74859.1705311798493797912 for ; Mon, 15 Jan 2024 01:43:18 -0800 X-Received: by mail-ua1-f43.google.com with SMTP id a1e0cc1a2514c-7ce603b9051so2410371241.2 for ; Mon, 15 Jan 2024 01:43:18 -0800 (PST) X-Gm-Message-State: jVpdEBtEBcoocpXHhrJtz3xFx7686176AA= X-Google-Smtp-Source: AGHT+IGrPYXqHqSVfNsGpYwlCfaoH9S96XVp4h+ogjdrtOt4f6eMi7H591PCTPo4/zoDh0lhHYWFGrTG2fv2qFKNGOE= X-Received: by 2002:a05:6122:4385:b0:4b6:d5f1:7c8a with SMTP id cq5-20020a056122438500b004b6d5f17c8amr2435030vkb.4.1705311797144; Mon, 15 Jan 2024 01:43:17 -0800 (PST) MIME-Version: 1.0 References: <20240110053828.1473-1-zhiguang.liu@intel.com> <95163437-75ea-9265-a5e1-35cf01f186eb@redhat.com> In-Reply-To: From: "Pedro Falcato" Date: Mon, 15 Jan 2024 09:43:06 +0000 Message-ID: Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix issue that IsModified is wrongly set in PageTableMap To: devel@edk2.groups.io, lersek@redhat.com Cc: ray.ni@intel.com, "Liu, Zhiguang" , "Kumar, Rahul R" , Gerd Hoffmann , "Lee, Crystal" 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,pedro.falcato@gmail.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=MiTeM4SE; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=gmail.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 Thu, Jan 11, 2024 at 8:56=E2=80=AFAM Laszlo Ersek wr= ote: > > 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 o= f > >> 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 sa= id > >> 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 "Acces= s" 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. 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; 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). I'd love to give out more feedback on this patch, but I *really* don't understand what any of that function is doing :/ --=20 Pedro -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113813): https://edk2.groups.io/g/devel/message/113813 Mute This Topic: https://groups.io/mt/103636407/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-