Re: [edk2] [PATCH 3/4] UefiCpuPkg/MtrrLib: Update algorithm to calculate optimal settings

[Ni, Ruiyu]

The old version is not just slow.
It cannot work in certain cases. That's why the new version was developed.

The new version adds a new API which allows caller to pass in the scratch
buffer instead of using the stack. If a platform has limited stack, it can use
that API.

Thanks/Ray

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Jordan Justen
> Sent: Thursday, November 9, 2017 2:56 PM
> To: Ni, Ruiyu ; Laszlo Ersek 
> Cc: Kinney, Michael D ; edk2-
> de...@lists.01.org; Yao, Jiewen ; Dong, Eric
> ; Ard Biesheuvel 
> Subject: Re: [edk2] [PATCH 3/4] UefiCpuPkg/MtrrLib: Update algorithm to
> calculate optimal settings
> 
> On 2017-11-08 19:04:35, Ni, Ruiyu wrote:
> > Jordan, Laszlo,
> >
> > I didn't realize that a platform may have less than 4-page stack
> > before memory is ready. If I was aware of that, I would change the
> > default scratch buffer size to 2 page, which should be enough too.
> 
> This does not sound much better. I'm saying that the BASE library should only
> use at most a few hundred bytes of stack.
> 
> Apparently the old algorithm did not use much memory, but perhaps was
> slow? Can we put it back in place for the BASE version of the library?
> Then, we can add a DXE specific version that uses a large buffer which it can
> allocate, and potentially free.
> 
> -Jordan
> ___
> 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 3/4] UefiCpuPkg/MtrrLib: Update algorithm to calculate optimal settings

[Jordan Justen]

On 2017-11-08 19:04:35, Ni, Ruiyu wrote:
> Jordan, Laszlo,
> 
> I didn't realize that a platform may have less than 4-page stack
> before memory is ready. If I was aware of that, I would change the
> default scratch buffer size to 2 page, which should be enough too.

This does not sound much better. I'm saying that the BASE library
should only use at most a few hundred bytes of stack.

Apparently the old algorithm did not use much memory, but perhaps was
slow? Can we put it back in place for the BASE version of the library?
Then, we can add a DXE specific version that uses a large buffer which
it can allocate, and potentially free.

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

Re: [edk2] [PATCH edk2-non-osi v1] Hisilicon D0x: Remove uncacheable attribute from memory resource HOB

[Heyi Guo]

Looks good to me.


On 2017/11/9 11:30, Ming Huang wrote:

If uncacheable attribute is included in memory resource HOB,
GCD spaces will also have EFI_MEMORY_UC capability,
then NonCoherentPciIoAllocateBuffer of NonDiscoverablePciDeviceDxe
module will allocate DMA buffer of EFI_MEMORY_UC type, which will
cause alignment fault exception with BaseMemoryLibOptDxe.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Liu Yi 
Signed-off-by: Heyi Guo 
---
  Platform/Hisilicon/D03/MemoryInitPei/MemoryInit.efi | Bin 90272 -> 90336 bytes
  Platform/Hisilicon/D05/MemoryInitPei/MemoryInit.efi | Bin 152576 -> 152480 
bytes
  2 files changed, 0 insertions(+), 0 deletions(-)

diff --git a/Platform/Hisilicon/D03/MemoryInitPei/MemoryInit.efi 
b/Platform/Hisilicon/D03/MemoryInitPei/MemoryInit.efi
index 354abcc..31e2903 100644
Binary files a/Platform/Hisilicon/D03/MemoryInitPei/MemoryInit.efi and 
b/Platform/Hisilicon/D03/MemoryInitPei/MemoryInit.efi differ
diff --git a/Platform/Hisilicon/D05/MemoryInitPei/MemoryInit.efi 
b/Platform/Hisilicon/D05/MemoryInitPei/MemoryInit.efi
index b94e0cb..eb71c44 100644
Binary files a/Platform/Hisilicon/D05/MemoryInitPei/MemoryInit.efi and 
b/Platform/Hisilicon/D05/MemoryInitPei/MemoryInit.efi differ



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

Re: [edk2] [PATCH v4 2/7] MdeModulePkg/SmmMemoryAttribute.h: Add new protocol definitions

[Wang, Jian J]

Good catch. "EFI" should be removed and "IN" should be "OUT". Thanks for the 
feedback.

