Re: [edk2-devel] [Patch V4 08/21] UefiCpuPkg/CpuPageTableLib:Add check for Mask and Attr

2023-03-23 Thread duntan
Thanks for the comments Ray. I will update the patches in next version patch 
set.

-Why break out of the loop when meeting a non-present entry?
Because I think the parameter check is only needed once.

Thanks,
Dun

-Original Message-
From: Ni, Ray  
Sent: Thursday, March 23, 2023 8:53 PM
To: Tan, Dun ; devel@edk2.groups.io
Cc: Dong, Eric ; Kumar, Rahul R ; 
Gerd Hoffmann 
Subject: RE: [Patch V4 08/21] UefiCpuPkg/CpuPageTableLib:Add check for Mask and 
Attr

The commit message is very detailed.

But the function header comments are not.
Can you please update the function header comments to add 4 lines of @retval 
RETURN_INVALID_PARAMETER, each line describes one invalid case.

It's ok to have multiple lines starting with @retval RETURN_INVALID_PARAMETER>

> +// If (LinearAddress + Length - 1) is not in the same 
> + ParentPagingEntry
> with (LinearAddress + Offset), then the remaining child PagingEntry
> +// starting from PagingEntryIndex of ParentPagingEntry is all 
> + covered by
> [LinearAddress + Offset, LinearAddress + Length - 1].
> +//
> +PagingEntryIndexEnd = (BitFieldRead64 (LinearAddress + Length - 
> + 1,
> BitStart + 9, 63) != BitFieldRead64 (LinearAddress + Offset, BitStart + 9, 
> 63)) ?
> 511 :
> +  (UINTN)BitFieldRead64 (LinearAddress + 
> + Length - 1, BitStart,
> BitStart + 9 - 1);
> +PagingEntry = (IA32_PAGING_ENTRY
> *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry-
> >Pnle);
> +for (Index = PagingEntryIndex; Index <= PagingEntryIndexEnd; Index++) {
> +  if (PagingEntry[Index].Pce.Present == 0) {
> +//
> +// [LinearAddress, LinearAddress + Length] contains non-present 
> range.
> +//
> +Status = IsAttributesAndMaskValidForNonPresentEntry 
> + (Attribute,
> Mask);
> +if (RETURN_ERROR (Status)) {
> +  return Status;
> +}
> +
> +break;

Why break out of the loop when meeting a non-present entry?



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101716): https://edk2.groups.io/g/devel/message/101716
Mute This Topic: https://groups.io/mt/97796382/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-




Re: [edk2-devel] [Patch V4 08/21] UefiCpuPkg/CpuPageTableLib:Add check for Mask and Attr

2023-03-23 Thread Ni, Ray
The commit message is very detailed.

But the function header comments are not.
Can you please update the function header comments to add 4 lines of
@retval RETURN_INVALID_PARAMETER, each line describes one invalid case.

It's ok to have multiple lines starting with @retval RETURN_INVALID_PARAMETER>

> +// If (LinearAddress + Length - 1) is not in the same ParentPagingEntry
> with (LinearAddress + Offset), then the remaining child PagingEntry
> +// starting from PagingEntryIndex of ParentPagingEntry is all covered by
> [LinearAddress + Offset, LinearAddress + Length - 1].
> +//
> +PagingEntryIndexEnd = (BitFieldRead64 (LinearAddress + Length - 1,
> BitStart + 9, 63) != BitFieldRead64 (LinearAddress + Offset, BitStart + 9, 
> 63)) ?
> 511 :
> +  (UINTN)BitFieldRead64 (LinearAddress + Length - 1, 
> BitStart,
> BitStart + 9 - 1);
> +PagingEntry = (IA32_PAGING_ENTRY
> *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry-
> >Pnle);
> +for (Index = PagingEntryIndex; Index <= PagingEntryIndexEnd; Index++) {
> +  if (PagingEntry[Index].Pce.Present == 0) {
> +//
> +// [LinearAddress, LinearAddress + Length] contains non-present 
> range.
> +//
> +Status = IsAttributesAndMaskValidForNonPresentEntry (Attribute,
> Mask);
> +if (RETURN_ERROR (Status)) {
> +  return Status;
> +}
> +
> +break;

Why break out of the loop when meeting a non-present entry?



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101675): https://edk2.groups.io/g/devel/message/101675
Mute This Topic: https://groups.io/mt/97796382/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-




[edk2-devel] [Patch V4 08/21] UefiCpuPkg/CpuPageTableLib:Add check for Mask and Attr

2023-03-23 Thread duntan
For different usage, check if the combination for Mask and
Attr is valid when creating or updating page table.

1.For non-present range
  1.1Mask.Present is 0 but some other attributes is provided.
 This case is invalid.
  1.2Mask.Present is 1 and Attr.Present is 0. In this case,all
 other attributes should not be provided.
  1.3Mask.Present is 1 and Attr.Present is 1. In this case,all
 attributes should be provided to intialize the attribute.

2.For present range
  2.1Mask.Present is 1 and Attr.Present is 0.In this case, all
 other attributes should not be provided.
All other usage for present range is permitted.
In the mentioned cases, 1.2 and 2.1 can be merged into 1 check.

Signed-off-by: Dun Tan 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Rahul Kumar 
Cc: Gerd Hoffmann 
---
 UefiCpuPkg/Include/Library/CpuPageTableLib.h |  2 +-
 UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 79 
+--
 2 files changed, 78 insertions(+), 3 deletions(-)

diff --git a/UefiCpuPkg/Include/Library/CpuPageTableLib.h 
b/UefiCpuPkg/Include/Library/CpuPageTableLib.h
index 5f44ece548..6bda15b5bc 100644
--- a/UefiCpuPkg/Include/Library/CpuPageTableLib.h
+++ b/UefiCpuPkg/Include/Library/CpuPageTableLib.h
@@ -76,7 +76,7 @@ typedef enum {
   @param[in]  Mask   The mask used for attribute. The 
corresponding field in Attribute is ignored if that in Mask is 0.
 
   @retval RETURN_UNSUPPORTEDPagingMode is not supported.
-  @retval RETURN_INVALID_PARAMETER  PageTable, BufferSize, Attribute or Mask 
is NULL.
+  @retval RETURN_INVALID_PARAMETER  PageTable, BufferSize, Attribute or Mask 
is NULL or the combination of Attribute and Mask is invalid.
   @retval RETURN_INVALID_PARAMETER  *BufferSize is not multiple of 4KB.
   @retval RETURN_BUFFER_TOO_SMALL   The buffer is too small for page table 
creation/updating.
 BufferSize is updated to indicate the 
expected buffer size.
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c 
b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 76febdd42d..2ad22b333d 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -215,6 +215,43 @@ PageTableLibSetPnle (
   Pnle->Bits.CacheDisabled = 0;
 }
 
+/**
+  Check if the combination for Attribute and Mask is valid for non-present 
entry.
+  1.Mask.Present is 0 but some other attributes is provided. This case should 
be invalid.
+  2.Map non-present range to present. In this case, all attributes should be 
provided.
+
+  @param[in] AttributeThe attribute of the linear address range.
+  @param[in] Mask The mask used for attribute to check.
+
+  @retval RETURN_INVALID_PARAMETERThe combination for Attribute and Mask 
is invalid.
+  @retval RETURN_SUCCESS  The combination for Attribute and Mask 
is valid.
+**/
+RETURN_STATUS
+IsAttributesAndMaskValidForNonPresentEntry (
+  IN IA32_MAP_ATTRIBUTE  *Attribute,
+  IN IA32_MAP_ATTRIBUTE  *Mask
+  )
+{
+  if ((Mask->Bits.Present == 1) && (Attribute->Bits.Present == 1)) {
+//
+// Creating new page table or remapping non-present range to present.
+//
+if ((Mask->Bits.ReadWrite == 0) || (Mask->Bits.UserSupervisor == 0) || 
(Mask->Bits.WriteThrough == 0) || (Mask->Bits.CacheDisabled == 0) ||
+(Mask->Bits.Accessed == 0) || (Mask->Bits.Dirty == 0) || 
(Mask->Bits.Pat == 0) || (Mask->Bits.Global == 0) ||
+(Mask->Bits.PageTableBaseAddress == 0) || (Mask->Bits.ProtectionKey == 
0) || (Mask->Bits.Nx == 0))
+{
+  return RETURN_INVALID_PARAMETER;
+}
+  } else if ((Mask->Bits.Present == 0) && (Mask->Uint64 > 1)) {
+//
+// Only change other attributes for non-present range is not permitted.
+//
+return RETURN_INVALID_PARAMETER;
+  }
+
+  return RETURN_SUCCESS;
+}
+
 /**
   Update page table to map [LinearAddress, LinearAddress + Length) with 
specified attribute in the specified level.
 
@@ -237,6 +274,7 @@ PageTableLibSetPnle (
 when a new physical base address is set.
   @param[in]  Mask  The mask used for attribute. The 
corresponding field in Attribute is ignored if that in Mask is 0.
 
+  @retval RETURN_INVALID_PARAMETER  The combination of Attribute and Mask for 
non-present entry is invalid.
   @retval RETURN_SUCCESSPageTable is created/updated successfully.
 **/
 RETURN_STATUS
@@ -260,6 +298,7 @@ PageTableLibMapInLevel (
   UINTN   Index;
   IA32_PAGING_ENTRY   *PagingEntry;
   UINTN   PagingEntryIndex;
+  UINTN   PagingEntryIndexEnd;
   IA32_PAGING_ENTRY   *CurrentPagingEntry;
   UINT64  RegionLength;
   UINT64  SubLength;
@@ -306,6 +345,14 @@ PageTableLibMapInLevel (
   //
 
   if (ParentPagingEntry->Pce.Present == 0) {
+//
+// [LinearAddress, LinearAddress + L