Re: [edk2] Authentication status for signed FVs extracted in PEI

[Cohen, Eugene]

[Corrected the typos with a new version - proofreading is a good thing]


I'm confused about something and hope I can get some help understanding this.

If we have a signed FV that is extracted in PEI it doesn't look like the 
AuthenticationStatus gets propagated to DXE.

The hob doesn't store authentication status and the core produces FVB with 
AuthenticationStatus forced to zero, even though the FV was signed and verified.

This seems to mess up policy code in DXE because it is the AuthenticationStatus 
is not accurate.

MdeModulePkg\Core\Dxe\FwVolBlock\FwVolBlock.c, FwVolBlockDriverInit:

  while ((FvHob.Raw = GetNextHob (EFI_HOB_TYPE_FV, FvHob.Raw)) != NULL) {
//
// Produce an FVB protocol for it
//
ProduceFVBProtocolOnBuffer (FvHob.FirmwareVolume->BaseAddress, 
FvHob.FirmwareVolume->Length, NULL, 0, NULL);
FvHob.Raw = GET_NEXT_HOB (FvHob);
  }

Note the hardcoded zero in the second-to-last argument.

Is this expected?  How would DXE policy code know if the FV was verified in 
PEI?  It looks like the HOB definitions do not propagate PEI-phase 
Authentication status forward.

Thanks,

Eugene

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Cohen, 
Eugene
Sent: Friday, November 06, 2015 11:12 AM
To: edk2-devel@lists.01.org
Cc: Thompson, Mark L. (Boise IPG) 
Subject: [edk2] Authentication status for signed FVs extracted in PEI

I'm confused about something and hope I can help some help understanding this.

If we have a signed FV that is extracted in PEI it doesn't look like the 
AuthenticationStatus gets propagated to DXE.

The hob doesn't store authentication status and the core products FVB with 
AuthenticationStatus forced to zero, even though the FV was signed and verified.

This seems to mess up policy code we want to have in DXE because it is not 
accurate.

MdeModulePkg\Core\Dxe\FwVolBlock\FwVolBlock.c, FwVolBlockDriverInit:

  while ((FvHob.Raw = GetNextHob (EFI_HOB_TYPE_FV, FvHob.Raw)) != NULL) {
//
// Produce an FVB protocol for it
//
ProduceFVBProtocolOnBuffer (FvHob.FirmwareVolume->BaseAddress, 
FvHob.FirmwareVolume->Length, NULL, 0, NULL);
FvHob.Raw = GET_NEXT_HOB (FvHob);
  }

Is this expected?  How would DXE policy code know if the FV was verified in PEI?

Thanks,

Eugene

___
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] ShellPkg: Don't strip positional parameters of quotation marks.

[Qiu Shumin]

Per Shell SPEC 2.1 'Double-quotation marks that surround arguments are not 
stripped in positional parameters'. This patch makes Shell implementation to 
follow SPEC.

Cc: Jaben Carsey 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Qiu Shumin 
---
 ShellPkg/Application/Shell/Shell.c | 30 ++---
 ShellPkg/Application/Shell/Shell.h |  8 
 .../Application/Shell/ShellParametersProtocol.c| 52 --
 .../Application/Shell/ShellParametersProtocol.h| 36 ++-
 ShellPkg/Application/Shell/ShellProtocol.c |  2 +-
 5 files changed, 80 insertions(+), 48 deletions(-)

diff --git a/ShellPkg/Application/Shell/Shell.c 
b/ShellPkg/Application/Shell/Shell.c
index cb9d969..8af6647 100644
--- a/ShellPkg/Application/Shell/Shell.c
+++ b/ShellPkg/Application/Shell/Shell.c
@@ -1863,7 +1863,7 @@ IsValidSplit(
   return (EFI_OUT_OF_RESOURCES);
 }
 TempWalker = (CHAR16*)Temp;
