public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Laszlo Ersek" <lersek@redhat.com>
To: devel@edk2.groups.io, dun.tan@intel.com
Cc: Zhou Jianfeng <jianfeng.zhou@intel.com>,
	Ray Ni <ray.ni@intel.com>, Rahul Kumar <rahul1.kumar@intel.com>,
	Gerd Hoffmann <kraxel@redhat.com>
Subject: Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Reduce and optimize access to attribute
Date: Tue, 6 Feb 2024 14:32:30 +0100	[thread overview]
Message-ID: <a8b16559-4747-4ca4-f4aa-d958bb8c0a73@redhat.com> (raw)
In-Reply-To: <20240205140345.1437-2-dun.tan@intel.com>

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.

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

Feel free to merge this with Ray's R-b.

Laszlo

> 
> Signed-off-by: Zhou Jianfeng <jianfeng.zhou@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------
>  1 file changed, 53 insertions(+), 33 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> index 36b2c4e6a3..ae4caf8dfe 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> @@ -26,52 +26,59 @@ PageTableLibSetPte4K (
>    IN IA32_MAP_ATTRIBUTE  *Mask
>    )
>  {
> +  IA32_PTE_4K  LocalPte4K;
> +
> +  LocalPte4K.Uint64 = Pte4K->Uint64;
>    if (Mask->Bits.PageTableBaseAddressLow || Mask->Bits.PageTableBaseAddressHigh) {
> -    Pte4K->Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset) | (Pte4K->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_40);
> +    LocalPte4K.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset) | (Pte4K->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_40);
>    }
>  
>    if (Mask->Bits.Present) {
> -    Pte4K->Bits.Present = Attribute->Bits.Present;
> +    LocalPte4K.Bits.Present = Attribute->Bits.Present;
>    }
>  
>    if (Mask->Bits.ReadWrite) {
> -    Pte4K->Bits.ReadWrite = Attribute->Bits.ReadWrite;
> +    LocalPte4K.Bits.ReadWrite = Attribute->Bits.ReadWrite;
>    }
>  
>    if (Mask->Bits.UserSupervisor) {
> -    Pte4K->Bits.UserSupervisor = Attribute->Bits.UserSupervisor;
> +    LocalPte4K.Bits.UserSupervisor = Attribute->Bits.UserSupervisor;
>    }
>  
>    if (Mask->Bits.WriteThrough) {
> -    Pte4K->Bits.WriteThrough = Attribute->Bits.WriteThrough;
> +    LocalPte4K.Bits.WriteThrough = Attribute->Bits.WriteThrough;
>    }
>  
>    if (Mask->Bits.CacheDisabled) {
> -    Pte4K->Bits.CacheDisabled = Attribute->Bits.CacheDisabled;
> +    LocalPte4K.Bits.CacheDisabled = Attribute->Bits.CacheDisabled;
>    }
>  
>    if (Mask->Bits.Accessed) {
> -    Pte4K->Bits.Accessed = Attribute->Bits.Accessed;
> +    LocalPte4K.Bits.Accessed = Attribute->Bits.Accessed;
>    }
>  
>    if (Mask->Bits.Dirty) {
> -    Pte4K->Bits.Dirty = Attribute->Bits.Dirty;
> +    LocalPte4K.Bits.Dirty = Attribute->Bits.Dirty;
>    }
>  
>    if (Mask->Bits.Pat) {
> -    Pte4K->Bits.Pat = Attribute->Bits.Pat;
> +    LocalPte4K.Bits.Pat = Attribute->Bits.Pat;
>    }
>  
>    if (Mask->Bits.Global) {
> -    Pte4K->Bits.Global = Attribute->Bits.Global;
> +    LocalPte4K.Bits.Global = Attribute->Bits.Global;
>    }
>  
>    if (Mask->Bits.ProtectionKey) {
> -    Pte4K->Bits.ProtectionKey = Attribute->Bits.ProtectionKey;
> +    LocalPte4K.Bits.ProtectionKey = Attribute->Bits.ProtectionKey;
>    }
>  
>    if (Mask->Bits.Nx) {
> -    Pte4K->Bits.Nx = Attribute->Bits.Nx;
> +    LocalPte4K.Bits.Nx = Attribute->Bits.Nx;
> +  }
> +
> +  if (Pte4K->Uint64 != LocalPte4K.Uint64) {
> +    Pte4K->Uint64 = LocalPte4K.Uint64;
>    }
>  }
>  
> @@ -93,54 +100,61 @@ PageTableLibSetPleB (
>    IN IA32_MAP_ATTRIBUTE                 *Mask
>    )
>  {
> +  IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  LocalPleB;
> +
> +  LocalPleB.Uint64 = PleB->Uint64;
>    if (Mask->Bits.PageTableBaseAddressLow || Mask->Bits.PageTableBaseAddressHigh) {
> -    PleB->Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset) | (PleB->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_39);
> +    LocalPleB.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset) | (PleB->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_39);
>    }
>  
> -  PleB->Bits.MustBeOne = 1;
> +  LocalPleB.Bits.MustBeOne = 1;
>  
>    if (Mask->Bits.Present) {
> -    PleB->Bits.Present = Attribute->Bits.Present;
> +    LocalPleB.Bits.Present = Attribute->Bits.Present;
>    }
>  
>    if (Mask->Bits.ReadWrite) {
> -    PleB->Bits.ReadWrite = Attribute->Bits.ReadWrite;
> +    LocalPleB.Bits.ReadWrite = Attribute->Bits.ReadWrite;
>    }
>  
>    if (Mask->Bits.UserSupervisor) {
> -    PleB->Bits.UserSupervisor = Attribute->Bits.UserSupervisor;
> +    LocalPleB.Bits.UserSupervisor = Attribute->Bits.UserSupervisor;
>    }
>  
>    if (Mask->Bits.WriteThrough) {
> -    PleB->Bits.WriteThrough = Attribute->Bits.WriteThrough;
> +    LocalPleB.Bits.WriteThrough = Attribute->Bits.WriteThrough;
>    }
>  
>    if (Mask->Bits.CacheDisabled) {
> -    PleB->Bits.CacheDisabled = Attribute->Bits.CacheDisabled;
> +    LocalPleB.Bits.CacheDisabled = Attribute->Bits.CacheDisabled;
>    }
>  
>    if (Mask->Bits.Accessed) {
> -    PleB->Bits.Accessed = Attribute->Bits.Accessed;
> +    LocalPleB.Bits.Accessed = Attribute->Bits.Accessed;
>    }
>  
>    if (Mask->Bits.Dirty) {
> -    PleB->Bits.Dirty = Attribute->Bits.Dirty;
> +    LocalPleB.Bits.Dirty = Attribute->Bits.Dirty;
>    }
>  
>    if (Mask->Bits.Pat) {
> -    PleB->Bits.Pat = Attribute->Bits.Pat;
> +    LocalPleB.Bits.Pat = Attribute->Bits.Pat;
>    }
>  
>    if (Mask->Bits.Global) {
> -    PleB->Bits.Global = Attribute->Bits.Global;
> +    LocalPleB.Bits.Global = Attribute->Bits.Global;
>    }
>  
>    if (Mask->Bits.ProtectionKey) {
> -    PleB->Bits.ProtectionKey = Attribute->Bits.ProtectionKey;
> +    LocalPleB.Bits.ProtectionKey = Attribute->Bits.ProtectionKey;
>    }
>  
>    if (Mask->Bits.Nx) {
> -    PleB->Bits.Nx = Attribute->Bits.Nx;
> +    LocalPleB.Bits.Nx = Attribute->Bits.Nx;
> +  }
> +
> +  if (PleB->Uint64 != LocalPleB.Uint64) {
> +    PleB->Uint64 = LocalPleB.Uint64;
>    }
>  }
>  
> @@ -186,24 +200,27 @@ PageTableLibSetPnle (
>    IN IA32_MAP_ATTRIBUTE        *Mask
>    )
>  {
> +  IA32_PAGE_NON_LEAF_ENTRY  LocalPnle;
> +
> +  LocalPnle.Uint64 = Pnle->Uint64;
>    if (Mask->Bits.Present) {
> -    Pnle->Bits.Present = Attribute->Bits.Present;
> +    LocalPnle.Bits.Present = Attribute->Bits.Present;
>    }
>  
>    if (Mask->Bits.ReadWrite) {
> -    Pnle->Bits.ReadWrite = Attribute->Bits.ReadWrite;
> +    LocalPnle.Bits.ReadWrite = Attribute->Bits.ReadWrite;
>    }
>  
>    if (Mask->Bits.UserSupervisor) {
> -    Pnle->Bits.UserSupervisor = Attribute->Bits.UserSupervisor;
> +    LocalPnle.Bits.UserSupervisor = Attribute->Bits.UserSupervisor;
>    }
>  
>    if (Mask->Bits.Nx) {
> -    Pnle->Bits.Nx = Attribute->Bits.Nx;
> +    LocalPnle.Bits.Nx = Attribute->Bits.Nx;
>    }
>  
> -  Pnle->Bits.Accessed   = 0;
> -  Pnle->Bits.MustBeZero = 0;
> +  LocalPnle.Bits.Accessed   = 0;
> +  LocalPnle.Bits.MustBeZero = 0;
>  
>    //
>    // Set the attributes (WT, CD, A) to 0.
> @@ -211,8 +228,11 @@ PageTableLibSetPnle (
>    // So, it implictly requires PAT[0] is Write Back.
>    // Create a new parameter if caller requires to use a different memory type for accessing page directories.
>    //
> -  Pnle->Bits.WriteThrough  = 0;
> -  Pnle->Bits.CacheDisabled = 0;
> +  LocalPnle.Bits.WriteThrough  = 0;
> +  LocalPnle.Bits.CacheDisabled = 0;
> +  if (Pnle->Uint64 != LocalPnle.Uint64) {
> +    Pnle->Uint64 = LocalPnle.Uint64;
> +  }
>  }
>  
>  /**



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



  parent reply	other threads:[~2024-02-06 13:32 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 [this message]
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
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=a8b16559-4747-4ca4-f4aa-d958bb8c0a73@redhat.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