[edk2] [Patch][edk2-platforms/devel-MinnowBoard3-UDK2017] Add UID for SueCreek LEDs.

[zwei4]

The _UID must be unique across all devices with a common _HID.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: zwei4 
---
 .../Common/Acpi/AcpiTablesPCAT/PlatformSsdt/SueCreek/SueCreekLeds.asl   | 2 ++
 1 file changed, 2 insertions(+)

diff --git 
a/Platform/BroxtonPlatformPkg/Common/Acpi/AcpiTablesPCAT/PlatformSsdt/SueCreek/SueCreekLeds.asl
 
b/Platform/BroxtonPlatformPkg/Common/Acpi/AcpiTablesPCAT/PlatformSsdt/SueCreek/SueCreekLeds.asl
index 2fcc45742..2f00f6795 100644
--- 
a/Platform/BroxtonPlatformPkg/Common/Acpi/AcpiTablesPCAT/PlatformSsdt/SueCreek/SueCreekLeds.asl
+++ 
b/Platform/BroxtonPlatformPkg/Common/Acpi/AcpiTablesPCAT/PlatformSsdt/SueCreek/SueCreekLeds.asl
@@ -15,6 +15,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 Scope(\_SB.PCI0.I2C5) {
   Device (LED0) {
 Name (_HID, "PCA9956")
+Name (_UID, 0)
 Name (_DDN, "SueCreekLed, CS0")
 Name (SBUF, ResourceTemplate () {
   I2cSerialBus (
@@ -42,6 +43,7 @@ Scope(\_SB.PCI0.I2C5) {
   }
   Device (LED1) {
 Name (_HID, "PCA9956")
+Name (_UID, 1)
 Name (_DDN, "SueCreekLed, CS0")
 Name (SBUF, ResourceTemplate () {
   I2cSerialBus (
-- 
2.14.1.windows.1

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

Re: [edk2] [Patch 3/3] MdeModulePkg/BootLogoLib: Use Boot Logo 2 Protocol

[Sean Brogan]

Mike,

Looking over the MdeModulePkg library changes it seems wasteful to call both 
protocols for Set.  In the previously submitted protocol implementation you 
actually allocate/free memory and copy the buffer in the set routines.   For a 
high resolution screen/logo this could be an expensive operation.

I would suggest that if BootLogo2 exists then maybe BootLogo can be ignored?



   if (!EFI_ERROR (Status)) {
-BootLogo->SetBootLogo (BootLogo, LogoBlt, LogoDestX, LogoDestY, LogoWidth, 
LogoHeight);
+if (BootLogo != NULL) {
+  BootLogo->SetBootLogo (BootLogo, LogoBlt, LogoDestX, LogoDestY, 
LogoWidth, LogoHeight);
+}
+if (BootLogo2 != NULL) {
+  BootLogo2->SetBootLogo (BootLogo2, LogoBlt, LogoDestX, LogoDestY, 
LogoWidth, LogoHeight);
+}
   }



Thanks

Sean



From: Kinney, Michael D 
Sent: Tuesday, February 13, 2018 6:07 PM
To: edk2-devel@lists.01.org
Cc: Sean Brogan; Bret Barkelew; Jiewen Yao; Star Zeng; Eric Dong; Michael D 
Kinney
Subject: [Patch 3/3] MdeModulePkg/BootLogoLib: Use Boot Logo 2 Protocol

https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D799=04%7C01%7Csean.brogan%40microsoft.com%7Cb3a0f58079524830920308d5734fb3d2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636541708515157356%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-1=p7ewroezUhc94MuBjA%2Fo9GziZ5wSpS9Yb876GKyzvuY%3D=0

Based on content from the following branch/commit:
https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMicrosoft%2FMS_UEFI%2Ftree%2Fshare%2FMsCapsuleSupport=04%7C01%7Csean.brogan%40microsoft.com%7Cb3a0f58079524830920308d5734fb3d2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636541708515157356%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-1=ByXtODKvhwrOV3mkK5MnCXLzYwFy9tKyIr16FRkDmB0%3D=0
https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMicrosoft%2FMS_UEFI%2Fcommit%2F33bab4031a417d7d5a7d356c15a14c2e60302b2d=04%7C01%7Csean.brogan%40microsoft.com%7Cb3a0f58079524830920308d5734fb3d2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636541708515157356%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-1=TWY5Okohr1%2FbJ1Wpyx12QF4ZgRikfvuCk%2FE6B3DT9sM%3D=0

Add check to see if the Boot Logo 2 Protocol is available
and attempt to set the location and size of the boot logo
using both the Boot Logo Protocol and the Boot Logo 2
Protocol.

Cc: Sean Brogan 
Cc: Bret Barkelew 
Cc: Jiewen Yao 
Cc: Star Zeng 
Cc: Eric Dong 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Michael D Kinney 
---
 MdeModulePkg/Library/BootLogoLib/BootLogoLib.c   | 18 +-
 MdeModulePkg/Library/BootLogoLib/BootLogoLib.inf |  2 ++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Library/BootLogoLib/BootLogoLib.c 
b/MdeModulePkg/Library/BootLogoLib/BootLogoLib.c
index 8bd9985cb2..9872f7eeea 100644
--- a/MdeModulePkg/Library/BootLogoLib/BootLogoLib.c
+++ b/MdeModulePkg/Library/BootLogoLib/BootLogoLib.c
@@ -3,6 +3,7 @@
   to show progress bar and LOGO.

 Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.
+Copyright (c) 2016, Microsoft Corporation
 This program and the accompanying materials are licensed and made available 
under
 the terms and conditions of the BSD License that accompanies this distribution.
 The full text of the license may be found at
@@ -26,6 +27,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include 
 #include 
 #include 
+#include 

 /**
   Show LOGO returned from Edkii Platform Logo protocol on all consoles.
@@ -56,6 +58,7 @@ BootLogoEnableLogo (
   UINT32RefreshRate;
   EFI_GRAPHICS_OUTPUT_PROTOCOL  *GraphicsOutput;
   EFI_BOOT_LOGO_PROTOCOL*BootLogo;
+  EDKII_BOOT_LOGO2_PROTOCOL *BootLogo2;
   UINTN NumberOfLogos;
   EFI_GRAPHICS_OUTPUT_BLT_PIXEL *LogoBlt;
   UINTN LogoDestX;
@@ -98,6 +101,14 @@ BootLogoEnableLogo (
 BootLogo = NULL;
   }

+  //
+  // Try to open Boot Logo 2 Protocol.
+  //
+  Status = gBS->LocateProtocol (, NULL, (VOID **) 
);
+  if (EFI_ERROR (Status)) {
+BootLogo2 = NULL;
+  }
+
   //
   // Erase Cursor from screen
   //
@@ -330,7 +341,12 @@ BootLogoEnableLogo (
   }

   if (!EFI_ERROR (Status)) {
-BootLogo->SetBootLogo (BootLogo, LogoBlt, LogoDestX, LogoDestY, LogoWidth, 
LogoHeight);
+if (BootLogo != NULL) {
+  BootLogo->SetBootLogo (BootLogo, LogoBlt, LogoDestX, LogoDestY, 
LogoWidth, LogoHeight);
+}
+if (BootLogo2 != NULL) {
+  BootLogo2->SetBootLogo (BootLogo2, 

[edk2] [Patch][edk2-platforms/devel-MinnowBoard3-UDK2017] IDTP9180 PMIC Power Sequence Configuration.

[zwei4]

Change Bit 2 (SUSPWRDNACKCFG) of Power Sequence Configuration register (offset 
0x2A) to 1.
If SUSPWRDNACKCFG is 0, SUSPWRDNACK signal is ignored. PMIC will not go to G3 
when SUSPWRDNACK goes high in S4 state.
If SUSPWRDNACKCFG is 1, PMIC responses to SUSPWRDNACK signal.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: zwei4 
---
 .../AuroraGlacier/BoardInitDxe/BoardInitDxe.c  | 114 
 .../AuroraGlacier/BoardInitDxe/BoardInitDxe.inf|   1 +
 .../BensonGlacier/BoardInitDxe/BoardInitDxe.c  | 115 -
 .../BensonGlacier/BoardInitDxe/BoardInitDxe.inf|   1 +
 .../PlatformSettings/PlatformDxe/PlatformDxe.inf   |   2 +-
 Platform/BroxtonPlatformPkg/PlatformPkg.fdf|   5 +
 Silicon/BroxtonSoC/BroxtonSiPkg/SiPkgDxe.dsc   |  15 ++-
 Silicon/BroxtonSoC/BroxtonSiPkg/SiPkgDxeLib.dsc|   6 +-
 .../BroxtonSiPkg/SouthCluster/Smbus/Dxe/PchSmbus.h |   2 +-
 .../SouthCluster/Smbus/Dxe/PchSmbusEntry.c |   2 +
 10 files changed, 258 insertions(+), 5 deletions(-)

diff --git 
a/Platform/BroxtonPlatformPkg/Board/AuroraGlacier/BoardInitDxe/BoardInitDxe.c 
b/Platform/BroxtonPlatformPkg/Board/AuroraGlacier/BoardInitDxe/BoardInitDxe.c
index e948594c8..f06a540eb 100644
--- 
a/Platform/BroxtonPlatformPkg/Board/AuroraGlacier/BoardInitDxe/BoardInitDxe.c
+++ 
b/Platform/BroxtonPlatformPkg/Board/AuroraGlacier/BoardInitDxe/BoardInitDxe.c
@@ -15,6 +15,7 @@
 **/
 
 #include "BoardInitDxe.h"
+#include 
 
 GET_BOARD_NAME mAuroraGetBoardNamePtr = AuroraGetBoardName;
 
@@ -38,6 +39,111 @@ AuroraGetBoardName (
 }
 
 
+VOID
+EFIAPI
+AuroraProgramPmicPowerSequence (
+  EFI_EVENT  Event,
+  VOID   *Context
+  )
+{
+  EFI_STATUSStatus;
+  EFI_SMBUS_DEVICE_ADDRESS  SlaveAddress;
+  EFI_SMBUS_DEVICE_COMMAND  Command;
+  UINTN Length;
+  UINT8 BufferData[1];
+  EFI_SMBUS_HC_PROTOCOL *SmbusControllerProtocol;
+  
+  //
+  // Programe IDTP9810 PMIC.
+  //
+  
+  DEBUG ((EFI_D_INFO, "Programe PMIC. \n"));
+  
+  //
+  // Locate SMBus protocol
+  //
+  Status  = gBS->LocateProtocol (, NULL, (VOID 
**));
+  ASSERT_EFI_ERROR(Status);
+  
+  SlaveAddress.SmbusDeviceAddress = (0xBC >> 1); // 0x5E
+  Command = 0x00; // Offset
+  Length  = 1;
+  
+  //
+  // Read one byte
+  //
+  Status = SmbusControllerProtocol->Execute ( 
+  SmbusControllerProtocol,
+  SlaveAddress,
+  Command,
+  EfiSmbusReadByte,
+  FALSE,
+  ,
+  BufferData
+  );
+  
+  
+  DEBUG ((EFI_D_INFO, "PMIC Vendor ID = %0x. \n", (UINT32) BufferData[0]));
+  
+
+  SlaveAddress.SmbusDeviceAddress = (0xBC >> 1); // 0x5E
+  Command = 0x2A; // Offset
+  Length  = 1;
+  
+  //
+  // Read one byte
+  //
+  Status = SmbusControllerProtocol->Execute ( 
+  SmbusControllerProtocol,
+  SlaveAddress,
+  Command,
+  EfiSmbusReadByte,
+  FALSE,
+  ,
+  BufferData
+  );
+  
+  
+  DEBUG ((EFI_D_INFO, "PMIC Power Sequence Configuration  Offset 0x2A 
PWRSEQCFG = %0x. \n", (UINT32) BufferData[0])); 
+
+  //
+  // Set Bit 2 (SUSPWRDNACKCFG) of PWRSEQCFG.
+  // 0 = SUSPWRDNACK signal is ignored. PMIC will not go to G3 when 
SUSPWRDNACK goes high in S4 state.
+  // 1 = PMIC responses to SUSPWRDNACK signal.
+  //
+  //
+  BufferData[0] = BufferData[0] | 0x04;
+  Status = SmbusControllerProtocol->Execute ( 
+  SmbusControllerProtocol,
+  SlaveAddress,
+  Command,
+  EfiSmbusWriteByte,
+  FALSE,
+  ,
+  BufferData
+  );
+ DEBUG ((EFI_D_INFO, "PMIC Power Sequence Configuration  Set Bit 2 
(SUSPWRDNACKCFG) of PWRSEQCFG. \n")); 
+
+
+  //
+  // Read one byte
+  //
+  Status = SmbusControllerProtocol->Execute ( 
+  SmbusControllerProtocol,
+  SlaveAddress,
+  Command,
+  EfiSmbusReadByte,
+  FALSE,
+  ,
+  BufferData
+  );
+  
+  
+  DEBUG ((EFI_D_INFO, "PMIC Power Sequence Configuration  Offset 0x2A 
PWRSEQCFG = %0x. 

[edk2] [Patch 2/3] MdeModulePkg/BootGraphicsResourceDxe: Add Boot Logo 2 Protocol

[Kinney, Michael D]

From: Michael D Kinney 

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

Based on content from the following branch/commit:
https://github.com/Microsoft/MS_UEFI/tree/share/MsCapsuleSupport
https://github.com/Microsoft/MS_UEFI/commit/33bab4031a417d7d5a7d356c15a14c2e60302b2d

Update BootGraphicsResourceDxe to produce both the Boot Logo
Protocol and the Boot Logo 2 Protocol.

The Boot Logo 2 Protocol service GetBootLogo() is amended
to return the pointer to the GOP BLT buffer previously
registered with the SetBootLogo() service.

Cc: Sean Brogan 
Cc: Bret Barkelew 
Cc: Jiewen Yao 
Cc: Star Zeng 
Cc: Eric Dong 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Michael D Kinney 
---
 .../BootGraphicsResourceTableDxe.c | 208 +++--
 .../BootGraphicsResourceTableDxe.inf   |   2 +
 2 files changed, 196 insertions(+), 14 deletions(-)

diff --git 
a/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.c
 
b/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.c
index 118fb4a922..85e79c6ce6 100644
--- 
a/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.c
+++ 
b/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.c
@@ -2,6 +2,7 @@
   This module install ACPI Boot Graphics Resource Table (BGRT).
 
   Copyright (c) 2011 - 2018, Intel Corporation. All rights reserved.
+  Copyright (c) 2016, Microsoft Corporation
   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
@@ -18,6 +19,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -33,20 +35,19 @@
 /**
   Update information of logo image drawn on screen.
 
-  @param  This   The pointer to the Boot Logo protocol instance.
-  @param  BltBuffer  The BLT buffer for logo drawn on screen. If BltBuffer
- is set to NULL, it indicates that logo image is no
- longer on the screen.
-  @param  DestinationX   X coordinate of destination for the BltBuffer.
-  @param  DestinationY   Y coordinate of destination for the BltBuffer.
-  @param  Width  Width of rectangle in BltBuffer in pixels.
-  @param  Height Hight of rectangle in BltBuffer in pixels.
-
-  @retval EFI_SUCCESS The boot logo information was updated.
-  @retval EFI_INVALID_PARAMETER   One of the parameters has an invalid value.
-  @retval EFI_OUT_OF_RESOURCESThe logo information was not updated due to
-  insufficient memory resources.
+  @param[in] This  The pointer to the Boot Logo protocol 2 instance.
+  @param[in] BltBuffer The BLT buffer for logo drawn on screen. If 
BltBuffer
+   is set to NULL, it indicates that logo image is no
+   longer on the screen.
+  @param[in] DestinationX  X coordinate of destination for the BltBuffer.
+  @param[in] DestinationY  Y coordinate of destination for the BltBuffer.
+  @param[in] Width Width of rectangle in BltBuffer in pixels.
+  @param[in] HeightHight of rectangle in BltBuffer in pixels.
 
+  @retval EFI_SUCCESSThe boot logo information was updated.
+  @retval EFI_INVALID_PARAMETER  One of the parameters has an invalid value.
+  @retval EFI_OUT_OF_RESOURCES   The logo information was not updated due to
+ insufficient memory resources.
 **/
 EFI_STATUS
 EFIAPI
@@ -59,6 +60,69 @@ SetBootLogo (
   IN UINTN  Height
   );
 
+/**
+  Update information of logo image drawn on screen.
+
+  @param[in] This  The pointer to the Boot Logo protocol 2 instance.
+  @param[in] BltBuffer The BLT buffer for logo drawn on screen. If 
BltBuffer
+   is set to NULL, it indicates that logo image is no
+   longer on the screen.
+  @param[in] DestinationX  X coordinate of destination for the BltBuffer.
+  @param[in] DestinationY  Y coordinate of destination for the BltBuffer.
+  @param[in] Width Width of rectangle in BltBuffer in pixels.
+  @param[in] HeightHight of rectangle in BltBuffer in pixels.
+
+  @retval EFI_SUCCESSThe boot logo information was updated.
+  @retval EFI_INVALID_PARAMETER  One of the parameters has an invalid value.
+  @retval EFI_OUT_OF_RESOURCES   The logo information was not updated due to
+ insufficient memory resources.
+**/
+EFI_STATUS
+EFIAPI
+SetBootLogo2 (
+  IN EDKII_BOOT_LOGO2_PROTOCOL  *This,
+  IN EFI_GRAPHICS_OUTPUT_BLT_PIXEL  *BltBuffer 

[edk2] [Patch 0/3] MdeModulePkg: Add Boot Logo 2 Protocol

[Kinney, Michael D]

Branch for review:
https://github.com/mdkinney/edk2/tree/Bug_799_BootLogo2Protocol_V3

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

Based on content from the following branch/commit:
https://github.com/Microsoft/MS_UEFI/tree/share/MsCapsuleSupport
https://github.com/Microsoft/MS_UEFI/commit/33bab4031a417d7d5a7d356c15a14c2e60302b2d

Add new Boot Logo 2 Protocol that adds a GetBootLogo()
service that can be used to retrieve the GOP BLT buffer,
location, and size of the boot logo that was previously
registered with the SetBootLogo() service.

The Boot Logo 2 Protocol service GetBootLogo() is amended
to return the pointer to the GOP BLT buffer previously
registered with the SetBootLogo() service.

Cc: Sean Brogan 
Cc: Bret Barkelew 
Cc: Jiewen Yao 
Cc: Star Zeng 
Cc: Eric Dong 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Michael D Kinney 

Kinney, Michael D (2):
  MdeModulePkg: Add Boot Logo 2 Protocol
  MdeModulePkg/BootLogoLib: Use Boot Logo 2 Protocol

Michael D Kinney (1):
  MdeModulePkg/BootGraphicsResourceDxe: Add Boot Logo 2 Protocol

 MdeModulePkg/Include/Protocol/BootLogo2.h  | 114 +++
 MdeModulePkg/Library/BootLogoLib/BootLogoLib.c |  18 +-
 MdeModulePkg/Library/BootLogoLib/BootLogoLib.inf   |   2 +
 MdeModulePkg/MdeModulePkg.dec  |   3 +
 .../BootGraphicsResourceTableDxe.c | 208 +++--
 .../BootGraphicsResourceTableDxe.inf   |   2 +
 6 files changed, 332 insertions(+), 15 deletions(-)
 create mode 100644 MdeModulePkg/Include/Protocol/BootLogo2.h

-- 
2.14.2.windows.3

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

[edk2] [Patch 1/3] MdeModulePkg: Add Boot Logo 2 Protocol

[Kinney, Michael D]

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

Based on content from the following branch/commit:
https://github.com/Microsoft/MS_UEFI/tree/share/MsCapsuleSupport
https://github.com/Microsoft/MS_UEFI/commit/33bab4031a417d7d5a7d356c15a14c2e60302b2d

Add new Boot Logo 2 Protocol that adds a GetBootLogo()
service that can be used to retrieve the GOP BLT buffer,
location, and size of the boot logo that was previously
registered with the SetBootLogo() service.

The Boot Logo 2 Protocol service GetBootLogo() is amended
to return the pointer to the GOP BLT buffer previously
registered with the SetBootLogo() service.

Cc: Sean Brogan 
Cc: Bret Barkelew 
Cc: Jiewen Yao 
Cc: Star Zeng 
Cc: Eric Dong 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Michael D Kinney 
---
 MdeModulePkg/Include/Protocol/BootLogo2.h | 114 ++
 MdeModulePkg/MdeModulePkg.dec |   3 +
 2 files changed, 117 insertions(+)
 create mode 100644 MdeModulePkg/Include/Protocol/BootLogo2.h

diff --git a/MdeModulePkg/Include/Protocol/BootLogo2.h 
b/MdeModulePkg/Include/Protocol/BootLogo2.h
new file mode 100644
index 00..91318c8adb
--- /dev/null
+++ b/MdeModulePkg/Include/Protocol/BootLogo2.h
@@ -0,0 +1,114 @@
+/**
+Boot Logo 2 Protocol is used to convey information of Logo dispayed during 
boot.
+
+Copyright (c) 2016, Microsoft Corporation
+Copyright (c) 2018, Intel Corporation. All rights reserved.
+
+All rights reserved.
+Redistribution and use in source and binary forms, with or without
+modification, are permitted provided that the following conditions are met:
+1. Redistributions of source code must retain the above copyright notice,
+this list of conditions and the following disclaimer.
+2. Redistributions in binary form must reproduce the above copyright notice,
+this list of conditions and the following disclaimer in the documentation
+ and/or other materials provided with the distribution.
+
+THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
+ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE 
DISCLAIMED.
+IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY 
DIRECT,
+INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
+BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE
+OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+**/
+
+#ifndef _BOOT_LOGO2_H_
+#define _BOOT_LOGO2_H_
+
+#include 
+
+#define EDKII_BOOT_LOGO2_PROTOCOL_GUID \
+  { \
+0x4b5dc1df, 0x1eaa, 0x48b2, { 0xa7, 0xe9, 0xea, 0xc4, 0x89, 0xa0, 0xb, 
0x5c } \
+  }
+
+//
+// Forward reference for pure ANSI compatability
+//
+typedef struct _EDKII_BOOT_LOGO2_PROTOCOL EDKII_BOOT_LOGO2_PROTOCOL;
+
+/**
+  Update information of logo image drawn on screen.
+
+  @param[in] This  The pointer to the Boot Logo protocol 2 instance.
+  @param[in] BltBuffer The BLT buffer for logo drawn on screen. If 
BltBuffer
+   is set to NULL, it indicates that logo image is no
+   longer on the screen.
+  @param[in] DestinationX  X coordinate of destination for the BltBuffer.
+  @param[in] DestinationY  Y coordinate of destination for the BltBuffer.
+  @param[in] Width Width of rectangle in BltBuffer in pixels.
+  @param[in] HeightHight of rectangle in BltBuffer in pixels.
+
+  @retval EFI_SUCCESSThe boot logo information was updated.
+  @retval EFI_INVALID_PARAMETER  One of the parameters has an invalid value.
+  @retval EFI_OUT_OF_RESOURCES   The logo information was not updated due to
+ insufficient memory resources.
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EDKII_SET_BOOT_LOGO2)(
+  IN EDKII_BOOT_LOGO2_PROTOCOL  *This,
+  IN EFI_GRAPHICS_OUTPUT_BLT_PIXEL  *BltBuffer   OPTIONAL,
+  IN UINTN  DestinationX,
+  IN UINTN  DestinationY,
+  IN UINTN  Width,
+  IN UINTN  Height
+  );
+
+/**
+  Get the location of the boot logo on the screen.
+
+  @param[in]  This  The pointer to the Boot Logo Protocol 2 instance
+  @param[out] BltBuffer Returns pointer to the GOP BLT buffer that was
+previously registered with SetBootLogo2(). The
+buffer returned must not be modified or freed.
+  @param[out] DestinationX  Returns the X start position of the GOP BLT buffer
+   

[edk2] [Patch 3/3] MdeModulePkg/BootLogoLib: Use Boot Logo 2 Protocol

[Kinney, Michael D]

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

Based on content from the following branch/commit:
https://github.com/Microsoft/MS_UEFI/tree/share/MsCapsuleSupport
https://github.com/Microsoft/MS_UEFI/commit/33bab4031a417d7d5a7d356c15a14c2e60302b2d

Add check to see if the Boot Logo 2 Protocol is available
and attempt to set the location and size of the boot logo
using both the Boot Logo Protocol and the Boot Logo 2
Protocol.

Cc: Sean Brogan 
Cc: Bret Barkelew 
Cc: Jiewen Yao 
Cc: Star Zeng 
Cc: Eric Dong 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Michael D Kinney 
---
 MdeModulePkg/Library/BootLogoLib/BootLogoLib.c   | 18 +-
 MdeModulePkg/Library/BootLogoLib/BootLogoLib.inf |  2 ++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Library/BootLogoLib/BootLogoLib.c 
b/MdeModulePkg/Library/BootLogoLib/BootLogoLib.c
index 8bd9985cb2..9872f7eeea 100644
--- a/MdeModulePkg/Library/BootLogoLib/BootLogoLib.c
+++ b/MdeModulePkg/Library/BootLogoLib/BootLogoLib.c
@@ -3,6 +3,7 @@
   to show progress bar and LOGO.
 
 Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.
+Copyright (c) 2016, Microsoft Corporation
 This program and the accompanying materials are licensed and made available 
under
 the terms and conditions of the BSD License that accompanies this distribution.
 The full text of the license may be found at
@@ -26,6 +27,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include 
 #include 
 #include 
+#include 
 
 /**
   Show LOGO returned from Edkii Platform Logo protocol on all consoles.
@@ -56,6 +58,7 @@ BootLogoEnableLogo (
   UINT32RefreshRate;
   EFI_GRAPHICS_OUTPUT_PROTOCOL  *GraphicsOutput;
   EFI_BOOT_LOGO_PROTOCOL*BootLogo;
+  EDKII_BOOT_LOGO2_PROTOCOL *BootLogo2;
   UINTN NumberOfLogos;
   EFI_GRAPHICS_OUTPUT_BLT_PIXEL *LogoBlt;
   UINTN LogoDestX;
@@ -98,6 +101,14 @@ BootLogoEnableLogo (
 BootLogo = NULL;
   }
 
+  //
+  // Try to open Boot Logo 2 Protocol.
+  //
+  Status = gBS->LocateProtocol (, NULL, (VOID **) 
);
+  if (EFI_ERROR (Status)) {
+BootLogo2 = NULL;
+  }
+
   //
   // Erase Cursor from screen
   //
@@ -330,7 +341,12 @@ BootLogoEnableLogo (
   }
 
   if (!EFI_ERROR (Status)) {
-BootLogo->SetBootLogo (BootLogo, LogoBlt, LogoDestX, LogoDestY, LogoWidth, 
LogoHeight);
+if (BootLogo != NULL) {
+  BootLogo->SetBootLogo (BootLogo, LogoBlt, LogoDestX, LogoDestY, 
LogoWidth, LogoHeight);
+}
+if (BootLogo2 != NULL) {
+  BootLogo2->SetBootLogo (BootLogo2, LogoBlt, LogoDestX, LogoDestY, 
LogoWidth, LogoHeight);
+}
   }
   FreePool (LogoBlt);
 
diff --git a/MdeModulePkg/Library/BootLogoLib/BootLogoLib.inf 
b/MdeModulePkg/Library/BootLogoLib/BootLogoLib.inf
index 79b5fc511a..47969cc05a 100644
--- a/MdeModulePkg/Library/BootLogoLib/BootLogoLib.inf
+++ b/MdeModulePkg/Library/BootLogoLib/BootLogoLib.inf
@@ -3,6 +3,7 @@
 #  to show progress bar and logo.
 #  
 #  Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.
+#  Copyright (c) 2016, Microsoft Corporation
 #  This program and the accompanying materials are licensed and made available 
under
 #  the terms and conditions of the BSD License that accompanies this 
distribution.
 #  The full text of the license may be found at
@@ -49,6 +50,7 @@ [Protocols]
   gEfiGraphicsOutputProtocolGuid## SOMETIMES_CONSUMES
   gEfiUgaDrawProtocolGuid |PcdUgaConsumeSupport ## SOMETIMES_CONSUMES
   gEfiBootLogoProtocolGuid  ## SOMETIMES_CONSUMES  
+  gEdkiiBootLogo2ProtocolGuid   ## SOMETIMES_CONSUMES
   gEfiUserManagerProtocolGuid   ## CONSUMES
   gEdkiiPlatformLogoProtocolGuid## CONSUMES
 
-- 
2.14.2.windows.3

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

Re: [edk2] [PATCH] MdeModulePkg/ResetUtilityLib: Fix GCC build failure

[Kinney, Michael D]

Reviewed-by: Michael D Kinney 

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org]
> On Behalf Of Zeng, Star
> Sent: Monday, February 12, 2018 11:28 PM
> To: Ni, Ruiyu ; edk2-
> de...@lists.01.org
> Cc: Bi, Dandan ; Zeng, Star
> 
> Subject: Re: [edk2] [PATCH] MdeModulePkg/ResetUtilityLib:
> Fix GCC build failure
> 
> Reviewed-by: Star Zeng 
> 
> Thanks,
> Star
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org]
> On Behalf Of Ruiyu Ni
> Sent: Tuesday, February 13, 2018 11:03 AM
> To: edk2-devel@lists.01.org
> Cc: Bi, Dandan 
> Subject: [edk2] [PATCH] MdeModulePkg/ResetUtilityLib: Fix
> GCC build failure
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni 
> Cc: Dandan Bi 
> ---
>  MdeModulePkg/Library/ResetUtilityLib/ResetUtility.c | 4
> ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git
> a/MdeModulePkg/Library/ResetUtilityLib/ResetUtility.c
> b/MdeModulePkg/Library/ResetUtilityLib/ResetUtility.c
> index 90de94ca9b..da5fa7b094 100644
> --- a/MdeModulePkg/Library/ResetUtilityLib/ResetUtility.c
> +++ b/MdeModulePkg/Library/ResetUtilityLib/ResetUtility.c
> @@ -1,7 +1,7 @@
>  /** @file
>This contains the business logic for the module-
> specific Reset Helper functions.
> 
> -  Copyright (c) 2017 Intel Corporation. All rights
> reserved.
> +  Copyright (c) 2017 - 2018 Intel Corporation. All
> rights reserved.
>Copyright (c) 2016 Microsoft Corporation. All rights
> reserved.
> 
>This program and the accompanying materials are
> licensed and made available under @@ -100,7 +100,7 @@
> GetResetPlatformSpecificGuid (
>//
>if ((ResetDataStringSize < DataSize) && (DataSize -
> ResetDataStringSize) >= sizeof (GUID)) {
>  ResetSubtypeGuid = (GUID *)((UINT8 *)ResetData +
> ResetDataStringSize);
> -DEBUG ((DEBUG_VERBOSE, __FUNCTION__" - Detected
> reset subtype %g...\n", ResetSubtypeGuid));
> +DEBUG ((DEBUG_VERBOSE, "%a - Detected reset subtype
> %g...\n",
> + __FUNCTION__, ResetSubtypeGuid));
>  return ResetSubtypeGuid;
>}
>return NULL;
> --
> 2.16.1.windows.1
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [edk2-platforms Patch] Platform: Add SafeIntLib and BmpSupportLib to DSC files

[Kinney, Michael D]

From: Michael D Kinney 

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

Based on content from the following branch/commits:
https://github.com/Microsoft/MS_UEFI/tree/share/MsCapsuleSupport
https://github.com/Microsoft/MS_UEFI/commit/33bab4031a417d7d5a7d356c15a14c2e60302b2d
https://github.com/Microsoft/MS_UEFI/commit/ca516b1a61315c2d823f453e12d2135098f53d61
https://github.com/Microsoft/MS_UEFI/commit/2b9f111f2e74a4c2ef4c4e32379e111f016dbd9b

BootGraphicsResourceTableDxe, GenericBdsLib, and CapsuleLib
use BmpSupportLib and SafeIntLib to convert a GOP BLT
buffers to a BMP graphics images and BMP graphics images to
GOP BLT buffers.  Add library mappings for these new
library classes.

Cc: Sean Brogan 
Cc: Jiewen Yao 
Cc: Ard Biesheuvel 
Cc: Leif Lindholm 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Michael D Kinney 
---
 Platform/AMD/OverdriveBoard/OverdriveBoard.dsc   | 2 ++
 Platform/Hisilicon/D02/Pv660D02.dsc  | 2 ++
 Platform/Hisilicon/D03/D03.dsc   | 2 ++
 Platform/Hisilicon/D05/D05.dsc   | 2 ++
 Platform/Socionext/DeveloperBox/DeveloperBox.dsc | 2 ++
 Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc | 2 ++
 6 files changed, 12 insertions(+)

diff --git a/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc 
b/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
index 48018abc6..5e879b16f 100644
--- a/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
+++ b/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
@@ -49,6 +49,8 @@ [LibraryClasses.common]
   
DebugPrintErrorLevelLib|MdePkg/Library/BaseDebugPrintErrorLevelLib/BaseDebugPrintErrorLevelLib.inf
 
   BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
+  SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
+  BmpSupportLib|MdeModulePkg/Library/BaseBmpSupportLib/BaseBmpSupportLib.inf
   
SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
   
PerformanceLib|MdePkg/Library/BasePerformanceLibNull/BasePerformanceLibNull.inf
   PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
diff --git a/Platform/Hisilicon/D02/Pv660D02.dsc 
b/Platform/Hisilicon/D02/Pv660D02.dsc
index 9e826aeb6..c6e539c0c 100644
--- a/Platform/Hisilicon/D02/Pv660D02.dsc
+++ b/Platform/Hisilicon/D02/Pv660D02.dsc
@@ -65,6 +65,8 @@ [LibraryClasses.common]
 
   CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
   GenericBdsLib|IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf
+  SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
+  BmpSupportLib|MdeModulePkg/Library/BaseBmpSupportLib/BaseBmpSupportLib.inf
   
PlatformBdsLib|Silicon/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
   
CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
 
diff --git a/Platform/Hisilicon/D03/D03.dsc b/Platform/Hisilicon/D03/D03.dsc
index c49630637..e922f98d3 100644
--- a/Platform/Hisilicon/D03/D03.dsc
+++ b/Platform/Hisilicon/D03/D03.dsc
@@ -66,6 +66,8 @@ [LibraryClasses.common]
   
PlatformSysCtrlLib|Silicon/Hisilicon/Hi1610/Library/PlatformSysCtrlLibHi1610/PlatformSysCtrlLibHi1610.inf
 
   GenericBdsLib|IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf
+  SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
+  BmpSupportLib|MdeModulePkg/Library/BaseBmpSupportLib/BaseBmpSupportLib.inf
   
PlatformBdsLib|Silicon/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
   
BmcConfigBootLib|Silicon/Hisilicon/Library/BmcConfigBootLib/BmcConfigBootLib.inf
   
UefiBootManagerLib|MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf
diff --git a/Platform/Hisilicon/D05/D05.dsc b/Platform/Hisilicon/D05/D05.dsc
index 0792b0814..a0922b388 100644
--- a/Platform/Hisilicon/D05/D05.dsc
+++ b/Platform/Hisilicon/D05/D05.dsc
@@ -82,6 +82,8 @@ [LibraryClasses.common]
   
PlatformSysCtrlLib|Silicon/Hisilicon/Hi1616/Library/PlatformSysCtrlLibHi1616/PlatformSysCtrlLibHi1616.inf
 
   GenericBdsLib|IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf
+  SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
+  BmpSupportLib|MdeModulePkg/Library/BaseBmpSupportLib/BaseBmpSupportLib.inf
   
PlatformBdsLib|Silicon/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
   
BmcConfigBootLib|Silicon/Hisilicon/Library/BmcConfigBootLib/BmcConfigBootLib.inf
   
UefiBootManagerLib|MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf
diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc 
b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
index 7e69eaba9..fdfaadd97 100644
--- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
+++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
@@ -75,6 +75,8 @@ [LibraryClasses.common]
   

Re: [edk2] error C0DE: Unknown fatal error when processing

[Laszlo Ersek]

Hi Hristo,

On 02/13/18 16:44, Hristo Mihaylov wrote:
> Hello,
> 
> I just hit this and it told me to send you the stack trace. I think it's
> due to the fact that I created an empty FDF file.
> 
> ```
> hrimih@lnxclnt2002:~/work/edk2.git$ build clean && build && cp
> Build/Hvmf/DEBUG_GCC5/X64/HelloWorld.efi ~/work/ovmf/hda-contents

[...]

> Active Platform  = /home/hrimih/work/edk2.git/HvmfPkg/HvmfPkg.dsc
> Flash Image Definition   = /home/hrimih/work/edk2.git/HvmfPkg/HvmfPkg.fdf

If you only want to build EFI binaries in your platform DSC file, then
you don't need an FDF file. Simply remove the FLASH_DEFINITION entry
from the [Defines] section of the DSC file.

edk2 contains several DSC files that only build EFI binaries, and no
flash images; for example:
- AppPkg/AppPkg.dsc
- ShellPkg/ShellPkg.dsc


If you want to build only one EFI file (regardless of whether you have a
FLASH_DEFINITION setting in your DSC file, or not), then you can pass the

  --module=Relative/Path/To/Module.inf

option to the "build" utility. Then "build" will only build "Module.inf".

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

Re: [edk2] [PATCH edk2-platforms] AMD, Hisilicon, Socionext: fix build after DxeCapsuleLibFmp changes

[Kinney, Michael D]

Hi Leif,

I forgot to send the edk2-platforms patch that I had prepared
that goes with the BmpSupportLib patch on edk2/master.

I found a few more DSC file that needed updates.

I will send out my patch.

Mike

> -Original Message-
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> Sent: Tuesday, February 13, 2018 8:46 AM
> To: edk2-devel@lists.01.org
> Cc: ard.biesheu...@linaro.org; Kinney, Michael D
> 
> Subject: [PATCH edk2-platforms] AMD, Hisilicon,
> Socionext: fix build after DxeCapsuleLibFmp changes
> 
> edk2 commit 1ec2e7d0e8db
> ("MdeModulePkg/DxeCapsuleLibFmp: Use BmpSupportLib")
> broke the build of
> all platforms that include DxeCapsuleLibFmp, since none
> of them included
> a BmpSupportLib (added as part of the same series).
> 
> BmpSupportLib itself depends on SafeIntLib, so add the
> two libraries to
> all affected platforms.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Leif Lindholm 
> ---
>  Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
> | 2 ++
>  Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> | 2 ++
> 
> Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.
> dsc | 2 ++
>  Silicon/Hisilicon/Hisilicon.dsc.inc
> | 2 ++
>  4 files changed, 8 insertions(+)
> 
> diff --git
> a/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
> b/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
> index 48018abc69..7d85b78642 100644
> --- a/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
> +++ b/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
> @@ -49,6 +49,8 @@ [LibraryClasses.common]
> 
> DebugPrintErrorLevelLib|MdePkg/Library/BaseDebugPrintErro
> rLevelLib/BaseDebugPrintErrorLevelLib.inf
> 
>BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
> +
> BmpSupportLib|MdeModulePkg/Library/BaseBmpSupportLib/Base
> BmpSupportLib.inf
> +
> SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.i
> nf
> 
> SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/
> BaseSynchronizationLib.inf
> 
> PerformanceLib|MdePkg/Library/BasePerformanceLibNull/Base
> PerformanceLibNull.inf
>PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
> diff --git
> a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> index 7e69eaba9b..3c109b495f 100644
> --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> @@ -75,6 +75,8 @@ [LibraryClasses.common]
> 
> ArmGenericTimerCounterLib|ArmPkg/Library/ArmGenericTimerP
> hyCounterLib/ArmGenericTimerPhyCounterLib.inf
> 
>BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
> +
> BmpSupportLib|MdeModulePkg/Library/BaseBmpSupportLib/Base
> BmpSupportLib.inf
> +
> SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.i
> nf
> 
> SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/
> BaseSynchronizationLib.inf
> 
> PerformanceLib|MdePkg/Library/BasePerformanceLibNull/Base
> PerformanceLibNull.inf
>PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
> diff --git
> a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoar
> d.dsc
> b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoar
> d.dsc
> index e35c17f0bc..45ab2afc40 100644
> ---
> a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoar
> d.dsc
> +++
> b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoar
> d.dsc
> @@ -75,6 +75,8 @@ [LibraryClasses.common]
> 
> ArmGenericTimerCounterLib|ArmPkg/Library/ArmGenericTimerP
> hyCounterLib/ArmGenericTimerPhyCounterLib.inf
> 
>BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
> +
> BmpSupportLib|MdeModulePkg/Library/BaseBmpSupportLib/Base
> BmpSupportLib.inf
> +
> SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.i
> nf
> 
> SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/
> BaseSynchronizationLib.inf
> 
> PerformanceLib|MdePkg/Library/BasePerformanceLibNull/Base
> PerformanceLibNull.inf
>PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
> diff --git a/Silicon/Hisilicon/Hisilicon.dsc.inc
> b/Silicon/Hisilicon/Hisilicon.dsc.inc
> index 57668294cb..1d6d20ca1d 100644
> --- a/Silicon/Hisilicon/Hisilicon.dsc.inc
> +++ b/Silicon/Hisilicon/Hisilicon.dsc.inc
> @@ -22,6 +22,8 @@ [LibraryClasses.common]
> 
> DebugPrintErrorLevelLib|MdePkg/Library/BaseDebugPrintErro
> rLevelLib/BaseDebugPrintErrorLevelLib.inf
> 
>BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
> +
> BmpSupportLib|MdeModulePkg/Library/BaseBmpSupportLib/Base
> BmpSupportLib.inf
> +
> SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.i
> nf
> 
> SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/
> BaseSynchronizationLib.inf
> 
> PerformanceLib|MdePkg/Library/BasePerformanceLibNull/Base
> PerformanceLibNull.inf
>PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
> --
> 2.11.0

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

Re: [edk2] [Patch] MdePkg/BaseSafeIntLib: Add SafeIntLib class and instance

[Bret Barkelew]

Ah, yes, this makes more sense. It was difficult to review all your comments on 
my phone.

Thanks for clarifying!

- Bret

From: Laszlo Ersek
Sent: Tuesday, February 13, 2018 9:29 AM
To: Bret Barkelew; Kinney, Michael 
D; Sean 
Brogan
Cc: edk2-devel@lists.01.org; Gao, 
Liming
Subject: Re: [edk2] [Patch] MdePkg/BaseSafeIntLib: Add SafeIntLib class and 
instance

On 02/13/18 17:56, Bret Barkelew wrote:
> In response to the original question, I would content that our goal
> should be "a". We should be allowing universal detection of errors
> without the caller having to carry this detection code itself.

OK.

The question is how the detection is implemented internally. Is that
implementation (in edk2) allowed to rely on behavior that the ISO C
standard leaves undefined, and -- consequently -- compilers might
exploit for code generation?

> The analog would be the safe string functions: if a buffer overflow
> occurs, they don't find a way to "fix" the operation, but they
> faithfully report an error.

Precisely.

Correspondingly, translated to the safe string functions, my question
becomes:

Is the implementation of the safe string functions allowed to employ
buffer overflow internally, and detect the overflow after the fact?

> As such, I believe from my review that these functions work as
> intended.

Please let me quote the function again, from
"MdePkg/Library/BaseSafeIntLib/SafeIntLib.c":

  3831  RETURN_STATUS
  3832  EFIAPI
  3833  SafeInt64Sub (
  3834IN  INT64  Minuend,
  3835IN  INT64  Subtrahend,
  3836OUT INT64  *Result
  3837)
  3838  {
  3839RETURN_STATUS  Status;
  3840INT64  SignedResult;
  3841
  3842if (Result == NULL) {
  3843  return RETURN_INVALID_PARAMETER;
  3844}
  3845
  3846SignedResult = Minuend - Subtrahend;
  3847
  3848//
  3849// Subtracting a positive number from a positive number never 
overflows.
  3850// Subtracting a negative number from a negative number never 
overflows.
  3851// If you subtract a negative number from a positive number, you 
expect a positive result.
  3852// If you subtract a positive number from a negative number, you 
expect a negative result.
  3853// Overflow if inputs vary in sign and the output does not have the 
same sign as the first input.
  3854//
  3855if (((Minuend < 0) != (Subtrahend < 0)) &&
  3856((Minuend < 0) != (SignedResult < 0))) {
  3857  *Result = INT64_ERROR;
  3858  Status = RETURN_BUFFER_TOO_SMALL;
  3859} else {
  3860  *Result = SignedResult;
  3861  Status = RETURN_SUCCESS;
  3862}
  3863
  3864return Status;
  3865  }

On line 3846, integer overflow is possible. If that happens, not only is
the resultant value of "SignedResult" undefined, but the behavior of the
rest of the function is undefined, according to ISO C.

In other words, just because we move the checking into a library
function, we cannot check *after the fact*. The subtraction on line 3846
can invoke undefined behavior *first*, and we check only afterwards,
starting on line 3855.

Here's an analogy. Various C compilers regularly equate "undefined
behavior" with "never happens" (which is a valid interpretation of the
ISO C standard for the compiler to make). For example, given the code

  int f(int *x)
  {
*x = 3;
if (x == NULL) {
  return 1;
}
return 0;
}

the compiler may compile f() to unconditionally return 0, such as:

  int f(int *x)
  {
*x = 3;
return 0;
  }

Because, the (x == NULL) branch would depend on undefined behavior
invoked by the assignment to *x. Given that the (*x = 3) assignment is
undefined for (x==NULL), according to ISO C, the subsequent (x == NULL)
check can be taken as constant false, and eliminated.

Similarly, a sufficiently smart compiler may assume that the subtraction
on line 3846 will never overflow. (Because, such an overflow would be
undefined behavior.) Consequently, it may deduce that the overflow
checks, *after the fact*, evaluate to constant false, and can be
eliminated. It might compile the function as in:

  {
RETURN_STATUS  Status;
INT64  SignedResult;

if (Result == NULL) {
  return RETURN_INVALID_PARAMETER;
}

SignedResult = Minuend - Subtrahend;

*Result = SignedResult;
Status = RETURN_SUCCESS;

return Status;
  }

I apoligize if I'm unclear; I really don't know how to put it better.
The subtraction on line 3846 runs the risk of undefined behavior, and
the checks starting on line 3855 are too late.

Thanks
Laszlo

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

Re: [edk2] [Patch] MdePkg/BaseSafeIntLib: Add SafeIntLib class and instance

[Laszlo Ersek]

On 02/13/18 18:51, Laszlo Ersek wrote:

> So, my point is, we should be aware of what ISO C says about integer
> overflow, and then pick one:
>
> - we target strict ISO C compliance (wrt. integer arithmetic) with
> SafeIntLib -- in which case a re-evaluation and patches are necessary,
>
> - or else we define additional C language guarantees, and then we
> *ensure* those via compiler flags, universally.

Specifically, see "-fwrapv" for GCC:

   -fwrapv
   This option instructs the compiler to assume that signed
   arithmetic overflow of addition, subtraction and
   multiplication wraps around using twos-complement
   representation.  This flag enables some optimizations and
   disables others.  This option is enabled by default for the
   Java front end, as required by the Java language
   specification.

It is used by QEMU, for example. Citing the "configure" script:

> # default flags for all hosts
> # We use -fwrapv to tell the compiler that we require a C dialect where
> # left shift of signed integers is well defined and has the expected
> # 2s-complement style results. (Both clang and gcc agree that it
> # provides these semantics.)
> QEMU_CFLAGS="-fno-strict-aliasing -fno-common -fwrapv $QEMU_CFLAGS"

edk2 doesn't use "-fwrapv" (yet?), and I'm not sure an equivalent flag
exists for VS / MSVC at all.

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

Re: [edk2] [Patch] MdePkg/BaseSafeIntLib: Add SafeIntLib class and instance

[Laszlo Ersek]

On 02/13/18 18:15, Andrew Fish wrote:
> 
> 
>> On Feb 13, 2018, at 8:56 AM, Bret Barkelew
>> > wrote:
>>
>> In response to the original question, I would content that our goal
>> should be "a". We should be allowing universal detection of errors
>> without the caller having to carry this detection code itself.
>>
>> The analog would be the safe string functions: if a buffer overflow
>> occurs, they don't find a way to "fix" the operation, but they
>> faithfully report an error.
>>
>> As such, I believe from my review that these functions work as intended.
>>
> 
> Bret,
> 
> I think Lazlo's point is that undefined behavior[1]  can cause the math
> function to break in the future and that we have to be very pedantic in
> how it is coded.

That's *exactly* my point, yes.

> Per the C standard it is legal for the compiler to
> optimized away undefined behavior[2], and clang is very aggressive about
> warning on undefined behavior and then updating the optimizer to remove
> the code in a future release.

Thank you for the independent confirmation :) (I'm reading and answering
your email after sending my previous one.)

> For example the BaseTool compression code
> broke with Xcode 9 recently due to the presence of an illegal 32-bit
> shift that was only hit when the optimizer inlined the function. While
> the compiler tries to emit warnings, or at least traps, for undefined
> behavior what we have seen with the Link Time Optimization is the code
> can just get removed. 

Huge kudos for this example!

> 
> [1] - Kind of clangs point of view on undefined behavior in
> C: http://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html
> [2] - Example of undefined behavior in clang that emits a trap.
> Dereferencing NULL is undefined behavior in C so clang emits a trap, and
> optimizes way the code after the trap. 
> 
> ~/work/Compiler>cat undefined.c
> 
> int
> main ()
> {
>   int *Yikes = 0;
> 
>   *Yikes = 1;
>   return 0;
> }
> 
> ~/work/Compiler>clang -S -Os undefined.c
> ~/work/Compiler>cat undefined.S
> .section__TEXT,__text,regular,pure_instructions
> .macosx_version_min 10, 12
> .globl_main
> _main:                                  ## @main
> .cfi_startproc
> ## BB#0:
> pushq%rbp
> Lcfi0:
> .cfi_def_cfa_offset 16
> Lcfi1:
> .cfi_offset %rbp, -16
> movq%rsp, %rbp
> Lcfi2:
> .cfi_def_cfa_register %rbp
> ud2
> .cfi_endproc
> 
> 
> .subsections_via_symbols

and for this one.

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

Re: [edk2] [Patch] MdePkg/BaseSafeIntLib: Add SafeIntLib class and instance

[Ard Biesheuvel]

On 13 February 2018 at 17:51, Laszlo Ersek  wrote:
> On 02/13/18 18:18, Ard Biesheuvel wrote:
>> On 13 February 2018 at 16:17, Kinney, Michael D
>>  wrote:
>>> +Bret
>>>
>>> Mike
>>>
>>
>> Why has this patch been submitted if there are unanswered questions of
>> such a fundamental nature?
>> Could someone please revert it until there is agreement about its
>> inclusion (and in which form)?
>
> I think your question is "why has this patch been *committed* / pushed
> if such questions remain"; is that correct?
>

Ah yes, apologies.

> The patch was correctly submitted to edk2-devel for review back in
> December 2017. At that time I quickly glanced at the diffstat, and I
> didn't even start reviewing the patch -- "15 files changed, 8915
> insertions(+), 9 deletions(-)". For me to review such a large amount of
> code in detail, it would have to be split into tens of patches, and I'd
> likely have to work on the review intensely for a week or more. So, I
> asumed that SafeIntLib had been carefully reviewed for strict standards
> compliance.
>
> My interest in SafeIntLib was renewed recently, upon seeing:
>
> [edk2] [Patch 05/10] OvmfPkg: Add SafeIntLib and BmpSupportLib to DSC
>  files
> [edk2] [Patch 10/10] ArmVirtPkg: Add SafeIntLib and BmpSupportLib to DSC
>  files
>
> With those patches committed (which is the current status), OvmfPkg and
> ArmVirtPkg produce and include EFI binaries that contain SafeIntLib
> code, according to the build report files -- namely
> BootGraphicsResourceTableDxe. (As Mike explained in the thread,
> BootGraphicsResourceTableDxe uses BmpSupportLib, and BaseBmpSupportLib
> uses SafeIntLib as part of the pixel format conversion, and/or as part
> of the BMP<->GOP BLT format conversion -- if I understood correctly.)
>
> Due to SafeIntLib being indirectly pulled into the OVMF and ArmVirt
> firmware binaries (and due to expecting more wide-spread use of
> SafeIntLib in the future -- which is a good thing!), I figured I'd take
> one look. Because developers frequently miss that signed integer
> overflow is actually undefined behavior, I checked SafeInt64Sub().
> Indeed it is affected.
>
> I don't necessarily think we should revert the patch, but it certainly
> needs a re-evaluation (I proposed a fix for SafeInt64Sub() up-thread).
>
> *Alternatively* -- and we should be fully aware of this as well! --, we
> *can* define C language behavior, for edk2, *on top* of ISO C. For
> example, we can say, "given the compiler options that we use with edk2,
> signed integer overflow is actually well-defined: it behaves as you
> would expect from the two's complement representation".
>
> This is a perfectly valid thing to say, and we are already saying things
> like it: for example, ISO C also leaves violations of the effective type
> / strict aliasing rules "undefined behavior", but we don't care (edk2 is
> chock-full of type punning!): we pass "-fno-strict-aliasing" to all GCC
> and CLANG toolchains that we support.
>
>
> So, my point is, we should be aware of what ISO C says about integer
> overflow, and then pick one:
>
> - we target strict ISO C compliance (wrt. integer arithmetic) with
> SafeIntLib -- in which case a re-evaluation and patches are necessary,
>
> - or else we define additional C language guarantees, and then we
> *ensure* those via compiler flags, universally.
>
> Thanks,
> Laszlo
>
>
>>
>>
 -Original Message-
 From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org]
 On Behalf Of Laszlo Ersek
 Sent: Tuesday, February 13, 2018 4:24 AM
 To: Kinney, Michael D ; Sean
 Brogan 
 Cc: edk2-devel@lists.01.org; Gao, Liming
 
 Subject: Re: [edk2] [Patch] MdePkg/BaseSafeIntLib: Add
 SafeIntLib class and instance

 Sean, Michael,

 can you please follow up on this?

 To clarify, I think this is a serious bug in SafeIntLib,
 dependent on
 what we want to use this library for. As far as I
 understand, SafeIntLib
 intends to centralize integer manipulation / arithmetic,
 so that client
 code need not concern itself with overflow checking and
 such (on the C
 expression level -- it still has to check return
 statuses, of course).
 In other words, undefined behavior related to integer
 arithmetic is
 supposed to be prevented in various modules by moving all
 such
 operations into SafeIntLib, and letting client code use
 SafeIntLib APIs.

 However, for this to work, SafeIntLib itself must be 100%
 free of
 undefined behavior. And that's not the case (unless we
 define additional
 guarantees -- on top of ISO C -- for edk2 target
 architectures). Should
 I file a TianoCore BZ? Or is someone already (re)auditing
 the library?
 Or else, is my concern unjustified? Please 

Re: [edk2] [Patch] MdePkg/BaseSafeIntLib: Add SafeIntLib class and instance

[Laszlo Ersek]

On 02/13/18 18:18, Ard Biesheuvel wrote:
> On 13 February 2018 at 16:17, Kinney, Michael D
>  wrote:
>> +Bret
>>
>> Mike
>>
> 
> Why has this patch been submitted if there are unanswered questions of
> such a fundamental nature?
> Could someone please revert it until there is agreement about its
> inclusion (and in which form)?

I think your question is "why has this patch been *committed* / pushed
if such questions remain"; is that correct?

The patch was correctly submitted to edk2-devel for review back in
December 2017. At that time I quickly glanced at the diffstat, and I
didn't even start reviewing the patch -- "15 files changed, 8915
insertions(+), 9 deletions(-)". For me to review such a large amount of
code in detail, it would have to be split into tens of patches, and I'd
likely have to work on the review intensely for a week or more. So, I
asumed that SafeIntLib had been carefully reviewed for strict standards
compliance.

My interest in SafeIntLib was renewed recently, upon seeing:

[edk2] [Patch 05/10] OvmfPkg: Add SafeIntLib and BmpSupportLib to DSC
 files
[edk2] [Patch 10/10] ArmVirtPkg: Add SafeIntLib and BmpSupportLib to DSC
 files

With those patches committed (which is the current status), OvmfPkg and
ArmVirtPkg produce and include EFI binaries that contain SafeIntLib
code, according to the build report files -- namely
BootGraphicsResourceTableDxe. (As Mike explained in the thread,
BootGraphicsResourceTableDxe uses BmpSupportLib, and BaseBmpSupportLib
uses SafeIntLib as part of the pixel format conversion, and/or as part
of the BMP<->GOP BLT format conversion -- if I understood correctly.)

Due to SafeIntLib being indirectly pulled into the OVMF and ArmVirt
firmware binaries (and due to expecting more wide-spread use of
SafeIntLib in the future -- which is a good thing!), I figured I'd take
one look. Because developers frequently miss that signed integer
overflow is actually undefined behavior, I checked SafeInt64Sub().
Indeed it is affected.

I don't necessarily think we should revert the patch, but it certainly
needs a re-evaluation (I proposed a fix for SafeInt64Sub() up-thread).

*Alternatively* -- and we should be fully aware of this as well! --, we
*can* define C language behavior, for edk2, *on top* of ISO C. For
example, we can say, "given the compiler options that we use with edk2,
signed integer overflow is actually well-defined: it behaves as you
would expect from the two's complement representation".

This is a perfectly valid thing to say, and we are already saying things
like it: for example, ISO C also leaves violations of the effective type
/ strict aliasing rules "undefined behavior", but we don't care (edk2 is
chock-full of type punning!): we pass "-fno-strict-aliasing" to all GCC
and CLANG toolchains that we support.


So, my point is, we should be aware of what ISO C says about integer
overflow, and then pick one:

- we target strict ISO C compliance (wrt. integer arithmetic) with
SafeIntLib -- in which case a re-evaluation and patches are necessary,

- or else we define additional C language guarantees, and then we
*ensure* those via compiler flags, universally.

Thanks,
Laszlo


> 
> 
>>> -Original Message-
>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org]
>>> On Behalf Of Laszlo Ersek
>>> Sent: Tuesday, February 13, 2018 4:24 AM
>>> To: Kinney, Michael D ; Sean
>>> Brogan 
>>> Cc: edk2-devel@lists.01.org; Gao, Liming
>>> 
>>> Subject: Re: [edk2] [Patch] MdePkg/BaseSafeIntLib: Add
>>> SafeIntLib class and instance
>>>
>>> Sean, Michael,
>>>
>>> can you please follow up on this?
>>>
>>> To clarify, I think this is a serious bug in SafeIntLib,
>>> dependent on
>>> what we want to use this library for. As far as I
>>> understand, SafeIntLib
>>> intends to centralize integer manipulation / arithmetic,
>>> so that client
>>> code need not concern itself with overflow checking and
>>> such (on the C
>>> expression level -- it still has to check return
>>> statuses, of course).
>>> In other words, undefined behavior related to integer
>>> arithmetic is
>>> supposed to be prevented in various modules by moving all
>>> such
>>> operations into SafeIntLib, and letting client code use
>>> SafeIntLib APIs.
>>>
>>> However, for this to work, SafeIntLib itself must be 100%
>>> free of
>>> undefined behavior. And that's not the case (unless we
>>> define additional
>>> guarantees -- on top of ISO C -- for edk2 target
>>> architectures). Should
>>> I file a TianoCore BZ? Or is someone already (re)auditing
>>> the library?
>>> Or else, is my concern unjustified? Please comment.
>>>
>>> Thanks,
>>> Laszlo
>>>
>>> On 02/08/18 01:45, Laszlo Ersek wrote:
 On 02/08/18 01:32, Laszlo Ersek wrote:
> On 12/19/17 20:36, Kinney, Michael D wrote:
>> From: Sean Brogan 
>>
>> SafeIntLib 

Re: [edk2] [Patch] MdePkg/BaseSafeIntLib: Add SafeIntLib class and instance

[Laszlo Ersek]

On 02/13/18 17:56, Bret Barkelew wrote:
> In response to the original question, I would content that our goal
> should be "a". We should be allowing universal detection of errors
> without the caller having to carry this detection code itself.

OK.

The question is how the detection is implemented internally. Is that
implementation (in edk2) allowed to rely on behavior that the ISO C
standard leaves undefined, and -- consequently -- compilers might
exploit for code generation?

> The analog would be the safe string functions: if a buffer overflow
> occurs, they don't find a way to "fix" the operation, but they
> faithfully report an error.

Precisely.

Correspondingly, translated to the safe string functions, my question
becomes:

Is the implementation of the safe string functions allowed to employ
buffer overflow internally, and detect the overflow after the fact?

> As such, I believe from my review that these functions work as
> intended.

Please let me quote the function again, from
"MdePkg/Library/BaseSafeIntLib/SafeIntLib.c":

  3831  RETURN_STATUS
  3832  EFIAPI
  3833  SafeInt64Sub (
  3834IN  INT64  Minuend,
  3835IN  INT64  Subtrahend,
  3836OUT INT64  *Result
  3837)
  3838  {
  3839RETURN_STATUS  Status;
  3840INT64  SignedResult;
  3841
  3842if (Result == NULL) {
  3843  return RETURN_INVALID_PARAMETER;
  3844}
  3845
  3846SignedResult = Minuend - Subtrahend;
  3847
  3848//
  3849// Subtracting a positive number from a positive number never 
overflows.
  3850// Subtracting a negative number from a negative number never 
overflows.
  3851// If you subtract a negative number from a positive number, you 
expect a positive result.
  3852// If you subtract a positive number from a negative number, you 
expect a negative result.
  3853// Overflow if inputs vary in sign and the output does not have the 
same sign as the first input.
  3854//
  3855if (((Minuend < 0) != (Subtrahend < 0)) &&
  3856((Minuend < 0) != (SignedResult < 0))) {
  3857  *Result = INT64_ERROR;
  3858  Status = RETURN_BUFFER_TOO_SMALL;
  3859} else {
  3860  *Result = SignedResult;
  3861  Status = RETURN_SUCCESS;
  3862}
  3863
  3864return Status;
  3865  }

On line 3846, integer overflow is possible. If that happens, not only is
the resultant value of "SignedResult" undefined, but the behavior of the
rest of the function is undefined, according to ISO C.

In other words, just because we move the checking into a library
function, we cannot check *after the fact*. The subtraction on line 3846
can invoke undefined behavior *first*, and we check only afterwards,
starting on line 3855.

Here's an analogy. Various C compilers regularly equate "undefined
behavior" with "never happens" (which is a valid interpretation of the
ISO C standard for the compiler to make). For example, given the code

  int f(int *x)
  {
*x = 3;
if (x == NULL) {
  return 1;
}
return 0;
}

the compiler may compile f() to unconditionally return 0, such as:

  int f(int *x)
  {
*x = 3;
return 0;
  }

Because, the (x == NULL) branch would depend on undefined behavior
invoked by the assignment to *x. Given that the (*x = 3) assignment is
undefined for (x==NULL), according to ISO C, the subsequent (x == NULL)
check can be taken as constant false, and eliminated.

Similarly, a sufficiently smart compiler may assume that the subtraction
on line 3846 will never overflow. (Because, such an overflow would be
undefined behavior.) Consequently, it may deduce that the overflow
checks, *after the fact*, evaluate to constant false, and can be
eliminated. It might compile the function as in:

  {
RETURN_STATUS  Status;
INT64  SignedResult;

if (Result == NULL) {
  return RETURN_INVALID_PARAMETER;
}

SignedResult = Minuend - Subtrahend;

*Result = SignedResult;
Status = RETURN_SUCCESS;

return Status;
  }

I apoligize if I'm unclear; I really don't know how to put it better.
The subtraction on line 3846 runs the risk of undefined behavior, and
the checks starting on line 3855 are too late.

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

Re: [edk2] [Patch] MdePkg/BaseSafeIntLib: Add SafeIntLib class and instance

[Ard Biesheuvel]

On 13 February 2018 at 16:17, Kinney, Michael D
 wrote:
> +Bret
>
> Mike
>

Why has this patch been submitted if there are unanswered questions of
such a fundamental nature?
Could someone please revert it until there is agreement about its
inclusion (and in which form)?


>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org]
>> On Behalf Of Laszlo Ersek
>> Sent: Tuesday, February 13, 2018 4:24 AM
>> To: Kinney, Michael D ; Sean
>> Brogan 
>> Cc: edk2-devel@lists.01.org; Gao, Liming
>> 
>> Subject: Re: [edk2] [Patch] MdePkg/BaseSafeIntLib: Add
>> SafeIntLib class and instance
>>
>> Sean, Michael,
>>
>> can you please follow up on this?
>>
>> To clarify, I think this is a serious bug in SafeIntLib,
>> dependent on
>> what we want to use this library for. As far as I
>> understand, SafeIntLib
>> intends to centralize integer manipulation / arithmetic,
>> so that client
>> code need not concern itself with overflow checking and
>> such (on the C
>> expression level -- it still has to check return
>> statuses, of course).
>> In other words, undefined behavior related to integer
>> arithmetic is
>> supposed to be prevented in various modules by moving all
>> such
>> operations into SafeIntLib, and letting client code use
>> SafeIntLib APIs.
>>
>> However, for this to work, SafeIntLib itself must be 100%
>> free of
>> undefined behavior. And that's not the case (unless we
>> define additional
>> guarantees -- on top of ISO C -- for edk2 target
>> architectures). Should
>> I file a TianoCore BZ? Or is someone already (re)auditing
>> the library?
>> Or else, is my concern unjustified? Please comment.
>>
>> Thanks,
>> Laszlo
>>
>> On 02/08/18 01:45, Laszlo Ersek wrote:
>> > On 02/08/18 01:32, Laszlo Ersek wrote:
>> >> On 12/19/17 20:36, Kinney, Michael D wrote:
>> >>> From: Sean Brogan 
>> >>>
>> >>> SafeIntLib provides helper functions to prevent
>> integer overflow
>> >>> during type conversion, addition, subtraction, and
>> multiplication.
>> >>
>> >> I clearly cannot review such a huge patch, but I've
>> noticed something
>> >> and would like to ask for clarification:
>> >>
>> >>> +/**
>> >>> +  INT64 Subtraction
>> >>> +
>> >>> +  Performs the requested operation using the input
>> parameters into a value
>> >>> +  specified by Result type and stores the converted
>> value into the caller
>> >>> +  allocated output buffer specified by Result.  The
>> caller must pass in a
>> >>> +  Result buffer that is at least as large as the
>> Result type.
>> >>> +
>> >>> +  If Result is NULL, RETURN_INVALID_PARAMETER is
>> returned.
>> >>> +
>> >>> +  If the requested operation results in an overflow
>> or an underflow condition,
>> >>> +  then Result is set to INT64_ERROR and
>> RETURN_BUFFER_TOO_SMALL is returned.
>> >>> +
>> >>> +  @param[in]   Minuend A number from which
>> another is to be subtracted.
>> >>> +  @param[in]   Subtrahend  A number to be subtracted
>> from another
>> >>> +  @param[out]  Result  Pointer to the result of
>> subtraction
>> >>> +
>> >>> +  @retval  RETURN_SUCCESSSuccessful
>> subtraction
>> >>> +  @retval  RETURN_BUFFER_TOO_SMALL   Underflow
>> >>> +  @retval  RETURN_INVALID_PARAMETER  Result is NULL
>> >>> +**/
>> >>> +RETURN_STATUS
>> >>> +EFIAPI
>> >>> +SafeInt64Sub (
>> >>> +  IN  INT64  Minuend,
>> >>> +  IN  INT64  Subtrahend,
>> >>> +  OUT INT64  *Result
>> >>> +  )
>> >>> +{
>> >>> +  RETURN_STATUS  Status;
>> >>> +  INT64  SignedResult;
>> >>> +
>> >>> +  if (Result == NULL) {
>> >>> +return RETURN_INVALID_PARAMETER;
>> >>> +  }
>> >>> +
>> >>> +  SignedResult = Minuend - Subtrahend;
>> >>> +
>> >>> +  //
>> >>> +  // Subtracting a positive number from a positive
>> number never overflows.
>> >>> +  // Subtracting a negative number from a negative
>> number never overflows.
>> >>> +  // If you subtract a negative number from a
>> positive number, you expect a positive result.
>> >>> +  // If you subtract a positive number from a
>> negative number, you expect a negative result.
>> >>> +  // Overflow if inputs vary in sign and the output
>> does not have the same sign as the first input.
>> >>> +  //
>> >>> +  if (((Minuend < 0) != (Subtrahend < 0)) &&
>> >>> +  ((Minuend < 0) != (SignedResult < 0))) {
>> >>> +*Result = INT64_ERROR;
>> >>> +Status = RETURN_BUFFER_TOO_SMALL;
>> >>> +  } else {
>> >>> +*Result = SignedResult;
>> >>> +Status = RETURN_SUCCESS;
>> >>> +  }
>> >>> +
>> >>> +  return Status;
>> >>> +}
>> >>
>> >> Is our goal to
>> >>
>> >> (a) catch overflows before the caller goes wrong due
>> to them, or
>> >> (b) completely prevent undefined behavior from
>> happening, even inside
>> >> SafeInt64Sub()?
>> >>
>> >> The above implementation may be good for (a), but it's
>> not correct for
>> >> (b). The
>> >>
>> >>   SignedResult = Minuend - Subtrahend;
>> 

Re: [edk2] [Patch] MdePkg/BaseSafeIntLib: Add SafeIntLib class and instance

[Andrew Fish]

> On Feb 13, 2018, at 8:56 AM, Bret Barkelew  
> wrote:
> 
> In response to the original question, I would content that our goal should be 
> "a". We should be allowing universal detection of errors without the caller 
> having to carry this detection code itself.
> 
> The analog would be the safe string functions: if a buffer overflow occurs, 
> they don't find a way to "fix" the operation, but they faithfully report an 
> error.
> 
> As such, I believe from my review that these functions work as intended.
> 

Bret,

I think Lazlo's point is that undefined behavior[1]  can cause the math 
function to break in the future and that we have to be very pedantic in how it 
is coded. Per the C standard it is legal for the compiler to optimized away 
undefined behavior[2], and clang is very aggressive about warning on undefined 
behavior and then updating the optimizer to remove the code in a future 
release. For example the BaseTool compression code broke with Xcode 9 recently 
due to the presence of an illegal 32-bit shift that was only hit when the 
optimizer inlined the function. While the compiler tries to emit warnings, or 
at least traps, for undefined behavior what we have seen with the Link Time 
Optimization is the code can just get removed. 

[1] - Kind of clangs point of view on undefined behavior in C: 
http://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html 

[2] - Example of undefined behavior in clang that emits a trap. Dereferencing 
NULL is undefined behavior in C so clang emits a trap, and optimizes way the 
code after the trap. 

~/work/Compiler>cat undefined.c

int
main ()
{
  int *Yikes = 0;

  *Yikes = 1;
  return 0;
}

~/work/Compiler>clang -S -Os undefined.c
~/work/Compiler>cat undefined.S
.section__TEXT,__text,regular,pure_instructions
.macosx_version_min 10, 12
.globl  _main
_main:  ## @main
.cfi_startproc
## BB#0:
pushq   %rbp
Lcfi0:
.cfi_def_cfa_offset 16
Lcfi1:
.cfi_offset %rbp, -16
movq%rsp, %rbp
Lcfi2:
.cfi_def_cfa_register %rbp
ud2
.cfi_endproc


.subsections_via_symbols

Thanks,

Andrew Fish

> - Bret
> 
> From: Kinney, Michael D  >
> Sent: Tuesday, February 13, 2018 8:17:48 AM
> To: Laszlo Ersek; Sean Brogan; Bret Barkelew
> Cc: edk2-devel@lists.01.org ; Gao, Liming
> Subject: RE: [edk2] [Patch] MdePkg/BaseSafeIntLib: Add SafeIntLib class and 
> instance
> 
> +Bret
> 
> Mike
> 
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org]
>> On Behalf Of Laszlo Ersek
>> Sent: Tuesday, February 13, 2018 4:24 AM
>> To: Kinney, Michael D ; Sean
>> Brogan 
>> Cc: edk2-devel@lists.01.org; Gao, Liming
>> 
>> Subject: Re: [edk2] [Patch] MdePkg/BaseSafeIntLib: Add
>> SafeIntLib class and instance
>> 
>> Sean, Michael,
>> 
>> can you please follow up on this?
>> 
>> To clarify, I think this is a serious bug in SafeIntLib,
>> dependent on
>> what we want to use this library for. As far as I
>> understand, SafeIntLib
>> intends to centralize integer manipulation / arithmetic,
>> so that client
>> code need not concern itself with overflow checking and
>> such (on the C
>> expression level -- it still has to check return
>> statuses, of course).
>> In other words, undefined behavior related to integer
>> arithmetic is
>> supposed to be prevented in various modules by moving all
>> such
>> operations into SafeIntLib, and letting client code use
>> SafeIntLib APIs.
>> 
>> However, for this to work, SafeIntLib itself must be 100%
>> free of
>> undefined behavior. And that's not the case (unless we
>> define additional
>> guarantees -- on top of ISO C -- for edk2 target
>> architectures). Should
>> I file a TianoCore BZ? Or is someone already (re)auditing
>> the library?
>> Or else, is my concern unjustified? Please comment.
>> 
>> Thanks,
>> Laszlo
>> 
>> On 02/08/18 01:45, Laszlo Ersek wrote:
>>> On 02/08/18 01:32, Laszlo Ersek wrote:
 On 12/19/17 20:36, Kinney, Michael D wrote:
> From: Sean Brogan 
> 
> SafeIntLib provides helper functions to prevent
>> integer overflow
> during type conversion, addition, subtraction, and
>> multiplication.
 
 I clearly cannot review such a huge patch, but I've
>> noticed something
 and would like to ask for clarification:
 
> +/**
> +  INT64 Subtraction
> +
> +  Performs the requested operation using the input
>> parameters into a value
> +  specified by Result type and stores the converted
>> value into the caller
> +  allocated output buffer specified by Result.  The
>> caller must pass in a

Re: [edk2] [Patch] MdePkg/BaseSafeIntLib: Add SafeIntLib class and instance

[Bret Barkelew]

In response to the original question, I would content that our goal should be 
"a". We should be allowing universal detection of errors without the caller 
having to carry this detection code itself.

The analog would be the safe string functions: if a buffer overflow occurs, 
they don't find a way to "fix" the operation, but they faithfully report an 
error.

As such, I believe from my review that these functions work as intended.

- Bret

From: Kinney, Michael D 
Sent: Tuesday, February 13, 2018 8:17:48 AM
To: Laszlo Ersek; Sean Brogan; Bret Barkelew
Cc: edk2-devel@lists.01.org; Gao, Liming
Subject: RE: [edk2] [Patch] MdePkg/BaseSafeIntLib: Add SafeIntLib class and 
instance

+Bret

Mike

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org]
> On Behalf Of Laszlo Ersek
> Sent: Tuesday, February 13, 2018 4:24 AM
> To: Kinney, Michael D ; Sean
> Brogan 
> Cc: edk2-devel@lists.01.org; Gao, Liming
> 
> Subject: Re: [edk2] [Patch] MdePkg/BaseSafeIntLib: Add
> SafeIntLib class and instance
>
> Sean, Michael,
>
> can you please follow up on this?
>
> To clarify, I think this is a serious bug in SafeIntLib,
> dependent on
> what we want to use this library for. As far as I
> understand, SafeIntLib
> intends to centralize integer manipulation / arithmetic,
> so that client
> code need not concern itself with overflow checking and
> such (on the C
> expression level -- it still has to check return
> statuses, of course).
> In other words, undefined behavior related to integer
> arithmetic is
> supposed to be prevented in various modules by moving all
> such
> operations into SafeIntLib, and letting client code use
> SafeIntLib APIs.
>
> However, for this to work, SafeIntLib itself must be 100%
> free of
> undefined behavior. And that's not the case (unless we
> define additional
> guarantees -- on top of ISO C -- for edk2 target
> architectures). Should
> I file a TianoCore BZ? Or is someone already (re)auditing
> the library?
> Or else, is my concern unjustified? Please comment.
>
> Thanks,
> Laszlo
>
> On 02/08/18 01:45, Laszlo Ersek wrote:
> > On 02/08/18 01:32, Laszlo Ersek wrote:
> >> On 12/19/17 20:36, Kinney, Michael D wrote:
> >>> From: Sean Brogan 
> >>>
> >>> SafeIntLib provides helper functions to prevent
> integer overflow
> >>> during type conversion, addition, subtraction, and
> multiplication.
> >>
> >> I clearly cannot review such a huge patch, but I've
> noticed something
> >> and would like to ask for clarification:
> >>
> >>> +/**
> >>> +  INT64 Subtraction
> >>> +
> >>> +  Performs the requested operation using the input
> parameters into a value
> >>> +  specified by Result type and stores the converted
> value into the caller
> >>> +  allocated output buffer specified by Result.  The
> caller must pass in a
> >>> +  Result buffer that is at least as large as the
> Result type.
> >>> +
> >>> +  If Result is NULL, RETURN_INVALID_PARAMETER is
> returned.
> >>> +
> >>> +  If the requested operation results in an overflow
> or an underflow condition,
> >>> +  then Result is set to INT64_ERROR and
> RETURN_BUFFER_TOO_SMALL is returned.
> >>> +
> >>> +  @param[in]   Minuend A number from which
> another is to be subtracted.
> >>> +  @param[in]   Subtrahend  A number to be subtracted
> from another
> >>> +  @param[out]  Result  Pointer to the result of
> subtraction
> >>> +
> >>> +  @retval  RETURN_SUCCESSSuccessful
> subtraction
> >>> +  @retval  RETURN_BUFFER_TOO_SMALL   Underflow
> >>> +  @retval  RETURN_INVALID_PARAMETER  Result is NULL
> >>> +**/
> >>> +RETURN_STATUS
> >>> +EFIAPI
> >>> +SafeInt64Sub (
> >>> +  IN  INT64  Minuend,
> >>> +  IN  INT64  Subtrahend,
> >>> +  OUT INT64  *Result
> >>> +  )
> >>> +{
> >>> +  RETURN_STATUS  Status;
> >>> +  INT64  SignedResult;
> >>> +
> >>> +  if (Result == NULL) {
> >>> +return RETURN_INVALID_PARAMETER;
> >>> +  }
> >>> +
> >>> +  SignedResult = Minuend - Subtrahend;
> >>> +
> >>> +  //
> >>> +  // Subtracting a positive number from a positive
> number never overflows.
> >>> +  // Subtracting a negative number from a negative
> number never overflows.
> >>> +  // If you subtract a negative number from a
> positive number, you expect a positive result.
> >>> +  // If you subtract a positive number from a
> negative number, you expect a negative result.
> >>> +  // Overflow if inputs vary in sign and the output
> does not have the same sign as the first input.
> >>> +  //
> >>> +  if (((Minuend < 0) != (Subtrahend < 0)) &&
> >>> +  ((Minuend < 0) != (SignedResult < 0))) {
> >>> +*Result = INT64_ERROR;
> >>> +Status = RETURN_BUFFER_TOO_SMALL;
> >>> +  } else {
> >>> +*Result = SignedResult;
> >>> +Status = RETURN_SUCCESS;
> >>> +  }
> >>> +
> >>> +  return Status;
> >>> +}
> >>
> >> Is our goal to
> >>
> >> 

[edk2] [PATCH edk2-platforms] AMD, Hisilicon, Socionext: fix build after DxeCapsuleLibFmp changes

[Leif Lindholm]

edk2 commit 1ec2e7d0e8db
("MdeModulePkg/DxeCapsuleLibFmp: Use BmpSupportLib") broke the build of
all platforms that include DxeCapsuleLibFmp, since none of them included
a BmpSupportLib (added as part of the same series).

BmpSupportLib itself depends on SafeIntLib, so add the two libraries to
all affected platforms.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Leif Lindholm 
---
 Platform/AMD/OverdriveBoard/OverdriveBoard.dsc   | 2 ++
 Platform/Socionext/DeveloperBox/DeveloperBox.dsc | 2 ++
 Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc | 2 ++
 Silicon/Hisilicon/Hisilicon.dsc.inc  | 2 ++
 4 files changed, 8 insertions(+)

diff --git a/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc 
b/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
index 48018abc69..7d85b78642 100644
--- a/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
+++ b/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
@@ -49,6 +49,8 @@ [LibraryClasses.common]
   
DebugPrintErrorLevelLib|MdePkg/Library/BaseDebugPrintErrorLevelLib/BaseDebugPrintErrorLevelLib.inf
 
   BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
+  BmpSupportLib|MdeModulePkg/Library/BaseBmpSupportLib/BaseBmpSupportLib.inf
+  SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
   
SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
   
PerformanceLib|MdePkg/Library/BasePerformanceLibNull/BasePerformanceLibNull.inf
   PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc 
b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
index 7e69eaba9b..3c109b495f 100644
--- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
+++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
@@ -75,6 +75,8 @@ [LibraryClasses.common]
   
ArmGenericTimerCounterLib|ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.inf
 
   BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
+  BmpSupportLib|MdeModulePkg/Library/BaseBmpSupportLib/BaseBmpSupportLib.inf
+  SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
   
SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
   
PerformanceLib|MdePkg/Library/BasePerformanceLibNull/BasePerformanceLibNull.inf
   PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
diff --git a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc 
b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
index e35c17f0bc..45ab2afc40 100644
--- a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
+++ b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
@@ -75,6 +75,8 @@ [LibraryClasses.common]
   
ArmGenericTimerCounterLib|ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.inf
 
   BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
+  BmpSupportLib|MdeModulePkg/Library/BaseBmpSupportLib/BaseBmpSupportLib.inf
+  SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
   
SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
   
PerformanceLib|MdePkg/Library/BasePerformanceLibNull/BasePerformanceLibNull.inf
   PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
diff --git a/Silicon/Hisilicon/Hisilicon.dsc.inc 
b/Silicon/Hisilicon/Hisilicon.dsc.inc
index 57668294cb..1d6d20ca1d 100644
--- a/Silicon/Hisilicon/Hisilicon.dsc.inc
+++ b/Silicon/Hisilicon/Hisilicon.dsc.inc
@@ -22,6 +22,8 @@ [LibraryClasses.common]
   
DebugPrintErrorLevelLib|MdePkg/Library/BaseDebugPrintErrorLevelLib/BaseDebugPrintErrorLevelLib.inf
 
   BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
+  BmpSupportLib|MdeModulePkg/Library/BaseBmpSupportLib/BaseBmpSupportLib.inf
+  SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
   
SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
   
PerformanceLib|MdePkg/Library/BasePerformanceLibNull/BasePerformanceLibNull.inf
   PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
-- 
2.11.0

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

Re: [edk2] how do I use RamDiskDxe?

[Rick Warner]

On 02/12/2018 10:05 PM, Wu, Hao A wrote:

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
Andrew Fish
Sent: Tuesday, February 13, 2018 8:40 AM
To: Rick Warner
Cc: edk2-devel@lists.01.org
Subject: Re: [edk2] how do I use RamDiskDxe?




On Feb 12, 2018, at 1:07 PM, Rick Warner  wrote:

Hi All,

I'm trying to develop a tool for automated BIOS flashing from the EFI shell

using IPMI serial over lan to drive it.  To accomplish this, my plan is to use a
network booted EFI shell with a tftp client and a ramdisk to download the flash
tools and ROM into as a workspace and then run the flash commands from that
ram disk.

I've been able to successfully build and boot an efi shell from the edk 2

sources including the tftp client.  I've also been able to separately build the
RamDiskDxe driver from the MdeModulePkg. Loading the RamDiskDxe.efi
driver file results in a successful message, but I do not get any usable ram 
disk
made available to me?

How do I get a usable drive letter/name (ie blk0: or any other name) from the

RamDiskDxe driver?
Looks like when you load the RamDiskDxe it registers gEfiRamDiskProtocolGuid
https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Protocol/Ra
mDisk.h
 which notes this API was added in UEFI 2.6 so you can read up on it in
that version of the EFI Spec.

 From a quick site read of the code it looks like the driver just produces a
Protocol that lets you create a RAM disk and VRF (Setup Pages) that let you
configure one. Seems like you still need to call it and give the raw disk image.





Once I've done that, will it be possible to include the RamDiskDxe driver as

part of the network bootable efi shell image?  I assume I'll need to modify
the .dsc file for the ShellPkg to include the MdeModulePkg pieces.

If anyone has any other suggestions for this, I'm open to ideas. The only way

I've been successful in network booting EFI shell is using just that file 
directly.  I
tried creating a dos filesystem image with the efi shell file in it just like 
would
work with a USB key, but that would not boot over the network stack. If there is
some way of creating a network bootable EFI shell filesystem image (similar to
how Linux can network boot a separate vmlinuz kernel and initrd image)?

Are there any docs that I've missed (I've tried looking in the user docs and on

the wiki) regarding setting up RamDiskDxe?
Traditionally network booting has involved loading a single file over the
network (like an OS Loader) and that file uses the UEFI networking stack to
download (TFTP read) more images (like the Kernel) from a location implied by
the location used to download the single file (OS Loader).


Hi,

For the RamDiskDxe part:

Just as the previous reply, the RamDiskDxe driver provides:
a). A protocol to (un)register a range of memory to a Ram disk
b). A setup page for user to create:
 a raw Ram disk (no data on the created one) or,
 from a selected file (the file will be the content in the Ram disk,
 no file system though)

If you want to place the flash tools in the Ram disk and use it, I think
you also need to format the Ram disk and create file system on it first
(e.g. FAT).

Best Regards,
Hao Wu


Thanks,

Andrew Fish


Thank you both very much!

How do I use the setup page to create either a raw (empty) or filled w/ 
a file ramdisk?  How do I call that?  I assume the file to create it 
from would need to already be local to the system.  Is that correct? Or 
would I be able to directly pull a filesystem image from tftp into the 
ramdisk upon creation?


Thanks,
Rick Warner


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

Re: [edk2] [Patch] MdePkg/BaseSafeIntLib: Add SafeIntLib class and instance

[Kinney, Michael D]

+Bret

Mike

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org]
> On Behalf Of Laszlo Ersek
> Sent: Tuesday, February 13, 2018 4:24 AM
> To: Kinney, Michael D ; Sean
> Brogan 
> Cc: edk2-devel@lists.01.org; Gao, Liming
> 
> Subject: Re: [edk2] [Patch] MdePkg/BaseSafeIntLib: Add
> SafeIntLib class and instance
> 
> Sean, Michael,
> 
> can you please follow up on this?
> 
> To clarify, I think this is a serious bug in SafeIntLib,
> dependent on
> what we want to use this library for. As far as I
> understand, SafeIntLib
> intends to centralize integer manipulation / arithmetic,
> so that client
> code need not concern itself with overflow checking and
> such (on the C
> expression level -- it still has to check return
> statuses, of course).
> In other words, undefined behavior related to integer
> arithmetic is
> supposed to be prevented in various modules by moving all
> such
> operations into SafeIntLib, and letting client code use
> SafeIntLib APIs.
> 
> However, for this to work, SafeIntLib itself must be 100%
> free of
> undefined behavior. And that's not the case (unless we
> define additional
> guarantees -- on top of ISO C -- for edk2 target
> architectures). Should
> I file a TianoCore BZ? Or is someone already (re)auditing
> the library?
> Or else, is my concern unjustified? Please comment.
> 
> Thanks,
> Laszlo
> 
> On 02/08/18 01:45, Laszlo Ersek wrote:
> > On 02/08/18 01:32, Laszlo Ersek wrote:
> >> On 12/19/17 20:36, Kinney, Michael D wrote:
> >>> From: Sean Brogan 
> >>>
> >>> SafeIntLib provides helper functions to prevent
> integer overflow
> >>> during type conversion, addition, subtraction, and
> multiplication.
> >>
> >> I clearly cannot review such a huge patch, but I've
> noticed something
> >> and would like to ask for clarification:
> >>
> >>> +/**
> >>> +  INT64 Subtraction
> >>> +
> >>> +  Performs the requested operation using the input
> parameters into a value
> >>> +  specified by Result type and stores the converted
> value into the caller
> >>> +  allocated output buffer specified by Result.  The
> caller must pass in a
> >>> +  Result buffer that is at least as large as the
> Result type.
> >>> +
> >>> +  If Result is NULL, RETURN_INVALID_PARAMETER is
> returned.
> >>> +
> >>> +  If the requested operation results in an overflow
> or an underflow condition,
> >>> +  then Result is set to INT64_ERROR and
> RETURN_BUFFER_TOO_SMALL is returned.
> >>> +
> >>> +  @param[in]   Minuend A number from which
> another is to be subtracted.
> >>> +  @param[in]   Subtrahend  A number to be subtracted
> from another
> >>> +  @param[out]  Result  Pointer to the result of
> subtraction
> >>> +
> >>> +  @retval  RETURN_SUCCESSSuccessful
> subtraction
> >>> +  @retval  RETURN_BUFFER_TOO_SMALL   Underflow
> >>> +  @retval  RETURN_INVALID_PARAMETER  Result is NULL
> >>> +**/
> >>> +RETURN_STATUS
> >>> +EFIAPI
> >>> +SafeInt64Sub (
> >>> +  IN  INT64  Minuend,
> >>> +  IN  INT64  Subtrahend,
> >>> +  OUT INT64  *Result
> >>> +  )
> >>> +{
> >>> +  RETURN_STATUS  Status;
> >>> +  INT64  SignedResult;
> >>> +
> >>> +  if (Result == NULL) {
> >>> +return RETURN_INVALID_PARAMETER;
> >>> +  }
> >>> +
> >>> +  SignedResult = Minuend - Subtrahend;
> >>> +
> >>> +  //
> >>> +  // Subtracting a positive number from a positive
> number never overflows.
> >>> +  // Subtracting a negative number from a negative
> number never overflows.
> >>> +  // If you subtract a negative number from a
> positive number, you expect a positive result.
> >>> +  // If you subtract a positive number from a
> negative number, you expect a negative result.
> >>> +  // Overflow if inputs vary in sign and the output
> does not have the same sign as the first input.
> >>> +  //
> >>> +  if (((Minuend < 0) != (Subtrahend < 0)) &&
> >>> +  ((Minuend < 0) != (SignedResult < 0))) {
> >>> +*Result = INT64_ERROR;
> >>> +Status = RETURN_BUFFER_TOO_SMALL;
> >>> +  } else {
> >>> +*Result = SignedResult;
> >>> +Status = RETURN_SUCCESS;
> >>> +  }
> >>> +
> >>> +  return Status;
> >>> +}
> >>
> >> Is our goal to
> >>
> >> (a) catch overflows before the caller goes wrong due
> to them, or
> >> (b) completely prevent undefined behavior from
> happening, even inside
> >> SafeInt64Sub()?
> >>
> >> The above implementation may be good for (a), but it's
> not correct for
> >> (b). The
> >>
> >>   SignedResult = Minuend - Subtrahend;
> >>
> >> subtraction invokes undefined behavior if the result
> cannot be
> >> represented [*]; the rest of the code cannot help.
> >>
> >> Now if we say that such subtractions always occur
> according to the
> >> "usual two's complement definition", on all
> architectures that edk2
> >> targets, and we're sure that no compiler or analysis
> tool will flag --
> >> or exploit -- the UB either, then the code is fine;
> meaning 

[edk2] error C0DE: Unknown fatal error when processing

[Hristo Mihaylov]

Hello,

I just hit this and it told me to send you the stack trace. I think it's due to 
the fact that I created an empty FDF file.

```
hrimih@lnxclnt2002:~/work/edk2.git$ build clean && build && cp 
Build/Hvmf/DEBUG_GCC5/X64/HelloWorld.efi ~/work/ovmf/hda-contents
Build environment: Linux-4.9.0-5-amd64-x86_64-with-debian-9.3
Build start time: 16:36:02, Feb.13 2018

WORKSPACE= /home/hrimih/work/edk2.git
ECP_SOURCE   = /home/hrimih/work/edk2.git/EdkCompatibilityPkg
EDK_SOURCE   = /home/hrimih/work/edk2.git/EdkCompatibilityPkg
EFI_SOURCE   = /home/hrimih/work/edk2.git/EdkCompatibilityPkg
EDK_TOOLS_PATH   = /home/hrimih/work/edk2.git/BaseTools
CONF_PATH= /home/hrimih/work/edk2.git/Conf


Architecture(s)  = X64
Build target = DEBUG
Toolchain= GCC5

Active Platform  = /home/hrimih/work/edk2.git/HvmfPkg/HvmfPkg.dsc
Flash Image Definition   = /home/hrimih/work/edk2.git/HvmfPkg/HvmfPkg.fdf

Processing meta-data


build.py...
 : error C0DE: Unknown fatal error when processing 
[/home/hrimih/work/edk2.git/HvmfPkg/HvmfPkg.dsc]

(Please send email to edk2-devel@lists.01.org for help, attaching following 
call stack trace!)

(Python 2.7.13 on linux2) Traceback (most recent call last):
  File 
"/home/hrimih/work/edk2.git/BaseTools/BinWrappers/PosixLike/../../Source/Python/build/build.py",
 line 2488, in Main
MyBuild.Launch()
  File 
"/home/hrimih/work/edk2.git/BaseTools/BinWrappers/PosixLike/../../Source/Python/build/build.py",
 line 2219, in Launch
self._BuildPlatform()
  File 
"/home/hrimih/work/edk2.git/BaseTools/BinWrappers/PosixLike/../../Source/Python/build/build.py",
 line 1743, in _BuildPlatform
self.Progress
  File "/home/hrimih/work/edk2.git/BaseTools/Source/Python/AutoGen/AutoGen.py", 
line 184, in __new__
if not AutoGenObject._Init(Workspace, MetaFile, Target, Toolchain, Arch, 
*args, **kwargs):
  File "/home/hrimih/work/edk2.git/BaseTools/Source/Python/AutoGen/AutoGen.py", 
line 330, in _Init
Fdf.ParseFile()
  File 
"/home/hrimih/work/edk2.git/BaseTools/Source/Python/GenFds/FdfParser.py", line 
1410, in ParseFile
self.Preprocess()
  File 
"/home/hrimih/work/edk2.git/BaseTools/Source/Python/GenFds/FdfParser.py", line 
1386, in Preprocess
self.__StringToList()
  File 
"/home/hrimih/work/edk2.git/BaseTools/Source/Python/GenFds/FdfParser.py", line 
448, in __StringToList
self.Profile.FileLinesList[-1].append(' ')
IndexError: list index out of range


- Failed -
Build end time: 16:36:03, Feb.13 2018
Build total time: 00:00:01
```

--
Regards,
Hristo Mihaylov

Designer
Prodrive Technologies B.V.
Mobile: +31 64 68 58 953
Phone:  +31 64 68 58 953
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] ShellPkg/ShellLib: Fix a bug in InternalShellIsHexOrDecimalNumber

[Carsey, Jaben]

Reviewed-by: Jaben Carsey 

> -Original Message-
> From: Ni, Ruiyu
> Sent: Tuesday, February 13, 2018 12:54 AM
> To: edk2-devel@lists.01.org
> Cc: Vladimir Olovyannikov ; Carsey,
> Jaben 
> Subject: [PATCH] ShellPkg/ShellLib: Fix a bug in
> InternalShellIsHexOrDecimalNumber
> Importance: High
> 
> InternalShellIsHexOrDecimalNumber() wrongly treats "-" as a number.
> The patch fixes this issue.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=730
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni 
> Cc: Vladimir Olovyannikov 
> Cc: Jaben Carsey 
> ---
>  ShellPkg/Library/UefiShellLib/UefiShellLib.c | 23 +++
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> index 7f6389f655..e53985e2d7 100644
> --- a/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> +++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> @@ -3,7 +3,7 @@
> 
>(C) Copyright 2016 Hewlett Packard Enterprise Development LP
>Copyright 2016 Dell Inc.
> -  Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
> +  Copyright (c) 2006 - 2018, 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
> @@ -3636,29 +3636,36 @@ InternalShellIsHexOrDecimalNumber (
>)
>  {
>BOOLEAN Hex;
> +  BOOLEAN LeadingZero;
> +
> +  if (String == NULL) {
> +return FALSE;
> +  }
> 
>//
>// chop off a single negative sign
>//
> -  if (String != NULL && *String == L'-') {
> +  if (*String == L'-') {
>  String++;
>}
> 
> -  if (String == NULL) {
> -return (FALSE);
> +  if (*String == CHAR_NULL) {
> +return FALSE;
>}
> 
>//
>// chop leading zeroes
>//
> -  while(String != NULL && *String == L'0'){
> +  LeadingZero = FALSE;
> +  while(*String == L'0'){
>  String++;
> +LeadingZero = TRUE;
>}
>//
>// allow '0x' or '0X', but not 'x' or 'X'
>//
> -  if (String != NULL && (*String == L'x' || *String == L'X')) {
> -if (*(String-1) != L'0') {
> +  if (*String == L'x' || *String == L'X') {
> +if (!LeadingZero) {
>//
>// we got an x without a preceeding 0
>//
> @@ -3675,7 +3682,7 @@ InternalShellIsHexOrDecimalNumber (
>//
>// loop through the remaining characters and use the lib function
>//
> -  for ( ; String != NULL && *String != CHAR_NULL && !(StopAtSpace &&
> *String == L' ') ; String++){
> +  for ( ; *String != CHAR_NULL && !(StopAtSpace && *String == L' ') ;
> String++){
>  if (TimeNumbers && (String[0] == L':')) {
>continue;
>  }
> --
> 2.16.1.windows.1

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

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

[Carsey, Jaben]

So I thought we are keeping the command, but I do agree with Leif that better 
error path logic would be good.  We can wait for Ray to confirm if he has 
different plans.

-Jaben

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Meenakshi Aggarwal
> Sent: Tuesday, February 13, 2018 7:19 AM
> To: Carsey, Jaben ; Leif Lindholm
> ; Ni, Ruiyu 
> Cc: Ye, Ting ; edk2-devel@lists.01.org; Fu, Siyuan
> ; ard.biesheu...@linaro.org
> Subject: Re: [edk2] [PATCH] Tftp assert fix for openfile failure case
> Importance: High
> 
> Jaben,
> 
> The patch was not accepted last time because as per Leif
> "
> > > > > > > This shell command was introduced in the heyday of "let's
> > > > > > > reimplement U-Boot in the EDK2 tree". Mainly, from my
> impression,
> > it
> > > > > > > seems to be used in order that people don't need to learn how
> boot
> > > > > > > managers and device paths work.
> > > > > >
> > > > > > When you say about complete boot, then this may not be useful.
> > > > > >"
> 
> 
> So, if we are maintaining tftp command, then i will resend the patch with
> inclusion of one comment of Leif
> "
> > > > > > > If the code is to be kept, I think (from a quick glance) that I
> > > > > > > would also like to see
> > > > > > >   *Data = NULL
> > > > > > > in the error path of DownloadFile().
> "
> 
> Thanks,
> Meenakshi
> 
> > -Original Message-
> > From: Carsey, Jaben [mailto:jaben.car...@intel.com]
> > Sent: Tuesday, February 13, 2018 8:44 PM
> > To: Meenakshi Aggarwal ; Leif Lindholm
> > ; Ni, Ruiyu 
> > Cc: ard.biesheu...@linaro.org; Ye, Ting ; edk2-
> > de...@lists.01.org; Fu, Siyuan ; Udit Kumar
> > 
> > Subject: RE: [PATCH] Tftp assert fix for openfile failure case
> >
> > Meenakshi,
> >
> > The TFTP command is outside the UEFI Shell specification, therefore it is
> > included as a DynamicCommand, not a command built into the shell itself.
> >
> > I a little confused by your last sentence.  Do you want to send a new patch?
> > or do you have a branch to pick changes from ?
> >
> > -Jaben
> >
> >
> > > -Original Message-
> > > From: Meenakshi Aggarwal [mailto:meenakshi.aggar...@nxp.com]
> > > Sent: Tuesday, February 13, 2018 1:43 AM
> > > To: Leif Lindholm ; Ni, Ruiyu
> > 
> > > Cc: ard.biesheu...@linaro.org; Ye, Ting ; edk2-
> > > de...@lists.01.org; Carsey, Jaben ; Fu, Siyuan
> > > ; Udit Kumar 
> > > Subject: RE: [PATCH] Tftp assert fix for openfile failure case
> > > Importance: High
> > >
> > > Hi,
> > >
> > > As per commit 0961002352e9115b72f544dded239ad226efe87b
> > >
> > > Tftp command will be maintained to extend internal commands and
> > >
> > > ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c
> > >
> > > Looks like a copy of ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > >
> > > So, below fix is needed in this case as well.
> > >
> > > Please suggest, so we can send the updated patch [incorporating Leif's
> > > comments]
> > >
> > >
> > > Thanks,
> > > Meenakshi
> > >
> > > > -Original Message-
> > > > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
> Of
> > > > Udit Kumar
> > > > Sent: Thursday, November 09, 2017 10:13 AM
> > > > To: Leif Lindholm 
> > > > Cc: ruiyu...@intel.com; ard.biesheu...@linaro.org; ting...@intel.com;
> > > edk2-
> > > > de...@lists.01.org; jaben.car...@intel.com; siyuan...@intel.com
> > > > Subject: Re: [edk2] [PATCH] Tftp assert fix for openfile failure case
> > > >
> > > >
> > > >
> > > > > -Original Message-
> > > > > From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> > > > > Sent: Wednesday, November 08, 2017 8:52 PM
> > > > > To: Udit Kumar 
> > > > > Cc: Vabhav Sharma ; edk2-
> > > de...@lists.01.org;
> > > > > ruiyu...@intel.com; jaben.car...@intel.com;
> > ard.biesheu...@linaro.org;
> > > > > siyuan...@intel.com; ting...@intel.com
> > > > > Subject: Re: [PATCH] Tftp assert fix for openfile failure case
> > > > >
> > > > > On Wed, Nov 08, 2017 at 05:15:49AM +, Udit Kumar wrote:
> > > > > > > > diff --git a/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > > > > > > b/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > > > > > > index fbde3bf..6425fc5 100755
> > > > > > > > --- a/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > > > > > > +++ b/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > > > > > > @@ -509,6 +509,7 @@ ShellCommandRunTftp (
> > > > > > > >);
> > > > > > > >goto NextHandle;
> > > > > > >
> > > > > > > Wow, a goto in a foor loop in a 320-line function.
> > > > > > > 

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

[Meenakshi Aggarwal]

Jaben,

The patch was not accepted last time because as per Leif
"
> > > > > > This shell command was introduced in the heyday of "let's
> > > > > > reimplement U-Boot in the EDK2 tree". Mainly, from my impression,
> it
> > > > > > seems to be used in order that people don't need to learn how boot
> > > > > > managers and device paths work.
> > > > >
> > > > > When you say about complete boot, then this may not be useful.
> > > > >"


So, if we are maintaining tftp command, then i will resend the patch with 
inclusion of one comment of Leif
"
> > > > > > If the code is to be kept, I think (from a quick glance) that I
> > > > > > would also like to see
> > > > > >   *Data = NULL
> > > > > > in the error path of DownloadFile().
"

Thanks,
Meenakshi 

> -Original Message-
> From: Carsey, Jaben [mailto:jaben.car...@intel.com]
> Sent: Tuesday, February 13, 2018 8:44 PM
> To: Meenakshi Aggarwal ; Leif Lindholm
> ; Ni, Ruiyu 
> Cc: ard.biesheu...@linaro.org; Ye, Ting ; edk2-
> de...@lists.01.org; Fu, Siyuan ; Udit Kumar
> 
> Subject: RE: [PATCH] Tftp assert fix for openfile failure case
> 
> Meenakshi,
> 
> The TFTP command is outside the UEFI Shell specification, therefore it is
> included as a DynamicCommand, not a command built into the shell itself.
> 
> I a little confused by your last sentence.  Do you want to send a new patch?
> or do you have a branch to pick changes from ?
> 
> -Jaben
> 
> 
> > -Original Message-
> > From: Meenakshi Aggarwal [mailto:meenakshi.aggar...@nxp.com]
> > Sent: Tuesday, February 13, 2018 1:43 AM
> > To: Leif Lindholm ; Ni, Ruiyu
> 
> > Cc: ard.biesheu...@linaro.org; Ye, Ting ; edk2-
> > de...@lists.01.org; Carsey, Jaben ; Fu, Siyuan
> > ; Udit Kumar 
> > Subject: RE: [PATCH] Tftp assert fix for openfile failure case
> > Importance: High
> >
> > Hi,
> >
> > As per commit 0961002352e9115b72f544dded239ad226efe87b
> >
> > Tftp command will be maintained to extend internal commands and
> >
> > ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c
> >
> > Looks like a copy of ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> >
> > So, below fix is needed in this case as well.
> >
> > Please suggest, so we can send the updated patch [incorporating Leif's
> > comments]
> >
> >
> > Thanks,
> > Meenakshi
> >
> > > -Original Message-
> > > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > > Udit Kumar
> > > Sent: Thursday, November 09, 2017 10:13 AM
> > > To: Leif Lindholm 
> > > Cc: ruiyu...@intel.com; ard.biesheu...@linaro.org; ting...@intel.com;
> > edk2-
> > > de...@lists.01.org; jaben.car...@intel.com; siyuan...@intel.com
> > > Subject: Re: [edk2] [PATCH] Tftp assert fix for openfile failure case
> > >
> > >
> > >
> > > > -Original Message-
> > > > From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> > > > Sent: Wednesday, November 08, 2017 8:52 PM
> > > > To: Udit Kumar 
> > > > Cc: Vabhav Sharma ; edk2-
> > de...@lists.01.org;
> > > > ruiyu...@intel.com; jaben.car...@intel.com;
> ard.biesheu...@linaro.org;
> > > > siyuan...@intel.com; ting...@intel.com
> > > > Subject: Re: [PATCH] Tftp assert fix for openfile failure case
> > > >
> > > > On Wed, Nov 08, 2017 at 05:15:49AM +, Udit Kumar wrote:
> > > > > > > diff --git a/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > > > > > b/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > > > > > index fbde3bf..6425fc5 100755
> > > > > > > --- a/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > > > > > +++ b/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > > > > > @@ -509,6 +509,7 @@ ShellCommandRunTftp (
> > > > > > >);
> > > > > > >goto NextHandle;
> > > > > >
> > > > > > Wow, a goto in a foor loop in a 320-line function.
> > > > > > What could possibly go wrong?
> > > > >
> > > > > Instead of being on some volume, if you are on Shell.
> > > > > Then file open will fail.
> > > >
> > > > Sure. The above was a snarky comment on the state of the existing
> code.
> > > >
> > > > > > >  }
> > > > > > > +DataSize = FileSize;
> > > > > > >
> > > > > > >  Status = DownloadFile (Mtftp4, RemoteFilePath,
> > > > > > > AsciiRemoteFilePath,
> > > > > > FileSize, BlockSize, );
> > > > > > >  if (EFI_ERROR (Status)) {
> > > > > > > @@ -539,7 +540,6 @@ ShellCommandRunTftp (
> > > > > > >goto NextHandle;
> > > > > > >  }
> > > > > > >
> > > > > > > -DataSize = FileSize;
> > > > > > >  Status = ShellWriteFile (FileHandle, , Data);
> > > > > > >  if (!EFI_ERROR (Status)) {
> > > > > > >ShellStatus = SHELL_SUCCESS;
> > > > > > > --
> > > > > > > 1.9.1
> > > > > >
> > > > > > So, a wider question:

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

[Carsey, Jaben]

Meenakshi,

The TFTP command is outside the UEFI Shell specification, therefore it is 
included as a DynamicCommand, not a command built into the shell itself.

I a little confused by your last sentence.  Do you want to send a new patch? or 
do you have a branch to pick changes from ?

-Jaben


> -Original Message-
> From: Meenakshi Aggarwal [mailto:meenakshi.aggar...@nxp.com]
> Sent: Tuesday, February 13, 2018 1:43 AM
> To: Leif Lindholm ; Ni, Ruiyu 
> Cc: ard.biesheu...@linaro.org; Ye, Ting ; edk2-
> de...@lists.01.org; Carsey, Jaben ; Fu, Siyuan
> ; Udit Kumar 
> Subject: RE: [PATCH] Tftp assert fix for openfile failure case
> Importance: High
> 
> Hi,
> 
> As per commit 0961002352e9115b72f544dded239ad226efe87b
> 
> Tftp command will be maintained to extend internal commands and
> 
> ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c
> 
> Looks like a copy of ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> 
> So, below fix is needed in this case as well.
> 
> Please suggest, so we can send the updated patch [incorporating Leif's
> comments]
> 
> 
> Thanks,
> Meenakshi
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > Udit Kumar
> > Sent: Thursday, November 09, 2017 10:13 AM
> > To: Leif Lindholm 
> > Cc: ruiyu...@intel.com; ard.biesheu...@linaro.org; ting...@intel.com;
> edk2-
> > de...@lists.01.org; jaben.car...@intel.com; siyuan...@intel.com
> > Subject: Re: [edk2] [PATCH] Tftp assert fix for openfile failure case
> >
> >
> >
> > > -Original Message-
> > > From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> > > Sent: Wednesday, November 08, 2017 8:52 PM
> > > To: Udit Kumar 
> > > Cc: Vabhav Sharma ; edk2-
> de...@lists.01.org;
> > > ruiyu...@intel.com; jaben.car...@intel.com; ard.biesheu...@linaro.org;
> > > siyuan...@intel.com; ting...@intel.com
> > > Subject: Re: [PATCH] Tftp assert fix for openfile failure case
> > >
> > > On Wed, Nov 08, 2017 at 05:15:49AM +, Udit Kumar wrote:
> > > > > > diff --git a/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > > > > b/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > > > > index fbde3bf..6425fc5 100755
> > > > > > --- a/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > > > > +++ b/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > > > > @@ -509,6 +509,7 @@ ShellCommandRunTftp (
> > > > > >);
> > > > > >goto NextHandle;
> > > > >
> > > > > Wow, a goto in a foor loop in a 320-line function.
> > > > > What could possibly go wrong?
> > > >
> > > > Instead of being on some volume, if you are on Shell.
> > > > Then file open will fail.
> > >
> > > Sure. The above was a snarky comment on the state of the existing code.
> > >
> > > > > >  }
> > > > > > +DataSize = FileSize;
> > > > > >
> > > > > >  Status = DownloadFile (Mtftp4, RemoteFilePath,
> > > > > > AsciiRemoteFilePath,
> > > > > FileSize, BlockSize, );
> > > > > >  if (EFI_ERROR (Status)) {
> > > > > > @@ -539,7 +540,6 @@ ShellCommandRunTftp (
> > > > > >goto NextHandle;
> > > > > >  }
> > > > > >
> > > > > > -DataSize = FileSize;
> > > > > >  Status = ShellWriteFile (FileHandle, , Data);
> > > > > >  if (!EFI_ERROR (Status)) {
> > > > > >ShellStatus = SHELL_SUCCESS;
> > > > > > --
> > > > > > 1.9.1
> > > > >
> > > > > So, a wider question:
> > > > > This shell command was introduced in the heyday of "let's
> > > > > reimplement U-Boot in the EDK2 tree". Mainly, from my impression, it
> > > > > seems to be used in order that people don't need to learn how boot
> > > > > managers and device paths work.
> > > >
> > > > When you say about complete boot, then this may not be useful.
> > > >
> > > > > Am I being too harsh?
> > > > > Are there practical uses for this?
> > > >
> > > > For doing some sort of unit testing of given interface. I found this
> > > > useful. During development, this is useful to transfer generic file to
> > > > development board.
> > >
> > > OK, I can see how it could be useful.
> > > My opposition is based on three things:
> > > 1) people _are_ trying to use it for boot
> >
> > I agree with this, please see my previous comments,
> > ' When you say about complete boot, then this may not be useful.'
> >
> > > 2) not a command described by UEFI Shell spec, but I keep seeing
> > >platforms including it even in RELEASE builds (most likely because 1)
> > > 3) code quality/maintainability
> >
> > > > > If the code is to be kept, I think (from a quick glance) that I
> > > > > would also like to see
> > > > >   *Data = NULL
> > > > > in the error path of DownloadFile().
> > >
> > > OK, so we don't need to drop it right now, but what's your take on this
> > > comment?
> >
> > I am fine, if you prefer to remove this then we will 

Re: [edk2] [PATCH 0/4] minuscule cleanups for Shell library resolutions

[Laszlo Ersek]

On 02/12/18 14:45, Laszlo Ersek wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: shell_libs_cleanup
> 
> Cc: Andrew Fish 
> Cc: Ard Biesheuvel 
> Cc: Eric Dong 
> Cc: Jaben Carsey 
> Cc: Jordan Justen 
> Cc: Ruiyu Ni 
> Cc: Star Zeng 
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (4):
>   OvmfPkg: drop stale SafeBlockIoLib and SafeOpenProtocolLib resolutions
>   EmulatorPkg: drop stale SafeBlockIoLib and SafeOpenProtocolLib
> resolutions
>   MdeModulePkg/UefiBootManagerLib: remove superfluous TimerLib
> dependency
>   ShellPkg: remove superfluous TimerLib resolution
> 
>  EmulatorPkg/EmulatorPkg.dsc| 2 --
>  MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h   | 1 -
>  MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf | 1 -
>  OvmfPkg/OvmfPkgIa32.dsc| 2 --
>  OvmfPkg/OvmfPkgIa32X64.dsc | 2 --
>  OvmfPkg/OvmfPkgX64.dsc | 2 --
>  ShellPkg/ShellPkg.dsc  | 1 -
>  7 files changed, 11 deletions(-)
> 

Thank you all for the feedpack; I pushed the following commits:

 1  f3f80a57e164 OvmfPkg: drop stale SafeBlockIoLib and SafeOpenProtocolLib 
resolutions
 2  bad0b5e87165 MdeModulePkg/UefiBootManagerLib: remove superfluous 
TimerLib dependency
 3  7a141b1306f6 ShellPkg: remove superfluous TimerLib resolution

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

Re: [edk2] [Patch] MdePkg/BaseSafeIntLib: Add SafeIntLib class and instance

[Laszlo Ersek]

Sean, Michael,

can you please follow up on this?

To clarify, I think this is a serious bug in SafeIntLib, dependent on
what we want to use this library for. As far as I understand, SafeIntLib
intends to centralize integer manipulation / arithmetic, so that client
code need not concern itself with overflow checking and such (on the C
expression level -- it still has to check return statuses, of course).
In other words, undefined behavior related to integer arithmetic is
supposed to be prevented in various modules by moving all such
operations into SafeIntLib, and letting client code use SafeIntLib APIs.

However, for this to work, SafeIntLib itself must be 100% free of
undefined behavior. And that's not the case (unless we define additional
guarantees -- on top of ISO C -- for edk2 target architectures). Should
I file a TianoCore BZ? Or is someone already (re)auditing the library?
Or else, is my concern unjustified? Please comment.

Thanks,
Laszlo

On 02/08/18 01:45, Laszlo Ersek wrote:
> On 02/08/18 01:32, Laszlo Ersek wrote:
>> On 12/19/17 20:36, Kinney, Michael D wrote:
>>> From: Sean Brogan 
>>>
>>> SafeIntLib provides helper functions to prevent integer overflow
>>> during type conversion, addition, subtraction, and multiplication.
>>
>> I clearly cannot review such a huge patch, but I've noticed something
>> and would like to ask for clarification:
>>
>>> +/**
>>> +  INT64 Subtraction
>>> +
>>> +  Performs the requested operation using the input parameters into a value
>>> +  specified by Result type and stores the converted value into the caller
>>> +  allocated output buffer specified by Result.  The caller must pass in a
>>> +  Result buffer that is at least as large as the Result type.
>>> +
>>> +  If Result is NULL, RETURN_INVALID_PARAMETER is returned.
>>> +
>>> +  If the requested operation results in an overflow or an underflow 
>>> condition,
>>> +  then Result is set to INT64_ERROR and RETURN_BUFFER_TOO_SMALL is 
>>> returned.
>>> +
>>> +  @param[in]   Minuend A number from which another is to be subtracted.
>>> +  @param[in]   Subtrahend  A number to be subtracted from another
>>> +  @param[out]  Result  Pointer to the result of subtraction
>>> +
>>> +  @retval  RETURN_SUCCESSSuccessful subtraction
>>> +  @retval  RETURN_BUFFER_TOO_SMALL   Underflow
>>> +  @retval  RETURN_INVALID_PARAMETER  Result is NULL
>>> +**/
>>> +RETURN_STATUS
>>> +EFIAPI
>>> +SafeInt64Sub (
>>> +  IN  INT64  Minuend,
>>> +  IN  INT64  Subtrahend,
>>> +  OUT INT64  *Result
>>> +  )
>>> +{
>>> +  RETURN_STATUS  Status;
>>> +  INT64  SignedResult;
>>> +
>>> +  if (Result == NULL) {
>>> +return RETURN_INVALID_PARAMETER;
>>> +  }
>>> +
>>> +  SignedResult = Minuend - Subtrahend;
>>> +
>>> +  //
>>> +  // Subtracting a positive number from a positive number never overflows.
>>> +  // Subtracting a negative number from a negative number never overflows.
>>> +  // If you subtract a negative number from a positive number, you expect 
>>> a positive result.
>>> +  // If you subtract a positive number from a negative number, you expect 
>>> a negative result.
>>> +  // Overflow if inputs vary in sign and the output does not have the same 
>>> sign as the first input.
>>> +  //
>>> +  if (((Minuend < 0) != (Subtrahend < 0)) &&
>>> +  ((Minuend < 0) != (SignedResult < 0))) {
>>> +*Result = INT64_ERROR;
>>> +Status = RETURN_BUFFER_TOO_SMALL;
>>> +  } else {
>>> +*Result = SignedResult;
>>> +Status = RETURN_SUCCESS;
>>> +  }
>>> +
>>> +  return Status;
>>> +}
>>
>> Is our goal to
>>
>> (a) catch overflows before the caller goes wrong due to them, or
>> (b) completely prevent undefined behavior from happening, even inside
>> SafeInt64Sub()?
>>
>> The above implementation may be good for (a), but it's not correct for
>> (b). The
>>
>>   SignedResult = Minuend - Subtrahend;
>>
>> subtraction invokes undefined behavior if the result cannot be
>> represented [*]; the rest of the code cannot help.
>>
>> Now if we say that such subtractions always occur according to the
>> "usual two's complement definition", on all architectures that edk2
>> targets, and we're sure that no compiler or analysis tool will flag --
>> or exploit -- the UB either, then the code is fine; meaning our choice
>> is (a).
>>
>> But, if (b) is our goal, I would replace the current error condition with:
>>
>>   (((Subtrahend > 0) && (Minuend < MIN_INT64 + Subtrahend)) ||
>>((Subtrahend < 0) && (Minuend > MAX_INT64 + Subtrahend)))
> 
> To clarify, I wouldn't just replace the error condition. In addition to
> that, I would remove the SignedResult helper variable (together with the
> current subtraction), and calculate & assign
> 
>   *Result = Minuend - Subtrahend;
> 
> only after the error condition fails (i.e. the subtraction is safe).
> 
> Thanks,
> Laszlo
> 
> 
>> Justification:
>>
>> * Subtrahend==0 can never cause overflow
>>
>> * Subtrahend>0 can only cause overflow at the 

[edk2] [PATCH] ShellPkg/[hex]edit: Fix CTRL+ doesn't work from hyper terminal

[Ruiyu Ni]

After commit 20ddbc133f679b7895dfdaf2fd58ec4c8183a1d8
* MdeModulePkg/ConSplitter: ReadKeyStrokeEx always return key state

When one physical console supports to report the shift key state,
the key data returned from ConSplitter driver at least carries
the shift key valid bit.
The patch fixes the edit/hexedit to accept Unicode (1) when
the no shift key is pressed or reported.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni 
Cc: Jaben Carsey 
---
 .../Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c   | 10 ++
 ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c  | 10 ++
 .../Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c | 10 ++
 3 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c 
b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
index 4eb998bf5f..6832441e81 100644
--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
@@ -1387,18 +1387,20 @@ MainCommandDisplayHelp (
   continue;
 }
 
-if ((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) == 0) {
+if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) == 0) ||
+(KeyData.KeyState.KeyShiftState == EFI_SHIFT_STATE_VALID)) {
   //
-  // For consoles that don't support shift state reporting,
+  // For consoles that don't support/report shift state,
   // CTRL+W is translated to L'W' - L'A' + 1.
   //
   if (KeyData.Key.UnicodeChar == L'W' - L'A' + 1) {
 break;
   }
-} else if (((KeyData.KeyState.KeyShiftState & (EFI_LEFT_CONTROL_PRESSED | 
EFI_RIGHT_CONTROL_PRESSED)) != 0) &&
+} else if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0) 
&&
+   ((KeyData.KeyState.KeyShiftState & (EFI_LEFT_CONTROL_PRESSED | 
EFI_RIGHT_CONTROL_PRESSED)) != 0) &&
((KeyData.KeyState.KeyShiftState & ~(EFI_SHIFT_STATE_VALID | 
EFI_LEFT_CONTROL_PRESSED | EFI_RIGHT_CONTROL_PRESSED)) == 0)) {
   //
-  // For consoles that supports shift state reporting,
+  // For consoles that supports/reports shift state,
   // make sure that only CONTROL shift key is pressed.
   //
   if ((KeyData.Key.UnicodeChar == 'w') || (KeyData.Key.UnicodeChar == 
'W')) {
diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c 
b/ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c
index b86594bb28..58e90ac5b2 100644
--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c
+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c
@@ -183,16 +183,18 @@ MenuBarDispatchControlHotKey (
   //
   ControlIndex = MAX_UINT16;
 
-  if ((KeyData->KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) == 0) {
+  if (((KeyData->KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) == 0) ||
+  (KeyData->KeyState.KeyShiftState == EFI_SHIFT_STATE_VALID)) {
 //
-// For those console devices that cannot report the CONTROL state,
+// For consoles that don't support/report shift state,
 // Ctrl+A is translated to 1 (UnicodeChar).
 //
 ControlIndex = KeyData->Key.UnicodeChar;
-  } else if (((KeyData->KeyState.KeyShiftState & (EFI_RIGHT_CONTROL_PRESSED | 
EFI_LEFT_CONTROL_PRESSED)) != 0) &&
+  } else if (((KeyData->KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0) 
&&
+ ((KeyData->KeyState.KeyShiftState & (EFI_RIGHT_CONTROL_PRESSED | 
EFI_LEFT_CONTROL_PRESSED)) != 0) &&
  ((KeyData->KeyState.KeyShiftState & ~(EFI_SHIFT_STATE_VALID | 
EFI_RIGHT_CONTROL_PRESSED | EFI_LEFT_CONTROL_PRESSED)) == 0)) {
 //
-// For those console devices that can report the CONTROL state,
+// For consoles that supports/reports shift state,
 // make sure only CONTROL is pressed.
 //
 if ((KeyData->Key.UnicodeChar >= L'A') && (KeyData->Key.UnicodeChar <= 
L'Z')) {
diff --git 
a/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c 
b/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c
index 2b096d7168..a2e52ea39c 100644
--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c
+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c
@@ -134,18 +134,20 @@ HMainCommandDisplayHelp (
   continue;
 }
 
-if ((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) == 0) {
+if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) == 0) ||
+(KeyData.KeyState.KeyShiftState == EFI_SHIFT_STATE_VALID)) {
   //
-  // For consoles that don't support shift state reporting,
+  // For consoles that don't support/report shift state,
   // CTRL+W is translated to L'W' - L'A' + 1.
   //
   if (KeyData.Key.UnicodeChar == L'W' - L'A' + 1) {
 break;
   }
-} else if (((KeyData.KeyState.KeyShiftState & 

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

[Meenakshi Aggarwal]

Hi,

As per commit 0961002352e9115b72f544dded239ad226efe87b

Tftp command will be maintained to extend internal commands and

ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c

Looks like a copy of ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c

So, below fix is needed in this case as well.

Please suggest, so we can send the updated patch [incorporating Leif's comments]


Thanks,
Meenakshi

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Udit Kumar
> Sent: Thursday, November 09, 2017 10:13 AM
> To: Leif Lindholm 
> Cc: ruiyu...@intel.com; ard.biesheu...@linaro.org; ting...@intel.com; edk2-
> de...@lists.01.org; jaben.car...@intel.com; siyuan...@intel.com
> Subject: Re: [edk2] [PATCH] Tftp assert fix for openfile failure case
> 
> 
> 
> > -Original Message-
> > From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> > Sent: Wednesday, November 08, 2017 8:52 PM
> > To: Udit Kumar 
> > Cc: Vabhav Sharma ; edk2-devel@lists.01.org;
> > ruiyu...@intel.com; jaben.car...@intel.com; ard.biesheu...@linaro.org;
> > siyuan...@intel.com; ting...@intel.com
> > Subject: Re: [PATCH] Tftp assert fix for openfile failure case
> >
> > On Wed, Nov 08, 2017 at 05:15:49AM +, Udit Kumar wrote:
> > > > > diff --git a/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > > > b/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > > > index fbde3bf..6425fc5 100755
> > > > > --- a/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > > > +++ b/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > > > @@ -509,6 +509,7 @@ ShellCommandRunTftp (
> > > > >);
> > > > >goto NextHandle;
> > > >
> > > > Wow, a goto in a foor loop in a 320-line function.
> > > > What could possibly go wrong?
> > >
> > > Instead of being on some volume, if you are on Shell.
> > > Then file open will fail.
> >
> > Sure. The above was a snarky comment on the state of the existing code.
> >
> > > > >  }
> > > > > +DataSize = FileSize;
> > > > >
> > > > >  Status = DownloadFile (Mtftp4, RemoteFilePath,
> > > > > AsciiRemoteFilePath,
> > > > FileSize, BlockSize, );
> > > > >  if (EFI_ERROR (Status)) {
> > > > > @@ -539,7 +540,6 @@ ShellCommandRunTftp (
> > > > >goto NextHandle;
> > > > >  }
> > > > >
> > > > > -DataSize = FileSize;
> > > > >  Status = ShellWriteFile (FileHandle, , Data);
> > > > >  if (!EFI_ERROR (Status)) {
> > > > >ShellStatus = SHELL_SUCCESS;
> > > > > --
> > > > > 1.9.1
> > > >
> > > > So, a wider question:
> > > > This shell command was introduced in the heyday of "let's
> > > > reimplement U-Boot in the EDK2 tree". Mainly, from my impression, it
> > > > seems to be used in order that people don't need to learn how boot
> > > > managers and device paths work.
> > >
> > > When you say about complete boot, then this may not be useful.
> > >
> > > > Am I being too harsh?
> > > > Are there practical uses for this?
> > >
> > > For doing some sort of unit testing of given interface. I found this
> > > useful. During development, this is useful to transfer generic file to
> > > development board.
> >
> > OK, I can see how it could be useful.
> > My opposition is based on three things:
> > 1) people _are_ trying to use it for boot
> 
> I agree with this, please see my previous comments,
> ' When you say about complete boot, then this may not be useful.'
> 
> > 2) not a command described by UEFI Shell spec, but I keep seeing
> >platforms including it even in RELEASE builds (most likely because 1)
> > 3) code quality/maintainability
> 
> > > > If the code is to be kept, I think (from a quick glance) that I
> > > > would also like to see
> > > >   *Data = NULL
> > > > in the error path of DownloadFile().
> >
> > OK, so we don't need to drop it right now, but what's your take on this
> > comment?
> 
> I am fine, if you prefer to remove this then we will develop some test
> application
> for unit tests.
> In case, we need to maintain this piece of code then above needs to fix as
> well.
> 
> 
> > /
> > Leif
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> s.01.org%2Fmailman%2Flistinfo%2Fedk2-
> devel=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7Cc2673b1a07e
> 94e9f937b08d5272c6e5b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> %7C636457994167308954=gBy9RA5d1NpsvxQkmET0HFzsJB8FK7KLueE
> NXFTjSHY%3D=0
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH] ShellPkg/ShellLib: Fix a bug in InternalShellIsHexOrDecimalNumber

[Ruiyu Ni]

InternalShellIsHexOrDecimalNumber() wrongly treats "-" as a number.
The patch fixes this issue.

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

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni 
Cc: Vladimir Olovyannikov 
Cc: Jaben Carsey 
---
 ShellPkg/Library/UefiShellLib/UefiShellLib.c | 23 +++
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c 
b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
index 7f6389f655..e53985e2d7 100644
--- a/ShellPkg/Library/UefiShellLib/UefiShellLib.c
+++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
@@ -3,7 +3,7 @@
 
   (C) Copyright 2016 Hewlett Packard Enterprise Development LP
   Copyright 2016 Dell Inc.
-  Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
+  Copyright (c) 2006 - 2018, 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
@@ -3636,29 +3636,36 @@ InternalShellIsHexOrDecimalNumber (
   )
 {
   BOOLEAN Hex;
+  BOOLEAN LeadingZero;
+
+  if (String == NULL) {
+return FALSE;
+  }
 
   //
   // chop off a single negative sign
   //
-  if (String != NULL && *String == L'-') {
+  if (*String == L'-') {
 String++;
   }
 
-  if (String == NULL) {
-return (FALSE);
+  if (*String == CHAR_NULL) {
+return FALSE;
   }
 
   //
   // chop leading zeroes
   //
-  while(String != NULL && *String == L'0'){
+  LeadingZero = FALSE;
+  while(*String == L'0'){
 String++;
+LeadingZero = TRUE;
   }
   //
   // allow '0x' or '0X', but not 'x' or 'X'
   //
-  if (String != NULL && (*String == L'x' || *String == L'X')) {
-if (*(String-1) != L'0') {
+  if (*String == L'x' || *String == L'X') {
+if (!LeadingZero) {
   //
   // we got an x without a preceeding 0
   //
@@ -3675,7 +3682,7 @@ InternalShellIsHexOrDecimalNumber (
   //
   // loop through the remaining characters and use the lib function
   //
-  for ( ; String != NULL && *String != CHAR_NULL && !(StopAtSpace && *String 
== L' ') ; String++){
+  for ( ; *String != CHAR_NULL && !(StopAtSpace && *String == L' ') ; 
String++){
 if (TimeNumbers && (String[0] == L':')) {
   continue;
 }
-- 
2.16.1.windows.1

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