[edk2] [Patch][edk2-platforms/devel-MinnowBoard3-UDK2017] Build Script.

[lushifex]

Change build script to generate different BIOS ID to differentiate Minnow3 and 
Benson Glacier board.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: lushifex 
---
 BuildBIOS.bat  | 19 +-
 BuildBIOS.sh   |  8 ++-
 Platform/BroxtonPlatformPkg/BuildBios.bat  | 39 ++---
 Platform/BroxtonPlatformPkg/BuildBios.sh   | 67 +++---
 Platform/BroxtonPlatformPkg/BuildIFWI.bat  | 20 +--
 Platform/BroxtonPlatformPkg/BuildIFWI.sh   |  5 ++
 .../Common/Tools/Stitch/IFWIStitch_Simple.bat  | 40 +
 7 files changed, 151 insertions(+), 47 deletions(-)

diff --git a/BuildBIOS.bat b/BuildBIOS.bat
index 3bc68e5..4bb4b03 100644
--- a/BuildBIOS.bat
+++ b/BuildBIOS.bat
@@ -11,6 +11,7 @@ set BuildTarget=Debug
 set Compiler=/vs13
 set Arch=/x64
 set FabId=/B
+set BoardId=/MN
 
 :: Optional arguments
 :OptLoop
@@ -72,6 +73,18 @@ if /i "%~1"=="/B" (
 shift
 goto OptLoop
 )
+if /i "%~1"=="/MN" (
+set BoardId=/MN
+echo.
+shift
+goto OptLoop
+)
+if /i "%~1"=="/BG" (
+set BoardId=/BG
+echo.
+shift
+goto OptLoop
+)
 
 :: Required argument(s)
 :: Require 2 input parameters
@@ -83,8 +96,8 @@ set BuildTarget=%~2
 
 :OptLoopEnd
 echo  Call Build Script of Broxton 
-echo calling : Platform\%PlatformName%PlatformPkg\BuildIFWI.bat  %Compiler% 
%Arch% %FabId% /fspw %BuildFlags% MINN %BuildTarget% 
-call Platform\%PlatformName%PlatformPkg\BuildIFWI.bat  %Compiler% %Arch% 
%FabId% /fspw %BuildFlags% MINN %BuildTarget% 
+echo calling : Platform\%PlatformName%PlatformPkg\BuildIFWI.bat  %Compiler% 
%Arch% %BoardId% %FabId% /fspw %BuildFlags% MINN %BuildTarget% 
+call Platform\%PlatformName%PlatformPkg\BuildIFWI.bat  %Compiler% %Arch% 
%BoardId% %FabId% /fspw %BuildFlags% MINN %BuildTarget% 
 
 goto Exit
 
@@ -99,6 +112,8 @@ echo/x64   Set Arch to X64  (default: X64)
 echo/IA32  Set Arch to IA32 (default: X64)
 echo/A Set FabId to A (default:  FAB_B)
 echo/B Set FabId to B (default:  FAB_B)
+echo/MNMinnow3 Board (default: MN)
+echo/BGBenson Glacier Board
 echoPlatformName:  Broxton
 echoBuildTargets:  Release, Debug
 
diff --git a/BuildBIOS.sh b/BuildBIOS.sh
index 552cf2d..aea8daa 100755
--- a/BuildBIOS.sh
+++ b/BuildBIOS.sh
@@ -18,6 +18,8 @@ function Usage () {
   echo
   echo "Usage: BuildBIOS.sh Build_Flags [PlatformName]  Target_Flag"
   echo
+  echo "   Build_Flags: /MNMinnow3 Board (default: MN)"
+  echo "   Build_Flags: /BGBenson Glacier Board"
   echo "   Build_Flags: /A Set FabId to A (default:  
FAB_B)"
   echo "   Build_Flags: /B Set FabId to B (default:  
FAB_B)"
   echo "   PlatformName [optional]: Broxton  "   
