Hi Pedro, >> AFAIK, it's not allowed as IIRC APs cannot run arbitrary EFI boot services code. [Zhou] The scenario I mentioned/The case we hit is during BIOS boot, not software after EndOfBootService. >> 1) Your fix is not correct. The compiler can tear your store, you need to use a volatile store for this. [Zhou] Assembly code of function PageTableLibSetPnle attached. The code is expected. Can't get "need to use a volatile store for this", would you please share more detail about it? >> 2) What kind of page table manipulation is happening while APs are running code, and does this mean you need a TLB shootdown mechanism? [Zhou] a) happened while split 2M Page to 4K to make full use of memory: it is long story. Anyway, the bit operation code is unexpected, and my change is harmless. b) my understanding, TLB shootdown mechanism not suitable for this case. It's too late to "shootdown". AP might exception before "shootdown" One more extreme scenario, the code " Pnle->Bits.Present = Attribute->Bits.Present " happened to be mapped by Pnle, it will lead to exception even in one processor case. Just mentioning, please ignore this scenario as it is hard to verify. Thanks & Regards, Zhou Jianfeng -----Original Message----- From: Pedro Falcato Sent: Wednesday, February 7, 2024 9:05 AM To: Zhou, Jianfeng Cc: devel@edk2.groups.io; lersek@redhat.com; Tan, Dun ; Ni, Ray ; Kumar, Rahul R ; Gerd Hoffmann Subject: Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Reduce and optimize access to attribute 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. > > 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. 2) What kind of page table manipulation is happening while APs are running code, and does this mean you need a TLB shootdown mechanism? -- Pedro -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#115257): https://edk2.groups.io/g/devel/message/115257 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] -=-=-=-=-=-=-=-=-=-=-=-