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

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

Sorry for misleading information in the commit message.
1) regarding the compiler tear down, I have reservations.  Anyway, adding volatile is harmless,  will submit patch for it
2) regarding "IN OUT" for parameter,  will add "OUT" for the parameter in the patch

Thanks & Regards,
Zhou Jianfeng

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com> 
Sent: Thursday, February 8, 2024 4:42 AM
To: Zhou, Jianfeng <jianfeng.zhou@intel.com>; Pedro Falcato <pedro.falcato@gmail.com>
Cc: devel@edk2.groups.io; Tan, Dun <dun.tan@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 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 (#115297): https://edk2.groups.io/g/devel/message/115297
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]
-=-=-=-=-=-=-=-=-=-=-=-