> -Original Message-
> From: Zeng, Star
> Sent: Thursday, November 09, 2017 1:13 PM
> To: Wang, Jian J ; edk2-devel@lists.01.org
> Cc: Dong, Eric ; Yao, Jiewen ; Ni,
> Ruiyu ; Zeng, Star 
> Subject: RE: [PATCH v4 2/7] MdeModulePkg/SmmMemoryAttribute.h: Add new
> protocol definitions
> 
> Suggest to use " SMM Memory Attribute Protocol " instead of " EFI SMM
> Memory Attribute Protocol " in the comments.
> 
> Should "+  IN  UINT64  *Attributes" be "+  OUT  
> UINT64
> *Attributes" for EDKII_SMM_GET_MEMORY_ATTRIBUTES?
> 
> Thanks,
> Star
> -Original Message-
> From: Wang, Jian J
> Sent: Friday, October 27, 2017 2:12 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star ; Dong, Eric ; Yao,
> Jiewen ; Ni, Ruiyu 
> Subject: [PATCH v4 2/7] MdeModulePkg/SmmMemoryAttribute.h: Add new
> protocol definitions
> 
> The new protocol gEdkiiSmmMemoryAttributeProtocolGuid is intended for
> PiSmmCore to be able to change memory page attributes for the sake of heap
> guard feature.
> 
> This protocol provides three interfaces to get/set/clear page attribute.
> 
> struct _EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL {
>   EDKII_SMM_GET_MEMORY_ATTRIBUTES   GetMemoryAttributes;
>   EDKII_SMM_SET_MEMORY_ATTRIBUTES   SetMemoryAttributes;
>   EDKII_SMM_CLEAR_MEMORY_ATTRIBUTES ClearMemoryAttributes;
> };
> 
> Cc: Star Zeng 
> Cc: Eric Dong 
> Cc: Jiewen Yao 
> Cc: Ruiyu Ni 
> Suggested-by: Ayellet Wolman 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> ---
>  MdeModulePkg/Include/Protocol/SmmMemoryAttribute.h | 136
> +
>  1 file changed, 136 insertions(+)
>  create mode 100644 MdeModulePkg/Include/Protocol/SmmMemoryAttribute.h
> 
> diff --git a/MdeModulePkg/Include/Protocol/SmmMemoryAttribute.h
> b/MdeModulePkg/Include/Protocol/SmmMemoryAttribute.h
> new file mode 100644
> index 00..20e145c2dc
> --- /dev/null
> +++ b/MdeModulePkg/Include/Protocol/SmmMemoryAttribute.h
> @@ -0,0 +1,136 @@
> +/** @file
> +  EFI SMM Memory Attribute Protocol provides retrieval and update
> +service
> +  for memory attributes in EFI SMM environment.
> +
> +  Copyright (c) 2017, 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  which
> + accompanies this distribution.  The full text of the license may be
> + found at  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#ifndef __SMM_MEMORYATTRIBUTE_H__
> +#define __SMM_MEMORYATTRIBUTE_H__
> +
> +//{69B792EA-39CE-402D-A2A6-F721DE351DFE}
> +#define EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL_GUID \
> +  { \
> +0x69b792ea, 0x39ce, 0x402d, { 0xa2, 0xa6, 0xf7, 0x21, 0xde, 0x35,
> +0x1d, 0xfe } \
> +  }
> +
> +typedef struct _EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL
> +EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL;
> +
> +/**
> +  This function set given attributes of the memory region specified by
> +  BaseAddress and Length.
> +
> +  @param  This  The EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL
> instance.
> +  @param  BaseAddress   The physical address that is the start address of
> +a memory region.
> +  @param  LengthThe size in bytes of the memory region.
> +  @param  AttributesThe bit mask of attributes to set for the memory
> +region.
> +
> +  @retval EFI_SUCCESS   The attributes were set for the memory 
> region.
> +  @retval EFI_INVALID_PARAMETER Length is zero.
> +Attributes specified an illegal combination 
> of
> +attributes that cannot be set together.
> +  @retval EFI_UNSUPPORTED   The processor does not support one or more
> +bytes of the memory resource range specified
> +by BaseAddress and Length.
> +The bit mask of attributes is not support for
> +the memory resource range specified by
> +BaseAddress and Length.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *EDKII_SMM_SET_MEMORY_ATTRIBUTES)(
> +  IN  EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL *This,
> +  IN  EFI_PHYSICAL_ADDRESSBaseAddress,
> +  IN  UINT64  Length,
> +  IN  UINT64

Re: [edk2] [PATCH v4 2/7] MdeModulePkg/SmmMemoryAttribute.h: Add new protocol definitions

[Zeng, Star]

Suggest to use " SMM Memory Attribute Protocol " instead of " EFI SMM Memory 
Attribute Protocol " in the comments.

Should "+  IN  UINT64  *Attributes" be "+  OUT  
UINT64  *Attributes" for 
EDKII_SMM_GET_MEMORY_ATTRIBUTES?

Thanks,
Star
-Original Message-
From: Wang, Jian J 
Sent: Friday, October 27, 2017 2:12 PM
To: edk2-devel@lists.01.org
Cc: Zeng, Star ; Dong, Eric ; Yao, 
Jiewen ; Ni, Ruiyu 
Subject: [PATCH v4 2/7] MdeModulePkg/SmmMemoryAttribute.h: Add new protocol 
definitions

The new protocol gEdkiiSmmMemoryAttributeProtocolGuid is intended for PiSmmCore 
to be able to change memory page attributes for the sake of heap guard feature.

This protocol provides three interfaces to get/set/clear page attribute.

struct _EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL {
  EDKII_SMM_GET_MEMORY_ATTRIBUTES   GetMemoryAttributes;
  EDKII_SMM_SET_MEMORY_ATTRIBUTES   SetMemoryAttributes;
  EDKII_SMM_CLEAR_MEMORY_ATTRIBUTES ClearMemoryAttributes;
};

Cc: Star Zeng 
Cc: Eric Dong 
Cc: Jiewen Yao 
Cc: Ruiyu Ni 
Suggested-by: Ayellet Wolman 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
 MdeModulePkg/Include/Protocol/SmmMemoryAttribute.h | 136 +
 1 file changed, 136 insertions(+)
 create mode 100644 MdeModulePkg/Include/Protocol/SmmMemoryAttribute.h

diff --git a/MdeModulePkg/Include/Protocol/SmmMemoryAttribute.h 
b/MdeModulePkg/Include/Protocol/SmmMemoryAttribute.h
new file mode 100644
index 00..20e145c2dc
--- /dev/null
+++ b/MdeModulePkg/Include/Protocol/SmmMemoryAttribute.h
@@ -0,0 +1,136 @@
+/** @file
+  EFI SMM Memory Attribute Protocol provides retrieval and update 
+service
+  for memory attributes in EFI SMM environment.
+
+  Copyright (c) 2017, 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  which 
+ accompanies this distribution.  The full text of the license may be 
+ found at  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,  
+ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#ifndef __SMM_MEMORYATTRIBUTE_H__
+#define __SMM_MEMORYATTRIBUTE_H__
+
+//{69B792EA-39CE-402D-A2A6-F721DE351DFE}
+#define EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL_GUID \
+  { \
+0x69b792ea, 0x39ce, 0x402d, { 0xa2, 0xa6, 0xf7, 0x21, 0xde, 0x35, 
+0x1d, 0xfe } \
+  }
+
+typedef struct _EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL 
+EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL;
+
+/**
+  This function set given attributes of the memory region specified by
+  BaseAddress and Length.
+
+  @param  This  The EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL instance.
+  @param  BaseAddress   The physical address that is the start address of
+a memory region.
+  @param  LengthThe size in bytes of the memory region.
+  @param  AttributesThe bit mask of attributes to set for the memory
+region.
+
+  @retval EFI_SUCCESS   The attributes were set for the memory region.
+  @retval EFI_INVALID_PARAMETER Length is zero.
+Attributes specified an illegal combination of
+attributes that cannot be set together.
+  @retval EFI_UNSUPPORTED   The processor does not support one or more
+bytes of the memory resource range specified
+by BaseAddress and Length.
+The bit mask of attributes is not support for
+the memory resource range specified by
+BaseAddress and Length.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EDKII_SMM_SET_MEMORY_ATTRIBUTES)(
+  IN  EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL *This,
+  IN  EFI_PHYSICAL_ADDRESSBaseAddress,
+  IN  UINT64  Length,
+  IN  UINT64  Attributes
+  );
+
+/**
+  This function clears given attributes of the memory region specified 
+by
+  BaseAddress and Length.
+
+  @param  This  The EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL instance.
+  @param  BaseAddress   The physical address that is the start address of
+a memory region.
+  @param  LengthThe size in bytes of the memory region.
+  @param  AttributesThe bit mask of attributes to set for the memory
+region.
+
+  @retval EFI_SUCCESS   The attributes were set for the memory region.
+  @retval EFI_INVALID_PARAMETER Length is zero.
+  

Re: [edk2] [PATCH] Tftp assert fix for openfile failure case

[Udit Kumar]

> -Original Message-
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> Sent: Wednesday, November 08, 2017 8:52 PM
> To: Udit Kumar 
> Cc: Vabhav Sharma ; edk2-devel@lists.01.org;
> ruiyu...@intel.com; jaben.car...@intel.com; ard.biesheu...@linaro.org;
> siyuan...@intel.com; ting...@intel.com
> Subject: Re: [PATCH] Tftp assert fix for openfile failure case
> 
> On Wed, Nov 08, 2017 at 05:15:49AM +, Udit Kumar wrote:
> > > > diff --git a/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > > b/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > > index fbde3bf..6425fc5 100755
> > > > --- a/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > > +++ b/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > > @@ -509,6 +509,7 @@ ShellCommandRunTftp (
> > > >);
> > > >goto NextHandle;
> > >
> > > Wow, a goto in a foor loop in a 320-line function.
> > > What could possibly go wrong?
> >
> > Instead of being on some volume, if you are on Shell.
> > Then file open will fail.
> 
> Sure. The above was a snarky comment on the state of the existing code.
> 
> > > >  }
> > > > +DataSize = FileSize;
> > > >
> > > >  Status = DownloadFile (Mtftp4, RemoteFilePath,
> > > > AsciiRemoteFilePath,
> > > FileSize, BlockSize, );
> > > >  if (EFI_ERROR (Status)) {
> > > > @@ -539,7 +540,6 @@ ShellCommandRunTftp (
> > > >goto NextHandle;
> > > >  }
> > > >
> > > > -DataSize = FileSize;
> > > >  Status = ShellWriteFile (FileHandle, , Data);
> > > >  if (!EFI_ERROR (Status)) {
> > > >ShellStatus = SHELL_SUCCESS;
> > > > --
> > > > 1.9.1
> > >
> > > So, a wider question:
> > > This shell command was introduced in the heyday of "let's
> > > reimplement U-Boot in the EDK2 tree". Mainly, from my impression, it
> > > seems to be used in order that people don't need to learn how boot
> > > managers and device paths work.
> >
> > When you say about complete boot, then this may not be useful.
> >
> > > Am I being too harsh?
> > > Are there practical uses for this?
> >
> > For doing some sort of unit testing of given interface. I found this
> > useful. During development, this is useful to transfer generic file to
> > development board.
> 
> OK, I can see how it could be useful.
> My opposition is based on three things:
> 1) people _are_ trying to use it for boot

I agree with this, please see my previous comments, 
' When you say about complete boot, then this may not be useful.'

> 2) not a command described by UEFI Shell spec, but I keep seeing
>platforms including it even in RELEASE builds (most likely because 1)
> 3) code quality/maintainability

> > > If the code is to be kept, I think (from a quick glance) that I
> > > would also like to see
> > >   *Data = NULL
> > > in the error path of DownloadFile().
> 
> OK, so we don't need to drop it right now, but what's your take on this
> comment?

I am fine, if you prefer to remove this then we will develop some test 
application
for unit tests.
In case, we need to maintain this piece of code then above needs to fix as 
well. 

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

[edk2] [PATCH edk2-non-osi v1] Update D03/D05 MemoryInit.efi binary for bug 3419

[Ming Huang]

https://bugs.linaro.org/show_bug.cgi?id=3061
For fix this bug,the function PciIoPciRead of NonDiscoverablePciDeviceDxe 
should be modifyed also.

Code can also be found in github:
https://github.com/hisilicon/OpenPlatformPkg.git
branch: rp-osi-bug-v1


Ming Huang (1):
  Hisilicon D0x: Remove uncacheable attribute from memory resource HOB

 Platform/Hisilicon/D03/MemoryInitPei/MemoryInit.efi | Bin 90272 -> 90336 bytes
 Platform/Hisilicon/D05/MemoryInitPei/MemoryInit.efi | Bin 152576 -> 152480 
bytes
 2 files changed, 0 insertions(+), 0 deletions(-)

-- 
1.9.1

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

[edk2] [PATCH edk2-non-osi v1] Hisilicon D0x: Remove uncacheable attribute from memory resource HOB

[Ming Huang]

If uncacheable attribute is included in memory resource HOB,
GCD spaces will also have EFI_MEMORY_UC capability,
then NonCoherentPciIoAllocateBuffer of NonDiscoverablePciDeviceDxe
module will allocate DMA buffer of EFI_MEMORY_UC type, which will
cause alignment fault exception with BaseMemoryLibOptDxe.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Liu Yi 
Signed-off-by: Heyi Guo 
---
 Platform/Hisilicon/D03/MemoryInitPei/MemoryInit.efi | Bin 90272 -> 90336 bytes
 Platform/Hisilicon/D05/MemoryInitPei/MemoryInit.efi | Bin 152576 -> 152480 
bytes
 2 files changed, 0 insertions(+), 0 deletions(-)

diff --git a/Platform/Hisilicon/D03/MemoryInitPei/MemoryInit.efi 
b/Platform/Hisilicon/D03/MemoryInitPei/MemoryInit.efi
index 354abcc..31e2903 100644
Binary files a/Platform/Hisilicon/D03/MemoryInitPei/MemoryInit.efi and 
b/Platform/Hisilicon/D03/MemoryInitPei/MemoryInit.efi differ
diff --git a/Platform/Hisilicon/D05/MemoryInitPei/MemoryInit.efi 
b/Platform/Hisilicon/D05/MemoryInitPei/MemoryInit.efi
index b94e0cb..eb71c44 100644
Binary files a/Platform/Hisilicon/D05/MemoryInitPei/MemoryInit.efi and 
b/Platform/Hisilicon/D05/MemoryInitPei/MemoryInit.efi differ
-- 
1.9.1

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

[edk2] 答复: [PATCH 3/4] UefiCpuPkg/MtrrLib: Update algorithm to calculate optimal settings

[Fan Jeff]

Laszlo,



PiSmmCpuDxeSmm isn’t the problem, it also only consumed MtrrSetAllMtrrs() that 
will not consume big local variable buffer.



Jeff




From: edk2-devel  on behalf of Ni, Ruiyu 

Sent: Thursday, November 9, 2017 11:04:35 AM
To: Justen, Jordan L; Laszlo Ersek
Cc: Kinney, Michael D; edk2-devel@lists.01.org; Yao, Jiewen; Dong, Eric; Ard 
Biesheuvel
Subject: Re: [edk2] [PATCH 3/4] UefiCpuPkg/MtrrLib: Update algorithm to 
calculate optimal settings

Jordan, Laszlo,

I didn't realize that a platform may have less than 4-page stack before memory 
is ready.
If I was aware of that, I would change the default scratch buffer size to 2 
page, which
should be enough too.

But I do not think we may need to change the scratch buffer size.
Let me clarify about the MtrrLib API usage:
  Though the library is a BASE type, and it's MP-safe, it's not recommended to 
call
  MtrrSetMemoryAttribute...() in AP. Per IA32 SDM, all processors should use the
  same MTRR settings. In UEFI practice, we always just call the 
MtrrSetMemoryAttribute...()
  in BSP side, and then use MtrrGetAllMtrrs()/MtrrSetAllMtrrs() to sync the 
changes to
  all other Aps.


Thanks/Ray

> -Original Message-
> From: Justen, Jordan L
> Sent: Thursday, November 9, 2017 9:54 AM
> To: Laszlo Ersek ; Ni, Ruiyu 
> Cc: Dong, Eric ; Ard Biesheuvel
> ; edk2-devel@lists.01.org; Yao, Jiewen
> ; Kinney, Michael D 
> Subject: Re: [edk2] [PATCH 3/4] UefiCpuPkg/MtrrLib: Update algorithm to
> calculate optimal settings
>
> On 2017-11-08 17:36:01, Laszlo Ersek wrote:
> > Hi Ray,
> >
> > On 10/12/17 10:48, Ruiyu Ni wrote:
> > > The new algorithm converts the problem calculating optimal MTRR
> > > settings (using least MTRR registers) to the problem finding the
> > > shortest path in a graph.
> > > The memory required in extreme but rare case can be up to 256KB, so
> > > using local stack buffer is impossible considering current DxeIpl
> > > only allocates 128KB stack.
> > >
> > > The patch changes existing MtrrSetMemoryAttributeInMtrrSettings()
> > > and
> > > MtrrSetMemoryAttribute() to use the 4-page stack buffer for
> > > calculation. The two APIs return BUFFER_TOO_SMALL when the buffer is
> > > too small for calculation.
> >
> > [snip]
> >
> > > +#define SCRATCH_BUFFER_SIZE   (4 * SIZE_4KB)
> >
> > [snip]
> >
> > >  RETURN_STATUS
> > >  EFIAPI
> > > -MtrrSetMemoryAttribute (
> > > +MtrrSetMemoryAttributeInMtrrSettings (
> > > +  IN OUT MTRR_SETTINGS   *MtrrSetting,
> > >IN PHYSICAL_ADDRESSBaseAddress,
> > >IN UINT64  Length,
> > >IN MTRR_MEMORY_CACHE_TYPE  Attribute
> > >)
> > >  {
> > >RETURN_STATUS  Status;
> > > +  UINT8  Scratch[SCRATCH_BUFFER_SIZE];
> >
> > [snip]
> >
> > (This patch is now commit 2bbd7e2fbd4b.)
> >
> > Today I managed to spend time on
> >
> >   https://bugzilla.tianocore.org/show_bug.cgi?id=747
> >
> > (which is in turn based on the earlier mailing list thread
> >
> >   [edk2] dynamic PCD impact on temporary PEI memory
> >   https://lists.01.org/pipermail/edk2-devel/2017-October/016213.html
> > ).
> >
> > While writing the patches, I found the root cause of BZ#747:
> > "OvmfPkg/PlatformPei" calls MtrrLib APIs, and due to the above 16KB
> > stack allocation, MtrrLib overflow's OVMF's 16KB (total) temp SEC/PEI
> > stack.
>
> I thought it was considered bad form to use a significant portion of the 
> stack (>
> ~100 bytes) via local variables. This used to occasionally break MSVC builds 
> as
> MS would insert a stack check call if the locals size exceeded some threshold.
>
> For a BASE library, I think this should go beyond "bad form" and into not
> allowed.
>
> -Jordan
___
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 v4 0/7] Implement heap guard feature

[Wang, Jian J]

Jiewen,

Thanks for the comments.

1) I agree.
2) I forgot to remove them since we added new SMM protocol to do the same thing 
instead.

> -Original Message-
> From: Yao, Jiewen
> Sent: Thursday, November 09, 2017 10:53 AM
> To: Wang, Jian J ; Dong, Eric ;
> Zeng, Star ; Ni, Ruiyu ; Laszlo Ersek
> ; Kinney, Michael D 
> Cc: edk2-devel@lists.01.org
> Subject: RE: [edk2] [PATCH v4 0/7] Implement heap guard feature
> 
> That is great.
> 
> I have minor suggestion below:
> 1) Since StaticPaging and HeapGuard are conflicted feature, I suggest we use
> ASSERT to tell the end user, instead of disable StaticPaging silently.
> 
> +  //
> +  // Don't mark page table as read-only if heap guard is enabled.
> +  //
> +  //  BIT2: SMM page guard enabled
> +  //  BIT3: SMM pool guard enabled
> +  //
> +  if ((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT3 | BIT2)) != 0) {
> +return ;
> +  }
> +
> 
> 2) I do not think we need add below in
> MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf. Would you please double
> confirm?
> 
> +  CpuLib
> 
> +
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
> ## CONSUMES
> 
> 
> With ASSERT added and cleanup in PiSmmCore.inf, reviewed-by:
> jiewen@intel.com
> 
> 
> Thank you
> Yao Jiewen
> 
> 
> 
> > -Original Message-
> > From: Wang, Jian J
> > Sent: Thursday, November 9, 2017 10:00 AM
> > To: Yao, Jiewen ; Dong, Eric ;
> > Zeng, Star ; Ni, Ruiyu ; Laszlo
> Ersek
> > ; Kinney, Michael D 
> > Cc: Wang, Jian J ; edk2-devel@lists.01.org
> > Subject: RE: [edk2] [PATCH v4 0/7] Implement heap guard feature
> >
> > Just a friendly reminder.
> >
> > The recent commits have fixed two blocking issues for checking in heap guard
> > feature.
> >
> >   6fe575d052e36b243657a5885b5457decac41f03 (BaseCryptLib memory
> > overflow)
> >   cf8197a39d07179027455421a182598bd698
> >   5df73e2cc8e39da97d56da058667607f1c43acac (AllocateCopyPool
> memory
> > overflow)
> >   2a6ede28fd8efd3051794e1f2727a692d2725fe9
> >   469293f8ee406f2b0bad2cf3bbbc510b2a1364eb
> >
> > Please give your r-b if no more comments. I'd be happy to check in this 
> > patch
> > soon.
> >
> > Thanks,
> > Jian
> >
> > > -Original Message-
> > > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Jian J
> > > Wang
> > > Sent: Friday, October 27, 2017 2:12 PM
> > > To: edk2-devel@lists.01.org
> > > Subject: [edk2] [PATCH v4 0/7] Implement heap guard feature
> > >
> > > > Path V4 changes:
> > > > a. Change names of gEdkiiSmmMemoryAttributeProtocolGuid related
> > > >definitions from EFI_ to EDKII_
> > > > b. Coding style cleanup
> > > > c. Split patches in a more reasonable order and groups
> > >
> > > > Patch V3 changes:
> > > > a. Add new protocol gEdkiiSmmMemoryAttributeProtocolGuid to do
> > > >memory attributes update instead of doing it directly in SmmCore
> > > > b. Fix GCC build error
> > >
> > > > Patch V2 changes:
> > > > a. Remove local variable initializer with memory copy from globals
> > > > b. Change map table dump code to use DEBUG_PAGE|DEBUG_POOL level
> > > >message
> > > > c. Fix malfunction in 32-bit boot mode
> > > > d. Add comment for the use of mOnGuarding
> > > > e. Change name of function InitializePageTableLib to
> > > >InitializePageTableGlobals
> > > > f. Add code in 32-bit code to bypass setting page table to read-only
> > > > g. Coding style clean-up
> > > >
> > >
> > > This feature makes use of paging mechanism to add a hidden (not present)
> > > page just before and after the allocated memory block. If the code tries
> > > to access memory outside of the allocated part, page fault exception will
> > > be triggered.
> > >
> > > This feature is disabled by default and is not recommended to enable it
> > > in production build of BIOS.
> > >
> > > This patch has passed following validations:
> > >
> > >   a. Boot to shell (OVMF, Intel real platform)(32/64)
> > >   b. Boot to Fedora 25 (64)
> > >
> > > NT32 emulation platform was not validated with this feature enabled
> > > due to the fact that it doesn't support paging which is needed for
> > > this feature to work. But all are validated with feature is disabled.
> > >
> > > Suggested-by: Ayellet Wolman 
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Jian J Wang 
> > >
> > > Jian J Wang (7):
> > >   MdeModulePkg/MdeModulePkg.dec,.uni: Add Protocol, PCDs and string
> > > tokens
> > >   MdeModulePkg/SmmMemoryAttribute.h: Add new protocol definitions
> > >   UefiCpuPkg/CpuDxe: Reduce debug message
> > >   MdeModulePkg/DxeIpl: Enable paging for heap guard
> > >   MdeModulePkg/DxeCore: 

Re: [edk2] [PATCH 3/4] UefiCpuPkg/MtrrLib: Update algorithm to calculate optimal settings

[Ni, Ruiyu]

Jordan, Laszlo,

I didn't realize that a platform may have less than 4-page stack before memory 
is ready.
If I was aware of that, I would change the default scratch buffer size to 2 
page, which
should be enough too.

But I do not think we may need to change the scratch buffer size.
Let me clarify about the MtrrLib API usage:
  Though the library is a BASE type, and it's MP-safe, it's not recommended to 
call
  MtrrSetMemoryAttribute...() in AP. Per IA32 SDM, all processors should use the
  same MTRR settings. In UEFI practice, we always just call the 
MtrrSetMemoryAttribute...()
  in BSP side, and then use MtrrGetAllMtrrs()/MtrrSetAllMtrrs() to sync the 
changes to
  all other Aps.


Thanks/Ray

> -Original Message-
> From: Justen, Jordan L
> Sent: Thursday, November 9, 2017 9:54 AM
> To: Laszlo Ersek ; Ni, Ruiyu 
> Cc: Dong, Eric ; Ard Biesheuvel
> ; edk2-devel@lists.01.org; Yao, Jiewen
> ; Kinney, Michael D 
> Subject: Re: [edk2] [PATCH 3/4] UefiCpuPkg/MtrrLib: Update algorithm to
> calculate optimal settings
> 
> On 2017-11-08 17:36:01, Laszlo Ersek wrote:
> > Hi Ray,
> >
> > On 10/12/17 10:48, Ruiyu Ni wrote:
> > > The new algorithm converts the problem calculating optimal MTRR
> > > settings (using least MTRR registers) to the problem finding the
> > > shortest path in a graph.
> > > The memory required in extreme but rare case can be up to 256KB, so
> > > using local stack buffer is impossible considering current DxeIpl
> > > only allocates 128KB stack.
> > >
> > > The patch changes existing MtrrSetMemoryAttributeInMtrrSettings()
> > > and
> > > MtrrSetMemoryAttribute() to use the 4-page stack buffer for
> > > calculation. The two APIs return BUFFER_TOO_SMALL when the buffer is
> > > too small for calculation.
> >
> > [snip]
> >
> > > +#define SCRATCH_BUFFER_SIZE   (4 * SIZE_4KB)
> >
> > [snip]
> >
> > >  RETURN_STATUS
> > >  EFIAPI
> > > -MtrrSetMemoryAttribute (
> > > +MtrrSetMemoryAttributeInMtrrSettings (
> > > +  IN OUT MTRR_SETTINGS   *MtrrSetting,
> > >IN PHYSICAL_ADDRESSBaseAddress,
> > >IN UINT64  Length,
> > >IN MTRR_MEMORY_CACHE_TYPE  Attribute
> > >)
> > >  {
> > >RETURN_STATUS  Status;
> > > +  UINT8  Scratch[SCRATCH_BUFFER_SIZE];
> >
> > [snip]
> >
> > (This patch is now commit 2bbd7e2fbd4b.)
> >
> > Today I managed to spend time on
> >
> >   https://bugzilla.tianocore.org/show_bug.cgi?id=747
> >
> > (which is in turn based on the earlier mailing list thread
> >
> >   [edk2] dynamic PCD impact on temporary PEI memory
> >   https://lists.01.org/pipermail/edk2-devel/2017-October/016213.html
> > ).
> >
> > While writing the patches, I found the root cause of BZ#747:
> > "OvmfPkg/PlatformPei" calls MtrrLib APIs, and due to the above 16KB
> > stack allocation, MtrrLib overflow's OVMF's 16KB (total) temp SEC/PEI
> > stack.
> 
> I thought it was considered bad form to use a significant portion of the 
> stack (>
> ~100 bytes) via local variables. This used to occasionally break MSVC builds 
> as
> MS would insert a stack check call if the locals size exceeded some threshold.
> 
> For a BASE library, I think this should go beyond "bad form" and into not
> allowed.
> 
> -Jordan
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] MdeModulePkg/TerminalDxe: Fix PCANSI mapping for TRIANGLE and ARROW

[Ni, Ruiyu]

Thanks for the explanation.
Reviewed-by: Ruiyu Ni 

Thanks/Ray

> -Original Message-
> From: Kinney, Michael D
> Sent: Thursday, November 9, 2017 12:29 AM
> To: Ni, Ruiyu ; edk2-devel@lists.01.org; Kinney, Michael
> D 
> Cc: Dong, Eric ; Zeng, Star 
> Subject: RE: [edk2] [Patch] MdeModulePkg/TerminalDxe: Fix PCANSI
> mapping for TRIANGLE and ARROW
> 
> When DOS uses CodePage 437, 18 is an up arrow glyph.
> 
> A PC ANSI terminal interprets 18 as a control character.
> 
> Mike
> 
> > -Original Message-
> > From: Ni, Ruiyu
> > Sent: Tuesday, November 7, 2017 11:47 PM
> > To: Kinney, Michael D ;
> > edk2-devel@lists.01.org
> > Cc: Dong, Eric ; Zeng, Star 
> > Subject: RE: [edk2] [Patch] MdeModulePkg/TerminalDxe:
> > Fix PCANSI mapping for TRIANGLE and ARROW
> >
> > Mike,
> > I am a bit confused about mapping 0x18 to upper arrow.
> > I remembered that in old days, pressing ALT+18 in DOS window can
> > generate upper arrow.
> >
> > Thanks/Ray
> >
> > > -Original Message-
> > > From: edk2-devel [mailto:edk2-devel-
> > boun...@lists.01.org] On Behalf Of
> > > Michael D Kinney
> > > Sent: Wednesday, November 8, 2017 12:24 PM
> > > To: edk2-devel@lists.01.org
> > > Cc: Dong, Eric ; Zeng, Star
> > 
> > > Subject: [edk2] [Patch] MdeModulePkg/TerminalDxe: Fix
> > PCANSI mapping
> > > for TRIANGLE and ARROW
> > >
> > > https://bugzilla.tianocore.org/show_bug.cgi?id=761
> > >
> > > When a TerminalType is set to PCANSI, characters in
> > the range 0x00 to 0x1F
> > > are control characters.  The mapping table for PCANSI
> > maps TRIANGLE glyphs,
> > > ARROW_UP glyph, and ARROW_DOWN glyph into this
> > control character
> > > range and that causes no characters to be displayed
> > by PCANSI compatible
> > > terminal emulators.
> > >
> > > The mappings are updated so these glyphs are mapped
> > to ANSI characters in
> > > the range 0x20 to 0x7E.
> > >
> > > GEOMETRICSHAPE_UP_TRIANGLE '^'
> > > GEOMETRICSHAPE_RIGHT_TRIANGLE  '>'
> > > GEOMETRICSHAPE_DOWN_TRIANGLE   'v'
> > > GEOMETRICSHAPE_LEFT_TRIANGLE   '<'
> > > ARROW_UP   '^'
> > > ARROW_DOWN 'v'
> > >
> > > Cc: Star Zeng 
> > > Cc: Eric Dong 
> > > Contributed-under: TianoCore Contribution Agreement
> > 1.1
> > > Signed-off-by: Michael D Kinney
> > 
> > > ---
> > >  .../Universal/Console/TerminalDxe/TerminalConOut.c
> > | 18 +
> > > -
> > >  1 file changed, 9 insertions(+), 9 deletions(-)
> > >
> > > diff --git
> > >
> > a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalCo
> > nOut.c
> > >
> > b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalCo
> > nOut.c
> > > index e677a76e6b..5a8343162f 100644
> > > ---
> > a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalCo
> > nOut.c
> > > +++
> > b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalCo
> > nOut.c
> > > @@ -66,15 +66,15 @@ UNICODE_TO_CHAR
> > UnicodeToPcAnsiOrAscii[] = {
> > >{ BLOCKELEMENT_FULL_BLOCK,0xdb, L'*'
> > },
> > >{ BLOCKELEMENT_LIGHT_SHADE,   0xb0, L'+'
> > },
> > >
> > > -  { GEOMETRICSHAPE_UP_TRIANGLE, 0x1e, L'^'
> > },
> > > -  { GEOMETRICSHAPE_RIGHT_TRIANGLE,  0x10, L'>'
> > },
> > > -  { GEOMETRICSHAPE_DOWN_TRIANGLE,   0x1f, L'v'
> > },
> > > -  { GEOMETRICSHAPE_LEFT_TRIANGLE,   0x11, L'<'
> > },
> > > -
> > > -  { ARROW_LEFT, 0x3c, L'<'
> > },
> > > -  { ARROW_UP,   0x18, L'^'
> > },
> > > -  { ARROW_RIGHT,0x3e, L'>'
> > },
> > > -  { ARROW_DOWN, 0x19, L'v'
> > },
> > > +  { GEOMETRICSHAPE_UP_TRIANGLE, '^', L'^' },
> > > +  { GEOMETRICSHAPE_RIGHT_TRIANGLE,  '>', L'>' },
> > > +  { GEOMETRICSHAPE_DOWN_TRIANGLE,   'v', L'v' },
> > > +  { GEOMETRICSHAPE_LEFT_TRIANGLE,   '<', L'<' },
> > > +
> > > +  { ARROW_LEFT, '<', L'<' },
> > > +  { ARROW_UP,   '^', L'^' },
> > > +  { ARROW_RIGHT,'>', L'>' },
> > > +  { ARROW_DOWN, 'v', L'v' },
> > >
> > >{ 0x, 0x00, L'\0'
> > }
> > >  };
> > > --
> > > 2.14.2.windows.3
> > >
> > > ___
> > > 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 v4 0/7] Implement heap guard feature

[Yao, Jiewen]

That is great.

I have minor suggestion below:
1) Since StaticPaging and HeapGuard are conflicted feature, I suggest we use 
ASSERT to tell the end user, instead of disable StaticPaging silently.

+  //
+  // Don't mark page table as read-only if heap guard is enabled.
+  //
+  //  BIT2: SMM page guard enabled
+  //  BIT3: SMM pool guard enabled
+  //
+  if ((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT3 | BIT2)) != 0) {
+return ;
+  }
+

2) I do not think we need add below in 
MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf. Would you please double confirm?

+  CpuLib

+  gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask## 
CONSUMES


With ASSERT added and cleanup in PiSmmCore.inf, reviewed-by: 
jiewen@intel.com


Thank you
Yao Jiewen



> -Original Message-
> From: Wang, Jian J
> Sent: Thursday, November 9, 2017 10:00 AM
> To: Yao, Jiewen ; Dong, Eric ;
> Zeng, Star ; Ni, Ruiyu ; Laszlo Ersek
> ; Kinney, Michael D 
> Cc: Wang, Jian J ; edk2-devel@lists.01.org
> Subject: RE: [edk2] [PATCH v4 0/7] Implement heap guard feature
> 
> Just a friendly reminder.
> 
> The recent commits have fixed two blocking issues for checking in heap guard
> feature.
> 
>   6fe575d052e36b243657a5885b5457decac41f03 (BaseCryptLib memory
> overflow)
>   cf8197a39d07179027455421a182598bd698
>   5df73e2cc8e39da97d56da058667607f1c43acac (AllocateCopyPool memory
> overflow)
>   2a6ede28fd8efd3051794e1f2727a692d2725fe9
>   469293f8ee406f2b0bad2cf3bbbc510b2a1364eb
> 
> Please give your r-b if no more comments. I'd be happy to check in this patch
> soon.
> 
> Thanks,
> Jian
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jian 
> > J
> > Wang
> > Sent: Friday, October 27, 2017 2:12 PM
> > To: edk2-devel@lists.01.org
> > Subject: [edk2] [PATCH v4 0/7] Implement heap guard feature
> >
> > > Path V4 changes:
> > > a. Change names of gEdkiiSmmMemoryAttributeProtocolGuid related
> > >definitions from EFI_ to EDKII_
> > > b. Coding style cleanup
> > > c. Split patches in a more reasonable order and groups
> >
> > > Patch V3 changes:
> > > a. Add new protocol gEdkiiSmmMemoryAttributeProtocolGuid to do
> > >memory attributes update instead of doing it directly in SmmCore
> > > b. Fix GCC build error
> >
> > > Patch V2 changes:
> > > a. Remove local variable initializer with memory copy from globals
> > > b. Change map table dump code to use DEBUG_PAGE|DEBUG_POOL level
> > >message
> > > c. Fix malfunction in 32-bit boot mode
> > > d. Add comment for the use of mOnGuarding
> > > e. Change name of function InitializePageTableLib to
> > >InitializePageTableGlobals
> > > f. Add code in 32-bit code to bypass setting page table to read-only
> > > g. Coding style clean-up
> > >
> >
> > This feature makes use of paging mechanism to add a hidden (not present)
> > page just before and after the allocated memory block. If the code tries
> > to access memory outside of the allocated part, page fault exception will
> > be triggered.
> >
> > This feature is disabled by default and is not recommended to enable it
> > in production build of BIOS.
> >
> > This patch has passed following validations:
> >
> >   a. Boot to shell (OVMF, Intel real platform)(32/64)
> >   b. Boot to Fedora 25 (64)
> >
> > NT32 emulation platform was not validated with this feature enabled
> > due to the fact that it doesn't support paging which is needed for
> > this feature to work. But all are validated with feature is disabled.
> >
> > Suggested-by: Ayellet Wolman 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang 
> >
> > Jian J Wang (7):
> >   MdeModulePkg/MdeModulePkg.dec,.uni: Add Protocol, PCDs and string
> > tokens
> >   MdeModulePkg/SmmMemoryAttribute.h: Add new protocol definitions
> >   UefiCpuPkg/CpuDxe: Reduce debug message
> >   MdeModulePkg/DxeIpl: Enable paging for heap guard
> >   MdeModulePkg/DxeCore: Implement heap guard feature for UEFI
> >   UefiCpuPkg/PiSmmCpuDxeSmm: Add SmmMemoryAttribute protocol
> >   MdeModulePkg/PiSmmCore: Implement heap guard feature for SMM
> mode
> >
> >  MdeModulePkg/Core/Dxe/DxeMain.inf  |4 +
> >  MdeModulePkg/Core/Dxe/Mem/HeapGuard.c  | 1182
> > 
> >  MdeModulePkg/Core/Dxe/Mem/HeapGuard.h  |  394
> ++
> >  MdeModulePkg/Core/Dxe/Mem/Imem.h   |   38 +-
> >  MdeModulePkg/Core/Dxe/Mem/Page.c   |  130 +-
> >  MdeModulePkg/Core/Dxe/Mem/Pool.c   |  154 +-
> >  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf|1 +
> >  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c|   36 +-
> >  MdeModulePkg/Core/PiSmmCore/HeapGuard.c| 1467
> > 

Re: [edk2] [PATCH v4 0/7] Implement heap guard feature

[Wang, Jian J]

Just a friendly reminder.

The recent commits have fixed two blocking issues for checking in heap guard 
feature.

  6fe575d052e36b243657a5885b5457decac41f03 (BaseCryptLib memory overflow)
  cf8197a39d07179027455421a182598bd698
  5df73e2cc8e39da97d56da058667607f1c43acac (AllocateCopyPool memory 
overflow)
  2a6ede28fd8efd3051794e1f2727a692d2725fe9
  469293f8ee406f2b0bad2cf3bbbc510b2a1364eb

Please give your r-b if no more comments. I'd be happy to check in this patch 
soon.

Thanks,
Jian

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jian J
> Wang
> Sent: Friday, October 27, 2017 2:12 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH v4 0/7] Implement heap guard feature
> 
> > Path V4 changes:
> > a. Change names of gEdkiiSmmMemoryAttributeProtocolGuid related
> >definitions from EFI_ to EDKII_
> > b. Coding style cleanup
> > c. Split patches in a more reasonable order and groups
> 
> > Patch V3 changes:
> > a. Add new protocol gEdkiiSmmMemoryAttributeProtocolGuid to do
> >memory attributes update instead of doing it directly in SmmCore
> > b. Fix GCC build error
> 
> > Patch V2 changes:
> > a. Remove local variable initializer with memory copy from globals
> > b. Change map table dump code to use DEBUG_PAGE|DEBUG_POOL level
> >message
> > c. Fix malfunction in 32-bit boot mode
> > d. Add comment for the use of mOnGuarding
> > e. Change name of function InitializePageTableLib to
> >InitializePageTableGlobals
> > f. Add code in 32-bit code to bypass setting page table to read-only
> > g. Coding style clean-up
> >
> 
> This feature makes use of paging mechanism to add a hidden (not present)
> page just before and after the allocated memory block. If the code tries
> to access memory outside of the allocated part, page fault exception will
> be triggered.
> 
> This feature is disabled by default and is not recommended to enable it
> in production build of BIOS.
> 
> This patch has passed following validations:
> 
>   a. Boot to shell (OVMF, Intel real platform)(32/64)
>   b. Boot to Fedora 25 (64)
> 
> NT32 emulation platform was not validated with this feature enabled
> due to the fact that it doesn't support paging which is needed for
> this feature to work. But all are validated with feature is disabled.
> 
> Suggested-by: Ayellet Wolman 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> 
> Jian J Wang (7):
>   MdeModulePkg/MdeModulePkg.dec,.uni: Add Protocol, PCDs and string
> tokens
>   MdeModulePkg/SmmMemoryAttribute.h: Add new protocol definitions
>   UefiCpuPkg/CpuDxe: Reduce debug message
>   MdeModulePkg/DxeIpl: Enable paging for heap guard
>   MdeModulePkg/DxeCore: Implement heap guard feature for UEFI
>   UefiCpuPkg/PiSmmCpuDxeSmm: Add SmmMemoryAttribute protocol
>   MdeModulePkg/PiSmmCore: Implement heap guard feature for SMM mode
> 
>  MdeModulePkg/Core/Dxe/DxeMain.inf  |4 +
>  MdeModulePkg/Core/Dxe/Mem/HeapGuard.c  | 1182
> 
>  MdeModulePkg/Core/Dxe/Mem/HeapGuard.h  |  394 ++
>  MdeModulePkg/Core/Dxe/Mem/Imem.h   |   38 +-
>  MdeModulePkg/Core/Dxe/Mem/Page.c   |  130 +-
>  MdeModulePkg/Core/Dxe/Mem/Pool.c   |  154 +-
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf|1 +
>  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c|   36 +-
>  MdeModulePkg/Core/PiSmmCore/HeapGuard.c| 1467
> 
>  MdeModulePkg/Core/PiSmmCore/HeapGuard.h|  398 ++
>  MdeModulePkg/Core/PiSmmCore/Page.c |   52 +-
>  MdeModulePkg/Core/PiSmmCore/PiSmmCore.c|7 +-
>  MdeModulePkg/Core/PiSmmCore/PiSmmCore.h|   81 +-
>  MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf  |8 +
>  MdeModulePkg/Core/PiSmmCore/Pool.c |   81 +-
>  MdeModulePkg/Include/Protocol/SmmMemoryAttribute.h |  136 ++
>  MdeModulePkg/MdeModulePkg.dec  |   60 +
>  MdeModulePkg/MdeModulePkg.uni  |   58 +
>  UefiCpuPkg/CpuDxe/CpuPageTable.c   |5 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c   |   10 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c |   20 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h |   98 ++
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf   |2 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c |  163 +++
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c|   10 +-
>  25 files changed, 4496 insertions(+), 99 deletions(-)
>  create mode 100644 MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
>  create mode 100644 MdeModulePkg/Core/Dxe/Mem/HeapGuard.h
>  create mode 100644 MdeModulePkg/Core/PiSmmCore/HeapGuard.c
>  create mode 100644 MdeModulePkg/Core/PiSmmCore/HeapGuard.h
>  create mode 100644 

Re: [edk2] [PATCH 3/4] UefiCpuPkg/MtrrLib: Update algorithm to calculate optimal settings

[Jordan Justen]

On 2017-11-08 17:36:01, Laszlo Ersek wrote:
> Hi Ray,
> 
> On 10/12/17 10:48, Ruiyu Ni wrote:
> > The new algorithm converts the problem calculating optimal
> > MTRR settings (using least MTRR registers) to the problem finding
> > the shortest path in a graph.
> > The memory required in extreme but rare case can be up to 256KB,
> > so using local stack buffer is impossible considering current
> > DxeIpl only allocates 128KB stack.
> > 
> > The patch changes existing MtrrSetMemoryAttributeInMtrrSettings() and
> > MtrrSetMemoryAttribute() to use the 4-page stack buffer for
> > calculation. The two APIs return BUFFER_TOO_SMALL when the buffer
> > is too small for calculation.
> 
> [snip]
> 
> > +#define SCRATCH_BUFFER_SIZE   (4 * SIZE_4KB)
> 
> [snip]
> 
> >  RETURN_STATUS
> >  EFIAPI
> > -MtrrSetMemoryAttribute (
> > +MtrrSetMemoryAttributeInMtrrSettings (
> > +  IN OUT MTRR_SETTINGS   *MtrrSetting,
> >IN PHYSICAL_ADDRESSBaseAddress,
> >IN UINT64  Length,
> >IN MTRR_MEMORY_CACHE_TYPE  Attribute
> >)
> >  {
> >RETURN_STATUS  Status;
> > +  UINT8  Scratch[SCRATCH_BUFFER_SIZE];
> 
> [snip]
> 
> (This patch is now commit 2bbd7e2fbd4b.)
> 
> Today I managed to spend time on
> 
>   https://bugzilla.tianocore.org/show_bug.cgi?id=747
> 
> (which is in turn based on the earlier mailing list thread
> 
>   [edk2] dynamic PCD impact on temporary PEI memory
>   https://lists.01.org/pipermail/edk2-devel/2017-October/016213.html
> ).
> 
> While writing the patches, I found the root cause of BZ#747:
> "OvmfPkg/PlatformPei" calls MtrrLib APIs, and due to the above 16KB
> stack allocation, MtrrLib overflow's OVMF's 16KB (total) temp SEC/PEI
> stack.

I thought it was considered bad form to use a significant portion of
the stack (> ~100 bytes) via local variables. This used to
occasionally break MSVC builds as MS would insert a stack check call
if the locals size exceeded some threshold.

For a BASE library, I think this should go beyond "bad form" and into
not allowed.

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

Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map

[Wang, Jian J]

Great. Glad to know my memory is still good enough.

> -Original Message-
> From: Yao, Jiewen
> Sent: Thursday, November 09, 2017 9:49 AM
> To: Wang, Jian J ; Laszlo Ersek ;
> edk2-devel@lists.01.org
> Cc: Dong, Eric 
> Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> RT_CODE in memory map
> 
> FED0 is HPET region. It is MMIO.
> 
> Thank you
> Yao Jiewen
> 
> > -Original Message-
> > From: Wang, Jian J
> > Sent: Thursday, November 9, 2017 8:42 AM
> > To: Laszlo Ersek ; edk2-devel@lists.01.org
> > Cc: Yao, Jiewen ; Dong, Eric 
> > Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> > RT_CODE in memory map
> >
> > Hi Laszlo,
> >
> > The memory range is FED0 - FED003FF. Actually I
> don't
> > know if it's for MMIO or not. I might be mess it with other things.
> >
> > Thanks,
> > Jian
> > > -Original Message-
> > > From: Laszlo Ersek [mailto:ler...@redhat.com]
> > > Sent: Wednesday, November 08, 2017 10:17 PM
> > > To: Wang, Jian J ; edk2-devel@lists.01.org
> > > Cc: Yao, Jiewen ; Dong, Eric 
> > > Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> > > RT_CODE in memory map
> > >
> > > On 11/08/17 01:10, Wang, Jian J wrote:
> > > > Hi Laszlo,
> > > >
> > > >> -Original Message-
> > > >> From: Laszlo Ersek [mailto:ler...@redhat.com]
> > > >> Sent: Wednesday, November 08, 2017 1:14 AM
> > > >> To: Wang, Jian J ; edk2-devel@lists.01.org
> > > >> Cc: Yao, Jiewen ; Dong, Eric
> 
> > > >> Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries
> of
> > > >> RT_CODE in memory map
> > > >>
> > > >> sorry about the late response
> > > >>
> > > >> On 11/03/17 01:57, Jian J Wang wrote:
> > >  v2
> > >  a. Fix an issue which will cause setting capability failure if size 
> > >  is smaller
> > > than a page.
> > > >>>
> > > >>> More than one entry of RT_CODE memory might cause boot problem
> for
> > > >> some
> > > >>> old OSs. This patch will fix this issue to keep OS compatibility as 
> > > >>> much
> > > >>> as possible.
> > > >>>
> > > >>> More detailed information, please refer to
> > > >>> https://bugzilla.tianocore.org/show_bug.cgi?id=753
> > > >>>
> > > >>> Cc: Eric Dong 
> > > >>> Cc: Jiewen Yao 
> > > >>> Cc: Laszlo Ersek 
> > > >>> Contributed-under: TianoCore Contribution Agreement 1.1
> > > >>> Signed-off-by: Jian J Wang 
> > > >>> ---
> > > >>>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++
> > > >>>  1 file changed, 14 insertions(+), 4 deletions(-)
> > > >>>
> > > >>> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > > >> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > > >>> index d312eb66f8..4a7827ebc9 100644
> > > >>> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > > >>> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > > >>> @@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging (
> > > >>>PageLength= 0;
> > > >>>
> > > >>>for (Index = 0; Index < NumberOfDescriptors; Index++) {
> > > >>> -if (MemorySpaceMap[Index].GcdMemoryType ==
> > > >> EfiGcdMemoryTypeNonExistent) {
> > > >>> +if (MemorySpaceMap[Index].GcdMemoryType ==
> > > >> EfiGcdMemoryTypeNonExistent
> > > >>> +|| (MemorySpaceMap[Index].BaseAddress &
> > EFI_PAGE_MASK) != 0
> > > >>> +|| (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0)
> > {
> > > >>>continue;
> > > >>>  }
> > > >>
> > > >> When exactly do the new conditions match?
> > > >>
> > > >> I thought the base addresses and the lengths in the GCD memory space
> > map
> > > >> are all page aligned. Is that not the case?
> > > >>
> > > >> If these conditions are just a sanity check (i.e. we never expect them
> > > >> to fire), then should we perpahs turn them into ASSERT()s?
> > > >>
> > > >
> > > > I found that there's a mmio entry in memory map on OVMF which has size
> > > > less than a page. I didn't encounter this before. Maybe some recent
> changes
> > > > in other part of EDKII caused this situation. So ASSERT is not enough.
> > >
> > > Can you describe the base address and size of this MMIO entry?
> > >
> > > I don't see how it is possible to add such an entry in the first place
> > > -- if you try to add it in PEI, via a HOB, then IIRC HobLib will
> > > ASSERT(). If you try to add it with gDS->AddMemorySpace() in DXE, then
> > > the call should fail.
> > >
> > > I believe it might be useful to investigate this entry more closely.
> > > Knowing the base address could help us.
> > >
> > > Thanks!
> > > Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org

Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map

[Yao, Jiewen]

FED0 is HPET region. It is MMIO.

Thank you
Yao Jiewen

> -Original Message-
> From: Wang, Jian J
> Sent: Thursday, November 9, 2017 8:42 AM
> To: Laszlo Ersek ; edk2-devel@lists.01.org
> Cc: Yao, Jiewen ; Dong, Eric 
> Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> RT_CODE in memory map
> 
> Hi Laszlo,
> 
> The memory range is FED0 - FED003FF. Actually I don't
> know if it's for MMIO or not. I might be mess it with other things.
> 
> Thanks,
> Jian
> > -Original Message-
> > From: Laszlo Ersek [mailto:ler...@redhat.com]
> > Sent: Wednesday, November 08, 2017 10:17 PM
> > To: Wang, Jian J ; edk2-devel@lists.01.org
> > Cc: Yao, Jiewen ; Dong, Eric 
> > Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> > RT_CODE in memory map
> >
> > On 11/08/17 01:10, Wang, Jian J wrote:
> > > Hi Laszlo,
> > >
> > >> -Original Message-
> > >> From: Laszlo Ersek [mailto:ler...@redhat.com]
> > >> Sent: Wednesday, November 08, 2017 1:14 AM
> > >> To: Wang, Jian J ; edk2-devel@lists.01.org
> > >> Cc: Yao, Jiewen ; Dong, Eric 
> > >> Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> > >> RT_CODE in memory map
> > >>
> > >> sorry about the late response
> > >>
> > >> On 11/03/17 01:57, Jian J Wang wrote:
> >  v2
> >  a. Fix an issue which will cause setting capability failure if size is 
> >  smaller
> > than a page.
> > >>>
> > >>> More than one entry of RT_CODE memory might cause boot problem for
> > >> some
> > >>> old OSs. This patch will fix this issue to keep OS compatibility as much
> > >>> as possible.
> > >>>
> > >>> More detailed information, please refer to
> > >>> https://bugzilla.tianocore.org/show_bug.cgi?id=753
> > >>>
> > >>> Cc: Eric Dong 
> > >>> Cc: Jiewen Yao 
> > >>> Cc: Laszlo Ersek 
> > >>> Contributed-under: TianoCore Contribution Agreement 1.1
> > >>> Signed-off-by: Jian J Wang 
> > >>> ---
> > >>>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++
> > >>>  1 file changed, 14 insertions(+), 4 deletions(-)
> > >>>
> > >>> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > >> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > >>> index d312eb66f8..4a7827ebc9 100644
> > >>> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > >>> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > >>> @@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging (
> > >>>PageLength= 0;
> > >>>
> > >>>for (Index = 0; Index < NumberOfDescriptors; Index++) {
> > >>> -if (MemorySpaceMap[Index].GcdMemoryType ==
> > >> EfiGcdMemoryTypeNonExistent) {
> > >>> +if (MemorySpaceMap[Index].GcdMemoryType ==
> > >> EfiGcdMemoryTypeNonExistent
> > >>> +|| (MemorySpaceMap[Index].BaseAddress &
> EFI_PAGE_MASK) != 0
> > >>> +|| (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0)
> {
> > >>>continue;
> > >>>  }
> > >>
> > >> When exactly do the new conditions match?
> > >>
> > >> I thought the base addresses and the lengths in the GCD memory space
> map
> > >> are all page aligned. Is that not the case?
> > >>
> > >> If these conditions are just a sanity check (i.e. we never expect them
> > >> to fire), then should we perpahs turn them into ASSERT()s?
> > >>
> > >
> > > I found that there's a mmio entry in memory map on OVMF which has size
> > > less than a page. I didn't encounter this before. Maybe some recent 
> > > changes
> > > in other part of EDKII caused this situation. So ASSERT is not enough.
> >
> > Can you describe the base address and size of this MMIO entry?
> >
> > I don't see how it is possible to add such an entry in the first place
> > -- if you try to add it in PEI, via a HOB, then IIRC HobLib will
> > ASSERT(). If you try to add it with gDS->AddMemorySpace() in DXE, then
> > the call should fail.
> >
> > I believe it might be useful to investigate this entry more closely.
> > Knowing the base address could help us.
> >
> > Thanks!
> > Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH v3] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map

[Jian J Wang]

> v4:
> a. Remove DoUpdate and check attributes mismatch all the time to avoid
>a logic hole
> b. Add warning message if failed to update capability
> c. Add local variable to hold new attributes to make code cleaner

> v3:
> a. Add comment to explain more on updating memory capabilities
> b. Fix logic hole in updating attributes
> c. Instead of checking illegal memory space address and size, use return
>status of gDS->SetMemorySpaceCapabilities() to skip memory block which
>cannot be updated with new capabilities.

> v2
> a. Fix an issue which will cause setting capability failure if size is smaller
>than a page.

More than one entry of RT_CODE memory might cause boot problem for some
old OSs. This patch will fix this issue to keep OS compatibility as much
as possible.

More detailed information, please refer to
https://bugzilla.tianocore.org/show_bug.cgi?id=753

Cc: Eric Dong 
Cc: Jiewen Yao 
Cc: Laszlo Ersek 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
 UefiCpuPkg/CpuDxe/CpuPageTable.c | 69 +---
 1 file changed, 50 insertions(+), 19 deletions(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index d312eb66f8..a1d804caed 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -789,8 +789,7 @@ RefreshGcdMemoryAttributesFromPaging (
   UINT64  BaseAddress;
   UINT64  PageStartAddress;
   UINT64  Attributes;
-  UINT64  Capabilities;
-  BOOLEAN DoUpdate;
+  UINT64  NewAttributes;
   UINTN   Index;
 
   //
@@ -802,9 +801,8 @@ RefreshGcdMemoryAttributesFromPaging (
 
   GetCurrentPagingContext ();
 
-  DoUpdate  = FALSE;
-  Capabilities  = 0;
   Attributes= 0;
+  NewAttributes = 0;
   BaseAddress   = 0;
   PageLength= 0;
 
@@ -813,6 +811,34 @@ RefreshGcdMemoryAttributesFromPaging (
   continue;
 }
 
+//
+// Sync the actual paging related capabilities back to GCD service first.
+// As a side effect (good one), this can also help to avoid unnecessary
+// memory map entries due to the different capabilities of the same type
+// memory, such as multiple RT_CODE and RT_DATA entries in memory map,
+// which could cause boot failure of some old Linux distro (before v4.3).
+//
+Status = gDS->SetMemorySpaceCapabilities (
+MemorySpaceMap[Index].BaseAddress,
+MemorySpaceMap[Index].Length,
+MemorySpaceMap[Index].Capabilities |
+EFI_MEMORY_PAGETYPE_MASK
+);
+if (EFI_ERROR (Status)) {
+  //
+  // If we cannot udpate the capabilities, we cannot update its
+  // attributes either. So just simply skip current block of memory.
+  //
+  DEBUG ((
+DEBUG_WARN,
+"Failed to update capability: [%d] %016lx - %016lx (%016lx -> 
%016lx)\r\n",
+Index, BaseAddress, BaseAddress + Length - 1,
+MemorySpaceMap[Index].Capabilities,
+MemorySpaceMap[Index].Capabilities | EFI_MEMORY_PAGETYPE_MASK
+));
+  continue;
+}
+
 if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
   //
   // Current memory space starts at a new page. Resetting PageLength will
@@ -826,7 +852,9 @@ RefreshGcdMemoryAttributesFromPaging (
   PageLength -= (MemorySpaceMap[Index].BaseAddress - BaseAddress);
 }
 
-// Sync real page attributes to GCD
+//
+// Sync actual page attributes to GCD
+//
 BaseAddress   = MemorySpaceMap[Index].BaseAddress;
 MemorySpaceLength = MemorySpaceMap[Index].Length;
 while (MemorySpaceLength > 0) {
@@ -842,23 +870,26 @@ RefreshGcdMemoryAttributesFromPaging (
 PageStartAddress  = (*PageEntry) & 
(UINT64)PageAttributeToMask(PageAttribute);
 PageLength= PageAttributeToLength (PageAttribute) - 
(BaseAddress - PageStartAddress);
 Attributes= GetAttributesFromPageEntry (PageEntry);
-
-if (Attributes != (MemorySpaceMap[Index].Attributes & 
EFI_MEMORY_PAGETYPE_MASK)) {
-  DoUpdate = TRUE;
-  Attributes |= (MemorySpaceMap[Index].Attributes & 
~EFI_MEMORY_PAGETYPE_MASK);
-  Capabilities = Attributes | MemorySpaceMap[Index].Capabilities;
-} else {
-  DoUpdate = FALSE;
-}
   }
 
   Length = MIN (PageLength, MemorySpaceLength);
-  if (DoUpdate) {
-gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
-gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
-DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx - 
%016lx (%08lx -> %08lx)\r\n",
- 

Re: [edk2] [PATCH 3/4] UefiCpuPkg/MtrrLib: Update algorithm to calculate optimal settings

[Laszlo Ersek]

Hi Ray,

On 10/12/17 10:48, Ruiyu Ni wrote:
> The new algorithm converts the problem calculating optimal
> MTRR settings (using least MTRR registers) to the problem finding
> the shortest path in a graph.
> The memory required in extreme but rare case can be up to 256KB,
> so using local stack buffer is impossible considering current
> DxeIpl only allocates 128KB stack.
> 
> The patch changes existing MtrrSetMemoryAttributeInMtrrSettings() and
> MtrrSetMemoryAttribute() to use the 4-page stack buffer for
> calculation. The two APIs return BUFFER_TOO_SMALL when the buffer
> is too small for calculation.

[snip]

> +#define SCRATCH_BUFFER_SIZE   (4 * SIZE_4KB)

[snip]

>  RETURN_STATUS
>  EFIAPI
> -MtrrSetMemoryAttribute (
> +MtrrSetMemoryAttributeInMtrrSettings (
> +  IN OUT MTRR_SETTINGS   *MtrrSetting,
>IN PHYSICAL_ADDRESSBaseAddress,
>IN UINT64  Length,
>IN MTRR_MEMORY_CACHE_TYPE  Attribute
>)
>  {
>RETURN_STATUS  Status;
> +  UINT8  Scratch[SCRATCH_BUFFER_SIZE];

[snip]

(This patch is now commit 2bbd7e2fbd4b.)

Today I managed to spend time on

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

(which is in turn based on the earlier mailing list thread

  [edk2] dynamic PCD impact on temporary PEI memory
  https://lists.01.org/pipermail/edk2-devel/2017-October/016213.html
).

While writing the patches, I found the root cause of BZ#747:
"OvmfPkg/PlatformPei" calls MtrrLib APIs, and due to the above 16KB
stack allocation, MtrrLib overflow's OVMF's 16KB (total) temp SEC/PEI
stack. Because the temp SEC/PEI heap is just below the stack, and the
stack grows down, this overflow by to the large Scratch array corrupts
the heap (for example, various HOBs).

Now, I'm fixing this for OVMF by enlarging its temp SEC/PEI RAM (the
patches are mostly ready for posting), but I have a different concern:

MtrrLib is MP-safe, meaning that it can be called from APs as well, for
setting up MTRRs on APs. (The Intel SDM basically requires all
processors to use the same MTRR settings, which must be configured on
all APs in parallel.) Hence it seems to me that the above Stack array
(16KB in size) must fit into the stack of *each* AP.

In particular I'm concerned about the UefiCpuPkg/PiSmmCpuDxeSmm driver.
In OVMF we have seen SMM stack overflow before. The following two
commits were added back then:

- 509f8425b75d ("UefiCpuPkg: change PcdCpuSmmStackGuard default to
TRUE", 2016-06-01)

- 0d0c245dfb14 ("OvmfPkg: set SMM stack size to 16KB", 2016-06-01)

However: the default SMM stack size (in "UefiCpuPkg.dec") remains 8KB
(PcdCpuSmmStackSize). Furthermore, the guard page can only catch
accesses that are *slightly* below the stack base address. If an
overflow is several pages out of bounds, then the wrong access will skip
over the guard page.

The same worry might apply, via MpInitLib, and the PcdCpuApStackSize
PCD, to:

- UefiCpuPkg/CpuMpPei/CpuMpPei.inf (produces the MP services PPI),
- UefiCpuPkg/CpuDxe/CpuDxe.inf (produces the MP services protocol).

(Although the default value for PcdCpuApStackSize is larger: 32KB).

Do we need to audit all the AP stacks to see if they can accommodate the
Scratch array?

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

Re: [edk2] [PATCH v3 2/3] ShellPkg: Fix misuses of AllocateCopyPool

[Wang, Jian J]

You're right StrCpyS is better for this case. Since the patch has checked in,
maybe we can refine those code in another patch. Thanks for the comment.

> -Original Message-
> From: Carsey, Jaben
> Sent: Wednesday, November 08, 2017 11:50 PM
> To: Wang, Jian J ; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu ; Bi, Dandan 
> Subject: RE: [PATCH v3 2/3] ShellPkg: Fix misuses of AllocateCopyPool
> 
> Why not use the StrCpy_s function to copy strings?  CopyMem and StrSize feels
> odd to me.
> 
> > -Original Message-
> > From: Wang, Jian J
> > Sent: Tuesday, November 07, 2017 6:12 PM
> > To: edk2-devel@lists.01.org
> > Cc: Carsey, Jaben ; Ni, Ruiyu
> > ; Bi, Dandan 
> > Subject: [PATCH v3 2/3] ShellPkg: Fix misuses of AllocateCopyPool
> > Importance: High
> >
> > > v3:
> > >   No update
> >
> > > v2:
> > > a. Use ReallocatePool instead of allocating then copying wherever
> > applicable
> >
> > AllocateCopyPool(AllocationSize, *Buffer) will copy "AllocationSize" bytes 
> > of
> > memory from old "Buffer" to new allocated one. If "AllocationSize" is bigger
> > than size of "Buffer", heap memory overflow occurs during copy.
> >
> > One solution is to allocate pool first then copy the necessary bytes to new
> > memory. Another is using ReallocatePool instead if old buffer will be freed
> > on spot.
> >
> > Cc: Jaben Carsey 
> > Cc: Ruiyu Ni 
> > Cc: Bi Dandan 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang 
> > ---
> >  ShellPkg/Application/Shell/Shell.c | 4 +++-
> >  ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c | 7
> > +--
> >  2 files changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/ShellPkg/Application/Shell/Shell.c
> > b/ShellPkg/Application/Shell/Shell.c
> > index 5471930ba1..656206fdce 100644
> > --- a/ShellPkg/Application/Shell/Shell.c
> > +++ b/ShellPkg/Application/Shell/Shell.c
> > @@ -1646,7 +1646,7 @@ ShellConvertVariables (
> >//
> >// now do the replacements...
> >//
> > -  NewCommandLine1 = AllocateCopyPool(NewSize, OriginalCommandLine);
> > +  NewCommandLine1 = AllocateZeroPool (NewSize);
> >NewCommandLine2 = AllocateZeroPool(NewSize);
> >ItemTemp= AllocateZeroPool(ItemSize+(2*sizeof(CHAR16)));
> >if (NewCommandLine1 == NULL || NewCommandLine2 == NULL ||
> > ItemTemp == NULL) {
> > @@ -1655,6 +1655,8 @@ ShellConvertVariables (
> >  SHELL_FREE_NON_NULL(ItemTemp);
> >  return (NULL);
> >}
> > +  CopyMem (NewCommandLine1, OriginalCommandLine, StrSize
> > (OriginalCommandLine));
> > +
> >for (MasterEnvList = EfiShellGetEnv(NULL)
> >  ;  MasterEnvList != NULL && *MasterEnvList != CHAR_NULL
> >  ;  MasterEnvList += StrLen(MasterEnvList) + 1
> > diff --git
> > a/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
> > b/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
> > index 1122c89b8b..ee3db63358 100644
> > ---
> > a/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
> > +++
> > b/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
> > @@ -143,10 +143,11 @@ UpdateOptionalData(
> >  OriginalOptionDataSize += (*(UINT16*)(OriginalData + sizeof(UINT32)));
> >  OriginalOptionDataSize -= OriginalSize;
> >  NewSize = OriginalSize - OriginalOptionDataSize + DataSize;
> > -NewData = AllocateCopyPool(NewSize, OriginalData);
> > +NewData = AllocatePool(NewSize);
> >  if (NewData == NULL) {
> >Status = EFI_OUT_OF_RESOURCES;
> >  } else {
> > +  CopyMem (NewData, OriginalData, OriginalSize -
> > OriginalOptionDataSize);
> >CopyMem(NewData + OriginalSize - OriginalOptionDataSize, Data,
> > DataSize);
> >  }
> >}
> > @@ -1120,11 +1121,13 @@ BcfgAddOpt(
> >  // Now we know how many EFI_INPUT_KEY structs we need to attach to
> > the end of the EFI_KEY_OPTION struct.
> >  // Re-allocate with the added information.
> >  //
> > -KeyOptionBuffer = AllocateCopyPool(sizeof(EFI_KEY_OPTION) +
> > (sizeof(EFI_INPUT_KEY) * NewKeyOption.KeyData.Options.InputKeyCount),
> > );
> > +KeyOptionBuffer = AllocatePool (sizeof(EFI_KEY_OPTION) +
> > (sizeof(EFI_INPUT_KEY) *
> > NewKeyOption.KeyData.Options.InputKeyCount));
> >  if (KeyOptionBuffer == NULL) {
> >ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_NO_MEM),
> > gShellBcfgHiiHandle, L"bcfg");
> >ShellStatus = SHELL_OUT_OF_RESOURCES;
> > +  return ShellStatus;
> >  }
> > +CopyMem (KeyOptionBuffer, ,
> > sizeof(EFI_KEY_OPTION));
> >}
> >for (LoopCounter = 0 ; ShellStatus == SHELL_SUCCESS && LoopCounter <
> > NewKeyOption.KeyData.Options.InputKeyCount; 

Re: [edk2] [PATCH V2] MdeModulePkg SerialDxe: Handle Timeout change more robustly

[Zeng, Star]

Pushed at a7fd8452964c1a6ffeee1fe07537cb900c0ccb07.
Thanks for review and test.

Star
-Original Message-
From: Julien Grall [mailto:julien.gr...@linaro.org] 
Sent: Wednesday, November 8, 2017 10:36 PM
To: Zeng, Star ; edk2-devel@lists.01.org
Cc: Laszlo Ersek ; Ni, Ruiyu 
Subject: Re: [PATCH V2] MdeModulePkg SerialDxe: Handle Timeout change more 
robustly

Hi Star,

On 07/11/17 01:36, Star Zeng wrote:
> https://lists.01.org/pipermail/edk2-devel/2017-October/016479.html
> reported "Xen Console input very slow in recent UEFI" that appears 
> after 4cf3f37c87ba1f9d58072444bd735e40e4779e70 "MdeModulePkg
> SerialDxe: Process timeout consistently in SerialRead".
> 
> Julien did more debugging and find out the following is happening in 
> TerminalConInTimerHandler (MdeModulePkg/Universal/Console/TerminalDxe)
> when a character is received:
> 1) GetControl will return EFI_SERIAL_INPUT_BUFFER_EMPTY unset
>=> Entering in the loop to fetch character from the serial
> 2) GetOneKeyFromSerial()
>=> Return directly with the character read
> 3) Looping as the fifo is not full and no error
> 4) GetOneKeyFromSerial() -> SerialRead()
>=> No more character so SerialPortPoll() will return FALSE and loop
>   until timeout
>=> Return EFI_TIMEOUT
> 5) Exiting the loop from TerminalConInTimerHandler
> 6) Characters are printed
> 
> After some investigation, I found it is related to the Timeout value.
> 
> The Timeout is 100 (1s) by default to follow UEFI spec.
> And the Terminal driver will recalculate and set the Timeout value 
> based on the properties of UART in TerminalDriverBindingStart()/ 
> TerminalConInTimerHandler().
> 
>SerialInTimeOut = 0;
>if (Mode->BaudRate != 0) {
>  //
>  // According to BAUD rate to calculate the timeout value.
>  //
>  SerialInTimeOut = (1 + Mode->DataBits + Mode->StopBits) *
>2 * 100 / (UINTN) Mode->BaudRate;
>}
> 
> For example, based on the PCD values of PcdUartDefaultBaudRate, 
> PcdUartDefaultDataBits and PcdUartDefaultStopBits, SerialInTimeOut =
> (1 + 8  + 1) * 2 * 100 / (UINTN) 115200 = 173 (us).
> 
> When SerialDxe is used,
> TerminalDriverBindingStart()/TerminalConInTimerHandler() ->
>SerialIo->SetAttributes() ->
>  SerialSetAttributes() ->
>SerialPortSetAttributes()
> 
> Some implementations of SerialPortSetAttributes() could handle the 
> input parameters and return RETURN_SUCCESS, for example 
> BaseSerialPortLib16550, then Timeout value will be changed to 173 
> (us), no "slow down" will be observed.
> But some implementations of SerialPortSetAttributes() just return 
> RETURN_UNSUPPORTED, for example XenConsoleSerialPortLib, then Timeout 
> value will be not changed and kept 100 (1s), "slow down" will be 
> observed.
> 
> SerialPortLib instance can be enhanced to 1. Handle the input 
> parameters and return status accordingly instead of just returning 
> RETURN_UNSUPPORTED in SerialPortSetAttributes().
> 2. Just return RETURN_SUCCESS instead of RETURN_UNSUPPORTED in
> SerialPortSetAttributes() if the instance does not care the input 
> parameters at all.
> 
> And SerialDxe can also be enhanced like this patch to be more robust 
> to handle Timeout change.
> 
> Cc: Julien Grall 
> Cc: Laszlo Ersek 
> Cc: Ruiyu Ni 
> Compare against the original parameters
>Suggested-by: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng 

Tested-by: Julien Grall 

Cheers,

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

Re: [edk2] [PATCH v3] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map

[Wang, Jian J]

Hi Laszlo,

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Wednesday, November 08, 2017 10:37 PM
> To: Wang, Jian J ; edk2-devel@lists.01.org
> Cc: Dong, Eric ; Yao, Jiewen 
> Subject: Re: [PATCH v3] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in
> memory map
> 
> On 11/08/17 11:52, Jian J Wang wrote:
> >> v3:
> >> a. Add comment to explain more on updating memory capabilities
> >> b. Fix logic hole in updating attributes
> >> c. Instead of checking illegal memory space address and size, use return
> >>status of gDS->SetMemorySpaceCapabilities() to skip memory block which
> >>cannot be updated with new capabilities.
> >
> >> v2
> >> a. Fix an issue which will cause setting capability failure if size is 
> >> smaller
> >>than a page.
> >
> > More than one entry of RT_CODE memory might cause boot problem for
> some
> > old OSs. This patch will fix this issue to keep OS compatibility as much
> > as possible.
> >
> > More detailed information, please refer to
> > https://bugzilla.tianocore.org/show_bug.cgi?id=753
> >
> > Cc: Eric Dong 
> > Cc: Jiewen Yao 
> > Cc: Laszlo Ersek 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang 
> > ---
> >  UefiCpuPkg/CpuDxe/CpuPageTable.c | 48
> +++-
> >  1 file changed, 38 insertions(+), 10 deletions(-)
> >
> > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > index d312eb66f8..455c713dfc 100644
> > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > @@ -789,7 +789,6 @@ RefreshGcdMemoryAttributesFromPaging (
> >UINT64  BaseAddress;
> >UINT64  PageStartAddress;
> >UINT64  Attributes;
> > -  UINT64  Capabilities;
> >BOOLEAN DoUpdate;
> >UINTN   Index;
> >
> > @@ -803,7 +802,6 @@ RefreshGcdMemoryAttributesFromPaging (
> >GetCurrentPagingContext ();
> >
> >DoUpdate  = FALSE;
> > -  Capabilities  = 0;
> >Attributes= 0;
> >BaseAddress   = 0;
> >PageLength= 0;
> > @@ -813,6 +811,27 @@ RefreshGcdMemoryAttributesFromPaging (
> >continue;
> >  }
> >
> > +//
> > +// Sync the actual paging related capabilities back to GCD service 
> > first.
> > +// As a side effect (good one), this can also help to avoid unnecessary
> > +// memory map entries due to the different capabilities of the same 
> > type
> > +// memory, such as multiple RT_CODE and RT_DATA entries in memory
> map,
> > +// which could cause boot failure of some old Linux distro (before 
> > v4.3).
> > +//
> > +Status = gDS->SetMemorySpaceCapabilities (
> > +MemorySpaceMap[Index].BaseAddress,
> > +MemorySpaceMap[Index].Length,
> > +MemorySpaceMap[Index].Capabilities |
> > +EFI_MEMORY_PAGETYPE_MASK
> > +);
> > +if (EFI_ERROR (Status)) {
> > +  //
> > +  // If we cannot udpate the capabilities, we cannot update its
> > +  // attributes either. So just simply skip current block of memory.
> > +  //
> 
> (1) Can you perhaps add a DEBUG_WARN here?

Sure.

> 
> > +  continue;
> > +}
> > +
> >  if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
> >//
> >// Current memory space starts at a new page. Resetting PageLength 
> > will
> > @@ -826,7 +845,9 @@ RefreshGcdMemoryAttributesFromPaging (
> >PageLength -= (MemorySpaceMap[Index].BaseAddress - BaseAddress);
> >  }
> >
> > -// Sync real page attributes to GCD
> > +//
> > +// Sync actual page attributes to GCD
> > +//
> >  BaseAddress   = MemorySpaceMap[Index].BaseAddress;
> >  MemorySpaceLength = MemorySpaceMap[Index].Length;
> >  while (MemorySpaceLength > 0) {
> > @@ -845,8 +866,6 @@ RefreshGcdMemoryAttributesFromPaging (
> >
> >  if (Attributes != (MemorySpaceMap[Index].Attributes &
> EFI_MEMORY_PAGETYPE_MASK)) {
> >DoUpdate = TRUE;
> > -  Attributes |= (MemorySpaceMap[Index].Attributes &
> ~EFI_MEMORY_PAGETYPE_MASK);
> > -  Capabilities = Attributes | MemorySpaceMap[Index].Capabilities;
> >  } else {
> >DoUpdate = FALSE;
> >  }
> 
> (2) To me it seems like we can remove the "DoUpdate" local variable
> completely. Below, we can replace the DoUpdate check with the actual
> 
>   (Attributes != (MemorySpaceMap[Index].Attributes &
>   EFI_MEMORY_PAGETYPE_MASK))
> 
> check.
> 
> The point is that we check the EFI_MEMORY_PAGETYPE_MASK bit-field of the
> entry's attributes. If they do not 

Re: [edk2] Why do we use -g option of gcc even for RELEASE build?

[Heyi Guo]

That makes sense. Thank you all :)

Regards,

Heyi


在 11/8/2017 11:32 PM, Gao, Liming 写道:

In build_rule.txt, GCC built image will be strip first, then be converted to 
EFI image by GenFw. There is no symbol in the final EFI image. All symbols are 
kept into original GCC built image.

Thanks
Liming

-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com]
Sent: Wednesday, November 8, 2017 10:54 PM
To: Heyi Guo ; edk2-devel@lists.01.org
Cc: Gao, Liming 
Subject: Re: [edk2] Why do we use -g option of gcc even for RELEASE build?

On 11/08/17 03:25, Heyi Guo wrote:

Hi folks,

 From gcc manual, -g option seems to produce debugging information. In
tools_def.template, -g is included in GCC_ALL_CC_FLAGS, so it will also
be enabled for RELEASE build with gcc tool chain. Any special reason to
do that?

In the edk2 tree, -g was added to GCC_ALL_CC_FLAGS and
GCC44_ALL_CC_FLAGS in commit 52302d4dee58 ("Sync EDKII BaseTools to
BaseTools project r1903.", 2010-02-28).

If you check the history of the now-historical separate BaseTools
project , "-g"
was introduced in commit 46c1e64305d4 ("Upgrade the binutil 2.18.50.0.5
to 2.20.51.0.5 for UNIXGCC tool chain", 2010-02-25).

I guess that the same  for [Dynamic-Library-File] must be
able to work for both DEBUG/NOOPT and RELEASE builds. So -g is included
for all of those build targets, in the C flags. Ultimately the debug
symbols are not copied into the .efi binaries, for RELEASE, I believe.

Thanks
Laszlo


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

Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map

[Wang, Jian J]

Hi Laszlo,

The memory range is FED0 - FED003FF. Actually I don't 
know if it's for MMIO or not. I might be mess it with other things.

Thanks,
Jian
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Wednesday, November 08, 2017 10:17 PM
> To: Wang, Jian J ; edk2-devel@lists.01.org
> Cc: Yao, Jiewen ; Dong, Eric 
> Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> RT_CODE in memory map
> 
> On 11/08/17 01:10, Wang, Jian J wrote:
> > Hi Laszlo,
> >
> >> -Original Message-
> >> From: Laszlo Ersek [mailto:ler...@redhat.com]
> >> Sent: Wednesday, November 08, 2017 1:14 AM
> >> To: Wang, Jian J ; edk2-devel@lists.01.org
> >> Cc: Yao, Jiewen ; Dong, Eric 
> >> Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> >> RT_CODE in memory map
> >>
> >> sorry about the late response
> >>
> >> On 11/03/17 01:57, Jian J Wang wrote:
>  v2
>  a. Fix an issue which will cause setting capability failure if size is 
>  smaller
> than a page.
> >>>
> >>> More than one entry of RT_CODE memory might cause boot problem for
> >> some
> >>> old OSs. This patch will fix this issue to keep OS compatibility as much
> >>> as possible.
> >>>
> >>> More detailed information, please refer to
> >>> https://bugzilla.tianocore.org/show_bug.cgi?id=753
> >>>
> >>> Cc: Eric Dong 
> >>> Cc: Jiewen Yao 
> >>> Cc: Laszlo Ersek 
> >>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>> Signed-off-by: Jian J Wang 
> >>> ---
> >>>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++
> >>>  1 file changed, 14 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> >> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> >>> index d312eb66f8..4a7827ebc9 100644
> >>> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> >>> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> >>> @@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging (
> >>>PageLength= 0;
> >>>
> >>>for (Index = 0; Index < NumberOfDescriptors; Index++) {
> >>> -if (MemorySpaceMap[Index].GcdMemoryType ==
> >> EfiGcdMemoryTypeNonExistent) {
> >>> +if (MemorySpaceMap[Index].GcdMemoryType ==
> >> EfiGcdMemoryTypeNonExistent
> >>> +|| (MemorySpaceMap[Index].BaseAddress & EFI_PAGE_MASK) != 0
> >>> +|| (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0) {
> >>>continue;
> >>>  }
> >>
> >> When exactly do the new conditions match?
> >>
> >> I thought the base addresses and the lengths in the GCD memory space map
> >> are all page aligned. Is that not the case?
> >>
> >> If these conditions are just a sanity check (i.e. we never expect them
> >> to fire), then should we perpahs turn them into ASSERT()s?
> >>
> >
> > I found that there's a mmio entry in memory map on OVMF which has size
> > less than a page. I didn't encounter this before. Maybe some recent changes
> > in other part of EDKII caused this situation. So ASSERT is not enough.
> 
> Can you describe the base address and size of this MMIO entry?
> 
> I don't see how it is possible to add such an entry in the first place
> -- if you try to add it in PEI, via a HOB, then IIRC HobLib will
> ASSERT(). If you try to add it with gDS->AddMemorySpace() in DXE, then
> the call should fail.
> 
> I believe it might be useful to investigate this entry more closely.
> Knowing the base address could help us.
> 
> Thanks!
> Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [Patch] QuarkPlatformPkg/Readme.md: Fix markdown format issue

[Michael D Kinney]

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

Remove extra carriage return that causes formatting
issues.

Cc: Kelly Steele 
Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Michael D Kinney 
---
 QuarkPlatformPkg/Readme.md | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/QuarkPlatformPkg/Readme.md b/QuarkPlatformPkg/Readme.md
index aa9d9856bd..126de6937b 100644
--- a/QuarkPlatformPkg/Readme.md
+++ b/QuarkPlatformPkg/Readme.md
@@ -244,8 +244,7 @@ Enable UEFI Secure Boot features:
 
 Enable UEFI Secure Boot and Measured Boot using Atmel I2C TPM hardware device:
 
-```build -a IA32 -t VS2015x86 -p QuarkPlatformPkg/Quark.dsc -D UEFI_SECURE_BOOT
--D MEASURED_BOOT_ENABLE -D TPM_12_HARDWARE=ATMEL_I2C```
+```build -a IA32 -t VS2015x86 -p QuarkPlatformPkg/Quark.dsc -D 
UEFI_SECURE_BOOT -D MEASURED_BOOT_ENABLE -D TPM_12_HARDWARE=ATMEL_I2C```
 
 ## **FLASH Update using DediProg SF100**
 
-- 
2.14.2.windows.3

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

[edk2] [Patch] QuarkPlatformPkg/PlatformBootManagerLib: Update boot mode handling

[Michael D Kinney]

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

Update and simplify boot mode handling logic.  This includes
performing minimum connects for the following boot modes:

  BOOT_ASSUMING_NO_CONFIGURATION_CHANGES
  BOOT_WITH_MINIMAL_CONFIGURATION
  BOOT_ON_S4_RESUME

Cc: Kelly Steele 
Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Michael D Kinney 
---
 .../PlatformBootManagerLib/PlatformBootManager.c   | 30 +-
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git 
a/QuarkPlatformPkg/Library/PlatformBootManagerLib/PlatformBootManager.c 
b/QuarkPlatformPkg/Library/PlatformBootManagerLib/PlatformBootManager.c
index 3c21318069..53391c6077 100644
--- a/QuarkPlatformPkg/Library/PlatformBootManagerLib/PlatformBootManager.c
+++ b/QuarkPlatformPkg/Library/PlatformBootManagerLib/PlatformBootManager.c
@@ -2,7 +2,7 @@
 This file include all platform action which can be customized
 by IBV/OEM.
 
-Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.
+Copyright (c) 2015 - 2017, 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
 which accompanies this distribution.  The full text of the license may be 
found at
@@ -354,9 +354,17 @@ PlatformBootManagerAfterConsole (
   }
 
   BootMode = GetBootModeHob();
+
+  DEBUG((DEBUG_INFO, "PlatformBootManagerAfterConsole(): BootMode = %02x\n", 
BootMode));
+
   switch (BootMode) {
+  case BOOT_ASSUMING_NO_CONFIGURATION_CHANGES:
+  case BOOT_WITH_MINIMAL_CONFIGURATION:
+  case BOOT_ON_S4_RESUME:
+EfiBootManagerRefreshAllBootOption ();
+break;
+
   case BOOT_ON_FLASH_UPDATE:
-DEBUG((DEBUG_INFO, "Capsule Mode detected\n"));
 if (FeaturePcdGet(PcdSupportUpdateCapsuleReset)) {
   EfiBootManagerConnectAll ();
   EfiBootManagerRefreshAllBootOption ();
@@ -374,15 +382,6 @@ PlatformBootManagerAfterConsole (
 }
 break;
 
-  case BOOT_IN_RECOVERY_MODE:
-DEBUG((DEBUG_INFO, "Recovery Mode detected\n"));
-// Passthrough
-
-  case BOOT_ASSUMING_NO_CONFIGURATION_CHANGES:
-  case BOOT_WITH_MINIMAL_CONFIGURATION:
-  case BOOT_WITH_FULL_CONFIGURATION:
-  case BOOT_WITH_FULL_CONFIGURATION_PLUS_DIAGNOSTICS:
-  case BOOT_WITH_DEFAULT_SETTINGS:
   default:
 EfiBootManagerConnectAll ();
 EfiBootManagerRefreshAllBootOption ();
@@ -390,14 +389,9 @@ PlatformBootManagerAfterConsole (
 //
 // Sync ESRT Cache from FMP Instance on demand after Connect All
 //
-if ((BootMode != BOOT_ASSUMING_NO_CONFIGURATION_CHANGES) &&
-(BootMode != BOOT_WITH_MINIMAL_CONFIGURATION) &&
-(BootMode != BOOT_ON_S4_RESUME)) {
-  if (EsrtManagement != NULL) {
-EsrtManagement->SyncEsrtFmp();
-  }
+if (EsrtManagement != NULL) {
+  EsrtManagement->SyncEsrtFmp();
 }
-
 break;
   }
 
-- 
2.14.2.windows.3

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

[edk2] [Patch] MdeModulePkg/UsbMassStorageDxe: Verify Get Max LUN value

[Michael D Kinney]

The USB Mass Storage Class Specification states that a
maximum LUN value larger than 0x0F is invalid.  Add
a check to make sure this maximum LUN value is in this
valid range, and if it is not, then assume that the
device does not support multiple LUNs and return a
maximum LUN value of 0.

This change improves compatibility with USB FLASH drives
that have a single LUN, but return invalid maximum LUN
values.

Cc: Star Zeng 
Cc: Eric Dong 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Michael D Kinney 
---
 MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBot.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBot.c 
b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBot.c
index 4bb7222b89..c7436cf036 100644
--- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBot.c
+++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBot.c
@@ -2,7 +2,7 @@
   Implementation of the USB mass storage Bulk-Only Transport protocol,
   according to USB Mass Storage Class Bulk-Only Transport, Revision 1.0.
 
-Copyright (c) 2007 - 2011, Intel Corporation. All rights reserved.
+Copyright (c) 2007 - 2017, 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
 which accompanies this distribution.  The full text of the license may be 
found at
@@ -576,6 +576,14 @@ UsbBotGetMaxLun (
 1,
 
 );
+  if (!EFI_ERROR (Status) && *MaxLun > USB_BOT_MAX_LUN) {
+//
+// If MaxLun is larger than the maximum LUN value (0x0f) supported by the
+// USB Mass Storage Class Bulk-Only Transport Spec, then set MaxLun to 0
+// which means no LUN is associated with the device.
+//
+*MaxLun = 0;
+  }
 
   return Status;
 }
-- 
2.14.2.windows.3

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

[edk2] [Patch] MdeModulePkg/UsbMassStorageDxe: Fix USB Mass Storage detection

[Michael D Kinney]

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

Update logic to not return an error from UsbBootRequestSense()
if a Request Sense command responds with no sense information.
It is legal for a USB mass storage device to respond to a
Request Sense command with a SenseKey of
USB_BOOT_SENSE_NO_SENSE and an Additional Sense
Code of USB_BOOT_ASC_NO_ADDITIONAL_SENSE_INFORMATION.

This is described in Section 3.3 of the Universal Serial
Bus Mass Storage Specification For Bootability:

http://www.usb.org/developers/docs/devclass_docs/usb_msc_boot_1.0.pdf

The previous logic returned an error of EFI_NO_RESPONSE
and this caused USB mass storage devices such as a USB
floppy drive to not be detected.

Cc: Star Zeng 
Cc: Eric Dong 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Michael D Kinney 
---
 MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c | 11 +--
 MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h |  9 +
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c 
b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
index 0c46f888eb..2eb30f0c5f 100644
--- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
+++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
@@ -2,7 +2,7 @@
   Implementation of the command set of USB Mass Storage Specification
   for Bootability, Revision 1.0.
 
-Copyright (c) 2007 - 2016, Intel Corporation. All rights reserved.
+Copyright (c) 2007 - 2017, 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
 which accompanies this distribution.  The full text of the license may be 
found at
@@ -80,7 +80,14 @@ UsbBootRequestSense (
   switch (USB_BOOT_SENSE_KEY (SenseData.SenseKey)) {
 
   case USB_BOOT_SENSE_NO_SENSE:
-Status = EFI_NO_RESPONSE;
+if (SenseData.Asc == USB_BOOT_ASC_NO_ADDITIONAL_SENSE_INFORMATION) {
+  //
+  // It is not an error if a device does not have additional sense 
information
+  //
+  Status = EFI_SUCCESS;
+} else {
+  Status = EFI_NO_RESPONSE;
+}
 break;
 
   case USB_BOOT_SENSE_RECOVERED:
diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h 
b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h
index c4082558fa..13a926035c 100644
--- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h
+++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h
@@ -2,7 +2,7 @@
   Definition of the command set of USB Mass Storage Specification
   for Bootability, Revision 1.0.
 
-Copyright (c) 2007 - 2012, Intel Corporation. All rights reserved.
+Copyright (c) 2007 - 2017, 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
 which accompanies this distribution.  The full text of the license may be 
found at
@@ -51,9 +51,10 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #define USB_BOOT_SENSE_VOLUME_OVERFLOW  0x0D ///< Partition overflow
 #define USB_BOOT_SENSE_MISCOMPARE   0x0E ///< Source data mis-match while 
verfying.
 
-#define USB_BOOT_ASC_NOT_READY  0x04
-#define USB_BOOT_ASC_NO_MEDIA   0x3A
-#define USB_BOOT_ASC_MEDIA_CHANGE   0x28
+#define USB_BOOT_ASC_NO_ADDITIONAL_SENSE_INFORMATION  0x00
+#define USB_BOOT_ASC_NOT_READY0x04
+#define USB_BOOT_ASC_NO_MEDIA 0x3A
+#define USB_BOOT_ASC_MEDIA_CHANGE 0x28
 
 //
 // Supported PDT codes, or Peripheral Device Type
-- 
2.14.2.windows.3

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

Re: [edk2] [RFC PATCH edk2-platforms 1/3] Silicon/NXP: add RTC support library for PCF8563 I2C IP

[Leif Lindholm]

On Wed, Nov 08, 2017 at 05:45:11PM +, Ard Biesheuvel wrote:
> On 8 November 2017 at 16:47, Leif Lindholm  wrote:
> > On Fri, Nov 03, 2017 at 10:16:52AM +, Ard Biesheuvel wrote:
> >> Add a RealTimeClockLib implementation for the NXP PCF8563 as used on
> >> the Socionext Developer Box board. Note that the standard I2C protocol
> >> stack does not support runtime use, so this driver invokes the I2C master
> >> protocol directly. This requires support from the platform side as well,
> >> and so this driver will only attach to a I2C master that has the
> >> gPcf8563RealTimeClockLibI2cMasterProtolGuid protocol installed on its
> >> handle. It is up to the platform to ensure that the driver producing
> >> the I2C master protocol in question is runtime capable, and is not
> >> shared with the I2C protocol stack (i.e., it should not have the I2C
> >> Bus Configuration Management protocol installed as well).
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Ard Biesheuvel 
> >
> > Reviewed-by: Leif Lindholm 
> 
> Ehm, is this in response to the correct patch, given that you already
> asked for some changes?

No, that's me getting confused by the patches appearing out of order
in my inbox... Apologies.

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

[edk2] [Patch] MdeModulePkg/Core/Dxe: Remove extra connects for UEFI Applications

[Michael D Kinney]

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

The UEFI Specification Boot Services chapter, StartImage() service,
EFF 1.10 Extension requires extra calls to ConnectController()
if a UEFI Driver produces handles. The DXE Core is performing these
extra calls to ConnectController() without evaluating the ImageType.

A filter is added to not make extra calls to ConnectController()
if the ImageType is EFI_IMAGE_SUBSYSTEM_EFI_APPLICATION.

Without this filter, extra calls to ConnectController() may be
performed by UEFI Applications or a UEFI Shell Applications that
also call ConnectController().

Cc: Star Zeng 
Cc: Eric Dong 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Michael D Kinney 
---
 MdeModulePkg/Core/Dxe/Image/Image.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c 
b/MdeModulePkg/Core/Dxe/Image/Image.c
index 4e22aa6dc7..c6b8ff44b9 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -1703,9 +1703,17 @@ CoreStartImage (
   mCurrentImage = LastImage;
 
   //
-  // Go connect any handles that were created or modified while the image 
executed.
+  // UEFI Specification - StartImage() - EFI 1.10 Extension
+  // To maintain compatibility with UEFI drivers that are written to the EFI
+  // 1.02 Specification, StartImage() must monitor the handle database before
+  // and after each image is started. If any handles are created or modified
+  // when an image is started, then EFI_BOOT_SERVICES.ConnectController() must
+  // be called with the Recursive parameter set to TRUE for each of the newly
+  // created or modified handles before StartImage() returns.
   //
-  CoreConnectHandlesByKey (HandleDatabaseKey);
+  if (Image->ImageContext.ImageType != EFI_IMAGE_SUBSYSTEM_EFI_APPLICATION) {
+CoreConnectHandlesByKey (HandleDatabaseKey);
+  }
 
   //
   // Handle the image's returned ExitData
-- 
2.14.2.windows.3

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

Re: [edk2] [RFC PATCH edk2-platforms 1/3] Silicon/NXP: add RTC support library for PCF8563 I2C IP

[Ard Biesheuvel]

On 8 November 2017 at 16:47, Leif Lindholm  wrote:
> On Fri, Nov 03, 2017 at 10:16:52AM +, Ard Biesheuvel wrote:
>> Add a RealTimeClockLib implementation for the NXP PCF8563 as used on
>> the Socionext Developer Box board. Note that the standard I2C protocol
>> stack does not support runtime use, so this driver invokes the I2C master
>> protocol directly. This requires support from the platform side as well,
>> and so this driver will only attach to a I2C master that has the
>> gPcf8563RealTimeClockLibI2cMasterProtolGuid protocol installed on its
>> handle. It is up to the platform to ensure that the driver producing
>> the I2C master protocol in question is runtime capable, and is not
>> shared with the I2C protocol stack (i.e., it should not have the I2C
>> Bus Configuration Management protocol installed as well).
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel 
>
> Reviewed-by: Leif Lindholm 
>

Ehm, is this in response to the correct patch, given that you already
asked for some changes?
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [RFC PATCH edk2-platforms 1/3] Silicon/NXP: add RTC support library for PCF8563 I2C IP

[Ard Biesheuvel]

On 8 November 2017 at 16:43, Leif Lindholm  wrote:
> On Fri, Nov 03, 2017 at 10:16:52AM +, Ard Biesheuvel wrote:
>> Add a RealTimeClockLib implementation for the NXP PCF8563 as used on
>> the Socionext Developer Box board. Note that the standard I2C protocol
>> stack does not support runtime use, so this driver invokes the I2C master
>> protocol directly. This requires support from the platform side as well,
>> and so this driver will only attach to a I2C master that has the
>> gPcf8563RealTimeClockLibI2cMasterProtolGuid protocol installed on its
>> handle. It is up to the platform to ensure that the driver producing
>> the I2C master protocol in question is runtime capable, and is not
>> shared with the I2C protocol stack (i.e., it should not have the I2C
>> Bus Configuration Management protocol installed as well).
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel 
>> ---
>>  Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c   | 
>> 385 
>>  Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.dec |  
>> 29 ++
>>  Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.inf |  
>> 52 +++
>>  3 files changed, 466 insertions(+)
>>
>> diff --git 
>> a/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c 
>> b/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c
>> new file mode 100644
>> index ..fea65a225d7f
>> --- /dev/null
>> +++ b/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c
>> @@ -0,0 +1,385 @@
>> +/** @file
>> +
>> +  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
>> +
>> +  This program and the accompanying materials
>> +  are licensed and made available under the terms and conditions of the BSD 
>> License
>> +  which accompanies this distribution.  The full text of the license may be 
>> found at
>> +  http://opensource.org/licenses/bsd-license.php
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
>> IMPLIED.
>> +
>> +**/
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#define SLAVE_ADDRESS (FixedPcdGet8 (PcdI2cSlaveAddress))
>> +#define PCF8563_DATA_REG_OFFSET   0x2
>> +
>> +#define PCF8563_SECONDS_MASK  0x7f
>> +#define PCF8563_MINUTES_MASK  0x7f
>> +#define PCF8563_HOURS_MASK0x3f
>> +#define PCF8563_DAYS_MASK 0x3f
>> +#define PCF8563_WEEKDAYS_MASK 0x07
>> +#define PCF8563_MONTHS_MASK   0x1f
>> +#define PCF8563_CENTURY_MASK  0x80
>> +
>> +#define EPOCH_BASE2000
>> +
>> +STATIC EFI_HANDLE mI2cMasterHandle;
>> +STATIC VOID   *mDriverEventRegistration;
>> +STATIC EFI_I2C_MASTER_PROTOCOL*mI2cMaster;
>> +STATIC EFI_EVENT  mRtcVirtualAddrChangeEvent;
>> +
>> +typedef struct {
>> +  UINTN   OperationCount;
>> +  EFI_I2C_OPERATION   SetAddressOp;
>> +  EFI_I2C_OPERATION   GetSetDateTimeOp;
>> +} RTC_I2C_REQUEST;
>> +
>> +#pragma pack(1)
>> +typedef struct {
>> +  UINT8 VL_seconds;
>> +  UINT8 Minutes;
>> +  UINT8 Hours;
>> +  UINT8 Days;
>> +  UINT8 Weekdays;
>> +  UINT8 Century_months;
>> +  UINT8 Years;
>> +} RTC_DATETIME;
>> +#pragma pack()
>> +
>> +/**
>> +  Returns the current time and date information, and the time-keeping
>> +  capabilities of the hardware platform.
>> +
>> +  @param  Time  A pointer to storage to receive a snapshot 
>> of
>> +the current time.
>> +  @param  Capabilities  An optional pointer to a buffer to receive 
>> the
>> +real time clock device's capabilities.
>> +
>> +  @retval EFI_SUCCESS   The operation completed successfully.
>> +  @retval EFI_INVALID_PARAMETER Time is NULL.
>> +  @retval EFI_DEVICE_ERROR  The time could not be retrieved due to 
>> hardware
>> +error.
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +LibGetTime (
>> +  OUT EFI_TIME*Time,
>> +  OUT EFI_TIME_CAPABILITIES   *Capabilities
>> +  )
>> +{
>> +  RTC_I2C_REQUEST Op;
>> +  RTC_DATETIMEDateTime;
>> +  EFI_STATUS  Status;
>> +  UINT8   Reg;
>> +
>> +  if (Time == NULL) {
>> +return EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  if (mI2cMaster == NULL) {
>> +return EFI_DEVICE_ERROR;
>> +  }
>> +
>> +  Reg = PCF8563_DATA_REG_OFFSET;
>> +
>> +  Op.OperationCount = 2;
>> +
>> +  Op.SetAddressOp.Flags = 0;
>> +  Op.SetAddressOp.LengthInBytes = 1;
>> +  Op.SetAddressOp.Buffer = 
>> +
>> +  Op.GetSetDateTimeOp.Flags = I2C_FLAG_READ;
>> +  Op.GetSetDateTimeOp.LengthInBytes = sizeof 

Re: [edk2] [RFC PATCH edk2-platforms 1/3] Silicon/NXP: add RTC support library for PCF8563 I2C IP

[Leif Lindholm]

On Fri, Nov 03, 2017 at 10:16:52AM +, Ard Biesheuvel wrote:
> Add a RealTimeClockLib implementation for the NXP PCF8563 as used on
> the Socionext Developer Box board. Note that the standard I2C protocol
> stack does not support runtime use, so this driver invokes the I2C master
> protocol directly. This requires support from the platform side as well,
> and so this driver will only attach to a I2C master that has the
> gPcf8563RealTimeClockLibI2cMasterProtolGuid protocol installed on its
> handle. It is up to the platform to ensure that the driver producing
> the I2C master protocol in question is runtime capable, and is not
> shared with the I2C protocol stack (i.e., it should not have the I2C
> Bus Configuration Management protocol installed as well).
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 

Reviewed-by: Leif Lindholm 

> ---
>  Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c   | 
> 385 
>  Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.dec |  
> 29 ++
>  Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.inf |  
> 52 +++
>  3 files changed, 466 insertions(+)
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [RFC PATCH edk2-platforms 3/3] Platform/DeveloperBox: wire up RTC support

[Leif Lindholm]

On Fri, Nov 03, 2017 at 10:16:54AM +, Ard Biesheuvel wrote:
> Add the drivers, library resolutions and PCD settings to enable RTC
> support on DeveloperBox. Also, update PlatformDxe to register the
> non-discoverable device handles for both I2C controllers.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 

Reviewed-by: Leif Lindholm 

> ---
>  Platform/Socionext/DeveloperBox/DeveloperBox.dsc 
>  |  8 ++-
>  Platform/Socionext/DeveloperBox/DeveloperBox.fdf 
>  |  5 ++
>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
>  | 76 +---
>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf  
>  |  6 +-
>  Silicon/Socionext/SynQuacer/Include/Platform/MemoryMap.h 
>  |  8 +++
>  
> Silicon/Socionext/SynQuacer/Library/SynQuacerMemoryInitPeiLib/SynQuacerMemoryInitPeiLib.c
>  |  4 ++
>  6 files changed, 96 insertions(+), 11 deletions(-)
> 
> diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc 
> b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> index b558b1f805b4..91898faa6ecc 100644
> --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> @@ -446,8 +446,7 @@ [Components.common]
>
> MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
>EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf {
>  
> -  ## TODO
> -  
> RealTimeClockLib|EmbeddedPkg/Library/TemplateRealTimeClockLib/TemplateRealTimeClockLib.inf
> +  
> RealTimeClockLib|Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.inf
>}
>MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf
>MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
> @@ -627,6 +626,11 @@ [Components.common]
>  
>Platform/Socionext/DeveloperBox/OsInstallerMenuDxe/OsInstallerMenuDxe.inf
>  
> +  #
> +  # I2C
> +  #
> +  Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.inf
> +
>  !if $(DO_X86EMU) == TRUE
>#
># x86 emulator
> diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.fdf 
> b/Platform/Socionext/DeveloperBox/DeveloperBox.fdf
> index 19dbcf17c1e1..368bf801e036 100644
> --- a/Platform/Socionext/DeveloperBox/DeveloperBox.fdf
> +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.fdf
> @@ -236,6 +236,11 @@ [FV.FvMain]
>  
>INF 
> Platform/Socionext/DeveloperBox/OsInstallerMenuDxe/OsInstallerMenuDxe.inf
>  
> +  #
> +  # I2C
> +  #
> +  INF Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.inf
> +
>  !if $(DO_X86EMU) == TRUE
>#
># x86 emulator
> diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c 
> b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
> index 1d300b15e25b..b39cbc5f5291 100644
> --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
> +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
> @@ -62,27 +62,61 @@ STATIC EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR mNetsecDesc[] = {
>}
>  };
>  
> +STATIC EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR mI2c0Desc[] = {
> +  {
> +ACPI_ADDRESS_SPACE_DESCRIPTOR,// Desc
> +sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3,   // Len
> +ACPI_ADDRESS_SPACE_TYPE_MEM,  // ResType
> +0,// GenFlag
> +0,// SpecificFlag
> +32,   // AddrSpaceGranularity
> +SYNQUACER_I2C0_BASE,  // AddrRangeMin
> +SYNQUACER_I2C0_BASE + SYNQUACER_I2C0_SIZE - 1,// AddrRangeMax
> +0,// 
> AddrTranslationOffset
> +SYNQUACER_I2C0_SIZE,  // AddrLen
> +  }, {
> +ACPI_END_TAG_DESCRIPTOR   // Desc
> +  }
> +};
> +
> +STATIC EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR mI2c1Desc[] = {
> +  {
> +ACPI_ADDRESS_SPACE_DESCRIPTOR,// Desc
> +sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3,   // Len
> +ACPI_ADDRESS_SPACE_TYPE_MEM,  // ResType
> +0,// GenFlag
> +0,// SpecificFlag
> +32,   // AddrSpaceGranularity
> +SYNQUACER_I2C1_BASE,  // AddrRangeMin
> +SYNQUACER_I2C1_BASE + SYNQUACER_I2C1_SIZE - 1,// AddrRangeMax
> +0,// 
> AddrTranslationOffset
> +SYNQUACER_I2C1_SIZE,  // AddrLen
> +  }, {
> +ACPI_END_TAG_DESCRIPTOR   

Re: [edk2] [RFC PATCH edk2-platforms 1/3] Silicon/NXP: add RTC support library for PCF8563 I2C IP

[Leif Lindholm]

On Fri, Nov 03, 2017 at 10:16:52AM +, Ard Biesheuvel wrote:
> Add a RealTimeClockLib implementation for the NXP PCF8563 as used on
> the Socionext Developer Box board. Note that the standard I2C protocol
> stack does not support runtime use, so this driver invokes the I2C master
> protocol directly. This requires support from the platform side as well,
> and so this driver will only attach to a I2C master that has the
> gPcf8563RealTimeClockLibI2cMasterProtolGuid protocol installed on its
> handle. It is up to the platform to ensure that the driver producing
> the I2C master protocol in question is runtime capable, and is not
> shared with the I2C protocol stack (i.e., it should not have the I2C
> Bus Configuration Management protocol installed as well).
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 
> ---
>  Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c   | 
> 385 
>  Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.dec |  
> 29 ++
>  Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.inf |  
> 52 +++
>  3 files changed, 466 insertions(+)
> 
> diff --git 
> a/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c 
> b/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c
> new file mode 100644
> index ..fea65a225d7f
> --- /dev/null
> +++ b/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c
> @@ -0,0 +1,385 @@
> +/** @file
> +
> +  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD 
> License
> +  which accompanies this distribution.  The full text of the license may be 
> found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +
> +**/
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define SLAVE_ADDRESS (FixedPcdGet8 (PcdI2cSlaveAddress))
> +#define PCF8563_DATA_REG_OFFSET   0x2
> +
> +#define PCF8563_SECONDS_MASK  0x7f
> +#define PCF8563_MINUTES_MASK  0x7f
> +#define PCF8563_HOURS_MASK0x3f
> +#define PCF8563_DAYS_MASK 0x3f
> +#define PCF8563_WEEKDAYS_MASK 0x07
> +#define PCF8563_MONTHS_MASK   0x1f
> +#define PCF8563_CENTURY_MASK  0x80
> +
> +#define EPOCH_BASE2000
> +
> +STATIC EFI_HANDLE mI2cMasterHandle;
> +STATIC VOID   *mDriverEventRegistration;
> +STATIC EFI_I2C_MASTER_PROTOCOL*mI2cMaster;
> +STATIC EFI_EVENT  mRtcVirtualAddrChangeEvent;
> +
> +typedef struct {
> +  UINTN   OperationCount;
> +  EFI_I2C_OPERATION   SetAddressOp;
> +  EFI_I2C_OPERATION   GetSetDateTimeOp;
> +} RTC_I2C_REQUEST;
> +
> +#pragma pack(1)
> +typedef struct {
> +  UINT8 VL_seconds;
> +  UINT8 Minutes;
> +  UINT8 Hours;
> +  UINT8 Days;
> +  UINT8 Weekdays;
> +  UINT8 Century_months;
> +  UINT8 Years;
> +} RTC_DATETIME;
> +#pragma pack()
> +
> +/**
> +  Returns the current time and date information, and the time-keeping
> +  capabilities of the hardware platform.
> +
> +  @param  Time  A pointer to storage to receive a snapshot of
> +the current time.
> +  @param  Capabilities  An optional pointer to a buffer to receive 
> the
> +real time clock device's capabilities.
> +
> +  @retval EFI_SUCCESS   The operation completed successfully.
> +  @retval EFI_INVALID_PARAMETER Time is NULL.
> +  @retval EFI_DEVICE_ERROR  The time could not be retrieved due to 
> hardware
> +error.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +LibGetTime (
> +  OUT EFI_TIME*Time,
> +  OUT EFI_TIME_CAPABILITIES   *Capabilities
> +  )
> +{
> +  RTC_I2C_REQUEST Op;
> +  RTC_DATETIMEDateTime;
> +  EFI_STATUS  Status;
> +  UINT8   Reg;
> +
> +  if (Time == NULL) {
> +return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (mI2cMaster == NULL) {
> +return EFI_DEVICE_ERROR;
> +  }
> +
> +  Reg = PCF8563_DATA_REG_OFFSET;
> +
> +  Op.OperationCount = 2;
> +
> +  Op.SetAddressOp.Flags = 0;
> +  Op.SetAddressOp.LengthInBytes = 1;
> +  Op.SetAddressOp.Buffer = 
> +
> +  Op.GetSetDateTimeOp.Flags = I2C_FLAG_READ;
> +  Op.GetSetDateTimeOp.LengthInBytes = sizeof (RTC_DATETIME);
> +  Op.GetSetDateTimeOp.Buffer = (VOID *)
> +
> +  Status = mI2cMaster->StartRequest (mI2cMaster, SLAVE_ADDRESS,
> + (VOID *), NULL, NULL);
> +  if (EFI_ERROR (Status)) {
> +return 

Re: [edk2] [Patch] MdeModulePkg/TerminalDxe: Fix PCANSI mapping for TRIANGLE and ARROW

[Kinney, Michael D]

When DOS uses CodePage 437, 18 is an up arrow glyph.

A PC ANSI terminal interprets 18 as a control character.

Mike

> -Original Message-
> From: Ni, Ruiyu
> Sent: Tuesday, November 7, 2017 11:47 PM
> To: Kinney, Michael D ;
> edk2-devel@lists.01.org
> Cc: Dong, Eric ; Zeng, Star
> 
> Subject: RE: [edk2] [Patch] MdeModulePkg/TerminalDxe:
> Fix PCANSI mapping for TRIANGLE and ARROW
> 
> Mike,
> I am a bit confused about mapping 0x18 to upper arrow.
> I remembered that in old days, pressing ALT+18 in DOS
> window can generate upper arrow.
> 
> Thanks/Ray
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-
> boun...@lists.01.org] On Behalf Of
> > Michael D Kinney
> > Sent: Wednesday, November 8, 2017 12:24 PM
> > To: edk2-devel@lists.01.org
> > Cc: Dong, Eric ; Zeng, Star
> 
> > Subject: [edk2] [Patch] MdeModulePkg/TerminalDxe: Fix
> PCANSI mapping
> > for TRIANGLE and ARROW
> >
> > https://bugzilla.tianocore.org/show_bug.cgi?id=761
> >
> > When a TerminalType is set to PCANSI, characters in
> the range 0x00 to 0x1F
> > are control characters.  The mapping table for PCANSI
> maps TRIANGLE glyphs,
> > ARROW_UP glyph, and ARROW_DOWN glyph into this
> control character
> > range and that causes no characters to be displayed
> by PCANSI compatible
> > terminal emulators.
> >
> > The mappings are updated so these glyphs are mapped
> to ANSI characters in
> > the range 0x20 to 0x7E.
> >
> > GEOMETRICSHAPE_UP_TRIANGLE '^'
> > GEOMETRICSHAPE_RIGHT_TRIANGLE  '>'
> > GEOMETRICSHAPE_DOWN_TRIANGLE   'v'
> > GEOMETRICSHAPE_LEFT_TRIANGLE   '<'
> > ARROW_UP   '^'
> > ARROW_DOWN 'v'
> >
> > Cc: Star Zeng 
> > Cc: Eric Dong 
> > Contributed-under: TianoCore Contribution Agreement
> 1.1
> > Signed-off-by: Michael D Kinney
> 
> > ---
> >  .../Universal/Console/TerminalDxe/TerminalConOut.c
> | 18 +
> > -
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git
> >
> a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalCo
> nOut.c
> >
> b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalCo
> nOut.c
> > index e677a76e6b..5a8343162f 100644
> > ---
> a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalCo
> nOut.c
> > +++
> b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalCo
> nOut.c
> > @@ -66,15 +66,15 @@ UNICODE_TO_CHAR
> UnicodeToPcAnsiOrAscii[] = {
> >{ BLOCKELEMENT_FULL_BLOCK,0xdb, L'*'
> },
> >{ BLOCKELEMENT_LIGHT_SHADE,   0xb0, L'+'
> },
> >
> > -  { GEOMETRICSHAPE_UP_TRIANGLE, 0x1e, L'^'
> },
> > -  { GEOMETRICSHAPE_RIGHT_TRIANGLE,  0x10, L'>'
> },
> > -  { GEOMETRICSHAPE_DOWN_TRIANGLE,   0x1f, L'v'
> },
> > -  { GEOMETRICSHAPE_LEFT_TRIANGLE,   0x11, L'<'
> },
> > -
> > -  { ARROW_LEFT, 0x3c, L'<'
> },
> > -  { ARROW_UP,   0x18, L'^'
> },
> > -  { ARROW_RIGHT,0x3e, L'>'
> },
> > -  { ARROW_DOWN, 0x19, L'v'
> },
> > +  { GEOMETRICSHAPE_UP_TRIANGLE, '^', L'^' },
> > +  { GEOMETRICSHAPE_RIGHT_TRIANGLE,  '>', L'>' },
> > +  { GEOMETRICSHAPE_DOWN_TRIANGLE,   'v', L'v' },
> > +  { GEOMETRICSHAPE_LEFT_TRIANGLE,   '<', L'<' },
> > +
> > +  { ARROW_LEFT, '<', L'<' },
> > +  { ARROW_UP,   '^', L'^' },
> > +  { ARROW_RIGHT,'>', L'>' },
> > +  { ARROW_DOWN, 'v', L'v' },
> >
> >{ 0x, 0x00, L'\0'
> }
> >  };
> > --
> > 2.14.2.windows.3
> >
> > ___
> > 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] ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI core

[Leif Lindholm]

On Sun, Nov 05, 2017 at 04:29:15PM +, Ard Biesheuvel wrote:
> >> OK, this may sound completely unreasonable, but seeing those
> >> implementations overwrite callee-saved registers without saving them
> >> makes my brain unhappy. (Yes, I know.)
> >>
> >> Could they either:
> >> - Have a comment prepended establishing the implicit ABI of which
> >>   registers the caller cannot rely on reusing after return.
> >>   Preferably somewhat echoed at the call site.
> >> - Be rewritten to use only scratch registers?
> >>
> >
> > I think it is implied that the startup code does not adhere to the
> > AAPCS. That code already uses r5 and r6 without stacking them, simply
> > because we're in the middle of preparing the stack and other execution
> > context, precisely so the C code we call into can rely on AAPCS
> > guarantees.
> 
> Ehm, hold on, what do you mean by 'call site'? This code just runs and
> jumps back to a local label. There are no functions calls here until
> the point where we call into C (with the exception of the lovely
> ArmPlatformPeiBootAction() we added so Juno can find out how much DRAM
> it can use)

Yeah, you're right, I was misreading the block as a subroutine.

Seems the only register that must be preserved across jumps is r5/x5,
and neither of these modifications touch those (or change that fact).

Reviewed-by: Leif Lindholm 

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

Re: [edk2] [MdeModulePkg/TerminalDxe] Why do we delay 2s for ESC being pressed?

[Brian J. Johnson]

On 11/08/2017 07:34 AM, Heyi Guo wrote:



On 11/08/2017 05:07 PM, Gerd Hoffmann wrote:

On Wed, Nov 08, 2017 at 04:44:37PM +0800, Heyi Guo wrote:


在 11/8/2017 4:34 PM, Ni, Ruiyu 写道:

No.
Even a terminal tool can recognize F10, it still needs to translate 
it into "ESC [ V"

and send the three bytes to firmware.

Got it. But the 2 seconds timeout is not for this situation, right? If
terminal tool could translate and send the key sequence, I think it can
complete 3 bytes transfer in a very short time, isn't it? E.g. 9600 
baud / 8

= 1200 Bytes/s (ignore control bits).

So 2 seconds timeout is still for user to enter the sequence "ESC [ V"
manually?

No.  Alot of software has this kind of delay because it is recommended
in some classic unix documentation to avoid mis-interpreting incomplete
terminal control sequences coming from slow terminals.

Where a "slow terminal" which actually would need such a long delay is a
physical terminal from the 70ies of the last century, or a virtual
terminal hooked up over a *really* slow network connection.

Reducing the delay from 2 seconds to roughly 0.2 seconds should be
pretty safe, things are not that slow any more these days :)

That will be great if we can make such change :)



You'd be surprised how much delay you can get with a few layers of 
firewalls, VPNs, and cross-country (or intercontinental) WAN links. 
That's the advantage of serial:  you can tunnel it anywhere.


Here's a quick workaround:  if you start typing other text after the 
ESC, the terminal driver will see the new keystrokes and resolve the ESC 
immediately, without the delay.  For instance, at the shell prompt, type 
something, press ESC to clear the line, then just start typing new text 
without waiting for the timeout.  The line will be cleared and the new 
text will appear correctly, without delay.


On setup screens, I usually hit escape to return to the previous screen, 
then hit one of the arrow keys to cause the ESC to be processed without 
the timeout.  That works pretty well in practice.


I'd think a PCD to control this delay would be appropriate, though.
--
Brian J. Johnson
Enterprise X86 Lab

Hewlett Packard Enterprise

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

Re: [edk2] [PATCH v3 2/3] ShellPkg: Fix misuses of AllocateCopyPool

[Carsey, Jaben]

Why not use the StrCpy_s function to copy strings?  CopyMem and StrSize feels 
odd to me.

> -Original Message-
> From: Wang, Jian J
> Sent: Tuesday, November 07, 2017 6:12 PM
> To: edk2-devel@lists.01.org
> Cc: Carsey, Jaben ; Ni, Ruiyu
> ; Bi, Dandan 
> Subject: [PATCH v3 2/3] ShellPkg: Fix misuses of AllocateCopyPool
> Importance: High
> 
> > v3:
> >   No update
> 
> > v2:
> > a. Use ReallocatePool instead of allocating then copying wherever
> applicable
> 
> AllocateCopyPool(AllocationSize, *Buffer) will copy "AllocationSize" bytes of
> memory from old "Buffer" to new allocated one. If "AllocationSize" is bigger
> than size of "Buffer", heap memory overflow occurs during copy.
> 
> One solution is to allocate pool first then copy the necessary bytes to new
> memory. Another is using ReallocatePool instead if old buffer will be freed
> on spot.
> 
> Cc: Jaben Carsey 
> Cc: Ruiyu Ni 
> Cc: Bi Dandan 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> ---
>  ShellPkg/Application/Shell/Shell.c | 4 +++-
>  ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c | 7
> +--
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/ShellPkg/Application/Shell/Shell.c
> b/ShellPkg/Application/Shell/Shell.c
> index 5471930ba1..656206fdce 100644
> --- a/ShellPkg/Application/Shell/Shell.c
> +++ b/ShellPkg/Application/Shell/Shell.c
> @@ -1646,7 +1646,7 @@ ShellConvertVariables (
>//
>// now do the replacements...
>//
> -  NewCommandLine1 = AllocateCopyPool(NewSize, OriginalCommandLine);
> +  NewCommandLine1 = AllocateZeroPool (NewSize);
>NewCommandLine2 = AllocateZeroPool(NewSize);
>ItemTemp= AllocateZeroPool(ItemSize+(2*sizeof(CHAR16)));
>if (NewCommandLine1 == NULL || NewCommandLine2 == NULL ||
> ItemTemp == NULL) {
> @@ -1655,6 +1655,8 @@ ShellConvertVariables (
>  SHELL_FREE_NON_NULL(ItemTemp);
>  return (NULL);
>}
> +  CopyMem (NewCommandLine1, OriginalCommandLine, StrSize
> (OriginalCommandLine));
> +
>for (MasterEnvList = EfiShellGetEnv(NULL)
>  ;  MasterEnvList != NULL && *MasterEnvList != CHAR_NULL
>  ;  MasterEnvList += StrLen(MasterEnvList) + 1
> diff --git
> a/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
> b/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
> index 1122c89b8b..ee3db63358 100644
> ---
> a/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
> +++
> b/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
> @@ -143,10 +143,11 @@ UpdateOptionalData(
>  OriginalOptionDataSize += (*(UINT16*)(OriginalData + sizeof(UINT32)));
>  OriginalOptionDataSize -= OriginalSize;
>  NewSize = OriginalSize - OriginalOptionDataSize + DataSize;
> -NewData = AllocateCopyPool(NewSize, OriginalData);
> +NewData = AllocatePool(NewSize);
>  if (NewData == NULL) {
>Status = EFI_OUT_OF_RESOURCES;
>  } else {
> +  CopyMem (NewData, OriginalData, OriginalSize -
> OriginalOptionDataSize);
>CopyMem(NewData + OriginalSize - OriginalOptionDataSize, Data,
> DataSize);
>  }
>}
> @@ -1120,11 +1121,13 @@ BcfgAddOpt(
>  // Now we know how many EFI_INPUT_KEY structs we need to attach to
> the end of the EFI_KEY_OPTION struct.
>  // Re-allocate with the added information.
>  //
> -KeyOptionBuffer = AllocateCopyPool(sizeof(EFI_KEY_OPTION) +
> (sizeof(EFI_INPUT_KEY) * NewKeyOption.KeyData.Options.InputKeyCount),
> );
> +KeyOptionBuffer = AllocatePool (sizeof(EFI_KEY_OPTION) +
> (sizeof(EFI_INPUT_KEY) *
> NewKeyOption.KeyData.Options.InputKeyCount));
>  if (KeyOptionBuffer == NULL) {
>ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_NO_MEM),
> gShellBcfgHiiHandle, L"bcfg");
>ShellStatus = SHELL_OUT_OF_RESOURCES;
> +  return ShellStatus;
>  }
> +CopyMem (KeyOptionBuffer, ,
> sizeof(EFI_KEY_OPTION));
>}
>for (LoopCounter = 0 ; ShellStatus == SHELL_SUCCESS && LoopCounter <
> NewKeyOption.KeyData.Options.InputKeyCount; LoopCounter++) {
>  //
> --
> 2.14.1.windows.1

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

Re: [edk2] Why do we use -g option of gcc even for RELEASE build?

[Paolo Bonzini]

On 08/11/2017 03:25, Heyi Guo wrote:
> From gcc manual, -g option seems to produce debugging information. In
> tools_def.template, -g is included in GCC_ALL_CC_FLAGS, so it will also
> be enabled for RELEASE build with gcc tool chain. Any special reason to
> do that?

Why *not* actually?  Debug information only costs disk space, it is not
part of the firmware image (and even for native Linux binaries it is not
loaded in memory).

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

Re: [edk2] Why do we use -g option of gcc even for RELEASE build?

[Gao, Liming]

In build_rule.txt, GCC built image will be strip first, then be converted to 
EFI image by GenFw. There is no symbol in the final EFI image. All symbols are 
kept into original GCC built image. 

Thanks
Liming
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Wednesday, November 8, 2017 10:54 PM
> To: Heyi Guo ; edk2-devel@lists.01.org
> Cc: Gao, Liming 
> Subject: Re: [edk2] Why do we use -g option of gcc even for RELEASE build?
> 
> On 11/08/17 03:25, Heyi Guo wrote:
> > Hi folks,
> >
> > From gcc manual, -g option seems to produce debugging information. In
> > tools_def.template, -g is included in GCC_ALL_CC_FLAGS, so it will also
> > be enabled for RELEASE build with gcc tool chain. Any special reason to
> > do that?
> 
> In the edk2 tree, -g was added to GCC_ALL_CC_FLAGS and
> GCC44_ALL_CC_FLAGS in commit 52302d4dee58 ("Sync EDKII BaseTools to
> BaseTools project r1903.", 2010-02-28).
> 
> If you check the history of the now-historical separate BaseTools
> project , "-g"
> was introduced in commit 46c1e64305d4 ("Upgrade the binutil 2.18.50.0.5
> to 2.20.51.0.5 for UNIXGCC tool chain", 2010-02-25).
> 
> I guess that the same  for [Dynamic-Library-File] must be
> able to work for both DEBUG/NOOPT and RELEASE builds. So -g is included
> for all of those build targets, in the C flags. Ultimately the debug
> symbols are not copied into the .efi binaries, for RELEASE, I believe.
> 
> Thanks
> Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] Tftp assert fix for openfile failure case

[Leif Lindholm]

On Wed, Nov 08, 2017 at 05:15:49AM +, Udit Kumar wrote:
> > > diff --git a/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > b/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > index fbde3bf..6425fc5 100755
> > > --- a/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > +++ b/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > @@ -509,6 +509,7 @@ ShellCommandRunTftp (
> > >);
> > >goto NextHandle;
> > 
> > Wow, a goto in a foor loop in a 320-line function.
> > What could possibly go wrong?
> 
> Instead of being on some volume, if you are on Shell. 
> Then file open will fail. 

Sure. The above was a snarky comment on the state of the existing
code.

> > >  }
> > > +DataSize = FileSize;
> > >
> > >  Status = DownloadFile (Mtftp4, RemoteFilePath, AsciiRemoteFilePath,
> > FileSize, BlockSize, );
> > >  if (EFI_ERROR (Status)) {
> > > @@ -539,7 +540,6 @@ ShellCommandRunTftp (
> > >goto NextHandle;
> > >  }
> > >
> > > -DataSize = FileSize;
> > >  Status = ShellWriteFile (FileHandle, , Data);
> > >  if (!EFI_ERROR (Status)) {
> > >ShellStatus = SHELL_SUCCESS;
> > > --
> > > 1.9.1
> > 
> > So, a wider question:
> > This shell command was introduced in the heyday of "let's
> > reimplement U-Boot in the EDK2 tree". Mainly, from my impression,
> > it seems to be used in order that people don't need to learn how
> > boot managers and device paths work.
> 
> When you say about complete boot, then this may not be useful. 
> 
> > Am I being too harsh?
> > Are there practical uses for this?
> 
> For doing some sort of unit testing of given interface. I found this
> useful. During development, this is useful to transfer generic file
> to development board.

OK, I can see how it could be useful.
My opposition is based on three things:
1) people _are_ trying to use it for boot
2) not a command described by UEFI Shell spec, but I keep seeing
   platforms including it even in RELEASE builds (most likely because 1)
3) code quality/maintainability

> > If the code is to be kept, I think (from a quick glance) that I
> > would also like to see
> >   *Data = NULL
> > in the error path of DownloadFile().

OK, so we don't need to drop it right now, but what's your take on
this comment?

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

Re: [edk2] Why do we use -g option of gcc even for RELEASE build?

[Laszlo Ersek]

On 11/08/17 03:25, Heyi Guo wrote:
> Hi folks,
> 
> From gcc manual, -g option seems to produce debugging information. In
> tools_def.template, -g is included in GCC_ALL_CC_FLAGS, so it will also
> be enabled for RELEASE build with gcc tool chain. Any special reason to
> do that?

In the edk2 tree, -g was added to GCC_ALL_CC_FLAGS and
GCC44_ALL_CC_FLAGS in commit 52302d4dee58 ("Sync EDKII BaseTools to
BaseTools project r1903.", 2010-02-28).

If you check the history of the now-historical separate BaseTools
project , "-g"
was introduced in commit 46c1e64305d4 ("Upgrade the binutil 2.18.50.0.5
to 2.20.51.0.5 for UNIXGCC tool chain", 2010-02-25).

I guess that the same  for [Dynamic-Library-File] must be
able to work for both DEBUG/NOOPT and RELEASE builds. So -g is included
for all of those build targets, in the C flags. Ultimately the debug
symbols are not copied into the .efi binaries, for RELEASE, I believe.

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

Re: [edk2] [PATCH v3] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map

[Laszlo Ersek]

On 11/08/17 11:52, Jian J Wang wrote:
>> v3:
>> a. Add comment to explain more on updating memory capabilities
>> b. Fix logic hole in updating attributes
>> c. Instead of checking illegal memory space address and size, use return
>>status of gDS->SetMemorySpaceCapabilities() to skip memory block which
>>cannot be updated with new capabilities.
> 
>> v2
>> a. Fix an issue which will cause setting capability failure if size is 
>> smaller
>>than a page.
> 
> More than one entry of RT_CODE memory might cause boot problem for some
> old OSs. This patch will fix this issue to keep OS compatibility as much
> as possible.
> 
> More detailed information, please refer to
> https://bugzilla.tianocore.org/show_bug.cgi?id=753
> 
> Cc: Eric Dong 
> Cc: Jiewen Yao 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> ---
>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 48 
> +++-
>  1 file changed, 38 insertions(+), 10 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c 
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index d312eb66f8..455c713dfc 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -789,7 +789,6 @@ RefreshGcdMemoryAttributesFromPaging (
>UINT64  BaseAddress;
>UINT64  PageStartAddress;
>UINT64  Attributes;
> -  UINT64  Capabilities;
>BOOLEAN DoUpdate;
>UINTN   Index;
>  
> @@ -803,7 +802,6 @@ RefreshGcdMemoryAttributesFromPaging (
>GetCurrentPagingContext ();
>  
>DoUpdate  = FALSE;
> -  Capabilities  = 0;
>Attributes= 0;
>BaseAddress   = 0;
>PageLength= 0;
> @@ -813,6 +811,27 @@ RefreshGcdMemoryAttributesFromPaging (
>continue;
>  }
>  
> +//
> +// Sync the actual paging related capabilities back to GCD service first.
> +// As a side effect (good one), this can also help to avoid unnecessary
> +// memory map entries due to the different capabilities of the same type
> +// memory, such as multiple RT_CODE and RT_DATA entries in memory map,
> +// which could cause boot failure of some old Linux distro (before v4.3).
> +//
> +Status = gDS->SetMemorySpaceCapabilities (
> +MemorySpaceMap[Index].BaseAddress,
> +MemorySpaceMap[Index].Length,
> +MemorySpaceMap[Index].Capabilities |
> +EFI_MEMORY_PAGETYPE_MASK
> +);
> +if (EFI_ERROR (Status)) {
> +  //
> +  // If we cannot udpate the capabilities, we cannot update its
> +  // attributes either. So just simply skip current block of memory.
> +  //

(1) Can you perhaps add a DEBUG_WARN here?

> +  continue;
> +}
> +
>  if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
>//
>// Current memory space starts at a new page. Resetting PageLength will
> @@ -826,7 +845,9 @@ RefreshGcdMemoryAttributesFromPaging (
>PageLength -= (MemorySpaceMap[Index].BaseAddress - BaseAddress);
>  }
>  
> -// Sync real page attributes to GCD
> +//
> +// Sync actual page attributes to GCD
> +//
>  BaseAddress   = MemorySpaceMap[Index].BaseAddress;
>  MemorySpaceLength = MemorySpaceMap[Index].Length;
>  while (MemorySpaceLength > 0) {
> @@ -845,8 +866,6 @@ RefreshGcdMemoryAttributesFromPaging (
>  
>  if (Attributes != (MemorySpaceMap[Index].Attributes & 
> EFI_MEMORY_PAGETYPE_MASK)) {
>DoUpdate = TRUE;
> -  Attributes |= (MemorySpaceMap[Index].Attributes & 
> ~EFI_MEMORY_PAGETYPE_MASK);
> -  Capabilities = Attributes | MemorySpaceMap[Index].Capabilities;
>  } else {
>DoUpdate = FALSE;
>  }

(2) To me it seems like we can remove the "DoUpdate" local variable
completely. Below, we can replace the DoUpdate check with the actual

  (Attributes != (MemorySpaceMap[Index].Attributes &
  EFI_MEMORY_PAGETYPE_MASK))

check.

The point is that we check the EFI_MEMORY_PAGETYPE_MASK bit-field of the
entry's attributes. If they do not match Attributes, we clear the full
bit-field, and then add Attributes back in. I.e., we set the bit-field
to the desired Attributes.

> @@ -854,11 +873,20 @@ RefreshGcdMemoryAttributesFromPaging (
>  
>Length = MIN (PageLength, MemorySpaceLength);
>if (DoUpdate) {
> -gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
> -gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
> -DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx - 
> %016lx (%08lx -> %08lx)\r\n",
> - Index, BaseAddress, 

Re: [edk2] [PATCH V2] MdeModulePkg SerialDxe: Handle Timeout change more robustly

[Julien Grall]

Hi Star,

On 07/11/17 01:36, Star Zeng wrote:

https://lists.01.org/pipermail/edk2-devel/2017-October/016479.html
reported "Xen Console input very slow in recent UEFI" that appears
after 4cf3f37c87ba1f9d58072444bd735e40e4779e70 "MdeModulePkg
SerialDxe: Process timeout consistently in SerialRead".

Julien did more debugging and find out the following is happening in
TerminalConInTimerHandler (MdeModulePkg/Universal/Console/TerminalDxe)
when a character is received:
1) GetControl will return EFI_SERIAL_INPUT_BUFFER_EMPTY unset
   => Entering in the loop to fetch character from the serial
2) GetOneKeyFromSerial()
   => Return directly with the character read
3) Looping as the fifo is not full and no error
4) GetOneKeyFromSerial() -> SerialRead()
   => No more character so SerialPortPoll() will return FALSE and loop
  until timeout
   => Return EFI_TIMEOUT
5) Exiting the loop from TerminalConInTimerHandler
6) Characters are printed

After some investigation, I found it is related to the Timeout value.

The Timeout is 100 (1s) by default to follow UEFI spec.
And the Terminal driver will recalculate and set the Timeout value
based on the properties of UART in TerminalDriverBindingStart()/
TerminalConInTimerHandler().

   SerialInTimeOut = 0;
   if (Mode->BaudRate != 0) {
 //
 // According to BAUD rate to calculate the timeout value.
 //
 SerialInTimeOut = (1 + Mode->DataBits + Mode->StopBits) *
   2 * 100 / (UINTN) Mode->BaudRate;
   }

For example, based on the PCD values of PcdUartDefaultBaudRate,
PcdUartDefaultDataBits and PcdUartDefaultStopBits, SerialInTimeOut =
(1 + 8  + 1) * 2 * 100 / (UINTN) 115200 = 173 (us).

When SerialDxe is used,
TerminalDriverBindingStart()/TerminalConInTimerHandler() ->
   SerialIo->SetAttributes() ->
 SerialSetAttributes() ->
   SerialPortSetAttributes()

Some implementations of SerialPortSetAttributes() could handle the
input parameters and return RETURN_SUCCESS, for example
BaseSerialPortLib16550, then Timeout value will be changed to 173 (us),
no "slow down" will be observed.
But some implementations of SerialPortSetAttributes() just return
RETURN_UNSUPPORTED, for example XenConsoleSerialPortLib, then Timeout
value will be not changed and kept 100 (1s), "slow down" will be
observed.

SerialPortLib instance can be enhanced to
1. Handle the input parameters and return status accordingly instead of
just returning RETURN_UNSUPPORTED in SerialPortSetAttributes().
2. Just return RETURN_SUCCESS instead of RETURN_UNSUPPORTED in
SerialPortSetAttributes() if the instance does not care the input
parameters at all.

And SerialDxe can also be enhanced like this patch to be more robust
to handle Timeout change.

Cc: Julien Grall 
Cc: Laszlo Ersek 
Cc: Ruiyu Ni 
Compare against the original parameters
   Suggested-by: Laszlo Ersek 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 


Tested-by: Julien Grall 

Cheers,

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

Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map

[Laszlo Ersek]

On 11/08/17 01:10, Wang, Jian J wrote:
> Hi Laszlo,
> 
>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Wednesday, November 08, 2017 1:14 AM
>> To: Wang, Jian J ; edk2-devel@lists.01.org
>> Cc: Yao, Jiewen ; Dong, Eric 
>> Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
>> RT_CODE in memory map
>>
>> sorry about the late response
>>
>> On 11/03/17 01:57, Jian J Wang wrote:
 v2
 a. Fix an issue which will cause setting capability failure if size is 
 smaller
than a page.
>>>
>>> More than one entry of RT_CODE memory might cause boot problem for
>> some
>>> old OSs. This patch will fix this issue to keep OS compatibility as much
>>> as possible.
>>>
>>> More detailed information, please refer to
>>> https://bugzilla.tianocore.org/show_bug.cgi?id=753
>>>
>>> Cc: Eric Dong 
>>> Cc: Jiewen Yao 
>>> Cc: Laszlo Ersek 
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Jian J Wang 
>>> ---
>>>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++
>>>  1 file changed, 14 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
>> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
>>> index d312eb66f8..4a7827ebc9 100644
>>> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
>>> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
>>> @@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging (
>>>PageLength= 0;
>>>
>>>for (Index = 0; Index < NumberOfDescriptors; Index++) {
>>> -if (MemorySpaceMap[Index].GcdMemoryType ==
>> EfiGcdMemoryTypeNonExistent) {
>>> +if (MemorySpaceMap[Index].GcdMemoryType ==
>> EfiGcdMemoryTypeNonExistent
>>> +|| (MemorySpaceMap[Index].BaseAddress & EFI_PAGE_MASK) != 0
>>> +|| (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0) {
>>>continue;
>>>  }
>>
>> When exactly do the new conditions match?
>>
>> I thought the base addresses and the lengths in the GCD memory space map
>> are all page aligned. Is that not the case?
>>
>> If these conditions are just a sanity check (i.e. we never expect them
>> to fire), then should we perpahs turn them into ASSERT()s?
>>
> 
> I found that there's a mmio entry in memory map on OVMF which has size
> less than a page. I didn't encounter this before. Maybe some recent changes
> in other part of EDKII caused this situation. So ASSERT is not enough.

Can you describe the base address and size of this MMIO entry?

I don't see how it is possible to add such an entry in the first place
-- if you try to add it in PEI, via a HOB, then IIRC HobLib will
ASSERT(). If you try to add it with gDS->AddMemorySpace() in DXE, then
the call should fail.

I believe it might be useful to investigate this entry more closely.
Knowing the base address could help us.

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

Re: [edk2] GRUB issue on device priority

[Leif Lindholm]

On Tue, Nov 07, 2017 at 10:02:30PM +0800, Haojian Zhuang wrote:
> Hi all,
> 
> It seems there's a device priority issue in GRUB.

GRUB is behaving as expected.

> All block io handles are linked into the list in edk2, and GRUB could fetch
> it. Then GRUB creates its own ascending on HD priority.
> add_device():
>   for (p = devices; *p; p = &((*p)->next)) {
> ret = grub_efi_compare_device_paths (grub_efi_find_last_device_path
> ((*p->device_path),
>  grub_efi_find_last_device_path->device_path));
> if (ret == 0) {
>   ret = grub_efi_compare_device_paths ((*p)->device_path,
>d->device_path);
> }
> if (ret == 0) {
>   return;
> } else if (ret > 0) {
>   break;
> }
>   }
>   ...

UEFI guarantees no ordering of block devices. GRUB is a UEFI
application, so it cannot assume anything with regards to device
ordering.

> In the HiKey platform, I prepared the same driver for both eMMC and
> SD. So the device paths are in below.
> SD: /HardwareVendor(0d51905b-b77e-452a-a2c0-eca0cc8d514a)[9: 00 e0 23 f7 00 
> 00 00 00 00 ]/UnknownMessaging(1a)/EndEntire
> eMMC: /HardwareVendor(0d51905b-b77e-452a-a2c0-eca0cc8d514a)[9: 00 d0 23 f7 00 
> 00 00 00 00]/UnknownMessaging(1d)/Ctrl(0)/EndEntire
> 
> #define MSG_SD_DP   0x1A
> #define MSG_EMMC_DP 0x1D
> 
> In the second level, the device paths are different.
> 
> And GRUB resort the sequence by ascending order (with above
> code). So SD device always gets higher priority than eMMC device.
> 
> If we always use installer to install OS, it may not an issue. Since
> installer could create grub.cfg by itself. But it imports another
> issue on lacking of persistent variable storage. And we need to
> deploy system without installer on embedded device.

Yes, this is the bit which is interesting, and why I requested you
post this problem to edk2-devel.

I believe that we need to have a sensible way to deal with embedded
platforms that do not have operating systems "installed" to them, but
rather written as images to flash devices.

So, can you clarify a few things for me?:
- Where is GRUB located in your (pre-SD) system?
- Where is GRUB located in your SD system?
- Are you looking for a way to support GRUB being located on either
  eMMC or SD? Or are you looking to always have GRUB loaded from eMMC?

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

Re: [edk2] GRUB issue on device priority

[Leif Lindholm]

On Wed, Nov 08, 2017 at 01:46:13PM +0800, Haojian Zhuang wrote:
> On 2017/11/7 23:14, Vladimir 'phcoder' Serbinenko wrote:
> > On Tue, Nov 7, 2017, 15:23 Haojian Zhuang  > > wrote:
> > 
> > Hi all,
> > 
> > It seems there's a device priority issue in GRUB.
> > 
> > Please use mailing list for grub, not just messaging random people some
> > of whom have left project 10 years ago.
> 
> Sure. Just find the mailing account. Loop grub-de...@gnu.org.

I don't think we need to loop in grub-devel:
If you tell grub to boot from the first block device it encounters, it
will boot from the first block device it encounters. That is as
expected. I will delete grub-devel and Vladimir from the recipient
list beyond this email and then respond to your original email on
edk2-devel.

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

[edk2] UserIdentification in Security Package

[Wim Vervoorn]

Hello,

I am trying to make the UserIdentification from the security package to work or 
at least see how this behaves.

At this point I am missing out on something to get this working in my tree.

Can you point out what needs to be done to give this a try?

At this point I added the stuff to the dsc and fdf file but I am wondering if 
this should be sufficient or if I need to implement additional items.

Best regards,

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

Re: [edk2] [MdeModulePkg/TerminalDxe] Why do we delay 2s for ESC being pressed?

[Heyi Guo]

On 11/08/2017 05:07 PM, Gerd Hoffmann wrote:

On Wed, Nov 08, 2017 at 04:44:37PM +0800, Heyi Guo wrote:


在 11/8/2017 4:34 PM, Ni, Ruiyu 写道:

No.
Even a terminal tool can recognize F10, it still needs to translate it into "ESC [ 
V"
and send the three bytes to firmware.

Got it. But the 2 seconds timeout is not for this situation, right? If
terminal tool could translate and send the key sequence, I think it can
complete 3 bytes transfer in a very short time, isn't it? E.g. 9600 baud / 8
= 1200 Bytes/s (ignore control bits).

So 2 seconds timeout is still for user to enter the sequence "ESC [ V"
manually?

No.  Alot of software has this kind of delay because it is recommended
in some classic unix documentation to avoid mis-interpreting incomplete
terminal control sequences coming from slow terminals.

Where a "slow terminal" which actually would need such a long delay is a
physical terminal from the 70ies of the last century, or a virtual
terminal hooked up over a *really* slow network connection.

Reducing the delay from 2 seconds to roughly 0.2 seconds should be
pretty safe, things are not that slow any more these days :)

That will be great if we can make such change :)

Thanks,

Heyi



HTH,
   Gerd



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

Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map

[Laszlo Ersek]

On 11/08/17 04:13, Zeng, Star wrote:
> https://bugzilla.tianocore.org/show_bug.cgi?id=763 is submitted to track this.

I'd like to ask a question about the impact of BZ#763:

Regarding BZ#753 ("UEFI memory map regression (runtime code entry
splitting) introduced by c1cab54ce57c"), we seem to have agreed that any
firmware that has commit c1cab54ce57c will need the fix for BZ#753.
Otherwise the OS may get an invalid UEFI memory map, and if the OS is
not specifically prepared for that, it can crash.

My question is if the issue described in BZ#763 is similarly serious;
i.e., whether any firmware that has commit 14dde9e903bb
("MdeModulePkg/Core: Fix out-of-sync issue in GCD", 2017-09-19) should
urgently get the fix for BZ#763.

In other words, does BZ#763 have a direct OS-level impact that needs to
be fixed quickly?

Thanks!
Laszlo


> -Original Message-
> From: Wang, Jian J 
> Sent: Tuesday, November 7, 2017 8:55 AM
> To: Zeng, Star ; edk2-devel@lists.01.org
> Cc: Laszlo Ersek ; Yao, Jiewen ; 
> Dong, Eric 
> Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of 
> RT_CODE in memory map
> 
> Thanks for the review. And I agree that GCD.SetMemoryAttributes should be 
> used all the time in DxeCore. Let's fix it in another patch.
> 
>> -Original Message-
>> From: Zeng, Star
>> Sent: Monday, November 06, 2017 5:16 PM
>> To: Wang, Jian J ; edk2-devel@lists.01.org
>> Cc: Laszlo Ersek ; Yao, Jiewen 
>> ; Dong, Eric ; Zeng, Star 
>> 
>> Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries 
>> of RT_CODE in memory map
>>
>> I am ok to this code approach.
>>
>> Acknowledged-by: Star Zeng 
>>
>> Besides, I think except GCD services, DxeCore should not call gCpu-
>>> SetMemoryAttributes() directly, for example 
>>> ApplyMemoryProtectionPolicy(), it
>> should have no assumption of the CPU code (to set capabilities), and 
>> call GCD setcapabilities first, and then call GCD setattributes since
>> 14dde9e903bb9a719ebb8f3381da72b19509bc36 "MdeModulePkg/Core: Fix out- 
>> of-sync issue in GCD", otherwise there may be mismatch of page 
>> attributes between GCD and gCPU after 
>> RefreshGcdMemoryAttributesFromPaging() by the calling 
>> gCpu->SetMemoryAttributes() in ApplyMemoryProtectionPolicy().
>>
>> Anyway, that could be in a separated patch. :)
>>
>> Thanks,
>> Star
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
>> Jian J Wang
>> Sent: Friday, November 3, 2017 8:57 AM
>> To: edk2-devel@lists.01.org
>> Cc: Laszlo Ersek ; Yao, Jiewen 
>> ; Dong, Eric 
>> Subject: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of 
>> RT_CODE in memory map
>>
>>> v2
>>> a. Fix an issue which will cause setting capability failure if size is 
>>> smaller
>>>than a page.
>>
>> More than one entry of RT_CODE memory might cause boot problem for 
>> some old OSs. This patch will fix this issue to keep OS compatibility as 
>> much as possible.
>>
>> More detailed information, please refer to
>> https://bugzilla.tianocore.org/show_bug.cgi?id=753
>>
>> Cc: Eric Dong 
>> Cc: Jiewen Yao 
>> Cc: Laszlo Ersek 
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Jian J Wang 
>> ---
>>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++
>>  1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
>> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
>> index d312eb66f8..4a7827ebc9 100644
>> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
>> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
>> @@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging (
>>PageLength= 0;
>>
>>for (Index = 0; Index < NumberOfDescriptors; Index++) {
>> -if (MemorySpaceMap[Index].GcdMemoryType ==
>> EfiGcdMemoryTypeNonExistent) {
>> +if (MemorySpaceMap[Index].GcdMemoryType ==
>> EfiGcdMemoryTypeNonExistent
>> +|| (MemorySpaceMap[Index].BaseAddress & EFI_PAGE_MASK) != 0
>> +|| (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0) {
>>continue;
>>  }
>>
>> @@ -829,6 +831,15 @@ RefreshGcdMemoryAttributesFromPaging (
>>  // Sync real page attributes to GCD
>>  BaseAddress   = MemorySpaceMap[Index].BaseAddress;
>>  MemorySpaceLength = MemorySpaceMap[Index].Length;
>> +Capabilities  = MemorySpaceMap[Index].Capabilities |
>> +EFI_MEMORY_PAGETYPE_MASK;
>> +Status = gDS->SetMemorySpaceCapabilities (
>> +BaseAddress,
>> +MemorySpaceLength,
>> +Capabilities
>> +);
>> +ASSERT_EFI_ERROR (Status);

[edk2] [PATCH] SecurityPkg/SecureBootConfigDxe: Fix deleting signature data issue.

[chenc2]

Replace "(UINT8 *)NewVariableData" with (UINT8 *)NewVariableData + Offset"
to avoid the header of EFI_SIGNATURE_LIST being copied to the front of
NewVariableData every time and update ListWalker when handling the current
EFI_SIGNATURE_LIST finishes.

Cc: Zhang Chao 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: chenc2 
---
 .../SecureBootConfigDxe/SecureBootConfigImpl.c  | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git 
a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c 
b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
index d035763106..618c972ce3 100644
--- 
a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
+++ 
b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
@@ -3155,6 +3155,9 @@ DeleteSignatureEx (
 }
 
 if (CheckedCount == SIGNATURE_DATA_COUNTS (ListWalker) || DelType == 
Delete_Signature_List_One) {
+  //
+  //  If delete the whole EFI_SIGNATURE_LIST, skip and continue to next 
EFI_SIGNATURE_LIST.
+  //
   RemainingSize -= ListWalker->SignatureListSize;
   ListWalker = (EFI_SIGNATURE_LIST *)((UINT8 *)ListWalker + 
ListWalker->SignatureListSize);
 } else {
@@ -3162,7 +3165,7 @@ DeleteSignatureEx (
   //
   // Copy header.
   //
-  CopyMem ((UINT8 *)NewVariableData, ListWalker, sizeof 
(EFI_SIGNATURE_LIST) + ListWalker->SignatureHeaderSize);
+  CopyMem ((UINT8 *)NewVariableData + Offset, ListWalker, sizeof 
(EFI_SIGNATURE_LIST) + ListWalker->SignatureHeaderSize);
   Offset += sizeof (EFI_SIGNATURE_LIST) + ListWalker->SignatureHeaderSize;
 
   DataWalker = (EFI_SIGNATURE_DATA *)((UINT8 *)ListWalker + 
sizeof(EFI_SIGNATURE_LIST) + ListWalker->SignatureHeaderSize);
@@ -3183,6 +3186,7 @@ DeleteSignatureEx (
   }
 
   RemainingSize -= ListWalker->SignatureListSize;
+  ListWalker = (EFI_SIGNATURE_LIST *)((UINT8 *)ListWalker + 
ListWalker->SignatureListSize);
 }
 
 //
-- 
2.13.2.windows.1

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

[edk2] [PATCH v3] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map

[Jian J Wang]

> v3:
> a. Add comment to explain more on updating memory capabilities
> b. Fix logic hole in updating attributes
> c. Instead of checking illegal memory space address and size, use return
>status of gDS->SetMemorySpaceCapabilities() to skip memory block which
>cannot be updated with new capabilities.

> v2
> a. Fix an issue which will cause setting capability failure if size is smaller
>than a page.

More than one entry of RT_CODE memory might cause boot problem for some
old OSs. This patch will fix this issue to keep OS compatibility as much
as possible.

More detailed information, please refer to
https://bugzilla.tianocore.org/show_bug.cgi?id=753

Cc: Eric Dong 
Cc: Jiewen Yao 
Cc: Laszlo Ersek 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
 UefiCpuPkg/CpuDxe/CpuPageTable.c | 48 +++-
 1 file changed, 38 insertions(+), 10 deletions(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index d312eb66f8..455c713dfc 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -789,7 +789,6 @@ RefreshGcdMemoryAttributesFromPaging (
   UINT64  BaseAddress;
   UINT64  PageStartAddress;
   UINT64  Attributes;
-  UINT64  Capabilities;
   BOOLEAN DoUpdate;
   UINTN   Index;
 
@@ -803,7 +802,6 @@ RefreshGcdMemoryAttributesFromPaging (
   GetCurrentPagingContext ();
 
   DoUpdate  = FALSE;
-  Capabilities  = 0;
   Attributes= 0;
   BaseAddress   = 0;
   PageLength= 0;
@@ -813,6 +811,27 @@ RefreshGcdMemoryAttributesFromPaging (
   continue;
 }
 
+//
+// Sync the actual paging related capabilities back to GCD service first.
+// As a side effect (good one), this can also help to avoid unnecessary
+// memory map entries due to the different capabilities of the same type
+// memory, such as multiple RT_CODE and RT_DATA entries in memory map,
+// which could cause boot failure of some old Linux distro (before v4.3).
+//
+Status = gDS->SetMemorySpaceCapabilities (
+MemorySpaceMap[Index].BaseAddress,
+MemorySpaceMap[Index].Length,
+MemorySpaceMap[Index].Capabilities |
+EFI_MEMORY_PAGETYPE_MASK
+);
+if (EFI_ERROR (Status)) {
+  //
+  // If we cannot udpate the capabilities, we cannot update its
+  // attributes either. So just simply skip current block of memory.
+  //
+  continue;
+}
+
 if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
   //
   // Current memory space starts at a new page. Resetting PageLength will
@@ -826,7 +845,9 @@ RefreshGcdMemoryAttributesFromPaging (
   PageLength -= (MemorySpaceMap[Index].BaseAddress - BaseAddress);
 }
 
-// Sync real page attributes to GCD
+//
+// Sync actual page attributes to GCD
+//
 BaseAddress   = MemorySpaceMap[Index].BaseAddress;
 MemorySpaceLength = MemorySpaceMap[Index].Length;
 while (MemorySpaceLength > 0) {
@@ -845,8 +866,6 @@ RefreshGcdMemoryAttributesFromPaging (
 
 if (Attributes != (MemorySpaceMap[Index].Attributes & 
EFI_MEMORY_PAGETYPE_MASK)) {
   DoUpdate = TRUE;
-  Attributes |= (MemorySpaceMap[Index].Attributes & 
~EFI_MEMORY_PAGETYPE_MASK);
-  Capabilities = Attributes | MemorySpaceMap[Index].Capabilities;
 } else {
   DoUpdate = FALSE;
 }
@@ -854,11 +873,20 @@ RefreshGcdMemoryAttributesFromPaging (
 
   Length = MIN (PageLength, MemorySpaceLength);
   if (DoUpdate) {
-gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
-gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
-DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx - 
%016lx (%08lx -> %08lx)\r\n",
- Index, BaseAddress, BaseAddress + Length - 1,
- MemorySpaceMap[Index].Attributes, Attributes));
+Status = gDS->SetMemorySpaceAttributes (
+BaseAddress,
+Length,
+(MemorySpaceMap[Index].Attributes
+ & ~EFI_MEMORY_PAGETYPE_MASK) | Attributes
+);
+ASSERT_EFI_ERROR (Status);
+DEBUG ((
+  DEBUG_INFO,
+  "Update memory space attribute: [%02d] %016lx - %016lx (%016lx -> 
%016lx)\r\n",
+  Index, BaseAddress, BaseAddress + Length - 1,
+  MemorySpaceMap[Index].Attributes,
+  (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_PAGETYPE_MASK) | 
Attributes
+  ));
   }
 
   PageLength-= 

Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map

[Wang, Jian J]

Some updates below

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Wang,
> Jian J
> Sent: Wednesday, November 08, 2017 8:11 AM
> To: Laszlo Ersek ; edk2-devel@lists.01.org
> Cc: Yao, Jiewen ; Dong, Eric 
> Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> RT_CODE in memory map
> 
> Hi Laszlo,
> 
> > -Original Message-
> > From: Laszlo Ersek [mailto:ler...@redhat.com]
> > Sent: Wednesday, November 08, 2017 1:14 AM
> > To: Wang, Jian J ; edk2-devel@lists.01.org
> > Cc: Yao, Jiewen ; Dong, Eric 
> > Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> > RT_CODE in memory map
> >
> > sorry about the late response
> >
> > On 11/03/17 01:57, Jian J Wang wrote:
> > >> v2
> > >> a. Fix an issue which will cause setting capability failure if size is 
> > >> smaller
> > >>than a page.
> > >
> > > More than one entry of RT_CODE memory might cause boot problem for
> > some
> > > old OSs. This patch will fix this issue to keep OS compatibility as much
> > > as possible.
> > >
> > > More detailed information, please refer to
> > > https://bugzilla.tianocore.org/show_bug.cgi?id=753
> > >
> > > Cc: Eric Dong 
> > > Cc: Jiewen Yao 
> > > Cc: Laszlo Ersek 
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Jian J Wang 
> > > ---
> > >  UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++
> > >  1 file changed, 14 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > > index d312eb66f8..4a7827ebc9 100644
> > > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > > @@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging (
> > >PageLength= 0;
> > >
> > >for (Index = 0; Index < NumberOfDescriptors; Index++) {
> > > -if (MemorySpaceMap[Index].GcdMemoryType ==
> > EfiGcdMemoryTypeNonExistent) {
> > > +if (MemorySpaceMap[Index].GcdMemoryType ==
> > EfiGcdMemoryTypeNonExistent
> > > +|| (MemorySpaceMap[Index].BaseAddress & EFI_PAGE_MASK) != 0
> > > +|| (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0) {
> > >continue;
> > >  }
> >
> > When exactly do the new conditions match?
> >
> > I thought the base addresses and the lengths in the GCD memory space map
> > are all page aligned. Is that not the case?
> >
> > If these conditions are just a sanity check (i.e. we never expect them
> > to fire), then should we perpahs turn them into ASSERT()s?
> >
> 
> I found that there's a mmio entry in memory map on OVMF which has size
> less than a page. I didn't encounter this before. Maybe some recent changes
> in other part of EDKII caused this situation. So ASSERT is not enough.
> 

I changed my original fix in v2 to not check the Address and Size. Instead,
I'll use the Status of gDS->SetMemorySpaceCapabilities() to skip those memory
block which cannot be updated with new capabilities. This can avoid the 
assumption that only the address and size will cause the calling failure. And I
found a logic hole in code. You'll find new changes in v3 patch.

> > >
> > > @@ -829,6 +831,15 @@ RefreshGcdMemoryAttributesFromPaging (
> > >  // Sync real page attributes to GCD
> > >  BaseAddress   = MemorySpaceMap[Index].BaseAddress;
> > >  MemorySpaceLength = MemorySpaceMap[Index].Length;
> > > +Capabilities  = MemorySpaceMap[Index].Capabilities |
> > > +EFI_MEMORY_PAGETYPE_MASK;
> > > +Status = gDS->SetMemorySpaceCapabilities (
> > > +BaseAddress,
> > > +MemorySpaceLength,
> > > +Capabilities
> > > +);
> > > +ASSERT_EFI_ERROR (Status);
> > > +
> >
> > OK, so I guess we simply add EFI_MEMORY_PAGETYPE_MASK to the
> > capabilities of all memory space map entries that have a type different
> > from non-existent. We discussed it before and (apparently) it is
> > considered safe.
> >
> 
> Yes. I've validated different OSs boot. It's safe to stay this way.
> 
> > >  while (MemorySpaceLength > 0) {
> > >if (PageLength == 0) {
> > >  PageEntry = GetPageTableEntry (, BaseAddress,
> > );
> > > @@ -846,7 +857,6 @@ RefreshGcdMemoryAttributesFromPaging (
> > >  if (Attributes != (MemorySpaceMap[Index].Attributes &
> > EFI_MEMORY_PAGETYPE_MASK)) {
> > >DoUpdate = TRUE;
> > >Attributes |= (MemorySpaceMap[Index].Attributes &
> > ~EFI_MEMORY_PAGETYPE_MASK);
> > > -  Capabilities = Attributes | MemorySpaceMap[Index].Capabilities;
> > >  } else {
> > >DoUpdate = FALSE;
> > >  }
> > > @@ -854,8 +864,8 @@ 

Re: [edk2] [MdeModulePkg/TerminalDxe] Why do we delay 2s for ESC being pressed?

[Gerd Hoffmann]

On Wed, Nov 08, 2017 at 04:44:37PM +0800, Heyi Guo wrote:
> 
> 
> 在 11/8/2017 4:34 PM, Ni, Ruiyu 写道:
> > No.
> > Even a terminal tool can recognize F10, it still needs to translate it into 
> > "ESC [ V"
> > and send the three bytes to firmware.
> Got it. But the 2 seconds timeout is not for this situation, right? If
> terminal tool could translate and send the key sequence, I think it can
> complete 3 bytes transfer in a very short time, isn't it? E.g. 9600 baud / 8
> = 1200 Bytes/s (ignore control bits).
> 
> So 2 seconds timeout is still for user to enter the sequence "ESC [ V"
> manually?

No.  Alot of software has this kind of delay because it is recommended
in some classic unix documentation to avoid mis-interpreting incomplete
terminal control sequences coming from slow terminals.

Where a "slow terminal" which actually would need such a long delay is a
physical terminal from the 70ies of the last century, or a virtual
terminal hooked up over a *really* slow network connection.

Reducing the delay from 2 seconds to roughly 0.2 seconds should be
pretty safe, things are not that slow any more these days :)

HTH,
  Gerd

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

Re: [edk2] [MdeModulePkg/TerminalDxe] Why do we delay 2s for ESC being pressed?

[Heyi Guo]

That makes sense. Thanks very much for your explanation.

Regards,

Heyi


在 11/8/2017 4:46 PM, Ni, Ruiyu 写道:

Yes. 2 seconds is for some other terminal tools, that cannot do the F10 
translation.

Thanks/Ray


-Original Message-
From: Heyi Guo [mailto:heyi@linaro.org]
Sent: Wednesday, November 8, 2017 4:45 PM
To: Ni, Ruiyu ; Zeng, Star ; edk2-
de...@lists.01.org
Cc: Dong, Eric 
Subject: Re: [MdeModulePkg/TerminalDxe] Why do we delay 2s for ESC
being pressed?



在 11/8/2017 4:34 PM, Ni, Ruiyu 写道:

No.
Even a terminal tool can recognize F10, it still needs to translate it into "ESC

[ V"

and send the three bytes to firmware.

Got it. But the 2 seconds timeout is not for this situation, right? If terminal
tool could translate and send the key sequence, I think it can complete 3
bytes transfer in a very short time, isn't it? E.g. 9600 baud / 8 = 1200 Bytes/s
(ignore control bits).

So 2 seconds timeout is still for user to enter the sequence "ESC [ V"
manually?

Thanks,

Heyi


Thanks/Ray


-Original Message-
From: Heyi Guo [mailto:heyi@linaro.org]
Sent: Wednesday, November 8, 2017 4:31 PM
To: Ni, Ruiyu ; Zeng, Star ;
edk2- de...@lists.01.org
Cc: Dong, Eric 
Subject: Re: [MdeModulePkg/TerminalDxe] Why do we delay 2s for ESC
being pressed?



在 11/8/2017 3:55 PM, Ni, Ruiyu 写道:

Heyi,

If you check the comments below in TerminalConIn.c:


https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal

/C

onsole/TerminalDxe/TerminalConIn.c#L1319

TerminalDxe driver needs to determine whether user wants to press
ESC alone, or press "ESC [ V" for F10 (PCANSI terminal).

Do you mean F10 is not directly supported on some terminal tools so
that we need to press 3 keys "ESC [ V" quickly and continuously to

emulate F10?

Thanks,

Heyi

So a 2 second timeout is added to wait additional keys.

Thanks/Ray


-Original Message-
From: Zeng, Star
Sent: Wednesday, November 8, 2017 3:25 PM
To: Heyi Guo ; edk2-devel@lists.01.org
Cc: Ni, Ruiyu ; Dong, Eric
; Zeng, Star 
Subject: RE: [MdeModulePkg/TerminalDxe] Why do we delay 2s for

ESC

being pressed?

Cc Terminal expert Ray to see if any comments on this.


Thanks,
Star
-Original Message-
From: Heyi Guo [mailto:heyi@linaro.org]
Sent: Wednesday, November 8, 2017 3:04 PM
To: edk2-devel@lists.01.org
Cc: Zeng, Star ; Dong, Eric

Subject: [MdeModulePkg/TerminalDxe] Why do we delay 2s for ESC

being

pressed?

Hi folks,

We found ESC key responded fairly slow on serial port terminal, and
we think it might be caused by the code in UnicodeToEfiKey in

TerminalConIn.c:

    if (UnicodeChar == ESC) {
  TerminalDevice->InputState = INPUT_STATE_ESC;
    }

    if (UnicodeChar == CSI) {
  TerminalDevice->InputState = INPUT_STATE_CSI;
    }

    if (TerminalDevice->InputState != INPUT_STATE_DEFAULT) {
  Status = gBS->SetTimer(
  TerminalDevice->TwoSecondTimeOut,
  TimerRelative,
  (UINT64)2000
  );
  ASSERT_EFI_ERROR (Status);
  continue;
    }

It seems we intentionally add 2 seconds delay for ESC key press.
This provides not so good user experience when we press ESC to exit
or cancel some operation.

We tried reducing this timeout value to 1 second, then the
experience improved much and we didn't find any issue introduced.

What's the reason for this timeout value and is there any improvement?

Thanks and regards,

Heyi


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

Re: [edk2] [MdeModulePkg/TerminalDxe] Why do we delay 2s for ESC being pressed?

[Ni, Ruiyu]

Yes. 2 seconds is for some other terminal tools, that cannot do the F10 
translation.

Thanks/Ray

> -Original Message-
> From: Heyi Guo [mailto:heyi@linaro.org]
> Sent: Wednesday, November 8, 2017 4:45 PM
> To: Ni, Ruiyu ; Zeng, Star ; edk2-
> de...@lists.01.org
> Cc: Dong, Eric 
> Subject: Re: [MdeModulePkg/TerminalDxe] Why do we delay 2s for ESC
> being pressed?
> 
> 
> 
> 在 11/8/2017 4:34 PM, Ni, Ruiyu 写道:
> > No.
> > Even a terminal tool can recognize F10, it still needs to translate it into 
> > "ESC
> [ V"
> > and send the three bytes to firmware.
> Got it. But the 2 seconds timeout is not for this situation, right? If 
> terminal
> tool could translate and send the key sequence, I think it can complete 3
> bytes transfer in a very short time, isn't it? E.g. 9600 baud / 8 = 1200 
> Bytes/s
> (ignore control bits).
> 
> So 2 seconds timeout is still for user to enter the sequence "ESC [ V"
> manually?
> 
> Thanks,
> 
> Heyi
> 
> >
> > Thanks/Ray
> >
> >> -Original Message-
> >> From: Heyi Guo [mailto:heyi@linaro.org]
> >> Sent: Wednesday, November 8, 2017 4:31 PM
> >> To: Ni, Ruiyu ; Zeng, Star ;
> >> edk2- de...@lists.01.org
> >> Cc: Dong, Eric 
> >> Subject: Re: [MdeModulePkg/TerminalDxe] Why do we delay 2s for ESC
> >> being pressed?
> >>
> >>
> >>
> >> 在 11/8/2017 3:55 PM, Ni, Ruiyu 写道:
> >>> Heyi,
> >>>
> >>> If you check the comments below in TerminalConIn.c:
> >>>
> >>
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal
> >> /C
> >>> onsole/TerminalDxe/TerminalConIn.c#L1319
> >>>
> >>> TerminalDxe driver needs to determine whether user wants to press
> >>> ESC alone, or press "ESC [ V" for F10 (PCANSI terminal).
> >> Do you mean F10 is not directly supported on some terminal tools so
> >> that we need to press 3 keys "ESC [ V" quickly and continuously to
> emulate F10?
> >>
> >> Thanks,
> >>
> >> Heyi
> >>> So a 2 second timeout is added to wait additional keys.
> >>>
> >>> Thanks/Ray
> >>>
>  -Original Message-
>  From: Zeng, Star
>  Sent: Wednesday, November 8, 2017 3:25 PM
>  To: Heyi Guo ; edk2-devel@lists.01.org
>  Cc: Ni, Ruiyu ; Dong, Eric
>  ; Zeng, Star 
>  Subject: RE: [MdeModulePkg/TerminalDxe] Why do we delay 2s for
> ESC
>  being pressed?
> 
>  Cc Terminal expert Ray to see if any comments on this.
> 
> 
>  Thanks,
>  Star
>  -Original Message-
>  From: Heyi Guo [mailto:heyi@linaro.org]
>  Sent: Wednesday, November 8, 2017 3:04 PM
>  To: edk2-devel@lists.01.org
>  Cc: Zeng, Star ; Dong, Eric
>  
>  Subject: [MdeModulePkg/TerminalDxe] Why do we delay 2s for ESC
> >> being
>  pressed?
> 
>  Hi folks,
> 
>  We found ESC key responded fairly slow on serial port terminal, and
>  we think it might be caused by the code in UnicodeToEfiKey in
> >> TerminalConIn.c:
>     if (UnicodeChar == ESC) {
>   TerminalDevice->InputState = INPUT_STATE_ESC;
>     }
> 
>     if (UnicodeChar == CSI) {
>   TerminalDevice->InputState = INPUT_STATE_CSI;
>     }
> 
>     if (TerminalDevice->InputState != INPUT_STATE_DEFAULT) {
>   Status = gBS->SetTimer(
>   TerminalDevice->TwoSecondTimeOut,
>   TimerRelative,
>   (UINT64)2000
>   );
>   ASSERT_EFI_ERROR (Status);
>   continue;
>     }
> 
>  It seems we intentionally add 2 seconds delay for ESC key press.
>  This provides not so good user experience when we press ESC to exit
>  or cancel some operation.
> 
>  We tried reducing this timeout value to 1 second, then the
>  experience improved much and we didn't find any issue introduced.
> 
>  What's the reason for this timeout value and is there any improvement?
> 
>  Thanks and regards,
> 
>  Heyi

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

Re: [edk2] [MdeModulePkg/TerminalDxe] Why do we delay 2s for ESC being pressed?

[Heyi Guo]

在 11/8/2017 4:34 PM, Ni, Ruiyu 写道:

No.
Even a terminal tool can recognize F10, it still needs to translate it into "ESC [ 
V"
and send the three bytes to firmware.
Got it. But the 2 seconds timeout is not for this situation, right? If 
terminal tool could translate and send the key sequence, I think it can 
complete 3 bytes transfer in a very short time, isn't it? E.g. 9600 baud 
/ 8 = 1200 Bytes/s (ignore control bits).


So 2 seconds timeout is still for user to enter the sequence "ESC [ V" 
manually?


Thanks,

Heyi



Thanks/Ray


-Original Message-
From: Heyi Guo [mailto:heyi@linaro.org]
Sent: Wednesday, November 8, 2017 4:31 PM
To: Ni, Ruiyu ; Zeng, Star ; edk2-
de...@lists.01.org
Cc: Dong, Eric 
Subject: Re: [MdeModulePkg/TerminalDxe] Why do we delay 2s for ESC
being pressed?



在 11/8/2017 3:55 PM, Ni, Ruiyu 写道:

Heyi,

If you check the comments below in TerminalConIn.c:


https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal
/C

onsole/TerminalDxe/TerminalConIn.c#L1319

TerminalDxe driver needs to determine whether user wants to press ESC
alone, or press "ESC [ V" for F10 (PCANSI terminal).

Do you mean F10 is not directly supported on some terminal tools so that we
need to press 3 keys "ESC [ V" quickly and continuously to emulate F10?

Thanks,

Heyi

So a 2 second timeout is added to wait additional keys.

Thanks/Ray


-Original Message-
From: Zeng, Star
Sent: Wednesday, November 8, 2017 3:25 PM
To: Heyi Guo ; edk2-devel@lists.01.org
Cc: Ni, Ruiyu ; Dong, Eric ;
Zeng, Star 
Subject: RE: [MdeModulePkg/TerminalDxe] Why do we delay 2s for ESC
being pressed?

Cc Terminal expert Ray to see if any comments on this.


Thanks,
Star
-Original Message-
From: Heyi Guo [mailto:heyi@linaro.org]
Sent: Wednesday, November 8, 2017 3:04 PM
To: edk2-devel@lists.01.org
Cc: Zeng, Star ; Dong, Eric

Subject: [MdeModulePkg/TerminalDxe] Why do we delay 2s for ESC

being

pressed?

Hi folks,

We found ESC key responded fairly slow on serial port terminal, and
we think it might be caused by the code in UnicodeToEfiKey in

TerminalConIn.c:

       if (UnicodeChar == ESC) {
     TerminalDevice->InputState = INPUT_STATE_ESC;
       }

       if (UnicodeChar == CSI) {
     TerminalDevice->InputState = INPUT_STATE_CSI;
       }

       if (TerminalDevice->InputState != INPUT_STATE_DEFAULT) {
     Status = gBS->SetTimer(
     TerminalDevice->TwoSecondTimeOut,
     TimerRelative,
     (UINT64)2000
     );
     ASSERT_EFI_ERROR (Status);
     continue;
       }

It seems we intentionally add 2 seconds delay for ESC key press. This
provides not so good user experience when we press ESC to exit or
cancel some operation.

We tried reducing this timeout value to 1 second, then the experience
improved much and we didn't find any issue introduced.

What's the reason for this timeout value and is there any improvement?

Thanks and regards,

Heyi


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

Re: [edk2] [MdeModulePkg/TerminalDxe] Why do we delay 2s for ESC being pressed?

[Ni, Ruiyu]

No.
Even a terminal tool can recognize F10, it still needs to translate it into 
"ESC [ V"
and send the three bytes to firmware.

Thanks/Ray

> -Original Message-
> From: Heyi Guo [mailto:heyi@linaro.org]
> Sent: Wednesday, November 8, 2017 4:31 PM
> To: Ni, Ruiyu ; Zeng, Star ; edk2-
> de...@lists.01.org
> Cc: Dong, Eric 
> Subject: Re: [MdeModulePkg/TerminalDxe] Why do we delay 2s for ESC
> being pressed?
> 
> 
> 
> 在 11/8/2017 3:55 PM, Ni, Ruiyu 写道:
> > Heyi,
> >
> > If you check the comments below in TerminalConIn.c:
> >
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal
> /C
> > onsole/TerminalDxe/TerminalConIn.c#L1319
> >
> > TerminalDxe driver needs to determine whether user wants to press ESC
> > alone, or press "ESC [ V" for F10 (PCANSI terminal).
> Do you mean F10 is not directly supported on some terminal tools so that we
> need to press 3 keys "ESC [ V" quickly and continuously to emulate F10?
> 
> Thanks,
> 
> Heyi
> >
> > So a 2 second timeout is added to wait additional keys.
> >
> > Thanks/Ray
> >
> >> -Original Message-
> >> From: Zeng, Star
> >> Sent: Wednesday, November 8, 2017 3:25 PM
> >> To: Heyi Guo ; edk2-devel@lists.01.org
> >> Cc: Ni, Ruiyu ; Dong, Eric ;
> >> Zeng, Star 
> >> Subject: RE: [MdeModulePkg/TerminalDxe] Why do we delay 2s for ESC
> >> being pressed?
> >>
> >> Cc Terminal expert Ray to see if any comments on this.
> >>
> >>
> >> Thanks,
> >> Star
> >> -Original Message-
> >> From: Heyi Guo [mailto:heyi@linaro.org]
> >> Sent: Wednesday, November 8, 2017 3:04 PM
> >> To: edk2-devel@lists.01.org
> >> Cc: Zeng, Star ; Dong, Eric
> >> 
> >> Subject: [MdeModulePkg/TerminalDxe] Why do we delay 2s for ESC
> being
> >> pressed?
> >>
> >> Hi folks,
> >>
> >> We found ESC key responded fairly slow on serial port terminal, and
> >> we think it might be caused by the code in UnicodeToEfiKey in
> TerminalConIn.c:
> >>
> >>       if (UnicodeChar == ESC) {
> >>     TerminalDevice->InputState = INPUT_STATE_ESC;
> >>       }
> >>
> >>       if (UnicodeChar == CSI) {
> >>     TerminalDevice->InputState = INPUT_STATE_CSI;
> >>       }
> >>
> >>       if (TerminalDevice->InputState != INPUT_STATE_DEFAULT) {
> >>     Status = gBS->SetTimer(
> >>     TerminalDevice->TwoSecondTimeOut,
> >>     TimerRelative,
> >>     (UINT64)2000
> >>     );
> >>     ASSERT_EFI_ERROR (Status);
> >>     continue;
> >>       }
> >>
> >> It seems we intentionally add 2 seconds delay for ESC key press. This
> >> provides not so good user experience when we press ESC to exit or
> >> cancel some operation.
> >>
> >> We tried reducing this timeout value to 1 second, then the experience
> >> improved much and we didn't find any issue introduced.
> >>
> >> What's the reason for this timeout value and is there any improvement?
> >>
> >> Thanks and regards,
> >>
> >> Heyi

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

Re: [edk2] [MdeModulePkg/TerminalDxe] Why do we delay 2s for ESC being pressed?

[Heyi Guo]

在 11/8/2017 3:55 PM, Ni, Ruiyu 写道:

Heyi,

If you check the comments below in TerminalConIn.c:
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c#L1319

TerminalDxe driver needs to determine whether user wants to press ESC alone,
or press "ESC [ V" for F10 (PCANSI terminal).
Do you mean F10 is not directly supported on some terminal tools so that 
we need to press 3 keys "ESC [ V" quickly and continuously to emulate F10?


Thanks,

Heyi


So a 2 second timeout is added to wait additional keys.

Thanks/Ray


-Original Message-
From: Zeng, Star
Sent: Wednesday, November 8, 2017 3:25 PM
To: Heyi Guo ; edk2-devel@lists.01.org
Cc: Ni, Ruiyu ; Dong, Eric ; Zeng,
Star 
Subject: RE: [MdeModulePkg/TerminalDxe] Why do we delay 2s for ESC
being pressed?

Cc Terminal expert Ray to see if any comments on this.


Thanks,
Star
-Original Message-
From: Heyi Guo [mailto:heyi@linaro.org]
Sent: Wednesday, November 8, 2017 3:04 PM
To: edk2-devel@lists.01.org
Cc: Zeng, Star ; Dong, Eric 
Subject: [MdeModulePkg/TerminalDxe] Why do we delay 2s for ESC being
pressed?

Hi folks,

We found ESC key responded fairly slow on serial port terminal, and we think
it might be caused by the code in UnicodeToEfiKey in TerminalConIn.c:

      if (UnicodeChar == ESC) {
    TerminalDevice->InputState = INPUT_STATE_ESC;
      }

      if (UnicodeChar == CSI) {
    TerminalDevice->InputState = INPUT_STATE_CSI;
      }

      if (TerminalDevice->InputState != INPUT_STATE_DEFAULT) {
    Status = gBS->SetTimer(
    TerminalDevice->TwoSecondTimeOut,
    TimerRelative,
    (UINT64)2000
    );
    ASSERT_EFI_ERROR (Status);
    continue;
      }

It seems we intentionally add 2 seconds delay for ESC key press. This
provides not so good user experience when we press ESC to exit or cancel
some operation.

We tried reducing this timeout value to 1 second, then the experience
improved much and we didn't find any issue introduced.

What's the reason for this timeout value and is there any improvement?

Thanks and regards,

Heyi


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

Re: [edk2] [PATCH v2 0/2] Add NVDIMM related spec definitions

[Gao, Liming]

Reviewed-by: Liming Gao 

>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>Ruiyu Ni
>Sent: Wednesday, November 08, 2017 2:33 PM
>To: edk2-devel@lists.01.org
>Subject: [edk2] [PATCH v2 0/2] Add NVDIMM related spec definitions
>
>v2:
>  1. Create Btt.h in Include/Guid directory per Liming's comments.
>  2. Change the GUID C name to match macro name.
>
>Ruiyu Ni (2):
>  MdePkg/Btt.h: Add Block Translation Table definitions
>  MdePkg/NvdimmLabel.h: Add NVDIMM_LABEL protocol definition
>
> MdePkg/Include/Guid/Btt.h | 228 ++
> MdePkg/Include/Protocol/NvdimmLabel.h | 351
>++
> MdePkg/MdePkg.dec |   9 +
> 3 files changed, 588 insertions(+)
> create mode 100644 MdePkg/Include/Guid/Btt.h
> create mode 100644 MdePkg/Include/Protocol/NvdimmLabel.h
>
>--
>2.12.2.windows.2
>
>___
>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