[edk2] [Patch] BaseTools: Fix two warning reported in the make phase.

[Yonghong Zhu]

when we make BaseTools, it report warnings about VfrError.cpp and VolInfo,
so this patch fix this warning.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Yonghong Zhu 
---
 Source/C/VfrCompile/VfrError.cpp |  2 +-
 Source/C/VolInfo/VolInfo.c   | 19 +--
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/Source/C/VfrCompile/VfrError.cpp b/Source/C/VfrCompile/VfrError.cpp
index 3be2bd8..3c506ec 100644
--- a/Source/C/VfrCompile/VfrError.cpp
+++ b/Source/C/VfrCompile/VfrError.cpp
@@ -278,11 +278,11 @@ CVfrErrorHandle::HandleWarning (
   }
 
   GetFileNameLineNum (LineNum, &FileName, &FileLine);
 
   if (mWarningAsError) {
-Error (FileName, FileLine, 0x2220, "warning treated as error", NULL);
+Error (FileName, FileLine, 0x2220, (CHAR8 *) "warning treated as error", 
NULL);
   }
 
   for (Index = 0; mVfrWarningHandleTable[Index].mWarningCode != 
VFR_WARNING_CODEUNDEFINED; Index++) {
 if (WarningCode == mVfrWarningHandleTable[Index].mWarningCode) {
   WarningMsg = mVfrWarningHandleTable[Index].mWarningMsg;
diff --git a/Source/C/VolInfo/VolInfo.c b/Source/C/VolInfo/VolInfo.c
index 7e79d75..87e78d4 100644
--- a/Source/C/VolInfo/VolInfo.c
+++ b/Source/C/VolInfo/VolInfo.c
@@ -1,9 +1,9 @@
 /** @file
 The tool dumps the contents of a firmware volume
 
-Copyright (c) 1999 - 2014, Intel Corporation. All rights reserved.
+Copyright (c) 1999 - 2015, Intel Corporation. All rights reserved.
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD 
License
 which accompanies this distribution.  The full text of the license may be 
found at
 http://opensource.org/licenses/bsd-license.php
 
@@ -15,10 +15,13 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include 
 #include 
 #include 
 #include 
 #include 
+#ifdef __GNUC__
+#include 
+#endif
 
 #include 
 #include 
 #include 
 #include 
@@ -1420,13 +1423,25 @@ Returns:
   mParsedGuidedSectionTools,
   EfiGuid
   );
 
   if (ExtractionTool != NULL) {
-
+   #ifndef __GNUC__
 ToolInputFile = CloneString (tmpnam (NULL));
 ToolOutputFile = CloneString (tmpnam (NULL));
+   #else
+char tmp1[] = "/tmp/fileXX";
+char tmp2[] = "/tmp/fileXX";
+int fd1;
+int fd2;
+fd1 = mkstemp(tmp1);
+fd2 = mkstemp(tmp2);
+ToolInputFile = CloneString(tmp1);
+ToolOutputFile = CloneString(tmp2);
+close(fd1);
+close(fd2);
+   #endif
 
 //
 // Construction 'system' command string
 //
 SystemCommandFormatString = "%s -d -o %s %s";
-- 
2.6.1.windows.1

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

[edk2] [Patch] BaseTools/toolsetup.bat: fixed the error when the path contains space

[Yonghong Zhu]

when the path contains space, it will report error for PATH Environment
update.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Yonghong Zhu 
---
 toolsetup.bat | 21 -
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/toolsetup.bat b/toolsetup.bat
index 59874c5..310ddd0 100755
--- a/toolsetup.bat
+++ b/toolsetup.bat
@@ -320,17 +320,28 @@ goto end
 if not defined PYTHON_FREEZER_PATH (
   echo.
   echo !!! WARNING !!! Will not be able to compile Python programs to .exe
   echo Will setup environment to run Python scripts directly.
   echo.
-  set PATH=%BASETOOLS_PYTHON_SOURCE%\Trim;%PATH%
-  set PATH=%BASETOOLS_PYTHON_SOURCE%\GenFds;%PATH%
-  set PATH=%BASETOOLS_PYTHON_SOURCE%\build;%PATH%
-  set PATHEXT=%PATHEXT%;.py
+  goto UpdatePATH
 )
+else (
+  goto UpdateEnv
+)
+  )
+  else (
+goto UpdateEnv
   )
-  
+ 
+:UpdatePATH
+  set PATH=%BASETOOLS_PYTHON_SOURCE%\Trim;%PATH%
+  set PATH=%BASETOOLS_PYTHON_SOURCE%\GenFds;%PATH%
+  set PATH=%BASETOOLS_PYTHON_SOURCE%\build;%PATH%
+  set PATHEXT=%PATHEXT%;.py
+  goto UpdateEnv
+ 
+:UpdateEnv
   echo BASE_TOOLS_PATH = %BASE_TOOLS_PATH%
   echo PYTHON_PATH = %PYTHON_PATH%
   echo PYTHON_FREEZER_PATH = %PYTHON_FREEZER_PATH%
   echo.
 
-- 
2.6.1.windows.1

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

Re: [edk2] EFI GOP with manual vsync trigger

[Andrew Fish]

> On Nov 11, 2015, at 11:51 PM, Michael Zimmermann  
> wrote:
> 
> I've started investigating in the timer event problem and I think I have
> some weird problem with my platform drivers(I hope, so it's not a EDK2 bug).
> 
> If I create a timer that runs every 100ms which does nothing but a
> stall(1), the thread mode code stops after some random time(usually in edk2
> shell so I guess it's a race condition which needs some cpu load).
> 

Michael,

Watch out as Stall() is Microseconds, and SetTimer() is 100ns I've seen bugs 
like that before in code. 

> When threadmode code is stopped the timer continues getting called and even
> if I stop the timer afterwards(with CloseEvent) it keeps being stopped.
> 
> Is there a way to get the threadmode context from inside a timer callback?
> This way I could read the PC to check what's going on.
> 

There are no threads. EFI is an event model. If your code is running you are 
blocking every one else's forward progress. Only code running at a higher TPL 
can preempt. So when your event is running it is blocking the main flow and any 
event trying to run at <= TPL of your event from making forward progress. 
gBS->Stall() does not yield, it is no different than running code. 

So there is only one context, you can print it out any time you want. 

Thanks,

Andrew Fish

PS When folks yell at us for not having threads in EFI we point them at: 
https://web.stanford.edu/~ouster/cgi-bin/papers/threads.pdf


> Michael
> 
> On Wed, Nov 11, 2015 at 8:10 PM, Kinney, Michael D <
> michael.d.kin...@intel.com> wrote:
> 
>> Michael,
>> 
>> A periodic event timer at 30 times a second should not cause pauses
>> forever, unless the action you are performing in the event notification
>> function takes more than 1/30 of a second to complete.  You should be able
>> to just add a periodic event handler that does nothing, so you can measure
>> what the overhead is.
>> 
>> Another option is to use the performance counter in the TimerLib each time
>> a Blt() is called (GetPerformanceCounterProperties() and
>> GetPerformanceCounter()).  When Blt() is called frequently, the amount of
>> time since last vsync will have elapsed, and you can go the vsync action
>> within the Blt() call.  If you also set a one shot timer event, so if the
>> last call to Blt() did not do a vsync and there are no more Blt() calls,
>> 1/30th of a second later, the vsync action can be done.  Every time Blt()
>> is called, the one shot timer can be re-armed.  This way, the one shot
>> timer event is not actually executed very often.
>> 
>> Mike
>> 
>>> -Original Message-
>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>> Michael Zimmermann
>>> Sent: Wednesday, November 11, 2015 12:32 AM
>>> To: edk2-devel@lists.01.org
>>> Subject: [edk2] EFI GOP with manual vsync trigger
>>> 
>>> Hi,
>>> 
>>> my Graphics HW uses a manual vsync trigger. That means that after drawing
>>> to the framebuffer I need to manually trigger vsync(you can compare it to
>>> switching between double buffers).
>>> 
>>> The problem is that UEFI's GraphicsOutputProtocol(GOP) doesn't take care
>> of
>>> HW that needs a flush.
>>> While issuing the trigger after every Blt Operation works, this obviously
>>> causes extremely slow rendering for applications like the Shell which call
>>> Blt very often(like for every character).
>>> 
>>> Also I can't use a timer to set the trigger(like 30times a second) because
>>> it takes too much time and the Timer Interrupt ends up consuming too much
>>> time and the "normal" code gets paused forever.
>>> 
>>> Do you have any other ideas how to handle this?
>>> 
>>> Thx
>>> Michael
>>> ___
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

Re: [edk2] EFI GOP with manual vsync trigger

[Michael Zimmermann]

Stall was just an example, I can also use DEBUG. My timer interval is
100ms(I converted it from the 100ns unit).

What I meant with "thread mode code" is the "normal" non IRQ context code
running on the CPU. That means that there are actually two contexts in
EDK2, the exception(i.e. IRQ's like the Timer/Watchdog) and the normal mode
all code is running in.

On Thu, Nov 12, 2015 at 9:27 AM, Andrew Fish  wrote:

>
> > On Nov 11, 2015, at 11:51 PM, Michael Zimmermann <
> sigmaepsilo...@gmail.com> wrote:
> >
> > I've started investigating in the timer event problem and I think I have
> > some weird problem with my platform drivers(I hope, so it's not a EDK2
> bug).
> >
> > If I create a timer that runs every 100ms which does nothing but a
> > stall(1), the thread mode code stops after some random time(usually in
> edk2
> > shell so I guess it's a race condition which needs some cpu load).
> >
>
> Michael,
>
> Watch out as Stall() is Microseconds, and SetTimer() is 100ns I've seen
> bugs like that before in code.
>
> > When threadmode code is stopped the timer continues getting called and
> even
> > if I stop the timer afterwards(with CloseEvent) it keeps being stopped.
> >
> > Is there a way to get the threadmode context from inside a timer
> callback?
> > This way I could read the PC to check what's going on.
> >
>
> There are no threads. EFI is an event model. If your code is running you
> are blocking every one else's forward progress. Only code running at a
> higher TPL can preempt. So when your event is running it is blocking the
> main flow and any event trying to run at <= TPL of your event from making
> forward progress. gBS->Stall() does not yield, it is no different than
> running code.
>
> So there is only one context, you can print it out any time you want.
>
> Thanks,
>
> Andrew Fish
>
> PS When folks yell at us for not having threads in EFI we point them at:
> https://web.stanford.edu/~ouster/cgi-bin/papers/threads.pdf
>
>
> > Michael
> >
> > On Wed, Nov 11, 2015 at 8:10 PM, Kinney, Michael D <
> > michael.d.kin...@intel.com> wrote:
> >
> >> Michael,
> >>
> >> A periodic event timer at 30 times a second should not cause pauses
> >> forever, unless the action you are performing in the event notification
> >> function takes more than 1/30 of a second to complete.  You should be
> able
> >> to just add a periodic event handler that does nothing, so you can
> measure
> >> what the overhead is.
> >>
> >> Another option is to use the performance counter in the TimerLib each
> time
> >> a Blt() is called (GetPerformanceCounterProperties() and
> >> GetPerformanceCounter()).  When Blt() is called frequently, the amount
> of
> >> time since last vsync will have elapsed, and you can go the vsync action
> >> within the Blt() call.  If you also set a one shot timer event, so if
> the
> >> last call to Blt() did not do a vsync and there are no more Blt() calls,
> >> 1/30th of a second later, the vsync action can be done.  Every time
> Blt()
> >> is called, the one shot timer can be re-armed.  This way, the one shot
> >> timer event is not actually executed very often.
> >>
> >> Mike
> >>
> >>> -Original Message-
> >>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> >> Michael Zimmermann
> >>> Sent: Wednesday, November 11, 2015 12:32 AM
> >>> To: edk2-devel@lists.01.org
> >>> Subject: [edk2] EFI GOP with manual vsync trigger
> >>>
> >>> Hi,
> >>>
> >>> my Graphics HW uses a manual vsync trigger. That means that after
> drawing
> >>> to the framebuffer I need to manually trigger vsync(you can compare it
> to
> >>> switching between double buffers).
> >>>
> >>> The problem is that UEFI's GraphicsOutputProtocol(GOP) doesn't take
> care
> >> of
> >>> HW that needs a flush.
> >>> While issuing the trigger after every Blt Operation works, this
> obviously
> >>> causes extremely slow rendering for applications like the Shell which
> call
> >>> Blt very often(like for every character).
> >>>
> >>> Also I can't use a timer to set the trigger(like 30times a second)
> because
> >>> it takes too much time and the Timer Interrupt ends up consuming too
> much
> >>> time and the "normal" code gets paused forever.
> >>>
> >>> Do you have any other ideas how to handle this?
> >>>
> >>> Thx
> >>> Michael
> >>> ___
> >>> edk2-devel mailing list
> >>> edk2-devel@lists.01.org
> >>> https://lists.01.org/mailman/listinfo/edk2-devel
> >>
> > ___
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
>
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] EFI GOP with manual vsync trigger

[Andrew Fish]

> On Nov 12, 2015, at 12:49 AM, Michael Zimmermann  
> wrote:
> 
> Stall was just an example, I can also use DEBUG. My timer interval is
> 100ms(I converted it from the 100ns unit).
> 
> What I meant with "thread mode code" is the "normal" non IRQ context code
> running on the CPU. That means that there are actually two contexts in
> EDK2, the exception(i.e. IRQ's like the Timer/Watchdog) and the normal mode
> all code is running in.
> 

OK thanks that helps. The timer ISR runs in interrupt context, and from an ARM 
point of view EFI is running the ARM "Thread mode".  The ISR context is an 
implementation detail and not really defined by the specification. 

I'd also point out the Events dispatch independent of the interrupt context. 
gBS->RestoreTpl() can cause events to dispatch. For example a lot of the EFI 
Protocol services have locks that raise TPL to prevent recursion. When these 
locks are released and the TPL is restored events are dispatched. So just 
calling EFI services can cause events to run. So conceptually you can ignore 
the interrupt context, as that is used to implement the timer tick. Every thing 
else is an event that runs in the main EFI context.  

https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Event/Tpl.c#L126

Thanks,

Andrew Fish


> On Thu, Nov 12, 2015 at 9:27 AM, Andrew Fish  wrote:
> 
>> 
>>> On Nov 11, 2015, at 11:51 PM, Michael Zimmermann <
>> sigmaepsilo...@gmail.com> wrote:
>>> 
>>> I've started investigating in the timer event problem and I think I have
>>> some weird problem with my platform drivers(I hope, so it's not a EDK2
>> bug).
>>> 
>>> If I create a timer that runs every 100ms which does nothing but a
>>> stall(1), the thread mode code stops after some random time(usually in
>> edk2
>>> shell so I guess it's a race condition which needs some cpu load).
>>> 
>> 
>> Michael,
>> 
>> Watch out as Stall() is Microseconds, and SetTimer() is 100ns I've seen
>> bugs like that before in code.
>> 
>>> When threadmode code is stopped the timer continues getting called and
>> even
>>> if I stop the timer afterwards(with CloseEvent) it keeps being stopped.
>>> 
>>> Is there a way to get the threadmode context from inside a timer
>> callback?
>>> This way I could read the PC to check what's going on.
>>> 
>> 
>> There are no threads. EFI is an event model. If your code is running you
>> are blocking every one else's forward progress. Only code running at a
>> higher TPL can preempt. So when your event is running it is blocking the
>> main flow and any event trying to run at <= TPL of your event from making
>> forward progress. gBS->Stall() does not yield, it is no different than
>> running code.
>> 
>> So there is only one context, you can print it out any time you want.
>> 
>> Thanks,
>> 
>> Andrew Fish
>> 
>> PS When folks yell at us for not having threads in EFI we point them at:
>> https://web.stanford.edu/~ouster/cgi-bin/papers/threads.pdf
>> 
>> 
>>> Michael
>>> 
>>> On Wed, Nov 11, 2015 at 8:10 PM, Kinney, Michael D <
>>> michael.d.kin...@intel.com> wrote:
>>> 
 Michael,
 
 A periodic event timer at 30 times a second should not cause pauses
 forever, unless the action you are performing in the event notification
 function takes more than 1/30 of a second to complete.  You should be
>> able
 to just add a periodic event handler that does nothing, so you can
>> measure
 what the overhead is.
 
 Another option is to use the performance counter in the TimerLib each
>> time
 a Blt() is called (GetPerformanceCounterProperties() and
 GetPerformanceCounter()).  When Blt() is called frequently, the amount
>> of
 time since last vsync will have elapsed, and you can go the vsync action
 within the Blt() call.  If you also set a one shot timer event, so if
>> the
 last call to Blt() did not do a vsync and there are no more Blt() calls,
 1/30th of a second later, the vsync action can be done.  Every time
>> Blt()
 is called, the one shot timer can be re-armed.  This way, the one shot
 timer event is not actually executed very often.
 
 Mike
 
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
 Michael Zimmermann
> Sent: Wednesday, November 11, 2015 12:32 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] EFI GOP with manual vsync trigger
> 
> Hi,
> 
> my Graphics HW uses a manual vsync trigger. That means that after
>> drawing
> to the framebuffer I need to manually trigger vsync(you can compare it
>> to
> switching between double buffers).
> 
> The problem is that UEFI's GraphicsOutputProtocol(GOP) doesn't take
>> care
 of
> HW that needs a flush.
> While issuing the trigger after every Blt Operation works, this
>> obviously
> causes extremely slow rendering for applications like the Shell which
>> call
> Blt very often(like for every character).
> 
>>>

Re: [edk2] EFI GOP with manual vsync trigger

[Andrew Fish]

> On Nov 12, 2015, at 3:22 AM, Andrew Fish  wrote:
> 
> 
>> On Nov 12, 2015, at 12:49 AM, Michael Zimmermann  
>> wrote:
>> 
>> Stall was just an example, I can also use DEBUG. My timer interval is
>> 100ms(I converted it from the 100ns unit).
>> 
>> What I meant with "thread mode code" is the "normal" non IRQ context code
>> running on the CPU. That means that there are actually two contexts in
>> EDK2, the exception(i.e. IRQ's like the Timer/Watchdog) and the normal mode
>> all code is running in.
>> 
> 
> OK thanks that helps. The timer ISR runs in interrupt context, and from an 
> ARM point of view EFI is running the ARM "Thread mode".  The ISR context is 
> an implementation detail and not really defined by the specification. 
> 
> I'd also point out the Events dispatch independent of the interrupt context. 
> gBS->RestoreTpl() can cause events to dispatch. For example a lot of the EFI 
> Protocol services have locks that raise TPL to prevent recursion. When these 
> locks are released and the TPL is restored events are dispatched. So just 
> calling EFI services can cause events to run. So conceptually you can ignore 
> the interrupt context, as that is used to implement the timer tick. Every 
> thing else is an event that runs in the main EFI context.  
> 
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Event/Tpl.c#L126
> 

As Kinney pointed out the events are cooperative and there is no scheduler, so 
if you are getting stuck some chunk of code is running too long at an elevated 
TPL. You may need to performance profile to figure out the bad code. 

Thanks,

Andrew Fish


> Thanks,
> 
> Andrew Fish
> 
> 
>> On Thu, Nov 12, 2015 at 9:27 AM, Andrew Fish  wrote:
>> 
>>> 
 On Nov 11, 2015, at 11:51 PM, Michael Zimmermann <
>>> sigmaepsilo...@gmail.com> wrote:
 
 I've started investigating in the timer event problem and I think I have
 some weird problem with my platform drivers(I hope, so it's not a EDK2
>>> bug).
 
 If I create a timer that runs every 100ms which does nothing but a
 stall(1), the thread mode code stops after some random time(usually in
>>> edk2
 shell so I guess it's a race condition which needs some cpu load).
 
>>> 
>>> Michael,
>>> 
>>> Watch out as Stall() is Microseconds, and SetTimer() is 100ns I've seen
>>> bugs like that before in code.
>>> 
 When threadmode code is stopped the timer continues getting called and
>>> even
 if I stop the timer afterwards(with CloseEvent) it keeps being stopped.
 
 Is there a way to get the threadmode context from inside a timer
>>> callback?
 This way I could read the PC to check what's going on.
 
>>> 
>>> There are no threads. EFI is an event model. If your code is running you
>>> are blocking every one else's forward progress. Only code running at a
>>> higher TPL can preempt. So when your event is running it is blocking the
>>> main flow and any event trying to run at <= TPL of your event from making
>>> forward progress. gBS->Stall() does not yield, it is no different than
>>> running code.
>>> 
>>> So there is only one context, you can print it out any time you want.
>>> 
>>> Thanks,
>>> 
>>> Andrew Fish
>>> 
>>> PS When folks yell at us for not having threads in EFI we point them at:
>>> https://web.stanford.edu/~ouster/cgi-bin/papers/threads.pdf
>>> 
>>> 
 Michael
 
 On Wed, Nov 11, 2015 at 8:10 PM, Kinney, Michael D <
 michael.d.kin...@intel.com> wrote:
 
> Michael,
> 
> A periodic event timer at 30 times a second should not cause pauses
> forever, unless the action you are performing in the event notification
> function takes more than 1/30 of a second to complete.  You should be
>>> able
> to just add a periodic event handler that does nothing, so you can
>>> measure
> what the overhead is.
> 
> Another option is to use the performance counter in the TimerLib each
>>> time
> a Blt() is called (GetPerformanceCounterProperties() and
> GetPerformanceCounter()).  When Blt() is called frequently, the amount
>>> of
> time since last vsync will have elapsed, and you can go the vsync action
> within the Blt() call.  If you also set a one shot timer event, so if
>>> the
> last call to Blt() did not do a vsync and there are no more Blt() calls,
> 1/30th of a second later, the vsync action can be done.  Every time
>>> Blt()
> is called, the one shot timer can be re-armed.  This way, the one shot
> timer event is not actually executed very often.
> 
> Mike
> 
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Michael Zimmermann
>> Sent: Wednesday, November 11, 2015 12:32 AM
>> To: edk2-devel@lists.01.org
>> Subject: [edk2] EFI GOP with manual vsync trigger
>> 
>> Hi,
>> 
>> my Graphics HW uses a manual vsync trigger. That means that after
>>> drawing
>> to the framebuff

Re: [edk2] [PATCH] ArmPkg/ArmLib: mark all cached mappings as (inner) shareable

[Leif Lindholm]

Hi Ard,

On Mon, Nov 09, 2015 at 02:18:58PM +0100, Ard Biesheuvel wrote:
> Mark all cached memory mappings as shareable (or inner shareable on
> AArch64) so that our view of memory is kept coherent by the hardware.
> 
> This is primarily relevant under virtualization (where a guest may migrate
> to another core) but in general, since UEFI on ARM is mostly used in a
> context where the secure firmware and possibly a secure OS are already up
> and running, it is best to refrain from using any non-shareable mappings.

Thanks, this is both an important correctness fix and nice code
cleanup.

I ran into an issue while testing this, which is why I haven't
responded to this, but I've bisected it down to r18503/3a05b13, and am
looking into what causes an issue with Linux booting.

Regardless - this patch wasn't the cause of my issue, and it looks
fine.

Reviewed-by: Leif Lindholm 

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmPkg/Include/Chipset/AArch64Mmu.h|  5 +
>  ArmPkg/Include/Chipset/ArmV7Mmu.h  |  8 
>  ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 21 ++--
>  3 files changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/ArmPkg/Include/Chipset/AArch64Mmu.h 
> b/ArmPkg/Include/Chipset/AArch64Mmu.h
> index 22e492d61dc0..3c3df6d9835c 100644
> --- a/ArmPkg/Include/Chipset/AArch64Mmu.h
> +++ b/ArmPkg/Include/Chipset/AArch64Mmu.h
> @@ -74,6 +74,11 @@
>  #define TT_NS   BIT5
>  #define TT_AF   BIT10
>  
> +#define TT_SH_NON_SHAREABLE (0x0 << 8)
> +#define TT_SH_OUTER_SHAREABLE   (0x2 << 8)
> +#define TT_SH_INNER_SHAREABLE   (0x3 << 8)
> +#define TT_SH_MASK  (0x3 << 8)
> +
>  #define TT_PXN_MASK BIT53
>  #define TT_UXN_MASK BIT54   // EL1&0
>  #define TT_XN_MASK  BIT54   // EL2 / EL3
> diff --git a/ArmPkg/Include/Chipset/ArmV7Mmu.h 
> b/ArmPkg/Include/Chipset/ArmV7Mmu.h
> index 24ab1755faa7..aaa0977205fa 100644
> --- a/ArmPkg/Include/Chipset/ArmV7Mmu.h
> +++ b/ArmPkg/Include/Chipset/ArmV7Mmu.h
> @@ -175,14 +175,14 @@
>  #define TT_DESCRIPTOR_SECTION_WRITE_BACK(NonSecure) 
> (TT_DESCRIPTOR_SECTION_TYPE_SECTION   
> | \
>  ((NonSecure) ?  
> TT_DESCRIPTOR_SECTION_NS : 0)| \
>  
> TT_DESCRIPTOR_SECTION_NG_GLOBAL | \
> -
> TT_DESCRIPTOR_SECTION_S_NOT_SHARED  | \
> +
> TT_DESCRIPTOR_SECTION_S_SHARED  | \
>  
> TT_DESCRIPTOR_SECTION_DOMAIN(0) | \
>  
> TT_DESCRIPTOR_SECTION_AP_RW_RW  | \
>  
> TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC)
>  #define TT_DESCRIPTOR_SECTION_WRITE_THROUGH(NonSecure)  
> (TT_DESCRIPTOR_SECTION_TYPE_SECTION   
> | \
>  ((NonSecure) ?  
> TT_DESCRIPTOR_SECTION_NS : 0)| \
>  
> TT_DESCRIPTOR_SECTION_NG_GLOBAL | \
> -
> TT_DESCRIPTOR_SECTION_S_NOT_SHARED  | \
> +
> TT_DESCRIPTOR_SECTION_S_SHARED  | \
>  
> TT_DESCRIPTOR_SECTION_DOMAIN(0) | \
>  
> TT_DESCRIPTOR_SECTION_AP_RW_RW  | \
>  
> TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC)
> @@ -203,12 +203,12 @@
>  
>  #define TT_DESCRIPTOR_PAGE_WRITE_BACK  
> (TT_DESCRIPTOR_PAGE_TYPE_PAGE 
>   | \
>  
> TT_DESCRIPTOR_PAGE_NG_GLOBAL  
> | \
> -
> TT_DESCRIPTOR_PAGE_S_NOT_SHARED   
> | \
> +
> TT_DESCRIPTOR_PAGE_S_SHARED   
> | \
>

Re: [edk2] [PATCH] ArmPkg/ArmLib: mark all cached mappings as (inner) shareable

[Ard Biesheuvel]

On 12 November 2015 at 12:35, Leif Lindholm  wrote:
> Hi Ard,
>
> On Mon, Nov 09, 2015 at 02:18:58PM +0100, Ard Biesheuvel wrote:
>> Mark all cached memory mappings as shareable (or inner shareable on
>> AArch64) so that our view of memory is kept coherent by the hardware.
>>
>> This is primarily relevant under virtualization (where a guest may migrate
>> to another core) but in general, since UEFI on ARM is mostly used in a
>> context where the secure firmware and possibly a secure OS are already up
>> and running, it is best to refrain from using any non-shareable mappings.
>
> Thanks, this is both an important correctness fix and nice code
> cleanup.
>
> I ran into an issue while testing this, which is why I haven't
> responded to this, but I've bisected it down to r18503/3a05b13, and am
> looking into what causes an issue with Linux booting.
>
> Regardless - this patch wasn't the cause of my issue, and it looks
> fine.
>
> Reviewed-by: Leif Lindholm 
>

Thanks. Committed as SVN r18778

-- 
Ard.


>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel 
>> ---
>>  ArmPkg/Include/Chipset/AArch64Mmu.h|  5 +
>>  ArmPkg/Include/Chipset/ArmV7Mmu.h  |  8 
>>  ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 21 ++--
>>  3 files changed, 19 insertions(+), 15 deletions(-)
>>
>> diff --git a/ArmPkg/Include/Chipset/AArch64Mmu.h 
>> b/ArmPkg/Include/Chipset/AArch64Mmu.h
>> index 22e492d61dc0..3c3df6d9835c 100644
>> --- a/ArmPkg/Include/Chipset/AArch64Mmu.h
>> +++ b/ArmPkg/Include/Chipset/AArch64Mmu.h
>> @@ -74,6 +74,11 @@
>>  #define TT_NS   BIT5
>>  #define TT_AF   BIT10
>>
>> +#define TT_SH_NON_SHAREABLE (0x0 << 8)
>> +#define TT_SH_OUTER_SHAREABLE   (0x2 << 8)
>> +#define TT_SH_INNER_SHAREABLE   (0x3 << 8)
>> +#define TT_SH_MASK  (0x3 << 8)
>> +
>>  #define TT_PXN_MASK BIT53
>>  #define TT_UXN_MASK BIT54   // EL1&0
>>  #define TT_XN_MASK  BIT54   // EL2 / EL3
>> diff --git a/ArmPkg/Include/Chipset/ArmV7Mmu.h 
>> b/ArmPkg/Include/Chipset/ArmV7Mmu.h
>> index 24ab1755faa7..aaa0977205fa 100644
>> --- a/ArmPkg/Include/Chipset/ArmV7Mmu.h
>> +++ b/ArmPkg/Include/Chipset/ArmV7Mmu.h
>> @@ -175,14 +175,14 @@
>>  #define TT_DESCRIPTOR_SECTION_WRITE_BACK(NonSecure) 
>> (TT_DESCRIPTOR_SECTION_TYPE_SECTION  
>>  | \
>>  ((NonSecure) ?  
>> TT_DESCRIPTOR_SECTION_NS : 0)| \
>>  
>> TT_DESCRIPTOR_SECTION_NG_GLOBAL | \
>> -
>> TT_DESCRIPTOR_SECTION_S_NOT_SHARED  | \
>> +
>> TT_DESCRIPTOR_SECTION_S_SHARED  | \
>>  
>> TT_DESCRIPTOR_SECTION_DOMAIN(0) | \
>>  
>> TT_DESCRIPTOR_SECTION_AP_RW_RW  | \
>>  
>> TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC)
>>  #define TT_DESCRIPTOR_SECTION_WRITE_THROUGH(NonSecure)  
>> (TT_DESCRIPTOR_SECTION_TYPE_SECTION  
>>  | \
>>  ((NonSecure) ?  
>> TT_DESCRIPTOR_SECTION_NS : 0)| \
>>  
>> TT_DESCRIPTOR_SECTION_NG_GLOBAL | \
>> -
>> TT_DESCRIPTOR_SECTION_S_NOT_SHARED  | \
>> +
>> TT_DESCRIPTOR_SECTION_S_SHARED  | \
>>  
>> TT_DESCRIPTOR_SECTION_DOMAIN(0) | \
>>  
>> TT_DESCRIPTOR_SECTION_AP_RW_RW  | \
>>  
>> TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC)
>> @@ -203,12 +203,12 @@
>>
>>  #define TT_DESCRIPTOR_PAGE_WRITE_BACK  
>> (TT_DESCRIPTOR_PAGE_TYPE_PAGE
>>| \
>>  
>> TT_DESCRIPTOR_PAGE_NG_GLOBAL 
>>  | \
>> -
>> TT_DESCRIPTOR_PAGE_S_NOT_SHARED   

Re: [edk2] [PATCH] ArmPkg/ArmLib: mark all cached mappings as (inner) shareable

[Mark Rutland]

On Thu, Nov 12, 2015 at 11:35:28AM +, Leif Lindholm wrote:
> Hi Ard,
> 
> On Mon, Nov 09, 2015 at 02:18:58PM +0100, Ard Biesheuvel wrote:
> > Mark all cached memory mappings as shareable (or inner shareable on
> > AArch64) so that our view of memory is kept coherent by the hardware.
> > 
> > This is primarily relevant under virtualization (where a guest may migrate
> > to another core) but in general, since UEFI on ARM is mostly used in a
> > context where the secure firmware and possibly a secure OS are already up
> > and running, it is best to refrain from using any non-shareable mappings.
> 
> Thanks, this is both an important correctness fix and nice code
> cleanup.
> 
> I ran into an issue while testing this, which is why I haven't
> responded to this, but I've bisected it down to r18503/3a05b13, and am
> looking into what causes an issue with Linux booting.

FWIW, I had issues with that which Ard worked around for virtual
machines in r18533/2f71ad11d6eaa2af ("ArmVirtPkg: reduce preallocation
of boot services data pages").

You may be suffering a similar issue.

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

Re: [edk2] EFI GOP with manual vsync trigger

[Andrew Fish]

> On Nov 12, 2015, at 3:28 AM, Andrew Fish  wrote:
> 
>> 
>> On Nov 12, 2015, at 3:22 AM, Andrew Fish  wrote:
>> 
>> 
>>> On Nov 12, 2015, at 12:49 AM, Michael Zimmermann  
>>> wrote:
>>> 
>>> Stall was just an example, I can also use DEBUG. My timer interval is
>>> 100ms(I converted it from the 100ns unit).
>>> 
>>> What I meant with "thread mode code" is the "normal" non IRQ context code
>>> running on the CPU. That means that there are actually two contexts in
>>> EDK2, the exception(i.e. IRQ's like the Timer/Watchdog) and the normal mode
>>> all code is running in.
>>> 
>> 
>> OK thanks that helps. The timer ISR runs in interrupt context, and from an 
>> ARM point of view EFI is running the ARM "Thread mode".  The ISR context is 
>> an implementation detail and not really defined by the specification. 
>> 
>> I'd also point out the Events dispatch independent of the interrupt context. 
>> gBS->RestoreTpl() can cause events to dispatch. For example a lot of the EFI 
>> Protocol services have locks that raise TPL to prevent recursion. When these 
>> locks are released and the TPL is restored events are dispatched. So just 
>> calling EFI services can cause events to run. So conceptually you can ignore 
>> the interrupt context, as that is used to implement the timer tick. Every 
>> thing else is an event that runs in the main EFI context.  
>> 
>> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Event/Tpl.c#L126
>> 
> 
> As Kinney pointed out the events are cooperative and there is no scheduler, 
> so if you are getting stuck some chunk of code is running too long at an 
> elevated TPL. You may need to performance profile to figure out the bad code. 
> 

The events are managed by queues in the DXE Core. The events are described by 
the IEVENT data structure and linked into queues based on state. The state 
transition to the event is calling a C function stored in the IEVENT. The event 
must return, or call an EFI service, to give control back to the DXE Core. 

https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Event/Event.c

///
/// gEventQueueLock - Protects the event queues
///
EFI_LOCK gEventQueueLock = EFI_INITIALIZE_LOCK_VARIABLE (TPL_HIGH_LEVEL);

///
/// gEventQueue - A list of event's to notify for each priority level
///
LIST_ENTRY  gEventQueue[TPL_HIGH_LEVEL + 1];

///
/// gEventPending - A bitmask of the EventQueues that are pending
///
UINTN   gEventPending = 0;

///
/// gEventSignalQueue - A list of events to signal based on EventGroup type
///
LIST_ENTRY  gEventSignalQueue = INITIALIZE_LIST_HEAD_VARIABLE 
(gEventSignalQueue);

It is possible to write a debugger script to dump out the info about the events 
if you have source level debug available for the DXE Core. 

Thanks,

Andrew Fish

> Thanks,
> 
> Andrew Fish
> 
> 
>> Thanks,
>> 
>> Andrew Fish
>> 
>> 
>>> On Thu, Nov 12, 2015 at 9:27 AM, Andrew Fish  wrote:
>>> 
 
> On Nov 11, 2015, at 11:51 PM, Michael Zimmermann <
 sigmaepsilo...@gmail.com> wrote:
> 
> I've started investigating in the timer event problem and I think I have
> some weird problem with my platform drivers(I hope, so it's not a EDK2
 bug).
> 
> If I create a timer that runs every 100ms which does nothing but a
> stall(1), the thread mode code stops after some random time(usually in
 edk2
> shell so I guess it's a race condition which needs some cpu load).
> 
 
 Michael,
 
 Watch out as Stall() is Microseconds, and SetTimer() is 100ns I've seen
 bugs like that before in code.
 
> When threadmode code is stopped the timer continues getting called and
 even
> if I stop the timer afterwards(with CloseEvent) it keeps being stopped.
> 
> Is there a way to get the threadmode context from inside a timer
 callback?
> This way I could read the PC to check what's going on.
> 
 
 There are no threads. EFI is an event model. If your code is running you
 are blocking every one else's forward progress. Only code running at a
 higher TPL can preempt. So when your event is running it is blocking the
 main flow and any event trying to run at <= TPL of your event from making
 forward progress. gBS->Stall() does not yield, it is no different than
 running code.
 
 So there is only one context, you can print it out any time you want.
 
 Thanks,
 
 Andrew Fish
 
 PS When folks yell at us for not having threads in EFI we point them at:
 https://web.stanford.edu/~ouster/cgi-bin/papers/threads.pdf
 
 
> Michael
> 
> On Wed, Nov 11, 2015 at 8:10 PM, Kinney, Michael D <
> michael.d.kin...@intel.com> wrote:
> 
>> Michael,
>> 
>> A periodic event timer at 30 times a second should not cause pauses
>> forever, unless the action you are performing in the event notification
>> function takes more than 1/30 of a second to complete.  Y

Re: [edk2] EFI GOP with manual vsync trigger

[Michael Zimmermann]

thx, this perfectly explains my situation(that EDK2 shell stops while
printing sth. like the map or waiting for this 5s startup.nsh timeout).

So this means that processing the event queue is caused by any API call
which never returns to the DXE phase for some reason.

Is there a special way to debug the event system or do I have to put DEBUG
calls all over the place?

On Thu, Nov 12, 2015 at 5:44 PM, Andrew Fish  wrote:

>
> > On Nov 12, 2015, at 3:28 AM, Andrew Fish  wrote:
> >
> >>
> >> On Nov 12, 2015, at 3:22 AM, Andrew Fish  wrote:
> >>
> >>
> >>> On Nov 12, 2015, at 12:49 AM, Michael Zimmermann <
> sigmaepsilo...@gmail.com> wrote:
> >>>
> >>> Stall was just an example, I can also use DEBUG. My timer interval is
> >>> 100ms(I converted it from the 100ns unit).
> >>>
> >>> What I meant with "thread mode code" is the "normal" non IRQ context
> code
> >>> running on the CPU. That means that there are actually two contexts in
> >>> EDK2, the exception(i.e. IRQ's like the Timer/Watchdog) and the normal
> mode
> >>> all code is running in.
> >>>
> >>
> >> OK thanks that helps. The timer ISR runs in interrupt context, and from
> an ARM point of view EFI is running the ARM "Thread mode".  The ISR context
> is an implementation detail and not really defined by the specification.
> >>
> >> I'd also point out the Events dispatch independent of the interrupt
> context. gBS->RestoreTpl() can cause events to dispatch. For example a lot
> of the EFI Protocol services have locks that raise TPL to prevent
> recursion. When these locks are released and the TPL is restored events are
> dispatched. So just calling EFI services can cause events to run. So
> conceptually you can ignore the interrupt context, as that is used to
> implement the timer tick. Every thing else is an event that runs in the
> main EFI context.
> >>
> >>
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Event/Tpl.c#L126
> >>
> >
> > As Kinney pointed out the events are cooperative and there is no
> scheduler, so if you are getting stuck some chunk of code is running too
> long at an elevated TPL. You may need to performance profile to figure out
> the bad code.
> >
>
> The events are managed by queues in the DXE Core. The events are described
> by the IEVENT data structure and linked into queues based on state. The
> state transition to the event is calling a C function stored in the IEVENT.
> The event must return, or call an EFI service, to give control back to the
> DXE Core.
>
>
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Event/Event.c
>
> ///
> /// gEventQueueLock - Protects the event queues
> ///
> EFI_LOCK gEventQueueLock = EFI_INITIALIZE_LOCK_VARIABLE (TPL_HIGH_LEVEL);
>
> ///
> /// gEventQueue - A list of event's to notify for each priority level
> ///
> LIST_ENTRY  gEventQueue[TPL_HIGH_LEVEL + 1];
>
> ///
> /// gEventPending - A bitmask of the EventQueues that are pending
> ///
> UINTN   gEventPending = 0;
>
> ///
> /// gEventSignalQueue - A list of events to signal based on EventGroup type
> ///
> LIST_ENTRY  gEventSignalQueue = INITIALIZE_LIST_HEAD_VARIABLE
> (gEventSignalQueue);
>
> It is possible to write a debugger script to dump out the info about the
> events if you have source level debug available for the DXE Core.
>
> Thanks,
>
> Andrew Fish
>
> > Thanks,
> >
> > Andrew Fish
> >
> >
> >> Thanks,
> >>
> >> Andrew Fish
> >>
> >>
> >>> On Thu, Nov 12, 2015 at 9:27 AM, Andrew Fish  wrote:
> >>>
> 
> > On Nov 11, 2015, at 11:51 PM, Michael Zimmermann <
>  sigmaepsilo...@gmail.com> wrote:
> >
> > I've started investigating in the timer event problem and I think I
> have
> > some weird problem with my platform drivers(I hope, so it's not a
> EDK2
>  bug).
> >
> > If I create a timer that runs every 100ms which does nothing but a
> > stall(1), the thread mode code stops after some random time(usually
> in
>  edk2
> > shell so I guess it's a race condition which needs some cpu load).
> >
> 
>  Michael,
> 
>  Watch out as Stall() is Microseconds, and SetTimer() is 100ns I've
> seen
>  bugs like that before in code.
> 
> > When threadmode code is stopped the timer continues getting called
> and
>  even
> > if I stop the timer afterwards(with CloseEvent) it keeps being
> stopped.
> >
> > Is there a way to get the threadmode context from inside a timer
>  callback?
> > This way I could read the PC to check what's going on.
> >
> 
>  There are no threads. EFI is an event model. If your code is running
> you
>  are blocking every one else's forward progress. Only code running at a
>  higher TPL can preempt. So when your event is running it is blocking
> the
>  main flow and any event trying to run at <= TPL of your event from
> making
>  forward progress. gBS->Stall() does not yield, it is no different than
>  running code.
> 
>  So there i

Re: [edk2] testing Centos 7.1 boot from iscsi with EDK -> kernel crashes after launch by GRUB

[Laszlo Ersek]

Please don't drop the list and other CC's from the discussion when the
initial report is sent to the list. Keeping full context.

On 11/11/15 08:38, P.A.M. van Dam (Pascal) wrote:
> On Mon, Nov 09, 2015 at 10:02:52PM +0100, Laszlo Ersek wrote:
>> On 11/09/15 14:50, P.A.M. van Dam (Pascal) wrote:
>>>
>>> Good day all,
>>>
>>> I decided to do some testing with iSCSI boot from within a KVM. 
>>>
>>> My setup is a Centos 7.1 KVM host with several Fedora 22 and Centos 7.1 
>>> guests in it. The iSCSI storage has been configured
>>> from a LenovoEMC EPX6300 NAS.
>>>
>>> 1. I've configured the iSCSI bootlun to be available for the OS install.
>>> 2. Within the KVM guest OS install I assign the LUN to the Centos 7.1 OS.
>>> 3. The OS installs without errors ont he iSCSI LUN.
>>>
>>> After reboot of the freshly installed OS on the KVM
>>> I configure the iSCSI device in the OVMF user interface.
>>>
>>> 4. The iSCSI BOOT LUN is visible from the UEFI shell.
>>> 5. When booting from the LUN, GRUB correcly displays the bootmenu.
>>
>> Am I to understand that GRUB (and its config) are correctly loaded via
>> iSCSI?
> 
> Yep. You are correct in this. GRUB loads without issue and both the menu and 
> commandline work. I can even access the kernel and the initrd from GRUB's 
> commandline.
> 
>>
>>> 6. When selecting and starting a kernel/initrd pair the started kernel 
>>> quickly crashes reporting a fault.
>>>
>>> My questions;
>>>
>>> 1. Is iSCSI boot support in the OVMF/EDKII currrently usable?
> 
> 
>>
>> I never tested it, but I guess it should work (for some value of
>> "work"). Since I never tested it, I can't say what code in OvmfPkg would
>> require specific support (perhaps the boot order library... dunno).
>>
>>> 2. Can I help in further analyzing the issue? It could be an issue in EDKII 
>>> (drivers?), in grub or in the Linux kernel.
>>
>> Your email is missing almost all of the important details, so let's
>> start again. :)
> 
> I will try to supply the info needed. This is my first time on this 
> mailinglist and it's long time since I've joined a Linux project.
> 
>>
>> - Are you using libvirt? If so, what version? What is your domain XML?
>>
> 
> Yes, I am using libvirt. I am currently using libvirt-1.2.8-16.el7_1.5.x86_64.
> 
> [virtlabn17.xml]
> 
> [root@kallista:38 qemu]# cat virtlabn17.xml
> 
> 
> 
>   virtlabn17
>   ab2b120f-8a4f-4f90-bd32-fa78fedd51b0
>   Centos 7.1 UEFI/ISCSI BOOT virtlabn17
>   2097152
>   2097152

Okay, so 2GB RAM.

>   2

2 VCPUs

>   
> hvm
>  type='pflash'>/usr/share/edk2.git/ovmf-x64/OVMF_CODE-pure-efi.fd
>  template='/usr/share/edk2.git/ovmf-x64/OVMF_VARS-pure-efi.fd'>/var/lib/libvirt/qemu/nvram/virtlabn17_VARS.fd

Looks good. The OVMF binary is from Gerd.

> 

This is kinda useless here (you don't have a disk in your config). In
any case, as a side point, the  elements within device
elements are much more flexible than this. See
, and then search
that page for more occurrences of the string "boot order=". See also the
"boot" element's docs at
.

> 

This will drop you to the edk2 setup page; I guess it's fine.

>   
>   
> 
> 
> 
>   
>   
> SandyBridge
>   
>   
> 
> 
> 
>   
>   destroy
>   restart
>   restart
>   
> /usr/libexec/qemu-kvm
> 
>function='0x7'/>
> 
> 
>   
>function='0x0' multifunction='on'/>
> 
> 
>   
>function='0x1'/>
> 
> 
>   
>function='0x2'/>
> 
> 
> 
>function='0x0'/>
> 
> 
>   
>   
>   
>function='0x0'/>
> 

I see. This is the "Bridge to LAN" config
.

You are using virtio-net, and the iPXE oprom will be present in the PCI
ROM BAR.

Idea (1): not that it might matter a lot, but you could try with the
builtin virtio-net driver. For that, add the following element within
:



> 
>   
> 
> 
>   
> 
> 
> 
>function='0x0'/>
> 
> 
> 
> 
> 
> 
>function='0x0'/>
> 
>   
> 
> 
> [/virtlabn17.xml]
> 
>  
>> - What version of QEMU are you using? What is your command line?
>>
> 
> /usr/libexec/qemu-kvm --version
> QEMU emulator version 2.1.2 (qemu-kvm-ev-2.1.2-23.el7_1.8.1), Copyright (c) 
> 2003-2008 Fabrice Bellard
> 
> Commandline appears to be (as invoked by virsh)
> 
> /usr/libexec/qemu-kvm -name virtlabn17 -S -machine
> pc-i440fx-rhel7.1.0,accel=kvm,usb=off -cpu SandyBridge -drive
> file=/usr/share/edk2.git/ovmf-x64/OVMF_CODE-pure-efi.fd,if=pflash,format=raw,unit=0,readonly=on
> -drive
> file=/var/lib/libvirt/qemu/nvram/virtlabn17_VARS.fd,if=pflash,format=raw,unit=1
> -m 2048 -realtime mlock=off -smp 2,sockets=2,cores=1,threads=1 -uuid
> ab2b120f-8a4f-4f90-bd32-fa78fedd51b0 -nographic -no-user-config
> -nodefaults -chardev
> socket,id=charmo

Re: [edk2] EFI GOP with manual vsync trigger

[Andrew Fish]

> On Nov 12, 2015, at 9:05 AM, Michael Zimmermann  
> wrote:
> 
> thx, this perfectly explains my situation(that EDK2 shell stops while 
> printing sth. like the map or waiting for this 5s startup.nsh timeout).
> 
> So this means that processing the event queue is caused by any API call which 
> never returns to the DXE phase for some reason.
> 

You should also check your code looking for TPL violations. Calling EFI 
Services at to high a TPL can lead to undefined behavior. 

If you look at the UEFI 2.5 spec, Section 6.1 Event, Timer, and Task Priority 
Services Table 23 lists TPL Restrictions. You can check that your code is not 
violating any of these restrictions. The natural reaction for some one having 
performance issues is to cheat and try to elevate their TPL. 


...

The class of bugs created by calling EFI Services at to hight a TPL are usually 
reentrancy related. Basically the locks in the DXE Core raise TPL to protect 
critical sections, like the protocol data base or event queue. Calling at an 
illegal TPL can cause corruption of some of these structures, and thus 
undefined behavior. 

Another possibility is an event TPL deadlock. When your event runs at a TPL 
only events of a higher TPL can run. Your event is blocking code at <= current 
TPL from running. So if your event is waiting on something to complete that 
needs to run at <= the current TPL that code is starved and will never get run. 
The TPL Restriction table is the guide here also. For example if you driver 
implements Block IO it can be called at TPL <= TPL_CALLBACK, so if your driver 
has an event that needs to complete to make forward progress it needs to run at 
TPL_NOTIFY. 

Since you are producing GOP, there is going to be system code that implements 
Simple Text Output on top of GOP so you inherit those TPL restrictions. 

> Is there a special way to debug the event system or do I have to put DEBUG 
> calls all over the place?
> 

Actually putting DEBUG prints all over the place can make it worse as you can 
increase the amount of time spent processing events. 

I'm not sure if you have a debugger, but if you do
1) I'd break in a few times during the hang to try and get an idea of which 
event is taking all the time from the back trace
2) Pay attention to the TPL when you break in, dump gEfiCurrentTpl
3) You can walk the gEventQueue[] LIST_ENTRY to see what events are active. >= 
your current TPL. 
4) As I mentioned I have lldb scripts to dump queues so you can figure out the 
high frequency timer functions and inspect that code. 

If you don't have a debugger...
1) Add DEBUG print to CoreCreateEventInternal() and print out info about timer 
events that get created. Then you can look at the timer event functions. 
2) CoreDispatchEventNotifies() calls the event notify functions. 
  Event->NotifyFunction (Event, Event->NotifyContext);
You can use the TimerLib to try and figure out what event is taking al the 
time. You can DEBUG print the slowest function, and maybe only do it every Nth 
time so you don't do it 1,000 times a second. 
You could also add code to figure out if the function is running longer than 
(or some large percentage of) its period. 


Caveat Emptor 
Assume it is your event and figure out if you are doing something slow at an 
elevated TPL,  calling a service at too hight a TPL., or have TPL related 
deadlock based on the TPL restrictions. 

Thanks,

Andrew Fish

PS Sorry if this is more detail than you needed, but I figured other folks 
might find this useful. 


> On Thu, Nov 12, 2015 at 5:44 PM, Andrew Fish  > wrote:
> 
> > On Nov 12, 2015, at 3:28 AM, Andrew Fish  > > wrote:
> >
> >>
> >> On Nov 12, 2015, at 3:22 AM, Andrew Fish  >> > wrote:
> >>
> >>
> >>> On Nov 12, 2015, at 12:49 AM, Michael Zimmermann 
> >>> mailto:sigmaepsilo...@gmail.com>> wrote:
> >>>
> >>> Stall was just an example, I can also use DEBUG. My timer interval is
> >>> 100ms(I converted it from the 100ns unit).
> >>>
> >>> What I meant with "thread mode code" is the "normal" non IRQ context code
> >>> running on the CPU. That means that there are actually two contexts in
> >>> EDK2, the exception(i.e. IRQ's like the Timer/Watchdog) and the normal 
> >>> mode
> >>> all code is running in.
> >>>
> >>
> >> OK thanks that helps. The timer ISR runs in interrupt context, and from an 
> >> ARM point of view EFI is running the ARM "Thread mode".  The ISR context 
> >> is an implementation detail and not really defined by the specification.
> >>
> >> I'd also point out the Events dispatch independent of the interrupt 
> >> context. gBS->RestoreTpl() can cause events to dispatch. For example a lot 
> >> of the EFI Protocol services have locks that raise TPL to prevent 
> >> recursion. When these locks are released and the TPL is restored events 
> >> are dispatched. So just calling EFI services can cause events to run. So 
> >> conceptually you can ignore the in

Re: [edk2] EFI GOP with manual vsync trigger

[Michael Zimmermann]

thx for this information.
I don't have any debug hw but UART.

After some careful DEBUG printing and tracing the call stack using
'__builtin_return_address(0)' I found that the application's dead happens
at 'WaitForEvent' and that there aren't any long running events(it returns
from CoreDispatchEventNotifies and CoreRestoreTpl everytime).

I'm not sure this is always the case though because without the DEBUG's the
application can even stop when the edk2 shell is printing the device map
and WaitForEvent usually is only used for key input.

isn't there some GCC option to save the stacktrace so I can dump it after
some seconds runtime(when the system died) and just print the whole
backtrace?(like the linux kernel does).

On Thu, Nov 12, 2015 at 7:39 PM, Andrew Fish  wrote:

>
> On Nov 12, 2015, at 9:05 AM, Michael Zimmermann 
> wrote:
>
> thx, this perfectly explains my situation(that EDK2 shell stops while
> printing sth. like the map or waiting for this 5s startup.nsh timeout).
>
> So this means that processing the event queue is caused by any API call
> which never returns to the DXE phase for some reason.
>
>
> You should also check your code looking for TPL violations. Calling EFI
> Services at to high a TPL can lead to undefined behavior.
>
> If you look at the UEFI 2.5 spec, Section 6.1 Event, Timer, and Task
> Priority Services Table 23 lists TPL Restrictions. You can check that your
> code is not violating any of these restrictions. The natural reaction for
> some one having performance issues is to cheat and try to elevate their
> TPL.
>
>
> ...
>
> The class of bugs created by calling EFI Services at to hight a TPL are
> usually reentrancy related. Basically the locks in the DXE Core raise TPL
> to protect critical sections, like the protocol data base or event queue.
> Calling at an illegal TPL can cause corruption of some of these structures,
> and thus undefined behavior.
>
> Another possibility is an event TPL deadlock. When your event runs at a
> TPL only events of a higher TPL can run. Your event is blocking code at <=
> current TPL from running. So if your event is waiting on something to
> complete that needs to run at <= the current TPL that code is starved and
> will never get run. The TPL Restriction table is the guide here also. For
> example if you driver implements Block IO it can be called at TPL <=
> TPL_CALLBACK, so if your driver has an event that needs to complete to make
> forward progress it needs to run at TPL_NOTIFY.
>
> Since you are producing GOP, there is going to be system code that
> implements Simple Text Output on top of GOP so you inherit those TPL
> restrictions.
>
> Is there a special way to debug the event system or do I have to put DEBUG
> calls all over the place?
>
>
> Actually putting DEBUG prints all over the place can make it worse as you
> can increase the amount of time spent processing events.
>
> I'm not sure if you have a debugger, but if you do
> 1) I'd break in a few times during the hang to try and get an idea of
> which event is taking all the time from the back trace
> 2) Pay attention to the TPL when you break in, dump gEfiCurrentTpl
> 3) You can walk the gEventQueue[] LIST_ENTRY to see what events are
> active. >= your current TPL.
> 4) As I mentioned I have lldb scripts to dump queues so you can figure out
> the high frequency timer functions and inspect that code.
>
> If you don't have a debugger...
> 1) Add DEBUG print to CoreCreateEventInternal() and print out info about
> timer events that get created. Then you can look at the timer event
> functions.
> 2) CoreDispatchEventNotifies() calls the event notify functions.
>   Event->NotifyFunction (Event, Event->NotifyContext);
> You can use the TimerLib to try and figure out what event is taking al the
> time. You can DEBUG print the slowest function, and maybe only do it every
> Nth time so you don't do it 1,000 times a second.
> You could also add code to figure out if the function is running longer
> than (or some large percentage of) its period.
>
>
> Caveat Emptor 
> Assume it is your event and figure out if you are doing something slow at
> an elevated TPL,  calling a service at too hight a TPL., or have TPL
> related deadlock based on the TPL restrictions.
>
> Thanks,
>
> Andrew Fish
>
> PS Sorry if this is more detail than you needed, but I figured other folks
> might find this useful.
>
>
> On Thu, Nov 12, 2015 at 5:44 PM, Andrew Fish  wrote:
>
>>
>> > On Nov 12, 2015, at 3:28 AM, Andrew Fish  wrote:
>> >
>> >>
>> >> On Nov 12, 2015, at 3:22 AM, Andrew Fish  wrote:
>> >>
>> >>
>> >>> On Nov 12, 2015, at 12:49 AM, Michael Zimmermann <
>> sigmaepsilo...@gmail.com> wrote:
>> >>>
>> >>> Stall was just an example, I can also use DEBUG. My timer interval is
>> >>> 100ms(I converted it from the 100ns unit).
>> >>>
>> >>> What I meant with "thread mode code" is the "normal" non IRQ context
>> code
>> >>> running on the CPU. That means that there are actually two contexts in
>> >>> EDK2, 

[edk2] [PATCH 2/3] AppPkg/Python-2.7.10/edk2module.c: Rename posix_ to edk2_.

[Daryl McDaniel]

AppPkg/Python-2.7.10: Rename identifiers beginning with "posix_" to "edk2_".

Rename identifiers to have an edk2_ prefix instead of posix_ in order to
conform to the convention used in other environment-specific Python modules.
This also makes the names match the module instead of implying that these are
Posix compatible routines.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Daryl McDaniel 
---
 .../PyMod-2.7.10/Modules/edk2module.c  | 794 ++---
 1 file changed, 397 insertions(+), 397 deletions(-)

diff --git 
a/AppPkg/Applications/Python/Python-2.7.10/PyMod-2.7.10/Modules/edk2module.c 
b/AppPkg/Applications/Python/Python-2.7.10/PyMod-2.7.10/Modules/edk2module.c
index d9d4916..4cf09ca 100644
--- a/AppPkg/Applications/Python/Python-2.7.10/PyMod-2.7.10/Modules/edk2module.c
+++ b/AppPkg/Applications/Python/Python-2.7.10/PyMod-2.7.10/Modules/edk2module.c
@@ -201,19 +201,19 @@ convertenviron(void)
 /* Set a POSIX-specific error from errno, and return NULL */

 static PyObject *
-posix_error(void)
+edk2_error(void)
 {
 return PyErr_SetFromErrno(PyExc_OSError);
 }
 static PyObject *
-posix_error_with_filename(char* name)
+edk2_error_with_filename(char* name)
 {
 return PyErr_SetFromErrnoWithFilename(PyExc_OSError, name);
 }


 static PyObject *
-posix_error_with_allocated_filename(char* name)
+edk2_error_with_allocated_filename(char* name)
 {
 PyObject *rc = PyErr_SetFromErrnoWithFilename(PyExc_OSError, name);
 PyMem_Free(name);
@@ -224,7 +224,7 @@ posix_error_with_allocated_filename(char* name)

 #ifndef UEFI_C_SOURCE
   static PyObject *
-  posix_fildes(PyObject *fdobj, int (*func)(int))
+  edk2_fildes(PyObject *fdobj, int (*func)(int))
   {
   int fd;
   int res;
@@ -232,19 +232,19 @@ posix_error_with_allocated_filename(char* name)
   if (fd < 0)
   return NULL;
   if (!_PyVerify_fd(fd))
-  return posix_error();
+  return edk2_error();
   Py_BEGIN_ALLOW_THREADS
   res = (*func)(fd);
   Py_END_ALLOW_THREADS
   if (res < 0)
-  return posix_error();
+  return edk2_error();
   Py_INCREF(Py_None);
   return Py_None;
   }
 #endif  /* UEFI_C_SOURCE */

 static PyObject *
-posix_1str(PyObject *args, char *format, int (*func)(const char*))
+edk2_1str(PyObject *args, char *format, int (*func)(const char*))
 {
 char *path1 = NULL;
 int res;
@@ -255,14 +255,14 @@ posix_1str(PyObject *args, char *format, int 
(*func)(const char*))
 res = (*func)(path1);
 Py_END_ALLOW_THREADS
 if (res < 0)
-return posix_error_with_allocated_filename(path1);
+return edk2_error_with_allocated_filename(path1);
 PyMem_Free(path1);
 Py_INCREF(Py_None);
 return Py_None;
 }

 static PyObject *
-posix_2str(PyObject *args,
+edk2_2str(PyObject *args,
char *format,
int (*func)(const char *, const char *))
 {
@@ -279,7 +279,7 @@ posix_2str(PyObject *args,
 PyMem_Free(path2);
 if (res != 0)
 /* XXX how to report both path1 and path2??? */
-return posix_error();
+return edk2_error();
 Py_INCREF(Py_None);
 return Py_None;
 }
@@ -486,7 +486,7 @@ fill_time(PyObject *v, int index, time_t sec, unsigned long 
nsec)
 }

 /* pack a system stat C structure into the Python stat tuple
-   (used by posix_stat() and posix_fstat()) */
+   (used by edk2_stat() and edk2_fstat()) */
 static PyObject*
 _pystat_fromstructstat(STRUCT_STAT *st)
 {
@@ -556,7 +556,7 @@ _pystat_fromstructstat(STRUCT_STAT *st)
 }

 static PyObject *
-posix_do_stat(PyObject *self, PyObject *args,
+edk2_do_stat(PyObject *self, PyObject *args,
   char *format,
   int (*statfunc)(const char *, STRUCT_STAT *),
   char *wformat,
@@ -578,7 +578,7 @@ posix_do_stat(PyObject *self, PyObject *args,
 Py_END_ALLOW_THREADS

 if (res != 0) {
-result = posix_error_with_filename(pathfree);
+result = edk2_error_with_filename(pathfree);
 }
 else
 result = _pystat_fromstructstat(&st);
@@ -589,7 +589,7 @@ posix_do_stat(PyObject *self, PyObject *args,

 /* POSIX methods */

-PyDoc_STRVAR(posix_access__doc__,
+PyDoc_STRVAR(edk2_access__doc__,
 "access(path, mode) -> True if granted, False otherwise\n\n\
 Use the real uid/gid to test for access to a path.  Note that most\n\
 operations will use the effective uid/gid, therefore this routine can\n\
@@ -598,7 +598,7 @@ specified access to the path.  The mode argument can be 
F_OK to test\n\
 existence, or the inclusive-OR of R_OK, W_OK, and X_OK.");

 static PyObject *
-posix_access(PyObject *self, PyObject *args)
+edk2_access(PyObject *self, PyObject *args)
 {
 char *path;
 int mode;
@@ -627,22 +627,22 @@ posix_access(PyObject *self, PyObject *args)
   #define X_OK 1
 #endif

-PyDoc_STRVAR(posix_chdir__doc__,
+PyDoc_STRVAR(edk2_chdir__doc__,
 "chdir(path)\n\n\
 Change the current working directory to the specified path.");

 stati

[edk2] [PATCH 3/3] AppPkg/Python-2.7.10/edk2module.c: Update for Python 2.7.10.

[Daryl McDaniel]

AppPkg/Python-2.7.10: Update file for Python-2.7.10 inclusion.

Add copyright message.
Remove some redundant blank lines.
Remove a superfluous call to setup_confname_tables(m) from INITFUNC().

Signed-off-by: Daryl McDaniel 
---
 .../Python/Python-2.7.10/PyMod-2.7.10/Modules/edk2module.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git 
a/AppPkg/Applications/Python/Python-2.7.10/PyMod-2.7.10/Modules/edk2module.c 
b/AppPkg/Applications/Python/Python-2.7.10/PyMod-2.7.10/Modules/edk2module.c
index 4cf09ca..42a9aea 100644
--- a/AppPkg/Applications/Python/Python-2.7.10/PyMod-2.7.10/Modules/edk2module.c
+++ b/AppPkg/Applications/Python/Python-2.7.10/PyMod-2.7.10/Modules/edk2module.c
@@ -2,6 +2,7 @@
 OS-specific module implementation for EDK II and UEFI.
 Derived from posixmodule.c in Python 2.7.2.

+Copyright (c) 2015, Daryl McDaniel. All rights reserved.
 Copyright (c) 2011 - 2012, Intel Corporation. All rights reserved.
 This program and the accompanying materials are licensed and made 
available under
 the terms and conditions of the BSD License that accompanies this 
distribution.
@@ -2248,6 +2249,7 @@ edk2_getpid(PyObject *self, PyObject *noargs)
 return PyLong_FromPid(getpid());
 }

+
 #ifdef HAVE_GETLOGIN
 PyDoc_STRVAR(edk2_getlogin__doc__,
 "getlogin() -> string\n\n\
@@ -2340,7 +2342,6 @@ PyDoc_STRVAR(edk2_popen__doc__,
 "popen(command [, mode='r' [, bufsize]]) -> pipe\n\n\
 Open a pipe to/from a command returning a file object.");

-/* standard posix version of popen() support */
 static PyObject *
 edk2_popen(PyObject *self, PyObject *args)
 {
@@ -2369,6 +2370,7 @@ edk2_popen(PyObject *self, PyObject *args)

 #endif /* HAVE_POPEN */

+
 #if defined(HAVE_WAIT3) || defined(HAVE_WAIT4)
 static PyObject *
 wait_helper(pid_t pid, int status, struct rusage *ru)
@@ -4187,9 +4189,6 @@ INITFUNC(void)
 if (all_ins(m))
 return;

-if (setup_confname_tables(m))
-return;
-
 Py_INCREF(PyExc_OSError);
 PyModule_AddObject(m, "error", PyExc_OSError);

--
1.9.5.msysgit.1

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

[edk2] [PATCH 0/3] AppPkg/Python-2.7.10/edk2module.c: Reviewable Revision Resubmission

[Daryl McDaniel]

AppPkg/Python-2.7.10: Present patch in three reviewable chunks.

Due to the large number of changes, the previous submission of this patch
was not reviewable.  This patch set presents the changes as three patches, each
containing a single type of change.

Patch 1/3: Remove irrelevant code.
  Remove sections of conditional code that are not relevant to the EDK II
  or UEFI environments.

Patch 2/3: Rename posix_ to edk2_.
  Rename objects beginning with posix_ to edk2_.

Patch 3/3: Update for Python 2.7.10.
  Add copyright message.
  Remove some redundant blank lines.
  Remove a superfluous call to setup_confname_tables(m) from INITFUNC().

 .../PyMod-2.7.10/Modules/edk2module.c  | 5692 +---
 1 file changed, 1253 insertions(+), 4439 deletions(-)

--
1.9.5.msysgit.1

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

Re: [edk2] EFI GOP with manual vsync trigger

[Andrew Fish]

> On Nov 12, 2015, at 11:21 AM, Michael Zimmermann  
> wrote:
> 
> thx for this information.
> I don't have any debug hw but UART.
> 
> After some careful DEBUG printing and tracing the call stack using
> '__builtin_return_address(0)'

FYI the edk2 has a portable form of this RETURN_ADDRESS(0) 

> I found that the application's dead happens
> at 'WaitForEvent'

The WaitForEvent() is what happens at the shell prompt when it is waiting for 
keyboard input. So that is functioning as intended. gBS->WaitForEvent() is the 
power efficient way to wait. 

If you look at CoreWaitForEvent() it checks all the events, and then calls 
CoreSignalEvent with the gIdleLoopEvent. If the DXE Cpu driver supports this 
feature gIdleLoopEvent will put the CPU in a low power state until the next 
timer tick. Since  CoreWaitForEvent() checked all the event state it can not 
change until the next timer tick so this lets the system spend its idle time in 
a lower power mode. 

https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Event/Event.c#L685
  for(;;) {

for(Index = 0; Index < NumberOfEvents; Index++) {

  Status = CoreCheckEvent (UserEvents[Index]);

  //
  // provide index of event that caused problem
  //
  if (Status != EFI_NOT_READY) {
if (UserIndex != NULL) {
  *UserIndex = Index;
}
return Status;
  }
}

//
// Signal the Idle event
//
CoreSignalEvent (gIdleLoopEvent);
  }

So if you are really stuck here it could imply your Simple Text In is hung? 
Maybe your issue is data corruption vs an event issue?

> and that there aren't any long running events(it returns
> from CoreDispatchEventNotifies and CoreRestoreTpl everytime).
> 
> I'm not sure this is always the case though because without the DEBUG's the
> application can even stop when the edk2 shell is printing the device map
> and WaitForEvent usually is only used for key input.
> 
> isn't there some GCC option to save the stacktrace so I can dump it after
> some seconds runtime(when the system died) and just print the whole
> backtrace?(like the linux kernel does).
> 

Well this is when it gets tricky. For VC++ you can only _ReturnAddress(), and 
that is equivalent of __builtin_return_address(0) on the GCC side. But it gets 
worse... To get a stack trace for X64 with VC++ you need symbols as the unwind 
is in the .PDB file. 

For clang, and I think with the right GCC compiler flags, you can walk the 
stack as each routine saves the frame pointer on entry. This is a simple X64 C 
function that just returns 0. The push of the %rbp and saving %rsp allow the 
stack to be unwound in software. 

pushq   %rbp
movq%rsp, %rbp
xorl%eax, %eax
popq%rbp
retq

So it is possible to unwind the stack in software. 

Simple answer is try RETURN_ADDRESS(1), RETURN_ADDRESS(2), etc. If it is 
supported the compiler will generate the code to unwind for you.   

Complex answer is write the stack unwind code, if it is possible? Which 
Processor and I assume you are using GCC? 

You can use the GetImageName() function in 
https://github.com/tianocore/edk2/blob/master/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerUefi.c#L64
 to see how you can use the PE/COFF library functions to convert a FaultAddress 
to the ImageBase, and name of the image. If you know the offset into the image 
relative to the start you can load image in gdb from the build output directory 
and see what code it maps to. 

The X86 version of all this lives: 
https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c

Thanks,

Andrew Fish

> On Thu, Nov 12, 2015 at 7:39 PM, Andrew Fish  wrote:
> 
>> 
>> On Nov 12, 2015, at 9:05 AM, Michael Zimmermann 
>> wrote:
>> 
>> thx, this perfectly explains my situation(that EDK2 shell stops while
>> printing sth. like the map or waiting for this 5s startup.nsh timeout).
>> 
>> So this means that processing the event queue is caused by any API call
>> which never returns to the DXE phase for some reason.
>> 
>> 
>> You should also check your code looking for TPL violations. Calling EFI
>> Services at to high a TPL can lead to undefined behavior.
>> 
>> If you look at the UEFI 2.5 spec, Section 6.1 Event, Timer, and Task
>> Priority Services Table 23 lists TPL Restrictions. You can check that your
>> code is not violating any of these restrictions. The natural reaction for
>> some one having performance issues is to cheat and try to elevate their
>> TPL.
>> 
>> 
>> ...
>> 
>> The class of bugs created by calling EFI Services at to hight a TPL are
>> usually reentrancy related. Basically the locks in the DXE Core raise TPL
>> to protect critical sections, like the protocol data base or event queue.
>> Calling at an illegal TPL can cause corruption of some of these structures,
>> and thus undefined behavior.
>> 
>> Another possibility is an event TPL deadlock. When yo

[edk2] [PATCH v2] NetworkPkg: Httpboot will fail the 2nd time result by wrong TCP state.

[Zhang Lubo]

v2:
*version 1 can fix this issue by checking the TCP state. If it is in the 
close-wait state,
then reconnect TCP. But consider another scenario,if an error occurs of the TCP 
connection
between the first request() and second Request() to http server, it will not 
work. So we
will use the TCP state machine for checking instead of state defined for HTTP.
Also update some comments.

Cc: Ye Ting 
Cc: Fu Siyuan 
CC: Wu Jiaxin 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Zhang Lubo 
---
 NetworkPkg/HttpDxe/HttpImpl.c  | 25 -
 NetworkPkg/HttpDxe/HttpProto.c | 38 +-
 NetworkPkg/HttpDxe/HttpProto.h |  4 +++-
 3 files changed, 44 insertions(+), 23 deletions(-)

diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c
index 2f4ce89..4ad07d4 100644
--- a/NetworkPkg/HttpDxe/HttpImpl.c
+++ b/NetworkPkg/HttpDxe/HttpImpl.c
@@ -315,11 +315,15 @@ EfiHttpRequest (
 
   Status = HttpUrlGetPort (Url, UrlParser, &RemotePort);
   if (EFI_ERROR (Status)) {
 RemotePort = HTTP_DEFAULT_PORT;
   }
-
+  //
+  // If Configure is TRUE, it indicates the first time to call Request();
+  // If ReConfigure is TRUE, it indicates the request URL is not same
+  // with the previous call to Request();
+  //
   Configure   = TRUE;
   ReConfigure = TRUE;  
 
   if (HttpInstance->RemoteHost == NULL) {
 //
@@ -425,11 +429,15 @@ EfiHttpRequest (
 
   if (ReConfigure) {
 //
 // The request URL is different from previous calls to Request(), close 
existing TCP instance.
 //
-ASSERT (HttpInstance->Tcp4 != NULL &&HttpInstance->Tcp6 != NULL);
+if (!HttpInstance->LocalAddressIsIPv6) {
+  ASSERT (HttpInstance->Tcp4 != NULL);
+} else {
+  ASSERT (HttpInstance->Tcp6 != NULL);
+}
 HttpCloseConnection (HttpInstance);
 EfiHttpCancel (This, NULL);
   }
 
   //
@@ -443,26 +451,25 @@ EfiHttpRequest (
 
   Wrap->HttpToken  = Token;
   Wrap->HttpInstance   = HttpInstance;
   Wrap->TcpWrap.Method = Request->Method;
 
-  if (Configure) {
-Status = HttpInitTcp (HttpInstance, Wrap);
-if (EFI_ERROR (Status)) {
-  goto Error2;
-}
+  Status = HttpInitTcp (HttpInstance, Wrap, Configure);
+  if (EFI_ERROR (Status)) {
+goto Error2;
+  }  
 
-  } else {
+  if (!Configure) {
 //
 // For the new HTTP token, create TX TCP token events.
 //
 Status = HttpCreateTcpTxEvent (Wrap);
 if (EFI_ERROR (Status)) {
   goto Error1;
 }
   }
-
+  
   //
   // Create request message.
   //
   FileUrl = Url;
   if (*FileUrl != '/') {
diff --git a/NetworkPkg/HttpDxe/HttpProto.c b/NetworkPkg/HttpDxe/HttpProto.c
index 39837a3..9996951 100644
--- a/NetworkPkg/HttpDxe/HttpProto.c
+++ b/NetworkPkg/HttpDxe/HttpProto.c
@@ -1177,11 +1177,11 @@ HttpConnectTcp4 (
 {
   EFI_STATUSStatus;
   EFI_TCP4_CONNECTION_STATE Tcp4State;
 
 
-  if (HttpInstance->State != HTTP_STATE_TCP_CONFIGED || HttpInstance->Tcp4 == 
NULL) {
+  if (HttpInstance->State < HTTP_STATE_TCP_CONFIGED || HttpInstance->Tcp4 == 
NULL) {
 return EFI_NOT_READY;
   }
 
   Status = HttpInstance->Tcp4->GetModeData(
  HttpInstance->Tcp4, 
@@ -1194,13 +1194,15 @@ HttpConnectTcp4 (
   if (EFI_ERROR(Status)){
 DEBUG ((EFI_D_ERROR, "Tcp4 GetModeData fail - %x\n", Status));
 return Status;
   }
 
-  if (Tcp4State > Tcp4StateEstablished) {
+  if (Tcp4State == Tcp4StateEstablished) {
+return EFI_SUCCESS;
+  } else if (Tcp4State > Tcp4StateEstablished ) {
 HttpCloseConnection(HttpInstance);
-  }  
+  }
 
   return HttpCreateConnection (HttpInstance);
 }
 
 /**
@@ -1219,11 +1221,11 @@ HttpConnectTcp6 (
   )
 {
   EFI_STATUSStatus;
   EFI_TCP6_CONNECTION_STATE Tcp6State;
 
-  if (HttpInstance->State != HTTP_STATE_TCP_CONFIGED || HttpInstance->Tcp6 == 
NULL) {
+  if (HttpInstance->State < HTTP_STATE_TCP_CONFIGED || HttpInstance->Tcp6 == 
NULL) {
 return EFI_NOT_READY;
   }
 
   Status = HttpInstance->Tcp6->GetModeData (
  HttpInstance->Tcp6,
@@ -1237,44 +1239,51 @@ HttpConnectTcp6 (
   if (EFI_ERROR(Status)){
  DEBUG ((EFI_D_ERROR, "Tcp6 GetModeData fail - %x\n", Status));
  return Status;
   }
 
-  if (Tcp6State > Tcp6StateEstablished) {
-HttpCloseConnection (HttpInstance);
+  if (Tcp6State == Tcp6StateEstablished) {
+return EFI_SUCCESS;
+  } else if (Tcp6State > Tcp6StateEstablished ) {
+HttpCloseConnection(HttpInstance);
   }
 
   return HttpCreateConnection (HttpInstance);
 }
 
 /**
   Initialize TCP related data.
 
   @param[in]  HttpInstance   The HTTP instance private data.
   @param[in]  Wrap   The HTTP token's wrap data.
+  @param[in]  Configure  The Flag indicates whether the first time to 
initialize Tcp.
 
   @retval EFI_SUCCESSThe initialization of TCP instance is done. 
   @retval Others Other error as indicated.
 
 **/
 EFI_STATUS
 HttpInitTcp 

Re: [edk2] [PATCH] MdeModulePkg PeiCore: PEI dispatcher need retry to process NOT_DISPATCHED FV

[Gao, Liming]

Reviewed-by: Liming Gao 

> -Original Message-
> From: Zeng, Star
> Sent: Wednesday, November 11, 2015 6:07 PM
> To: edk2-devel@lists.01.org
> Cc: Gao, Liming
> Subject: [PATCH] MdeModulePkg PeiCore: PEI dispatcher need retry to process 
> NOT_DISPATCHED FV
> 
> A corner case like below will cause a NOT_DISPATCHED FV has no opportunity to
> be dispatched.
>   1. FV_RECOVERY has SecCore, PeiCore and some other PEI modules, a module 
> will
> report FVMAIN_COMPACT and FV_RECOVERY2 in sequence.
>   2. FVMAIN_COMPACT has a FV image file with GUIDED FV image section in it.
>   3. FV_RECOVERY2 has DxeIpl and other PEI modules, the DxeIpl will install
>  SectionExtractionPpi
>   If ALL the PEIMs in FV_RECOVERY and FV_RECOVERY2 have DEPEX satisfied and
>   executed in one loop, PeimNeedingDispatch will be always FALSE, 
> FVMAIN_COMPACT
>   will have no opportunity to be decompressed and dispatched as DxeIpl 
> executes
>   after the first processing to FVMAIN_COMPACT.
> 
> The patch is to set PeimNeedingDispatch to TRUE when ProcessFvFile() not 
> successfully,
> then the NOT_DISPATCHED FV could have another opportunity to be processed.
> 
> Cc: Liming Gao 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Star Zeng 
> ---
>  MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 15 +++
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c 
> b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> index 7480b66..e7e795d 100644
> --- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> +++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> @@ -1056,7 +1056,7 @@ PeiDispatcher (
>  ASSERT_EFI_ERROR (Status);
>  if (FvFileInfo.FileType == 
> EFI_FV_FILETYPE_FIRMWARE_VOLUME_IMAGE) {
>//
> -  // For Fv type file, Produce new FV PPI and FV hob
> +  // For Fv type file, Produce new FvInfo PPI and FV hob
>//
>Status = ProcessFvFile (Private, &Private->Fv[FvCount], 
> PeimFileHandle);
>if (Status == EFI_SUCCESS) {
> @@ -1065,6 +1065,13 @@ PeiDispatcher (
>  //
>  Private->Fv[FvCount].PeimState[PeimCount]++;
>  Private->PeimDispatchOnThisPass = TRUE;
> +  } else {
> +//
> +// The related GuidedSectionExtraction/Decompress PPI for the
> +// encapsulated FV image section may be installed in the rest
> +// of this do-while loop, so need to make another pass.
> +//
> +Private->PeimNeedingDispatch = TRUE;
>}
>  } else {
>//
> @@ -1192,11 +1199,11 @@ PeiDispatcher (
>  Private->CurrentPeimFvCount = 0;
> 
>  //
> -// PeimNeedingDispatch being TRUE means we found a PEIM that did not get
> +// PeimNeedingDispatch being TRUE means we found a PEIM/FV that did not 
> get
>  //  dispatched. So we need to make another pass
>  //
> -// PeimDispatchOnThisPass being TRUE means we dispatched a PEIM on this
> -//  pass. If we did not dispatch a PEIM there is no point in trying again
> +// PeimDispatchOnThisPass being TRUE means we dispatched a PEIM/FV on 
> this
> +//  pass. If we did not dispatch a PEIM/FV there is no point in trying 
> again
>  //  as it will fail the next time too (nothing has changed).
>  //
>} while (Private->PeimNeedingDispatch && Private->PeimDispatchOnThisPass);
> --
> 1.9.5.msysgit.0

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

Re: [edk2] [PATCH v2] NetworkPkg: Httpboot will fail the 2nd time result by wrong TCP state.

[Ye, Ting]

Looks good.

Reviewed-by: Ye Ting  

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Zhang 
Lubo
Sent: Friday, November 13, 2015 9:41 AM
To: edk2-devel@lists.01.org
Cc: Ye, Ting; Fu, Siyuan; Wu, Jiaxin
Subject: [edk2] [PATCH v2] NetworkPkg: Httpboot will fail the 2nd time result 
by wrong TCP state.

v2:
*version 1 can fix this issue by checking the TCP state. If it is in the 
close-wait state,
then reconnect TCP. But consider another scenario,if an error occurs of the TCP 
connection
between the first request() and second Request() to http server, it will not 
work. So we
will use the TCP state machine for checking instead of state defined for HTTP.
Also update some comments.

Cc: Ye Ting 
Cc: Fu Siyuan 
CC: Wu Jiaxin 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Zhang Lubo 
---
 NetworkPkg/HttpDxe/HttpImpl.c  | 25 -
 NetworkPkg/HttpDxe/HttpProto.c | 38 +-
 NetworkPkg/HttpDxe/HttpProto.h |  4 +++-
 3 files changed, 44 insertions(+), 23 deletions(-)

diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c
index 2f4ce89..4ad07d4 100644
--- a/NetworkPkg/HttpDxe/HttpImpl.c
+++ b/NetworkPkg/HttpDxe/HttpImpl.c
@@ -315,11 +315,15 @@ EfiHttpRequest (
 
   Status = HttpUrlGetPort (Url, UrlParser, &RemotePort);
   if (EFI_ERROR (Status)) {
 RemotePort = HTTP_DEFAULT_PORT;
   }
-
+  //
+  // If Configure is TRUE, it indicates the first time to call Request();
+  // If ReConfigure is TRUE, it indicates the request URL is not same
+  // with the previous call to Request();
+  //
   Configure   = TRUE;
   ReConfigure = TRUE;  
 
   if (HttpInstance->RemoteHost == NULL) {
 //
@@ -425,11 +429,15 @@ EfiHttpRequest (
 
   if (ReConfigure) {
 //
 // The request URL is different from previous calls to Request(), close 
existing TCP instance.
 //
-ASSERT (HttpInstance->Tcp4 != NULL &&HttpInstance->Tcp6 != NULL);
+if (!HttpInstance->LocalAddressIsIPv6) {
+  ASSERT (HttpInstance->Tcp4 != NULL);
+} else {
+  ASSERT (HttpInstance->Tcp6 != NULL);
+}
 HttpCloseConnection (HttpInstance);
 EfiHttpCancel (This, NULL);
   }
 
   //
@@ -443,26 +451,25 @@ EfiHttpRequest (
 
   Wrap->HttpToken  = Token;
   Wrap->HttpInstance   = HttpInstance;
   Wrap->TcpWrap.Method = Request->Method;
 
-  if (Configure) {
-Status = HttpInitTcp (HttpInstance, Wrap);
-if (EFI_ERROR (Status)) {
-  goto Error2;
-}
+  Status = HttpInitTcp (HttpInstance, Wrap, Configure);
+  if (EFI_ERROR (Status)) {
+goto Error2;
+  }  
 
-  } else {
+  if (!Configure) {
 //
 // For the new HTTP token, create TX TCP token events.
 //
 Status = HttpCreateTcpTxEvent (Wrap);
 if (EFI_ERROR (Status)) {
   goto Error1;
 }
   }
-
+  
   //
   // Create request message.
   //
   FileUrl = Url;
   if (*FileUrl != '/') {
diff --git a/NetworkPkg/HttpDxe/HttpProto.c b/NetworkPkg/HttpDxe/HttpProto.c
index 39837a3..9996951 100644
--- a/NetworkPkg/HttpDxe/HttpProto.c
+++ b/NetworkPkg/HttpDxe/HttpProto.c
@@ -1177,11 +1177,11 @@ HttpConnectTcp4 (
 {
   EFI_STATUSStatus;
   EFI_TCP4_CONNECTION_STATE Tcp4State;
 
 
-  if (HttpInstance->State != HTTP_STATE_TCP_CONFIGED || HttpInstance->Tcp4 == 
NULL) {
+  if (HttpInstance->State < HTTP_STATE_TCP_CONFIGED || HttpInstance->Tcp4 == 
NULL) {
 return EFI_NOT_READY;
   }
 
   Status = HttpInstance->Tcp4->GetModeData(
  HttpInstance->Tcp4, 
@@ -1194,13 +1194,15 @@ HttpConnectTcp4 (
   if (EFI_ERROR(Status)){
 DEBUG ((EFI_D_ERROR, "Tcp4 GetModeData fail - %x\n", Status));
 return Status;
   }
 
-  if (Tcp4State > Tcp4StateEstablished) {
+  if (Tcp4State == Tcp4StateEstablished) {
+return EFI_SUCCESS;
+  } else if (Tcp4State > Tcp4StateEstablished ) {
 HttpCloseConnection(HttpInstance);
-  }  
+  }
 
   return HttpCreateConnection (HttpInstance);
 }
 
 /**
@@ -1219,11 +1221,11 @@ HttpConnectTcp6 (
   )
 {
   EFI_STATUSStatus;
   EFI_TCP6_CONNECTION_STATE Tcp6State;
 
-  if (HttpInstance->State != HTTP_STATE_TCP_CONFIGED || HttpInstance->Tcp6 == 
NULL) {
+  if (HttpInstance->State < HTTP_STATE_TCP_CONFIGED || HttpInstance->Tcp6 == 
NULL) {
 return EFI_NOT_READY;
   }
 
   Status = HttpInstance->Tcp6->GetModeData (
  HttpInstance->Tcp6,
@@ -1237,44 +1239,51 @@ HttpConnectTcp6 (
   if (EFI_ERROR(Status)){
  DEBUG ((EFI_D_ERROR, "Tcp6 GetModeData fail - %x\n", Status));
  return Status;
   }
 
-  if (Tcp6State > Tcp6StateEstablished) {
-HttpCloseConnection (HttpInstance);
+  if (Tcp6State == Tcp6StateEstablished) {
+return EFI_SUCCESS;
+  } else if (Tcp6State > Tcp6StateEstablished ) {
+HttpCloseConnection(HttpInstance);
   }
 
   return HttpCreateConnection (HttpInstance);
 }
 
 /**
   Initialize TCP related data.
 
   @param[in]  HttpInstance   The HTTP in

[edk2] [Patch 2/2] MdeModulePkg: Change BootLogoEnableLogo use INTN for minus value

[Ruiyu Ni]

The parameter name is also changed from Coordinate* to Offset* to
reflect that it's the offset to the location specified by Attribute.
For example, when the Attribute is Center, OffsetX and OffsetY are
used to specify the offset to the Center. OffsetX = 100 means
100 pixels right to the Center.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni 
Cc: Feng Tian 
---
 MdeModulePkg/Include/Library/BootLogoLib.h |  8 ++--
 MdeModulePkg/Library/BootLogoLib/BootLogoLib.c | 63 --
 2 files changed, 34 insertions(+), 37 deletions(-)

diff --git a/MdeModulePkg/Include/Library/BootLogoLib.h 
b/MdeModulePkg/Include/Library/BootLogoLib.h
index 3637405..b39d61b 100644
--- a/MdeModulePkg/Include/Library/BootLogoLib.h
+++ b/MdeModulePkg/Include/Library/BootLogoLib.h
@@ -24,8 +24,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
   @param[in]  ImageFormat Format of the image file.
   @param[in]  LogoFileThe file name of logo to display.
   @param[in]  Attribute   The display attributes of the image returned.
-  @param[in]  CoordinateX The X coordinate of the image.
-  @param[in]  CoordinateY The Y coordinate of the image.
+  @param[in]  OffsetX The X offset of the image regarding the Attribute.
+  @param[in]  OffsetY The Y offset of the image regarding the Attribute.
 
   @retval EFI_SUCCESS Logo was displayed.
   @retval EFI_UNSUPPORTED Logo was not found or cannot be displayed.
@@ -36,8 +36,8 @@ BootLogoEnableLogo (
   IN  IMAGE_FORMAT  ImageFormat,
   IN  EFI_GUID  *Logo,
   IN  EDKII_PLATFORM_LOGO_DISPLAY_ATTRIBUTE Attribute,
-  IN  UINTN CoordinateX,
-  IN  UINTN CoordinateY
+  IN  INTN  OffsetX,
+  IN  INTN  OffsetY
   );
 
 
diff --git a/MdeModulePkg/Library/BootLogoLib/BootLogoLib.c 
b/MdeModulePkg/Library/BootLogoLib/BootLogoLib.c
index 312adb0..f824ae1 100644
--- a/MdeModulePkg/Library/BootLogoLib/BootLogoLib.c
+++ b/MdeModulePkg/Library/BootLogoLib/BootLogoLib.c
@@ -35,8 +35,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
   @param[in]  ImageFormat Format of the image file.
   @param[in]  LogoFileThe file name of logo to display.
   @param[in]  Attribute   The display attributes of the image returned.
-  @param[in]  CoordinateX The X coordinate of the image.
-  @param[in]  CoordinateY The Y coordinate of the image.
+  @param[in]  OffsetX The X offset of the image regarding the Attribute.
+  @param[in]  OffsetY The Y offset of the image regarding the Attribute.
 
   @retval EFI_SUCCESS Logo was displayed.
   @retval EFI_UNSUPPORTED Logo was not found or cannot be displayed.
@@ -47,8 +47,8 @@ BootLogoEnableLogo (
   IN  IMAGE_FORMAT  ImageFormat,
   IN  EFI_GUID  *Logo,
   IN  EDKII_PLATFORM_LOGO_DISPLAY_ATTRIBUTE Attribute,
-  IN  UINTN CoordinateX,
-  IN  UINTN CoordinateY
+  IN  INTN  OffsetX,
+  IN  INTN  OffsetY
   )
 {
   EFI_STATUSStatus;
@@ -160,8 +160,8 @@ BootLogoEnableLogo (
&ImageData,
&ImageSize,
&Attribute,
-   &CoordinateX,
-   &CoordinateY
+   &OffsetX,
+   &OffsetY
);
   if (EFI_ERROR (Status)) {
 break;
@@ -199,48 +199,42 @@ BootLogoEnableLogo (
 //
 switch (Attribute) {
 case EdkiiPlatformLogoDisplayAttributeLeftTop:
-  DestX = CoordinateX;
-  DestY = CoordinateY;
+  DestX = 0;
+  DestY = 0;
   break;
-
 case EdkiiPlatformLogoDisplayAttributeCenterTop:
   DestX = (SizeOfX - Width) / 2;
-  DestY = CoordinateY;
+  DestY = 0;
   break;
-
 case EdkiiPlatformLogoDisplayAttributeRightTop:
-  DestX = (SizeOfX - Width - CoordinateX);
-  DestY = CoordinateY;;
+  DestX = SizeOfX - Width;
+  DestY = 0;
   break;
 
-case EdkiiPlatformLogoDisplayAttributeCenterRight:
-  DestX = (SizeOfX - Width - CoordinateX);
+case EdkiiPlatformLogoDisplayAttributeCenterLeft:
+  DestX = 0;
   DestY = (SizeOfY - Height) / 2;
   break;
-
-case EdkiiPlatformLogoDisplayAttributeRightBottom:
-  DestX = (SizeOfX - Width - CoordinateX);
-  DestY = (SizeOfY - Height - CoordinateY);
-  break;
-
-case EdkiiPlatformLogoDisplayAttributeCenterBottom:
+case EdkiiPlatformLogoDisplayAttributeCenter:
   DestX = (SizeOfX - Width) / 2;
-  DestY = (SizeOfY - Height - CoordinateY);
-  break;
-
-case EdkiiPlatformLogoDisplayAttribut

[edk2] [Patch 1/2] MdeModulePkg: Change PlatformLogo.GetImage use INTN for minus value

[Ruiyu Ni]

The parameter name is also changed from Coordinate* to Offset* to
reflect that it's the offset to the location specified by Attribute.
For example, when the Attribute is Center, OffsetX and OffsetY are
used to specify the offset to the Center. OffsetX = 100 means
100 pixels right to the Center.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni 
Cc: Feng Tian 
---
 MdeModulePkg/Include/Protocol/PlatformLogo.h | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Include/Protocol/PlatformLogo.h 
b/MdeModulePkg/Include/Protocol/PlatformLogo.h
index 9ac87f1..8c1d3ca 100644
--- a/MdeModulePkg/Include/Protocol/PlatformLogo.h
+++ b/MdeModulePkg/Include/Protocol/PlatformLogo.h
@@ -55,8 +55,8 @@ typedef enum {
supports the .bmp file format. 
   @param ImageSize The size of the image returned.
   @param Attribute The display attributes of the image returned.
-  @param CoordinateX   The X coordinate of the image.
-  @param CoordinateY   The Y coordinate of the image.
+  @param OffsetX   The X offset of the image regarding the Attribute.
+  @param OffsetY   The Y offset of the image regarding the Attribute.
 
   @retval EFI_SUCCESS  The image was fetched successfully.
   @retval EFI_NOT_FOUNDThe specified image could not be found.
@@ -71,8 +71,8 @@ EFI_STATUS
  OUT UINT8 **ImageData,
  OUT UINTN *ImageSize,
  OUT EDKII_PLATFORM_LOGO_DISPLAY_ATTRIBUTE *Attribute,
- OUT UINTN *CoordinateX,
- OUT UINTN *CoordinateY
+ OUT INTN  *OffsetX,
+ OUT INTN  *OffsetY
 );
 
 
-- 
1.9.5.msysgit.1

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

[edk2] [Patch 0/2] Fix the interface/implementation of PlatformLogo/BootLogoLib

[Ruiyu Ni]

The bug was found when I wants to use this library to show logo in a specific
location other than the 9 pre-defined locations.
The change to this interface implictly tells that caller can specify other 
location
using Offset* parameters relative to the 9 pre-defined locations.

Ruiyu Ni (2):
  MdeModulePkg: Change PlatformLogo.GetImage use INTN for minus value
  MdeModulePkg: Change BootLogoEnableLogo use INTN for minus value

 MdeModulePkg/Include/Library/BootLogoLib.h |  8 ++--
 MdeModulePkg/Include/Protocol/PlatformLogo.h   |  8 ++--
 MdeModulePkg/Library/BootLogoLib/BootLogoLib.c | 63 --
 3 files changed, 38 insertions(+), 41 deletions(-)

-- 
1.9.5.msysgit.1

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

[edk2] [PATCH 1/2] ArmPkg/AArch64Mmu: remove unused GcdAttributeToArmAttribute()

[Ard Biesheuvel]

The function GcdAttributeToArmAttribute() is not used anywhere in the
code base, and is only defined for AARCH64 and not for ARM. It also
fails to set the bits for shareability and non-executability that we
require for correct operation. So remove it.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
---
 ArmPkg/Include/Chipset/AArch64.h   |  5 ---
 ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 42 
 2 files changed, 47 deletions(-)

diff --git a/ArmPkg/Include/Chipset/AArch64.h b/ArmPkg/Include/Chipset/AArch64.h
index 47993ec9fc3b..f6a89012898c 100644
--- a/ArmPkg/Include/Chipset/AArch64.h
+++ b/ArmPkg/Include/Chipset/AArch64.h
@@ -178,11 +178,6 @@ PageAttributeToGcdAttribute (
   IN UINT64 PageAttributes
   );
 
-UINT64
-GcdAttributeToPageAttribute (
-  IN UINT64 GcdAttributes
-  );
-
 UINTN
 ArmWriteCptr (
   IN  UINT64 Cptr
diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c 
b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
index 8829c6286b36..c8b3d4a121b1 100644
--- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
+++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
@@ -96,48 +96,6 @@ PageAttributeToGcdAttribute (
   return GcdAttributes;
 }
 
-UINT64
-GcdAttributeToPageAttribute (
-  IN UINT64 GcdAttributes
-  )
-{
-  UINT64  PageAttributes;
-
-  switch (GcdAttributes & 0xFF) {
-  case EFI_MEMORY_UC:
-PageAttributes = TT_ATTR_INDX_DEVICE_MEMORY;
-break;
-  case EFI_MEMORY_WC:
-PageAttributes = TT_ATTR_INDX_MEMORY_NON_CACHEABLE;
-break;
-  case EFI_MEMORY_WT:
-PageAttributes = TT_ATTR_INDX_MEMORY_WRITE_THROUGH;
-break;
-  case EFI_MEMORY_WB:
-PageAttributes = TT_ATTR_INDX_MEMORY_WRITE_BACK;
-break;
-  default:
-DEBUG ((EFI_D_ERROR, "GcdAttributeToPageAttribute: 0x%X attributes is not 
supported.\n", GcdAttributes));
-ASSERT (0);
-// If no match has been found then we mark the memory as device memory.
-// The only side effect of using device memory should be a slow down in 
the performance.
-PageAttributes = TT_ATTR_INDX_DEVICE_MEMORY;
-  }
-
-  // Determine protection attributes
-  if (GcdAttributes & EFI_MEMORY_WP) {
-// Read only cases map to write-protect
-PageAttributes |= TT_AP_RO_RO;
-  }
-
-  // Process eXecute Never attribute
-  if (GcdAttributes & EFI_MEMORY_XP) {
-PageAttributes |= (TT_PXN_MASK | TT_UXN_MASK);
-  }
-
-  return PageAttributes;
-}
-
 ARM_MEMORY_REGION_ATTRIBUTES
 GcdAttributeToArmAttribute (
   IN UINT64 GcdAttributes
-- 
1.9.1

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

[edk2] [PATCH 2/2] ArmPkg/AArch64Mmu: set required XN attributes for device mappings

[Ard Biesheuvel]

To prevent speculative intruction fetches from MMIO ranges that may
have side effects on reads, the architecture requires device mappings
to be created with the XN or UXN/PXN bits set (for the EL2 and EL1&0
translation regimes, respectively.)

Reported-by: Heyi Guo 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
---
 ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c 
b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
index c8b3d4a121b1..377a7858d436 100644
--- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
+++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
@@ -50,7 +50,10 @@ ArmMemoryAttributeToPageAttribute (
 ASSERT(0);
   case ARM_MEMORY_REGION_ATTRIBUTE_DEVICE:
   case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_DEVICE:
-return TT_ATTR_INDX_DEVICE_MEMORY;
+if (ArmReadCurrentEL () == AARCH64_EL2)
+  return TT_ATTR_INDX_DEVICE_MEMORY | TT_TABLE_XN;
+else
+  return TT_ATTR_INDX_DEVICE_MEMORY | TT_TABLE_UXN | TT_TABLE_PXN;
   }
 }
 
-- 
1.9.1

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