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 17855D80110 for ; Wed, 7 Feb 2024 20:33:57 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=9VwsF9DRqy/9RYtaO/ScQqelG+8rK5TqP3aiPhe0pUo=; 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=1707338036; v=1; b=JLLeTANN5KSx4uc02h3Uhp75yswnMeZaQop9486Ulpi8gEgZivAeYB8zyN0L7FZ/LUD8MHMQ /AZvzqXB02D4Hg8R+Sbg+5GpthUJQyozd7N+TlOHo5I2j8HO7piSnSr6iT2I9EgaszKLLcS0AtI OMh4BV80++UESlbyF3iIGYGk= X-Received: by 127.0.0.2 with SMTP id NKT8YY7687511xQ1OW3ahZ6v; Wed, 07 Feb 2024 12:33:56 -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.592.1707338035978152775 for ; Wed, 07 Feb 2024 12:33:56 -0800 X-Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-544-euJ6OB8lOW2Ri4vu8JSACg-1; Wed, 07 Feb 2024 15:33:51 -0500 X-MC-Unique: euJ6OB8lOW2Ri4vu8JSACg-1 X-Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (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 A76111C05AA6; Wed, 7 Feb 2024 20:33:50 +0000 (UTC) X-Received: from [10.39.195.126] (unknown [10.39.195.126]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 7D8731103A; Wed, 7 Feb 2024 20:33:49 +0000 (UTC) Message-ID: Date: Wed, 7 Feb 2024 21:33:48 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Reduce and optimize access to attribute To: Pedro Falcato , "Zhou, Jianfeng" Cc: "devel@edk2.groups.io" , "Tan, Dun" , "Ni, Ray" , "Kumar, Rahul R" , Gerd Hoffmann References: <20240205140345.1437-1-dun.tan@intel.com> <20240205140345.1437-2-dun.tan@intel.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.5 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: K8raMhdQNqIA6J7oxrUJIbz2x7686176AA= 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=JLLeTANN; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none) On 2/7/24 02:05, Pedro Falcato wrote: > On Wed, Feb 7, 2024 at 12:47 AM Zhou, Jianfeng wrote: >> >> Hi Laszlo, Pedro, >> >> Clarify one thing, this change is not for racing introduced by MP reading/writing to the same page table at the same time, but for unexpected behavior introduced by compiler. >> As my understanding, MP reading/writing to the same page table at the same time is not recommended, perhaps, it is not allowed. > > AFAIK, it's not allowed as IIRC APs cannot run arbitrary EFI boot services code. It need not be about two processors manipulating the same page table; it suffices (for the problem) if one processor is massaging the page table while another processor is accesing the affected virtual address range. The AP may be running custom code that does not touch UEFI / PI services at all (computing something in preallocated memory etc). > >> >> For bit operation code, such as Pnle->Bits.Present = Attribute->Bits.Present, we might think it is atomic assignment, while not. The assembly code looks like: >> and dword [rcx], 0xfffffffe >> and eax, 0x1 >> or [rcx], eax >> In case Pnle->Bits.Present = 1, Attribute->Bits.Present = 1, we might think it is harmless, as the value not changed. While actually, >> and dword [rcx], 0xfffffffe // the present bit set to 0 ---- this is unexpected !!!!! we don’t want the present bit set to 0! >> and eax, 0x1 >> or [rcx], eax // the present bit set to right value 1 >> >> Let's consider such a MP scenario: >> 1) one processor executing instruction "and dword [rcx], 0xfffffffe" >> 2) other processors happened to access the memory mapped by Pnle, it may lead to exception. > > I understand your original problem, but: > > 1) Your fix is not correct. The compiler can tear your store, you need > to use a volatile store for this. (Side comment: I don't disagree that "volatile" may be, and mostly is, sufficient for preventing tearing, but I'd like to note that the C99 spec itself does not seem to require that behavior of volatile. Section "5.1.2.3 Program execution", paragraph 5 says, "The least requirements on a conforming implementation are: — At sequence points, volatile objects are stable in the sense that previous accesses are complete and subsequent accesses have not yet occurred. [...]". I think Linux is very correct to use a dedicated macro WRITE_ONCE, and if that *happens* to expand to a volatile access on a particular platform & compiler, that's great, but an implementation detail. In edk2, I would like to see something stronger than just volatile; at least semantically. For example, we have MmioWrite64() from IoLib, we have EFI_CPU_IO2_PROTOCOL.Mem.Write() etc. If they are implemented with plain "volatile" in the background, that may be fine in practice. It's just always difficult to tell from the code and the patches whether an access is *supposed* to be "atomic" or not!) > 2) What kind of page table manipulation is happening while APs are > running code, and does this mean you need a TLB shootdown mechanism? > Yes, ideally, patch#1's commit message should include a stack trace for CPU A and another for CPU B. I reckon those may be difficult to collect *precisely*, but some natural language explanation would help. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#115250): https://edk2.groups.io/g/devel/message/115250 Mute This Topic: https://groups.io/mt/104176232/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-