Re: [edk2] [PATCH] MdeModulePkg/SerialDxe: Do not fail reset when SetAttributes is not supported

2017-10-23 Thread Zeng, Star 
I also agree the mapping.

Julien,
There is a typo in commit log " instropecting " should be " introspecting ".
Anyway, you may need the commit log according to the mapping, you can take care 
of the typo in the updated commit log.


Thanks,
Star

-Original Message-
From: Ni, Ruiyu 
Sent: Thursday, October 19, 2017 11:03 AM
To: Laszlo Ersek ; Julien Grall ; 
Zeng, Star ; Dong, Eric ; 
pankaj.ban...@nxp.com; leif.lindh...@linaro.org
Cc: edk2-devel@lists.01.org
Subject: RE: [edk2] [PATCH] MdeModulePkg/SerialDxe: Do not fail reset when 
SetAttributes is not supported

Laszlo,
I agree with your status mapping.
It will make the implementation more clear and easier to maintain.

Thanks/Ray

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> Laszlo Ersek
> Sent: Wednesday, October 18, 2017 8:12 PM
> To: Julien Grall ; Zeng, Star 
> ; Dong, Eric ; 
> pankaj.ban...@nxp.com; leif.lindh...@linaro.org
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH] MdeModulePkg/SerialDxe: Do not fail reset 
> when SetAttributes is not supported
> 
> On 10/18/17 12:19, Julien Grall wrote:
> > After commit 91cc526b15 "MdeModulePkg/SerialDxe: Fix not able to 
> > change serial attributes", serial is initialized using the reset 
> > method that will call SetAttributes.
> >
> > However, SetAttributes may not be supported by the driver and will 
> > return an error (i.e RETURN_UNSUPPORTED) that will be propagate and 
> > lead to UEFI failing to get the console setup.
> >
> > For instance, this is the case when using the Xen console driver.
> >
> > Fix it by instropecting the result and return RETURN_SUCCESS when 
> > the driver report it is not supported (i.e RETURN_UNSUPPORTED).
> >
> > Contributed-under: Tianocore Contribution Agreement 1.1
> > Signed-off-by: Julien Grall 
> > ---
> >  MdeModulePkg/Universal/SerialDxe/SerialIo.c | 6 ++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> > b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> > index ebcd927263..4253e0b8ea 100644
> > --- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> > +++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> > @@ -238,6 +238,12 @@ SerialReset (
> > (UINT8) This->Mode->DataBits,
> > (EFI_STOP_BITS_TYPE) This->Mode->StopBits
> > );
> > +  //
> > +  // The serial device may not support SetAttributes.
> > +  // Set the status to RETURN_SUCCESS to prevent later failure.
> > +  //
> > +  if ( Status == RETURN_UNSUPPORTED )
> 
> The extra spaces within the parens are Xen coding style, not edk2 
> coding style; please remove them.
> 
> > +  return RETURN_SUCCESS;
> 
> The edk2 coding style requires braces.
> 
> The edk2 coding style requires two spaces as indentation, in this context.
> 
> >
> >return Status;
> >  }
> >
> 
> The UEFI spec (v2.7) describes the following return values for
> EFI_SERIAL_IO_PROTOCOL.Reset():
> 
> - EFI_SUCCESS: The serial device was reset.
> - EFI_DEVICE_ERROR: The serial device could not be reset.
> 
> For the EFI_SERIAL_IO_PROTOCOL.SetAttributes() member function:
> 
> - EFI_SUCCESS: The new attributes were set on the serial device.
> - EFI_INVALID_PARAMETER: One or more of the attributes has an
>  unsupported value.
> - EFI_DEVICE_ERROR: The serial device is not functioning correctly.
> 
> In MdeModulePkg/Universal/SerialDxe, the SetAttributes() member 
> function is implemented by SerialSetAttributes(), and it delegates the 
> operation to the SerialPortSetAttributes() API from the following library 
> class:
> 
>   MdePkg/Include/Library/SerialPortLib.h
> 
> The API defines the following return codes:
> 
>   @retval RETURN_SUCCESSThe new attributes were set on the
> serial device.
>   @retval RETURN_UNSUPPORTEDThe serial device does not support
> this operation.
>   @retval RETURN_INVALID_PARAMETER  One or more of the attributes has 
> an
> unsupported value.
>   @retval RETURN_DEVICE_ERROR   The serial device is not functioning
> correctly.
> 
> Therefore I think the following should be done:
> 
> (1) The SerialPortSetAttributes() implementation in 
> "OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c" is 
> correct; returning RETURN_UNSUPPORTED is valid, according to the lib 
> class header.
> 
> (2) The direct propagation of the return value in 
> SerialSetAttributes() [MdeModulePkg/Universal/SerialDxe/SerialIo.c] 
> does not look correct. It should implement the following mapping:
> 
> RETURN_SUCCESS   -> EFI_SUCCESS
> RETURN_UNSUPPORTED   ->

[edk2] [PATCH] ShellPkg/HandleParsingLib: Remove unnecessary CatSPrint call

2017-10-23 Thread Ruiyu Ni 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni 
Cc: Jaben Carsey 
---
 ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c 
b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
index 15103160d9..d343f3352e 100644
--- a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
+++ b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
@@ -259,9 +259,7 @@ LoadedImageProtocolDumpInformation(
 } else {
   SHELL_FREE_NON_NULL(FilePath);
 }
-RetVal = CatSPrint(NULL, FileName);
-SHELL_FREE_NON_NULL(FileName);
-return RetVal;
+return FileName;
   }
 
   HandleParsingHiiInit();
-- 
2.12.2.windows.2

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

[edk2] [PATCH] MdeModulePkg/UfsBlockIoPei: Support IoMmu

2017-10-23 Thread Hao Wu 
Update the UfsBlockIoPei driver to consume IOMMU_PPI to allocate DMA
buffer.

If no IOMMU_PPI exists, this driver still calls PEI service
to allocate DMA buffer, with assumption that DRAM==DMA.

This is a compatible change.

Cc: Star Zeng 
Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu 
---
 MdeModulePkg/Bus/Ufs/UfsBlockIoPei/DmaMem.c  | 249 
 MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsBlockIoPei.c   |  56 -
 MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsBlockIoPei.h   | 127 +-
 MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsBlockIoPei.inf |   5 +-
 MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHcMem.c|  23 +-
 MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHcMem.h|   4 +-
 MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c  |  91 +--
 7 files changed, 517 insertions(+), 38 deletions(-)

