Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Reduce and optimize access to attribute

["Zhou, Jianfeng" <jianfeng.zhou@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3683 bytes --]

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 <pedro.falcato@gmail.com> 
Sent: Wednesday, February 7, 2024 9:05 AM
To: Zhou, Jianfeng <jianfeng.zhou@intel.com>
Cc: devel@edk2.groups.io; lersek@redhat.com; Tan, Dun <dun.tan@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
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 <jianfeng.zhou@intel.com> 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]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: PageTableLibSetPnle.s --]
[-- Type: application/octet-stream, Size: 1207 bytes --]

0x7FD42994 PageTableLibSetPnle:         test byte [r8], 0x1
0x7FD42998    mov r10, [rcx]
0x7FD4299B    mov [rsp+0x8], r10
0x7FD429A0    mov r9d, r10d
0x7FD429A3    jz 0x7fd429b0
0x7FD429A5    mov eax, [rdx]
0x7FD429A7    xor eax, r10d
0x7FD429AA    and eax, 0x1
0x7FD429AD    xor r9d, eax
0x7FD429B0    test byte [r8], 0x2
0x7FD429B4    jz 0x7fd429c1
0x7FD429B6    mov eax, [rdx]
0x7FD429B8    xor eax, r9d
0x7FD429BB    and eax, 0x2
0x7FD429BE    xor r9d, eax
0x7FD429C1    test byte [r8], 0x4
0x7FD429C5    jz 0x7fd429d2
0x7FD429C7    mov eax, [rdx]
0x7FD429C9    xor eax, r9d
0x7FD429CC    and eax, 0x4
0x7FD429CF    xor r9d, eax
0x7FD429D2    cmp dword [r8+0x4], 0x80000000
0x7FD429DA    jb 0x7fd429ee
0x7FD429DC    mov eax, [rdx+0x4]
0x7FD429DF    xor eax, [rsp+0xc]
0x7FD429E3    btr eax, 0x1f
0x7FD429E7    xor eax, [rdx+0x4]
0x7FD429EA    mov [rsp+0xc], eax
0x7FD429EE    and r9d, 0xffffff47
0x7FD429F5    mov [rsp+0x8], r9d   ###
0x7FD429FA    mov rax, [rsp+0x8]   ###
0x7FD429FF    cmp r10, rax         ### if (Pnle->Uint64 != LocalPnle.Uint64)
0x7FD42A02    jz 0x7fd42a07
0x7FD42A04    mov [rcx], rax       ### Pnle->Uint64 = LocalPnle.Uint64
0x7FD42A07    ret