Date   

Re: [PATCH] UefiCpuPkg/CpuFeature: reduce time complexty to calc CpuInfo.First

Ni, Ray
 

+ FirstPackage = MAX_UINT32;

+ for (PackageIndex = 0; PackageIndex < CpuStatus->PackageCount;
PackageIndex++) {

+ FirstCore[PackageIndex] = MAX_UINT32;

+ for (CoreIndex = 0; CoreIndex < CpuStatus->MaxCoreCount; CoreIndex++)
{

+ FirstThread[PackageIndex * CpuStatus->MaxCoreCount + CoreIndex] =
MAX_UINT32;

+ }

+ }
Could this code block be replaced by a SetMem32(xxx, xxx, MAX_UINT32) call?
Yes. it could. I thought the for loop is more readable. Maybe it doesn't help a lot
on the code readability. Let me send an updated version to use SetMem32.


[PATCH v2 edk2-platforms 2/2] Platform/ARM: Fix Ecc error 8005

PierreGondois
 

From: Pierre Gondois <Pierre.Gondois@...>

Following the Ecc reported error in the edk2 repository,
an enum and its elements have been renamed in:
ArmPlatformPkg/Include/Library/LcdPlatformLib.h

The Ecc error reported in edk2 is:
Variable name does not follow the rules:
1. First character should be upper case
2. Must contain lower case characters
3. No white space characters
4. Global variable name must start with a 'g'

Indeed, according to the EDK II C Coding Standards
Specification, s5.6.2.2 "Enumerated Types" and
s4.3.4 Function and Data Names, elements of an
enumerated type "shoud be a mixed upper- and
lower-case text".

Signed-off-by: Pierre Gondois <Pierre.Gondois@...>
---
The changes can be seen at: https://github.com/PierreARM/edk2-platforms/tree/pg/1537_Ecc_ArmPlatformPkg_v2

Notes:
v2:
-Rename enum ELcd... to Lcd... [asked by Ard]

.../Library/HdLcdArmJunoLib/HdLcdArmJuno.c | 4 +--
.../Library/HdLcdArmSgiLib/HdLcdArmSgi.c | 4 +--
.../Library/ArmMaliDpLib/ArmMaliDpLib.c | 4 +--
.../HdLcdArmVExpressLib/HdLcdArmVExpress.c | 4 +--
.../PL111LcdArmVExpress.c | 34 +++++++++----------
5 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/Platform/ARM/JunoPkg/Library/HdLcdArmJunoLib/HdLcdArmJuno.c b/Platform/ARM/JunoPkg/Library/HdLcdArmJunoLib/HdLcdArmJuno.c
index 4b961b7a9f7f..30558878f068 100644
--- a/Platform/ARM/JunoPkg/Library/HdLcdArmJunoLib/HdLcdArmJuno.c
+++ b/Platform/ARM/JunoPkg/Library/HdLcdArmJunoLib/HdLcdArmJuno.c
@@ -1,6 +1,6 @@
/** @file

- Copyright (c) 2013-2018, ARM Ltd. All rights reserved.
+ Copyright (c) 2013 - 2020, Arm Limited. All rights reserved.<BR>

SPDX-License-Identifier: BSD-2-Clause-Patent

@@ -543,7 +543,7 @@ LcdPlatformGetBpp (

ASSERT (Bpp != NULL);

- *Bpp = LCD_BITS_PER_PIXEL_24;
+ *Bpp = LcdBitsPerPixel_24;

return EFI_SUCCESS;
}
diff --git a/Platform/ARM/SgiPkg/Library/HdLcdArmSgiLib/HdLcdArmSgi.c b/Platform/ARM/SgiPkg/Library/HdLcdArmSgiLib/HdLcdArmSgi.c
index 6f747d2545db..561f3f6747e8 100644
--- a/Platform/ARM/SgiPkg/Library/HdLcdArmSgiLib/HdLcdArmSgi.c
+++ b/Platform/ARM/SgiPkg/Library/HdLcdArmSgiLib/HdLcdArmSgi.c
@@ -1,6 +1,6 @@
/** @file
*
-* Copyright (c) 2018, ARM Limited. All rights reserved.
+* Copyright (c) 2018 - 2020, Arm Limited. All rights reserved.<BR>
*
* SPDX-License-Identifier: BSD-2-Clause-Patent
*
@@ -245,7 +245,7 @@ LcdPlatformGetBpp (

ASSERT (Bpp != NULL);

- *Bpp = LCD_BITS_PER_PIXEL_24;
+ *Bpp = LcdBitsPerPixel_24;

return EFI_SUCCESS;
}
diff --git a/Platform/ARM/VExpressPkg/Library/ArmMaliDpLib/ArmMaliDpLib.c b/Platform/ARM/VExpressPkg/Library/ArmMaliDpLib/ArmMaliDpLib.c
index aa55bc2e5bee..a62dce1331d9 100644
--- a/Platform/ARM/VExpressPkg/Library/ArmMaliDpLib/ArmMaliDpLib.c
+++ b/Platform/ARM/VExpressPkg/Library/ArmMaliDpLib/ArmMaliDpLib.c
@@ -2,7 +2,7 @@

The file contains Arm Mali DP platform specific implementation.

- Copyright (c) 2017-2018, Arm Limited. All rights reserved.
+ Copyright (c) 2017 - 2020, Arm Limited. All rights reserved.<BR>

SPDX-License-Identifier: BSD-2-Clause-Patent

@@ -375,7 +375,7 @@ LcdPlatformGetBpp (
return EFI_INVALID_PARAMETER;
}

- *Bpp = LCD_BITS_PER_PIXEL_24;
+ *Bpp = LcdBitsPerPixel_24;

return EFI_SUCCESS;
}
diff --git a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
index c4b163d35f18..919ef30019ec 100644
--- a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
+++ b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
@@ -1,6 +1,6 @@
/** @file

- Copyright (c) 2012-2018, ARM Ltd. All rights reserved.
+ Copyright (c) 2012 - 2020, Arm Limited. All rights reserved.<BR>

SPDX-License-Identifier: BSD-2-Clause-Patent

@@ -361,7 +361,7 @@ LcdPlatformGetBpp (
return EFI_INVALID_PARAMETER;
}

- *Bpp = LCD_BITS_PER_PIXEL_24;
+ *Bpp = LcdBitsPerPixel_24;

return EFI_SUCCESS;
}
diff --git a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
index b7396a87bd05..87b82e9f97dc 100644
--- a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
+++ b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
@@ -1,6 +1,6 @@
/** @file

- Copyright (c) 2011-2018, ARM Ltd. All rights reserved.<BR>
+ Copyright (c) 2011 - 2020, Arm Limited. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -33,97 +33,97 @@ typedef struct {
**/
STATIC DISPLAY_MODE mDisplayModes[] = {
{ // Mode 0 : VGA : 640 x 480 x 24 bpp
- VGA, LCD_BITS_PER_PIXEL_24,
+ VGA, LcdBitsPerPixel_24,
VGA_OSC_FREQUENCY,
{VGA_H_RES_PIXELS, VGA_H_SYNC, VGA_H_BACK_PORCH, VGA_H_FRONT_PORCH},
{VGA_V_RES_PIXELS, VGA_V_SYNC, VGA_V_BACK_PORCH, VGA_V_FRONT_PORCH}
},
{ // Mode 1 : SVGA : 800 x 600 x 24 bpp
- SVGA, LCD_BITS_PER_PIXEL_24,
+ SVGA, LcdBitsPerPixel_24,
SVGA_OSC_FREQUENCY,
{SVGA_H_RES_PIXELS, SVGA_H_SYNC, SVGA_H_BACK_PORCH, SVGA_H_FRONT_PORCH},
{SVGA_V_RES_PIXELS, SVGA_V_SYNC, SVGA_V_BACK_PORCH, SVGA_V_FRONT_PORCH}
},
{ // Mode 2 : XGA : 1024 x 768 x 24 bpp
- XGA, LCD_BITS_PER_PIXEL_24,
+ XGA, LcdBitsPerPixel_24,
XGA_OSC_FREQUENCY,
{XGA_H_RES_PIXELS, XGA_H_SYNC, XGA_H_BACK_PORCH, XGA_H_FRONT_PORCH},
{XGA_V_RES_PIXELS, XGA_V_SYNC, XGA_V_BACK_PORCH, XGA_V_FRONT_PORCH}
},
{ // Mode 3 : SXGA : 1280 x 1024 x 24 bpp
- SXGA, LCD_BITS_PER_PIXEL_24,
+ SXGA, LcdBitsPerPixel_24,
(SXGA_OSC_FREQUENCY/2),
{SXGA_H_RES_PIXELS, SXGA_H_SYNC, SXGA_H_BACK_PORCH, SXGA_H_FRONT_PORCH},
{SXGA_V_RES_PIXELS, SXGA_V_SYNC, SXGA_V_BACK_PORCH, SXGA_V_FRONT_PORCH}
},
{ // Mode 4 : UXGA : 1600 x 1200 x 24 bpp
- UXGA, LCD_BITS_PER_PIXEL_24,
+ UXGA, LcdBitsPerPixel_24,
(UXGA_OSC_FREQUENCY/2),
{UXGA_H_RES_PIXELS, UXGA_H_SYNC, UXGA_H_BACK_PORCH, UXGA_H_FRONT_PORCH},
{UXGA_V_RES_PIXELS, UXGA_V_SYNC, UXGA_V_BACK_PORCH, UXGA_V_FRONT_PORCH}
},
{ // Mode 5 : HD : 1920 x 1080 x 24 bpp
- HD, LCD_BITS_PER_PIXEL_24,
+ HD, LcdBitsPerPixel_24,
(HD_OSC_FREQUENCY/2),
{HD_H_RES_PIXELS, HD_H_SYNC, HD_H_BACK_PORCH, HD_H_FRONT_PORCH},
{HD_V_RES_PIXELS, HD_V_SYNC, HD_V_BACK_PORCH, HD_V_FRONT_PORCH}
},
{ // Mode 6 : VGA : 640 x 480 x 16 bpp (565 Mode)
- VGA, LCD_BITS_PER_PIXEL_16_565,
+ VGA, LcdBitsPerPixel_16_565,
VGA_OSC_FREQUENCY,
{VGA_H_RES_PIXELS, VGA_H_SYNC, VGA_H_BACK_PORCH, VGA_H_FRONT_PORCH},
{VGA_V_RES_PIXELS, VGA_V_SYNC, VGA_V_BACK_PORCH, VGA_V_FRONT_PORCH}
},
{ // Mode 7 : SVGA : 800 x 600 x 16 bpp (565 Mode)
- SVGA, LCD_BITS_PER_PIXEL_16_565,
+ SVGA, LcdBitsPerPixel_16_565,
SVGA_OSC_FREQUENCY,
{SVGA_H_RES_PIXELS, SVGA_H_SYNC, SVGA_H_BACK_PORCH, SVGA_H_FRONT_PORCH},
{SVGA_V_RES_PIXELS, SVGA_V_SYNC, SVGA_V_BACK_PORCH, SVGA_V_FRONT_PORCH}
},
{ // Mode 8 : XGA : 1024 x 768 x 16 bpp (565 Mode)
- XGA, LCD_BITS_PER_PIXEL_16_565,
+ XGA, LcdBitsPerPixel_16_565,
XGA_OSC_FREQUENCY,
{XGA_H_RES_PIXELS, XGA_H_SYNC, XGA_H_BACK_PORCH, XGA_H_FRONT_PORCH},
{XGA_V_RES_PIXELS, XGA_V_SYNC, XGA_V_BACK_PORCH, XGA_V_FRONT_PORCH}
},
{ // Mode 9 : VGA : 640 x 480 x 15 bpp
- VGA, LCD_BITS_PER_PIXEL_16_555,
+ VGA, LcdBitsPerPixel_16_555,
VGA_OSC_FREQUENCY,
{VGA_H_RES_PIXELS, VGA_H_SYNC, VGA_H_BACK_PORCH, VGA_H_FRONT_PORCH},
{VGA_V_RES_PIXELS, VGA_V_SYNC, VGA_V_BACK_PORCH, VGA_V_FRONT_PORCH}
},
{ // Mode 10 : SVGA : 800 x 600 x 15 bpp
- SVGA, LCD_BITS_PER_PIXEL_16_555,
+ SVGA, LcdBitsPerPixel_16_555,
SVGA_OSC_FREQUENCY,
{SVGA_H_RES_PIXELS, SVGA_H_SYNC, SVGA_H_BACK_PORCH, SVGA_H_FRONT_PORCH},
{SVGA_V_RES_PIXELS, SVGA_V_SYNC, SVGA_V_BACK_PORCH, SVGA_V_FRONT_PORCH}
},
{ // Mode 11 : XGA : 1024 x 768 x 15 bpp
- XGA, LCD_BITS_PER_PIXEL_16_555,
+ XGA, LcdBitsPerPixel_16_555,
XGA_OSC_FREQUENCY,
{XGA_H_RES_PIXELS, XGA_H_SYNC, XGA_H_BACK_PORCH, XGA_H_FRONT_PORCH},
{XGA_V_RES_PIXELS, XGA_V_SYNC, XGA_V_BACK_PORCH, XGA_V_FRONT_PORCH}
},
{ // Mode 12 : XGA : 1024 x 768 x 15 bpp - All the timing info is derived from Linux Kernel Driver Settings
- XGA, LCD_BITS_PER_PIXEL_16_555,
+ XGA, LcdBitsPerPixel_16_555,
63500000,
{XGA_H_RES_PIXELS, XGA_H_SYNC, XGA_H_BACK_PORCH, XGA_H_FRONT_PORCH},
{XGA_V_RES_PIXELS, XGA_V_SYNC, XGA_V_BACK_PORCH, XGA_V_FRONT_PORCH}
},
{ // Mode 13 : VGA : 640 x 480 x 12 bpp (444 Mode)
- VGA, LCD_BITS_PER_PIXEL_12_444,
+ VGA, LcdBitsPerPixel_12_444,
VGA_OSC_FREQUENCY,
{VGA_H_RES_PIXELS, VGA_H_SYNC, VGA_H_BACK_PORCH, VGA_H_FRONT_PORCH},
{VGA_V_RES_PIXELS, VGA_V_SYNC, VGA_V_BACK_PORCH, VGA_V_FRONT_PORCH}
},
{ // Mode 14 : SVGA : 800 x 600 x 12 bpp (444 Mode)
- SVGA, LCD_BITS_PER_PIXEL_12_444,
+ SVGA, LcdBitsPerPixel_12_444,
SVGA_OSC_FREQUENCY,
{SVGA_H_RES_PIXELS, SVGA_H_SYNC, SVGA_H_BACK_PORCH, SVGA_H_FRONT_PORCH},
{SVGA_V_RES_PIXELS, SVGA_V_SYNC, SVGA_V_BACK_PORCH, SVGA_V_FRONT_PORCH}
},
{ // Mode 15 : XGA : 1024 x 768 x 12 bpp (444 Mode)
- XGA, LCD_BITS_PER_PIXEL_12_444,
+ XGA, LcdBitsPerPixel_12_444,
XGA_OSC_FREQUENCY,
{XGA_H_RES_PIXELS, XGA_H_SYNC, XGA_H_BACK_PORCH, XGA_H_FRONT_PORCH},
{XGA_V_RES_PIXELS, XGA_V_SYNC, XGA_V_BACK_PORCH, XGA_V_FRONT_PORCH}
--
2.17.1


[PATCH v2 edk2-platforms 1/2] Platform/ARM/VExpressPkg: Remove LcdPlatformLib.h

PierreGondois
 

From: Pierre Gondois <Pierre.Gondois@...>

LcdPlatformLib.h was introduced by 8ad58788b5c3 as a duplicate
of the edk2 original file at:
edk2/ArmPlatformPkg/Include/Library/LcdPlatformLib.h

The list of files in edk2-plarforms currently using LcdPlatformLib.h
is the following:
Platform/ARM/SgiPkg/Library/HdLcdArmSgiLib/HdLcdArmSgi.c:11
Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
Platform/ARM/VExpressPkg/Library/ArmMaliDpLib/ArmMaliDpLib.c
Platform/ARM/JunoPkg/Library/HdLcdArmJunoLib/HdLcdArmJuno.c

They are all using the SCAN_TIMINGS structure. However, this structure
is only defined in the original file in the edk2 repository. Thus,
the edk2-platforms is un-used and can be safely removed.

The build system prefers the edk2 version of the file over its
edk2-platforms copy due to the .dec files order in
each .inf file where the the library is used.

Signed-off-by: Pierre Gondois <Pierre.Gondois@...>
---
The changes can be seen at: https://github.com/PierreARM/edk2-platforms/tree/pg/1537_Ecc_ArmPlatformPkg_v2

Notes:
v2:
-Remove reference to LcdPlatformLib.h in .dec file
[asked by Ard]

Platform/ARM/VExpressPkg/ArmVExpressPkg.dec | 3 +-
.../Include/Library/LcdPlatformLib.h | 215 ------------------
2 files changed, 1 insertion(+), 217 deletions(-)
delete mode 100644 Platform/ARM/VExpressPkg/Include/Library/LcdPlatformLib.h

diff --git a/Platform/ARM/VExpressPkg/ArmVExpressPkg.dec b/Platform/ARM/VExpressPkg/ArmVExpressPkg.dec
index f78c5ce7c764..848510bff17e 100644
--- a/Platform/ARM/VExpressPkg/ArmVExpressPkg.dec
+++ b/Platform/ARM/VExpressPkg/ArmVExpressPkg.dec
@@ -1,7 +1,7 @@
#/** @file
# Arm Versatile Express package.
#
-# Copyright (c) 2012-2018, ARM Limited. All rights reserved.
+# Copyright (c) 2012-2020, Arm Limited. All rights reserved.<BR>
#
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
@@ -27,7 +27,6 @@ [Includes.common]

[LibraryClasses]
ArmPlatformSysConfigLib|Include/Library/ArmPlatformSysConfigLib.h
- LcdPlatformLib|Include/Library/LcdPlatformLib.h

[Guids.common]
gArmVExpressTokenSpaceGuid = { 0x9c0aaed4, 0x74c5, 0x4043, { 0xb4, 0x17, 0xa3, 0x22, 0x38, 0x14, 0xce, 0x76 } }
diff --git a/Platform/ARM/VExpressPkg/Include/Library/LcdPlatformLib.h b/Platform/ARM/VExpressPkg/Include/Library/LcdPlatformLib.h
deleted file mode 100644
index 5591147313ac..000000000000
--- a/Platform/ARM/VExpressPkg/Include/Library/LcdPlatformLib.h
+++ /dev/null
@@ -1,215 +0,0 @@
-/** @file
-
- Copyright (c) 2011, ARM Ltd. All rights reserved.<BR>
- SPDX-License-Identifier: BSD-2-Clause-Patent
-
- **/
-
-#ifndef __LCDPLATFORMLIB_H
-#define __LCDPLATFORMLIB_H
-
-#include <Protocol/GraphicsOutput.h>
-
-#define LCD_VRAM_SIZE SIZE_8MB
-
-//
-// Modes definitions
-//
-#define VGA 0
-#define SVGA 1
-#define XGA 2
-#define SXGA 3
-#define WSXGA 4
-#define UXGA 5
-#define HD 6
-
-//
-// VGA Mode: 640 x 480
-//
-#define VGA_H_RES_PIXELS 640
-#define VGA_V_RES_PIXELS 480
-#define VGA_OSC_FREQUENCY 23750000 /* 0x016A6570 */
-
-#define VGA_H_SYNC ( 80 - 1)
-#define VGA_H_FRONT_PORCH ( 16 - 1)
-#define VGA_H_BACK_PORCH ( 64 - 1)
-
-#define VGA_V_SYNC ( 4 - 1)
-#define VGA_V_FRONT_PORCH ( 3 - 1)
-#define VGA_V_BACK_PORCH ( 13 - 1)
-
-//
-// SVGA Mode: 800 x 600
-//
-#define SVGA_H_RES_PIXELS 800
-#define SVGA_V_RES_PIXELS 600
-#define SVGA_OSC_FREQUENCY 38250000 /* 0x0247A610 */
-
-#define SVGA_H_SYNC ( 80 - 1)
-#define SVGA_H_FRONT_PORCH ( 32 - 1)
-#define SVGA_H_BACK_PORCH (112 - 1)
-
-#define SVGA_V_SYNC ( 4 - 1)
-#define SVGA_V_FRONT_PORCH ( 3 - 1)
-#define SVGA_V_BACK_PORCH ( 17 - 1)
-
-//
-// XGA Mode: 1024 x 768
-//
-#define XGA_H_RES_PIXELS 1024
-#define XGA_V_RES_PIXELS 768
-#define XGA_OSC_FREQUENCY 63500000 /* 0x03C8EEE0 */
-
-#define XGA_H_SYNC (104 - 1)
-#define XGA_H_FRONT_PORCH ( 48 - 1)
-#define XGA_H_BACK_PORCH (152 - 1)
-
-#define XGA_V_SYNC ( 4 - 1)
-#define XGA_V_FRONT_PORCH ( 3 - 1)
-#define XGA_V_BACK_PORCH ( 23 - 1)
-
-//
-// SXGA Mode: 1280 x 1024
-//
-#define SXGA_H_RES_PIXELS 1280
-#define SXGA_V_RES_PIXELS 1024
-#define SXGA_OSC_FREQUENCY 109000000 /* 0x067F3540 */
-
-#define SXGA_H_SYNC (136 - 1)
-#define SXGA_H_FRONT_PORCH ( 80 - 1)
-#define SXGA_H_BACK_PORCH (216 - 1)
-
-#define SXGA_V_SYNC ( 7 - 1)
-#define SXGA_V_FRONT_PORCH ( 3 - 1)
-#define SXGA_V_BACK_PORCH ( 29 - 1)
-
-//
-// WSXGA+ Mode: 1680 x 1050
-//
-#define WSXGA_H_RES_PIXELS 1680
-#define WSXGA_V_RES_PIXELS 1050
-#define WSXGA_OSC_FREQUENCY 147000000 /* 0x08C30AC0 */
-
-#define WSXGA_H_SYNC (170 - 1)
-#define WSXGA_H_FRONT_PORCH (104 - 1)
-#define WSXGA_H_BACK_PORCH (274 - 1)
-
-#define WSXGA_V_SYNC ( 5 - 1)
-#define WSXGA_V_FRONT_PORCH ( 4 - 1)
-#define WSXGA_V_BACK_PORCH ( 41 - 1)
-
-//
-// UXGA Mode: 1600 x 1200
-//
-#define UXGA_H_RES_PIXELS 1600
-#define UXGA_V_RES_PIXELS 1200
-#define UXGA_OSC_FREQUENCY 161000000 /* 0x0998AA40 */
-
-#define UXGA_H_SYNC (168 - 1)
-#define UXGA_H_FRONT_PORCH (112 - 1)
-#define UXGA_H_BACK_PORCH (280 - 1)
-
-#define UXGA_V_SYNC ( 4 - 1)
-#define UXGA_V_FRONT_PORCH ( 3 - 1)
-#define UXGA_V_BACK_PORCH ( 38 - 1)
-
-//
-// HD Mode: 1920 x 1080
-//
-#define HD_H_RES_PIXELS 1920
-#define HD_V_RES_PIXELS 1080
-#define HD_OSC_FREQUENCY 165000000 /* 0x09D5B340 */
-
-#define HD_H_SYNC ( 79 - 1)
-#define HD_H_FRONT_PORCH (128 - 1)
-#define HD_H_BACK_PORCH (328 - 1)
-
-#define HD_V_SYNC ( 5 - 1)
-#define HD_V_FRONT_PORCH ( 3 - 1)
-#define HD_V_BACK_PORCH ( 32 - 1)
-
-//
-// Colour Masks
-//
-
-#define LCD_24BPP_RED_MASK 0x00FF0000
-#define LCD_24BPP_GREEN_MASK 0x0000FF00
-#define LCD_24BPP_BLUE_MASK 0x000000FF
-#define LCD_24BPP_RESERVED_MASK 0xFF000000
-
-#define LCD_16BPP_555_RED_MASK 0x00007C00
-#define LCD_16BPP_555_GREEN_MASK 0x000003E0
-#define LCD_16BPP_555_BLUE_MASK 0x0000001F
-#define LCD_16BPP_555_RESERVED_MASK 0x00000000
-
-#define LCD_16BPP_565_RED_MASK 0x0000F800
-#define LCD_16BPP_565_GREEN_MASK 0x000007E0
-#define LCD_16BPP_565_BLUE_MASK 0x0000001F
-#define LCD_16BPP_565_RESERVED_MASK 0x00008000
-
-#define LCD_12BPP_444_RED_MASK 0x00000F00
-#define LCD_12BPP_444_GREEN_MASK 0x000000F0
-#define LCD_12BPP_444_BLUE_MASK 0x0000000F
-#define LCD_12BPP_444_RESERVED_MASK 0x0000F000
-
-
-// The enumeration indexes maps the PL111 LcdBpp values used in the LCD Control Register
-typedef enum {
- LCD_BITS_PER_PIXEL_1 = 0,
- LCD_BITS_PER_PIXEL_2,
- LCD_BITS_PER_PIXEL_4,
- LCD_BITS_PER_PIXEL_8,
- LCD_BITS_PER_PIXEL_16_555,
- LCD_BITS_PER_PIXEL_24,
- LCD_BITS_PER_PIXEL_16_565,
- LCD_BITS_PER_PIXEL_12_444
-} LCD_BPP;
-
-
-EFI_STATUS
-LcdPlatformInitializeDisplay (
- IN EFI_HANDLE Handle
- );
-
-EFI_STATUS
-LcdPlatformGetVram (
- OUT EFI_PHYSICAL_ADDRESS* VramBaseAddress,
- OUT UINTN* VramSize
- );
-
-UINT32
-LcdPlatformGetMaxMode (
- VOID
- );
-
-EFI_STATUS
-LcdPlatformSetMode (
- IN UINT32 ModeNumber
- );
-
-EFI_STATUS
-LcdPlatformQueryMode (
- IN UINT32 ModeNumber,
- OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *Info
- );
-
-EFI_STATUS
-LcdPlatformGetTimings (
- IN UINT32 ModeNumber,
- OUT UINT32* HRes,
- OUT UINT32* HSync,
- OUT UINT32* HBackPorch,
- OUT UINT32* HFrontPorch,
- OUT UINT32* VRes,
- OUT UINT32* VSync,
- OUT UINT32* VBackPorch,
- OUT UINT32* VFrontPorch
- );
-
-EFI_STATUS
-LcdPlatformGetBpp (
- IN UINT32 ModeNumber,
- OUT LCD_BPP* Bpp
- );
-
-#endif
--
2.17.1


[PATCH v2 edk2-platforms 0/2] Fix Ecc reported errors from ArmPlatformPkg

PierreGondois
 

From: Pierre Gondois <Pierre.Gondois@...>

The Ecc tools available in the BaseTools package checks for
good practice coding standards. Some errors reported while
running Ecc on edk2/ArmPlatformPkg require modifications
in the edk2-platforms.

This patch set has a dependency over a similar patch set
named "Fix Ecc reported errors in ArmPlatformPkg" for edk2
and should not be merged independently.

The changes can be seen at: https://github.com/PierreARM/edk2-platforms/tree/pg/1537_Ecc_ArmPlatformPkg_v2

Pierre Gondois (2):
Platform/ARM/VExpressPkg: Remove LcdPlatformLib.h
Platform/ARM: Fix Ecc error 8005

.../Library/HdLcdArmJunoLib/HdLcdArmJuno.c | 4 +-
.../Library/HdLcdArmSgiLib/HdLcdArmSgi.c | 4 +-
Platform/ARM/VExpressPkg/ArmVExpressPkg.dec | 3 +-
.../Include/Library/LcdPlatformLib.h | 215 ------------------
.../Library/ArmMaliDpLib/ArmMaliDpLib.c | 4 +-
.../HdLcdArmVExpressLib/HdLcdArmVExpress.c | 4 +-
.../PL111LcdArmVExpress.c | 34 +--
7 files changed, 26 insertions(+), 242 deletions(-)
delete mode 100644 Platform/ARM/VExpressPkg/Include/Library/LcdPlatformLib.h

--
2.17.1


Re: [PATCH] UefiCpuPkg RegisterCpuFeaturesLib: Use AllocatePages() for InitOrder

Ni, Ray
 

Reviewed-by: Ray Ni <ray.ni@...>

-----Original Message-----
From: Zeng, Star <star.zeng@...>
Sent: Wednesday, December 9, 2020 7:28 PM
To: devel@edk2.groups.io
Cc: Zeng, Star <star.zeng@...>; Ni, Ray <ray.ni@...>; Dong, Eric <eric.dong@...>; Laszlo Ersek
<lersek@...>
Subject: [PATCH] UefiCpuPkg RegisterCpuFeaturesLib: Use AllocatePages() for InitOrder

The required buffer size for InitOrder will be 96K when NumberOfCpus=1024.
sizeof (CPU_FEATURES_INIT_ORDER) = 96
NumberOfCpus = 1024 = 1K
sizeof (CPU_FEATURES_INIT_ORDER) * NumberOfCpus = 96K

AllocateZeroPool() will call to PeiServicesAllocatePool() which will use
EFI_HOB_MEMORY_POOL to management memory pool.
EFI_HOB_MEMORY_POOL.Header.HobLength is UINT16 type, so there is no way
for AllocateZeroPool() to allocate > 64K memory.

So AllocateZeroPool() could not be used anymore for the case above or
even bigger required buffer size.

This patch updates the code to use AllocatePages() instead of
AllocateZeroPool() to allocate buffer for InitOrder.

Signed-off-by: Star Zeng <star.zeng@...>
Cc: Ray Ni <ray.ni@...>
Cc: Eric Dong <eric.dong@...>
Cc: Laszlo Ersek <lersek@...>
---
.../Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
index d4a576385f0f..ba052732a86c 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
@@ -126,8 +126,9 @@ CpuInitDataInitialize (

GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors);

- CpuFeaturesData->InitOrder = AllocateZeroPool (sizeof (CPU_FEATURES_INIT_ORDER) * NumberOfCpus);
+ CpuFeaturesData->InitOrder = AllocatePages (EFI_SIZE_TO_PAGES (sizeof (CPU_FEATURES_INIT_ORDER) *
NumberOfCpus));
ASSERT (CpuFeaturesData->InitOrder != NULL);
+ ZeroMem (CpuFeaturesData->InitOrder, sizeof (CPU_FEATURES_INIT_ORDER) * NumberOfCpus);

//
// Collect CPU Features information
--
2.21.0.windows.1


[PATCH 2/2] MdeModulePkg/AtaAtapiPassThru: Add support for drives in RAID mode

Vitaly Cheptsov
 

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

This resolves the problem of using drivers connected to Intel G33
builtin SATA controller when run from DuetPkg when it can only
be configured in RAID mode through the firmware settings.

Cc: Jian J Wang <jian.j.wang@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Mateusz Albecki <mateusz.albecki@...>
Cc: Laszlo Ersek <lersek@...>
Signed-off-by: Vitaly Cheptsov <cheptsov@...>
---
MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c b/M=
deModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
index 86fe9d954f..5892508aef 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
@@ -620,7 +620,7 @@ AtaAtapiPassThruSupported (
return EFI_UNSUPPORTED;
}
=20
- if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData)) {
+ if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) || IS_PCI_RAID =
(&PciData)) {
return EFI_SUCCESS;
}
=20
@@ -1208,6 +1208,12 @@ EnumerateAttachedDevice (
goto Done;
}
break;
+ case PCI_CLASS_MASS_STORAGE_RAID :
+ Instance->AtaPassThruMode.Attributes &=3D ~EFI_ATA_PASS_THRU_ATTRI=
BUTES_PHYSICAL;
+ Instance->ExtScsiPassThruMode.Attributes &=3D ~EFI_EXT_SCSI_PASS_T=
HRU_ATTRIBUTES_PHYSICAL;
+ //
+ // Fall through to AHCI
+ //
case PCI_CLASS_MASS_STORAGE_SATADPA :
//
// The ATA controller is working at AHCI mode
--=20
2.24.3 (Apple Git-128)


[PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

Vitaly Cheptsov
 

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

This resolves the problem of using drivers connected to Intel G33
builtin SATA controller when run from DuetPkg when it can only
be configured in RAID mode through the firmware settings.

Cc: Jian J Wang <jian.j.wang@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Mateusz Albecki <mateusz.albecki@...>
Cc: Laszlo Ersek <lersek@...>
Signed-off-by: Vitaly Cheptsov <cheptsov@...>
---
MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c b/Md=
eModulePkg/Bus/Pci/SataControllerDxe/SataController.c
index ab06e2833c..301335c967 100644
--- a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
+++ b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
@@ -324,7 +324,7 @@ SataControllerSupported (
return EFI_UNSUPPORTED;
}
=20
- if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData)) {
+ if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) || IS_PCI_RAID =
(&PciData)) {
return EFI_SUCCESS;
}
=20
@@ -465,7 +465,7 @@ SataControllerStart (
if (IS_PCI_IDE (&PciData)) {
Private->IdeInit.ChannelCount =3D IDE_MAX_CHANNEL;
Private->DeviceCount =3D IDE_MAX_DEVICES;
- } else if (IS_PCI_SATADPA (&PciData)) {
+ } else if (IS_PCI_SATADPA (&PciData) || IS_PCI_RAID (&PciData)) {
//
// Read Ports Implemented(PI) to calculate max port number (0 based)=
.
//
--=20
2.24.3 (Apple Git-128)


EDK II CI Working - Resume normal activities

Michael D Kinney
 

 


Re: EDK II CI failing - Do not submit PRs with "push" label set

Michael D Kinney
 

The issue is resolved.

PRs are being processed correctly now.

Mike

-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@...>
Sent: Thursday, December 10, 2020 10:23 PM
To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@...>; bret.barkelew@...
Subject: EDK II CI failing - Do not submit PRs with "push" label set

Hello,

The repo for the cmocka submodule is not available right now, and this is failing
CI checks that run unit tests. Pre-commit checks that do not have any unit tests
will pass, but the post commit checks that run everything will always fail.

PROGRESS - ## Checking Git repository: UnitTestFrameworkPkg/Library/CmockaLib/cmocka...
ERROR - Failed to fetch UnitTestFrameworkPkg/Library/CmockaLib/cmocka
ERROR - FAILED!
ERROR - Failed to fetch required repository!
ERROR - Unable to checkout repo due to error

This repo access error to the cmocka has occurred one other time in the past. Bret had
suggested the idea of using a different repo for cmocka, perhaps a fork in GitHub,
to avoid this issue.

We are hoping the issue will be resolved very soon. but until then, please do not
submit any PRs with a “push” label set.

Best regards,

Mike


Re: [Patch v4 0/2] MdeModulePkg/Variable/RuntimeDxe: Restore Variable Lock Protocol behavior

Wu, Hao A
 

For the series:
Reviewed-by: Hao A Wu <hao.a.wu@...>

Best Regards,
Hao Wu

-----Original Message-----
From: Michael D Kinney <michael.d.kinney@...>
Sent: Friday, December 11, 2020 4:01 PM
To: devel@edk2.groups.io
Cc: Wu, Hao A <hao.a.wu@...>; Liming Gao
<gaoliming@...>; Bret Barkelew
<Bret.Barkelew@...>
Subject: [Patch v4 0/2] MdeModulePkg/Variable/RuntimeDxe: Restore
Variable Lock Protocol behavior

New in V4
==========
* Fix spelling in unit tests
* Call ValidateSetVariable() with DataSize=0, Attributes=0

New in V3
==========
* Split into 2 patches. One for code change. Second for unit tests.
* Remove duplicate code and use ValidateSetVariable() to detect if a
variable is already locked.

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

The VariableLock shim currently fails if called twice because the underlying
Variable Policy engine returns an error if a policy is set on an existing variable.

This breaks existing code which expect it to silently pass if a variable is locked
multiple times (because it should "be locked").

Refactor the shim to confirm that the variable is indeed locked and then
change the error to EFI_SUCCESS and generate a DEBUG_ERROR message so
the duplicate lock can be reported in a debug log and removed.

Add host based unit tests for the multiple lock case using Variable Lock
Protocol, Variable Policy Protocol, and mixes of Variable Lock Protocol and
Variable Policy Protocol.

Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Liming Gao <gaoliming@...>
Signed-off-by: Bret Barkelew <Bret.Barkelew@...>

Bret Barkelew (1):
MdeModulePkg/Variable/RuntimeDxe: Restore Variable Lock Protocol
behavior

Michael D Kinney (1):
MdeModulePkg/Variable/RuntimeDxe: Add Variable Lock Protocol Unit
Tests

MdeModulePkg/Test/MdeModulePkgHostTest.dsc | 11 +
.../VariableLockRequestToLockUnitTest.c | 565 ++++++++++++++++++
.../VariableLockRequestToLockUnitTest.inf | 36 ++
.../RuntimeDxe/VariableLockRequestToLock.c | 95 +--
4 files changed, 671 insertions(+), 36 deletions(-) create mode 100644
MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/Vari
ableLockRequestToLockUnitTest.c
create mode 100644
MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/Vari
ableLockRequestToLockUnitTest.inf

--
2.29.2.windows.2


[Patch v4 2/2] MdeModulePkg/Variable/RuntimeDxe: Add Variable Lock Protocol Unit Tests

Michael D Kinney
 

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

Add host based unit tests for the multiple lock case using Variable Lock
Protocol, Variable Policy Protocol, and mixes of Variable Lock Protocol
and Variable Policy Protocol.

Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Liming Gao <gaoliming@...>
Signed-off-by: Bret Barkelew <Bret.Barkelew@...>
---
MdeModulePkg/Test/MdeModulePkgHostTest.dsc | 11 +
.../VariableLockRequestToLockUnitTest.c | 565 ++++++++++++++++++
.../VariableLockRequestToLockUnitTest.inf | 36 ++
3 files changed, 612 insertions(+)
create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/VariableLockRequestToLockUnitTest.c
create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/VariableLockRequestToLockUnitTest.inf

diff --git a/MdeModulePkg/Test/MdeModulePkgHostTest.dsc b/MdeModulePkg/Test/MdeModulePkgHostTest.dsc
index 72a119db4568..4da4692c8451 100644
--- a/MdeModulePkg/Test/MdeModulePkgHostTest.dsc
+++ b/MdeModulePkg/Test/MdeModulePkgHostTest.dsc
@@ -19,6 +19,9 @@ [Defines]

!include UnitTestFrameworkPkg/UnitTestFrameworkPkgHost.dsc.inc

+[LibraryClasses]
+ SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
+
[Components]
MdeModulePkg/Library/DxeResetSystemLib/UnitTest/MockUefiRuntimeServicesTableLib.inf

@@ -30,3 +33,11 @@ [Components]
ResetSystemLib|MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.inf
UefiRuntimeServicesTableLib|MdeModulePkg/Library/DxeResetSystemLib/UnitTest/MockUefiRuntimeServicesTableLib.inf
}
+
+ MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/VariableLockRequestToLockUnitTest.inf {
+ <LibraryClasses>
+ VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf
+ VariablePolicyHelperLib|MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.inf
+ <PcdsFixedAtBuild>
+ gEfiMdeModulePkgTokenSpaceGuid.PcdAllowVariablePolicyEnforcementDisable|TRUE
+ }
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/VariableLockRequestToLockUnitTest.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/VariableLockRequestToLockUnitTest.c
new file mode 100644
index 000000000000..44d70e639d77
--- /dev/null
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/VariableLockRequestToLockUnitTest.c
@@ -0,0 +1,565 @@
+/** @file
+ This is a host-based unit test for the VariableLockRequestToLock shim.
+
+ Copyright (c) Microsoft Corporation.
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <stdio.h>
+#include <string.h>
+#include <stdarg.h>
+#include <stddef.h>
+#include <setjmp.h>
+#include <cmocka.h>
+
+#include <Uefi.h>
+#include <Library/DebugLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/UnitTestLib.h>
+#include <Library/VariablePolicyLib.h>
+#include <Library/VariablePolicyHelperLib.h>
+
+#include <Protocol/VariableLock.h>
+
+#define UNIT_TEST_NAME "VarPol/VarLock Shim Unit Test"
+#define UNIT_TEST_VERSION "1.0"
+
+///=== CODE UNDER TEST ===========================================================================
+
+EFI_STATUS
+EFIAPI
+VariableLockRequestToLock (
+ IN CONST EDKII_VARIABLE_LOCK_PROTOCOL *This,
+ IN CHAR16 *VariableName,
+ IN EFI_GUID *VendorGuid
+ );
+
+///=== TEST DATA ==================================================================================
+
+//
+// Test GUID 1 {F955BA2D-4A2C-480C-BFD1-3CC522610592}
+//
+EFI_GUID mTestGuid1 = {
+ 0xf955ba2d, 0x4a2c, 0x480c, {0xbf, 0xd1, 0x3c, 0xc5, 0x22, 0x61, 0x5, 0x92}
+};
+
+//
+// Test GUID 2 {2DEA799E-5E73-43B9-870E-C945CE82AF3A}
+//
+EFI_GUID mTestGuid2 = {
+ 0x2dea799e, 0x5e73, 0x43b9, {0x87, 0xe, 0xc9, 0x45, 0xce, 0x82, 0xaf, 0x3a}
+};
+
+//
+// Test GUID 3 {698A2BFD-A616-482D-B88C-7100BD6682A9}
+//
+EFI_GUID mTestGuid3 = {
+ 0x698a2bfd, 0xa616, 0x482d, {0xb8, 0x8c, 0x71, 0x0, 0xbd, 0x66, 0x82, 0xa9}
+};
+
+#define TEST_VAR_1_NAME L"TestVar1"
+#define TEST_VAR_2_NAME L"TestVar2"
+#define TEST_VAR_3_NAME L"TestVar3"
+
+#define TEST_POLICY_ATTRIBUTES_NULL 0
+#define TEST_POLICY_MIN_SIZE_NULL 0
+#define TEST_POLICY_MAX_SIZE_NULL MAX_UINT32
+
+#define TEST_POLICY_MIN_SIZE_10 10
+#define TEST_POLICY_MAX_SIZE_200 200
+
+///=== HELPER FUNCTIONS ===========================================================================
+
+/**
+ Mocked version of GetVariable, for testing.
+
+ @param VariableName
+ @param VendorGuid
+ @param Attributes
+ @param DataSize
+ @param Data
+**/
+EFI_STATUS
+EFIAPI
+StubGetVariableNull (
+ IN CHAR16 *VariableName,
+ IN EFI_GUID *VendorGuid,
+ OUT UINT32 *Attributes, OPTIONAL
+ IN OUT UINTN *DataSize,
+ OUT VOID *Data OPTIONAL
+ )
+{
+ UINT32 MockedAttr;
+ UINTN MockedDataSize;
+ VOID *MockedData;
+ EFI_STATUS MockedReturn;
+
+ check_expected_ptr (VariableName);
+ check_expected_ptr (VendorGuid);
+ check_expected_ptr (DataSize);
+
+ MockedAttr = (UINT32)mock();
+ MockedDataSize = (UINTN)mock();
+ MockedData = (VOID*)(UINTN)mock();
+ MockedReturn = (EFI_STATUS)mock();
+
+ if (Attributes != NULL) {
+ *Attributes = MockedAttr;
+ }
+ if (Data != NULL && !EFI_ERROR (MockedReturn)) {
+ CopyMem (Data, MockedData, MockedDataSize);
+ }
+
+ *DataSize = MockedDataSize;
+
+ return MockedReturn;
+}
+
+//
+// Anything you think might be helpful that isn't a test itself.
+//
+
+/**
+ This is a common setup function that will ensure the library is always
+ initialized with the stubbed GetVariable.
+
+ Not used by all test cases, but by most.
+
+ @param[in] Context Unit test case context
+**/
+STATIC
+UNIT_TEST_STATUS
+EFIAPI
+LibInitMocked (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ return EFI_ERROR (InitVariablePolicyLib (StubGetVariableNull)) ? UNIT_TEST_ERROR_PREREQUISITE_NOT_MET : UNIT_TEST_PASSED;
+}
+
+/**
+ Common cleanup function to make sure that the library is always de-initialized
+ prior to the next test case.
+
+ @param[in] Context Unit test case context
+**/
+STATIC
+VOID
+EFIAPI
+LibCleanup (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ DeinitVariablePolicyLib();
+}
+
+///=== TEST CASES =================================================================================
+
+///===== SHIM SUITE ===========================================================
+
+/**
+ Test Case that locks a single variable using the Variable Lock Protocol.
+ The call is expected to succeed.
+
+ @param[in] Context Unit test case context
+**/
+UNIT_TEST_STATUS
+EFIAPI
+LockingWithoutAnyPoliciesShouldSucceed (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ EFI_STATUS Status;
+
+ Status = VariableLockRequestToLock (NULL, TEST_VAR_1_NAME, &mTestGuid1);
+ UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ return UNIT_TEST_PASSED;
+}
+
+/**
+ Test Case that locks the same variable twice using the Variable Lock Protocol.
+ Both calls are expected to succeed.
+
+ @param[in] Context Unit test case context
+ **/
+UNIT_TEST_STATUS
+EFIAPI
+LockingTwiceShouldSucceed (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ EFI_STATUS Status;
+
+ Status = VariableLockRequestToLock (NULL, TEST_VAR_1_NAME, &mTestGuid1);
+ UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ Status = VariableLockRequestToLock (NULL, TEST_VAR_1_NAME, &mTestGuid1);
+ UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ return UNIT_TEST_PASSED;
+}
+
+/**
+ Test Case that locks a variable using the Variable Policy Protocol then locks
+ the same variable using the Variable Lock Protocol.
+ Both calls are expected to succeed.
+
+ @param[in] Context Unit test case context
+ **/
+UNIT_TEST_STATUS
+EFIAPI
+LockingALockedVariableShouldSucceed (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ EFI_STATUS Status;
+ VARIABLE_POLICY_ENTRY *NewEntry;
+
+ //
+ // Create a variable policy that locks the variable.
+ //
+ Status = CreateBasicVariablePolicy (
+ &mTestGuid1,
+ TEST_VAR_1_NAME,
+ TEST_POLICY_MIN_SIZE_NULL,
+ TEST_POLICY_MAX_SIZE_200,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ VARIABLE_POLICY_TYPE_LOCK_NOW,
+ &NewEntry
+ );
+ UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ //
+ // Register the new policy.
+ //
+ Status = RegisterVariablePolicy (NewEntry);
+
+ Status = VariableLockRequestToLock (NULL, TEST_VAR_1_NAME, &mTestGuid1);
+ UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ FreePool (NewEntry);
+
+ return UNIT_TEST_PASSED;
+}
+
+/**
+ Test Case that locks a variable using the Variable Policy Protocol with a
+ policy other than LOCK_NOW then attempts to lock the same variable using the
+ Variable Lock Protocol. The call to Variable Policy is expected to succeed
+ and the call to Variable Lock is expected to fail.
+
+ @param[in] Context Unit test case context
+ **/
+UNIT_TEST_STATUS
+EFIAPI
+LockingAnUnlockedVariableShouldFail (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ EFI_STATUS Status;
+ VARIABLE_POLICY_ENTRY *NewEntry;
+
+ // Create a variable policy that locks the variable.
+ Status = CreateVarStateVariablePolicy (&mTestGuid1,
+ TEST_VAR_1_NAME,
+ TEST_POLICY_MIN_SIZE_NULL,
+ TEST_POLICY_MAX_SIZE_200,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ &mTestGuid2,
+ 1,
+ TEST_VAR_2_NAME,
+ &NewEntry);
+ UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ // Register the new policy.
+ Status = RegisterVariablePolicy (NewEntry);
+
+ // Configure the stub to not care about parameters. We're testing errors.
+ expect_any_always( StubGetVariableNull, VariableName );
+ expect_any_always( StubGetVariableNull, VendorGuid );
+ expect_any_always( StubGetVariableNull, DataSize );
+
+ // With a policy, make sure that writes still work, since the variable doesn't exist.
+ will_return( StubGetVariableNull, TEST_POLICY_ATTRIBUTES_NULL ); // Attributes
+ will_return( StubGetVariableNull, 0 ); // Size
+ will_return( StubGetVariableNull, NULL ); // DataPtr
+ will_return( StubGetVariableNull, EFI_NOT_FOUND); // Status
+
+ Status = VariableLockRequestToLock (NULL, TEST_VAR_1_NAME, &mTestGuid1);
+ UT_ASSERT_TRUE (EFI_ERROR (Status));
+
+ FreePool (NewEntry);
+
+ return UNIT_TEST_PASSED;
+}
+
+/**
+ Test Case that locks a variable using the Variable Policy Protocol with a
+ policy other than LOCK_NOW, but is currently locked. Then attempts to lock
+ the same variable using the Variable Lock Protocol. The call to Variable
+ Policy is expected to succeed and the call to Variable Lock also expected to
+ succeed.
+
+ @param[in] Context Unit test case context
+ **/
+UNIT_TEST_STATUS
+EFIAPI
+LockingALockedVariableWithMatchingDataShouldSucceed (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ EFI_STATUS Status;
+ VARIABLE_POLICY_ENTRY *NewEntry;
+ UINT8 Data;
+
+ // Create a variable policy that locks the variable.
+ Status = CreateVarStateVariablePolicy (&mTestGuid1,
+ TEST_VAR_1_NAME,
+ TEST_POLICY_MIN_SIZE_NULL,
+ TEST_POLICY_MAX_SIZE_200,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ &mTestGuid2,
+ 1,
+ TEST_VAR_2_NAME,
+ &NewEntry);
+ UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ // Register the new policy.
+ Status = RegisterVariablePolicy (NewEntry);
+
+ // Configure the stub to not care about parameters. We're testing errors.
+ expect_any_always( StubGetVariableNull, VariableName );
+ expect_any_always( StubGetVariableNull, VendorGuid );
+ expect_any_always( StubGetVariableNull, DataSize );
+
+ // With a policy, make sure that writes still work, since the variable doesn't exist.
+ Data = 1;
+ will_return( StubGetVariableNull, TEST_POLICY_ATTRIBUTES_NULL ); // Attributes
+ will_return( StubGetVariableNull, sizeof (Data) ); // Size
+ will_return( StubGetVariableNull, &Data ); // DataPtr
+ will_return( StubGetVariableNull, EFI_SUCCESS); // Status
+
+ Status = VariableLockRequestToLock (NULL, TEST_VAR_1_NAME, &mTestGuid1);
+ UT_ASSERT_TRUE (!EFI_ERROR (Status));
+
+ FreePool (NewEntry);
+
+ return UNIT_TEST_PASSED;
+}
+
+/**
+ Test Case that locks a variable using the Variable Policy Protocol with a
+ policy other than LOCK_NOW, but variable data does not match. Then attempts
+ to lock the same variable using the Variable Lock Protocol. The call to
+ Variable Policy is expected to succeed and the call to Variable Lock is
+ expected to fail.
+
+ @param[in] Context Unit test case context
+ **/
+UNIT_TEST_STATUS
+EFIAPI
+LockingALockedVariableWithNonMatchingDataShouldFail (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ EFI_STATUS Status;
+ VARIABLE_POLICY_ENTRY *NewEntry;
+ UINT8 Data;
+
+ // Create a variable policy that locks the variable.
+ Status = CreateVarStateVariablePolicy (&mTestGuid1,
+ TEST_VAR_1_NAME,
+ TEST_POLICY_MIN_SIZE_NULL,
+ TEST_POLICY_MAX_SIZE_200,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ &mTestGuid2,
+ 1,
+ TEST_VAR_2_NAME,
+ &NewEntry);
+ UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ // Register the new policy.
+ Status = RegisterVariablePolicy (NewEntry);
+
+ // Configure the stub to not care about parameters. We're testing errors.
+ expect_any_always( StubGetVariableNull, VariableName );
+ expect_any_always( StubGetVariableNull, VendorGuid );
+ expect_any_always( StubGetVariableNull, DataSize );
+
+ // With a policy, make sure that writes still work, since the variable doesn't exist.
+ Data = 2;
+ will_return( StubGetVariableNull, TEST_POLICY_ATTRIBUTES_NULL ); // Attributes
+ will_return( StubGetVariableNull, sizeof (Data) ); // Size
+ will_return( StubGetVariableNull, &Data ); // DataPtr
+ will_return( StubGetVariableNull, EFI_SUCCESS); // Status
+
+ Status = VariableLockRequestToLock (NULL, TEST_VAR_1_NAME, &mTestGuid1);
+ UT_ASSERT_TRUE (EFI_ERROR (Status));
+
+ FreePool (NewEntry);
+
+ return UNIT_TEST_PASSED;
+}
+
+/**
+ Test Case that locks a variable using Variable Lock Protocol Policy Protocol
+ then and then attempts to lock the same variable using the Variable Policy
+ Protocol. The call to Variable Lock is expected to succeed and the call to
+ Variable Policy is expected to fail.
+
+ @param[in] Context Unit test case context
+ **/
+UNIT_TEST_STATUS
+EFIAPI
+SettingPolicyForALockedVariableShouldFail (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ EFI_STATUS Status;
+ VARIABLE_POLICY_ENTRY *NewEntry;
+
+ // Lock the variable.
+ Status = VariableLockRequestToLock (NULL, TEST_VAR_1_NAME, &mTestGuid1);
+ UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ // Create a variable policy that locks the variable.
+ Status = CreateVarStateVariablePolicy (&mTestGuid1,
+ TEST_VAR_1_NAME,
+ TEST_POLICY_MIN_SIZE_NULL,
+ TEST_POLICY_MAX_SIZE_200,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ &mTestGuid2,
+ 1,
+ TEST_VAR_2_NAME,
+ &NewEntry);
+ UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ // Register the new policy.
+ Status = RegisterVariablePolicy (NewEntry);
+ UT_ASSERT_TRUE (EFI_ERROR (Status));
+
+ FreePool (NewEntry);
+
+ return UNIT_TEST_PASSED;
+}
+
+/**
+ Main entry point to this unit test application.
+
+ Sets up and runs the test suites.
+**/
+VOID
+EFIAPI
+UnitTestMain (
+ VOID
+ )
+{
+ EFI_STATUS Status;
+ UNIT_TEST_FRAMEWORK_HANDLE Framework;
+ UNIT_TEST_SUITE_HANDLE ShimTests;
+
+ Framework = NULL;
+
+ DEBUG ((DEBUG_INFO, "%a v%a\n", UNIT_TEST_NAME, UNIT_TEST_VERSION));
+
+ //
+ // Start setting up the test framework for running the tests.
+ //
+ Status = InitUnitTestFramework (&Framework, UNIT_TEST_NAME, gEfiCallerBaseName, UNIT_TEST_VERSION);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Failed in InitUnitTestFramework. Status = %r\n", Status));
+ goto EXIT;
+ }
+
+ //
+ // Add all test suites and tests.
+ //
+ Status = CreateUnitTestSuite (
+ &ShimTests, Framework,
+ "Variable Lock Shim Tests", "VarPolicy.VarLockShim", NULL, NULL
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Failed in CreateUnitTestSuite for ShimTests\n"));
+ Status = EFI_OUT_OF_RESOURCES;
+ goto EXIT;
+ }
+ AddTestCase (
+ ShimTests,
+ "Locking a variable with no matching policies should always work", "EmptyPolicies",
+ LockingWithoutAnyPoliciesShouldSucceed, LibInitMocked, LibCleanup, NULL
+ );
+ AddTestCase (
+ ShimTests,
+ "Locking a variable twice should always work", "DoubleLock",
+ LockingTwiceShouldSucceed, LibInitMocked, LibCleanup, NULL
+ );
+ AddTestCase (
+ ShimTests,
+ "Locking a variable that's already locked by another policy should work", "LockAfterPolicy",
+ LockingALockedVariableShouldSucceed, LibInitMocked, LibCleanup, NULL
+ );
+ AddTestCase (
+ ShimTests,
+ "Locking a variable that already has an unlocked policy should fail", "LockAfterUnlockedPolicy",
+ LockingAnUnlockedVariableShouldFail, LibInitMocked, LibCleanup, NULL
+ );
+ AddTestCase (
+ ShimTests,
+ "Locking a variable that already has an locked policy should succeed", "LockAfterLockedPolicyMatchingData",
+ LockingALockedVariableWithMatchingDataShouldSucceed, LibInitMocked, LibCleanup, NULL
+ );
+ AddTestCase (
+ ShimTests,
+ "Locking a variable that already has an locked policy with matching data should succeed", "LockAfterLockedPolicyNonMatchingData",
+ LockingALockedVariableWithNonMatchingDataShouldFail, LibInitMocked, LibCleanup, NULL
+ );
+ AddTestCase (
+ ShimTests,
+ "Adding a policy for a variable that has previously been locked should always fail", "SetPolicyAfterLock",
+ SettingPolicyForALockedVariableShouldFail, LibInitMocked, LibCleanup, NULL
+ );
+
+ //
+ // Execute the tests.
+ //
+ Status = RunAllTestSuites (Framework);
+
+EXIT:
+ if (Framework != NULL) {
+ FreeUnitTestFramework (Framework);
+ }
+
+ return;
+}
+
+///
+/// Avoid ECC error for function name that starts with lower case letter
+///
+#define Main main
+
+/**
+ Standard POSIX C entry point for host based unit test execution.
+
+ @param[in] Argc Number of arguments
+ @param[in] Argv Array of pointers to arguments
+
+ @retval 0 Success
+ @retval other Error
+**/
+INT32
+Main (
+ IN INT32 Argc,
+ IN CHAR8 *Argv[]
+ )
+{
+ UnitTestMain ();
+ return 0;
+}
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/VariableLockRequestToLockUnitTest.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/VariableLockRequestToLockUnitTest.inf
new file mode 100644
index 000000000000..2a659d7e1370
--- /dev/null
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/VariableLockRequestToLockUnitTest.inf
@@ -0,0 +1,36 @@
+## @file
+# This is a host-based unit test for the VariableLockRequestToLock shim.
+#
+# Copyright (c) Microsoft Corporation.
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+##
+
+[Defines]
+ INF_VERSION = 0x00010017
+ BASE_NAME = VariableLockRequestToLockUnitTest
+ FILE_GUID = A7388B6C-7274-4717-9649-BDC5DFD1FCBE
+ VERSION_STRING = 1.0
+ MODULE_TYPE = HOST_APPLICATION
+
+#
+# The following information is for reference only and not required by the build tools.
+#
+# VALID_ARCHITECTURES = IA32 X64 ARM AARCH64
+#
+
+[Sources]
+ VariableLockRequestToLockUnitTest.c
+ ../VariableLockRequestToLock.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ UnitTestFrameworkPkg/UnitTestFrameworkPkg.dec
+
+[LibraryClasses]
+ UnitTestLib
+ DebugLib
+ VariablePolicyLib
+ VariablePolicyHelperLib
+ BaseMemoryLib
+ MemoryAllocationLib
--
2.29.2.windows.2