-if (!EFI_ERROR(GetNextParameter(, , 
StrSize(CmdLine {
+if (!EFI_ERROR(GetNextParameter(, , 
StrSize(CmdLine), TRUE))) {
   if (GetOperationType(FirstParameter) == Unknown_Invalid) {
 ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_SHELL_NOT_FOUND), 
ShellInfoObject.HiiHandle, FirstParameter);
 SetLastError(SHELL_NOT_FOUND);
@@ -2029,7 +2029,7 @@ DoHelpUpdate(
 
   Walker = *CmdLine;
   while(Walker != NULL && *Walker != CHAR_NULL) {
-if (!EFI_ERROR(GetNextParameter(, , 
StrSize(*CmdLine {
+if (!EFI_ERROR(GetNextParameter(, , 
StrSize(*CmdLine), TRUE))) {
   if (StrStr(CurrentParameter, L"-?") == CurrentParameter) {
 CurrentParameter[0] = L' ';
 CurrentParameter[1] = L' ';
@@ -2173,7 +2173,7 @@ RunInternalCommand(
   //
   // get the argc and argv updated for internal commands
   //
-  Status = UpdateArgcArgv(ParamProtocol, NewCmdLine, , );
+  Status = UpdateArgcArgv(ParamProtocol, NewCmdLine, Internal_Command, , 
);
   if (!EFI_ERROR(Status)) {
 //
 // Run the internal command.
@@ -2520,7 +2520,7 @@ RunShellCommand(
 return (EFI_OUT_OF_RESOURCES);
   }
   TempWalker = CleanOriginal;
-  if (!EFI_ERROR(GetNextParameter(, , 
StrSize(CleanOriginal {
+  if (!EFI_ERROR(GetNextParameter(, , 
StrSize(CleanOriginal), TRUE))) {
 //
 // Depending on the first parameter we change the behavior
 //
@@ -2767,34 +2767,34 @@ RunScriptFileHandle (
   if (NewScriptFile->Argv != NULL) {
 switch (NewScriptFile->Argc) {
   default:
-Status = ShellCopySearchAndReplace(CommandLine2,  CommandLine, 
PrintBuffSize, L"%9", NewScriptFile->Argv[9], FALSE, TRUE);
+Status = ShellCopySearchAndReplace(CommandLine2,  CommandLine, 
PrintBuffSize, L"%9", NewScriptFile->Argv[9], FALSE, FALSE);
 ASSERT_EFI_ERROR(Status);
   case 9:
-Status = ShellCopySearchAndReplace(CommandLine,  CommandLine2, 
PrintBuffSize, L"%8", NewScriptFile->Argv[8], FALSE, TRUE);
+Status = ShellCopySearchAndReplace(CommandLine,  CommandLine2, 
PrintBuffSize, L"%8", NewScriptFile->Argv[8], FALSE, FALSE);
 ASSERT_EFI_ERROR(Status);
   case 8:
-Status = ShellCopySearchAndReplace(CommandLine2,  CommandLine, 
PrintBuffSize, L"%7", NewScriptFile->Argv[7], FALSE, TRUE);
+Status = ShellCopySearchAndReplace(CommandLine2,  CommandLine, 
PrintBuffSize, L"%7", NewScriptFile->Argv[7], FALSE, FALSE);
 ASSERT_EFI_ERROR(Status);
   case 7:
-Status = ShellCopySearchAndReplace(CommandLine,  CommandLine2, 
PrintBuffSize, L"%6", NewScriptFile->Argv[6], FALSE, TRUE);
+Status = ShellCopySearchAndReplace(CommandLine,  CommandLine2, 
PrintBuffSize, L"%6", NewScriptFile->Argv[6], FALSE, FALSE);
 ASSERT_EFI_ERROR(Status);
   case 6:
-Status = ShellCopySearchAndReplace(CommandLine2,  CommandLine, 
PrintBuffSize, L"%5", NewScriptFile->Argv[5], FALSE, TRUE);
+Status = ShellCopySearchAndReplace(CommandLine2,  CommandLine, 
PrintBuffSize, L"%5", NewScriptFile->Argv[5], FALSE, FALSE);
 ASSERT_EFI_ERROR(Status);
   case 5:
-Status = ShellCopySearchAndReplace(CommandLine,  CommandLine2, 
PrintBuffSize, L"%4", NewScriptFile->Argv[4], FALSE, TRUE);
+Status = ShellCopySearchAndReplace(CommandLine,  CommandLine2, 
PrintBuffSize, L"%4", NewScriptFile->Argv[4], FALSE, FALSE);
 ASSERT_EFI_ERROR(Status);
   case 4:
-Status = ShellCopySearchAndReplace(CommandLine2,  CommandLine, 
PrintBuffSize, L"%3", NewScriptFile->Argv[3], FALSE, TRUE);
+Status = ShellCopySearchAndReplace(CommandLine2,  CommandLine, 
PrintBuffSize, L"%3", NewScriptFile->Argv[3], FALSE, FALSE);
 ASSERT_EFI_ERROR(Status);
   case 3:
-Status = ShellCopySearchAndReplace(CommandLine,  CommandLine2, 

[edk2] [patch] MdeModulePkg:Fix a bug that HttpLib can not parse Ipv6 address correctly.

[Zhang Lubo]

v2:
*Add a Url Parse state machine to indicate the host is ipv6 expressed url and 
can parse
the ipv6 address correctly.

Cc: Fu Siyuan 
Cc: Ye Ting 
CC: Wu Jiaxin 
CC: Gary Ching-Pang Lin 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Zhang Lubo 
---
 MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 33 
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c 
b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
index aeb52d0..500b3c7 100644
--- a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
+++ b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
@@ -68,10 +68,11 @@ typedef enum {
   UrlParserFragmentStart, // "#"
   UrlParserFragment,
   UrlParserUserInfo,
   UrlParserHostStart, // "@"
   UrlParserHost,
+  UrlParserHostIpv6,  // "["(Ipv6 address) "]"
   UrlParserPortStart, // ":"
   UrlParserPort,
   UrlParserStateMax
 } HTTP_URL_PARSE_STATE;
 
@@ -136,17 +137,20 @@ UriPercentDecode (
   This function return the updated state accroding to the input state and next 
character of
   the authority.
 
   @param[in]   Char   Next character.
   @param[in]   State  Current value of the parser state machine.
+  @param[in]   IsRightBracket TRUE if there is an sign ']' in the 
authority component and 
+  indicates the next part is ':' before Port.  
  
 
   @return  Updated state value.
 **/
 HTTP_URL_PARSE_STATE
 NetHttpParseAuthorityChar (
   IN  CHAR8  Char,
-  IN  HTTP_URL_PARSE_STATE   State
+  IN  HTTP_URL_PARSE_STATE   State,
+  IN  BOOLEAN*IsRightBracket
   )
 {
 
   //
   // RFC 3986:
@@ -167,16 +171,31 @@ NetHttpParseAuthorityChar (
   return UrlParserHostStart;
 }
 break;
 
   case UrlParserHost:
-  case UrlParserHostStart:
+  case UrlParserHostStart:  
+if (Char == '[') {
+  return UrlParserHostIpv6;
+}
+
 if (Char == ':') {
   return UrlParserPortStart;
 }
+
 return UrlParserHost;
-
+
+  case UrlParserHostIpv6:  
+if (Char == ']') {
+  *IsRightBracket = TRUE;
+}
+
+if (Char == ':' && *IsRightBracket == TRUE) {
+  return UrlParserPortStart;
+}
+return UrlParserHostIpv6;
+
   case UrlParserPort:
   case UrlParserPortStart:
 return UrlParserPort;
 
   default:
@@ -208,10 +227,11 @@ NetHttpParseAuthority (
   CHAR8 *Authority;
   UINT32Length;
   HTTP_URL_PARSE_STATE  State;
   UINT32Field;
   UINT32OldField;
+  BOOLEAN   IsrightBracket;
   
   ASSERT ((UrlParser->FieldBitMap & BIT (HTTP_URI_FIELD_AUTHORITY)) != 0);
 
   //
   // authority   = [ userinfo "@" ] host [ ":" port ]
@@ -220,16 +240,17 @@ NetHttpParseAuthority (
 State = UrlParserUserInfo;
   } else {
 State = UrlParserHost;
   }
 
+  IsrightBracket = FALSE;
   Field = HTTP_URI_FIELD_MAX;
   OldField = Field;
   Authority = Url + UrlParser->FieldData[HTTP_URI_FIELD_AUTHORITY].Offset;
   Length = UrlParser->FieldData[HTTP_URI_FIELD_AUTHORITY].Length;
   for (Char = Authority; Char < Authority + Length; Char++) {
-State = NetHttpParseAuthorityChar (*Char, State);
+State = NetHttpParseAuthorityChar (*Char, State, );
 switch (State) {
 case UrlParserStateMax:
   return EFI_INVALID_PARAMETER;
 
 case UrlParserHostStart:
@@ -241,10 +262,14 @@ NetHttpParseAuthority (
   break;
   
 case UrlParserHost:
   Field = HTTP_URI_FIELD_HOST;
   break;
+
+case UrlParserHostIpv6:
+  Field = HTTP_URI_FIELD_HOST;
+  break;
   
 case UrlParserPort:
   Field = HTTP_URI_FIELD_PORT;
   break;
 
-- 
1.9.5.msysgit.1

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

Re: [edk2] [PATCH] NetworkPkg/HttpDxe: Missing CloseEvent() in HttpResponseWorker

[Fu, Siyuan]

Hegde,

The patch is good.

Reviewed-by: Fu Siyuan 


-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Nagaraj 
Hegde
Sent: Tuesday, November 3, 2015 5:11 PM
To: edk2-devel@lists.01.org
Cc: Ye, Ting ; Fu, Siyuan ; Nagaraj 
Hegde 
Subject: [edk2] [PATCH] NetworkPkg/HttpDxe: Missing CloseEvent() in 
HttpResponseWorker

Two additional scenarios in which CloseEvent() needs to be called:

We sent a Request to HTTP server, following which we did a
Response() call, with the intent of capturing only the headers.
In this case, we execute an if condition in HttpResponseWorker do a goto Exit. 
HttpDxe will cache the body that it received for which we dint ask for right 
now. After we have received the headers, we call a Response() again. Now, we 
get the data out of cache and again do a goto Exit. In both cases, two 
CreateEvent() are called, one on Event in RxToken in Wrap structure and another 
on Event in RxToken in HttpInstance structure. In Exit label, we are missing 
the CloseEvent() call for both the Events created.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Nagaraj Hegde 
---
 NetworkPkg/HttpDxe/HttpImpl.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c 
index 3094400..0e7e60d 100644
--- a/NetworkPkg/HttpDxe/HttpImpl.c
+++ b/NetworkPkg/HttpDxe/HttpImpl.c
@@ -1130,6 +1130,18 @@ Exit:
   }
   Token->Status = Status;
   gBS->SignalEvent (Token->Event);
+
+  if (Wrap != NULL) {
+if (Wrap->TcpWrap.RxToken.CompletionToken.Event != NULL) {
+  gBS->CloseEvent (Wrap->TcpWrap.RxToken.CompletionToken.Event);
+}
+  }
+
+  if (HttpInstance->RxToken.CompletionToken.Event != NULL) {
+gBS->CloseEvent (HttpInstance->RxToken.CompletionToken.Event);
+HttpInstance->RxToken.CompletionToken.Event = NULL;  }
+
   FreePool (Wrap);
   return Status;
 
--
2.6.2.windows.1

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

Re: [edk2] [patch] MdeModulePkg:Fix a bug that HttpLib can not parse Ipv6 address correctly.

[Gary Ching-Pang Lin]

On Fri, Nov 06, 2015 at 04:11:45PM +0800, Zhang Lubo wrote:
> v2:
> *Add a Url Parse state machine to indicate the host is ipv6 expressed url and 
> can parse
> the ipv6 address correctly.
> 
This patch works for me. The IPv6 address was extracted from my url.

Tested-by: Gary Ching-Pang Lin 

> Cc: Fu Siyuan 
> Cc: Ye Ting 
> CC: Wu Jiaxin 
> CC: Gary Ching-Pang Lin 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Zhang Lubo 
> ---
>  MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 33 
> 
>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c 
> b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> index aeb52d0..500b3c7 100644
> --- a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> +++ b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> @@ -68,10 +68,11 @@ typedef enum {
>UrlParserFragmentStart, // "#"
>UrlParserFragment,
>UrlParserUserInfo,
>UrlParserHostStart, // "@"
>UrlParserHost,
> +  UrlParserHostIpv6,  // "["(Ipv6 address) "]"
>UrlParserPortStart, // ":"
>UrlParserPort,
>UrlParserStateMax
>  } HTTP_URL_PARSE_STATE;
>  
> @@ -136,17 +137,20 @@ UriPercentDecode (
>This function return the updated state accroding to the input state and 
> next character of
>the authority.
>  
>@param[in]   Char   Next character.
>@param[in]   State  Current value of the parser state machine.
> +  @param[in]   IsRightBracket TRUE if there is an sign ']' in the 
> authority component and 
> +  indicates the next part is ':' before 
> Port.
>  
>@return  Updated state value.
>  **/
>  HTTP_URL_PARSE_STATE
>  NetHttpParseAuthorityChar (
>IN  CHAR8  Char,
> -  IN  HTTP_URL_PARSE_STATE   State
> +  IN  HTTP_URL_PARSE_STATE   State,
> +  IN  BOOLEAN*IsRightBracket
>)
>  {
>  
>//
>// RFC 3986:
> @@ -167,16 +171,31 @@ NetHttpParseAuthorityChar (
>return UrlParserHostStart;
>  }
>  break;
>  
>case UrlParserHost:
> -  case UrlParserHostStart:
> +  case UrlParserHostStart:  
> +if (Char == '[') {
> +  return UrlParserHostIpv6;
> +}
> +
>  if (Char == ':') {
>return UrlParserPortStart;
>  }
> +
>  return UrlParserHost;
> -
> +
> +  case UrlParserHostIpv6:  
> +if (Char == ']') {
> +  *IsRightBracket = TRUE;
> +}
> +
> +if (Char == ':' && *IsRightBracket == TRUE) {
> +  return UrlParserPortStart;
> +}
> +return UrlParserHostIpv6;
> +
>case UrlParserPort:
>case UrlParserPortStart:
>  return UrlParserPort;
>  
>default:
> @@ -208,10 +227,11 @@ NetHttpParseAuthority (
>CHAR8 *Authority;
>UINT32Length;
>HTTP_URL_PARSE_STATE  State;
>UINT32Field;
>UINT32OldField;
> +  BOOLEAN   IsrightBracket;
>
>ASSERT ((UrlParser->FieldBitMap & BIT (HTTP_URI_FIELD_AUTHORITY)) != 0);
>  
>//
>// authority   = [ userinfo "@" ] host [ ":" port ]
> @@ -220,16 +240,17 @@ NetHttpParseAuthority (
>  State = UrlParserUserInfo;
>} else {
>  State = UrlParserHost;
>}
>  
> +  IsrightBracket = FALSE;
>Field = HTTP_URI_FIELD_MAX;
>OldField = Field;
>Authority = Url + UrlParser->FieldData[HTTP_URI_FIELD_AUTHORITY].Offset;
>Length = UrlParser->FieldData[HTTP_URI_FIELD_AUTHORITY].Length;
>for (Char = Authority; Char < Authority + Length; Char++) {
> -State = NetHttpParseAuthorityChar (*Char, State);
> +State = NetHttpParseAuthorityChar (*Char, State, );
>  switch (State) {
>  case UrlParserStateMax:
>return EFI_INVALID_PARAMETER;
>  
>  case UrlParserHostStart:
> @@ -241,10 +262,14 @@ NetHttpParseAuthority (
>break;
>
>  case UrlParserHost:
>Field = HTTP_URI_FIELD_HOST;
>break;
> +
> +case UrlParserHostIpv6:
> +  Field = HTTP_URI_FIELD_HOST;
> +  break;
>
>  case UrlParserPort:
>Field = HTTP_URI_FIELD_PORT;
>break;
>  
> -- 
> 1.9.5.msysgit.1
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [MdeModulePkg] PeiMain PeiCore does not compile with Xcode 6.3.2

[Zeng, Star]

On 2015/10/26 9:54, Zeng, Star wrote:

Reviewed-by: Star Zeng 

to the attached patch.

I could not see the email I replied to edk2-devel-01 , 
so re-reply to edk2-devel@lists.01.org.


Andrew,

Will you commit this patch by yourself, or need me help to do that?


Thanks
Star



Thanks,
Star
From: af...@apple.com [mailto:af...@apple.com]
Sent: Monday, October 26, 2015 9:31 AM
To: Zeng, Star
Cc: edk2-devel-01
Subject: Re: [edk2] [MdeModulePkg] PeiMain PeiCore does not compile with Xcode 
6.3.2



On Oct 25, 2015, at 6:04 PM, Zeng, Star  wrote:

On 2015/10/23 22:11, Andrew Fish wrote:



On Oct 23, 2015, at 12:53 AM, Zeng, Star  wrote:

On 2015/10/22 23:54, Andrew Fish wrote:

/Users/andrewfish/work/src/edk2/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c:682:28:
 error: for loop has empty body [-Werror,-Wempty-body]
 StackPointer ++);
 ^
/Users/andrewfish/work/src/edk2/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c:682:28:
 note: put the semicolon on a separate line to silence this warning
1 error generated.


Do you have full code base and all package build about this warning?
I can some for loop like below in UefiLib.c AddUnicodeString2(). And there 
maybe many other places have similar cases I guess since I never met this build 
failure before.

for (; *SupportedLanguages != 0 && *SupportedLanguages == ';'; 
SupportedLanguages++);



That is legal


What's the real value of this build warning?


-Wempty-body is designed to catch code bugs like:

if (X);
X++;

In the above code X++ always runs.



And could you disable this warning the tool chain?



Yes any clang diagnostic that prints out the -W* you can do -Wno-*


I realize my fix is bad. The error is really the next DEBUG() macro is indented 
like it is in a loop body. That is what is triggering the diagnostic.

for (; *SupportedLanguages != 0 && *SupportedLanguages == ';'; 
SupportedLanguages++);
  WarningForLoopIndentation();


  DEBUG_CODE_BEGIN ();
UINT32  *StackPointer;

for (StackPointer = (UINT32*)SecCoreData->StackBase;
 (StackPointer < (UINT32*)((UINTN)SecCoreData->StackBase + 
SecCoreData->StackSize)) \
 && (*StackPointer == INIT_CAR_VALUE);
 StackPointer ++);

  DEBUG ((EFI_D_INFO, "Temp Stack : BaseAddress=0x%p Length=0x%X\n", 
SecCoreData->StackBase, (UINT32)SecCoreData->StackSize));
  DEBUG ((EFI_D_INFO, "Temp Heap  : BaseAddress=0x%p Length=0x%X\n", 
Private->HobList.Raw, (UINT32)((UINTN) 
Private->HobList.HandoffInformationTable->EfiFreeMemoryBottom - (UINTN) 
Private->HobList.Raw)));
  DEBUG ((EFI_D_INFO, "Total temporary memory:%d bytes.\n", 
(UINT32)SecCoreData->TemporaryRamSize));
  DEBUG ((EFI_D_INFO, "  temporary memory stack ever used: %d bytes.\n",
 (UINT32)(SecCoreData->StackSize - ((UINTN) StackPointer - 
(UINTN)SecCoreData->StackBase))
));
  DEBUG ((EFI_D_INFO, "  temporary memory heap used:   %d bytes.\n",
 
(UINT32)((UINTN)Private->HobList.HandoffInformationTable->EfiFreeMemoryBottom - 
(UINTN)Private->HobList.Raw)
));
  DEBUG_CODE_END ();

Fixing the indentation to follow the coding standard makes the warning go away. 
That is the correct fix.


Yes, I like this method to fix the indentation.
Could you help provide the formal patch?




Thanks,

Andrew Fish


Thanks,
Star



Thanks,

Andrew Fish


Thanks,
Star



This code change removes the warning.
diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c 
b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
index 7480b66..c563775 100644
--- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
+++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
@@ -679,7 +679,8 @@ PeiCheckAndSwitchStack (
 for (StackPointer = (UINT32*)SecCoreData->StackBase;
  (StackPointer < (UINT32*)((UINTN)SecCoreData->StackBase + 
SecCoreData->StackSize)) \
  && (*StackPointer == INIT_CAR_VALUE);
-   StackPointer ++);
+   StackPointer ++)
+ ;

   DEBUG ((EFI_D_INFO, "Temp Stack : BaseAddress=0x%p Length=0x%X\n", 
SecCoreData->StackBase, (UINT32)SecCoreData->StackSize));
   DEBUG ((EFI_D_INFO, "Temp Heap  : BaseAddress=0x%p Length=0x%X\n", 
Private->HobList.Raw, (UINT32)((UINTN) 
Private->HobList.HandoffInformationTable->EfiFreeMemoryBottom - (UINTN) 
Private->HobList.Raw)));


Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Andrew Fish >

Thanks,

Andrew Fish

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


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

Re: [edk2] [PATCH 2/2] ShellPkg/UefiDpLib: Support dumping cumulative data

[Zeng, Star]

On 2015/11/3 21:09, Shia, Cinnamon wrote:

Stat,

Thanks for your feedback and review.
Will submit the v2 patch for addressing your comments.
And submit another patch for PerformancePkg\Dp_App


Shia,

Since the changes in UefiDpLib has been committed.
Will you submit patch for PerformancePkg\Dp_App?


Thanks,
Star



Thanks,
Cinnamon Shia

-Original Message-
From: Zeng, Star [mailto:star.z...@intel.com]
Sent: Tuesday, November 3, 2015 11:16 AM
To: Shia, Cinnamon; edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH 2/2] ShellPkg/UefiDpLib: Support dumping cumulative 
data

On 2015/11/2 17:11, Cinnamon Shia wrote:

Add a new option -c to dump cumulative data.
For example:
shell> dp -c
==[ Cumulative ]
(Times in microsec.) Cumulative   Average ShortestLongest
 Name  CountDurationDurationDurationDuration
LoadImage: 200 1007000   0  10
StartImage:2002000   9   0 700
DB:Start:2002000  10   0 900
DB:Support: 20  10   0   07000

shell> dp -c DXE
==[ Cumulative ]
(Times in microsec.) Cumulative   Average ShortestLongest
 Name  CountDurationDurationDurationDuration
LoadImage: 200 1007000   0  10
StartImage:2002000   9   0 700
DB:Start:2002000  10   0 900
DB:Support: 20  10   0   07000
  DXE  130003000   03000

Signed-off-by: Cinnamon Shia 
---
   ShellPkg/Library/UefiDpLib/Dp.c  |  33 +--
   ShellPkg/Library/UefiDpLib/DpInternal.h  |   9 +++--
   ShellPkg/Library/UefiDpLib/DpTrace.c |  55 
---
   ShellPkg/Library/UefiDpLib/UefiDpLib.uni | Bin 17466 -> 18146 bytes
   4 files changed, 87 insertions(+), 10 deletions(-)


Could the "HP_ISS" be removed from the patch?
Could you help sync the change to PerformancePkg\Dp_App?

Thanks,
Star



diff --git a/ShellPkg/Library/UefiDpLib/Dp.c
b/ShellPkg/Library/UefiDpLib/Dp.c index 62a4e7b..4d109d0 100644
--- a/ShellPkg/Library/UefiDpLib/Dp.c
+++ b/ShellPkg/Library/UefiDpLib/Dp.c
@@ -79,6 +79,7 @@ STATIC CONST SHELL_PARAM_ITEM ParamList[] = {
   #endif // PROFILING_IMPLEMENTED
 {L"-x", TypeFlag},   // -x   eXclude Cumulative Items
 {L"-i", TypeFlag},   // -i   Display Identifier
+  {L"-c", TypeValue},  // -c   Display cumulative data.
 {L"-n", TypeValue},  // -n # Number of records to display for A and R
 {L"-t", TypeValue},  // -t # Threshold of interest
 {NULL, TypeMax}
@@ -164,6 +165,9 @@ ShellCommandRunDp (
 BOOLEAN   TraceMode;
 BOOLEAN   ProfileMode;
 BOOLEAN   ExcludeMode;
+  BOOLEAN   CumulativeMode;
+  CONST CHAR16  *CustomCumulativeToken;
+  PERF_CUM_DATA *CustomCumulativeData;

 StringPtr   = NULL;
 SummaryMode = FALSE;
@@ -173,6 +177,8 @@ ShellCommandRunDp (
 TraceMode   = FALSE;
 ProfileMode = FALSE;
 ExcludeMode = FALSE;
+  CumulativeMode = FALSE;
+  CustomCumulativeData = NULL;

 // Get DP's entry time as soon as possible.
 // This is used as the Shell-Phase end time.
@@ -210,6 +216,7 @@ ShellCommandRunDp (
   #endif  // PROFILING_IMPLEMENTED
 ExcludeMode = ShellCommandLineGetFlag (ParamPackage, L"-x");
 mShowId = ShellCommandLineGetFlag (ParamPackage, L"-i");
+  CumulativeMode = ShellCommandLineGetFlag (ParamPackage, L"-c");

 // Options with Values
 CmdLineArg  = ShellCommandLineGetValue (ParamPackage, L"-n"); @@
-244,6 +251,20 @@ ShellCommandRunDp (
 InitCumulativeData ();

 //
+  // Init the custom cumulative data.
+  //
+  CustomCumulativeToken = ShellCommandLineGetValue (ParamPackage,
+ L"-c");  if (CustomCumulativeToken != NULL) {
+CustomCumulativeData = AllocateZeroPool (sizeof (PERF_CUM_DATA));
+CustomCumulativeData->MinDur = 0;
+CustomCumulativeData->MaxDur = 0;
+CustomCumulativeData->Count  = 0;
+CustomCumulativeData->Duration = 0;
+CustomCumulativeData->Name   = AllocateZeroPool (StrLen 
(CustomCumulativeToken) + 1);
+UnicodeStrToAsciiStr (CustomCumulativeToken,
+ CustomCumulativeData->Name);  }
+
+  //
 // Timer specific processing
 //
 // Get the Performance counter characteristics:
@@ -302,8 +323,10 @@ ShellCommandRunDp (
   !T &&  P  := (2) Only Profile records are displayed
    T &&  P  := (3) Same as Default, both are displayed

**
**/
-  GatherStatistics();
-  if (AllMode) {
+  GatherStatistics (CustomCumulativeData);  if (CumulativeMode) {
+ProcessCumulative (CustomCumulativeData);  } else if 

Re: [edk2] [MdeModulePkg] MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf will not compile with Xcode 6.3.2

[Zeng, Star]

On 2015/10/23 14:04, Andrew Fish wrote:



On Oct 22, 2015, at 10:10 PM, Zeng, Star  wrote:

On 2015/10/22 23:29, Andrew Fish wrote:



On Oct 21, 2015, at 6:34 PM, Zeng, Star  wrote:

On 2015/10/22 7:02, Andrew Fish wrote:

"/usr/bin/clang" -target x86_64-pc-win32-macho -c -g -Os -Wall -Werror -Wextra 
-include AutoGen.h -funsigned-char -fno-ms-extensions -fno-stack-protector -fno-builtin 
-fshort-wchar -mno-implicit-float -mms-bitfields -Wno-unused-parameter 
-Wno-missing-braces -Wno-missing-field-initializers -Wno-tautological-compare 
-Wno-sign-compare -ftrap-function=undefined_behavior_has_been_optimized_away_by_clang  -o 
/Users/andrewfish/work/src/edk2/Build/MdeModule/DEBUG_XCODE5/X64/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe/OUTPUT/./Variable.obj
 -I/Users/andrewfish/work/src/edk2/MdeModulePkg/Universal/Variable/RuntimeDxe 
-I/Users/andrewfish/work/src/edk2/Build/MdeModule/DEBUG_XCODE5/X64/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe/DEBUG
 -I/Users/andrewfish/work/src

  /e

  dk2

  /MdePkg -I/Users/andrewfish/work/src/edk2/MdePkg/Include 
-I/Users/andrewfish/work/src/edk2/MdePkg/Include/X64 
-I/Users/andrewfish/work/src/edk2/MdeModulePkg 
-I/Users/andrewfish/work/src/edk2/MdeModu
  lePkg/Include 
/Users/andrewfish/work/src/edk2/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
/Users/andrewfish/work/src/edk2/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c:1713:8:
 error: array type 'VA_LIST' (aka '__builtin_va_list') is not assignable
   Args = Marker;
    ^

it is NOT portable C code to use = with markers, you need to use VA_COPY() 
defined in Base.h.

This fixes the compiler warning with Xcode.


I met below commit failure when I try to help commit this patch.

ERROR from SVN:
A repository hook failed: MERGE request failed on '/p/edk2/code/trunk/edk2': 
Commit blocked by pre-commit hook (exit code 1) with output:
The commit message format is not valid:
* Line 3 of commit message is too long.
* Line 4 of commit message is too long.
https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format 


Could you update the commit log and commit the patch by yourself?



Sorry I just put the diff in an email to discuss the issue. I will reformat as 
a patch.


Is there any following step for this contribution?

Thanks,
Star



Thanks,

Andrew Fish


Thanks,
Star


~/work/src/edk2(master)>git diff 
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c 
b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index 31e1937..ef3dab3 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -1710,7 +1710,7 @@ CheckRemainingSpaceForConsistencyInternal (
ASSERT_EFI_ERROR (Status);

TotalNeededSize = 0;
-  Args = Marker;
+  VA_COPY (Args, Marker);
VariableEntry = VA_ARG (Args, VARIABLE_ENTRY_CONSISTENCY *);
while (VariableEntry != NULL) {
  //
@@ -1739,7 +1739,7 @@ CheckRemainingSpaceForConsistencyInternal (
  return FALSE;
}

-  Args = Marker;
+  VA_COPY (Args, Marker);
VariableEntry = VA_ARG (Args, VARIABLE_ENTRY_CONSISTENCY *);
while (VariableEntry != NULL) {
  //


Contributed-under: TianoCore Contribution Agreement 1.0
Reviewed-by: Andrew Fish 


Reviewed-by: Star Zeng 

A little confusion that why there is no Signed-off-by for this contribution?



Sorry did that backwards...

Signed-off-by: Andrew Fish 

Thanks,

Andrew Fish


Thanks,
Star



Thanks,

Andrew Fish



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


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


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

Re: [edk2] Strange behavior of the DS-5 debugger on AARCH64 with step-by-step debugging in uefi

[Cohen, Eugene]

Vladimir,

Since the UEFI images are PE-COFF but DS-5 builds ELF there is a conversion 
process in the edk2 build.  This can result in a couple of issues with debug if 
not handled correctly:

1. The start of the image for debugger load purposes must be adjusted to 
reflect the PE/TE header in the image.  If this isn't done correctly the code 
and data will be shifted and the debugger is useless.

2. The relative position of .text and .data must be consistent across ELF and 
PE-COFF.  If this isn't done correctly you may be able to get correlated source 
code but all references to data (like dumping global variables) are messed up.

The way I go about debugging this is to dump the PE-COFF image (.efi) with 
dumpbin (from Visual Studio) and dump the ELF image (.dll) with fromelf (from 
RVCT) and compare the positions of the .text and .data sections.

Based on Ard's response I think you are hitting issue #1.

Eugene

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Vladimir 
Olovyannikov
Sent: Thursday, November 05, 2015 3:52 PM
To: edk2-devel@lists.01.org
Subject: [edk2] Strange behavior of the DS-5 debugger on AARCH64 with 
step-by-step debugging in uefi

Hello All,

I faced a very strange behavior of the DS-5 debugger when I debug AARCH64  UEFI 
with the latest (well, the one which contains
DEFINE GCC_DLINK2_FLAGS_COMMON = 
--script=$(EDK_TOOLS_PATH)/Scripts/GccBase.lds
)
tools_def.template.
Whenever I step in the debugger it never matches the source where the execution 
point currently is.
It is impossible to step-by-step debug with this...
If I switch to the older tools_def.template, I don't have those issues, and can 
debug with no problem.
However, the ShellPkg cannot be built with older tools_def.template if I want 
to have ShellPkg (New Shell) instead of ShellBinPkg.

Please advise if I am doing anything wrong?

This is how I debug if step-by-step debug is needed:

I place a while(1) in a place of interest. Build UEFI, and then reboot the 
board and run the uefi; then when it reaches the point of while1, I connect 
DS-5 to the device and execute ArmPlatformPkg/Scripts/Ds5/cmd_load_symbols.py 
-f (0x8500,0x0028) -m (0x8000,0x4000) -a

And expect to be at the (while1 line). With the latest tools_def.tempalte the 
source point is weird; with the previous ones - OK.
Any advice would be appreciated.

Thank you,
Vladimir

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

Re: [edk2] [PATCH v2 09/10] ArmPkg/ArmLib: add accessor function for Cache Writeback Granularity

[Ard Biesheuvel]

On 6 November 2015 at 15:19, Mark Rutland  wrote:
> On Fri, Nov 06, 2015 at 03:03:57PM +0100, Ard Biesheuvel wrote:
>> Add a function to ArmLib that provides access to the Cache Writeback
>> Granularity (CWG) field in CTR_EL0. This information is required when
>> performing non-coherent DMA.
>
> Nit: s/Granularity/Granule/
>
> Likewise for the patch body.
>

OK

> [...]
>
>> +UINTN
>> +EFIAPI
>> +ArmCacheWritebackGranularity (
>> +  VOID
>> +  )
>> +{
>> +  UINTN   CWG;
>> +
>> +  CWG = (ArmCacheInfo () >> 24) & 0xf; // CTR_EL0.CWG
>> +
>> +  if (CWG == 0) {
>> +return SIZE_512KB;
>> +  }
>
> This should be SIZE_2KB, no?
>
> The ARM ARM says the architectural maximum is 512 /words/ (i.e. 2KB).
>

Yes, thanks for spotting that.

> With those changes:
>
> Reviewed-by: Mark Rutland 
>

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

Re: [edk2] [PATCH] ArmPkg/BdsLib: Increase fallback tftp buffer size

[Kinney, Michael D]

Hi,

There are macros in MdePkg/Include/Base.h that help align values and pointers 
to the next boundary.  
This patch could use the following:

TftpBufferSize = ALIGN_VALUE (TftpBufferSize, SIZE_16MB);

/**
  Rounds a value up to the next boundary using a specified alignment.

  This function rounds Value up to the next boundary using the specified 
Alignment.
  This aligned value is returned.

  @param   Value  The value to round up.
  @param   Alignment  The alignment boundary used to return the aligned value.

  @return  A value up to the next boundary.

**/
#define ALIGN_VALUE(Value, Alignment) ((Value) + (((Alignment) - (Value)) & 
((Alignment) - 1)))

Mike

>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>Ashutosh Singh
>Sent: Friday, November 06, 2015 9:01 AM
>To: edk2-de...@ml01.01.org
>Cc: ryan.har...@linaro.org; ler...@redhat.com; james.k...@arm.com;
>leif.lindh...@linaro.org; ashutosh.si...@arm.com
>Subject: [edk2] [PATCH] ArmPkg/BdsLib: Increase fallback tftp buffer size
>
>When performing a tftp download from a server which does not support
>rfc2349 transfer size option (such as netkit-tftpd), the existing code
>falls back to allocating an 8MB buffer. Since this is insufficient for
>an uncompressed AArch64 Linux kernel image, double the size to 16MB.
>
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: Ashutosh Singh 
>---
> ArmPkg/Library/BdsLib/BdsFilePath.c |4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/ArmPkg/Library/BdsLib/BdsFilePath.c
>b/ArmPkg/Library/BdsLib/BdsFilePath.c
>index ff42175..0410236 100644
>--- a/ArmPkg/Library/BdsLib/BdsFilePath.c
>+++ b/ArmPkg/Library/BdsLib/BdsFilePath.c
>@@ -1198,7 +1198,7 @@ BdsTftpLoadImage (
>   if (Mtftp4GetFileSize (Mtftp4, AsciiFilePath, ) == EFI_SUCCESS) {
> TftpBufferSize = FileSize;
>   } else {
>-TftpBufferSize = SIZE_8MB;
>+TftpBufferSize = SIZE_16MB;
>   }
>
>   TftpContext = AllocatePool (sizeof (BDS_TFTP_CONTEXT));
>@@ -1209,7 +1209,7 @@ BdsTftpLoadImage (
>   TftpContext->FileSize = FileSize;
>
>   for (; TftpBufferSize <= FixedPcdGet32 (PcdMaxTftpFileSize);
>- TftpBufferSize = (TftpBufferSize + SIZE_8MB) & (~(SIZE_8MB-1))) {
>+ TftpBufferSize = (TftpBufferSize + SIZE_16MB) & (~(SIZE_16MB-1))) {
> //
> // Allocate a buffer to hold the whole file.
> //
>--
>1.7.9.5
>
>
>
>
>-- 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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [MdeModulePkg] MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf will not compile with Xcode 6.3.2

[Andrew Fish]

> On Nov 6, 2015, at 1:51 AM, Zeng, Star  wrote:
> 
> On 2015/10/23 14:04, Andrew Fish wrote:
>> 
>>> On Oct 22, 2015, at 10:10 PM, Zeng, Star  wrote:
>>> 
>>> On 2015/10/22 23:29, Andrew Fish wrote:
 
> On Oct 21, 2015, at 6:34 PM, Zeng, Star  wrote:
> 
> On 2015/10/22 7:02, Andrew Fish wrote:
>> "/usr/bin/clang" -target x86_64-pc-win32-macho -c -g -Os -Wall -Werror 
>> -Wextra -include AutoGen.h -funsigned-char -fno-ms-extensions 
>> -fno-stack-protector -fno-builtin -fshort-wchar -mno-implicit-float 
>> -mms-bitfields -Wno-unused-parameter -Wno-missing-braces 
>> -Wno-missing-field-initializers -Wno-tautological-compare 
>> -Wno-sign-compare 
>> -ftrap-function=undefined_behavior_has_been_optimized_away_by_clang  -o 
>> /Users/andrewfish/work/src/edk2/Build/MdeModule/DEBUG_XCODE5/X64/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe/OUTPUT/./Variable.obj
>>  
>> -I/Users/andrewfish/work/src/edk2/MdeModulePkg/Universal/Variable/RuntimeDxe
>>  
>> -I/Users/andrewfish/work/src/edk2/Build/MdeModule/DEBUG_XCODE5/X64/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe/DEBUG
>>  -I/Users/andrewfish/work/src
>>  /e
  dk2
>>  /MdePkg -I/Users/andrewfish/work/src/edk2/MdePkg/Include 
>> -I/Users/andrewfish/work/src/edk2/MdePkg/Include/X64 
>> -I/Users/andrewfish/work/src/edk2/MdeModulePkg 
>> -I/Users/andrewfish/work/src/edk2/MdeModu
>>  lePkg/Include 
>> /Users/andrewfish/work/src/edk2/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
>> /Users/andrewfish/work/src/edk2/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c:1713:8:
>>  error: array type 'VA_LIST' (aka '__builtin_va_list') is not assignable
>>   Args = Marker;
>>    ^
>> 
>> it is NOT portable C code to use = with markers, you need to use 
>> VA_COPY() defined in Base.h.
>> 
>> This fixes the compiler warning with Xcode.
>>> 
>>> I met below commit failure when I try to help commit this patch.
>>> 
>>> ERROR from SVN:
>>> A repository hook failed: MERGE request failed on 
>>> '/p/edk2/code/trunk/edk2': Commit blocked by pre-commit hook (exit code 1) 
>>> with output:
>>> The commit message format is not valid:
>>> * Line 3 of commit message is too long.
>>> * Line 4 of commit message is too long.
>>> https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format 
>>> 
>>> 
>>> Could you update the commit log and commit the patch by yourself?
>>> 
>> 
>> Sorry I just put the diff in an email to discuss the issue. I will reformat 
>> as a patch.
> 
> Is there any following step for this contribution?
> 

Star,

I'll send a set of patches soon for the clang build issues. 

Thanks,

Andrew Fish

> Thanks,
> Star
> 
>> 
>> Thanks,
>> 
>> Andrew Fish
>> 
>>> Thanks,
>>> Star
>> 
>> ~/work/src/edk2(master)>git diff 
>> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
>> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c 
>> b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
>> index 31e1937..ef3dab3 100644
>> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
>> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
>> @@ -1710,7 +1710,7 @@ CheckRemainingSpaceForConsistencyInternal (
>>ASSERT_EFI_ERROR (Status);
>> 
>>TotalNeededSize = 0;
>> -  Args = Marker;
>> +  VA_COPY (Args, Marker);
>>VariableEntry = VA_ARG (Args, VARIABLE_ENTRY_CONSISTENCY *);
>>while (VariableEntry != NULL) {
>>  //
>> @@ -1739,7 +1739,7 @@ CheckRemainingSpaceForConsistencyInternal (
>>  return FALSE;
>>}
>> 
>> -  Args = Marker;
>> +  VA_COPY (Args, Marker);
>>VariableEntry = VA_ARG (Args, VARIABLE_ENTRY_CONSISTENCY *);
>>while (VariableEntry != NULL) {
>>  //
>> 
>> 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Reviewed-by: Andrew Fish 
> 
> Reviewed-by: Star Zeng 
> 
> A little confusion that why there is no Signed-off-by for this 
> contribution?
> 
 
 Sorry did that backwards...
 
 Signed-off-by: Andrew Fish 
 
 Thanks,
 
 Andrew Fish
 
> Thanks,
> Star
> 
>> 
>> Thanks,
>> 
>> Andrew Fish
>> 
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>>> 
>>> ___
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
> 
> ___
> edk2-devel mailing list
> 

Re: [edk2] [Patch 2/4] AppPkg/Python-2.7.10: New helper scripts

[Bjorge, Erik C]

Reviewed-by: Erik Bjorge 

From: Daryl McDaniel [mailto:edk2-li...@mc2research.org]
Sent: Thursday, November 5, 2015 2:32 PM
To: edk2-devel@lists.01.org; Carsey, Jaben ; Bjorge, 
Erik C 
Subject: [Patch 2/4] AppPkg/Python-2.7.10: New helper scripts

AppPkg/Python-2.7.10: Patch 2 of 4 -- New helper scripts

New files libprep.bat and srcprep.bat are included here in their entirety,
along with their diffs.  The whole files are located between the " BEGIN" 
and
" END" lines with the diffs following.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Daryl McDaniel 
>

 BEGIN CUT HERE -
@echo off
REM libprep.bat
REM
REM SYNTAX: libprep 
REM
SETLOCAL

set dest=%1

echo Copying files to %dest%.
echo Existing files will be overwritten.
echo.
PAUSE

REM Copy Distro then PyMod files to the destination
XCOPY Lib %dest% /S /I /Y
XCOPY PyMod-2.7.10\Lib %dest% /S /I /Y

echo DONE
ENDLOCAL
 END   CUT HERE -

 BEGIN CUT HERE -
@echo off
REM Prepare the Python sources for EDK II by copying
REM the .h files from the PyMod tree into the Python tree.
REM Directory correspondence is maintained.

FOR %%d IN (Include Modules Objects Python) DO (
  echo.
  echo Processing the %%d directory.
  XCOPY /S /Y /Q PyMod-2.7.10\%%d\*.h %%d
)

echo.
echo DONE!
 END   CUT HERE -


diff --git a/AppPkg/Applications/Python/Python-2.7.10/libprep.bat 
b/AppPkg/Applications/Python/Python-2.7.10/libprep.bat
new file mode 100644
index 000..b623b4b
--- /dev/null
+++ b/AppPkg/Applications/Python/Python-2.7.10/libprep.bat
@@ -0,0 +1,20 @@
+@echo off
+REM libprep.bat
+REM
+REM SYNTAX: libprep 
+REM
+SETLOCAL
+
+set dest=%1
+
+echo Copying files to %dest%.
+echo Existing files will be overwritten.
+echo.
+PAUSE
+
+REM Copy Distro then PyMod files to the destination
+XCOPY Lib %dest% /S /I /Y
+XCOPY PyMod-2.7.10\Lib %dest% /S /I /Y
+
+echo DONE
+ENDLOCAL
diff --git a/AppPkg/Applications/Python/Python-2.7.10/srcprep.bat 
b/AppPkg/Applications/Python/Python-2.7.10/srcprep.bat
new file mode 100644
index 000..d7a5823
--- /dev/null
+++ b/AppPkg/Applications/Python/Python-2.7.10/srcprep.bat
@@ -0,0 +1,13 @@
+@echo off
+REM Prepare the Python sources for EDK II by copying
+REM the .h files from the PyMod tree into the Python tree.
+REM Directory correspondence is maintained.
+
+FOR %%d IN (Include Modules Objects Python) DO (
+ echo.
+ echo Processing the %%d directory.
+ XCOPY /S /Y /Q PyMod-2.7.10\%%d\*.h %%d
+)
+
+echo.
+echo DONE!

END OF PATCH 2 of 4

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

Re: [edk2] I don't understand AllocatePages

[Andrew Fish]

> On Nov 6, 2015, at 10:28 AM, Shubha Ramani  wrote:
> 
> Here:http://wiki.phoenix.com/wiki/index.php/EFI_BOOT_SERVICES#AllocatePages.28.29
> 
> First of all, is there a concept of Virtual Memory in UEFI ? 

EFI Boot Services runs with physical addresses. When the OS takes over it can 
provide a virtual address space for the EFI Runtime Services. See UEFI 2.5 
Chapter 7 Runtime Services - 7.4 Virtual Memory Services. That is why a memory 
descriptor contains the VirtualStart.

> Why does EFI_MEMORY_DESCRIPTOR have VirtualStart ? What is it used for ?
> typedef struct {
>  UINT32   Type;
>  EFI_PHYSICAL_ADDRESS PhysicalStart;
>  EFI_VIRTUAL_ADDRESS  VirtualStart;
>  UINT64   NumberOfPages;
>  UINT64   Attribute;
> } EFI_MEMORY_DESCRIPTOR; Finally, if there is no virtual memory, then why is 
> AllocatePages even required ?  If all you care about is reading/writing to 
> pure physical memory addresses, then do you need to call AllocatePages ?

To allocate page aligned memory. The OS uses virtual memory even if EFI does 
not, thus the EFI Runtime Services code, and tables passed to the OS are page 
aligned to make it easier for the OS to map. 

Thanks,

Andrew Fish

> Thanks,
> Shubha
> Shubha D. ramanishubharam...@gmail.com
> shubharam...@yahoo.com
> ___
> 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] Increase tftp buffer size

[Laszlo Ersek]

On 11/06/15 15:19, Laszlo Ersek wrote:
> On 11/06/15 14:31, Ashutosh Singh wrote:
>>> So, I'm not a fan of hard-coded buffer sizes, or how this existing bit
>>> of code duplicates functionality already available elsewhere in the
>>> tree ... but I also don't care much about ArmBds.
>>>
>>> What I'm curious about is - why is your Mtftp4GetFileSize call failing
>>> such that you need to use a hardcoded buffer size?
>>> Does your TFTP server not support rfc2349?
>>> Would changing that (for example by switching to tftpd-hpa over the
>>> unmaintained netkit-tftpd) not make more sense?
>>
>> Took a while to test with tftpd-hpa, with this it does work fine, i.e.
>> Mtftp4GetFileSize does succeed and buffer size is allocated accordingly.
>> I would still request you to merge the change, as it will mean people
>> using the default tftpd that comes with ubuntu distribution
> 
> You mean the one:
> 
> http://packages.ubuntu.com/wily/tftpd
> http://packages.ubuntu.com/xenial/tftpd
> 
> that is explicitly advertised in Debian (Ubuntu's parent distro):
> 
> https://packages.debian.org/jessie/tftpd
> 
> and in Ubuntu as well:
> 
> http://archive.ubuntu.com/ubuntu/pool/universe/n/netkit-tftp/netkit-tftp_0.17-18ubuntu2.diff.gz
> 
> as unsuitable for PXE booting?
> 
> See also
> 
> http://thread.gmane.org/gmane.comp.bios.edk2.devel/2515/focus=2612
> http://thread.gmane.org/gmane.comp.bios.edk2.devel/2515/focus=2638
> 
>> will also
>> be able to use the tftp boot for ARM platforms.
> 
> Since this patch intends to modify the ARM BDS, I don't really have a
> horse in the race, but please be aware that this is just a band-aid.
> Debian and Ubuntu users should use the right tftp server (and/or file a
> bug with their distro to change the default TFTP server).
> 
> I recommend to state the above in the commit message.

(Sorry about the followup.)

Also, the subject should say:

ArmPkg: BdsLib: increase tftp buffer size

(When I first saw the email, I thought the patch was for MdeModulePkg or
NetworkPkg.)

Thanks
Laszlo

> 
>>
>> -Original Message-
>> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
>> Sent: 05 November 2015 12:26
>> To: Ashutosh Singh
>> Cc: edk2-de...@ml01.01.org; ard.biesheu...@linaro.org; 
>> ryan.har...@linaro.org; James King
>> Subject: Re: [PATCH] Increase tftp buffer size
>>
>> On Thu, Nov 05, 2015 at 11:37:43AM +, Ashutosh Singh wrote:
>>> Default tftp buffer size (8MB) is too small for standard
>>> ARM kernel images, which results in multiple aborted tftp fetches.
>>> This fix increase the buffer size to 16MB.
>>
>> So, I'm not a fan of hard-coded buffer sizes, or how this existing bit
>> of code duplicates functionality already available elsewhere in the
>> tree ... but I also don't care much about ArmBds.
>>
>> What I'm curious about is - why is your Mtftp4GetFileSize call failing
>> such that you need to use a hardcoded buffer size?
>> Does your TFTP server not support rfc2349?
>> Would changing that (for example by switching to tftpd-hpa over the
>> unmaintained netkit-tftpd) not make more sense?
>>
>> /
>> Leif
>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ashutosh Singh 
>>> ---
>>>  ArmPkg/Library/BdsLib/BdsFilePath.c |4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/ArmPkg/Library/BdsLib/BdsFilePath.c 
>>> b/ArmPkg/Library/BdsLib/BdsFilePath.c
>>> index ff42175..0410236 100644
>>> --- a/ArmPkg/Library/BdsLib/BdsFilePath.c
>>> +++ b/ArmPkg/Library/BdsLib/BdsFilePath.c
>>> @@ -1198,7 +1198,7 @@ BdsTftpLoadImage (
>>>if (Mtftp4GetFileSize (Mtftp4, AsciiFilePath, ) == EFI_SUCCESS) 
>>> {
>>>  TftpBufferSize = FileSize;
>>>} else {
>>> -TftpBufferSize = SIZE_8MB;
>>> +TftpBufferSize = SIZE_16MB;
>>>}
>>>
>>>TftpContext = AllocatePool (sizeof (BDS_TFTP_CONTEXT));
>>> @@ -1209,7 +1209,7 @@ BdsTftpLoadImage (
>>>TftpContext->FileSize = FileSize;
>>>
>>>for (; TftpBufferSize <= FixedPcdGet32 (PcdMaxTftpFileSize);
>>> - TftpBufferSize = (TftpBufferSize + SIZE_8MB) & (~(SIZE_8MB-1))) {
>>> + TftpBufferSize = (TftpBufferSize + SIZE_16MB) & (~(SIZE_16MB-1))) 
>>> {
>>>  //
>>>  // Allocate a buffer to hold the whole file.
>>>  //
>>> --
>>> 1.7.9.5
>>>
>>>
>>> 
>>>
>>> -- 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.
>>>
>>
>>
>> 
>>
>> -- 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 

Re: [edk2] [PATCH] Increase tftp buffer size

[Ryan Harkin]

I think the summary here is
- nobody cares about ArmBds (it will be deleted asap anyway)
- the patch is technically no worse than what's there already
- the original code should have dynamically allocated the buffer, but doesn't
- the code should have given a sensible error message mentioning that
the file size wasn't returned by the server, but doesn't
- Ashu is using the "wrong" tftp server, which is why he found this problem

So I'd say we just submit the patch.  Until we get around to deleting
ArmBds, it's one less chance for other innocent victims to stumble
across this problem and come to us for help.

On 6 November 2015 at 14:22, Laszlo Ersek  wrote:
> On 11/06/15 15:19, Laszlo Ersek wrote:
>> On 11/06/15 14:31, Ashutosh Singh wrote:
 So, I'm not a fan of hard-coded buffer sizes, or how this existing bit
 of code duplicates functionality already available elsewhere in the
 tree ... but I also don't care much about ArmBds.

 What I'm curious about is - why is your Mtftp4GetFileSize call failing
 such that you need to use a hardcoded buffer size?
 Does your TFTP server not support rfc2349?
 Would changing that (for example by switching to tftpd-hpa over the
 unmaintained netkit-tftpd) not make more sense?
>>>
>>> Took a while to test with tftpd-hpa, with this it does work fine, i.e.
>>> Mtftp4GetFileSize does succeed and buffer size is allocated accordingly.
>>> I would still request you to merge the change, as it will mean people
>>> using the default tftpd that comes with ubuntu distribution
>>
>> You mean the one:
>>
>> http://packages.ubuntu.com/wily/tftpd
>> http://packages.ubuntu.com/xenial/tftpd
>>
>> that is explicitly advertised in Debian (Ubuntu's parent distro):
>>
>> https://packages.debian.org/jessie/tftpd
>>
>> and in Ubuntu as well:
>>
>> http://archive.ubuntu.com/ubuntu/pool/universe/n/netkit-tftp/netkit-tftp_0.17-18ubuntu2.diff.gz
>>
>> as unsuitable for PXE booting?
>>
>> See also
>>
>> http://thread.gmane.org/gmane.comp.bios.edk2.devel/2515/focus=2612
>> http://thread.gmane.org/gmane.comp.bios.edk2.devel/2515/focus=2638
>>
>>> will also
>>> be able to use the tftp boot for ARM platforms.
>>
>> Since this patch intends to modify the ARM BDS, I don't really have a
>> horse in the race, but please be aware that this is just a band-aid.
>> Debian and Ubuntu users should use the right tftp server (and/or file a
>> bug with their distro to change the default TFTP server).
>>
>> I recommend to state the above in the commit message.
>
> (Sorry about the followup.)
>
> Also, the subject should say:
>
> ArmPkg: BdsLib: increase tftp buffer size
>
> (When I first saw the email, I thought the patch was for MdeModulePkg or
> NetworkPkg.)
>
> Thanks
> Laszlo
>
>>
>>>
>>> -Original Message-
>>> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
>>> Sent: 05 November 2015 12:26
>>> To: Ashutosh Singh
>>> Cc: edk2-de...@ml01.01.org; ard.biesheu...@linaro.org; 
>>> ryan.har...@linaro.org; James King
>>> Subject: Re: [PATCH] Increase tftp buffer size
>>>
>>> On Thu, Nov 05, 2015 at 11:37:43AM +, Ashutosh Singh wrote:
 Default tftp buffer size (8MB) is too small for standard
 ARM kernel images, which results in multiple aborted tftp fetches.
 This fix increase the buffer size to 16MB.
>>>
>>> So, I'm not a fan of hard-coded buffer sizes, or how this existing bit
>>> of code duplicates functionality already available elsewhere in the
>>> tree ... but I also don't care much about ArmBds.
>>>
>>> What I'm curious about is - why is your Mtftp4GetFileSize call failing
>>> such that you need to use a hardcoded buffer size?
>>> Does your TFTP server not support rfc2349?
>>> Would changing that (for example by switching to tftpd-hpa over the
>>> unmaintained netkit-tftpd) not make more sense?
>>>
>>> /
>>> Leif
>>>
 Contributed-under: TianoCore Contribution Agreement 1.0
 Signed-off-by: Ashutosh Singh 
 ---
  ArmPkg/Library/BdsLib/BdsFilePath.c |4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

 diff --git a/ArmPkg/Library/BdsLib/BdsFilePath.c 
 b/ArmPkg/Library/BdsLib/BdsFilePath.c
 index ff42175..0410236 100644
 --- a/ArmPkg/Library/BdsLib/BdsFilePath.c
 +++ b/ArmPkg/Library/BdsLib/BdsFilePath.c
 @@ -1198,7 +1198,7 @@ BdsTftpLoadImage (
if (Mtftp4GetFileSize (Mtftp4, AsciiFilePath, ) == 
 EFI_SUCCESS) {
  TftpBufferSize = FileSize;
} else {
 -TftpBufferSize = SIZE_8MB;
 +TftpBufferSize = SIZE_16MB;
}

TftpContext = AllocatePool (sizeof (BDS_TFTP_CONTEXT));
 @@ -1209,7 +1209,7 @@ BdsTftpLoadImage (
TftpContext->FileSize = FileSize;

for (; TftpBufferSize <= FixedPcdGet32 (PcdMaxTftpFileSize);
 - TftpBufferSize = (TftpBufferSize + SIZE_8MB) & (~(SIZE_8MB-1))) {
 + TftpBufferSize = (TftpBufferSize + SIZE_16MB) & 
 

Re: [edk2] [PATCH v2 10/10] ArmPkg/ArmDmaLib: use the cache writeback granularity for alignment

[Mark Rutland]

On Fri, Nov 06, 2015 at 03:03:58PM +0100, Ard Biesheuvel wrote:
> When allocating memory to perform non-coherent DMA, use the cache
> writeback granularity rather than the data cache linesize for
> alignment. This prevents the explicit cache management from corrupting
> unrelated adjacent data if the cache writeback granularity exceeds
> the cache linesize.
> 
> Reported-by: Mark Rutland 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c 
> b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> index 12b194046ae7..7e352e665a30 100755
> --- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> +++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> @@ -277,7 +277,7 @@ ArmDmaLibConstructor (
>Status = gBS->LocateProtocol (, NULL, (VOID 
> **));
>ASSERT_EFI_ERROR(Status);
>  
> -  gCacheAlignment = ArmDataCacheLineLength ();
> +  gCacheAlignment = ArmCacheWritebackGranularity ();

I assume that per my comment on the previous patch, the function name
might change here.

Regardless, this makes the logic look correct to me:

Reviewed-by: Mark Rutland 

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

Re: [edk2] [Patch 0/4] AppPkg/Python: Port Python 2.7.10 to EDK II

[Scott Duplichan]

Hello Liming,

I see what you mean. With Microsoft Outlook File->Save as->Save as Txt,
a tab is added after from:, sent:, to:, and subject:. Worse yet, the
lines are wrapped to 78 or so columns. The only solution I have found
is to avoid File->SaveAs altogether and instead use:

 Select All
 Copy

... then paste the patch into a text editor and save it that way.
It is truly frightening to see how many data corruption methods
Microsoft has invented.

Thanks,
Scott


-Original Message-
From: Gao, Liming [mailto:liming@intel.com] 
Sent: Friday, November 06, 2015 01:46 AM
To: Scott Duplichan ; 'Daryl McDaniel' 

Cc: edk2-de...@ml01.01.org
Subject: RE: [edk2] [Patch 0/4] AppPkg/Python: Port Python 2.7.10 to EDK II

Scott:
  When I get the mail in outlook, how I save this mail as Git patch? 

  I try saving the mail by File->Save as->Save as Txt. I find the saved txt 
file includes Tab in header and the wrapped line. It can't be applied as the 
git patch. 

Thanks
Liming
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Scott 
Duplichan
Sent: Friday, November 06, 2015 10:35 AM
To: 'Daryl McDaniel'
Cc: edk2-de...@ml01.01.org
Subject: Re: [edk2] [Patch 0/4] AppPkg/Python: Port Python 2.7.10 to EDK II

Jordan Justen [mailto:jordan.l.jus...@intel.com] wrote:

]Sent: Thursday, November 05, 2015 07:35 PM
]To: Daryl McDaniel ; edk2-de...@ml01.01.org; Jaben 
Carsey (Intel) ]; Erik Bjorge (Intel) 

]Subject: Re: [edk2] [Patch 0/4] AppPkg/Python: Port Python 2.7.10 to EDK II
]
]Have you considered using git?
]
]For one, EDK II is moving to git.
]
]It would have enabled you to easily generate and send the patch files
]for your patch series. (I am guessing you spend considerable time
]generating these emails, and yet, I don't think they are usable to
]actually apply to a tree.)
]
]I think Outlook may have munged some of the lines in your file. Some
]of the lines seem to have been wrapped.

Correct. Daryl is using Outlook 2013, same as me. Here is a partial
solution: In File, Options, Mail, Message Format...
Automatically wrap text at character [Set to maximum (132)]
Remove extra line breaks in plain text messages [Uncheck]
Then be sure to select FORMAT TEXT, Plain Text for messages to the
list. When typing text, put in line breaks manually.

Amazing Microsoft doesn't allow disabling line wrapping altogether.
If I remember correctly, Outlook 2007 is better. Next time I rebuild
my drive, I am switching back to Outlook 2007.

So anyway, this is not a rock solid solution. But EDK2 code doesn't
have lines long enough to make the patch fail from wrapping to 132
columns.

Here is a test line of length 131:
00 0 0 0 0 0 0 
0 0 1 1 1 1
01 2 3 4 5 6 7 
8 9 0 1 2 3
12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901

Thanks,
Scott


]You also could easily post a branch of your patches (for example, on
]github).
]
]Also, git send-email nicely uses the In-Reply-To email header to
]thread all the patches of your series together.
]
]-Jordan
]
<...>


___
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] Increase tftp buffer size

[Leif Lindholm]

On Fri, Nov 06, 2015 at 02:31:46PM +, Ryan Harkin wrote:
> I think the summary here is
> - nobody cares about ArmBds (it will be deleted asap anyway)

Yup.

> - the patch is technically no worse than what's there already

Yup.

> - the original code should have dynamically allocated the buffer,
>   but doesn't

Well, it does try. But instead of returning an error it tried to
guess.

> - the code should have given a sensible error message mentioning that
> the file size wasn't returned by the server, but doesn't

Yup.

> - Ashu is using the "wrong" tftp server, which is why he found this problem

Yup.

> So I'd say we just submit the patch.  Until we get around to deleting
> ArmBds, it's one less chance for other innocent victims to stumble
> across this problem and come to us for help.

Yup.
An arm64 "allyesconfig" linux kernel is currently around 9MB, so this
should be sufficient for the remaining lifetime of the ArmBds.

/
Leif

> On 6 November 2015 at 14:22, Laszlo Ersek  wrote:
> > On 11/06/15 15:19, Laszlo Ersek wrote:
> >> On 11/06/15 14:31, Ashutosh Singh wrote:
>  So, I'm not a fan of hard-coded buffer sizes, or how this existing bit
>  of code duplicates functionality already available elsewhere in the
>  tree ... but I also don't care much about ArmBds.
> 
>  What I'm curious about is - why is your Mtftp4GetFileSize call failing
>  such that you need to use a hardcoded buffer size?
>  Does your TFTP server not support rfc2349?
>  Would changing that (for example by switching to tftpd-hpa over the
>  unmaintained netkit-tftpd) not make more sense?
> >>>
> >>> Took a while to test with tftpd-hpa, with this it does work fine, i.e.
> >>> Mtftp4GetFileSize does succeed and buffer size is allocated accordingly.
> >>> I would still request you to merge the change, as it will mean people
> >>> using the default tftpd that comes with ubuntu distribution
> >>
> >> You mean the one:
> >>
> >> http://packages.ubuntu.com/wily/tftpd
> >> http://packages.ubuntu.com/xenial/tftpd
> >>
> >> that is explicitly advertised in Debian (Ubuntu's parent distro):
> >>
> >> https://packages.debian.org/jessie/tftpd
> >>
> >> and in Ubuntu as well:
> >>
> >> http://archive.ubuntu.com/ubuntu/pool/universe/n/netkit-tftp/netkit-tftp_0.17-18ubuntu2.diff.gz
> >>
> >> as unsuitable for PXE booting?
> >>
> >> See also
> >>
> >> http://thread.gmane.org/gmane.comp.bios.edk2.devel/2515/focus=2612
> >> http://thread.gmane.org/gmane.comp.bios.edk2.devel/2515/focus=2638
> >>
> >>> will also
> >>> be able to use the tftp boot for ARM platforms.
> >>
> >> Since this patch intends to modify the ARM BDS, I don't really have a
> >> horse in the race, but please be aware that this is just a band-aid.
> >> Debian and Ubuntu users should use the right tftp server (and/or file a
> >> bug with their distro to change the default TFTP server).
> >>
> >> I recommend to state the above in the commit message.
> >
> > (Sorry about the followup.)
> >
> > Also, the subject should say:
> >
> > ArmPkg: BdsLib: increase tftp buffer size
> >
> > (When I first saw the email, I thought the patch was for MdeModulePkg or
> > NetworkPkg.)
> >
> > Thanks
> > Laszlo
> >
> >>
> >>>
> >>> -Original Message-
> >>> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> >>> Sent: 05 November 2015 12:26
> >>> To: Ashutosh Singh
> >>> Cc: edk2-de...@ml01.01.org; ard.biesheu...@linaro.org; 
> >>> ryan.har...@linaro.org; James King
> >>> Subject: Re: [PATCH] Increase tftp buffer size
> >>>
> >>> On Thu, Nov 05, 2015 at 11:37:43AM +, Ashutosh Singh wrote:
>  Default tftp buffer size (8MB) is too small for standard
>  ARM kernel images, which results in multiple aborted tftp fetches.
>  This fix increase the buffer size to 16MB.
> >>>
> >>> So, I'm not a fan of hard-coded buffer sizes, or how this existing bit
> >>> of code duplicates functionality already available elsewhere in the
> >>> tree ... but I also don't care much about ArmBds.
> >>>
> >>> What I'm curious about is - why is your Mtftp4GetFileSize call failing
> >>> such that you need to use a hardcoded buffer size?
> >>> Does your TFTP server not support rfc2349?
> >>> Would changing that (for example by switching to tftpd-hpa over the
> >>> unmaintained netkit-tftpd) not make more sense?
> >>>
> >>> /
> >>> Leif
> >>>
>  Contributed-under: TianoCore Contribution Agreement 1.0
>  Signed-off-by: Ashutosh Singh 
>  ---
>   ArmPkg/Library/BdsLib/BdsFilePath.c |4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
>  diff --git a/ArmPkg/Library/BdsLib/BdsFilePath.c 
>  b/ArmPkg/Library/BdsLib/BdsFilePath.c
>  index ff42175..0410236 100644
>  --- a/ArmPkg/Library/BdsLib/BdsFilePath.c
>  +++ b/ArmPkg/Library/BdsLib/BdsFilePath.c
>  @@ -1198,7 +1198,7 @@ BdsTftpLoadImage (
> if (Mtftp4GetFileSize (Mtftp4, AsciiFilePath, ) == 
>  

Re: [edk2] [PATCH] Increase tftp buffer size

[Laszlo Ersek]

On 11/06/15 15:31, Ryan Harkin wrote:
> I think the summary here is
> - nobody cares about ArmBds (it will be deleted asap anyway)
> - the patch is technically no worse than what's there already
> - the original code should have dynamically allocated the buffer, but doesn't
> - the code should have given a sensible error message mentioning that
> the file size wasn't returned by the server, but doesn't
> - Ashu is using the "wrong" tftp server, which is why he found this problem
> 
> So I'd say we just submit the patch.  Until we get around to deleting
> ArmBds, it's one less chance for other innocent victims to stumble
> across this problem and come to us for help.

Okay with me, but please make the subject conform to the format edk2
uses, plus your nice summary should be included in the commit message in
some form.

I didn't intend to "block" the patch at all. :)

Thanks
Laszlo

> 
> On 6 November 2015 at 14:22, Laszlo Ersek  wrote:
>> On 11/06/15 15:19, Laszlo Ersek wrote:
>>> On 11/06/15 14:31, Ashutosh Singh wrote:
> So, I'm not a fan of hard-coded buffer sizes, or how this existing bit
> of code duplicates functionality already available elsewhere in the
> tree ... but I also don't care much about ArmBds.
>
> What I'm curious about is - why is your Mtftp4GetFileSize call failing
> such that you need to use a hardcoded buffer size?
> Does your TFTP server not support rfc2349?
> Would changing that (for example by switching to tftpd-hpa over the
> unmaintained netkit-tftpd) not make more sense?

 Took a while to test with tftpd-hpa, with this it does work fine, i.e.
 Mtftp4GetFileSize does succeed and buffer size is allocated accordingly.
 I would still request you to merge the change, as it will mean people
 using the default tftpd that comes with ubuntu distribution
>>>
>>> You mean the one:
>>>
>>> http://packages.ubuntu.com/wily/tftpd
>>> http://packages.ubuntu.com/xenial/tftpd
>>>
>>> that is explicitly advertised in Debian (Ubuntu's parent distro):
>>>
>>> https://packages.debian.org/jessie/tftpd
>>>
>>> and in Ubuntu as well:
>>>
>>> http://archive.ubuntu.com/ubuntu/pool/universe/n/netkit-tftp/netkit-tftp_0.17-18ubuntu2.diff.gz
>>>
>>> as unsuitable for PXE booting?
>>>
>>> See also
>>>
>>> http://thread.gmane.org/gmane.comp.bios.edk2.devel/2515/focus=2612
>>> http://thread.gmane.org/gmane.comp.bios.edk2.devel/2515/focus=2638
>>>
 will also
 be able to use the tftp boot for ARM platforms.
>>>
>>> Since this patch intends to modify the ARM BDS, I don't really have a
>>> horse in the race, but please be aware that this is just a band-aid.
>>> Debian and Ubuntu users should use the right tftp server (and/or file a
>>> bug with their distro to change the default TFTP server).
>>>
>>> I recommend to state the above in the commit message.
>>
>> (Sorry about the followup.)
>>
>> Also, the subject should say:
>>
>> ArmPkg: BdsLib: increase tftp buffer size
>>
>> (When I first saw the email, I thought the patch was for MdeModulePkg or
>> NetworkPkg.)
>>
>> Thanks
>> Laszlo
>>
>>>

 -Original Message-
 From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
 Sent: 05 November 2015 12:26
 To: Ashutosh Singh
 Cc: edk2-de...@ml01.01.org; ard.biesheu...@linaro.org; 
 ryan.har...@linaro.org; James King
 Subject: Re: [PATCH] Increase tftp buffer size

 On Thu, Nov 05, 2015 at 11:37:43AM +, Ashutosh Singh wrote:
> Default tftp buffer size (8MB) is too small for standard
> ARM kernel images, which results in multiple aborted tftp fetches.
> This fix increase the buffer size to 16MB.

 So, I'm not a fan of hard-coded buffer sizes, or how this existing bit
 of code duplicates functionality already available elsewhere in the
 tree ... but I also don't care much about ArmBds.

 What I'm curious about is - why is your Mtftp4GetFileSize call failing
 such that you need to use a hardcoded buffer size?
 Does your TFTP server not support rfc2349?
 Would changing that (for example by switching to tftpd-hpa over the
 unmaintained netkit-tftpd) not make more sense?

 /
 Leif

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ashutosh Singh 
> ---
>  ArmPkg/Library/BdsLib/BdsFilePath.c |4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/ArmPkg/Library/BdsLib/BdsFilePath.c 
> b/ArmPkg/Library/BdsLib/BdsFilePath.c
> index ff42175..0410236 100644
> --- a/ArmPkg/Library/BdsLib/BdsFilePath.c
> +++ b/ArmPkg/Library/BdsLib/BdsFilePath.c
> @@ -1198,7 +1198,7 @@ BdsTftpLoadImage (
>if (Mtftp4GetFileSize (Mtftp4, AsciiFilePath, ) == 
> EFI_SUCCESS) {
>  TftpBufferSize = FileSize;
>} else {
> -TftpBufferSize = SIZE_8MB;
> +TftpBufferSize = SIZE_16MB;
>}
>

Re: [edk2] [PATCH] ArmPlatformPkg/ArmJunoPkg: Create SMBIOS/DMI data for Juno

[Jeremy Linton]

On 11/06/2015 01:31 AM, Ard Biesheuvel wrote:

On 5 November 2015 at 22:51, Jeremy Linton  wrote:


  [Guids.common]
gArmJunoTokenSpaceGuid=  { 0xa1147a20, 0x3144, 0x4f8d, { 0x82, 0x95, 
0xb4, 0x83, 0x11, 0xc8, 0xe4, 0xa4 } }
+  gArmJunoDxe   =  { 0x1484ebe8, 0x2681, 0x45f1, { 0xa2, 0xe5, 
0x12, 0xec, 0xad, 0x89, 0x3b, 0x62 } } # match the ArmJunoDxe FILE GUID



I am not too crazy about using the BEFORE/AFTER depexes. Is this the
only way to achieve the ordering?



Ard,

I'm sort of a EDK2 newbie, so there may be a lot of alternatives I 
didn't consider.


That said, that dependency is there to assure that the PcdJunoRevision 
is set before the SMBIOS module loads. Initially I tried to make this 
module sort of "generic" but as I progressed it became more Juno 
specific, so I moved it into the Juno directory and set this dependency.


I am keenly aware of the fact that there isn't a single BEFORE/AFTER 
dependency in the whole project. Further I don't really like the fact 
that i couldn't directly reference the FILE GUID (rather having to 
duplicate it). I considered SOR (which results in a reverse  dependency 
if you will) and adding a bogus protocol/etc, duplicating the juno 
revision detection logic, none of which seemed like a particularly clean 
implementation.  To my limited knowledge this seemed like the least evil 
solution. It also bothered me that I couldn't set an AFTER with a 
protocol dependency


Suggestions welcome,











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

Re: [edk2] [PATCH] ArmPlatformPkg/ArmJunoPkg: Create SMBIOS/DMI data for Juno

[Ard Biesheuvel]

On 6 November 2015 at 16:11, Jeremy Linton  wrote:
> On 11/06/2015 01:31 AM, Ard Biesheuvel wrote:
>>
>> On 5 November 2015 at 22:51, Jeremy Linton  wrote:
>>>
>>>
>>>   [Guids.common]
>>> gArmJunoTokenSpaceGuid=  { 0xa1147a20, 0x3144, 0x4f8d, { 0x82,
>>> 0x95, 0xb4, 0x83, 0x11, 0xc8, 0xe4, 0xa4 } }
>>> +  gArmJunoDxe   =  { 0x1484ebe8, 0x2681, 0x45f1, { 0xa2,
>>> 0xe5, 0x12, 0xec, 0xad, 0x89, 0x3b, 0x62 } } # match the ArmJunoDxe FILE
>>> GUID
>>>
>>
>> I am not too crazy about using the BEFORE/AFTER depexes. Is this the
>> only way to achieve the ordering?
>>
>
> Ard,
>
> I'm sort of a EDK2 newbie, so there may be a lot of alternatives I didn't
> consider.
>
> That said, that dependency is there to assure that the PcdJunoRevision is
> set before the SMBIOS module loads. Initially I tried to make this module
> sort of "generic" but as I progressed it became more Juno specific, so I
> moved it into the Juno directory and set this dependency.
>
> I am keenly aware of the fact that there isn't a single BEFORE/AFTER
> dependency in the whole project. Further I don't really like the fact that i
> couldn't directly reference the FILE GUID (rather having to duplicate it). I
> considered SOR (which results in a reverse  dependency if you will) and
> adding a bogus protocol/etc, duplicating the juno revision detection logic,
> none of which seemed like a particularly clean implementation.  To my
> limited knowledge this seemed like the least evil solution. It also bothered
> me that I couldn't set an AFTER with a protocol dependency
>
> Suggestions welcome,
>

AFAICT, this is all you need to do to get the revision, right?

--8<
  //
  // We detect whether we are running on a Juno r0 or Juno r1 board at
  // runtime by checking the value of the MIDR register.
  //

  Midr = ArmReadMidr ();
  CpuType  = (Midr >> ARM_CPU_TYPE_SHIFT) & ARM_CPU_TYPE_MASK;
  CpuRev   = Midr & ARM_CPU_REV_MASK;

  switch (CpuType) {
  case ARM_CPU_TYPE_A53:
if (CpuRev == ARM_CPU_REV (0, 0)) {
  JunoRevision = JUNO_R0;
} else if (CpuRev == ARM_CPU_REV (0, 3)) {
  JunoRevision = JUNO_R1;
}
break;

  case ARM_CPU_TYPE_A57:
if (CpuRev == ARM_CPU_REV (0, 0)) {
  JunoRevision = JUNO_R0;
} else if (CpuRev == ARM_CPU_REV (1, 1)) {
  JunoRevision = JUNO_R1;
}
  }
--8<

I would much prefer duplicating this over adding dynamic PCDs and
BEFORE/AFTER depexes. If you really insist, you can break it out into
a separate static library, but I wouldn't even bother tbh.

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

Re: [edk2] [PATCH] ArmPlatformPkg/ArmJunoPkg: Create SMBIOS/DMI data for Juno

[Leif Lindholm]

On Fri, Nov 06, 2015 at 04:21:47PM +0100, Ard Biesheuvel wrote:
> On 6 November 2015 at 16:11, Jeremy Linton  wrote:
> > On 11/06/2015 01:31 AM, Ard Biesheuvel wrote:
> >>
> >> On 5 November 2015 at 22:51, Jeremy Linton  wrote:
> >>>
> >>>
> >>>   [Guids.common]
> >>> gArmJunoTokenSpaceGuid=  { 0xa1147a20, 0x3144, 0x4f8d, { 0x82,
> >>> 0x95, 0xb4, 0x83, 0x11, 0xc8, 0xe4, 0xa4 } }
> >>> +  gArmJunoDxe   =  { 0x1484ebe8, 0x2681, 0x45f1, { 0xa2,
> >>> 0xe5, 0x12, 0xec, 0xad, 0x89, 0x3b, 0x62 } } # match the ArmJunoDxe FILE
> >>> GUID
> >>>
> >>
> >> I am not too crazy about using the BEFORE/AFTER depexes. Is this the
> >> only way to achieve the ordering?
> >>
> >
> > Ard,
> >
> > I'm sort of a EDK2 newbie, so there may be a lot of alternatives I didn't
> > consider.
> >
> > That said, that dependency is there to assure that the PcdJunoRevision is
> > set before the SMBIOS module loads. Initially I tried to make this module
> > sort of "generic" but as I progressed it became more Juno specific, so I
> > moved it into the Juno directory and set this dependency.
> >
> > I am keenly aware of the fact that there isn't a single BEFORE/AFTER
> > dependency in the whole project. Further I don't really like the fact that i
> > couldn't directly reference the FILE GUID (rather having to duplicate it). I
> > considered SOR (which results in a reverse  dependency if you will) and
> > adding a bogus protocol/etc, duplicating the juno revision detection logic,
> > none of which seemed like a particularly clean implementation.  To my
> > limited knowledge this seemed like the least evil solution. It also bothered
> > me that I couldn't set an AFTER with a protocol dependency
> >
> > Suggestions welcome,
> >
> 
> AFAICT, this is all you need to do to get the revision, right?
> 
> --8<
>   //
>   // We detect whether we are running on a Juno r0 or Juno r1 board at
>   // runtime by checking the value of the MIDR register.
>   //
> 
>   Midr = ArmReadMidr ();
>   CpuType  = (Midr >> ARM_CPU_TYPE_SHIFT) & ARM_CPU_TYPE_MASK;
>   CpuRev   = Midr & ARM_CPU_REV_MASK;
> 
>   switch (CpuType) {
>   case ARM_CPU_TYPE_A53:
> if (CpuRev == ARM_CPU_REV (0, 0)) {
>   JunoRevision = JUNO_R0;
> } else if (CpuRev == ARM_CPU_REV (0, 3)) {
>   JunoRevision = JUNO_R1;
> }
> break;
> 
>   case ARM_CPU_TYPE_A57:
> if (CpuRev == ARM_CPU_REV (0, 0)) {
>   JunoRevision = JUNO_R0;
> } else if (CpuRev == ARM_CPU_REV (1, 1)) {
>   JunoRevision = JUNO_R1;
> }
>   }
> --8<
> 
> I would much prefer duplicating this over adding dynamic PCDs and
> BEFORE/AFTER depexes. If you really insist, you can break it out into
> a separate static library, but I wouldn't even bother tbh.

Static inline macro in ArmJunoPkg/Include/ArmPlatform.h?

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

Re: [edk2] [PATCH] ArmPlatformPkg/ArmJunoPkg: Create SMBIOS/DMI data for Juno

[Jeremy Linton]

On 11/06/2015 09:21 AM, Ard Biesheuvel wrote:

I would much prefer duplicating this over adding dynamic PCDs and
BEFORE/AFTER depexes. If you really insist, you can break it out into
a separate static library, but I wouldn't even bother tbh.


That solves the short term problem, but I expect that the SMBIOS and 
ACPI tables (and probably DT too) to have a lot more dynamic content in 
the future. Solving the problem of providing a common platform data 
provider may be useful.






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

Re: [edk2] [PATCH v2 05/10] ArmPkg/ArmLib: retrieve cache line length from CTR not CCSIDR

[Leif Lindholm]

On Fri, Nov 06, 2015 at 03:03:53PM +0100, Ard Biesheuvel wrote:
> The stride used by the cache maintenance by MVA instructions should
> be retrieved from CTR_EL0.DminLine and CTR_EL0.IminLine, whose values
> reflect the actual geometry of the caches. Using CCSIDR for this purpose
> violates the architecture.

Does it?
Sure, you'd need to iterate over all levels of I/D/U cache and pick
the smallest, but... If the current code doesn't, then it's broken (as
opposed to violating the architecture).
 
> Also, move the line length accessors to common code, since there is no
> need to keep them separate between ARMv7 and AArch64.

There is for Arm9 though - the CTR format changed for ARMv7.
So either we turn this back into an Arm9 purge as part of the cache
maintenance fixes (which I would like to avoid) or I think we need to
keep the per-arch line length accessors for now.

/
Leif

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 
> Reviewed-by: Mark Rutland 
> ---
>  ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c | 25 --
>  ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c | 27 
>  ArmPkg/Library/ArmLib/Common/ArmLib.c  | 18 +
>  3 files changed, 18 insertions(+), 52 deletions(-)
> 
> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c 
> b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
> index 4bea20356fa3..dec125f248cd 100644
> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
> @@ -21,31 +21,6 @@
>  #include "AArch64Lib.h"
>  #include "ArmLibPrivate.h"
>  
> -UINTN
> -EFIAPI
> -ArmDataCacheLineLength (
> -  VOID
> -  )
> -{
> -  UINT32 CCSIDR = ReadCCSIDR (0) & 7;
> -
> -  // * 4 converts to bytes
> -  return (1 << (CCSIDR + 2)) * 4;
> -}
> -
> -UINTN
> -EFIAPI
> -ArmInstructionCacheLineLength (
> -  VOID
> -  )
> -{
> -  UINT32 CCSIDR = ReadCCSIDR (1) & 7;
> -
> -  // * 4 converts to bytes
> -  return (1 << (CCSIDR + 2)) * 4;
> -}
> -
> -
>  VOID
>  AArch64DataCacheOperation (
>IN  AARCH64_CACHE_OPERATION  DataCacheOperation
> diff --git a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c 
> b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c
> index 44edff869eae..b53f455bfad2 100644
> --- a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c
> +++ b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c
> @@ -20,33 +20,6 @@
>  #include "ArmV7Lib.h"
>  #include "ArmLibPrivate.h"
>  
> -UINTN
> -EFIAPI
> -ArmDataCacheLineLength (
> -  VOID
> -  )
> -{
> -  UINT32 CCSIDR = ReadCCSIDR (0) & 7;
> -
> -  // * 4 converts to bytes
> -  return (1 << (CCSIDR + 2)) * 4;
> -}
> -
> -UINTN
> -EFIAPI
> -ArmInstructionCacheLineLength (
> -  VOID
> -  )
> -{
> -  UINT32 CCSIDR = ReadCCSIDR (1) & 7;
> -
> -  // * 4 converts to bytes
> -  return (1 << (CCSIDR + 2)) * 4;
> -
> -//  return 64;
> -}
> -
> -
>  VOID
>  ArmV7DataCacheOperation (
>IN  ARM_V7_CACHE_OPERATION  DataCacheOperation
> diff --git a/ArmPkg/Library/ArmLib/Common/ArmLib.c 
> b/ArmPkg/Library/ArmLib/Common/ArmLib.c
> index 4febc45220a3..ad0a265e9f59 100644
> --- a/ArmPkg/Library/ArmLib/Common/ArmLib.c
> +++ b/ArmPkg/Library/ArmLib/Common/ArmLib.c
> @@ -70,3 +70,21 @@ ArmUnsetCpuActlrBit (
>Value &= ~Bits;
>ArmWriteCpuActlr (Value);
>  }
> +
> +UINTN
> +EFIAPI
> +ArmDataCacheLineLength (
> +  VOID
> +  )
> +{
> +  return 4 << ((ArmCacheInfo () >> 16) & 0xf); // CTR_EL0.DminLine
> +}
> +
> +UINTN
> +EFIAPI
> +ArmInstructionCacheLineLength (
> +  VOID
> +  )
> +{
> +  return 4 << (ArmCacheInfo () & 0xf); // CTR_EL0.IminLine
> +}
> -- 
> 1.9.1
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] Increase tftp buffer size

[Ashutosh Singh]

Leif: Yes, the commit message seems fine, I will resubmit the patch
with new commit messages.
Laszlo: I understand that it is a band-aid, but I will leave that bigger
fight (of fixing ArmBds) for another day :) . It isn't very obvious to me that
pxe not being supported by default tftpd will cause problems with bare
tftp as well. Though reading rfc2349 does clarify this.

Thanks all for the review, I will resubmit the patch with proper commit message.

BR
Ashu


-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com]
Sent: 06 November 2015 14:48
To: ryan.har...@linaro.org
Cc: Ashutosh Singh; Leif Lindholm; edk2-de...@ml01.01.org; James King; 
ard.biesheu...@linaro.org
Subject: Re: [edk2] [PATCH] Increase tftp buffer size

On 11/06/15 15:31, Ryan Harkin wrote:
> I think the summary here is
> - nobody cares about ArmBds (it will be deleted asap anyway)
> - the patch is technically no worse than what's there already
> - the original code should have dynamically allocated the buffer, but doesn't
> - the code should have given a sensible error message mentioning that
> the file size wasn't returned by the server, but doesn't
> - Ashu is using the "wrong" tftp server, which is why he found this problem
>
> So I'd say we just submit the patch.  Until we get around to deleting
> ArmBds, it's one less chance for other innocent victims to stumble
> across this problem and come to us for help.

Okay with me, but please make the subject conform to the format edk2
uses, plus your nice summary should be included in the commit message in
some form.

I didn't intend to "block" the patch at all. :)

Thanks
Laszlo

>
> On 6 November 2015 at 14:22, Laszlo Ersek  wrote:
>> On 11/06/15 15:19, Laszlo Ersek wrote:
>>> On 11/06/15 14:31, Ashutosh Singh wrote:
> So, I'm not a fan of hard-coded buffer sizes, or how this existing bit
> of code duplicates functionality already available elsewhere in the
> tree ... but I also don't care much about ArmBds.
>
> What I'm curious about is - why is your Mtftp4GetFileSize call failing
> such that you need to use a hardcoded buffer size?
> Does your TFTP server not support rfc2349?
> Would changing that (for example by switching to tftpd-hpa over the
> unmaintained netkit-tftpd) not make more sense?

 Took a while to test with tftpd-hpa, with this it does work fine, i.e.
 Mtftp4GetFileSize does succeed and buffer size is allocated accordingly.
 I would still request you to merge the change, as it will mean people
 using the default tftpd that comes with ubuntu distribution
>>>
>>> You mean the one:
>>>
>>> http://packages.ubuntu.com/wily/tftpd
>>> http://packages.ubuntu.com/xenial/tftpd
>>>
>>> that is explicitly advertised in Debian (Ubuntu's parent distro):
>>>
>>> https://packages.debian.org/jessie/tftpd
>>>
>>> and in Ubuntu as well:
>>>
>>> http://archive.ubuntu.com/ubuntu/pool/universe/n/netkit-tftp/netkit-tftp_0.17-18ubuntu2.diff.gz
>>>
>>> as unsuitable for PXE booting?
>>>
>>> See also
>>>
>>> http://thread.gmane.org/gmane.comp.bios.edk2.devel/2515/focus=2612
>>> http://thread.gmane.org/gmane.comp.bios.edk2.devel/2515/focus=2638
>>>
 will also
 be able to use the tftp boot for ARM platforms.
>>>
>>> Since this patch intends to modify the ARM BDS, I don't really have a
>>> horse in the race, but please be aware that this is just a band-aid.
>>> Debian and Ubuntu users should use the right tftp server (and/or file a
>>> bug with their distro to change the default TFTP server).
>>>
>>> I recommend to state the above in the commit message.
>>
>> (Sorry about the followup.)
>>
>> Also, the subject should say:
>>
>> ArmPkg: BdsLib: increase tftp buffer size
>>
>> (When I first saw the email, I thought the patch was for MdeModulePkg or
>> NetworkPkg.)
>>
>> Thanks
>> Laszlo
>>
>>>

 -Original Message-
 From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
 Sent: 05 November 2015 12:26
 To: Ashutosh Singh
 Cc: edk2-de...@ml01.01.org; ard.biesheu...@linaro.org; 
 ryan.har...@linaro.org; James King
 Subject: Re: [PATCH] Increase tftp buffer size

 On Thu, Nov 05, 2015 at 11:37:43AM +, Ashutosh Singh wrote:
> Default tftp buffer size (8MB) is too small for standard
> ARM kernel images, which results in multiple aborted tftp fetches.
> This fix increase the buffer size to 16MB.

 So, I'm not a fan of hard-coded buffer sizes, or how this existing bit
 of code duplicates functionality already available elsewhere in the
 tree ... but I also don't care much about ArmBds.

 What I'm curious about is - why is your Mtftp4GetFileSize call failing
 such that you need to use a hardcoded buffer size?
 Does your TFTP server not support rfc2349?
 Would changing that (for example by switching to tftpd-hpa over the
 unmaintained netkit-tftpd) not make more sense?

 /
 Leif

> 

Re: [edk2] [PATCH v2 05/10] ArmPkg/ArmLib: retrieve cache line length from CTR not CCSIDR

[Ard Biesheuvel]

On 6 November 2015 at 17:02, Leif Lindholm  wrote:
> On Fri, Nov 06, 2015 at 03:03:53PM +0100, Ard Biesheuvel wrote:
>> The stride used by the cache maintenance by MVA instructions should
>> be retrieved from CTR_EL0.DminLine and CTR_EL0.IminLine, whose values
>> reflect the actual geometry of the caches. Using CCSIDR for this purpose
>> violates the architecture.
>
> Does it?
> Sure, you'd need to iterate over all levels of I/D/U cache and pick
> the smallest, but... If the current code doesn't, then it's broken (as
> opposed to violating the architecture).
>

Yes, it does. The ARM ARM states quite clearly that the contents of
CCSIDR should not be used to make any inferences about the actual
cache geometry.

>> Also, move the line length accessors to common code, since there is no
>> need to keep them separate between ARMv7 and AArch64.
>
> There is for Arm9 though - the CTR format changed for ARMv7.
> So either we turn this back into an Arm9 purge as part of the cache
> maintenance fixes (which I would like to avoid) or I think we need to
> keep the per-arch line length accessors for now.
>

Actually, you can't build the ARM9 version anymore, since ArmLib now
depends on  whereas the ARM9 specific code includes
 as well, resulting in a conflict. I don't think
this has been buildable since bd6b97994ab6 ("ArmPkg/ArmLib: Clean
ArmV7Lib") dated somewhere back in 2011



>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel 
>> Reviewed-by: Mark Rutland 
>> ---
>>  ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c | 25 --
>>  ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c | 27 
>>  ArmPkg/Library/ArmLib/Common/ArmLib.c  | 18 +
>>  3 files changed, 18 insertions(+), 52 deletions(-)
>>
>> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c 
>> b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
>> index 4bea20356fa3..dec125f248cd 100644
>> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
>> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
>> @@ -21,31 +21,6 @@
>>  #include "AArch64Lib.h"
>>  #include "ArmLibPrivate.h"
>>
>> -UINTN
>> -EFIAPI
>> -ArmDataCacheLineLength (
>> -  VOID
>> -  )
>> -{
>> -  UINT32 CCSIDR = ReadCCSIDR (0) & 7;
>> -
>> -  // * 4 converts to bytes
>> -  return (1 << (CCSIDR + 2)) * 4;
>> -}
>> -
>> -UINTN
>> -EFIAPI
>> -ArmInstructionCacheLineLength (
>> -  VOID
>> -  )
>> -{
>> -  UINT32 CCSIDR = ReadCCSIDR (1) & 7;
>> -
>> -  // * 4 converts to bytes
>> -  return (1 << (CCSIDR + 2)) * 4;
>> -}
>> -
>> -
>>  VOID
>>  AArch64DataCacheOperation (
>>IN  AARCH64_CACHE_OPERATION  DataCacheOperation
>> diff --git a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c 
>> b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c
>> index 44edff869eae..b53f455bfad2 100644
>> --- a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c
>> +++ b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c
>> @@ -20,33 +20,6 @@
>>  #include "ArmV7Lib.h"
>>  #include "ArmLibPrivate.h"
>>
>> -UINTN
>> -EFIAPI
>> -ArmDataCacheLineLength (
>> -  VOID
>> -  )
>> -{
>> -  UINT32 CCSIDR = ReadCCSIDR (0) & 7;
>> -
>> -  // * 4 converts to bytes
>> -  return (1 << (CCSIDR + 2)) * 4;
>> -}
>> -
>> -UINTN
>> -EFIAPI
>> -ArmInstructionCacheLineLength (
>> -  VOID
>> -  )
>> -{
>> -  UINT32 CCSIDR = ReadCCSIDR (1) & 7;
>> -
>> -  // * 4 converts to bytes
>> -  return (1 << (CCSIDR + 2)) * 4;
>> -
>> -//  return 64;
>> -}
>> -
>> -
>>  VOID
>>  ArmV7DataCacheOperation (
>>IN  ARM_V7_CACHE_OPERATION  DataCacheOperation
>> diff --git a/ArmPkg/Library/ArmLib/Common/ArmLib.c 
>> b/ArmPkg/Library/ArmLib/Common/ArmLib.c
>> index 4febc45220a3..ad0a265e9f59 100644
>> --- a/ArmPkg/Library/ArmLib/Common/ArmLib.c
>> +++ b/ArmPkg/Library/ArmLib/Common/ArmLib.c
>> @@ -70,3 +70,21 @@ ArmUnsetCpuActlrBit (
>>Value &= ~Bits;
>>ArmWriteCpuActlr (Value);
>>  }
>> +
>> +UINTN
>> +EFIAPI
>> +ArmDataCacheLineLength (
>> +  VOID
>> +  )
>> +{
>> +  return 4 << ((ArmCacheInfo () >> 16) & 0xf); // CTR_EL0.DminLine
>> +}
>> +
>> +UINTN
>> +EFIAPI
>> +ArmInstructionCacheLineLength (
>> +  VOID
>> +  )
>> +{
>> +  return 4 << (ArmCacheInfo () & 0xf); // CTR_EL0.IminLine
>> +}
>> --
>> 1.9.1
>>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 05/10] ArmPkg/ArmLib: retrieve cache line length from CTR not CCSIDR

[Ard Biesheuvel]

On 6 November 2015 at 17:13, Ard Biesheuvel  wrote:
> On 6 November 2015 at 17:02, Leif Lindholm  wrote:
>> On Fri, Nov 06, 2015 at 03:03:53PM +0100, Ard Biesheuvel wrote:
>>> The stride used by the cache maintenance by MVA instructions should
>>> be retrieved from CTR_EL0.DminLine and CTR_EL0.IminLine, whose values
>>> reflect the actual geometry of the caches. Using CCSIDR for this purpose
>>> violates the architecture.
>>
>> Does it?
>> Sure, you'd need to iterate over all levels of I/D/U cache and pick
>> the smallest, but... If the current code doesn't, then it's broken (as
>> opposed to violating the architecture).
>>
>
> Yes, it does. The ARM ARM states quite clearly that the contents of
> CCSIDR should not be used to make any inferences about the actual
> cache geometry.
>

And yes, it is indeed broken as well since it only looks at the line
length of the L1.

>>> Also, move the line length accessors to common code, since there is no
>>> need to keep them separate between ARMv7 and AArch64.
>>
>> There is for Arm9 though - the CTR format changed for ARMv7.
>> So either we turn this back into an Arm9 purge as part of the cache
>> maintenance fixes (which I would like to avoid) or I think we need to
>> keep the per-arch line length accessors for now.
>>
>
> Actually, you can't build the ARM9 version anymore, since ArmLib now
> depends on  whereas the ARM9 specific code includes
>  as well, resulting in a conflict. I don't think
> this has been buildable since bd6b97994ab6 ("ArmPkg/ArmLib: Clean
> ArmV7Lib") dated somewhere back in 2011
>
>
>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel 
>>> Reviewed-by: Mark Rutland 
>>> ---
>>>  ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c | 25 --
>>>  ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c | 27 
>>>  ArmPkg/Library/ArmLib/Common/ArmLib.c  | 18 +
>>>  3 files changed, 18 insertions(+), 52 deletions(-)
>>>
>>> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c 
>>> b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
>>> index 4bea20356fa3..dec125f248cd 100644
>>> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
>>> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
>>> @@ -21,31 +21,6 @@
>>>  #include "AArch64Lib.h"
>>>  #include "ArmLibPrivate.h"
>>>
>>> -UINTN
>>> -EFIAPI
>>> -ArmDataCacheLineLength (
>>> -  VOID
>>> -  )
>>> -{
>>> -  UINT32 CCSIDR = ReadCCSIDR (0) & 7;
>>> -
>>> -  // * 4 converts to bytes
>>> -  return (1 << (CCSIDR + 2)) * 4;
>>> -}
>>> -
>>> -UINTN
>>> -EFIAPI
>>> -ArmInstructionCacheLineLength (
>>> -  VOID
>>> -  )
>>> -{
>>> -  UINT32 CCSIDR = ReadCCSIDR (1) & 7;
>>> -
>>> -  // * 4 converts to bytes
>>> -  return (1 << (CCSIDR + 2)) * 4;
>>> -}
>>> -
>>> -
>>>  VOID
>>>  AArch64DataCacheOperation (
>>>IN  AARCH64_CACHE_OPERATION  DataCacheOperation
>>> diff --git a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c 
>>> b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c
>>> index 44edff869eae..b53f455bfad2 100644
>>> --- a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c
>>> +++ b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c
>>> @@ -20,33 +20,6 @@
>>>  #include "ArmV7Lib.h"
>>>  #include "ArmLibPrivate.h"
>>>
>>> -UINTN
>>> -EFIAPI
>>> -ArmDataCacheLineLength (
>>> -  VOID
>>> -  )
>>> -{
>>> -  UINT32 CCSIDR = ReadCCSIDR (0) & 7;
>>> -
>>> -  // * 4 converts to bytes
>>> -  return (1 << (CCSIDR + 2)) * 4;
>>> -}
>>> -
>>> -UINTN
>>> -EFIAPI
>>> -ArmInstructionCacheLineLength (
>>> -  VOID
>>> -  )
>>> -{
>>> -  UINT32 CCSIDR = ReadCCSIDR (1) & 7;
>>> -
>>> -  // * 4 converts to bytes
>>> -  return (1 << (CCSIDR + 2)) * 4;
>>> -
>>> -//  return 64;
>>> -}
>>> -
>>> -
>>>  VOID
>>>  ArmV7DataCacheOperation (
>>>IN  ARM_V7_CACHE_OPERATION  DataCacheOperation
>>> diff --git a/ArmPkg/Library/ArmLib/Common/ArmLib.c 
>>> b/ArmPkg/Library/ArmLib/Common/ArmLib.c
>>> index 4febc45220a3..ad0a265e9f59 100644
>>> --- a/ArmPkg/Library/ArmLib/Common/ArmLib.c
>>> +++ b/ArmPkg/Library/ArmLib/Common/ArmLib.c
>>> @@ -70,3 +70,21 @@ ArmUnsetCpuActlrBit (
>>>Value &= ~Bits;
>>>ArmWriteCpuActlr (Value);
>>>  }
>>> +
>>> +UINTN
>>> +EFIAPI
>>> +ArmDataCacheLineLength (
>>> +  VOID
>>> +  )
>>> +{
>>> +  return 4 << ((ArmCacheInfo () >> 16) & 0xf); // CTR_EL0.DminLine
>>> +}
>>> +
>>> +UINTN
>>> +EFIAPI
>>> +ArmInstructionCacheLineLength (
>>> +  VOID
>>> +  )
>>> +{
>>> +  return 4 << (ArmCacheInfo () & 0xf); // CTR_EL0.IminLine
>>> +}
>>> --
>>> 1.9.1
>>>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] ArmPlatformPkg/ArmJunoPkg: Create SMBIOS/DMI data for Juno

[Ard Biesheuvel]

On 6 November 2015 at 16:38, Jeremy Linton  wrote:
> On 11/06/2015 09:21 AM, Ard Biesheuvel wrote:
>>
>> I would much prefer duplicating this over adding dynamic PCDs and
>> BEFORE/AFTER depexes. If you really insist, you can break it out into
>> a separate static library, but I wouldn't even bother tbh.
>
>
> That solves the short term problem, but I expect that the SMBIOS and ACPI
> tables (and probably DT too) to have a lot more dynamic content  in the
> future. Solving the problem of providing a common platform data provider may
> be useful.
>

Indeed. But we are not solving that by adding more dynamic PCDs and
BEFORE/AFTER depexes either.

So instead, you will need to define some Juno-specific protocol that
exposes this platform information, and make the consumers depend on it
via a depex

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

Re: [edk2] [PATCH] ShellPkg: Don't strip positional parameters of quotation marks.

[Carsey, Jaben]

Reviewed-by: Jaben Carsey 

> -Original Message-
> From: Qiu, Shumin
> Sent: Friday, November 06, 2015 12:06 AM
> To: edk2-devel@lists.01.org
> Cc: Qiu, Shumin ; Carsey, Jaben
> 
> Subject: [PATCH] ShellPkg: Don't strip positional parameters of quotation
> marks.
> Importance: High
> 
> Per Shell SPEC 2.1 'Double-quotation marks that surround arguments are not
> stripped in positional parameters'. This patch makes Shell implementation to
> follow SPEC.
> 
> Cc: Jaben Carsey 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Qiu Shumin 
> ---
>  ShellPkg/Application/Shell/Shell.c | 30 ++---
>  ShellPkg/Application/Shell/Shell.h |  8 
>  .../Application/Shell/ShellParametersProtocol.c| 52 ---
> ---
>  .../Application/Shell/ShellParametersProtocol.h| 36 ++-
>  ShellPkg/Application/Shell/ShellProtocol.c |  2 +-
>  5 files changed, 80 insertions(+), 48 deletions(-)
> 
> diff --git a/ShellPkg/Application/Shell/Shell.c
> b/ShellPkg/Application/Shell/Shell.c
> index cb9d969..8af6647 100644
> --- a/ShellPkg/Application/Shell/Shell.c
> +++ b/ShellPkg/Application/Shell/Shell.c
> @@ -1863,7 +1863,7 @@ IsValidSplit(
>return (EFI_OUT_OF_RESOURCES);
>  }
>  TempWalker = (CHAR16*)Temp;
> -if (!EFI_ERROR(GetNextParameter(, ,
> StrSize(CmdLine {
> +if (!EFI_ERROR(GetNextParameter(, ,
> StrSize(CmdLine), TRUE))) {
>if (GetOperationType(FirstParameter) == Unknown_Invalid) {
>  ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_SHELL_NOT_FOUND),
> ShellInfoObject.HiiHandle, FirstParameter);
>  SetLastError(SHELL_NOT_FOUND);
> @@ -2029,7 +2029,7 @@ DoHelpUpdate(
> 
>Walker = *CmdLine;
>while(Walker != NULL && *Walker != CHAR_NULL) {
> -if (!EFI_ERROR(GetNextParameter(, ,
> StrSize(*CmdLine {
> +if (!EFI_ERROR(GetNextParameter(, ,
> StrSize(*CmdLine), TRUE))) {
>if (StrStr(CurrentParameter, L"-?") == CurrentParameter) {
>  CurrentParameter[0] = L' ';
>  CurrentParameter[1] = L' ';
> @@ -2173,7 +2173,7 @@ RunInternalCommand(
>//
>// get the argc and argv updated for internal commands
>//
> -  Status = UpdateArgcArgv(ParamProtocol, NewCmdLine, , );
> +  Status = UpdateArgcArgv(ParamProtocol, NewCmdLine, Internal_Command,
> , );
>if (!EFI_ERROR(Status)) {
>  //
>  // Run the internal command.
> @@ -2520,7 +2520,7 @@ RunShellCommand(
>  return (EFI_OUT_OF_RESOURCES);
>}
>TempWalker = CleanOriginal;
> -  if (!EFI_ERROR(GetNextParameter(, ,
> StrSize(CleanOriginal {
> +  if (!EFI_ERROR(GetNextParameter(, ,
> StrSize(CleanOriginal), TRUE))) {
>  //
>  // Depending on the first parameter we change the behavior
>  //
> @@ -2767,34 +2767,34 @@ RunScriptFileHandle (
>if (NewScriptFile->Argv != NULL) {
>  switch (NewScriptFile->Argc) {
>default:
> -Status = ShellCopySearchAndReplace(CommandLine2,  CommandLine,
> PrintBuffSize, L"%9", NewScriptFile->Argv[9], FALSE, TRUE);
> +Status = ShellCopySearchAndReplace(CommandLine2,  CommandLine,
> PrintBuffSize, L"%9", NewScriptFile->Argv[9], FALSE, FALSE);
>  ASSERT_EFI_ERROR(Status);
>case 9:
> -Status = ShellCopySearchAndReplace(CommandLine,  CommandLine2,
> PrintBuffSize, L"%8", NewScriptFile->Argv[8], FALSE, TRUE);
> +Status = ShellCopySearchAndReplace(CommandLine,  CommandLine2,
> PrintBuffSize, L"%8", NewScriptFile->Argv[8], FALSE, FALSE);
>  ASSERT_EFI_ERROR(Status);
>case 8:
> -Status = ShellCopySearchAndReplace(CommandLine2,  CommandLine,
> PrintBuffSize, L"%7", NewScriptFile->Argv[7], FALSE, TRUE);
> +Status = ShellCopySearchAndReplace(CommandLine2,  CommandLine,
> PrintBuffSize, L"%7", NewScriptFile->Argv[7], FALSE, FALSE);
>  ASSERT_EFI_ERROR(Status);
>case 7:
> -Status = ShellCopySearchAndReplace(CommandLine,  CommandLine2,
> PrintBuffSize, L"%6", NewScriptFile->Argv[6], FALSE, TRUE);
> +Status = ShellCopySearchAndReplace(CommandLine,  CommandLine2,
> PrintBuffSize, L"%6", NewScriptFile->Argv[6], FALSE, FALSE);
>  ASSERT_EFI_ERROR(Status);
>case 6:
> -Status = ShellCopySearchAndReplace(CommandLine2,  CommandLine,
> PrintBuffSize, L"%5", NewScriptFile->Argv[5], FALSE, TRUE);
> +Status = ShellCopySearchAndReplace(CommandLine2,  CommandLine,
> PrintBuffSize, L"%5", NewScriptFile->Argv[5], FALSE, FALSE);
>  ASSERT_EFI_ERROR(Status);
>case 5:
> -Status = ShellCopySearchAndReplace(CommandLine,  CommandLine2,
> PrintBuffSize, L"%4", NewScriptFile->Argv[4], FALSE, TRUE);
> +Status = ShellCopySearchAndReplace(CommandLine,  

Re: [edk2] [PATCH v2 05/10] ArmPkg/ArmLib: retrieve cache line length from CTR not CCSIDR

[Leif Lindholm]

On Fri, Nov 06, 2015 at 05:13:44PM +0100, Ard Biesheuvel wrote:
> On 6 November 2015 at 17:02, Leif Lindholm  wrote:
> > On Fri, Nov 06, 2015 at 03:03:53PM +0100, Ard Biesheuvel wrote:
> >> The stride used by the cache maintenance by MVA instructions should
> >> be retrieved from CTR_EL0.DminLine and CTR_EL0.IminLine, whose values
> >> reflect the actual geometry of the caches. Using CCSIDR for this purpose
> >> violates the architecture.
> >
> > Does it?
> > Sure, you'd need to iterate over all levels of I/D/U cache and pick
> > the smallest, but... If the current code doesn't, then it's broken (as
> > opposed to violating the architecture).
> 
> Yes, it does. The ARM ARM states quite clearly that the contents of
> CCSIDR should not be used to make any inferences about the actual
> cache geometry.

It says it should not be used to infer actual sizes of caches, but is
also says that it describes "architecturally visible parameters that
are required for the cache maintenance by Set/Way instructions". (At
least the v7 one.)

It's a note of warning to people to not try to infer performance
characteristics based on these values.

> >> Also, move the line length accessors to common code, since there is no
> >> need to keep them separate between ARMv7 and AArch64.
> >
> > There is for Arm9 though - the CTR format changed for ARMv7.
> > So either we turn this back into an Arm9 purge as part of the cache
> > maintenance fixes (which I would like to avoid) or I think we need to
> > keep the per-arch line length accessors for now.
> 
> Actually, you can't build the ARM9 version anymore, since ArmLib now
> depends on  whereas the ARM9 specific code includes
>  as well, resulting in a conflict. I don't think
> this has been buildable since bd6b97994ab6 ("ArmPkg/ArmLib: Clean
> ArmV7Lib") dated somewhere back in 2011

Yeah :/

Oh well, guess it's time for a purge.

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

Re: [edk2] [PATCH v2 05/10] ArmPkg/ArmLib: retrieve cache line length from CTR not CCSIDR

[Ard Biesheuvel]

On 6 November 2015 at 17:33, Leif Lindholm  wrote:
> On Fri, Nov 06, 2015 at 05:13:44PM +0100, Ard Biesheuvel wrote:
>> On 6 November 2015 at 17:02, Leif Lindholm  wrote:
>> > On Fri, Nov 06, 2015 at 03:03:53PM +0100, Ard Biesheuvel wrote:
>> >> The stride used by the cache maintenance by MVA instructions should
>> >> be retrieved from CTR_EL0.DminLine and CTR_EL0.IminLine, whose values
>> >> reflect the actual geometry of the caches. Using CCSIDR for this purpose
>> >> violates the architecture.
>> >
>> > Does it?
>> > Sure, you'd need to iterate over all levels of I/D/U cache and pick
>> > the smallest, but... If the current code doesn't, then it's broken (as
>> > opposed to violating the architecture).
>>
>> Yes, it does. The ARM ARM states quite clearly that the contents of
>> CCSIDR should not be used to make any inferences about the actual
>> cache geometry.
>
> It says it should not be used to infer actual sizes of caches, but is
> also says that it describes "architecturally visible parameters that
> are required for the cache maintenance by Set/Way instructions". (At
> least the v7 one.)
>

Indeed. The Set/Way fields of those cache maintenance instructions are
formatted according to these parameters, so the CCSIDR values need to
be in sync with what those instructions expect. It is thus legal to
expose 1 set, 1 way and a 16 byte linesize (i.e., all zeroes, as the
Denver CPU does afaik) while the actual geometry is completely
different.

> It's a note of warning to people to not try to infer performance
> characteristics based on these values.
>

No, it's a note saying 'look here for the values to poke into the DC
ISW/CSW/CISW instructions but don't use them for anything else'
In the kernel, we were using this information to calculate the waysize
and compare it to the PAGE_SIZE, to infer whether a VIPT I-cache could
alias or not, and the NVIDIA engineers proposed some patches to remove
it since the CCSIDR info is not suitable for doing that.

>> >> Also, move the line length accessors to common code, since there is no
>> >> need to keep them separate between ARMv7 and AArch64.
>> >
>> > There is for Arm9 though - the CTR format changed for ARMv7.
>> > So either we turn this back into an Arm9 purge as part of the cache
>> > maintenance fixes (which I would like to avoid) or I think we need to
>> > keep the per-arch line length accessors for now.
>>
>> Actually, you can't build the ARM9 version anymore, since ArmLib now
>> depends on  whereas the ARM9 specific code includes
>>  as well, resulting in a conflict. I don't think
>> this has been buildable since bd6b97994ab6 ("ArmPkg/ArmLib: Clean
>> ArmV7Lib") dated somewhere back in 2011
>
> Yeah :/
>
> Oh well, guess it's time for a purge.
>

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

[edk2] [PATCH] ArmPkg/BdsLib: Increase fallback tftp buffer size

[Ashutosh Singh]

When performing a tftp download from a server which does not support
rfc2349 transfer size option (such as netkit-tftpd), the existing code
falls back to allocating an 8MB buffer. Since this is insufficient for
an uncompressed AArch64 Linux kernel image, double the size to 16MB.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ashutosh Singh 
---
 ArmPkg/Library/BdsLib/BdsFilePath.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ArmPkg/Library/BdsLib/BdsFilePath.c 
b/ArmPkg/Library/BdsLib/BdsFilePath.c
index ff42175..0410236 100644
--- a/ArmPkg/Library/BdsLib/BdsFilePath.c
+++ b/ArmPkg/Library/BdsLib/BdsFilePath.c
@@ -1198,7 +1198,7 @@ BdsTftpLoadImage (
   if (Mtftp4GetFileSize (Mtftp4, AsciiFilePath, ) == EFI_SUCCESS) {
 TftpBufferSize = FileSize;
   } else {
-TftpBufferSize = SIZE_8MB;
+TftpBufferSize = SIZE_16MB;
   }

   TftpContext = AllocatePool (sizeof (BDS_TFTP_CONTEXT));
@@ -1209,7 +1209,7 @@ BdsTftpLoadImage (
   TftpContext->FileSize = FileSize;

   for (; TftpBufferSize <= FixedPcdGet32 (PcdMaxTftpFileSize);
- TftpBufferSize = (TftpBufferSize + SIZE_8MB) & (~(SIZE_8MB-1))) {
+ TftpBufferSize = (TftpBufferSize + SIZE_16MB) & (~(SIZE_16MB-1))) {
 //
 // Allocate a buffer to hold the whole file.
 //
--
1.7.9.5




-- 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] Increase tftp buffer size

[Ashutosh Singh]

> So, I'm not a fan of hard-coded buffer sizes, or how this existing bit
> of code duplicates functionality already available elsewhere in the
> tree ... but I also don't care much about ArmBds.
>
> What I'm curious about is - why is your Mtftp4GetFileSize call failing
> such that you need to use a hardcoded buffer size?
> Does your TFTP server not support rfc2349?
> Would changing that (for example by switching to tftpd-hpa over the
> unmaintained netkit-tftpd) not make more sense?

Took a while to test with tftpd-hpa, with this it does work fine, i.e.
Mtftp4GetFileSize does succeed and buffer size is allocated accordingly.
I would still request you to merge the change, as it will mean people
using the default tftpd that comes with ubuntu distribution will also
be able to use the tftp boot for ARM platforms.

-Original Message-
From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
Sent: 05 November 2015 12:26
To: Ashutosh Singh
Cc: edk2-de...@ml01.01.org; ard.biesheu...@linaro.org; ryan.har...@linaro.org; 
James King
Subject: Re: [PATCH] Increase tftp buffer size

On Thu, Nov 05, 2015 at 11:37:43AM +, Ashutosh Singh wrote:
> Default tftp buffer size (8MB) is too small for standard
> ARM kernel images, which results in multiple aborted tftp fetches.
> This fix increase the buffer size to 16MB.

So, I'm not a fan of hard-coded buffer sizes, or how this existing bit
of code duplicates functionality already available elsewhere in the
tree ... but I also don't care much about ArmBds.

What I'm curious about is - why is your Mtftp4GetFileSize call failing
such that you need to use a hardcoded buffer size?
Does your TFTP server not support rfc2349?
Would changing that (for example by switching to tftpd-hpa over the
unmaintained netkit-tftpd) not make more sense?

/
Leif

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ashutosh Singh 
> ---
>  ArmPkg/Library/BdsLib/BdsFilePath.c |4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/ArmPkg/Library/BdsLib/BdsFilePath.c 
> b/ArmPkg/Library/BdsLib/BdsFilePath.c
> index ff42175..0410236 100644
> --- a/ArmPkg/Library/BdsLib/BdsFilePath.c
> +++ b/ArmPkg/Library/BdsLib/BdsFilePath.c
> @@ -1198,7 +1198,7 @@ BdsTftpLoadImage (
>if (Mtftp4GetFileSize (Mtftp4, AsciiFilePath, ) == EFI_SUCCESS) {
>  TftpBufferSize = FileSize;
>} else {
> -TftpBufferSize = SIZE_8MB;
> +TftpBufferSize = SIZE_16MB;
>}
>
>TftpContext = AllocatePool (sizeof (BDS_TFTP_CONTEXT));
> @@ -1209,7 +1209,7 @@ BdsTftpLoadImage (
>TftpContext->FileSize = FileSize;
>
>for (; TftpBufferSize <= FixedPcdGet32 (PcdMaxTftpFileSize);
> - TftpBufferSize = (TftpBufferSize + SIZE_8MB) & (~(SIZE_8MB-1))) {
> + TftpBufferSize = (TftpBufferSize + SIZE_16MB) & (~(SIZE_16MB-1))) {
>  //
>  // Allocate a buffer to hold the whole file.
>  //
> --
> 1.7.9.5
>
>
> 
>
> -- 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.
>




-- 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

[edk2] [patch] Add error handling for TPM in S3 resume failure.

[jiewen yao]

If TPM2_Startup(TPM_SU_STATE) to return an error, the system
 firmware that resumes from S3 MUST deal with a TPM2_Startup
 error appropriately.
For example, issuing a TPM2_Startup(TPM_SU_CLEAR) command and
 configuring the device securely by taking actions like extending
 a separator with an error digest (0x01) into PCRs 0 through 7.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Yao, Jiewen 
Cc: Zhang, Chao B 
---
 SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c | 43 +++
 1 file changed, 43 insertions(+)

diff --git a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c 
b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
index 4ecfbe3..2e4ad53 100644
--- a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
+++ b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
@@ -829,6 +829,33 @@ PeimEntryMP (
 }
 
 /**
+  Measure and log Separator event with error, and extend the measurement 
result into a specific PCR.
+
+  @param[in] PCRIndex PCR index.  
+
+  @retval EFI_SUCCESS Operation completed successfully.
+  @retval EFI_DEVICE_ERRORThe operation was unsuccessful.
+
+**/
+EFI_STATUS
+MeasureSeparatorEventWithError (
+  IN  TPM_PCRINDEX  PCRIndex
+  )
+{
+  TCG_PCR_EVENT_HDR TcgEvent;
+  UINT32EventData;
+
+  //
+  // Use EventData 0x1 to indicate there is error.
+  //
+  EventData = 0x1;
+  TcgEvent.PCRIndex  = PCRIndex;
+  TcgEvent.EventType = EV_SEPARATOR;
+  TcgEvent.EventSize = (UINT32)sizeof (EventData);
+  return HashLogExtendEvent(0,(UINT8 *), TcgEvent.EventSize, 
,(UINT8 *));
+}
+
+/**
   Entry point of this module.
 
   @param[in] FileHandle   Handle of the file being invoked.
@@ -847,6 +874,7 @@ PeimEntryMA (
   EFI_STATUSStatus;
   EFI_STATUSStatus2;
   EFI_BOOT_MODE BootMode;
+  TPM_PCRINDEX  PcrIndex;
 
   if (CompareGuid (PcdGetPtr(PcdTpmInstanceGuid), 
) ||
   CompareGuid (PcdGetPtr(PcdTpmInstanceGuid), 
)){
@@ -889,7 +917,22 @@ PeimEntryMA (
   if (BootMode == BOOT_ON_S3_RESUME) {
 Status = Tpm2Startup (TPM_SU_STATE);
 if (EFI_ERROR (Status) ) {
+  //
+  // The system firmware that resumes from S3 MUST deal with a
+  // TPM2_Startup error appropriately.
+  // For example, issue a TPM2_Startup(TPM_SU_CLEAR) command and
+  // configuring the device securely by taking actions like extending a
+  // separator with an error digest (0x01) into PCRs 0 through 7.
+  //
   Status = Tpm2Startup (TPM_SU_CLEAR);
+  if (!EFI_ERROR(Status)) {
+for (PcrIndex = 0; PcrIndex < 8; PcrIndex++) {
+  Status = MeasureSeparatorEventWithError (PcrIndex);
+  if (EFI_ERROR (Status)) {
+DEBUG ((EFI_D_ERROR, "Seperator Event with Error not Measured. 
Error!\n"));
+  }
+}
+  }
 }
   } else {
 Status = Tpm2Startup (TPM_SU_CLEAR);
-- 
1.9.5.msysgit.0

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

[edk2] [PATCH] ArmPkg: Purge unused/unneeded CPU-specific header files

[Leif Lindholm]

In ArmPkg/Include/Chipset, several CPU-specific header files reside.
Most of these provide no actual, or very little, use.
ARM1176JZ-S.h   is not used at all (and unusable since SVN r18237).
ArmAemV8.h  simply includes AArch64.h.
ArmCortexA15.h  defines one processor-specific configuration bit and
then includes ArmV7.h.

Delete these include files, and update their sole users to function
without them.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Leif Lindholm 
---
 .../ArmCpuLib/ArmCortexA15Lib/ArmCortexA15Lib.c|   4 +-
 .../ArmCortexAEMv8Lib/ArmCortexAEMv8Lib.c  |   2 +-
 ArmPkg/Include/Chipset/ARM1176JZ-S.h   | 127 -
 ArmPkg/Include/Chipset/ArmAemV8.h  |  21 
 ArmPkg/Include/Chipset/ArmCortexA15.h  |  25 
 5 files changed, 4 insertions(+), 175 deletions(-)
 delete mode 100644 ArmPkg/Include/Chipset/ARM1176JZ-S.h
 delete mode 100644 ArmPkg/Include/Chipset/ArmAemV8.h
 delete mode 100644 ArmPkg/Include/Chipset/ArmCortexA15.h

diff --git a/ArmPkg/Drivers/ArmCpuLib/ArmCortexA15Lib/ArmCortexA15Lib.c 
b/ArmPkg/Drivers/ArmCpuLib/ArmCortexA15Lib/ArmCortexA15Lib.c
index a1678fe..e05e8ab 100644
--- a/ArmPkg/Drivers/ArmCpuLib/ArmCortexA15Lib/ArmCortexA15Lib.c
+++ b/ArmPkg/Drivers/ArmCpuLib/ArmCortexA15Lib/ArmCortexA15Lib.c
@@ -19,7 +19,9 @@
 #include 
 #include 
 
-#include 
+#include 
+
+#define A15_FEATURE_SMP (1<<6)
 
 VOID
 ArmCpuSetup (
diff --git a/ArmPkg/Drivers/ArmCpuLib/ArmCortexAEMv8Lib/ArmCortexAEMv8Lib.c 
b/ArmPkg/Drivers/ArmCpuLib/ArmCortexAEMv8Lib/ArmCortexAEMv8Lib.c
index 9b1815f..ee2c057 100644
--- a/ArmPkg/Drivers/ArmCpuLib/ArmCortexAEMv8Lib/ArmCortexAEMv8Lib.c
+++ b/ArmPkg/Drivers/ArmCpuLib/ArmCortexAEMv8Lib/ArmCortexAEMv8Lib.c
@@ -17,7 +17,7 @@
 #include 
 #include 
 
-#include 
+#include 
 
 VOID
 ArmCpuSetup (
diff --git a/ArmPkg/Include/Chipset/ARM1176JZ-S.h 
b/ArmPkg/Include/Chipset/ARM1176JZ-S.h
deleted file mode 100644
index ba24bcb..000
--- a/ArmPkg/Include/Chipset/ARM1176JZ-S.h
+++ /dev/null
@@ -1,127 +0,0 @@
-/** @file
-
-  Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.
-  Copyright (c) 2011-2012, 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
-  which accompanies this distribution.  The full text of the license may be 
found at
-  http://opensource.org/licenses/bsd-license.php
-
-  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
-  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
-
-**/
-
-#ifndef __ARM1176JZ_S_H__
-#define __ARM1176JZ_S_H__
-
-// Domain Access Control Register
-#define DOMAIN_ACCESS_CONTROL_MASK(a) (3UL << (2 * (a)))
-#define DOMAIN_ACCESS_CONTROL_NONE(a) (0UL << (2 * (a)))
-#define DOMAIN_ACCESS_CONTROL_CLIENT(a)   (1UL << (2 * (a)))
-#define DOMAIN_ACCESS_CONTROL_RESERVED(a) (2UL << (2 * (a)))
-#define DOMAIN_ACCESS_CONTROL_MANAGER(a)  (3UL << (2 * (a)))
-
-#define TRANSLATION_TABLE_SIZE(16 * 1024)
-#define TRANSLATION_TABLE_ALIGNMENT   (16 * 1024)
-#define TRANSLATION_TABLE_ALIGNMENT_MASK  (TRANSLATION_TABLE_ALIGNMENT - 1)
-
-#define TRANSLATION_TABLE_ENTRY_FOR_VIRTUAL_ADDRESS(table, address) ((UINT32 
*)(table) + (((UINTN)(address)) >> 20))
-
-// Translation table descriptor types
-#define TT_DESCRIPTOR_TYPE_MASK ((1UL << 18) | (3UL << 0))
-#define TT_DESCRIPTOR_TYPE_PAGE_TABLE   ((0UL << 18) | (1UL << 0))
-#define TT_DESCRIPTOR_TYPE_SECTION  ((0UL << 18) | (2UL << 0))
-#define TT_DESCRIPTOR_TYPE_SUPERSECTION ((1UL << 18) | (2UL << 0))
-
-// Section descriptor definitions
-#define TT_DESCRIPTOR_SECTION_SIZE  (0x0010)
-
-#define TT_DESCRIPTOR_SECTION_NS_MASK   (1UL << 19)
-#define TT_DESCRIPTOR_SECTION_NS(1UL << 19)
-
-#define TT_DESCRIPTOR_SECTION_NG_MASK   (1UL << 17)
-#define TT_DESCRIPTOR_SECTION_NG_GLOBAL (0UL << 17)
-#define TT_DESCRIPTOR_SECTION_NG_LOCAL  (1UL << 17)
-
-#define TT_DESCRIPTOR_SECTION_S_MASK(1UL << 16)
-#define TT_DESCRIPTOR_SECTION_S_NOT_SHARED  (0UL << 16)
-#define TT_DESCRIPTOR_SECTION_S_SHARED  (1UL << 16)
-
-#define TT_DESCRIPTOR_SECTION_AP_MASK   ((1UL << 15) | 
(3UL << 10))
-#define TT_DESCRIPTOR_SECTION_AP_NO_NO  ((0UL << 15) | 
(0UL << 10))
-#define TT_DESCRIPTOR_SECTION_AP_RW_NO  ((0UL << 15) | 
(1UL << 10))
-#define TT_DESCRIPTOR_SECTION_AP_RW_RO  ((0UL << 15) | 
(2UL << 10))
-#define TT_DESCRIPTOR_SECTION_AP_RW_RW  ((0UL << 15) | 
(3UL << 10))
-#define TT_DESCRIPTOR_SECTION_AP_RO_NO  ((1UL 

Re: [edk2] [PATCH] ArmPkg: Purge unused/unneeded CPU-specific header files

[Ard Biesheuvel]

On 6 November 2015 at 18:16, Leif Lindholm  wrote:
> In ArmPkg/Include/Chipset, several CPU-specific header files reside.
> Most of these provide no actual, or very little, use.
> ARM1176JZ-S.h   is not used at all (and unusable since SVN r18237).
> ArmAemV8.h  simply includes AArch64.h.
> ArmCortexA15.h  defines one processor-specific configuration bit and
> then includes ArmV7.h.
>
> Delete these include files, and update their sole users to function
> without them.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Leif Lindholm 

Nice!

Reviewed-by: Ard Biesheuvel 


> ---
>  .../ArmCpuLib/ArmCortexA15Lib/ArmCortexA15Lib.c|   4 +-
>  .../ArmCortexAEMv8Lib/ArmCortexAEMv8Lib.c  |   2 +-
>  ArmPkg/Include/Chipset/ARM1176JZ-S.h   | 127 
> -
>  ArmPkg/Include/Chipset/ArmAemV8.h  |  21 
>  ArmPkg/Include/Chipset/ArmCortexA15.h  |  25 
>  5 files changed, 4 insertions(+), 175 deletions(-)
>  delete mode 100644 ArmPkg/Include/Chipset/ARM1176JZ-S.h
>  delete mode 100644 ArmPkg/Include/Chipset/ArmAemV8.h
>  delete mode 100644 ArmPkg/Include/Chipset/ArmCortexA15.h
>
> diff --git a/ArmPkg/Drivers/ArmCpuLib/ArmCortexA15Lib/ArmCortexA15Lib.c 
> b/ArmPkg/Drivers/ArmCpuLib/ArmCortexA15Lib/ArmCortexA15Lib.c
> index a1678fe..e05e8ab 100644
> --- a/ArmPkg/Drivers/ArmCpuLib/ArmCortexA15Lib/ArmCortexA15Lib.c
> +++ b/ArmPkg/Drivers/ArmCpuLib/ArmCortexA15Lib/ArmCortexA15Lib.c
> @@ -19,7 +19,9 @@
>  #include 
>  #include 
>
> -#include 
> +#include 
> +
> +#define A15_FEATURE_SMP (1<<6)
>
>  VOID
>  ArmCpuSetup (
> diff --git a/ArmPkg/Drivers/ArmCpuLib/ArmCortexAEMv8Lib/ArmCortexAEMv8Lib.c 
> b/ArmPkg/Drivers/ArmCpuLib/ArmCortexAEMv8Lib/ArmCortexAEMv8Lib.c
> index 9b1815f..ee2c057 100644
> --- a/ArmPkg/Drivers/ArmCpuLib/ArmCortexAEMv8Lib/ArmCortexAEMv8Lib.c
> +++ b/ArmPkg/Drivers/ArmCpuLib/ArmCortexAEMv8Lib/ArmCortexAEMv8Lib.c
> @@ -17,7 +17,7 @@
>  #include 
>  #include 
>
> -#include 
> +#include 
>
>  VOID
>  ArmCpuSetup (
> diff --git a/ArmPkg/Include/Chipset/ARM1176JZ-S.h 
> b/ArmPkg/Include/Chipset/ARM1176JZ-S.h
> deleted file mode 100644
> index ba24bcb..000
> --- a/ArmPkg/Include/Chipset/ARM1176JZ-S.h
> +++ /dev/null
> @@ -1,127 +0,0 @@
> -/** @file
> -
> -  Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.
> -  Copyright (c) 2011-2012, 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
> -  which accompanies this distribution.  The full text of the license may be 
> found at
> -  http://opensource.org/licenses/bsd-license.php
> -
> -  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> -  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> -
> -**/
> -
> -#ifndef __ARM1176JZ_S_H__
> -#define __ARM1176JZ_S_H__
> -
> -// Domain Access Control Register
> -#define DOMAIN_ACCESS_CONTROL_MASK(a) (3UL << (2 * (a)))
> -#define DOMAIN_ACCESS_CONTROL_NONE(a) (0UL << (2 * (a)))
> -#define DOMAIN_ACCESS_CONTROL_CLIENT(a)   (1UL << (2 * (a)))
> -#define DOMAIN_ACCESS_CONTROL_RESERVED(a) (2UL << (2 * (a)))
> -#define DOMAIN_ACCESS_CONTROL_MANAGER(a)  (3UL << (2 * (a)))
> -
> -#define TRANSLATION_TABLE_SIZE(16 * 1024)
> -#define TRANSLATION_TABLE_ALIGNMENT   (16 * 1024)
> -#define TRANSLATION_TABLE_ALIGNMENT_MASK  (TRANSLATION_TABLE_ALIGNMENT - 1)
> -
> -#define TRANSLATION_TABLE_ENTRY_FOR_VIRTUAL_ADDRESS(table, address) ((UINT32 
> *)(table) + (((UINTN)(address)) >> 20))
> -
> -// Translation table descriptor types
> -#define TT_DESCRIPTOR_TYPE_MASK ((1UL << 18) | (3UL << 0))
> -#define TT_DESCRIPTOR_TYPE_PAGE_TABLE   ((0UL << 18) | (1UL << 0))
> -#define TT_DESCRIPTOR_TYPE_SECTION  ((0UL << 18) | (2UL << 0))
> -#define TT_DESCRIPTOR_TYPE_SUPERSECTION ((1UL << 18) | (2UL << 0))
> -
> -// Section descriptor definitions
> -#define TT_DESCRIPTOR_SECTION_SIZE  (0x0010)
> -
> -#define TT_DESCRIPTOR_SECTION_NS_MASK   (1UL << 19)
> -#define TT_DESCRIPTOR_SECTION_NS(1UL << 19)
> -
> -#define TT_DESCRIPTOR_SECTION_NG_MASK   (1UL << 17)
> -#define TT_DESCRIPTOR_SECTION_NG_GLOBAL (0UL << 17)
> -#define TT_DESCRIPTOR_SECTION_NG_LOCAL  (1UL << 17)
> -
> -#define TT_DESCRIPTOR_SECTION_S_MASK(1UL << 16)
> -#define TT_DESCRIPTOR_SECTION_S_NOT_SHARED  (0UL << 16)
> -#define TT_DESCRIPTOR_SECTION_S_SHARED  (1UL << 16)
> -
> -#define TT_DESCRIPTOR_SECTION_AP_MASK   ((1UL << 15) 
> | (3UL << 10))
> -#define TT_DESCRIPTOR_SECTION_AP_NO_NO  ((0UL << 

[edk2] [PATCH v2 02/10] ArmPkg BeagleBoardPkg Omap35xxPkg: fix typo 'ArmDataSyncronizationBarrier'

[Ard Biesheuvel]

Replace all instances of ArmDataSyncronizationBarrier with
ArmDataSynchronizationBarrier.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
Acked-by: Mark Rutland 
---
 ArmPkg/Include/Library/ArmLib.h | 2 +-
 ArmPkg/Library/ArmLib/AArch64/AArch64Support.S  | 4 ++--
 ArmPkg/Library/ArmLib/Arm9/Arm9Support.S| 4 ++--
 ArmPkg/Library/ArmLib/Arm9/Arm9Support.asm  | 4 ++--
 ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S  | 4 ++--
 ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm| 4 ++--
 BeagleBoardPkg/Library/BeagleBoardLib/BeagleBoard.c | 4 ++--
 Omap35xxPkg/InterruptDxe/HardwareInterrupt.c| 6 +++---
 Omap35xxPkg/Library/DebugAgentTimerLib/DebugAgentTimerLib.c | 2 +-
 9 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
index b4768841bd9d..58116663b28d 100644
--- a/ArmPkg/Include/Library/ArmLib.h
+++ b/ArmPkg/Include/Library/ArmLib.h
@@ -483,7 +483,7 @@ ArmDataMemoryBarrier (
 
 VOID
 EFIAPI
-ArmDataSyncronizationBarrier (
+ArmDataSynchronizationBarrier (
   VOID
   );
 
diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S 
b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
index 28cf27fbd1b6..8b5e0fb6e7fe 100644
--- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
+++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
@@ -42,7 +42,7 @@ GCC_ASM_EXPORT (ArmDisableBranchPrediction)
 GCC_ASM_EXPORT (AArch64AllDataCachesOperation)
 GCC_ASM_EXPORT (AArch64PerformPoUDataCacheOperation)
 GCC_ASM_EXPORT (ArmDataMemoryBarrier)
-GCC_ASM_EXPORT (ArmDataSyncronizationBarrier)
+GCC_ASM_EXPORT (ArmDataSynchronizationBarrier)
 GCC_ASM_EXPORT (ArmInstructionSynchronizationBarrier)
 GCC_ASM_EXPORT (ArmWriteVBar)
 GCC_ASM_EXPORT (ArmReadVBar)
@@ -389,7 +389,7 @@ ASM_PFX(ArmDataMemoryBarrier):
   ret
 
 
-ASM_PFX(ArmDataSyncronizationBarrier):
+ASM_PFX(ArmDataSynchronizationBarrier):
 ASM_PFX(ArmDrainWriteBuffer):
   dsb   sy
   ret
diff --git a/ArmPkg/Library/ArmLib/Arm9/Arm9Support.S 
b/ArmPkg/Library/ArmLib/Arm9/Arm9Support.S
index c708d212a9a8..d75f8bea1b04 100644
--- a/ArmPkg/Library/ArmLib/Arm9/Arm9Support.S
+++ b/ArmPkg/Library/ArmLib/Arm9/Arm9Support.S
@@ -31,7 +31,7 @@ GCC_ASM_EXPORT(ArmDisableInstructionCache)
 GCC_ASM_EXPORT(ArmEnableBranchPrediction)
 GCC_ASM_EXPORT(ArmDisableBranchPrediction)
 GCC_ASM_EXPORT(ArmDataMemoryBarrier)
-GCC_ASM_EXPORT(ArmDataSyncronizationBarrier)
+GCC_ASM_EXPORT(ArmDataSynchronizationBarrier)
 GCC_ASM_EXPORT(ArmInstructionSynchronizationBarrier)
 
 
@@ -139,7 +139,7 @@ ASM_PFX(ArmDataMemoryBarrier):
   mcr P15, #0, R0, C7, C10, #5@ check if this is OK?
   bx  LR
 
-ASM_PFX(ArmDataSyncronizationBarrier):
+ASM_PFX(ArmDataSynchronizationBarrier):
   mov R0, #0
   mcr P15, #0, R0, C7, C10, #4   @ check if this is OK?
   bx  LR
diff --git a/ArmPkg/Library/ArmLib/Arm9/Arm9Support.asm 
b/ArmPkg/Library/ArmLib/Arm9/Arm9Support.asm
index 4aaa546ca0c8..712c6a3f5fd7 100644
--- a/ArmPkg/Library/ArmLib/Arm9/Arm9Support.asm
+++ b/ArmPkg/Library/ArmLib/Arm9/Arm9Support.asm
@@ -29,7 +29,7 @@
 EXPORT  ArmEnableBranchPrediction
 EXPORT  ArmDisableBranchPrediction
 EXPORT  ArmDataMemoryBarrier
-EXPORT  ArmDataSyncronizationBarrier
+EXPORT  ArmDataSynchronizationBarrier
 EXPORT  ArmInstructionSynchronizationBarrier
 
 
@@ -140,7 +140,7 @@ ASM_PFX(ArmDataMemoryBarrier):
   mcr P15, #0, R0, C7, C10, #5  ; Check to see if this is correct
   bx  LR
 
-ASM_PFX(ArmDataSyncronizationBarrier):
+ASM_PFX(ArmDataSynchronizationBarrier):
   mov R0, #0
   mcr P15, #0, R0, C7, C10, #4 ; Check to see if this is correct
   bx  LR
diff --git a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S 
b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S
index af5ec23a1a4f..f59cd5f32e6b 100644
--- a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S
+++ b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S
@@ -40,7 +40,7 @@ GCC_ASM_EXPORT (ArmSetHighVectors)
 GCC_ASM_EXPORT (ArmV7AllDataCachesOperation)
 GCC_ASM_EXPORT (ArmV7PerformPoUDataCacheOperation)
 GCC_ASM_EXPORT (ArmDataMemoryBarrier)
-GCC_ASM_EXPORT (ArmDataSyncronizationBarrier)
+GCC_ASM_EXPORT (ArmDataSynchronizationBarrier)
 GCC_ASM_EXPORT (ArmInstructionSynchronizationBarrier)
 GCC_ASM_EXPORT (ArmReadVBar)
 GCC_ASM_EXPORT (ArmWriteVBar)
@@ -321,7 +321,7 @@ ASM_PFX(ArmDataMemoryBarrier):
   dmb
   bx  LR
 
-ASM_PFX(ArmDataSyncronizationBarrier):
+ASM_PFX(ArmDataSynchronizationBarrier):
 ASM_PFX(ArmDrainWriteBuffer):
   dsb
   bx  LR
diff --git a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm 
b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm
index 2b13811dc6cf..07ff1ae15a6a 100644
--- a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm
+++ b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm
@@ -37,7 +37,7 @@
 EXPORT  ArmV7AllDataCachesOperation
 EXPORT  

[edk2] [PATCH v2 01/10] ArmPkg/ArmLib: fix barriers in AArch64 ArmEnableMmu

[Ard Biesheuvel]

From: Mark Rutland 

The ARM architecture requires a DSB to complete TLB maintenance, with a
subsequent ISB being required to synchronize subsequent items in the
current instruction stream against the completed TLB maintenance.

The ArmEnableMmu function is currently missing the DSB, and hence the
TLB maintenance is not guaranteed to have completed at the point the MMU
is enabled. This may result in unpredictable behaviour.

The DSB subsequent to the write to SCTLR_EL1 is unnecessary; the ISB
alone is sufficient to complete all prior instructions and to
synchronise the new context with any subsequent instructions.

This patch adds missing DSBs to complete TLB maintenance, and removes
the unnecessary trailing DSB.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Mark Rutland 
Reviewed-by: Ard Biesheuvel 
---
 ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S 
b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
index bdede48724e6..28cf27fbd1b6 100644
--- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
+++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
@@ -123,18 +123,20 @@ ASM_PFX(ArmEnableMmu):
 4: orr x0, x0, #CTRL_M_BIT // Set MMU enable bit
EL1_OR_EL2_OR_EL3(x1)
 1: tlbivmalle1
+   dsb nsh
isb
msr sctlr_el1, x0   // Write back
b   4f
 2: tlbialle2
+   dsb nsh
isb
msr sctlr_el2, x0   // Write back
b   4f
 3: tlbialle3
+   dsb nsh
isb
msr sctlr_el3, x0   // Write back
-4: dsb sy
-   isb
+4: isb
ret
 
 
-- 
1.9.1

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

Re: [edk2] [PATCH] Increase tftp buffer size

[Laszlo Ersek]

On 11/06/15 14:31, Ashutosh Singh wrote:
>> So, I'm not a fan of hard-coded buffer sizes, or how this existing bit
>> of code duplicates functionality already available elsewhere in the
>> tree ... but I also don't care much about ArmBds.
>>
>> What I'm curious about is - why is your Mtftp4GetFileSize call failing
>> such that you need to use a hardcoded buffer size?
>> Does your TFTP server not support rfc2349?
>> Would changing that (for example by switching to tftpd-hpa over the
>> unmaintained netkit-tftpd) not make more sense?
> 
> Took a while to test with tftpd-hpa, with this it does work fine, i.e.
> Mtftp4GetFileSize does succeed and buffer size is allocated accordingly.
> I would still request you to merge the change, as it will mean people
> using the default tftpd that comes with ubuntu distribution

You mean the one:

http://packages.ubuntu.com/wily/tftpd
http://packages.ubuntu.com/xenial/tftpd

that is explicitly advertised in Debian (Ubuntu's parent distro):

https://packages.debian.org/jessie/tftpd

and in Ubuntu as well:

http://archive.ubuntu.com/ubuntu/pool/universe/n/netkit-tftp/netkit-tftp_0.17-18ubuntu2.diff.gz

as unsuitable for PXE booting?

See also

http://thread.gmane.org/gmane.comp.bios.edk2.devel/2515/focus=2612
http://thread.gmane.org/gmane.comp.bios.edk2.devel/2515/focus=2638

> will also
> be able to use the tftp boot for ARM platforms.

Since this patch intends to modify the ARM BDS, I don't really have a
horse in the race, but please be aware that this is just a band-aid.
Debian and Ubuntu users should use the right tftp server (and/or file a
bug with their distro to change the default TFTP server).

I recommend to state the above in the commit message.

Thanks
Laszlo

> 
> -Original Message-
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> Sent: 05 November 2015 12:26
> To: Ashutosh Singh
> Cc: edk2-de...@ml01.01.org; ard.biesheu...@linaro.org; 
> ryan.har...@linaro.org; James King
> Subject: Re: [PATCH] Increase tftp buffer size
> 
> On Thu, Nov 05, 2015 at 11:37:43AM +, Ashutosh Singh wrote:
>> Default tftp buffer size (8MB) is too small for standard
>> ARM kernel images, which results in multiple aborted tftp fetches.
>> This fix increase the buffer size to 16MB.
> 
> So, I'm not a fan of hard-coded buffer sizes, or how this existing bit
> of code duplicates functionality already available elsewhere in the
> tree ... but I also don't care much about ArmBds.
> 
> What I'm curious about is - why is your Mtftp4GetFileSize call failing
> such that you need to use a hardcoded buffer size?
> Does your TFTP server not support rfc2349?
> Would changing that (for example by switching to tftpd-hpa over the
> unmaintained netkit-tftpd) not make more sense?
> 
> /
> Leif
> 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ashutosh Singh 
>> ---
>>  ArmPkg/Library/BdsLib/BdsFilePath.c |4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/ArmPkg/Library/BdsLib/BdsFilePath.c 
>> b/ArmPkg/Library/BdsLib/BdsFilePath.c
>> index ff42175..0410236 100644
>> --- a/ArmPkg/Library/BdsLib/BdsFilePath.c
>> +++ b/ArmPkg/Library/BdsLib/BdsFilePath.c
>> @@ -1198,7 +1198,7 @@ BdsTftpLoadImage (
>>if (Mtftp4GetFileSize (Mtftp4, AsciiFilePath, ) == EFI_SUCCESS) {
>>  TftpBufferSize = FileSize;
>>} else {
>> -TftpBufferSize = SIZE_8MB;
>> +TftpBufferSize = SIZE_16MB;
>>}
>>
>>TftpContext = AllocatePool (sizeof (BDS_TFTP_CONTEXT));
>> @@ -1209,7 +1209,7 @@ BdsTftpLoadImage (
>>TftpContext->FileSize = FileSize;
>>
>>for (; TftpBufferSize <= FixedPcdGet32 (PcdMaxTftpFileSize);
>> - TftpBufferSize = (TftpBufferSize + SIZE_8MB) & (~(SIZE_8MB-1))) {
>> + TftpBufferSize = (TftpBufferSize + SIZE_16MB) & (~(SIZE_16MB-1))) {
>>  //
>>  // Allocate a buffer to hold the whole file.
>>  //
>> --
>> 1.7.9.5
>>
>>
>> 
>>
>> -- 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.
>>
> 
> 
> 
> 
> -- 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
> 

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

Re: [edk2] [PATCH v2 09/10] ArmPkg/ArmLib: add accessor function for Cache Writeback Granularity

[Mark Rutland]

On Fri, Nov 06, 2015 at 03:03:57PM +0100, Ard Biesheuvel wrote:
> Add a function to ArmLib that provides access to the Cache Writeback
> Granularity (CWG) field in CTR_EL0. This information is required when
> performing non-coherent DMA.

Nit: s/Granularity/Granule/ 

Likewise for the patch body.

[...]

> +UINTN
> +EFIAPI
> +ArmCacheWritebackGranularity (
> +  VOID
> +  )
> +{
> +  UINTN   CWG;
> +
> +  CWG = (ArmCacheInfo () >> 24) & 0xf; // CTR_EL0.CWG
> +
> +  if (CWG == 0) {
> +return SIZE_512KB;
> +  }

This should be SIZE_2KB, no?

The ARM ARM says the architectural maximum is 512 /words/ (i.e. 2KB).

With those changes:

Reviewed-by: Mark Rutland 

Thanks,
Mark.

> +
> +  return 4 << CWG;
> +}
> -- 
> 1.9.1
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] I don't understand AllocatePages

[Shubha Ramani]

Here:http://wiki.phoenix.com/wiki/index.php/EFI_BOOT_SERVICES#AllocatePages.28.29

First of all, is there a concept of Virtual Memory in UEFI ? 
Why does EFI_MEMORY_DESCRIPTOR have VirtualStart ? What is it used for ?
typedef struct {
  UINT32   Type;
  EFI_PHYSICAL_ADDRESS PhysicalStart;
  EFI_VIRTUAL_ADDRESS  VirtualStart;
  UINT64   NumberOfPages;
  UINT64   Attribute;
} EFI_MEMORY_DESCRIPTOR; Finally, if there is no virtual memory, then why is 
AllocatePages even required ?  If all you care about is reading/writing to pure 
physical memory addresses, then do you need to call AllocatePages ?
Thanks,
Shubha
Shubha D. ramanishubharam...@gmail.com
shubharam...@yahoo.com
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 1/4] AppPkg/Python-2.7.10: ReadMe and .inf files

[Bjorge, Erik C]

Reviewed-by: Erik Bjorge 

-Original Message-
From: Daryl McDaniel [mailto:edk2-li...@mc2research.org] 
Sent: Thursday, November 5, 2015 2:31 PM
To: edk2-devel@lists.01.org; Carsey, Jaben ; Bjorge, 
Erik C 
Subject: [Patch 1/4] AppPkg/Python-2.7.10: ReadMe and .inf files

AppPkg/Python-2.7.10: Patch 1 of 4 -- ReadMe and .inf files

Files Py2710ReadMe.txt and Python2710.inf are included here in their
entirety,
along with their diffs.  The whole files are located between the "
BEGIN" and
" END" lines with the diffs following.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Daryl McDaniel 

 BEGIN CUT HERE -
EDK II Python
   ReadMe
Version 2.7.10
 Release 1.00
  3 Nov. 2015


1. OVERVIEW
===
This document is devoted to general information on building and setup of the
Python environment for UEFI, the invocation of the interpreter, and things
that make working with Python easier.

It is assumed that you already have UDK2010 or later, or a current snapshot
of
the EDK II sources from www.tianocore.org, and that you can successfully
build
packages within that distribution.

2. Release Notes

  1)  All C extension modules must be statically linked (built in)
  2)  The site and os modules must exist as discrete files in
...\lib\python27.10
  3)  User-specific configurations are not supported.
  4)  Environment variables are not supported.

3. Getting and Building Python
==
  3.1 Getting Python
  ==
  This file describes the UEFI port of version 2.7.10 of the CPython
distribution.
  For development ease, a subset of the Python 2.7.10 distribution has been
  included as part of the AppPkg/Applications/Python/Python-2.7.10 source
tree.
  If this is sufficient, you may skip to section 3.2, Building Python.

  If a full distribution is desired, it can be merged into the Python-2.7.10
  source tree.  Directory AppPkg/Applications/Python/Python-2.7.10
corresponds
  to the root directory of the CPython 2.7.10 distribution.  The full
  CPython 2.7.10 source code may be downloaded from
  http://www.python.org/ftp/python/2.7.10/.

  A.  Within your EDK II development tree, extract the Python distribution
into
AppPkg/Applications/Python/Python-2.7.10.  This should merge the
additional
files into the source tree.  It will also create the following
directories:
Demo  Doc Grammar Mac   Misc
PCPCbuild RISCOS  Tools

The greatest change will be within the Python-2.7.10/Lib directory where
many more packages and modules will be added.  These additional
components
may not have been ported to EDK II yet.

  3.2 Building Python
  ===
  B.  From the AppPkg/Applications/Python/Python-2.7.10 directory, execute
the
srcprep.bat (srcprep.sh) script to copy the header files from within the
PyMod-2.7.10 sub-tree into their corresponding directories within the
distribution.  This step only needs to be performed prior to the first
build of Python, or if one of the header files within the PyMod tree has
been
modified.

  A.  Edit PyMod-2.7.10\Modules\config.c to enable the built-in modules you
need.  By default,
it is configured for the minimally required set of modules.
Mandatory Built-in Modules:
edk2  errno   imp marshal

  Additional built-in modules which are required to use the help()
  functionality provided by PyDoc, are:
_codecs _collections_functools_random
_sre_struct _weakref  binascii
cStringIO   gc  itertools math
operatortime

  B.  Edit AppPkg/AppPkg.dsc to enable (uncomment) the Python2710.inf line
within the [Components] section.

  C.  Build AppPkg using the standard "build" command:
For example, to build Python for an X64 CPU architecture:
build -a X64 -p AppPkg\AppPkg.dsc

4. Python-related paths and files
=
Python depends upon the existence of several directories and files on the
target system.

  \EFI  Root of the UEFI system area.
   |- \ToolsLocation of the Python.efi executable.
   |- \Boot UEFI specified Boot directory.
   |- \StdLib   Root of the Standard Libraries sub-tree.
   |- \etc  Configuration files used by libraries.
   |- \tmp  Temporary files created by tmpfile(),
etc.
   |- \lib  Root of the libraries tree.
 

Re: [edk2] [PATCH] ArmPkg/BdsLib: Increase fallback tftp buffer size

[Laszlo Ersek]

On 11/06/15 18:01, Ashutosh Singh wrote:
> When performing a tftp download from a server which does not support
> rfc2349 transfer size option (such as netkit-tftpd), the existing code
> falls back to allocating an 8MB buffer. Since this is insufficient for
> an uncompressed AArch64 Linux kernel image, double the size to 16MB.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ashutosh Singh 
> ---
>  ArmPkg/Library/BdsLib/BdsFilePath.c |4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

The commit message looks good to me, thanks.
Laszlo

> diff --git a/ArmPkg/Library/BdsLib/BdsFilePath.c 
> b/ArmPkg/Library/BdsLib/BdsFilePath.c
> index ff42175..0410236 100644
> --- a/ArmPkg/Library/BdsLib/BdsFilePath.c
> +++ b/ArmPkg/Library/BdsLib/BdsFilePath.c
> @@ -1198,7 +1198,7 @@ BdsTftpLoadImage (
>if (Mtftp4GetFileSize (Mtftp4, AsciiFilePath, ) == EFI_SUCCESS) {
>  TftpBufferSize = FileSize;
>} else {
> -TftpBufferSize = SIZE_8MB;
> +TftpBufferSize = SIZE_16MB;
>}
> 
>TftpContext = AllocatePool (sizeof (BDS_TFTP_CONTEXT));
> @@ -1209,7 +1209,7 @@ BdsTftpLoadImage (
>TftpContext->FileSize = FileSize;
> 
>for (; TftpBufferSize <= FixedPcdGet32 (PcdMaxTftpFileSize);
> - TftpBufferSize = (TftpBufferSize + SIZE_8MB) & (~(SIZE_8MB-1))) {
> + TftpBufferSize = (TftpBufferSize + SIZE_16MB) & (~(SIZE_16MB-1))) {
>  //
>  // Allocate a buffer to hold the whole file.
>  //
> --
> 1.7.9.5
> 
> 
> 
> 
> -- 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

[edk2] Authentication status for signed FVs extracted in PEI

[Cohen, Eugene]

I'm confused about something and hope I can help some help understanding this.

If we have a signed FV that is extracted in PEI it doesn't look like the 
AuthenticationStatus gets propagated to DXE.

The hob doesn't store authentication status and the core products FVB with 
AuthenticationStatus forced to zero, even though the FV was signed and verified.

This seems to mess up policy code we want to have in DXE because it is not 
accurate.

MdeModulePkg\Core\Dxe\FwVolBlock\FwVolBlock.c, FwVolBlockDriverInit:

  while ((FvHob.Raw = GetNextHob (EFI_HOB_TYPE_FV, FvHob.Raw)) != NULL) {
//
// Produce an FVB protocol for it
//
ProduceFVBProtocolOnBuffer (FvHob.FirmwareVolume->BaseAddress, 
FvHob.FirmwareVolume->Length, NULL, 0, NULL);
FvHob.Raw = GET_NEXT_HOB (FvHob);
  }

Is this expected?  How would DXE policy code know if the FV was verified in PEI?

Thanks,

Eugene

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

Re: [edk2] Strange behavior of the DS-5 debugger on AARCH64 with step-by-step debugging in uefi

[Vladimir Olovyannikov]

Hello Ard, Eugene,
Thank you for explanation.

Ard, I tried the patch, but it cannot be applied to the latest (pulled a minute 
ago, git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@18732 
6f19259b-4bc3-4df7-8a09-765794883524)
tree: all 3 hunks failed. Which commit should I be based on to apply the patch?

Anyway I found the lines manually and changed them. However, when I try to 

source /uefi/ArmPlatformPkg/Scripts/Ds5/cmd_load_symbols.py -f 
(0x8500,0x0028) -m (0x8000,0x4000) -a
I am getting

ERROR(?): ValueError: need more than 1 value to unpack
  File " /uefi/ArmPlatformPkg/Scripts/Ds5/cmd_load_symbols.py", line 94, in 

armplatform_debugger.load_all_symbols()
ERROR(CMD656): 
# in /uefi/BroadcomPlatformPkg/NS2Pkg/Scripts/armpkg_syms.ds:2 while executing: 
source /uefi/ArmPlatformPkg/Scripts/Ds5/cmd_load_symbols.py -f 
(0x8500,0x0028) -m (0x8000,0x4000) -a
! The script /uefi/ArmPlatformPkg/Scripts/Ds5/cmd_load_symbols.py failed to 
complete due to an error during execution of the script

Replacing the script with the older version makes it work as before, but again 
the debugger is useless.

Any idea what is wrong?

Eugene, so you do not use the so convenient cmd_load_symbols.py script to load 
all symbols? 
I dumped efi and dll as you suggested. The .text and .data sections in PE-COFF 
and ELF match 1:1 for me. Still the debugger is useless as it points to 
non-relevant code.

Please help me to get back on track with debugger...

Thank you,
Vladimir

-Original Message-
From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] 
Sent: Friday, November 06, 2015 5:36 AM
To: Cohen, Eugene
Cc: Vladimir Olovyannikov; edk2-devel@lists.01.org
Subject: Re: [edk2] Strange behavior of the DS-5 debugger on AARCH64 with 
step-by-step debugging in uefi

On 6 November 2015 at 14:27, Cohen, Eugene  wrote:
> Vladimir,
>
> Since the UEFI images are PE-COFF but DS-5 builds ELF there is a conversion 
> process in the edk2 build.  This can result in a couple of issues with debug 
> if not handled correctly:
>
> 1. The start of the image for debugger load purposes must be adjusted to 
> reflect the PE/TE header in the image.  If this isn't done correctly the code 
> and data will be shifted and the debugger is useless.
>
> 2. The relative position of .text and .data must be consistent across ELF and 
> PE-COFF.  If this isn't done correctly you may be able to get correlated 
> source code but all references to data (like dumping global variables) are 
> messed up.
>
> The way I go about debugging this is to dump the PE-COFF image (.efi) with 
> dumpbin (from Visual Studio) and dump the ELF image (.dll) with fromelf (from 
> RVCT) and compare the positions of the .text and .data sections.
>
> Based on Ard's response I think you are hitting issue #1.
>

Indeed. I made some changes to the GCC linker scripts recently to
ensure that the PE/COFF and ELF layouts are identical in memory. The
primary reason for doing so was the recent introduction of a security
feature in UEFI that requires the PE/COFF sections to be page aligned,
making it more likely that you would hit #2 in Eugene's list above.

-- 
Ard.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> Vladimir Olovyannikov
> Sent: Thursday, November 05, 2015 3:52 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] Strange behavior of the DS-5 debugger on AARCH64 with 
> step-by-step debugging in uefi
>
> Hello All,
>
> I faced a very strange behavior of the DS-5 debugger when I debug AARCH64  
> UEFI with the latest (well, the one which contains
> DEFINE GCC_DLINK2_FLAGS_COMMON = 
> --script=$(EDK_TOOLS_PATH)/Scripts/GccBase.lds
> )
> tools_def.template.
> Whenever I step in the debugger it never matches the source where the 
> execution point currently is.
> It is impossible to step-by-step debug with this...
> If I switch to the older tools_def.template, I don't have those issues, and 
> can debug with no problem.
> However, the ShellPkg cannot be built with older tools_def.template if I want 
> to have ShellPkg (New Shell) instead of ShellBinPkg.
>
> Please advise if I am doing anything wrong?
>
> This is how I debug if step-by-step debug is needed:
>
> I place a while(1) in a place of interest. Build UEFI, and then reboot the 
> board and run the uefi; then when it reaches the point of while1, I connect 
> DS-5 to the device and execute ArmPlatformPkg/Scripts/Ds5/cmd_load_symbols.py 
> -f (0x8500,0x0028) -m (0x8000,0x4000) -a
>
> And expect to be at the (while1 line). With the latest tools_def.tempalte the 
> source point is weird; with the previous ones - OK.
> Any advice would be appreciated.
>
> Thank you,
> Vladimir
>
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> ___
> edk2-devel 

[edk2] [PATCH v2 09/10] ArmPkg/ArmLib: add accessor function for Cache Writeback Granularity

[Ard Biesheuvel]

Add a function to ArmLib that provides access to the Cache Writeback
Granularity (CWG) field in CTR_EL0. This information is required when
performing non-coherent DMA.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
---
 ArmPkg/Include/Library/ArmLib.h   |  6 ++
 ArmPkg/Library/ArmLib/Common/ArmLib.c | 17 +
 2 files changed, 23 insertions(+)

diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
index 2059a67bbf3c..6a52006b504e 100644
--- a/ArmPkg/Include/Library/ArmLib.h
+++ b/ArmPkg/Include/Library/ArmLib.h
@@ -116,6 +116,12 @@ ArmInstructionCacheLineLength (
 
 UINTN
 EFIAPI
+ArmCacheWritebackGranularity (
+  VOID
+  );
+
+UINTN
+EFIAPI
 ArmIsArchTimerImplemented (
   VOID
   );
diff --git a/ArmPkg/Library/ArmLib/Common/ArmLib.c 
b/ArmPkg/Library/ArmLib/Common/ArmLib.c
index ad0a265e9f59..0de323a30ff1 100644
--- a/ArmPkg/Library/ArmLib/Common/ArmLib.c
+++ b/ArmPkg/Library/ArmLib/Common/ArmLib.c
@@ -88,3 +88,20 @@ ArmInstructionCacheLineLength (
 {
   return 4 << (ArmCacheInfo () & 0xf); // CTR_EL0.IminLine
 }
+
+UINTN
+EFIAPI
+ArmCacheWritebackGranularity (
+  VOID
+  )
+{
+  UINTN   CWG;
+
+  CWG = (ArmCacheInfo () >> 24) & 0xf; // CTR_EL0.CWG
+
+  if (CWG == 0) {
+return SIZE_512KB;
+  }
+
+  return 4 << CWG;
+}
-- 
1.9.1

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

[edk2] [PATCH v2 06/10] ArmPkg/ArmLib: move cache maintenance sync barriers out of loop

[Ard Biesheuvel]

There is no need to issue a full data synchronization barrier and an
instruction synchronization barrier after each and every set/way or
MVA cache maintenance operation. For the set/way case, we can simply
remove them, since the set/way outer loop already issues the required
barriers after completing its traversal over all the cache levels.

For the MVA case, move the data synchronization barrier out of the
loop, and add the instruction synchronization barrier to the I-cache
invalidation by MVA routine.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
Reviewed-by: Mark Rutland 
---
 ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c |  1 +
 ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 12 

 ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S | 12 

 ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm   | 12 

 4 files changed, 1 insertion(+), 36 deletions(-)

diff --git a/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c 
b/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c
index d8e53df6096e..d4c16a607465 100644
--- a/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c
+++ b/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c
@@ -35,6 +35,7 @@ CacheRangeOperation (
 LineOperation(AlignedAddress);
 AlignedAddress += ArmCacheLineLength;
   }
+  ArmDataSynchronizationBarrier ();
 }
 
 VOID
diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S 
b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
index f973a35c21d6..df2dc935c122 100644
--- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
+++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
@@ -65,43 +65,31 @@ GCC_ASM_EXPORT (ArmReadCurrentEL)
 
 ASM_PFX(ArmInvalidateDataCacheEntryByMVA):
   dc  ivac, x0// Invalidate single data cache line
-  dsb sy
-  isb
   ret
 
 
 ASM_PFX(ArmCleanDataCacheEntryByMVA):
   dc  cvac, x0// Clean single data cache line
-  dsb sy
-  isb
   ret
 
 
 ASM_PFX(ArmCleanInvalidateDataCacheEntryByMVA):
   dc  civac, x0   // Clean and invalidate single data cache line
-  dsb sy
-  isb
   ret
 
 
 ASM_PFX(ArmInvalidateDataCacheEntryBySetWay):
   dc  isw, x0 // Invalidate this line
-  dsb sy
-  isb
   ret
 
 
 ASM_PFX(ArmCleanInvalidateDataCacheEntryBySetWay):
   dc  cisw, x0// Clean and Invalidate this line
-  dsb sy
-  isb
   ret
 
 
 ASM_PFX(ArmCleanDataCacheEntryBySetWay):
   dc  csw, x0 // Clean this line
-  dsb sy
-  isb
   ret
 
 
diff --git a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S 
b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S
index fdc4d03776c8..93164b8f0ea4 100644
--- a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S
+++ b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S
@@ -62,42 +62,30 @@ GCC_ASM_EXPORT (ArmReadIdPfr1)
 
 ASM_PFX(ArmInvalidateDataCacheEntryByMVA):
   mcr p15, 0, r0, c7, c6, 1   @invalidate single data cache line
-  dsb
-  isb
   bx  lr
 
 ASM_PFX(ArmCleanDataCacheEntryByMVA):
   mcr p15, 0, r0, c7, c10, 1  @clean single data cache line
-  dsb
-  isb
   bx  lr
 
 
 ASM_PFX(ArmCleanInvalidateDataCacheEntryByMVA):
   mcr p15, 0, r0, c7, c14, 1  @clean and invalidate single data cache line
-  dsb
-  isb
   bx  lr
 
 
 ASM_PFX(ArmInvalidateDataCacheEntryBySetWay):
   mcr p15, 0, r0, c7, c6, 2@ Invalidate this line
-  dsb
-  isb
   bx  lr
 
 
 ASM_PFX(ArmCleanInvalidateDataCacheEntryBySetWay):
   mcr p15, 0, r0, c7, c14, 2   @ Clean and Invalidate this line
-  dsb
-  isb
   bx  lr
 
 
 ASM_PFX(ArmCleanDataCacheEntryBySetWay):
   mcr p15, 0, r0, c7, c10, 2   @ Clean this line
-  dsb
-  isb
   bx  lr
 
 ASM_PFX(ArmInvalidateInstructionCache):
diff --git a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm 
b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm
index f16dd4a4ab01..d6f249038a05 100644
--- a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm
+++ b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm
@@ -62,42 +62,30 @@ CTRL_I_BIT  EQU (1 << 12)
 
 ArmInvalidateDataCacheEntryByMVA
   mcr p15, 0, r0, c7, c6, 1   ; invalidate single data cache line
-  dsb
-  isb
   bx  lr
 
 ArmCleanDataCacheEntryByMVA
   mcr p15, 0, r0, c7, c10, 1  ; clean single data cache line
-  dsb
-  isb
   bx  lr
 
 
 ArmCleanInvalidateDataCacheEntryByMVA
   mcr p15, 0, r0, c7, c14, 1  ; clean and invalidate single data cache line
-  dsb
-  isb
   bx  lr
 
 
 ArmInvalidateDataCacheEntryBySetWay
   mcr p15, 0, r0, c7, c6, 2; Invalidate this line
-  dsb
-  isb
   bx  lr
 
 
 ArmCleanInvalidateDataCacheEntryBySetWay
   mcr p15, 0, r0, c7, c14, 2   ; Clean and Invalidate this line
-  dsb
-  isb
   bx  lr
 
 
 ArmCleanDataCacheEntryBySetWay
   mcr p15, 0, r0, c7, c10, 2   ; Clean this line
-  dsb
-  isb
   bx  lr
 
 
-- 
1.9.1

[edk2] [PATCH v2 10/10] ArmPkg/ArmDmaLib: use the cache writeback granularity for alignment

[Ard Biesheuvel]

When allocating memory to perform non-coherent DMA, use the cache
writeback granularity rather than the data cache linesize for
alignment. This prevents the explicit cache management from corrupting
unrelated adjacent data if the cache writeback granularity exceeds
the cache linesize.

Reported-by: Mark Rutland 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
---
 ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c 
b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
index 12b194046ae7..7e352e665a30 100755
--- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
+++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
@@ -277,7 +277,7 @@ ArmDmaLibConstructor (
   Status = gBS->LocateProtocol (, NULL, (VOID 
**));
   ASSERT_EFI_ERROR(Status);
 
-  gCacheAlignment = ArmDataCacheLineLength ();
+  gCacheAlignment = ArmCacheWritebackGranularity ();
 
   return Status;
 }
-- 
1.9.1

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

[edk2] [PATCH v2 08/10] ArmVirtPkg/PrePi: do not invalidate the entire data cache at startup

[Ard Biesheuvel]

Drop the call to ArmInvalidateDataCache () from the PrePi startup
sequence. This kind of data cache maintenance should not be performed
when running under virtualization.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
Acked-by: Laszlo Ersek 
Acked-by: Mark Rutland 
---
 ArmVirtPkg/PrePi/PrePi.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/ArmVirtPkg/PrePi/PrePi.c b/ArmVirtPkg/PrePi/PrePi.c
index f9ad37427217..fe7612cec74b 100755
--- a/ArmVirtPkg/PrePi/PrePi.c
+++ b/ArmVirtPkg/PrePi/PrePi.c
@@ -194,8 +194,6 @@ CEntryPoint (
 
   // Data Cache enabled on Primary core when MMU is enabled.
   ArmDisableDataCache ();
-  // Invalidate Data cache
-  ArmInvalidateDataCache ();
   // Invalidate instruction cache
   ArmInvalidateInstructionCache ();
   // Enable Instruction Caches on all cores.
-- 
1.9.1

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

[edk2] [PATCH v2 00/10] ARM cache maintenance cleanup

[Ard Biesheuvel]

This is a followup to the v1 I sent out last Tuesday, to address problems with
the cache maintenance practices in ArmPkg.

Changes:
- Dropped the patch that removes the ARM9 version of ArmLib. There's no rush
doing that, and people may still want us to hold on to it. Also updated patch #2
to fix the same type in the ARM9 files.
- Dropped the patch that removes ArmInvalidateDataCache() calls from platforms
under ArmPlatformPkg, since it is not harmful and we have no visibility on cases
where removing it may cause regressions.
- Added patches to use the correct granularity for non-coherent DMA, as reported
by Mark Rutland.
- Dropped redundant ArmInvalidateInstructionCache() added in patch #6
- Added acks from Mark Rutland and Laszlo Ersek

Patch #1 is Mark Rutland's patch to fix the barriers used in the AArch64
version of ArmEnableMmu ()

Patch #2 fixes a typo 'ArmDataSyncronizationBarrier' across all the
packages where it occurred.

Patch #3 removes the 'clean to PoU by set/way' routines.

Patch #4 removes the ARM_CACHE_INFO routines that mostly infer cache geometry
from CCSIDR which is explicitly forbidden by the architecture. Since there are
no users anyway, I just removed all of it.

Patch #5 fixes another illegal use of CCSIDR: cache maintenance by virtual
address needs to use the stride reported in CTR not CCSIDR

Patch #6 moves the sync barriers out of the loop for MVA cache maintenance, and
removes them entirely from set/way maintenance since in that case, the outer
loop already contains sync barriers as well.

Patch #7 updates the ArmCacheMaintenanceLib whole-cache maintenance routines to
ASSERT() rather than silently invoke the data cache maintenance by set/way
routines which we know are not appropriate at runtime.

Patch #8 removes the use of ArmInvalidateDataCache() at startup time from the
ArmVirtPkg version of PrePi, which is only intended to run under virtualization,
where this does more harm than good.

Patch #9 adds an accessor function to the CTR_EL0.CWG field, which contains the
maximum data cache linesize.

Patch #10 wires up the CTR_EL0.CWG into the ArmDmaLib function. This fixes a
potential problem when the cache writeback granule exceeds the minimum cache
linesize.

Ard Biesheuvel (9):
  ArmPkg BeagleBoardPkg Omap35xxPkg: fix typo
'ArmDataSyncronizationBarrier'
  ArmPkg/ArmLib: remove unused ArmCleanDataCacheToPoU()
  ArmPkg/ArmLib: remove CCSIDR based cache info routines
  ArmPkg/ArmLib: retrieve cache line length from CTR not CCSIDR
  ArmPkg/ArmLib: move cache maintenance sync barriers out of loop
  ArmCacheMaintenanceLib: disallow whole D-cache maintenance operations
  ArmVirtPkg/PrePi: do not invalidate the entire data cache at startup
  ArmPkg/ArmLib: add accessor function for Cache Writeback Granularity
  ArmPkg/ArmDmaLib: use the cache writeback granularity for alignment

Mark Rutland (1):
  ArmPkg/ArmLib: fix barriers in AArch64 ArmEnableMmu

 ArmPkg/Include/Library/ArmLib.h|  84 +
 ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c |   9 +-
 ArmPkg/Library/ArmDmaLib/ArmDmaLib.c   |   2 +-
 ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c | 196 
---
 ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h |   6 -
 ArmPkg/Library/ArmLib/AArch64/AArch64Support.S |  36 +---
 ArmPkg/Library/ArmLib/Arm9/Arm9Support.S   |   4 +-
 ArmPkg/Library/ArmLib/Arm9/Arm9Support.asm |   4 +-
 ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.c | 199 

 ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.h |   6 -
 ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S |  66 +--
 ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm   |  66 +--
 ArmPkg/Library/ArmLib/Common/ArmLib.c  |  55 --
 ArmPkg/Library/ArmLib/Null/NullArmCacheInformation.c   | 106 
---
 ArmPkg/Library/ArmLib/Null/NullArmLib.inf  |   1 -
 ArmVirtPkg/PrePi/PrePi.c   |   2 -
 BeagleBoardPkg/Library/BeagleBoardLib/BeagleBoard.c|   4 +-
 Omap35xxPkg/InterruptDxe/HardwareInterrupt.c   |   6 +-
 Omap35xxPkg/Library/DebugAgentTimerLib/DebugAgentTimerLib.c|   2 +-
 19 files changed, 64 insertions(+), 790 deletions(-)
 delete mode 100644 ArmPkg/Library/ArmLib/Null/NullArmCacheInformation.c

-- 
1.9.1

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

[edk2] [PATCH v2 07/10] ArmCacheMaintenanceLib: disallow whole D-cache maintenance operations

[Ard Biesheuvel]

The ARM architecture provides no reliable way to clean or invalidate
the entire data cache at runtime. The reason is that such maintenance
requires the use of set/way maintenance operations, which are suitable
only for the kind of maintenance that is carried out when the cache is
taken offline entirely.

So ASSERT () when any of the CacheMaintenanceLib whole data cache routines
are invoked rather than pretending we can do anything meaningful here.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
---
 ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c 
b/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c
index d4c16a607465..65ba8749e796 100644
--- a/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c
+++ b/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c
@@ -14,6 +14,7 @@
 **/
 #include 
 #include 
+#include 
 #include 
 
 VOID
@@ -44,7 +45,6 @@ InvalidateInstructionCache (
   VOID
   )
 {
-  ArmCleanDataCache();
   ArmInvalidateInstructionCache();
 }
 
@@ -54,7 +54,7 @@ InvalidateDataCache (
   VOID
   )
 {
-  ArmInvalidateDataCache();
+  ASSERT (FALSE);
 }
 
 VOID *
@@ -75,7 +75,7 @@ WriteBackInvalidateDataCache (
   VOID
   )
 {
-  ArmCleanInvalidateDataCache();
+  ASSERT (FALSE);
 }
 
 VOID *
@@ -95,7 +95,7 @@ WriteBackDataCache (
   VOID
   )
 {
-  ArmCleanDataCache();
+  ASSERT (FALSE);
 }
 
 VOID *
-- 
1.9.1

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

Re: [edk2] Strange behavior of the DS-5 debugger on AARCH64 with step-by-step debugging in uefi

[Ard Biesheuvel]

On 6 November 2015 at 14:27, Cohen, Eugene  wrote:
> Vladimir,
>
> Since the UEFI images are PE-COFF but DS-5 builds ELF there is a conversion 
> process in the edk2 build.  This can result in a couple of issues with debug 
> if not handled correctly:
>
> 1. The start of the image for debugger load purposes must be adjusted to 
> reflect the PE/TE header in the image.  If this isn't done correctly the code 
> and data will be shifted and the debugger is useless.
>
> 2. The relative position of .text and .data must be consistent across ELF and 
> PE-COFF.  If this isn't done correctly you may be able to get correlated 
> source code but all references to data (like dumping global variables) are 
> messed up.
>
> The way I go about debugging this is to dump the PE-COFF image (.efi) with 
> dumpbin (from Visual Studio) and dump the ELF image (.dll) with fromelf (from 
> RVCT) and compare the positions of the .text and .data sections.
>
> Based on Ard's response I think you are hitting issue #1.
>

Indeed. I made some changes to the GCC linker scripts recently to
ensure that the PE/COFF and ELF layouts are identical in memory. The
primary reason for doing so was the recent introduction of a security
feature in UEFI that requires the PE/COFF sections to be page aligned,
making it more likely that you would hit #2 in Eugene's list above.

-- 
Ard.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> Vladimir Olovyannikov
> Sent: Thursday, November 05, 2015 3:52 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] Strange behavior of the DS-5 debugger on AARCH64 with 
> step-by-step debugging in uefi
>
> Hello All,
>
> I faced a very strange behavior of the DS-5 debugger when I debug AARCH64  
> UEFI with the latest (well, the one which contains
> DEFINE GCC_DLINK2_FLAGS_COMMON = 
> --script=$(EDK_TOOLS_PATH)/Scripts/GccBase.lds
> )
> tools_def.template.
> Whenever I step in the debugger it never matches the source where the 
> execution point currently is.
> It is impossible to step-by-step debug with this...
> If I switch to the older tools_def.template, I don't have those issues, and 
> can debug with no problem.
> However, the ShellPkg cannot be built with older tools_def.template if I want 
> to have ShellPkg (New Shell) instead of ShellBinPkg.
>
> Please advise if I am doing anything wrong?
>
> This is how I debug if step-by-step debug is needed:
>
> I place a while(1) in a place of interest. Build UEFI, and then reboot the 
> board and run the uefi; then when it reaches the point of while1, I connect 
> DS-5 to the device and execute ArmPlatformPkg/Scripts/Ds5/cmd_load_symbols.py 
> -f (0x8500,0x0028) -m (0x8000,0x4000) -a
>
> And expect to be at the (while1 line). With the latest tools_def.tempalte the 
> source point is weird; with the previous ones - OK.
> Any advice would be appreciated.
>
> Thank you,
> Vladimir
>
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel