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

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

On 2/7/24 02:57, Zhou, Jianfeng 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.
> 
>>>  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?

With the patch in place, the compiler *happens* to generate code that
uses a single instruction. That's nice, but how stable is that?

IIUC, Pedro's point is that the "Pte4K" parameter of
PageTableLibSetPte4K() should point to a volatile IA32_PTE_4K object,
because that would *guarantee* that the compiler *always* generates a
single instruction for the final assignment.

(Now, I'm not sure about that, i.e. that even volatile is strong enough,
but that's a different topic.)

--*--

BTW, with this patch in place (as commit 30a25f277821, "UefiCpuPkg:
Reduce and optimize access to attribute", 2024-02-06), we have:

VOID
PageTableLibSetPte4K (
  IN IA32_PTE_4K         *Pte4K,
  IN UINT64              Offset,
  IN IA32_MAP_ATTRIBUTE  *Attribute,
  IN IA32_MAP_ATTRIBUTE  *Mask
  )
{
  IA32_PTE_4K  LocalPte4K;

  LocalPte4K.Uint64 = Pte4K->Uint64;
...
  if (Pte4K->Uint64 != LocalPte4K.Uint64) {
    Pte4K->Uint64 = LocalPte4K.Uint64;
  }
}

This means that the "Pte4K" parameter should be marked "IN OUT", not
just "IN". (Independently of whether we also qualify (*Pte4K) as volatile.)

Of course, it's not a bug in the patch, it's a (documentation) bug in
the pre-patch code. Can you perhaps submit a patch to fix that?

Thanks,
Laszlo



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