[Patch v4 1/2] MdeModulePkg/Variable/RuntimeDxe: Restore Variable Lock Protocol behavior

Michael D Kinney
 

From: Bret Barkelew <bret.barkelew@...>

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

The VariableLock shim currently fails if called twice because the
underlying Variable Policy engine returns an error if a policy is set
on an existing variable.

This breaks existing code which expect it to silently pass if a variable
is locked multiple times (because it should "be locked").

Refactor the shim to confirm that the variable is indeed locked and then
change the error to EFI_SUCCESS and generate a DEBUG_ERROR message so
the duplicate lock can be reported in a debug log and removed.

Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Liming Gao <gaoliming@...>
Signed-off-by: Bret Barkelew <Bret.Barkelew@...>
---
.../RuntimeDxe/VariableLockRequestToLock.c | 95 ++++++++++++-------
1 file changed, 59 insertions(+), 36 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToLock.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToLock.c
index 4aa854aaf260..7d87e50efdcd 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToLock.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToLock.c
@@ -1,67 +1,90 @@
-/** @file -- VariableLockRequestToLock.c
-Temporary location of the RequestToLock shim code while
-projects are moved to VariablePolicy. Should be removed when deprecated.
+/** @file
+ Temporary location of the RequestToLock shim code while projects
+ are moved to VariablePolicy. Should be removed when deprecated.

-Copyright (c) Microsoft Corporation.
-SPDX-License-Identifier: BSD-2-Clause-Patent
+ Copyright (c) Microsoft Corporation.
+ SPDX-License-Identifier: BSD-2-Clause-Patent

**/

