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

["Laszlo Ersek" <lersek@redhat.com>]

On 2/7/24 01:47, 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.

I thought that patch#1 was related to MP paging because:

- patch#3 *was* indeed related to MP paging, and these patches are,
well, part of the same series;

- more importantly, the commit message on patch#1 explicitly says, "If
*some other core* is accessing the page, it may leads to expection" --
emphasis mine.

(The first patch also uses the word "optimize", which I find very
misleading.)

Furthermore, the logic change in the first patch targets
CpuPageTableLib. As far as I can tell, the logic change bubbles up to
the public PageTableMap() API. I don't know if that API is designed for
just UP usage (a processor can use it only for manipulating its own page
tables), or for MP usage. Given the larger context of patch#1, I assumed
there was an MP context to patch#1 as well. (And, again, the commit
message explicitly references other cores.)

> As my understanding,  MP reading/writing to the same page table at the same time is not recommended, perhaps, it is not allowed.
> 
> 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.
> 
> We hit this case recently.  Several engineers pay days for test, root case and verification:  the reproducibility rate is low and not reproduced on every system.

OK, so it *does* happen in an MP context, too.

> 
> We can fix it by other solution, while we decided to upstream this change for:
> 1) the change is harmless
> 2) It is a defect
> 3) It hard to debug and root cause
> 4) We don't want other engineers to spend a lot of time dealing with this kind of problem.

This is all very nice; I can accept that the present code change may be
a useful practical improvement. My issue is that the commit message does
not represent either the problem faithfully, or the scope / intent of
the code changes.

Laszlo



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