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

["Pedro Falcato" <pedro.falcato@gmail.com>]

On Wed, Feb 7, 2024 at 1:57 AM Zhou, Jianfeng <jianfeng.zhou@intel.com> wrote:
>
> 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.

I know, but the PI spec mentions (in the EFI_MP_SERVICES section) that
you need to be careful and can't call EFI code willy-nilly, which is
why (most) EFI code does not need to handle concurrency and locking.

>
> >>  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?

The compiler (and this is defined by the C spec) does not know that
your write has side effects. As such, it can re-order it, delete it,
split it in half, etc.
Say, for example:

UINT64 gGlobal;
UINT64 gGlobal2;

VOID
Foo (VOID)
{
    gGlobal = 10;
    gGlobal2 = 20;
}

The compiler can legally generate this (I hope i get the intel syntax
right, i usually write AT&T :)):
mov qword ptr [gGlobal2], 20
mov qword ptr [gGlobal], 10

or

mov dword ptr [gGlobal2], 20
mov dword ptr [gGlobal2 + 4], 0
mov dword ptr [gGlobal], 10
mov dword ptr [gGlobal + 4], 0

or any other variant, because "there are no side effects".
With volatile, the compiler knows there are side-effects so the
optimization it can do is quite limited.

>
> >>  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"

This is a parallel concern to this patch.
If you're running other APs and changing page table entries you *need*
to shoot their TLB entries down, for correctness. Or at least make
sure that when an AP goes to execute code, the TLB is completely
synchronized (by, say, a CR3 reload in every AP right after a wakeup).

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115241): https://edk2.groups.io/g/devel/message/115241
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]
-=-=-=-=-=-=-=-=-=-=-=-