Re: [edk2] [PATCH 0/2] Add Erase Block Protocol support for UFS devices

[Tian, Feng]

Looks good to me

Reviewed-by: Feng Tian 

Thanks
Feng

-Original Message-
From: Wu, Hao A 
Sent: Monday, May 9, 2016 11:39 AM
To: edk2-devel@lists.01.org; Tian, Feng 
Cc: Wu, Hao A 
Subject: [PATCH 0/2] Add Erase Block Protocol support for UFS devices

This patch series add the Erase Block Protocol implementation for UFS devices.

Since the UFS transport layer follows the SCSI architecture, therefore, the 
implementation is added in the ScsiDiskDxe driver.

Hao Wu (2):
  MdePkg IndustryStandard/Scsi.h: Add Unmap command support
  MdeModulePkg ScsiDiskDxe: Add Erase Block Protocol support for UFS
devices

 MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c  | 700 +-
 MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.h  |  85 ++-
 MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf |   3 +-
 MdePkg/Include/IndustryStandard/Scsi.h|  64 +-
 4 files changed, 821 insertions(+), 31 deletions(-)

--
1.9.5.msysgit.0

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

Re: [edk2] [BaseTools] RuleOverride = BINARY code location?

[Andrew Fish]

> On May 19, 2016, at 8:18 PM, Gao, Liming  wrote:
> 
> Thanks Andrew. I will continue to investigate it. 
> 

>>> F = 'FileNoExtention'
>>> os.path.splitext(F)[1]
''
>>> '' in '.out'
True

So I think the bug is the `in` it should be `==`
>>> '' == '.out'
False

Thanks,

Andrew Fish


>> -Original Message-
>> From: af...@apple.com [mailto:af...@apple.com]
>> Sent: Friday, May 20, 2016 10:01 AM
>> To: Gao, Liming 
>> Cc: edk2-devel 
>> Subject: Re: [edk2] [BaseTools] RuleOverride = BINARY code location?
>> 
>> 
>>> On May 18, 2016, at 5:44 PM, Andrew Fish  wrote:
>>> 
 
 On May 18, 2016, at 5:43 PM, Gao, Liming  wrote:
 
 Andrew:
 |.bin will search the file with the postfix .bin in INF file directory and 
 its
>> output directory. So, there may be more than .bin files are found. Please see
>> the code login in GetFileList() from
>> C:\R9Tip\edk2\BaseTools\Source\Python\GenFds\Section.py.
 
>>> 
>> 
>> Thanks for the pointer.
>> 
>> This is the code that adding the OS executables in GetFileList() that is 
>> adding
>> the OS executables without the .bin extension.
>>if os.path.exists(Makefile):
>># Update to search files with suffix in all sub-dirs.
>>Tuple = os.walk(FfsInf.EfiOutputPath)
>>for Dirpath, Dirnames, Filenames in Tuple:
>>for F in Filenames:
>>if os.path.splitext(F)[1] in (Suffix):
>>FullName = os.path.join(Dirpath, F)
>>if os.path.getmtime(FullName) > 
>> os.path.getmtime(Makefile):
>>FileList.append(FullName)
>> 
>> 
>> I think the issue is:
>>   if os.path.splitext(F)[1] in (Suffix):
>> 
>> If the file does not have an extension it matches.
>> 
>> Sorry might be wrong have to go out to a writers reading with my wife so
>> have to stop looking right now.
>> 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> 
>> Thanks,
>> 
>> Andrew Fish
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[edk2] [PATCH] Vlv2TbltDevicePkg: Fixed an Array overflow issue. Remove some useless debug message.

[Guo, Mang]

---
 Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbService.c | 14 +++---
 .../Library/DxeEsrtCapsuleBsLib/DxeCapsuleLib.c  | 20 
 2 files changed, 7 insertions(+), 27 deletions(-)

