public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Zhou, Jianfeng" <jianfeng.zhou@intel.com>
To: Laszlo Ersek <lersek@redhat.com>,
	Pedro Falcato <pedro.falcato@gmail.com>
Cc: "devel@edk2.groups.io" <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
Date: Thu, 8 Feb 2024 02:29:20 +0000	[thread overview]
Message-ID: <PH7PR11MB66732817AE09324BEA58EA78EF442@PH7PR11MB6673.namprd11.prod.outlook.com> (raw)
In-Reply-To: <7262d528-ee3d-d8f8-cad7-9bddec057a1b@redhat.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]
-=-=-=-=-=-=-=-=-=-=-=-



  reply	other threads:[~2024-02-09 10:08 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-05 14:03 [edk2-devel] [PATCH 0/3] Fix potential issue in CpuPageTableLib and SMM page table initialization duntan
2024-02-05 14:03 ` [edk2-devel] [PATCH 1/3] UefiCpuPkg: Reduce and optimize access to attribute duntan
2024-02-06  1:20   ` Ni, Ray
2024-02-06 13:32   ` Laszlo Ersek
2024-02-06 15:02     ` Ni, Ray
2024-02-06 17:34     ` Pedro Falcato
2024-02-07  0:47       ` Zhou, Jianfeng
2024-02-07  1:05         ` Pedro Falcato
2024-02-07  1:57           ` Zhou, Jianfeng
2024-02-07 17:52             ` Pedro Falcato
2024-02-07 20:42             ` Laszlo Ersek
2024-02-08  2:29               ` Zhou, Jianfeng [this message]
2024-02-07 20:33           ` Laszlo Ersek
2024-02-07 20:17         ` Laszlo Ersek
2024-02-05 14:03 ` [edk2-devel] [PATCH 2/3] UefiCpuPkg: Add more Paging mode enumeration duntan
2024-02-06  1:21   ` Ni, Ray
2024-02-05 14:03 ` [edk2-devel] [PATCH 3/3] UefiCpuPkg/PiSmmCpuDxeSmm:Map SMRAM in 4K page granularity duntan
2024-02-06  1:23   ` Ni, Ray
2024-02-06 13:33   ` Laszlo Ersek
2024-02-06  1:48 ` [edk2-devel] [PATCH 0/3] Fix potential issue in CpuPageTableLib and SMM page table initialization Ni, Ray

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=PH7PR11MB66732817AE09324BEA58EA78EF442@PH7PR11MB6673.namprd11.prod.outlook.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox