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 54B3F740038 for ; Sun, 25 Feb 2024 13:47:27 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=/PxPng4d4Gq14IE03fubHz2O27yoFPR0m3gOF6XwJ04=; 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=1708868845; v=1; b=dzQUumv3vjpcvNMeXYQ4wPhANGBKNpahDEGSw+BhuzOCx4lMJitNuWe1Fi52rc90nX3cbCoQ kuF1N5nceJobgmRgHMcU09d70hpiDdqIi5OnNvw+cA50S3YpADONeVyduvYWeXwTgwYw/u3UJ7f of2E0CXP/nSAG7eZRS54UkD4= X-Received: by 127.0.0.2 with SMTP id is5UYY7687511xvnqsyCKear; Sun, 25 Feb 2024 05:47:25 -0800 X-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.39005.1708868844941039613 for ; Sun, 25 Feb 2024 05:47:25 -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-333-VYM22jDcOe2zI0-HcdhA7w-1; Sun, 25 Feb 2024 08:47:18 -0500 X-MC-Unique: VYM22jDcOe2zI0-HcdhA7w-1 X-Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (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 2C266811E81; Sun, 25 Feb 2024 13:47:18 +0000 (UTC) X-Received: from [10.39.192.57] (unknown [10.39.192.57]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 4F76E1121313; Sun, 25 Feb 2024 13:47:16 +0000 (UTC) Message-ID: Date: Sun, 25 Feb 2024 14:47:15 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile To: Michael Brown , "Zhou, Jianfeng" , "devel@edk2.groups.io" Cc: "Ni, Ray" , "Kumar, Rahul R" , Gerd Hoffmann , Pedro Falcato , "Zhang, Di" , "Tan, Dun" References: <20240222084128.30000-1-jianfeng.zhou@intel.com> <0102018dd5d57f6b-a9056a17-e950-472d-acdc-644ba55bcc65-000000@eu-west-1.amazonses.com> <0102018dd6a9d291-d987d495-f53a-4e39-8782-336e3c2e13da-000000@eu-west-1.amazonses.com> From: "Laszlo Ersek" In-Reply-To: <0102018dd6a9d291-d987d495-f53a-4e39-8782-336e3c2e13da-000000@eu-west-1.amazonses.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.3 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: RJW0uquODzxUfJQanwi64rzpx7686176AA= 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=dzQUumv3; 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 2/23/24 16:51, Michael Brown wrote: > On 23/02/2024 15:12, Zhou, Jianfeng wrote: >>> While it may well cause the compiler to generate less optimised code, >>> there is absolutely no way that this volatile declaration on a local >>> stack variable can possibly change the outcome of the code. >>> There can never be any meaningful side-effects from reading or >>> writing a stack variable. >>> I would suggest dropping the volatile on LocalPte4K, since its *only* >>> possible impact is to confuse a future reader of the code. >> >> The change is for preventing compiler from optimizing. >> As a temporary variable,  LocalPte4K may be replaced by function >> parameter Pte4K. > > No, it can't.  If Pte4K is marked as a volatile pointer, then the > compiler is not allowed to unilaterally decide to treat it as a > non-volatile pointer. > >> In this case, code like "LocalPte4K.Bits.Present = >> Attribute->Bits.Present" may lead to unexpected result, as it is not >> atomic. Assembly code look like: >>     mov eax, [r8] >>     and dword [rcx], 0xfffffffe  // this instruction clear the present >> bit and may leads to unexpected result. >>     and eax, 0x1 >>     or [rcx], eax > > Please test with Pte4K marked as volatile and LocalPte4K marked as > non-volatile.  If you can still generate assembly code that writes to > *Pte4K more than once, then that would be a serious compiler bug. > > > As a separate note, I would also suggest removing the unnecessary second > read through Pte4K, since once Pte4K is marked as volatile the compiler > will generate an extra read from that address: > > --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c > +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c > @@ -30,7 +30,7 @@ PageTableLibSetPte4K ( > >    LocalPte4K.Uint64 = Pte4K->Uint64; >    if (Mask->Bits.PageTableBaseAddressLow || > Mask->Bits.PageTableBaseAddressHigh) { > -    LocalPte4K.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS > (Attribute) + Offset) | (Pte4K->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_40); > +    LocalPte4K.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS > (Attribute) + Offset) | (LocalPte4K.Uint64 & > ~IA32_PE_BASE_ADDRESS_MASK_40); >    } > >    if (Mask->Bits.Present) { Agreed on each point; couldn't have expressed them any better. (I didn't even notice the second read through Pte4K.) Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#115925): https://edk2.groups.io/g/devel/message/115925 Mute This Topic: https://groups.io/mt/104524857/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-