@@ -49,7 +51,11 @@ fi
 ## Build Flags
 for (( i=1; i<=$#; ))
   do
-if [ "$(echo $1 | tr 'a-z' 'A-Z')" == "/B" ]; then
+if [ "$(echo $1 | tr 'a-z' 'A-Z')" == "/BG" ]; then
+  BoardId=BG
+  Build_Flags="$Build_Flags /BG"
+  shift
+elif [ "$(echo $1 | tr 'a-z' 'A-Z')" == "/B" ]; then
   FabId=B
   Build_Flags="$Build_Flags /B"
   shift
diff --git a/Platform/BroxtonPlatformPkg/BuildBios.bat 
b/Platform/BroxtonPlatformPkg/BuildBios.bat
index 7aca176..3ac411e 100644
--- a/Platform/BroxtonPlatformPkg/BuildBios.bat
+++ b/Platform/BroxtonPlatformPkg/BuildBios.bat
@@ -25,6 +25,7 @@ set exitCode=0
 set Arch=X64
 set Compiler=VS2013
 set FabId=B
+set BoardId=MN
 if not defined BiosVersion set BiosVersion=DEV
 
 
@@ -165,6 +166,18 @@ if /i "%~1"=="/B" (
 shift
 goto OptLoop
 )
+if /i "%~1"=="/MN" (
+set BoardId=MN
+echo.
+shift
+goto OptLoop
+)
+if /i "%~1"=="/BG" (
+set BoardId=BG
+echo.
+shift
+goto OptLoop
+)
 
 :: Required argument(s)
 if "%~2"=="" (
@@ -178,7 +191,11 @@ echo. & echo -- Setting compiler to %Compiler% -- & echo.
 :: BOARD_ID needs to be exactly 7 characters (GenBiosId.exe limitation)
 echo Setting  %1  platform configuration and BIOS ID...
 if /i "%~1" == "%Minnow_RVP%" (
-set BOARD_ID=MINNOWV
+  if %BoardId%==MN (
+set BOARD_ID=MINNOW3
+  ) else if %BoardId%==BG (
+set BOARD_ID=BENSONV
+  )
 set ENBDT_PF_BUILD=TRUE
 set PLATFORM_NAME=BroxtonPlatformPkg
 set PLATFORM_PACKAGE=%PLATFORM_PATH%
@@ -216,9 +233,9 @@ if "%Arch%"=="IA32" (
 ::Stage of copy of BiosId.env in Conf/ with Platform_Type and Build_Target 
values removed
 
 if "%Arch%"=="X64" (
-findstr /b /v "BOARD_ID BUILD_TYPE VERSION_MINOR" 
%PLATFORM_PACKAGE%\BiosId.env > Conf\BiosId.env
+findstr /b /v "BOARD_ID BUILD_TYPE BOARD_REV" 
%PLATFORM_PACKAGE%\BiosId.env > Conf\BiosId.env
 ) else if "%Arch%"=="IA32" (
-findstr /b /v "BOARD_ID BUILD_TYPE VERSION_MINOR BOARD_EXT" 
%PLATFORM_PACKAGE%\BiosId.env > Conf\BiosId.env
+ 

Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled.

[Jordan Justen]

On 2017-09-13 18:17:26, Wang, Jian J wrote:
> Thanks for the comments and good advices. Sorry the format issues.
> This is my first patch for this project. Too many details for me to get 
> familiar with. 
> 
> (1) Sure.
> (2) I'll change that.
> (3) I'll use the tool to ensure the patch format.
> (4) I'll remove the ',' in name
> (5) I'll add more description about it.
> (6) You're right. I should use SetMemorySpaceAttributes() of DXE service
>  instead. The only reason I didn't do it is that I found 
>   GetMemorySpaceDescriptor() doesn't return the same information
>  which SetMemorySpaceAttributes() just changed. So I feel using CPU
> arch protocol is a bit safer. Anyway, I'll change it.
> (7) I did put those macros in the install function before. To reduce the
>  number of changed files, I made current changes. You're right it's
>  not worthy.
> (8) Using macro can help the readability, which is more important to me.
> I know function can do the same. But it looks a bit heavy in this 
> situation.

A macro can sometimes help readibility if it is doing a very common
task. I see the macros are only being used in 2 places. (Once each.)

In this case I would prefer you to just write the code all out rather
than using macros. I don't think it will make the code that much
bigger in this case, and it'll be easier to know what the code is
actually doing.

-Jordan

> I have to admit replacing  the macros with a library is a very good idea, 
>  
> which brings the same readability. I didn't think of that before. 
> Although 
> Library is still a little bit heavy to me but it's in a different way, I 
> think it 
> worth a trying.
> (9) Putting a space before open parenthesis is forced style? If so, I'll add 
> it.
> (10) You're right. Using library can reduce the disturbs to affected drivers
>by this feature to the minimum.
> 
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com] 
> Sent: Thursday, September 14, 2017 7:35 AM
> To: Wang, Jian J 
> Cc: edk2-devel@lists.01.org; Justen, Jordan L ; 
> Dong, Eric ; Kinney, Michael D 
> ; Wolman, Ayellet ; 
> Yao, Jiewen ; Zeng, Star 
> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe 
> driver to bypass NULL pointer detection if enabled.
> 
> Hi,
> 
> some of the points I'm going to make have already been pointed out by
> Jordan:
> 
> (1) When posting a patch series, please collect the Cc: tags from all of
> the patches, and add them *all* to the cover letter. This way everyone
> will get a personal copy of the general description.
> 
> 
> (2) The subject line is too long. One possible simplification:
> 
> OvmfPkg/QemuVideoDxe: bypass NULL pointer detection
> 
> 
> On 09/13/17 11:25, Wang, Jian J wrote:
> > QemuVideoDxe driver will install VBE SHIM into page 0. If NULL pointer
> > detection is enabled, page 0 must be enabled temporarily before
> > installing and disabled again afterwards. For Windows 7 boot, BIT7 of
> > PcdNullPointerDetectionPropertyMask must still be set to avoid hang.
> 
> (3) Subject line and commit message both should not exceed 74 characters
> line length. (Not sure how many chars PatchCheck.py actually enforces, I
> always stick with 74, following the Linux kernel tradition.)
> 
> I rewrapped the commit message here for readability.
> 
> 
> >
> > Cc: Jiewen Yao 
> > Cc: Eric Dong 
> > Cc: Star Zeng 
> > Cc: Laszlo Ersek 
> > Cc: Justen, Jordan L 
> > Cc: Kinney, Michael D 
> > Cc: Wolman, Ayellet 
> > Suggested-by: Wolman, Ayellet 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Wang, Jian J 
> 
> (4) I think this is also something that Jordan had pointed out a long
> time ago (apologies if I mis-remember):
> 
> The strings after the tags should form correct email addresses, and if
> there are various email meta-characters in them, like "." (dot) and ","
> (comma), then they should be quoted, like this:
> 
> Cc: "Justen, Jordan L" 
> Cc: "Kinney, Michael D" 
> Cc: "Wolman, Ayellet" 
> Suggested-by: "Wolman, Ayellet" 
> Signed-off-by: "Wang, Jian J" 
> 
> If you look at the actual addresses on the emails that have been sent
> out, you can see they are all malformed. For example, I have:
> 
> "Jordan L " for Jordan -- the part before the
> comma was taken to be a separate email address (a malformed one).
> 
> At least for my reply, I have fixed up the email addresses.
> 
> 
> (5) The 

Re: [edk2] [Patch] NetworkPkg: Remove the redundant '/' in the end of returned ISCSIMacAddr keyword.

[Wu, Jiaxin]

Reviewed-by: Wu Jiaxin 


> -Original Message-
> From: Fu, Siyuan
> Sent: Thursday, September 14, 2017 11:18 AM
> To: edk2-devel@lists.01.org
> Cc: Ye, Ting ; Wu, Jiaxin 
> Subject: [Patch] NetworkPkg: Remove the redundant '/' in the end of
> returned ISCSIMacAddr keyword.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Fu Siyuan 
> Cc: Ye Ting 
> Cc: Wu Jiaxin 
> ---
>  NetworkPkg/IScsiDxe/IScsiConfig.c | 16 ++--
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/NetworkPkg/IScsiDxe/IScsiConfig.c
> b/NetworkPkg/IScsiDxe/IScsiConfig.c
> index 498221a339..c0dd305ecf 100644
> --- a/NetworkPkg/IScsiDxe/IScsiConfig.c
> +++ b/NetworkPkg/IScsiDxe/IScsiConfig.c
> @@ -749,14 +749,15 @@
> IScsiConvertAttemptConfigDataToIfrNvDataByKeyword (
>  }
>  CopyMem(IfrNvData->ISCSIDisplayAttemptList, AttemptNameList,
> ATTEMPT_NAME_LIST_SIZE);
> 
> +ZeroMem (IfrNvData->ISCSIMacAddr, sizeof (IfrNvData->ISCSIMacAddr));
>  NET_LIST_FOR_EACH (Entry, >NicInfoList) {
>NicInfo = NET_LIST_USER_STRUCT (Entry, ISCSI_NIC_INFO, Link);
>IScsiMacAddrToStr (
> -  >PermanentAddress,
> -  NicInfo->HwAddressSize,
> -  NicInfo->VlanId,
> -  MacString
> -  );
> +>PermanentAddress,
> +NicInfo->HwAddressSize,
> +NicInfo->VlanId,
> +MacString
> +);
>CopyMem (
>  IfrNvData->ISCSIMacAddr + StrLen (IfrNvData->ISCSIMacAddr),
>  MacString,
> @@ -764,7 +765,10 @@
> IScsiConvertAttemptConfigDataToIfrNvDataByKeyword (
>  );
> 
>*(IfrNvData->ISCSIMacAddr + StrLen (IfrNvData->ISCSIMacAddr)) = L'/';
> - }
> +}
> +if (StrLen (IfrNvData->ISCSIMacAddr) != 0) {
> +  *(IfrNvData->ISCSIMacAddr + StrLen (IfrNvData->ISCSIMacAddr) - 1) =
> L'\0';
> +}
>}
>  }
> 
> --
> 2.13.0.windows.1

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

Re: [edk2] [PATCH] IntelFsp2Pkg-Tools: GenCfgOpt.py shouldn't include specific UPD name

[Yao, Jiewen]

I just got an idea on release mode checking.

Because the build.exe will parse target.txt to decide if current build is DEBUG 
 or RELEASE, I think we can enhance the GenCftOpt.py to get same information 
from target.txt.

In such way, all tools get same result on debug/release.

Thank you
Yao Jiewen

From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Yao, 
Jiewen
Sent: Thursday, September 14, 2017 10:47 AM
To: Chiu, Chasel ; edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH] IntelFsp2Pkg-Tools: GenCfgOpt.py shouldn't include 
specific UPD name

Thanks to catch this.

Reviewed-by: jiewen@intel.com

> -Original Message-
> From: Chiu, Chasel
> Sent: Thursday, September 14, 2017 10:44 AM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen >; Chiu, 
> Chasel >
> Subject: [PATCH] IntelFsp2Pkg-Tools: GenCfgOpt.py shouldn't include specific
> UPD name
>
> PcdSerialIoUartDebugEnable UPD is platform specific and should not
> be included in generic GenCfgOpt.py script. Remove this and platform
> DSC should control the default value instead.
>
> Cc: Jiewen Yao >
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Chasel Chiu 
> >
> ---
>  IntelFsp2Pkg/Tools/GenCfgOpt.py | 12 
>  1 file changed, 12 deletions(-)
>
> diff --git a/IntelFsp2Pkg/Tools/GenCfgOpt.py
> b/IntelFsp2Pkg/Tools/GenCfgOpt.py
> index 6dc1b10b34..c9b7bc5373 100644
> --- a/IntelFsp2Pkg/Tools/GenCfgOpt.py
> +++ b/IntelFsp2Pkg/Tools/GenCfgOpt.py
> @@ -289,7 +289,6 @@ class CGenCfgOpt:
>  def __init__(self):
>  self.Debug  = False
>  self.Error  = ''
> -self.ReleaseMode= True
>
>  self._GlobalDataDef = """
>  GlobalDataDef
> @@ -318,13 +317,6 @@ EndList
>  self._FvDir   = ''
>  self._MapVer  = 0
>
> -def ParseBuildMode (self, OutputStr):
> -if "RELEASE_" in OutputStr:
> -self.ReleaseMode = True
> -if "DEBUG_" in OutputStr:
> -self.ReleaseMode = False
> -return
> -
>  def ParseMacros (self, MacroDefStr):
>  # ['-DABC=1', '-D', 'CFG_DEBUG=1', '-D', 'CFG_OUTDIR=Build']
>  self._MacroDict = {}
> @@ -815,9 +807,6 @@ EndList
>
> TxtFd.write("%s.UnusedUpdSpace%d|%s0x%04X|0x%04X|{0}\n" %
> (Item['space'], SpaceIdx, Default, NextOffset - StartAddr, Offset - 
> NextOffset))
>  SpaceIdx = SpaceIdx + 1
>  NextOffset = Offset + Item['length']
> -if Item['cname'] == 'PcdSerialIoUartDebugEnable':
> -if self.ReleaseMode == False:
> -Item['value'] = 0x01
>  TxtFd.write("%s.%s|%s0x%04X|%s|%s\n" %
> (Item['space'],Item['cname'],Default,Item['offset'] -
> StartAddr,Item['length'],Item['value']))
>  TxtFd.close()
>  return 0
> @@ -1437,7 +1426,6 @@ def Main():
>  print "ERROR: Macro parsing failed !"
>  return 3
>
> -GenCfgOpt.ParseBuildMode(sys.argv[3])
>  FvDir = sys.argv[3]
>  if not os.path.exists(FvDir):
>  os.makedirs(FvDir)
> --
> 2.13.3.windows.1

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

[edk2] [Patch] NetworkPkg: Remove the redundant '/' in the end of returned ISCSIMacAddr keyword.

[Fu Siyuan]

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Fu Siyuan 
Cc: Ye Ting 
Cc: Wu Jiaxin 
---
 NetworkPkg/IScsiDxe/IScsiConfig.c | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/NetworkPkg/IScsiDxe/IScsiConfig.c 
b/NetworkPkg/IScsiDxe/IScsiConfig.c
index 498221a339..c0dd305ecf 100644
--- a/NetworkPkg/IScsiDxe/IScsiConfig.c
+++ b/NetworkPkg/IScsiDxe/IScsiConfig.c
@@ -749,14 +749,15 @@ IScsiConvertAttemptConfigDataToIfrNvDataByKeyword (
 }
 CopyMem(IfrNvData->ISCSIDisplayAttemptList, AttemptNameList, 
ATTEMPT_NAME_LIST_SIZE);
 
+ZeroMem (IfrNvData->ISCSIMacAddr, sizeof (IfrNvData->ISCSIMacAddr));
 NET_LIST_FOR_EACH (Entry, >NicInfoList) {
   NicInfo = NET_LIST_USER_STRUCT (Entry, ISCSI_NIC_INFO, Link);
   IScsiMacAddrToStr (
-  >PermanentAddress,
-  NicInfo->HwAddressSize,
-  NicInfo->VlanId,
-  MacString
-  );
+>PermanentAddress,
+NicInfo->HwAddressSize,
+NicInfo->VlanId,
+MacString
+);
   CopyMem (
 IfrNvData->ISCSIMacAddr + StrLen (IfrNvData->ISCSIMacAddr),
 MacString,
@@ -764,7 +765,10 @@ IScsiConvertAttemptConfigDataToIfrNvDataByKeyword (
 );
 
   *(IfrNvData->ISCSIMacAddr + StrLen (IfrNvData->ISCSIMacAddr)) = L'/';
- } 
+}
+if (StrLen (IfrNvData->ISCSIMacAddr) != 0) {
+  *(IfrNvData->ISCSIMacAddr + StrLen (IfrNvData->ISCSIMacAddr) - 1) = 
L'\0';
+}
   }
 }
 
-- 
2.13.0.windows.1

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

Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled.

[Wang, Jian J]

For the use of arch protocol, there's one thing I mentioned is not accurate. I 
actually tried gDS->SetMemorySpaceAttributes() but it cannot change page 
attributes. That's why I have to turn to cpu arch protocol.

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Wang, 
Jian J
Sent: Thursday, September 14, 2017 9:17 AM
To: Laszlo Ersek 
Cc: Dong, Eric ; Justen, Jordan L 
; edk2-devel@lists.01.org; Yao, Jiewen 
; Wolman, Ayellet ; Kinney, 
Michael D ; Zeng, Star 
Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe 
driver to bypass NULL pointer detection if enabled.

Thanks for the comments and good advices. Sorry the format issues.
This is my first patch for this project. Too many details for me to get 
familiar with. 

(1) Sure.
(2) I'll change that.
(3) I'll use the tool to ensure the patch format.
(4) I'll remove the ',' in name
(5) I'll add more description about it.
(6) You're right. I should use SetMemorySpaceAttributes() of DXE service
 instead. The only reason I didn't do it is that I found 
  GetMemorySpaceDescriptor() doesn't return the same information
 which SetMemorySpaceAttributes() just changed. So I feel using CPU
arch protocol is a bit safer. Anyway, I'll change it.
(7) I did put those macros in the install function before. To reduce the
 number of changed files, I made current changes. You're right it's
 not worthy.
(8) Using macro can help the readability, which is more important to me.
I know function can do the same. But it looks a bit heavy in this situation.
I have to admit replacing  the macros with a library is a very good idea,  
which brings the same readability. I didn't think of that before. Although 
Library is still a little bit heavy to me but it's in a different way, I 
think it 
worth a trying.
(9) Putting a space before open parenthesis is forced style? If so, I'll add it.
(10) You're right. Using library can reduce the disturbs to affected drivers
   by this feature to the minimum.

-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com] 
Sent: Thursday, September 14, 2017 7:35 AM
To: Wang, Jian J 
Cc: edk2-devel@lists.01.org; Justen, Jordan L ; 
Dong, Eric ; Kinney, Michael D 
; Wolman, Ayellet ; Yao, 
Jiewen ; Zeng, Star 
Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe 
driver to bypass NULL pointer detection if enabled.

Hi,

some of the points I'm going to make have already been pointed out by
Jordan:

(1) When posting a patch series, please collect the Cc: tags from all of
the patches, and add them *all* to the cover letter. This way everyone
will get a personal copy of the general description.


(2) The subject line is too long. One possible simplification:

OvmfPkg/QemuVideoDxe: bypass NULL pointer detection


On 09/13/17 11:25, Wang, Jian J wrote:
> QemuVideoDxe driver will install VBE SHIM into page 0. If NULL pointer
> detection is enabled, page 0 must be enabled temporarily before
> installing and disabled again afterwards. For Windows 7 boot, BIT7 of
> PcdNullPointerDetectionPropertyMask must still be set to avoid hang.

(3) Subject line and commit message both should not exceed 74 characters
line length. (Not sure how many chars PatchCheck.py actually enforces, I
always stick with 74, following the Linux kernel tradition.)

I rewrapped the commit message here for readability.


>
> Cc: Jiewen Yao 
> Cc: Eric Dong 
> Cc: Star Zeng 
> Cc: Laszlo Ersek 
> Cc: Justen, Jordan L 
> Cc: Kinney, Michael D 
> Cc: Wolman, Ayellet 
> Suggested-by: Wolman, Ayellet 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Wang, Jian J 

(4) I think this is also something that Jordan had pointed out a long
time ago (apologies if I mis-remember):

The strings after the tags should form correct email addresses, and if
there are various email meta-characters in them, like "." (dot) and ","
(comma), then they should be quoted, like this:

Cc: "Justen, Jordan L" 
Cc: "Kinney, Michael D" 
Cc: "Wolman, Ayellet" 
Suggested-by: "Wolman, Ayellet" 
Signed-off-by: "Wang, Jian J" 

If you look at the actual addresses on the emails that have been sent
out, you can see they are all malformed. For example, I have:

"Jordan L 

Re: [edk2] [PATCH] IntelFsp2Pkg-Tools: GenCfgOpt.py shouldn't include specific UPD name

[Yao, Jiewen]

Thanks to catch this.

Reviewed-by: jiewen@intel.com

> -Original Message-
> From: Chiu, Chasel
> Sent: Thursday, September 14, 2017 10:44 AM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen ; Chiu, Chasel 
> Subject: [PATCH] IntelFsp2Pkg-Tools: GenCfgOpt.py shouldn't include specific
> UPD name
> 
> PcdSerialIoUartDebugEnable UPD is platform specific and should not
> be included in generic GenCfgOpt.py script. Remove this and platform
> DSC should control the default value instead.
> 
> Cc: Jiewen Yao 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Chasel Chiu 
> ---
>  IntelFsp2Pkg/Tools/GenCfgOpt.py | 12 
>  1 file changed, 12 deletions(-)
> 
> diff --git a/IntelFsp2Pkg/Tools/GenCfgOpt.py
> b/IntelFsp2Pkg/Tools/GenCfgOpt.py
> index 6dc1b10b34..c9b7bc5373 100644
> --- a/IntelFsp2Pkg/Tools/GenCfgOpt.py
> +++ b/IntelFsp2Pkg/Tools/GenCfgOpt.py
> @@ -289,7 +289,6 @@ class CGenCfgOpt:
>  def __init__(self):
>  self.Debug  = False
>  self.Error  = ''
> -self.ReleaseMode= True
> 
>  self._GlobalDataDef = """
>  GlobalDataDef
> @@ -318,13 +317,6 @@ EndList
>  self._FvDir   = ''
>  self._MapVer  = 0
> 
> -def ParseBuildMode (self, OutputStr):
> -if "RELEASE_" in OutputStr:
> -self.ReleaseMode = True
> -if "DEBUG_" in OutputStr:
> -self.ReleaseMode = False
> -return
> -
>  def ParseMacros (self, MacroDefStr):
>  # ['-DABC=1', '-D', 'CFG_DEBUG=1', '-D', 'CFG_OUTDIR=Build']
>  self._MacroDict = {}
> @@ -815,9 +807,6 @@ EndList
> 
> TxtFd.write("%s.UnusedUpdSpace%d|%s0x%04X|0x%04X|{0}\n" %
> (Item['space'], SpaceIdx, Default, NextOffset - StartAddr, Offset - 
> NextOffset))
>  SpaceIdx = SpaceIdx + 1
>  NextOffset = Offset + Item['length']
> -if Item['cname'] == 'PcdSerialIoUartDebugEnable':
> -if self.ReleaseMode == False:
> -Item['value'] = 0x01
>  TxtFd.write("%s.%s|%s0x%04X|%s|%s\n" %
> (Item['space'],Item['cname'],Default,Item['offset'] -
> StartAddr,Item['length'],Item['value']))
>  TxtFd.close()
>  return 0
> @@ -1437,7 +1426,6 @@ def Main():
>  print "ERROR: Macro parsing failed !"
>  return 3
> 
> -GenCfgOpt.ParseBuildMode(sys.argv[3])
>  FvDir = sys.argv[3]
>  if not os.path.exists(FvDir):
>  os.makedirs(FvDir)
> --
> 2.13.3.windows.1

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

[edk2] [PATCH] IntelFsp2Pkg-Tools: GenCfgOpt.py shouldn't include specific UPD name

[Chasel, Chiu]

PcdSerialIoUartDebugEnable UPD is platform specific and should not
be included in generic GenCfgOpt.py script. Remove this and platform
DSC should control the default value instead.

Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Chasel Chiu 
---
 IntelFsp2Pkg/Tools/GenCfgOpt.py | 12 
 1 file changed, 12 deletions(-)

diff --git a/IntelFsp2Pkg/Tools/GenCfgOpt.py b/IntelFsp2Pkg/Tools/GenCfgOpt.py
index 6dc1b10b34..c9b7bc5373 100644
--- a/IntelFsp2Pkg/Tools/GenCfgOpt.py
+++ b/IntelFsp2Pkg/Tools/GenCfgOpt.py
@@ -289,7 +289,6 @@ class CGenCfgOpt:
 def __init__(self):
 self.Debug  = False
 self.Error  = ''
-self.ReleaseMode= True
 
 self._GlobalDataDef = """
 GlobalDataDef
@@ -318,13 +317,6 @@ EndList
 self._FvDir   = ''
 self._MapVer  = 0
 
-def ParseBuildMode (self, OutputStr):
-if "RELEASE_" in OutputStr:
-self.ReleaseMode = True
-if "DEBUG_" in OutputStr:
-self.ReleaseMode = False
-return
-
 def ParseMacros (self, MacroDefStr):
 # ['-DABC=1', '-D', 'CFG_DEBUG=1', '-D', 'CFG_OUTDIR=Build']
 self._MacroDict = {}
@@ -815,9 +807,6 @@ EndList
 TxtFd.write("%s.UnusedUpdSpace%d|%s0x%04X|0x%04X|{0}\n" % 
(Item['space'], SpaceIdx, Default, NextOffset - StartAddr, Offset - NextOffset))
 SpaceIdx = SpaceIdx + 1
 NextOffset = Offset + Item['length']
-if Item['cname'] == 'PcdSerialIoUartDebugEnable':
-if self.ReleaseMode == False:
-Item['value'] = 0x01
 TxtFd.write("%s.%s|%s0x%04X|%s|%s\n" % 
(Item['space'],Item['cname'],Default,Item['offset'] - 
StartAddr,Item['length'],Item['value']))
 TxtFd.close()
 return 0
@@ -1437,7 +1426,6 @@ def Main():
 print "ERROR: Macro parsing failed !"
 return 3
 
-GenCfgOpt.ParseBuildMode(sys.argv[3])
 FvDir = sys.argv[3]
 if not os.path.exists(FvDir):
 os.makedirs(FvDir)
-- 
2.13.3.windows.1

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

Re: [edk2] StartImage() return value an error or return code ?

[Gao, Liming]

Right. UEFI spec defines ExitData format that null terminated string + optional 
binary data. 

> -Original Message-
> From: David F. [mailto:df7...@gmail.com]
> Sent: Wednesday, September 13, 2017 12:07 PM
> To: Gao, Liming 
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] StartImage() return value an error or return code ?
> 
> But what if you're running some unknown app that you don't know the
> return code format in the ExitData?  It seems to just read that the
> ExitData will be some text followed by some binary data.
> 
> On Mon, Sep 11, 2017 at 12:43 AM, Gao, Liming  wrote:
> > Per UEFI spec, StartImage() has the optional output ExitData. If ExitData 
> > is not NULL, the exit status will be from the image.
> >
> >>-Original Message-
> >>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> >>David F.
> >>Sent: Friday, September 08, 2017 2:53 PM
> >>To: edk2-devel@lists.01.org
> >>Subject: [edk2] StartImage() return value an error or return code ?
> >>
> >>Hi,
> >>
> >>Is there a way to tell if the return code from StartImage() is
> >>actually an error from StartImage() vs the exit code of some
> >>application (generic application that may return an error code value
> >>but not set exitdata).It would seem to conflict with
> >>EFI_INVALID_PARAMETER and EFI_SECURITY_VIOLATION  so where actual
> >>value came from is unknown?
> >>
> >>TIA!!
> >>___
> >>edk2-devel mailing list
> >>edk2-devel@lists.01.org
> >>https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 1/4] MdeModulePkg/Core: Implement NULL pointer detection in EDK-II Core.

[Wang, Jian J]

Thanks for the comment. I think there's no problem here because I'm using two 
separate HOB pointers (RscDescHob and MemAllocHob) to do the traversal. One 
won't interfere with another. 

-Original Message-
From: Johnson, Brian (EXL - Eagan) [mailto:brian.john...@hpe.com] 
Sent: Thursday, September 14, 2017 12:33 AM
To: Wang, Jian J ; edk2-devel@lists.01.org
Cc: jus...@ml01.01.org; Dong, Eric ; kin...@ml01.01.org; 
Justen, Jordan L ; wol...@ml01.01.org; Yao, Jiewen 
; Wolman, Ayellet ; Kinney, 
Michael D ; Laszlo Ersek ; Zeng, 
Star 
Subject: RE: [edk2] [PATCH 1/4] MdeModulePkg/Core: Implement NULL pointer 
detection in EDK-II Core.

ClearLegacyMemory() assumes that the memory allocation HOB comes after the 
resource descriptor HOB in the HOB list.  Is that guaranteed?  I'd think that 
the memory allocation HOB traversal should be a separate loop, after the 
resource descriptor HOB traversal loop.

Other than that:
Reviewed-by: Brian J. Johnson 

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Wang, 
Jian J
Sent: Wednesday, September 13, 2017 4:25 AM
To: edk2-devel@lists.01.org
Cc: jus...@ml01.01.org; Eric Dong ; kin...@ml01.01.org; 
Jordan L ; wol...@ml01.01.org; Jiewen Yao 
; Ayellet ; Michael D 
; Laszlo Ersek ; Star Zeng 

Subject: [edk2] [PATCH 1/4] MdeModulePkg/Core: Implement NULL pointer detection 
in EDK-II Core.

The mechanism behind is to trigger a page fault exception at address 0. This 
can be made by disabling page 0 (0-4095) during page table setup. So this 
feature can only be available on platform with paging enabled. Once this 
feature is enabled, any code, like CSM, which has to access memory in page 0 
needs to enable this page temporarily in advance and disable it afterwards. 
PcdNullPointerDetectionPropertyMask is used to control and elaborate the use 
cases.

Cc: Jiewen Yao 
Cc: Eric Dong 
Cc: Star Zeng 
Cc: Laszlo Ersek 
Cc: Justen, Jordan L 
Cc: Kinney, Michael D 
Cc: Wolman, Ayellet 
Suggested-by: Wolman, Ayellet 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wang, Jian J 
---
 MdeModulePkg/Core/Dxe/DxeMain.inf|  3 +-
 MdeModulePkg/Core/Dxe/Mem/Page.c | 21 ++
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c| 47 +
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.h| 15 +++
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf  |  3 +-
 MdeModulePkg/Core/DxeIplPeim/DxeLoad.c   | 53 
 MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  8 +++-
 MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 23 ++
 MdeModulePkg/MdeModulePkg.dec| 12 ++
 10 files changed, 167 insertions(+), 20 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf 
b/MdeModulePkg/Core/Dxe/DxeMain.inf
index 30d5984f7c..273b8b7c0e 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.inf
+++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
@@ -179,7 +179,7 @@
   gEfiWatchdogTimerArchProtocolGuid ## CONSUMES
 
 [FeaturePcd]
-  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport   ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport  ## 
CONSUMES
 
 [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdLoadFixAddressBootTimeCodePageNumber## 
SOMETIMES_CONSUMES
@@ -192,6 +192,7 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable   ## 
CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy   ## 
CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy ## 
CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask## 
CONSUMES
 
 # [Hob]
 # RESOURCE_DESCRIPTOR   ## CONSUMES
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index a142c79ee2..2e0b72f864 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -170,6 +170,7 @@ CoreAddRange (
 {
   LIST_ENTRY*Link;
   MEMORY_MAP*Entry;
+  EFI_STATUSStatus;
 
   ASSERT ((Start & EFI_PAGE_MASK) == 0);
   ASSERT (End > Start) ;
@@ -188,7 +189,17 @@ CoreAddRange (
   // used for other purposes.
   //  
   if (Type == EfiConventionalMemory && Start == 0 && (End >= EFI_PAGE_SIZE - 
1)) {
-SetMem ((VOID *)(UINTN)Start, 

Re: [edk2] [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code.

[Wang, Jian J]

Thanks for the comments. See my comment start with [Jian] below.

-Original Message-
From: Johnson, Brian (EXL - Eagan) [mailto:brian.john...@hpe.com] 
Sent: Thursday, September 14, 2017 12:34 AM
To: Wang, Jian J ; edk2-devel@lists.01.org
Cc: jus...@ml01.01.org; Dong, Eric ; kin...@ml01.01.org; 
Justen, Jordan L ; wol...@ml01.01.org; Yao, Jiewen 
; Wolman, Ayellet ; Kinney, 
Michael D ; Laszlo Ersek ; Zeng, 
Star 
Subject: RE: [edk2] [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL 
pointer detection for SMM mode code.

Comments below.

Brian

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Wang, 
Jian J
Sent: Wednesday, September 13, 2017 4:25 AM
To: edk2-devel@lists.01.org
Cc: jus...@ml01.01.org; Eric Dong ; kin...@ml01.01.org; 
Jordan L ; wol...@ml01.01.org; Jiewen Yao 
; Ayellet ; Michael D 
; Laszlo Ersek ; Star Zeng 

Subject: [edk2] [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer 
detection for SMM mode code.

The mechanism behind is the same as NULL pointer detection enabled in EDK-II 
core. SMM has its own page table and we have to disable page 0 again in SMM 
mode.

Cc: Jiewen Yao 
Cc: Eric Dong 
Cc: Star Zeng 
Cc: Laszlo Ersek 
Cc: Justen, Jordan L 
Cc: Kinney, Michael D 
Cc: Wolman, Ayellet 
Suggested-by: Wolman, Ayellet 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wang, Jian J 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 11 +++
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c| 25 -
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   |  2 ++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 17 +
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c  | 11 +++
 5 files changed, 57 insertions(+), 9 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
index f295c2ebf2..d423958783 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
@@ -155,6 +155,17 @@ SmiPFHandler (
 }
   }
 
+  //
+  // If NULL pointer was just accessed
+  //
+  if (NULL_DETECTION_ENABLED && (PFAddress >= 0 && PFAddress < EFI_PAGE_SIZE)) 
{

[Brian] PFAddress is unsigned, so it will always be >= 0.  Some compilers 
complain about this  Should probably remove that part of the test.
[Jian] You're right. The first test is not necessary.

+DEBUG ((DEBUG_ERROR, "!!! NULL pointer access !!!\n"));
+DEBUG_CODE (
+  DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextIa32->Rip);
+);
+CpuDeadLoop ();
+  }
+
   if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
 SmmProfilePFHandler (
   SystemContext.SystemContextIa32->Eip,
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index f086b97c30..81c5ac9d11 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -855,10 +855,10 @@ Gen4GPageTable (
 Pte[Index] = (Index << 21) | mAddressEncMask | IA32_PG_PS | 
PAGE_ATTRIBUTE_BITS;
   }
 
+  Pdpte = (UINT64*)PageTable;
   if (FeaturePcdGet (PcdCpuSmmStackGuard)) {
 Pages = (UINTN)PageTable + EFI_PAGES_TO_SIZE (5);
 GuardPage = mSmmStackArrayBase + EFI_PAGE_SIZE;
-Pdpte = (UINT64*)PageTable;
 for (PageIndex = Low2MBoundary; PageIndex <= High2MBoundary; PageIndex += 
SIZE_2MB) {
   Pte = (UINT64*)(UINTN)(Pdpte[BitFieldRead32 ((UINT32)PageIndex, 30, 31)] 
& ~mAddressEncMask & ~(EFI_PAGE_SIZE - 1));
   Pte[BitFieldRead32 ((UINT32)PageIndex, 21, 29)] = (UINT64)Pages | 
mAddressEncMask | PAGE_ATTRIBUTE_BITS;
@@ -886,6 +886,29 @@ Gen4GPageTable (
 }
   }
 
+  if (NULL_DETECTION_ENABLED) {
+Pte = (UINT64*)(UINT64)(Pdpte[0] & ~mAddressEncMask & ~(EFI_PAGE_SIZE - 
1));

[Brian] Shouldn't the inner cast be (UINTN), not (UINT64)?  That would match 
the PcdCpuSmmStackGuard section above.
[Jian] You're right. Inner cast should be UINTN.

+if ((Pte[0] & IA32_PG_PS) == 0) {
+  // 4K-page entries are already mapped. Just hide the first one anyway.
+  Pte = (UINT64*)(UINT64)(Pte[0] & ~mAddressEncMask & ~(EFI_PAGE_SIZE - 
1));

[Brian] Same comment re. the inner cast.

+  Pte[0] &= ~1; // Hide page 0
+} else {
+  // Create 4K-page entries
+  Pages = (UINTN)AllocatePageTableMemory (1);
+  ASSERT (Pages != 0);
+
+  Pte[0] = (UINT64)(Pages | mAddressEncMask | PAGE_ATTRIBUTE_BITS);

Re: [edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

[Gao, Liming]

Right. Just initialize them. They are not reported in VS2013, because 
VS2013 and VS2015 disables this warning in ProcessorBind.h. If they are all 
false warning messages, I think we can propose to disable this warning for all 
VS tool chain. 

For GCC, if this warning is also false, could we add compiler to disable it, 
like -Wno-unused-but-set-variable.

> -Original Message-
> From: Zeng, Star
> Sent: Thursday, September 14, 2017 9:21 AM
> To: Ard Biesheuvel ; Laszlo Ersek 
> ; Gao, Liming ; Bi, Dandan
> ; Wu, Hao A 
> Cc: Ni, Ruiyu ; Dong, Eric ; 
> edk2-devel-01 
> Subject: RE: [edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect 
> compiler warning in ReadFile()
> 
> Seemingly, VS has similar issue with GCC.
> 
> VS2010/VS2012 still have the building failures below after this patch. :(
> edk2\mdemodulepkg\universal\disk\udfdxe\filesystemoperations.c(1083) : error 
> C2220: warning treated as error - no 'executable' file
> generated
> edk2\mdemodulepkg\universal\disk\udfdxe\filesystemoperations.c(1083) : 
> warning C4701: potentially uninitialized local variable
> 'FilePosition' used
> edk2\mdemodulepkg\universal\disk\udfdxe\filesystemoperations.c(1078) : 
> warning C4701: potentially uninitialized local variable
> 'FinishedSeeking' used
> edk2\mdemodulepkg\universal\disk\udfdxe\filesystemoperations.c(1167) : 
> warning C4701: potentially uninitialized local variable
> 'Data' used
> edk2\mdemodulepkg\universal\disk\udfdxe\filesystemoperations.c(1167) : 
> warning C4703: potentially uninitialized local pointer
> variable 'Data' used
> 
> 
> Liming, Dandan and Hao,
> Do you remember how we fix this kind of false alarm before?
> Just initialize the variable at the beginning of the function?
> 
> 
> 
> Thanks,
> Star
> -Original Message-
> From: Zeng, Star
> Sent: Thursday, September 14, 2017 8:43 AM
> To: Ard Biesheuvel ; Laszlo Ersek 
> 
> Cc: Ni, Ruiyu ; Dong, Eric ; 
> edk2-devel-01 ; Gao, Liming
> ; Zeng, Star 
> Subject: RE: [edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect 
> compiler warning in ReadFile()
> 
> Comparing adding workaround in code with suppressing it in *OLD* version 
> GCCs, I prefer the latter personally.
> 
> 
> Thanks,
> Star
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ard 
> Biesheuvel
> Sent: Thursday, September 14, 2017 2:52 AM
> To: Laszlo Ersek 
> Cc: Ni, Ruiyu ; Dong, Eric ; 
> edk2-devel-01 ; Gao, Liming
> ; Zeng, Star 
> Subject: Re: [edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect 
> compiler warning in ReadFile()
> 
> On 13 September 2017 at 11:49, Laszlo Ersek  wrote:
> > On 09/13/17 08:43, Zeng, Star wrote:
> >> Beyond the Rb (I do not want to block this patch series), I am curious 
> >> about one question.
> >>
> >> There may be more this kind of workarounds to fix the build failure.
> >> Is it possible to disable the warning (like below example for VS) for 
> >> specific version of GCC for this kind of false alarm?
> >>
> >>
> >> ProcessorBind.h:
> >> #if defined(_MSC_EXTENSIONS)
> >>
> >> ...
> >>
> >> #if _MSC_VER == 1800 || _MSC_VER == 1900
> >>
> >> //
> >> // Disable these warnings for VS2013.
> >> //
> >>
> >> //
> >> // This warning is for potentially uninitialized local variable, and
> >> it may cause false // positive issues in VS2013 and VS2015 build //
> >> #pragma warning ( disable : 4701 )
> >>
> >> //
> >> // This warning is for potentially uninitialized local pointer
> >> variable, and it may cause // false positive issues in VS2013 and
> >> VS2015 build // #pragma warning ( disable : 4703 )
> >>
> >> #endif
> >>
> >> #endif
> >
> > I think starting with gcc-4.6, gcc supports the "diagnostics" pragma,
> > which can be used to suppress warnings.
> >
> > Unfortunately, there's no pragma to suppress *only* the incorrect
> > warnings :) So if we set the pragma, we could lose even those warnings
> > that point out real bugs.
> >
> 
> That applies to the VS case as well. But I think doing this for older GCCs is 
> fine, most EDK2 developers use a newer version anyway, so
> we will not lose any coverage by doing so.
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 1/4] MdeModulePkg/Core: Implement NULL pointer detection in EDK-II Core.

[Wang, Jian J]

See my comments start with [Jian] below.

-Original Message-
From: Justen, Jordan L 
Sent: Thursday, September 14, 2017 1:28 AM
To: Wang, Jian J ; edk2-devel@lists.01.org
Cc: Yao, Jiewen ; Dong, Eric ; Zeng, 
Star ; Laszlo Ersek ; Kinney, Michael D 
; Wolman, Ayellet 
Subject: Re: [PATCH 1/4] MdeModulePkg/Core: Implement NULL pointer detection in 
EDK-II Core.

On 2017-09-13 02:25:04, Wang, Jian J wrote:
> The mechanism behind is to trigger a page fault exception at address 0. This 
> can be made by disabling page 0 (0-4095) during page table setup. So this 
> feature can only be available on platform with paging enabled. Once this 
> feature is enabled, any code, like CSM, which has to access memory in page 0 
> needs to enable this page temporarily in advance and disable it afterwards. 
> PcdNullPointerDetectionPropertyMask is used to control and elaborate the use 
> cases.
>

Your commit message line is > 450 columns.

https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format

I think BaseTools/Scripts/PatchCheck.py can help detect this issue.

(more below...)

[Jian] Sure.

> Cc: Jiewen Yao 
> Cc: Eric Dong 
> Cc: Star Zeng 
> Cc: Laszlo Ersek 
> Cc: Justen, Jordan L 
> Cc: Kinney, Michael D 
> Cc: Wolman, Ayellet 
> Suggested-by: Wolman, Ayellet 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Wang, Jian J 
> ---
>  MdeModulePkg/Core/Dxe/DxeMain.inf|  3 +-
>  MdeModulePkg/Core/Dxe/Mem/Page.c | 21 ++
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c| 47 +
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.h| 15 +++
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf  |  3 +-
>  MdeModulePkg/Core/DxeIplPeim/DxeLoad.c   | 53 
> 
>  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  8 +++-
>  MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 23 ++
>  MdeModulePkg/MdeModulePkg.dec| 12 ++
>  10 files changed, 167 insertions(+), 20 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf 
> b/MdeModulePkg/Core/Dxe/DxeMain.inf
> index 30d5984f7c..273b8b7c0e 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> @@ -179,7 +179,7 @@
>gEfiWatchdogTimerArchProtocolGuid ## CONSUMES
>  
>  [FeaturePcd]
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport ## 
> CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport  
> ## CONSUMES

Why is this line changed?
[Jian] Just align the comment followed because the new one added

>  
>  [Pcd]
>gEfiMdeModulePkgTokenSpaceGuid.PcdLoadFixAddressBootTimeCodePageNumber
> ## SOMETIMES_CONSUMES
> @@ -192,6 +192,7 @@
>gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable   
> ## CONSUMES
>gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy   
> ## CONSUMES
>gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy 
> ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
> ## CONSUMES
>  
>  # [Hob]
>  # RESOURCE_DESCRIPTOR   ## CONSUMES
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c 
> b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index a142c79ee2..2e0b72f864 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -170,6 +170,7 @@ CoreAddRange (
>  {
>LIST_ENTRY*Link;
>MEMORY_MAP*Entry;
> +  EFI_STATUSStatus;
>  
>ASSERT ((Start & EFI_PAGE_MASK) == 0);
>ASSERT (End > Start) ;
> @@ -188,7 +189,17 @@ CoreAddRange (
>// used for other purposes.
>//  
>if (Type == EfiConventionalMemory && Start == 0 && (End >= EFI_PAGE_SIZE - 
> 1)) {
> -SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
> +if ((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT0) == 0) {
> +  SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
> +} else if (gCpu != NULL) {
> +  Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, 0);
> +  ASSERT_EFI_ERROR(Status);
> +
> +  SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
> +
> +  Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, 
> EFI_MEMORY_RP);
> +  ASSERT_EFI_ERROR(Status);
> +}
>}
>
>//
> @@ -1972,11 +1983,3 @@ Done:
>return Status;
>  }
>  
> -
> -
> -
> -
> -
> -
> -
> -
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c 
> b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> 

Re: [edk2] [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code.

[Wang, Jian J]

I'll use the tool to check the format. For the macro, it's for readability 
purpose. How's the library replacement suggestion from Laszlo? 

-Original Message-
From: Justen, Jordan L 
Sent: Thursday, September 14, 2017 1:32 AM
To: Wang, Jian J ; edk2-devel@lists.01.org
Cc: Yao, Jiewen ; Dong, Eric ; Zeng, 
Star ; Laszlo Ersek ; Justen; Kinney; 
Kinney, Michael D ; Wolman; Wolman, Ayellet 

Subject: Re: [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer 
detection for SMM mode code.

On 2017-09-13 02:25:05, Wang, Jian J wrote:
> The mechanism behind is the same as NULL pointer detection enabled in EDK-II 
> core. SMM has its own page table and we have to disable page 0 again in SMM 
> mode.
> 
> Cc: Jiewen Yao 
> Cc: Eric Dong 
> Cc: Star Zeng 
> Cc: Laszlo Ersek 
> Cc: Justen, Jordan L 
> Cc: Kinney, Michael D 
> Cc: Wolman, Ayellet 
> Suggested-by: Wolman, Ayellet 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Wang, Jian J 
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 11 +++
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c| 25 -
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   |  2 ++
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 17 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c  | 11 +++
>  5 files changed, 57 insertions(+), 9 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> index f295c2ebf2..d423958783 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> @@ -155,6 +155,17 @@ SmiPFHandler (
>  }
>}
>  
> +  //
> +  // If NULL pointer was just accessed
> +  //
> +  if (NULL_DETECTION_ENABLED && (PFAddress >= 0 && PFAddress < 
> EFI_PAGE_SIZE)) {
> +DEBUG ((DEBUG_ERROR, "!!! NULL pointer access !!!\n"));
> +DEBUG_CODE (
> +  DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextIa32->Rip);
> +);
> +CpuDeadLoop ();
> +  }
> +
>if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
>  SmmProfilePFHandler (
>SystemContext.SystemContextIa32->Eip,
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index f086b97c30..81c5ac9d11 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -855,10 +855,10 @@ Gen4GPageTable (
>  Pte[Index] = (Index << 21) | mAddressEncMask | IA32_PG_PS | 
> PAGE_ATTRIBUTE_BITS;
>}
>  
> +  Pdpte = (UINT64*)PageTable;
>if (FeaturePcdGet (PcdCpuSmmStackGuard)) {
>  Pages = (UINTN)PageTable + EFI_PAGES_TO_SIZE (5);
>  GuardPage = mSmmStackArrayBase + EFI_PAGE_SIZE;
> -Pdpte = (UINT64*)PageTable;
>  for (PageIndex = Low2MBoundary; PageIndex <= High2MBoundary; PageIndex 
> += SIZE_2MB) {
>Pte = (UINT64*)(UINTN)(Pdpte[BitFieldRead32 ((UINT32)PageIndex, 30, 
> 31)] & ~mAddressEncMask & ~(EFI_PAGE_SIZE - 1));
>Pte[BitFieldRead32 ((UINT32)PageIndex, 21, 29)] = (UINT64)Pages | 
> mAddressEncMask | PAGE_ATTRIBUTE_BITS;
> @@ -886,6 +886,29 @@ Gen4GPageTable (
>  }
>}
>  
> +  if (NULL_DETECTION_ENABLED) {
> +Pte = (UINT64*)(UINT64)(Pdpte[0] & ~mAddressEncMask & ~(EFI_PAGE_SIZE - 
> 1));
> +if ((Pte[0] & IA32_PG_PS) == 0) {
> +  // 4K-page entries are already mapped. Just hide the first one anyway.
> +  Pte = (UINT64*)(UINT64)(Pte[0] & ~mAddressEncMask & ~(EFI_PAGE_SIZE - 
> 1));
> +  Pte[0] &= ~1; // Hide page 0
> +} else {
> +  // Create 4K-page entries
> +  Pages = (UINTN)AllocatePageTableMemory (1);
> +  ASSERT (Pages != 0);
> +
> +  Pte[0] = (UINT64)(Pages | mAddressEncMask | PAGE_ATTRIBUTE_BITS);
> +
> +  Pte = (UINT64*)Pages;
> +  PageAddress = 0;
> +  Pte[0] = PageAddress | mAddressEncMask; // Hide page 0 but present left
> +  for (Index = 1; Index < EFI_PAGE_SIZE / sizeof (*Pte); Index++) {
> +PageAddress += EFI_PAGE_SIZE;
> +Pte[Index] = PageAddress | mAddressEncMask | PAGE_ATTRIBUTE_BITS;
> +  }
> +}
> +  }
> +
>return (UINT32)(UINTN)PageTable;
>  }
>  
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 1cf85c1481..bcb3032db8 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -153,6 +153,8 @@ typedef UINT32  
> SMM_CPU_ARRIVAL_EXCEPTIONS;
>  #define ARRIVAL_EXCEPTION_DELAYED   0x2
>  #define ARRIVAL_EXCEPTION_SMI_DISABLED  0x4
>  
> +#define 

Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled.

[Wang, Jian J]

Thanks for the comments and good advices. Sorry the format issues.
This is my first patch for this project. Too many details for me to get 
familiar with. 

(1) Sure.
(2) I'll change that.
(3) I'll use the tool to ensure the patch format.
(4) I'll remove the ',' in name
(5) I'll add more description about it.
(6) You're right. I should use SetMemorySpaceAttributes() of DXE service
 instead. The only reason I didn't do it is that I found 
  GetMemorySpaceDescriptor() doesn't return the same information
 which SetMemorySpaceAttributes() just changed. So I feel using CPU
arch protocol is a bit safer. Anyway, I'll change it.
(7) I did put those macros in the install function before. To reduce the
 number of changed files, I made current changes. You're right it's
 not worthy.
(8) Using macro can help the readability, which is more important to me.
I know function can do the same. But it looks a bit heavy in this situation.
I have to admit replacing  the macros with a library is a very good idea,  
which brings the same readability. I didn't think of that before. Although 
Library is still a little bit heavy to me but it's in a different way, I 
think it 
worth a trying.
(9) Putting a space before open parenthesis is forced style? If so, I'll add it.
(10) You're right. Using library can reduce the disturbs to affected drivers
   by this feature to the minimum.

-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com] 
Sent: Thursday, September 14, 2017 7:35 AM
To: Wang, Jian J 
Cc: edk2-devel@lists.01.org; Justen, Jordan L ; 
Dong, Eric ; Kinney, Michael D 
; Wolman, Ayellet ; Yao, 
Jiewen ; Zeng, Star 
Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe 
driver to bypass NULL pointer detection if enabled.

Hi,

some of the points I'm going to make have already been pointed out by
Jordan:

(1) When posting a patch series, please collect the Cc: tags from all of
the patches, and add them *all* to the cover letter. This way everyone
will get a personal copy of the general description.


(2) The subject line is too long. One possible simplification:

OvmfPkg/QemuVideoDxe: bypass NULL pointer detection


On 09/13/17 11:25, Wang, Jian J wrote:
> QemuVideoDxe driver will install VBE SHIM into page 0. If NULL pointer
> detection is enabled, page 0 must be enabled temporarily before
> installing and disabled again afterwards. For Windows 7 boot, BIT7 of
> PcdNullPointerDetectionPropertyMask must still be set to avoid hang.

(3) Subject line and commit message both should not exceed 74 characters
line length. (Not sure how many chars PatchCheck.py actually enforces, I
always stick with 74, following the Linux kernel tradition.)

I rewrapped the commit message here for readability.


>
> Cc: Jiewen Yao 
> Cc: Eric Dong 
> Cc: Star Zeng 
> Cc: Laszlo Ersek 
> Cc: Justen, Jordan L 
> Cc: Kinney, Michael D 
> Cc: Wolman, Ayellet 
> Suggested-by: Wolman, Ayellet 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Wang, Jian J 

(4) I think this is also something that Jordan had pointed out a long
time ago (apologies if I mis-remember):

The strings after the tags should form correct email addresses, and if
there are various email meta-characters in them, like "." (dot) and ","
(comma), then they should be quoted, like this:

Cc: "Justen, Jordan L" 
Cc: "Kinney, Michael D" 
Cc: "Wolman, Ayellet" 
Suggested-by: "Wolman, Ayellet" 
Signed-off-by: "Wang, Jian J" 

If you look at the actual addresses on the emails that have been sent
out, you can see they are all malformed. For example, I have:

"Jordan L " for Jordan -- the part before the
comma was taken to be a separate email address (a malformed one).

At least for my reply, I have fixed up the email addresses.


(5) The commit message mentions BIT7 of the new PCD.

First, thanks for checking Windows 7 boot (and I'm happy that I got
suspicious of the feature with regard to Windows 7). I think BIT7 is a
good feature.

However, please include the short description of that feature in the
commit message -- it is one sentence; "Disable NULL pointer detection
just after EndOfDxe."

(I think BIT7 is a really smart feature, and I like *how* it is used in
"NULL_DETECTION_ENABLED" below. The check means, "if the protection is
enabled for DXE, and *not disabled* (== also enabled) after End-of-Dxe".

This doesn't mean that I like the 

[edk2] EDK2 + VT-x

[Patchmail]

I cannot find anywhere how to enable VT-x in OVMF, if anyone can point me
in the right direction, I'd greatly appreciate it.

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

Re: [edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

[Zeng, Star]

Comparing adding workaround in code with suppressing it in *OLD* version GCCs, 
I prefer the latter personally.


Thanks,
Star
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ard 
Biesheuvel
Sent: Thursday, September 14, 2017 2:52 AM
To: Laszlo Ersek 
Cc: Ni, Ruiyu ; Dong, Eric ; 
edk2-devel-01 ; Gao, Liming ; 
Zeng, Star 
Subject: Re: [edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect 
compiler warning in ReadFile()

On 13 September 2017 at 11:49, Laszlo Ersek  wrote:
> On 09/13/17 08:43, Zeng, Star wrote:
>> Beyond the Rb (I do not want to block this patch series), I am curious about 
>> one question.
>>
>> There may be more this kind of workarounds to fix the build failure.
>> Is it possible to disable the warning (like below example for VS) for 
>> specific version of GCC for this kind of false alarm?
>>
>>
>> ProcessorBind.h:
>> #if defined(_MSC_EXTENSIONS)
>>
>> ...
>>
>> #if _MSC_VER == 1800 || _MSC_VER == 1900
>>
>> //
>> // Disable these warnings for VS2013.
>> //
>>
>> //
>> // This warning is for potentially uninitialized local variable, and 
>> it may cause false // positive issues in VS2013 and VS2015 build // 
>> #pragma warning ( disable : 4701 )
>>
>> //
>> // This warning is for potentially uninitialized local pointer 
>> variable, and it may cause // false positive issues in VS2013 and 
>> VS2015 build // #pragma warning ( disable : 4703 )
>>
>> #endif
>>
>> #endif
>
> I think starting with gcc-4.6, gcc supports the "diagnostics" pragma, 
> which can be used to suppress warnings.
>
> Unfortunately, there's no pragma to suppress *only* the incorrect 
> warnings :) So if we set the pragma, we could lose even those warnings 
> that point out real bugs.
>

That applies to the VS case as well. But I think doing this for older GCCs is 
fine, most EDK2 developers use a newer version anyway, so we will not lose any 
coverage by doing so.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled.

[Laszlo Ersek]

Hi,

some of the points I'm going to make have already been pointed out by
Jordan:

(1) When posting a patch series, please collect the Cc: tags from all of
the patches, and add them *all* to the cover letter. This way everyone
will get a personal copy of the general description.


(2) The subject line is too long. One possible simplification:

OvmfPkg/QemuVideoDxe: bypass NULL pointer detection


On 09/13/17 11:25, Wang, Jian J wrote:
> QemuVideoDxe driver will install VBE SHIM into page 0. If NULL pointer
> detection is enabled, page 0 must be enabled temporarily before
> installing and disabled again afterwards. For Windows 7 boot, BIT7 of
> PcdNullPointerDetectionPropertyMask must still be set to avoid hang.

(3) Subject line and commit message both should not exceed 74 characters
line length. (Not sure how many chars PatchCheck.py actually enforces, I
always stick with 74, following the Linux kernel tradition.)

I rewrapped the commit message here for readability.


>
> Cc: Jiewen Yao 
> Cc: Eric Dong 
> Cc: Star Zeng 
> Cc: Laszlo Ersek 
> Cc: Justen, Jordan L 
> Cc: Kinney, Michael D 
> Cc: Wolman, Ayellet 
> Suggested-by: Wolman, Ayellet 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Wang, Jian J 

(4) I think this is also something that Jordan had pointed out a long
time ago (apologies if I mis-remember):

The strings after the tags should form correct email addresses, and if
there are various email meta-characters in them, like "." (dot) and ","
(comma), then they should be quoted, like this:

Cc: "Justen, Jordan L" 
Cc: "Kinney, Michael D" 
Cc: "Wolman, Ayellet" 
Suggested-by: "Wolman, Ayellet" 
Signed-off-by: "Wang, Jian J" 

If you look at the actual addresses on the emails that have been sent
out, you can see they are all malformed. For example, I have:

"Jordan L " for Jordan -- the part before the
comma was taken to be a separate email address (a malformed one).

At least for my reply, I have fixed up the email addresses.


(5) The commit message mentions BIT7 of the new PCD.

First, thanks for checking Windows 7 boot (and I'm happy that I got
suspicious of the feature with regard to Windows 7). I think BIT7 is a
good feature.

However, please include the short description of that feature in the
commit message -- it is one sentence; "Disable NULL pointer detection
just after EndOfDxe."

(I think BIT7 is a really smart feature, and I like *how* it is used in
"NULL_DETECTION_ENABLED" below. The check means, "if the protection is
enabled for DXE, and *not disabled* (== also enabled) after End-of-Dxe".

This doesn't mean that I like the NULL_DETECTION_ENABLED macro itself;
more on that below.)


> ---
>  OvmfPkg/QemuVideoDxe/Driver.c | 15 ++-
>  OvmfPkg/QemuVideoDxe/Qemu.h   | 16 
>  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf |  2 ++
>  3 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
> index 0dce80e59b..ee0eed7214 100644
> --- a/OvmfPkg/QemuVideoDxe/Driver.c
> +++ b/OvmfPkg/QemuVideoDxe/Driver.c
> @@ -194,6 +194,7 @@ QemuVideoControllerDriverStart (
>PCI_TYPE00Pci;
>QEMU_VIDEO_CARD   *Card;
>EFI_PCI_IO_PROTOCOL   *ChildPciIo;
> +  EFI_CPU_ARCH_PROTOCOL *Cpu;

(6) I believe I mentioned this in the earlier discussion, in some form,
but now I'll say it again:

A UEFI driver has no business poking at the CPU Arch protocol. The PI
spec (1.6) states,

  12.3 CPU Architectural Protocol
  EFI_CPU_ARCH_PROTOCOL

  Summary

  Abstracts the processor services that are required to implement some
  of the DXE services. This protocol must be produced by a boot service
  or runtime DXE driver and may only be consumed by the DXE Foundation
  and DXE drivers that produce architectural protocols.

The DXE core is obviously free to use the CPU Arch protocol, but a UEFI
driver is forbidden from it, *even if* we say that, in this UEFI driver,
we are going to use DXE services. (Which come from the PI spec, and not
the UEFI spec.)

Therefore, here we have to use gDS->SetMemorySpaceAttributes().

The gDS->SetMemorySpaceAttributes() service depends on the CPU Arch
protocol, by the PI spec. It is not easy to see, because the PI spec has
a formatting error in this area. If you look under
GetMemorySpaceDescriptor(), there is an error code

  EFI_NOT_AVAILABLE_YET  The attributes cannot be set because CPU
 architectural protocol is not available yet.

Obviously this error code belongs to SetMemorySpaceAttributes(), 

Re: [edk2] [PATCH 0/2] MdeModulePkg: UDF fixes and cleanups, round 2

[Laszlo Ersek]

On 09/13/17 15:42, Paulo Alcantara wrote:
> 
> 
> On September 12, 2017 7:26:10 PM GMT-03:00, Laszlo Ersek  
> wrote:
>> Repo:   https://github.com/lersek/edk2.git
>> Branch: udf_fixes_cleanups_round2
>>
>> Once these patches are sufficiently reviewed, please don't wait for me
>> to commit them.
>>
>> Further UdfDxe issues should be please reported in the TianoCore
>> Bugzilla.
>>
>> Cc: Ard Biesheuvel 
>> Cc: Eric Dong 
>> Cc: Paulo Alcantara 
>> Cc: Ruiyu Ni 
>> Cc: Star Zeng 
>>
>> Thanks
>> Laszlo
>>
>> Laszlo Ersek (2):
>>  MdeModulePkg/UdfDxe: reject reserved values in ICB.Flags[2:0]
>> MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()
>>
>> MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 13
>> +
>> 1 file changed, 13 insertions(+)
> 
> Reviewed-by: Paulo Alcantara 

Thanks Everyone, pushed as c3246da7bf0a..5afa5b815936.

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

Re: [edk2] [Patch] AppPkg/WebServer: Fix build failure.

[Laszlo Ersek]

On 09/13/17 12:09, Eric Dong wrote:
> Fix build failure caused by UefiCpuPkg/MtrrLib removes deprecated macros.
> 
> Cc: Daryl McDaniel 
> Cc: Jaben Carsey 
> Cc: Ruiyu Ni 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong 
> ---
>  AppPkg/Applications/Sockets/WebServer/Mtrr.c  | 31 
> +++
>  AppPkg/Applications/Sockets/WebServer/WebServer.h |  1 +
>  2 files changed, 16 insertions(+), 16 deletions(-)

Thank you very much for fixing this, Eric!

Before you push the patch, can you please update the commit message so
that it names the following BZ?

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

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

Re: [edk2] [PATCH v2 7/8] OvmfPkg/VirtioNetDxe: map caller-supplied Tx packet to device-address

[Laszlo Ersek]

On 09/11/17 14:16, Brijesh Singh wrote:
> When device is behind the IOMMU, driver is require to pass the device
> address of caller-supplied transmit buffer for the bus master operations.
> 
> The patch uses VirtioNetMapTxBuf() to map caller-supplied Tx packet to a
> device-address and enqueue the device address in VRING for transfer and
> perform the reverse mapping when transfer is completed so that we can
> return the caller-supplied buffer.
> 
> Cc: Ard Biesheuvel 
> Cc: Jordan Justen 
> Cc: Tom Lendacky 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh 
> ---
>  OvmfPkg/VirtioNetDxe/SnpGetStatus.c | 30 +
>  OvmfPkg/VirtioNetDxe/SnpTransmit.c  | 34 
>  2 files changed, 50 insertions(+), 14 deletions(-)
> 
> diff --git a/OvmfPkg/VirtioNetDxe/SnpGetStatus.c 
> b/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
> index 694940ea1d97..9bd62fbe8ec0 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
> @@ -61,11 +61,12 @@ VirtioNetGetStatus (
>OUT VOID   **TxBuf OPTIONAL
>)
>  {
> -  VNET_DEV   *Dev;
> -  EFI_TPLOldTpl;
> -  EFI_STATUS Status;
> -  UINT16 RxCurUsed;
> -  UINT16 TxCurUsed;
> +  VNET_DEV *Dev;
> +  EFI_TPL  OldTpl;
> +  EFI_STATUS   Status;
> +  UINT16   RxCurUsed;
> +  UINT16   TxCurUsed;
> +  EFI_PHYSICAL_ADDRESS DeviceAddress;
>  
>if (This == NULL) {
>  return EFI_INVALID_PARAMETER;
> @@ -141,9 +142,24 @@ VirtioNetGetStatus (
>ASSERT (DescIdx < (UINT32) (2 * Dev->TxMaxPending - 1));
>  
>//
> -  // report buffer address to caller that has been enqueued by caller
> +  // get the device address to caller that has been enqueued by caller

(1) I suggest, "get the device address that has been enqueued for the
caller's transmit buffer".

>//
> -  *TxBuf = (VOID *)(UINTN) Dev->TxRing.Desc[DescIdx + 1].Addr;
> +  DeviceAddress = Dev->TxRing.Desc[DescIdx + 1].Addr;
> +
> +  //
> +  // Unmap the device address and perform the reverse mapping to find the
> +  // caller buffer address.
> +  //
> +  Status = VirtioNetUnmapTxBuf (
> + Dev,
> + DescIdx + 1,

(2) As discussed, this argument should go away.

> + TxBuf,
> + DeviceAddress
> + );
> +  if (EFI_ERROR (Status)) {
> +Status = EFI_DEVICE_ERROR;
> +goto Exit;
> +  }

Now, normally I would request the following: "Because you are
introducing a new error path here, it is now possible that
*InterruptStatus is modified earlier, but we exit with and error. Please
introduce a local variable for InterruptStatus, and only assign
*InterruptStatus when everything is fine."

However, VirtioNetUnmapTxBuf() should never fail. It can only return an
error if "DeviceAddress is not mapped", and that means our internal
state has been corrupted somehow.

(3) Therefore, please add such a comment, plus an "ASSERT (FALSE)" right
above the "Status = EFI_DEVICE_ERROR" assignment.


>  
>//
>// now this descriptor can be used again to enqueue a transmit buffer

(4) Please move the VirtioNetUnmapTxBuf() call *under* the TxFreeStack
manipulation. So that the order of operations is ultimately:

(4a) fetch DeviceAddress

(4b) put DescIdx back on TxFreeStack

(4c) call VirtioNetUnmapTxBuf() as final operation (followed by the
error check + ASSERT, as discussed above)


> diff --git a/OvmfPkg/VirtioNetDxe/SnpTransmit.c 
> b/OvmfPkg/VirtioNetDxe/SnpTransmit.c
> index 7ca40d5d0650..0be3243b4606 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpTransmit.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpTransmit.c
> @@ -73,11 +73,12 @@ VirtioNetTransmit (
>IN UINT16  *Protocol OPTIONAL
>)
>  {
> -  VNET_DEV   *Dev;
> -  EFI_TPLOldTpl;
> -  EFI_STATUS Status;
> -  UINT16 DescIdx;
> -  UINT16 AvailIdx;
> +  VNET_DEV  *Dev;
> +  EFI_TPL   OldTpl;
> +  EFI_STATUSStatus;
> +  UINT16DescIdx;
> +  UINT16AvailIdx;
> +  EFI_PHYSICAL_ADDRESS  DeviceAddress;
>  
>if (This == NULL || BufferSize == 0 || Buffer == NULL) {
>  return EFI_INVALID_PARAMETER;
> @@ -144,10 +145,29 @@ VirtioNetTransmit (
>}
>  
>//
> -  // virtio-0.9.5, 2.4.1 Supplying Buffers to The Device
> +  // Get the free VRING descriptor
>//
>DescIdx = Dev->TxFreeStack[Dev->TxCurPending++];
> -  Dev->TxRing.Desc[DescIdx + 1].Addr  = (UINTN) Buffer;
> +
> +  //
> +  // Map the transmit buffer system physical address to device address.
> +  //
> +  Status = VirtioNetMapTxBuf (
> + Dev,
> + DescIdx + 1,

(5) this argument is going away


> + Buffer,
> + 

Re: [edk2] [PATCH v2 6/8] OvmfPkg/VirtioNetDxe: add Tx packet map/unmap helper functions

[Laszlo Ersek]

On 09/13/17 20:07, Laszlo Ersek wrote:
> On 09/11/17 14:16, Brijesh Singh wrote:

>> +*/
>> +EFI_STATUS
>> +EFIAPI
>> +VirtioNetMapTxBuf (
>> +  IN  VNET_DEV  *Dev,
>> +  IN  UINT16DescIdx,
>> +  IN  VOID  *Buffer,
>> +  IN  UINTN NumberOfBytes,
>> +  OUT EFI_PHYSICAL_ADDRESS  *DeviceAddress
>> +  )
>> +{
>> +  EFI_STATUSStatus;
>> +  TX_BUF_MAP_INFO   *TxBufMapInfo;
>> +  EFI_PHYSICAL_ADDRESS  Address;
>> +  VOID  *Mapping;
>> +  ORDERED_COLLECTION_ENTRY  *Entry;
>> +
>> +  TxBufMapInfo = AllocatePool (sizeof (*TxBufMapInfo));
>> +  if (TxBufMapInfo == NULL) {
>> +return EFI_OUT_OF_RESOURCES;
>> +  }
>> +
>> +  Status = VirtioMapAllBytesInSharedBuffer (
>> + Dev->VirtIo,
>> + VirtioOperationBusMasterRead,
>> + Buffer,
>> + NumberOfBytes,
>> + ,
>> + 
>> +);
>> +  if (EFI_ERROR (Status)) {
>> +goto FreeTxBufMapInfo;
>> +  }
>> +
>> +  TxBufMapInfo->DescIdx = DescIdx;
>> +  TxBufMapInfo->Buffer = Buffer;
>> +  TxBufMapInfo->DeviceAddress = Address;
>> +  TxBufMapInfo->BufMap = Mapping;
>> +
>> +  Status = OrderedCollectionInsert (
>> + Dev->TxBufMapInfoCollection,
>> + ,
>> + TxBufMapInfo
>> + );
>> +  switch (Status) {
>> +  case RETURN_OUT_OF_RESOURCES:
>> +Status = EFI_OUT_OF_RESOURCES;
>> +goto UnmapTxBufBuffer;
>> +  case RETURN_ALREADY_STARTED:
>> +Status = EFI_INVALID_PARAMETER;
>> +goto UnmapTxBufBuffer;
>> +  default:
>> +ASSERT (Status == RETURN_SUCCESS);
>> +break;
>> +  }
>
> (14) Given that, in v3, the ordering key will be
> "TX_BUF_MAP_INFO.DeviceAddress", the Status check after
> OrderedCollectionInsert() should work like this (i.e., replace the
> "switch" with the following):
>
>   if (Status == EFI_OUT_OF_RESOURCES) {
> goto UnmapTxBufBuffer;
>   }
>   ASSERT_EFI_ERROR (Status);
>
> In other words, ALREADY_STARTED should *never* be returned, because
> the key comes from VirtioMapAllBytesInSharedBuffer(), and should be
> unique. If there is a conflict, then the breakage is so serious that
> we cannot do anything about it.

I'd like to elaborate on my above comment.

Let's consider what happens when client code calls SNP.Transmit()
several times, in quick succession, using the *exact same* Buffer
argument -- for queueing the same packet several times, for whatever
reason --, *AND* we are using a virtio protocol implementation that
identity-maps the packets.

That means ALREADY_STARTED *will* be returned, because DeviceAddress
will not be unique.

The question is: is this a valid use of SNP.Transmit(), so that we must
accommodate it?

In order to answer this, let's look at the SNP.GetStatus() interface.
SNP.GetStatus() reports transmit completion by returning the original
TxBuf address. From the UEFI-2.7 spec, "EFI_SIMPLE_NETWORK.GetStatus()":

If TxBuf is not NULL, a recycled transmit buffer address will be
retrieved. If a recycled transmit buffer address is returned in
TxBuf, then the buffer has been successfully transmitted, and the
status for that buffer is cleared.

It is clear that the transmit buffer address shall uniquely identify the
transmit buffer; and that given a transmit buffer address, there is
exactly *one status* for that transmit buffer / transmit buffer address.

Therefore the use pattern I described above is invalid.

However, to be on the safe side, even in RELEASE builds, I suggest that
we keep your original error handling code, with the following
modification (note that I'm replacing RETURN_ with EFI_, because we're
already investigating an EFI_STATUS variable):


  switch (Status) {
  case EFI_OUT_OF_RESOURCES:
goto UnmapTxBufBuffer;
  case EFI_ALREADY_STARTED:
//
// This should never happen: it implies
//
// - an identity-mapping VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer()
//   implementation -- which is fine,
//
// - and an SNP client that queues multiple instances of the exact same
//   buffer address with SNP.Transmit() -- which is undefined behavior,
//   based on the TxBuf language in UEFI-2.7,
//   EFI_SIMPLE_NETWORK.GetStatus().
//
ASSERT (FALSE);
Status = EFI_INVALID_PARAMETER;
goto UnmapTxBufBuffer;
  default:
ASSERT_EFI_ERROR (Status);
break;
  }


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

Re: [edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

[Ard Biesheuvel]

On 13 September 2017 at 11:49, Laszlo Ersek  wrote:
> On 09/13/17 08:43, Zeng, Star wrote:
>> Beyond the Rb (I do not want to block this patch series), I am curious about 
>> one question.
>>
>> There may be more this kind of workarounds to fix the build failure.
>> Is it possible to disable the warning (like below example for VS) for 
>> specific version of GCC for this kind of false alarm?
>>
>>
>> ProcessorBind.h:
>> #if defined(_MSC_EXTENSIONS)
>>
>> ...
>>
>> #if _MSC_VER == 1800 || _MSC_VER == 1900
>>
>> //
>> // Disable these warnings for VS2013.
>> //
>>
>> //
>> // This warning is for potentially uninitialized local variable, and it may 
>> cause false
>> // positive issues in VS2013 and VS2015 build
>> //
>> #pragma warning ( disable : 4701 )
>>
>> //
>> // This warning is for potentially uninitialized local pointer variable, and 
>> it may cause
>> // false positive issues in VS2013 and VS2015 build
>> //
>> #pragma warning ( disable : 4703 )
>>
>> #endif
>>
>> #endif
>
> I think starting with gcc-4.6, gcc supports the "diagnostics" pragma,
> which can be used to suppress warnings.
>
> Unfortunately, there's no pragma to suppress *only* the incorrect
> warnings :) So if we set the pragma, we could lose even those warnings
> that point out real bugs.
>

That applies to the VS case as well. But I think doing this for older
GCCs is fine, most EDK2 developers use a newer version anyway, so we
will not lose any coverage by doing so.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

[Laszlo Ersek]

On 09/13/17 08:43, Zeng, Star wrote:
> Beyond the Rb (I do not want to block this patch series), I am curious about 
> one question.
> 
> There may be more this kind of workarounds to fix the build failure.
> Is it possible to disable the warning (like below example for VS) for 
> specific version of GCC for this kind of false alarm?
> 
> 
> ProcessorBind.h:
> #if defined(_MSC_EXTENSIONS)
> 
> ...
> 
> #if _MSC_VER == 1800 || _MSC_VER == 1900
> 
> //
> // Disable these warnings for VS2013.
> //
> 
> //
> // This warning is for potentially uninitialized local variable, and it may 
> cause false 
> // positive issues in VS2013 and VS2015 build
> //
> #pragma warning ( disable : 4701 )
>   
> //
> // This warning is for potentially uninitialized local pointer variable, and 
> it may cause 
> // false positive issues in VS2013 and VS2015 build
> //
> #pragma warning ( disable : 4703 )
>   
> #endif
> 
> #endif

I think starting with gcc-4.6, gcc supports the "diagnostics" pragma,
which can be used to suppress warnings.

Unfortunately, there's no pragma to suppress *only* the incorrect
warnings :) So if we set the pragma, we could lose even those warnings
that point out real bugs.

Thanks
Laszlo

> 
> 
> Thanks,
> Star
> -Original Message-
> From: Zeng, Star 
> Sent: Wednesday, September 13, 2017 2:34 PM
> To: Laszlo Ersek ; edk2-devel-01 
> Cc: Ard Biesheuvel ; Dong, Eric 
> ; Paulo Alcantara ; Ni, Ruiyu 
> ; Zeng, Star 
> Subject: RE: [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler 
> warning in ReadFile()
> 
> Reviewed-by: Star Zeng 
> 
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Wednesday, September 13, 2017 6:26 AM
> To: edk2-devel-01 
> Cc: Ard Biesheuvel ; Dong, Eric 
> ; Paulo Alcantara ; Ni, Ruiyu 
> ; Zeng, Star 
> Subject: [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning 
> in ReadFile()
> 
> When building the driver for DEBUG/RELEASE, GCC48/GCC49 warn about
> ReadFile() possibly using "BytesLeft" without initializing it first.
> 
> This is not the case. The reads of "BytesLeft" are only reachable if 
> (ReadFileInfo->Flags == READ_FILE_SEEK_AND_READ). But, in that case, we also 
> set "BytesLeft" to "ReadFileInfo->FileDataSize", near the top of the function.
> 
> Assign "BytesLeft" zero at the top, and add a comment that conforms to the 
> pending Coding Style Spec feature request at 
> .
> 
> This issue was reported by Ard's and Gerd's CI systems independently.
> 
> Cc: Ard Biesheuvel 
> Cc: Eric Dong 
> Cc: Paulo Alcantara 
> Cc: Ruiyu Ni 
> Cc: Star Zeng 
> Reported-by: Ard Biesheuvel 
> Reported-by: Gerd Hoffmann 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek 
> ---
>  MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c 
> b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> index 096fbb4452cb..392494b2eb3f 100644
> --- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> @@ -893,6 +893,11 @@ ReadFile (
>LogicalBlockSize  = LV_BLOCK_SIZE (Volume, UDF_DEFAULT_LV_NUM);
>DoFreeAed = FALSE;
>  
> +  //
> +  // set BytesLeft to suppress incorrect compiler/analyzer warnings  // 
> + BytesLeft = 0;
> +
>switch (ReadFileInfo->Flags) {
>case READ_FILE_GET_FILESIZE:
>case READ_FILE_ALLOCATE_AND_READ:
> --
> 2.14.1.3.gb7cf6e02401b
> 

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

[edk2] [PATCH 1/2] ArmPlatformPkg: Juno networking build option

[evan . lloyd]

From: Sami 

When network functionality is not required, the boot process is
impeded by redundant network timeouts. Moreover Juno is a mobile
platform so it makes sense to have an option to disable the
(ethernet) networking support.  We therefore introduce the
DISABLE_NETWORK build option.

By default ArmJunoDxe configures the MAC address. This is redundant
when networking is disabled, so the MAC Address configuration is
removed when the DISABLE_NETWORK build option is defined.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Sami Mujawar 
Signed-off-by: Evan Lloyd 
---
 ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c 
b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
index 
18491c7378523f365644658c270de95e711c5ac1..456e21ba47db7ec440ac1ef5554eccd5e4d2bcf9
 100644
--- a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
+++ b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
@@ -1,6 +1,6 @@
 /** @file
 *
-*  Copyright (c) 2013-2015, ARM Limited. All rights reserved.
+*  Copyright (c) 2013-2017, ARM Limited. All rights reserved.
 *
 *  This program and the accompanying materials
 *  are licensed and made available under the terms and conditions of the BSD 
License
@@ -71,6 +71,7 @@ STATIC CONST EFI_PCI_ROOT_BRIDGE_DEVICE_PATH 
mPciRootComplexDevicePath = {
 
 EFI_EVENT mAcpiRegistration = NULL;
 
+#ifndef DISABLE_NETWORK
 /**
   This function reads PCI ID of the controller.
 
@@ -355,6 +356,7 @@ ArmJunoSetNicMacAddress ()
 
   return EFI_SUCCESS;
 }
+#endif
 
 /**
   Notification function of the event defined as belonging to the
@@ -395,10 +397,12 @@ OnEndOfDxe (
   Status = gBS->ConnectController (Handle, NULL, PciRootComplexDevicePath, 
FALSE);
   ASSERT_EFI_ERROR (Status);
 
+#ifndef DISABLE_NETWORK
   Status = ArmJunoSetNicMacAddress ();
   if (EFI_ERROR (Status)) {
 DEBUG ((DEBUG_ERROR, "ArmJunoDxe: Failed to set Marvell Yukon NIC MAC 
address\n"));
   }
+#endif
 }
 
 EFI_STATUS
-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

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

[edk2] [PATCH 2/2] ArmPlatformPkg: Remove DT code when unsupported

[evan . lloyd]

From: Sami Mujawar 

>From the UEFI perspective Device Tree is a non standard extra.
There is, thus, an existing build flag DT_SUPPORTED.
This change removes Device Tree specific code unless DT_SUPPORTED is
set.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Sami Mujawar 
Signed-off-by: Evan Lloyd 
---
 ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c 
b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
index 
456e21ba47db7ec440ac1ef5554eccd5e4d2bcf9..893dd80b6755d68a05b8c2b4dedb8f60401b503b
 100644
--- a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
+++ b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
@@ -414,9 +414,11 @@ ArmJunoEntryPoint (
 {
   EFI_STATUSStatus;
   EFI_PHYSICAL_ADDRESS  HypBase;
+#ifdef DT_SUPPORTED
   CHAR16*TextDevicePath;
   UINTN TextDevicePathSize;
   VOID  *Buffer;
+#endif
   UINT32JunoRevision;
   EFI_EVENT EndOfDxeEvent;
 
@@ -530,6 +532,7 @@ ArmJunoEntryPoint (
 );
   }
 
+#ifdef DT_SUPPORTED
   //
   // Set up the device path to the FDT.
   //
@@ -549,6 +552,7 @@ ArmJunoEntryPoint (
   );
 return Status;
   }
+#endif
 
   return Status;
 }
-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

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

[edk2] [PATCH 0/2] Options to remove code

[evan . lloyd]

From: EvanLloyd 

There are a number of cases where it is desirable to remove unused code.
An obvious instance might be on an emulator, where executing irrelevant
code can add hours to the boot time.  This change enables removal of
some code that might not be relevant, using build options that can be
configured in the platform build.

Sami (1):
  ArmPlatformPkg: Juno networking build option

Sami Mujawar (1):
  ArmPlatformPkg: Remove DT code when unsupported

 ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

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

Re: [edk2] [PATCH v2 6/8] OvmfPkg/VirtioNetDxe: add Tx packet map/unmap helper functions

[Laszlo Ersek]

On 09/11/17 14:16, Brijesh Singh wrote:
> When device is behind IOMMU, driver is require to pass the device address
> of TxBuf in the Tx VRING. The patch adds helper functions and data
> structure to map and unmap the TxBuf system physical address to a device
> address.
>
> Since the TxBuf is returned back to caller from VirtioNetGetStatus() hence
> we use OrderedCollection interface to save the TxBuf system physical to
> device address mapping. After the TxBuf is succesfully transmitted
> VirtioNetUnmapTxBuf() does the reverse lookup in OrderedCollection data
> structure to get the system physical address of TxBuf for a given device
> address.
>
> Cc: Ard Biesheuvel 
> Cc: Jordan Justen 
> Cc: Tom Lendacky 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh 
> ---
>  OvmfPkg/VirtioNetDxe/VirtioNet.inf  |   1 +
>  OvmfPkg/VirtioNetDxe/VirtioNet.h|  32 
>  OvmfPkg/VirtioNetDxe/SnpInitialize.c|  15 +-
>  OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 188 
>  4 files changed, 235 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.inf 
> b/OvmfPkg/VirtioNetDxe/VirtioNet.inf
> index a855ad4ac154..9ff6d87e6190 100644
> --- a/OvmfPkg/VirtioNetDxe/VirtioNet.inf
> +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.inf
> @@ -49,6 +49,7 @@
>DebugLib
>DevicePathLib
>MemoryAllocationLib
> +  OrderedCollectionLib
>UefiBootServicesTableLib
>UefiDriverEntryPoint
>UefiLib
> diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h 
> b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> index 3f48bcc6b67c..906bec8e88f3 100644
> --- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
> +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> @@ -26,6 +26,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #define VNET_SIG SIGNATURE_32 ('V', 'N', 'E', 'T')
>
> @@ -100,6 +101,7 @@ typedef struct {
>VOID*TxSharedReqMap;   // VirtioNetInitTx
>UINT16  TxLastUsed;// VirtioNetInitTx
>EFI_PHYSICAL_ADDRESSRxBufDeviceBase;   // VirtioNetInitRx
> +  ORDERED_COLLECTION  *TxBufMapInfoCollection; // VirtioNetInitTx

(1) Hmm, I like the name, but I don't like the unaligned comment.

I also don't like the idea of moving all the other comments.

Can you just call this field "TxBufCollection"? Then the comment can be
aligned.


>  } VNET_DEV;
>
>
> @@ -281,6 +283,36 @@ VirtioNetUninitRing (
>);
>
>  //
> +// utility functions to map caller-supplied Tx buffer system physical address
> +// to a device address and vice versa
> +//

I like this comment!

> +EFI_STATUS
> +EFIAPI
> +VirtioNetMapTxBuf (
> +  IN  VNET_DEV  *Dev,
> +  IN  UINT16DescIdx,
> +  IN  VOID  *Buffer,
> +  IN  UINTN NumberOfBytes,
> +  OUT EFI_PHYSICAL_ADDRESS  *DeviceAddress
> +  );
> +
> +INTN
> +EFIAPI
> +VirtioNetTxMapInfoCompare (
> +  IN CONST VOID *UserStruct1,
> +  IN CONST VOID *UserStruct2
> +  );
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioNetUnmapTxBuf (
> +  IN  VNET_DEV  *Dev,
> +  IN  UINT16DescIdx,
> +  OUT VOID  **Buffer,
> +  IN  EFI_PHYSICAL_ADDRESS  DeviceAddress
> +  );
> +

I will comment on these functions in detail below, I just want to say:

- the location of the declarations in this header file is fine, but

(2) Please try to keep the same order between the new functions in
"VirtioNet.h", and in "SnpSharedHelpers.c".


> +//
>  // event callbacks
>  //
>  VOID
> diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c 
> b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> index 6cedb406a172..a8ffb9a8a7b1 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> @@ -176,6 +176,15 @@ VirtioNetInitTx (
>  return EFI_OUT_OF_RESOURCES;
>}
>
> +  Dev->TxBufMapInfoCollection = OrderedCollectionInit (
> +  VirtioNetTxMapInfoCompare,
> +  VirtioNetTxMapInfoCompare
> +  );

(3) VirtioNetTxMapInfoCompare is not right for both of the arguments,
but I'll come to that below.


> +  if (Dev->TxBufMapInfoCollection == NULL) {
> +Status = EFI_OUT_OF_RESOURCES;
> +goto FreeTxFreeStack;
> +  }
> +

(4) The code is good, but please update the documentation of "@retval
EFI_OUT_OF_RESOURCES".


>//
>// Allocate TxSharedReq header and map with BusMasterCommonBuffer so that 
> it
>// can be accessed equally by both processor and device.
> @@ -186,7 +195,7 @@ VirtioNetInitTx (
>
>);
>if (EFI_ERROR (Status)) {
> -goto FreeTxFreeStack;
> +goto UninitMapInfoCollection;
>}
>
>ZeroMem (TxSharedReqBuffer, sizeof *Dev->TxSharedReq);
> @@ -267,6 +276,10 @@ FreeTxSharedReqBuffer:
>   

Re: [edk2] [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code.

[Jordan Justen]

On 2017-09-13 02:25:05, Wang, Jian J wrote:
> The mechanism behind is the same as NULL pointer detection enabled in EDK-II 
> core. SMM has its own page table and we have to disable page 0 again in SMM 
> mode.
> 
> Cc: Jiewen Yao 
> Cc: Eric Dong 
> Cc: Star Zeng 
> Cc: Laszlo Ersek 
> Cc: Justen, Jordan L 
> Cc: Kinney, Michael D 
> Cc: Wolman, Ayellet 
> Suggested-by: Wolman, Ayellet 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Wang, Jian J 
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 11 +++
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c| 25 -
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   |  2 ++
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 17 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c  | 11 +++
>  5 files changed, 57 insertions(+), 9 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> index f295c2ebf2..d423958783 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> @@ -155,6 +155,17 @@ SmiPFHandler (
>  }
>}
>  
> +  //
> +  // If NULL pointer was just accessed
> +  //
> +  if (NULL_DETECTION_ENABLED && (PFAddress >= 0 && PFAddress < 
> EFI_PAGE_SIZE)) {
> +DEBUG ((DEBUG_ERROR, "!!! NULL pointer access !!!\n"));
> +DEBUG_CODE (
> +  DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextIa32->Rip);
> +);
> +CpuDeadLoop ();
> +  }
> +
>if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
>  SmmProfilePFHandler (
>SystemContext.SystemContextIa32->Eip,
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index f086b97c30..81c5ac9d11 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -855,10 +855,10 @@ Gen4GPageTable (
>  Pte[Index] = (Index << 21) | mAddressEncMask | IA32_PG_PS | 
> PAGE_ATTRIBUTE_BITS;
>}
>  
> +  Pdpte = (UINT64*)PageTable;
>if (FeaturePcdGet (PcdCpuSmmStackGuard)) {
>  Pages = (UINTN)PageTable + EFI_PAGES_TO_SIZE (5);
>  GuardPage = mSmmStackArrayBase + EFI_PAGE_SIZE;
> -Pdpte = (UINT64*)PageTable;
>  for (PageIndex = Low2MBoundary; PageIndex <= High2MBoundary; PageIndex 
> += SIZE_2MB) {
>Pte = (UINT64*)(UINTN)(Pdpte[BitFieldRead32 ((UINT32)PageIndex, 30, 
> 31)] & ~mAddressEncMask & ~(EFI_PAGE_SIZE - 1));
>Pte[BitFieldRead32 ((UINT32)PageIndex, 21, 29)] = (UINT64)Pages | 
> mAddressEncMask | PAGE_ATTRIBUTE_BITS;
> @@ -886,6 +886,29 @@ Gen4GPageTable (
>  }
>}
>  
> +  if (NULL_DETECTION_ENABLED) {
> +Pte = (UINT64*)(UINT64)(Pdpte[0] & ~mAddressEncMask & ~(EFI_PAGE_SIZE - 
> 1));
> +if ((Pte[0] & IA32_PG_PS) == 0) {
> +  // 4K-page entries are already mapped. Just hide the first one anyway.
> +  Pte = (UINT64*)(UINT64)(Pte[0] & ~mAddressEncMask & ~(EFI_PAGE_SIZE - 
> 1));
> +  Pte[0] &= ~1; // Hide page 0
> +} else {
> +  // Create 4K-page entries
> +  Pages = (UINTN)AllocatePageTableMemory (1);
> +  ASSERT (Pages != 0);
> +
> +  Pte[0] = (UINT64)(Pages | mAddressEncMask | PAGE_ATTRIBUTE_BITS);
> +
> +  Pte = (UINT64*)Pages;
> +  PageAddress = 0;
> +  Pte[0] = PageAddress | mAddressEncMask; // Hide page 0 but present left
> +  for (Index = 1; Index < EFI_PAGE_SIZE / sizeof (*Pte); Index++) {
> +PageAddress += EFI_PAGE_SIZE;
> +Pte[Index] = PageAddress | mAddressEncMask | PAGE_ATTRIBUTE_BITS;
> +  }
> +}
> +  }
> +
>return (UINT32)(UINTN)PageTable;
>  }
>  
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 1cf85c1481..bcb3032db8 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -153,6 +153,8 @@ typedef UINT32  
> SMM_CPU_ARRIVAL_EXCEPTIONS;
>  #define ARRIVAL_EXCEPTION_DELAYED   0x2
>  #define ARRIVAL_EXCEPTION_SMI_DISABLED  0x4
>  
> +#define NULL_DETECTION_ENABLED
> ((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT1) != 0)
> +

Again, I think the NULL_DETECTION_ENABLED macro code style looks odd.
Maybe it is just better to include the duplicated code directly in the
3 places?

The commit message for this patch has a long line length.

-Jordan

>  //
>  // Private structure for the SMM CPU module that is stored in DXE Runtime 
> memory
>  // Contains the SMM Configuration Protocols that is produced.
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> index 099792e6ce..57a14d9f24 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> +++ 

Re: [edk2] [PATCH 1/4] MdeModulePkg/Core: Implement NULL pointer detection in EDK-II Core.

[Jordan Justen]

On 2017-09-13 02:25:04, Wang, Jian J wrote:
> The mechanism behind is to trigger a page fault exception at address 0. This 
> can be made by disabling page 0 (0-4095) during page table setup. So this 
> feature can only be available on platform with paging enabled. Once this 
> feature is enabled, any code, like CSM, which has to access memory in page 0 
> needs to enable this page temporarily in advance and disable it afterwards. 
> PcdNullPointerDetectionPropertyMask is used to control and elaborate the use 
> cases.
>

Your commit message line is > 450 columns.

https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format

I think BaseTools/Scripts/PatchCheck.py can help detect this issue.

(more below...)

> Cc: Jiewen Yao 
> Cc: Eric Dong 
> Cc: Star Zeng 
> Cc: Laszlo Ersek 
> Cc: Justen, Jordan L 
> Cc: Kinney, Michael D 
> Cc: Wolman, Ayellet 
> Suggested-by: Wolman, Ayellet 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Wang, Jian J 
> ---
>  MdeModulePkg/Core/Dxe/DxeMain.inf|  3 +-
>  MdeModulePkg/Core/Dxe/Mem/Page.c | 21 ++
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c| 47 +
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.h| 15 +++
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf  |  3 +-
>  MdeModulePkg/Core/DxeIplPeim/DxeLoad.c   | 53 
> 
>  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  8 +++-
>  MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 23 ++
>  MdeModulePkg/MdeModulePkg.dec| 12 ++
>  10 files changed, 167 insertions(+), 20 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf 
> b/MdeModulePkg/Core/Dxe/DxeMain.inf
> index 30d5984f7c..273b8b7c0e 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> @@ -179,7 +179,7 @@
>gEfiWatchdogTimerArchProtocolGuid ## CONSUMES
>  
>  [FeaturePcd]
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport ## 
> CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport  
> ## CONSUMES

Why is this line changed?

>  
>  [Pcd]
>gEfiMdeModulePkgTokenSpaceGuid.PcdLoadFixAddressBootTimeCodePageNumber
> ## SOMETIMES_CONSUMES
> @@ -192,6 +192,7 @@
>gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable   
> ## CONSUMES
>gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy   
> ## CONSUMES
>gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy 
> ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
> ## CONSUMES
>  
>  # [Hob]
>  # RESOURCE_DESCRIPTOR   ## CONSUMES
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c 
> b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index a142c79ee2..2e0b72f864 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -170,6 +170,7 @@ CoreAddRange (
>  {
>LIST_ENTRY*Link;
>MEMORY_MAP*Entry;
> +  EFI_STATUSStatus;
>  
>ASSERT ((Start & EFI_PAGE_MASK) == 0);
>ASSERT (End > Start) ;
> @@ -188,7 +189,17 @@ CoreAddRange (
>// used for other purposes.
>//  
>if (Type == EfiConventionalMemory && Start == 0 && (End >= EFI_PAGE_SIZE - 
> 1)) {
> -SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
> +if ((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT0) == 0) {
> +  SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
> +} else if (gCpu != NULL) {
> +  Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, 0);
> +  ASSERT_EFI_ERROR(Status);
> +
> +  SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
> +
> +  Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, 
> EFI_MEMORY_RP);
> +  ASSERT_EFI_ERROR(Status);
> +}
>}
>
>//
> @@ -1972,11 +1983,3 @@ Done:
>return Status;
>  }
>  
> -
> -
> -
> -
> -
> -
> -
> -
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c 
> b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index a73c4ccd64..2367d674e1 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> @@ -995,6 +995,36 @@ MemoryProtectionExitBootServicesCallback (
>}
>  }
>  
> +/**
> +  Disable NULL pointer detection after EndOfDxe. This is a workaround resort 
> in 
> +  order to skip unfixable NULL pointer access issues detected in OptionROM 
> or 
> +  boot loaders.
> +
> +  @param[in]  Event The Event this notify function registered to.
> +  @param[in]  Context   Pointer to the context data registered to the Event.
> +**/
> +VOID
> +EFIAPI
> 

Re: [edk2] [PATCH 1/4] MdeModulePkg/Core: Implement NULL pointer detection in EDK-II Core.

[Johnson, Brian (EXL - Eagan)]

ClearLegacyMemory() assumes that the memory allocation HOB comes after the 
resource descriptor HOB in the HOB list.  Is that guaranteed?  I'd think that 
the memory allocation HOB traversal should be a separate loop, after the 
resource descriptor HOB traversal loop.

Other than that:
Reviewed-by: Brian J. Johnson 

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Wang, 
Jian J
Sent: Wednesday, September 13, 2017 4:25 AM
To: edk2-devel@lists.01.org
Cc: jus...@ml01.01.org; Eric Dong ; kin...@ml01.01.org; 
Jordan L ; wol...@ml01.01.org; Jiewen Yao 
; Ayellet ; Michael D 
; Laszlo Ersek ; Star Zeng 

Subject: [edk2] [PATCH 1/4] MdeModulePkg/Core: Implement NULL pointer detection 
in EDK-II Core.

The mechanism behind is to trigger a page fault exception at address 0. This 
can be made by disabling page 0 (0-4095) during page table setup. So this 
feature can only be available on platform with paging enabled. Once this 
feature is enabled, any code, like CSM, which has to access memory in page 0 
needs to enable this page temporarily in advance and disable it afterwards. 
PcdNullPointerDetectionPropertyMask is used to control and elaborate the use 
cases.

Cc: Jiewen Yao 
Cc: Eric Dong 
Cc: Star Zeng 
Cc: Laszlo Ersek 
Cc: Justen, Jordan L 
Cc: Kinney, Michael D 
Cc: Wolman, Ayellet 
Suggested-by: Wolman, Ayellet 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wang, Jian J 
---
 MdeModulePkg/Core/Dxe/DxeMain.inf|  3 +-
 MdeModulePkg/Core/Dxe/Mem/Page.c | 21 ++
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c| 47 +
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.h| 15 +++
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf  |  3 +-
 MdeModulePkg/Core/DxeIplPeim/DxeLoad.c   | 53 
 MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  8 +++-
 MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 23 ++
 MdeModulePkg/MdeModulePkg.dec| 12 ++
 10 files changed, 167 insertions(+), 20 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf 
b/MdeModulePkg/Core/Dxe/DxeMain.inf
index 30d5984f7c..273b8b7c0e 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.inf
+++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
@@ -179,7 +179,7 @@
   gEfiWatchdogTimerArchProtocolGuid ## CONSUMES
 
 [FeaturePcd]
-  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport ## 
CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport  ## 
CONSUMES
 
 [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdLoadFixAddressBootTimeCodePageNumber## 
SOMETIMES_CONSUMES
@@ -192,6 +192,7 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable   ## 
CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy   ## 
CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy ## 
CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask## 
CONSUMES
 
 # [Hob]
 # RESOURCE_DESCRIPTOR   ## CONSUMES
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index a142c79ee2..2e0b72f864 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -170,6 +170,7 @@ CoreAddRange (
 {
   LIST_ENTRY*Link;
   MEMORY_MAP*Entry;
+  EFI_STATUSStatus;
 
   ASSERT ((Start & EFI_PAGE_MASK) == 0);
   ASSERT (End > Start) ;
@@ -188,7 +189,17 @@ CoreAddRange (
   // used for other purposes.
   //  
   if (Type == EfiConventionalMemory && Start == 0 && (End >= EFI_PAGE_SIZE - 
1)) {
-SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
+if ((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT0) == 0) {
+  SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
+} else if (gCpu != NULL) {
+  Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, 0);
+  ASSERT_EFI_ERROR(Status);
+
+  SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
+
+  Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, 
EFI_MEMORY_RP);
+  ASSERT_EFI_ERROR(Status);
+}
   }
   
   //
@@ -1972,11 +1983,3 @@ Done:
   return Status;
 }
 
-
-
-
-
-
-
-
-
diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c 
b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index a73c4ccd64..2367d674e1 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -995,6 +995,36 @@ 

Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled.

[Johnson, Brian (EXL - Eagan)]

Acked-by: Brian J. Johnson 

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Wang, 
Jian J
Sent: Wednesday, September 13, 2017 4:25 AM
To: edk2-devel@lists.01.org
Cc: jus...@ml01.01.org; Eric Dong ; kin...@ml01.01.org; 
Jordan L ; wol...@ml01.01.org; Jiewen Yao 
; Ayellet ; Michael D 
; Laszlo Ersek ; Star Zeng 

Subject: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to 
bypass NULL pointer detection if enabled.

QemuVideoDxe driver will install VBE SHIM into page 0. If NULL pointer 
detection is enabled, page 0 must be enabled temporarily before installing and 
disabled again afterwards. For Windows 7 boot, BIT7 of 
PcdNullPointerDetectionPropertyMask must still be set to avoid hang.

Cc: Jiewen Yao 
Cc: Eric Dong 
Cc: Star Zeng 
Cc: Laszlo Ersek 
Cc: Justen, Jordan L 
Cc: Kinney, Michael D 
Cc: Wolman, Ayellet 
Suggested-by: Wolman, Ayellet 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wang, Jian J 
---
 OvmfPkg/QemuVideoDxe/Driver.c | 15 ++-
 OvmfPkg/QemuVideoDxe/Qemu.h   | 16 
 OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf |  2 ++
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
index 0dce80e59b..ee0eed7214 100644
--- a/OvmfPkg/QemuVideoDxe/Driver.c
+++ b/OvmfPkg/QemuVideoDxe/Driver.c
@@ -194,6 +194,7 @@ QemuVideoControllerDriverStart (
   PCI_TYPE00Pci;
   QEMU_VIDEO_CARD   *Card;
   EFI_PCI_IO_PROTOCOL   *ChildPciIo;
+  EFI_CPU_ARCH_PROTOCOL *Cpu;
 
   OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
 
@@ -479,7 +480,19 @@ QemuVideoControllerDriverStart (
 #if defined MDE_CPU_IA32 || defined MDE_CPU_X64
   if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO ||
   Private->Variant == QEMU_VIDEO_BOCHS) {
-InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase);
+//
+// Prepare CPU arch protocol for NULL pointer detection
+//
+Status = gBS->LocateProtocol (
+,
+NULL, 
+(VOID **) 
+);
+ASSERT_EFI_ERROR (Status);
+
+DISABLE_NULL_DETECTION(Cpu);
+  InstallVbeShim (Card->Name, 
Private->GraphicsOutput.Mode->FrameBufferBase);
+ENABLE_NULL_DETECTION(Cpu);
   }
 #endif
 
diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h
index 7fbb25b3ef..bb3bc6eb0f 100644
--- a/OvmfPkg/QemuVideoDxe/Qemu.h
+++ b/OvmfPkg/QemuVideoDxe/Qemu.h
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -82,6 +83,21 @@ typedef struct {
 
 #define GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER  0x
 
+//
+// VBE code will access memory between 0-4095 which will cause page fault 
exception 
+// if NULL pointer detection mechanism is enabled. Following macros can be 
used to 
+// disable/enable NULL pointer detection before/after accessing those memory.
+//
+#define NULL_DETECTION_ENABLED  ((PcdGet8(PcdNullPointerDetectionPropertyMask) 
& (BIT0|BIT7)) == BIT0)
+#define DISABLE_NULL_DETECTION(Cpu)
 \
+  if (NULL_DETECTION_ENABLED) {
 \
+(Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, 0);
 \
+  }
+#define ENABLE_NULL_DETECTION(Cpu) 
 \
+  if (NULL_DETECTION_ENABLED) {
 \
+(Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, EFI_MEMORY_RP);
 \
+  }
+
 //
 // QEMU Video Private Data Structure
 //
diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf 
b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
index 7c7d429bca..5d166eb99c 100644
--- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
+++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
@@ -72,7 +72,9 @@
   gEfiGraphicsOutputProtocolGuid# PROTOCOL BY_START
   gEfiDevicePathProtocolGuid# PROTOCOL BY_START
   gEfiPciIoProtocolGuid # PROTOCOL TO_START
+  gEfiCpuArchProtocolGuid
 
 [Pcd]
   gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
 
-- 
2.14.1.windows.1

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

Re: [edk2] [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code.

[Johnson, Brian (EXL - Eagan)]

Comments below.

Brian

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Wang, 
Jian J
Sent: Wednesday, September 13, 2017 4:25 AM
To: edk2-devel@lists.01.org
Cc: jus...@ml01.01.org; Eric Dong ; kin...@ml01.01.org; 
Jordan L ; wol...@ml01.01.org; Jiewen Yao 
; Ayellet ; Michael D 
; Laszlo Ersek ; Star Zeng 

Subject: [edk2] [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer 
detection for SMM mode code.

The mechanism behind is the same as NULL pointer detection enabled in EDK-II 
core. SMM has its own page table and we have to disable page 0 again in SMM 
mode.

Cc: Jiewen Yao 
Cc: Eric Dong 
Cc: Star Zeng 
Cc: Laszlo Ersek 
Cc: Justen, Jordan L 
Cc: Kinney, Michael D 
Cc: Wolman, Ayellet 
Suggested-by: Wolman, Ayellet 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wang, Jian J 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 11 +++
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c| 25 -
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   |  2 ++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 17 +
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c  | 11 +++
 5 files changed, 57 insertions(+), 9 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
index f295c2ebf2..d423958783 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
@@ -155,6 +155,17 @@ SmiPFHandler (
 }
   }
 
+  //
+  // If NULL pointer was just accessed
+  //
+  if (NULL_DETECTION_ENABLED && (PFAddress >= 0 && PFAddress < EFI_PAGE_SIZE)) 
{

[Brian] PFAddress is unsigned, so it will always be >= 0.  Some compilers 
complain about this  Should probably remove that part of the test.

+DEBUG ((DEBUG_ERROR, "!!! NULL pointer access !!!\n"));
+DEBUG_CODE (
+  DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextIa32->Rip);
+);
+CpuDeadLoop ();
+  }
+
   if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
 SmmProfilePFHandler (
   SystemContext.SystemContextIa32->Eip,
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index f086b97c30..81c5ac9d11 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -855,10 +855,10 @@ Gen4GPageTable (
 Pte[Index] = (Index << 21) | mAddressEncMask | IA32_PG_PS | 
PAGE_ATTRIBUTE_BITS;
   }
 
+  Pdpte = (UINT64*)PageTable;
   if (FeaturePcdGet (PcdCpuSmmStackGuard)) {
 Pages = (UINTN)PageTable + EFI_PAGES_TO_SIZE (5);
 GuardPage = mSmmStackArrayBase + EFI_PAGE_SIZE;
-Pdpte = (UINT64*)PageTable;
 for (PageIndex = Low2MBoundary; PageIndex <= High2MBoundary; PageIndex += 
SIZE_2MB) {
   Pte = (UINT64*)(UINTN)(Pdpte[BitFieldRead32 ((UINT32)PageIndex, 30, 31)] 
& ~mAddressEncMask & ~(EFI_PAGE_SIZE - 1));
   Pte[BitFieldRead32 ((UINT32)PageIndex, 21, 29)] = (UINT64)Pages | 
mAddressEncMask | PAGE_ATTRIBUTE_BITS;
@@ -886,6 +886,29 @@ Gen4GPageTable (
 }
   }
 
+  if (NULL_DETECTION_ENABLED) {
+Pte = (UINT64*)(UINT64)(Pdpte[0] & ~mAddressEncMask & ~(EFI_PAGE_SIZE - 
1));

[Brian] Shouldn't the inner cast be (UINTN), not (UINT64)?  That would match 
the PcdCpuSmmStackGuard section above.

+if ((Pte[0] & IA32_PG_PS) == 0) {
+  // 4K-page entries are already mapped. Just hide the first one anyway.
+  Pte = (UINT64*)(UINT64)(Pte[0] & ~mAddressEncMask & ~(EFI_PAGE_SIZE - 
1));

[Brian] Same comment re. the inner cast.

+  Pte[0] &= ~1; // Hide page 0
+} else {
+  // Create 4K-page entries
+  Pages = (UINTN)AllocatePageTableMemory (1);
+  ASSERT (Pages != 0);
+
+  Pte[0] = (UINT64)(Pages | mAddressEncMask | PAGE_ATTRIBUTE_BITS);
+
+  Pte = (UINT64*)Pages;
+  PageAddress = 0;
+  Pte[0] = PageAddress | mAddressEncMask; // Hide page 0 but present left
+  for (Index = 1; Index < EFI_PAGE_SIZE / sizeof (*Pte); Index++) {
+PageAddress += EFI_PAGE_SIZE;
+Pte[Index] = PageAddress | mAddressEncMask | PAGE_ATTRIBUTE_BITS;
+  }
+}
+  }
+
   return (UINT32)(UINTN)PageTable;
 }
 
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 1cf85c1481..bcb3032db8 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -153,6 +153,8 @@ typedef UINT32  
SMM_CPU_ARRIVAL_EXCEPTIONS;
 #define ARRIVAL_EXCEPTION_DELAYED   0x2
 #define ARRIVAL_EXCEPTION_SMI_DISABLED  0x4
 
+#define 

Re: [edk2] [PATCH 3/4] IntelFrameworkModulePkg/Csm: Update CSM code to temporarily bypass NULL pointer detection if enabled.

[Johnson, Brian (EXL - Eagan)]

Acked-by: Brian J. Johnson 

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Wang, 
Jian J
Sent: Wednesday, September 13, 2017 4:25 AM
To: edk2-devel@lists.01.org
Cc: jus...@ml01.01.org; Eric Dong ; kin...@ml01.01.org; 
Jordan L ; wol...@ml01.01.org; Jiewen Yao 
; Ayellet ; Michael D 
; Laszlo Ersek ; Star Zeng 

Subject: [edk2] [PATCH 3/4] IntelFrameworkModulePkg/Csm: Update CSM code to 
temporarily bypass NULL pointer detection if enabled.

CSM code has to access memory below 4096 (BDA, int vector, etc.). If NULL 
pointer detection is enabled, the page 0 must be enabled temporarily before 
accessing it and disabled again afterwards. Otherwise page fault will be 
triggered.

Cc: Jiewen Yao 
Cc: Eric Dong 
Cc: Star Zeng 
Cc: Laszlo Ersek 
Cc: Justen, Jordan L 
Cc: Kinney, Michael D 
Cc: Wolman, Ayellet 
Suggested-by: Wolman, Ayellet 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wang, Jian J 
---
 .../Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c   | 10 +++-
 .../Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h   | 18 +++
 .../Csm/BiosThunk/KeyboardDxe/KeyboardDxe.inf  |  2 +
 .../Csm/LegacyBiosDxe/LegacyBda.c  |  4 ++
 .../Csm/LegacyBiosDxe/LegacyBios.c | 55 ++
 .../Csm/LegacyBiosDxe/LegacyBiosDxe.inf|  2 +
 .../Csm/LegacyBiosDxe/LegacyBiosInterface.h| 23 +
 .../Csm/LegacyBiosDxe/LegacyBootSupport.c  | 33 ++---
 .../Csm/LegacyBiosDxe/LegacyPci.c  | 17 ++-
 IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c  | 41 ++--
 10 files changed, 173 insertions(+), 32 deletions(-)

diff --git a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c 
b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c
index 7308523ad8..96148ae367 100644
--- a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c
+++ b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c
@@ -248,7 +248,7 @@ BiosKeyboardDriverBindingStart (
   //
   // Allocate the private device structure
   //
-BiosKeyboardPrivate = (BIOS_KEYBOARD_DEV *) AllocateZeroPool (sizeof 
(BIOS_KEYBOARD_DEV));
+  BiosKeyboardPrivate = (BIOS_KEYBOARD_DEV *) AllocateZeroPool (sizeof 
(BIOS_KEYBOARD_DEV));
   if (NULL == BiosKeyboardPrivate) {
 Status = EFI_OUT_OF_RESOURCES;
 goto Done;
@@ -281,6 +281,9 @@ BiosKeyboardDriverBindingStart (
   BiosKeyboardPrivate->SimpleTextInputEx.UnregisterKeyNotify = 
BiosKeyboardUnregisterKeyNotify;
   InitializeListHead (>NotifyList);
 
+  Status = gBS->LocateProtocol (, NULL, (VOID **) 
>Cpu);
+  ASSERT_EFI_ERROR(Status);
+
   //
   // Report that the keyboard is being enabled
   //
@@ -1842,7 +1845,9 @@ BiosKeyboardTimerHandler (
   //
   // Clear the CTRL and ALT BDA flag
   //
-  KbFlag1 = *((UINT8 *) (UINTN) 0x417);  // read the STATUS FLAGS 1
+  DISABLE_NULL_DETECTION(BiosKeyboardPrivate);
+
+  KbFlag1 = *((UINT8 *) (UINTN) 0x417); // read the STATUS FLAGS 1
   KbFlag2 = *((UINT8 *) (UINTN) 0x418); // read STATUS FLAGS 2
 
   DEBUG_CODE (
@@ -1916,6 +1921,7 @@ BiosKeyboardTimerHandler (
   KbFlag1 &= ~0x0C;  
   *((UINT8 *) (UINTN) 0x417) = KbFlag1; 
 
+  ENABLE_NULL_DETECTION(BiosKeyboardPrivate);
   
   //
   // Output EFI input key and shift/toggle state
diff --git a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h 
b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h
index 0bf28ea140..b717ef676b 100644
--- a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h
+++ b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h
@@ -26,6 +26,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -212,6 +213,7 @@ typedef struct {
   EFI_HANDLE  Handle;
   EFI_LEGACY_BIOS_PROTOCOL*LegacyBios;
   EFI_ISA_IO_PROTOCOL *IsaIo;
+  EFI_CPU_ARCH_PROTOCOL   *Cpu;
   EFI_SIMPLE_TEXT_INPUT_PROTOCOL  SimpleTextIn;
   EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL   SimpleTextInputEx;
   UINT16  DataRegisterAddress;
@@ -242,6 +244,22 @@ typedef struct {
   BIOS_KEYBOARD_DEV_SIGNATURE \
   )
 
+//
+// CSM needs to access memory between 0-4095, which will cause page fault 
exception 
+// if NULL pointer detection mechanism is enabled. Following macros can be used
+// to disable/enable 

Re: [edk2] [Patch] AppPkg/WebServer: Fix build failure.

[Daryl McDaniel (EDK2 Lists)]

Looks good.

Reviewed-by: Daryl McDaniel 

> -Original Message-
> From: Eric Dong [mailto:eric.d...@intel.com]
> Sent: Wednesday, September 13, 2017 3:10 AM
> To: edk2-devel@lists.01.org
> Cc: Daryl McDaniel ; Jaben Carsey
> ; Ruiyu Ni 
> Subject: [Patch] AppPkg/WebServer: Fix build failure.
> 
> Fix build failure caused by UefiCpuPkg/MtrrLib removes deprecated macros.
> 
> Cc: Daryl McDaniel 
> Cc: Jaben Carsey 
> Cc: Ruiyu Ni 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong 
> ---
>  AppPkg/Applications/Sockets/WebServer/Mtrr.c  | 31 
> +++
>  AppPkg/Applications/Sockets/WebServer/WebServer.h |  1 +
>  2 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/AppPkg/Applications/Sockets/WebServer/Mtrr.c
> b/AppPkg/Applications/Sockets/WebServer/Mtrr.c
> index 92f90b0..54356bd 100644
> --- a/AppPkg/Applications/Sockets/WebServer/Mtrr.c
> +++ b/AppPkg/Applications/Sockets/WebServer/Mtrr.c
> @@ -146,12 +146,12 @@ MemoryTypeRegistersPage (
>  {
>UINT64 Addr;
>BOOLEAN bValid;
> -  UINT64 Capabilities;
> +  MSR_IA32_MTRRCAP_REGISTER Capabilities;
>UINTN Count;
> -  UINT64 DefType;
> +  MSR_IA32_MTRR_DEF_TYPE_REGISTER DefType;
>UINTN Index;
>UINT64 Mask;
> -  UINT64 MaxMtrrs;
> +
>CONST UINT64 mFixedAddresses [( 8 * MTRR_NUMBER_OF_FIXED_MTRR ) + 1 ] = {
> 0ULL,
>   0x1ULL,
> @@ -302,8 +302,8 @@ MemoryTypeRegistersPage (
>//
>//  Get the capabilities
>//
> -  Capabilities = AsmReadMsr64 ( MTRR_LIB_IA32_MTRR_CAP );
> -  DefType =  AsmReadMsr64 ( MTRR_LIB_IA32_MTRR_DEF_TYPE );
> +  Capabilities.Uint64 = AsmReadMsr64 ( MSR_IA32_MTRRCAP );
> +  DefType.Uint64 =  AsmReadMsr64 ( MSR_IA32_MTRR_DEF_TYPE );
> 
>//
>//  Display the capabilities
> @@ -316,7 +316,7 @@ MemoryTypeRegistersPage (
>}
>Status = HttpSendHexValue ( SocketFD,
>pPort,
> -  Capabilities );
> +  Capabilities.Uint64 );
>if ( EFI_ERROR ( Status )) {
>  break;
>}
> @@ -338,7 +338,7 @@ MemoryTypeRegistersPage (
>}
>Status = HttpSendHexValue ( SocketFD,
>pPort,
> -  DefType );
> +  DefType.Uint64);
>if ( EFI_ERROR ( Status )) {
>  break;
>}
> @@ -350,7 +350,7 @@ MemoryTypeRegistersPage (
>}
>Status = HttpSendAnsiString ( SocketFD,
>  pPort,
> -( 0 != ( DefType &
> MTRR_LIB_CACHE_MTRR_ENABLED ))
> +( 0 != DefType.Bits.E )
>  ? "Enabled"
>  : "Disabled" );
>if ( EFI_ERROR ( Status )) {
> @@ -364,7 +364,7 @@ MemoryTypeRegistersPage (
>}
>Status = HttpSendAnsiString ( SocketFD,
>  pPort,
> -( 0 != ( DefType &
> MTRR_LIB_CACHE_FIXED_MTRR_ENABLED ))
> +( 0 != DefType.Bits.FE )
>  ? "Enabled"
>  : "Disabled" );
>if ( EFI_ERROR ( Status )) {
> @@ -376,7 +376,7 @@ MemoryTypeRegistersPage (
>if ( EFI_ERROR ( Status )) {
>  break;
>}
> -  Type = DefType & 0xff;
> +  Type = DefType.Uint64 & 0xff;
>Status = HttpSendAnsiString ( SocketFD,
>  pPort,
>  ( DIM ( mMemoryType ) > Type )
> @@ -395,7 +395,7 @@ MemoryTypeRegistersPage (
>//
>//  Determine if MTRRs are enabled
>//
> -  if ( 0 == ( DefType & MTRR_LIB_CACHE_MTRR_ENABLED )) {
> +  if ( 0 == DefType.Bits.E ) {
>  Status = HttpSendAnsiString ( SocketFD,
>pPort,
>"All memory is uncached!\r\n" );
> @@ -412,8 +412,8 @@ MemoryTypeRegistersPage (
>  //
>  //  Determine if the fixed MTRRs are supported
>  //
> -if (( 0 != ( Capabilities & 0x100 ))
> -  && ( 0 != ( DefType & MTRR_LIB_CACHE_FIXED_MTRR_ENABLED ))) {
> +if (( 0 != Capabilities.Bits.FIX )
> +  && ( 0 != DefType.Bits.FE)) {
> 
>//
>//  Beginning of table
> @@ -615,8 +615,7 @@ MemoryTypeRegistersPage (
>  //
>  //  Determine if the variable MTRRs are supported
>  //
> -MaxMtrrs = Capabilities & MTRR_LIB_IA32_MTRR_CAP_VCNT_MASK;
> -if ( 0 < MaxMtrrs ) {
> +if ( 0 < 

Re: [edk2] [PATCH v2 5/8] OvmfPkg/VirtioNetDxe: update TechNotes

[Laszlo Ersek]

On 09/11/17 14:16, Brijesh Singh wrote:
> In next patches we will update Virtio transmit to use the device-mapped
> address of the caller-supplied packet. The patch documents the new model.
> 
> Cc: Ard Biesheuvel 
> Cc: Jordan Justen 
> Cc: Tom Lendacky 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh 
> ---
>  OvmfPkg/VirtioNetDxe/TechNotes.txt | 20 +---
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/OvmfPkg/VirtioNetDxe/TechNotes.txt 
> b/OvmfPkg/VirtioNetDxe/TechNotes.txt
> index f39426fb13e4..c6ca341ead15 100644
> --- a/OvmfPkg/VirtioNetDxe/TechNotes.txt
> +++ b/OvmfPkg/VirtioNetDxe/TechNotes.txt
> @@ -312,10 +312,14 @@ in the following:
>that is shared by all of the head descriptors. This virtio-net request 
> header
>is never modified by the host.
>  
> -- Each tail descriptor is re-pointed to the caller-supplied packet buffer
> -  whenever VirtioNetTransmit places the corresponding head descriptor on the
> -  Available Ring. The caller is responsible to hang on to the unmodified 
> buffer
> -  until it is reported transmitted by VirtioNetGetStatus.
> +- Each tail descriptor is re-pointed to the device-mapped address of the
> +  caller-supplied packet buffer whenever VirtioNetTransmit places the
> +  corresponding head descriptor on the Available Ring. A reverse mapping, 
> from
> +  the device-mapped address to the caller-supplied packet address, is saved 
> in
> +  an associative data structure that belongs to the driver instance.
> +
> +- Per spec, the caller is responsible to hang on to the unmodified packet
> +  buffer until it is reported transmitted by VirtioNetGetStatus.
>  
>  Steps of packet transmission:
>  
> @@ -338,9 +342,11 @@ Steps of packet transmission:
>  - Client code calls VirtioNetGetStatus. In case the Used Ring is empty, the
>function reports no Tx completion. Otherwise, a head descriptor's index is
>consumed from the Used Ring and recycled to the private stack. The client
> -  code's original packet buffer address is fetched from the tail descriptor
> -  (where it has been stored at VirtioNetTransmit time) and returned to the
> -  caller.
> +  code's original packet buffer address is calculated by fetching the
> +  device-mapped address from the tail descriptor (where it has been stored at
> +  VirtioNetTransmit time), and by looking up the device-mapped address in the
> +  associative data structure. The reverse-mapped packet buffer address is
> +  returned to the caller.
>  
>  - The Len field of the Used Ring Element is not checked. The host is assumed 
> to
>have transmitted the entire packet -- VirtioNetTransmit had forced it below
> 

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

Re: [edk2] [PATCH v2 4/8] OvmfPkg/VirtioNetDxe: dynamically alloc transmit header

[Laszlo Ersek]

On 09/11/17 14:16, Brijesh Singh wrote:
> Each network packet is submitted for transmission by pushing the head
> descriptor of a two-part descriptor chain to the Available Ring of the
> TX queue. VirtioNetInitTx() sets up the the descriptor chains for all
> queueable packets in advance, and points all the head descriptors to the
> same shared, never modified, VIRTIO_1_0_NET_REQ header object (or its
> initial VIRTIO_NET_REQ sub-object, dependent on virtio version).
> VirtioNetInitTx() currently uses the header object's system physical
> address for populating the head descriptors.
> 
> When device is behind the IOMMU, VirtioNet driver is required to provide
> the device address of VIRTIO_1_0_NET_REQ header. In this patch we
> dynamically allocate the header using AllocateSharedPages() and map with
> BusMasterCommonBuffer so that header can be accessed by both processor
> and the device.
> 
> We map the header object for CommonBuffer operation because, in order to
> stick with the current code order, we populate the head descriptors with
> the header's device address first, and fill in the header itself second.
> 
> Cc: Ard Biesheuvel 
> Cc: Jordan Justen 
> Cc: Tom Lendacky 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh 
> ---
>  OvmfPkg/VirtioNetDxe/VirtioNet.h|  3 +-
>  OvmfPkg/VirtioNetDxe/SnpInitialize.c| 64 +---
>  OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c |  7 +++
>  3 files changed, 65 insertions(+), 9 deletions(-)
> 
> diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h 
> b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> index 7d5f33b01dc8..3f48bcc6b67c 100644
> --- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
> +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> @@ -96,7 +96,8 @@ typedef struct {
>UINT16  TxMaxPending;  // VirtioNetInitTx
>UINT16  TxCurPending;  // VirtioNetInitTx
>UINT16  *TxFreeStack;  // VirtioNetInitTx
> -  VIRTIO_1_0_NET_REQ  TxSharedReq;   // VirtioNetInitTx
> +  VIRTIO_1_0_NET_REQ  *TxSharedReq;  // VirtioNetInitTx
> +  VOID*TxSharedReqMap;   // VirtioNetInitTx
>UINT16  TxLastUsed;// VirtioNetInitTx
>EFI_PHYSICAL_ADDRESSRxBufDeviceBase;   // VirtioNetInitRx
>  } VNET_DEV;
> diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c 
> b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> index 54c808c501bf..6cedb406a172 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> @@ -18,6 +18,7 @@
>  **/
>  
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -147,6 +148,9 @@ ReleaseQueue:
>  
>@retval EFI_OUT_OF_RESOURCES  Failed to allocate the stack to track the 
> heads
>  of free descriptor chains.
> +  @return   Status codes from VIRTIO_DEVICE_PROTOCOL.
> +AllocateSharedPages() or
> +VirtioMapAllBytesInSharedBuffer()
>@retval EFI_SUCCESS   TX setup successful.
>  */
>  
> @@ -157,8 +161,11 @@ VirtioNetInitTx (
>IN OUT VNET_DEV *Dev
>)
>  {
> -  UINTN TxSharedReqSize;
> -  UINTN PktIdx;
> +  UINTN TxSharedReqSize;
> +  UINTN PktIdx;
> +  EFI_STATUSStatus;
> +  EFI_PHYSICAL_ADDRESS  DeviceAddress;
> +  VOID  *TxSharedReqBuffer;
>  
>Dev->TxMaxPending = (UINT16) MIN (Dev->TxRing.QueueSize / 2,
>   VNET_MAX_PENDING);
> @@ -170,12 +177,42 @@ VirtioNetInitTx (
>}
>  
>//
> +  // Allocate TxSharedReq header and map with BusMasterCommonBuffer so that 
> it
> +  // can be accessed equally by both processor and device.
> +  //
> +  Status = Dev->VirtIo->AllocateSharedPages (
> +  Dev->VirtIo,
> +  EFI_SIZE_TO_PAGES (sizeof *Dev->TxSharedReq),
> +  
> +  );
> +  if (EFI_ERROR (Status)) {
> +goto FreeTxFreeStack;
> +  }
> +
> +  ZeroMem (TxSharedReqBuffer, sizeof *Dev->TxSharedReq);
> +
> +  Status = VirtioMapAllBytesInSharedBuffer (
> + Dev->VirtIo,
> + VirtioOperationBusMasterCommonBuffer,
> + TxSharedReqBuffer,
> + sizeof *(Dev->TxSharedReq),
> + ,
> + >TxSharedReqMap
> + );
> +  if (EFI_ERROR (Status)) {
> +goto FreeTxSharedReqBuffer;
> +  }
> +
> +  Dev->TxSharedReq = TxSharedReqBuffer;
> +
> +
> +  //
>// In VirtIo 1.0, the NumBuffers field is mandatory. In 0.9.5, it depends 
> on
>// VIRTIO_NET_F_MRG_RXBUF, which we never negotiate.
>//
>TxSharedReqSize = (Dev->VirtIo->Revision < VIRTIO_SPEC_REVISION (1, 0, 0)) 
> ?
> -sizeof 

Re: [edk2] [Patch] AppPkg/WebServer: Fix build failure.

[Carsey, Jaben]

Reviewed-by: Jaben Carsey 

> -Original Message-
> From: Dong, Eric
> Sent: Wednesday, September 13, 2017 3:10 AM
> To: edk2-devel@lists.01.org
> Cc: Daryl McDaniel ; Carsey, Jaben
> ; Ni, Ruiyu 
> Subject: [Patch] AppPkg/WebServer: Fix build failure.
> Importance: High
> 
> Fix build failure caused by UefiCpuPkg/MtrrLib removes deprecated macros.
> 
> Cc: Daryl McDaniel 
> Cc: Jaben Carsey 
> Cc: Ruiyu Ni 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong 
> ---
>  AppPkg/Applications/Sockets/WebServer/Mtrr.c  | 31 +++---
> -
>  AppPkg/Applications/Sockets/WebServer/WebServer.h |  1 +
>  2 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/AppPkg/Applications/Sockets/WebServer/Mtrr.c
> b/AppPkg/Applications/Sockets/WebServer/Mtrr.c
> index 92f90b0..54356bd 100644
> --- a/AppPkg/Applications/Sockets/WebServer/Mtrr.c
> +++ b/AppPkg/Applications/Sockets/WebServer/Mtrr.c
> @@ -146,12 +146,12 @@ MemoryTypeRegistersPage (
>  {
>UINT64 Addr;
>BOOLEAN bValid;
> -  UINT64 Capabilities;
> +  MSR_IA32_MTRRCAP_REGISTER Capabilities;
>UINTN Count;
> -  UINT64 DefType;
> +  MSR_IA32_MTRR_DEF_TYPE_REGISTER DefType;
>UINTN Index;
>UINT64 Mask;
> -  UINT64 MaxMtrrs;
> +
>CONST UINT64 mFixedAddresses [( 8 * MTRR_NUMBER_OF_FIXED_MTRR )
> + 1 ] = {
> 0ULL,
>   0x1ULL,
> @@ -302,8 +302,8 @@ MemoryTypeRegistersPage (
>//
>//  Get the capabilities
>//
> -  Capabilities = AsmReadMsr64 ( MTRR_LIB_IA32_MTRR_CAP );
> -  DefType =  AsmReadMsr64 ( MTRR_LIB_IA32_MTRR_DEF_TYPE );
> +  Capabilities.Uint64 = AsmReadMsr64 ( MSR_IA32_MTRRCAP );
> +  DefType.Uint64 =  AsmReadMsr64 ( MSR_IA32_MTRR_DEF_TYPE );
> 
>//
>//  Display the capabilities
> @@ -316,7 +316,7 @@ MemoryTypeRegistersPage (
>}
>Status = HttpSendHexValue ( SocketFD,
>pPort,
> -  Capabilities );
> +  Capabilities.Uint64 );
>if ( EFI_ERROR ( Status )) {
>  break;
>}
> @@ -338,7 +338,7 @@ MemoryTypeRegistersPage (
>}
>Status = HttpSendHexValue ( SocketFD,
>pPort,
> -  DefType );
> +  DefType.Uint64);
>if ( EFI_ERROR ( Status )) {
>  break;
>}
> @@ -350,7 +350,7 @@ MemoryTypeRegistersPage (
>}
>Status = HttpSendAnsiString ( SocketFD,
>  pPort,
> -( 0 != ( DefType & 
> MTRR_LIB_CACHE_MTRR_ENABLED ))
> +( 0 != DefType.Bits.E )
>  ? "Enabled"
>  : "Disabled" );
>if ( EFI_ERROR ( Status )) {
> @@ -364,7 +364,7 @@ MemoryTypeRegistersPage (
>}
>Status = HttpSendAnsiString ( SocketFD,
>  pPort,
> -( 0 != ( DefType &
> MTRR_LIB_CACHE_FIXED_MTRR_ENABLED ))
> +( 0 != DefType.Bits.FE )
>  ? "Enabled"
>  : "Disabled" );
>if ( EFI_ERROR ( Status )) {
> @@ -376,7 +376,7 @@ MemoryTypeRegistersPage (
>if ( EFI_ERROR ( Status )) {
>  break;
>}
> -  Type = DefType & 0xff;
> +  Type = DefType.Uint64 & 0xff;
>Status = HttpSendAnsiString ( SocketFD,
>  pPort,
>  ( DIM ( mMemoryType ) > Type )
> @@ -395,7 +395,7 @@ MemoryTypeRegistersPage (
>//
>//  Determine if MTRRs are enabled
>//
> -  if ( 0 == ( DefType & MTRR_LIB_CACHE_MTRR_ENABLED )) {
> +  if ( 0 == DefType.Bits.E ) {
>  Status = HttpSendAnsiString ( SocketFD,
>pPort,
>"All memory is uncached!\r\n" );
> @@ -412,8 +412,8 @@ MemoryTypeRegistersPage (
>  //
>  //  Determine if the fixed MTRRs are supported
>  //
> -if (( 0 != ( Capabilities & 0x100 ))
> -  && ( 0 != ( DefType & MTRR_LIB_CACHE_FIXED_MTRR_ENABLED ))) {
> +if (( 0 != Capabilities.Bits.FIX )
> +  && ( 0 != DefType.Bits.FE)) {
> 
>//
>//  Beginning of table
> @@ -615,8 +615,7 @@ MemoryTypeRegistersPage (
>  //
>  //  Determine if the variable MTRRs are supported
>  //
> -MaxMtrrs = Capabilities & MTRR_LIB_IA32_MTRR_CAP_VCNT_MASK;
> -if ( 0 < MaxMtrrs ) {
> +if ( 0 < Capabilities.Bits.VCNT ) {

Re: [edk2] [PATCH 0/2] MdeModulePkg: UDF fixes and cleanups, round 2

[Paulo Alcantara]

On September 12, 2017 7:26:10 PM GMT-03:00, Laszlo Ersek  
wrote:
>Repo:   https://github.com/lersek/edk2.git
>Branch: udf_fixes_cleanups_round2
>
>Once these patches are sufficiently reviewed, please don't wait for me
>to commit them.
>
>Further UdfDxe issues should be please reported in the TianoCore
>Bugzilla.
>
>Cc: Ard Biesheuvel 
>Cc: Eric Dong 
>Cc: Paulo Alcantara 
>Cc: Ruiyu Ni 
>Cc: Star Zeng 
>
>Thanks
>Laszlo
>
>Laszlo Ersek (2):
>  MdeModulePkg/UdfDxe: reject reserved values in ICB.Flags[2:0]
> MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()
>
>MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 13
>+
> 1 file changed, 13 insertions(+)

Reviewed-by: Paulo Alcantara 

Thanks,
Paulo

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] Failed to clear configuration in Ip4Config2 Protocol

[Karunakar P]

Hello All,

I was trying to verify the feature "Allow SetData to clear configuration in 
Ip4Config2/Ip6Config Protocol" , But SetData returns with Write Protected Error 
Status

[Steps followed]

1.   I've taken the above feature changes.

2.   I've a UEFI test Application which call to SetData with DataSize is 0 
and Data is NULL

Status = Ip4Cfg2->SetData (

Ip4Cfg2,

Ip4Config2DataTypeManualAddress,

0,

0

);

3.   But SetData returns with Write Protected Error Status// Status 
= Write Protected

4.Faced the same error for setting Gateway & DnsServer

Guess the return is happening from
Ip4Config2SetManualAddress() ->
...
  if (Instance->Policy != Ip4Config2PolicyStatic) {
return EFI_WRITE_PROTECTED;
  }
...

Could you please help on this whether am I missing anything or anything else 
need to be done to resolve this?

Thanks,
karunakar


From: Karunakar P
Sent: Wednesday, September 13, 2017 7:00 PM
To: edk2-devel@lists.01.org
Subject: RE: Failed to clear configuration in Ip4Config2 Protocol

Hello All,

I was trying to verify the feature "Allow SetData to clear configuration in 
Ip4Config2/Ip6Config Protocol" , But SetData returns with Write Protected Error 
Status

[Steps followed]

1.   I've taken the above feature changes.

2.   I've a UEFI test Application which call to SetData with DataSize is 0 
and Data is NULL

Status = Ip4Cfg2->SetData (

Ip4Cfg2,

Ip4Config2DataTypeManualAddress,

0,

0

);

3.   But SetData returns with Write Protected Error Status// Status 
= Write Protected

4.Faced the same error for setting Gateway 

Guess the error is happening from


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

Re: [edk2] Failed to clear configuration in Ip4Config2 Protocol

[Karunakar P]

Hello All,

I was trying to verify the feature "Allow SetData to clear configuration in 
Ip4Config2/Ip6Config Protocol" , But SetData returns with Write Protected Error 
Status

[Steps followed]

1.   I've taken the above feature changes.

2.   I've a UEFI test Application which call to SetData with DataSize is 0 
and Data is NULL

Status = Ip4Cfg2->SetData (

Ip4Cfg2,

Ip4Config2DataTypeManualAddress,

0,

0

);

3.   But SetData returns with Write Protected Error Status// Status 
= Write Protected

4.Faced the same error for setting Gateway 

Guess the error is happening from


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

[edk2] HEADLESS flag in ACPI

[Evan Lloyd]

We have a minor conundrum, and would like an element of consensus before we 
make a commit.
We want to (re-)submit patches adding HDLCD/GOP for some ARM platforms.
We have previously been asked to make this optional, as some people do not want 
the overhead of GOP (and the frame buffer) in their image/RAM.  This is fine, 
and a build flag called HEADLESS would seem to fit the bill.

The query arises from the ACPI spec which has (in FADT): "
HEADLESS 1 12 System Type Attribute. If set indicates the system cannot detect 
the monitor or keyboard / mouse devices."

And what we need to confirm is that there is no case of a system with no 
display still wanting to have keyboard and/or mouse.  I can't imagine there is, 
but thought it best to check.

Any thoughts?


Evan

IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 00/11] Add IOMMU PEI support.

[Yao, Jiewen]

Good question.

Yes, I think we can put all VTd related components to 
IntelSiliconPkg/Feature/VTd.
Once we finished the review, I would submit another patch to move all 
components to the new location.

Thank you
Yao Jiewen


> -Original Message-
> From: Zeng, Star
> Sent: Wednesday, September 13, 2017 6:14 PM
> To: Yao, Jiewen ; edk2-devel@lists.01.org
> Cc: Zeng, Star 
> Subject: RE: [edk2] [PATCH 00/11] Add IOMMU PEI support.
> 
> I have been confused by where should new module be put for some time.
> When should a new module be put at XXXPkg/Universal? For example,
> MdeModule/Universal, UefiCpuPkg/Universal, 
> When should a new module be put at XXXPkg/Feature? For example,
> UefiCpuPkg/Feature.
> When should a new module be put at the root of a package folder? For example,
> UefiCpuPkg/PiSmmCpuDxeSmm, IntelSiliconPkg/IntelVTdDxe, 
> 
> Is it better or not to put IntelVTdDxe, PlatformVTdSampleDxe and new
> IntelVTdPmrPei, PlatformVTdInfoSamplePei in IntelSiliconPkg/Feature/VTd
> together?
> 
> 
> Thanks,
> Star
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jiewen
> Yao
> Sent: Friday, September 8, 2017 11:04 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH 00/11] Add IOMMU PEI support.
> 
> This series patch added IOMMU PEI support.
> It is also posted to https://github.com/jyao1/edk2/tree/IoMmuPpi.
> 
> 1) Patch 1 and 2 add EDKII_IOMMU_PPI.
> It is similar to EDKII_IOMMU_PROTOCOL.
> 
> 2) Patch 3 and 4 add Intel VTD PMR register support for DXE phase IntelVTdDxe.
> This is to prepare handle PMR usage in PEI phase.
> 
> 3) Patch 5 and 6 add EDKII_VTD_INFO_PPI.
> This PPI is to provide Intel VTD information in PEI.
> In DXE, the VTd driver can get VTD info from ACPI DMAR table.
> But in PEI, there is no way to get VTD info before.
> VTD_INFO_PPI is added to resolve the problem.
> 
> 4) Patch 7 and 8 add IntelVTdPmrPei driver.
> This driver consumes EDKII_VTD_INFO_PPI and produces IOMMU_PPI.
> It enables VTD PMR register to provide DMA protection.
> The PMR based DMA protection is a simple solution to mark 2 regions can be
> DMA protected.
> The IntelVTdPmrPei allocates a small chunk buffer for DMA and protect the rest
> memory.
> 
> 5) Patch 9 and 10 add a sample VTdInfo PEI driver.
> It provides a sample to show how to report VTd info in PEI phase.
> 
> 6) Patch 11 updates XhciPei driver to consume IOMMU_PPI.
> If the IOMMU_PPI is present, XhciPei will use IOMMU_PPI to allocate DMA
> buffer. Or the XhciPei will still use old way - PeiServiceAllocatePage to 
> allocate
> DRAM as DMA buffer.
> This is the first PEI device driver consuming IOMMU_PPI to show the concept.
> The rest PEI device drivers will be updated in separated patches.
> 
> 
> This series patch is validated on Intel Kabylake Platform.
> 1) We can use XHCI to do file transfer in PEI phase,
> 2) We can still use XHCI in DXE phase, such as shell environment.
> 3) If the device driver does not consume IOMMU_PPI, the DMA fails.
> 
> Jiewen Yao (11):
>   MdeModulePkg/Include: Add IOMMU_PPI.
>   MdeModulePkg/Dec: Add IOMMU_PPI GUID.
>   IntelSiliconPkg/Vtd.h: Add definition for PMR.
>   IntelSiliconPkg/VTdDxe: Disable PMR
>   IntelSiliconPkg/include: Add VTD_INFO PPI.
>   IntelSiliconPkg/dec: Add VTD_INFO PPI GUID
>   IntelSiliconPkg: Add IntelVTdPmrPei.
>   IntelSiliconPkg/dsc: Add IntelVTdPmrPeim.
>   IntelSiliconPkg: Add PlatformVTdInfoSamplePei.
>   IntelSiliconPkg/dsc: Add PlatformVTdInfoSamplePei.
>   MdeModulePkg/XhciPei: Support IoMmu.
> 
>  IntelSiliconPkg/Include/IndustryStandard/Vtd.h
> |   6 +
>  IntelSiliconPkg/Include/Ppi/VtdInfo.h
> |  40 ++
>  IntelSiliconPkg/IntelSiliconPkg.dec
> |   3 +
>  IntelSiliconPkg/IntelSiliconPkg.dsc
> |  10 +
>  IntelSiliconPkg/IntelVTdDxe/VtdReg.c
> |  51 +-
>  IntelSiliconPkg/IntelVTdPmrPei/IntelVTdPmr.c
> | 314 ++
>  IntelSiliconPkg/IntelVTdPmrPei/IntelVTdPmrPei.c
> | 615 
>  IntelSiliconPkg/IntelVTdPmrPei/IntelVTdPmrPei.h
> |  68 +++
>  IntelSiliconPkg/IntelVTdPmrPei/IntelVTdPmrPei.inf
> |  59 ++
>  IntelSiliconPkg/IntelVTdPmrPei/IntelVTdPmrPei.uni
> |  20 +
>  IntelSiliconPkg/IntelVTdPmrPei/IntelVTdPmrPeiExtra.uni
> |  20 +
>  IntelSiliconPkg/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.c
> |  65 +++
>  IntelSiliconPkg/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.inf
> |  51 ++
>  IntelSiliconPkg/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.uni
> |  20 +
>  IntelSiliconPkg/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePeiExtra.uni
> |  20 +
>  MdeModulePkg/Bus/Pci/XhciPei/DmaMem.c
> | 249 
>  MdeModulePkg/Bus/Pci/XhciPei/UsbHcMem.c
> |  55 +-
>  MdeModulePkg/Bus/Pci/XhciPei/UsbHcMem.h
> |   9 +-
>  MdeModulePkg/Bus/Pci/XhciPei/XhcPeim.c
> |  55 +-
>  MdeModulePkg/Bus/Pci/XhciPei/XhcPeim.h
> | 107 
>  MdeModulePkg/Bus/Pci/XhciPei/XhciPei.inf
> |   3 +
>  MdeModulePkg/Bus/Pci/XhciPei/XhciSched.c
> |  47 +-
>  

Re: [edk2] [PATCH 00/11] Add IOMMU PEI support.

[Zeng, Star]

I have been confused by where should new module be put for some time.
When should a new module be put at XXXPkg/Universal? For example, 
MdeModule/Universal, UefiCpuPkg/Universal, 
When should a new module be put at XXXPkg/Feature? For example, 
UefiCpuPkg/Feature.
When should a new module be put at the root of a package folder? For example, 
UefiCpuPkg/PiSmmCpuDxeSmm, IntelSiliconPkg/IntelVTdDxe, 

Is it better or not to put IntelVTdDxe, PlatformVTdSampleDxe and new 
IntelVTdPmrPei, PlatformVTdInfoSamplePei in IntelSiliconPkg/Feature/VTd 
together?


Thanks,
Star
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jiewen 
Yao
Sent: Friday, September 8, 2017 11:04 PM
To: edk2-devel@lists.01.org
Subject: [edk2] [PATCH 00/11] Add IOMMU PEI support.

This series patch added IOMMU PEI support.
It is also posted to https://github.com/jyao1/edk2/tree/IoMmuPpi.

1) Patch 1 and 2 add EDKII_IOMMU_PPI.
It is similar to EDKII_IOMMU_PROTOCOL.

2) Patch 3 and 4 add Intel VTD PMR register support for DXE phase IntelVTdDxe.
This is to prepare handle PMR usage in PEI phase.

3) Patch 5 and 6 add EDKII_VTD_INFO_PPI.
This PPI is to provide Intel VTD information in PEI.
In DXE, the VTd driver can get VTD info from ACPI DMAR table.
But in PEI, there is no way to get VTD info before.
VTD_INFO_PPI is added to resolve the problem.

4) Patch 7 and 8 add IntelVTdPmrPei driver.
This driver consumes EDKII_VTD_INFO_PPI and produces IOMMU_PPI.
It enables VTD PMR register to provide DMA protection.
The PMR based DMA protection is a simple solution to mark 2 regions can be DMA 
protected.
The IntelVTdPmrPei allocates a small chunk buffer for DMA and protect the rest 
memory.

5) Patch 9 and 10 add a sample VTdInfo PEI driver.
It provides a sample to show how to report VTd info in PEI phase.

6) Patch 11 updates XhciPei driver to consume IOMMU_PPI.
If the IOMMU_PPI is present, XhciPei will use IOMMU_PPI to allocate DMA buffer. 
Or the XhciPei will still use old way - PeiServiceAllocatePage to allocate DRAM 
as DMA buffer.
This is the first PEI device driver consuming IOMMU_PPI to show the concept. 
The rest PEI device drivers will be updated in separated patches.


This series patch is validated on Intel Kabylake Platform.
1) We can use XHCI to do file transfer in PEI phase,
2) We can still use XHCI in DXE phase, such as shell environment.
3) If the device driver does not consume IOMMU_PPI, the DMA fails.

Jiewen Yao (11):
  MdeModulePkg/Include: Add IOMMU_PPI.
  MdeModulePkg/Dec: Add IOMMU_PPI GUID.
  IntelSiliconPkg/Vtd.h: Add definition for PMR.
  IntelSiliconPkg/VTdDxe: Disable PMR
  IntelSiliconPkg/include: Add VTD_INFO PPI.
  IntelSiliconPkg/dec: Add VTD_INFO PPI GUID
  IntelSiliconPkg: Add IntelVTdPmrPei.
  IntelSiliconPkg/dsc: Add IntelVTdPmrPeim.
  IntelSiliconPkg: Add PlatformVTdInfoSamplePei.
  IntelSiliconPkg/dsc: Add PlatformVTdInfoSamplePei.
  MdeModulePkg/XhciPei: Support IoMmu.

 IntelSiliconPkg/Include/IndustryStandard/Vtd.h |   
6 +
 IntelSiliconPkg/Include/Ppi/VtdInfo.h  |  
40 ++
 IntelSiliconPkg/IntelSiliconPkg.dec|   
3 +
 IntelSiliconPkg/IntelSiliconPkg.dsc|  
10 +
 IntelSiliconPkg/IntelVTdDxe/VtdReg.c   |  
51 +-
 IntelSiliconPkg/IntelVTdPmrPei/IntelVTdPmr.c   | 
314 ++
 IntelSiliconPkg/IntelVTdPmrPei/IntelVTdPmrPei.c| 
615 
 IntelSiliconPkg/IntelVTdPmrPei/IntelVTdPmrPei.h|  
68 +++
 IntelSiliconPkg/IntelVTdPmrPei/IntelVTdPmrPei.inf  |  
59 ++
 IntelSiliconPkg/IntelVTdPmrPei/IntelVTdPmrPei.uni  |  
20 +
 IntelSiliconPkg/IntelVTdPmrPei/IntelVTdPmrPeiExtra.uni |  
20 +
 IntelSiliconPkg/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.c|  
65 +++
 IntelSiliconPkg/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.inf  |  
51 ++
 IntelSiliconPkg/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.uni  |  
20 +
 IntelSiliconPkg/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePeiExtra.uni |  
20 +
 MdeModulePkg/Bus/Pci/XhciPei/DmaMem.c  | 
249 
 MdeModulePkg/Bus/Pci/XhciPei/UsbHcMem.c|  
55 +-
 MdeModulePkg/Bus/Pci/XhciPei/UsbHcMem.h|   
9 +-
 MdeModulePkg/Bus/Pci/XhciPei/XhcPeim.c |  
55 +-
 MdeModulePkg/Bus/Pci/XhciPei/XhcPeim.h | 
107 
 MdeModulePkg/Bus/Pci/XhciPei/XhciPei.inf   |   
3 +
 MdeModulePkg/Bus/Pci/XhciPei/XhciSched.c   |  
47 +-
 MdeModulePkg/Bus/Pci/XhciPei/XhciSched.h   |   
1 +
 

[edk2] [Patch] AppPkg/WebServer: Fix build failure.

[Eric Dong]

Fix build failure caused by UefiCpuPkg/MtrrLib removes deprecated macros.

Cc: Daryl McDaniel 
Cc: Jaben Carsey 
Cc: Ruiyu Ni 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong 
---
 AppPkg/Applications/Sockets/WebServer/Mtrr.c  | 31 +++
 AppPkg/Applications/Sockets/WebServer/WebServer.h |  1 +
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/AppPkg/Applications/Sockets/WebServer/Mtrr.c 
b/AppPkg/Applications/Sockets/WebServer/Mtrr.c
index 92f90b0..54356bd 100644
--- a/AppPkg/Applications/Sockets/WebServer/Mtrr.c
+++ b/AppPkg/Applications/Sockets/WebServer/Mtrr.c
@@ -146,12 +146,12 @@ MemoryTypeRegistersPage (
 {
   UINT64 Addr;
   BOOLEAN bValid;
-  UINT64 Capabilities;
+  MSR_IA32_MTRRCAP_REGISTER Capabilities;
   UINTN Count;
-  UINT64 DefType;
+  MSR_IA32_MTRR_DEF_TYPE_REGISTER DefType;
   UINTN Index;
   UINT64 Mask;
-  UINT64 MaxMtrrs;
+
   CONST UINT64 mFixedAddresses [( 8 * MTRR_NUMBER_OF_FIXED_MTRR ) + 1 ] = {
0ULL,
  0x1ULL,
@@ -302,8 +302,8 @@ MemoryTypeRegistersPage (
   //
   //  Get the capabilities
   //
-  Capabilities = AsmReadMsr64 ( MTRR_LIB_IA32_MTRR_CAP );
-  DefType =  AsmReadMsr64 ( MTRR_LIB_IA32_MTRR_DEF_TYPE );
+  Capabilities.Uint64 = AsmReadMsr64 ( MSR_IA32_MTRRCAP );
+  DefType.Uint64 =  AsmReadMsr64 ( MSR_IA32_MTRR_DEF_TYPE );
 
   //
   //  Display the capabilities
@@ -316,7 +316,7 @@ MemoryTypeRegistersPage (
   }
   Status = HttpSendHexValue ( SocketFD,
   pPort,
-  Capabilities );
+  Capabilities.Uint64 );
   if ( EFI_ERROR ( Status )) {
 break;
   }
@@ -338,7 +338,7 @@ MemoryTypeRegistersPage (
   }
   Status = HttpSendHexValue ( SocketFD,
   pPort,
-  DefType );
+  DefType.Uint64);
   if ( EFI_ERROR ( Status )) {
 break;
   }
@@ -350,7 +350,7 @@ MemoryTypeRegistersPage (
   }
   Status = HttpSendAnsiString ( SocketFD,
 pPort,
-( 0 != ( DefType & 
MTRR_LIB_CACHE_MTRR_ENABLED ))
+( 0 != DefType.Bits.E )
 ? "Enabled"
 : "Disabled" );
   if ( EFI_ERROR ( Status )) {
@@ -364,7 +364,7 @@ MemoryTypeRegistersPage (
   }
   Status = HttpSendAnsiString ( SocketFD,
 pPort,
-( 0 != ( DefType & 
MTRR_LIB_CACHE_FIXED_MTRR_ENABLED ))
+( 0 != DefType.Bits.FE )
 ? "Enabled"
 : "Disabled" );
   if ( EFI_ERROR ( Status )) {
@@ -376,7 +376,7 @@ MemoryTypeRegistersPage (
   if ( EFI_ERROR ( Status )) {
 break;
   }
-  Type = DefType & 0xff;
+  Type = DefType.Uint64 & 0xff;
   Status = HttpSendAnsiString ( SocketFD,
 pPort,
 ( DIM ( mMemoryType ) > Type )
@@ -395,7 +395,7 @@ MemoryTypeRegistersPage (
   //
   //  Determine if MTRRs are enabled
   //
-  if ( 0 == ( DefType & MTRR_LIB_CACHE_MTRR_ENABLED )) {
+  if ( 0 == DefType.Bits.E ) {
 Status = HttpSendAnsiString ( SocketFD,
   pPort,
   "All memory is uncached!\r\n" );
@@ -412,8 +412,8 @@ MemoryTypeRegistersPage (
 //
 //  Determine if the fixed MTRRs are supported
 //
-if (( 0 != ( Capabilities & 0x100 ))
-  && ( 0 != ( DefType & MTRR_LIB_CACHE_FIXED_MTRR_ENABLED ))) {
+if (( 0 != Capabilities.Bits.FIX )
+  && ( 0 != DefType.Bits.FE)) {
 
   //
   //  Beginning of table
@@ -615,8 +615,7 @@ MemoryTypeRegistersPage (
 //
 //  Determine if the variable MTRRs are supported
 //
-MaxMtrrs = Capabilities & MTRR_LIB_IA32_MTRR_CAP_VCNT_MASK;
-if ( 0 < MaxMtrrs ) {
+if ( 0 < Capabilities.Bits.VCNT ) {
   //
   //  Beginning of table
   //
@@ -632,7 +631,7 @@ MemoryTypeRegistersPage (
   //
   //  Display the variable MTRRs
   //
-  for ( Count = 0; MaxMtrrs > Count; Count++ ) {
+  for ( Count = 0; Capabilities.Bits.VCNT > Count; Count++ ) {
 //
 //  Start the row
 //
diff --git a/AppPkg/Applications/Sockets/WebServer/WebServer.h 
b/AppPkg/Applications/Sockets/WebServer/WebServer.h
index 16c30c8..21b07b6 100644
--- a/AppPkg/Applications/Sockets/WebServer/WebServer.h
+++ 

[edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled.

[Wang, Jian J]

QemuVideoDxe driver will install VBE SHIM into page 0. If NULL pointer 
detection is enabled, page 0 must be enabled temporarily before installing and 
disabled again afterwards. For Windows 7 boot, BIT7 of 
PcdNullPointerDetectionPropertyMask must still be set to avoid hang.

Cc: Jiewen Yao 
Cc: Eric Dong 
Cc: Star Zeng 
Cc: Laszlo Ersek 
Cc: Justen, Jordan L 
Cc: Kinney, Michael D 
Cc: Wolman, Ayellet 
Suggested-by: Wolman, Ayellet 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wang, Jian J 
---
 OvmfPkg/QemuVideoDxe/Driver.c | 15 ++-
 OvmfPkg/QemuVideoDxe/Qemu.h   | 16 
 OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf |  2 ++
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
index 0dce80e59b..ee0eed7214 100644
--- a/OvmfPkg/QemuVideoDxe/Driver.c
+++ b/OvmfPkg/QemuVideoDxe/Driver.c
@@ -194,6 +194,7 @@ QemuVideoControllerDriverStart (
   PCI_TYPE00Pci;
   QEMU_VIDEO_CARD   *Card;
   EFI_PCI_IO_PROTOCOL   *ChildPciIo;
+  EFI_CPU_ARCH_PROTOCOL *Cpu;
 
   OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
 
@@ -479,7 +480,19 @@ QemuVideoControllerDriverStart (
 #if defined MDE_CPU_IA32 || defined MDE_CPU_X64
   if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO ||
   Private->Variant == QEMU_VIDEO_BOCHS) {
-InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase);
+//
+// Prepare CPU arch protocol for NULL pointer detection
+//
+Status = gBS->LocateProtocol (
+,
+NULL, 
+(VOID **) 
+);
+ASSERT_EFI_ERROR (Status);
+
+DISABLE_NULL_DETECTION(Cpu);
+  InstallVbeShim (Card->Name, 
Private->GraphicsOutput.Mode->FrameBufferBase);
+ENABLE_NULL_DETECTION(Cpu);
   }
 #endif
 
diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h
index 7fbb25b3ef..bb3bc6eb0f 100644
--- a/OvmfPkg/QemuVideoDxe/Qemu.h
+++ b/OvmfPkg/QemuVideoDxe/Qemu.h
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -82,6 +83,21 @@ typedef struct {
 
 #define GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER  0x
 
+//
+// VBE code will access memory between 0-4095 which will cause page fault 
exception 
+// if NULL pointer detection mechanism is enabled. Following macros can be 
used to 
+// disable/enable NULL pointer detection before/after accessing those memory.
+//
+#define NULL_DETECTION_ENABLED  ((PcdGet8(PcdNullPointerDetectionPropertyMask) 
& (BIT0|BIT7)) == BIT0)
+#define DISABLE_NULL_DETECTION(Cpu)
 \
+  if (NULL_DETECTION_ENABLED) {
 \
+(Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, 0);
 \
+  }
+#define ENABLE_NULL_DETECTION(Cpu) 
 \
+  if (NULL_DETECTION_ENABLED) {
 \
+(Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, EFI_MEMORY_RP);
 \
+  }
+
 //
 // QEMU Video Private Data Structure
 //
diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf 
b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
index 7c7d429bca..5d166eb99c 100644
--- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
+++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
@@ -72,7 +72,9 @@
   gEfiGraphicsOutputProtocolGuid# PROTOCOL BY_START
   gEfiDevicePathProtocolGuid# PROTOCOL BY_START
   gEfiPciIoProtocolGuid # PROTOCOL TO_START
+  gEfiCpuArchProtocolGuid
 
 [Pcd]
   gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
 
-- 
2.14.1.windows.1

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

[edk2] [PATCH 1/4] MdeModulePkg/Core: Implement NULL pointer detection in EDK-II Core.

[Wang, Jian J]

The mechanism behind is to trigger a page fault exception at address 0. This 
can be made by disabling page 0 (0-4095) during page table setup. So this 
feature can only be available on platform with paging enabled. Once this 
feature is enabled, any code, like CSM, which has to access memory in page 0 
needs to enable this page temporarily in advance and disable it afterwards. 
PcdNullPointerDetectionPropertyMask is used to control and elaborate the use 
cases.

Cc: Jiewen Yao 
Cc: Eric Dong 
Cc: Star Zeng 
Cc: Laszlo Ersek 
Cc: Justen, Jordan L 
Cc: Kinney, Michael D 
Cc: Wolman, Ayellet 
Suggested-by: Wolman, Ayellet 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wang, Jian J 
---
 MdeModulePkg/Core/Dxe/DxeMain.inf|  3 +-
 MdeModulePkg/Core/Dxe/Mem/Page.c | 21 ++
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c| 47 +
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.h| 15 +++
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf  |  3 +-
 MdeModulePkg/Core/DxeIplPeim/DxeLoad.c   | 53 
 MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  8 +++-
 MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 23 ++
 MdeModulePkg/MdeModulePkg.dec| 12 ++
 10 files changed, 167 insertions(+), 20 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf 
b/MdeModulePkg/Core/Dxe/DxeMain.inf
index 30d5984f7c..273b8b7c0e 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.inf
+++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
@@ -179,7 +179,7 @@
   gEfiWatchdogTimerArchProtocolGuid ## CONSUMES
 
 [FeaturePcd]
-  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport ## 
CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport  ## 
CONSUMES
 
 [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdLoadFixAddressBootTimeCodePageNumber## 
SOMETIMES_CONSUMES
@@ -192,6 +192,7 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable   ## 
CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy   ## 
CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy ## 
CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask## 
CONSUMES
 
 # [Hob]
 # RESOURCE_DESCRIPTOR   ## CONSUMES
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index a142c79ee2..2e0b72f864 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -170,6 +170,7 @@ CoreAddRange (
 {
   LIST_ENTRY*Link;
   MEMORY_MAP*Entry;
+  EFI_STATUSStatus;
 
   ASSERT ((Start & EFI_PAGE_MASK) == 0);
   ASSERT (End > Start) ;
@@ -188,7 +189,17 @@ CoreAddRange (
   // used for other purposes.
   //  
   if (Type == EfiConventionalMemory && Start == 0 && (End >= EFI_PAGE_SIZE - 
1)) {
-SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
+if ((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT0) == 0) {
+  SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
+} else if (gCpu != NULL) {
+  Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, 0);
+  ASSERT_EFI_ERROR(Status);
+
+  SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
+
+  Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, 
EFI_MEMORY_RP);
+  ASSERT_EFI_ERROR(Status);
+}
   }
   
   //
@@ -1972,11 +1983,3 @@ Done:
   return Status;
 }
 
-
-
-
-
-
-
-
-
diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c 
b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index a73c4ccd64..2367d674e1 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -995,6 +995,36 @@ MemoryProtectionExitBootServicesCallback (
   }
 }
 
+/**
+  Disable NULL pointer detection after EndOfDxe. This is a workaround resort 
in 
+  order to skip unfixable NULL pointer access issues detected in OptionROM or 
+  boot loaders.
+
+  @param[in]  Event The Event this notify function registered to.
+  @param[in]  Context   Pointer to the context data registered to the Event.
+**/
+VOID
+EFIAPI
+DisableNullDetectionAtTheEndOfDxe (
+  EFI_EVENT   Event,
+  VOID*Context
+  )
+{
+  EFI_STATUSStatus;
+
+  DEBUG((DEBUG_INFO, "DisableNullDetectionAtTheEndOfDxe(): start\r\n"));
+  //
+  // Disable NULL pointer detection by enabling first 4K page
+  //
+  Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, 0);
+  ASSERT_EFI_ERROR(Status);
+
+  CoreCloseEvent (Event);
+  DEBUG((DEBUG_INFO, "DisableNullDetectionAtTheEndOfDxe(): end\r\n"));
+
+  

[edk2] [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code.

[Wang, Jian J]

The mechanism behind is the same as NULL pointer detection enabled in EDK-II 
core. SMM has its own page table and we have to disable page 0 again in SMM 
mode.

Cc: Jiewen Yao 
Cc: Eric Dong 
Cc: Star Zeng 
Cc: Laszlo Ersek 
Cc: Justen, Jordan L 
Cc: Kinney, Michael D 
Cc: Wolman, Ayellet 
Suggested-by: Wolman, Ayellet 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wang, Jian J 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 11 +++
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c| 25 -
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   |  2 ++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 17 +
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c  | 11 +++
 5 files changed, 57 insertions(+), 9 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
index f295c2ebf2..d423958783 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
@@ -155,6 +155,17 @@ SmiPFHandler (
 }
   }
 
+  //
+  // If NULL pointer was just accessed
+  //
+  if (NULL_DETECTION_ENABLED && (PFAddress >= 0 && PFAddress < EFI_PAGE_SIZE)) 
{
+DEBUG ((DEBUG_ERROR, "!!! NULL pointer access !!!\n"));
+DEBUG_CODE (
+  DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextIa32->Rip);
+);
+CpuDeadLoop ();
+  }
+
   if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
 SmmProfilePFHandler (
   SystemContext.SystemContextIa32->Eip,
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index f086b97c30..81c5ac9d11 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -855,10 +855,10 @@ Gen4GPageTable (
 Pte[Index] = (Index << 21) | mAddressEncMask | IA32_PG_PS | 
PAGE_ATTRIBUTE_BITS;
   }
 
+  Pdpte = (UINT64*)PageTable;
   if (FeaturePcdGet (PcdCpuSmmStackGuard)) {
 Pages = (UINTN)PageTable + EFI_PAGES_TO_SIZE (5);
 GuardPage = mSmmStackArrayBase + EFI_PAGE_SIZE;
-Pdpte = (UINT64*)PageTable;
 for (PageIndex = Low2MBoundary; PageIndex <= High2MBoundary; PageIndex += 
SIZE_2MB) {
   Pte = (UINT64*)(UINTN)(Pdpte[BitFieldRead32 ((UINT32)PageIndex, 30, 31)] 
& ~mAddressEncMask & ~(EFI_PAGE_SIZE - 1));
   Pte[BitFieldRead32 ((UINT32)PageIndex, 21, 29)] = (UINT64)Pages | 
mAddressEncMask | PAGE_ATTRIBUTE_BITS;
@@ -886,6 +886,29 @@ Gen4GPageTable (
 }
   }
 
+  if (NULL_DETECTION_ENABLED) {
+Pte = (UINT64*)(UINT64)(Pdpte[0] & ~mAddressEncMask & ~(EFI_PAGE_SIZE - 
1));
+if ((Pte[0] & IA32_PG_PS) == 0) {
+  // 4K-page entries are already mapped. Just hide the first one anyway.
+  Pte = (UINT64*)(UINT64)(Pte[0] & ~mAddressEncMask & ~(EFI_PAGE_SIZE - 
1));
+  Pte[0] &= ~1; // Hide page 0
+} else {
+  // Create 4K-page entries
+  Pages = (UINTN)AllocatePageTableMemory (1);
+  ASSERT (Pages != 0);
+
+  Pte[0] = (UINT64)(Pages | mAddressEncMask | PAGE_ATTRIBUTE_BITS);
+
+  Pte = (UINT64*)Pages;
+  PageAddress = 0;
+  Pte[0] = PageAddress | mAddressEncMask; // Hide page 0 but present left
+  for (Index = 1; Index < EFI_PAGE_SIZE / sizeof (*Pte); Index++) {
+PageAddress += EFI_PAGE_SIZE;
+Pte[Index] = PageAddress | mAddressEncMask | PAGE_ATTRIBUTE_BITS;
+  }
+}
+  }
+
   return (UINT32)(UINTN)PageTable;
 }
 
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 1cf85c1481..bcb3032db8 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -153,6 +153,8 @@ typedef UINT32  
SMM_CPU_ARRIVAL_EXCEPTIONS;
 #define ARRIVAL_EXCEPTION_DELAYED   0x2
 #define ARRIVAL_EXCEPTION_SMI_DISABLED  0x4
 
+#define NULL_DETECTION_ENABLED
((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT1) != 0)
+
 //
 // Private structure for the SMM CPU module that is stored in DXE Runtime 
memory
 // Contains the SMM Configuration Protocols that is produced.
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
index 099792e6ce..57a14d9f24 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
@@ -138,14 +138,14 @@
   gEdkiiPiSmmMemoryAttributesTableGuid ## CONSUMES ## SystemTable
 
 [FeaturePcd]
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmDebug ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmBlockStartupThisAp## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport   ## CONSUMES
-  

[edk2] [PATCH 3/4] IntelFrameworkModulePkg/Csm: Update CSM code to temporarily bypass NULL pointer detection if enabled.

[Wang, Jian J]

CSM code has to access memory below 4096 (BDA, int vector, etc.). If NULL 
pointer detection is enabled, the page 0 must be enabled temporarily before 
accessing it and disabled again afterwards. Otherwise page fault will be 
triggered.

Cc: Jiewen Yao 
Cc: Eric Dong 
Cc: Star Zeng 
Cc: Laszlo Ersek 
Cc: Justen, Jordan L 
Cc: Kinney, Michael D 
Cc: Wolman, Ayellet 
Suggested-by: Wolman, Ayellet 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wang, Jian J 
---
 .../Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c   | 10 +++-
 .../Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h   | 18 +++
 .../Csm/BiosThunk/KeyboardDxe/KeyboardDxe.inf  |  2 +
 .../Csm/LegacyBiosDxe/LegacyBda.c  |  4 ++
 .../Csm/LegacyBiosDxe/LegacyBios.c | 55 ++
 .../Csm/LegacyBiosDxe/LegacyBiosDxe.inf|  2 +
 .../Csm/LegacyBiosDxe/LegacyBiosInterface.h| 23 +
 .../Csm/LegacyBiosDxe/LegacyBootSupport.c  | 33 ++---
 .../Csm/LegacyBiosDxe/LegacyPci.c  | 17 ++-
 IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c  | 41 ++--
 10 files changed, 173 insertions(+), 32 deletions(-)

diff --git a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c 
b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c
index 7308523ad8..96148ae367 100644
--- a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c
+++ b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c
@@ -248,7 +248,7 @@ BiosKeyboardDriverBindingStart (
   //
   // Allocate the private device structure
   //
-BiosKeyboardPrivate = (BIOS_KEYBOARD_DEV *) AllocateZeroPool (sizeof 
(BIOS_KEYBOARD_DEV));
+  BiosKeyboardPrivate = (BIOS_KEYBOARD_DEV *) AllocateZeroPool (sizeof 
(BIOS_KEYBOARD_DEV));
   if (NULL == BiosKeyboardPrivate) {
 Status = EFI_OUT_OF_RESOURCES;
 goto Done;
@@ -281,6 +281,9 @@ BiosKeyboardDriverBindingStart (
   BiosKeyboardPrivate->SimpleTextInputEx.UnregisterKeyNotify = 
BiosKeyboardUnregisterKeyNotify;
   InitializeListHead (>NotifyList);
 
+  Status = gBS->LocateProtocol (, NULL, (VOID **) 
>Cpu);
+  ASSERT_EFI_ERROR(Status);
+
   //
   // Report that the keyboard is being enabled
   //
@@ -1842,7 +1845,9 @@ BiosKeyboardTimerHandler (
   //
   // Clear the CTRL and ALT BDA flag
   //
-  KbFlag1 = *((UINT8 *) (UINTN) 0x417);  // read the STATUS FLAGS 1
+  DISABLE_NULL_DETECTION(BiosKeyboardPrivate);
+
+  KbFlag1 = *((UINT8 *) (UINTN) 0x417); // read the STATUS FLAGS 1
   KbFlag2 = *((UINT8 *) (UINTN) 0x418); // read STATUS FLAGS 2
 
   DEBUG_CODE (
@@ -1916,6 +1921,7 @@ BiosKeyboardTimerHandler (
   KbFlag1 &= ~0x0C;  
   *((UINT8 *) (UINTN) 0x417) = KbFlag1; 
 
+  ENABLE_NULL_DETECTION(BiosKeyboardPrivate);
   
   //
   // Output EFI input key and shift/toggle state
diff --git a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h 
b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h
index 0bf28ea140..b717ef676b 100644
--- a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h
+++ b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h
@@ -26,6 +26,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -212,6 +213,7 @@ typedef struct {
   EFI_HANDLE  Handle;
   EFI_LEGACY_BIOS_PROTOCOL*LegacyBios;
   EFI_ISA_IO_PROTOCOL *IsaIo;
+  EFI_CPU_ARCH_PROTOCOL   *Cpu;
   EFI_SIMPLE_TEXT_INPUT_PROTOCOL  SimpleTextIn;
   EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL   SimpleTextInputEx;
   UINT16  DataRegisterAddress;
@@ -242,6 +244,22 @@ typedef struct {
   BIOS_KEYBOARD_DEV_SIGNATURE \
   )
 
+//
+// CSM needs to access memory between 0-4095, which will cause page fault 
exception 
+// if NULL pointer detection mechanism is enabled. Following macros can be used
+// to disable/enable NULL pointer detection before/after accessing those 
memory.
+//
+#define NULL_POINTER_DETECTION_ENABLED  
((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT0) != 0)
+#define DISABLE_NULL_DETECTION(Instance)   
 \
+  if (NULL_POINTER_DETECTION_ENABLED && (Instance)->Cpu != NULL) { 
 \
+(Instance)->Cpu->SetMemoryAttributes((Instance)->Cpu, 0, EFI_PAGE_SIZE, 
0); \
+  }
+  
+#define ENABLE_NULL_DETECTION(Instance)
 \
+  if (NULL_POINTER_DETECTION_ENABLED && (Instance)->Cpu != NULL) { 
 \
+(Instance)->Cpu->SetMemoryAttributes((Instance)->Cpu, 0, 

[edk2] [PATCH 0/4] Implement NULL pointer detection feature for special pool

[Wang, Jian J]

The mechanism behind is to trigger a page fault exception at address 0. This 
can be made by disabling page 0 (0-4095) during page table setup. So this 
feature can only be available on platform with paging enabled. Once this 
feature is enabled, any code, like CSM, which has to access memory in page 0 
needs to enable this page temporarily in advance and disable it afterwards. 
PcdNullPointerDetectionPropertyMask is used to control and elaborate the use 
cases. For example, BIT7 of this PCD must be set for Windows 7 boot on Qemu if 
BIT0 set; or boot will fail.

Wang, Jian J (4):
  Implement NULL pointer detection in EDK-II Core.
  Implement NULL pointer detection for SMM mode code.
  Update CSM code to temporarily bypass NULL pointer detection if
enabled.
  Update QemuVideoDxe driver to bypass NULL pointer detection if
enabled.

 .../Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c   | 10 +++-
 .../Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h   | 18 +++
 .../Csm/BiosThunk/KeyboardDxe/KeyboardDxe.inf  |  2 +
 .../Csm/LegacyBiosDxe/LegacyBda.c  |  4 ++
 .../Csm/LegacyBiosDxe/LegacyBios.c | 55 ++
 .../Csm/LegacyBiosDxe/LegacyBiosDxe.inf|  2 +
 .../Csm/LegacyBiosDxe/LegacyBiosInterface.h| 23 +
 .../Csm/LegacyBiosDxe/LegacyBootSupport.c  | 33 ++---
 .../Csm/LegacyBiosDxe/LegacyPci.c  | 17 ++-
 IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c  | 41 ++--
 MdeModulePkg/Core/Dxe/DxeMain.inf  |  3 +-
 MdeModulePkg/Core/Dxe/Mem/Page.c   | 21 +
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c  | 47 ++
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.h  | 15 ++
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf|  3 +-
 MdeModulePkg/Core/DxeIplPeim/DxeLoad.c | 53 +
 MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c|  8 +++-
 MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c |  2 +
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c   | 23 ++---
 MdeModulePkg/MdeModulePkg.dec  | 12 +
 OvmfPkg/QemuVideoDxe/Driver.c  | 15 +-
 OvmfPkg/QemuVideoDxe/Qemu.h| 16 +++
 OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf  |  2 +
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c   | 11 +
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c  | 25 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h |  2 +
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf   | 17 +++
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c| 11 +
 28 files changed, 429 insertions(+), 62 deletions(-)

-- 
2.14.1.windows.1

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

Re: [edk2] [PATCH v2 3/8] OvmfPkg/VirtioNetDxe: alloc RxBuf using AllocateSharedPages()

[Laszlo Ersek]

On 09/11/17 14:16, Brijesh Singh wrote:
> When device is behind the IOMMU, VirtioNetDxe is required to use the
> device address in bus master operations. RxBuf is allocated using
> AllocatePool() which returns the system physical address.
> 
> The patch uses VIRTIO_DEVICE_PROTOCOL.AllocateSharedPages() to allocate
> the RxBuf and map with VirtioMapAllBytesInSharedBuffer() so that we can
> obtain the device address for RxBuf.
> 
> Cc: Ard Biesheuvel 
> Cc: Jordan Justen 
> Cc: Tom Lendacky 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh 
> ---
>  OvmfPkg/VirtioNetDxe/VirtioNet.h|  4 +
>  OvmfPkg/VirtioNetDxe/SnpInitialize.c| 77 +++-
>  OvmfPkg/VirtioNetDxe/SnpReceive.c   |  5 +-
>  OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c |  7 +-
>  OvmfPkg/VirtioNetDxe/TechNotes.txt  |  2 +-
>  5 files changed, 74 insertions(+), 21 deletions(-)
> 
> diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h 
> b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> index 6762fc9d1d6e..7d5f33b01dc8 100644
> --- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
> +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> @@ -4,6 +4,7 @@
>Protocol instances for virtio-net devices.
>  
>Copyright (C) 2013, Red Hat, Inc.
> +  Copyright (c) 2017, AMD Inc, All rights reserved.
>  
>This program and the accompanying materials are licensed and made available
>under the terms and conditions of the BSD License which accompanies this
> @@ -86,6 +87,8 @@ typedef struct {
>   // VirtioNetInitRing
>UINT8   *RxBuf;// VirtioNetInitRx
>UINT16  RxLastUsed;// VirtioNetInitRx
> +  UINTN   RxBufNrPages;  // VirtioNetInitRx
> +  VOID*RxBufMap; // VirtioNetInitRx
>  
>VRING   TxRing;// VirtioNetInitRing
>VOID*TxRingMap;// VirtioRingMap and
> @@ -95,6 +98,7 @@ typedef struct {
>UINT16  *TxFreeStack;  // VirtioNetInitTx
>VIRTIO_1_0_NET_REQ  TxSharedReq;   // VirtioNetInitTx
>UINT16  TxLastUsed;// VirtioNetInitTx
> +  EFI_PHYSICAL_ADDRESSRxBufDeviceBase;   // VirtioNetInitRx

(1) For clarity, please move the new "RxBufDeviceBase" field between
"RxBufNrPages" and "RxBufMap".


>  } VNET_DEV;
>  
>  
> diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c 
> b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> index 8eabdbff6f5e..54c808c501bf 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> @@ -242,8 +242,10 @@ VirtioNetInitTx (
>@param[in,out] Dev   The VNET_DEV driver instance about to enter the
> EfiSimpleNetworkInitialized state.
>  
> -  @retval EFI_OUT_OF_RESOURCES  Failed to allocate RX destination area.
> -  @return   Status codes from VIRTIO_CFG_WRITE().
> +  @retval EFI_OUT_OF_RESOURCES  Failed to allocate or map RX destination 
> area.
> +  @return   Status codes from VIRTIO_CFG_WRITE() or
> +VIRTIO_DEVICE_PROTOCOL.AllocateSharedPages or
> +VirtioMapAllBytesInSharedBuffer().
>@retval EFI_SUCCESS   RX setup successful. The device is live and 
> may
>  already be writing to the receive area.
>  */

The update to "@return ..." is fine.

(2) But, please remove the "@retval EFI_OUT_OF_RESOURCES" line entirely;
we no longer return this status code explicitly.


> @@ -255,13 +257,15 @@ VirtioNetInitRx (
>IN OUT VNET_DEV *Dev
>)
>  {
> -  EFI_STATUS Status;
> -  UINTN  VirtioNetReqSize;
> -  UINTN  RxBufSize;
> -  UINT16 RxAlwaysPending;
> -  UINTN  PktIdx;
> -  UINT16 DescIdx;
> -  UINT8  *RxPtr;
> +  EFI_STATUSStatus;
> +  UINTN VirtioNetReqSize;
> +  UINTN RxBufSize;
> +  UINT16RxAlwaysPending;
> +  UINTN PktIdx;
> +  UINT16DescIdx;
> +  UINTN NumBytes;
> +  EFI_PHYSICAL_ADDRESS  RxBufDeviceAddress;
> +  VOID  *RxBuffer;
>  
>//
>// In VirtIo 1.0, the NumBuffers field is mandatory. In 0.9.5, it depends 
> on
> @@ -286,11 +290,37 @@ VirtioNetInitRx (
>//
>RxAlwaysPending = (UINT16) MIN (Dev->RxRing.QueueSize / 2, 
> VNET_MAX_PENDING);
>  
> -  Dev->RxBuf = AllocatePool (RxAlwaysPending * RxBufSize);
> -  if (Dev->RxBuf == NULL) {
> -return EFI_OUT_OF_RESOURCES;
> +  //
> +  // The RxBuf is shared between guest and hypervisor, use
> +  // AllocateSharedPages() to allocate this memory region and map it with
> +  // BusMasterCommonBuffer so that it can be accessed by both 

Re: [edk2] [PATCH 0/4] Implement NULL pointer detection feature for special pool

[Wang, Jian J]

My git has problem in email send. Please ignore these serial patches. I'll send 
new ones later.

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
w...@ml01.01.org
Sent: Wednesday, September 13, 2017 4:45 PM
To: edk2-devel@lists.01.org
Subject: [edk2] [PATCH 0/4] Implement NULL pointer detection feature for 
special pool

The mechanism behind is to trigger a page fault exception at address 0. This 
can be made by disabling page 0 (0-4095) during page table setup. So this 
feature can only be available on platform with paging enabled. Once this 
feature is enabled, any code, like CSM, which has to access memory in page 0 
needs to enable this page temporarily in advance and disable it afterwards. 
PcdNullPointerDetectionPropertyMask is used to control and elaborate the use 
cases. For example, BIT7 of this PCD must be set for Windows 7 boot on Qemu if 
BIT0 set; or boot will fail.

Wang, Jian J (4):
  Implement NULL pointer detection in EDK-II Core.
  Implement NULL pointer detection for SMM mode code.
  Update CSM code to temporarily bypass NULL pointer detection if
enabled.
  Update QemuVideoDxe driver to bypass NULL pointer detection if
enabled.

 .../Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c   | 10 +++-
 .../Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h   | 18 +++
 .../Csm/BiosThunk/KeyboardDxe/KeyboardDxe.inf  |  2 +
 .../Csm/LegacyBiosDxe/LegacyBda.c  |  4 ++
 .../Csm/LegacyBiosDxe/LegacyBios.c | 55 ++
 .../Csm/LegacyBiosDxe/LegacyBiosDxe.inf|  2 +
 .../Csm/LegacyBiosDxe/LegacyBiosInterface.h| 23 +
 .../Csm/LegacyBiosDxe/LegacyBootSupport.c  | 33 ++---
 .../Csm/LegacyBiosDxe/LegacyPci.c  | 17 ++-
 IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c  | 41 ++--
 MdeModulePkg/Core/Dxe/DxeMain.inf  |  3 +-
 MdeModulePkg/Core/Dxe/Mem/Page.c   | 21 +
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c  | 47 ++
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.h  | 15 ++
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf|  3 +-
 MdeModulePkg/Core/DxeIplPeim/DxeLoad.c | 53 +
 MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c|  8 +++-
 MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c |  2 +
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c   | 23 ++---
 MdeModulePkg/MdeModulePkg.dec  | 12 +
 OvmfPkg/QemuVideoDxe/Driver.c  | 15 +-
 OvmfPkg/QemuVideoDxe/Qemu.h| 16 +++
 OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf  |  2 +
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c   | 11 +
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c  | 25 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h |  2 +
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf   | 17 +++
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c| 11 +
 28 files changed, 429 insertions(+), 62 deletions(-)

-- 
2.14.1.windows.1


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

Re: [edk2] [edk2-platforms] Spi/MvSpiDxe likely NullPtrException

[Leif Lindholm]

Hi Pipat,

It is useful to prefix emails regarding edk2-platforms with
[edk2-platforms] on the subject line.

+Jan Dąbroś  (original author, a fact that has been
lost in the migration from OpenPlatformPkg.
also +Marcin Wojtas , Marvell platform owner.
(Neither of which are obvious from looking at the source tree.)

/
Leif

On Wed, Sep 13, 2017 at 12:20:33AM +, methavanitpong.pi...@socionext.com 
wrote:
> Hi,
> 
> MvSpiDxe.c:MvSpiTransfer() attempts load data into DataInPtr, which is
> a possibly NULL pointer. 
> 
> 
> https://github.com/tianocore/edk2-platforms/blob/master/Platform/Marvell/Drivers/Spi/MvSpiDxe.c#L228
> / SNIP /
> for (Iterator = 0; Iterator < SPI_TIMEOUT; Iterator++) {
>   if (MmioRead32 (SpiRegBase + SPI_INT_CAUSE_REG)) {
> *DataInPtr = MmioRead32 (SpiRegBase + SPI_DATA_IN_REG);
> 
> 
> One example that DataInPtr == NULL is in MvSpiDxe.c:MvReadWrite() in 
> the first MvSpiTransfer() call. 
> 
> 
> https://github.com/tianocore/edk2-platforms/blob/master/Platform/Marvell/Drivers/Spi/MvSpiDxe.c#L276
> / SNIP /
> Status = MvSpiTransfer (This, Slave, CmdSize, Cmd, NULL, 
> SPI_TRANSFER_BEGIN);
> if (EFI_ERROR (Status)) {
>   Print (L"Spi Transfer Error\n");
>   return EFI_DEVICE_ERROR;
> }
> 
> Status = MvSpiTransfer (This, Slave, DataSize, DataOut, DataIn, 
> SPI_TRANSFER_END);
> if (EFI_ERROR (Status)) {
>   Print (L"Spi Transfer Error\n");
>   return EFI_DEVICE_ERROR;
> }
> 
> 
> It should store data from SPI_DATA_IN_REG in a temporary variable first, 
> then passes the data into DataInPtr if DataInPtr != NULL. 
> --
> Pipat Methavanitpong
> Software Developer, S-Project 3
> Socionext Inc.
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH 1/4] MdeModulePkg/Core: Implement NULL pointer detection in EDK-II Core.

[Wang]

The mechanism behind is to trigger a page fault exception at address 0. This 
can be made by disabling page 0 (0-4095) during page table setup. So this 
feature can only be available on platform with paging enabled. Once this 
feature is enabled, any code, like CSM, which has to access memory in page 0 
needs to enable this page temporarily in advance and disable it afterwards. 
PcdNullPointerDetectionPropertyMask is used to control and elaborate the use 
cases.

Cc: Jiewen Yao 
Cc: Eric Dong 
Cc: Star Zeng 
Cc: Laszlo Ersek 
Cc: Justen, Jordan L 
Cc: Kinney, Michael D 
Cc: Wolman, Ayellet 
Suggested-by: Wolman, Ayellet 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wang, Jian J 
---
 MdeModulePkg/Core/Dxe/DxeMain.inf|  3 +-
 MdeModulePkg/Core/Dxe/Mem/Page.c | 21 ++
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c| 47 +
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.h| 15 +++
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf  |  3 +-
 MdeModulePkg/Core/DxeIplPeim/DxeLoad.c   | 53 
 MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  8 +++-
 MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 23 ++
 MdeModulePkg/MdeModulePkg.dec| 12 ++
 10 files changed, 167 insertions(+), 20 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf 
b/MdeModulePkg/Core/Dxe/DxeMain.inf
index 30d5984f7c..273b8b7c0e 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.inf
+++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
@@ -179,7 +179,7 @@
   gEfiWatchdogTimerArchProtocolGuid ## CONSUMES
 
 [FeaturePcd]
-  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport ## 
CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport  ## 
CONSUMES
 
 [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdLoadFixAddressBootTimeCodePageNumber## 
SOMETIMES_CONSUMES
@@ -192,6 +192,7 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable   ## 
CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy   ## 
CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy ## 
CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask## 
CONSUMES
 
 # [Hob]
 # RESOURCE_DESCRIPTOR   ## CONSUMES
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index a142c79ee2..2e0b72f864 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -170,6 +170,7 @@ CoreAddRange (
 {
   LIST_ENTRY*Link;
   MEMORY_MAP*Entry;
+  EFI_STATUSStatus;
 
   ASSERT ((Start & EFI_PAGE_MASK) == 0);
   ASSERT (End > Start) ;
@@ -188,7 +189,17 @@ CoreAddRange (
   // used for other purposes.
   //  
   if (Type == EfiConventionalMemory && Start == 0 && (End >= EFI_PAGE_SIZE - 
1)) {
-SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
+if ((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT0) == 0) {
+  SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
+} else if (gCpu != NULL) {
+  Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, 0);
+  ASSERT_EFI_ERROR(Status);
+
+  SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
+
+  Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, 
EFI_MEMORY_RP);
+  ASSERT_EFI_ERROR(Status);
+}
   }
   
   //
@@ -1972,11 +1983,3 @@ Done:
   return Status;
 }
 
-
-
-
-
-
-
-
-
diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c 
b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index a73c4ccd64..2367d674e1 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -995,6 +995,36 @@ MemoryProtectionExitBootServicesCallback (
   }
 }
 
+/**
+  Disable NULL pointer detection after EndOfDxe. This is a workaround resort 
in 
+  order to skip unfixable NULL pointer access issues detected in OptionROM or 
+  boot loaders.
+
+  @param[in]  Event The Event this notify function registered to.
+  @param[in]  Context   Pointer to the context data registered to the Event.
+**/
+VOID
+EFIAPI
+DisableNullDetectionAtTheEndOfDxe (
+  EFI_EVENT   Event,
+  VOID*Context
+  )
+{
+  EFI_STATUSStatus;
+
+  DEBUG((DEBUG_INFO, "DisableNullDetectionAtTheEndOfDxe(): start\r\n"));
+  //
+  // Disable NULL pointer detection by enabling first 4K page
+  //
+  Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, 0);
+  ASSERT_EFI_ERROR(Status);
+
+  CoreCloseEvent (Event);
+  DEBUG((DEBUG_INFO, "DisableNullDetectionAtTheEndOfDxe(): end\r\n"));
+
+  

[edk2] [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code.

[Wang]

The mechanism behind is the same as NULL pointer detection enabled in EDK-II 
core. SMM has its own page table and we have to disable page 0 again in SMM 
mode.

Cc: Jiewen Yao 
Cc: Eric Dong 
Cc: Star Zeng 
Cc: Laszlo Ersek 
Cc: Justen, Jordan L 
Cc: Kinney, Michael D 
Cc: Wolman, Ayellet 
Suggested-by: Wolman, Ayellet 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wang, Jian J 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 11 +++
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c| 25 -
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   |  2 ++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 17 +
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c  | 11 +++
 5 files changed, 57 insertions(+), 9 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
index f295c2ebf2..d423958783 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
@@ -155,6 +155,17 @@ SmiPFHandler (
 }
   }
 
+  //
+  // If NULL pointer was just accessed
+  //
+  if (NULL_DETECTION_ENABLED && (PFAddress >= 0 && PFAddress < EFI_PAGE_SIZE)) 
{
+DEBUG ((DEBUG_ERROR, "!!! NULL pointer access !!!\n"));
+DEBUG_CODE (
+  DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextIa32->Rip);
+);
+CpuDeadLoop ();
+  }
+
   if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
 SmmProfilePFHandler (
   SystemContext.SystemContextIa32->Eip,
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index f086b97c30..81c5ac9d11 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -855,10 +855,10 @@ Gen4GPageTable (
 Pte[Index] = (Index << 21) | mAddressEncMask | IA32_PG_PS | 
PAGE_ATTRIBUTE_BITS;
   }
 
+  Pdpte = (UINT64*)PageTable;
   if (FeaturePcdGet (PcdCpuSmmStackGuard)) {
 Pages = (UINTN)PageTable + EFI_PAGES_TO_SIZE (5);
 GuardPage = mSmmStackArrayBase + EFI_PAGE_SIZE;
-Pdpte = (UINT64*)PageTable;
 for (PageIndex = Low2MBoundary; PageIndex <= High2MBoundary; PageIndex += 
SIZE_2MB) {
   Pte = (UINT64*)(UINTN)(Pdpte[BitFieldRead32 ((UINT32)PageIndex, 30, 31)] 
& ~mAddressEncMask & ~(EFI_PAGE_SIZE - 1));
   Pte[BitFieldRead32 ((UINT32)PageIndex, 21, 29)] = (UINT64)Pages | 
mAddressEncMask | PAGE_ATTRIBUTE_BITS;
@@ -886,6 +886,29 @@ Gen4GPageTable (
 }
   }
 
+  if (NULL_DETECTION_ENABLED) {
+Pte = (UINT64*)(UINT64)(Pdpte[0] & ~mAddressEncMask & ~(EFI_PAGE_SIZE - 
1));
+if ((Pte[0] & IA32_PG_PS) == 0) {
+  // 4K-page entries are already mapped. Just hide the first one anyway.
+  Pte = (UINT64*)(UINT64)(Pte[0] & ~mAddressEncMask & ~(EFI_PAGE_SIZE - 
1));
+  Pte[0] &= ~1; // Hide page 0
+} else {
+  // Create 4K-page entries
+  Pages = (UINTN)AllocatePageTableMemory (1);
+  ASSERT (Pages != 0);
+
+  Pte[0] = (UINT64)(Pages | mAddressEncMask | PAGE_ATTRIBUTE_BITS);
+
+  Pte = (UINT64*)Pages;
+  PageAddress = 0;
+  Pte[0] = PageAddress | mAddressEncMask; // Hide page 0 but present left
+  for (Index = 1; Index < EFI_PAGE_SIZE / sizeof (*Pte); Index++) {
+PageAddress += EFI_PAGE_SIZE;
+Pte[Index] = PageAddress | mAddressEncMask | PAGE_ATTRIBUTE_BITS;
+  }
+}
+  }
+
   return (UINT32)(UINTN)PageTable;
 }
 
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 1cf85c1481..bcb3032db8 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -153,6 +153,8 @@ typedef UINT32  
SMM_CPU_ARRIVAL_EXCEPTIONS;
 #define ARRIVAL_EXCEPTION_DELAYED   0x2
 #define ARRIVAL_EXCEPTION_SMI_DISABLED  0x4
 
+#define NULL_DETECTION_ENABLED
((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT1) != 0)
+
 //
 // Private structure for the SMM CPU module that is stored in DXE Runtime 
memory
 // Contains the SMM Configuration Protocols that is produced.
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
index 099792e6ce..57a14d9f24 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
@@ -138,14 +138,14 @@
   gEdkiiPiSmmMemoryAttributesTableGuid ## CONSUMES ## SystemTable
 
 [FeaturePcd]
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmDebug ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmBlockStartupThisAp## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport   ## CONSUMES
-  

[edk2] [PATCH 3/4] IntelFrameworkModulePkg/Csm: Update CSM code to temporarily bypass NULL pointer detection if enabled.

[Wang]

CSM code has to access memory below 4096 (BDA, int vector, etc.). If NULL 
pointer detection is enabled, the page 0 must be enabled temporarily before 
accessing it and disabled again afterwards. Otherwise page fault will be 
triggered.

Cc: Jiewen Yao 
Cc: Eric Dong 
Cc: Star Zeng 
Cc: Laszlo Ersek 
Cc: Justen, Jordan L 
Cc: Kinney, Michael D 
Cc: Wolman, Ayellet 
Suggested-by: Wolman, Ayellet 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wang, Jian J 
---
 .../Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c   | 10 +++-
 .../Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h   | 18 +++
 .../Csm/BiosThunk/KeyboardDxe/KeyboardDxe.inf  |  2 +
 .../Csm/LegacyBiosDxe/LegacyBda.c  |  4 ++
 .../Csm/LegacyBiosDxe/LegacyBios.c | 55 ++
 .../Csm/LegacyBiosDxe/LegacyBiosDxe.inf|  2 +
 .../Csm/LegacyBiosDxe/LegacyBiosInterface.h| 23 +
 .../Csm/LegacyBiosDxe/LegacyBootSupport.c  | 33 ++---
 .../Csm/LegacyBiosDxe/LegacyPci.c  | 17 ++-
 IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c  | 41 ++--
 10 files changed, 173 insertions(+), 32 deletions(-)

diff --git a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c 
b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c
index 7308523ad8..96148ae367 100644
--- a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c
+++ b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c
@@ -248,7 +248,7 @@ BiosKeyboardDriverBindingStart (
   //
   // Allocate the private device structure
   //
-BiosKeyboardPrivate = (BIOS_KEYBOARD_DEV *) AllocateZeroPool (sizeof 
(BIOS_KEYBOARD_DEV));
+  BiosKeyboardPrivate = (BIOS_KEYBOARD_DEV *) AllocateZeroPool (sizeof 
(BIOS_KEYBOARD_DEV));
   if (NULL == BiosKeyboardPrivate) {
 Status = EFI_OUT_OF_RESOURCES;
 goto Done;
@@ -281,6 +281,9 @@ BiosKeyboardDriverBindingStart (
   BiosKeyboardPrivate->SimpleTextInputEx.UnregisterKeyNotify = 
BiosKeyboardUnregisterKeyNotify;
   InitializeListHead (>NotifyList);
 
+  Status = gBS->LocateProtocol (, NULL, (VOID **) 
>Cpu);
+  ASSERT_EFI_ERROR(Status);
+
   //
   // Report that the keyboard is being enabled
   //
@@ -1842,7 +1845,9 @@ BiosKeyboardTimerHandler (
   //
   // Clear the CTRL and ALT BDA flag
   //
-  KbFlag1 = *((UINT8 *) (UINTN) 0x417);  // read the STATUS FLAGS 1
+  DISABLE_NULL_DETECTION(BiosKeyboardPrivate);
+
+  KbFlag1 = *((UINT8 *) (UINTN) 0x417); // read the STATUS FLAGS 1
   KbFlag2 = *((UINT8 *) (UINTN) 0x418); // read STATUS FLAGS 2
 
   DEBUG_CODE (
@@ -1916,6 +1921,7 @@ BiosKeyboardTimerHandler (
   KbFlag1 &= ~0x0C;  
   *((UINT8 *) (UINTN) 0x417) = KbFlag1; 
 
+  ENABLE_NULL_DETECTION(BiosKeyboardPrivate);
   
   //
   // Output EFI input key and shift/toggle state
diff --git a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h 
b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h
index 0bf28ea140..b717ef676b 100644
--- a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h
+++ b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h
@@ -26,6 +26,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -212,6 +213,7 @@ typedef struct {
   EFI_HANDLE  Handle;
   EFI_LEGACY_BIOS_PROTOCOL*LegacyBios;
   EFI_ISA_IO_PROTOCOL *IsaIo;
+  EFI_CPU_ARCH_PROTOCOL   *Cpu;
   EFI_SIMPLE_TEXT_INPUT_PROTOCOL  SimpleTextIn;
   EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL   SimpleTextInputEx;
   UINT16  DataRegisterAddress;
@@ -242,6 +244,22 @@ typedef struct {
   BIOS_KEYBOARD_DEV_SIGNATURE \
   )
 
+//
+// CSM needs to access memory between 0-4095, which will cause page fault 
exception 
+// if NULL pointer detection mechanism is enabled. Following macros can be used
+// to disable/enable NULL pointer detection before/after accessing those 
memory.
+//
+#define NULL_POINTER_DETECTION_ENABLED  
((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT0) != 0)
+#define DISABLE_NULL_DETECTION(Instance)   
 \
+  if (NULL_POINTER_DETECTION_ENABLED && (Instance)->Cpu != NULL) { 
 \
+(Instance)->Cpu->SetMemoryAttributes((Instance)->Cpu, 0, EFI_PAGE_SIZE, 
0); \
+  }
+  
+#define ENABLE_NULL_DETECTION(Instance)
 \
+  if (NULL_POINTER_DETECTION_ENABLED && (Instance)->Cpu != NULL) { 
 \
+(Instance)->Cpu->SetMemoryAttributes((Instance)->Cpu, 0, 

[edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled.

[Wang]

QemuVideoDxe driver will install VBE SHIM into page 0. If NULL pointer 
detection is enabled, page 0 must be enabled temporarily before installing and 
disabled again afterwards. For Windows 7 boot, BIT7 of 
PcdNullPointerDetectionPropertyMask must still be set to avoid hang.

Cc: Jiewen Yao 
Cc: Eric Dong 
Cc: Star Zeng 
Cc: Laszlo Ersek 
Cc: Justen, Jordan L 
Cc: Kinney, Michael D 
Cc: Wolman, Ayellet 
Suggested-by: Wolman, Ayellet 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wang, Jian J 
---
 OvmfPkg/QemuVideoDxe/Driver.c | 15 ++-
 OvmfPkg/QemuVideoDxe/Qemu.h   | 16 
 OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf |  2 ++
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
index 0dce80e59b..ee0eed7214 100644
--- a/OvmfPkg/QemuVideoDxe/Driver.c
+++ b/OvmfPkg/QemuVideoDxe/Driver.c
@@ -194,6 +194,7 @@ QemuVideoControllerDriverStart (
   PCI_TYPE00Pci;
   QEMU_VIDEO_CARD   *Card;
   EFI_PCI_IO_PROTOCOL   *ChildPciIo;
+  EFI_CPU_ARCH_PROTOCOL *Cpu;
 
   OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
 
@@ -479,7 +480,19 @@ QemuVideoControllerDriverStart (
 #if defined MDE_CPU_IA32 || defined MDE_CPU_X64
   if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO ||
   Private->Variant == QEMU_VIDEO_BOCHS) {
-InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase);
+//
+// Prepare CPU arch protocol for NULL pointer detection
+//
+Status = gBS->LocateProtocol (
+,
+NULL, 
+(VOID **) 
+);
+ASSERT_EFI_ERROR (Status);
+
+DISABLE_NULL_DETECTION(Cpu);
+  InstallVbeShim (Card->Name, 
Private->GraphicsOutput.Mode->FrameBufferBase);
+ENABLE_NULL_DETECTION(Cpu);
   }
 #endif
 
diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h
index 7fbb25b3ef..bb3bc6eb0f 100644
--- a/OvmfPkg/QemuVideoDxe/Qemu.h
+++ b/OvmfPkg/QemuVideoDxe/Qemu.h
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -82,6 +83,21 @@ typedef struct {
 
 #define GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER  0x
 
+//
+// VBE code will access memory between 0-4095 which will cause page fault 
exception 
+// if NULL pointer detection mechanism is enabled. Following macros can be 
used to 
+// disable/enable NULL pointer detection before/after accessing those memory.
+//
+#define NULL_DETECTION_ENABLED  ((PcdGet8(PcdNullPointerDetectionPropertyMask) 
& (BIT0|BIT7)) == BIT0)
+#define DISABLE_NULL_DETECTION(Cpu)
 \
+  if (NULL_DETECTION_ENABLED) {
 \
+(Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, 0);
 \
+  }
+#define ENABLE_NULL_DETECTION(Cpu) 
 \
+  if (NULL_DETECTION_ENABLED) {
 \
+(Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, EFI_MEMORY_RP);
 \
+  }
+
 //
 // QEMU Video Private Data Structure
 //
diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf 
b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
index 7c7d429bca..5d166eb99c 100644
--- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
+++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
@@ -72,7 +72,9 @@
   gEfiGraphicsOutputProtocolGuid# PROTOCOL BY_START
   gEfiDevicePathProtocolGuid# PROTOCOL BY_START
   gEfiPciIoProtocolGuid # PROTOCOL TO_START
+  gEfiCpuArchProtocolGuid
 
 [Pcd]
   gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
 
-- 
2.14.1.windows.1


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

[edk2] [PATCH 0/4] Implement NULL pointer detection feature for special pool

[Wang]

The mechanism behind is to trigger a page fault exception at address 0. This 
can be made by disabling page 0 (0-4095) during page table setup. So this 
feature can only be available on platform with paging enabled. Once this 
feature is enabled, any code, like CSM, which has to access memory in page 0 
needs to enable this page temporarily in advance and disable it afterwards. 
PcdNullPointerDetectionPropertyMask is used to control and elaborate the use 
cases. For example, BIT7 of this PCD must be set for Windows 7 boot on Qemu if 
BIT0 set; or boot will fail.

Wang, Jian J (4):
  Implement NULL pointer detection in EDK-II Core.
  Implement NULL pointer detection for SMM mode code.
  Update CSM code to temporarily bypass NULL pointer detection if
enabled.
  Update QemuVideoDxe driver to bypass NULL pointer detection if
enabled.

 .../Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c   | 10 +++-
 .../Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h   | 18 +++
 .../Csm/BiosThunk/KeyboardDxe/KeyboardDxe.inf  |  2 +
 .../Csm/LegacyBiosDxe/LegacyBda.c  |  4 ++
 .../Csm/LegacyBiosDxe/LegacyBios.c | 55 ++
 .../Csm/LegacyBiosDxe/LegacyBiosDxe.inf|  2 +
 .../Csm/LegacyBiosDxe/LegacyBiosInterface.h| 23 +
 .../Csm/LegacyBiosDxe/LegacyBootSupport.c  | 33 ++---
 .../Csm/LegacyBiosDxe/LegacyPci.c  | 17 ++-
 IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c  | 41 ++--
 MdeModulePkg/Core/Dxe/DxeMain.inf  |  3 +-
 MdeModulePkg/Core/Dxe/Mem/Page.c   | 21 +
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c  | 47 ++
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.h  | 15 ++
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf|  3 +-
 MdeModulePkg/Core/DxeIplPeim/DxeLoad.c | 53 +
 MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c|  8 +++-
 MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c |  2 +
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c   | 23 ++---
 MdeModulePkg/MdeModulePkg.dec  | 12 +
 OvmfPkg/QemuVideoDxe/Driver.c  | 15 +-
 OvmfPkg/QemuVideoDxe/Qemu.h| 16 +++
 OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf  |  2 +
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c   | 11 +
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c  | 25 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h |  2 +
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf   | 17 +++
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c| 11 +
 28 files changed, 429 insertions(+), 62 deletions(-)

-- 
2.14.1.windows.1


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

Re: [edk2] [PATCH v2 2/8] OvmfPkg/VirtioNetDxe: map VRINGs using VirtioRingMap()

[Laszlo Ersek]

On 09/11/17 14:16, Brijesh Singh wrote:
> When device is behind the IOMMU then driver need to pass the device
> address when programing the bus master. The patch uses VirtioRingMap() to
> map the VRING system physical address[es] to device address[es].
>
> Cc: Ard Biesheuvel 
> Cc: Jordan Justen 
> Cc: Tom Lendacky 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh 
> ---
>  OvmfPkg/VirtioNetDxe/VirtioNet.h|  7 ++-
>  OvmfPkg/VirtioNetDxe/SnpInitialize.c| 50 +++-
>  OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 10 ++--
>  OvmfPkg/VirtioNetDxe/SnpShutdown.c  |  4 +-
>  OvmfPkg/VirtioNetDxe/TechNotes.txt  |  1 +
>  5 files changed, 55 insertions(+), 17 deletions(-)
>
> diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h 
> b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> index 87a0f06e01a4..6762fc9d1d6e 100644
> --- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
> +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> @@ -82,10 +82,14 @@ typedef struct {
>EFI_HANDLE  MacHandle; // 
> VirtioNetDriverBindingStart
>
>VRING   RxRing;// VirtioNetInitRing
> +  VOID*RxRingMap;// VirtioRingMap and
> + // VirtioNetInitRing
>UINT8   *RxBuf;// VirtioNetInitRx
>UINT16  RxLastUsed;// VirtioNetInitRx
>
>VRING   TxRing;// VirtioNetInitRing
> +  VOID*TxRingMap;// VirtioRingMap and
> + // VirtioNetInitRing
>UINT16  TxMaxPending;  // VirtioNetInitTx
>UINT16  TxCurPending;  // VirtioNetInitTx
>UINT16  *TxFreeStack;  // VirtioNetInitTx
> @@ -267,7 +271,8 @@ VOID
>  EFIAPI
>  VirtioNetUninitRing (
>IN OUT VNET_DEV *Dev,
> -  IN OUT VRING*Ring
> +  IN OUT VRING*Ring,
> +  IN VOID *RingMap
>);
>
>  //
> diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c 
> b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> index 637c978709fd..8eabdbff6f5e 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> @@ -35,11 +35,13 @@
> the network device.
>@param[out]Ring  The virtio-ring inside the VNET_DEV structure,
> corresponding to Selector.
> +  @param[out]Mapping   A resulting token to pass to VirtioNetUninitRing()
>
>@retval EFI_UNSUPPORTED  The queue size reported by the virtio-net device 
> is
> too small.
>@return  Status codes from VIRTIO_CFG_WRITE(),
> -   VIRTIO_CFG_READ() and VirtioRingInit().
> +   VIRTIO_CFG_READ(), VirtioRingInit() and
> +   VirtioRingMap().
>@retval EFI_SUCCESS  Ring initialized.
>  */
>
> @@ -49,11 +51,14 @@ EFIAPI
>  VirtioNetInitRing (
>IN OUT VNET_DEV *Dev,
>IN UINT16   Selector,
> -  OUTVRING*Ring
> +  OUTVRING*Ring,
> +  OUTVOID **Mapping
>)
>  {
>EFI_STATUS Status;
>UINT16 QueueSize;
> +  UINT64 RingBaseShift;
> +  VOID   *MapInfo;
>
>//
>// step 4b -- allocate selected queue
> @@ -80,29 +85,42 @@ VirtioNetInitRing (
>}
>
>//
> +  // If anything fails from here on, we must release the ring resources.
> +  //
> +  Status = VirtioRingMap (Dev->VirtIo, Ring, , );
> +  if (EFI_ERROR (Status)) {
> +goto ReleaseQueue;
> +  }
> +
> +  //
>// Additional steps for MMIO: align the queue appropriately, and set the
> -  // size. If anything fails from here on, we must release the ring 
> resources.
> +  // size. If anything fails from here on, we must unmap the ring resources.
>//
>Status = Dev->VirtIo->SetQueueNum (Dev->VirtIo, QueueSize);
>if (EFI_ERROR (Status)) {
> -goto ReleaseQueue;
> +goto UnmapQueue;
>}
>
>Status = Dev->VirtIo->SetQueueAlign (Dev->VirtIo, EFI_PAGE_SIZE);
>if (EFI_ERROR (Status)) {
> -goto ReleaseQueue;
> +goto UnmapQueue;
>}
>
>//
>// step 4c -- report GPFN (guest-physical frame number) of queue
>//
> -  Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, Ring, 0);
> +  Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, Ring, RingBaseShift);
>if (EFI_ERROR (Status)) {
> -goto ReleaseQueue;
> +goto UnmapQueue;
>}
>
> +  *Mapping = MapInfo;
> +
>return EFI_SUCCESS;
>
> +UnmapQueue:
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, MapInfo);
> +
>  ReleaseQueue:
>VirtioRingUninit (Dev->VirtIo, Ring);
>
> @@ -456,12 +474,22 @@ VirtioNetInitialize (
>//
>// step 4b, 4c -- allocate and report virtqueues
>   

Re: [edk2] [patch] MdeModulePkg: Add UdfDxe to the dsc file

[Zeng, Star]

Reviewed-by: Star Zeng 

-Original Message-
From: Bi, Dandan 
Sent: Wednesday, September 13, 2017 3:37 PM
To: edk2-devel@lists.01.org
Cc: Zeng, Star ; Dong, Eric ; Ni, 
Ruiyu 
Subject: [patch] MdeModulePkg: Add UdfDxe to the dsc file

Cc: Star Zeng 
Cc: Eric Dong 
Cc: Ruiyu Ni 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Dandan Bi 
---
 MdeModulePkg/MdeModulePkg.dsc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc 
index ad85776..ad34254 100644
--- a/MdeModulePkg/MdeModulePkg.dsc
+++ b/MdeModulePkg/MdeModulePkg.dsc
@@ -347,10 +347,11 @@
   MdeModulePkg/Universal/DebugPortDxe/DebugPortDxe.inf
   MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
   MdeModulePkg/Universal/PrintDxe/PrintDxe.inf
   MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIoDxe.inf
   MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf
+  MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf
   MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf
   MdeModulePkg/Universal/Disk/CdExpressPei/CdExpressPei.inf
   MdeModulePkg/Universal/DriverSampleDxe/DriverSampleDxe.inf
   MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf
   
MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/GenericMemoryTestDxe.inf
--
1.9.5.msysgit.1

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

[edk2] [patch] MdeModulePkg: Add UdfDxe to the dsc file

[Dandan Bi]

Cc: Star Zeng 
Cc: Eric Dong 
Cc: Ruiyu Ni 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Dandan Bi 
---
 MdeModulePkg/MdeModulePkg.dsc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc
index ad85776..ad34254 100644
--- a/MdeModulePkg/MdeModulePkg.dsc
+++ b/MdeModulePkg/MdeModulePkg.dsc
@@ -347,10 +347,11 @@
   MdeModulePkg/Universal/DebugPortDxe/DebugPortDxe.inf
   MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
   MdeModulePkg/Universal/PrintDxe/PrintDxe.inf
   MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIoDxe.inf
   MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf
+  MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf
   MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf
   MdeModulePkg/Universal/Disk/CdExpressPei/CdExpressPei.inf
   MdeModulePkg/Universal/DriverSampleDxe/DriverSampleDxe.inf
   MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf
   
MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/GenericMemoryTestDxe.inf
-- 
1.9.5.msysgit.1

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

Re: [edk2] [PATCH v2 1/8] OvmfPkg/VirtioNetDxe: add helper VirtioNetUninitRing()

[Laszlo Ersek]

On 09/11/17 14:16, Brijesh Singh wrote:
> Consolidate the virtio VRING resource cleanup into VirtioNetUninitRing().
> 
> Cc: Ard Biesheuvel 
> Cc: Jordan Justen 
> Cc: Tom Lendacky 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh 
> ---
>  OvmfPkg/VirtioNetDxe/VirtioNet.h|  7 +++
>  OvmfPkg/VirtioNetDxe/SnpInitialize.c|  4 ++--
>  OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 16 
>  OvmfPkg/VirtioNetDxe/SnpShutdown.c  |  4 ++--
>  OvmfPkg/VirtioNetDxe/TechNotes.txt  |  5 +++--
>  5 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h 
> b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> index 710859bc6115..87a0f06e01a4 100644
> --- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
> +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
> @@ -263,6 +263,13 @@ VirtioNetShutdownTx (
>IN OUT VNET_DEV *Dev
>);
>  
> +VOID
> +EFIAPI
> +VirtioNetUninitRing (
> +  IN OUT VNET_DEV *Dev,
> +  IN OUT VRING*Ring
> +  );
> +
>  //
>  // event callbacks
>  //
> diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c 
> b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> index 0ecfe044a977..637c978709fd 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> @@ -510,10 +510,10 @@ AbortDevice:
>Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
>  
>  ReleaseTxRing:
> -  VirtioRingUninit (Dev->VirtIo, >TxRing);
> +  VirtioNetUninitRing (Dev, >TxRing);
>  
>  ReleaseRxRing:
> -  VirtioRingUninit (Dev->VirtIo, >RxRing);
> +  VirtioNetUninitRing (Dev, >RxRing);
>  
>  DeviceFailed:
>//
> diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c 
> b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> index 9fedb72fdbd4..5b75eabc7a6b 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
> @@ -51,3 +51,19 @@ VirtioNetShutdownTx (
>  {
>FreePool (Dev->TxFreeStack);
>  }
> +
> +/**
> +  Release TX and RX VRING resources.
> +
> +  @param[in,out] Dev   The VNET_DEV driver instance which was using the ring.
> +  @param[in,out] Ring  The virtio ring to clean up.
> +*/
> +VOID
> +EFIAPI
> +VirtioNetUninitRing (
> +  IN OUT VNET_DEV *Dev,
> +  IN OUT VRING*Ring
> +  )
> +{
> +  VirtioRingUninit (Dev->VirtIo, Ring);
> +}
> diff --git a/OvmfPkg/VirtioNetDxe/SnpShutdown.c 
> b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
> index 5e84191fbbdd..432e0691d457 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpShutdown.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
> @@ -67,8 +67,8 @@ VirtioNetShutdown (
>Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
>VirtioNetShutdownRx (Dev);
>VirtioNetShutdownTx (Dev);
> -  VirtioRingUninit (Dev->VirtIo, >TxRing);
> -  VirtioRingUninit (Dev->VirtIo, >RxRing);
> +  VirtioNetUninitRing (Dev, >TxRing);
> +  VirtioNetUninitRing (Dev, >RxRing);
>  
>Dev->Snm.State = EfiSimpleNetworkStarted;
>Status = EFI_SUCCESS;
> diff --git a/OvmfPkg/VirtioNetDxe/TechNotes.txt 
> b/OvmfPkg/VirtioNetDxe/TechNotes.txt
> index 9c1dfe6a773e..86b91f561495 100644
> --- a/OvmfPkg/VirtioNetDxe/TechNotes.txt
> +++ b/OvmfPkg/VirtioNetDxe/TechNotes.txt
> @@ -70,8 +70,9 @@ faithfully indented) that implement the transition.
>VirtioNetInitialize  |  | VirtioNetShutdown
>  VirtioNetInitRing {Rx, Tx} |  |   VirtioNetShutdownRx 
> [SnpSharedHelpers.c]
>VirtioRingInit   |  |   VirtioNetShutdownTx 
> [SnpSharedHelpers.c]
> -VirtioNetInitTx|  |   VirtioRingUninit {Tx, Rx}
> -VirtioNetInitRx|  |
> +VirtioNetInitTx|  |   VirtioNetUninitRing 
> [SnpSharedHelpers.c]
> +VirtioNetInitRx|  |   {Tx, Rx}
> +   |  | VirtioRingUninit
> v  |
>+-+
>| EfiSimpleNetworkInitialized |
> 

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

Re: [edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

[Zeng, Star]

Beyond the Rb (I do not want to block this patch series), I am curious about 
one question.

There may be more this kind of workarounds to fix the build failure.
Is it possible to disable the warning (like below example for VS) for specific 
version of GCC for this kind of false alarm?


ProcessorBind.h:
#if defined(_MSC_EXTENSIONS)

...

#if _MSC_VER == 1800 || _MSC_VER == 1900

//
// Disable these warnings for VS2013.
//

//
// This warning is for potentially uninitialized local variable, and it may 
cause false 
// positive issues in VS2013 and VS2015 build
//
#pragma warning ( disable : 4701 )
  
//
// This warning is for potentially uninitialized local pointer variable, and it 
may cause 
// false positive issues in VS2013 and VS2015 build
//
#pragma warning ( disable : 4703 )
  
#endif

#endif


Thanks,
Star
-Original Message-
From: Zeng, Star 
Sent: Wednesday, September 13, 2017 2:34 PM
To: Laszlo Ersek ; edk2-devel-01 
Cc: Ard Biesheuvel ; Dong, Eric 
; Paulo Alcantara ; Ni, Ruiyu 
; Zeng, Star 
Subject: RE: [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler 
warning in ReadFile()

Reviewed-by: Star Zeng 

-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com]
Sent: Wednesday, September 13, 2017 6:26 AM
To: edk2-devel-01 
Cc: Ard Biesheuvel ; Dong, Eric 
; Paulo Alcantara ; Ni, Ruiyu 
; Zeng, Star 
Subject: [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning 
in ReadFile()

When building the driver for DEBUG/RELEASE, GCC48/GCC49 warn about
ReadFile() possibly using "BytesLeft" without initializing it first.

This is not the case. The reads of "BytesLeft" are only reachable if 
(ReadFileInfo->Flags == READ_FILE_SEEK_AND_READ). But, in that case, we also 
set "BytesLeft" to "ReadFileInfo->FileDataSize", near the top of the function.

Assign "BytesLeft" zero at the top, and add a comment that conforms to the 
pending Coding Style Spec feature request at 
.

This issue was reported by Ard's and Gerd's CI systems independently.

Cc: Ard Biesheuvel 
Cc: Eric Dong 
Cc: Paulo Alcantara 
Cc: Ruiyu Ni 
Cc: Star Zeng 
Reported-by: Ard Biesheuvel 
Reported-by: Gerd Hoffmann 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek 
---
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c 
b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 096fbb4452cb..392494b2eb3f 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -893,6 +893,11 @@ ReadFile (
   LogicalBlockSize  = LV_BLOCK_SIZE (Volume, UDF_DEFAULT_LV_NUM);
   DoFreeAed = FALSE;
 
+  //
+  // set BytesLeft to suppress incorrect compiler/analyzer warnings  // 
+ BytesLeft = 0;
+
   switch (ReadFileInfo->Flags) {
   case READ_FILE_GET_FILESIZE:
   case READ_FILE_ALLOCATE_AND_READ:
--
2.14.1.3.gb7cf6e02401b

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

Re: [edk2] [PATCH 1/2] MdeModulePkg/UdfDxe: reject reserved values in ICB.Flags[2:0]

[Zeng, Star]

Reviewed-by: Star Zeng 

-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com] 
Sent: Wednesday, September 13, 2017 6:26 AM
To: edk2-devel-01 
Cc: Ard Biesheuvel ; Dong, Eric 
; Paulo Alcantara ; Ni, Ruiyu 
; Zeng, Star 
Subject: [PATCH 1/2] MdeModulePkg/UdfDxe: reject reserved values in 
ICB.Flags[2:0]

The ECMA-167 standard (3rd Edition, June 1997) reserves values 4 through 7
in the ICB.Flags[2:0] bit-field for future standardization; see "14.6 ICB
Tag" / "14.6.8 Flags (RBP 18)".

https://www.ecma-international.org/publications/standards/Ecma-167.htm

The

  switch (RecordingFlags)

statement in the ReadFile() function handles all the standard values,
using the constants of the UDF_FE_RECORDING_FLAGS enum type. However, the
reserved values are not caught with a "default" case label, which both
breaks the edk2 Coding Style Spec, and leaves the Status variable
un-initialized, before we return Status under the Done label.

Set Status to EFI_UNSUPPORTED if we encounter a reserved value.

This issue was reported by Ard's and Gerd's CI systems independently
(through build failures with GCC48/GCC49, DEBUG/RELEASE targets).

Cc: Ard Biesheuvel 
Cc: Eric Dong 
Cc: Paulo Alcantara 
Cc: Ruiyu Ni 
Cc: Star Zeng 
Reported-by: Ard Biesheuvel 
Reported-by: Gerd Hoffmann 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek 
---
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c 
b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 72862653738e..096fbb4452cb 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -1150,6 +1150,14 @@ ReadFile (
 ASSERT (FALSE);
 Status = EFI_UNSUPPORTED;
 break;
+
+  default:
+//
+// A flag value reserved by the ECMA-167 standard (3rd Edition - June
+// 1997); 14.6 ICB Tag; 14.6.8 Flags (RBP 18); was found.
+//
+Status = EFI_UNSUPPORTED;
+break;
   }
 
 Done:
-- 
2.14.1.3.gb7cf6e02401b


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

Re: [edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

[Zeng, Star]

Reviewed-by: Star Zeng 

-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com] 
Sent: Wednesday, September 13, 2017 6:26 AM
To: edk2-devel-01 
Cc: Ard Biesheuvel ; Dong, Eric 
; Paulo Alcantara ; Ni, Ruiyu 
; Zeng, Star 
Subject: [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning 
in ReadFile()

When building the driver for DEBUG/RELEASE, GCC48/GCC49 warn about
ReadFile() possibly using "BytesLeft" without initializing it first.

This is not the case. The reads of "BytesLeft" are only reachable if 
(ReadFileInfo->Flags == READ_FILE_SEEK_AND_READ). But, in that case, we also 
set "BytesLeft" to "ReadFileInfo->FileDataSize", near the top of the function.

Assign "BytesLeft" zero at the top, and add a comment that conforms to the 
pending Coding Style Spec feature request at 
.

This issue was reported by Ard's and Gerd's CI systems independently.

Cc: Ard Biesheuvel 
Cc: Eric Dong 
Cc: Paulo Alcantara 
Cc: Ruiyu Ni 
Cc: Star Zeng 
Reported-by: Ard Biesheuvel 
Reported-by: Gerd Hoffmann 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek 
---
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c 
b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 096fbb4452cb..392494b2eb3f 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -893,6 +893,11 @@ ReadFile (
   LogicalBlockSize  = LV_BLOCK_SIZE (Volume, UDF_DEFAULT_LV_NUM);
   DoFreeAed = FALSE;
 
+  //
+  // set BytesLeft to suppress incorrect compiler/analyzer warnings  //  
+ BytesLeft = 0;
+
   switch (ReadFileInfo->Flags) {
   case READ_FILE_GET_FILESIZE:
   case READ_FILE_ALLOCATE_AND_READ:
--
2.14.1.3.gb7cf6e02401b

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

Re: [edk2] [PATCH] MdeModulePkg/UdfDxe: Remove negative comparison of unsigned number

[Zeng, Star]

Ok, got it.
Really thanks for the contribution. :)

Reviewed-by: Star Zeng  and pushed the patch at 
35aec96c221b643b26e78305f6acda8ab0647cf3.

Thanks,
Star
-Original Message-
From: Paulo Alcantara [mailto:pca...@zytor.com] 
Sent: Wednesday, September 13, 2017 1:47 PM
To: Zeng, Star ; edk2-devel@lists.01.org
Cc: Dong, Eric ; Ni, Ruiyu ; Bi, 
Dandan 
Subject: RE: [PATCH] MdeModulePkg/UdfDxe: Remove negative comparison of 
unsigned number



On September 13, 2017 2:08:54 AM GMT-03:00, "Zeng, Star"  
wrote:
>I do not understand the context of the code.
>The change is good to fix the build failure, but I want to ask a 
>question before I gave Rb. :)
>
>Is it possible ReadFileInfo->FilePosition less than FilePosition?

Nope. When doing my tests, I briefly looked at code how it's used and also 
added an ASSERT() to make sure it is never lesser than FilePosition.

BTW, I *do* know that the code really needs refactoring, documentation, etc. -- 
 I didnt do that before because I believed that it would never get upstream -- 
since its now -- I will look forward to that in my free time.

Its 2:46am here so I should get some sleep :-) Thanks!

Paulo

>
>
>Thanks,
>Star
>-Original Message-
>From: Paulo Alcantara [mailto:pca...@zytor.com]
>Sent: Wednesday, September 13, 2017 12:45 PM
>To: edk2-devel@lists.01.org
>Cc: Paulo Alcantara ; Zeng, Star 
>; Dong, Eric ; Ni, Ruiyu 
>; Bi, Dandan 
>Subject: [PATCH] MdeModulePkg/UdfDxe: Remove negative comparison of 
>unsigned number
>
>This patch gets rid of a negative comparison of an UINT64 type (Offset) 
>as it'll never evaluate to true.
>
>Cc: Star Zeng 
>Cc: Eric Dong 
>Cc: Ruiyu Ni 
>Cc: Dandan Bi 
>Contributed-under: TianoCore Contribution Agreement 1.1
>Reported-by: Star Zeng 
>Signed-off-by: Paulo Alcantara 
>---
> MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 3 ---
> 1 file changed, 3 deletions(-)
>
>diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>index 7286265373..2039f80289 100644
>--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>@@ -1082,9 +1082,6 @@ ReadFile (
> 
>if (FilePosition + ExtentLength > ReadFileInfo->FilePosition) {
>   Offset = ReadFileInfo->FilePosition - FilePosition;
>-  if (Offset < 0) {
>-Offset = -(Offset);
>-  }
> } else {
>   Offset = 0;
> }
>--
>2.11.0

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [edk2-VfrSpecification PATCH] Add union data type and bit fields in VFR Data Struct Definition

[Dong, Eric]

Reviewed-by:  Eric Dong 

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Dandan Bi
Sent: Monday, September 04, 2017 4:39 PM
To: edk2-devel@lists.01.org
Cc: Dong, Eric ; Gao, Liming 
Subject: [edk2] [edk2-VfrSpecification PATCH] Add union data type and bit 
fields in VFR Data Struct Definition

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

Cc: Eric Dong 
Cc: Liming Gao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dandan Bi 
---
 .../23_vfr_data_struct_definition.md   | 39 ++
 README.md  |  1 +
 2 files changed, 40 insertions(+)

diff --git a/2_vfr_description_in_bnf/23_vfr_data_struct_definition.md 
b/2_vfr_description_in_bnf/23_vfr_data_struct_definition.md
index 6d6730d..4937e6a 100644
--- a/2_vfr_description_in_bnf/23_vfr_data_struct_definition.md
+++ b/2_vfr_description_in_bnf/23_vfr_data_struct_definition.md
@@ -36,10 +36,16 @@ vfrDataStructDefinition ::=
  { "typedef" } "struct"
  { StringIdentifier }
  "{" vfrDataStructFields "}"
  { StringIdentifier } ";"
 
+vfrDataStructDefinition ::=
+ { "typedef" } "union"
+ { StringIdentifier }
+ "{" vfrDataStructFields "}"
+ { StringIdentifier } ";"
+
 vfrDataStructFields ::=
   (
   dataStructField64
 | dataStructField32
 | dataStructField16
@@ -48,10 +54,14 @@ vfrDataStructFields ::=
 | dataStructFieldString
 | dataStructFieldDate
 | dataStructFieldTime
 | dataStructFieldRef
 | dataStructFieldUser
+| dataStructBitField64
+| dataStructBitField32
+| dataStructBitField16
+| dataStructBitField8
   )*
 
 dataStructField64 ::=
   "UINT64"
   StringIdentifier { "[" Number "]" } ";"
@@ -89,10 +99,26 @@ dataStructFieldRef ::=
   StringIdentifier { "[" Number "]" } ";"
 
 dataStructFieldUser ::=
   StringIdentifier
   StringIdentifier { "[" Number "]" } ";"
+
+dataStructBitField64 ::=
+  "UINT64"
+  { StringIdentifier } ":" Number ";"
+
+dataStructBitField32 ::=
+  "UINT32"
+  { StringIdentifier } ":" Number ";"
+
+dataStructBitField16 ::=
+  "UINT16"
+  { StringIdentifier } ":" Number ";"
+
+dataStructBitField8 ::=
+  "UINT8"
+  { StringIdentifier } ":" Number ";"
 ```
 
  BEHAVIORS AND RESTRICTIONS
 
 The data structure definition is in C-style language. `enum` type is not @@ 
-108,10 +134,23 @@ typedef struct {
   UINT8 mU8;
   UINT16 mU16;
   UINT32 mU32[10];
   UINT64 mU64;
 } MyData;
+
+typedef union {
+  UINT16   Field16;
+  UINT8Field8;
+} MyUnionData;
+
+typedef struct {
+  UINT16   Field16;
+  UINT8MyBits1 : 1;
+  UINT8MyBits2 : 3;
+  UINT8MyBits3 : 3;
+  UINT16   MyBits4 : 4;
+} MyBitsData;
 ```
 
 **Unsupported Example of enum type:**
 
 ```c
diff --git a/README.md b/README.md
index 888eb81..0a596eb 100644
--- a/README.md
+++ b/README.md
@@ -89,5 +89,6 @@ Copyright (c) 2007-2017, Intel Corporation. All rights 
reserved.
 | 1.60 | Update syntax for goto, image, questionref and constant value 
opcodes, correct CALLBACK flag to INTEREACTIVE, correct help string for old 
syntax date/time example, and add examples for expression opcodes.   | December 
1, 2011  |
 | 1.70 | Clarify restriction that enum type and struct data filed with 
more than one dimensions array are not supported.   
 | May 18, 2012 
 |
 | 1.80 | Add syntax for warningif opcode, update definition for name/value 
varstore and subtitle opcode, update referenced UEFI spec version info. 
 | Jan 14, 2014 
 |
 | 1.90 | Correct sample code for catenate/match/cond opcode. Add syntax 
for match2 opcode. Add sample code to show the buffer type constant value for 
orderedlist opcode and default opcode.| July 2, 
2015  |
 | 1.91 | Convert to Gitbook

 | April 2017   
 |
+|  | [#683](https://bugzilla.tianocore.org/show_bug.cgi?id=683) VFR 
Spec: Add union data type and bit fields in VFR Data Struct Definition  
|   
|
--
1.9.5.msysgit.1

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