diff --git a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/DmaMem.c 
b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/DmaMem.c
new file mode 100644
index 00..0a939a3879
--- /dev/null
+++ b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/DmaMem.c
@@ -0,0 +1,249 @@
+/** @file
+  The DMA memory help function.
+
+  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.
+
+**/
+
+#include "UfsBlockIoPei.h"
+
+EDKII_IOMMU_PPI  *mIoMmu;
+
+/**
+  Provides the controller-specific addresses required to access system memory 
from a
+  DMA bus master.
+
+  @param  Operation Indicates if the bus master is going to read 
or write to system memory.
+  @param  HostAddress   The system memory address to map to the PCI 
controller.
+  @param  NumberOfBytes On input the number of bytes to map. On output 
the number of bytes
+that were mapped.
+  @param  DeviceAddress The resulting map address for the bus master 
PCI controller to use to
+access the hosts HostAddress.
+  @param  Mapping   A resulting value to pass to Unmap().
+
+  @retval EFI_SUCCESS   The range was mapped for the returned 
NumberOfBytes.
+  @retval EFI_UNSUPPORTED   The HostAddress cannot be mapped as a common 
buffer.
+  @retval EFI_INVALID_PARAMETER One or more parameters are invalid.
+  @retval EFI_OUT_OF_RESOURCES  The request could not be completed due to a 
lack of resources.
+  @retval EFI_DEVICE_ERROR  The system hardware could not map the 
requested address.
+
+**/
+EFI_STATUS
+IoMmuMap (
+  IN  EDKII_IOMMU_OPERATION Operation,
+  IN VOID   *HostAddress,
+  IN  OUT UINTN *NumberOfBytes,
+  OUT EFI_PHYSICAL_ADDRESS  *DeviceAddress,
+  OUT VOID  **Mapping
+  )
+{
+  EFI_STATUS  Status;
+  UINT64  Attribute;
+
+  if (mIoMmu != NULL) {
+Status = mIoMmu->Map (
+   mIoMmu,
+   Operation,
+   HostAddress,
+   NumberOfBytes,
+   DeviceAddress,
+   Mapping
+   );
+if (EFI_ERROR (Status)) {
+  return EFI_OUT_OF_RESOURCES;
+}
+switch (Operation) {
+case EdkiiIoMmuOperationBusMasterRead:
+case EdkiiIoMmuOperationBusMasterRead64:
+  Attribute = EDKII_IOMMU_ACCESS_READ;
+  break;
+case EdkiiIoMmuOperationBusMasterWrite:
+case EdkiiIoMmuOperationBusMasterWrite64:
+  Attribute = EDKII_IOMMU_ACCESS_WRITE;
+  break;
+case EdkiiIoMmuOperationBusMasterCommonBuffer:
+case EdkiiIoMmuOperationBusMasterCommonBuffer64:
+  Attribute = EDKII_IOMMU_ACCESS_READ | EDKII_IOMMU_ACCESS_WRITE;
+  break;
+default:
+  ASSERT(FALSE);
+  return EFI_INVALID_PARAMETER;
+}
+Status = mIoMmu->SetAttribute (
+   mIoMmu,
+   *Mapping,
+   Attribute
+   );
+if (EFI_ERROR (Status)) {
+  return Status;
+}
+  } else {
+*DeviceAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress;
+*Mapping = NULL;
+Status = EFI_SUCCESS;
+  }
+  return Status;
+}
+
+/**
+  Completes the Map() operation and releases any corresponding resources.
+
+  @param  Mapping   The mapping value returned from Map().
+
+  @retval EFI_SUCCESS   The range was unmapped.
+  @retval EFI_INVALID_PARAMETER Mapping is not a value that was returned by 
Map().
+  @retval EFI_DEVICE_ERROR  The data was not committed to the target 
system memory.
+**/
+EFI_STATUS
+IoMmuUnmap (
+  IN VOID

Re: [edk2] [Patch 0/2] Add IPv6 support condition check for HTTP/ISCSI.

2017-10-23 Thread Wu, Jiaxin 
Hi Karunakar,

For #3, it's the non-regression issue that will cause the target IP is invalid 
if it's the same with previous one. And I'd like to fix it by another patch 
since it's not related to the InitiatorInfo display.

Can you please report another Bugzilla for the ISCSI target address issue?

Thanks,
Jiaxin

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Karunakar P
> Sent: Tuesday, October 24, 2017 12:41 AM
> To: Wu, Jiaxin ; edk2-devel@lists.01.org
> Cc: Ye, Ting ; Fu, Siyuan 
> Subject: Re: [edk2] [Patch 0/2] Add IPv6 support condition check for
> HTTP/ISCSI.
> 
> Hi Jiaxin,
> 
> 1. I've verified, IPv6 support condition check for HTTP/ISCSI, It works fine 
> and
> the ASSERT issue also resolved.
> 2. Regards Display Initiator IP, It resolves the issues the issues which I've
> mentioned previously.
> 3. I found some other issue in ISCSI, Below are the details
> A. Add Attempt1 with Target Info via Static and provide Target Name &
> Target IP, Save changes.
> B. If we are trying to add another attempt , It is taking Target IP as 
> default IP
> which is same Target IP given in Attempt1.
> 
> Could you please check at your end provide your comments on 3rd one.
> 
> Thank You,
> Karunakar
> 
> -Original Message-
> From: Wu, Jiaxin [mailto:jiaxin...@intel.com]
> Sent: Wednesday, October 18, 2017 2:30 PM
> To: Karunakar P; edk2-devel@lists.01.org
> Cc: Ye, Ting; Fu, Siyuan
> Subject: RE: [edk2] [Patch 0/2] Add IPv6 support condition check for
> HTTP/ISCSI.
> 
> Hi Karunakar,
> 
> Actually, in my part, I didn't meet the ASSERT no matter the "Ipv6Available"
> returned from HttpBootCheckIpv6Support is true or false. According your
> ASSERT case that happened in Ip4DxeDriverBindingStart, which is caused by
> the FreePool of Private in Ip6DxeDriverBindingStart, so I guess the
> Ip6DxeDriverBindingStart may be involved ahead of
> Ip4DxeDriverBindingStart, then something wrong happened in
> Ip6DxeDriverBindingStart and goto the ON_ERROR (Private is freed here!). so,
> you can add the breakpoint within
> Ip6DxeDriverBindingStart/Ip4DxeDriverBindingStart to check it.
> 
> Per my analysis above, it does the issue that may trigger the potential 
> ASSERT.
> So, I refined the patch as version2. The principle of patch v2 is that IPv6 
> and
> IPv4 should not affect each other even any failure happen, but the original
> code doesn't follow that:).
> 
> 
> Thanks,
> Jiaxin
> 
> 
> > -Original Message-
> > From: Karunakar P [mailto:karunak...@amiindia.co.in]
> > Sent: Wednesday, October 18, 2017 4:06 PM
> > To: Wu, Jiaxin ; edk2-devel@lists.01.org
> > Cc: Ye, Ting ; Fu, Siyuan 
> > Subject: RE: [edk2] [Patch 0/2] Add IPv6 support condition check for
> > HTTP/ISCSI.
> >
> > Hi Jiaxin,
> >
> > 1. Cleaning the ConfigData when switching the mode will resolve the Issue
> A.
> > 2. Will also verify the ASSERT issue and update you.
> >
> > Could you please help in clarifying the below items, 1. Could you
> > please let us know why the ASSERT happens?
> > 2. I've not faced any ASSERT with the changes attached in Bugzilla,
> > Did you find any issues/drawbacks with that?
> > -> In HttpBootCheckIpv6Support () definition Instead of getting
> > -> Ipv6Support
> > from Private->Nii->Ipv6Supported if we open the protocol there itself,
> > Then there will not be issues in Destroying Children or FreePool(Private)
> right.
> > Because we're going to check  HttpBootCheckIpv6Support() before
> > opening any instances in HttpBootIp6DxeDriverBindingStart().
> >
> > Thanks for your great support.
> >
> > Thank You,
> > Karunakar
> > 
> > From: Wu, Jiaxin [jiaxin...@intel.com]
> > Sent: 18 October 2017 12:35
> > To: Karunakar P; edk2-devel@lists.01.org
> > Cc: Ye, Ting; Fu, Siyuan
> > Subject: RE: [edk2] [Patch 0/2] Add IPv6 support condition check for
> > HTTP/ISCSI.
> >
> > Hi Karunakar,
> >
> > Thanks your verification. Base on your comments, I refined the series
> > patches as below to fix the issues:
> >
> > [Patch v3 0/3] NetworkPkg/IScsiDxe: Display InitiatorInfo correctly.
> > NetworkPkg/IScsiDxe: Fix the incorrect/needless DHCP process.
> > NetworkPkg/IScsiDxe: Clean the previous ConfigData when
> > switching the IP mode. /// This one is to fix the issue A.
> > NetworkPkg/IScsiDxe: Display InitiatorInfo in attempt page
> > even DHCP enabled.
> >
> > [Patch v2 0/2] Add IPv6 support condition check.
> >  NetworkPkg/HttpBootDxe: Add IPv6 support condition check.
> > /// B && C have been fixed in version 2.
> >  NetworkPkg/IScsiDxe: Add IPv6 support condition check.
> >
> > Please help to verify them.
> >
> > Best Regards,
> > Jiaxin
> >
> >
> > > -Original Message-
> > > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On

Re: [edk2] [PATCH] IntelSiliconPkg/VtdInfoSample: Fix IGD RMRR memory.

2017-10-23 Thread Zeng, Star 
Reviewed-by: Star Zeng 

-Original Message-
From: Yao, Jiewen 
Sent: Friday, October 20, 2017 5:07 PM
To: edk2-devel@lists.01.org; Zeng, Star 
Subject: Re: [edk2] [PATCH] IntelSiliconPkg/VtdInfoSample: Fix IGD RMRR memory.

Correct email address

thank you!
Yao, Jiewen


> 在 2017年10月20日，下午4:51，Jiewen Yao  写道：
> 
> Fix a calculation problem in IGD RMRR memory.
> 
> Cc: Zeng Star 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jiewen Yao 
> ---
> IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.c
>  | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git 
> a/IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.c
>  
> b/IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.c
> index 08a4db3..6267da7 100644
> --- 
> a/IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.c
> +++ 
> b/IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.c
> @@ -159,7 +159,7 @@ PatchDmar (
>   /// Calculate GTT mem size
>   ///
>   GttMemSize = 0;
> -  GttMode = PciRead16 (PCI_LIB_ADDRESS(0, 0, 0, R_SA_GGC) & 
> B_SKL_SA_GGC_GGMS_MASK) >> N_SKL_SA_GGC_GGMS_OFFSET;
> +  GttMode = (PciRead16 (PCI_LIB_ADDRESS(0, 0, 0, R_SA_GGC)) & 
> B_SKL_SA_GGC_GGMS_MASK) >> N_SKL_SA_GGC_GGMS_OFFSET;
>   if (GttMode <= V_SKL_SA_GGC_GGMS_8MB) {
> GttMemSize = (1 << GttMode) * (1024) * (1024);
>   }
> -- 
> 2.7.4.windows.1
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] MdeModulePkg/Variable/RuntimeDxe: delete & lock MOR in the absence of SMM

2017-10-23 Thread Zeng, Star 
I prefer to set the attributes to 0 for the MorLock deletion in the same patch.

It is fine to have empty MorLockInitAtEndOfDxe() in TcgMorLockDxe.c with your 
explanation.


Thanks,
Star
-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com] 
Sent: Wednesday, October 11, 2017 1:22 AM
To: Zeng, Star ; edk2-devel-01 
Cc: Dong, Eric ; Yao, Jiewen 
Subject: Re: [PATCH] MdeModulePkg/Variable/RuntimeDxe: delete & lock MOR in the 
absence of SMM

