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

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

On 2/7/24 02:05, Pedro Falcato wrote:
> 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.

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]
-=-=-=-=-=-=-=-=-=-=-=-