Re: [edk2] [PATCH v2 0/2] Fix bugs in HiiDatabase driver

[Gao, Liming]

This is to fix the security issue. I agree it is an import bug fix. I am OK to 
push it for edk2-stable201903 tag

Thanks
Liming
> -Original Message-
> From: Wang, Jian J
> Sent: Thursday, March 7, 2019 7:17 PM
> To: Ni, Ray ; edk2-devel@lists.01.org
> Cc: Cetola, Stephano ; Gao, Liming 
> 
> Subject: RE: [edk2] [PATCH v2 0/2] Fix bugs in HiiDatabase driver
> 
> Hi all,
> 
> This is a very important fix for this issue. If no objection, I'd like the 
> patch be part of this stable tag.
> 
> 
> As to this patch series,
> 
> Reviewed-by: Jian J Wang 
> 
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ray 
> > Ni
> > Sent: Friday, March 08, 2019 10:35 AM
> > To: edk2-devel@lists.01.org
> > Subject: [edk2] [PATCH v2 0/2] Fix bugs in HiiDatabase driver
> >
> > v2: put the CVE number in patch title.
> >
> > Ray Ni (2):
> >   MdeModulePkg/HiiDatabase: Fix potential integer overflow
> > (CVE-2018-12181)
> >   MdeModulePkg/HiiImage: Fix stack overflow when corrupted BMP is parsed
> > (CVE-2018-12181)
> >
> >  MdeModulePkg/Universal/HiiDatabaseDxe/Image.c | 130 ++
> >  1 file changed, 105 insertions(+), 25 deletions(-)
> >
> > --
> > 2.20.1.windows.1
> >
> > ___
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] UefiCpuPkg CpuDxe GDT init question?

[Andrew Fish via edk2-devel]

Actually it looks like the the CpuDxe driver is coded to only run if it it is 
loaded under 4 GB? Is that following the spec? Is that intentional?

I noticed that SetCodeSelector is coded to use a far jump and that is a 32-bit 
absolute value? Note [rsp+4]
https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm#L28
ASM_PFX(SetCodeSelector):
sub rsp, 0x10
lea rax, [setCodeSelectorLongJump]
mov [rsp], rax
mov [rsp+4], cx
jmp dword far [rsp]
setCodeSelectorLongJump:
add rsp, 0x10
ret


Thanks,

Andrew Fish

> On Mar 7, 2019, at 2:37 PM, Andrew Fish  wrote:
> 
> I'm trying to understand why gdtPtr.Base is casting to (UINT32)? 
> 1) gdtPtr.Base is a a UINTN
> 2) It is legal for AllocateRuntimePool() to return an address > 4GB
> 
> It seems like the code should just cast to (UINTN)?
> 
> 
> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/CpuDxe/CpuGdt.c#L151 
> 
> 
> 
> 
> VOID
> InitGlobalDescriptorTable (
>   VOID
>   )
> {
>   GDT_ENTRIES *gdt;
>   IA32_DESCRIPTOR gdtPtr;
> 
>   //
>   // Allocate Runtime Data for the GDT
>   //
>   gdt = AllocateRuntimePool (sizeof (GdtTemplate) + 8);
>   ASSERT (gdt != NULL);
>   gdt = ALIGN_POINTER (gdt, 8);
> 
>   //
>   // Initialize all GDT entries
>   //
>   CopyMem (gdt, , sizeof (GdtTemplate));
> 
>   //
>   // Write GDT register
>   //
>   gdtPtr.Base = (UINT32)(UINTN)(VOID*) gdt;
>   gdtPtr.Limit = (UINT16) (sizeof (GdtTemplate) - 1);
>   AsmWriteGdtr ();
> 
> Thanks,
> 
> Andrew Fish

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 0/2] Fix bugs in HiiDatabase driver

[Wang, Jian J]

Hi all,

This is a very important fix for this issue. If no objection, I'd like the 
patch be part of this stable tag.


As to this patch series, 

Reviewed-by: Jian J Wang 


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ray Ni
> Sent: Friday, March 08, 2019 10:35 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH v2 0/2] Fix bugs in HiiDatabase driver
> 
> v2: put the CVE number in patch title.
> 
> Ray Ni (2):
>   MdeModulePkg/HiiDatabase: Fix potential integer overflow
> (CVE-2018-12181)
>   MdeModulePkg/HiiImage: Fix stack overflow when corrupted BMP is parsed
> (CVE-2018-12181)
> 
>  MdeModulePkg/Universal/HiiDatabaseDxe/Image.c | 130 ++
>  1 file changed, 105 insertions(+), 25 deletions(-)
> 
> --
> 2.20.1.windows.1
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] Maintainers.txt: Change package maintainer and reviewer of SecurityPkg.

[Wang, Jian J]

Reviewed-by: Jian J Wang 

> -Original Message-
> From: Zhang, Chao B
> Sent: Friday, March 08, 2019 10:57 AM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen ; Wang, Jian J 
> Subject: [Patch] Maintainers.txt: Change package maintainer and reviewer of
> SecurityPkg.
> 
> Cc: Yao Jiewen 
> Cc: Jian Wang 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Zhang, Chao B 
> ---
>  Maintainers.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Maintainers.txt b/Maintainers.txt
> index 7772926b2f..08a676b236 100644
> --- a/Maintainers.txt
> +++ b/Maintainers.txt
> @@ -1,6 +1,6 @@
> -EDK II Maintainers
> +EDK II Maintainers
>  ==
> 
>  This file provides information about the primary maintainers for
>  EDK II.
> 
> @@ -237,10 +237,11 @@ M: Kelly Steele 
> 
>  SecurityPkg
>  W: https://github.com/tianocore/tianocore.github.io/wiki/SecurityPkg
>  M: Chao Zhang 
>  M: Jiewen Yao 
> +M: Jian Wang 
> 
>  ShellBinPkg
>  W: https://github.com/tianocore/tianocore.github.io/wiki/ShellPkg
>  M: Jaben Carsey   (Ia32/X64)
>  M: Ray Ni   (Ia32/X64)
> --
> 2.16.2.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [Patch] Maintainers.txt: Change package maintainer and reviewer of SecurityPkg.

[Zhang, Chao B]

Cc: Yao Jiewen 
Cc: Jian Wang 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Zhang, Chao B 
---
 Maintainers.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Maintainers.txt b/Maintainers.txt
index 7772926b2f..08a676b236 100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -1,6 +1,6 @@
-EDK II Maintainers
+EDK II Maintainers
 ==
 
 This file provides information about the primary maintainers for
 EDK II.
 
@@ -237,10 +237,11 @@ M: Kelly Steele 
 
 SecurityPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/SecurityPkg
 M: Chao Zhang 
 M: Jiewen Yao 
+M: Jian Wang 
 
 ShellBinPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/ShellPkg
 M: Jaben Carsey   (Ia32/X64)
 M: Ray Ni   (Ia32/X64)
-- 
2.16.2.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 1/2] MdeModulePkg/HiiDatabase: Fix potential integer overflow (CVE-2018-12181)

[Wu, Hao A]

Reviewed-by: Hao Wu 

Best Regards,
Hao Wu


> -Original Message-
> From: Ni, Ray
> Sent: Friday, March 08, 2019 10:35 AM
> To: edk2-devel@lists.01.org
> Cc: Bi, Dandan; Wu, Hao A
> Subject: [PATCH v2 1/2] MdeModulePkg/HiiDatabase: Fix potential integer
> overflow (CVE-2018-12181)
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1135
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ray Ni 
> Cc: Dandan Bi 
> Cc: Hao A Wu 
> ---
>  MdeModulePkg/Universal/HiiDatabaseDxe/Image.c | 126
> ++
>  1 file changed, 103 insertions(+), 23 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
> b/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
> index 71ebc559c0..80a4ec1114 100644
> --- a/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
> +++ b/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
> @@ -16,6 +16,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
> 
>  #include "HiiDatabase.h"
> 
> +#define MAX_UINT240xFF
> 
>  /**
>Get the imageid of last image block: EFI_HII_IIBT_END_BLOCK when input
> @@ -651,8 +652,16 @@ HiiNewImage (
> 
>EfiAcquireLock ();
> 
> -  NewBlockSize = sizeof (EFI_HII_IIBT_IMAGE_24BIT_BLOCK) - sizeof
> (EFI_HII_RGB_PIXEL) +
> - BITMAP_LEN_24_BIT ((UINT32) Image->Width, Image->Height);
> +  //
> +  // Calcuate the size of new image.
> +  // Make sure the size doesn't overflow UINT32.
> +  // Note: 24Bit BMP occpuies 3 bytes per pixel.
> +  //
> +  NewBlockSize = (UINT32)Image->Width * Image->Height;
> +  if (NewBlockSize > (MAX_UINT32 - (sizeof
> (EFI_HII_IIBT_IMAGE_24BIT_BLOCK) - sizeof (EFI_HII_RGB_PIXEL))) / 3) {
> +return EFI_OUT_OF_RESOURCES;
> +  }
> +  NewBlockSize = NewBlockSize * 3 + (sizeof
> (EFI_HII_IIBT_IMAGE_24BIT_BLOCK) - sizeof (EFI_HII_RGB_PIXEL));
> 
>//
>// Get the image package in the package list,
> @@ -671,6 +680,18 @@ HiiNewImage (
>  //
>  // Update the package's image block by appending the new block to the
> end.
>  //
> +
> +//
> +// Make sure the final package length doesn't overflow.
> +// Length of the package header is represented using 24 bits. So MAX
> length is MAX_UINT24.
> +//
> +if (NewBlockSize > MAX_UINT24 - ImagePackage-
> >ImagePkgHdr.Header.Length) {
> +  return EFI_OUT_OF_RESOURCES;
> +}
> +//
> +// Because ImagePackage->ImageBlockSize < ImagePackage-
> >ImagePkgHdr.Header.Length,
> +// So (ImagePackage->ImageBlockSize + NewBlockSize) <= MAX_UINT24
> +//
>  ImageBlocks = AllocatePool (ImagePackage->ImageBlockSize +
> NewBlockSize);
>  if (ImageBlocks == NULL) {
>EfiReleaseLock ();
> @@ -701,6 +722,13 @@ HiiNewImage (
>  PackageListNode->PackageListHdr.PackageLength += NewBlockSize;
> 
>} else {
> +//
> +// Make sure the final package length doesn't overflow.
> +// Length of the package header is represented using 24 bits. So MAX
> length is MAX_UINT24.
> +//
> +if (NewBlockSize > MAX_UINT24 - (sizeof
> (EFI_HII_IMAGE_PACKAGE_HDR) + sizeof (EFI_HII_IIBT_END_BLOCK))) {
> +  return EFI_OUT_OF_RESOURCES;
> +}
>  //
>  // The specified package list does not contain image package.
>  // Create one to add this image block.
> @@ -902,8 +930,11 @@ IGetImage (
>  // Use the common block code since the definition of these structures is
> the same.
>  //
>  CopyMem (, CurrentImageBlock, sizeof
> (EFI_HII_IIBT_IMAGE_1BIT_BLOCK));
> -ImageLength = sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL) *
> -  ((UINT32) Iibt1bit.Bitmap.Width * Iibt1bit.Bitmap.Height);
> +ImageLength = (UINTN) Iibt1bit.Bitmap.Width * Iibt1bit.Bitmap.Height;
> +if (ImageLength > MAX_UINTN / sizeof
> (EFI_GRAPHICS_OUTPUT_BLT_PIXEL)) {
> +  return EFI_OUT_OF_RESOURCES;
> +}
> +ImageLength  *= sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL);
>  Image->Bitmap = AllocateZeroPool (ImageLength);
>  if (Image->Bitmap == NULL) {
>return EFI_OUT_OF_RESOURCES;
> @@ -952,9 +983,13 @@ IGetImage (
>  // fall through
>  //
>case EFI_HII_IIBT_IMAGE_24BIT:
> -Width = ReadUnaligned16 ((VOID *)
> &((EFI_HII_IIBT_IMAGE_24BIT_BLOCK *) CurrentImageBlock)-
> >Bitmap.Width);
> +Width  = ReadUnaligned16 ((VOID *)
> &((EFI_HII_IIBT_IMAGE_24BIT_BLOCK *) CurrentImageBlock)-
> >Bitmap.Width);
>  Height = ReadUnaligned16 ((VOID *)
> &((EFI_HII_IIBT_IMAGE_24BIT_BLOCK *) CurrentImageBlock)-
> >Bitmap.Height);
> -ImageLength = sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL) * ((UINT32)
> Width * Height);
> +ImageLength = (UINTN)Width * Height;
> +if (ImageLength > MAX_UINTN / sizeof
> (EFI_GRAPHICS_OUTPUT_BLT_PIXEL)) {
> +  return EFI_OUT_OF_RESOURCES;
> +}
> +ImageLength  *= sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL);
>  Image->Bitmap = AllocateZeroPool (ImageLength);
>  if (Image->Bitmap == NULL) {
>return EFI_OUT_OF_RESOURCES;
> @@ -1124,8 +1159,23 @@ 

Re: [edk2] [PATCH 0/2] Fix bugs in HiiDatabase driver

[Ni, Ray]

Thanks for the comments.
Sent out V2 with correct patch subject.

> -Original Message-
> From: Wu, Hao A
> Sent: Friday, March 8, 2019 10:22 AM
> To: Ni, Ray ; edk2-devel@lists.01.org
> Subject: RE: [edk2] [PATCH 0/2] Fix bugs in HiiDatabase driver
> 
> Quick comment, please add the CVE number in the patch subject.
> 
> Liming has already documented the new rule for this kind of fix:
> https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-
> Format
> 
> Best Regards,
> Hao Wu
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > Ray Ni
> > Sent: Friday, March 08, 2019 10:21 AM
> > To: edk2-devel@lists.01.org
> > Subject: [edk2] [PATCH 0/2] Fix bugs in HiiDatabase driver
> >
> > Ray Ni (2):
> >   MdeModulePkg/HiiDatabase: Fix potential integer overflow
> >   MdeModulePkg/HiiImage: Fix stack overflow when corrupted BMP is
> > parsed
> >
> >  MdeModulePkg/Universal/HiiDatabaseDxe/Image.c | 130
> > ++
> >  1 file changed, 105 insertions(+), 25 deletions(-)
> >
> > --
> > 2.20.1.windows.1
> >
> > ___
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH v2 0/2] Fix bugs in HiiDatabase driver

[Ray Ni]

v2: put the CVE number in patch title.

Ray Ni (2):
  MdeModulePkg/HiiDatabase: Fix potential integer overflow
(CVE-2018-12181)
  MdeModulePkg/HiiImage: Fix stack overflow when corrupted BMP is parsed
(CVE-2018-12181)

 MdeModulePkg/Universal/HiiDatabaseDxe/Image.c | 130 ++
 1 file changed, 105 insertions(+), 25 deletions(-)

-- 
2.20.1.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH v2 2/2] MdeModulePkg/HiiImage: Fix stack overflow when corrupted BMP is parsed (CVE-2018-12181)

[Ray Ni]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1135

For 4bit BMP, there are only 2^4 = 16 colors in the palette.
But when a corrupted BMP contains more than 16 colors in the palette,
today's implementation wrongly copies all colors to the local
PaletteValue[16] array which causes stack overflow.

The similar issue also exists in the logic to handle 8bit BMP.

The patch fixes the issue by only copies the first 16 or 256 colors
in the palette depending on the BMP type.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ray Ni 
Cc: Liming Gao 
Cc: Jiewen Yao 
---
 MdeModulePkg/Universal/HiiDatabaseDxe/Image.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c 
b/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
index 80a4ec1114..8532f272eb 100644
--- a/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
+++ b/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
@@ -370,7 +370,7 @@ Output4bitPixel (
   PaletteNum = (UINT16)(Palette->PaletteSize / sizeof (EFI_HII_RGB_PIXEL));
 
   ZeroMem (PaletteValue, sizeof (PaletteValue));
-  CopyRgbToGopPixel (PaletteValue, Palette->PaletteValue, PaletteNum);
+  CopyRgbToGopPixel (PaletteValue, Palette->PaletteValue, MIN (PaletteNum, 
ARRAY_SIZE (PaletteValue)));
   FreePool (Palette);
 
   //
@@ -447,7 +447,7 @@ Output8bitPixel (
   CopyMem (Palette, PaletteInfo, PaletteSize);
   PaletteNum = (UINT16)(Palette->PaletteSize / sizeof (EFI_HII_RGB_PIXEL));
   ZeroMem (PaletteValue, sizeof (PaletteValue));
-  CopyRgbToGopPixel (PaletteValue, Palette->PaletteValue, PaletteNum);
+  CopyRgbToGopPixel (PaletteValue, Palette->PaletteValue, MIN (PaletteNum, 
ARRAY_SIZE (PaletteValue)));
   FreePool (Palette);
 
   //
-- 
2.20.1.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH v2 1/2] MdeModulePkg/HiiDatabase: Fix potential integer overflow (CVE-2018-12181)

[Ray Ni]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1135

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ray Ni 
Cc: Dandan Bi 
Cc: Hao A Wu 
---
 MdeModulePkg/Universal/HiiDatabaseDxe/Image.c | 126 ++
 1 file changed, 103 insertions(+), 23 deletions(-)

diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c 
b/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
index 71ebc559c0..80a4ec1114 100644
--- a/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
+++ b/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
@@ -16,6 +16,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 
 #include "HiiDatabase.h"
 
+#define MAX_UINT240xFF
 
 /**
   Get the imageid of last image block: EFI_HII_IIBT_END_BLOCK when input
@@ -651,8 +652,16 @@ HiiNewImage (
 
   EfiAcquireLock ();
 
-  NewBlockSize = sizeof (EFI_HII_IIBT_IMAGE_24BIT_BLOCK) - sizeof 
(EFI_HII_RGB_PIXEL) +
- BITMAP_LEN_24_BIT ((UINT32) Image->Width, Image->Height);
+  //
+  // Calcuate the size of new image.
+  // Make sure the size doesn't overflow UINT32.
+  // Note: 24Bit BMP occpuies 3 bytes per pixel.
+  //
+  NewBlockSize = (UINT32)Image->Width * Image->Height;
+  if (NewBlockSize > (MAX_UINT32 - (sizeof (EFI_HII_IIBT_IMAGE_24BIT_BLOCK) - 
sizeof (EFI_HII_RGB_PIXEL))) / 3) {
+return EFI_OUT_OF_RESOURCES;
+  }
+  NewBlockSize = NewBlockSize * 3 + (sizeof (EFI_HII_IIBT_IMAGE_24BIT_BLOCK) - 
sizeof (EFI_HII_RGB_PIXEL));
 
   //
   // Get the image package in the package list,
@@ -671,6 +680,18 @@ HiiNewImage (
 //
 // Update the package's image block by appending the new block to the end.
 //
+
+//
+// Make sure the final package length doesn't overflow.
+// Length of the package header is represented using 24 bits. So MAX 
length is MAX_UINT24.
+//
+if (NewBlockSize > MAX_UINT24 - ImagePackage->ImagePkgHdr.Header.Length) {
+  return EFI_OUT_OF_RESOURCES;
+}
+//
+// Because ImagePackage->ImageBlockSize < 
ImagePackage->ImagePkgHdr.Header.Length,
+// So (ImagePackage->ImageBlockSize + NewBlockSize) <= MAX_UINT24
+//
 ImageBlocks = AllocatePool (ImagePackage->ImageBlockSize + NewBlockSize);
 if (ImageBlocks == NULL) {
   EfiReleaseLock ();
@@ -701,6 +722,13 @@ HiiNewImage (
 PackageListNode->PackageListHdr.PackageLength += NewBlockSize;
 
   } else {
+//
+// Make sure the final package length doesn't overflow.
+// Length of the package header is represented using 24 bits. So MAX 
length is MAX_UINT24.
+//
+if (NewBlockSize > MAX_UINT24 - (sizeof (EFI_HII_IMAGE_PACKAGE_HDR) + 
sizeof (EFI_HII_IIBT_END_BLOCK))) {
+  return EFI_OUT_OF_RESOURCES;
+}
 //
 // The specified package list does not contain image package.
 // Create one to add this image block.
@@ -902,8 +930,11 @@ IGetImage (
 // Use the common block code since the definition of these structures is 
the same.
 //
 CopyMem (, CurrentImageBlock, sizeof 
(EFI_HII_IIBT_IMAGE_1BIT_BLOCK));
-ImageLength = sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL) *
-  ((UINT32) Iibt1bit.Bitmap.Width * Iibt1bit.Bitmap.Height);
+ImageLength = (UINTN) Iibt1bit.Bitmap.Width * Iibt1bit.Bitmap.Height;
+if (ImageLength > MAX_UINTN / sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL)) {
+  return EFI_OUT_OF_RESOURCES;
+}
+ImageLength  *= sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL);
 Image->Bitmap = AllocateZeroPool (ImageLength);
 if (Image->Bitmap == NULL) {
   return EFI_OUT_OF_RESOURCES;
@@ -952,9 +983,13 @@ IGetImage (
 // fall through
 //
   case EFI_HII_IIBT_IMAGE_24BIT:
-Width = ReadUnaligned16 ((VOID *) &((EFI_HII_IIBT_IMAGE_24BIT_BLOCK *) 
CurrentImageBlock)->Bitmap.Width);
+Width  = ReadUnaligned16 ((VOID *) &((EFI_HII_IIBT_IMAGE_24BIT_BLOCK *) 
CurrentImageBlock)->Bitmap.Width);
 Height = ReadUnaligned16 ((VOID *) &((EFI_HII_IIBT_IMAGE_24BIT_BLOCK *) 
CurrentImageBlock)->Bitmap.Height);
-ImageLength = sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL) * ((UINT32) Width * 
Height);
+ImageLength = (UINTN)Width * Height;
+if (ImageLength > MAX_UINTN / sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL)) {
+  return EFI_OUT_OF_RESOURCES;
+}
+ImageLength  *= sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL);
 Image->Bitmap = AllocateZeroPool (ImageLength);
 if (Image->Bitmap == NULL) {
   return EFI_OUT_OF_RESOURCES;
@@ -1124,8 +1159,23 @@ HiiSetImage (
   //
   // Create the new image block according to input image.
   //
-  NewBlockSize = sizeof (EFI_HII_IIBT_IMAGE_24BIT_BLOCK) - sizeof 
(EFI_HII_RGB_PIXEL) +
- BITMAP_LEN_24_BIT ((UINT32) Image->Width, Image->Height);
+
+  //
+  // Make sure the final package length doesn't overflow.
+  // Length of the package header is represented using 24 bits. So MAX length 
is MAX_UINT24.
+  // 24Bit BMP occpuies 3 bytes per pixel.
+  //
+  NewBlockSize = (UINT32)Image->Width * Image->Height;
+  if 

Re: [edk2] [PATCH 0/2] Fix bugs in HiiDatabase driver

[Wu, Hao A]

Quick comment, please add the CVE number in the patch subject.

Liming has already documented the new rule for this kind of fix:
https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format

Best Regards,
Hao Wu

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Ray Ni
> Sent: Friday, March 08, 2019 10:21 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH 0/2] Fix bugs in HiiDatabase driver
> 
> Ray Ni (2):
>   MdeModulePkg/HiiDatabase: Fix potential integer overflow
>   MdeModulePkg/HiiImage: Fix stack overflow when corrupted BMP is
> parsed
> 
>  MdeModulePkg/Universal/HiiDatabaseDxe/Image.c | 130
> ++
>  1 file changed, 105 insertions(+), 25 deletions(-)
> 
> --
> 2.20.1.windows.1
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/2] Fix bugs in HiiDatabase driver

[Gao, Liming]

Please follow CVE format in 
https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ray Ni
> Sent: Thursday, March 7, 2019 6:21 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH 0/2] Fix bugs in HiiDatabase driver
> 
> Ray Ni (2):
>   MdeModulePkg/HiiDatabase: Fix potential integer overflow
>   MdeModulePkg/HiiImage: Fix stack overflow when corrupted BMP is parsed
> 
>  MdeModulePkg/Universal/HiiDatabaseDxe/Image.c | 130 ++
>  1 file changed, 105 insertions(+), 25 deletions(-)
> 
> --
> 2.20.1.windows.1
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH 0/2] Fix bugs in HiiDatabase driver

[Ray Ni]

Ray Ni (2):
  MdeModulePkg/HiiDatabase: Fix potential integer overflow
  MdeModulePkg/HiiImage: Fix stack overflow when corrupted BMP is parsed

 MdeModulePkg/Universal/HiiDatabaseDxe/Image.c | 130 ++
 1 file changed, 105 insertions(+), 25 deletions(-)

-- 
2.20.1.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH 1/2] MdeModulePkg/HiiDatabase: Fix potential integer overflow

[Ray Ni]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1135
CVE number: CVE-2018-12181

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ray Ni 
Cc: Dandan Bi 
Cc: Hao A Wu 
---
 MdeModulePkg/Universal/HiiDatabaseDxe/Image.c | 126 ++
 1 file changed, 103 insertions(+), 23 deletions(-)

diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c 
b/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
index 71ebc559c0..80a4ec1114 100644
--- a/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
+++ b/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
@@ -16,6 +16,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 
 #include "HiiDatabase.h"
 
+#define MAX_UINT240xFF
 
 /**
   Get the imageid of last image block: EFI_HII_IIBT_END_BLOCK when input
@@ -651,8 +652,16 @@ HiiNewImage (
 
   EfiAcquireLock ();
 
-  NewBlockSize = sizeof (EFI_HII_IIBT_IMAGE_24BIT_BLOCK) - sizeof 
(EFI_HII_RGB_PIXEL) +
- BITMAP_LEN_24_BIT ((UINT32) Image->Width, Image->Height);
+  //
+  // Calcuate the size of new image.
+  // Make sure the size doesn't overflow UINT32.
+  // Note: 24Bit BMP occpuies 3 bytes per pixel.
+  //
+  NewBlockSize = (UINT32)Image->Width * Image->Height;
+  if (NewBlockSize > (MAX_UINT32 - (sizeof (EFI_HII_IIBT_IMAGE_24BIT_BLOCK) - 
sizeof (EFI_HII_RGB_PIXEL))) / 3) {
+return EFI_OUT_OF_RESOURCES;
+  }
+  NewBlockSize = NewBlockSize * 3 + (sizeof (EFI_HII_IIBT_IMAGE_24BIT_BLOCK) - 
sizeof (EFI_HII_RGB_PIXEL));
 
   //
   // Get the image package in the package list,
@@ -671,6 +680,18 @@ HiiNewImage (
 //
 // Update the package's image block by appending the new block to the end.
 //
+
+//
+// Make sure the final package length doesn't overflow.
+// Length of the package header is represented using 24 bits. So MAX 
length is MAX_UINT24.
+//
+if (NewBlockSize > MAX_UINT24 - ImagePackage->ImagePkgHdr.Header.Length) {
+  return EFI_OUT_OF_RESOURCES;
+}
+//
+// Because ImagePackage->ImageBlockSize < 
ImagePackage->ImagePkgHdr.Header.Length,
+// So (ImagePackage->ImageBlockSize + NewBlockSize) <= MAX_UINT24
+//
 ImageBlocks = AllocatePool (ImagePackage->ImageBlockSize + NewBlockSize);
 if (ImageBlocks == NULL) {
   EfiReleaseLock ();
@@ -701,6 +722,13 @@ HiiNewImage (
 PackageListNode->PackageListHdr.PackageLength += NewBlockSize;
 
   } else {
+//
+// Make sure the final package length doesn't overflow.
+// Length of the package header is represented using 24 bits. So MAX 
length is MAX_UINT24.
+//
+if (NewBlockSize > MAX_UINT24 - (sizeof (EFI_HII_IMAGE_PACKAGE_HDR) + 
sizeof (EFI_HII_IIBT_END_BLOCK))) {
+  return EFI_OUT_OF_RESOURCES;
+}
 //
 // The specified package list does not contain image package.
 // Create one to add this image block.
@@ -902,8 +930,11 @@ IGetImage (
 // Use the common block code since the definition of these structures is 
the same.
 //
 CopyMem (, CurrentImageBlock, sizeof 
(EFI_HII_IIBT_IMAGE_1BIT_BLOCK));
-ImageLength = sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL) *
-  ((UINT32) Iibt1bit.Bitmap.Width * Iibt1bit.Bitmap.Height);
+ImageLength = (UINTN) Iibt1bit.Bitmap.Width * Iibt1bit.Bitmap.Height;
+if (ImageLength > MAX_UINTN / sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL)) {
+  return EFI_OUT_OF_RESOURCES;
+}
+ImageLength  *= sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL);
 Image->Bitmap = AllocateZeroPool (ImageLength);
 if (Image->Bitmap == NULL) {
   return EFI_OUT_OF_RESOURCES;
@@ -952,9 +983,13 @@ IGetImage (
 // fall through
 //
   case EFI_HII_IIBT_IMAGE_24BIT:
-Width = ReadUnaligned16 ((VOID *) &((EFI_HII_IIBT_IMAGE_24BIT_BLOCK *) 
CurrentImageBlock)->Bitmap.Width);
+Width  = ReadUnaligned16 ((VOID *) &((EFI_HII_IIBT_IMAGE_24BIT_BLOCK *) 
CurrentImageBlock)->Bitmap.Width);
 Height = ReadUnaligned16 ((VOID *) &((EFI_HII_IIBT_IMAGE_24BIT_BLOCK *) 
CurrentImageBlock)->Bitmap.Height);
-ImageLength = sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL) * ((UINT32) Width * 
Height);
+ImageLength = (UINTN)Width * Height;
+if (ImageLength > MAX_UINTN / sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL)) {
+  return EFI_OUT_OF_RESOURCES;
+}
+ImageLength  *= sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL);
 Image->Bitmap = AllocateZeroPool (ImageLength);
 if (Image->Bitmap == NULL) {
   return EFI_OUT_OF_RESOURCES;
@@ -1124,8 +1159,23 @@ HiiSetImage (
   //
   // Create the new image block according to input image.
   //
-  NewBlockSize = sizeof (EFI_HII_IIBT_IMAGE_24BIT_BLOCK) - sizeof 
(EFI_HII_RGB_PIXEL) +
- BITMAP_LEN_24_BIT ((UINT32) Image->Width, Image->Height);
+
+  //
+  // Make sure the final package length doesn't overflow.
+  // Length of the package header is represented using 24 bits. So MAX length 
is MAX_UINT24.
+  // 24Bit BMP occpuies 3 bytes per pixel.
+  //
+  NewBlockSize = (UINT32)Image->Width * 

[edk2] [PATCH 2/2] MdeModulePkg/HiiImage: Fix stack overflow when corrupted BMP is parsed

[Ray Ni]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1135
CVE number: CVE-2018-12181

For 4bit BMP, there are only 2^4 = 16 colors in the palette.
But when a corrupted BMP contains more than 16 colors in the palette,
today's implementation wrongly copies all colors to the local
PaletteValue[16] array which causes stack overflow.

The similar issue also exists in the logic to handle 8bit BMP.

The patch fixes the issue by only copies the first 16 or 256 colors
in the palette depending on the BMP type.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ray Ni 
Cc: Liming Gao 
Cc: Jiewen Yao 
---
 MdeModulePkg/Universal/HiiDatabaseDxe/Image.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c 
b/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
index 80a4ec1114..8532f272eb 100644
--- a/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
+++ b/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
@@ -370,7 +370,7 @@ Output4bitPixel (
   PaletteNum = (UINT16)(Palette->PaletteSize / sizeof (EFI_HII_RGB_PIXEL));
 
   ZeroMem (PaletteValue, sizeof (PaletteValue));
-  CopyRgbToGopPixel (PaletteValue, Palette->PaletteValue, PaletteNum);
+  CopyRgbToGopPixel (PaletteValue, Palette->PaletteValue, MIN (PaletteNum, 
ARRAY_SIZE (PaletteValue)));
   FreePool (Palette);
 
   //
@@ -447,7 +447,7 @@ Output8bitPixel (
   CopyMem (Palette, PaletteInfo, PaletteSize);
   PaletteNum = (UINT16)(Palette->PaletteSize / sizeof (EFI_HII_RGB_PIXEL));
   ZeroMem (PaletteValue, sizeof (PaletteValue));
-  CopyRgbToGopPixel (PaletteValue, Palette->PaletteValue, PaletteNum);
+  CopyRgbToGopPixel (PaletteValue, Palette->PaletteValue, MIN (PaletteNum, 
ARRAY_SIZE (PaletteValue)));
   FreePool (Palette);
 
   //
-- 
2.20.1.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] StdLib: Update resolv.conf to use Google's public DNS servers

[Rebecca Cran via edk2-devel]

On 2/26/19 5:31 AM, Laszlo Ersek wrote:



Sorry for missing this earlier, but the patch misses the
Contributed-under and Signed-off-by lines, from the end of the commit
message.

Please repost like that, and then please also include the Reviewed-by
tags from Jaben and myself.



I'm just catching up on edk2 work again. I've just sent out a new patch.


--
Rebecca Cran

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH] StdLib: Update resolv.conf to use Google's DNS servers

[Rebecca Cran via edk2-devel]

The current servers listed appear to be unusable. I suspect most
people will get correct DNS servers via DHCP, but the defaults
should work for anyone.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Rebecca Cran 
Reviewed-by: Jaben Carsey 
Reviewed-by: Laszlo Ersek 
---
 StdLib/Efi/StdLib/etc/resolv.conf | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/StdLib/Efi/StdLib/etc/resolv.conf 
b/StdLib/Efi/StdLib/etc/resolv.conf
index 3ac16ac230..724e6297b2 100644
--- a/StdLib/Efi/StdLib/etc/resolv.conf
+++ b/StdLib/Efi/StdLib/etc/resolv.conf
@@ -1,13 +1,13 @@
 #
 #   Domain name
 #
-domain  intel.com
+domain  example.com
 
 ;
 ;   Name Servers
 ;
-nameserver  206.63.63.61
-nameserver  216.251.100.1
+nameserver  8.8.8.8
+nameserver  8.8.4.4
 
 ; nameserver  10.248.2.1
 ; nameserver  10.22.224.204
-- 
2.21.0

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [RFC] Change EDK II to BSD+Patent License

[Doran, Mark]

Hi Kevin:

I'm not a lawyer and even if I were I couldn't give you legal advice of course.

That said, I believe the intent of the BSD+patent license is well stated in the 
note that is included immediately above the actual rendition of terms on this 
page here: https://opensource.org/licenses/BSDplusPatent

That note says:

"Note: This license is designed to provide: a) a simple permissive license; b) 
that is compatible with the GNU General Public License (GPL), version 2; and c) 
which also has an express patent grant included." 

I'll re-iterate what I said before in making the original proposal: the intent 
is to make sure that the code in the project continues to have permissive terms 
for users and that users of the code need not have a concern about any 
potential for IP infringement as a result of using any Contributions that are 
made part of the project.
--
Cheers,

Mark.


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Kevin D Davis
> Sent: Tuesday, March 5, 2019 12:19 PM
> To: Kinney, Michael D ; edk2-devel@lists.01.org
> Subject: Re: [edk2] [RFC] Change EDK II to BSD+Patent License
> 
> 
> 
> 
>   Mr. Kinney,
> Wow.  Of all the many licenses I’ve read, this one takes the cake at having
> the highest confusion to words ratio for my reading comprehension
> level.  I’ll admit my level is a lot lower than some on this reflector.
> Maybe if I knew the intent of this license when reading it I would find it
> clear.  Is there an opinion about the intentions around these two
> questions?
> A) am I granting patent rights if I add patentable/patented code?
> B) do I need to get a patent license from all of the copyright holders to
> use this code for technology covered by their code?
> 
> 
>   Thanks,Kevin
> 
> 
> 
> 
> 
> On Tue, Mar 5, 2019 at 1:10 PM -0600, "Kinney, Michael D"
>  wrote:
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> Hello,
> 
> This RFC follows up on the proposal from Mark Doran to change the
> EDK II Project to a BSD+Patent License.
> 
>   https://lists.01.org/pipermail/edk2-devel/2019-February/036260.html
> 
> The review period for this license change is 30 days.  If there is no
> unresolved feedback on April 9, 2019, then commits of the license change
> patches will begin on April 9, 2019.
> 
>   ** Please provide feedback on the proposal by Monday April 8, 2019. **
> 
> Feedback can be sent to edk2-devel@lists.01.org, the EDK II community
> manager or any of the EDK II stewards.
> 
>   * Stephano CetolaCommunity Manager
>   * Leif Lindholm   Steward
>   * Andrew Fish  Steward
>   * Laszlo Ersek   Steward
>   * Michael KinneySteward
> 
> The goal is to convert all of the files in the edk2 repository that are
> currently covered by the BSD 2-Clause License and the TianoCore
> Contribution Agreement to a BSD+Patent License.
> 
> I will be following up with pointers to public GitHub branches that
> contain the set of changes to the edk2 repository for review.
> 
> The proposal is to perform this change to edk2/master in the steps listed
> below. The license change will not be applied to any of the other existing
> branches in the edk2 repository.
> 
> 1) Add a License-History.txt file to the root of the edk2 repository that
>contains the BSD 2-Clause License and the TianoCore Contribution
>Agreement along with the details on the license change to BSD+Patent.
> 
> 2) Change all files currently covered by a BSD 2-Clause license and the
>TianoCore Contribution Agreement to a BSD+Patent license and add an
>SPDX-License-Identifier statement.  The link to the BSD+Patent license
>and the text for file headers is listed below.
> 
>https://opensource.org/licenses/BSDplusPatent
> 
>==
>SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>Redistribution and use in source and binary forms, with or without
>modification, are permitted provided that the following conditions are
> met:
> 
>1. Redistributions of source code must retain the above copyright
> notice,
>   this list of conditions and the following disclaimer.
> 
>2. Redistributions in binary form must reproduce the above copyright
> notice,
>   this list of conditions and the following disclaimer in the
> documentation
>   and/or other materials provided with the distribution.
> 
>Subject to the terms and conditions of this license, each copyright
> holder
>and contributor hereby grants to those receiving rights under this
> license
>a perpetual, worldwide, non-exclusive, no-charge, royalty-free,
> irrevocable
>(except for failure to satisfy the conditions of this license) patent
>license to make, have made, use, offer to sell, sell, import, and
> otherwise
>transfer this software, where such license applies only to those patent
>claims, already acquired or hereafter acquired, licensable by 

[edk2] UefiCpuPkg CpuDxe GDT init question?

[Andrew Fish via edk2-devel]

I'm trying to understand why gdtPtr.Base is casting to (UINT32)? 
1) gdtPtr.Base is a a UINTN
2) It is legal for AllocateRuntimePool() to return an address > 4GB

It seems like the code should just cast to (UINTN)?


https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/CpuDxe/CpuGdt.c#L151



VOID
InitGlobalDescriptorTable (
  VOID
  )
{
  GDT_ENTRIES *gdt;
  IA32_DESCRIPTOR gdtPtr;

  //
  // Allocate Runtime Data for the GDT
  //
  gdt = AllocateRuntimePool (sizeof (GdtTemplate) + 8);
  ASSERT (gdt != NULL);
  gdt = ALIGN_POINTER (gdt, 8);

  //
  // Initialize all GDT entries
  //
  CopyMem (gdt, , sizeof (GdtTemplate));

  //
  // Write GDT register
  //
  gdtPtr.Base = (UINT32)(UINTN)(VOID*) gdt;
  gdtPtr.Limit = (UINT16) (sizeof (GdtTemplate) - 1);
  AsmWriteGdtr ();

Thanks,

Andrew Fish
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [edk2-test][Patch 1/1] uefi-sct/SctPkg:Fix flaw in BBTestCreateEventEx_Func_Sub3

[Supreeth Venkatesh]

On Thu, 2019-03-07 at 09:23 +0800, Eric Jin wrote:
> The intention of test is to validate the signal sequence among
> three events with gEfiEventMemoryMapChangeGuid and different
> Tpl. The call of AllocatePages() causes memorymap change and
> trigger event Notify.
> But the test has an assumption that CreateEventEx() will not
> cause memorymap change itself, which is not true. When memory
> pool is out of resource, the memory service will be called to
> reserve more memory pool.
> The fix is to filter the exception and only leave memorymap
> change caused by AllocatePages() in test.
> 
> Cc: Supreeth Venkatesh 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Jin 


Minor comment about parenthesis to make multiple conditions priority
more readable. Please fix this. With that
Reviewed-by: Supreeth Venkatesh 

> ---
>  uefi-
> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServi
> ces/BlackBoxTest/EventTimerTaskPriorityServicesBBTestMain.h  
> |  5 +++--
>  uefi-
> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServi
> ces/BlackBoxTest/EventTimerTaskPriorityServicesBBTestCreateEventEx.c
> | 26 +++---
>  uefi-
> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServi
> ces/BlackBoxTest/Support.c   
> | 23 +--
>  3 files changed, 39 insertions(+), 15 deletions(-)
> 
> diff --git a/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServi
> ces/BlackBoxTest/EventTimerTaskPriorityServicesBBTestMain.h b/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServi
> ces/BlackBoxTest/EventTimerTaskPriorityServicesBBTestMain.h
> index 4431b5b22888..87451f9f9a91 100644
> --- a/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServi
> ces/BlackBoxTest/EventTimerTaskPriorityServicesBBTestMain.h
> +++ b/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServi
> ces/BlackBoxTest/EventTimerTaskPriorityServicesBBTestMain.h
> @@ -1,7 +1,7 @@
>  /** @file
>  
>Copyright 2006 - 2016 Unified EFI, Inc.
> -  Copyright (c) 2010 - 2016, Intel Corporation. All rights
> reserved.
> +  Copyright (c) 2010 - 2019, Intel Corporation. All rights
> reserved.
>  
>This program and the accompanying materials
>are licensed and made available under the terms and conditions of
> the BSD License
> @@ -49,7 +49,8 @@ Abstract:
>   Status \
>   );
>  
> -#define MAX_TEST_EVENT_NUM 3
> +#define MAX_TEST_EVENT_NUM3
> +#define SIGNAL_CONTEXT0xAA//To check the buffer content
> is modifed by exception or not
>  //
>  // Prototypes: Test Cases
>  //
> diff --git a/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServi
> ces/BlackBoxTest/EventTimerTaskPriorityServicesBBTestCreateEventEx.c
> b/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServi
> ces/BlackBoxTest/EventTimerTaskPriorityServicesBBTestCreateEventEx.c
> index e2e173ab04c0..4a8e44e29b4e 100644
> --- a/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServi
> ces/BlackBoxTest/EventTimerTaskPriorityServicesBBTestCreateEventEx.c
> +++ b/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServi
> ces/BlackBoxTest/EventTimerTaskPriorityServicesBBTestCreateEventEx.c
> @@ -1,7 +1,7 @@
>  /** @file
>  
>Copyright 2006 - 2016 Unified EFI, Inc.
> -  Copyright (c) 2010 - 2016, Intel Corporation. All rights
> reserved.
> +  Copyright (c) 2010 - 2019, Intel Corporation. All rights
> reserved.
>  
>This program and the accompanying materials
>are licensed and made available under the terms and conditions of
> the BSD License
> @@ -192,6 +192,10 @@ BBTestCreateEventEx_Func (
>BBTestCreateEventEx_Func_Sub2 (StandardLib);
>  #endif
>  
> +  //
> +  // The test for the EFI_EVENT_GROUP_MEMORY_MAP_CHANGE
> +  // This event group is notified by the system when the memory map
> has changed.
> +  //
>BBTestCreateEventEx_Func_Sub3 (StandardLib);
>  
>//
> @@ -599,12 +603,12 @@ BBTestCreateEventEx_Func_Sub1 (
>UINTN   Buffer[MAX_TEST_EVENT_NUM +
> MAX_TEST_EVENT_NUM*2];
>  
>//
> -  // Initialize Buffer
> +  // Initialize Buffer as SIGNAL_CONTEXT
>//
>for (Index = 0; Index < MAX_TEST_EVENT_NUM; Index ++) {
>  Buffer[Index] = Index;
> -Buffer[Index + MAX_TEST_EVENT_NUM + Index] = (UINTN)(-1);
> -Buffer[Index + MAX_TEST_EVENT_NUM + 1 + Index] = (UINTN)(-1);
> +Buffer[Index + MAX_TEST_EVENT_NUM + Index] =
> (UINTN)(SIGNAL_CONTEXT);
> +Buffer[Index + MAX_TEST_EVENT_NUM + 1 + Index] =
> (UINTN)(SIGNAL_CONTEXT);
>}
>  
>//
> @@ -755,12 +759,12 @@ BBTestCreateEventEx_Func_Sub2 (
>UINTN   Buffer[MAX_TEST_EVENT_NUM +
> MAX_TEST_EVENT_NUM*2];
>  
>//
> -  // Initialize Buffer
> +  // Initialize Buffer 

Re: [edk2] [edk2-test][Patch 1/1] uefi-sct/SctPkg:update BlueTooth test with LE support

[Supreeth Venkatesh]

On Thu, 2019-03-07 at 15:05 +0800, Eric Jin wrote:
> revisit BlueTooth checkpoint for UEFI2.7 spec update
> with LE support
> Change original test to classic/low energy checkpoints
> Add marco MAX_LENGTH for pre-defined string value
> (yes or no) in EfiCompliant.ini.
> 
> Cc: Supreeth Venkatesh 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Jin 


There are few unintended alignment edits on conditional statements and
other minor alignment edits needed. Comments inline.

Please fix those, before check-in. With that
Reviewed-by: Supreeth Venkatesh 

> ---
>  uefi-
> sct/SctPkg/TestCase/UEFI/EFI/Generic/EfiCompliant/BlackBoxTest/Guid_u
> efi.h|   7 ++-
>  uefi-
> sct/SctPkg/TestCase/UEFI/EFI/Generic/EfiCompliant/BlackBoxTest/EfiCom
> pliantBBTestPlatform_uefi.c  | 377
> +
> +
> +
> ++---
> ---
> --
>  uefi-
> sct/SctPkg/TestCase/UEFI/EFI/Generic/EfiCompliant/BlackBoxTest/Guid_u
> efi.c|   4 +++-
>  uefi-
> sct/SctPkg/TestCase/UEFI/EFI/Generic/EfiCompliant/BlackBoxTest/Depend
> ency/Config/EfiCompliant.ini |   9 ++---
>  4 files changed, 240 insertions(+), 157 deletions(-)
> 
> diff --git a/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/Generic/EfiCompliant/BlackBoxTest/Guid_u
> efi.h b/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/Generic/EfiCompliant/BlackBoxTest/Guid_u
> efi.h
> index b6e1f3797462..158231c46d37 100644
> --- a/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/Generic/EfiCompliant/BlackBoxTest/Guid_u
> efi.h
> +++ b/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/Generic/EfiCompliant/BlackBoxTest/Guid_u
> efi.h
> @@ -1,7 +1,7 @@
>  /** @file
>  
>Copyright 2006 - 2016 Unified EFI, Inc.
> -  Copyright (c) 2010 - 2016, Intel Corporation. All rights
> reserved.
> +  Copyright (c) 2010 - 2019, Intel Corporation. All rights
> reserved.
>  
>This program and the accompanying materials
>are licensed and made available under the terms and conditions of
> the BSD License
> @@ -160,6 +160,11 @@ extern EFI_GUID
> gEfiCompliantBbTestPlatformAssertionGuid026;
>  
>  extern EFI_GUID gEfiCompliantBbTestPlatformAssertionGuid027;
>  
> +#define EFI_TEST_EFICOMPLIANTBBTESTPLATFORM_ASSERTION_028_GUID \
> +{ 0xeff461eb, 0x4f56, 0x44a5, { 0x89, 0x5e, 0xee, 0x5e, 0xe4, 0x2a,
> 0xd3, 0x9 }}
> +
> +extern EFI_GUID gEfiCompliantBbTestPlatformAssertionGuid028;
> +
>  #define EFI_TEST_EFICOMPLIANTBBTESTREQUIRED_ASSERTION_001_GUID \
>  { 0xf6a871e3, 0xef8a, 0x420f, {0x82, 0x01, 0x35, 0xb6, 0x1c, 0xe2,
> 0xe8, 0xdb }}
>  
> diff --git a/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/Generic/EfiCompliant/BlackBoxTest/EfiCom
> pliantBBTestPlatform_uefi.c b/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/Generic/EfiCompliant/BlackBoxTest/EfiCom
> pliantBBTestPlatform_uefi.c
> index 186e44bfb1ed..b3f1b8c557d2 100644
> --- a/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/Generic/EfiCompliant/BlackBoxTest/EfiCom
> pliantBBTestPlatform_uefi.c
> +++ b/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/Generic/EfiCompliant/BlackBoxTest/EfiCom
> pliantBBTestPlatform_uefi.c
> @@ -1,7 +1,7 @@
>  /** @file
>  
>Copyright 2006 - 2016 Unified EFI, Inc.
> -  Copyright (c) 2010 - 2018, Intel Corporation. All rights
> reserved.
> +  Copyright (c) 2010 - 2019, Intel Corporation. All rights
> reserved.
>  
>This program and the accompanying materials
>are licensed and made available under the terms and conditions of
> the BSD License
> @@ -166,6 +166,17 @@ EFI_GUID gEfiIPSecConfigProtocolGuid = {
> 0xce5e5929, 0xc7a3, 0x4602, {0xad, 0x9e
>  
>  EFI_GUID gEfiIPSec2ProtocolGuid = { 0xa3979e64, 0xace8, 0x4ddc,
> {0xbc, 0x07, 0x4d, 0x66, 0xb8, 0xfd, 0x09, 0x77 }};
>  
> +EFI_GUID gEfiBlueToothAttributeProtocolGuid = { 0x898890e9, 0x84b2,
> 0x4f3a, { 0x8c, 0x58, 0xd8, 0x57, 0x78, 0x13, 0xe0, 0xac }};
> +
> +EFI_GUID gEfiBlueToothLEConfigProtocolGuid = { 0x8f76da58, 0x1f99,
> 0x4275, { 0xa4, 0xec, 0x47, 0x56, 0x51, 0x5b, 0x1c, 0xe8 }};
> +
> +//
> +// The Max length of pre-defined string value(yes or no)
> +// in the EfiCompliant.ini
> +// which is the platform specific configuration
> +//
> +#define MAX_LENGTH  10
> +
>  //
>  // Internal functions declarations
>  //
> @@ -353,7 +364,13 @@ CheckEAPProtocols (
>);
>  
>  EFI_STATUS
> -CheckBlueToothProtocols (
> +CheckBlueToothClassicProtocols (
> +  IN EFI_STANDARD_TEST_LIBRARY_PROTOCOL   *StandardLib,
> +  IN EFI_INI_FILE_HANDLE  IniFile
> +  );
> +
> +EFI_STATUS
> +CheckBlueToothLEProtocols (
>IN EFI_STANDARD_TEST_LIBRARY_PROTOCOL   *StandardLib,
>IN EFI_INI_FILE_HANDLE  IniFile
>);
> @@ -564,7 +581,8 @@ Routine Description:
>//
>// Check the BlueTooth 

Re: [edk2] [PATCH v2] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM

[Kinney, Michael D]

Laszlo,

The information I provided below is incorrect.  The PCD referenced
does support all PCD types as Jiewen noted.

Mike

> -Original Message-
> From: Kinney, Michael D
> Sent: Thursday, March 7, 2019 10:10 AM
> To: Laszlo Ersek ; Vanguput,
> Narendra K ; edk2-
> de...@lists.01.org; Kinney, Michael D
> 
> Cc: Yao, Jiewen ; Dong, Eric
> 
> Subject: RE: [edk2] [PATCH v2] UefiCpuPkg\CpuSmm: Save
> & restore CR2 on-demand paging in SMM
> 
> Laszlo,
> 
> Good news is that the PCD being used is a Feature Flag.
> 
> [PcdsFeatureFlag]
>   ## Indicates if SMM Profile will be enabled.
>   #  If enabled, instruction executions in and data
> accesses to memory outside of SMRAM will be logged.
>   #  It could not be enabled at the same time with SMM
> static page table feature (PcdCpuSmmStaticPageTable).
>   #  This PCD is only for validation purpose. It should
> be set to false in production.
>   #   TRUE  - SMM Profile will be enabled.
>   #   FALSE - SMM Profile will be disabled.
>   # @Prompt Enable SMM Profile.
> 
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileEnable|FALSE|
> BOOLEAN|0x32132109
> 
> That means a different PcdLib function should be used
> to look up
> the value so it is clear that it is safe at SMM
> runtime.
> 
> FeaturePcdGet(TokenName)
> 
> Best regards,
> 
> Mike
> 
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-
> > boun...@lists.01.org] On Behalf Of Laszlo Ersek
> > Sent: Thursday, March 7, 2019 9:58 AM
> > To: Vanguput, Narendra K
> > ; edk2-
> > de...@lists.01.org
> > Cc: Yao, Jiewen ; Dong, Eric
> > 
> > Subject: Re: [edk2] [PATCH v2] UefiCpuPkg\CpuSmm:
> Save
> > & restore CR2 on-demand paging in SMM
> >
> > On 03/07/19 12:14, nkvangup wrote:
> > > BZ:
> > https://bugzilla.tianocore.org/show_bug.cgi?id=1593
> > >
> > > For every SMI occurrence, save and restore CR2
> > register only when SMM
> > > on-demand paging support is enabled in 64 bit
> > operation mode.
> > >
> > > Contributed-under: TianoCore Contribution Agreement
> > 1.1
> > > Signed-off-by: Vanguput Narendra K
> > 
> > > Cc: Eric Dong 
> > > Cc: Ray Ni 
> > > Cc: Laszlo Ersek 
> > > Cc: Yao Jiewen 
> > > ---
> > >  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 20
> > 
> > >  1 file changed, 12 insertions(+), 8 deletions(-)
> >
> > (1) There is an open question about the usefulness of
> > this patch in
> >
>  > >. It should be
> > answered in the BZ, or the same description should be
> > included in the
> > commit message.
> >
> > (2) Also, the commit message should refer to the BZ.
> >
> >
> > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > > index 3b0b3b52ac..5be4a2b020 100644
> > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > > @@ -,10 +,12 @@ SmiRendezvous (
> > >
> > >ASSERT(CpuIndex < mMaxNumberOfCpus);
> > >
> > > -  //
> > > -  // Save Cr2 because Page Fault exception in SMM
> > may override its value
> > > -  //
> > > -  Cr2 = AsmReadCr2 ();
> > > +  if ((sizeof (UINTN) == sizeof (UINT64)) &&
> > (!PcdGetBool (PcdCpuSmmStaticPageTable))) {
> >
> > (3) It doesn't look like a good idea to me to call
> > PcdGetBool() in the
> > SmiRendezvous() function.
> >
> > If the PCD is not fixed-at-build (but dynamic), then
> > we'll end up
> > calling a PI protocol member from a function that is
> by
> > definition
> > executed by multiple processors at the same time.
> >
> > "X64/PageTbl.c" already defines the global variable
> > "mCpuSmmStaticPageTable", setting it from the PCD on
> > the call stack of
> > the entry point function of the driver. That is safe
> --
> > we can call PI /
> > UEFI protocols in the entry point functions of a
> > DXE_SMM_DRIVER.
> >
> > Now, the fact that "mCpuSmmStaticPageTable" is only
> > defined in the X64
> > build (that is, in "X64/PageTbl.c"), is actually
> quite
> > informative. It
> > means that, instead of this conditional code in
> > "MpService.c", we should
> > introduce two new helper functions, "SaveCr2" and
> > "RestoreCr2". And we
> > should provide separate implementations for IA32 and
> > X64. For IA32, the
> > function should do nothing. For X64, the function
> > should depend on
> > "mCpuSmmStaticPageTable", and massage CR2 as
> necessary.
> >
> > However: that *still* depends on whether this change
> is
> > useful. I
> > realize the CR2 manipulation may not be overly useful
> > on IA32 (we can't
> > address >4GB memory, so demand paging for >4GB makes
> no
> > sense), but its
> > performance hit should be negligible. Again, back to
> > point (1): what is
> > the actual issue with the current code?
> >
> > Thanks
> > Laszlo
> >
> > > +//
> > > +// Save Cr2 because Page Fault exception in
> SMM
> > may override its value
> > > +//
> > > +Cr2 = AsmReadCr2 ();
> > > +  }
> > >
> > >//
> > >// Perform CPU specific entry hooks
> > > @@ 

Re: [edk2] [PATCH v2] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM

[Yao, Jiewen]

Good catch Laszo!!!

I found PcdCpuSmmStaticPageTable is [PcdsFixedAtBuild, PcdsPatchableInModule, 
PcdsDynamic, PcdsDynamicEx].
I think it should only be static, but I am wrong. Thanks to point it out.
Then I think we need get the PCD value at the entrypoint.


Another option is just to move the CR2 access from C code to ASM code, to 
isolate the CR2 access in C code.


Thank you
Yao Jiewen


> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Thursday, March 7, 2019 9:58 AM
> To: Vanguput, Narendra K ;
> edk2-devel@lists.01.org
> Cc: Dong, Eric ; Ni, Ray ; Yao,
> Jiewen 
> Subject: Re: [PATCH v2] UefiCpuPkg\CpuSmm: Save & restore CR2
> on-demand paging in SMM
> 
> On 03/07/19 12:14, nkvangup wrote:
> > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1593
> >
> > For every SMI occurrence, save and restore CR2 register only when SMM
> > on-demand paging support is enabled in 64 bit operation mode.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Vanguput Narendra K 
> > Cc: Eric Dong 
> > Cc: Ray Ni 
> > Cc: Laszlo Ersek 
> > Cc: Yao Jiewen 
> > ---
> >  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 20 
> >  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> (1) There is an open question about the usefulness of this patch in
> . It should be
> answered in the BZ, or the same description should be included in the
> commit message.
> 
> (2) Also, the commit message should refer to the BZ.
> 
> 
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > index 3b0b3b52ac..5be4a2b020 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > @@ -,10 +,12 @@ SmiRendezvous (
> >
> >ASSERT(CpuIndex < mMaxNumberOfCpus);
> >
> > -  //
> > -  // Save Cr2 because Page Fault exception in SMM may override its value
> > -  //
> > -  Cr2 = AsmReadCr2 ();
> > +  if ((sizeof (UINTN) == sizeof (UINT64)) && (!PcdGetBool
> (PcdCpuSmmStaticPageTable))) {
> 
> (3) It doesn't look like a good idea to me to call PcdGetBool() in the
> SmiRendezvous() function.
> 
> If the PCD is not fixed-at-build (but dynamic), then we'll end up
> calling a PI protocol member from a function that is by definition
> executed by multiple processors at the same time.
> 
> "X64/PageTbl.c" already defines the global variable
> "mCpuSmmStaticPageTable", setting it from the PCD on the call stack of
> the entry point function of the driver. That is safe -- we can call PI /
> UEFI protocols in the entry point functions of a DXE_SMM_DRIVER.
> 
> Now, the fact that "mCpuSmmStaticPageTable" is only defined in the X64
> build (that is, in "X64/PageTbl.c"), is actually quite informative. It
> means that, instead of this conditional code in "MpService.c", we should
> introduce two new helper functions, "SaveCr2" and "RestoreCr2". And we
> should provide separate implementations for IA32 and X64. For IA32, the
> function should do nothing. For X64, the function should depend on
> "mCpuSmmStaticPageTable", and massage CR2 as necessary.
> 
> However: that *still* depends on whether this change is useful. I
> realize the CR2 manipulation may not be overly useful on IA32 (we can't
> address >4GB memory, so demand paging for >4GB makes no sense), but its
> performance hit should be negligible. Again, back to point (1): what is
> the actual issue with the current code?
> 
> Thanks
> Laszlo
> 
> > +//
> > +// Save Cr2 because Page Fault exception in SMM may override its
> value
> > +//
> > +Cr2 = AsmReadCr2 ();
> > +  }
> >
> >//
> >// Perform CPU specific entry hooks
> > @@ -1253,10 +1255,12 @@ SmiRendezvous (
> >
> >  Exit:
> >SmmCpuFeaturesRendezvousExit (CpuIndex);
> > -  //
> > -  // Restore Cr2
> > -  //
> > -  AsmWriteCr2 (Cr2);
> > +  if ((sizeof (UINTN) == sizeof (UINT64)) && (!PcdGetBool
> (PcdCpuSmmStaticPageTable))) {
> > +//
> > +// Restore Cr2
> > +//
> > +AsmWriteCr2 (Cr2);
> > +  }
> >  }
> >
> >  /**
> >

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM

[Kinney, Michael D]

Laszlo,

Good news is that the PCD being used is a Feature Flag.

[PcdsFeatureFlag]
  ## Indicates if SMM Profile will be enabled.
  #  If enabled, instruction executions in and data accesses to memory outside 
of SMRAM will be logged.
  #  It could not be enabled at the same time with SMM static page table 
feature (PcdCpuSmmStaticPageTable).
  #  This PCD is only for validation purpose. It should be set to false in 
production.
  #   TRUE  - SMM Profile will be enabled.
  #   FALSE - SMM Profile will be disabled.
  # @Prompt Enable SMM Profile.
  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileEnable|FALSE|BOOLEAN|0x32132109

That means a different PcdLib function should be used to look up
the value so it is clear that it is safe at SMM runtime.

FeaturePcdGet(TokenName)

Best regards,

Mike


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-
> boun...@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Thursday, March 7, 2019 9:58 AM
> To: Vanguput, Narendra K
> ; edk2-
> de...@lists.01.org
> Cc: Yao, Jiewen ; Dong, Eric
> 
> Subject: Re: [edk2] [PATCH v2] UefiCpuPkg\CpuSmm: Save
> & restore CR2 on-demand paging in SMM
> 
> On 03/07/19 12:14, nkvangup wrote:
> > BZ:
> https://bugzilla.tianocore.org/show_bug.cgi?id=1593
> >
> > For every SMI occurrence, save and restore CR2
> register only when SMM
> > on-demand paging support is enabled in 64 bit
> operation mode.
> >
> > Contributed-under: TianoCore Contribution Agreement
> 1.1
> > Signed-off-by: Vanguput Narendra K
> 
> > Cc: Eric Dong 
> > Cc: Ray Ni 
> > Cc: Laszlo Ersek 
> > Cc: Yao Jiewen 
> > ---
> >  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 20
> 
> >  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> (1) There is an open question about the usefulness of
> this patch in
>  >. It should be
> answered in the BZ, or the same description should be
> included in the
> commit message.
> 
> (2) Also, the commit message should refer to the BZ.
> 
> 
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > index 3b0b3b52ac..5be4a2b020 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > @@ -,10 +,12 @@ SmiRendezvous (
> >
> >ASSERT(CpuIndex < mMaxNumberOfCpus);
> >
> > -  //
> > -  // Save Cr2 because Page Fault exception in SMM
> may override its value
> > -  //
> > -  Cr2 = AsmReadCr2 ();
> > +  if ((sizeof (UINTN) == sizeof (UINT64)) &&
> (!PcdGetBool (PcdCpuSmmStaticPageTable))) {
> 
> (3) It doesn't look like a good idea to me to call
> PcdGetBool() in the
> SmiRendezvous() function.
> 
> If the PCD is not fixed-at-build (but dynamic), then
> we'll end up
> calling a PI protocol member from a function that is by
> definition
> executed by multiple processors at the same time.
> 
> "X64/PageTbl.c" already defines the global variable
> "mCpuSmmStaticPageTable", setting it from the PCD on
> the call stack of
> the entry point function of the driver. That is safe --
> we can call PI /
> UEFI protocols in the entry point functions of a
> DXE_SMM_DRIVER.
> 
> Now, the fact that "mCpuSmmStaticPageTable" is only
> defined in the X64
> build (that is, in "X64/PageTbl.c"), is actually quite
> informative. It
> means that, instead of this conditional code in
> "MpService.c", we should
> introduce two new helper functions, "SaveCr2" and
> "RestoreCr2". And we
> should provide separate implementations for IA32 and
> X64. For IA32, the
> function should do nothing. For X64, the function
> should depend on
> "mCpuSmmStaticPageTable", and massage CR2 as necessary.
> 
> However: that *still* depends on whether this change is
> useful. I
> realize the CR2 manipulation may not be overly useful
> on IA32 (we can't
> address >4GB memory, so demand paging for >4GB makes no
> sense), but its
> performance hit should be negligible. Again, back to
> point (1): what is
> the actual issue with the current code?
> 
> Thanks
> Laszlo
> 
> > +//
> > +// Save Cr2 because Page Fault exception in SMM
> may override its value
> > +//
> > +Cr2 = AsmReadCr2 ();
> > +  }
> >
> >//
> >// Perform CPU specific entry hooks
> > @@ -1253,10 +1255,12 @@ SmiRendezvous (
> >
> >  Exit:
> >SmmCpuFeaturesRendezvousExit (CpuIndex);
> > -  //
> > -  // Restore Cr2
> > -  //
> > -  AsmWriteCr2 (Cr2);
> > +  if ((sizeof (UINTN) == sizeof (UINT64)) &&
> (!PcdGetBool (PcdCpuSmmStaticPageTable))) {
> > +//
> > +// Restore Cr2
> > +//
> > +AsmWriteCr2 (Cr2);
> > +  }
> >  }
> >
> >  /**
> >
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM

[Laszlo Ersek]

On 03/07/19 12:14, nkvangup wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1593
> 
> For every SMI occurrence, save and restore CR2 register only when SMM
> on-demand paging support is enabled in 64 bit operation mode.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Vanguput Narendra K 
> Cc: Eric Dong 
> Cc: Ray Ni 
> Cc: Laszlo Ersek 
> Cc: Yao Jiewen 
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 20 
>  1 file changed, 12 insertions(+), 8 deletions(-)

(1) There is an open question about the usefulness of this patch in
. It should be
answered in the BZ, or the same description should be included in the
commit message.

(2) Also, the commit message should refer to the BZ.


> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 3b0b3b52ac..5be4a2b020 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -,10 +,12 @@ SmiRendezvous (
>  
>ASSERT(CpuIndex < mMaxNumberOfCpus);
>  
> -  //
> -  // Save Cr2 because Page Fault exception in SMM may override its value
> -  //
> -  Cr2 = AsmReadCr2 ();
> +  if ((sizeof (UINTN) == sizeof (UINT64)) && (!PcdGetBool 
> (PcdCpuSmmStaticPageTable))) {

(3) It doesn't look like a good idea to me to call PcdGetBool() in the
SmiRendezvous() function.

If the PCD is not fixed-at-build (but dynamic), then we'll end up
calling a PI protocol member from a function that is by definition
executed by multiple processors at the same time.

"X64/PageTbl.c" already defines the global variable
"mCpuSmmStaticPageTable", setting it from the PCD on the call stack of
the entry point function of the driver. That is safe -- we can call PI /
UEFI protocols in the entry point functions of a DXE_SMM_DRIVER.

Now, the fact that "mCpuSmmStaticPageTable" is only defined in the X64
build (that is, in "X64/PageTbl.c"), is actually quite informative. It
means that, instead of this conditional code in "MpService.c", we should
introduce two new helper functions, "SaveCr2" and "RestoreCr2". And we
should provide separate implementations for IA32 and X64. For IA32, the
function should do nothing. For X64, the function should depend on
"mCpuSmmStaticPageTable", and massage CR2 as necessary.

However: that *still* depends on whether this change is useful. I
realize the CR2 manipulation may not be overly useful on IA32 (we can't
address >4GB memory, so demand paging for >4GB makes no sense), but its
performance hit should be negligible. Again, back to point (1): what is
the actual issue with the current code?

Thanks
Laszlo

> +//
> +// Save Cr2 because Page Fault exception in SMM may override its value
> +//
> +Cr2 = AsmReadCr2 ();
> +  }
>  
>//
>// Perform CPU specific entry hooks
> @@ -1253,10 +1255,12 @@ SmiRendezvous (
>  
>  Exit:
>SmmCpuFeaturesRendezvousExit (CpuIndex);
> -  //
> -  // Restore Cr2
> -  //
> -  AsmWriteCr2 (Cr2);
> +  if ((sizeof (UINTN) == sizeof (UINT64)) && (!PcdGetBool 
> (PcdCpuSmmStaticPageTable))) {
> +//
> +// Restore Cr2
> +//
> +AsmWriteCr2 (Cr2);
> +  }
>  }
>  
>  /**
> 

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] UefiCpuPkg/MpInitLib: Direct allocate buffer for Wake up Buffer.

[Laszlo Ersek]

On 03/07/19 03:53, Dong, Eric wrote:
> Hi Star,
> 
> This logic seems much complicated than mine. Also after CSM retired from 
> EDKII, we will change this code back to only require allocate buffer below 
> 1M. I will add such notes in the code comments.  So I prefer to use my change.

I apologize for my inability to follow the recent developments in this
thread. However, what I recall is that we may not retire the CSM from
edk2 entirely. Instead, if someone volunteers to maintain the CSM under
OvmfPkg for example, then we'll keep it there.

Personally I'm not interested in the CSM, but it would be nice if we
didn't implement logic in UefiCpuPkg that would be, by design,
incompatible with an optional feature that we might carry forward in
OvmfPkg.

Thanks
Laszlo

> Thanks,
> Eric
> 
>> -Original Message-
>> From: Zeng, Star
>> Sent: Tuesday, March 5, 2019 3:33 PM
>> To: Dong, Eric ; edk2-devel@lists.01.org
>> Cc: Ni, Ray ; Laszlo Ersek (ler...@redhat.com)
>> ; Zeng, Star 
>> Subject: RE: [edk2] [Patch] UefiCpuPkg/MpInitLib: Direct allocate buffer for
>> Wake up Buffer.
>>
>> Just an idea to avoid hard code value 0x88000.
>>
>> Before EndOfDxe: Allocate buffer in AllocateResetVector(), and free buffer
>> in FreeResetVector().
>> At EndOfDxe (it is after LegacyBiosDxe driver entry point) callback: Allocate
>> buffer and record it to CpuMpData->WakeupBuffer, and always occupy the
>> buffer, that means no free.
>> After EndOfDxe: Use CpuMpData->WakeupBuffer.
>>
>>
>> Thanks,
>> Star
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>> Eric Dong
>> Sent: Tuesday, March 5, 2019 10:07 AM
>> To: edk2-devel@lists.01.org
>> Subject: [edk2] [Patch] UefiCpuPkg/MpInitLib: Direct allocate buffer for
>> Wake up Buffer.
>>
>> https://bugzilla.tianocore.org/show_bug.cgi?id=1538
>>
>> Current CpuDxe driver "borrows" Wakeup Buffer (through Allocate & free to
>> get the buffer pointer, backup the buffer data before using it and restore it
>> after using).  Now this logic met a problem described in the above BZ
>> because the test tool and the CpuDxe both use the same memory at the
>> same time.
>>
>> In order to fix the above issue, CpuDxe changed to allocate the buffer below
>> 1M instead of borrow it. After investigation, we found below
>> 0x88000 is the possible space which can be used. For now, range
>> 0x6 ~ 0x88000 used for Legacy OPROMs by LegacyBios driver. And it tries
>> to allocate these range page(4K size) by page. It just reports warning
>> message if specific page been used by others already.
>>
>> Also CpuDxe driver will produce CPU arch protocol and LegacyBios driver has
>> dependency for this protocol. So CpuDxe driver will start before LegacyBios
>> driver and CpuDxe driver can allocate that space successful.
>>
>> With this change, CpuDxe driver will use 0x87000 ~ 0x88000 as wakeup buffer.
>>
>> Cc: Ray Ni 
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Eric Dong 
>> ---
>>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 30 +++
>> ---
>>  1 file changed, 19 insertions(+), 11 deletions(-)
>>
>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>> index b2307cbb61..5bc9a47efb 100644
>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>> @@ -76,7 +76,7 @@ SaveCpuMpData (
>>  }
>>
>>  /**
>> -  Get available system memory below 1MB by specified size.
>> +  Get available system memory below 0x88000 by specified size.
>>
>>@param[in] WakeupBufferSize   Wakeup buffer size required
>>
>> @@ -91,7 +91,19 @@ GetWakeupBuffer (
>>EFI_STATUS  Status;
>>EFI_PHYSICAL_ADDRESSStartAddress;
>>
>> -  StartAddress = BASE_1MB;
>> +  //
>> +  // Current "Borrow" space mechanism caused potential race condition
>> + if both  // AP and the original owner use the share space.
>> +  //
>> +  // LegacyBios driver tries to allocate 4K pages between 0x6 ~
>> + 0x88000  // space. It will just report an waring message if the page
>> + has been allocate  // by other drivers.
>> +  // LagacyBios driver depends on CPU Arch protocol, so it will start
>> + after  // CpuDxe driver which produce Cpu Arch Protocol and use this
>> library.
>> +  // So below allocate logic will be trigged before LegacyBios driver
>> + and it  // will always return success.
>> +  //
>> +  StartAddress = BASE_512KB + BASE_32KB;
>>Status = gBS->AllocatePages (
>>AllocateMaxAddress,
>>EfiBootServicesData,
>> @@ -99,17 +111,13 @@ GetWakeupBuffer (
>>
>>);
>>ASSERT_EFI_ERROR (Status);
>> -  if (!EFI_ERROR (Status)) {
>> -Status = gBS->FreePages(
>> -   StartAddress,
>> -   EFI_SIZE_TO_PAGES (WakeupBufferSize)
>> -   );
>> -ASSERT_EFI_ERROR (Status);
>> -DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, 

[edk2] Regarding PXE boot NACK error.

[MohammadYounasKhan.P]

Hi All,

When one of DHCP cum PXE server which is not configured properly and returns 
NACK.

When PxeBcDhcp4Dora() returns EFI_TIME_OUT and PxeBcSelectBootPrompt () returns 
EFI_NOT_FOUND, do we need to abort the DHCP transaction or not in 
PxeBcDiscoverBootFile()? Or should we retry?
Currently it calls PxeBcDhcp4BootInfo() to get the boot info.

PxeBcDhcp4CallBack is getting called 2 times:

Dhcp4Event: 0x1 (Dhcp4RcvdOffer)
Dhcp4Event: 0xC (Dhcp4Fail)

Do we need to retry PxeBcDhcp4CallBack() for more times even if it is Dhcp4Fail?

EfiPxeLoadFile() -> PxeBcLoadBootFile () -> PxeBcDiscoverBootFile() -> 
PxeBcSelectBootPrompt()

Attached the transaction debug log for reference.
Please share your comments.

Thank you,
Younas.
EfiPxeLoadFile Entry
MediaStatus: Success
UsingIpv6: 0
EfiPxeBcStart Entry
Mode->UsingIpv6: 0Start PXE over IPv6
Private->Udp4Read->Configure Status: Success
PxeBcSeedDhcp4Packet Entry
PxeBcSeedDhcp4Packet Exit
CreateEvent for Arp Status: Success
SetTimer for ARP Status: Success
CreateEvent for Icmp Status: Success
PxeBcSetIp4Policy Entry
Ip4Config2->GetData Status: Success
PxeBcSetIp4Policy Exit
PxeBcSetIp4Policy Status: Success
CreateEvent for Udptimeout Status: Success
Everything is success
PxeBc->Start() Status: Success
PxeBcLoadBootFile Entry
PxeBcInstallCallback Entry
InstallProtocolInterface: 245DCA21-FB7B-11D3-8F01-00A0C969723B B4962190
PxeBcInstallCallback Exit Status: Success
PxeBcInstallCallback Status: Success
Private->BootFileSize: 0
PxeBcDiscoverBootFile Entry
EfiPxeBcDhcp Entry
Mode->UsingIpv6: 0PxeBcDhcp4Dora Entry
PxeBcBuildDhcp4Options Entry
PxeBcBuildDhcp4Options Exit Index: 0x5
OptCount: 0x5
Dhcp4->Configure Status: Success
PxeBcDhcp4CallBack Entry, Dhcp4Event: 0x1
PxeBcParseDhcp4Options Entry
Found the required option. So return the option
Callback Status: Warning Unknown Glyph
PxeBcDhcp4CallBack Exit Status: Success
PxeBcDhcp4CallBack Entry, Dhcp4Event: 0xC
returns success
Dhcp4->Start Status: Time out
PxeBcDhcp4Dora Exit Status: Time out
PxeBcDhcp4Dora Status: Time out
Private->Udp6Read->Configure Status: Success
EfiPxeBcDhcp Exit status:Success
PxeBc->Dhcp Status: Success
PxeBcSelectBootPrompt Status: Not Found
Mode->ProxyOfferReceived: 0x0
Mode->UsingIpv6: 0

DXE_ASSERT!: c:\dummy\NetworkPkg\UefiPxeBcDxe\PxeBcBoot.c (508): 
Cache4->OptList[6] != ((void *) 0)
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 00/10] Remove .S files for IA32 and X64 arch in MdePkg and UefiCpuPkg

[Laszlo Ersek]

Hi,

On 03/07/19 03:30, Shenglei Zhang wrote:
> .nasm file has been added for X86 arch. .S assembly code
> is not required any more.
> https://bugzilla.tianocore.org/show_bug.cgi?id=1594
> 
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Cc: Eric Dong 
> Cc: Ray Ni 
> Cc: Laszlo Ersek 
> Shenglei Zhang (10):
>   UefiCpuPkg/SmmCpuFeaturesLib: Remove .S files for IA32 and X64 arch
>   UefiCpuPkg/BaseUefiCpuLib: Remove .S files for IA32 and X64 arch
>   UefiCpuPkg/CpuExceptionHandlerLib:Remove.S files for IA32 and X64 arch

Thanks for the CC; I'll skip this review, and defer to Eric, Mike and Ray.

Laszlo

>   MdePkg/BaseCpuLib: Remove .S files for IA32 and X64 arch
>   MdePkg/BaseLib: Remove .S files for IA32 and X64 arch
>   MdePkg/BaseMemoryLibMmx: Remove .S files for IA32 and X64 arch
>   MdePkg/BaseMemoryLibOptDxe: Remove .S files for IA32 and X64 arch
>   MdePkg/BaseMemoryLibOptPei: Remove .S files for IA32 and X64 arch
>   MdePkg/BaseMemoryLibRepStr: Remove .S files for IA32 and X64 arch
>   MdePkg/BaseMemoryLibSse2: Remove .S files for IA32 and X64 arch


___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM

[Yao, Jiewen]

Reviewed-by: jiewen@intel.com

> -Original Message-
> From: Vanguput, Narendra K
> Sent: Thursday, March 7, 2019 3:15 AM
> To: edk2-devel@lists.01.org
> Cc: Vanguput, Narendra K ; Dong, Eric
> ; Ni, Ray ; Laszlo Ersek
> ; Yao, Jiewen 
> Subject: [PATCH v2] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand
> paging in SMM
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1593
> 
> For every SMI occurrence, save and restore CR2 register only when SMM
> on-demand paging support is enabled in 64 bit operation mode.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Vanguput Narendra K 
> Cc: Eric Dong 
> Cc: Ray Ni 
> Cc: Laszlo Ersek 
> Cc: Yao Jiewen 
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 20 
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 3b0b3b52ac..5be4a2b020 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -,10 +,12 @@ SmiRendezvous (
> 
>ASSERT(CpuIndex < mMaxNumberOfCpus);
> 
> -  //
> -  // Save Cr2 because Page Fault exception in SMM may override its value
> -  //
> -  Cr2 = AsmReadCr2 ();
> +  if ((sizeof (UINTN) == sizeof (UINT64)) && (!PcdGetBool
> (PcdCpuSmmStaticPageTable))) {
> +//
> +// Save Cr2 because Page Fault exception in SMM may override its
> value
> +//
> +Cr2 = AsmReadCr2 ();
> +  }
> 
>//
>// Perform CPU specific entry hooks
> @@ -1253,10 +1255,12 @@ SmiRendezvous (
> 
>  Exit:
>SmmCpuFeaturesRendezvousExit (CpuIndex);
> -  //
> -  // Restore Cr2
> -  //
> -  AsmWriteCr2 (Cr2);
> +  if ((sizeof (UINTN) == sizeof (UINT64)) && (!PcdGetBool
> (PcdCpuSmmStaticPageTable))) {
> +//
> +// Restore Cr2
> +//
> +AsmWriteCr2 (Cr2);
> +  }
>  }
> 
>  /**
> --
> 2.16.2.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] Does ARM platform produce MP protocol?

[Achin Gupta]

On Wed, Mar 06, 2019 at 02:22:25PM +0100, Ard Biesheuvel wrote:
> On Wed, 6 Mar 2019 at 13:41, Achin Gupta  wrote:
> >
> > On Wed, Mar 06, 2019 at 10:37:58AM +0100, Ard Biesheuvel wrote:
> > > (adding Achin and Charles)
> > >
> > > On Wed, 6 Mar 2019 at 10:16, Ni, Ray  wrote:
> > > >
> > > > > -Original Message-
> > > > > From: edk2-devel  On Behalf Of Ard
> > > > > Biesheuvel
> > > > > Sent: Wednesday, March 6, 2019 3:38 PM
> > > > > To: Ni, Ray 
> > > > > Cc: edk2-devel@lists.01.org
> > > > > Subject: Re: [edk2] Does ARM platform produce MP protocol?
> > > > >
> > > > > On Wed, 6 Mar 2019 at 06:44, Ni, Ray  wrote:
> > > > > >
> > > > > > Ard, Leif,
> > > > > > I am a bit interested in how ARM platform supports the MP?
> > > > > > PI Spec defines below protocol but I failed to find a driver in ARM 
> > > > > > platform
> > > > > producing this protocol.
> > > > > > Or did I miss anything?
> > > > > >
> > > > >
> > > > > No you are right. We don't expose that on ARM, since UEFI only runs 
> > > > > on a
> > > > > single core. Bringing up and taking down cores is done via a protocol 
> > > > > called
> > > > > PSCI, which is implemented by firmware running at a higher privilege 
> > > > > level.
> > > > >
> > > > > So while it would be possible to implement the MP protocol on top of 
> > > > > PSCI,
> > > > > we haven't identified a use case for it yet. (The OS calls PSCI 
> > > > > directly to boot
> > > > > the secondary cores)
> >
> > IIUC, the MP protocol enables communication between processors that are 
> > already
> > up instead of bringing them up or taking them down. So, it is orthogonal to
> > PSCI. Is that what you meant?
> >
>
> Surely, StartupThisAP starts up the AP, no?

I was talking about the EFI_MM_MP_PROTOCOL which I thought was the original bone
of contention.

PSCI has a direct intersection with the EFI_MP_SERVICES_PROTOCOL.

cheers,
Achin

>
> In any case, I didn't dig any deeper, but I know that PSCI can be used
> (even in the UEFI context) to execute pieces of code on another core
> (ACS uses this IIRC)
>
> > > >
> > > > Is below EFI_MM_MP_PROTOCOL (added in PI spec 1.6) implemented in ARM?
> > > > Or will it be implemented by an ARM module?
> > >
> > > No it is currently not implemented, and I am not aware of any plans to do 
> > > so.
> >
> > +1. There is no need to do this until UEFI runs on a single core on Arm.
> >
>
> until -> as long as ??
>
> > >
> > > > I am asking this because MP_SERVICES protocol exposes CPU location 
> > > > information
> > > > (Package/Core/Thread) through *GetProcessorInfo*, but MM_MP protocol 
> > > > doesn't
> > > > have a way to expose that information.
> > > > If such location information isn't exposed by MM_MP, is that because 
> > > > ARM doesn't
> > > > care about the location information? Or because ARM cares but forgot to 
> > > > add similar
> > > > *GetProcessorInfo* interface to MM_MP when changing the PI spec?
> > > > Or ARM doesn't use the MM_MP at all?
> >
> > Even if Arm used this protocol, it can work with the logical processor 
> > number. I
> > don't see a need to expose the location information to the caller. It seems 
> > very
> > Intel specific. Is the EFI_MP_SERVICES_PROTOCOL used on Arm?
> >
>
> No, that is what started the discussion.
>
> > > >
> > >
> > > I don't know the history of this protocol and who proposed it, but
> > > today, we don't have a need for running per-CPU initialization code in
> > > the context of MM. Even if MM is a component of the more privileged
> > > firmware running on an ARM system, it is running in a sandbox that was
> > > primarily designed around exposing MM services to UEFI code running at
> > > the same privilege level as the OS (such as the authenticated variable
> > > store). Platform initialization code (which is more likely to require
> > > dispatch to each core) runs in the secure world as well, but not in
> > > the context of MM.
> > >
> > > I will let Achin chime in here as well.
> >
> > +1.
> >
> > I will let Charles comment on the history. Maybe this protocol was designed
> > for Arm systems where MM is the most privileged firmware. The upstream
> > implementation runs MM in the lowest privilege level. Either way, this 
> > protocol
> > sense only when MM on Arm is MP capable.
> >
> > cheers,
> > Achin
> >
> >
> > >
> > >
> > > > typedef struct _EFI_MM_MP_PROTOCOL {
> > > > UINT32   Revision,
> > > > UINT32   Attributes,
> > > > EFI_MM_ GET_NUMBER_OF_PROCESSORS GetNumberOfProcessors,
> > > > EFI_MM_DISPATCH_PROCEDUREDispatchProcedure,
> > > > EFI_MM_BROADCAST_PROCEDURE   BroadcastProcedure,
> > > > EFI_MM_SET_STARTUP_PROCEDURE SetStartupProcedure,
> > > > EFI_CHECK_FOR_PROCEDURE  CheckOnProcedure,
> > > > EFI_WAIT_FOR_PROCEDURE   WaitForProcedure,
> > > > }EFI_MM_MP_PROTOCOL;
> > > > >
> > > > > > typedef struct _EFI_MP_SERVICES_PROTOCOL 

Re: [edk2] [PATCH 02/10] StandaloneMmPkg: drop unused PCD PcdStandaloneMmEnable

[Achin Gupta]

On Thu, Mar 07, 2019 at 11:09:35AM +0100, Ard Biesheuvel wrote:
> On Wed, 6 Mar 2019 at 16:37, Achin Gupta  wrote:
> >
> > On Wed, Mar 06, 2019 at 04:17:51PM +0100, Ard Biesheuvel wrote:
> > > On Wed, 6 Mar 2019 at 16:16, Achin Gupta  wrote:
> > > >
> > > > Hi Ard,
> > > >
> > > > On Tue, Mar 05, 2019 at 02:32:40PM +0100, Ard Biesheuvel wrote:
> > > > > The PCD PcdStandaloneMmEnable is unused, and shouldn't exist in the
> > > > > first place since the value is implied by the context (it is never
> > > > > valid to set it to FALSE for standalone MM or TRUE for traditional
> > > > > MM). So drop it.
> > > >
> > > > This is being used to determine if the ArmVExpressPkg should include 
> > > > support for
> > > > StMM comm. buffer or not [1] but it does look redundant now.
> > > >
> > >
> > > If that is the case, the PCD should be defined in that package.
> >
> > The Arm FVP port for StMM needs a rewrite on the lines of other platforms. 
> > This
> > change is fine.
> >
>
> Yes, you are right. SynQuacer also needs some tweaks to align with
> these changes, but I will post those separately.
>
> So with those changes merged, the only thing preventing us from
> building the SynQuacer + MM platform from upstream sources is the
> MmCommunicate VA vs PA issue. Is there any progress on that front?

I am looking at this now after my holiday. I have some questions that I will
post separately.

cheers,
Achin

>
> Thanks,
> Ard.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH v2] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM

[nkvangup]

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1593

For every SMI occurrence, save and restore CR2 register only when SMM
on-demand paging support is enabled in 64 bit operation mode.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Vanguput Narendra K 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Laszlo Ersek 
Cc: Yao Jiewen 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index 3b0b3b52ac..5be4a2b020 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -,10 +,12 @@ SmiRendezvous (
 
   ASSERT(CpuIndex < mMaxNumberOfCpus);
 
-  //
-  // Save Cr2 because Page Fault exception in SMM may override its value
-  //
-  Cr2 = AsmReadCr2 ();
+  if ((sizeof (UINTN) == sizeof (UINT64)) && (!PcdGetBool 
(PcdCpuSmmStaticPageTable))) {
+//
+// Save Cr2 because Page Fault exception in SMM may override its value
+//
+Cr2 = AsmReadCr2 ();
+  }
 
   //
   // Perform CPU specific entry hooks
@@ -1253,10 +1255,12 @@ SmiRendezvous (
 
 Exit:
   SmmCpuFeaturesRendezvousExit (CpuIndex);
-  //
-  // Restore Cr2
-  //
-  AsmWriteCr2 (Cr2);
+  if ((sizeof (UINTN) == sizeof (UINT64)) && (!PcdGetBool 
(PcdCpuSmmStaticPageTable))) {
+//
+// Restore Cr2
+//
+AsmWriteCr2 (Cr2);
+  }
 }
 
 /**
-- 
2.16.2.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 02/10] StandaloneMmPkg: drop unused PCD PcdStandaloneMmEnable

[Ard Biesheuvel]

On Wed, 6 Mar 2019 at 16:37, Achin Gupta  wrote:
>
> On Wed, Mar 06, 2019 at 04:17:51PM +0100, Ard Biesheuvel wrote:
> > On Wed, 6 Mar 2019 at 16:16, Achin Gupta  wrote:
> > >
> > > Hi Ard,
> > >
> > > On Tue, Mar 05, 2019 at 02:32:40PM +0100, Ard Biesheuvel wrote:
> > > > The PCD PcdStandaloneMmEnable is unused, and shouldn't exist in the
> > > > first place since the value is implied by the context (it is never
> > > > valid to set it to FALSE for standalone MM or TRUE for traditional
> > > > MM). So drop it.
> > >
> > > This is being used to determine if the ArmVExpressPkg should include 
> > > support for
> > > StMM comm. buffer or not [1] but it does look redundant now.
> > >
> >
> > If that is the case, the PCD should be defined in that package.
>
> The Arm FVP port for StMM needs a rewrite on the lines of other platforms. 
> This
> change is fine.
>

Yes, you are right. SynQuacer also needs some tweaks to align with
these changes, but I will post those separately.

So with those changes merged, the only thing preventing us from
building the SynQuacer + MM platform from upstream sources is the
MmCommunicate VA vs PA issue. Is there any progress on that front?

Thanks,
Ard.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch V2 1/1] Document: Add PCD flexible format value EBNF in Fdf.

[Zhu, Yonghong]

Reviewed-by: Yonghong Zhu  

Best Regards,
Zhu Yonghong


-Original Message-
From: Feng, Bob C 
Sent: Thursday, March 07, 2019 1:42 PM
To: edk2-devel@lists.01.org
Cc: Feng, Bob C ; Gao, Liming ; 
Zhu, Yonghong 
Subject: [Patch V2 1/1] Document: Add PCD flexible format value EBNF in Fdf.

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=541

This patch is to add flexible PCD value format EBNF into Fdf spec.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Bob Feng 
Cc: Liming Gao 
Cc: Yonghong Zhu 
---
 3_edk_ii_fdf_file_format/32_fdf_definition.md | 25 
+
 3_edk_ii_fdf_file_format/35_[fd]_sections.md  |  4 ++--
 3_edk_ii_fdf_file_format/36_[fv]_sections.md  |  4 ++--
 3_edk_ii_fdf_file_format/37_[capsule]_sections.md |  4 ++--
 README.md |  1 +
 5 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/3_edk_ii_fdf_file_format/32_fdf_definition.md 
b/3_edk_ii_fdf_file_format/32_fdf_definition.md
index db098cf..2b044ab 100644
--- a/3_edk_ii_fdf_file_format/32_fdf_definition.md
+++ b/3_edk_ii_fdf_file_format/32_fdf_definition.md
@@ -1,9 +1,9 @@