#include <Uefi.h>
-
#include <Library/DebugLib.h>
#include <Library/MemoryAllocationLib.h>
-
-#include <Protocol/VariableLock.h>
-
-#include <Protocol/VariablePolicy.h>
#include <Library/VariablePolicyLib.h>
#include <Library/VariablePolicyHelperLib.h>
-
+#include <Protocol/VariableLock.h>

/**
DEPRECATED. THIS IS ONLY HERE AS A CONVENIENCE WHILE PORTING.
- Mark a variable that will become read-only after leaving the DXE phase of execution.
- Write request coming from SMM environment through EFI_SMM_VARIABLE_PROTOCOL is allowed.
+ Mark a variable that will become read-only after leaving the DXE phase of
+ execution. Write request coming from SMM environment through
+ EFI_SMM_VARIABLE_PROTOCOL is allowed.

@param[in] This The VARIABLE_LOCK_PROTOCOL instance.
- @param[in] VariableName A pointer to the variable name that will be made read-only subsequently.
- @param[in] VendorGuid A pointer to the vendor GUID that will be made read-only subsequently.
+ @param[in] VariableName A pointer to the variable name that will be made
+ read-only subsequently.
+ @param[in] VendorGuid A pointer to the vendor GUID that will be made
+ read-only subsequently.

- @retval EFI_SUCCESS The variable specified by the VariableName and the VendorGuid was marked
- as pending to be read-only.
+ @retval EFI_SUCCESS The variable specified by the VariableName and
+ the VendorGuid was marked as pending to be
+ read-only.
@retval EFI_INVALID_PARAMETER VariableName or VendorGuid is NULL.
Or VariableName is an empty string.
- @retval EFI_ACCESS_DENIED EFI_END_OF_DXE_EVENT_GROUP_GUID or EFI_EVENT_GROUP_READY_TO_BOOT has
- already been signaled.
- @retval EFI_OUT_OF_RESOURCES There is not enough resource to hold the lock request.
+ @retval EFI_ACCESS_DENIED EFI_END_OF_DXE_EVENT_GROUP_GUID or
+ EFI_EVENT_GROUP_READY_TO_BOOT has already been
+ signaled.
+ @retval EFI_OUT_OF_RESOURCES There is not enough resource to hold the lock
+ request.
**/
EFI_STATUS
EFIAPI
VariableLockRequestToLock (
- IN CONST EDKII_VARIABLE_LOCK_PROTOCOL *This,
- IN CHAR16 *VariableName,
- IN EFI_GUID *VendorGuid
+ IN CONST EDKII_VARIABLE_LOCK_PROTOCOL *This,
+ IN CHAR16 *VariableName,
+ IN EFI_GUID *VendorGuid
)
{
- EFI_STATUS Status;
- VARIABLE_POLICY_ENTRY *NewPolicy;
+ EFI_STATUS Status;
+ VARIABLE_POLICY_ENTRY *NewPolicy;
+
+ DEBUG ((DEBUG_ERROR, "!!! DEPRECATED INTERFACE !!! %a() will go away soon!\n", __FUNCTION__));
+ DEBUG ((DEBUG_ERROR, "!!! DEPRECATED INTERFACE !!! Please move to use Variable Policy!\n"));
+ DEBUG ((DEBUG_ERROR, "!!! DEPRECATED INTERFACE !!! Variable: %g %s\n", VendorGuid, VariableName));

NewPolicy = NULL;
- Status = CreateBasicVariablePolicy( VendorGuid,
- VariableName,
- VARIABLE_POLICY_NO_MIN_SIZE,
- VARIABLE_POLICY_NO_MAX_SIZE,
- VARIABLE_POLICY_NO_MUST_ATTR,
- VARIABLE_POLICY_NO_CANT_ATTR,
- VARIABLE_POLICY_TYPE_LOCK_NOW,
- &NewPolicy );
+ Status = CreateBasicVariablePolicy(
+ VendorGuid,
+ VariableName,
+ VARIABLE_POLICY_NO_MIN_SIZE,
+ VARIABLE_POLICY_NO_MAX_SIZE,
+ VARIABLE_POLICY_NO_MUST_ATTR,
+ VARIABLE_POLICY_NO_CANT_ATTR,
+ VARIABLE_POLICY_TYPE_LOCK_NOW,
+ &NewPolicy
+ );
if (!EFI_ERROR( Status )) {
- Status = RegisterVariablePolicy( NewPolicy );
+ Status = RegisterVariablePolicy (NewPolicy);
+
+ //
+ // If the error returned is EFI_ALREADY_STARTED, we need to check the
+ // current database for the variable and see whether it's locked. If it's
+ // locked, we're still fine, but also generate a DEBUG_ERROR message so the
+ // duplicate lock can be removed.
+ //
+ if (Status == EFI_ALREADY_STARTED) {
+ Status = ValidateSetVariable (VariableName, VendorGuid, 0, 0, NULL);
+ if (Status == EFI_WRITE_PROTECTED) {
+ DEBUG ((DEBUG_ERROR, " Variable: %g %s is already locked!\n", VendorGuid, VariableName));
+ Status = EFI_SUCCESS;
+ } else {
+ DEBUG ((DEBUG_ERROR, " Variable: %g %s can not be locked!\n", VendorGuid, VariableName));
+ Status = EFI_ACCESS_DENIED;
+ }
+ }
}
- if (EFI_ERROR( Status )) {
+ if (EFI_ERROR (Status)) {
DEBUG(( DEBUG_ERROR, "%a - Failed to lock variable %s! %r\n", __FUNCTION__, VariableName, Status ));
- ASSERT_EFI_ERROR( Status );
}
if (NewPolicy != NULL) {
FreePool( NewPolicy );
--
2.29.2.windows.2


[Patch v4 0/2] MdeModulePkg/Variable/RuntimeDxe: Restore Variable Lock Protocol behavior

Michael D Kinney
 

New in V4
==========
* Fix spelling in unit tests
* Call ValidateSetVariable() with DataSize=0, Attributes=0

New in V3
==========
* Split into 2 patches. One for code change. Second for unit tests.
* Remove duplicate code and use ValidateSetVariable() to detect if a
variable is already locked.

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

The VariableLock shim currently fails if called twice because the
underlying Variable Policy engine returns an error if a policy is set
on an existing variable.

This breaks existing code which expect it to silently pass if a variable
is locked multiple times (because it should "be locked").

Refactor the shim to confirm that the variable is indeed locked and then
change the error to EFI_SUCCESS and generate a DEBUG_ERROR message so
the duplicate lock can be reported in a debug log and removed.

Add host based unit tests for the multiple lock case using Variable Lock
Protocol, Variable Policy Protocol, and mixes of Variable Lock Protocol
and Variable Policy Protocol.

Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Liming Gao <gaoliming@...>
Signed-off-by: Bret Barkelew <Bret.Barkelew@...>

Bret Barkelew (1):
MdeModulePkg/Variable/RuntimeDxe: Restore Variable Lock Protocol
behavior

Michael D Kinney (1):
MdeModulePkg/Variable/RuntimeDxe: Add Variable Lock Protocol Unit
Tests

MdeModulePkg/Test/MdeModulePkgHostTest.dsc | 11 +
.../VariableLockRequestToLockUnitTest.c | 565 ++++++++++++++++++
.../VariableLockRequestToLockUnitTest.inf | 36 ++
.../RuntimeDxe/VariableLockRequestToLock.c | 95 +--
4 files changed, 671 insertions(+), 36 deletions(-)
create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/VariableLockRequestToLockUnitTest.c
create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/VariableLockRequestToLockUnitTest.inf

--
2.29.2.windows.2


Re: [Patch v3 1/2] MdeModulePkg/Variable/RuntimeDxe: Restore Variable Lock Protocol behavior

Michael D Kinney
 

-----Original Message-----
From: Wu, Hao A <hao.a.wu@...>
Sent: Thursday, December 10, 2020 9:40 PM
To: Kinney, Michael D <michael.d.kinney@...>; devel@edk2.groups.io
Cc: Bret Barkelew <bret.barkelew@...>; Liming Gao <gaoliming@...>; Bret Barkelew
<Bret.Barkelew@...>
Subject: RE: [Patch v3 1/2] MdeModulePkg/Variable/RuntimeDxe: Restore Variable Lock Protocol behavior

-----Original Message-----
From: Michael D Kinney <michael.d.kinney@...>
Sent: Friday, December 11, 2020 12:52 PM
To: devel@edk2.groups.io
Cc: Bret Barkelew <bret.barkelew@...>; Wu, Hao A
<hao.a.wu@...>; Liming Gao <gaoliming@...>; Bret
Barkelew <Bret.Barkelew@...>
Subject: [Patch v3 1/2] MdeModulePkg/Variable/RuntimeDxe: Restore
Variable Lock Protocol behavior

From: Bret Barkelew <bret.barkelew@...>

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

The VariableLock shim currently fails if called twice because the underlying
Variable Policy engine returns an error if a policy is set on an existing variable.

This breaks existing code which expect it to silently pass if a variable is locked
multiple times (because it should "be locked").

Refactor the shim to confirm that the variable is indeed locked and then
change the error to EFI_SUCCESS and generate a DEBUG_ERROR message so
the duplicate lock can be reported in a debug log and removed.

Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Liming Gao <gaoliming@...>
Signed-off-by: Bret Barkelew <Bret.Barkelew@...>
---
.../RuntimeDxe/VariableLockRequestToLock.c | 95 ++++++++++++-------
1 file changed, 59 insertions(+), 36 deletions(-)

diff --git
a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToL
ock.c
b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToL
ock.c
index 4aa854aaf260..0715b527a0f7 100644
---
a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToL
ock.c
+++
b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToL
o
+++ ck.c
@@ -1,67 +1,90 @@
-/** @file -- VariableLockRequestToLock.c -Temporary location of the
RequestToLock shim code while -projects are moved to VariablePolicy.
Should be removed when deprecated.
+/** @file
+ Temporary location of the RequestToLock shim code while projects
+ are moved to VariablePolicy. Should be removed when deprecated.

-Copyright (c) Microsoft Corporation.
-SPDX-License-Identifier: BSD-2-Clause-Patent
+ Copyright (c) Microsoft Corporation.
+ SPDX-License-Identifier: BSD-2-Clause-Patent

**/

#include <Uefi.h>
-
#include <Library/DebugLib.h>
#include <Library/MemoryAllocationLib.h>
-
-#include <Protocol/VariableLock.h>
-
-#include <Protocol/VariablePolicy.h>
#include <Library/VariablePolicyLib.h>
#include <Library/VariablePolicyHelperLib.h>
-
+#include <Protocol/VariableLock.h>

/**
DEPRECATED. THIS IS ONLY HERE AS A CONVENIENCE WHILE PORTING.
- Mark a variable that will become read-only after leaving the DXE phase of
execution.
- Write request coming from SMM environment through
EFI_SMM_VARIABLE_PROTOCOL is allowed.
+ Mark a variable that will become read-only after leaving the DXE
+ phase of execution. Write request coming from SMM environment
through
+ EFI_SMM_VARIABLE_PROTOCOL is allowed.

@param[in] This The VARIABLE_LOCK_PROTOCOL instance.
- @param[in] VariableName A pointer to the variable name that will be
made read-only subsequently.
- @param[in] VendorGuid A pointer to the vendor GUID that will be made
read-only subsequently.
+ @param[in] VariableName A pointer to the variable name that will be
made
+ read-only subsequently.
+ @param[in] VendorGuid A pointer to the vendor GUID that will be made
+ read-only subsequently.

- @retval EFI_SUCCESS The variable specified by the VariableName and
the VendorGuid was marked
- as pending to be read-only.
+ @retval EFI_SUCCESS The variable specified by the VariableName and
+ the VendorGuid was marked as pending to be
+ read-only.
@retval EFI_INVALID_PARAMETER VariableName or VendorGuid is NULL.
Or VariableName is an empty string.
- @retval EFI_ACCESS_DENIED EFI_END_OF_DXE_EVENT_GROUP_GUID
or EFI_EVENT_GROUP_READY_TO_BOOT has
- already been signaled.
- @retval EFI_OUT_OF_RESOURCES There is not enough resource to hold
the lock request.
+ @retval EFI_ACCESS_DENIED EFI_END_OF_DXE_EVENT_GROUP_GUID
or
+ EFI_EVENT_GROUP_READY_TO_BOOT has already been
+ signaled.
+ @retval EFI_OUT_OF_RESOURCES There is not enough resource to hold
the lock
+ request.
**/
EFI_STATUS
EFIAPI
VariableLockRequestToLock (
- IN CONST EDKII_VARIABLE_LOCK_PROTOCOL *This,
- IN CHAR16 *VariableName,
- IN EFI_GUID *VendorGuid
+ IN CONST EDKII_VARIABLE_LOCK_PROTOCOL *This,
+ IN CHAR16 *VariableName,
+ IN EFI_GUID *VendorGuid
)
{
- EFI_STATUS Status;
- VARIABLE_POLICY_ENTRY *NewPolicy;
+ EFI_STATUS Status;
+ VARIABLE_POLICY_ENTRY *NewPolicy;
+
+ DEBUG ((DEBUG_ERROR, "!!! DEPRECATED INTERFACE !!! %a() will go away
+ soon!\n", __FUNCTION__)); DEBUG ((DEBUG_ERROR, "!!! DEPRECATED
+ INTERFACE !!! Please move to use Variable Policy!\n")); DEBUG
+ ((DEBUG_ERROR, "!!! DEPRECATED INTERFACE !!! Variable: %g %s\n",
+ VendorGuid, VariableName));

NewPolicy = NULL;
- Status = CreateBasicVariablePolicy( VendorGuid,
- VariableName,
- VARIABLE_POLICY_NO_MIN_SIZE,
- VARIABLE_POLICY_NO_MAX_SIZE,
- VARIABLE_POLICY_NO_MUST_ATTR,
- VARIABLE_POLICY_NO_CANT_ATTR,
- VARIABLE_POLICY_TYPE_LOCK_NOW,
- &NewPolicy );
+ Status = CreateBasicVariablePolicy(
+ VendorGuid,
+ VariableName,
+ VARIABLE_POLICY_NO_MIN_SIZE,
+ VARIABLE_POLICY_NO_MAX_SIZE,
+ VARIABLE_POLICY_NO_MUST_ATTR,
+ VARIABLE_POLICY_NO_CANT_ATTR,
+ VARIABLE_POLICY_TYPE_LOCK_NOW,
+ &NewPolicy
+ );
if (!EFI_ERROR( Status )) {
- Status = RegisterVariablePolicy( NewPolicy );
+ Status = RegisterVariablePolicy (NewPolicy);
+
+ //
+ // If the error returned is EFI_ALREADY_STARTED, we need to check the
+ // current database for the variable and see whether it's locked. If it's
+ // locked, we're still fine, but also generate a DEBUG_ERROR message so
the
+ // duplicate lock can be removed.
+ //
+ if (Status == EFI_ALREADY_STARTED) {
+ Status = ValidateSetVariable (VariableName, VendorGuid, 0, sizeof(""),
"");

Hello Mike,

Sorry for one thing to confirm.

Is it possible when passing "0" as the "Attributes" parameter to function ValidateSetVariable()
causes the below case in ValidateSetVariable():

// Check for attribute constraints.
if ((ActivePolicy->AttributesMustHave & Attributes) != ActivePolicy->AttributesMustHave ||
(ActivePolicy->AttributesCantHave & Attributes) != 0) {
ReturnStatus = EFI_INVALID_PARAMETER;
DEBUG(( DEBUG_VERBOSE, "%a - Bad Attributes. 0x%X <> 0x%X:0x%X\n", __FUNCTION__,
Attributes, ActivePolicy->AttributesMustHave, ActivePolicy->AttributesCantHave ));
goto Exit;
}

if "ActivePolicy->AttributesMustHave" have any bit set?
This will lead to VariableLockRequestToLock() to return an error status.
Thank you! You are exactly right. We do not want this logic used because
we do not know what the valid DataSize and Attribute values are for the
existing policy. For the call to ValidateSetVariable(), we should use
DataSize=0 and Attributes=0.

Status = ValidateSetVariable (VariableName, VendorGuid, 0, 0, NULL);


Best Regards,
Hao Wu


+ if (Status == EFI_WRITE_PROTECTED) {
+ DEBUG ((DEBUG_ERROR, " Variable: %g %s is already locked!\n",
VendorGuid, VariableName));
+ Status = EFI_SUCCESS;
+ } else {
+ DEBUG ((DEBUG_ERROR, " Variable: %g %s can not be locked!\n",
VendorGuid, VariableName));
+ Status = EFI_ACCESS_DENIED;
+ }
+ }
}
- if (EFI_ERROR( Status )) {
+ if (EFI_ERROR (Status)) {
DEBUG(( DEBUG_ERROR, "%a - Failed to lock variable %s! %r\n",
__FUNCTION__, VariableName, Status ));
- ASSERT_EFI_ERROR( Status );
}
if (NewPolicy != NULL) {
FreePool( NewPolicy );
--
2.29.2.windows.2


Re: EDK II CI failing - Do not submit PRs with "push" label set

Michael D Kinney
 

This appears to be a good mirror of cmocka that is being kept up to date.

https://github.com/neverware-mirrors/cmocka

Mike

-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@...>
Sent: Thursday, December 10, 2020 10:23 PM
To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@...>; bret.barkelew@...
Subject: EDK II CI failing - Do not submit PRs with "push" label set

Hello,

The repo for the cmocka submodule is not available right now, and this is failing
CI checks that run unit tests. Pre-commit checks that do not have any unit tests
will pass, but the post commit checks that run everything will always fail.

PROGRESS - ## Checking Git repository: UnitTestFrameworkPkg/Library/CmockaLib/cmocka...
ERROR - Failed to fetch UnitTestFrameworkPkg/Library/CmockaLib/cmocka
ERROR - FAILED!
ERROR - Failed to fetch required repository!
ERROR - Unable to checkout repo due to error

This repo access error to the cmocka has occurred one other time in the past. Bret had
suggested the idea of using a different repo for cmocka, perhaps a fork in GitHub,
to avoid this issue.

We are hoping the issue will be resolved very soon. but until then, please do not
submit any PRs with a “push” label set.

Best regards,

Mike


EDK II CI failing - Do not submit PRs with "push" label set

Michael D Kinney
 

Hello,

The repo for the cmocka submodule is not available right now, and this is failing
CI checks that run unit tests. Pre-commit checks that do not have any unit tests
will pass, but the post commit checks that run everything will always fail.

PROGRESS - ## Checking Git repository: UnitTestFrameworkPkg/Library/CmockaLib/cmocka...
ERROR - Failed to fetch UnitTestFrameworkPkg/Library/CmockaLib/cmocka
ERROR - FAILED!
ERROR - Failed to fetch required repository!
ERROR - Unable to checkout repo due to error

This repo access error to the cmocka has occurred one other time in the past. Bret had
suggested the idea of using a different repo for cmocka, perhaps a fork in GitHub,
to avoid this issue.

We are hoping the issue will be resolved very soon. but until then, please do not
submit any PRs with a “push” label set.

Best regards,

Mike


Re: [Patch 1/1] MdeModulePkg/PciSioSerialDxe: Flush Tx before config change

Wu, Hao A
 

Reviewed-by: Hao A Wu <hao.a.wu@...>

Best Regards,
Hao Wu

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael
D Kinney
Sent: Friday, December 11, 2020 8:33 AM
To: devel@edk2.groups.io
Cc: Wu, Hao A <hao.a.wu@...>; Ni, Ray <ray.ni@...>
Subject: [edk2-devel] [Patch 1/1] MdeModulePkg/PciSioSerialDxe: Flush Tx
before config change

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

Add logic to flush all UART transmit buffers if there is a config change from
Reset(), SetAttributes() or SetControl().
Use a timeout in the flush operation, so the system can continue to boot if
the transmit buffers can not be flushed for any reason.

This change prevents lost characters on serial debug logs and serial consoles
when a config change is made. It also prevents a UART from getting into a
bad state or reporting error status due to characters being transmitted at the
same time registers are updated with new communications settings.

Cc: Hao A Wu <hao.a.wu@...>
Cc: Ray Ni <ray.ni@...>
Signed-off-by: Michael D Kinney <michael.d.kinney@...>
---
.../Bus/Pci/PciSioSerialDxe/SerialIo.c | 91 ++++++++++++++++++-
1 file changed, 90 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Pci/PciSioSerialDxe/SerialIo.c
b/MdeModulePkg/Bus/Pci/PciSioSerialDxe/SerialIo.c
index 8377ffa13c7a..56c5faf5db1f 100644
--- a/MdeModulePkg/Bus/Pci/PciSioSerialDxe/SerialIo.c
+++ b/MdeModulePkg/Bus/Pci/PciSioSerialDxe/SerialIo.c
@@ -1,7 +1,7 @@
/** @file
SerialIo implementation for PCI or SIO UARTs.

-Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2020, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -436,6 +436,68 @@ SerialReceiveTransmit (
return EFI_SUCCESS;
}

+/**
+ Flush the serial hardware transmit FIFO, holding register, and shift register.
+
+ @param SerialDevice The device to flush.
+
+ @retval EFI_SUCCESS The transmit FIFO is completely flushed.
+ @retval EFI_TIMEOUT A timeout occured waiting for the transmit FIFO to
flush.
+**/
+EFI_STATUS
+SerialFlushTransmitFifo (
+ SERIAL_DEV *SerialDevice
+ )
+{
+ SERIAL_PORT_LSR Lsr;
+ UINTN Timeout;
+ UINTN Elapsed;
+
+ //
+ // If this is the first time the UART is being configured, then the
+ current // UART settings are not known, so compute a timeout to wait
+ for the Tx FIFO // assuming the worst case current settings.
+ //
+ // Timeout = (Max Bits per Char) * (Max Pending Chars) / (Slowest Baud
Rate)
+ // Max Bits per Char = Start bit + 8 data bits + parity + 2 stop bits = 12
+ // Max Pending Chars = Largest Tx FIFO + hold + shift = 64 + 1 + 1 = 66
+ // Slowest Reasonable Baud Rate = 300 baud
+ // Timeout = 12 * 66 / 300 = 2.64 seconds = 2,640,000 uS // Timeout
+ = 2640000;
+
+ //
+ // Use the largest of the computed timeout, the default timeout, and
+ the // currently set timeout.
+ //
+ Timeout = MAX (Timeout, SERIAL_PORT_DEFAULT_TIMEOUT); Timeout =
MAX
+ (Timeout, SerialDevice->SerialMode.Timeout);
+
+ //
+ // Wait for the shortest time possible for the serial port to be
+ ready making // sure the transmit FIFO, holding register, and shift
+ register are all // empty. The actual wait time is expected to be
+ very small because the // number characters currently in the FIFO
+ should be small when a // configuration change is requested.
+ //
+ // NOTE: Do not use any DEBUG() or REPORT_STATUS_CODE() or any other
+ calls // in the rest of this function that may send additional
+ characters to this // UART device invalidating the flush operation.
+ //
+ Elapsed = 0;
+ Lsr.Data = READ_LSR (SerialDevice);
+ while (Lsr.Bits.Temt == 0 || Lsr.Bits.Thre == 0) {
+ if (Elapsed >= Timeout) {
+ return EFI_TIMEOUT;
+ }
+ gBS->Stall (TIMEOUT_STALL_INTERVAL);
+ Elapsed += TIMEOUT_STALL_INTERVAL;
+ Lsr.Data = READ_LSR (SerialDevice); }
+
+ return EFI_SUCCESS;
+}
+
//
// Interface Functions
//
@@ -476,6 +538,15 @@ SerialReset (

Tpl = gBS->RaiseTPL (TPL_NOTIFY);

+ //
+ // Wait for all data to be transmitted before changing the UART
configuration.
+ //
+ // NOTE: Do not use any DEBUG() or REPORT_STATUS_CODE() or any other
+ calls // that may send additional characters to this UART device
+ until the UART // configuration change is complete.
+ //
+ SerialFlushTransmitFifo (SerialDevice);
+
//
// Make sure DLAB is 0.
//
@@ -654,6 +725,15 @@ SerialSetAttributes (

Tpl = gBS->RaiseTPL (TPL_NOTIFY);

+ //
+ // Wait for all data to be transmitted before changing the UART
configuration.
+ //
+ // NOTE: Do not use any DEBUG() or REPORT_STATUS_CODE() or any other
+ calls // that may send additional characters to this UART device
+ until the UART // configuration change is complete.
+ //
+ SerialFlushTransmitFifo (SerialDevice);
+
//
// Put serial port on Divisor Latch Mode
//
@@ -826,6 +906,15 @@ SerialSetControl (

Tpl = gBS->RaiseTPL (TPL_NOTIFY);

+ //
+ // Wait for all data to be transmitted before changing the UART
configuration.
+ //
+ // NOTE: Do not use any DEBUG() or REPORT_STATUS_CODE() or any other
+ calls // that may send additional characters to this UART device
+ until the UART // configuration change is complete.
+ //
+ SerialFlushTransmitFifo (SerialDevice);
+
Mcr.Data = READ_MCR (SerialDevice);
Mcr.Bits.DtrC = 0;
Mcr.Bits.Rts = 0;
--
2.29.2.windows.2





Re: [Patch v3 2/2] MdeModulePkg/Variable/RuntimeDxe: Add Variable Lock Protocol Unit Tests

Wu, Hao A
 

With below typos fixed:
"Procol" -> "Protocol"
"succced" -> "succeed"

Reviewed-by: Hao A Wu <hao.a.wu@...>

Best Regards,
Hao Wu

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael
D Kinney
Sent: Friday, December 11, 2020 12:52 PM
To: devel@edk2.groups.io
Cc: Wu, Hao A <hao.a.wu@...>; Liming Gao
<gaoliming@...>; Bret Barkelew
<Bret.Barkelew@...>
Subject: [edk2-devel] [Patch v3 2/2] MdeModulePkg/Variable/RuntimeDxe:
Add Variable Lock Protocol Unit Tests

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

Add host based unit tests for the multiple lock case using Variable Lock
Protocol, Variable Policy Protocol, and mixes of Variable Lock Protocol and
Variable Policy Protocol.

Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Liming Gao <gaoliming@...>
Signed-off-by: Bret Barkelew <Bret.Barkelew@...>
---
MdeModulePkg/Test/MdeModulePkgHostTest.dsc | 11 +
.../VariableLockRequestToLockUnitTest.c | 565 ++++++++++++++++++
.../VariableLockRequestToLockUnitTest.inf | 36 ++
3 files changed, 612 insertions(+)
create mode 100644
MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/Vari
ableLockRequestToLockUnitTest.c
create mode 100644
MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/Vari
ableLockRequestToLockUnitTest.inf

diff --git a/MdeModulePkg/Test/MdeModulePkgHostTest.dsc
b/MdeModulePkg/Test/MdeModulePkgHostTest.dsc
index 72a119db4568..4da4692c8451 100644
--- a/MdeModulePkg/Test/MdeModulePkgHostTest.dsc
+++ b/MdeModulePkg/Test/MdeModulePkgHostTest.dsc
@@ -19,6 +19,9 @@ [Defines]

!include UnitTestFrameworkPkg/UnitTestFrameworkPkgHost.dsc.inc

+[LibraryClasses]
+ SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
+
[Components]

MdeModulePkg/Library/DxeResetSystemLib/UnitTest/MockUefiRuntimeSer
vicesTableLib.inf

@@ -30,3 +33,11 @@ [Components]

ResetSystemLib|MdeModulePkg/Library/DxeResetSystemLib/DxeResetSyst
emLib.inf

UefiRuntimeServicesTableLib|MdeModulePkg/Library/DxeResetSystemLib/
UnitTest/MockUefiRuntimeServicesTableLib.inf
}
+
+
MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/Vari
ableLockRequestToLockUnitTest.inf {
+ <LibraryClasses>
+
VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLi
b.inf
+
VariablePolicyHelperLib|MdeModulePkg/Library/VariablePolicyHelperLib/Va
riablePolicyHelperLib.inf
+ <PcdsFixedAtBuild>
+
+
gEfiMdeModulePkgTokenSpaceGuid.PcdAllowVariablePolicyEnforcementDis
abl
+ e|TRUE
+ }
diff --git
a/MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/Va
riableLockRequestToLockUnitTest.c
b/MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/Va
riableLockRequestToLockUnitTest.c
new file mode 100644
index 000000000000..a5bf720f42e1
--- /dev/null
+++
b/MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/Va
ri
+++ ableLockRequestToLockUnitTest.c
@@ -0,0 +1,565 @@
+/** @file
+ This is a host-based unit test for the VariableLockRequestToLock shim.
+
+ Copyright (c) Microsoft Corporation.
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <stdio.h>
+#include <string.h>
+#include <stdarg.h>
+#include <stddef.h>
+#include <setjmp.h>
+#include <cmocka.h>
+
+#include <Uefi.h>
+#include <Library/DebugLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/MemoryAllocationLib.h> #include
+<Library/UnitTestLib.h> #include <Library/VariablePolicyLib.h> #include
+<Library/VariablePolicyHelperLib.h>
+
+#include <Protocol/VariableLock.h>
+
+#define UNIT_TEST_NAME "VarPol/VarLock Shim Unit Test"
+#define UNIT_TEST_VERSION "1.0"
+
+///=== CODE UNDER TEST
+=========================================================
==============
+====
+
+EFI_STATUS
+EFIAPI
+VariableLockRequestToLock (
+ IN CONST EDKII_VARIABLE_LOCK_PROTOCOL *This,
+ IN CHAR16 *VariableName,
+ IN EFI_GUID *VendorGuid
+ );
+
+///=== TEST DATA
+=========================================================
==============
+===========
+
+//
+// Test GUID 1 {F955BA2D-4A2C-480C-BFD1-3CC522610592}
+//
+EFI_GUID mTestGuid1 = {
+ 0xf955ba2d, 0x4a2c, 0x480c, {0xbf, 0xd1, 0x3c, 0xc5, 0x22, 0x61, 0x5,
+0x92} };
+
+//
+// Test GUID 2 {2DEA799E-5E73-43B9-870E-C945CE82AF3A}
+//
+EFI_GUID mTestGuid2 = {
+ 0x2dea799e, 0x5e73, 0x43b9, {0x87, 0xe, 0xc9, 0x45, 0xce, 0x82, 0xaf,
+0x3a} };
+
+//
+// Test GUID 3 {698A2BFD-A616-482D-B88C-7100BD6682A9}
+//
+EFI_GUID mTestGuid3 = {
+ 0x698a2bfd, 0xa616, 0x482d, {0xb8, 0x8c, 0x71, 0x0, 0xbd, 0x66, 0x82,
+0xa9} };
+
+#define TEST_VAR_1_NAME L"TestVar1"
+#define TEST_VAR_2_NAME L"TestVar2"
+#define TEST_VAR_3_NAME L"TestVar3"
+
+#define TEST_POLICY_ATTRIBUTES_NULL 0
+#define TEST_POLICY_MIN_SIZE_NULL 0
+#define TEST_POLICY_MAX_SIZE_NULL MAX_UINT32
+
+#define TEST_POLICY_MIN_SIZE_10 10
+#define TEST_POLICY_MAX_SIZE_200 200
+
+///=== HELPER FUNCTIONS
+=========================================================
==============
+====
+
+/**
+ Mocked version of GetVariable, for testing.
+
+ @param VariableName
+ @param VendorGuid
+ @param Attributes
+ @param DataSize
+ @param Data
+**/
+EFI_STATUS
+EFIAPI
+StubGetVariableNull (
+ IN CHAR16 *VariableName,
+ IN EFI_GUID *VendorGuid,
+ OUT UINT32 *Attributes, OPTIONAL
+ IN OUT UINTN *DataSize,
+ OUT VOID *Data OPTIONAL
+ )
+{
+ UINT32 MockedAttr;
+ UINTN MockedDataSize;
+ VOID *MockedData;
+ EFI_STATUS MockedReturn;
+
+ check_expected_ptr (VariableName);
+ check_expected_ptr (VendorGuid);
+ check_expected_ptr (DataSize);
+
+ MockedAttr = (UINT32)mock();
+ MockedDataSize = (UINTN)mock();
+ MockedData = (VOID*)(UINTN)mock();
+ MockedReturn = (EFI_STATUS)mock();
+
+ if (Attributes != NULL) {
+ *Attributes = MockedAttr;
+ }
+ if (Data != NULL && !EFI_ERROR (MockedReturn)) {
+ CopyMem (Data, MockedData, MockedDataSize); }
+
+ *DataSize = MockedDataSize;
+
+ return MockedReturn;
+}
+
+//
+// Anything you think might be helpful that isn't a test itself.
+//
+
+/**
+ This is a common setup function that will ensure the library is
+always
+ initialized with the stubbed GetVariable.
+
+ Not used by all test cases, but by most.
+
+ @param[in] Context Unit test case context **/ STATIC
+UNIT_TEST_STATUS EFIAPI LibInitMocked (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ return EFI_ERROR (InitVariablePolicyLib (StubGetVariableNull)) ?
+UNIT_TEST_ERROR_PREREQUISITE_NOT_MET : UNIT_TEST_PASSED; }
+
+/**
+ Common cleanup function to make sure that the library is always
+de-initialized
+ prior to the next test case.
+
+ @param[in] Context Unit test case context **/ STATIC VOID EFIAPI
+LibCleanup (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ DeinitVariablePolicyLib();
+}
+
+///=== TEST CASES
+=========================================================
==============
+==========
+
+///===== SHIM SUITE
+=========================================================
==
+
+/**
+ Test Case that locks a single variable using the Variable Lock Protocol.
+ The call is expected to succeed.
+
+ @param[in] Context Unit test case context **/ UNIT_TEST_STATUS
+EFIAPI LockingWithoutAnyPoliciesShouldSucceed (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ EFI_STATUS Status;
+
+ Status = VariableLockRequestToLock (NULL, TEST_VAR_1_NAME,
+ &mTestGuid1); UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ return UNIT_TEST_PASSED;
+}
+
+/**
+ Test Case that locks the same variable twice using the Variable Lock Procol.
+ Both calls are expected to succeed.
+
+ @param[in] Context Unit test case context
+ **/
+UNIT_TEST_STATUS
+EFIAPI
+LockingTwiceShouldSucceed (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ EFI_STATUS Status;
+
+ Status = VariableLockRequestToLock (NULL, TEST_VAR_1_NAME,
+ &mTestGuid1); UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ Status = VariableLockRequestToLock (NULL, TEST_VAR_1_NAME,
+ &mTestGuid1); UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ return UNIT_TEST_PASSED;
+}
+
+/**
+ Test Case that locks a variable using the Variable Policy Protocol
+then locks
+ the same variable using the Variable Lock Protocol.
+ Both calls are expected to succeed.
+
+ @param[in] Context Unit test case context
+ **/
+UNIT_TEST_STATUS
+EFIAPI
+LockingALockedVariableShouldSucceed (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ EFI_STATUS Status;
+ VARIABLE_POLICY_ENTRY *NewEntry;
+
+ //
+ // Create a variable policy that locks the variable.
+ //
+ Status = CreateBasicVariablePolicy (
+ &mTestGuid1,
+ TEST_VAR_1_NAME,
+ TEST_POLICY_MIN_SIZE_NULL,
+ TEST_POLICY_MAX_SIZE_200,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ VARIABLE_POLICY_TYPE_LOCK_NOW,
+ &NewEntry
+ );
+ UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ //
+ // Register the new policy.
+ //
+ Status = RegisterVariablePolicy (NewEntry);
+
+ Status = VariableLockRequestToLock (NULL, TEST_VAR_1_NAME,
+ &mTestGuid1); UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ FreePool (NewEntry);
+
+ return UNIT_TEST_PASSED;
+}
+
+/**
+ Test Case that locks a variable using the Variable Policy Protocol
+with a
+ policy other than LOCK_NOW then attempts to lock the same variable
+using the
+ Variable Lock Protocol. The call to Variable Policy is expected to
+succced
+ and the call to Variable Lock is expected to fail.
+
+ @param[in] Context Unit test case context
+ **/
+UNIT_TEST_STATUS
+EFIAPI
+LockingAnUnlockedVariableShouldFail (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ EFI_STATUS Status;
+ VARIABLE_POLICY_ENTRY *NewEntry;
+
+ // Create a variable policy that locks the variable.
+ Status = CreateVarStateVariablePolicy (&mTestGuid1,
+ TEST_VAR_1_NAME,
+ TEST_POLICY_MIN_SIZE_NULL,
+ TEST_POLICY_MAX_SIZE_200,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ &mTestGuid2,
+ 1,
+ TEST_VAR_2_NAME,
+ &NewEntry);
+ UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ // Register the new policy.
+ Status = RegisterVariablePolicy (NewEntry);
+
+ // Configure the stub to not care about parameters. We're testing errors.
+ expect_any_always( StubGetVariableNull, VariableName );
+ expect_any_always( StubGetVariableNull, VendorGuid );
+ expect_any_always( StubGetVariableNull, DataSize );
+
+ // With a policy, make sure that writes still work, since the variable doesn't
exist.
+ will_return( StubGetVariableNull, TEST_POLICY_ATTRIBUTES_NULL ); //
Attributes
+ will_return( StubGetVariableNull, 0 ); // Size
+ will_return( StubGetVariableNull, NULL ); // DataPtr
+ will_return( StubGetVariableNull, EFI_NOT_FOUND); // Status
+
+ Status = VariableLockRequestToLock (NULL, TEST_VAR_1_NAME,
+ &mTestGuid1); UT_ASSERT_TRUE (EFI_ERROR (Status));
+
+ FreePool (NewEntry);
+
+ return UNIT_TEST_PASSED;
+}
+
+/**
+ Test Case that locks a variable using the Variable Policy Protocol
+with a
+ policy other than LOCK_NOW, but is currently locked. Then attempts
+to lock
+ the same variable using the Variable Lock Protocol. The call to
+Variable
+ Policy is expected to succced and the call to Variable Lock also
+expected to
+ succeed.
+
+ @param[in] Context Unit test case context
+ **/
+UNIT_TEST_STATUS
+EFIAPI
+LockingALockedVariableWithMatchingDataShouldSucceed (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ EFI_STATUS Status;
+ VARIABLE_POLICY_ENTRY *NewEntry;
+ UINT8 Data;
+
+ // Create a variable policy that locks the variable.
+ Status = CreateVarStateVariablePolicy (&mTestGuid1,
+ TEST_VAR_1_NAME,
+ TEST_POLICY_MIN_SIZE_NULL,
+ TEST_POLICY_MAX_SIZE_200,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ &mTestGuid2,
+ 1,
+ TEST_VAR_2_NAME,
+ &NewEntry);
+ UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ // Register the new policy.
+ Status = RegisterVariablePolicy (NewEntry);
+
+ // Configure the stub to not care about parameters. We're testing errors.
+ expect_any_always( StubGetVariableNull, VariableName );
+ expect_any_always( StubGetVariableNull, VendorGuid );
+ expect_any_always( StubGetVariableNull, DataSize );
+
+ // With a policy, make sure that writes still work, since the variable doesn't
exist.
+ Data = 1;
+ will_return( StubGetVariableNull, TEST_POLICY_ATTRIBUTES_NULL ); //
Attributes
+ will_return( StubGetVariableNull, sizeof (Data) ); // Size
+ will_return( StubGetVariableNull, &Data ); // DataPtr
+ will_return( StubGetVariableNull, EFI_SUCCESS); // Status
+
+ Status = VariableLockRequestToLock (NULL, TEST_VAR_1_NAME,
+ &mTestGuid1); UT_ASSERT_TRUE (!EFI_ERROR (Status));
+
+ FreePool (NewEntry);
+
+ return UNIT_TEST_PASSED;
+}
+
+/**
+ Test Case that locks a variable using the Variable Policy Protocol
+with a
+ policy other than LOCK_NOW, but variable data does not match. Then
+attempts
+ to lock the same variable using the Variable Lock Protocol. The call
+to
+ Variable Policy is expected to succced and the call to Variable Lock
+is
+ expected to fail.
+
+ @param[in] Context Unit test case context
+ **/
+UNIT_TEST_STATUS
+EFIAPI
+LockingALockedVariableWithNonMatchingDataShouldFail (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ EFI_STATUS Status;
+ VARIABLE_POLICY_ENTRY *NewEntry;
+ UINT8 Data;
+
+ // Create a variable policy that locks the variable.
+ Status = CreateVarStateVariablePolicy (&mTestGuid1,
+ TEST_VAR_1_NAME,
+ TEST_POLICY_MIN_SIZE_NULL,
+ TEST_POLICY_MAX_SIZE_200,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ &mTestGuid2,
+ 1,
+ TEST_VAR_2_NAME,
+ &NewEntry);
+ UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ // Register the new policy.
+ Status = RegisterVariablePolicy (NewEntry);
+
+ // Configure the stub to not care about parameters. We're testing errors.
+ expect_any_always( StubGetVariableNull, VariableName );
+ expect_any_always( StubGetVariableNull, VendorGuid );
+ expect_any_always( StubGetVariableNull, DataSize );
+
+ // With a policy, make sure that writes still work, since the variable doesn't
exist.
+ Data = 2;
+ will_return( StubGetVariableNull, TEST_POLICY_ATTRIBUTES_NULL ); //
Attributes
+ will_return( StubGetVariableNull, sizeof (Data) ); // Size
+ will_return( StubGetVariableNull, &Data ); // DataPtr
+ will_return( StubGetVariableNull, EFI_SUCCESS); // Status
+
+ Status = VariableLockRequestToLock (NULL, TEST_VAR_1_NAME,
+ &mTestGuid1); UT_ASSERT_TRUE (EFI_ERROR (Status));
+
+ FreePool (NewEntry);
+
+ return UNIT_TEST_PASSED;
+}
+
+/**
+ Test Case that locks a variable using Variable Lock Protocol Policy
+Protocol
+ then and then attempts to lock the same variable using the Variable
+Policy
+ Protocol. The call to Variable Lock is expected to succced and the
+call to
+ Variable Policy is expected to fail.
+
+ @param[in] Context Unit test case context
+ **/
+UNIT_TEST_STATUS
+EFIAPI
+SettingPolicyForALockedVariableShouldFail (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ EFI_STATUS Status;
+ VARIABLE_POLICY_ENTRY *NewEntry;
+
+ // Lock the variable.
+ Status = VariableLockRequestToLock (NULL, TEST_VAR_1_NAME,
+ &mTestGuid1); UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ // Create a variable policy that locks the variable.
+ Status = CreateVarStateVariablePolicy (&mTestGuid1,
+ TEST_VAR_1_NAME,
+ TEST_POLICY_MIN_SIZE_NULL,
+ TEST_POLICY_MAX_SIZE_200,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ &mTestGuid2,
+ 1,
+ TEST_VAR_2_NAME,
+ &NewEntry);
+ UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ // Register the new policy.
+ Status = RegisterVariablePolicy (NewEntry); UT_ASSERT_TRUE
+ (EFI_ERROR (Status));
+
+ FreePool (NewEntry);
+
+ return UNIT_TEST_PASSED;
+}
+
+/**
+ Main entry point to this unit test application.
+
+ Sets up and runs the test suites.
+**/
+VOID
+EFIAPI
+UnitTestMain (
+ VOID
+ )
+{
+ EFI_STATUS Status;
+ UNIT_TEST_FRAMEWORK_HANDLE Framework;
+ UNIT_TEST_SUITE_HANDLE ShimTests;
+
+ Framework = NULL;
+
+ DEBUG ((DEBUG_INFO, "%a v%a\n", UNIT_TEST_NAME,
UNIT_TEST_VERSION));
+
+ //
+ // Start setting up the test framework for running the tests.
+ //
+ Status = InitUnitTestFramework (&Framework, UNIT_TEST_NAME,
+ gEfiCallerBaseName, UNIT_TEST_VERSION); if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Failed in InitUnitTestFramework. Status
= %r\n", Status));
+ goto EXIT;
+ }
+
+ //
+ // Add all test suites and tests.
+ //
+ Status = CreateUnitTestSuite (
+ &ShimTests, Framework,
+ "Variable Lock Shim Tests", "VarPolicy.VarLockShim", NULL, NULL
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Failed in CreateUnitTestSuite for
ShimTests\n"));
+ Status = EFI_OUT_OF_RESOURCES;
+ goto EXIT;
+ }
+ AddTestCase (
+ ShimTests,
+ "Locking a variable with no matching policies should always work",
"EmptyPolicies",
+ LockingWithoutAnyPoliciesShouldSucceed, LibInitMocked, LibCleanup,
NULL
+ );
+ AddTestCase (
+ ShimTests,
+ "Locking a variable twice should always work", "DoubleLock",
+ LockingTwiceShouldSucceed, LibInitMocked, LibCleanup, NULL
+ );
+ AddTestCase (
+ ShimTests,
+ "Locking a variable that's already locked by another policy should work",
"LockAfterPolicy",
+ LockingALockedVariableShouldSucceed, LibInitMocked, LibCleanup, NULL
+ );
+ AddTestCase (
+ ShimTests,
+ "Locking a variable that already has an unlocked policy should fail",
"LockAfterUnlockedPolicy",
+ LockingAnUnlockedVariableShouldFail, LibInitMocked, LibCleanup, NULL
+ );
+ AddTestCase (
+ ShimTests,
+ "Locking a variable that already has an locked policy should succeed",
"LockAfterLockedPolicyMatchingData",
+ LockingALockedVariableWithMatchingDataShouldSucceed, LibInitMocked,
LibCleanup, NULL
+ );
+ AddTestCase (
+ ShimTests,
+ "Locking a variable that already has an locked policy with matching data
should succeed", "LockAfterLockedPolicyNonMatchingData",
+ LockingALockedVariableWithNonMatchingDataShouldFail, LibInitMocked,
LibCleanup, NULL
+ );
+ AddTestCase (
+ ShimTests,
+ "Adding a policy for a variable that has previously been locked should
always fail", "SetPolicyAfterLock",
+ SettingPolicyForALockedVariableShouldFail, LibInitMocked, LibCleanup,
NULL
+ );
+
+ //
+ // Execute the tests.
+ //
+ Status = RunAllTestSuites (Framework);
+
+EXIT:
+ if (Framework != NULL) {
+ FreeUnitTestFramework (Framework);
+ }
+
+ return;
+}
+
+///
+/// Avoid ECC error for function name that starts with lower case
+letter /// #define Main main
+
+/**
+ Standard POSIX C entry point for host based unit test execution.
+
+ @param[in] Argc Number of arguments
+ @param[in] Argv Array of pointers to arguments
+
+ @retval 0 Success
+ @retval other Error
+**/
+INT32
+Main (
+ IN INT32 Argc,
+ IN CHAR8 *Argv[]
+ )
+{
+ UnitTestMain ();
+ return 0;
+}
diff --git
a/MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/Va
riableLockRequestToLockUnitTest.inf
b/MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/Va
riableLockRequestToLockUnitTest.inf
new file mode 100644
index 000000000000..2a659d7e1370
--- /dev/null
+++
b/MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/Va
ri
+++ ableLockRequestToLockUnitTest.inf
@@ -0,0 +1,36 @@
+## @file
+# This is a host-based unit test for the VariableLockRequestToLock shim.
+#
+# Copyright (c) Microsoft Corporation.
+# SPDX-License-Identifier: BSD-2-Clause-Patent ##
+
+[Defines]
+ INF_VERSION = 0x00010017
+ BASE_NAME = VariableLockRequestToLockUnitTest
+ FILE_GUID = A7388B6C-7274-4717-9649-BDC5DFD1FCBE
+ VERSION_STRING = 1.0
+ MODULE_TYPE = HOST_APPLICATION
+
+#
+# The following information is for reference only and not required by the
build tools.
+#
+# VALID_ARCHITECTURES = IA32 X64 ARM AARCH64
+#
+
+[Sources]
+ VariableLockRequestToLockUnitTest.c
+ ../VariableLockRequestToLock.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ UnitTestFrameworkPkg/UnitTestFrameworkPkg.dec
+
+[LibraryClasses]
+ UnitTestLib
+ DebugLib
+ VariablePolicyLib
+ VariablePolicyHelperLib
+ BaseMemoryLib
+ MemoryAllocationLib
--
2.29.2.windows.2





Re: [Patch v3 1/2] MdeModulePkg/Variable/RuntimeDxe: Restore Variable Lock Protocol behavior

Wu, Hao A
 

-----Original Message-----
From: Michael D Kinney <michael.d.kinney@...>
Sent: Friday, December 11, 2020 12:52 PM
To: devel@edk2.groups.io
Cc: Bret Barkelew <bret.barkelew@...>; Wu, Hao A
<hao.a.wu@...>; Liming Gao <gaoliming@...>; Bret
Barkelew <Bret.Barkelew@...>
Subject: [Patch v3 1/2] MdeModulePkg/Variable/RuntimeDxe: Restore
Variable Lock Protocol behavior

From: Bret Barkelew <bret.barkelew@...>

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

The VariableLock shim currently fails if called twice because the underlying
Variable Policy engine returns an error if a policy is set on an existing variable.

This breaks existing code which expect it to silently pass if a variable is locked
multiple times (because it should "be locked").

Refactor the shim to confirm that the variable is indeed locked and then
change the error to EFI_SUCCESS and generate a DEBUG_ERROR message so
the duplicate lock can be reported in a debug log and removed.

Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Liming Gao <gaoliming@...>
Signed-off-by: Bret Barkelew <Bret.Barkelew@...>
---
.../RuntimeDxe/VariableLockRequestToLock.c | 95 ++++++++++++-------
1 file changed, 59 insertions(+), 36 deletions(-)

diff --git
a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToL
ock.c
b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToL
ock.c
index 4aa854aaf260..0715b527a0f7 100644
---
a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToL
ock.c
+++
b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToL
o
+++ ck.c
@@ -1,67 +1,90 @@
-/** @file -- VariableLockRequestToLock.c -Temporary location of the
RequestToLock shim code while -projects are moved to VariablePolicy.
Should be removed when deprecated.
+/** @file
+ Temporary location of the RequestToLock shim code while projects
+ are moved to VariablePolicy. Should be removed when deprecated.

-Copyright (c) Microsoft Corporation.
-SPDX-License-Identifier: BSD-2-Clause-Patent
+ Copyright (c) Microsoft Corporation.
+ SPDX-License-Identifier: BSD-2-Clause-Patent

**/

#include <Uefi.h>
-
#include <Library/DebugLib.h>
#include <Library/MemoryAllocationLib.h>
-
-#include <Protocol/VariableLock.h>
-
-#include <Protocol/VariablePolicy.h>
#include <Library/VariablePolicyLib.h>
#include <Library/VariablePolicyHelperLib.h>
-
+#include <Protocol/VariableLock.h>

/**
DEPRECATED. THIS IS ONLY HERE AS A CONVENIENCE WHILE PORTING.
- Mark a variable that will become read-only after leaving the DXE phase of
execution.
- Write request coming from SMM environment through
EFI_SMM_VARIABLE_PROTOCOL is allowed.
+ Mark a variable that will become read-only after leaving the DXE
+ phase of execution. Write request coming from SMM environment
through
+ EFI_SMM_VARIABLE_PROTOCOL is allowed.

@param[in] This The VARIABLE_LOCK_PROTOCOL instance.
- @param[in] VariableName A pointer to the variable name that will be
made read-only subsequently.
- @param[in] VendorGuid A pointer to the vendor GUID that will be made
read-only subsequently.
+ @param[in] VariableName A pointer to the variable name that will be
made
+ read-only subsequently.
+ @param[in] VendorGuid A pointer to the vendor GUID that will be made
+ read-only subsequently.

- @retval EFI_SUCCESS The variable specified by the VariableName and
the VendorGuid was marked
- as pending to be read-only.
+ @retval EFI_SUCCESS The variable specified by the VariableName and
+ the VendorGuid was marked as pending to be
+ read-only.
@retval EFI_INVALID_PARAMETER VariableName or VendorGuid is NULL.
Or VariableName is an empty string.
- @retval EFI_ACCESS_DENIED EFI_END_OF_DXE_EVENT_GROUP_GUID
or EFI_EVENT_GROUP_READY_TO_BOOT has
- already been signaled.
- @retval EFI_OUT_OF_RESOURCES There is not enough resource to hold
the lock request.
+ @retval EFI_ACCESS_DENIED EFI_END_OF_DXE_EVENT_GROUP_GUID
or
+ EFI_EVENT_GROUP_READY_TO_BOOT has already been
+ signaled.
+ @retval EFI_OUT_OF_RESOURCES There is not enough resource to hold
the lock
+ request.
**/
EFI_STATUS
EFIAPI
VariableLockRequestToLock (
- IN CONST EDKII_VARIABLE_LOCK_PROTOCOL *This,
- IN CHAR16 *VariableName,
- IN EFI_GUID *VendorGuid
+ IN CONST EDKII_VARIABLE_LOCK_PROTOCOL *This,
+ IN CHAR16 *VariableName,
+ IN EFI_GUID *VendorGuid
)
{
- EFI_STATUS Status;
- VARIABLE_POLICY_ENTRY *NewPolicy;
+ EFI_STATUS Status;
+ VARIABLE_POLICY_ENTRY *NewPolicy;
+
+ DEBUG ((DEBUG_ERROR, "!!! DEPRECATED INTERFACE !!! %a() will go away
+ soon!\n", __FUNCTION__)); DEBUG ((DEBUG_ERROR, "!!! DEPRECATED
+ INTERFACE !!! Please move to use Variable Policy!\n")); DEBUG
+ ((DEBUG_ERROR, "!!! DEPRECATED INTERFACE !!! Variable: %g %s\n",
+ VendorGuid, VariableName));

NewPolicy = NULL;
- Status = CreateBasicVariablePolicy( VendorGuid,
- VariableName,
- VARIABLE_POLICY_NO_MIN_SIZE,
- VARIABLE_POLICY_NO_MAX_SIZE,
- VARIABLE_POLICY_NO_MUST_ATTR,
- VARIABLE_POLICY_NO_CANT_ATTR,
- VARIABLE_POLICY_TYPE_LOCK_NOW,
- &NewPolicy );
+ Status = CreateBasicVariablePolicy(
+ VendorGuid,
+ VariableName,
+ VARIABLE_POLICY_NO_MIN_SIZE,
+ VARIABLE_POLICY_NO_MAX_SIZE,
+ VARIABLE_POLICY_NO_MUST_ATTR,
+ VARIABLE_POLICY_NO_CANT_ATTR,
+ VARIABLE_POLICY_TYPE_LOCK_NOW,
+ &NewPolicy
+ );
if (!EFI_ERROR( Status )) {
- Status = RegisterVariablePolicy( NewPolicy );
+ Status = RegisterVariablePolicy (NewPolicy);
+
+ //
+ // If the error returned is EFI_ALREADY_STARTED, we need to check the
+ // current database for the variable and see whether it's locked. If it's
+ // locked, we're still fine, but also generate a DEBUG_ERROR message so
the
+ // duplicate lock can be removed.
+ //
+ if (Status == EFI_ALREADY_STARTED) {
+ Status = ValidateSetVariable (VariableName, VendorGuid, 0, sizeof(""),
"");

Hello Mike,

Sorry for one thing to confirm.

Is it possible when passing "0" as the "Attributes" parameter to function ValidateSetVariable()
causes the below case in ValidateSetVariable():

// Check for attribute constraints.
if ((ActivePolicy->AttributesMustHave & Attributes) != ActivePolicy->AttributesMustHave ||
(ActivePolicy->AttributesCantHave & Attributes) != 0) {
ReturnStatus = EFI_INVALID_PARAMETER;
DEBUG(( DEBUG_VERBOSE, "%a - Bad Attributes. 0x%X <> 0x%X:0x%X\n", __FUNCTION__,
Attributes, ActivePolicy->AttributesMustHave, ActivePolicy->AttributesCantHave ));
goto Exit;
}

if "ActivePolicy->AttributesMustHave" have any bit set?
This will lead to VariableLockRequestToLock() to return an error status.

Best Regards,
Hao Wu


+ if (Status == EFI_WRITE_PROTECTED) {
+ DEBUG ((DEBUG_ERROR, " Variable: %g %s is already locked!\n",
VendorGuid, VariableName));
+ Status = EFI_SUCCESS;
+ } else {
+ DEBUG ((DEBUG_ERROR, " Variable: %g %s can not be locked!\n",
VendorGuid, VariableName));
+ Status = EFI_ACCESS_DENIED;
+ }
+ }
}
- if (EFI_ERROR( Status )) {
+ if (EFI_ERROR (Status)) {
DEBUG(( DEBUG_ERROR, "%a - Failed to lock variable %s! %r\n",
__FUNCTION__, VariableName, Status ));
- ASSERT_EFI_ERROR( Status );
}
if (NewPolicy != NULL) {
FreePool( NewPolicy );
--
2.29.2.windows.2


[Patch v3 2/2] MdeModulePkg/Variable/RuntimeDxe: Add Variable Lock Protocol Unit Tests

Michael D Kinney
 

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

Add host based unit tests for the multiple lock case using Variable Lock
Protocol, Variable Policy Protocol, and mixes of Variable Lock Protocol
and Variable Policy Protocol.

Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Liming Gao <gaoliming@...>
Signed-off-by: Bret Barkelew <Bret.Barkelew@...>
---
MdeModulePkg/Test/MdeModulePkgHostTest.dsc | 11 +
.../VariableLockRequestToLockUnitTest.c | 565 ++++++++++++++++++
.../VariableLockRequestToLockUnitTest.inf | 36 ++
3 files changed, 612 insertions(+)
create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/VariableLockRequestToLockUnitTest.c
create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/VariableLockRequestToLockUnitTest.inf

diff --git a/MdeModulePkg/Test/MdeModulePkgHostTest.dsc b/MdeModulePkg/Test/MdeModulePkgHostTest.dsc
index 72a119db4568..4da4692c8451 100644
--- a/MdeModulePkg/Test/MdeModulePkgHostTest.dsc
+++ b/MdeModulePkg/Test/MdeModulePkgHostTest.dsc
@@ -19,6 +19,9 @@ [Defines]

!include UnitTestFrameworkPkg/UnitTestFrameworkPkgHost.dsc.inc

+[LibraryClasses]
+ SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
+
[Components]
MdeModulePkg/Library/DxeResetSystemLib/UnitTest/MockUefiRuntimeServicesTableLib.inf

@@ -30,3 +33,11 @@ [Components]
ResetSystemLib|MdeModulePkg/Library/DxeResetSystemLib/DxeResetSystemLib.inf
UefiRuntimeServicesTableLib|MdeModulePkg/Library/DxeResetSystemLib/UnitTest/MockUefiRuntimeServicesTableLib.inf
}
+
+ MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/VariableLockRequestToLockUnitTest.inf {
+ <LibraryClasses>
+ VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf
+ VariablePolicyHelperLib|MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.inf
+ <PcdsFixedAtBuild>
+ gEfiMdeModulePkgTokenSpaceGuid.PcdAllowVariablePolicyEnforcementDisable|TRUE
+ }
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/VariableLockRequestToLockUnitTest.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/VariableLockRequestToLockUnitTest.c
new file mode 100644
index 000000000000..a5bf720f42e1
--- /dev/null
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/VariableLockRequestToLockUnitTest.c
@@ -0,0 +1,565 @@
+/** @file
+ This is a host-based unit test for the VariableLockRequestToLock shim.
+
+ Copyright (c) Microsoft Corporation.
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <stdio.h>
+#include <string.h>
+#include <stdarg.h>
+#include <stddef.h>
+#include <setjmp.h>
+#include <cmocka.h>
+
+#include <Uefi.h>
+#include <Library/DebugLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/UnitTestLib.h>
+#include <Library/VariablePolicyLib.h>
+#include <Library/VariablePolicyHelperLib.h>
+
+#include <Protocol/VariableLock.h>
+
+#define UNIT_TEST_NAME "VarPol/VarLock Shim Unit Test"
+#define UNIT_TEST_VERSION "1.0"
+
+///=== CODE UNDER TEST ===========================================================================
+
+EFI_STATUS
+EFIAPI
+VariableLockRequestToLock (
+ IN CONST EDKII_VARIABLE_LOCK_PROTOCOL *This,
+ IN CHAR16 *VariableName,
+ IN EFI_GUID *VendorGuid
+ );
+
+///=== TEST DATA ==================================================================================
+
+//
+// Test GUID 1 {F955BA2D-4A2C-480C-BFD1-3CC522610592}
+//
+EFI_GUID mTestGuid1 = {
+ 0xf955ba2d, 0x4a2c, 0x480c, {0xbf, 0xd1, 0x3c, 0xc5, 0x22, 0x61, 0x5, 0x92}
+};
+
+//
+// Test GUID 2 {2DEA799E-5E73-43B9-870E-C945CE82AF3A}
+//
+EFI_GUID mTestGuid2 = {
+ 0x2dea799e, 0x5e73, 0x43b9, {0x87, 0xe, 0xc9, 0x45, 0xce, 0x82, 0xaf, 0x3a}
+};
+
+//
+// Test GUID 3 {698A2BFD-A616-482D-B88C-7100BD6682A9}
+//
+EFI_GUID mTestGuid3 = {
+ 0x698a2bfd, 0xa616, 0x482d, {0xb8, 0x8c, 0x71, 0x0, 0xbd, 0x66, 0x82, 0xa9}
+};
+
+#define TEST_VAR_1_NAME L"TestVar1"
+#define TEST_VAR_2_NAME L"TestVar2"
+#define TEST_VAR_3_NAME L"TestVar3"
+
+#define TEST_POLICY_ATTRIBUTES_NULL 0
+#define TEST_POLICY_MIN_SIZE_NULL 0
+#define TEST_POLICY_MAX_SIZE_NULL MAX_UINT32
+
+#define TEST_POLICY_MIN_SIZE_10 10
+#define TEST_POLICY_MAX_SIZE_200 200
+
+///=== HELPER FUNCTIONS ===========================================================================
+
+/**
+ Mocked version of GetVariable, for testing.
+
+ @param VariableName
+ @param VendorGuid
+ @param Attributes
+ @param DataSize
+ @param Data
+**/
+EFI_STATUS
+EFIAPI
+StubGetVariableNull (
+ IN CHAR16 *VariableName,
+ IN EFI_GUID *VendorGuid,
+ OUT UINT32 *Attributes, OPTIONAL
+ IN OUT UINTN *DataSize,
+ OUT VOID *Data OPTIONAL
+ )
+{
+ UINT32 MockedAttr;
+ UINTN MockedDataSize;
+ VOID *MockedData;
+ EFI_STATUS MockedReturn;
+
+ check_expected_ptr (VariableName);
+ check_expected_ptr (VendorGuid);
+ check_expected_ptr (DataSize);
+
+ MockedAttr = (UINT32)mock();
+ MockedDataSize = (UINTN)mock();
+ MockedData = (VOID*)(UINTN)mock();
+ MockedReturn = (EFI_STATUS)mock();
+
+ if (Attributes != NULL) {
+ *Attributes = MockedAttr;
+ }
+ if (Data != NULL && !EFI_ERROR (MockedReturn)) {
+ CopyMem (Data, MockedData, MockedDataSize);
+ }
+
+ *DataSize = MockedDataSize;
+
+ return MockedReturn;
+}
+
+//
+// Anything you think might be helpful that isn't a test itself.
+//
+
+/**
+ This is a common setup function that will ensure the library is always
+ initialized with the stubbed GetVariable.
+
+ Not used by all test cases, but by most.
+
+ @param[in] Context Unit test case context
+**/
+STATIC
+UNIT_TEST_STATUS
+EFIAPI
+LibInitMocked (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ return EFI_ERROR (InitVariablePolicyLib (StubGetVariableNull)) ? UNIT_TEST_ERROR_PREREQUISITE_NOT_MET : UNIT_TEST_PASSED;
+}
+
+/**
+ Common cleanup function to make sure that the library is always de-initialized
+ prior to the next test case.
+
+ @param[in] Context Unit test case context
+**/
+STATIC
+VOID
+EFIAPI
+LibCleanup (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ DeinitVariablePolicyLib();
+}
+
+///=== TEST CASES =================================================================================
+
+///===== SHIM SUITE ===========================================================
+
+/**
+ Test Case that locks a single variable using the Variable Lock Protocol.
+ The call is expected to succeed.
+
+ @param[in] Context Unit test case context
+**/
+UNIT_TEST_STATUS
+EFIAPI
+LockingWithoutAnyPoliciesShouldSucceed (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ EFI_STATUS Status;
+
+ Status = VariableLockRequestToLock (NULL, TEST_VAR_1_NAME, &mTestGuid1);
+ UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ return UNIT_TEST_PASSED;
+}
+
+/**
+ Test Case that locks the same variable twice using the Variable Lock Procol.
+ Both calls are expected to succeed.
+
+ @param[in] Context Unit test case context
+ **/
+UNIT_TEST_STATUS
+EFIAPI
+LockingTwiceShouldSucceed (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ EFI_STATUS Status;
+
+ Status = VariableLockRequestToLock (NULL, TEST_VAR_1_NAME, &mTestGuid1);
+ UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ Status = VariableLockRequestToLock (NULL, TEST_VAR_1_NAME, &mTestGuid1);
+ UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ return UNIT_TEST_PASSED;
+}
+
+/**
+ Test Case that locks a variable using the Variable Policy Protocol then locks
+ the same variable using the Variable Lock Protocol.
+ Both calls are expected to succeed.
+
+ @param[in] Context Unit test case context
+ **/
+UNIT_TEST_STATUS
+EFIAPI
+LockingALockedVariableShouldSucceed (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ EFI_STATUS Status;
+ VARIABLE_POLICY_ENTRY *NewEntry;
+
+ //
+ // Create a variable policy that locks the variable.
+ //
+ Status = CreateBasicVariablePolicy (
+ &mTestGuid1,
+ TEST_VAR_1_NAME,
+ TEST_POLICY_MIN_SIZE_NULL,
+ TEST_POLICY_MAX_SIZE_200,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ VARIABLE_POLICY_TYPE_LOCK_NOW,
+ &NewEntry
+ );
+ UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ //
+ // Register the new policy.
+ //
+ Status = RegisterVariablePolicy (NewEntry);
+
+ Status = VariableLockRequestToLock (NULL, TEST_VAR_1_NAME, &mTestGuid1);
+ UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ FreePool (NewEntry);
+
+ return UNIT_TEST_PASSED;
+}
+
+/**
+ Test Case that locks a variable using the Variable Policy Protocol with a
+ policy other than LOCK_NOW then attempts to lock the same variable using the
+ Variable Lock Protocol. The call to Variable Policy is expected to succced
+ and the call to Variable Lock is expected to fail.
+
+ @param[in] Context Unit test case context
+ **/
+UNIT_TEST_STATUS
+EFIAPI
+LockingAnUnlockedVariableShouldFail (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ EFI_STATUS Status;
+ VARIABLE_POLICY_ENTRY *NewEntry;
+
+ // Create a variable policy that locks the variable.
+ Status = CreateVarStateVariablePolicy (&mTestGuid1,
+ TEST_VAR_1_NAME,
+ TEST_POLICY_MIN_SIZE_NULL,
+ TEST_POLICY_MAX_SIZE_200,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ &mTestGuid2,
+ 1,
+ TEST_VAR_2_NAME,
+ &NewEntry);
+ UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ // Register the new policy.
+ Status = RegisterVariablePolicy (NewEntry);
+
+ // Configure the stub to not care about parameters. We're testing errors.
+ expect_any_always( StubGetVariableNull, VariableName );
+ expect_any_always( StubGetVariableNull, VendorGuid );
+ expect_any_always( StubGetVariableNull, DataSize );
+
+ // With a policy, make sure that writes still work, since the variable doesn't exist.
+ will_return( StubGetVariableNull, TEST_POLICY_ATTRIBUTES_NULL ); // Attributes
+ will_return( StubGetVariableNull, 0 ); // Size
+ will_return( StubGetVariableNull, NULL ); // DataPtr
+ will_return( StubGetVariableNull, EFI_NOT_FOUND); // Status
+
+ Status = VariableLockRequestToLock (NULL, TEST_VAR_1_NAME, &mTestGuid1);
+ UT_ASSERT_TRUE (EFI_ERROR (Status));
+
+ FreePool (NewEntry);
+
+ return UNIT_TEST_PASSED;
+}
+
+/**
+ Test Case that locks a variable using the Variable Policy Protocol with a
+ policy other than LOCK_NOW, but is currently locked. Then attempts to lock
+ the same variable using the Variable Lock Protocol. The call to Variable
+ Policy is expected to succced and the call to Variable Lock also expected to
+ succeed.
+
+ @param[in] Context Unit test case context
+ **/
+UNIT_TEST_STATUS
+EFIAPI
+LockingALockedVariableWithMatchingDataShouldSucceed (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ EFI_STATUS Status;
+ VARIABLE_POLICY_ENTRY *NewEntry;
+ UINT8 Data;
+
+ // Create a variable policy that locks the variable.
+ Status = CreateVarStateVariablePolicy (&mTestGuid1,
+ TEST_VAR_1_NAME,
+ TEST_POLICY_MIN_SIZE_NULL,
+ TEST_POLICY_MAX_SIZE_200,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ &mTestGuid2,
+ 1,
+ TEST_VAR_2_NAME,
+ &NewEntry);
+ UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ // Register the new policy.
+ Status = RegisterVariablePolicy (NewEntry);
+
+ // Configure the stub to not care about parameters. We're testing errors.
+ expect_any_always( StubGetVariableNull, VariableName );
+ expect_any_always( StubGetVariableNull, VendorGuid );
+ expect_any_always( StubGetVariableNull, DataSize );
+
+ // With a policy, make sure that writes still work, since the variable doesn't exist.
+ Data = 1;
+ will_return( StubGetVariableNull, TEST_POLICY_ATTRIBUTES_NULL ); // Attributes
+ will_return( StubGetVariableNull, sizeof (Data) ); // Size
+ will_return( StubGetVariableNull, &Data ); // DataPtr
+ will_return( StubGetVariableNull, EFI_SUCCESS); // Status
+
+ Status = VariableLockRequestToLock (NULL, TEST_VAR_1_NAME, &mTestGuid1);
+ UT_ASSERT_TRUE (!EFI_ERROR (Status));
+
+ FreePool (NewEntry);
+
+ return UNIT_TEST_PASSED;
+}
+
+/**
+ Test Case that locks a variable using the Variable Policy Protocol with a
+ policy other than LOCK_NOW, but variable data does not match. Then attempts
+ to lock the same variable using the Variable Lock Protocol. The call to
+ Variable Policy is expected to succced and the call to Variable Lock is
+ expected to fail.
+
+ @param[in] Context Unit test case context
+ **/
+UNIT_TEST_STATUS
+EFIAPI
+LockingALockedVariableWithNonMatchingDataShouldFail (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ EFI_STATUS Status;
+ VARIABLE_POLICY_ENTRY *NewEntry;
+ UINT8 Data;
+
+ // Create a variable policy that locks the variable.
+ Status = CreateVarStateVariablePolicy (&mTestGuid1,
+ TEST_VAR_1_NAME,
+ TEST_POLICY_MIN_SIZE_NULL,
+ TEST_POLICY_MAX_SIZE_200,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ &mTestGuid2,
+ 1,
+ TEST_VAR_2_NAME,
+ &NewEntry);
+ UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ // Register the new policy.
+ Status = RegisterVariablePolicy (NewEntry);
+
+ // Configure the stub to not care about parameters. We're testing errors.
+ expect_any_always( StubGetVariableNull, VariableName );
+ expect_any_always( StubGetVariableNull, VendorGuid );
+ expect_any_always( StubGetVariableNull, DataSize );
+
+ // With a policy, make sure that writes still work, since the variable doesn't exist.
+ Data = 2;
+ will_return( StubGetVariableNull, TEST_POLICY_ATTRIBUTES_NULL ); // Attributes
+ will_return( StubGetVariableNull, sizeof (Data) ); // Size
+ will_return( StubGetVariableNull, &Data ); // DataPtr
+ will_return( StubGetVariableNull, EFI_SUCCESS); // Status
+
+ Status = VariableLockRequestToLock (NULL, TEST_VAR_1_NAME, &mTestGuid1);
+ UT_ASSERT_TRUE (EFI_ERROR (Status));
+
+ FreePool (NewEntry);
+
+ return UNIT_TEST_PASSED;
+}
+
+/**
+ Test Case that locks a variable using Variable Lock Protocol Policy Protocol
+ then and then attempts to lock the same variable using the Variable Policy
+ Protocol. The call to Variable Lock is expected to succced and the call to
+ Variable Policy is expected to fail.
+
+ @param[in] Context Unit test case context
+ **/
+UNIT_TEST_STATUS
+EFIAPI
+SettingPolicyForALockedVariableShouldFail (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ EFI_STATUS Status;
+ VARIABLE_POLICY_ENTRY *NewEntry;
+
+ // Lock the variable.
+ Status = VariableLockRequestToLock (NULL, TEST_VAR_1_NAME, &mTestGuid1);
+ UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ // Create a variable policy that locks the variable.
+ Status = CreateVarStateVariablePolicy (&mTestGuid1,
+ TEST_VAR_1_NAME,
+ TEST_POLICY_MIN_SIZE_NULL,
+ TEST_POLICY_MAX_SIZE_200,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ TEST_POLICY_ATTRIBUTES_NULL,
+ &mTestGuid2,
+ 1,
+ TEST_VAR_2_NAME,
+ &NewEntry);
+ UT_ASSERT_NOT_EFI_ERROR (Status);
+
+ // Register the new policy.
+ Status = RegisterVariablePolicy (NewEntry);
+ UT_ASSERT_TRUE (EFI_ERROR (Status));
+
+ FreePool (NewEntry);
+
+ return UNIT_TEST_PASSED;
+}
+
+/**
+ Main entry point to this unit test application.
+
+ Sets up and runs the test suites.
+**/
+VOID
+EFIAPI
+UnitTestMain (
+ VOID
+ )
+{
+ EFI_STATUS Status;
+ UNIT_TEST_FRAMEWORK_HANDLE Framework;
+ UNIT_TEST_SUITE_HANDLE ShimTests;
+
+ Framework = NULL;
+
+ DEBUG ((DEBUG_INFO, "%a v%a\n", UNIT_TEST_NAME, UNIT_TEST_VERSION));
+
+ //
+ // Start setting up the test framework for running the tests.
+ //
+ Status = InitUnitTestFramework (&Framework, UNIT_TEST_NAME, gEfiCallerBaseName, UNIT_TEST_VERSION);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Failed in InitUnitTestFramework. Status = %r\n", Status));
+ goto EXIT;
+ }
+
+ //
+ // Add all test suites and tests.
+ //
+ Status = CreateUnitTestSuite (
+ &ShimTests, Framework,
+ "Variable Lock Shim Tests", "VarPolicy.VarLockShim", NULL, NULL
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Failed in CreateUnitTestSuite for ShimTests\n"));
+ Status = EFI_OUT_OF_RESOURCES;
+ goto EXIT;
+ }
+ AddTestCase (
+ ShimTests,
+ "Locking a variable with no matching policies should always work", "EmptyPolicies",
+ LockingWithoutAnyPoliciesShouldSucceed, LibInitMocked, LibCleanup, NULL
+ );
+ AddTestCase (
+ ShimTests,
+ "Locking a variable twice should always work", "DoubleLock",
+ LockingTwiceShouldSucceed, LibInitMocked, LibCleanup, NULL
+ );
+ AddTestCase (
+ ShimTests,
+ "Locking a variable that's already locked by another policy should work", "LockAfterPolicy",
+ LockingALockedVariableShouldSucceed, LibInitMocked, LibCleanup, NULL
+ );
+ AddTestCase (
+ ShimTests,
+ "Locking a variable that already has an unlocked policy should fail", "LockAfterUnlockedPolicy",
+ LockingAnUnlockedVariableShouldFail, LibInitMocked, LibCleanup, NULL
+ );
+ AddTestCase (
+ ShimTests,
+ "Locking a variable that already has an locked policy should succeed", "LockAfterLockedPolicyMatchingData",
+ LockingALockedVariableWithMatchingDataShouldSucceed, LibInitMocked, LibCleanup, NULL
+ );
+ AddTestCase (
+ ShimTests,
+ "Locking a variable that already has an locked policy with matching data should succeed", "LockAfterLockedPolicyNonMatchingData",
+ LockingALockedVariableWithNonMatchingDataShouldFail, LibInitMocked, LibCleanup, NULL
+ );
+ AddTestCase (
+ ShimTests,
+ "Adding a policy for a variable that has previously been locked should always fail", "SetPolicyAfterLock",
+ SettingPolicyForALockedVariableShouldFail, LibInitMocked, LibCleanup, NULL
+ );
+
+ //
+ // Execute the tests.
+ //
+ Status = RunAllTestSuites (Framework);
+
+EXIT:
+ if (Framework != NULL) {
+ FreeUnitTestFramework (Framework);
+ }
+
+ return;
+}
+
+///
+/// Avoid ECC error for function name that starts with lower case letter
+///
+#define Main main
+
+/**
+ Standard POSIX C entry point for host based unit test execution.
+
+ @param[in] Argc Number of arguments
+ @param[in] Argv Array of pointers to arguments
+
+ @retval 0 Success
+ @retval other Error
+**/
+INT32
+Main (
+ IN INT32 Argc,
+ IN CHAR8 *Argv[]
+ )
+{
+ UnitTestMain ();
+ return 0;
+}
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/VariableLockRequestToLockUnitTest.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/VariableLockRequestToLockUnitTest.inf
new file mode 100644
index 000000000000..2a659d7e1370
--- /dev/null
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/VariableLockRequestToLockUnitTest.inf
@@ -0,0 +1,36 @@
+## @file
+# This is a host-based unit test for the VariableLockRequestToLock shim.
+#
+# Copyright (c) Microsoft Corporation.
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+##
+
+[Defines]
+ INF_VERSION = 0x00010017
+ BASE_NAME = VariableLockRequestToLockUnitTest
+ FILE_GUID = A7388B6C-7274-4717-9649-BDC5DFD1FCBE
+ VERSION_STRING = 1.0
+ MODULE_TYPE = HOST_APPLICATION
+
+#
+# The following information is for reference only and not required by the build tools.
+#
+# VALID_ARCHITECTURES = IA32 X64 ARM AARCH64
+#
+
+[Sources]
+ VariableLockRequestToLockUnitTest.c
+ ../VariableLockRequestToLock.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ UnitTestFrameworkPkg/UnitTestFrameworkPkg.dec
+
+[LibraryClasses]
+ UnitTestLib
+ DebugLib
+ VariablePolicyLib
+ VariablePolicyHelperLib
+ BaseMemoryLib
+ MemoryAllocationLib
--
2.29.2.windows.2