On 10/10/17 15:54, Zeng, Star wrote:
> Could you help make the code consistent to call
> VariableServiceSetVariable() for the Attributes? One original for 
> MorLock is using real attributes, the new you added for Mor will use 0 
> attributes. How about to both use 0 attributes.

Sure, I can do that -- do you want me to set the attributes to 0 for the 
MorLock deletion in the same patch, or should I do it in a separate patch?

> Another question, why empty MorLockInitAtEndOfDxe() needs to be 
> implemented in TcgMorLockDxe.c and called in VariableDxe.c?

Because otherwise we would break the promise we make in
"PrivilegePolymorphic.h":

>   Polymorphic functions that are called from both the privileged driver (i.e.,
>   the DXE_SMM variable module) and the non-privileged drivers (i.e., one or
>   both of the DXE_RUNTIME variable modules).
>
>   Each of these functions has two implementations, appropriate for privileged
>   vs. non-privileged driver code.

If I don't implement MorLockInitAtEndOfDxe() for VariableRuntimeDxe, then we'll 
have a declaration with no definition in VariableRuntimeDxe, and only one 
definition in total.

The empty function call should be optimized out in RELEASE builds anyway 
(assuming LTO is used).

It's easy to remove the empty definition and the calls to it, but then the 
purpose of "PrivilegePolymorphic.h" becomes less clear.

For complete clarity, I would have had to introduce *two* new header
files:

- one for SecureBootHook(), MorLockInit() and
  SetVariableCheckHandlerMor() -- to be used by both SMM and non-SMM,

- and another header file for just MorLockInitAtEndOfDxe() -- to be used
  only by SMM. (This is necessary because the call is made from
  "VariableSmm.c" to "TcgMorLockSmm.c").

I thought that introducing two new header files would not be accepted, so I 
opted for one header file only. But then I felt that the empty
MorLockInitAtEndOfDxe() hook should be correctly implemented for 
VariableRuntimeDxe too.

Thanks,
Laszlo


>
>
> Thanks,
> Star
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Tuesday, October 10, 2017 9:25 PM
> To: edk2-devel-01 
> Cc: Dong, Eric ; Yao, Jiewen 
> ; Zeng, Star 
> Subject: [PATCH] MdeModulePkg/Variable/RuntimeDxe: delete & lock MOR 
> in the absence of SMM
>
> VariableRuntimeDxe deletes and locks the MorLock variable in 
> MorLockInit(), with the argument that any protection provided by 
> MorLock can be circumvented if MorLock can be overwritten by 
> unprivileged code (i.e., outside of SMM).
>
> Extend the argument and the logic to the MOR variable, which is 
> supposed to be protected by MorLock.
>
> This change was suggested by Star; it is inspired by earlier 
> VariableSmm commit fda8f631edbb ("MdeModulePkg/Variable/RuntimeDxe: 
> delete and lock OS-created MOR variable", 2017-10-03).
>
> Cc: Eric Dong 
> Cc: Jiewen Yao 
> Cc: Star Zeng 
> Suggested-by: Star Zeng 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek 
> ---
>
> Notes:
> Repo:   https://github.com/lersek/edk2.git
> Branch: del_and_lock_mor_without_smm
>
>  MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c | 24 
> 
>  1 file changed, 24 insertions(+)
>
> diff --git 
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c 
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> index 7142e2da2073..1610c7aa0706 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> @@ -69,29 +69,53 @@ EFI_STATUS
>  MorLockInit (
>VOID
>)
>  {
>//
>// Always clear variable to report unsupported to OS.
>// The reason is that the DXE version is not proper to provide 
> *protection*.
>// BIOS should use SMM version variable driver to provide such capability.
>//
>VariableServiceSetVariable (
>  MEMORY_OVERWRITE_REQUEST_CONTROL_LOCK_NAME,
>  ,
>  EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | 
> EFI_VARIABLE_RUNTIME_ACCESS,
>  0,
>  NULL
>  );
>
>//
>// Need set this variable to be read-only to prevent other module set it.
>//
>VariableLockRequestToLock (, 
>

Re: [edk2] Fwd: timer ticks ?

2017-10-23 Thread Ken Taylor 
Hi David,

I think EVT_TIMER is required functionality.  Many drivers depend on it.

I know it goes away at runtime, but so do the rest of boot services.  There's 
nothing that can be done about that; even the BIOS timer tick was taken over by 
all modern operating systems, so it wasn't reliably persistent post boot either.

Regards,
-Ken.

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of David F.
Sent: Monday, October 23, 2017 4:59 PM
To: edk2 developers list
Subject: [edk2] Fwd: timer ticks ?

Hi,

Is there a reliable (always available) way to get a timer tick in an 
application similar to the old 18.2 per second timer tick from BIOS.
That one was always there, high speed access (no slow access to a RTC)  and the 
tick count is known (to approx calculate intervals for polling and other 
things).  In UEFI,  EVT_TIMER may not be supported.
GetTime may be slow RTC access.It can be a counter only since
machine was on.

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

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

[edk2] Fwd: timer ticks ?

2017-10-23 Thread David F. 
Hi,

Is there a reliable (always available) way to get a timer tick in an
application similar to the old 18.2 per second timer tick from BIOS.
That one was always there, high speed access (no slow access to a RTC)
 and the tick count is known (to approx calculate intervals for
polling and other things).  In UEFI,  EVT_TIMER may not be supported.
GetTime may be slow RTC access.It can be a counter only since
machine was on.

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

Re: [edk2] [PATCH] MdeModulePkg/SerialDxe: Do not fail reset when SetAttributes is not supported

2017-10-23 Thread Julien Grall 

Hi Laszlo,

On 18/10/17 13:11, Laszlo Ersek wrote:

On 10/18/17 12:19, Julien Grall wrote:

After commit 91cc526b15 "MdeModulePkg/SerialDxe: Fix not able to change
serial attributes", serial is initialized using the reset method that
will call SetAttributes.

However, SetAttributes may not be supported by the driver and will
return an error (i.e RETURN_UNSUPPORTED) that will be propagate and lead
to UEFI failing to get the console setup.

For instance, this is the case when using the Xen console driver.

Fix it by instropecting the result and return RETURN_SUCCESS when the
driver report it is not supported (i.e RETURN_UNSUPPORTED).

Contributed-under: Tianocore Contribution Agreement 1.1
Signed-off-by: Julien Grall 
---
  MdeModulePkg/Universal/SerialDxe/SerialIo.c | 6 ++
  1 file changed, 6 insertions(+)

diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c 
b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
index ebcd927263..4253e0b8ea 100644
--- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
+++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
@@ -238,6 +238,12 @@ SerialReset (
 (UINT8) This->Mode->DataBits,
 (EFI_STOP_BITS_TYPE) This->Mode->StopBits
 );
+  //
+  // The serial device may not support SetAttributes.
+  // Set the status to RETURN_SUCCESS to prevent later failure.
+  //
+  if ( Status == RETURN_UNSUPPORTED )


The extra spaces within the parens are Xen coding style, not edk2 coding
style; please remove them.


Sorry for that, I will fix the coding style in the next version.

[...]


I suggest waiting for feedback from Star & Eric. Dependent on their
response, your patch could be good enough (once you fix up the coding
style issues). Or else, they could agree with me that the return value
mapping of SerialSetAttributes() should be corrected first (2), and then
your patch should be please adapted as well (3).


Thank you for the detailed explanation. I will wait a couple of days 
more for feedbacks and then send a new version.




Bonus comment:

(4) The propagation of the SerialPortInitialize() retval in
SerialReset() looks correct, thankfully. (Both callee and caller are
expected to return one of *_SUCCESS and *_DEVICE_ERROR.)


Cheers,

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

Re: [edk2] [Patch 0/2] Add IPv6 support condition check for HTTP/ISCSI.

2017-10-23 Thread Karunakar P 
Hi Jiaxin,

1. I've verified, IPv6 support condition check for HTTP/ISCSI, It works fine 
and the ASSERT issue also resolved.
2. Regards Display Initiator IP, It resolves the issues the issues which I've 
mentioned previously.
3. I found some other issue in ISCSI, Below are the details
A. Add Attempt1 with Target Info via Static and provide Target Name & Target 
IP, Save changes.
B. If we are trying to add another attempt , It is taking Target IP as default 
IP which is same Target IP given in Attempt1.
  
Could you please check at your end provide your comments on 3rd one.

Thank You,
Karunakar

-Original Message-
From: Wu, Jiaxin [mailto:jiaxin...@intel.com] 
Sent: Wednesday, October 18, 2017 2:30 PM
To: Karunakar P; edk2-devel@lists.01.org
Cc: Ye, Ting; Fu, Siyuan
Subject: RE: [edk2] [Patch 0/2] Add IPv6 support condition check for HTTP/ISCSI.

Hi Karunakar,

Actually, in my part, I didn't meet the ASSERT no matter the "Ipv6Available" 
returned from HttpBootCheckIpv6Support is true or false. According your ASSERT 
case that happened in Ip4DxeDriverBindingStart, which is caused by the FreePool 
of Private in Ip6DxeDriverBindingStart, so I guess the Ip6DxeDriverBindingStart 
may be involved ahead of Ip4DxeDriverBindingStart, then something wrong 
happened in Ip6DxeDriverBindingStart and goto the ON_ERROR (Private is freed 
here!). so, you can add the breakpoint within 
Ip6DxeDriverBindingStart/Ip4DxeDriverBindingStart to check it.

Per my analysis above, it does the issue that may trigger the potential ASSERT. 
So, I refined the patch as version2. The principle of patch v2 is that IPv6 and 
IPv4 should not affect each other even any failure happen, but the original 
code doesn't follow that:).
 

Thanks,
Jiaxin


> -Original Message-
> From: Karunakar P [mailto:karunak...@amiindia.co.in]
> Sent: Wednesday, October 18, 2017 4:06 PM
> To: Wu, Jiaxin ; edk2-devel@lists.01.org
> Cc: Ye, Ting ; Fu, Siyuan 
> Subject: RE: [edk2] [Patch 0/2] Add IPv6 support condition check for 
> HTTP/ISCSI.
> 
> Hi Jiaxin,
> 
> 1. Cleaning the ConfigData when switching the mode will resolve the Issue A.
> 2. Will also verify the ASSERT issue and update you.
> 
> Could you please help in clarifying the below items, 1. Could you 
> please let us know why the ASSERT happens?
> 2. I've not faced any ASSERT with the changes attached in Bugzilla, 
> Did you find any issues/drawbacks with that?
> -> In HttpBootCheckIpv6Support () definition Instead of getting 
> -> Ipv6Support
> from Private->Nii->Ipv6Supported if we open the protocol there itself, 
> Then there will not be issues in Destroying Children or FreePool(Private) 
> right.
> Because we're going to check  HttpBootCheckIpv6Support() before 
> opening any instances in HttpBootIp6DxeDriverBindingStart().
> 
> Thanks for your great support.
> 
> Thank You,
> Karunakar
> 
> From: Wu, Jiaxin [jiaxin...@intel.com]
> Sent: 18 October 2017 12:35
> To: Karunakar P; edk2-devel@lists.01.org
> Cc: Ye, Ting; Fu, Siyuan
> Subject: RE: [edk2] [Patch 0/2] Add IPv6 support condition check for 
> HTTP/ISCSI.
> 
> Hi Karunakar,
> 
> Thanks your verification. Base on your comments, I refined the series 
> patches as below to fix the issues:
> 
> [Patch v3 0/3] NetworkPkg/IScsiDxe: Display InitiatorInfo correctly.
> NetworkPkg/IScsiDxe: Fix the incorrect/needless DHCP process.
> NetworkPkg/IScsiDxe: Clean the previous ConfigData when 
> switching the IP mode. /// This one is to fix the issue A.
> NetworkPkg/IScsiDxe: Display InitiatorInfo in attempt page 
> even DHCP enabled.
> 
> [Patch v2 0/2] Add IPv6 support condition check.
>  NetworkPkg/HttpBootDxe: Add IPv6 support condition check. 
> /// B && C have been fixed in version 2.
>  NetworkPkg/IScsiDxe: Add IPv6 support condition check.
> 
> Please help to verify them.
> 
> Best Regards,
> Jiaxin
> 
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf 
> > Of Karunakar P
> > Sent: Tuesday, October 17, 2017 6:13 PM
> > To: Wu, Jiaxin ; edk2-devel@lists.01.org
> > Cc: Ye, Ting ; Fu, Siyuan 
> > Subject: Re: [edk2] [Patch 0/2] Add IPv6 support condition check for 
> > HTTP/ISCSI.
> >
> > Hi Jiaxin,
> >
> > I Reviewed the changes for 3 features/Bugs and verified the same, 
> > Please find my below comments and issues faced
> >
> > A. Display InitiatorInfo in attempt page even DHCP enabled
> > 
> > ---
> ---
> > 
> > 1. I applied IScsiConfigVfr.vfr  changes and as well IScsiMisc.c 
> > changes 2. It displays initiator info properly when it's Enabled for 
> > DHCP 3. But, I found some different behavior in below case
> > a. Add

Re: [edk2] Adding VLAN changing Boot order to default

2017-10-23 Thread Karunakar P 
Hello All,

Boot order is changing to default if we add VLAN, below are the steps followed.

[Steps]

1.   Change default boot order to some other, Then Commit changes and Exit. 
(In my case first boot option Is UEFI Internal Shell, then I changed PXEv4 as 
first boot option)

2.   Add a VLAN

Network Device List -> MAC -> VLAN Configuration -> Create new VLAN and Add VLAN

3.   Now check the Boot order

Observation:- The boot order was changed to default(In my case, UEFI Internal 
shell becomes first boot option)

Could you please provide your comments on this?

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

Re: [edk2] [PATCH] ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI core

2017-10-23 Thread Ard Biesheuvel 
On 23 October 2017 at 15:18, Gao, Liming  wrote:
> This is for debug purpose. I think UINT32 is OK if AARCH64 can operate 32bit 
> value.
>

Both my ARM and AARCH64 implementations write 16 bytes at a time, and
so whether the source value is UINT32 or UINT64 or UINT16 does not
make any difference at all.

>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Saturday, October 21, 2017 1:19 AM
>> To: Ard Biesheuvel 
>> Cc: Gao, Liming ; edk2-devel@lists.01.org; Leif 
>> Lindholm 
>> Subject: Re: [edk2] [PATCH] ArmPlatformPkg/PrePeiCore: seed temporary stack 
>> before entering PEI core
>>
>> On 10/20/17 18:52, Ard Biesheuvel wrote:
>> > On 20 October 2017 at 17:51, Laszlo Ersek  wrote:
>> >> On 10/20/17 18:39, Ard Biesheuvel wrote:
>> >>> On 20 October 2017 at 17:37, Gao, Liming  wrote:
>>  Ard:
>>    This case is to share the same value between PeiCore and SecCore. I 
>>  also think it will be better to define one fixed PCD in
>> MdeModulePkg.dec for this value. Could you submit bugzillar to catch this 
>> issue first?
>> 
>> >>>
>> >>> Certainly!
>> >>
>> >> Would it be possible to define the PCD as UINT32, and task 64-bit SEC
>> >> (and PEI_CORE) code to first construct the wider value manually (in a
>> >> register or otherwise)?
>> >>
>> >> Just thinking out loud.
>> >>
>> >
>> > Could you think the reasoning behind that out loud as well?
>>
>> Haha, good stab :) Sure.
>>
>> In your patch you have:
>>
>> +#define INIT_CAR_VALUE  0x5AA55AA55AA55AA5
>>
>> for 64-bit, and
>>
>> +#define INIT_CAR_VALUE  0x5AA55AA5
>>
>> for 32-bit.
>>
>> Both 64-bit assembly code in SEC, and 64-bit C-code in the PEI_CORE, can
>> easily compose the large value from the small value, starting from
>> FixedPcdGet32(). The alternatives are:
>>
>> - asking the 32-bit assembly code to truncate the 64-bit constant -- it
>> won't compile,
>>
>> - defining *two* FixedAtBuild PCDs, one for 32-bit, another for 64-bit
>> SEC -- an idea probably not universally liked.
>>
>> ... When I originally started writing my previous email, I even thought
>> about introducing the PCD as UINT16 :) But then I realized, if any
>> platform lacks *some* 32-bit mode when it sets up the stack in assembly,
>> for C-language entry, then the platform won't be supported by edk2.
>>
>> Thanks
>> Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI core

2017-10-23 Thread Gao, Liming 
This is for debug purpose. I think UINT32 is OK if AARCH64 can operate 32bit 
value. 

Thanks
Liming
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Saturday, October 21, 2017 1:19 AM
> To: Ard Biesheuvel 
> Cc: Gao, Liming ; edk2-devel@lists.01.org; Leif 
> Lindholm 
> Subject: Re: [edk2] [PATCH] ArmPlatformPkg/PrePeiCore: seed temporary stack 
> before entering PEI core
> 
> On 10/20/17 18:52, Ard Biesheuvel wrote:
> > On 20 October 2017 at 17:51, Laszlo Ersek  wrote:
> >> On 10/20/17 18:39, Ard Biesheuvel wrote:
> >>> On 20 October 2017 at 17:37, Gao, Liming  wrote:
>  Ard:
>    This case is to share the same value between PeiCore and SecCore. I 
>  also think it will be better to define one fixed PCD in
> MdeModulePkg.dec for this value. Could you submit bugzillar to catch this 
> issue first?
> 
> >>>
> >>> Certainly!
> >>
> >> Would it be possible to define the PCD as UINT32, and task 64-bit SEC
> >> (and PEI_CORE) code to first construct the wider value manually (in a
> >> register or otherwise)?
> >>
> >> Just thinking out loud.
> >>
> >
> > Could you think the reasoning behind that out loud as well?
> 
> Haha, good stab :) Sure.
> 
> In your patch you have:
> 
> +#define INIT_CAR_VALUE  0x5AA55AA55AA55AA5
> 
> for 64-bit, and
> 
> +#define INIT_CAR_VALUE  0x5AA55AA5
> 
> for 32-bit.
> 
> Both 64-bit assembly code in SEC, and 64-bit C-code in the PEI_CORE, can
> easily compose the large value from the small value, starting from
> FixedPcdGet32(). The alternatives are:
> 
> - asking the 32-bit assembly code to truncate the 64-bit constant -- it
> won't compile,
> 
> - defining *two* FixedAtBuild PCDs, one for 32-bit, another for 64-bit
> SEC -- an idea probably not universally liked.
> 
> ... When I originally started writing my previous email, I even thought
> about introducing the PCD as UINT16 :) But then I realized, if any
> platform lacks *some* 32-bit mode when it sets up the stack in assembly,
> for C-language entry, then the platform won't be supported by edk2.
> 
> Thanks
> Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v3] EmbeddedPkg: add driver to set graphical/serial console preference

