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

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

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.

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.

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.


Thanks & Regards,
Zhou Jianfeng

-----Original Message-----
From: Pedro Falcato <pedro.falcato@gmail.com> 
Sent: Wednesday, February 7, 2024 1:35 AM
To: devel@edk2.groups.io; lersek@redhat.com
Cc: Tan, Dun <dun.tan@intel.com>; Zhou, Jianfeng <jianfeng.zhou@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 Tue, Feb 6, 2024 at 1:32 PM Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 2/5/24 15:03, duntan wrote:
> > From: Zhou Jianfeng <jianfeng.zhou@intel.com>
> >
> > This commit is to reduce and optimize access to attribute in 
> > CpuPageTableLib.
> >
> > Unreasonable writing to attribute of page table may leads to 
> > expection.
> > The assembly code for C code Pnle->Bits.Present =
> > Attribute->Bits.Present looks like:
> >    and dword [rcx], 0xfffffffe
> >    and eax, 0x1
> >    or [rcx], eax
> > In case Pnle->Bits.Present and Attribute->Bits.Present is 1, 
> > Pnle->Bits.Present will be set to 0 for short
> > time(2 instructions) which is unexpected. If some other core is 
> > accessing the page, it may leads to expection.
> > This change reduce and optimize access to attribute of page table, 
> > attribute of page table is set only when it need to be changed.
>
> This patch does nothing to eliminate the actual race condition, it 
> only shrinks the window of potential corruption.

FWIW, it's still not entirely correct: the compiler can tear the Uint64 store.
You'd need something like WRITE_ONCE (which in Linux essentially does *(volatile Type *) ptr = val;)

> The PTEs continue to be overwritten without any kind of 
> synchronization with the other processors.

I don't think we should be messing with page tables while APs are up.
That will require a whole infrastructure to do TLB shootdowns.

Zhou, Ray, what exactly is racing here?

--
Pedro


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