diff --git a/Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbService.c 
b/Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbService.c
index 723728a..f86db18 100644
--- a/Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbService.c
+++ b/Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbService.c
@@ -1017,11 +1017,11 @@ FvbInitialize (
   UINTN Idx;
   UINT32MaxLbaSize;
   BOOLEAN   FvHeaderValid;
-EFI_BOOT_MODE BootMode;
-UINT32PlatformFvBaseAddress[2];
-UINT32PlatformFvBaseAddressCount;
-UINT32PlatformFvLockList[3];
-UINT32PlatformFvLockListCount;
+  EFI_BOOT_MODE BootMode;
+  UINT32PlatformFvBaseAddress[3];
+  UINT32PlatformFvBaseAddressCount;
+  UINT32PlatformFvLockList[2];
+  UINT32PlatformFvLockListCount;
   //
   // This platform driver knows there are 3 FVs on
   // FD, which are FvRecovery, FvMain and FvNvStorage.
@@ -1034,7 +1034,7 @@ UINT32
PlatformFvLockListCount;
 PlatformFvBaseAddressCount = 1;
 PlatformFvBaseAddress[0]   = PcdGet32 (PcdFlashNvStorageVariableBase);
   } else {
-PlatformFvBaseAddressCount = 2;
+PlatformFvBaseAddressCount = 3;
 PlatformFvBaseAddress[0]   = PcdGet32 (PcdFlashFvMainBase);
 PlatformFvBaseAddress[1]   = PcdGet32 (PcdFlashNvStorageVariableBase);
 PlatformFvBaseAddress[2]   = PcdGet32 (PcdFlashFvRecoveryBase);
@@ -1043,7 +1043,7 @@ UINT32
PlatformFvLockListCount;
   //
   // List of FVs that should be write protected on normal boots.
   //
-  PlatformFvLockListCount = 1;
+  PlatformFvLockListCount = 2;
   PlatformFvLockList[0]   = PcdGet32 (PcdFlashFvMainBase);
   PlatformFvLockList[1]   = PcdGet32 (PcdFlashFvRecoveryBase);
 
diff --git a/Vlv2TbltDevicePkg/Library/DxeEsrtCapsuleBsLib/DxeCapsuleLib.c 
b/Vlv2TbltDevicePkg/Library/DxeEsrtCapsuleBsLib/DxeCapsuleLib.c
index 4b1acb4..bb77abc 100644
--- a/Vlv2TbltDevicePkg/Library/DxeEsrtCapsuleBsLib/DxeCapsuleLib.c
+++ b/Vlv2TbltDevicePkg/Library/DxeEsrtCapsuleBsLib/DxeCapsuleLib.c
@@ -787,12 +787,10 @@ ProcessCapsuleImage (
   ProcessedFvImage = NULL;
   Status   = EFI_SUCCESS;
   AttemptStatus= LAST_ATTEMPT_STATUS_SUCCESS;
-  DEBUG ((DEBUG_INFO, "ProcessCapsuleImage1\n"));
 
   if (SupportCapsuleImage (CapsuleHeader) != EFI_SUCCESS) {
 return EFI_UNSUPPORTED;
   }
-  DEBUG ((DEBUG_INFO, "ProcessCapsuleImage2\n"));
 
   //
   // Display image in firmware update display capsule
@@ -802,7 +800,6 @@ ProcessCapsuleImage (
  (DISPLAY_DISPLAY_PAYLOAD *)(CapsuleHeader + 1), 
  (UINTN)(CapsuleHeader->CapsuleImageSize - 
sizeof(EFI_CAPSULE_HEADER)));
   }
-  DEBUG ((DEBUG_INFO, "ProcessCapsuleImage3\n"));
 
   //
   // Check FMP capsule layout
@@ -812,23 +809,18 @@ ProcessCapsuleImage (
 if (EFI_ERROR(Status)) {
   return Status;
 }
-   DEBUG ((DEBUG_INFO, "ProcessCapsuleImage4\n"));
 
 //
 // Press EFI FMP Capsule
 //
 Status = ProcessFmpCapsuleImage(CapsuleHeader);
-   DEBUG ((DEBUG_INFO, "ProcessCapsuleImage5\n"));
 
 //
 // Indicate to sync Esrt on next boot
 //
 PcdSetBool(PcdEsrtSyncFmp, TRUE);
-   DEBUG ((DEBUG_INFO, "ProcessCapsuleImage6\n"));
-
 return Status;
   }
-  DEBUG ((DEBUG_INFO, "ProcessCapsuleImage7\n"));
 
   //
   // Other non-FMP capsule handler
@@ -843,7 +835,6 @@ ProcessCapsuleImage (
 // Point to the next firmware volume header, and then
 // call the DXE service to process it.
 //
- DEBUG ((DEBUG_INFO, "ProcessCapsuleImage8\n"));
 if (FvImage->FvLength > (UINTN) Length) {
   //
   // Notes: need to stuff this status somewhere so that the
@@ -852,7 +843,6 @@ ProcessCapsuleImage (
   Status = EFI_VOLUME_CORRUPTED;
   break;
 }
-   DEBUG ((DEBUG_INFO, "ProcessCapsuleImage9\n"));
 
 FvAlignment = 1 << ((FvImage->Attributes & EFI_FVB2_ALIGNMENT) >> 16);
 //
@@ -861,7 +851,6 @@ ProcessCapsuleImage (
 if (FvAlignment < 8) {
   FvAlignment = 8;
 }
-DEBUG ((DEBUG_INFO, "ProcessCapsuleImage10\n"));
 //
 // Check FvImage Align is required.
 //
@@ -879,7 +868,6 @@ ProcessCapsuleImage (
   }
   CopyMem (ProcessedFvImage, FvImage, (UINTN) FvImage->FvLength);
 }
-DEBUG ((DEBUG_INFO, "ProcessCapsuleImage11\n"));
 
 Status = gDS->ProcessFirmwareVolume (
 (VOID *) ProcessedFvImage,
@@ -890,13 +878,11 @@ ProcessCapsuleImage (
   AttemptStatus = 

Re: [edk2] UEFI HTTP Boot Device Path and DNS

[Michael Chang]

Gently ping ..

Could anyone help to provide insights for this? In short the URI is absoultely
legit to have host name in it but current device path couldn't provide any dns
server to help in resolving it. How to deal with this problem ?

Thanks,
Michael

On Tue, Apr 19, 2016 at 06:18:35PM +0800, Michael Chang wrote:
> Background (And also Question):
> The PXE Base Code Protocol is no longer produced if selecting to boot from 
> UEFI
> HTTP device from boot menu. Does anyone here know why or is it's just a 
> problem
> of my environment (I'm using OVMF from openSUSE) ? 
> 
> That made the work to support it in grub2 more complicated than it should be,
> becuase it has to extract DHCP information from dhcp packets cached in
> EFI_PXE_BASE_CODE_MODE structure to setup it's own network interface. That's
> how it works before for TFTP, and at that time booted TFTP device path seems 
> to
> provide meaningless values.
> 
> Question:
> Now that we have to get it work by taking another approach to obtain the DHCP
> information from the booted (HTTP) URI device path if the PXE BC protocol
> cannot be located. But then we also bumped into another issue that DNS
> infomation is missing from within the device path nodes like
> (MAC(..)/IPv4(...)/URI(..)) so that server name in URI cannot work but can 
> only
> use IP address (v4 or v6). Is this a spec issue or intended to leave behind 
> DNS
> info ? 
> 
> Could anyone please help to provide more insights ?
> 
> Thanks. :)
> Michael
> ___
> 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] NetworkPkg/HttpDxe: Don't free Wrap in HttpTcpReceiveNotifyDpc

[Fu, Siyuan]

Reviewed-by: Fu Siyuan 


> -Original Message-
> From: Gary Lin [mailto:g...@suse.com]
> Sent: Friday, May 20, 2016 11:18 AM
> To: edk2-devel@lists.01.org
> Cc: Wu, Jiaxin ; Fu, Siyuan ; El-
> Haj-Mahmoud, Samer ; Laszlo Ersek
> ; Hegde, Nagaraj P 
> Subject: [PATCH v2] NetworkPkg/HttpDxe: Don't free Wrap in
> HttpTcpReceiveNotifyDpc
> 
> The HTTP Token Wrap is created in EfiHttpResponse() and then passed
> to the deferred Receive event callback, HttpTcpReceiveNotifyDpc.
> HttpTcpReceiveHeader and HttpTcpReceiveBody use a Tcp polling loop to
> monitor the socket status and trigger the Receive event when a new
> packet arrives. The Receive event brings up HttpTcpReceiveNotifyDpc
> to process the HTTP message and the function will set Wrap-
> >TcpWrap.IsRxDone
> to TRUE to break the Tcp polling loop.
> 
> However, HttpTcpReceiveNotifyDpc mistakenly freed Wrap, so the Tcp
> polling loop was actually checking a dead variable, and this led the
> system into an unstable status.
> 
> Given the fact that the HTTP Token Wrap will be freed in EfiHttpResponse
> or HttpResponseWorker, this commit removes every "FreePool (Wrap)" in
> HttpTcpReceiveNotifyDpc.
> 
> v2:
> * Free Wrap after HttpTcpReceiveBody returns normally.
> 
> Cc: "Wu, Jiaxin" 
> Cc: "Siyuan Fu" 
> Cc: "El-Haj-Mahmoud, Samer" 
> Cc: "Laszlo Ersek" 
> Cc: "Hegde, Nagaraj P" 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Gary Lin 
> ---
>  NetworkPkg/HttpDxe/HttpImpl.c  | 1 +
>  NetworkPkg/HttpDxe/HttpProto.c | 4 
>  2 files changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/NetworkPkg/HttpDxe/HttpImpl.c
> b/NetworkPkg/HttpDxe/HttpImpl.c
> index dd10f82..f4ae28a 100644
> --- a/NetworkPkg/HttpDxe/HttpImpl.c
> +++ b/NetworkPkg/HttpDxe/HttpImpl.c
> @@ -1237,6 +1237,7 @@ HttpResponseWorker (
>  goto Error;
>}
> 
> +  FreePool (Wrap);
>return Status;
> 
>  Exit:
> diff --git a/NetworkPkg/HttpDxe/HttpProto.c
> b/NetworkPkg/HttpDxe/HttpProto.c
> index f3992ed..afa7fe4 100644
> --- a/NetworkPkg/HttpDxe/HttpProto.c
> +++ b/NetworkPkg/HttpDxe/HttpProto.c
> @@ -152,7 +152,6 @@ HttpTcpReceiveNotifyDpc (
>  if (EFI_ERROR (Wrap->TcpWrap.Rx6Token.CompletionToken.Status)) {
>Wrap->HttpToken->Status = Wrap-
> >TcpWrap.Rx6Token.CompletionToken.Status;
>gBS->SignalEvent (Wrap->HttpToken->Event);
> -  FreePool (Wrap);
>return ;
>  }
> 
> @@ -162,7 +161,6 @@ HttpTcpReceiveNotifyDpc (
>  if (EFI_ERROR (Wrap->TcpWrap.Rx4Token.CompletionToken.Status)) {
>Wrap->HttpToken->Status = Wrap-
> >TcpWrap.Rx4Token.CompletionToken.Status;
>gBS->SignalEvent (Wrap->HttpToken->Event);
> -  FreePool (Wrap);
>return ;
>  }
>}
> @@ -234,8 +232,6 @@ HttpTcpReceiveNotifyDpc (
>// Check pending RxTokens and receive the HTTP message.
>//
>NetMapIterate (>HttpInstance->RxTokens, HttpTcpReceive, NULL);
> -
> -  FreePool (Wrap);
>  }
> 
>  /**
> --
> 2.8.2

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

Re: [edk2] [PATCH v2] NetworkPkg/HttpDxe: Don't free Wrap in HttpTcpReceiveNotifyDpc

[Wu, Jiaxin]

Reviewed-By: Wu Jiaxin 


> -Original Message-
> From: Gary Lin [mailto:g...@suse.com]
> Sent: Friday, May 20, 2016 11:18 AM
> To: edk2-devel@lists.01.org
> Cc: Wu, Jiaxin ; Fu, Siyuan ; El-
> Haj-Mahmoud, Samer ; Laszlo Ersek
> ; Hegde, Nagaraj P 
> Subject: [PATCH v2] NetworkPkg/HttpDxe: Don't free Wrap in
> HttpTcpReceiveNotifyDpc
> 
> The HTTP Token Wrap is created in EfiHttpResponse() and then passed to the
> deferred Receive event callback, HttpTcpReceiveNotifyDpc.
> HttpTcpReceiveHeader and HttpTcpReceiveBody use a Tcp polling loop to
> monitor the socket status and trigger the Receive event when a new packet
> arrives. The Receive event brings up HttpTcpReceiveNotifyDpc to process the
> HTTP message and the function will set Wrap->TcpWrap.IsRxDone to TRUE to
> break the Tcp polling loop.
> 
> However, HttpTcpReceiveNotifyDpc mistakenly freed Wrap, so the Tcp
> polling loop was actually checking a dead variable, and this led the system
> into an unstable status.
> 
> Given the fact that the HTTP Token Wrap will be freed in EfiHttpResponse or
> HttpResponseWorker, this commit removes every "FreePool (Wrap)" in
> HttpTcpReceiveNotifyDpc.
> 
> v2:
> * Free Wrap after HttpTcpReceiveBody returns normally.
> 
> Cc: "Wu, Jiaxin" 
> Cc: "Siyuan Fu" 
> Cc: "El-Haj-Mahmoud, Samer" 
> Cc: "Laszlo Ersek" 
> Cc: "Hegde, Nagaraj P" 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Gary Lin 
> ---
>  NetworkPkg/HttpDxe/HttpImpl.c  | 1 +
>  NetworkPkg/HttpDxe/HttpProto.c | 4 
>  2 files changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/NetworkPkg/HttpDxe/HttpImpl.c
> b/NetworkPkg/HttpDxe/HttpImpl.c index dd10f82..f4ae28a 100644
> --- a/NetworkPkg/HttpDxe/HttpImpl.c
> +++ b/NetworkPkg/HttpDxe/HttpImpl.c
> @@ -1237,6 +1237,7 @@ HttpResponseWorker (
>  goto Error;
>}
> 
> +  FreePool (Wrap);
>return Status;
> 
>  Exit:
> diff --git a/NetworkPkg/HttpDxe/HttpProto.c
> b/NetworkPkg/HttpDxe/HttpProto.c index f3992ed..afa7fe4 100644
> --- a/NetworkPkg/HttpDxe/HttpProto.c
> +++ b/NetworkPkg/HttpDxe/HttpProto.c
> @@ -152,7 +152,6 @@ HttpTcpReceiveNotifyDpc (
>  if (EFI_ERROR (Wrap->TcpWrap.Rx6Token.CompletionToken.Status)) {
>Wrap->HttpToken->Status = Wrap-
> >TcpWrap.Rx6Token.CompletionToken.Status;
>gBS->SignalEvent (Wrap->HttpToken->Event);
> -  FreePool (Wrap);
>return ;
>  }
> 
> @@ -162,7 +161,6 @@ HttpTcpReceiveNotifyDpc (
>  if (EFI_ERROR (Wrap->TcpWrap.Rx4Token.CompletionToken.Status)) {
>Wrap->HttpToken->Status = Wrap-
> >TcpWrap.Rx4Token.CompletionToken.Status;
>gBS->SignalEvent (Wrap->HttpToken->Event);
> -  FreePool (Wrap);
>return ;
>  }
>}
> @@ -234,8 +232,6 @@ HttpTcpReceiveNotifyDpc (
>// Check pending RxTokens and receive the HTTP message.
>//
>NetMapIterate (>HttpInstance->RxTokens, HttpTcpReceive, NULL);
> -
> -  FreePool (Wrap);
>  }
> 
>  /**
> --
> 2.8.2

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

[edk2] [PATCH v2] NetworkPkg/HttpDxe: Don't free Wrap in HttpTcpReceiveNotifyDpc

[Gary Lin]

The HTTP Token Wrap is created in EfiHttpResponse() and then passed
to the deferred Receive event callback, HttpTcpReceiveNotifyDpc.
HttpTcpReceiveHeader and HttpTcpReceiveBody use a Tcp polling loop to
monitor the socket status and trigger the Receive event when a new
packet arrives. The Receive event brings up HttpTcpReceiveNotifyDpc
to process the HTTP message and the function will set Wrap->TcpWrap.IsRxDone
to TRUE to break the Tcp polling loop.

However, HttpTcpReceiveNotifyDpc mistakenly freed Wrap, so the Tcp
polling loop was actually checking a dead variable, and this led the
system into an unstable status.

Given the fact that the HTTP Token Wrap will be freed in EfiHttpResponse
or HttpResponseWorker, this commit removes every "FreePool (Wrap)" in
HttpTcpReceiveNotifyDpc.

v2:
* Free Wrap after HttpTcpReceiveBody returns normally.

Cc: "Wu, Jiaxin" 
Cc: "Siyuan Fu" 
Cc: "El-Haj-Mahmoud, Samer" 
Cc: "Laszlo Ersek" 
Cc: "Hegde, Nagaraj P" 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Gary Lin 
---
 NetworkPkg/HttpDxe/HttpImpl.c  | 1 +
 NetworkPkg/HttpDxe/HttpProto.c | 4 
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c
index dd10f82..f4ae28a 100644
--- a/NetworkPkg/HttpDxe/HttpImpl.c
+++ b/NetworkPkg/HttpDxe/HttpImpl.c
@@ -1237,6 +1237,7 @@ HttpResponseWorker (
 goto Error;
   }
 
+  FreePool (Wrap);
   return Status;
 
 Exit:
diff --git a/NetworkPkg/HttpDxe/HttpProto.c b/NetworkPkg/HttpDxe/HttpProto.c
index f3992ed..afa7fe4 100644
--- a/NetworkPkg/HttpDxe/HttpProto.c
+++ b/NetworkPkg/HttpDxe/HttpProto.c
@@ -152,7 +152,6 @@ HttpTcpReceiveNotifyDpc (
 if (EFI_ERROR (Wrap->TcpWrap.Rx6Token.CompletionToken.Status)) {
   Wrap->HttpToken->Status = Wrap->TcpWrap.Rx6Token.CompletionToken.Status;
   gBS->SignalEvent (Wrap->HttpToken->Event);
-  FreePool (Wrap);
   return ;
 }
 
@@ -162,7 +161,6 @@ HttpTcpReceiveNotifyDpc (
 if (EFI_ERROR (Wrap->TcpWrap.Rx4Token.CompletionToken.Status)) {
   Wrap->HttpToken->Status = Wrap->TcpWrap.Rx4Token.CompletionToken.Status;
   gBS->SignalEvent (Wrap->HttpToken->Event);
-  FreePool (Wrap);
   return ;
 }
   }
@@ -234,8 +232,6 @@ HttpTcpReceiveNotifyDpc (
   // Check pending RxTokens and receive the HTTP message.
   //
   NetMapIterate (>HttpInstance->RxTokens, HttpTcpReceive, NULL);
-  
-  FreePool (Wrap);
 }
 
 /**
-- 
2.8.2

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

Re: [edk2] [BaseTools] RuleOverride = BINARY code location?

[Gao, Liming]

Thanks Andrew. I will continue to investigate it. 

> -Original Message-
> From: af...@apple.com [mailto:af...@apple.com]
> Sent: Friday, May 20, 2016 10:01 AM
> To: Gao, Liming 
> Cc: edk2-devel 
> Subject: Re: [edk2] [BaseTools] RuleOverride = BINARY code location?
> 
> 
> > On May 18, 2016, at 5:44 PM, Andrew Fish  wrote:
> >
> >>
> >> On May 18, 2016, at 5:43 PM, Gao, Liming  wrote:
> >>
> >> Andrew:
> >> |.bin will search the file with the postfix .bin in INF file directory and 
> >> its
> output directory. So, there may be more than .bin files are found. Please see
> the code login in GetFileList() from
> C:\R9Tip\edk2\BaseTools\Source\Python\GenFds\Section.py.
> >>
> >
> 
> Thanks for the pointer.
> 
> This is the code that adding the OS executables in GetFileList() that is 
> adding
> the OS executables without the .bin extension.
> if os.path.exists(Makefile):
> # Update to search files with suffix in all sub-dirs.
> Tuple = os.walk(FfsInf.EfiOutputPath)
> for Dirpath, Dirnames, Filenames in Tuple:
> for F in Filenames:
> if os.path.splitext(F)[1] in (Suffix):
> FullName = os.path.join(Dirpath, F)
> if os.path.getmtime(FullName) > 
> os.path.getmtime(Makefile):
> FileList.append(FullName)
> 
> 
> I think the issue is:
>if os.path.splitext(F)[1] in (Suffix):
> 
> If the file does not have an extension it matches.
> 
> Sorry might be wrong have to go out to a writers reading with my wife so
> have to stop looking right now.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> 
> Thanks,
> 
> Andrew Fish
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 1/2] NetworkPkg/HttpDxe: Don't free Wrap in HttpTcpReceiveNotifyDpc

[Gary Lin]

On Fri, May 20, 2016 at 12:21:39AM +, Wu, Jiaxin wrote:
> Gary, good catching! Wrap->TcpWrap.IsRxDone is invalid if it's freed in 
> HttpTcpReceiveNotifyDpc. But, if you remove it directly, Wrap will not be 
> freed for each TcpReceive in your patch. Actually, we should free it before 
> return HttpResponseWorker.
>//
>   // We still need receive more data when there is no cache data and 
> MsgParser is not NULL;
>   //
>   Status = HttpTcpReceiveBody (Wrap, HttpMsg, HttpInstance->TimeoutEvent);
>   gBS->SetTimer (HttpInstance->TimeoutEvent, TimerCancel, 0);
>   if (EFI_ERROR (Status)) {
>  goto Error;
>   }
>FreePool (Wrap);< Free it here.
>   return Status;
> 
Ah, right! I should add that. I'll send a V2.

Thanks,

Gary Lin

> Thanks.
> Jiaxin
> 
> > -Original Message-
> > From: Gary Lin [mailto:g...@suse.com]
> > Sent: Thursday, May 19, 2016 11:49 AM
> > To: edk2-devel@lists.01.org
> > Cc: Wu, Jiaxin ; Fu, Siyuan ; El-
> > Haj-Mahmoud, Samer ; Laszlo Ersek
> > ; Hegde, Nagaraj P 
> > Subject: [PATCH 1/2] NetworkPkg/HttpDxe: Don't free Wrap in
> > HttpTcpReceiveNotifyDpc
> > 
> > The HTTP Token Wrap is created in EfiHttpResponse() and then passed to the
> > deferred Receive event callback, HttpTcpReceiveNotifyDpc.
> > HttpTcpReceiveHeader and HttpTcpReceiveBody use a Tcp polling loop to
> > monitor the socket status and trigger the Receive event when a new packet
> > arrives. The Receive event brings up HttpTcpReceiveNotifyDpc to process the
> > HTTP message and the function will set Wrap->TcpWrap.IsRxDone to TRUE to
> > break the Tcp polling loop.
> > 
> > However, HttpTcpReceiveNotifyDpc mistakenly freed Wrap, so the Tcp
> > polling loop was actually checking a dead variable, and this led the system
> > into an unstable status.
> > 
> > Given the fact that the HTTP Token Wrap will be freed in EfiHttpResponse or
> > HttpResponseWorker, this commit removes every "FreePool (Wrap)" in
> > HttpTcpReceiveNotifyDpc.
> > 
> > Cc: "Wu, Jiaxin" 
> > Cc: "Siyuan Fu" 
> > Cc: "El-Haj-Mahmoud, Samer" 
> > Cc: "Laszlo Ersek" 
> > Cc: "Hegde, Nagaraj P" 
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Gary Lin 
> > ---
> >  NetworkPkg/HttpDxe/HttpProto.c | 4 
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/NetworkPkg/HttpDxe/HttpProto.c
> > b/NetworkPkg/HttpDxe/HttpProto.c index f3992ed..afa7fe4 100644
> > --- a/NetworkPkg/HttpDxe/HttpProto.c
> > +++ b/NetworkPkg/HttpDxe/HttpProto.c
> > @@ -152,7 +152,6 @@ HttpTcpReceiveNotifyDpc (
> >  if (EFI_ERROR (Wrap->TcpWrap.Rx6Token.CompletionToken.Status)) {
> >Wrap->HttpToken->Status = Wrap-
> > >TcpWrap.Rx6Token.CompletionToken.Status;
> >gBS->SignalEvent (Wrap->HttpToken->Event);
> > -  FreePool (Wrap);
> >return ;
> >  }
> > 
> > @@ -162,7 +161,6 @@ HttpTcpReceiveNotifyDpc (
> >  if (EFI_ERROR (Wrap->TcpWrap.Rx4Token.CompletionToken.Status)) {
> >Wrap->HttpToken->Status = Wrap-
> > >TcpWrap.Rx4Token.CompletionToken.Status;
> >gBS->SignalEvent (Wrap->HttpToken->Event);
> > -  FreePool (Wrap);
> >return ;
> >  }
> >}
> > @@ -234,8 +232,6 @@ HttpTcpReceiveNotifyDpc (
> >// Check pending RxTokens and receive the HTTP message.
> >//
> >NetMapIterate (>HttpInstance->RxTokens, HttpTcpReceive, NULL);
> > -
> > -  FreePool (Wrap);
> >  }
> > 
> >  /**
> > --
> > 2.8.2
> 
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH v2] MdeModulePkg/DisplayEngine: Fix memory leak issues in DisplayEngine

[Dandan Bi]

The following codes are useless and cause memory leak issues.
So now remove them.

Cc: Cecil Sheng 
Cc: Qiu Shumin 
Cc: Eric Dong 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Dandan Bi 
---
 MdeModulePkg/Universal/DisplayEngineDxe/InputHandler.c   |  5 +
 MdeModulePkg/Universal/DisplayEngineDxe/ProcessOptions.c | 12 +---
 2 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/MdeModulePkg/Universal/DisplayEngineDxe/InputHandler.c 
b/MdeModulePkg/Universal/DisplayEngineDxe/InputHandler.c
index 732dd2f..8e7b735 100644
--- a/MdeModulePkg/Universal/DisplayEngineDxe/InputHandler.c
+++ b/MdeModulePkg/Universal/DisplayEngineDxe/InputHandler.c
@@ -1,9 +1,9 @@
 /** @file
 Implementation for handling user input from the User Interfaces.
 
-Copyright (c) 2004 - 2015, Intel Corporation. All rights reserved.
+Copyright (c) 2004 - 2016, 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
 
@@ -1297,13 +1297,10 @@ GetSelectionInputPopUp (
   ValueType = 0;
   CurrentOption = NULL;
   ShowDownArrow = FALSE;
   ShowUpArrow   = FALSE;
 
-  StringPtr = AllocateZeroPool ((gOptionBlockWidth + 1) * 2);
-  ASSERT (StringPtr);
-
   ZeroMem (, sizeof (EFI_HII_VALUE));
 
   Question = MenuOption->ThisTag;
   if (Question->OpCode->OpCode == EFI_IFR_ORDERED_LIST_OP) {
 Link = GetFirstNode (>OptionListHead);
diff --git a/MdeModulePkg/Universal/DisplayEngineDxe/ProcessOptions.c 
b/MdeModulePkg/Universal/DisplayEngineDxe/ProcessOptions.c
index bb2faf3..c61a395 100644
--- a/MdeModulePkg/Universal/DisplayEngineDxe/ProcessOptions.c
+++ b/MdeModulePkg/Universal/DisplayEngineDxe/ProcessOptions.c
@@ -1,10 +1,10 @@
 /** @file
 Implementation for handling the User Interface option processing.
 
 
-Copyright (c) 2004 - 2015, Intel Corporation. All rights reserved.
+Copyright (c) 2004 - 2016, 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
 
@@ -883,22 +883,12 @@ PasswordProcess (
   if (StrCmp (StringPtr, TempString) == 0) { 
 gUserInput->InputValue.Buffer = AllocateCopyPool 
(Question->CurrentValue.BufferLen, StringPtr);
 gUserInput->InputValue.BufferLen = Question->CurrentValue.BufferLen;
 gUserInput->InputValue.Type = Question->CurrentValue.Type;
 gUserInput->InputValue.Value.string = HiiSetString(gFormData->HiiHandle, 
gUserInput->InputValue.Value.string, StringPtr, NULL);
-FreePool (StringPtr); 
 
 Status = EFI_SUCCESS;
-
-if (EFI_ERROR (Status)) {
-  //
-  // Reset state machine for password
-  //
-  Question->PasswordCheck (gFormData, Question, NULL);
-}
-
-return Status;
   } else {
 //
 // Reset state machine for password
 //
 Question->PasswordCheck (gFormData, Question, NULL);
-- 
1.9.5.msysgit.1

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

Re: [edk2] [patch] NetworkPkg: Refine codes of Http boot driver.

[Fu, Siyuan]

Hi, Samer

I think in PXE this issue was solved by the 
EFI_PXE_BASE_CODE_CALLBACK_PROTOCOL, which allows the platform owner to provide 
an callback implementation which will take precedence over the default 
implementation in PXE driver. The callback protocol allows the platform owner 
to display the process messages or even abort the PXE boot. Do you think we 
could reference this practice in HTTP boot?

Best Regards
Siyuan

> -Original Message-
> From: El-Haj-Mahmoud, Samer [mailto:samer.el-haj-mahm...@hpe.com]
> Sent: Thursday, May 19, 2016 9:46 PM
> To: Zhang, Lubo ; edk2-devel@lists.01.org
> Cc: Ye, Ting ; Fu, Siyuan ; Wu,
> Jiaxin 
> Subject: RE: [edk2] [patch] NetworkPkg: Refine codes of Http boot driver.
> 
> Thanks Zhang.
> 
> I know this makes sense (and does help during lengthy download), and I also
> know that PXE does something similar. I am only concerned about this
> happening after BGRT logo is displayed on the screen (if PXE/HTTP boot
> options are tried after a failed HDD boot option for instance). The result is
> that the messages will be printed on top of the already displayed logo, which
> is also reported in ACPI BGRT table. It may not be a big issue, but it did 
> result
> in some user complaints at times. What do you think?
> 
> Other than that, I would improve the messaging a bit. See feedback below.
> 
> 
> 
> 
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Zhang Lubo
> Sent: Wednesday, May 18, 2016 10:53 PM
> To: edk2-devel@lists.01.org
> Cc: Ye Ting ; Fu Siyuan ; Wu Jiaxin
> 
> Subject: [edk2] [patch] NetworkPkg: Refine codes of Http boot driver.
> 
> When downloading a big image as ram disk iso,we can print the progress
> message on screen to enhance the user experience.
> 
> Cc: Ye Ting 
> Cc: Fu Siyuan 
> Cc: Wu Jiaxin 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Zhang Lubo 
> ---
>  NetworkPkg/HttpBootDxe/HttpBootClient.c  | 29
> +
> NetworkPkg/HttpBootDxe/HttpBootSupport.c |  4 ++--
>  2 files changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/NetworkPkg/HttpBootDxe/HttpBootClient.c
> b/NetworkPkg/HttpBootDxe/HttpBootClient.c
> index 46cf9ca..9b2a8bd 100644
> --- a/NetworkPkg/HttpBootDxe/HttpBootClient.c
> +++ b/NetworkPkg/HttpBootDxe/HttpBootClient.c
> @@ -753,10 +753,12 @@ HttpBootGetBootFile (
>HTTP_BOOT_CACHE_CONTENT*Cache;
>UINT8  *Block;
>CHAR16 *Url;
>BOOLEANIdentityMode;
>UINTN  ReceivedSize;
> +  UINTN  Ratio;
> +  UINTN  NewRatio;
> 
>ASSERT (Private != NULL);
>ASSERT (Private->HttpCreated);
> 
>if (BufferSize == NULL || ImageType == NULL) { @@ -992,10 +994,13 @@
> HttpBootGetBootFile (
>  //
>  // 3.4.2, start the message-body download, the identity and chunked
> transfer-coding
>  // is handled in different path here.
>  //
>  ZeroMem (, sizeof (HTTP_IO_RESPONSE_DATA));
> +AsciiPrint ("\n");
> +AsciiPrint ("\n  Downloading NBP file... ");
> 
> 
> [Samer] How about printing the URL where the file is being downloaded from
> so the user knows that this is happening from HTTP Boot and not PXE (since
> the messages are identical)
> 
> 
> 
> +Ratio = 0;
>  if (IdentityMode) {
>//
>// In identity transfer-coding there is no need to parse the message 
> body,
>// just download the message body to the user provided buffer directly.
>//
> @@ -1010,10 +1015,25 @@ HttpBootGetBootFile (
> );
>  if (EFI_ERROR (Status)) {
>goto ERROR_6;
>  }
>  ReceivedSize += ResponseBody.BodyLength;
> +//
> +// Display the download progress.
> +//
> +NewRatio = (ReceivedSize * 100) / ContentLength;
> +if (NewRatio != Ratio) {
> +  if (Ratio !=0) {
> +if (Ratio >=10) {
> +  AsciiPrint ("\b\b\b");
> +} else {
> +  AsciiPrint ("\b\b");
> +}
> +  }
> +  Ratio = NewRatio;
> +  AsciiPrint ("%d%%", NewRatio);
> +}
>}
>  } else {
>//
>// In "chunked" transfer-coding mode, so we need to parse the received
>// data to get the real entity content.
> @@ -1058,10 +1078,19 @@ HttpBootGetBootFile (
> ResponseBody.Body
> );
>  if (EFI_ERROR (Status)) {
>goto ERROR_6;
>  }
> +
> +//
> +// Print '.' when using chunked mode to download bootfile.
> +//
> +Ratio ++;
> +if ((Ratio % 1024) == 0) {
> +  

Re: [edk2] edk2-staging/HTTPS-TLS

[Wu, Jiaxin]

Thanks all of your points. So, option 1 is preferred (Keep the liner commit 
history). And I agree to regular sync edk2-staging/master to latest edk2/master 
automatically, but not for each feature branch's rebase.

Jordan,
If no objection, can you help to set up this regular sync between 
edk2-staging/master and edk2/master? If so, you can avoid the frequency request.

Samer,
I will resolve the conflicts once the regular sync finished.

Thanks.
Jiaxin


> -Original Message-
> From: El-Haj-Mahmoud, Samer [mailto:samer.el-haj-mahm...@hpe.com]
> Sent: Thursday, May 19, 2016 9:27 PM
> To: Laszlo Ersek ; Gao, Liming ;
> Wu, Jiaxin ; Li, Ruth ; Ye, Ting
> ; Long, Qin ; edk2-
> de...@lists.01.org ; Justen, Jordan L
> ; Kinney, Michael D
> ; Leif Lindholm (Linaro address)
> 
> Subject: RE: [edk2] edk2-staging/HTTPS-TLS
> 
> I do have some experience maintaining my branches (in other projects) that
> eventually get submitted as pull requests to a master. And I always find it
> easier to keep my branch in sync by doing frequent merge form master. I
> agree that it probably makes more sense to do this a couple of times a week
> (or so) manually, and resolve any merge conflicts, than try to automate it.
> 
> At this point, I am working on enabling the HTTPS/TLS feature, but I need to
> do so in a code base that is up-to-date with EDK2 master. So the main issue I
> have is merging from (the now falling behind) HTTPS/TLS branch into my
> code base (which is tracking EDK2). As Laszlo said, it is already getting mad
> because of all of the HTTP fixes in EDK2.
> 
> Thanks,
> --Samer
> 
> 
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Thursday, May 19, 2016 5:51 AM
> To: Gao, Liming ; Wu, Jiaxin ;
> El-Haj-Mahmoud, Samer ; Li, Ruth
> ; Ye, Ting ; Long, Qin
> ; edk2-devel@lists.01.org ;
> Justen, Jordan L ; Kinney, Michael D
> ; Leif Lindholm (Linaro address)
> 
> Subject: Re: [edk2] edk2-staging/HTTPS-TLS
> 
> On 05/19/16 11:53, Gao, Liming wrote:
> > Ersek:
> > I remember we discuss Rebase or Merge before. The conclusion is to
> > keep the liner history in edk2 project. So, I think rebase is better.
> 
> Okay.
> 
> > If we choose option 1, I suggest to setup mirror task in server and
> > auto run regular sync. If the confliction happens, it sends the mail
> > to edk dev list and let owner follow up.
> 
> Hmm... I'm not sure about an automated rebase. I always have a number of
> downstream branches (in my private repo) that I rebase almost every day to
> new master.
> 
> However, occasionally I'm so deep in work that I don't want to rebase for a
> week, for example. An automated rebase would be very annoying in such
> situations, when I'm in the middle of shuffling around my patches.
> 
> I think it's the owner's responsibility after all. If they are fine with an
> automated rebase, so be it; but the automatism should be possible to disable.
> 
> Thanks
> Laszlo
> 
> > Thanks
> > Liming
> >> -Original Message-
> >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
> >> Of Laszlo Ersek
> >> Sent: Thursday, May 19, 2016 4:31 PM
> >> To: Wu, Jiaxin ; El-Haj-Mahmoud, Samer
> >> ; Li, Ruth ; Ye,
> >> Ting ; Long, Qin ; edk2-
> >> de...@lists.01.org ; Justen, Jordan L
> >> ; Kinney, Michael D
> >> ; Leif Lindholm (Linaro address)
> >> 
> >> Subject: Re: [edk2] edk2-staging/HTTPS-TLS
> >>
> >> On 05/19/16 05:35, Wu, Jiaxin wrote:
> >>> Hi all,
> >>> There are two ways to sync HTTPS-TLS branch with EDK2 master:
> >>>
> >>> 1. Sync all changes in EDK2 master to branch by rebasing the whole
> >>> feature branch. Need frequency request to sync edk2-staging/master
> >>> to latest edk2/master. This way can also keep branch code go
> >>> straight with edk2/master.
> >>>
> >>> 2. Only sync HTTPS/TLS related patches to branch (e.g. HttpDxe
> >>> update). That means to use one stable edk2 tag for
> >>> edk2-staging/HTTPS-TLS branch development. By this way, frequency
> >>> request to sync edk2-staging/master to latest edk2/master can be
> >>> avoided but maybe some bugs fixed in edk2/master can't be synced to
> >>> branch timely.
> >>>
> >>> Both of them are not easy since it's probable that each updates in
> >>> edk2 trunk may cause the conflicts with the HTTPS code (HttpDxe
> >>> driver and CryptyPkg especially).
> >>>
> >>> Effort 

Re: [edk2] [BaseTools] RuleOverride = BINARY code location?

[Andrew Fish]

> On May 18, 2016, at 5:44 PM, Andrew Fish  wrote:
> 
>> 
>> On May 18, 2016, at 5:43 PM, Gao, Liming  wrote:
>> 
>> Andrew:
>> |.bin will search the file with the postfix .bin in INF file directory and 
>> its output directory. So, there may be more than .bin files are found. 
>> Please see the code login in GetFileList() from 
>> C:\R9Tip\edk2\BaseTools\Source\Python\GenFds\Section.py.
>> 
> 

Thanks for the pointer. 

This is the code that adding the OS executables in GetFileList() that is adding 
the OS executables without the .bin extension. 
if os.path.exists(Makefile):
# Update to search files with suffix in all sub-dirs.
Tuple = os.walk(FfsInf.EfiOutputPath)
for Dirpath, Dirnames, Filenames in Tuple:
for F in Filenames:
if os.path.splitext(F)[1] in (Suffix):
FullName = os.path.join(Dirpath, F)
if os.path.getmtime(FullName) > 
os.path.getmtime(Makefile):
FileList.append(FullName)


I think the issue is:
   if os.path.splitext(F)[1] in (Suffix):

If the file does not have an extension it matches. 

Sorry might be wrong have to go out to a writers reading with my wife so have 
to stop looking right now.

Contributed-under: TianoCore Contribution Agreement 1.0

Thanks,

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

Re: [edk2] [patch] NetworkPkg: Refine codes of Http boot driver.

[Zhang, Lubo]

Thanks Siyuan, I will improve the codes.

lubo

From: Fu, Siyuan
Sent: Friday, May 20, 2016 9:19 AM
To: Zhang, Lubo ; edk2-devel@lists.01.org
Cc: Ye, Ting ; Wu, Jiaxin 
Subject: RE: [patch] NetworkPkg: Refine codes of Http boot driver.

And I think the "percentage" is what you actually means by the name "Ratio".

From: Fu, Siyuan
Sent: Friday, May 20, 2016 9:17 AM
To: Zhang, Lubo >; 
edk2-devel@lists.01.org
Cc: Ye, Ting >; Wu, Jiaxin 
>
Subject: RE: [patch] NetworkPkg: Refine codes of Http boot driver.

Hi, Lubo

1. The (ReceivedSize * 100) may lead to a integer overflow, especially in a 
32bit system.
2. You could use %[wide]d to control the width of the print, to avoid the if 
condition "Ratio>10"

Best Regards
Siyuan

> -Original Message-
> From: Zhang, Lubo
> Sent: Thursday, May 19, 2016 11:53 AM
> To: edk2-devel@lists.01.org
> Cc: Ye, Ting >; Fu, Siyuan 
> >; Wu,
> Jiaxin >
> Subject: [patch] NetworkPkg: Refine codes of Http boot driver.
>
> When downloading a big image as ram disk iso,we can
> print the progress message on screen to enhance the
> user experience.
>
> Cc: Ye Ting >
> Cc: Fu Siyuan >
> Cc: Wu Jiaxin >
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Zhang Lubo >
> ---
>  NetworkPkg/HttpBootDxe/HttpBootClient.c  | 29
> +
>  NetworkPkg/HttpBootDxe/HttpBootSupport.c |  4 ++--
>  2 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/NetworkPkg/HttpBootDxe/HttpBootClient.c
> b/NetworkPkg/HttpBootDxe/HttpBootClient.c
> index 46cf9ca..9b2a8bd 100644
> --- a/NetworkPkg/HttpBootDxe/HttpBootClient.c
> +++ b/NetworkPkg/HttpBootDxe/HttpBootClient.c
> @@ -753,10 +753,12 @@ HttpBootGetBootFile (
>HTTP_BOOT_CACHE_CONTENT*Cache;
>UINT8  *Block;
>CHAR16 *Url;
>BOOLEANIdentityMode;
>UINTN  ReceivedSize;
> +  UINTN  Ratio;
> +  UINTN  NewRatio;
>
>ASSERT (Private != NULL);
>ASSERT (Private->HttpCreated);
>
>if (BufferSize == NULL || ImageType == NULL) {
> @@ -992,10 +994,13 @@ HttpBootGetBootFile (
>  //
>  // 3.4.2, start the message-body download, the identity and chunked
> transfer-coding
>  // is handled in different path here.
>  //
>  ZeroMem (, sizeof (HTTP_IO_RESPONSE_DATA));
> +AsciiPrint ("\n");
> +AsciiPrint ("\n  Downloading NBP file... ");
> +Ratio = 0;
>  if (IdentityMode) {
>//
>// In identity transfer-coding there is no need to parse the message 
> body,
>// just download the message body to the user provided buffer directly.
>//
> @@ -1010,10 +1015,25 @@ HttpBootGetBootFile (
> );
>  if (EFI_ERROR (Status)) {
>goto ERROR_6;
>  }
>  ReceivedSize += ResponseBody.BodyLength;
> +//
> +// Display the download progress.
> +//
> +NewRatio = (ReceivedSize * 100) / ContentLength;
> +if (NewRatio != Ratio) {
> +  if (Ratio !=0) {
> +if (Ratio >=10) {
> +  AsciiPrint ("\b\b\b");
> +} else {
> +  AsciiPrint ("\b\b");
> +}
> +  }
> +  Ratio = NewRatio;
> +  AsciiPrint ("%d%%", NewRatio);
> +}
>}
>  } else {
>//
>// In "chunked" transfer-coding mode, so we need to parse the received
>// data to get the real entity content.
> @@ -1058,10 +1078,19 @@ HttpBootGetBootFile (
> ResponseBody.Body
> );
>  if (EFI_ERROR (Status)) {
>goto ERROR_6;
>  }
> +
> +//
> +// Print '.' when using chunked mode to download bootfile.
> +//
> +Ratio ++;
> +if ((Ratio % 1024) == 0) {
> +  AsciiPrint (".");
> +}
> +
>}
>  }
>}
>
>//
> diff --git a/NetworkPkg/HttpBootDxe/HttpBootSupport.c
> b/NetworkPkg/HttpBootDxe/HttpBootSupport.c
> index 66eca78..71e4826 100644
> --- a/NetworkPkg/HttpBootDxe/HttpBootSupport.c
> +++ b/NetworkPkg/HttpBootDxe/HttpBootSupport.c
> @@ -1121,11 +1121,11 @@ HttpBootRegisterRamDisk (
>ASSERT (Buffer != NULL);
>ASSERT (BufferSize != 0);
>
>Status = gBS->LocateProtocol (, NULL, (VOID**)
> );
>if (EFI_ERROR (Status)) {
> -DEBUG 

Re: [edk2] [PATCH 2/2] NetworkPkg/TcpDxe: Remove the status check of SockProcessRcvToken

[Fu, Siyuan]

Reviewed-by: Fu Siyuan 

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Gary Lin
> Sent: Thursday, May 19, 2016 11:49 AM
> To: edk2-devel@lists.01.org
> Cc: Fu, Siyuan ; Wu, Jiaxin 
> Subject: [edk2] [PATCH 2/2] NetworkPkg/TcpDxe: Remove the status check of
> SockProcessRcvToken
> 
> SockProcessRcvToken only returns the number of the received bytes, not
> an EFI Status.
> 
> Cc: "Siyuan Fu" 
> Cc: "Jiaxin Wu" 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Gary Lin 
> ---
>  NetworkPkg/TcpDxe/SockInterface.c | 6 +-
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/NetworkPkg/TcpDxe/SockInterface.c
> b/NetworkPkg/TcpDxe/SockInterface.c
> index 4abda74..1f3524b 100644
> --- a/NetworkPkg/TcpDxe/SockInterface.c
> +++ b/NetworkPkg/TcpDxe/SockInterface.c
> @@ -724,11 +724,7 @@ SockRcv (
>}
> 
>if (RcvdBytes != 0) {
> -Status = SockProcessRcvToken (Sock, RcvToken);
> -
> -if (EFI_ERROR (Status)) {
> -  goto Exit;
> -}
> +SockProcessRcvToken (Sock, RcvToken);
> 
>  Status = Sock->ProtoHandler (Sock, SOCK_CONSUMED, NULL);
>} else {
> --
> 2.8.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

Re: [edk2] [patch] NetworkPkg: Refine codes of Http boot driver.

[Fu, Siyuan]

And I think the "percentage" is what you actually means by the name "Ratio".

From: Fu, Siyuan
Sent: Friday, May 20, 2016 9:17 AM
To: Zhang, Lubo ; edk2-devel@lists.01.org
Cc: Ye, Ting ; Wu, Jiaxin 
Subject: RE: [patch] NetworkPkg: Refine codes of Http boot driver.

Hi, Lubo

1. The (ReceivedSize * 100) may lead to a integer overflow, especially in a 
32bit system.
2. You could use %[wide]d to control the width of the print, to avoid the if 
condition "Ratio>10"

Best Regards
Siyuan

> -Original Message-
> From: Zhang, Lubo
> Sent: Thursday, May 19, 2016 11:53 AM
> To: edk2-devel@lists.01.org
> Cc: Ye, Ting >; Fu, Siyuan 
> >; Wu,
> Jiaxin >
> Subject: [patch] NetworkPkg: Refine codes of Http boot driver.
>
> When downloading a big image as ram disk iso,we can
> print the progress message on screen to enhance the
> user experience.
>
> Cc: Ye Ting >
> Cc: Fu Siyuan >
> Cc: Wu Jiaxin >
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Zhang Lubo >
> ---
>  NetworkPkg/HttpBootDxe/HttpBootClient.c  | 29
> +
>  NetworkPkg/HttpBootDxe/HttpBootSupport.c |  4 ++--
>  2 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/NetworkPkg/HttpBootDxe/HttpBootClient.c
> b/NetworkPkg/HttpBootDxe/HttpBootClient.c
> index 46cf9ca..9b2a8bd 100644
> --- a/NetworkPkg/HttpBootDxe/HttpBootClient.c
> +++ b/NetworkPkg/HttpBootDxe/HttpBootClient.c
> @@ -753,10 +753,12 @@ HttpBootGetBootFile (
>HTTP_BOOT_CACHE_CONTENT*Cache;
>UINT8  *Block;
>CHAR16 *Url;
>BOOLEANIdentityMode;
>UINTN  ReceivedSize;
> +  UINTN  Ratio;
> +  UINTN  NewRatio;
>
>ASSERT (Private != NULL);
>ASSERT (Private->HttpCreated);
>
>if (BufferSize == NULL || ImageType == NULL) {
> @@ -992,10 +994,13 @@ HttpBootGetBootFile (
>  //
>  // 3.4.2, start the message-body download, the identity and chunked
> transfer-coding
>  // is handled in different path here.
>  //
>  ZeroMem (, sizeof (HTTP_IO_RESPONSE_DATA));
> +AsciiPrint ("\n");
> +AsciiPrint ("\n  Downloading NBP file... ");
> +Ratio = 0;
>  if (IdentityMode) {
>//
>// In identity transfer-coding there is no need to parse the message 
> body,
>// just download the message body to the user provided buffer directly.
>//
> @@ -1010,10 +1015,25 @@ HttpBootGetBootFile (
> );
>  if (EFI_ERROR (Status)) {
>goto ERROR_6;
>  }
>  ReceivedSize += ResponseBody.BodyLength;
> +//
> +// Display the download progress.
> +//
> +NewRatio = (ReceivedSize * 100) / ContentLength;
> +if (NewRatio != Ratio) {
> +  if (Ratio !=0) {
> +if (Ratio >=10) {
> +  AsciiPrint ("\b\b\b");
> +} else {
> +  AsciiPrint ("\b\b");
> +}
> +  }
> +  Ratio = NewRatio;
> +  AsciiPrint ("%d%%", NewRatio);
> +}
>}
>  } else {
>//
>// In "chunked" transfer-coding mode, so we need to parse the received
>// data to get the real entity content.
> @@ -1058,10 +1078,19 @@ HttpBootGetBootFile (
> ResponseBody.Body
> );
>  if (EFI_ERROR (Status)) {
>goto ERROR_6;
>  }
> +
> +//
> +// Print '.' when using chunked mode to download bootfile.
> +//
> +Ratio ++;
> +if ((Ratio % 1024) == 0) {
> +  AsciiPrint (".");
> +}
> +
>}
>  }
>}
>
>//
> diff --git a/NetworkPkg/HttpBootDxe/HttpBootSupport.c
> b/NetworkPkg/HttpBootDxe/HttpBootSupport.c
> index 66eca78..71e4826 100644
> --- a/NetworkPkg/HttpBootDxe/HttpBootSupport.c
> +++ b/NetworkPkg/HttpBootDxe/HttpBootSupport.c
> @@ -1121,11 +1121,11 @@ HttpBootRegisterRamDisk (
>ASSERT (Buffer != NULL);
>ASSERT (BufferSize != 0);
>
>Status = gBS->LocateProtocol (, NULL, (VOID**)
> );
>if (EFI_ERROR (Status)) {
> -DEBUG ((EFI_D_ERROR, "HTTP Boot: Couldn't find the RAM Disk protocol -
>  %r\n", Status));
> +AsciiPrint ("\n  HTTP Boot: Couldn't find the RAM Disk protocol - %r\n",
> Status);
>  return Status;
>}
>
>if (ImageType == ImageTypeVirtualCd) {
>  RamDiskType = 
> @@ -1141,11 +1141,11 @@ HttpBootRegisterRamDisk (
>   RamDiskType,
>   Private->UsingIpv6 ? Private->Ip6Nic->DevicePath : 

Re: [edk2] [patch] NetworkPkg: Refine codes of Http boot driver.

[Fu, Siyuan]

Hi, Lubo

1. The (ReceivedSize * 100) may lead to a integer overflow, especially in a 
32bit system.
2. You could use %[wide]d to control the width of the print, to avoid the if 
condition "Ratio>10"

Best Regards
Siyuan

> -Original Message-
> From: Zhang, Lubo
> Sent: Thursday, May 19, 2016 11:53 AM
> To: edk2-devel@lists.01.org
> Cc: Ye, Ting ; Fu, Siyuan ; Wu,
> Jiaxin 
> Subject: [patch] NetworkPkg: Refine codes of Http boot driver.
> 
> When downloading a big image as ram disk iso,we can
> print the progress message on screen to enhance the
> user experience.
> 
> Cc: Ye Ting 
> Cc: Fu Siyuan 
> Cc: Wu Jiaxin 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Zhang Lubo 
> ---
>  NetworkPkg/HttpBootDxe/HttpBootClient.c  | 29
> +
>  NetworkPkg/HttpBootDxe/HttpBootSupport.c |  4 ++--
>  2 files changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/NetworkPkg/HttpBootDxe/HttpBootClient.c
> b/NetworkPkg/HttpBootDxe/HttpBootClient.c
> index 46cf9ca..9b2a8bd 100644
> --- a/NetworkPkg/HttpBootDxe/HttpBootClient.c
> +++ b/NetworkPkg/HttpBootDxe/HttpBootClient.c
> @@ -753,10 +753,12 @@ HttpBootGetBootFile (
>HTTP_BOOT_CACHE_CONTENT*Cache;
>UINT8  *Block;
>CHAR16 *Url;
>BOOLEANIdentityMode;
>UINTN  ReceivedSize;
> +  UINTN  Ratio;
> +  UINTN  NewRatio;
> 
>ASSERT (Private != NULL);
>ASSERT (Private->HttpCreated);
> 
>if (BufferSize == NULL || ImageType == NULL) {
> @@ -992,10 +994,13 @@ HttpBootGetBootFile (
>  //
>  // 3.4.2, start the message-body download, the identity and chunked
> transfer-coding
>  // is handled in different path here.
>  //
>  ZeroMem (, sizeof (HTTP_IO_RESPONSE_DATA));
> +AsciiPrint ("\n");
> +AsciiPrint ("\n  Downloading NBP file... ");
> +Ratio = 0;
>  if (IdentityMode) {
>//
>// In identity transfer-coding there is no need to parse the message 
> body,
>// just download the message body to the user provided buffer directly.
>//
> @@ -1010,10 +1015,25 @@ HttpBootGetBootFile (
> );
>  if (EFI_ERROR (Status)) {
>goto ERROR_6;
>  }
>  ReceivedSize += ResponseBody.BodyLength;
> +//
> +// Display the download progress.
> +//
> +NewRatio = (ReceivedSize * 100) / ContentLength;
> +if (NewRatio != Ratio) {
> +  if (Ratio !=0) {
> +if (Ratio >=10) {
> +  AsciiPrint ("\b\b\b");
> +} else {
> +  AsciiPrint ("\b\b");
> +}
> +  }
> +  Ratio = NewRatio;
> +  AsciiPrint ("%d%%", NewRatio);
> +}
>}
>  } else {
>//
>// In "chunked" transfer-coding mode, so we need to parse the received
>// data to get the real entity content.
> @@ -1058,10 +1078,19 @@ HttpBootGetBootFile (
> ResponseBody.Body
> );
>  if (EFI_ERROR (Status)) {
>goto ERROR_6;
>  }
> +
> +//
> +// Print '.' when using chunked mode to download bootfile.
> +//
> +Ratio ++;
> +if ((Ratio % 1024) == 0) {
> +  AsciiPrint (".");
> +}
> +
>}
>  }
>}
> 
>//
> diff --git a/NetworkPkg/HttpBootDxe/HttpBootSupport.c
> b/NetworkPkg/HttpBootDxe/HttpBootSupport.c
> index 66eca78..71e4826 100644
> --- a/NetworkPkg/HttpBootDxe/HttpBootSupport.c
> +++ b/NetworkPkg/HttpBootDxe/HttpBootSupport.c
> @@ -1121,11 +1121,11 @@ HttpBootRegisterRamDisk (
>ASSERT (Buffer != NULL);
>ASSERT (BufferSize != 0);
> 
>Status = gBS->LocateProtocol (, NULL, (VOID**)
> );
>if (EFI_ERROR (Status)) {
> -DEBUG ((EFI_D_ERROR, "HTTP Boot: Couldn't find the RAM Disk protocol -
>  %r\n", Status));
> +AsciiPrint ("\n  HTTP Boot: Couldn't find the RAM Disk protocol - %r\n",
> Status);
>  return Status;
>}
> 
>if (ImageType == ImageTypeVirtualCd) {
>  RamDiskType = 
> @@ -1141,11 +1141,11 @@ HttpBootRegisterRamDisk (
>   RamDiskType,
>   Private->UsingIpv6 ? Private->Ip6Nic->DevicePath : 
> Private->Ip4Nic-
> >DevicePath,
>   
>   );
>if (EFI_ERROR (Status)) {
> -DEBUG ((EFI_D_ERROR, "HTTP Boot: Failed to register RAM Disk - %r\n",
> Status));
> +AsciiPrint ("\n  HTTP Boot: Failed to register RAM Disk - %r\n", Status);
>}
> 
>return Status;
>  }
> 
> --
> 1.9.5.msysgit.1

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

[edk2] [PATCH] MdePkg/WSMT.h: update header comment to use official URL link.

[Jiewen Yao]

Update WSMT table link to official MSDN URL.

Cc: Michael D Kinney 
Cc: Liming Gao 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiewen Yao 
---
 MdePkg/Include/IndustryStandard/WindowsSmmSecurityMitigationTable.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git 
a/MdePkg/Include/IndustryStandard/WindowsSmmSecurityMitigationTable.h 
b/MdePkg/Include/IndustryStandard/WindowsSmmSecurityMitigationTable.h
index bfbcf81..2b0a644 100644
--- a/MdePkg/Include/IndustryStandard/WindowsSmmSecurityMitigationTable.h
+++ b/MdePkg/Include/IndustryStandard/WindowsSmmSecurityMitigationTable.h
@@ -1,6 +1,6 @@
 /** @file
   Defines Windows SMM Security Mitigation Table
-  @ 
http://download.microsoft.com/download/1/8/A/18A21244-EB67-4538-BAA2-1A54E0E490B6/WSMT.docx
+  @ 
https://msdn.microsoft.com/windows/hardware/drivers/bringup/acpi-system-description-tables#wsmt
 
   Copyright (c) 2016, Intel Corporation. All rights reserved.
   This program and the accompanying materials 
-- 
2.7.4.windows.1

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

Re: [edk2] [patch] MdeModulePkg/DisplayEngine: Fix memory leak issues in DisplayEngine

[Bi, Dandan]

Hi Cecil,

Thanks for your comments!
I agree with you proposal. 

Hi All,
I will send a new patch to remove the useless code.
Please ignore this one!


Thanks,
Dandan

-Original Message-
From: Sheng, Cecil (HPS SW) [mailto:cecil.sh...@hpe.com] 
Sent: Friday, May 20, 2016 7:52 AM
To: Bi, Dandan ; edk2-devel@lists.01.org
Cc: Qiu, Shumin ; Dong, Eric 
Subject: RE: [edk2] [patch] MdeModulePkg/DisplayEngine: Fix memory leak issues 
in DisplayEngine

Hi Dandan,

The InputHandle.c one looks good. About the ProcessOptions.c one, I propose 
remove the lines starting "FreePool (StringPtr)", the following "if (EFI_ERROR 
(Status))" because it's  useless, and the "return Status" line. Then the 
clean-up block at the end of function frees the strings.


Sincerely,

Cecil Sheng
ISS Firmware Development
HPE Servers


-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Dandan Bi
Sent: Thursday, May 19, 2016 7:30 PM
To: edk2-devel@lists.01.org
Cc: Qiu Shumin ; Eric Dong 
Subject: [edk2] [patch] MdeModulePkg/DisplayEngine: Fix memory leak issues in 
DisplayEngine

Cc: Qiu Shumin 
Cc: Eric Dong 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Dandan Bi 
Reviewed-by: Eric Dong 
---
 MdeModulePkg/Universal/DisplayEngineDxe/InputHandler.c   | 5 +
 MdeModulePkg/Universal/DisplayEngineDxe/ProcessOptions.c | 3 ++-
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Universal/DisplayEngineDxe/InputHandler.c 
b/MdeModulePkg/Universal/DisplayEngineDxe/InputHandler.c
index 732dd2f..8e7b735 100644
--- a/MdeModulePkg/Universal/DisplayEngineDxe/InputHandler.c
+++ b/MdeModulePkg/Universal/DisplayEngineDxe/InputHandler.c
@@ -1,9 +1,9 @@
 /** @file
 Implementation for handling user input from the User Interfaces.
 
-Copyright (c) 2004 - 2015, Intel Corporation. All rights reserved.
+Copyright (c) 2004 - 2016, 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
 
@@ -1297,13 +1297,10 @@ GetSelectionInputPopUp (
   ValueType = 0;
   CurrentOption = NULL;
   ShowDownArrow = FALSE;
   ShowUpArrow   = FALSE;
 
-  StringPtr = AllocateZeroPool ((gOptionBlockWidth + 1) * 2);
-  ASSERT (StringPtr);
-
   ZeroMem (, sizeof (EFI_HII_VALUE));
 
   Question = MenuOption->ThisTag;
   if (Question->OpCode->OpCode == EFI_IFR_ORDERED_LIST_OP) {
 Link = GetFirstNode (>OptionListHead); diff --git 
a/MdeModulePkg/Universal/DisplayEngineDxe/ProcessOptions.c 
b/MdeModulePkg/Universal/DisplayEngineDxe/ProcessOptions.c
index bb2faf3..16aaf6c 100644
--- a/MdeModulePkg/Universal/DisplayEngineDxe/ProcessOptions.c
+++ b/MdeModulePkg/Universal/DisplayEngineDxe/ProcessOptions.c
@@ -1,10 +1,10 @@
 /** @file
 Implementation for handling the User Interface option processing.
 
 
-Copyright (c) 2004 - 2015, Intel Corporation. All rights reserved.
+Copyright (c) 2004 - 2016, 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
 
@@ -884,10 +884,11 @@ PasswordProcess (
 gUserInput->InputValue.Buffer = AllocateCopyPool 
(Question->CurrentValue.BufferLen, StringPtr);
 gUserInput->InputValue.BufferLen = Question->CurrentValue.BufferLen;
 gUserInput->InputValue.Type = Question->CurrentValue.Type;
 gUserInput->InputValue.Value.string = HiiSetString(gFormData->HiiHandle, 
gUserInput->InputValue.Value.string, StringPtr, NULL);
 FreePool (StringPtr); 
+FreePool (TempString);
 
 Status = EFI_SUCCESS;
 
 if (EFI_ERROR (Status)) {
   //
--
1.9.5.msysgit.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 2/2] NetworkPkg/TcpDxe: Remove the status check of SockProcessRcvToken

[Wu, Jiaxin]

MdeModulePkg\Universal\Network\Tcp4Dxe has the same issue, I will provide 
another patch to fix it also.

Thanks,
Jiaxin

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Wu, Jiaxin
> Sent: Friday, May 20, 2016 8:31 AM
> To: Gary Lin ; edk2-devel@lists.01.org
> Cc: Fu, Siyuan 
> Subject: Re: [edk2] [PATCH 2/2] NetworkPkg/TcpDxe: Remove the status
> check of SockProcessRcvToken
> 
> Reviewed-By: Wu Jiaxin 
> 
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > Gary Lin
> > Sent: Thursday, May 19, 2016 11:49 AM
> > To: edk2-devel@lists.01.org
> > Cc: Fu, Siyuan ; Wu, Jiaxin 
> > Subject: [edk2] [PATCH 2/2] NetworkPkg/TcpDxe: Remove the status
> check
> > of SockProcessRcvToken
> >
> > SockProcessRcvToken only returns the number of the received bytes, not
> > an EFI Status.
> >
> > Cc: "Siyuan Fu" 
> > Cc: "Jiaxin Wu" 
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Gary Lin 
> > ---
> >  NetworkPkg/TcpDxe/SockInterface.c | 6 +-
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/NetworkPkg/TcpDxe/SockInterface.c
> > b/NetworkPkg/TcpDxe/SockInterface.c
> > index 4abda74..1f3524b 100644
> > --- a/NetworkPkg/TcpDxe/SockInterface.c
> > +++ b/NetworkPkg/TcpDxe/SockInterface.c
> > @@ -724,11 +724,7 @@ SockRcv (
> >}
> >
> >if (RcvdBytes != 0) {
> > -Status = SockProcessRcvToken (Sock, RcvToken);
> > -
> > -if (EFI_ERROR (Status)) {
> > -  goto Exit;
> > -}
> > +SockProcessRcvToken (Sock, RcvToken);
> >
> >  Status = Sock->ProtoHandler (Sock, SOCK_CONSUMED, NULL);
> >} else {
> > --
> > 2.8.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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 2/2] NetworkPkg/TcpDxe: Remove the status check of SockProcessRcvToken

[Wu, Jiaxin]

Reviewed-By: Wu Jiaxin 


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Gary Lin
> Sent: Thursday, May 19, 2016 11:49 AM
> To: edk2-devel@lists.01.org
> Cc: Fu, Siyuan ; Wu, Jiaxin 
> Subject: [edk2] [PATCH 2/2] NetworkPkg/TcpDxe: Remove the status check
> of SockProcessRcvToken
> 
> SockProcessRcvToken only returns the number of the received bytes, not an
> EFI Status.
> 
> Cc: "Siyuan Fu" 
> Cc: "Jiaxin Wu" 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Gary Lin 
> ---
>  NetworkPkg/TcpDxe/SockInterface.c | 6 +-
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/NetworkPkg/TcpDxe/SockInterface.c
> b/NetworkPkg/TcpDxe/SockInterface.c
> index 4abda74..1f3524b 100644
> --- a/NetworkPkg/TcpDxe/SockInterface.c
> +++ b/NetworkPkg/TcpDxe/SockInterface.c
> @@ -724,11 +724,7 @@ SockRcv (
>}
> 
>if (RcvdBytes != 0) {
> -Status = SockProcessRcvToken (Sock, RcvToken);
> -
> -if (EFI_ERROR (Status)) {
> -  goto Exit;
> -}
> +SockProcessRcvToken (Sock, RcvToken);
> 
>  Status = Sock->ProtoHandler (Sock, SOCK_CONSUMED, NULL);
>} else {
> --
> 2.8.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

Re: [edk2] [PATCH 1/2] NetworkPkg/HttpDxe: Don't free Wrap in HttpTcpReceiveNotifyDpc

[Wu, Jiaxin]

Gary, good catching! Wrap->TcpWrap.IsRxDone is invalid if it's freed in 
HttpTcpReceiveNotifyDpc. But, if you remove it directly, Wrap will not be freed 
for each TcpReceive in your patch. Actually, we should free it before return 
HttpResponseWorker.
 //
// We still need receive more data when there is no cache data and 
MsgParser is not NULL;
//
Status = HttpTcpReceiveBody (Wrap, HttpMsg, HttpInstance->TimeoutEvent);
gBS->SetTimer (HttpInstance->TimeoutEvent, TimerCancel, 0);
if (EFI_ERROR (Status)) {
   goto Error;
}
 FreePool (Wrap);< Free it here.
return Status;

Thanks.
Jiaxin

> -Original Message-
> From: Gary Lin [mailto:g...@suse.com]
> Sent: Thursday, May 19, 2016 11:49 AM
> To: edk2-devel@lists.01.org
> Cc: Wu, Jiaxin ; Fu, Siyuan ; El-
> Haj-Mahmoud, Samer ; Laszlo Ersek
> ; Hegde, Nagaraj P 
> Subject: [PATCH 1/2] NetworkPkg/HttpDxe: Don't free Wrap in
> HttpTcpReceiveNotifyDpc
> 
> The HTTP Token Wrap is created in EfiHttpResponse() and then passed to the
> deferred Receive event callback, HttpTcpReceiveNotifyDpc.
> HttpTcpReceiveHeader and HttpTcpReceiveBody use a Tcp polling loop to
> monitor the socket status and trigger the Receive event when a new packet
> arrives. The Receive event brings up HttpTcpReceiveNotifyDpc to process the
> HTTP message and the function will set Wrap->TcpWrap.IsRxDone to TRUE to
> break the Tcp polling loop.
> 
> However, HttpTcpReceiveNotifyDpc mistakenly freed Wrap, so the Tcp
> polling loop was actually checking a dead variable, and this led the system
> into an unstable status.
> 
> Given the fact that the HTTP Token Wrap will be freed in EfiHttpResponse or
> HttpResponseWorker, this commit removes every "FreePool (Wrap)" in
> HttpTcpReceiveNotifyDpc.
> 
> Cc: "Wu, Jiaxin" 
> Cc: "Siyuan Fu" 
> Cc: "El-Haj-Mahmoud, Samer" 
> Cc: "Laszlo Ersek" 
> Cc: "Hegde, Nagaraj P" 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Gary Lin 
> ---
>  NetworkPkg/HttpDxe/HttpProto.c | 4 
>  1 file changed, 4 deletions(-)
> 
> diff --git a/NetworkPkg/HttpDxe/HttpProto.c
> b/NetworkPkg/HttpDxe/HttpProto.c index f3992ed..afa7fe4 100644
> --- a/NetworkPkg/HttpDxe/HttpProto.c
> +++ b/NetworkPkg/HttpDxe/HttpProto.c
> @@ -152,7 +152,6 @@ HttpTcpReceiveNotifyDpc (
>  if (EFI_ERROR (Wrap->TcpWrap.Rx6Token.CompletionToken.Status)) {
>Wrap->HttpToken->Status = Wrap-
> >TcpWrap.Rx6Token.CompletionToken.Status;
>gBS->SignalEvent (Wrap->HttpToken->Event);
> -  FreePool (Wrap);
>return ;
>  }
> 
> @@ -162,7 +161,6 @@ HttpTcpReceiveNotifyDpc (
>  if (EFI_ERROR (Wrap->TcpWrap.Rx4Token.CompletionToken.Status)) {
>Wrap->HttpToken->Status = Wrap-
> >TcpWrap.Rx4Token.CompletionToken.Status;
>gBS->SignalEvent (Wrap->HttpToken->Event);
> -  FreePool (Wrap);
>return ;
>  }
>}
> @@ -234,8 +232,6 @@ HttpTcpReceiveNotifyDpc (
>// Check pending RxTokens and receive the HTTP message.
>//
>NetMapIterate (>HttpInstance->RxTokens, HttpTcpReceive, NULL);
> -
> -  FreePool (Wrap);
>  }
> 
>  /**
> --
> 2.8.2

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

Re: [edk2] [patch] MdeModulePkg/DisplayEngine: Fix memory leak issues in DisplayEngine

[Sheng, Cecil (HPS SW)]

Hi Dandan,

The InputHandle.c one looks good. About the ProcessOptions.c one, I propose 
remove the lines starting "FreePool (StringPtr)", the following "if (EFI_ERROR 
(Status))" because it's  useless, and the "return Status" line. Then the 
clean-up block at the end of function frees the strings.


Sincerely,

Cecil Sheng
ISS Firmware Development
HPE Servers


-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Dandan Bi
Sent: Thursday, May 19, 2016 7:30 PM
To: edk2-devel@lists.01.org
Cc: Qiu Shumin ; Eric Dong 
Subject: [edk2] [patch] MdeModulePkg/DisplayEngine: Fix memory leak issues in 
DisplayEngine

Cc: Qiu Shumin 
Cc: Eric Dong 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Dandan Bi 
Reviewed-by: Eric Dong 
---
 MdeModulePkg/Universal/DisplayEngineDxe/InputHandler.c   | 5 +
 MdeModulePkg/Universal/DisplayEngineDxe/ProcessOptions.c | 3 ++-
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Universal/DisplayEngineDxe/InputHandler.c 
b/MdeModulePkg/Universal/DisplayEngineDxe/InputHandler.c
index 732dd2f..8e7b735 100644
--- a/MdeModulePkg/Universal/DisplayEngineDxe/InputHandler.c
+++ b/MdeModulePkg/Universal/DisplayEngineDxe/InputHandler.c
@@ -1,9 +1,9 @@
 /** @file
 Implementation for handling user input from the User Interfaces.
 
-Copyright (c) 2004 - 2015, Intel Corporation. All rights reserved.
+Copyright (c) 2004 - 2016, 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
 
@@ -1297,13 +1297,10 @@ GetSelectionInputPopUp (
   ValueType = 0;
   CurrentOption = NULL;
   ShowDownArrow = FALSE;
   ShowUpArrow   = FALSE;
 
-  StringPtr = AllocateZeroPool ((gOptionBlockWidth + 1) * 2);
-  ASSERT (StringPtr);
-
   ZeroMem (, sizeof (EFI_HII_VALUE));
 
   Question = MenuOption->ThisTag;
   if (Question->OpCode->OpCode == EFI_IFR_ORDERED_LIST_OP) {
 Link = GetFirstNode (>OptionListHead); diff --git 
a/MdeModulePkg/Universal/DisplayEngineDxe/ProcessOptions.c 
b/MdeModulePkg/Universal/DisplayEngineDxe/ProcessOptions.c
index bb2faf3..16aaf6c 100644
--- a/MdeModulePkg/Universal/DisplayEngineDxe/ProcessOptions.c
+++ b/MdeModulePkg/Universal/DisplayEngineDxe/ProcessOptions.c
@@ -1,10 +1,10 @@
 /** @file
 Implementation for handling the User Interface option processing.
 
 
-Copyright (c) 2004 - 2015, Intel Corporation. All rights reserved.
+Copyright (c) 2004 - 2016, 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
 
@@ -884,10 +884,11 @@ PasswordProcess (
 gUserInput->InputValue.Buffer = AllocateCopyPool 
(Question->CurrentValue.BufferLen, StringPtr);
 gUserInput->InputValue.BufferLen = Question->CurrentValue.BufferLen;
 gUserInput->InputValue.Type = Question->CurrentValue.Type;
 gUserInput->InputValue.Value.string = HiiSetString(gFormData->HiiHandle, 
gUserInput->InputValue.Value.string, StringPtr, NULL);
 FreePool (StringPtr); 
+FreePool (TempString);
 
 Status = EFI_SUCCESS;
 
 if (EFI_ERROR (Status)) {
   //
--
1.9.5.msysgit.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 v1 1/1] ShellPkg/App: Fix memory leak and save resources.

[Carsey, Jaben]

Marvin,

I will review your patches this week.

-Jaben

> -Original Message-
> From: Marvin Häuser [mailto:marvin.haeu...@outlook.com]
> Sent: Thursday, May 19, 2016 1:20 PM
> To: edk2-devel@lists.01.org
> Cc: Carsey, Jaben 
> Subject: RE: [edk2] [PATCH v1 1/1] ShellPkg/App: Fix memory leak and save
> resources.
> Importance: High
> 
> Hey Jaben,
> 
> As I mentioned in 3), there is still the CreateFileInterfaceMem() resource 
> leak
> if the list is not empty, so this is definitely still a todo, I do not know 
> how to
> solve (yet?). I only kept the ASSERT() because it was present before, I don't
> really see the point in stopping execution there to be honest.
> 
> By the way, if you have time, please review my other two patches as well:
> [PATCH v1] ShellPkg: Also accept gEfiUnicodeCollation2ProtocolGuid for
> parsing.
> [PATCH v1] ShellPkg/Bcfg: Add support for 'addp' command.
> 
> Thank you for your time!
> 
> Regards,
> Marvin.
> 
> > -Original Message-
> > From: Carsey, Jaben [mailto:jaben.car...@intel.com]
> > Sent: Thursday, May 19, 2016 10:09 PM
> > To: Marvin Häuser ; edk2-
> > de...@lists.01.org
> > Cc: Qiu, Shumin ; Carsey, Jaben
> > 
> > Subject: RE: [edk2] [PATCH v1 1/1] ShellPkg/App: Fix memory leak and save
> > resources.
> >
> > Can we safely remove the ASSERt now?
> >
> > > -Original Message-
> > > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > > Marvin Häuser
> > > Sent: Thursday, May 19, 2016 12:04 PM
> > > To: edk2-devel@lists.01.org
> > > Cc: Carsey, Jaben ; Qiu, Shumin
> > > 
> > > Subject: [edk2] [PATCH v1 1/1] ShellPkg/App: Fix memory leak and save
> > > resources.
> > > Importance: High
> > >
> > > 1) RunSplitCommand() allocates the initial SplitStdOut via
> > >CreateFileInterfaceMem(). Free SplitStdIn after the swap to fix
> > >the memory leak.
> > >
> > > 2) In RunSplitCommand(), SplitStdOut is checked for equality with
> > >StdIn. This cannot happen due to the if-check within the swap.
> > >Hence remove it.
> > >
> > > 3) UefiMain() doesn't free SplitList. Delete all list entries and
> > >reinitialize the list when in DEBUG. This does not include the
> > >CreateFileInterfaceMem()-allocated SplitStd mentioned in 1), so
> > >keep the ASSERT() until resolved.
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.0
> > > Signed-off-by: Marvin Haeuser 
> > > ---
> > >  ShellPkg/Application/Shell/Shell.c | 16 ++--
> > >  1 file changed, 14 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/ShellPkg/Application/Shell/Shell.c
> > > b/ShellPkg/Application/Shell/Shell.c
> > > index 47b3118ea701..a562c9a2c0bc 100644
> > > --- a/ShellPkg/Application/Shell/Shell.c
> > > +++ b/ShellPkg/Application/Shell/Shell.c
> > > @@ -342,6 +342,7 @@ UefiMain (
> > >UINTN   Size;
> > >EFI_HANDLE  ConInHandle;
> > >EFI_SIMPLE_TEXT_INPUT_PROTOCOL  *OldConIn;
> > > +  SPLIT_LIST  *Split;
> > >
> > >if (PcdGet8(PcdShellSupportLevel) > 3) {
> > >  return (EFI_UNSUPPORTED);
> > > @@ -675,7 +676,17 @@ FreeResources:
> > >}
> > >
> > >if (!IsListEmpty()){
> > > -ASSERT(FALSE); ///@todo finish this de-allocation.
> > > +ASSERT(FALSE); ///@todo finish this de-allocation (free
> > > + SplitStdIn/Out
> > > when needed).
> > > +
> > > +for ( Split = (SPLIT_LIST*)GetFirstNode 
> > > ()
> > > +; !IsNull (, >Link)
> > > +; Split = (SPLIT_LIST *)GetNextNode
> > > + (,
> > > >Link)
> > > + ) {
> > > +  RemoveEntryList (>Link);
> > > +  FreePool (Split);
> > > +}
> > > +
> > > +DEBUG_CODE (InitializeListHead
> > > + (););
> > >}
> > >
> > >if (ShellInfoObject.ShellInitSettings.FileName != NULL) { @@
> > > -1736,11 +1747,12 @@ RunSplitCommand(
> > >//
> > >// Note that the original StdIn is now the StdOut...
> > >//
> > > -  if (Split->SplitStdOut != NULL && Split->SplitStdOut != StdIn) {
> > > +  if (Split->SplitStdOut != NULL) {
> > >  ShellInfoObject.NewEfiShellProtocol-
> > > >CloseFile(ConvertShellHandleToEfiFileProtocol(Split->SplitStdOut));
> > >}
> > >if (Split->SplitStdIn != NULL) {
> > >  ShellInfoObject.NewEfiShellProtocol-
> > > >CloseFile(ConvertShellHandleToEfiFileProtocol(Split->SplitStdIn));
> > > +FreePool (Split->SplitStdIn);
> > >}
> > >
> > >FreePool(Split);
> > > --
> > > 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 v1 1/1] ShellPkg/App: Fix memory leak and save resources.

[Carsey, Jaben]

Can we safely remove the ASSERt now?

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Marvin Häuser
> Sent: Thursday, May 19, 2016 12:04 PM
> To: edk2-devel@lists.01.org
> Cc: Carsey, Jaben ; Qiu, Shumin
> 
> Subject: [edk2] [PATCH v1 1/1] ShellPkg/App: Fix memory leak and save
> resources.
> Importance: High
> 
> 1) RunSplitCommand() allocates the initial SplitStdOut via
>CreateFileInterfaceMem(). Free SplitStdIn after the swap to fix
>the memory leak.
> 
> 2) In RunSplitCommand(), SplitStdOut is checked for equality with
>StdIn. This cannot happen due to the if-check within the swap.
>Hence remove it.
> 
> 3) UefiMain() doesn't free SplitList. Delete all list entries and
>reinitialize the list when in DEBUG. This does not include the
>CreateFileInterfaceMem()-allocated SplitStd mentioned in 1), so
>keep the ASSERT() until resolved.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Marvin Haeuser 
> ---
>  ShellPkg/Application/Shell/Shell.c | 16 ++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/ShellPkg/Application/Shell/Shell.c
> b/ShellPkg/Application/Shell/Shell.c
> index 47b3118ea701..a562c9a2c0bc 100644
> --- a/ShellPkg/Application/Shell/Shell.c
> +++ b/ShellPkg/Application/Shell/Shell.c
> @@ -342,6 +342,7 @@ UefiMain (
>UINTN   Size;
>EFI_HANDLE  ConInHandle;
>EFI_SIMPLE_TEXT_INPUT_PROTOCOL  *OldConIn;
> +  SPLIT_LIST  *Split;
> 
>if (PcdGet8(PcdShellSupportLevel) > 3) {
>  return (EFI_UNSUPPORTED);
> @@ -675,7 +676,17 @@ FreeResources:
>}
> 
>if (!IsListEmpty()){
> -ASSERT(FALSE); ///@todo finish this de-allocation.
> +ASSERT(FALSE); ///@todo finish this de-allocation (free SplitStdIn/Out
> when needed).
> +
> +for ( Split = (SPLIT_LIST*)GetFirstNode ()
> +; !IsNull (, >Link)
> +; Split = (SPLIT_LIST *)GetNextNode (,
> >Link)
> + ) {
> +  RemoveEntryList (>Link);
> +  FreePool (Split);
> +}
> +
> +DEBUG_CODE (InitializeListHead (););
>}
> 
>if (ShellInfoObject.ShellInitSettings.FileName != NULL) {
> @@ -1736,11 +1747,12 @@ RunSplitCommand(
>//
>// Note that the original StdIn is now the StdOut...
>//
> -  if (Split->SplitStdOut != NULL && Split->SplitStdOut != StdIn) {
> +  if (Split->SplitStdOut != NULL) {
>  ShellInfoObject.NewEfiShellProtocol-
> >CloseFile(ConvertShellHandleToEfiFileProtocol(Split->SplitStdOut));
>}
>if (Split->SplitStdIn != NULL) {
>  ShellInfoObject.NewEfiShellProtocol-
> >CloseFile(ConvertShellHandleToEfiFileProtocol(Split->SplitStdIn));
> +FreePool (Split->SplitStdIn);
>}
> 
>FreePool(Split);
> --
> 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] [RFC] Structured PCD Proposal

[Kinney, Michael D]

Andrew,

My responses embedded below.

Mike

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Andrew 
> Fish
> Sent: Thursday, May 19, 2016 10:53 AM
> To: Kinney, Michael D ; edk2-devel@lists.01.org 
>  de...@ml01.01.org>
> Subject: Re: [edk2] [RFC] Structured PCD Proposal
> 
> 
> > On May 19, 2016, at 2:26 AM, Laszlo Ersek  wrote:
> >
> > On 05/19/16 01:28, Kinney, Michael D wrote:
> >
> > [snip]
> >
> >> ## C Structure Syntax Proposal
> >>  * Use C typedef syntax to describe complex C structures
> >>  * Use a C language parser with support for struct/union/array/bit 
> >> fields/pack
> >>  * Recommend use of libclang C language parser into Abstract Syntax Tree 
> >> (AST)
> >>- Included with LLVM release
> >>- http://llvm.org/releases/download.html
> >>- http://clang.llvm.org/doxygen/group__CINDEX.html
> >>  * Recommend use of Python bindings for CLANG for EDK II Python based 
> >> BaseTools
> >>- pip install clang
> >
> > What versions are necessary? On RHEL-7, the following versions are
> > available (from EPEL only; RHEL does not provide clang):
> >
> > clang-devel: 3.4.2-8.el7
> > python-pip: 7.1.0-1.el7
> >
> 
> Will it be possible to check everything into the edk2 git tree? Currently we 
> only
> require the developer to have Xcode installed and everything else is in the 
> git tree.
> I'm not sure it is possible to run pip on our production build servers?

Is clang and clang libs included in your environment?  That is not edk2 git 
today.
The only other component is the Python wrapper on top of the clang lib and that
is available from another github project if you want to pull python sources to 
it.

> 
> 
> >>
> >> ## DEC File Extensions
> >>  * Declare link between PCD and C data structure and token range for fields
> >>  * @Include path to include file with C typedef [Required]
> >>  * Replace |VOID*| with name of C typedef
> >>  * @TokenMap path to file with token number assignments [Optional]
> >>  * Range of token numbers to assign to fields in C typedef [Required]
> >>  * Example:
> >>
> >> ```
> >> # @Include  Include/Pcd/AcpiLowerPowerIdleTable.h
> >> # @TokenMap Include/Pcd/AcpiLowerPowerIdleTable.map
> >>
> gPlatformTokenSpaceGuid.AcpiLowPowerIdleTable|{}|ACPI_LOW_POWER_IDLE_TABLE|0x00010080
> |0x0E000200-0x0E0002FF
> >> ```
> >
> > What does 0x00010080 mean here?
> >
> >>
> >>  * Recommended File Paths
> >>- /Include/Pcd/.h
> >>- /Include/Pcd/.map  [Optional]
> >>- /Include/Pcd/.uni  [Optional]
> >>  * C Pre-Processor Support
> >>- Use of #include, #define, #if supported
> >>- #include limited to files in same package
> >>- Including files from other packages being evaluated
> >>
> 
> I think you need to share the C headers with the edk2 C and ASL code, so you 
> should
> have full access including all the things set in the [BuildOptions] flags in 
> the DSC.
> So use something like PP_FLAGS or ASLPP_FLAGS and allow customization via the
> Conf/build_rule.txt. I'd rather we not makeup a new thing and just use one of 
> the
> existing pre processor schemes.

I agree.  Any flags set up in [BuildOptions] or -D flags on command line would 
be
passed in, so the .h file associated with the Structured PCD is treated the same
as any other .h file used to build modules/libs.  There is no intention to 
create
any new build rules to support Structured PCDs.

> 
> IMHO it will be easy enough to follow the proposed restrictive rules when you 
> are
> starting from scratch, but I'm more worried about the case when you have an 
> existing
> (very complex) include infrastructure that supports feature X in C and ASL 
> and you
> decide to add a PCD for X. You don't want it to not work due to more 
> restrictive
> rules around the PCD.

A .h file can have lots of content in it.  Only the typedef structure name 
referenced
in the DEC file declaration has to follow any of the restrictions associated 
with
a Structured PCD.  The C Preprocessor statements above are there to show that 
there
.h file for a Structured PCD can use the same C Preprocessor directives that 
the rest
of the C code in modules/libs can use.  The biggest restriction is in the data 
types
of the leaf fields of the structures.  Those have to be compatible with the PCD
get/set APIs.

> 
> >> ## C Structure Syntax
> >>  * Support struct/union/array with nesting
> >>  * Support bit fields
> >
> > Bit fields will require new PcdLib interaces, right?
> >
> 
> Yikes bit fileds. My over all feedback is this feature seems very complex and
> designed by committee (too many features). The complexity already seems to be
> introducing restrictions and will possible make it harder for developers to
> understand.

The goal was to allow use of C syntax for a typedef struct with as few 
restrictions
as possible, so exiting C structs associated with VOID* PCDs could be used "as 
is".
Yes.  Supporting full C syntax 

[edk2] [PATCH v1 1/1] ShellPkg/App: Fix memory leak and save resources.

[Marvin Häuser]

1) RunSplitCommand() allocates the initial SplitStdOut via
   CreateFileInterfaceMem(). Free SplitStdIn after the swap to fix
   the memory leak.

2) In RunSplitCommand(), SplitStdOut is checked for equality with
   StdIn. This cannot happen due to the if-check within the swap.
   Hence remove it.

3) UefiMain() doesn't free SplitList. Delete all list entries and
   reinitialize the list when in DEBUG. This does not include the
   CreateFileInterfaceMem()-allocated SplitStd mentioned in 1), so
   keep the ASSERT() until resolved.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Marvin Haeuser 
---
 ShellPkg/Application/Shell/Shell.c | 16 ++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/ShellPkg/Application/Shell/Shell.c 
b/ShellPkg/Application/Shell/Shell.c
index 47b3118ea701..a562c9a2c0bc 100644
--- a/ShellPkg/Application/Shell/Shell.c
+++ b/ShellPkg/Application/Shell/Shell.c
@@ -342,6 +342,7 @@ UefiMain (
   UINTN   Size;
   EFI_HANDLE  ConInHandle;
   EFI_SIMPLE_TEXT_INPUT_PROTOCOL  *OldConIn;
+  SPLIT_LIST  *Split;
 
   if (PcdGet8(PcdShellSupportLevel) > 3) {
 return (EFI_UNSUPPORTED);
@@ -675,7 +676,17 @@ FreeResources:
   }
 
   if (!IsListEmpty()){
-ASSERT(FALSE); ///@todo finish this de-allocation.
+ASSERT(FALSE); ///@todo finish this de-allocation (free SplitStdIn/Out 
when needed).
+
+for ( Split = (SPLIT_LIST*)GetFirstNode ()
+; !IsNull (, >Link)
+; Split = (SPLIT_LIST *)GetNextNode (, 
>Link)
+ ) {
+  RemoveEntryList (>Link);
+  FreePool (Split);
+}
+
+DEBUG_CODE (InitializeListHead (););
   }
 
   if (ShellInfoObject.ShellInitSettings.FileName != NULL) {
@@ -1736,11 +1747,12 @@ RunSplitCommand(
   //
   // Note that the original StdIn is now the StdOut...
   //
-  if (Split->SplitStdOut != NULL && Split->SplitStdOut != StdIn) {
+  if (Split->SplitStdOut != NULL) {
 
ShellInfoObject.NewEfiShellProtocol->CloseFile(ConvertShellHandleToEfiFileProtocol(Split->SplitStdOut));
   }
   if (Split->SplitStdIn != NULL) {
 
ShellInfoObject.NewEfiShellProtocol->CloseFile(ConvertShellHandleToEfiFileProtocol(Split->SplitStdIn));
+FreePool (Split->SplitStdIn);
   }
 
   FreePool(Split);
-- 
2.7.4.windows.1

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

Re: [edk2] [RFC] Proposal to organize packages into directories

[Paolo Bonzini]

On 19/05/2016 20:21, Kinney, Michael D wrote:
> Paolo,
> 
> For this proposal, I am not only looking at existing packages/modules/libs, I 
> am also considering where new content might go over time.  For example, the
> MdeModulePkg has grown in size over the years.  The PEI Core, DXE Core, SMM
> Core content in MdeModulePkg is "Core" content, but much of the other content
> in that package would really fall into the "Drivers" category.  Instead of 
> adding more content to MdeModulePkg in the future, we should consider adding 
> it to "Drivers" instead.
> 
> There have also been questions on edk2-devel on where to add vendor specific
> drivers for devices/peripherals that attach to PCI/USB/I2C/etc. "Drivers" 
> seems like a better location than "Silicon".
> 
> We could even consider in the future figuring out ways to move some of the 
> content out packages like MdeModulePkg into "Drivers".

That would be even better. :)

Paolo

> So instead of removing "Drivers" from the proposal because there are only a
> couple packages in that category today, let's keep it and make sure we 
> add new content to the right directory going forward.
> 
> Mike
> 
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Paolo 
>> Bonzini
>> Sent: Thursday, May 19, 2016 10:21 AM
>> To: Kinney, Michael D ; Ryan Harkin
>> 
>> Cc: edk2-devel@lists.01.org 
>> Subject: Re: [edk2] [RFC] Proposal to organize packages into directories
>>
>>
>>
>> On 19/05/2016 18:21, Kinney, Michael D wrote:
>>> This is one of the reasons I wanted to have both a "Silicon" and a "Driver"
>>> top level directory.
>>>
>>> We can change names, but the idea is that the "Silicon" one would contains
>>> CPU/Chipset/SoC content that is usually contains the drivers to init the CPU
>>> complex, turn on caches, enable memory subsystems, and do basic init of the
>>> I/O subsystems built into the CPU/Chipset/SoC.
>>>
>>> The "Drivers" area is for modules that manage hardware and peripherals that
>>> attach to standard I/O subsystems such as PCI, USB, I2C, etc.
>>
>> Okay, that makes sense.  Indeed they are different, for example outside
>> Silicon/ the architecture is not particularly important.
>>
>> At the same time, with Drivers/ reduced to just three packages, I wonder
>> if it would make sense to move FatPkg and NetworkPkg to Core/, and
>> OptionRomPkg to Silicon/.  Most drivers are part of MdeModulePkg, which
>> is in Core/.
>>
>> For Silicon/, I think //FooPkg (which can be
>> reduced to /FooPkg or even just FooPkg) can be more useful
>> than Vendor//FooPkg.  This would give
>>
>> Silicon/
>>   Arm/   (architecture)
>> ArmPkg
>> ArmPlatformPkg
>> ArmVirtPkg
>> Arm/ (vendor)
>>   ArmJunoPkg (currently in ArmPlatformPkg/ArmJunoPkg)
>>   ArmVExpressPkg (currently in ArmPlatformPkg/ArmVExpressPkg)
>>   Omap35xxPkg
>>   IA32X64/
>> CorebootModulePkg
>> PcAtChipsetPkg
>> UefiCpuPkg
>> Intel/
>>   QuarkSocPkg
>>   Vlv2DeviceRefCodePkg
>>   OptionRomPkg
>>
>>> If we have an area for CPU/Chipset/SoC, then we can use some directory paths
>>> that clearly identify CPU architecture content as well as a Vendor specific
>>> content.
>>>
>>> I would hope we can concentrate the CPU architecture content that applies to
>>> all vendors in its own area, and only the vendor specific extensions go into
>>> vendor specific directories.
>>
>> This sounds good!
>>
>> Thanks,
>>
>> Paolo
>> ___
>> 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] [RFC] Proposal to organize packages into directories

[Kinney, Michael D]

Paolo,

For this proposal, I am not only looking at existing packages/modules/libs, I 
am also considering where new content might go over time.  For example, the
MdeModulePkg has grown in size over the years.  The PEI Core, DXE Core, SMM
Core content in MdeModulePkg is "Core" content, but much of the other content
in that package would really fall into the "Drivers" category.  Instead of 
adding more content to MdeModulePkg in the future, we should consider adding 
it to "Drivers" instead.

There have also been questions on edk2-devel on where to add vendor specific
drivers for devices/peripherals that attach to PCI/USB/I2C/etc. "Drivers" 
seems like a better location than "Silicon".

We could even consider in the future figuring out ways to move some of the 
content out packages like MdeModulePkg into "Drivers".

So instead of removing "Drivers" from the proposal because there are only a
couple packages in that category today, let's keep it and make sure we 
add new content to the right directory going forward.

Mike

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Paolo 
> Bonzini
> Sent: Thursday, May 19, 2016 10:21 AM
> To: Kinney, Michael D ; Ryan Harkin
> 
> Cc: edk2-devel@lists.01.org 
> Subject: Re: [edk2] [RFC] Proposal to organize packages into directories
> 
> 
> 
> On 19/05/2016 18:21, Kinney, Michael D wrote:
> > This is one of the reasons I wanted to have both a "Silicon" and a "Driver"
> > top level directory.
> >
> > We can change names, but the idea is that the "Silicon" one would contains
> > CPU/Chipset/SoC content that is usually contains the drivers to init the CPU
> > complex, turn on caches, enable memory subsystems, and do basic init of the
> > I/O subsystems built into the CPU/Chipset/SoC.
> >
> > The "Drivers" area is for modules that manage hardware and peripherals that
> > attach to standard I/O subsystems such as PCI, USB, I2C, etc.
> 
> Okay, that makes sense.  Indeed they are different, for example outside
> Silicon/ the architecture is not particularly important.
> 
> At the same time, with Drivers/ reduced to just three packages, I wonder
> if it would make sense to move FatPkg and NetworkPkg to Core/, and
> OptionRomPkg to Silicon/.  Most drivers are part of MdeModulePkg, which
> is in Core/.
> 
> For Silicon/, I think //FooPkg (which can be
> reduced to /FooPkg or even just FooPkg) can be more useful
> than Vendor//FooPkg.  This would give
> 
> Silicon/
>   Arm/   (architecture)
> ArmPkg
> ArmPlatformPkg
> ArmVirtPkg
> Arm/ (vendor)
>   ArmJunoPkg (currently in ArmPlatformPkg/ArmJunoPkg)
>   ArmVExpressPkg (currently in ArmPlatformPkg/ArmVExpressPkg)
>   Omap35xxPkg
>   IA32X64/
> CorebootModulePkg
> PcAtChipsetPkg
> UefiCpuPkg
> Intel/
>   QuarkSocPkg
>   Vlv2DeviceRefCodePkg
>   OptionRomPkg
> 
> > If we have an area for CPU/Chipset/SoC, then we can use some directory paths
> > that clearly identify CPU architecture content as well as a Vendor specific
> > content.
> >
> > I would hope we can concentrate the CPU architecture content that applies to
> > all vendors in its own area, and only the vendor specific extensions go into
> > vendor specific directories.
> 
> This sounds good!
> 
> Thanks,
> 
> Paolo
> ___
> 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] [RFC] Structured PCD Proposal

[Andrew Fish]

> On May 19, 2016, at 2:26 AM, Laszlo Ersek  wrote:
> 
> On 05/19/16 01:28, Kinney, Michael D wrote:
> 
> [snip]
> 
>> ## C Structure Syntax Proposal
>>  * Use C typedef syntax to describe complex C structures
>>  * Use a C language parser with support for struct/union/array/bit 
>> fields/pack
>>  * Recommend use of libclang C language parser into Abstract Syntax Tree 
>> (AST)
>>- Included with LLVM release
>>- http://llvm.org/releases/download.html
>>- http://clang.llvm.org/doxygen/group__CINDEX.html
>>  * Recommend use of Python bindings for CLANG for EDK II Python based 
>> BaseTools
>>- pip install clang
> 
> What versions are necessary? On RHEL-7, the following versions are
> available (from EPEL only; RHEL does not provide clang):
> 
> clang-devel: 3.4.2-8.el7
> python-pip: 7.1.0-1.el7
> 

Will it be possible to check everything into the edk2 git tree? Currently we 
only require the developer to have Xcode installed and everything else is in 
the git tree. I'm not sure it is possible to run pip on our production build 
servers?  


>> 
>> ## DEC File Extensions
>>  * Declare link between PCD and C data structure and token range for fields
>>  * @Include path to include file with C typedef [Required]
>>  * Replace |VOID*| with name of C typedef
>>  * @TokenMap path to file with token number assignments [Optional]
>>  * Range of token numbers to assign to fields in C typedef [Required]
>>  * Example:
>> 
>> ```
>> # @Include  Include/Pcd/AcpiLowerPowerIdleTable.h
>> # @TokenMap Include/Pcd/AcpiLowerPowerIdleTable.map
>> gPlatformTokenSpaceGuid.AcpiLowPowerIdleTable|{}|ACPI_LOW_POWER_IDLE_TABLE|0x00010080|0x0E000200-0x0E0002FF
>> ```
> 
> What does 0x00010080 mean here?
> 
>> 
>>  * Recommended File Paths
>>- /Include/Pcd/.h
>>- /Include/Pcd/.map  [Optional]
>>- /Include/Pcd/.uni  [Optional]
>>  * C Pre-Processor Support
>>- Use of #include, #define, #if supported
>>- #include limited to files in same package
>>- Including files from other packages being evaluated
>> 

I think you need to share the C headers with the edk2 C and ASL code, so you 
should have full access including all the things set in the [BuildOptions] 
flags in the DSC. So use something like PP_FLAGS or ASLPP_FLAGS and allow 
customization via the Conf/build_rule.txt. I'd rather we not makeup a new thing 
and just use one of the existing pre processor schemes. 

IMHO it will be easy enough to follow the proposed restrictive rules when you 
are starting from scratch, but I'm more worried about the case when you have an 
existing (very complex) include infrastructure that supports feature X in C and 
ASL and you decide to add a PCD for X. You don't want it to not work due to 
more restrictive rules around the PCD. 

>> ## C Structure Syntax
>>  * Support struct/union/array with nesting
>>  * Support bit fields
> 
> Bit fields will require new PcdLib interaces, right?
> 

Yikes bit fileds. My over all feedback is this feature seems very complex and 
designed by committee (too many features). The complexity already seems to be 
introducing restrictions and will possible make it harder for developers to 
understand. 

I'll take a quick stab at simplification. 
1) Add the new syntax to a new file type: *.pcd. This file would contain [Pcd*] 
sections and the C like syntax. 
2) Don't support default values in the .dec with the C like syntax. Recommend 
no defaults  and a build break if a PCD is not set by the platform build for 
complex structures. 
3) Use the C pre processor with PP_FLAGS or ASLPP_FLAGS to handle the include 
infrastructure for maximum compatibility. Just like other rules in 
Conf/build_rule.txt. ?.pcd could be added to build_rule.txt.  
4) Don't add new PcdLib APIs. Return a pointer to the structure and use C code 
to access the data. 
5) OK someone is going to complain about type checking so add a new version of 
PcdGetPtr() that casts the pointer to the C pointer type returned. The type is 
in the AutoGen.h file (_PCD_GET_PTR_TYPE_##TokenName). If you don't use the new 
.pcd file syntax then you get a build break. 
6) Don't be too restrictive on type checking. Support something like a device 
path. For example the globals in PlatformData.c of PlatformBdsLib could be PCD 
entries. This could be phase 2, and require a modified syntax if needed. 

This simpler scheme only requires initializing a global variable and being able 
to extract that information vs AST. The real value in this scheme is unifying 
the include infrastructure for C, ASL, and PCD. C can already access C 
structure members, seems like reinventing the wheel to come up with a complex 
scheme to abstract how to access C structure elements when a C pointer solves 
that problem. 

"The real art is knowing what to leave out, not what to put in" quote from 
Steve Jobs. 

Thanks,

Andrew Fish

___
edk2-devel mailing list
edk2-devel@lists.01.org

Re: [edk2] [RFC] Structured PCD Proposal

[Kinney, Michael D]

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
> Ersek
> Sent: Thursday, May 19, 2016 2:26 AM
> To: Kinney, Michael D 
> Cc: edk2-devel@lists.01.org 
> Subject: Re: [edk2] [RFC] Structured PCD Proposal
> 
> On 05/19/16 01:28, Kinney, Michael D wrote:
> 
> [snip]
> 
> > ## C Structure Syntax Proposal
> >   * Use C typedef syntax to describe complex C structures
> >   * Use a C language parser with support for struct/union/array/bit 
> > fields/pack
> >   * Recommend use of libclang C language parser into Abstract Syntax Tree 
> > (AST)
> > - Included with LLVM release
> > - http://llvm.org/releases/download.html
> > - http://clang.llvm.org/doxygen/group__CINDEX.html
> >   * Recommend use of Python bindings for CLANG for EDK II Python based 
> > BaseTools
> > - pip install clang
> 
> What versions are necessary? On RHEL-7, the following versions are
> available (from EPEL only; RHEL does not provide clang):
> 
> clang-devel: 3.4.2-8.el7
> python-pip: 7.1.0-1.el7

I have not tried the prototype on RHEL-7 yet.  For Fedora 23, we have used:
 
  $ sudo dnf install clang
  $ git clone -b release_37 https://github.com/trolldbois/python-clang
  $ cd python-clang
  $ sudo python setup.py install

> 
> >
> > ## DEC File Extensions
> >   * Declare link between PCD and C data structure and token range for fields
> >   * @Include path to include file with C typedef [Required]
> >   * Replace |VOID*| with name of C typedef
> >   * @TokenMap path to file with token number assignments [Optional]
> >   * Range of token numbers to assign to fields in C typedef [Required]
> >   * Example:
> >
> > ```
> > # @Include  Include/Pcd/AcpiLowerPowerIdleTable.h
> > # @TokenMap Include/Pcd/AcpiLowerPowerIdleTable.map
> >
> gPlatformTokenSpaceGuid.AcpiLowPowerIdleTable 
> |{}|ACPI_LOW_POWER_IDLE_TABLE|0x00010080
> |0x0E000200-0x0E0002FF
> > ```
> 
> What does 0x00010080 mean here?

That is the token number for the PCD 
gPlatformTokenSpaceGuid.AcpiLowPowerIdleTable.
That is the same syntax that is used for all PCDs today.  In the DEC file all
PCDs are assigned a token number.  For a Structured PCD, we have added the 5th
Parameter on this PCD declaration line that is the range of token number that 
Are assigned to fields within the Structured PCD.  We want to keep the token
number for the PCD declaration in the DEC file for backwards compatibility. 

For example, the following PCD declared in MdeModulePkg/MdeModulePkg.dec:

  ## PCI Serial Device Info. It is an array of Device, Function, and Power 
Management
  #  information that describes the path that contains zero or more PCI to PCI 
briges
  #  followed by a PCI serial device.  Each array entry is 4-bytes in length.  
The
  #  first byte is the PCI Device Number, then second byte is the PCI Function 
Number,
  #  and the last two bytes are the offset to the PCI power management 
capabilities
  #  register used to manage the D0-D3 states.  If a PCI power management 
capabilities
  #  register is not present, then the last two bytes in the offset is set to 
0.  The
  #  array is terminated by an array entry with a PCI Device Number of 0xFF.  
For a
  #  non-PCI fixed address serial device, such as an ISA serial device, the 
value is 0xFF.
  # @Prompt Pci Serial Device Info
  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialPciDeviceInfo|{0xFF}|VOID*|0x00010067

If we convert this VOID* PCD to a Structured PCD, the declaration could be 
updated to:

  ## PCI Serial Device Info. It is an array of Device, Function, and Power 
Management
  #  information that describes the path that contains zero or more PCI to PCI 
briges
  #  followed by a PCI serial device.  Each array entry is 4-bytes in length.  
The
  #  first byte is the PCI Device Number, then second byte is the PCI Function 
Number,
  #  and the last two bytes are the offset to the PCI power management 
capabilities
  #  register used to manage the D0-D3 states.  If a PCI power management 
capabilities
  #  register is not present, then the last two bytes in the offset is set to 
0.  The
  #  array is terminated by an array entry with a PCI Device Number of 0xFF.  
For a
  #  non-PCI fixed address serial device, such as an ISA serial device, the 
value is 0xFF.
  # @Prompt Pci Serial Device Info
  # @Include MdeModulePkg/Include/Pcd/SerialPciDeviceInfo.h
  
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialPciDeviceInfo|{}|VOID*|0x00010067|0xF000-0xF010

MdeModulePkg/Include/Pcd/SerialPciDeviceInfo.h
==
typedef struct {
  struct {
// @Prompt PCI Device Number
// @DefaultValue 0xFF
UINT8   DeviceNumber;
// @Prompt PCI Function Number
UINT8   FunctionNumber;
// @Prompt PCI Power Management Capabilities Register Offset
UINT16  PciPowerManagementCapabilitiesOffset;
  } PciDeviceInfo[0];
} PCD_SERIAL_PCI_DEVICE_INFO;

Notice that the token 

Re: [edk2] [RFC] Proposal to organize packages into directories

[Paolo Bonzini]

On 19/05/2016 18:21, Kinney, Michael D wrote:
> This is one of the reasons I wanted to have both a "Silicon" and a "Driver" 
> top level directory.
> 
> We can change names, but the idea is that the "Silicon" one would contains 
> CPU/Chipset/SoC content that is usually contains the drivers to init the CPU
> complex, turn on caches, enable memory subsystems, and do basic init of the 
> I/O subsystems built into the CPU/Chipset/SoC.
> 
> The "Drivers" area is for modules that manage hardware and peripherals that 
> attach to standard I/O subsystems such as PCI, USB, I2C, etc.

Okay, that makes sense.  Indeed they are different, for example outside
Silicon/ the architecture is not particularly important.

At the same time, with Drivers/ reduced to just three packages, I wonder
if it would make sense to move FatPkg and NetworkPkg to Core/, and
OptionRomPkg to Silicon/.  Most drivers are part of MdeModulePkg, which
is in Core/.

For Silicon/, I think //FooPkg (which can be
reduced to /FooPkg or even just FooPkg) can be more useful
than Vendor//FooPkg.  This would give

Silicon/
  Arm/   (architecture)
ArmPkg
ArmPlatformPkg
ArmVirtPkg
Arm/ (vendor)
  ArmJunoPkg (currently in ArmPlatformPkg/ArmJunoPkg)
  ArmVExpressPkg (currently in ArmPlatformPkg/ArmVExpressPkg)
  Omap35xxPkg
  IA32X64/
CorebootModulePkg
PcAtChipsetPkg
UefiCpuPkg
Intel/
  QuarkSocPkg
  Vlv2DeviceRefCodePkg
  OptionRomPkg

> If we have an area for CPU/Chipset/SoC, then we can use some directory paths 
> that clearly identify CPU architecture content as well as a Vendor specific 
> content.
> 
> I would hope we can concentrate the CPU architecture content that applies to
> all vendors in its own area, and only the vendor specific extensions go into 
> vendor specific directories.

This sounds good!

Thanks,

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

Re: [edk2] Acquire the permission to commit code in edk2-staging tree

[Kinney, Michael D]

Hi Abner,

I see you have set up a fork of edk2-staging at:

https://github.com/AbnerChang/edk2-staging

Can you add your new RISC-V branch in this fork first?  This will
give you a chance to prepare content in your own fork that follows
the edk2 process.

One of the maintainers, such as Jordan or Liming, can review the 
content in your fork and provide feedback if changes are required 
related to edk2 process.  When content follows edk2 process, the 
maintainers can sync content from your branch in  

https://github.com/AbnerChang/edk2-staging

to the same branch name in 

https://github.com/tianocore/edk2-staging 

to make the RISC-V content directly available from tianocore repos.

Thanks,

Mike

> -Original Message-
> From: Chang, Abner (HPS SW/FW Technologist) [mailto:abner.ch...@hpe.com]
> Sent: Thursday, May 19, 2016 6:47 AM
> To: Gao, Liming ; Justen, Jordan L 
> ;
> Kinney, Michael D 
> Cc: edk2-devel@lists.01.org
> Subject: RE: Acquire the permission to commit code in edk2-staging tree
> 
> So, I think I can be the owner at the beginning, says I can own the RISC-V 
> package
> and keep to sync with EDK2. However, I think it would be better to have the 
> co-owner,
> probably Jordan or Jiewen.
> I am not really clear about the entire process of edk2 code commitment, just 
> my first
> priority for RISC-V edk2 port is to commit all of RISC-V code to edk2-stagign 
> tree.
> Then RISC-V edk2 port is visible to all of RISC-V people.
> Thanks
> Abner
> 
> -Original Message-
> From: Gao, Liming [mailto:liming@intel.com]
> Sent: Thursday, May 19, 2016 6:14 PM
> To: Justen, Jordan L ; Chang, Abner (HPS SW/FW
> Technologist) ; Kinney, Michael D 
> 
> Cc: edk2-devel@lists.01.org
> Subject: RE: Acquire the permission to commit code in edk2-staging tree
> 
> Jordan:
>   I think the package maintainer has the access. The staging branch owner may 
> not
> have. So, if the contributor has no access, he needs to ask help for the 
> package
> maintainer. If it is TRUE, could you help update README to document it?
> 
> Thanks
> Liming
> > -Original Message-
> > From: Justen, Jordan L
> > Sent: Thursday, May 19, 2016 11:50 AM
> > To: Gao, Liming ; Chang, Abner (HPS SW/FW
> > Technologist) ; Kinney, Michael D
> > 
> > Cc: edk2-devel@lists.01.org
> > Subject: RE: Acquire the permission to commit code in edk2-staging
> > tree
> >
> > On 2016-05-18 18:13:18, Gao, Liming wrote:
> > > Jordan and Mike:
> > >   For new package and platform, I think we can grant access to new
> > >   owner. Do you think so?
> > >
> >
> > When discussing the staging process, I thought that staging branches
> > would have package maintainer sponsors that would own sync'ing updates
> > of the staging branch:
> >
> > http://thread.gmane.org/gmane.comp.bios.edk2.devel/8964/focus=8973
> >
> > I'll admit, that doesn't seem to be clear in the current README:
> >
> > https://raw.githubusercontent.com/tianocore/edk2-staging/about/README
> >
> > Does maintainer refer to the package maintainer sponsor, or the main
> > staging branch developer.
> >
> > Anyway, I would be willing to work with Abner, unless we decide we
> > should handle it another way.
> >
> > -Jordan
> >
> > >
> > > -Original Message-
> > > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
> > > Of
> > Chang, Abner (HPS SW/FW Technologist)
> > > Sent: Wednesday, May 18, 2016 12:01 PM
> > > To: Kinney, Michael D ; Justen, Jordan L
> > 
> > > Cc: edk2-devel@lists.01.org
> > > Subject: [edk2] Acquire the permission to commit code in
> > > edk2-staging tree
> > >
> > > Hi Michael and Jordan,
> > > I'm going to commit RISC-V to edk2-staging tree, however, I don't
> > > have the
> > permission to modify that repository.
> > > Could you please help to give me the permission?
> > > Thanks
> > > Abner
> > > ___
> > > 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] [RFC] Proposal to organize packages into directories

[Kinney, Michael D]

Paolo,

This is one of the reasons I wanted to have both a "Silicon" and a "Driver" 
top level directory.

We can change names, but the idea is that the "Silicon" one would contains 
CPU/Chipset/SoC content that is usually contains the drivers to init the CPU
complex, turn on caches, enable memory subsystems, and do basic init of the 
I/O subsystems built into the CPU/Chipset/SoC.

The "Drivers" area is for modules that manage hardware and peripherals that 
attach to standard I/O subsystems such as PCI, USB, I2C, etc.

If we have an area for CPU/Chipset/SoC, then we can use some directory paths 
that clearly identify CPU architecture content as well as a Vendor specific 
content.

I would hope we can concentrate the CPU architecture content that applies to
all vendors in its own area, and only the vendor specific extensions go into 
vendor specific directories.

Mike


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Paolo 
> Bonzini
> Sent: Thursday, May 19, 2016 9:06 AM
> To: Ryan Harkin 
> Cc: Kinney, Michael D ; edk2-devel@lists.01.org 
>  de...@ml01.01.org>
> Subject: Re: [edk2] [RFC] Proposal to organize packages into directories
> 
> 
> 
> On 19/05/2016 18:03, Ryan Harkin wrote:
> > > IA32X64 is not a great name, but neither is Intel.  X86 suggests 32-bit
> > > only.
> >
> > I prefer the idea of separating by vendor.  One vendor may have
> > multiple architectures, for example.
> 
> That's exactly why I want to separate by architecture. :)
> 
> When doing a change across the entire tree, it's way more common to care
> about 'all ARM boards' than 'all SoCs from Qualcomm'.
> 
> Thanks,
> 
> Paolo
> ___
> 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] [RFC] Proposal to organize packages into directories

[Paolo Bonzini]

On 19/05/2016 18:03, Ryan Harkin wrote:
> > IA32X64 is not a great name, but neither is Intel.  X86 suggests 32-bit
> > only.
>
> I prefer the idea of separating by vendor.  One vendor may have
> multiple architectures, for example.

That's exactly why I want to separate by architecture. :)

When doing a change across the entire tree, it's way more common to care
about 'all ARM boards' than 'all SoCs from Qualcomm'.

Thanks,

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

Re: [edk2] [RFC] Proposal to organize packages into directories

[Ryan Harkin]

On 19 May 2016 at 14:35, Paolo Bonzini  wrote:
>
>
> On 18/05/2016 01:57, Kinney, Michael D wrote:
>>   Core
>> CorebootModulePkg
>> CorebootPayloadPkg
>
> I think that anything with a .fdf file should be under Platform.
> CorebootPayloadPkg is the only outlier in your proposal.
>
>> Emulated
>>   DuetPkg
>>   EmulatorPkg
>>   Nt32Pkg
>>   OvmfPkg
>
> I think OvmfPkg is not emulated; certainly not in the same sense as
> EmulatorPkg, Nt32 or UnixPkg.  DuetPkg also seems more similar to
> OvmfPkg than to EmulatorPkg (and definitely most similar to
> CorebootPayloadPkg, which should be under Platform according to the rule
> I proposed above).
>
> In addition, I think that separation by architecture is more useful than
> separation by vendor.  This yields the following:
>
> Platform
>   Arm
> ArmPlatformPkg
> ArmVirtPkg
> BeagleBoardPkg
>   Emulated
> EmulatorPkg
> Nt32Pkg
> UnixPkg
>   IA32X64
> CorebootPayloadPkg
> DuetPkg
> Intel
>   QuarkPlatformPkg
>   Vlv2TbltDevicePkg
> OvmfPkg
>
> IA32X64 is not a great name, but neither is Intel.  X86 suggests 32-bit
> only.
>

I prefer the idea of separating by vendor.  One vendor may have
multiple architectures, for example.

But I'm also not keen on the "Vendor" sub-dir itself, it seems redundant:

  Platform/Vendor/ARM
vs
  Platform/ARM

OK, so the emulated platforms don't have a "vendor" as such, but
"Emulated" could be as good a name as any.


> In addition, I am not sure about the separation between "Drivers" and
> "Silicon".  My proposal here is to unify them as follows:
>
> Drivers
>   FatPkg
>   NetworkPkg
>   OptionRomPkg
>   Arm
> ArmPkg
> Omap35xxPkg
>   IA32X64
> PcAtChipsetPkg
> QuarkSocPkg
> UefiCpuPkg
> Vlv2DeviceRefCodePkg
>
> or alternatively Omap35xxPkg, QuarkSocPkg and Vlv2DeviceRefCode could
> move under Drivers/Vendor/{Arm,Intel}.
>
> Thanks,
>
> Paolo
> ___
> 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] Using the Shell to launch a kernel using a RAMDISK

[El-Haj-Mahmoud, Samer]

Yes this works, A simple checkbox in the HII form before starting File Explorer


-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
Ersek
Sent: Thursday, May 19, 2016 2:49 AM
To: Tian, Feng ; El-Haj-Mahmoud, Samer 
; Andrew Fish ; Foster, Matthew 
I 
Cc: Wu, Hao A ; edk2-devel@lists.01.org 

Subject: Re: [edk2] Using the Shell to launch a kernel using a RAMDISK

On 05/19/16 04:41, Tian, Feng wrote:
> Samer,
> 
> I am ok to add an item/option to allow user choose ramdisk memory 
> type. But I don't know how to organize the form to show that 
> straightforwardly, for example, how to add an item for those chosen by 
> File Explorer. Look forward to see your patch:)

* I think on the first form that lists

  > Create raw
  > Create from file

There could be a persistent checkbox above these options, called

  Make next created RAM disk bootable [ ]

* Alternatively, given that there aren't too many options yet, it shouldn't be 
a problem duplicating them:

  > Create raw (boot time only use)
  > Create raw (usable at runtime, bootable)
  > Create from file (boot time only use)
  > Create from file (usable at runtime, bootable)

* Whichever of the above two is chosen, it would be nice (if possible) to 
display under

  Created RAM disk list:

which RAM disks live in reserved memory and which don't. (This should really be 
optional though.)

Just some ideas, of course.

Thanks!
Laszlo


> -Original Message-
> From: El-Haj-Mahmoud, Samer [mailto:samer.el-haj-mahm...@hpe.com]
> Sent: Wednesday, May 18, 2016 10:10 PM
> To: Laszlo Ersek ; Andrew Fish ; 
> Foster, Matthew I ; Tian, Feng 
> 
> Cc: edk2-devel@lists.01.org ; El-Haj-Mahmoud, 
> Samer 
> Subject: RE: [edk2] Using the Shell to launch a kernel using a RAMDISK
> 
> Thanks Laszlo!
> 
> I missed that thread.
> 
> I did not participate in the original design either (where is the original 
> design anyway? Is it public on the EDK2 list)?
> 
> Feng,
> 
> Do you mind if I submit a patch to update the HII to support allocating 
> Reserved memory RAM Disks? This will help a lot in troubleshooting.
> 
> Thanks,
> --Samer
> 
> 
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> Laszlo Ersek
> Sent: Wednesday, May 18, 2016 9:04 AM
> To: El-Haj-Mahmoud, Samer ; Andrew Fish 
> ; Foster, Matthew I 
> Cc: edk2-devel@lists.01.org 
> Subject: Re: [edk2] Using the Shell to launch a kernel using a RAMDISK
> 
> On 05/18/16 15:48, El-Haj-Mahmoud, Samer wrote:
>> Does it make sense to update the RAM Disk HII (at
>> \MdeModulePkg\Universal\Disk\RamDiskDxe) to allow creating a RAM Disk 
>> of Type Reserved, for this kind of testing?
> 
> That's exactly what I suggested in
> 
> http://thread.gmane.org/gmane.comp.bios.edk2.devel/10766/focus=10770
> 
> Namely,
> 
>> [...] I think this question should be exposed with a checkbox on the
>> form: "will you want to boot from this RAM disk?" And then the answer 
>> to that question should determine the type of memory the RAM disk is 
>> allocated from.
> 
> but it was not approved; Feng said later in the thread,
> 
>> [...] For those ramdisk created from Ramdisk HII, the original design 
>> intention is only for boot time use.
> 
> I had not participated in the design, so I didn't question it.
> 
> Thanks
> Laszlo
> 
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf 
>> Of Laszlo Ersek
>> Sent: Wednesday, May 18, 2016 4:39 AM
>> To: Andrew Fish ; Foster, Matthew I 
>> 
>> Cc: edk2-devel@lists.01.org 
>> Subject: Re: [edk2] Using the Shell to launch a kernel using a 
>> RAMDISK
>>
>> On 05/18/16 01:16, Andrew Fish wrote:
>>>
 On May 17, 2016, at 4:08 PM, Foster, Matthew I 
  wrote:

 Thanks Ersek,

>>>
>>> Laszlo... 
>>
>> That's right, thanks Andrew :)
>>
 I did change my code to match what you have here as far as the 
 allocation. I did some more debugging and registered up a callback 
 in exit boot services, and did an integrity check of the memory I 
 previously allocated and put the ramdisk in. I checked the memory 
 right before I call loadimage (to launch the kernel), and then 
 again I check in the callback during exit boot services. So some 
 point between the memory actually does get changed. So it might be 
 something happening during exit boot services.

 Just a question, after allocating that memory as you mentioned 
 below, other 

Re: [edk2] Acquire the permission to commit code in edk2-staging tree

[Chang, Abner (HPS SW/FW Technologist)]

So, I think I can be the owner at the beginning, says I can own the RISC-V 
package and keep to sync with EDK2. However, I think it would be better to have 
the co-owner, probably Jordan or Jiewen.
I am not really clear about the entire process of edk2 code commitment, just my 
first priority for RISC-V edk2 port is to commit all of RISC-V code to 
edk2-stagign tree. Then RISC-V edk2 port is visible to all of RISC-V people.
Thanks
Abner

-Original Message-
From: Gao, Liming [mailto:liming@intel.com] 
Sent: Thursday, May 19, 2016 6:14 PM
To: Justen, Jordan L ; Chang, Abner (HPS SW/FW 
Technologist) ; Kinney, Michael D 

Cc: edk2-devel@lists.01.org
Subject: RE: Acquire the permission to commit code in edk2-staging tree

Jordan:
  I think the package maintainer has the access. The staging branch owner may 
not have. So, if the contributor has no access, he needs to ask help for the 
package maintainer. If it is TRUE, could you help update README to document it?

Thanks
Liming
> -Original Message-
> From: Justen, Jordan L
> Sent: Thursday, May 19, 2016 11:50 AM
> To: Gao, Liming ; Chang, Abner (HPS SW/FW
> Technologist) ; Kinney, Michael D 
> 
> Cc: edk2-devel@lists.01.org
> Subject: RE: Acquire the permission to commit code in edk2-staging 
> tree
> 
> On 2016-05-18 18:13:18, Gao, Liming wrote:
> > Jordan and Mike:
> >   For new package and platform, I think we can grant access to new
> >   owner. Do you think so?
> >
> 
> When discussing the staging process, I thought that staging branches 
> would have package maintainer sponsors that would own sync'ing updates 
> of the staging branch:
> 
> http://thread.gmane.org/gmane.comp.bios.edk2.devel/8964/focus=8973
> 
> I'll admit, that doesn't seem to be clear in the current README:
> 
> https://raw.githubusercontent.com/tianocore/edk2-staging/about/README
> 
> Does maintainer refer to the package maintainer sponsor, or the main 
> staging branch developer.
> 
> Anyway, I would be willing to work with Abner, unless we decide we 
> should handle it another way.
> 
> -Jordan
> 
> >
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf 
> > Of
> Chang, Abner (HPS SW/FW Technologist)
> > Sent: Wednesday, May 18, 2016 12:01 PM
> > To: Kinney, Michael D ; Justen, Jordan L
> 
> > Cc: edk2-devel@lists.01.org
> > Subject: [edk2] Acquire the permission to commit code in 
> > edk2-staging tree
> >
> > Hi Michael and Jordan,
> > I'm going to commit RISC-V to edk2-staging tree, however, I don't 
> > have the
> permission to modify that repository.
> > Could you please help to give me the permission?
> > Thanks
> > Abner
> > ___
> > 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 2/2] NetworkPkg/TcpDxe: Remove the status check of SockProcessRcvToken

[El-Haj-Mahmoud, Samer]

Reviewed-by: Samer El-Haj-Mahmoud 

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Gary Lin
Sent: Wednesday, May 18, 2016 10:49 PM
To: edk2-devel@lists.01.org
Cc: Siyuan Fu ; Jiaxin Wu 
Subject: [edk2] [PATCH 2/2] NetworkPkg/TcpDxe: Remove the status check of 
SockProcessRcvToken

SockProcessRcvToken only returns the number of the received bytes, not an EFI 
Status.

Cc: "Siyuan Fu" 
Cc: "Jiaxin Wu" 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Gary Lin 
---
 NetworkPkg/TcpDxe/SockInterface.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/NetworkPkg/TcpDxe/SockInterface.c 
b/NetworkPkg/TcpDxe/SockInterface.c
index 4abda74..1f3524b 100644
--- a/NetworkPkg/TcpDxe/SockInterface.c
+++ b/NetworkPkg/TcpDxe/SockInterface.c
@@ -724,11 +724,7 @@ SockRcv (
   }
 
   if (RcvdBytes != 0) {
-Status = SockProcessRcvToken (Sock, RcvToken);
-
-if (EFI_ERROR (Status)) {
-  goto Exit;
-}
+SockProcessRcvToken (Sock, RcvToken);
 
 Status = Sock->ProtoHandler (Sock, SOCK_CONSUMED, NULL);
   } else {
--
2.8.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

Re: [edk2] [PATCH 1/2] NetworkPkg/HttpDxe: Don't free Wrap in HttpTcpReceiveNotifyDpc

[El-Haj-Mahmoud, Samer]

Thanks for the fixes Gary

Reviewed-by: Samer El-Haj-Mahmoud 


-Original Message-
From: Gary Lin [mailto:g...@suse.com] 
Sent: Wednesday, May 18, 2016 10:49 PM
To: edk2-devel@lists.01.org
Cc: Wu, Jiaxin ; Siyuan Fu ; 
El-Haj-Mahmoud, Samer ; Laszlo Ersek 
; Hegde, Nagaraj P 
Subject: [PATCH 1/2] NetworkPkg/HttpDxe: Don't free Wrap in 
HttpTcpReceiveNotifyDpc

The HTTP Token Wrap is created in EfiHttpResponse() and then passed to the 
deferred Receive event callback, HttpTcpReceiveNotifyDpc.
HttpTcpReceiveHeader and HttpTcpReceiveBody use a Tcp polling loop to monitor 
the socket status and trigger the Receive event when a new packet arrives. The 
Receive event brings up HttpTcpReceiveNotifyDpc to process the HTTP message and 
the function will set Wrap->TcpWrap.IsRxDone to TRUE to break the Tcp polling 
loop.

However, HttpTcpReceiveNotifyDpc mistakenly freed Wrap, so the Tcp polling loop 
was actually checking a dead variable, and this led the system into an unstable 
status.

Given the fact that the HTTP Token Wrap will be freed in EfiHttpResponse or 
HttpResponseWorker, this commit removes every "FreePool (Wrap)" in 
HttpTcpReceiveNotifyDpc.

Cc: "Wu, Jiaxin" 
Cc: "Siyuan Fu" 
Cc: "El-Haj-Mahmoud, Samer" 
Cc: "Laszlo Ersek" 
Cc: "Hegde, Nagaraj P" 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Gary Lin 
---
 NetworkPkg/HttpDxe/HttpProto.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/NetworkPkg/HttpDxe/HttpProto.c b/NetworkPkg/HttpDxe/HttpProto.c 
index f3992ed..afa7fe4 100644
--- a/NetworkPkg/HttpDxe/HttpProto.c
+++ b/NetworkPkg/HttpDxe/HttpProto.c
@@ -152,7 +152,6 @@ HttpTcpReceiveNotifyDpc (
 if (EFI_ERROR (Wrap->TcpWrap.Rx6Token.CompletionToken.Status)) {
   Wrap->HttpToken->Status = Wrap->TcpWrap.Rx6Token.CompletionToken.Status;
   gBS->SignalEvent (Wrap->HttpToken->Event);
-  FreePool (Wrap);
   return ;
 }
 
@@ -162,7 +161,6 @@ HttpTcpReceiveNotifyDpc (
 if (EFI_ERROR (Wrap->TcpWrap.Rx4Token.CompletionToken.Status)) {
   Wrap->HttpToken->Status = Wrap->TcpWrap.Rx4Token.CompletionToken.Status;
   gBS->SignalEvent (Wrap->HttpToken->Event);
-  FreePool (Wrap);
   return ;
 }
   }
@@ -234,8 +232,6 @@ HttpTcpReceiveNotifyDpc (
   // Check pending RxTokens and receive the HTTP message.
   //
   NetMapIterate (>HttpInstance->RxTokens, HttpTcpReceive, NULL);
-
-  FreePool (Wrap);
 }
 
 /**
--
2.8.2

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

Re: [edk2] [PATCH 1/2] NetworkPkg/HttpDxe: Don't free Wrap in HttpTcpReceiveNotifyDpc

[El-Haj-Mahmoud, Samer]

Interesting (and accurate) deduction... :)


-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Gary Lin
Sent: Thursday, May 19, 2016 5:07 AM
To: Laszlo Ersek 
Cc: edk2-de...@ml01.01.org; Wu, Jiaxin ; Siyuan Fu 

Subject: Re: [edk2] [PATCH 1/2] NetworkPkg/HttpDxe: Don't free Wrap in 
HttpTcpReceiveNotifyDpc

On Thu, May 19, 2016 at 11:53:59AM +0200, Laszlo Ersek wrote:
> On 05/19/16 10:39, Gary Lin wrote:
> > Cc Samer and Nagaraj.
> > I saw them with "git sent-email --dry-run". Wonder how they were dropped.
> 
> They were not dropped. The list software is trying to be smart.
> 
> Namely, if you go to 
> ,
> and click "Unsubscribe or edit options", you can toggle a number of 
> mailing list options for your subscriptions. One of these options is:
> 
>   Avoid duplicate copies of messages?
> 
> This affects how the list software reflects an email to you that you 
> were also directly addressed on.
> 
> Some people (including me) like to receive duplicate copies. Meaning, 
> if you CC me on a patch that you send to the list, I will get a direct 
> copy from you (due to the CC), plus the list software will send me 
> another copy (*because* I have enabled duplicate copies). I like to 
> see one instance of your patch in my INBOX, and another one in my 
> edk2-devel folder (which is supposed to be a complete archive).
> 
> However, others don't like to receive duplicates. They prefer the list 
> software not to send them a reflected message, if they are also on the 
> CC list directly. When the list software is deciding about reflecting 
> a message to subscriber X, and subscriber X has disabled duplicates, 
> and subscriber X is also on the CC list of the message that the list 
> software received from the submitter, then the list software assumes 
> subscriber X has the message anyway (directly from the submitter), and 
> will not reflect the message to subscriber X.
> 
> *But*, to actually answer your question, in this latter case something 
> else happens to. Namely, subscriber X is *also* removed from the CC 
> list of the message that the list software sends out to any other 
> subscribers. I guess the idea is, "subscriber X likes to receive 
> exactly one copy of each message on the list; so he got one copy of 
> the thread starter directly from the submitter, and he will get one 
> copy of each followup message too, sent by others, reflected by the list 
> software".
> If the CC were preserved in the reflected thread starter, then 
> followup messages would arrive to subscriber X both directly and 
> reflected by the list.
> 
> For the submitter, this CC munging is incredibly confusing and annoying.
> But, if you know about the above, it is more tolerable.
> 
> As a side effect, you can now deduce that Samer and Nagaraj have *not* 
> enabled duplicate copies in their options. :)
> 
Thanks for the detailed explanation. So my mail server config is alright. I'm 
much relieved now :)

Thanks,

Gary Lin
___
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] [RFC] Proposal to organize packages into directories

[Paolo Bonzini]

On 18/05/2016 01:57, Kinney, Michael D wrote:
>   Core
> CorebootModulePkg
> CorebootPayloadPkg

I think that anything with a .fdf file should be under Platform.
CorebootPayloadPkg is the only outlier in your proposal.

> Emulated
>   DuetPkg
>   EmulatorPkg
>   Nt32Pkg
>   OvmfPkg

I think OvmfPkg is not emulated; certainly not in the same sense as
EmulatorPkg, Nt32 or UnixPkg.  DuetPkg also seems more similar to
OvmfPkg than to EmulatorPkg (and definitely most similar to
CorebootPayloadPkg, which should be under Platform according to the rule
I proposed above).

In addition, I think that separation by architecture is more useful than
separation by vendor.  This yields the following:

Platform
  Arm
ArmPlatformPkg
ArmVirtPkg
BeagleBoardPkg
  Emulated
EmulatorPkg
Nt32Pkg
UnixPkg
  IA32X64
CorebootPayloadPkg
DuetPkg
Intel
  QuarkPlatformPkg
  Vlv2TbltDevicePkg
OvmfPkg

IA32X64 is not a great name, but neither is Intel.  X86 suggests 32-bit
only.

In addition, I am not sure about the separation between "Drivers" and
"Silicon".  My proposal here is to unify them as follows:

Drivers
  FatPkg
  NetworkPkg
  OptionRomPkg
  Arm
ArmPkg
Omap35xxPkg
  IA32X64
PcAtChipsetPkg
QuarkSocPkg
UefiCpuPkg
Vlv2DeviceRefCodePkg

or alternatively Omap35xxPkg, QuarkSocPkg and Vlv2DeviceRefCode could
move under Drivers/Vendor/{Arm,Intel}.

Thanks,

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

Re: [edk2] edk2-staging/HTTPS-TLS

[El-Haj-Mahmoud, Samer]

I do have some experience maintaining my branches (in other projects) that 
eventually get submitted as pull requests to a master. And I always find it 
easier to keep my branch in sync by doing frequent merge form master. I agree 
that it probably makes more sense to do this a couple of times a week (or so) 
manually, and resolve any merge conflicts, than try to automate it.

At this point, I am working on enabling the HTTPS/TLS feature, but I need to do 
so in a code base that is up-to-date with EDK2 master. So the main issue I have 
is merging from (the now falling behind) HTTPS/TLS branch into my code base 
(which is tracking EDK2). As Laszlo said, it is already getting mad because of 
all of the HTTP fixes in EDK2.

Thanks,
--Samer


-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com] 
Sent: Thursday, May 19, 2016 5:51 AM
To: Gao, Liming ; Wu, Jiaxin ; 
El-Haj-Mahmoud, Samer ; Li, Ruth 
; Ye, Ting ; Long, Qin 
; edk2-devel@lists.01.org ; Justen, 
Jordan L ; Kinney, Michael D 
; Leif Lindholm (Linaro address) 

Subject: Re: [edk2] edk2-staging/HTTPS-TLS

On 05/19/16 11:53, Gao, Liming wrote:
> Ersek:
> I remember we discuss Rebase or Merge before. The conclusion is to 
> keep the liner history in edk2 project. So, I think rebase is better.

Okay.

> If we choose option 1, I suggest to setup mirror task in server and 
> auto run regular sync. If the confliction happens, it sends the mail 
> to edk dev list and let owner follow up.

Hmm... I'm not sure about an automated rebase. I always have a number of 
downstream branches (in my private repo) that I rebase almost every day to new 
master.

However, occasionally I'm so deep in work that I don't want to rebase for a 
week, for example. An automated rebase would be very annoying in such 
situations, when I'm in the middle of shuffling around my patches.

I think it's the owner's responsibility after all. If they are fine with an 
automated rebase, so be it; but the automatism should be possible to disable.

Thanks
Laszlo

> Thanks
> Liming
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf 
>> Of Laszlo Ersek
>> Sent: Thursday, May 19, 2016 4:31 PM
>> To: Wu, Jiaxin ; El-Haj-Mahmoud, Samer 
>> ; Li, Ruth ; Ye, 
>> Ting ; Long, Qin ; edk2- 
>> de...@lists.01.org ; Justen, Jordan L 
>> ; Kinney, Michael D 
>> ; Leif Lindholm (Linaro address) 
>> 
>> Subject: Re: [edk2] edk2-staging/HTTPS-TLS
>>
>> On 05/19/16 05:35, Wu, Jiaxin wrote:
>>> Hi all,
>>> There are two ways to sync HTTPS-TLS branch with EDK2 master:
>>>
>>> 1. Sync all changes in EDK2 master to branch by rebasing the whole 
>>> feature branch. Need frequency request to sync edk2-staging/master 
>>> to latest edk2/master. This way can also keep branch code go 
>>> straight with edk2/master.
>>>
>>> 2. Only sync HTTPS/TLS related patches to branch (e.g. HttpDxe 
>>> update). That means to use one stable edk2 tag for 
>>> edk2-staging/HTTPS-TLS branch development. By this way, frequency 
>>> request to sync edk2-staging/master to latest edk2/master can be 
>>> avoided but maybe some bugs fixed in edk2/master can't be synced to 
>>> branch timely.
>>>
>>> Both of them are not easy since it's probable that each updates in
>>> edk2 trunk may cause the conflicts with the HTTPS code (HttpDxe 
>>> driver and CryptyPkg especially).
>>>
>>> Effort is almost. Which one do you prefer?
>>
>> I'm chiming in here not because I have a direct interest in the 
>> HTTPS-TLS feature set, but because this feature branch seems to be 
>> the first real-life test case of edk2-staging. I'd like to add three points.
>>
>> (a) The ultimate goal of a staging branch is to be merged into edk2 
>> master when the feature is complete. If you try to save the effort 
>> now that would be necessary for rebasing the feature branch, you will 
>> go
>> *mad* when you finally want to merge the staging branch into edk2 
>> master. The larger you allow the divergence to grow, the 
>> (non-linearly) harder time you will have when you try to do the final 
>> merge into edk2 master. So, the trick is to stay sensibly close as 
>> possible to edk2 master on the staging branch.
>>
>> For this reason, option (1) is very strongly preferable. Option (2) 
>> -- which is called "cherry picking" or "backporting" -- is really 
>> only an option for branches that are never meant to be merged back 
>> into the master branch. Given that HTTPS-TLS is not such a downstream 
>> branch (instead, it is a development, topic branch), option (2) doesn't 
>> apply.
>>
>> (b) For 

Re: [edk2] [Patch] MdeModulePkg/PciHostBridgeDxe: Add CpuArch protocol dependency

[Ard Biesheuvel]

On 19 May 2016 at 13:04, Laszlo Ersek  wrote:
> On 05/19/16 12:22, Ard Biesheuvel wrote:
>> On 19 May 2016 at 12:04, Laszlo Ersek  wrote:
>
>>> On the other hand... the virtio-gpu device would require a GOP that is
>>> Blt()-only. That is, no direct framebuffer access. I very much hope this
>>> is not a problem for the edk2 code. I don't know if bootloaders like
>>> grub2 will be able to deal with it. (But, at least grub2 can be patched.
>>> Not sure about Windows-on-aarch64.) For runtime OSes (on aarch64), we
>>> expect that they ship a native virtio-gpu driver by default.
>>>
>>
>> I suppose this blt-only issue goes away as well once we fix the KVM
>> host cached mappings issue?
>
> No; as far as I recall the info I got from Gerd (I haven't wanted to
> look at the spec in detail yet), lack of a direct-access framebuffer is
> a design principle for virtio-gpu. It's all about transfering 2D (and
> later 3D) operations.
>

OK, fair enough

> If the KVM host cached mappings issue is fixed, we can simply use QXL /
> stdvga (with the framebuffer).
>

I see the use of GOP under the OS and efifb merely as stopgap
solutions, so I won't complain if we get something better in return.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [patch] MdeModulePkg/DisplayEngine: Fix memory leak issues in DisplayEngine

[Dandan Bi]

Cc: Qiu Shumin 
Cc: Eric Dong 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Dandan Bi 
Reviewed-by: Eric Dong 
---
 MdeModulePkg/Universal/DisplayEngineDxe/InputHandler.c   | 5 +
 MdeModulePkg/Universal/DisplayEngineDxe/ProcessOptions.c | 3 ++-
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Universal/DisplayEngineDxe/InputHandler.c 
b/MdeModulePkg/Universal/DisplayEngineDxe/InputHandler.c
index 732dd2f..8e7b735 100644
--- a/MdeModulePkg/Universal/DisplayEngineDxe/InputHandler.c
+++ b/MdeModulePkg/Universal/DisplayEngineDxe/InputHandler.c
@@ -1,9 +1,9 @@
 /** @file
 Implementation for handling user input from the User Interfaces.
 
-Copyright (c) 2004 - 2015, Intel Corporation. All rights reserved.
+Copyright (c) 2004 - 2016, 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
 
@@ -1297,13 +1297,10 @@ GetSelectionInputPopUp (
   ValueType = 0;
   CurrentOption = NULL;
   ShowDownArrow = FALSE;
   ShowUpArrow   = FALSE;
 
-  StringPtr = AllocateZeroPool ((gOptionBlockWidth + 1) * 2);
-  ASSERT (StringPtr);
-
   ZeroMem (, sizeof (EFI_HII_VALUE));
 
   Question = MenuOption->ThisTag;
   if (Question->OpCode->OpCode == EFI_IFR_ORDERED_LIST_OP) {
 Link = GetFirstNode (>OptionListHead);
diff --git a/MdeModulePkg/Universal/DisplayEngineDxe/ProcessOptions.c 
b/MdeModulePkg/Universal/DisplayEngineDxe/ProcessOptions.c
index bb2faf3..16aaf6c 100644
--- a/MdeModulePkg/Universal/DisplayEngineDxe/ProcessOptions.c
+++ b/MdeModulePkg/Universal/DisplayEngineDxe/ProcessOptions.c
@@ -1,10 +1,10 @@
 /** @file
 Implementation for handling the User Interface option processing.
 
 
-Copyright (c) 2004 - 2015, Intel Corporation. All rights reserved.
+Copyright (c) 2004 - 2016, 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
 
@@ -884,10 +884,11 @@ PasswordProcess (
 gUserInput->InputValue.Buffer = AllocateCopyPool 
(Question->CurrentValue.BufferLen, StringPtr);
 gUserInput->InputValue.BufferLen = Question->CurrentValue.BufferLen;
 gUserInput->InputValue.Type = Question->CurrentValue.Type;
 gUserInput->InputValue.Value.string = HiiSetString(gFormData->HiiHandle, 
gUserInput->InputValue.Value.string, StringPtr, NULL);
 FreePool (StringPtr); 
+FreePool (TempString);
 
 Status = EFI_SUCCESS;
 
 if (EFI_ERROR (Status)) {
   //
-- 
1.9.5.msysgit.1

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

Re: [edk2] [Patch] MdeModulePkg/PciHostBridgeDxe: Add CpuArch protocol dependency

[Laszlo Ersek]

On 05/19/16 12:22, Ard Biesheuvel wrote:
> On 19 May 2016 at 12:04, Laszlo Ersek  wrote:

>> On the other hand... the virtio-gpu device would require a GOP that is
>> Blt()-only. That is, no direct framebuffer access. I very much hope this
>> is not a problem for the edk2 code. I don't know if bootloaders like
>> grub2 will be able to deal with it. (But, at least grub2 can be patched.
>> Not sure about Windows-on-aarch64.) For runtime OSes (on aarch64), we
>> expect that they ship a native virtio-gpu driver by default.
>>
> 
> I suppose this blt-only issue goes away as well once we fix the KVM
> host cached mappings issue?

No; as far as I recall the info I got from Gerd (I haven't wanted to
look at the spec in detail yet), lack of a direct-access framebuffer is
a design principle for virtio-gpu. It's all about transfering 2D (and
later 3D) operations.

If the KVM host cached mappings issue is fixed, we can simply use QXL /
stdvga (with the framebuffer).

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

Re: [edk2] edk2-staging/HTTPS-TLS

[Laszlo Ersek]

On 05/19/16 11:53, Gao, Liming wrote:
> Ersek:
> I remember we discuss Rebase or Merge before. The conclusion is to
> keep the liner history in edk2 project. So, I think rebase is better.

Okay.

> If we choose option 1, I suggest to setup mirror task in server and
> auto run regular sync. If the confliction happens, it sends the mail
> to edk dev list and let owner follow up.

Hmm... I'm not sure about an automated rebase. I always have a number of
downstream branches (in my private repo) that I rebase almost every day
to new master.

However, occasionally I'm so deep in work that I don't want to rebase
for a week, for example. An automated rebase would be very annoying in
such situations, when I'm in the middle of shuffling around my patches.

I think it's the owner's responsibility after all. If they are fine with
an automated rebase, so be it; but the automatism should be possible to
disable.

Thanks
Laszlo

> Thanks
> Liming
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>> Laszlo Ersek
>> Sent: Thursday, May 19, 2016 4:31 PM
>> To: Wu, Jiaxin ; El-Haj-Mahmoud, Samer > haj-mahm...@hpe.com>; Li, Ruth ; Ye, Ting
>> ; Long, Qin ; edk2-
>> de...@lists.01.org ; Justen, Jordan L
>> ; Kinney, Michael D
>> ; Leif Lindholm (Linaro address)
>> 
>> Subject: Re: [edk2] edk2-staging/HTTPS-TLS
>>
>> On 05/19/16 05:35, Wu, Jiaxin wrote:
>>> Hi all,
>>> There are two ways to sync HTTPS-TLS branch with EDK2 master:
>>>
>>> 1. Sync all changes in EDK2 master to branch by rebasing the whole
>>> feature branch. Need frequency request to sync edk2-staging/master to
>>> latest edk2/master. This way can also keep branch code go straight
>>> with edk2/master.
>>>
>>> 2. Only sync HTTPS/TLS related patches to branch (e.g. HttpDxe
>>> update). That means to use one stable edk2 tag for
>>> edk2-staging/HTTPS-TLS branch development. By this way, frequency
>>> request to sync edk2-staging/master to latest edk2/master can be
>>> avoided but maybe some bugs fixed in edk2/master can't be synced to
>>> branch timely.
>>>
>>> Both of them are not easy since it's probable that each updates in
>>> edk2 trunk may cause the conflicts with the HTTPS code (HttpDxe
>>> driver and CryptyPkg especially).
>>>
>>> Effort is almost. Which one do you prefer?
>>
>> I'm chiming in here not because I have a direct interest in the
>> HTTPS-TLS feature set, but because this feature branch seems to be the
>> first real-life test case of edk2-staging. I'd like to add three points.
>>
>> (a) The ultimate goal of a staging branch is to be merged into edk2
>> master when the feature is complete. If you try to save the effort now
>> that would be necessary for rebasing the feature branch, you will go
>> *mad* when you finally want to merge the staging branch into edk2
>> master. The larger you allow the divergence to grow, the (non-linearly)
>> harder time you will have when you try to do the final merge into edk2
>> master. So, the trick is to stay sensibly close as possible to edk2
>> master on the staging branch.
>>
>> For this reason, option (1) is very strongly preferable. Option (2) --
>> which is called "cherry picking" or "backporting" -- is really only an
>> option for branches that are never meant to be merged back into the
>> master branch. Given that HTTPS-TLS is not such a downstream branch
>> (instead, it is a development, topic branch), option (2) doesn't apply.
>>
>> (b) For option (1), there are actually two methods, rebasing and merging:
>>
>> 1.1. Rebasing. It has the drawback that it throws away your prior
>> development history on the feature (staging) branch. Many people don't
>> like this. On the other hand, you can do it as frequently as you like,
>> without creating a mess of commit history.
>>
>> Here's a diagram:
>>
>> master final merge
>> *---*-*---**---*--**-->
>>  \ \\ /
>>   \ \\   /
>>*-*-*-[discont.]  *-*-*-[discont.] *-*-*-*
>>feature   rebased feature  rebased feature
>>
>>
>> 1.2. Merging. It has the benfit that your development history on the
>> feature (staging) branch will be preserved in edk2 master too. On the
>> other hand, you have to be careful about the frequency of
>> master-to-staging merges. If you do it very frequently, you'll end up
>> with a complex history.
>>
>> In the Linux community, merging is preferred, and a master-to-topic
>> merge is advised whenever the master branch receives a feature or a
>> bugfix that is important for the topic branch as well. That is, no
>> spurious merges.
>>
>> Here's a diagram:
>>
>> master   

Re: [edk2] [Patch] MdeModulePkg/PciHostBridgeDxe: Add CpuArch protocol dependency

[Ard Biesheuvel]

On 19 May 2016 at 12:04, Laszlo Ersek  wrote:
> On 05/19/16 10:52, Ni, Ruiyu wrote:
>>
>>
>> Regards,
>> Ray
>>
>>>-Original Message-
>>>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
>>>Laszlo Ersek
>>>Sent: Thursday, May 19, 2016 4:08 PM
>>>To: Ni, Ruiyu ; edk2-de...@ml01.01.org
>>>Cc: Gao, Liming 
>>>Subject: Re: [edk2] [Patch] MdeModulePkg/PciHostBridgeDxe: Add CpuArch 
>>>protocol dependency
>>>
>>>On 05/19/16 09:17, Ruiyu Ni wrote:
 The driver entry point calls gDS->SetMemorySpaceAttributes().
 This interface may return EFI_NOT_AVAILABLE_YET when CPU Arch
 protocol is not available.
 So we need to list CpuArch protocol in its INF dependency section.

 Contributed-under: TianoCore Contribution Agreement 1.0
 Signed-off-by: Ruiyu Ni 
 Cc: Liming Gao 
 ---
  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

 diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
>>>b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
 index ab5d87e..d8b0439 100644
 --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
 +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
 @@ -52,4 +52,5 @@ [Protocols]

  [Depex]
gEfiCpuIo2ProtocolGuid AND
 -  gEfiMetronomeArchProtocolGuid
 +  gEfiMetronomeArchProtocolGuid AND
 +  gEfiCpuArchProtocolGuid

>>>
>>>This reminds me of commit f9a8be423cdd5:
>>>
 Because gDS->SetMemorySpaceAttributes() is ultimately implemented by
 EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes() -- see
 "MdeModulePkg/Core/Dxe/Gcd/Gcd.c" and "ArmPkg/Drivers/CpuDxe/" -- we
 add the CPU architectural protocol to the module's DepEx.
>>
>>
>>
>> I saw the ArmVirtPkg/PciHostBridge driver set the MMIO to WB when
>>
>> PcdKludgeMapPciMmioAsCache is TRUE.
>>
>> So ArmVirtQemu platform cannot use the MdeModulePkg/PciHostBridge.
>>
>> Is my understanding right?
>
> Yes, your understanding is right; that's our main blocker.
>
> Please see: .
>
>> Do you have any solution?
>
> Not yet.
>
> Ard's recent argument was that it should be considered a KVM bug
> actually. I don't object to that idea at all, but it's not something I
> can fix. So if it's a KVM bug indeed, it remains a blocker for me
> nonetheless.
>

The reasoning is that the current KVM behavior is justified as the
expected result of cache coherent DMA, while I think that is
incorrect. MMIO windows exposed by PCI are disjoint from RAM, and
writes into such a window can only be observed by the device if they
ultimately reach this window, so it *never* makes sense to use
cacheable mappings here.

The problem is that KVM on ARM uses host RAM to emulate these virtual
device regions, which means that the emulated device uses cached
mappings for memory that the guest may have mapped as uncached. So the
correct fix is for either KVM or QEMU to perform explicit cache
maintenance on the regions it exposes to the guest as framebuffers or
other MMIO regions with memory semantics.

> The longer-term idea is a driver for QEMU's virtio-gpu device. The
> specification of that QEMU device is under review for the official
> virtio spec. Once it is accepted, I plan to implement a virtio-gpu
> driver for OvmfPkg and ArmVirtPkg. With that device and driver, we can
> (hopefully!) drop support for the legacy VGA device in ArmVirtPkg.
>

/me puts some beer in the fridge for the day Laszlo completes this task

> With VGA gone, we won't have any write-combining device where KVM backs
> a PCI MMIO BAR with cacheable host memory. This will side-step the
> caching issue, and we should be able to migrate to the core
> PciHostBridgeDxe.
>
> On the other hand... the virtio-gpu device would require a GOP that is
> Blt()-only. That is, no direct framebuffer access. I very much hope this
> is not a problem for the edk2 code. I don't know if bootloaders like
> grub2 will be able to deal with it. (But, at least grub2 can be patched.
> Not sure about Windows-on-aarch64.) For runtime OSes (on aarch64), we
> expect that they ship a native virtio-gpu driver by default.
>

I suppose this blt-only issue goes away as well once we fix the KVM
host cached mappings issue?

> This situation is really annoying. :( I have zero clue why the ARMv8
> architecture decided to combine guest and host caching attributes like
> this (i.e., why the strictest of the two takes effect, rather than the
> laxest).
>

Good question. I heard a reasonably convincing sounding explanation
from Peter at some point, but my memory fails me
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] Acquire the permission to commit code in edk2-staging tree

[Gao, Liming]

Jordan:
  I think the package maintainer has the access. The staging branch owner may 
not have. So, if the contributor has no access, he needs to ask help for the 
package maintainer. If it is TRUE, could you help update README to document it?

Thanks
Liming
> -Original Message-
> From: Justen, Jordan L
> Sent: Thursday, May 19, 2016 11:50 AM
> To: Gao, Liming ; Chang, Abner (HPS SW/FW
> Technologist) ; Kinney, Michael D
> 
> Cc: edk2-devel@lists.01.org
> Subject: RE: Acquire the permission to commit code in edk2-staging tree
> 
> On 2016-05-18 18:13:18, Gao, Liming wrote:
> > Jordan and Mike:
> >   For new package and platform, I think we can grant access to new
> >   owner. Do you think so?
> >
> 
> When discussing the staging process, I thought that staging branches
> would have package maintainer sponsors that would own sync'ing updates
> of the staging branch:
> 
> http://thread.gmane.org/gmane.comp.bios.edk2.devel/8964/focus=8973
> 
> I'll admit, that doesn't seem to be clear in the current README:
> 
> https://raw.githubusercontent.com/tianocore/edk2-staging/about/README
> 
> Does maintainer refer to the package maintainer sponsor, or the main
> staging branch developer.
> 
> Anyway, I would be willing to work with Abner, unless we decide we
> should handle it another way.
> 
> -Jordan
> 
> >
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Chang, Abner (HPS SW/FW Technologist)
> > Sent: Wednesday, May 18, 2016 12:01 PM
> > To: Kinney, Michael D ; Justen, Jordan L
> 
> > Cc: edk2-devel@lists.01.org
> > Subject: [edk2] Acquire the permission to commit code in edk2-staging tree
> >
> > Hi Michael and Jordan,
> > I'm going to commit RISC-V to edk2-staging tree, however, I don't have the
> permission to modify that repository.
> > Could you please help to give me the permission?
> > Thanks
> > Abner
> > ___
> > 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 1/2] NetworkPkg/HttpDxe: Don't free Wrap in HttpTcpReceiveNotifyDpc

[Gary Lin]

On Thu, May 19, 2016 at 11:53:59AM +0200, Laszlo Ersek wrote:
> On 05/19/16 10:39, Gary Lin wrote:
> > Cc Samer and Nagaraj.
> > I saw them with "git sent-email --dry-run". Wonder how they were dropped.
> 
> They were not dropped. The list software is trying to be smart.
> 
> Namely, if you go to ,
> and click "Unsubscribe or edit options", you can toggle a number of
> mailing list options for your subscriptions. One of these options is:
> 
>   Avoid duplicate copies of messages?
> 
> This affects how the list software reflects an email to you that you
> were also directly addressed on.
> 
> Some people (including me) like to receive duplicate copies. Meaning, if
> you CC me on a patch that you send to the list, I will get a direct copy
> from you (due to the CC), plus the list software will send me another
> copy (*because* I have enabled duplicate copies). I like to see one
> instance of your patch in my INBOX, and another one in my edk2-devel
> folder (which is supposed to be a complete archive).
> 
> However, others don't like to receive duplicates. They prefer the list
> software not to send them a reflected message, if they are also on the
> CC list directly. When the list software is deciding about reflecting a
> message to subscriber X, and subscriber X has disabled duplicates, and
> subscriber X is also on the CC list of the message that the list
> software received from the submitter, then the list software assumes
> subscriber X has the message anyway (directly from the submitter), and
> will not reflect the message to subscriber X.
> 
> *But*, to actually answer your question, in this latter case something
> else happens to. Namely, subscriber X is *also* removed from the CC list
> of the message that the list software sends out to any other
> subscribers. I guess the idea is, "subscriber X likes to receive exactly
> one copy of each message on the list; so he got one copy of the thread
> starter directly from the submitter, and he will get one copy of each
> followup message too, sent by others, reflected by the list software".
> If the CC were preserved in the reflected thread starter, then followup
> messages would arrive to subscriber X both directly and reflected by the
> list.
> 
> For the submitter, this CC munging is incredibly confusing and annoying.
> But, if you know about the above, it is more tolerable.
> 
> As a side effect, you can now deduce that Samer and Nagaraj have *not*
> enabled duplicate copies in their options. :)
> 
Thanks for the detailed explanation. So my mail server config is
alright. I'm much relieved now :)

Thanks,

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

Re: [edk2] [Patch] MdeModulePkg/PciHostBridgeDxe: Add CpuArch protocol dependency

[Laszlo Ersek]

On 05/19/16 10:52, Ni, Ruiyu wrote:
> 
> 
> Regards,
> Ray
> 
>>-Original Message-
>>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
>>Ersek
>>Sent: Thursday, May 19, 2016 4:08 PM
>>To: Ni, Ruiyu ; edk2-de...@ml01.01.org
>>Cc: Gao, Liming 
>>Subject: Re: [edk2] [Patch] MdeModulePkg/PciHostBridgeDxe: Add CpuArch 
>>protocol dependency
>>
>>On 05/19/16 09:17, Ruiyu Ni wrote:
>>> The driver entry point calls gDS->SetMemorySpaceAttributes().
>>> This interface may return EFI_NOT_AVAILABLE_YET when CPU Arch
>>> protocol is not available.
>>> So we need to list CpuArch protocol in its INF dependency section.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ruiyu Ni 
>>> Cc: Liming Gao 
>>> ---
>>>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
>>b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
>>> index ab5d87e..d8b0439 100644
>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
>>> @@ -52,4 +52,5 @@ [Protocols]
>>>
>>>  [Depex]
>>>gEfiCpuIo2ProtocolGuid AND
>>> -  gEfiMetronomeArchProtocolGuid
>>> +  gEfiMetronomeArchProtocolGuid AND
>>> +  gEfiCpuArchProtocolGuid
>>>
>>
>>This reminds me of commit f9a8be423cdd5:
>>
>>> Because gDS->SetMemorySpaceAttributes() is ultimately implemented by
>>> EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes() -- see
>>> "MdeModulePkg/Core/Dxe/Gcd/Gcd.c" and "ArmPkg/Drivers/CpuDxe/" -- we
>>> add the CPU architectural protocol to the module's DepEx.
> 
>  
> 
> I saw the ArmVirtPkg/PciHostBridge driver set the MMIO to WB when
> 
> PcdKludgeMapPciMmioAsCache is TRUE.
> 
> So ArmVirtQemu platform cannot use the MdeModulePkg/PciHostBridge.
> 
> Is my understanding right?

Yes, your understanding is right; that's our main blocker.

Please see: .

> Do you have any solution?

Not yet.

Ard's recent argument was that it should be considered a KVM bug
actually. I don't object to that idea at all, but it's not something I
can fix. So if it's a KVM bug indeed, it remains a blocker for me
nonetheless.

The longer-term idea is a driver for QEMU's virtio-gpu device. The
specification of that QEMU device is under review for the official
virtio spec. Once it is accepted, I plan to implement a virtio-gpu
driver for OvmfPkg and ArmVirtPkg. With that device and driver, we can
(hopefully!) drop support for the legacy VGA device in ArmVirtPkg.

With VGA gone, we won't have any write-combining device where KVM backs
a PCI MMIO BAR with cacheable host memory. This will side-step the
caching issue, and we should be able to migrate to the core
PciHostBridgeDxe.

On the other hand... the virtio-gpu device would require a GOP that is
Blt()-only. That is, no direct framebuffer access. I very much hope this
is not a problem for the edk2 code. I don't know if bootloaders like
grub2 will be able to deal with it. (But, at least grub2 can be patched.
Not sure about Windows-on-aarch64.) For runtime OSes (on aarch64), we
expect that they ship a native virtio-gpu driver by default.

This situation is really annoying. :( I have zero clue why the ARMv8
architecture decided to combine guest and host caching attributes like
this (i.e., why the strictest of the two takes effect, rather than the
laxest).

Thanks!
Laszlo

>>Reviewed-by: Laszlo Ersek 
>>___
>>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] OvmfPkg: prevent 64-bit MMIO BAR degradation if there is no CSM

[Laszlo Ersek]

On 05/19/16 10:42, Ni, Ruiyu wrote:
> I agree.
> 
> Reviewed-by: Ruiyu Ni 

Thanks Ray!

I'll await Jordan's feedback too.

Cheers!
Laszlo

>>-Original Message-
>>From: Laszlo Ersek [mailto:ler...@redhat.com]
>>Sent: Thursday, May 19, 2016 3:38 PM
>>To: Ni, Ruiyu ; edk2-devel-01 
>>Cc: Justen, Jordan L 
>>Subject: Re: [edk2] [PATCH] OvmfPkg: prevent 64-bit MMIO BAR degradation if 
>>there is no CSM
>>
>>On 05/19/16 04:02, Ni, Ruiyu wrote:
>>> Laszlo,
>>> All are good except: can you use EfiCreateProtocolNotifyEvent()
>>> exposed by UefiLib?
>>
>>I am aware of that function, but I dislike it. The reason I dislike it
>>is that (in my opinion) its internal error handling is inferior. For the
>>return values of gBS->CreateEvent() and gBS->RegisterProtocolNotify(),
>>is only uses ASSERT_EFI_ERROR(), and to the return value of
>>gBS->SignalEvent(), it doesn't apply any check.
>>
>>I think the error handling in my code below is superior. CreateEvent()
>>can legitimately fail with EFI_OUT_OF_RESOURCES. So can
>>RegisterProtocolNotify().
>>
>>I wondered before how the error handling could be fixed in
>>EfiCreateProtocolNotifyEvent(). One problem I see is that it returns an
>>EFI_EVENT object, not an EFI_STATUS one.
>>
>>Given that EFI_EVENT is just a typedef for VOID*, we could return a NULL
>>if something fails. However, in the library class header file, such a
>>return value is not permitted:
>>
>>  @return The notification event that was created.
>>
>>This means that the ASSERT_EFI_ERROR() macro invocations in the function
>>body actually conform to the specification: namely,
>>EfiCreateProtocolNotifyEvent() is never supposed to fail (and I guess
>>all of its callers rely on this).
>>
>>Too bad that this function specification doesn't actually reflect
>>reality. (See the above EFI_OUT_OF_RESOURCES failure modes for
>>CreateEvent() and RegisterProtocolNotify().)
>>
>>Which is, I guess, my ultimate reason for never using this utility
>>function, and always open-coding CreateEvent() +
>>RegisterProtocolNotify() + SignalEvent() with proper error checking
>>(using an assert for SignalEvent(), which really can't fail, according
>>to the UEFI spec).
>>
>>Thanks
>>Laszlo
>>
 -Original Message-
 From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
 Laszlo Ersek
 Sent: Thursday, May 19, 2016 6:13 AM
 To: edk2-devel-01 
 Cc: Ni, Ruiyu ; Justen, Jordan L 
 
 Subject: [edk2] [PATCH] OvmfPkg: prevent 64-bit MMIO BAR degradation if 
 there is no CSM

 According to edk2 commit

  "MdeModulePkg/PciBus: do not improperly degrade resource"

 and to the EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL definition in the
 Platform Init 1.4a specification, a platform can provide such a protocol
 in order to influence the PCI resource allocation performed by the PCI Bus
 driver.

 In particular it is possible instruct the PCI Bus driver, with a
 "wildcard" hint, to allocate the 64-bit MMIO BARs of a device in 64-bit
 address space, regardless of whether the device features an option ROM.

 (By default, the PCI Bus driver considers an option ROM reason enough for
 allocating the 64-bit MMIO BARs in 32-bit address space. It cannot know if
 BDS will launch a legacy boot option, and under legacy boot, a legacy BIOS
 binary from a combined option ROM could be dispatched, and fail to access
 MMIO BARs in 64-bit address space.)

 In platform code we can ascertain whether a CSM is present or not. If not,
 then legacy BIOS binaries in option ROMs can't be dispatched, hence the
 BAR degradation is detrimental, and we should prevent it. This is expected
 to conserve the 32-bit address space for 32-bit MMIO BARs.

 The driver added in this patch could be simplified based on the following
 facts:

 - In the Ia32 build, the 64-bit MMIO aperture is always zero-size, hence
  the driver will exit immediately. Therefore the driver could be omitted
  from the Ia32 build.

 - In the Ia32X64 and X64 builds, the driver could be omitted if CSM_ENABLE
  was defined (because in that case the degradation would be justified).
  On the other hand, if CSM_ENABLE was undefined, then the driver could be
  included, and it could provide the hint unconditionally (without looking
  for the Legacy BIOS protocol).

 These short-cuts are not taken because they would increase the differences
 between the OVMF DSC/FDF files. If we can manage without extreme
 complexity, we should use dynamic logic (vs. build time configuration),
 plus keep conditional compilation to a minimum.

 Cc: Jordan Justen 
 Cc: Ruiyu Ni 
 

Re: [edk2] [PATCH 1/2] NetworkPkg/HttpDxe: Don't free Wrap in HttpTcpReceiveNotifyDpc

[Laszlo Ersek]

On 05/19/16 10:39, Gary Lin wrote:
> Cc Samer and Nagaraj.
> I saw them with "git sent-email --dry-run". Wonder how they were dropped.

They were not dropped. The list software is trying to be smart.

Namely, if you go to ,
and click "Unsubscribe or edit options", you can toggle a number of
mailing list options for your subscriptions. One of these options is:

  Avoid duplicate copies of messages?

This affects how the list software reflects an email to you that you
were also directly addressed on.

Some people (including me) like to receive duplicate copies. Meaning, if
you CC me on a patch that you send to the list, I will get a direct copy
from you (due to the CC), plus the list software will send me another
copy (*because* I have enabled duplicate copies). I like to see one
instance of your patch in my INBOX, and another one in my edk2-devel
folder (which is supposed to be a complete archive).

However, others don't like to receive duplicates. They prefer the list
software not to send them a reflected message, if they are also on the
CC list directly. When the list software is deciding about reflecting a
message to subscriber X, and subscriber X has disabled duplicates, and
subscriber X is also on the CC list of the message that the list
software received from the submitter, then the list software assumes
subscriber X has the message anyway (directly from the submitter), and
will not reflect the message to subscriber X.

*But*, to actually answer your question, in this latter case something
else happens to. Namely, subscriber X is *also* removed from the CC list
of the message that the list software sends out to any other
subscribers. I guess the idea is, "subscriber X likes to receive exactly
one copy of each message on the list; so he got one copy of the thread
starter directly from the submitter, and he will get one copy of each
followup message too, sent by others, reflected by the list software".
If the CC were preserved in the reflected thread starter, then followup
messages would arrive to subscriber X both directly and reflected by the
list.

For the submitter, this CC munging is incredibly confusing and annoying.
But, if you know about the above, it is more tolerable.

As a side effect, you can now deduce that Samer and Nagaraj have *not*
enabled duplicate copies in their options. :)

HTH
Laszlo


> On Thu, May 19, 2016 at 11:49:17AM +0800, Gary Lin wrote:
>> The HTTP Token Wrap is created in EfiHttpResponse() and then passed
>> to the deferred Receive event callback, HttpTcpReceiveNotifyDpc.
>> HttpTcpReceiveHeader and HttpTcpReceiveBody use a Tcp polling loop to
>> monitor the socket status and trigger the Receive event when a new
>> packet arrives. The Receive event brings up HttpTcpReceiveNotifyDpc
>> to process the HTTP message and the function will set Wrap->TcpWrap.IsRxDone
>> to TRUE to break the Tcp polling loop.
>>
>> However, HttpTcpReceiveNotifyDpc mistakenly freed Wrap, so the Tcp
>> polling loop was actually checking a dead variable, and this led the
>> system into an unstable status.
>>
>> Given the fact that the HTTP Token Wrap will be freed in EfiHttpResponse
>> or HttpResponseWorker, this commit removes every "FreePool (Wrap)" in
>> HttpTcpReceiveNotifyDpc.
>>
>> Cc: "Wu, Jiaxin" 
>> Cc: "Siyuan Fu" 
>> Cc: "El-Haj-Mahmoud, Samer" 
>> Cc: "Laszlo Ersek" 
>> Cc: "Hegde, Nagaraj P" 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Gary Lin 
>> ---
>>  NetworkPkg/HttpDxe/HttpProto.c | 4 
>>  1 file changed, 4 deletions(-)
>>
>> diff --git a/NetworkPkg/HttpDxe/HttpProto.c b/NetworkPkg/HttpDxe/HttpProto.c
>> index f3992ed..afa7fe4 100644
>> --- a/NetworkPkg/HttpDxe/HttpProto.c
>> +++ b/NetworkPkg/HttpDxe/HttpProto.c
>> @@ -152,7 +152,6 @@ HttpTcpReceiveNotifyDpc (
>>  if (EFI_ERROR (Wrap->TcpWrap.Rx6Token.CompletionToken.Status)) {
>>Wrap->HttpToken->Status = 
>> Wrap->TcpWrap.Rx6Token.CompletionToken.Status;
>>gBS->SignalEvent (Wrap->HttpToken->Event);
>> -  FreePool (Wrap);
>>return ;
>>  }
>>  
>> @@ -162,7 +161,6 @@ HttpTcpReceiveNotifyDpc (
>>  if (EFI_ERROR (Wrap->TcpWrap.Rx4Token.CompletionToken.Status)) {
>>Wrap->HttpToken->Status = 
>> Wrap->TcpWrap.Rx4Token.CompletionToken.Status;
>>gBS->SignalEvent (Wrap->HttpToken->Event);
>> -  FreePool (Wrap);
>>return ;
>>  }
>>}
>> @@ -234,8 +232,6 @@ HttpTcpReceiveNotifyDpc (
>>// Check pending RxTokens and receive the HTTP message.
>>//
>>NetMapIterate (>HttpInstance->RxTokens, HttpTcpReceive, NULL);
>> -  
>> -  FreePool (Wrap);
>>  }
>>  
>>  /**
>> -- 
>> 2.8.2
>>
>> ___
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>>

Re: [edk2] edk2-staging/HTTPS-TLS

[Gao, Liming]

Ersek:
  I remember we discuss Rebase or Merge before. The conclusion is to keep the 
liner history in edk2 project. So, I think rebase is better. If we choose 
option 1, I suggest to setup mirror task in server and auto run regular sync. 
If the confliction happens, it sends the mail to edk dev list and let owner 
follow up. 

Thanks
Liming
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Thursday, May 19, 2016 4:31 PM
> To: Wu, Jiaxin ; El-Haj-Mahmoud, Samer  haj-mahm...@hpe.com>; Li, Ruth ; Ye, Ting
> ; Long, Qin ; edk2-
> de...@lists.01.org ; Justen, Jordan L
> ; Kinney, Michael D
> ; Leif Lindholm (Linaro address)
> 
> Subject: Re: [edk2] edk2-staging/HTTPS-TLS
> 
> On 05/19/16 05:35, Wu, Jiaxin wrote:
> > Hi all,
> > There are two ways to sync HTTPS-TLS branch with EDK2 master:
> >
> > 1. Sync all changes in EDK2 master to branch by rebasing the whole
> > feature branch. Need frequency request to sync edk2-staging/master to
> > latest edk2/master. This way can also keep branch code go straight
> > with edk2/master.
> >
> > 2. Only sync HTTPS/TLS related patches to branch (e.g. HttpDxe
> > update). That means to use one stable edk2 tag for
> > edk2-staging/HTTPS-TLS branch development. By this way, frequency
> > request to sync edk2-staging/master to latest edk2/master can be
> > avoided but maybe some bugs fixed in edk2/master can't be synced to
> > branch timely.
> >
> > Both of them are not easy since it's probable that each updates in
> > edk2 trunk may cause the conflicts with the HTTPS code (HttpDxe
> > driver and CryptyPkg especially).
> >
> > Effort is almost. Which one do you prefer?
> 
> I'm chiming in here not because I have a direct interest in the
> HTTPS-TLS feature set, but because this feature branch seems to be the
> first real-life test case of edk2-staging. I'd like to add three points.
> 
> (a) The ultimate goal of a staging branch is to be merged into edk2
> master when the feature is complete. If you try to save the effort now
> that would be necessary for rebasing the feature branch, you will go
> *mad* when you finally want to merge the staging branch into edk2
> master. The larger you allow the divergence to grow, the (non-linearly)
> harder time you will have when you try to do the final merge into edk2
> master. So, the trick is to stay sensibly close as possible to edk2
> master on the staging branch.
> 
> For this reason, option (1) is very strongly preferable. Option (2) --
> which is called "cherry picking" or "backporting" -- is really only an
> option for branches that are never meant to be merged back into the
> master branch. Given that HTTPS-TLS is not such a downstream branch
> (instead, it is a development, topic branch), option (2) doesn't apply.
> 
> (b) For option (1), there are actually two methods, rebasing and merging:
> 
> 1.1. Rebasing. It has the drawback that it throws away your prior
> development history on the feature (staging) branch. Many people don't
> like this. On the other hand, you can do it as frequently as you like,
> without creating a mess of commit history.
> 
> Here's a diagram:
> 
> master final merge
> *---*-*---**---*--**-->
>  \ \\ /
>   \ \\   /
>*-*-*-[discont.]  *-*-*-[discont.] *-*-*-*
>feature   rebased feature  rebased feature
> 
> 
> 1.2. Merging. It has the benfit that your development history on the
> feature (staging) branch will be preserved in edk2 master too. On the
> other hand, you have to be careful about the frequency of
> master-to-staging merges. If you do it very frequently, you'll end up
> with a complex history.
> 
> In the Linux community, merging is preferred, and a master-to-topic
> merge is advised whenever the master branch receives a feature or a
> bugfix that is important for the topic branch as well. That is, no
> spurious merges.
> 
> Here's a diagram:
> 
> master final merge
>back into master
> *---*-*---**---*--**-->
>  \ \\ /
>   \ \\   /
>*-*-*-*---**-- *-*-*-*
>feature   ^ merge from master  ^ another merge
>  triggered by from master
>  important new stuff
>  in master
> 
> I think that for edk2 staging branches, both 1.1. (= rebasing staging to
> master) and 1.2. (= merging 

Re: [edk2] [RFC] Structured PCD Proposal

[Gao, Liming]

Ersek:
  I add my comments. 

Thanks
Liming
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Thursday, May 19, 2016 5:26 PM
> To: Kinney, Michael D 
> Cc: edk2-devel@lists.01.org 
> Subject: Re: [edk2] [RFC] Structured PCD Proposal
> 
> On 05/19/16 01:28, Kinney, Michael D wrote:
> 
> [snip]
> 
> > ## C Structure Syntax Proposal
> >   * Use C typedef syntax to describe complex C structures
> >   * Use a C language parser with support for struct/union/array/bit
> fields/pack
> >   * Recommend use of libclang C language parser into Abstract Syntax Tree
> (AST)
> > - Included with LLVM release
> > - http://llvm.org/releases/download.html
> > - http://clang.llvm.org/doxygen/group__CINDEX.html
> >   * Recommend use of Python bindings for CLANG for EDK II Python based
> BaseTools
> > - pip install clang
> 
> What versions are necessary? On RHEL-7, the following versions are
> available (from EPEL only; RHEL does not provide clang):
> 
> clang-devel: 3.4.2-8.el7
> python-pip: 7.1.0-1.el7
> 
> >
> > ## DEC File Extensions
> >   * Declare link between PCD and C data structure and token range for
> fields
> >   * @Include path to include file with C typedef [Required]
> >   * Replace |VOID*| with name of C typedef
> >   * @TokenMap path to file with token number assignments [Optional]
> >   * Range of token numbers to assign to fields in C typedef [Required]
> >   * Example:
> >
> > ```
> > # @Include  Include/Pcd/AcpiLowerPowerIdleTable.h
> > # @TokenMap Include/Pcd/AcpiLowerPowerIdleTable.map
> >
> gPlatformTokenSpaceGuid.AcpiLowPowerIdleTable|{}|ACPI_LOW_POWER_I
> DLE_TABLE|0x00010080|0x0E000200-0x0E0002FF
> > ```
> 
> What does 0x00010080 mean here?
[Liming] It is PCD token space GUID. 0x0E000200-0x0E0002FF are for its field 
PCD token space GUID. 
> 
> >
> >   * Recommended File Paths
> > - /Include/Pcd/.h
> > - /Include/Pcd/.map  [Optional]
> > - /Include/Pcd/.uni  [Optional]
> >   * C Pre-Processor Support
> > - Use of #include, #define, #if supported
> > - #include limited to files in same package
> > - Including files from other packages being evaluated
> >
> > ## C Structure Syntax
> >   * Support struct/union/array with nesting
> >   * Support bit fields
> 
> Bit fields will require new PcdLib interaces, right?
[Liming] Yes. Bit Fields access will use new PcdFieldXXX APIs. 
> 
> [snip]
> 
> Thanks
> Laszlo
> ___
> 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 1/2] NetworkPkg/HttpDxe: Don't free Wrap in HttpTcpReceiveNotifyDpc

[Laszlo Ersek]

On 05/19/16 10:35, Gary Lin wrote:
> On Thu, May 19, 2016 at 10:01:16AM +0200, Laszlo Ersek wrote:
>> On 05/19/16 05:49, Gary Lin wrote:
>>> The HTTP Token Wrap is created in EfiHttpResponse() and then passed
>>> to the deferred Receive event callback, HttpTcpReceiveNotifyDpc.
>>> HttpTcpReceiveHeader and HttpTcpReceiveBody use a Tcp polling loop to
>>> monitor the socket status and trigger the Receive event when a new
>>> packet arrives. The Receive event brings up HttpTcpReceiveNotifyDpc
>>> to process the HTTP message and the function will set Wrap->TcpWrap.IsRxDone
>>> to TRUE to break the Tcp polling loop.
>>>
>>> However, HttpTcpReceiveNotifyDpc mistakenly freed Wrap, so the Tcp
>>> polling loop was actually checking a dead variable, and this led the
>>> system into an unstable status.
>>>
>>> Given the fact that the HTTP Token Wrap will be freed in EfiHttpResponse
>>> or HttpResponseWorker, this commit removes every "FreePool (Wrap)" in
>>> HttpTcpReceiveNotifyDpc.
>>>
>>> Cc: "Wu, Jiaxin" 
>>> Cc: "Siyuan Fu" 
>>> Cc: "El-Haj-Mahmoud, Samer" 
>>> Cc: "Laszlo Ersek" 
>>> Cc: "Hegde, Nagaraj P" 
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Gary Lin 
>>> ---
>>>  NetworkPkg/HttpDxe/HttpProto.c | 4 
>>>  1 file changed, 4 deletions(-)
>>>
>>> diff --git a/NetworkPkg/HttpDxe/HttpProto.c b/NetworkPkg/HttpDxe/HttpProto.c
>>> index f3992ed..afa7fe4 100644
>>> --- a/NetworkPkg/HttpDxe/HttpProto.c
>>> +++ b/NetworkPkg/HttpDxe/HttpProto.c
>>> @@ -152,7 +152,6 @@ HttpTcpReceiveNotifyDpc (
>>>  if (EFI_ERROR (Wrap->TcpWrap.Rx6Token.CompletionToken.Status)) {
>>>Wrap->HttpToken->Status = 
>>> Wrap->TcpWrap.Rx6Token.CompletionToken.Status;
>>>gBS->SignalEvent (Wrap->HttpToken->Event);
>>> -  FreePool (Wrap);
>>>return ;
>>>  }
>>>  
>>> @@ -162,7 +161,6 @@ HttpTcpReceiveNotifyDpc (
>>>  if (EFI_ERROR (Wrap->TcpWrap.Rx4Token.CompletionToken.Status)) {
>>>Wrap->HttpToken->Status = 
>>> Wrap->TcpWrap.Rx4Token.CompletionToken.Status;
>>>gBS->SignalEvent (Wrap->HttpToken->Event);
>>> -  FreePool (Wrap);
>>>return ;
>>>  }
>>>}
>>> @@ -234,8 +232,6 @@ HttpTcpReceiveNotifyDpc (
>>>// Check pending RxTokens and receive the HTTP message.
>>>//
>>>NetMapIterate (>HttpInstance->RxTokens, HttpTcpReceive, NULL);
>>> -  
>>> -  FreePool (Wrap);
>>>  }
>>>  
>>>  /**
>>>
>>
>> I'll let the NetworkPkg maintainers review this one; just one note /
>> question: didn't we recently encounter something similar elsewhere? I
>> vaguely recall a similar problem where it wasn't immediately obvious
>> whether the callback should free a UDP packet or the generic receive
>> function. But I cannot find the message :(
>>
> 
> Do you mean this?
> http://thread.gmane.org/gmane.comp.bios.edk2.devel/10156
> 
> The patch is simple but the sympton is nasty...

Yeah, that's it. Thanks for locating it! :)

... I guess the fact that others can find my past patches better than I
can is softly whispering in my ear that maybe I should work a bit less. Hm.

Laszlo

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

Re: [edk2] [RFC] Structured PCD Proposal

[Laszlo Ersek]

On 05/19/16 01:28, Kinney, Michael D wrote:

[snip]

> ## C Structure Syntax Proposal
>   * Use C typedef syntax to describe complex C structures
>   * Use a C language parser with support for struct/union/array/bit 
> fields/pack
>   * Recommend use of libclang C language parser into Abstract Syntax Tree 
> (AST)
> - Included with LLVM release
> - http://llvm.org/releases/download.html
> - http://clang.llvm.org/doxygen/group__CINDEX.html
>   * Recommend use of Python bindings for CLANG for EDK II Python based 
> BaseTools
> - pip install clang

What versions are necessary? On RHEL-7, the following versions are
available (from EPEL only; RHEL does not provide clang):

clang-devel: 3.4.2-8.el7
python-pip: 7.1.0-1.el7

> 
> ## DEC File Extensions
>   * Declare link between PCD and C data structure and token range for fields
>   * @Include path to include file with C typedef [Required]
>   * Replace |VOID*| with name of C typedef
>   * @TokenMap path to file with token number assignments [Optional]
>   * Range of token numbers to assign to fields in C typedef [Required]
>   * Example:
> 
> ```
> # @Include  Include/Pcd/AcpiLowerPowerIdleTable.h
> # @TokenMap Include/Pcd/AcpiLowerPowerIdleTable.map
> gPlatformTokenSpaceGuid.AcpiLowPowerIdleTable|{}|ACPI_LOW_POWER_IDLE_TABLE|0x00010080|0x0E000200-0x0E0002FF
> ```

What does 0x00010080 mean here?

> 
>   * Recommended File Paths
> - /Include/Pcd/.h
> - /Include/Pcd/.map  [Optional]
> - /Include/Pcd/.uni  [Optional]
>   * C Pre-Processor Support
> - Use of #include, #define, #if supported
> - #include limited to files in same package
> - Including files from other packages being evaluated
> 
> ## C Structure Syntax
>   * Support struct/union/array with nesting
>   * Support bit fields

Bit fields will require new PcdLib interaces, right?

[snip]

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

[edk2] [Patch] Vlv2TbltDevicePkg: Update BIOS ID to V0.92.

[lushifex]

Signed-off-by: lushifex 
---
 Vlv2TbltDevicePkg/BiosIdD.env| 2 +-
 Vlv2TbltDevicePkg/BiosIdR.env| 2 +-
 Vlv2TbltDevicePkg/BiosIdx64D.env | 2 +-
 Vlv2TbltDevicePkg/BiosIdx64R.env | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Vlv2TbltDevicePkg/BiosIdD.env b/Vlv2TbltDevicePkg/BiosIdD.env
index 28da4d3..80fe281 100644
--- a/Vlv2TbltDevicePkg/BiosIdD.env
+++ b/Vlv2TbltDevicePkg/BiosIdD.env
@@ -33,7 +33,7 @@
 BOARD_REV = 1
 OEM_ID= I32
 BUILD_TYPE= D
 
 BOARD_ID = BLAKCRB
-VERSION_MAJOR = 0091
+VERSION_MAJOR = 0092
 VERSION_MINOR = 01
diff --git a/Vlv2TbltDevicePkg/BiosIdR.env b/Vlv2TbltDevicePkg/BiosIdR.env
index 971f32d..219d2a2 100644
--- a/Vlv2TbltDevicePkg/BiosIdR.env
+++ b/Vlv2TbltDevicePkg/BiosIdR.env
@@ -33,7 +33,7 @@
 BOARD_REV = 1
 OEM_ID= I32
 BUILD_TYPE= R
 
 BOARD_ID = BLAKCRB
-VERSION_MAJOR = 0091
+VERSION_MAJOR = 0092
 VERSION_MINOR = 01
diff --git a/Vlv2TbltDevicePkg/BiosIdx64D.env b/Vlv2TbltDevicePkg/BiosIdx64D.env
index 061a991..a6fb2ed 100644
--- a/Vlv2TbltDevicePkg/BiosIdx64D.env
+++ b/Vlv2TbltDevicePkg/BiosIdx64D.env
@@ -23,8 +23,8 @@
 
 BOARD_REV = 1
 OEM_ID= X64
 BUILD_TYPE= D
 
-VERSION_MAJOR = 0091
+VERSION_MAJOR = 0092
 VERSION_MINOR = 01
 BOARD_ID = BBAYCRB 
diff --git a/Vlv2TbltDevicePkg/BiosIdx64R.env b/Vlv2TbltDevicePkg/BiosIdx64R.env
index 9334e55..fdf1356 100644
--- a/Vlv2TbltDevicePkg/BiosIdx64R.env
+++ b/Vlv2TbltDevicePkg/BiosIdx64R.env
@@ -23,8 +23,8 @@
 
 BOARD_REV = 1
 OEM_ID= X64
 BUILD_TYPE= R
 
-VERSION_MAJOR = 0091
+VERSION_MAJOR = 0092
 VERSION_MINOR = 01
 BOARD_ID = BBAYCRB 
-- 
2.6.2.windows.1


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

Re: [edk2] [Patch] MdeModulePkg/PciHostBridgeDxe: Add CpuArch protocol dependency

[Ni, Ruiyu]

Regards,
Ray

>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
>Ersek
>Sent: Thursday, May 19, 2016 4:08 PM
>To: Ni, Ruiyu ; edk2-de...@ml01.01.org
>Cc: Gao, Liming 
>Subject: Re: [edk2] [Patch] MdeModulePkg/PciHostBridgeDxe: Add CpuArch 
>protocol dependency
>
>On 05/19/16 09:17, Ruiyu Ni wrote:
>> The driver entry point calls gDS->SetMemorySpaceAttributes().
>> This interface may return EFI_NOT_AVAILABLE_YET when CPU Arch
>> protocol is not available.
>> So we need to list CpuArch protocol in its INF dependency section.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ruiyu Ni 
>> Cc: Liming Gao 
>> ---
>>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
>b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
>> index ab5d87e..d8b0439 100644
>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
>> @@ -52,4 +52,5 @@ [Protocols]
>>
>>  [Depex]
>>gEfiCpuIo2ProtocolGuid AND
>> -  gEfiMetronomeArchProtocolGuid
>> +  gEfiMetronomeArchProtocolGuid AND
>> +  gEfiCpuArchProtocolGuid
>>
>
>This reminds me of commit f9a8be423cdd5:
>
>> Because gDS->SetMemorySpaceAttributes() is ultimately implemented by
>> EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes() -- see
>> "MdeModulePkg/Core/Dxe/Gcd/Gcd.c" and "ArmPkg/Drivers/CpuDxe/" -- we
>> add the CPU architectural protocol to the module's DepEx.

I saw the ArmVirtPkg/PciHostBridge driver set the MMIO to WB when
PcdKludgeMapPciMmioAsCache is TRUE.
So ArmVirtQemu platform cannot use the MdeModulePkg/PciHostBridge.
Is my understanding right?
Do you have any solution?

>
>Reviewed-by: Laszlo Ersek 
>___
>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] OvmfPkg: prevent 64-bit MMIO BAR degradation if there is no CSM

[Ni, Ruiyu]

I agree.

Reviewed-by: Ruiyu Ni 


>-Original Message-
>From: Laszlo Ersek [mailto:ler...@redhat.com]
>Sent: Thursday, May 19, 2016 3:38 PM
>To: Ni, Ruiyu ; edk2-devel-01 
>Cc: Justen, Jordan L 
>Subject: Re: [edk2] [PATCH] OvmfPkg: prevent 64-bit MMIO BAR degradation if 
>there is no CSM
>
>On 05/19/16 04:02, Ni, Ruiyu wrote:
>> Laszlo,
>> All are good except: can you use EfiCreateProtocolNotifyEvent()
>> exposed by UefiLib?
>
>I am aware of that function, but I dislike it. The reason I dislike it
>is that (in my opinion) its internal error handling is inferior. For the
>return values of gBS->CreateEvent() and gBS->RegisterProtocolNotify(),
>is only uses ASSERT_EFI_ERROR(), and to the return value of
>gBS->SignalEvent(), it doesn't apply any check.
>
>I think the error handling in my code below is superior. CreateEvent()
>can legitimately fail with EFI_OUT_OF_RESOURCES. So can
>RegisterProtocolNotify().
>
>I wondered before how the error handling could be fixed in
>EfiCreateProtocolNotifyEvent(). One problem I see is that it returns an
>EFI_EVENT object, not an EFI_STATUS one.
>
>Given that EFI_EVENT is just a typedef for VOID*, we could return a NULL
>if something fails. However, in the library class header file, such a
>return value is not permitted:
>
>  @return The notification event that was created.
>
>This means that the ASSERT_EFI_ERROR() macro invocations in the function
>body actually conform to the specification: namely,
>EfiCreateProtocolNotifyEvent() is never supposed to fail (and I guess
>all of its callers rely on this).
>
>Too bad that this function specification doesn't actually reflect
>reality. (See the above EFI_OUT_OF_RESOURCES failure modes for
>CreateEvent() and RegisterProtocolNotify().)
>
>Which is, I guess, my ultimate reason for never using this utility
>function, and always open-coding CreateEvent() +
>RegisterProtocolNotify() + SignalEvent() with proper error checking
>(using an assert for SignalEvent(), which really can't fail, according
>to the UEFI spec).
>
>Thanks
>Laszlo
>
>>> -Original Message-
>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
>>> Laszlo Ersek
>>> Sent: Thursday, May 19, 2016 6:13 AM
>>> To: edk2-devel-01 
>>> Cc: Ni, Ruiyu ; Justen, Jordan L 
>>> 
>>> Subject: [edk2] [PATCH] OvmfPkg: prevent 64-bit MMIO BAR degradation if 
>>> there is no CSM
>>>
>>> According to edk2 commit
>>>
>>>  "MdeModulePkg/PciBus: do not improperly degrade resource"
>>>
>>> and to the EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL definition in the
>>> Platform Init 1.4a specification, a platform can provide such a protocol
>>> in order to influence the PCI resource allocation performed by the PCI Bus
>>> driver.
>>>
>>> In particular it is possible instruct the PCI Bus driver, with a
>>> "wildcard" hint, to allocate the 64-bit MMIO BARs of a device in 64-bit
>>> address space, regardless of whether the device features an option ROM.
>>>
>>> (By default, the PCI Bus driver considers an option ROM reason enough for
>>> allocating the 64-bit MMIO BARs in 32-bit address space. It cannot know if
>>> BDS will launch a legacy boot option, and under legacy boot, a legacy BIOS
>>> binary from a combined option ROM could be dispatched, and fail to access
>>> MMIO BARs in 64-bit address space.)
>>>
>>> In platform code we can ascertain whether a CSM is present or not. If not,
>>> then legacy BIOS binaries in option ROMs can't be dispatched, hence the
>>> BAR degradation is detrimental, and we should prevent it. This is expected
>>> to conserve the 32-bit address space for 32-bit MMIO BARs.
>>>
>>> The driver added in this patch could be simplified based on the following
>>> facts:
>>>
>>> - In the Ia32 build, the 64-bit MMIO aperture is always zero-size, hence
>>>  the driver will exit immediately. Therefore the driver could be omitted
>>>  from the Ia32 build.
>>>
>>> - In the Ia32X64 and X64 builds, the driver could be omitted if CSM_ENABLE
>>>  was defined (because in that case the degradation would be justified).
>>>  On the other hand, if CSM_ENABLE was undefined, then the driver could be
>>>  included, and it could provide the hint unconditionally (without looking
>>>  for the Legacy BIOS protocol).
>>>
>>> These short-cuts are not taken because they would increase the differences
>>> between the OVMF DSC/FDF files. If we can manage without extreme
>>> complexity, we should use dynamic logic (vs. build time configuration),
>>> plus keep conditional compilation to a minimum.
>>>
>>> Cc: Jordan Justen 
>>> Cc: Ruiyu Ni 
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Laszlo Ersek 
>>> ---
>>>
>>> Notes:
>>>This patch will make a difference only when Ray's patch at
>>>

Re: [edk2] [PATCH 1/2] NetworkPkg/HttpDxe: Don't free Wrap in HttpTcpReceiveNotifyDpc

[Gary Lin]

Cc Samer and Nagaraj.
I saw them with "git sent-email --dry-run". Wonder how they were dropped.

Gary Lin

On Thu, May 19, 2016 at 11:49:17AM +0800, Gary Lin wrote:
> The HTTP Token Wrap is created in EfiHttpResponse() and then passed
> to the deferred Receive event callback, HttpTcpReceiveNotifyDpc.
> HttpTcpReceiveHeader and HttpTcpReceiveBody use a Tcp polling loop to
> monitor the socket status and trigger the Receive event when a new
> packet arrives. The Receive event brings up HttpTcpReceiveNotifyDpc
> to process the HTTP message and the function will set Wrap->TcpWrap.IsRxDone
> to TRUE to break the Tcp polling loop.
> 
> However, HttpTcpReceiveNotifyDpc mistakenly freed Wrap, so the Tcp
> polling loop was actually checking a dead variable, and this led the
> system into an unstable status.
> 
> Given the fact that the HTTP Token Wrap will be freed in EfiHttpResponse
> or HttpResponseWorker, this commit removes every "FreePool (Wrap)" in
> HttpTcpReceiveNotifyDpc.
> 
> Cc: "Wu, Jiaxin" 
> Cc: "Siyuan Fu" 
> Cc: "El-Haj-Mahmoud, Samer" 
> Cc: "Laszlo Ersek" 
> Cc: "Hegde, Nagaraj P" 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Gary Lin 
> ---
>  NetworkPkg/HttpDxe/HttpProto.c | 4 
>  1 file changed, 4 deletions(-)
> 
> diff --git a/NetworkPkg/HttpDxe/HttpProto.c b/NetworkPkg/HttpDxe/HttpProto.c
> index f3992ed..afa7fe4 100644
> --- a/NetworkPkg/HttpDxe/HttpProto.c
> +++ b/NetworkPkg/HttpDxe/HttpProto.c
> @@ -152,7 +152,6 @@ HttpTcpReceiveNotifyDpc (
>  if (EFI_ERROR (Wrap->TcpWrap.Rx6Token.CompletionToken.Status)) {
>Wrap->HttpToken->Status = 
> Wrap->TcpWrap.Rx6Token.CompletionToken.Status;
>gBS->SignalEvent (Wrap->HttpToken->Event);
> -  FreePool (Wrap);
>return ;
>  }
>  
> @@ -162,7 +161,6 @@ HttpTcpReceiveNotifyDpc (
>  if (EFI_ERROR (Wrap->TcpWrap.Rx4Token.CompletionToken.Status)) {
>Wrap->HttpToken->Status = 
> Wrap->TcpWrap.Rx4Token.CompletionToken.Status;
>gBS->SignalEvent (Wrap->HttpToken->Event);
> -  FreePool (Wrap);
>return ;
>  }
>}
> @@ -234,8 +232,6 @@ HttpTcpReceiveNotifyDpc (
>// Check pending RxTokens and receive the HTTP message.
>//
>NetMapIterate (>HttpInstance->RxTokens, HttpTcpReceive, NULL);
> -  
> -  FreePool (Wrap);
>  }
>  
>  /**
> -- 
> 2.8.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

Re: [edk2] [PATCH 1/2] NetworkPkg/HttpDxe: Don't free Wrap in HttpTcpReceiveNotifyDpc

[Gary Lin]

On Thu, May 19, 2016 at 10:01:16AM +0200, Laszlo Ersek wrote:
> On 05/19/16 05:49, Gary Lin wrote:
> > The HTTP Token Wrap is created in EfiHttpResponse() and then passed
> > to the deferred Receive event callback, HttpTcpReceiveNotifyDpc.
> > HttpTcpReceiveHeader and HttpTcpReceiveBody use a Tcp polling loop to
> > monitor the socket status and trigger the Receive event when a new
> > packet arrives. The Receive event brings up HttpTcpReceiveNotifyDpc
> > to process the HTTP message and the function will set Wrap->TcpWrap.IsRxDone
> > to TRUE to break the Tcp polling loop.
> > 
> > However, HttpTcpReceiveNotifyDpc mistakenly freed Wrap, so the Tcp
> > polling loop was actually checking a dead variable, and this led the
> > system into an unstable status.
> > 
> > Given the fact that the HTTP Token Wrap will be freed in EfiHttpResponse
> > or HttpResponseWorker, this commit removes every "FreePool (Wrap)" in
> > HttpTcpReceiveNotifyDpc.
> > 
> > Cc: "Wu, Jiaxin" 
> > Cc: "Siyuan Fu" 
> > Cc: "El-Haj-Mahmoud, Samer" 
> > Cc: "Laszlo Ersek" 
> > Cc: "Hegde, Nagaraj P" 
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Gary Lin 
> > ---
> >  NetworkPkg/HttpDxe/HttpProto.c | 4 
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/NetworkPkg/HttpDxe/HttpProto.c b/NetworkPkg/HttpDxe/HttpProto.c
> > index f3992ed..afa7fe4 100644
> > --- a/NetworkPkg/HttpDxe/HttpProto.c
> > +++ b/NetworkPkg/HttpDxe/HttpProto.c
> > @@ -152,7 +152,6 @@ HttpTcpReceiveNotifyDpc (
> >  if (EFI_ERROR (Wrap->TcpWrap.Rx6Token.CompletionToken.Status)) {
> >Wrap->HttpToken->Status = 
> > Wrap->TcpWrap.Rx6Token.CompletionToken.Status;
> >gBS->SignalEvent (Wrap->HttpToken->Event);
> > -  FreePool (Wrap);
> >return ;
> >  }
> >  
> > @@ -162,7 +161,6 @@ HttpTcpReceiveNotifyDpc (
> >  if (EFI_ERROR (Wrap->TcpWrap.Rx4Token.CompletionToken.Status)) {
> >Wrap->HttpToken->Status = 
> > Wrap->TcpWrap.Rx4Token.CompletionToken.Status;
> >gBS->SignalEvent (Wrap->HttpToken->Event);
> > -  FreePool (Wrap);
> >return ;
> >  }
> >}
> > @@ -234,8 +232,6 @@ HttpTcpReceiveNotifyDpc (
> >// Check pending RxTokens and receive the HTTP message.
> >//
> >NetMapIterate (>HttpInstance->RxTokens, HttpTcpReceive, NULL);
> > -  
> > -  FreePool (Wrap);
> >  }
> >  
> >  /**
> > 
> 
> I'll let the NetworkPkg maintainers review this one; just one note /
> question: didn't we recently encounter something similar elsewhere? I
> vaguely recall a similar problem where it wasn't immediately obvious
> whether the callback should free a UDP packet or the generic receive
> function. But I cannot find the message :(
> 

Do you mean this?
http://thread.gmane.org/gmane.comp.bios.edk2.devel/10156

The patch is simple but the sympton is nasty...

Cheers,

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

Re: [edk2] edk2-staging/HTTPS-TLS

[Laszlo Ersek]

On 05/19/16 05:35, Wu, Jiaxin wrote:
> Hi all,
> There are two ways to sync HTTPS-TLS branch with EDK2 master:
> 
> 1. Sync all changes in EDK2 master to branch by rebasing the whole
> feature branch. Need frequency request to sync edk2-staging/master to
> latest edk2/master. This way can also keep branch code go straight
> with edk2/master.
> 
> 2. Only sync HTTPS/TLS related patches to branch (e.g. HttpDxe
> update). That means to use one stable edk2 tag for
> edk2-staging/HTTPS-TLS branch development. By this way, frequency
> request to sync edk2-staging/master to latest edk2/master can be
> avoided but maybe some bugs fixed in edk2/master can't be synced to
> branch timely.
> 
> Both of them are not easy since it's probable that each updates in
> edk2 trunk may cause the conflicts with the HTTPS code (HttpDxe
> driver and CryptyPkg especially).
> 
> Effort is almost. Which one do you prefer?

I'm chiming in here not because I have a direct interest in the
HTTPS-TLS feature set, but because this feature branch seems to be the
first real-life test case of edk2-staging. I'd like to add three points.

(a) The ultimate goal of a staging branch is to be merged into edk2
master when the feature is complete. If you try to save the effort now
that would be necessary for rebasing the feature branch, you will go
*mad* when you finally want to merge the staging branch into edk2
master. The larger you allow the divergence to grow, the (non-linearly)
harder time you will have when you try to do the final merge into edk2
master. So, the trick is to stay sensibly close as possible to edk2
master on the staging branch.

For this reason, option (1) is very strongly preferable. Option (2) --
which is called "cherry picking" or "backporting" -- is really only an
option for branches that are never meant to be merged back into the
master branch. Given that HTTPS-TLS is not such a downstream branch
(instead, it is a development, topic branch), option (2) doesn't apply.

(b) For option (1), there are actually two methods, rebasing and merging:

1.1. Rebasing. It has the drawback that it throws away your prior
development history on the feature (staging) branch. Many people don't
like this. On the other hand, you can do it as frequently as you like,
without creating a mess of commit history.

Here's a diagram:

master final merge
*---*-*---**---*--**-->
 \ \\ /
  \ \\   /
   *-*-*-[discont.]  *-*-*-[discont.] *-*-*-*
   feature   rebased feature  rebased feature


1.2. Merging. It has the benfit that your development history on the
feature (staging) branch will be preserved in edk2 master too. On the
other hand, you have to be careful about the frequency of
master-to-staging merges. If you do it very frequently, you'll end up
with a complex history.

In the Linux community, merging is preferred, and a master-to-topic
merge is advised whenever the master branch receives a feature or a
bugfix that is important for the topic branch as well. That is, no
spurious merges.

Here's a diagram:

master final merge
   back into master
*---*-*---**---*--**-->
 \ \\ /
  \ \\   /
   *-*-*-*---**-- *-*-*-*
   feature   ^ merge from master  ^ another merge
 triggered by from master
 important new stuff
 in master

I think that for edk2 staging branches, both 1.1. (= rebasing staging to
master) and 1.2. (= merging master into staging) are appropriate. Both
can facilitate the final merge back into master.

Option (2) (= cherry-picking patches from master to staging) is *not*
appropriate.

Thanks
Laszlo


> 
> Thanks.
> Jiaxin
> 
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of El-
>> Haj-Mahmoud, Samer
>> Sent: Thursday, May 19, 2016 6:10 AM
>> To: Wu, Jiaxin ; edk2-devel@lists.01.org
>> Subject: [edk2] edk2-staging/HTTPS-TLS
>>
>> Jiaxin,
>>
>> The HTTPs-TLS branch is behind EDK2 master, and they are quite a few
>> conflicts to try to keep up with both. Can you please sync the branch from
>> EDK2 master?
>>
>> Thanks,
>> --Samer
>>
>> ___
>> 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-devel mailing list

Re: [edk2] [Patch] MdeModulePkg/PciHostBridgeDxe: Add CpuArch protocol dependency

[Laszlo Ersek]

On 05/19/16 09:17, Ruiyu Ni wrote:
> The driver entry point calls gDS->SetMemorySpaceAttributes().
> This interface may return EFI_NOT_AVAILABLE_YET when CPU Arch
> protocol is not available.
> So we need to list CpuArch protocol in its INF dependency section.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ruiyu Ni 
> Cc: Liming Gao 
> ---
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf 
> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
> index ab5d87e..d8b0439 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
> @@ -52,4 +52,5 @@ [Protocols]
>  
>  [Depex]
>gEfiCpuIo2ProtocolGuid AND
> -  gEfiMetronomeArchProtocolGuid
> +  gEfiMetronomeArchProtocolGuid AND
> +  gEfiCpuArchProtocolGuid
> 

This reminds me of commit f9a8be423cdd5:

> Because gDS->SetMemorySpaceAttributes() is ultimately implemented by
> EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes() -- see
> "MdeModulePkg/Core/Dxe/Gcd/Gcd.c" and "ArmPkg/Drivers/CpuDxe/" -- we
> add the CPU architectural protocol to the module's DepEx.

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

Re: [edk2] [PATCH 1/2] NetworkPkg/HttpDxe: Don't free Wrap in HttpTcpReceiveNotifyDpc

[Laszlo Ersek]

On 05/19/16 05:49, Gary Lin wrote:
> The HTTP Token Wrap is created in EfiHttpResponse() and then passed
> to the deferred Receive event callback, HttpTcpReceiveNotifyDpc.
> HttpTcpReceiveHeader and HttpTcpReceiveBody use a Tcp polling loop to
> monitor the socket status and trigger the Receive event when a new
> packet arrives. The Receive event brings up HttpTcpReceiveNotifyDpc
> to process the HTTP message and the function will set Wrap->TcpWrap.IsRxDone
> to TRUE to break the Tcp polling loop.
> 
> However, HttpTcpReceiveNotifyDpc mistakenly freed Wrap, so the Tcp
> polling loop was actually checking a dead variable, and this led the
> system into an unstable status.
> 
> Given the fact that the HTTP Token Wrap will be freed in EfiHttpResponse
> or HttpResponseWorker, this commit removes every "FreePool (Wrap)" in
> HttpTcpReceiveNotifyDpc.
> 
> Cc: "Wu, Jiaxin" 
> Cc: "Siyuan Fu" 
> Cc: "El-Haj-Mahmoud, Samer" 
> Cc: "Laszlo Ersek" 
> Cc: "Hegde, Nagaraj P" 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Gary Lin 
> ---
>  NetworkPkg/HttpDxe/HttpProto.c | 4 
>  1 file changed, 4 deletions(-)
> 
> diff --git a/NetworkPkg/HttpDxe/HttpProto.c b/NetworkPkg/HttpDxe/HttpProto.c
> index f3992ed..afa7fe4 100644
> --- a/NetworkPkg/HttpDxe/HttpProto.c
> +++ b/NetworkPkg/HttpDxe/HttpProto.c
> @@ -152,7 +152,6 @@ HttpTcpReceiveNotifyDpc (
>  if (EFI_ERROR (Wrap->TcpWrap.Rx6Token.CompletionToken.Status)) {
>Wrap->HttpToken->Status = 
> Wrap->TcpWrap.Rx6Token.CompletionToken.Status;
>gBS->SignalEvent (Wrap->HttpToken->Event);
> -  FreePool (Wrap);
>return ;
>  }
>  
> @@ -162,7 +161,6 @@ HttpTcpReceiveNotifyDpc (
>  if (EFI_ERROR (Wrap->TcpWrap.Rx4Token.CompletionToken.Status)) {
>Wrap->HttpToken->Status = 
> Wrap->TcpWrap.Rx4Token.CompletionToken.Status;
>gBS->SignalEvent (Wrap->HttpToken->Event);
> -  FreePool (Wrap);
>return ;
>  }
>}
> @@ -234,8 +232,6 @@ HttpTcpReceiveNotifyDpc (
>// Check pending RxTokens and receive the HTTP message.
>//
>NetMapIterate (>HttpInstance->RxTokens, HttpTcpReceive, NULL);
> -  
> -  FreePool (Wrap);
>  }
>  
>  /**
> 

I'll let the NetworkPkg maintainers review this one; just one note /
question: didn't we recently encounter something similar elsewhere? I
vaguely recall a similar problem where it wasn't immediately obvious
whether the callback should free a UDP packet or the generic receive
function. But I cannot find the message :(

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

Re: [edk2] Using the Shell to launch a kernel using a RAMDISK

[Laszlo Ersek]

On 05/19/16 04:41, Tian, Feng wrote:
> Samer,
> 
> I am ok to add an item/option to allow user choose ramdisk memory
> type. But I don't know how to organize the form to show that
> straightforwardly, for example, how to add an item for those chosen
> by File Explorer. Look forward to see your patch:)

* I think on the first form that lists

  > Create raw
  > Create from file

There could be a persistent checkbox above these options, called

  Make next created RAM disk bootable [ ]

* Alternatively, given that there aren't too many options yet, it
shouldn't be a problem duplicating them:

  > Create raw (boot time only use)
  > Create raw (usable at runtime, bootable)
  > Create from file (boot time only use)
  > Create from file (usable at runtime, bootable)

* Whichever of the above two is chosen, it would be nice (if possible)
to display under

  Created RAM disk list:

which RAM disks live in reserved memory and which don't. (This should
really be optional though.)

Just some ideas, of course.

Thanks!
Laszlo


> -Original Message-
> From: El-Haj-Mahmoud, Samer [mailto:samer.el-haj-mahm...@hpe.com] 
> Sent: Wednesday, May 18, 2016 10:10 PM
> To: Laszlo Ersek ; Andrew Fish ; Foster, 
> Matthew I ; Tian, Feng 
> Cc: edk2-devel@lists.01.org ; El-Haj-Mahmoud, Samer 
> 
> Subject: RE: [edk2] Using the Shell to launch a kernel using a RAMDISK
> 
> Thanks Laszlo!
> 
> I missed that thread.
> 
> I did not participate in the original design either (where is the original 
> design anyway? Is it public on the EDK2 list)?
> 
> Feng,
> 
> Do you mind if I submit a patch to update the HII to support allocating 
> Reserved memory RAM Disks? This will help a lot in troubleshooting.
> 
> Thanks,
> --Samer
> 
> 
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
> Ersek
> Sent: Wednesday, May 18, 2016 9:04 AM
> To: El-Haj-Mahmoud, Samer ; Andrew Fish 
> ; Foster, Matthew I 
> Cc: edk2-devel@lists.01.org 
> Subject: Re: [edk2] Using the Shell to launch a kernel using a RAMDISK
> 
> On 05/18/16 15:48, El-Haj-Mahmoud, Samer wrote:
>> Does it make sense to update the RAM Disk HII (at
>> \MdeModulePkg\Universal\Disk\RamDiskDxe) to allow creating a RAM Disk 
>> of Type Reserved, for this kind of testing?
> 
> That's exactly what I suggested in
> 
> http://thread.gmane.org/gmane.comp.bios.edk2.devel/10766/focus=10770
> 
> Namely,
> 
>> [...] I think this question should be exposed with a checkbox on the
>> form: "will you want to boot from this RAM disk?" And then the answer 
>> to that question should determine the type of memory the RAM disk is 
>> allocated from.
> 
> but it was not approved; Feng said later in the thread,
> 
>> [...] For those ramdisk created from Ramdisk HII, the original design 
>> intention is only for boot time use.
> 
> I had not participated in the design, so I didn't question it.
> 
> Thanks
> Laszlo
> 
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
>> Laszlo Ersek
>> Sent: Wednesday, May 18, 2016 4:39 AM
>> To: Andrew Fish ; Foster, Matthew I 
>> 
>> Cc: edk2-devel@lists.01.org 
>> Subject: Re: [edk2] Using the Shell to launch a kernel using a RAMDISK
>>
>> On 05/18/16 01:16, Andrew Fish wrote:
>>>
 On May 17, 2016, at 4:08 PM, Foster, Matthew I 
  wrote:

 Thanks Ersek,

>>>
>>> Laszlo... 
>>
>> That's right, thanks Andrew :)
>>
 I did change my code to match what you have here as far as the 
 allocation. I did some more debugging and registered up a callback 
 in exit boot services, and did an integrity check of the memory I 
 previously allocated and put the ramdisk in. I checked the memory 
 right before I call loadimage (to launch the kernel), and then again 
 I check in the callback during exit boot services. So some point 
 between the memory actually does get changed. So it might be 
 something happening during exit boot services.

 Just a question, after allocating that memory as you mentioned 
 below, other code should not be able to allocate and use that same 
 address range right?
>>>
>>> The EFI Memory Map is well defined and you can read about it in 
>>> Section 6.2 Memory Allocation Services.
>>
>> Yeah, once allocated (and not freed), those pages should never be handed out 
>> to any other agent calling gBS->AllocatePool() or gBS->AllocatePages().
>>
>> The memory corruption issue is very interesting. I have faced similar 
>> earlier, several times. I've developed methods for trying to dealing 
>> with it ("develop methods" is a fancy expression for just a few 

Re: [edk2] [Patch 2/2] PcAtChipsetPkg/PcRtc: get century RTC address in entry point

[Laszlo Ersek]

On 05/19/16 04:27, Ni, Ruiyu wrote:
> Laszlo,
> 
> My understanding is: you have concern why I only call
> GetCenturyRtcAddress() in RTC driver’s entry point, but
> don’t write the century value to CMOS.
> Is my understanding correct?
> 
> Because RTC’s entry point calls PcRtcInit(), which calls
> PcRtcSetTime(), which writes the century value to CMOS,
> when the century RTC address is valid.

Thanks. I was missing the PcRtcInit() -> PcRtcSetTime() call.

Series
Reviewed-by: Laszlo Ersek 

> Before the patch, RTC driver doesn’t know the century
> RTC address in its entry point, so the address was assigned to
> zero. (It thought this address can only be known when the ACPI
> table callback was called. I guess my head was kicked very
> heavily when writing this codeL)

Haha, interesting expression :)

> But now with HP’s report, I found the driver could know the
> century RTC address in its entry point.
> 
> So I made such change. From my point of view, this change is very
> straightforward and simple to come up.

Yup, I agree.

Cheers
Laszlo

>>-Original Message-
>>From: Laszlo Ersek [mailto:ler...@redhat.com]
>>Sent: Wednesday, May 18, 2016 8:12 PM
>>To: Ni, Ruiyu ; edk2-de...@ml01.01.org
>>Cc: Anbazhagan Baraneedharan ; Zeng, Star 
>>
>>Subject: Re: [Patch 2/2] PcAtChipsetPkg/PcRtc: get century RTC address in 
>>entry point
>>
>>On 05/18/16 07:20, Ruiyu Ni wrote:
>>> When ACPI table is installed before PcRtc driver runs,
>>> the ACPI table installation callback isn't called which causes the
>>> century value isn't written to the CMOS.
>>> The patch calls GetCenturyRtcAddress() in entry point to fix
>>> the bug.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ruiyu Ni 
>>> Cc: Anbazhagan Baraneedharan 
>>> Cc: Laszlo Ersek 
>>> Cc: Star Zeng 
>>> ---
>>>  PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h  | 12 +++-
>>>  PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c |  4 ++--
>>>  2 files changed, 13 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h
>>b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h
>>> index 7fc19f9..ba6092d 100644
>>> --- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h
>>> +++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h
>>> @@ -1,7 +1,7 @@
>>>  /** @file
>>>Header file for real time clock driver.
>>>
>>> -Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.
>>> +Copyright (c) 2006 - 2016, 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
>>> @@ -360,6 +360,16 @@ IsLeapYear (
>>>);
>>>
>>>  /**
>>> +  Get the century RTC address from the ACPI FADT table.
>>> +
>>> +  @return  The century RTC address or 0 if not found.
>>> +**/
>>> +UINT8
>>> +GetCenturyRtcAddress (
>>> +  VOID
>>> +  );
>>> +
>>> +/**
>>>Notification function of ACPI Table change.
>>>
>>>This is a notification function registered on ACPI Table change event.
>>> diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c
>>b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c
>>> index 1cfb0cb..a61a35e 100644
>>> --- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c
>>> +++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c
>>> @@ -1,7 +1,7 @@
>>>  /** @file
>>>Provides Set/Get time operations.
>>>
>>> -Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.
>>> +Copyright (c) 2006 - 2016, 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
>>> @@ -135,7 +135,7 @@ InitializePcRtc (
>>>EFI_EVENT   Event;
>>>
>>>EfiInitializeLock (, TPL_CALLBACK);
>>> -  mModuleGlobal.CenturyRtcAddress = 0;
>>> +  mModuleGlobal.CenturyRtcAddress = GetCenturyRtcAddress ();
>>>
>>>Status = PcRtcInit ();
>>>ASSERT_EFI_ERROR (Status);
>>>
>>
>>Before the patches, the Century value is written to the CMOS in two cases:
>>
>>(1) if the ACPI platform driver is dispatched later, then for the first
>>time when the right ACPI tables are installed (and then immediately)
>>
>>(2) in the PcRtcSetTime() function (which is practically the
>>implementation of the SetTime() runtime service) if it is called after (1)
>>
>>After the patches, the Century value is written to the CMOS in three cases:
>>
>>(1) same as above
>>(2) same as above
>>(3) if the ACPI platform driver is dispatched earlier, then in all of
>>the PcRtcSetTime() calls
>>(4) the 

Re: [edk2] [PATCH] OvmfPkg: prevent 64-bit MMIO BAR degradation if there is no CSM

[Laszlo Ersek]

On 05/19/16 04:02, Ni, Ruiyu wrote:
> Laszlo,
> All are good except: can you use EfiCreateProtocolNotifyEvent()
> exposed by UefiLib?

I am aware of that function, but I dislike it. The reason I dislike it
is that (in my opinion) its internal error handling is inferior. For the
return values of gBS->CreateEvent() and gBS->RegisterProtocolNotify(),
is only uses ASSERT_EFI_ERROR(), and to the return value of
gBS->SignalEvent(), it doesn't apply any check.

I think the error handling in my code below is superior. CreateEvent()
can legitimately fail with EFI_OUT_OF_RESOURCES. So can
RegisterProtocolNotify().

I wondered before how the error handling could be fixed in
EfiCreateProtocolNotifyEvent(). One problem I see is that it returns an
EFI_EVENT object, not an EFI_STATUS one.

Given that EFI_EVENT is just a typedef for VOID*, we could return a NULL
if something fails. However, in the library class header file, such a
return value is not permitted:

  @return The notification event that was created.

This means that the ASSERT_EFI_ERROR() macro invocations in the function
body actually conform to the specification: namely,
EfiCreateProtocolNotifyEvent() is never supposed to fail (and I guess
all of its callers rely on this).

Too bad that this function specification doesn't actually reflect
reality. (See the above EFI_OUT_OF_RESOURCES failure modes for
CreateEvent() and RegisterProtocolNotify().)

Which is, I guess, my ultimate reason for never using this utility
function, and always open-coding CreateEvent() +
RegisterProtocolNotify() + SignalEvent() with proper error checking
(using an assert for SignalEvent(), which really can't fail, according
to the UEFI spec).

Thanks
Laszlo

>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
>> Laszlo Ersek
>> Sent: Thursday, May 19, 2016 6:13 AM
>> To: edk2-devel-01 
>> Cc: Ni, Ruiyu ; Justen, Jordan L 
>> 
>> Subject: [edk2] [PATCH] OvmfPkg: prevent 64-bit MMIO BAR degradation if 
>> there is no CSM
>>
>> According to edk2 commit
>>
>>  "MdeModulePkg/PciBus: do not improperly degrade resource"
>>
>> and to the EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL definition in the
>> Platform Init 1.4a specification, a platform can provide such a protocol
>> in order to influence the PCI resource allocation performed by the PCI Bus
>> driver.
>>
>> In particular it is possible instruct the PCI Bus driver, with a
>> "wildcard" hint, to allocate the 64-bit MMIO BARs of a device in 64-bit
>> address space, regardless of whether the device features an option ROM.
>>
>> (By default, the PCI Bus driver considers an option ROM reason enough for
>> allocating the 64-bit MMIO BARs in 32-bit address space. It cannot know if
>> BDS will launch a legacy boot option, and under legacy boot, a legacy BIOS
>> binary from a combined option ROM could be dispatched, and fail to access
>> MMIO BARs in 64-bit address space.)
>>
>> In platform code we can ascertain whether a CSM is present or not. If not,
>> then legacy BIOS binaries in option ROMs can't be dispatched, hence the
>> BAR degradation is detrimental, and we should prevent it. This is expected
>> to conserve the 32-bit address space for 32-bit MMIO BARs.
>>
>> The driver added in this patch could be simplified based on the following
>> facts:
>>
>> - In the Ia32 build, the 64-bit MMIO aperture is always zero-size, hence
>>  the driver will exit immediately. Therefore the driver could be omitted
>>  from the Ia32 build.
>>
>> - In the Ia32X64 and X64 builds, the driver could be omitted if CSM_ENABLE
>>  was defined (because in that case the degradation would be justified).
>>  On the other hand, if CSM_ENABLE was undefined, then the driver could be
>>  included, and it could provide the hint unconditionally (without looking
>>  for the Legacy BIOS protocol).
>>
>> These short-cuts are not taken because they would increase the differences
>> between the OVMF DSC/FDF files. If we can manage without extreme
>> complexity, we should use dynamic logic (vs. build time configuration),
>> plus keep conditional compilation to a minimum.
>>
>> Cc: Jordan Justen 
>> Cc: Ruiyu Ni 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek 
>> ---
>>
>> Notes:
>>This patch will make a difference only when Ray's patch at
>>
>>is committed.
>>
>> OvmfPkg/OvmfPkgIa32.dsc  |   
>> 1 +
>> OvmfPkg/OvmfPkgIa32X64.dsc   |   
>> 1 +
>> OvmfPkg/OvmfPkgX64.dsc   |   
>> 1 +
>> OvmfPkg/OvmfPkgIa32.fdf  |   
>> 1 +
>> OvmfPkg/OvmfPkgIa32X64.fdf

Re: [edk2] [patch] NetworkPkg: Refine codes of Http boot driver.

[Ye, Ting]

Reviewed-by: Ye Ting  

-Original Message-
From: Zhang, Lubo 
Sent: Thursday, May 19, 2016 11:53 AM
To: edk2-devel@lists.01.org
Cc: Ye, Ting ; Fu, Siyuan ; Wu, Jiaxin 

Subject: [patch] NetworkPkg: Refine codes of Http boot driver.

When downloading a big image as ram disk iso,we can print the progress message 
on screen to enhance the user experience.

Cc: Ye Ting 
Cc: Fu Siyuan 
Cc: Wu Jiaxin 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Zhang Lubo 
---
 NetworkPkg/HttpBootDxe/HttpBootClient.c  | 29 +  
NetworkPkg/HttpBootDxe/HttpBootSupport.c |  4 ++--
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/NetworkPkg/HttpBootDxe/HttpBootClient.c 
b/NetworkPkg/HttpBootDxe/HttpBootClient.c
index 46cf9ca..9b2a8bd 100644
--- a/NetworkPkg/HttpBootDxe/HttpBootClient.c
+++ b/NetworkPkg/HttpBootDxe/HttpBootClient.c
@@ -753,10 +753,12 @@ HttpBootGetBootFile (
   HTTP_BOOT_CACHE_CONTENT*Cache;
   UINT8  *Block;
   CHAR16 *Url;
   BOOLEANIdentityMode;
   UINTN  ReceivedSize;
+  UINTN  Ratio;
+  UINTN  NewRatio;
   
   ASSERT (Private != NULL);
   ASSERT (Private->HttpCreated);
 
   if (BufferSize == NULL || ImageType == NULL) { @@ -992,10 +994,13 @@ 
HttpBootGetBootFile (
 //
 // 3.4.2, start the message-body download, the identity and chunked 
transfer-coding
 // is handled in different path here.
 //
 ZeroMem (, sizeof (HTTP_IO_RESPONSE_DATA));
+AsciiPrint ("\n");
+AsciiPrint ("\n  Downloading NBP file... ");
+Ratio = 0;
 if (IdentityMode) {
   //
   // In identity transfer-coding there is no need to parse the message 
body,
   // just download the message body to the user provided buffer directly.
   //
@@ -1010,10 +1015,25 @@ HttpBootGetBootFile (
);
 if (EFI_ERROR (Status)) {
   goto ERROR_6;
 }
 ReceivedSize += ResponseBody.BodyLength;
+//
+// Display the download progress.
+//
+NewRatio = (ReceivedSize * 100) / ContentLength;
+if (NewRatio != Ratio) {
+  if (Ratio !=0) {
+if (Ratio >=10) {
+  AsciiPrint ("\b\b\b");
+} else {
+  AsciiPrint ("\b\b");
+}
+  }
+  Ratio = NewRatio;
+  AsciiPrint ("%d%%", NewRatio);
+}
   }
 } else {
   //
   // In "chunked" transfer-coding mode, so we need to parse the received
   // data to get the real entity content.
@@ -1058,10 +1078,19 @@ HttpBootGetBootFile (
ResponseBody.Body
);
 if (EFI_ERROR (Status)) {
   goto ERROR_6;
 }
+
+//
+// Print '.' when using chunked mode to download bootfile.
+//
+Ratio ++;
+if ((Ratio % 1024) == 0) {
+  AsciiPrint (".");
+}
+
   }
 }
   }
 
   //
diff --git a/NetworkPkg/HttpBootDxe/HttpBootSupport.c 
b/NetworkPkg/HttpBootDxe/HttpBootSupport.c
index 66eca78..71e4826 100644
--- a/NetworkPkg/HttpBootDxe/HttpBootSupport.c
+++ b/NetworkPkg/HttpBootDxe/HttpBootSupport.c
@@ -1121,11 +1121,11 @@ HttpBootRegisterRamDisk (
   ASSERT (Buffer != NULL);
   ASSERT (BufferSize != 0);
 
   Status = gBS->LocateProtocol (, NULL, (VOID**) 
);
   if (EFI_ERROR (Status)) {
-DEBUG ((EFI_D_ERROR, "HTTP Boot: Couldn't find the RAM Disk protocol - 
%r\n", Status));
+AsciiPrint ("\n  HTTP Boot: Couldn't find the RAM Disk protocol - 
+ %r\n", Status);
 return Status;
   }
 
   if (ImageType == ImageTypeVirtualCd) {
 RamDiskType = 
@@ -1141,11 +1141,11 @@ HttpBootRegisterRamDisk (
  RamDiskType,
  Private->UsingIpv6 ? Private->Ip6Nic->DevicePath : 
Private->Ip4Nic->DevicePath,
  
  );
   if (EFI_ERROR (Status)) {
-DEBUG ((EFI_D_ERROR, "HTTP Boot: Failed to register RAM Disk - %r\n", 
Status));
+AsciiPrint ("\n  HTTP Boot: Failed to register RAM Disk - %r\n", 
+ Status);
   }
 
   return Status;
 }
 
--
1.9.5.msysgit.1

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

Re: [edk2] System Table and more

[Laszlo Ersek]

On 05/19/16 01:27, Andrew Fish wrote:
> 
>> On May 18, 2016, at 2:57 PM, Laszlo Ersek  wrote:
>>
>> And, the availability of all of the arch protocols implies (as far as I
>> remember -- sorry I won't do a detailed tally now) that all gRT->
>> fields have been filled in with valid function pointers (by the DXE
>> drivers that produce the architectural protocols).
>>
>> (Explaining it more on the UEFI spec level: the runtime services are
>> guaranteed to exist for a UEFI driver or a UEFI application module.)
>>
>> I'm sure others will correct me if I made mistakes above.
> 
> The architectural protocols represent the generic hardware abstractions 
> required to provide both Boot and Runtime EFI services.  The reality is most 
> of the gRT services abstract hardware directly so they are patched. The gBS 
> services are more abstract, for example you can register for a timer event 
> prior to the gEfiTimerArchProtocolGuid AP (Architectural Protocol) getting 
> installed, but it will not start firing until the Timer AP is present.  
> 
> There is a good summary of APs in this global variable in the DXE Core:
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c#L27
> 
> EFI_CORE_PROTOCOL_NOTIFY_ENTRY  mArchProtocols[] = {
>   { , (VOID **),  NULL, 
> NULL, FALSE },
>   { ,  (VOID **),   NULL, 
> NULL, FALSE },
>   { ,(VOID **), NULL, 
> NULL, FALSE },
>   { ,(VOID **), NULL, 
> NULL, FALSE },
>   { ,  (VOID **),   NULL, 
> NULL, FALSE },
>   { ,(VOID **), NULL, 
> NULL, FALSE },
>   { ,  (VOID **),   NULL, 
> NULL, FALSE },
>   { , (VOID **)NULL,NULL, 
> NULL, FALSE },
>   { ,(VOID **)NULL,NULL, 
> NULL, FALSE },
>   { ,  (VOID **)NULL,NULL, 
> NULL, FALSE },
>   { , (VOID **)NULL,NULL, 
> NULL, FALSE },
>   { ,(VOID **)NULL,NULL, 
> NULL, FALSE },
>   { ,(VOID **)NULL,NULL, 
> NULL, FALSE },
>   { NULL,  (VOID **)NULL,NULL, 
> NULL, FALSE }
> };
> 
> The DXE Core provide functions for all gBS and gRT services. For things that 
> get patched later they have a CoreEfiNotAvailableYetArg1 like function so it 
> always safe to call, you may get EFI_NOT_AVAILABLE_YET if you call to early. 
> After ExitBootServices() is called some Boot Services things get NULLed out, 
> but at that point it is not legal to use those services. 
> 
> The simple answer is it is always safe to call a service as long as you 
> follow the ExitBootServices() rules, and that only applies for Runtime 
> drivers. 
> 
> If you look at the INF files and the Depex (Dependency Expressions) you will 
> notice it is common that a UEFI Driver will not have a Depex. Not having a 
> Depex means all the above architectural protocols are available and all of 
> the EFI services will just work. Some times people think no Depex is like a 
> Depex of TRUE. The reality is the Depex exist to solve all the chicken and 
> egg initializations problems in getting CPU/Chipset initialized and starting 
> up all the hardware required for EFI to function (Architectural Protocols). 

Thanks Andrew!
Laszlo

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

Re: [edk2] Driver dependency on boot

[Gao, Liming]

You can create ProtocolNotification function for gEfiCcidProtocolGuid in 
ccidboot driver. If so, you don't need add it into [Depex].
UefiLib EfiCreateProtocolNotifyEvent() API can help to create it. 

Thanks
Liming
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Iru
> Cai
> Sent: Thursday, May 19, 2016 2:35 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] Driver dependency on boot
> 
> Hi,
> 
> I'm having some dependency problem when developing on edk2. I've
> written a
> CCID device driver which provides a protocol EFI_CCID_PROTOCOL with GUID
> gEfiCcidProtocolGuid. And I have another application called ccidboot to be
> run on boot that uses gEfiCcidProtocolGuid, I set the module type as
> DXE_DRIVER.
> 
> If I add gEfiCcidProtocolGuid to the [Depex] of ccidboot, it works good
> when I have my CCID card plugged, but it'll not run if the card is
> unplugged. If I don't add this dependency, the ccidboot module will run
> before the driver starts the card. I don't know what I should do to solve
> this problem.
> 
> Thanks,
> Iru
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [Patch] MdeModulePkg/PciHostBridgeDxe: Add CpuArch protocol dependency

[Ruiyu Ni]

The driver entry point calls gDS->SetMemorySpaceAttributes().
This interface may return EFI_NOT_AVAILABLE_YET when CPU Arch
protocol is not available.
So we need to list CpuArch protocol in its INF dependency section.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni 
Cc: Liming Gao 
---
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf 
b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
index ab5d87e..d8b0439 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
@@ -52,4 +52,5 @@ [Protocols]
 
 [Depex]
   gEfiCpuIo2ProtocolGuid AND
-  gEfiMetronomeArchProtocolGuid
+  gEfiMetronomeArchProtocolGuid AND
+  gEfiCpuArchProtocolGuid
-- 
2.7.0.windows.1

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