2017-10-23 Thread Ard Biesheuvel 
On 23 October 2017 at 11:26, Leif Lindholm  wrote:
> On Fri, Oct 20, 2017 at 10:24:04PM +0100, Ard Biesheuvel wrote:
>> Linux on ARM/arm64 will infer from the presence of a /chosen/stdout-path
>> DT property or of a SPCR ACPI table that the primary console is the serial
>> port, even if a graphical console is available as well.
>>
>> So let's introduce a driver that allows the user to set a preference
>> between graphical and serial if both are available. If the preference
>> is set to 'Graphical', and any GOP protocol instances have been installed
>> by the time the ReadyToBoot event is signalled, remove the DT property
>> and/or the SPCR table entirely.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel 
>
> I don't have any comments beyond what I said for v1 - so many
> thanks for doing this rework!
>
> Reviewed-by: Leif Lindholm 
>

Pushed as d8e36289cef7bde628b023219cd65fa8e8d4562a

Thanks

>> ---
>> v3: - as suggested by Laszlo, move all logic into the OnReadyToBoot event
>>   callback so there is no need to register protocol notification handlers
>>   to look for the SDT protocol
>> - fixed overly long lines in license blocks
>>
>> v2: - use protocol register notify for gEfiAcpiSdtProtocolGuid, this is
>>   necessary because we may be loaded before the driver that produces it,
>>   and we cannot Depex on it either (because it is optional)
>> - add missing changes to .dec and Include/ files
>>
>>  EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.c   | 293 
>> 
>>  EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.h   |  31 +++
>>  EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.inf |  62 +
>>  EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefHii.uni |  27 ++
>>  EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefHii.vfr |  51 
>>  EmbeddedPkg/EmbeddedPkg.dec   |   3 +
>>  EmbeddedPkg/Include/Guid/ConsolePrefFormSet.h |  23 ++
>>  7 files changed, 490 insertions(+)
>>
>> diff --git a/EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.c 
>> b/EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.c
>> new file mode 100644
>> index ..996d1a6036b9
>> --- /dev/null
>> +++ b/EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.c
>> @@ -0,0 +1,293 @@
>> +/** @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 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include 
>> +#include 
>> +
>> +#include "ConsolePrefDxe.h"
>> +
>> +#define SPCR_SIG
>> EFI_ACPI_2_0_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE
>> +
>> +extern  UINT8 ConsolePrefHiiBin[];
>> +extern  UINT8 ConsolePrefDxeStrings[];
>> +
>> +typedef struct {
>> +  VENDOR_DEVICE_PATH  VendorDevicePath;
>> +  EFI_DEVICE_PATH_PROTOCOLEnd;
>> +} HII_VENDOR_DEVICE_PATH;
>> +
>> +STATIC HII_VENDOR_DEVICE_PATH mConsolePrefDxeVendorDevicePath = {
>> +  {
>> +{
>> +  HARDWARE_DEVICE_PATH,
>> +  HW_VENDOR_DP,
>> +  {
>> +(UINT8) (sizeof (VENDOR_DEVICE_PATH)),
>> +(UINT8) ((sizeof (VENDOR_DEVICE_PATH)) >> 8)
>> +  }
>> +},
>> +CONSOLE_PREF_FORMSET_GUID
>> +  },
>> +  {
>> +END_DEVICE_PATH_TYPE,
>> +END_ENTIRE_DEVICE_PATH_SUBTYPE,
>> +{
>> +  (UINT8) (END_DEVICE_PATH_LENGTH),
>> +  (UINT8) ((END_DEVICE_PATH_LENGTH) >> 8)
>> +}
>> +  }
>> +};
>> +
>> +STATIC EFI_EVENT  mReadyToBootEvent;
>> +
>> +STATIC
>> +EFI_STATUS
>> +InstallHiiPages (
>> +  VOID
>> +  )
>> +{
>> +  EFI_STATUS  Status;
>> +  EFI_HII_HANDLE  HiiHandle;
>> +  EFI_HANDLE  DriverHandle;
>> +
>> +  DriverHandle = NULL;
>> +  Status = gBS->InstallMultipleProtocolInterfaces (,
>> +  ,
>> +  ,
>> +  NULL);
>> +  if (EFI_ERROR (Status)) {
>> +return Status;
>> +  }
>> +
>> +  HiiHandle = HiiAddPackages (,
>> +  DriverHandle,
>> +  ConsolePrefDxeStrings,
>> +  ConsolePrefHiiBin,
>> +  NULL);
>> +
>> +  if (HiiHandle == NULL) {
>> +gBS->UninstallMultipleProtocolInterfaces (DriverHandle,
>> +   ,
>> +

Re: [edk2] [PATCH v3] EmbeddedPkg: add driver to set graphical/serial console preference

2017-10-23 Thread Leif Lindholm 
On Fri, Oct 20, 2017 at 10:24:04PM +0100, Ard Biesheuvel wrote:
> Linux on ARM/arm64 will infer from the presence of a /chosen/stdout-path
> DT property or of a SPCR ACPI table that the primary console is the serial
> port, even if a graphical console is available as well.
> 
> So let's introduce a driver that allows the user to set a preference
> between graphical and serial if both are available. If the preference
> is set to 'Graphical', and any GOP protocol instances have been installed
> by the time the ReadyToBoot event is signalled, remove the DT property
> and/or the SPCR table entirely.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 

I don't have any comments beyond what I said for v1 - so many
thanks for doing this rework!

Reviewed-by: Leif Lindholm 

> ---
> v3: - as suggested by Laszlo, move all logic into the OnReadyToBoot event
>   callback so there is no need to register protocol notification handlers
>   to look for the SDT protocol
> - fixed overly long lines in license blocks
> 
> v2: - use protocol register notify for gEfiAcpiSdtProtocolGuid, this is
>   necessary because we may be loaded before the driver that produces it,
>   and we cannot Depex on it either (because it is optional)
> - add missing changes to .dec and Include/ files
> 
>  EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.c   | 293 
> 
>  EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.h   |  31 +++
>  EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.inf |  62 +
>  EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefHii.uni |  27 ++
>  EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefHii.vfr |  51 
>  EmbeddedPkg/EmbeddedPkg.dec   |   3 +
>  EmbeddedPkg/Include/Guid/ConsolePrefFormSet.h |  23 ++
>  7 files changed, 490 insertions(+)
> 
> diff --git a/EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.c 
> b/EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.c
> new file mode 100644
> index ..996d1a6036b9
> --- /dev/null
> +++ b/EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.c
> @@ -0,0 +1,293 @@
> +/** @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 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +
> +#include "ConsolePrefDxe.h"
> +
> +#define SPCR_SIG
> EFI_ACPI_2_0_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE
> +
> +extern  UINT8 ConsolePrefHiiBin[];
> +extern  UINT8 ConsolePrefDxeStrings[];
> +
> +typedef struct {
> +  VENDOR_DEVICE_PATH  VendorDevicePath;
> +  EFI_DEVICE_PATH_PROTOCOLEnd;
> +} HII_VENDOR_DEVICE_PATH;
> +
> +STATIC HII_VENDOR_DEVICE_PATH mConsolePrefDxeVendorDevicePath = {
> +  {
> +{
> +  HARDWARE_DEVICE_PATH,
> +  HW_VENDOR_DP,
> +  {
> +(UINT8) (sizeof (VENDOR_DEVICE_PATH)),
> +(UINT8) ((sizeof (VENDOR_DEVICE_PATH)) >> 8)
> +  }
> +},
> +CONSOLE_PREF_FORMSET_GUID
> +  },
> +  {
> +END_DEVICE_PATH_TYPE,
> +END_ENTIRE_DEVICE_PATH_SUBTYPE,
> +{
> +  (UINT8) (END_DEVICE_PATH_LENGTH),
> +  (UINT8) ((END_DEVICE_PATH_LENGTH) >> 8)
> +}
> +  }
> +};
> +
> +STATIC EFI_EVENT  mReadyToBootEvent;
> +
> +STATIC
> +EFI_STATUS
> +InstallHiiPages (
> +  VOID
> +  )
> +{
> +  EFI_STATUS  Status;
> +  EFI_HII_HANDLE  HiiHandle;
> +  EFI_HANDLE  DriverHandle;
> +
> +  DriverHandle = NULL;
> +  Status = gBS->InstallMultipleProtocolInterfaces (,
> +  ,
> +  ,
> +  NULL);
> +  if (EFI_ERROR (Status)) {
> +return Status;
> +  }
> +
> +  HiiHandle = HiiAddPackages (,
> +  DriverHandle,
> +  ConsolePrefDxeStrings,
> +  ConsolePrefHiiBin,
> +  NULL);
> +
> +  if (HiiHandle == NULL) {
> +gBS->UninstallMultipleProtocolInterfaces (DriverHandle,
> +   ,
> +   ,
> +   NULL);
> +return EFI_OUT_OF_RESOURCES;
> +  }
> +  return EFI_SUCCESS;
> +}
> +
> +STATIC
> +VOID
> +RemoveDtStdoutPath (
> +  VOID
> +)
> +{
> +  VOID*Dtb;
> +  INT32   Node;
> +  INT32   Error;
> +  EFI_STATUS  Status;
> +
> +  Status

[edk2] [Patch][edk2-platforms/devel-MinnowBoard3-UDK2017] GCC FCE

2017-10-23 Thread Guo, Mang 
Changed tool path.

1. Add FCE for GCC build
2. Change build script to make sure that system can still boot after Setup 
variable deletion

Contributed-under: TianoCore Contribution Agreement 1.1

Signed-off-by: Guo Mang 
---
 Platform/BroxtonPlatformPkg/BuildBios.sh | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/Platform/BroxtonPlatformPkg/BuildBios.sh 
b/Platform/BroxtonPlatformPkg/BuildBios.sh
index 5d9a023..b6127e2 100644
--- a/Platform/BroxtonPlatformPkg/BuildBios.sh
+++ b/Platform/BroxtonPlatformPkg/BuildBios.sh
@@ -262,14 +262,18 @@ cp -f 
$WORKSPACE/Silicon/BroxtonSoC/BroxtonFspPkg/ApolloLakeFspBinPkg/FspBin/FSP
 cp -f 
$WORKSPACE/Silicon/BroxtonSoC/BroxtonFspPkg/ApolloLakeFspBinPkg/FspBin/FSP_M.Fv 
$WORKSPACE/Platform/BroxtonPlatformPkg/Common/Tools/Stitch
 cp -f 
$WORKSPACE/Silicon/BroxtonSoC/BroxtonFspPkg/ApolloLakeFspBinPkg/FspBin/FSP_S.Fv 
$WORKSPACE/Platform/BroxtonPlatformPkg/Common/Tools/Stitch
 
-#echo "Running fce..."
-## Extract Hii data from build and store in HiiDefaultData.txt
-#wine PlatformTools/FCE/FCE.exe read -i $BUILD_PATH/FV/SOC.fd > 
$BUILD_PATH/FV/HiiDefaultData.txt 1>>EDK2.log 2>&1
+echo "Running fce..."
+cat $BUILD_PATH/FV/FVIBBM.Fv $BUILD_PATH/FV/SOC.fd > $BUILD_PATH/FV/Temp.fd
+# Extract Hii data from build and store a copy in HiiDefaultData.txt
+# UQI 0006 005C 0078 0030 0031 0030 0031 is for question 
prompt(STR_IPU_ENABLED)
+# First 0006 is the length of string; Next six byte values are mapped to 
STR_IPU_ENABLED string value defined in 
Platform/BroxtonPlatformPkg/Common/PlatformSettings/PlatformSetupDxe/VfrStrings.uni.
+./$PLATFORM_PACKAGE/Common/Tools/FCE/FCE read -i $BUILD_PATH/FV/Temp.fd 0006 
005C 0078 0030 0031 0030 0031 > $BUILD_PATH/FV/HiiDefaultData.txt
 
 ## copy the Setup variable to the SetupDefault variable and save changes to 
BxtXXX.fd
-#wine PlatformTools/FCE/FCE.exe mirror -i $BUILD_PATH/FV/SOC.fd -o 
$BUILD_PATH/FV/Bxt"$Arch".fd Setup SetupDefault 1>>EDK2.log 2>&1
+./$PLATFORM_PACKAGE/Common/Tools/FCE/FCE update -i $BUILD_PATH/FV/Temp.fd -s 
$BUILD_PATH/FV/HiiDefaultData.txt -o $BUILD_PATH/FV/Bxt"$Arch".fd -g 
B73FE497-B92E-416e-8326-45AD0D270091 -a 1>>EDK2.log 2>&1
 #echo "Skip FCE tool..."
-cp $BUILD_PATH/FV/SOC.fd $BUILD_PATH/FV/Bxt"$Arch".fd
+Split -f $BUILD_PATH/FV/Bxt"$Arch".fd -s 0x35000 -o $BUILD_PATH/FV/FVIBBM.Fv
+#cp $BUILD_PATH/FV/SOC.fd $BUILD_PATH/FV/Bxt"$Arch".fd
 
 ## Set the Board_Id, Build_Type, Version_Major, and Version_Minor environment 
variables
 ##find /v "#" Conf\BiosId.env > ver_strings
-- 
2.10.1.windows.1

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

[edk2] [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.

2017-10-23 Thread Eric Dong 
Current logic always waiting for a specific value to collect all APs
count. This logic may caused some platforms cost too much time to
wait for time out.
This patch add new logic to collect APs count. It adds new variable
NumApsExecuting to detect whether all APs have finished initialization.
Each AP let NumApsExecuting++ when begin to initialize itself and let
NumApsExecuting-- when it finish the initialization. BSP base on whether
NumApsExecuting == 0  to finished the collect AP process.

Cc: Ruiyu Ni 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong 
---
 UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc|  1 +
 UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm |  6 ++
 UefiCpuPkg/Library/MpInitLib/MpLib.c   | 20 ++--
 UefiCpuPkg/Library/MpInitLib/MpLib.h   |  1 +
 UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc |  3 ++-
 UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm  |  6 ++
 6 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc 
b/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
index 976af1f..bdfe0d3 100644
--- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
+++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
@@ -40,4 +40,5 @@ EnableExecuteDisableLocation  equLockLocation + 30h
 Cr3Location   equLockLocation + 34h
 InitFlagLocation  equLockLocation + 38h
 CpuInfoLocation   equLockLocation + 3Ch
+NumApsExecutingLocation   equLockLocation + 40h
 
diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm 
b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
index 1b9c6a6..2b6c27d 100644
--- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
@@ -86,6 +86,12 @@ Flat32Start:   ; protected 
mode entry point
 
 movesi, ebx
 
+; Increment the number of APs executing here as early as possible
+; This is decremented in C code when AP is finished executing
+movedi, esi
+addedi, NumApsExecutingLocation
+lock inc   dword [edi]
+
 mov edi, esi
 add edi, EnableExecuteDisableLocation
 cmp byte [edi], 0
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c 
b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index db923c9..48f930b 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -662,6 +662,7 @@ ApWakeupFunction (
 // AP finished executing C code
 //
 InterlockedIncrement ((UINT32 *) >FinishedCount);
+InterlockedDecrement ((UINT32 *) 
>MpCpuExchangeInfo->NumApsExecuting);
 
 //
 // Place AP is specified loop mode
@@ -765,6 +766,7 @@ FillExchangeInfoData (
 
   ExchangeInfo->CFunction   = (UINTN) ApWakeupFunction;
   ExchangeInfo->ApIndex = 0;
+  ExchangeInfo->NumApsExecuting = 0;
   ExchangeInfo->InitFlag= (UINTN) CpuMpData->InitFlag;
   ExchangeInfo->CpuInfo = (CPU_INFO_IN_HOB *) (UINTN) 
CpuMpData->CpuInfoInHob;
   ExchangeInfo->CpuMpData   = CpuMpData;
@@ -934,13 +936,19 @@ WakeUpAP (
 }
 if (CpuMpData->InitFlag == ApInitConfig) {
   //
-  // Wait for all potential APs waken up in one specified period
+  // Wait for one potential AP waken up in one specified period
   //
-  TimedWaitForApFinish (
-CpuMpData,
-PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1,
-PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds)
-);
+  if (CpuMpData->CpuCount == 0) {
+TimedWaitForApFinish (
+  CpuMpData,
+  PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1,
+  PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds)
+  );
+  }
+
+  while (CpuMpData->MpCpuExchangeInfo->NumApsExecuting != 0) {
+CpuPause();
+  }
 } else {
   //
   // Wait all APs waken up if this is not the 1st broadcast of SIPI
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h 
b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index e41d2db..d13d5c0 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -176,6 +176,7 @@ typedef struct {
   UINTN Cr3;
   UINTN InitFlag;
   CPU_INFO_IN_HOB   *CpuInfo;
+  UINTN NumApsExecuting;
   CPU_MP_DATA   *CpuMpData;
   UINTN InitializeFloatingPointUnitsAddress;
 } MP_CPU_EXCHANGE_INFO;
diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc 
b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
index 114f4e0..d255ca5 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
@@ -40,5 +40,6 @@ EnableExecuteDisableLocation  equLockLocation + 5Ch
 Cr3Location   equLockLocation + 64h
 InitFlagLocation  equLockLocation + 6Ch
 CpuInfoLocation

[edk2] [Patch 1/2] UefiCpuPkg/MpInitLib: Change AP Index variable name.

2017-10-23 Thread Eric Dong 
Original AP index variable name not well express the meaning
of the variable. Also this name is better used in later patch.
So change the variable name for better understanding.

Cc: Ruiyu Ni 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong 
---
 UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc| 2 +-
 UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 4 ++--
 UefiCpuPkg/Library/MpInitLib/MpLib.c   | 6 +++---
 UefiCpuPkg/Library/MpInitLib/MpLib.h   | 2 +-
 UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc | 2 +-
 UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm  | 4 ++--
 6 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc 
b/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
index 6276230..976af1f 100644
--- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
+++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
@@ -33,7 +33,7 @@ GdtrLocation  equLockLocation + 10h
 IdtrLocation  equLockLocation + 16h
 BufferStartLocation   equLockLocation + 1Ch
 ModeOffsetLocationequLockLocation + 20h
-NumApsExecutingLocation   equLockLocation + 24h
+ApIndexLocation   equLockLocation + 24h
 CodeSegmentLocation   equLockLocation + 28h
 DataSegmentLocation   equLockLocation + 2Ch
 EnableExecuteDisableLocation  equLockLocation + 30h
diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm 
b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
index 52363e6..1b9c6a6 100644
--- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
@@ -130,7 +130,7 @@ TestLock:
 jz TestLock
 
 movecx, esi
-addecx, NumApsExecutingLocation
+addecx, ApIndexLocation
 incdword [ecx]
 movebx, [ecx]
 
@@ -200,7 +200,7 @@ CProcedureInvoke:
 moveax, ASM_PFX(InitializeFloatingPointUnits)
 call   eax   ; Call assembly function to initialize FPU 
per UEFI spec
 
-push   ebx   ; Push NumApsExecuting
+push   ebx   ; Push ApIndex
 moveax, esi
 addeax, LockLocation
 push   eax   ; push address of exchange info data buffer
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c 
b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index f3ee6d4..db923c9 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -542,7 +542,7 @@ VOID
 EFIAPI
 ApWakeupFunction (
   IN MP_CPU_EXCHANGE_INFO  *ExchangeInfo,
-  IN UINTN NumApsExecuting
+  IN UINTN ApIndex
   )
 {
   CPU_MP_DATA*CpuMpData;
@@ -574,7 +574,7 @@ ApWakeupFunction (
   // Add CPU number
   //
   InterlockedIncrement ((UINT32 *) >CpuCount);
-  ProcessorNumber = NumApsExecuting;
+  ProcessorNumber = ApIndex;
   //
   // This is first time AP wakeup, get BIST information from AP stack
   //
@@ -764,7 +764,7 @@ FillExchangeInfoData (
   ExchangeInfo->Cr3 = AsmReadCr3 ();
 
   ExchangeInfo->CFunction   = (UINTN) ApWakeupFunction;
-  ExchangeInfo->NumApsExecuting = 0;
+  ExchangeInfo->ApIndex = 0;
   ExchangeInfo->InitFlag= (UINTN) CpuMpData->InitFlag;
   ExchangeInfo->CpuInfo = (CPU_INFO_IN_HOB *) (UINTN) 
CpuMpData->CpuInfoInHob;
   ExchangeInfo->CpuMpData   = CpuMpData;
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h 
b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 84ae24f..e41d2db 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -169,7 +169,7 @@ typedef struct {
   IA32_DESCRIPTOR   IdtrProfile;
   UINTN BufferStart;
   UINTN ModeOffset;
-  UINTN NumApsExecuting;
+  UINTN ApIndex;
   UINTN CodeSegment;
   UINTN DataSegment;
   UINTN EnableExecuteDisable;
diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc 
b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
index 5b2529b..114f4e0 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
@@ -33,7 +33,7 @@ GdtrLocation  equLockLocation + 20h
 IdtrLocation  equLockLocation + 2Ah
 BufferStartLocation   equLockLocation + 34h
 ModeOffsetLocationequLockLocation + 3Ch
-NumApsExecutingLocation   equLockLocation + 44h
+ApIndexLocation   equLockLocation + 44h
 CodeSegmentLocation   equLockLocation + 4Ch
 DataSegmentLocation   equLockLocation + 54h
 EnableExecuteDisableLocation  equLockLocation + 5Ch
diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm

[edk2] [Patch 0/2] Enhance collect AP Count logic

2017-10-23 Thread Eric Dong 
Current logic always waiting for a specific value to collect all APs
count. This logic may caused some platforms cost too much time to
wait for time out.

This patch series add new logic to collect APs count. It adds new variable
NumApsExecuting to detect whether all APs have finished initialization.
Each AP let NumApsExecuting++ when begin to initialize itself and let
NumApsExecuting-- when it finish the initialization. BSP base on whether
NumApsExecuting == 0 to finished the collect AP process. 

Because current code already use NumApsExecuting variable, so add another
patch to change the variable name for the current code for better 
understanding.


Eric Dong (2):
  UefiCpuPkg/MpInitLib: Change AP Index variable name.
  UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.

 UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc|  3 ++-
 UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 10 --
 UefiCpuPkg/Library/MpInitLib/MpLib.c   | 24 
 UefiCpuPkg/Library/MpInitLib/MpLib.h   |  3 ++-
 UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc |  5 +++--
 UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm  | 10 --
 6 files changed, 39 insertions(+), 16 deletions(-)

-- 
2.7.0.windows.1

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

Re: [edk2] [PATCH] MdeModulePkg/PciBus: Fix bug that PCI BUS claims too much resource

2017-10-23 Thread Dong, Eric 
Reviewed-by: Eric Dong 

> -Original Message-
> From: Ni, Ruiyu
> Sent: Friday, October 20, 2017 5:56 PM
> To: edk2-devel@lists.01.org
> Cc: Dong, Eric 
> Subject: [PATCH] MdeModulePkg/PciBus: Fix bug that PCI BUS claims too
> much resource
> 
> The bug was caused by 728d74973c9262b6c7b7ef4be213223d55affec3
> "MdeModulePkg/PciBus: Count multiple hotplug resource paddings".
> 
> The patch firstly updated the Bridge->Alignment to the maximum alignment
> of all devices under the bridge, then aligned the
> Bridge->Length to Bridge->Alignment.
> It caused too much resources were claimed.
> 
> The new patch firstly aligns Bridge->Length to Bridge->Alignment, then
> updates the Bridge->Alignment to the maximum alignment of all devices
> under the bridge.
> Because the step to update the Bridge->Alignment is to make sure the
> resource allocated to the bus under the Bridge meets all devices alignment.
> But the Bridge->Length doesn't have to align to the maximum alignment.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni 
> Cc: Eric Dong 
> ---
>  .../Bus/Pci/PciBusDxe/PciResourceSupport.c | 24 +++--
> -
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> index 8dbe9a0038..2f713fcee9 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> @@ -389,18 +389,7 @@ CalculateResourceAperture (
>}
> 
>//
> -  // Adjust the bridge's alignment to the MAX (first) alignment of all 
> children.
> -  //
> -  CurrentLink = Bridge->ChildList.ForwardLink;
> -  if (CurrentLink != >ChildList) {
> -Node = RESOURCE_NODE_FROM_LINK (CurrentLink);
> -if (Node->Alignment > Bridge->Alignment) {
> -  Bridge->Alignment = Node->Alignment;
> -}
> -  }
> -
> -  //
> -  // At last, adjust the aperture with the bridge's alignment
> +  // Adjust the aperture with the bridge's alignment
>//
>Aperture[PciResUsageTypical] = ALIGN_VALUE
> (Aperture[PciResUsageTypical], Bridge->Alignment + 1);
>Aperture[PciResUsagePadding] = ALIGN_VALUE
> (Aperture[PciResUsagePadding], Bridge->Alignment + 1); @@ -410,6 +399,17
> @@ CalculateResourceAperture (
>// Use the larger one between the padding resource and actual occupied
> resource.
>//
>Bridge->Length = MAX (Aperture[PciResUsageTypical],
> Aperture[PciResUsagePadding]);
> +
> +  //
> +  // Adjust the bridge's alignment to the MAX (first) alignment of all 
> children.
> +  //
> +  CurrentLink = Bridge->ChildList.ForwardLink;  if (CurrentLink !=
> + >ChildList) {
> +Node = RESOURCE_NODE_FROM_LINK (CurrentLink);
> +if (Node->Alignment > Bridge->Alignment) {
> +  Bridge->Alignment = Node->Alignment;
> +}
> +  }
>  }
> 
>  /**
> --
> 2.12.2.windows.2

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

Re: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Use SetMem instead of SetMem64 to fix hang

2017-10-23 Thread Dong, Eric 
Reviewed-by: Eric Dong 

> -Original Message-
> From: Ni, Ruiyu
> Sent: Monday, October 23, 2017 1:12 PM
> To: edk2-devel@lists.01.org
> Cc: Dong, Eric 
> Subject: [PATCH] UefiCpuPkg/MtrrLib: Use SetMem instead of SetMem64 to
> fix hang
> 
> ClearMasks and OrMasks are not 8-byte aligned.
> But SetMem64 requires the input address is 8-byte aligned.
> If the input is not 8-byte aligned, assertion is hit.
> Use SetMem instead.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni 
> Cc: Eric Dong 
> ---
>  UefiCpuPkg/Library/MtrrLib/MtrrLib.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> index 200becdd4a..8e46e46cd4 100644
> --- a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> +++ b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> @@ -2123,8 +2123,8 @@ MtrrLibSetBelow1MBMemoryAttribute (
>//
>// (Value & ~0 | 0) still equals to (Value)
>//
> -  SetMem64 (ClearMasks, sizeof (ClearMasks), 0);
> -  SetMem64 (OrMasks, sizeof (OrMasks), 0);
> +  SetMem (ClearMasks, sizeof (ClearMasks), 0);  SetMem (OrMasks, sizeof
> + (OrMasks), 0);
> 
>MsrIndex = (UINT32)-1;
>while ((BaseAddress < BASE_1MB) && (Length != 0)) {
> --
> 2.12.2.windows.2

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

Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

2017-10-23 Thread Udit Kumar 
Hi Meenakshi/Liming, 
My 2 cents, around this. 

1)
Having a new lib for BE read might not be helpful for us, 
e.g. a IP which is in BE mode access the UART for print or system registers 
which are in LE, 
then with new Lib, we will get all read/write in BE mode 

2)
Especially for our IPs, which are changing from BE to LE depending on platform. 
As said before, having BE read lib with API name of MmioRead32 etc, will not 
help (I guess Meenakshi already seen some problems around this)
Adding a new lib with MmioRead32BE API name could help, but IP driver we need 
to take care of IP mode either by Pcd or #define, to select MmioRead32 or 
MmioRead32BE. 
This conditional compile needs to be done for all IPs (which works in BE/LE 
mode on different platforms). 

My preferred way of implementation to use one function in IP driver, 
And based on IP mode, do the switch. 

New Lib could have function like below 
MmioRead32Generic(IN  UINTN Address, BOOL IsIPBE) {
   UINT32Value;
 
   ASSERT ((Address & 3) == 0);
   Value = *(volatile UINT32*)Address;
   If(IsIPBE)
 Value = SwapBytes32(Value);
 return  Value;
}

And IP driver can use it 
MmioRead32Generic(ADDR, FixedPcdGet(This_IP_Mode_For_This_platform)

Comments are welcome. 

Regards
Udit

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Gao,
> Liming
> Sent: Monday, October 16, 2017 8:48 AM
> To: Meenakshi Aggarwal ; Ard Biesheuvel
> ; Kinney, Michael D ;
> edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian
> MMIO
> 
> Meenakshi:
>   I suggest to introduce new IoLib library instance, not to add new IoLib 
> APIs.
> New IoLib library instance will perform IO operation as the big endian. You 
> can
> update MdePkg/Library/BaseIoLibIntrinsic instance, add new source file and
> new INF for it.
> 
> UINT32
> EFIAPI
> MmioRead32 (
>   IN  UINTN Address
>   )
> {
>   UINT32Value;
> 
>   ASSERT ((Address & 3) == 0);
>   Value = *(volatile UINT32*)Address;
>   return SwapBytes32(Value);
> }
> 
> Thanks
> Liming
> >-Original Message-
> >From: Meenakshi Aggarwal [mailto:meenakshi.aggar...@nxp.com]
> >Sent: Friday, October 13, 2017 2:07 PM
> >To: Ard Biesheuvel ; Kinney, Michael D
> >; edk2-devel@lists.01.org; Gao, Liming
> >
> >Subject: RE: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for
> >big-endian MMIO
> >
> >Hi All,
> >
> >
> >It’s a pretty old discussion, we have left the upstreaming of NXP
> >package in between because of some other work, but have started it again
> now.
> >
> >
> >Issue  : Few NXP modules support Big Endian MMIOs as these are ported
> >from PowerPC.
> >
> >Solution suggested : Create a separate library for BE MMIO APIs.
> >
> >
> >So what I have done is, I have created a separate library to support BE
> >MMIO APIs and currently keeping it to my package.
> >This library is basically a wrapper over existing MMIO APIs.
> >
> >UINT32
> >EFIAPI
> >BeMmioRead32 (
> >  IN  UINTN Address
> >  )
> >{
> >  UINT32  Value;
> >
> >  Value = MmioRead32(Address);
> >
> >  return SwapBytes32(Value);
> >}
> >
> >
> >Need your opinion on below optinos:
> >
> >1. Will this be a good idea to make this library a part of MdePkg? OR
> >
> >2. Add a new file e.g. IoBeMmio.c like IoHighLevel.c in
> >MdePkg/Library/BaseIoLibIntrinsic/
> > And made these APIs a part of IoLib itself. OR
> >
> >3. Keep this library internal to NXP package.
> >
> >
> >Please provide your inputs.
> >
> >
> >Thanks & Regards,
> >Meenakshi
> >
> >> -Original Message-
> >> From: Bhupesh Sharma
> >> Sent: Monday, October 17, 2016 3:28 PM
> >> To: Ard Biesheuvel ; Kinney, Michael D
> >> 
> >> Cc: Gao, Liming ; edk2-de...@ml01.01.org;
> >> Meenakshi Aggarwal 
> >> Subject: RE: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for
> >> big-endian MMIO
> >>
> >> Hi Ard,
> >>
> >> > -Original Message-
> >> > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> >> > Sent: Monday, October 17, 2016 1:12 PM
> >> > To: Kinney, Michael D 
> >> > Cc: Gao, Liming ; Bhupesh Sharma
> >> > ; edk2-de...@ml01.01.org
> >> > Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-
> >> > endian MMIO
> >> >
> >> > On 17 October 2016 at 05:10, Kinney, Michael D
> >> >  wrote:
> >> > > Bhupesh,
> >> > >
> >> > > It is also possible to add an ARM specific PCD to select
> >> > > endianness and update
> >> > > MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c to use that PCD in
> >> > > MmioRead/Write() APIs in that file to support both endian types.
> >> > > You

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

2017-10-23 Thread Jian J Wang 
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.

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

diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index d312eb66f8..0802464b9d 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -829,6 +829,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);
+
 while (MemorySpaceLength > 0) {
   if (PageLength == 0) {
 PageEntry = GetPageTableEntry (, BaseAddress, 
);
@@ -846,7 +855,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 +862,8 @@ RefreshGcdMemoryAttributesFromPaging (
 
   Length = MIN (PageLength, MemorySpaceLength);
   if (DoUpdate) {
-gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
-gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
+Status = gDS->SetMemorySpaceAttributes (BaseAddress, Length, 
Attributes);
+ASSERT_EFI_ERROR (Status);
 DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx - 
%016lx (%08lx -> %08lx)\r\n",
  Index, BaseAddress, BaseAddress + Length - 1,
  MemorySpaceMap[Index].Attributes, Attributes));
-- 
2.14.1.windows.1

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