[edk2] RE uses the version information in application .EFI file: edk2-devel Digest, Vol 4, Issue 214

[Miller, Carl H]

Liming,
the reason i want to maintain the version information is so that I can deduce 
the version of the file by parsing the PE header, from a non-efi environment.
--

Message: 7
Date: Thu, 29 Oct 2015 09:15:07 +
From: "Gao, Liming" 
To: "Miller, Carl H" , "edk2-devel@lists.01.org"

Subject: Re: [edk2] [PATCH] BaseTools/GenFw: add option to retain
image   version info
Message-ID:
<4a89e2ef3dfedb4c8bfde51014f606a112e60...@shsmsx102.ccr.corp.intel.com>

Content-Type: text/plain; charset="us-ascii"

Carl:
  Could you let me how you uses the version information in application .EFI 
file?

Thanks
Liming
From: Miller, Carl H [mailto:carl.mil...@pnnl.gov]
Sent: Wednesday, October 28, 2015 10:32 PM
To: edk2-devel@lists.01.org
Cc: Zhu, Yonghong; Gao, Liming
Subject: [PATCH] BaseTools/GenFw: add option to retain image version info


add a command line option("-leaveversion") to GenFw to retain the version 
information in application .EFI files.



Cc: M: Yonghong Zhu >

   M: Liming Gao >

Contributed-under: TianoCore Contribution Agreement 1.0

Signed-off-by: Thompson, James J 
>

---
diff --git a/BaseTools/Source/C/GenFw/GenFw.c b/BaseTools/Source/C/GenFw/GenFw.c
index 4756c52..eea131f 100644
--- a/BaseTools/Source/C/GenFw/GenFw.c
+++ b/BaseTools/Source/C/GenFw/GenFw.c
@@ -1078,6 +1078,7 @@ Returns:
   UNWIND_INFO  *UnwindInfo;
   STATUS   Status;
   BOOLEAN  ReplaceFlag;
+  BOOLEAN  LeaveVersionFlag;
   BOOLEAN  KeepExceptionTableFlag;
   BOOLEAN  KeepZeroPendingFlag;
   UINT64   LogLevel;
@@ -1140,6 +1141,7 @@ Returns:
   InputFileLength   = 0;
   Optional32= NULL;
   Optional64= NULL;
+  LeaveVersionFlag  = FALSE;
   KeepExceptionTableFlag = FALSE;
   KeepZeroPendingFlag= FALSE;
   NumberOfFormPacakge= 0;
@@ -1262,7 +1264,14 @@ Returns:
   continue;
 }

-if (stricmp (argv[0], "--keepexceptiontable") == 0) {
+ if (stricmp(argv[0], "--leaveversion") == 0) {
+ LeaveVersionFlag = TRUE;
+ argc--;
+ argv++;
+ continue;
+ }
+
+ if (stricmp(argv[0], "--keepexceptiontable") == 0) {
   KeepExceptionTableFlag = TRUE;
   argc --;
   argv ++;
@@ -2397,8 +2406,10 @@ Returns:
 Optional64 = (EFI_IMAGE_OPTIONAL_HEADER64 *)>Pe32.OptionalHeader;
 Optional64->MajorOperatingSystemVersion = 0;
 Optional64->MinorOperatingSystemVersion = 0;
-Optional64->MajorImageVersion   = 0;
-Optional64->MinorImageVersion   = 0;
+ if (!LeaveVersionFlag) {
+ Optional64->MajorImageVersion = 0;
+ Optional64->MinorImageVersion = 0;
+ }
 Optional64->MajorSubsystemVersion   = 0;
 Optional64->MinorSubsystemVersion   = 0;
 Optional64->Win32VersionValue   = 0;


--

Subject: Digest Footer

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


--

End of edk2-devel Digest, Vol 4, Issue 214
***
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 4/6] UefiCpuPkg/CpuDxe: Allow BSP to continue once max APs are awake

[Laszlo Ersek]

On 10/29/15 02:32, Jordan Justen wrote:
> Since the CpuDxe has a fixed maximum number of APs that are controlled
> with PcdCpuMaxLogicalProcessorNumber, if the maximum number of APs are
> awake, then the BSP should be able to continue.
> 
> Since the APs may be updating the mMpSystemData.NumberOfProcessors
> during this early init phase, we use SynchronizationLib to update this
> variable on the AP and to read it on the BSP.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jordan Justen 
> Cc: Jeff Fan 
> Cc: Laszlo Ersek 
> ---
>  UefiCpuPkg/CpuDxe/CpuMp.c | 15 ---
>  UefiCpuPkg/CpuDxe/CpuMp.h |  2 +-
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c
> index 6a22b9d..1094292 100644
> --- a/UefiCpuPkg/CpuDxe/CpuMp.c
> +++ b/UefiCpuPkg/CpuDxe/CpuMp.c
> @@ -1474,7 +1474,7 @@ ApEntryPointInC (
>  // Store the Stack address, when reset the AP, We can found the original 
> address.
>  //
>  mMpSystemData.CpuDatas[mMpSystemData.NumberOfProcessors].TopOfStack = 
> TopOfApStack;
> -mMpSystemData.NumberOfProcessors++;
> +InterlockedIncrement ();
>  mMpSystemData.NumberOfEnabledProcessors++;
>} else {
>  Status = WhoAmI (, );
> @@ -1749,9 +1749,18 @@ InitializeMpSupport (
>  StartApsStackless ();
>  
>  //
> -// Wait for APs to arrive at the ApEntryPoint routine
> +// Wait for all APs to start
>  //
> -MicroSecondDelay (PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds));
> +Timeout = 0;
> +do {
> +  if (InterlockedCompareExchange32 (
> +, 0, 0) >=
> +  gMaxLogicalProcessorNumber) {
> +break;
> +  }

I suggest to add a CpuPause() call here. I think it might help on
physical platforms too, but it definitely helps on hypervisors / PCPUs
that support Pause Loop Exiting.

The CpuPause() call will likely make each iteration of this loop to take
longer, but gPollInterval below is a lower bound anyway.

Anyway, this is just an idea. With or without CpuPause():

Reviewed-by: Laszlo Ersek 

Thanks
Laszlo

> +  MicroSecondDelay (gPollInterval);
> +  Timeout += gPollInterval;
> +} while (Timeout <= PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds));
>}
>  
>DEBUG ((DEBUG_INFO, "Detect CPU count: %d\n", 
> mMpSystemData.NumberOfProcessors));
> diff --git a/UefiCpuPkg/CpuDxe/CpuMp.h b/UefiCpuPkg/CpuDxe/CpuMp.h
> index 503f3ae..ab7a7f5 100644
> --- a/UefiCpuPkg/CpuDxe/CpuMp.h
> +++ b/UefiCpuPkg/CpuDxe/CpuMp.h
> @@ -114,7 +114,7 @@ typedef struct {
>  **/
>  typedef struct {
>CPU_DATA_BLOCK  *CpuDatas;
> -  UINTN   NumberOfProcessors;
> +  UINT32  NumberOfProcessors;
>UINTN   NumberOfEnabledProcessors;
>  
>EFI_AP_PROCEDUREProcedure;
> 

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

Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug

[Leif Lindholm]

Hi Feng,

On Thu, Oct 29, 2015 at 12:23:04AM +, Tian, Feng wrote:
> Ray has sent the patch for review, you may ignore
> it. (http://article.gmane.org/gmane.comp.bios.edk2.devel/3484)

Yes, I see it has now been committed - which solves the known bug.

But this still leaves us with a clearly not sufficiently
reviewed/tested invasive patch in core PCI code, and a jumbled commit
history.

Since I have not received any response when asking previously, I will
ask again: Can we please revert SVN r18658 and introduce this
changeset in a more reviewable form, with proper review/testing?

Regards,

Leif

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Leif 
> Lindholm
> Sent: Wednesday, October 28, 2015 22:51
> To: Tian, Feng
> Cc: Ni, Ruiyu; edk2-devel@lists.01.org; Fan, Jeff
> Subject: Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug
> 
> Feng,
> 
> Any update on the below?
> The hard crash bug is still in SVN r18690.
> 
> Regards,
> 
> Leif
> 
> On Mon, Oct 26, 2015 at 03:35:15PM +, Leif Lindholm wrote:
> > Hi Ray,
> > 
> > On Mon, Oct 26, 2015 at 03:17:22PM +, Ni, Ruiyu wrote:
> > > Can you add a null pointer check to
> > > PciIoDevice->ResourcePaddingDescriptors before calling
> > > DumpPpbPaddingResource? Does it help?
> > 
> > It removes the delay and the crash - thanks!
> > 
> > But it does nothing for the commit history ;)
> > 
> > Regards,
> > 
> > Leif
> > 
> > > > 在 2015年10月26日，21:13，Leif Lindholm  写道：
> > > > 
> > > > Hi Ruiyu, Feng,
> > > > 
> > > > I am currently tracking down an issue on (at least) one of my 
> > > > platforms - that happens with this (now committed) patch, but not 
> > > > without it.
> > > > 
> > > > Symptoms are a _long_ delay, followed by an unaligned access in (I
> > > > think) DumpPpbPaddingResource.
> > > > 
> > > > Anyway, there could be other things playing in here - I'm testing 
> > > > a new card in a platform I haven't previously tested cards.
> > > > 
> > > > But this is one large patch, which could have been split up into 
> > > > multiple ones to make the introduced changes more reviewable:
> > > > - There are both functional changes and whitespace fixups.
> > > > - There are (text-only) changes to existing messages.
> > > > - There is refactoring of internal APIs.
> > > > 
> > > > It is certainly too invasive a change to be committed ~32 minutes 
> > > > after having first been posted to the list.
> > > > 
> > > > Any chance we can revert this one and introduce it in smaller 
> > > > portions, making the individual change sets more reviewable?
> > > > 
> > > > Regards,
> > > > 
> > > > Leif
> > > > 
> > > >> On Fri, Oct 23, 2015 at 03:57:40PM +0800, Ruiyu Ni wrote:
> > > >> For a hot plug bridge with device attached, PciBusDxe driver 
> > > >> reserves the resources which equal to the total amount of padding 
> > > >> resource returned from HotPlug->GetResourcePadding() and the 
> > > >> actual occupied resource by the attached device. The behavior is 
> > > >> incorrect.
> > > >> Correct behavior is to reserve the bigger one between the padding 
> > > >> resource and the actual occupied resource.
> > > >> 
> > > >> Contributed-under: TianoCore Contribution Agreement 1.0
> > > >> Signed-off-by: Ruiyu Ni 
> > > >> Cc: Jeff Fan 
> > > >> Cc: Feng Tian 
> > > >> ---
> > > >> .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.c   |  76 +-
> > > >> .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.h   |  13 ++
> > > >> MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c| 159 
> > > >> ++---
> > > >> .../Bus/Pci/PciBusDxe/PciResourceSupport.c |  58 +---
> > > >> 4 files changed, 204 insertions(+), 102 deletions(-)
> > > >> 
> > > >> diff --git 
> > > >> a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c 
> > > >> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > > >> index f7aea4f..030ef42 100644
> > > >> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > > >> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > > >> @@ -325,6 +325,77 @@ PciSearchDevice ( }
> > > >> 
> > > >> /**
> > > >> +  Dump the PPB padding resource information.
> > > >> +
> > > >> +  @param PciIoDevice PCI IO instance.
> > > >> +  @param ResourceTypeThe desired resource type to dump.
> > > >> + PciBarTypeUnknown means to dump all types of 
> > > >> resources.
> > > >> +**/
> > > >> +VOID
> > > >> +DumpPpbPaddingResource (
> > > >> +  IN PCI_IO_DEVICE*PciIoDevice,
> > > >> +  IN PCI_BAR_TYPE ResourceType
> > > >> +  )
> > > >> +{
> > > >> +  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptor;
> > > >> +  PCI_BAR_TYPE  Type;
> > > >> +
> > > >> +  if (ResourceType == PciBarTypeIo16 || ResourceType == 
> > > >> PciBarTypeIo32) {
> > > >> +ResourceType = PciBarTypeIo;  }
> > > >> +
> 

Re: [edk2] [PATCH v2] EmbeddedPkg: Add EFIAPI to several Ebl functions

[Ard Biesheuvel]

On 29 October 2015 at 02:30, Thomas Palmer  wrote:
> The EFIAPI function declaration is missing for several functions in the 
> EmbeddedPkg/Ebl directory. A few function pointer struct members expect 
> EFIAPI though and GCC46/X64 will fail to compile the directory without them.
>

Please line wrap your commit messages the next time.

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Thomas Palmer 

Reviewed-by: Ard Biesheuvel 

Committed as SVN r18697

Thanks,
Ard.



> ---
>  EmbeddedPkg/Ebl/Command.c   | 9 +
>  EmbeddedPkg/Ebl/Dir.c   | 3 +++
>  EmbeddedPkg/Ebl/Ebl.h   | 3 +++
>  EmbeddedPkg/Ebl/EfiDevice.c | 9 +
>  EmbeddedPkg/Ebl/Hob.c   | 2 ++
>  EmbeddedPkg/Ebl/HwDebug.c   | 4 
>  EmbeddedPkg/Ebl/HwIoDebug.c | 3 +++
>  EmbeddedPkg/Ebl/Main.c  | 3 +++
>  EmbeddedPkg/Ebl/Network.c   | 2 ++
>  EmbeddedPkg/Ebl/Script.c| 2 ++
>  EmbeddedPkg/Ebl/Variable.c  | 3 +++
>  EmbeddedPkg/Include/Library/EblCmdLib.h | 3 +++
>  12 files changed, 46 insertions(+)
>
> diff --git a/EmbeddedPkg/Ebl/Command.c b/EmbeddedPkg/Ebl/Command.c
> index 04ea794..e75c6a2 100644
> --- a/EmbeddedPkg/Ebl/Command.c
> +++ b/EmbeddedPkg/Ebl/Command.c
> @@ -3,6 +3,7 @@
>
>Copyright (c) 2007, Intel Corporation. All rights reserved.
>Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.
> +  (C) Copyright 2015 Hewlett Packard Enterprise Development LP
>
>This program and the accompanying materials
>are licensed and made available under the terms and conditions of the BSD 
> License
> @@ -256,6 +257,7 @@ CountNewLines (
>
>  **/
>  EFI_STATUS
> +EFIAPI
>  EblHelpCmd (
>IN UINTN  Argc,
>IN CHAR8  **Argv
> @@ -313,6 +315,7 @@ EblHelpCmd (
>
>  **/
>  EFI_STATUS
> +EFIAPI
>  EblExitCmd (
>IN UINTN  Argc,
>IN CHAR8  **Argv
> @@ -418,6 +421,7 @@ EblPauseCallback (
>
>  **/
>  EFI_STATUS
> +EFIAPI
>  EblPauseCmd (
>IN UINTN  Argc,
>IN CHAR8  **Argv
> @@ -452,6 +456,7 @@ EblPauseCmd (
>
>  **/
>  EFI_STATUS
> +EFIAPI
>  EblBreakPointCmd (
>IN UINTN  Argc,
>IN CHAR8  **Argv
> @@ -478,6 +483,7 @@ EblBreakPointCmd (
>
>  **/
>  EFI_STATUS
> +EFIAPI
>  EblResetCmd (
>IN UINTN  Argc,
>IN CHAR8  **Argv
> @@ -518,6 +524,7 @@ EblResetCmd (
>
>  **/
>  EFI_STATUS
> +EFIAPI
>  EblPageCmd (
>IN UINTN  Argc,
>IN CHAR8  **Argv
> @@ -542,6 +549,7 @@ EblPageCmd (
>  }
>
>  EFI_STATUS
> +EFIAPI
>  EblSleepCmd (
>IN UINTN Argc,
>IN CHAR8 **Argv
> @@ -741,6 +749,7 @@ WidthFromCommandName (
>
>  **/
>  EFI_STATUS
> +EFIAPI
>  EblHexdumpCmd (
>IN UINTN  Argc,
>IN CHAR8  **Argv
> diff --git a/EmbeddedPkg/Ebl/Dir.c b/EmbeddedPkg/Ebl/Dir.c
> index c45f67b..36095b6 100644
> --- a/EmbeddedPkg/Ebl/Dir.c
> +++ b/EmbeddedPkg/Ebl/Dir.c
> @@ -3,6 +3,7 @@
>
>Copyright (c) 2007, Intel Corporation. All rights reserved.
>Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.
> +  (C) Copyright 2015 Hewlett Packard Enterprise Development LP
>
>
>This program and the accompanying materials
> @@ -62,6 +63,7 @@ GLOBAL_REMOVE_IF_UNREFERENCED   CHAR8 *gFvFileType[] = {
>
>  **/
>  EFI_STATUS
> +EFIAPI
>  EblDirCmd (
>IN UINTN  Argc,
>IN CHAR8  **Argv
> @@ -307,6 +309,7 @@ Done:
>
>  **/
>  EFI_STATUS
> +EFIAPI
>  EblCdCmd (
>IN UINTN  Argc,
>IN CHAR8  **Argv
> diff --git a/EmbeddedPkg/Ebl/Ebl.h b/EmbeddedPkg/Ebl/Ebl.h
> index c2242df..e028735 100644
> --- a/EmbeddedPkg/Ebl/Ebl.h
> +++ b/EmbeddedPkg/Ebl/Ebl.h
> @@ -3,6 +3,7 @@
>
>Copyright (c) 2007, Intel Corporation. All rights reserved.
>Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.
> +  (C) Copyright 2015 Hewlett Packard Enterprise Development LP
>
>This program and the accompanying materials
>are licensed and made available under the terms and conditions of the BSD 
> License
> @@ -87,6 +88,7 @@ EblPathToDevice (
>);
>
>  BOOLEAN
> +EFIAPI
>  EblAnyKeyToContinueQtoQuit (
>IN  UINTN   *CurrentRow,
>IN  BOOLEAN PrefixNewline
> @@ -114,6 +116,7 @@ EblSetTextColor (
>
>
>  EFI_STATUS
> +EFIAPI
>  EblGetCharKey (
>IN OUT EFI_INPUT_KEY*Key,
>IN UINTNTimoutInSec,
> diff --git a/EmbeddedPkg/Ebl/EfiDevice.c b/EmbeddedPkg/Ebl/EfiDevice.c
> index 7d20609..ec9c331 100644
> --- a/EmbeddedPkg/Ebl/EfiDevice.c
> +++ b/EmbeddedPkg/Ebl/EfiDevice.c
> @@ -3,6 +3,7 @@
>
>Copyright (c) 2007, Intel Corporation. All rights reserved.
>Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.
> +  (C) Copyright 2015 Hewlett Packard Enterprise Development LP
>
>This program and the accompanying materials
>are licensed and made available under the terms and conditions of the BSD 
> License
> @@ -202,6 

Re: [edk2] [Patch] UefiCpuPkg/PiSmmCpuDxeSmm: Shouldn't use gSmst->CurrentlyExecutingCpu

[Fan, Jeff]

Laszlo,

SmmCoreEntry() is from the following code, and it is just PiSmmCore's 
SmmEntryPoint(). 
  //
  // Invoke SMM Foundation EntryPoint with the processor information context.
  //
  gSmmCpuPrivate->SmmCoreEntry (>SmmCoreEntryContext);

Yes. gSmst->CurrentlyExecutingCpu is not set in PiSmmCpuDxeSmm driver. It is 
set in PiSmmCore driver by the following code.
  //
  // Update SMST using the context
  //
  CopyMem (, SmmEntryContext, sizeof 
(EFI_SMM_ENTRY_CONTEXT));

Thanks!
Jeff
-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com] 
Sent: Thursday, October 29, 2015 8:46 PM
To: Fan, Jeff; edk2-de...@ml01.01.org
Cc: Kinney, Michael D
Subject: Re: [edk2] [Patch] UefiCpuPkg/PiSmmCpuDxeSmm: Shouldn't use 
gSmst->CurrentlyExecutingCpu

On 10/29/15 09:11, Jeff Fan wrote:
> In ConfigSmmCodeAccessCheck(), we used gSmst->CurrentlyExecutingCpu to 
> get the current SMM BSP. But ConfigSmmCodeAccessCheck() maybe invoked 
> before executing
> SmmCoreEntry() and gSmst->CurrentlyExecutingCpu hasn't been updated to 
> the latest value. Instead, we should use
> gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu directly.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeff Fan 
> Cc: Michael Kinney 
> Cc: Laszlo Ersek 
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> index e210c8d..c351875 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> @@ -1386,7 +1386,7 @@ ConfigSmmCodeAccessCheck (
>//
>// Check to see if the Feature Control MSR is supported on this CPU
>//
> -  Index = gSmst->CurrentlyExecutingCpu;
> +  Index = gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu;
>if (!SmmCpuFeaturesIsSmmRegisterSupported (Index, SmmRegFeatureControl)) {
>  mSmmCodeAccessCheckEnable = FALSE;
>  return;
> @@ -1428,7 +1428,7 @@ ConfigSmmCodeAccessCheck (
>// Enable SMM Code Access Check feature for the APs.
>//
>for (Index = 0; Index < gSmst->NumberOfCpus; Index++) {
> -if (Index != gSmst->CurrentlyExecutingCpu) {
> +if (Index != 
> + gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu) {
>  
>//
>// Acquire Config SMM Code Access Check spin lock.  The AP will 
> release the
> 

I found precisely one location in the entire codebase where 
"gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu" is set. The call 
tree is:

BSPHandler()   [UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c]
  // sets field
  gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu = CpuIndex;

  PerformRemainingTasks()  [UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c]
ConfigSmmCodeAccessCheck()
  // reads the value here

>From that aspect this patch looks correct.

However... I have also found that gSmst->CurrentlyExecutingCpu is *never* set.

Is that a correct observation?

And if it is, shouldn't we rather (or additionally) fix *that*?

The commit message references SmmCoreEntry(). That function doesn't seem to 
exist in edk2.

A field called "SmmCoreEntry" does exist in SMM_CPU_PRIVATE_DATA, and (IIRC) it 
does point to SmmEntryPoint() [MdeModulePkg/Core/PiSmmCore/PiSmmCore.c], after 
registration. But even SmmEntryPoint() does not seem to set 
gSmst->CurrentlyExecutingCpu.

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

Re: [edk2] [PATCH 5/6] UefiCpuPkg: Allow PcdCpuMaxLogicalProcessorNumber to be dynamic

[Laszlo Ersek]

On 10/29/15 02:32, Jordan Justen wrote:
> Move from [PcdsFixedAtBuild, PcdsPatchableInModule] to
> [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx].
> 
> This will allow a platform to set this PCD dynamically if it knows how
> many logical processors the system has. The CpuDxe driver can then use
> this information during multiprocessor initialization to know that all
> of the application processors have started.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jordan Justen 
> Cc: Jeff Fan 
> Cc: Laszlo Ersek 
> ---
>  UefiCpuPkg/UefiCpuPkg.dec | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index e783a7b..e16780e 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -128,10 +128,6 @@
># @Prompt Configure delay value after send an INIT IPI
>
> gUefiCpuPkgTokenSpaceGuid.PcdCpuInitIpiDelayInMicroSeconds|1|UINT32|0x3002
>  
> -  ## Specifies max supported number of Logical Processors.
> -  # @Prompt Configure max supported number of Logical Processors
> -  
> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64|UINT32|0x0002
> -
>## This value specifies the Application Processor (AP) stack size, used 
> for Mp Service, which must
>## aligns the address on a 4-KByte boundary.
># @Prompt Configure stack size for Application Processor (AP)
> @@ -178,6 +174,10 @@
># @Prompt Microcode Region size.
>
> gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize|0x0|UINT64|0x0006
>  
> +  ## Specifies max supported number of Logical Processors.
> +  # @Prompt Configure max supported number of Logical Processors
> +  
> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64|UINT32|0x0002
> +
>  [PcdsDynamic, PcdsDynamicEx]
>## Contains the pointer to a CPU S3 data buffer of structure ACPI_CPU_DATA.
># @Prompt The pointer to a CPU S3 data buffer.
> 

Two requests, one about your development environment, and another about
the patch itself.

(1) Please append the following to your ".git/info/attributes" file:


*.dec diff=ini
*.dsc diff=ini
*.dsc.inc diff=ini
*.fdf diff=ini
*.fdf.inc diff=ini
*.inf diff=ini


Then please append the below to your ".git/config" file:


[diff "ini"]
xfuncname = "^\\[[A-Za-z0-9_., ]+]"


Please format this patch with those settings in place, and then compare
the result against *this* formatting. You'll understand my request then. :)

(2) Regarding the patch, please add a comment to the PCD (or extend its
@Prompt):

"If the execution environment contains more logical processors than this
value, then the behavior is undefined."

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

Re: [edk2] [Patch] UefiCpuPkg/PiSmmCpuDxeSmm: Shouldn't use gSmst->CurrentlyExecutingCpu

[Laszlo Ersek]

On 10/29/15 14:17, Fan, Jeff wrote:
> Laszlo,
> 
> SmmCoreEntry() is from the following code, and it is just PiSmmCore's 
> SmmEntryPoint(). 
>   //
>   // Invoke SMM Foundation EntryPoint with the processor information context.
>   //
>   gSmmCpuPrivate->SmmCoreEntry (>SmmCoreEntryContext);
> 
> Yes. gSmst->CurrentlyExecutingCpu is not set in PiSmmCpuDxeSmm driver. It is 
> set in PiSmmCore driver by the following code.
>   //
>   // Update SMST using the context
>   //
>   CopyMem (, SmmEntryContext, sizeof 
> (EFI_SMM_ENTRY_CONTEXT));

I don't understand how that CopyMem()  call is supposed to set 
gSmst->CurrentlyExecutingCpu.

- gSmst points to an EFI_SMM_SYSTEM_TABLE2 object.
- gSmmCoreSmst is an object of the same type.

In the EFI_SMM_SYSTEM_TABLE2 structure, "SmmStartupThisAp" and 
"CurrentlyExecutingCpu" are *sibling* fields. Neither of both is contained in 
the other.

Therefore I don't understand how the above CopyMem() can set the 
CurrentlyExecutingCpu field.

... OMG... The CopyMem() actually *overflows* the SmmStartupThisAp field. That 
field is only used as a start offset into gSmmCoreSmst. EFI_SMM_ENTRY_CONTEXT 
contains a *subset* of the EFI_SMM_SYSTEM_TABLE2 fields, in the same order, and 
with the same sizes:
- SmmStartupThisAp
- CurrentlyExecutingCpu
- NumberOfCpus
- CpuSaveStateSize
- ...

Which is why the CopyMem() works.

I'm sorry to say, but  I think this is one of the ugliest pieces of code I've 
seen in my life. :(

Anyway, what we have now is the following order:

  BSPHandler()
gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu = CpuIndex;

gSmmCpuPrivate->SmmCoreEntry (>SmmCoreEntryContext);

  CopyMem (, SmmEntryContext, sizeof 
(EFI_SMM_ENTRY_CONTEXT));

PerformRemainingTasks()  [UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c]
  ConfigSmmCodeAccessCheck()
... gSmst->CurrentlyExecutingCpu ...

So, I don't understand how the bug is possible. In the commit message you say, 
"ConfigSmmCodeAccessCheck() maybe invoked before executing SmmCoreEntry()". How?

Ah! Now I understand, there is another ConfigSmmCodeAccessCheck() call in 
BSPHandler(), *before* SmmCoreEntry. It depends on 
"mRestoreSmmConfigurationInS3".

Can you please add the following to the commit message:

---
  BSPHandler()
gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu =
  CpuIndex;

//
// when mRestoreSmmConfigurationInS3 is set:
//
ConfigSmmCodeAccessCheck()
  //
  // reads gSmst->CurrentlyExecutingCpu to early
  //

gSmmCpuPrivate->SmmCoreEntry (
  >SmmCoreEntryContext)
  //
  // sets gSmst->CurrentlyExecutingCpu with CopyMem() too late
  //
  CopyMem (,
SmmEntryContext, sizeof (EFI_SMM_ENTRY_CONTEXT));
---

With that change:

Reviewed-by: Laszlo Ersek 

Thanks
Laszlo


> 
> Thanks!
> Jeff
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com] 
> Sent: Thursday, October 29, 2015 8:46 PM
> To: Fan, Jeff; edk2-de...@ml01.01.org
> Cc: Kinney, Michael D
> Subject: Re: [edk2] [Patch] UefiCpuPkg/PiSmmCpuDxeSmm: Shouldn't use 
> gSmst->CurrentlyExecutingCpu
> 
> On 10/29/15 09:11, Jeff Fan wrote:
>> In ConfigSmmCodeAccessCheck(), we used gSmst->CurrentlyExecutingCpu to 
>> get the current SMM BSP. But ConfigSmmCodeAccessCheck() maybe invoked 
>> before executing
>> SmmCoreEntry() and gSmst->CurrentlyExecutingCpu hasn't been updated to 
>> the latest value. Instead, we should use
>> gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu directly.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Jeff Fan 
>> Cc: Michael Kinney 
>> Cc: Laszlo Ersek 
>> ---
>>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c 
>> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> index e210c8d..c351875 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> @@ -1386,7 +1386,7 @@ ConfigSmmCodeAccessCheck (
>>//
>>// Check to see if the Feature Control MSR is supported on this CPU
>>//
>> -  Index = gSmst->CurrentlyExecutingCpu;
>> +  Index = gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu;
>>if (!SmmCpuFeaturesIsSmmRegisterSupported (Index, SmmRegFeatureControl)) {
>>  mSmmCodeAccessCheckEnable = FALSE;
>>  return;
>> @@ -1428,7 +1428,7 @@ ConfigSmmCodeAccessCheck (
>>// Enable SMM Code Access Check feature for the APs.
>>//
>>for (Index = 0; Index < gSmst->NumberOfCpus; Index++) {
>> -if (Index != gSmst->CurrentlyExecutingCpu) {
>> +if (Index != 
>> + gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu) {
>>  
>>//
>>// Acquire Config SMM Code Access Check spin lock.  The AP will 
>> release the
>>
> 
> I found precisely one 

Re: [edk2] about the SMM_S3_RESUME_SMM_64 branch in S3Resume2Pei

[Laszlo Ersek]

Hi Jiewen,

On 10/29/15 03:28, Yao, Jiewen wrote:
> Thanks for the info. I think we might have to dig out why LONG_JUMP fail here.
> Most our IA BIOS support Ia32X64 mode, so I am sure it should work.

I think I have found the bug.

(1)
I added a debug patch to OvmfPkg/QuarkPort/CpuS3DataDxe. I'm attaching it for 
illustration. The debug patch dumps the following information:
- the location of the GDT descriptor (that gets loaded into the GDT register)
- the contents of the GDT descriptor (i.e., the base and limit of the global 
descriptor table)
- the contents of the GDT entries (the individual segment descriptors)

CpuS3DataDxe saves these in AcpiNVS. Later they are saved by PiSmmCpuDxeSmm 
from AcpiNVS to SMRAM (during boot), and then restored from SMRAM to AcpiNVS 
(during S3 Resume).

The patch dumps these details when the preparation for PiSmmCpuDxeSmm, in 
AcpiNVS, is ready.

Here's the debug output:

SaveCpuS3Data: 350: GDT descriptor at 0x7E3E2050
SaveCpuS3Data: 353: GDT Base=0x7F706000 Limit=0x3F
SaveCpuS3Data: 363: Idx=0x00 Base=0x Limit=0x Type=0x0 
System=0x0 Dpl=0x0 Present=0x0 Software=0x0 LCodeSeg=0x0 DefaultSize=0x0
SaveCpuS3Data: 363: Idx=0x08 Base=0x Limit=0x Type=0x3 
System=0x1 Dpl=0x0 Present=0x1 Software=0x0 LCodeSeg=0x0 DefaultSize=0x1
SaveCpuS3Data: 363: Idx=0x10 Base=0x Limit=0x Type=0xB 
System=0x1 Dpl=0x0 Present=0x1 Software=0x0 LCodeSeg=0x0 DefaultSize=0x1
SaveCpuS3Data: 363: Idx=0x18 Base=0x Limit=0x Type=0x2 
System=0x1 Dpl=0x0 Present=0x1 Software=0x0 LCodeSeg=0x0 DefaultSize=0x1
SaveCpuS3Data: 363: Idx=0x20 Base=0x Limit=0x Type=0xA 
System=0x1 Dpl=0x0 Present=0x1 Software=0x0 LCodeSeg=0x0 DefaultSize=0x1
SaveCpuS3Data: 363: Idx=0x28 Base=0x Limit=0x Type=0xB 
System=0x1 Dpl=0x0 Present=0x1 Software=0x0 LCodeSeg=0x1 DefaultSize=0x0
SaveCpuS3Data: 363: Idx=0x30 Base=0x Limit=0x Type=0x0 
System=0x0 Dpl=0x0 Present=0x0 Software=0x0 LCodeSeg=0x0 DefaultSize=0x0
SaveCpuS3Data: 363: Idx=0x38 Base=0x Limit=0x Type=0x0 
System=0x0 Dpl=0x0 Present=0x0 Software=0x0 LCodeSeg=0x0 DefaultSize=0x0

(Importantly, the debug output is identical between the pure Ia32 build and the 
Ia32X64 build, except the addresses in the first two lines -- the location of 
the GDT descriptor, and the location of the GDT itself.)


(2) Now locate the offset (= selector) of the *only* segment that is advertised 
as "64-bit code". The LCodeSeg bit-field is 1 only for Idx=0x28.


(3) If you examine "UefiCpuPkg/PiSmmCpuDxeSmm/X64/MpFuncs.asm", you can see:

LONG_JUMP::

db 67h,  0EAh ; far jump
dd 0h ; 32-bit offset
dw 38h; 16-bit selector

The segment selector is 0x38 here, which not only lacks the LCodeSeg (= long 
mode code) bit, but it is actually a null-descriptor.

So this is the direct cause for the triple fault, but why does it work in the 
pure Ia32 case, and what is the *root* cause for the triple fault in the 
Ia32X64 case?

We can investigate some more:


(4) The DXE IPL PEIM actually sets up the 0x38 segment correctly. In the 
HandOffToDxeCore() function [MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c], 
branch "PcdDxeIplSwitchToLongMode", the last statement is:

//
// Go to Long Mode and transfer control to DxeCore.
// Interrupts will not get turned on until the CPU AP is loaded.
// Call x64 drivers passing in single argument, a pointer to the HOBs.
//
AsmEnablePaging64 (
  SYS_CODE64_SEL,
  DxeCoreEntryPoint,
  (EFI_PHYSICAL_ADDRESS)(UINTN)(HobList.Raw),
  0,
  TopOfStack
  );

where SYS_CODE64_SEL equals 0x38. Because the 64-bit DXE core starts running, 
segment 0x38 is obviously correctly configured at this point. (See also the 
"gGdtEntries" array in "MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c" -- the 
entry at byte offset 0x38 sets the LCodeSeg bit.)


(5) *However*, when the CpuDxe driver starts, the InitializeCpu() entry point 
function does the following:

InitializeCpu() [UefiCpuPkg/CpuDxe/CpuDxe.c]

  //
  // Init GDT for DXE
  //
  InitGlobalDescriptorTable ();
 
//
// Initialize all GDT entries
//
CopyMem (gdt, , sizeof (GdtTemplate));

And the GdtTemplate array [UefiCpuPkg/CpuDxe/CpuGdt.c] is what we can see in 
the debug dump above!

0x38 corresponds to "SPARE5_SEL" in CpuDxe, and the only long-mode code segment 
is LINEAR_CODE64_SEL (0x28).

In summary, the bug is that CpuDxe sets up a GDT that is incompatible with 
"UefiCpuPkg/PiSmmCpuDxeSmm/X64/MpFuncs.*".


(6) I'm unsure how this could be fixed. *Although* both "gGdtEntries" from the 
DXE IPL, and "GdtTemplate" from CpuDxe, have one 64-bit code segment each, the 
segment descriptors are not identical. (For which reason I'm unsure if we can 
simply point "UefiCpuPkg/PiSmmCpuDxeSmm/X64/MpFuncs.*" to the segment that 
comes from CpuDxe, when the 

Re: [edk2] about the SMM_S3_RESUME_SMM_64 branch in S3Resume2Pei

[Kinney, Michael D]

Laszlo,

Thanks for the very detailed and complete root cause on this issue.  I agree 
that the change you suggest is functional, but I am going to recommend a 
different solution.

Unfortunately, there are several modules that set up a GDT and it is easier 
from an implementation perspective and for debugging if all the modules agree 
to use the same GDT layout.  It would be even better if no modules make any 
assumptions about GDT layouts, but that is a bigger change that we can tackle 
later.

The root cause of this specific issue is that the GDT that is set up by 
MdeModulePkg/Core/DxeIplPeim is not the same GDT that is set up by UefiCpuPkg 
/CpuDxe.  And the UefiCpuPkg/PiSmmCpuDxeSmm module has some assumptions that 
the GDT is the one setup by MdeModulePkg/Core/DxeIplPeim.  I recommend we 
update UefiCpuPkg/CpuDxe to use the same GDT layout as 
MdeModulePkg/Core/DxeIplPeim and then the changes you found worked for 
PiSmmCpuDxeSmm are not required.

I will work on a patch for you to test.

Mike

>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>Laszlo Ersek
>Sent: Thursday, October 29, 2015 9:19 AM
>To: Yao, Jiewen; Kinney, Michael D; Fan, Jeff
>Cc: Justen, Jordan L; edk2-devel-01
>Subject: Re: [edk2] about the SMM_S3_RESUME_SMM_64 branch in
>S3Resume2Pei
>
>Hi Jiewen,
>
>On 10/29/15 03:28, Yao, Jiewen wrote:
>> Thanks for the info. I think we might have to dig out why LONG_JUMP fail
>here.
>> Most our IA BIOS support Ia32X64 mode, so I am sure it should work.
>
>I think I have found the bug.
>
>(1)
>I added a debug patch to OvmfPkg/QuarkPort/CpuS3DataDxe. I'm attaching it
>for illustration. The debug patch dumps the following information:
>- the location of the GDT descriptor (that gets loaded into the GDT register)
>- the contents of the GDT descriptor (i.e., the base and limit of the global
>descriptor table)
>- the contents of the GDT entries (the individual segment descriptors)
>
>CpuS3DataDxe saves these in AcpiNVS. Later they are saved by
>PiSmmCpuDxeSmm from AcpiNVS to SMRAM (during boot), and then restored
>from SMRAM to AcpiNVS (during S3 Resume).
>
>The patch dumps these details when the preparation for PiSmmCpuDxeSmm,
>in AcpiNVS, is ready.
>
>Here's the debug output:
>
>SaveCpuS3Data: 350: GDT descriptor at 0x7E3E2050
>SaveCpuS3Data: 353: GDT Base=0x7F706000 Limit=0x3F
>SaveCpuS3Data: 363: Idx=0x00 Base=0x Limit=0x Type=0x0
>System=0x0 Dpl=0x0 Present=0x0 Software=0x0 LCodeSeg=0x0 DefaultSize=0x0
>SaveCpuS3Data: 363: Idx=0x08 Base=0x Limit=0x Type=0x3
>System=0x1 Dpl=0x0 Present=0x1 Software=0x0 LCodeSeg=0x0 DefaultSize=0x1
>SaveCpuS3Data: 363: Idx=0x10 Base=0x Limit=0x Type=0xB
>System=0x1 Dpl=0x0 Present=0x1 Software=0x0 LCodeSeg=0x0 DefaultSize=0x1
>SaveCpuS3Data: 363: Idx=0x18 Base=0x Limit=0x Type=0x2
>System=0x1 Dpl=0x0 Present=0x1 Software=0x0 LCodeSeg=0x0 DefaultSize=0x1
>SaveCpuS3Data: 363: Idx=0x20 Base=0x Limit=0x Type=0xA
>System=0x1 Dpl=0x0 Present=0x1 Software=0x0 LCodeSeg=0x0 DefaultSize=0x1
>SaveCpuS3Data: 363: Idx=0x28 Base=0x Limit=0x Type=0xB
>System=0x1 Dpl=0x0 Present=0x1 Software=0x0 LCodeSeg=0x1 DefaultSize=0x0
>SaveCpuS3Data: 363: Idx=0x30 Base=0x Limit=0x Type=0x0
>System=0x0 Dpl=0x0 Present=0x0 Software=0x0 LCodeSeg=0x0 DefaultSize=0x0
>SaveCpuS3Data: 363: Idx=0x38 Base=0x Limit=0x Type=0x0
>System=0x0 Dpl=0x0 Present=0x0 Software=0x0 LCodeSeg=0x0 DefaultSize=0x0
>
>(Importantly, the debug output is identical between the pure Ia32 build and
>the Ia32X64 build, except the addresses in the first two lines -- the location 
>of
>the GDT descriptor, and the location of the GDT itself.)
>
>
>(2) Now locate the offset (= selector) of the *only* segment that is advertised
>as "64-bit code". The LCodeSeg bit-field is 1 only for Idx=0x28.
>
>
>(3) If you examine "UefiCpuPkg/PiSmmCpuDxeSmm/X64/MpFuncs.asm", you
>can see:
>
>LONG_JUMP::
>
>db 67h,  0EAh ; far jump
>dd 0h ; 32-bit offset
>dw 38h; 16-bit selector
>
>The segment selector is 0x38 here, which not only lacks the LCodeSeg (= long
>mode code) bit, but it is actually a null-descriptor.
>
>So this is the direct cause for the triple fault, but why does it work in the 
>pure
>Ia32 case, and what is the *root* cause for the triple fault in the Ia32X64
>case?
>
>We can investigate some more:
>
>
>(4) The DXE IPL PEIM actually sets up the 0x38 segment correctly. In the
>HandOffToDxeCore() function
>[MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c], branch
>"PcdDxeIplSwitchToLongMode", the last statement is:
>
>//
>// Go to Long Mode and transfer control to DxeCore.
>// Interrupts will not get turned on until the CPU AP is loaded.
>// Call x64 drivers passing in single argument, a pointer to the HOBs.
>//
>AsmEnablePaging64 (
>  

Re: [edk2] [PATCH 6/6] OvmfPkg/PlatformPei: Set PcdCpuMaxLogicalProcessorNumber using QEMU fw_cfg

[Jordan Justen]

On 2015-10-29 04:45:37, Laszlo Ersek wrote:
> On 10/29/15 02:32, Jordan Justen wrote:
> > +ASSERT (MaxProcessors > 0);
> > +PcdSet32 (PcdCpuMaxLogicalProcessorNumber, MaxProcessors);
> 
> I think that when this branch is active, then
> PcdCpuApInitTimeOutInMicroSeconds should *also* be set, namely to
> MAX_UINT32 (~71 minutes, the closest we can get to "infinity"). When
> this hint is available from QEMU, then we should practically disable the
> timeout option in CpuDxe's AP counting.

I think this is a good idea, but I don't think 71 minutes is useful.
Perhaps 30 seconds? This seems more than adequate for hundreds of
processors to startup. Or perhaps some timeout based on the number of
processors?

Janusz and I were discussing
https://github.com/tianocore/edk2/issues/21 on irc. We increased the
timeout to 10 seconds, and with only 8 processors it was still timing
out.

Obviously we are somehow failing to start the processors correctly, or
QEMU/KVM is doing something wrong.

Have you been able to reproduce this issue? It seems like we need to
set the timeout to 71 minutes, and then debug QEMU/KVM to see what
state the APs are in...

Unfortunately I haven't yet been able to reproduce the bug on my
system. :(

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

Re: [edk2] [PATCH 6/6] OvmfPkg/PlatformPei: Set PcdCpuMaxLogicalProcessorNumber using QEMU fw_cfg

[Laszlo Ersek]

On 10/29/15 19:39, Jordan Justen wrote:
> On 2015-10-29 04:45:37, Laszlo Ersek wrote:
>> On 10/29/15 02:32, Jordan Justen wrote:
>>> +ASSERT (MaxProcessors > 0);
>>> +PcdSet32 (PcdCpuMaxLogicalProcessorNumber, MaxProcessors);
>>
>> I think that when this branch is active, then
>> PcdCpuApInitTimeOutInMicroSeconds should *also* be set, namely to
>> MAX_UINT32 (~71 minutes, the closest we can get to "infinity"). When
>> this hint is available from QEMU, then we should practically disable the
>> timeout option in CpuDxe's AP counting.
> 
> I think this is a good idea, but I don't think 71 minutes is useful.
> Perhaps 30 seconds? This seems more than adequate for hundreds of
> processors to startup. Or perhaps some timeout based on the number of
> processors?

No, my suggestion with the 71 minutes didn't aim at a "useful" timeout.
Instead, when QEMU provides the number of VCPUs via fw_cfg, I'd like to
take the timeout *completely* out of the picture. Wait until the
advertised number of VCPUs come up, period. If they don't all appear,
then hang forever. Well, at least for 71 minutes, which is the same for
interactive users.

If 30 seconds elapse and we boot with 1 or 2 VCPUs missing, then things
will break hard. I don't actually *expect* this to occur against a 30
second timeout, but 30 seconds still sends the wrong message to the
programmer and the user. It looks like a real, reasonable timeout. While
in this case, the loop should never exit on a timeout, and 0x
communicates that.

> Janusz and I were discussing
> https://github.com/tianocore/edk2/issues/21 on irc. We increased the
> timeout to 10 seconds, and with only 8 processors it was still timing
> out.

Ugh.

> Obviously we are somehow failing to start the processors correctly, or
> QEMU/KVM is doing something wrong.

I think the actual issue we're fighting here is described in
. Due to the
kernel commit named there, and due to a physical device being assigned
to the guest, guest memory becomes uncacheable for each AP, until the AP
clears CR0.CD. I guess... And that should slow it down extremely.

> Have you been able to reproduce this issue?

I think I have, although I didn't try. :) My current host kernel is
based on v4.3-rc3 (upon which kvm/master is based, upon which I have a
fix), and the commit in question (b18d5431acc7) is part of v4.2-rc1.

If you have a host kernel at least as fresh as v4.2-rc1 (and I do, see
above), then you run into the issue automatically. For which reason I've
been carrying my patch referenced above in my development branches --
I've been focusing on the SMM issues, and solving (or working around)
the MP startup problem is a prerequisite for that.

So, yes, saw it, worked around it immediately, forgot about it. :)

> It seems like we need to
> set the timeout to 71 minutes, and then debug QEMU/KVM to see what
> state the APs are in...

I'm a bit overloaded to tackle this right now, but...

> Unfortunately I haven't yet been able to reproduce the bug on my
> system. :(

if you install a host kernel at least as recent as v4.2-rc1, then the
bug should pop up at once.

Thanks
Laszlo

> 
> -Jordan
> 

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

Re: [edk2] [PATCH] UefiCpuPkg: CpuDxe: Update GDT to be consistent with DxeIplPeim

[Laszlo Ersek]

On 10/29/15 21:35, Michael Kinney wrote:
> The PiSmmCpuDxeSmm module makes some assumptions about GDT selectors
> that are based on the GDT layout from the DxeIplPeim.  This updates the
> CpuDxe module to use the same GDT layout as the DxeIplPeim.
> 
> Using a consistent GDT also improves debug experience.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Michael Kinney 
> ---
>  UefiCpuPkg/CpuDxe/CpuGdt.c | 83 
> ++
>  UefiCpuPkg/CpuDxe/CpuGdt.h | 10 +++---
>  2 files changed, 53 insertions(+), 40 deletions(-)

I'll try to test this and report back, but until then:

- Can you please add a note to the commit message about this bug
  breaking S3 resume? For example:

  [The PiSmmCpuDxeSmm module makes some assumptions about GDT selectors
  that are based on the GDT layout from the DxeIplPeim.] For example,
  the protected mode entry code and (where appropriate) the long mode
  entry code in the UefiCpuPkg/PiSmmCpuDxeSmm/*/MpFuncs.* assembly
  files, which are used during S3 resume, open-code segment selector
  values that depend on DxeIplPeim's GDT layout.

  [This patch updates the CpuDxe module to use the same GDT layout as
  the DxeIplPeim.] This enables modules that are dispatched after
  CpuDxe to find, and potentially save and restore, a GDT layout that
  matches that of DxeIplPeim.

  [Using a consistent GDT also...]

- Can you please add the following tags
  (bragging rights are important! ;))

  Reported-by: Laszlo Ersek 
  Analyzed-by: Laszlo Ersek 
  Link: http://article.gmane.org/gmane.comp.bios.edk2.devel/3568

Thank you,
Laszlo

> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuGdt.c b/UefiCpuPkg/CpuDxe/CpuGdt.c
> index 35a87a6..9ef2fdf 100644
> --- a/UefiCpuPkg/CpuDxe/CpuGdt.c
> +++ b/UefiCpuPkg/CpuDxe/CpuGdt.c
> @@ -1,10 +1,10 @@
>  /** @file
>C based implemention of IA32 interrupt handling only
>requiring a minimal assembly interrupt entry point.
>  
> -  Copyright (c) 2006 - 2010, Intel Corporation. All rights reserved.
> +  Copyright (c) 2006 - 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
>  
> @@ -33,82 +33,93 @@ STATIC GDT_ENTRIES GdtTemplate = {
>},
>//
>// LINEAR_SEL
>//
>{
> -0x0,// limit 0xF
> -0x0,// base 0
> -0x0,
> -0x092,  // present, ring 0, data, expand-up, writable
> +0x0,// limit 15:0
> +0x0,// base 15:0
> +0x0,// base 23:16
> +0x092,  // present, ring 0, data, read/write
>  0x0CF,  // page-granular, 32-bit
>  0x0,
>},
>//
>// LINEAR_CODE_SEL
>//
>{
> -0x0,// limit 0xF
> -0x0,// base 0
> -0x0,
> -0x09A,  // present, ring 0, data, expand-up, writable
> +0x0,// limit 15:0
> +0x0,// base 15:0
> +0x0,// base 23:16
> +0x09F,  // present, ring 0, code, execute/read, conforming, 
> accessed
>  0x0CF,  // page-granular, 32-bit
>  0x0,
>},
>//
>// SYS_DATA_SEL
>//
>{
> -0x0,// limit 0xF
> -0x0,// base 0
> -0x0,
> -0x092,  // present, ring 0, data, expand-up, writable
> +0x0,// limit 15:0
> +0x0,// base 15:0
> +0x0,// base 23:16
> +0x093,  // present, ring 0, data, read/write, accessed
>  0x0CF,  // page-granular, 32-bit
>  0x0,
>},
>//
>// SYS_CODE_SEL
>//
>{
> -0x0,// limit 0xF
> -0x0,// base 0
> -0x0,
> -0x09A,  // present, ring 0, data, expand-up, writable
> +0x0,// limit 15:0
> +0x0,// base 15:0
> +0x0,// base 23:16
> +0x09A,  // present, ring 0, code, execute/read
>  0x0CF,  // page-granular, 32-bit
>  0x0,
>},
>//
> -  // LINEAR_CODE64_SEL
> +  // SPARE4_SEL
>//
>{
> -0x0,// limit 0xF
> -0x0,// base 0
> -0x0,
> -0x09B,  // present, ring 0, code, expand-up, writable
> -0x0AF,  // LimitHigh (CS.L=1, CS.D=0)
> -0x0,// base (high)
> +0x0,// limit 15:0
> +0x0,// base 15:0
> +0x0,// base 23:16
> +0x0,// type
> +0x0,// limit 19:16, flags
> +0x0,// base 31:24
>},
>//
> -  // SPARE4_SEL
> +  // LINEAR_DATA64_SEL
>//
>{
> -0x0,// limit 0
> -0x0,// base 0
> -

[edk2] [PATCH] UefiCpuPkg: CpuDxe: Update GDT to be consistent with DxeIplPeim

[Michael Kinney]

The PiSmmCpuDxeSmm module makes some assumptions about GDT selectors
that are based on the GDT layout from the DxeIplPeim.  This updates the
CpuDxe module to use the same GDT layout as the DxeIplPeim.

Using a consistent GDT also improves debug experience.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Michael Kinney 
---
 UefiCpuPkg/CpuDxe/CpuGdt.c | 83 ++
 UefiCpuPkg/CpuDxe/CpuGdt.h | 10 +++---
 2 files changed, 53 insertions(+), 40 deletions(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuGdt.c b/UefiCpuPkg/CpuDxe/CpuGdt.c
index 35a87a6..9ef2fdf 100644
--- a/UefiCpuPkg/CpuDxe/CpuGdt.c
+++ b/UefiCpuPkg/CpuDxe/CpuGdt.c
@@ -1,10 +1,10 @@
 /** @file
   C based implemention of IA32 interrupt handling only
   requiring a minimal assembly interrupt entry point.
 
-  Copyright (c) 2006 - 2010, Intel Corporation. All rights reserved.
+  Copyright (c) 2006 - 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
 
@@ -33,82 +33,93 @@ STATIC GDT_ENTRIES GdtTemplate = {
   },
   //
   // LINEAR_SEL
   //
   {
-0x0,// limit 0xF
-0x0,// base 0
-0x0,
-0x092,  // present, ring 0, data, expand-up, writable
+0x0,// limit 15:0
+0x0,// base 15:0
+0x0,// base 23:16
+0x092,  // present, ring 0, data, read/write
 0x0CF,  // page-granular, 32-bit
 0x0,
   },
   //
   // LINEAR_CODE_SEL
   //
   {
-0x0,// limit 0xF
-0x0,// base 0
-0x0,
-0x09A,  // present, ring 0, data, expand-up, writable
+0x0,// limit 15:0
+0x0,// base 15:0
+0x0,// base 23:16
+0x09F,  // present, ring 0, code, execute/read, conforming, 
accessed
 0x0CF,  // page-granular, 32-bit
 0x0,
   },
   //
   // SYS_DATA_SEL
   //
   {
-0x0,// limit 0xF
-0x0,// base 0
-0x0,
-0x092,  // present, ring 0, data, expand-up, writable
+0x0,// limit 15:0
+0x0,// base 15:0
+0x0,// base 23:16
+0x093,  // present, ring 0, data, read/write, accessed
 0x0CF,  // page-granular, 32-bit
 0x0,
   },
   //
   // SYS_CODE_SEL
   //
   {
-0x0,// limit 0xF
-0x0,// base 0
-0x0,
-0x09A,  // present, ring 0, data, expand-up, writable
+0x0,// limit 15:0
+0x0,// base 15:0
+0x0,// base 23:16
+0x09A,  // present, ring 0, code, execute/read
 0x0CF,  // page-granular, 32-bit
 0x0,
   },
   //
-  // LINEAR_CODE64_SEL
+  // SPARE4_SEL
   //
   {
-0x0,// limit 0xF
-0x0,// base 0
-0x0,
-0x09B,  // present, ring 0, code, expand-up, writable
-0x0AF,  // LimitHigh (CS.L=1, CS.D=0)
-0x0,// base (high)
+0x0,// limit 15:0
+0x0,// base 15:0
+0x0,// base 23:16
+0x0,// type
+0x0,// limit 19:16, flags
+0x0,// base 31:24
   },
   //
-  // SPARE4_SEL
+  // LINEAR_DATA64_SEL
   //
   {
-0x0,// limit 0
-0x0,// base 0
-0x0,
-0x0,// present, ring 0, data, expand-up, writable
-0x0,// page-granular, 32-bit
+0x0,// limit 15:0
+0x0,// base 15:0
+0x0,// base 23:16
+0x092,  // present, ring 0, data, read/write
+0x0CF,  // page-granular, 32-bit
 0x0,
   },
   //
+  // LINEAR_CODE64_SEL
+  //
+  {
+0x0,// limit 15:0
+0x0,// base 15:0
+0x0,// base 23:16
+0x09A,  // present, ring 0, code, execute/read
+0x0AF,  // page-granular, 64-bit code
+0x0,// base (high)
+  },
+  //
   // SPARE5_SEL
   //
   {
-0x0,// limit 0
-0x0,// base 0
-0x0,
-0x0,// present, ring 0, data, expand-up, writable
-0x0,// page-granular, 32-bit
-0x0,
+0x0,// limit 15:0
+0x0,// base 15:0
+0x0,// base 23:16
+0x0,// type
+0x0,// limit 19:16, flags
+0x0,// base 31:24
   },
 };
 
 /**
   Initialize Global Descriptor Table.
diff --git a/UefiCpuPkg/CpuDxe/CpuGdt.h b/UefiCpuPkg/CpuDxe/CpuGdt.h
index 7ecec5d..2a00751 100644
--- a/UefiCpuPkg/CpuDxe/CpuGdt.h
+++ b/UefiCpuPkg/CpuDxe/CpuGdt.h
@@ -1,10 +1,10 @@
 /** @file
   C based implemention of IA32 interrupt 

Re: [edk2] [PATCH 6/6] OvmfPkg/PlatformPei: Set PcdCpuMaxLogicalProcessorNumber using QEMU fw_cfg

[Laszlo Ersek]

On 10/29/15 22:13, Laszlo Ersek wrote:
> On 10/29/15 19:39, Jordan Justen wrote:
>> On 2015-10-29 04:45:37, Laszlo Ersek wrote:
>>> On 10/29/15 02:32, Jordan Justen wrote:
 +ASSERT (MaxProcessors > 0);
 +PcdSet32 (PcdCpuMaxLogicalProcessorNumber, MaxProcessors);
>>>
>>> I think that when this branch is active, then
>>> PcdCpuApInitTimeOutInMicroSeconds should *also* be set, namely to
>>> MAX_UINT32 (~71 minutes, the closest we can get to "infinity"). When
>>> this hint is available from QEMU, then we should practically disable the
>>> timeout option in CpuDxe's AP counting.
>>
>> I think this is a good idea, but I don't think 71 minutes is useful.
>> Perhaps 30 seconds? This seems more than adequate for hundreds of
>> processors to startup. Or perhaps some timeout based on the number of
>> processors?
> 
> No, my suggestion with the 71 minutes didn't aim at a "useful" timeout.
> Instead, when QEMU provides the number of VCPUs via fw_cfg, I'd like to
> take the timeout *completely* out of the picture. Wait until the
> advertised number of VCPUs come up, period. If they don't all appear,
> then hang forever. Well, at least for 71 minutes, which is the same for
> interactive users.
> 
> If 30 seconds elapse and we boot with 1 or 2 VCPUs missing, then things
> will break hard. I don't actually *expect* this to occur against a 30
> second timeout, but 30 seconds still sends the wrong message to the
> programmer and the user. It looks like a real, reasonable timeout. While
> in this case, the loop should never exit on a timeout, and 0x
> communicates that.
> 
>> Janusz and I were discussing
>> https://github.com/tianocore/edk2/issues/21 on irc. We increased the
>> timeout to 10 seconds, and with only 8 processors it was still timing
>> out.
> 
> Ugh.
> 
>> Obviously we are somehow failing to start the processors correctly, or
>> QEMU/KVM is doing something wrong.
> 
> I think the actual issue we're fighting here is described in
> . Due to the
> kernel commit named there, and due to a physical device being assigned
> to the guest, guest memory becomes uncacheable for each AP, until the AP
> clears CR0.CD. I guess... And that should slow it down extremely.
> 
>> Have you been able to reproduce this issue?
> 
> I think I have, although I didn't try. :) My current host kernel is
> based on v4.3-rc3 (upon which kvm/master is based, upon which I have a
> fix), and the commit in question (b18d5431acc7) is part of v4.2-rc1.
> 
> If you have a host kernel at least as fresh as v4.2-rc1 (and I do, see
> above), then you run into the issue automatically. For which reason I've
> been carrying my patch referenced above in my development branches --
> I've been focusing on the SMM issues, and solving (or working around)
> the MP startup problem is a prerequisite for that.
> 
> So, yes, saw it, worked around it immediately, forgot about it. :)
> 
>> It seems like we need to
>> set the timeout to 71 minutes, and then debug QEMU/KVM to see what
>> state the APs are in...
> 
> I'm a bit overloaded to tackle this right now, but...
> 
>> Unfortunately I haven't yet been able to reproduce the bug on my
>> system. :(
> 
> if you install a host kernel at least as recent as v4.2-rc1, then the
> bug should pop up at once.

Sorry, forgot about a tiny circumstance: you'll also have to assign a
physical device (like a GPU) to your guest. Now *that* is a whole story
per se. See .

In fact, I'm now thinking that I might not have reproduced the issue at
all! (And perhaps I just applied the workaround patch as precaution.) My
workstation does have an assigned GPU, but the kernel on it is older
than v4.2-rc1 -- no issues there. And on my laptop (which has the recent
kernel) no devices can be reasonably assigned to guests. (It has VT-d,
and even the IOMMU groups look good, but the PCI devices are not
"discrete" enough from a guest driver perspective.)

Apologies, I can't help with this right now...

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

Re: [edk2] [PATCH 0/2] Add API to manage Local APIC SoftwareEnable

[Fan, Jeff]

I agree with this fix. 

Reviewed-by: Jeff Fan 

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Michael 
Kinney
Sent: Thursday, October 29, 2015 11:19 AM
To: edk2-devel@lists.01.org
Subject: [edk2] [PATCH 0/2] Add API to manage Local APIC SoftwareEnable

The LocalApicLib does not provide any APIs to manage the Local APIC 
SoftwareEnable state without side effects on other registers.  This feature is 
required by the SourceLevelDebugAgent to make sure the Local APIC is in the 
correct state to use the Local APIC Timer.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Michael Kinney 

Michael Kinney (2):
  UefiCpuPkg: LocalApicLib: Add API to set SoftwareEnable bit
  SourceLevelDebugPkg: DebugAgent: Set Local APIC SoftwareEnable

 .../DebugAgent/DebugAgentCommon/DebugTimer.c   |  1 +
 .../DebugAgent/DxeDebugAgent/DxeDebugAgentLib.c|  1 +
 .../DebugAgent/SmmDebugAgent/SmmDebugAgentLib.c|  1 +
 UefiCpuPkg/Include/Library/LocalApicLib.h  | 16 -
 UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c | 38 +++---
 .../BaseXApicX2ApicLib/BaseXApicX2ApicLib.c| 38 +++---
 6 files changed, 86 insertions(+), 9 deletions(-)

--
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] [PATCH] UefiCpuPkg: CpuDxe: Update GDT to be consistent with DxeIplPeim

[Laszlo Ersek]

comments below

On 10/29/15 21:35, Michael Kinney wrote:
> The PiSmmCpuDxeSmm module makes some assumptions about GDT selectors
> that are based on the GDT layout from the DxeIplPeim.  This updates the
> CpuDxe module to use the same GDT layout as the DxeIplPeim.
> 
> Using a consistent GDT also improves debug experience.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Michael Kinney 
> ---
>  UefiCpuPkg/CpuDxe/CpuGdt.c | 83 
> ++
>  UefiCpuPkg/CpuDxe/CpuGdt.h | 10 +++---
>  2 files changed, 53 insertions(+), 40 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuGdt.c b/UefiCpuPkg/CpuDxe/CpuGdt.c
> index 35a87a6..9ef2fdf 100644
> --- a/UefiCpuPkg/CpuDxe/CpuGdt.c
> +++ b/UefiCpuPkg/CpuDxe/CpuGdt.c
> @@ -1,10 +1,10 @@
>  /** @file
>C based implemention of IA32 interrupt handling only
>requiring a minimal assembly interrupt entry point.
>  
> -  Copyright (c) 2006 - 2010, Intel Corporation. All rights reserved.
> +  Copyright (c) 2006 - 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
>  
> @@ -33,82 +33,93 @@ STATIC GDT_ENTRIES GdtTemplate = {
>},
>//
>// LINEAR_SEL
>//
>{
> -0x0,// limit 0xF
> -0x0,// base 0
> -0x0,
> -0x092,  // present, ring 0, data, expand-up, writable
> +0x0,// limit 15:0
> +0x0,// base 15:0
> +0x0,// base 23:16
> +0x092,  // present, ring 0, data, read/write
>  0x0CF,  // page-granular, 32-bit
>  0x0,
>},
>//
>// LINEAR_CODE_SEL
>//
>{
> -0x0,// limit 0xF
> -0x0,// base 0
> -0x0,
> -0x09A,  // present, ring 0, data, expand-up, writable
> +0x0,// limit 15:0
> +0x0,// base 15:0
> +0x0,// base 23:16
> +0x09F,  // present, ring 0, code, execute/read, conforming, 
> accessed
>  0x0CF,  // page-granular, 32-bit
>  0x0,
>},
>//
>// SYS_DATA_SEL
>//
>{
> -0x0,// limit 0xF
> -0x0,// base 0
> -0x0,
> -0x092,  // present, ring 0, data, expand-up, writable
> +0x0,// limit 15:0
> +0x0,// base 15:0
> +0x0,// base 23:16
> +0x093,  // present, ring 0, data, read/write, accessed
>  0x0CF,  // page-granular, 32-bit
>  0x0,
>},
>//
>// SYS_CODE_SEL
>//
>{
> -0x0,// limit 0xF
> -0x0,// base 0
> -0x0,
> -0x09A,  // present, ring 0, data, expand-up, writable
> +0x0,// limit 15:0
> +0x0,// base 15:0
> +0x0,// base 23:16
> +0x09A,  // present, ring 0, code, execute/read
>  0x0CF,  // page-granular, 32-bit
>  0x0,
>},
>//
> -  // LINEAR_CODE64_SEL
> +  // SPARE4_SEL
>//
>{
> -0x0,// limit 0xF
> -0x0,// base 0
> -0x0,
> -0x09B,  // present, ring 0, code, expand-up, writable
> -0x0AF,  // LimitHigh (CS.L=1, CS.D=0)
> -0x0,// base (high)
> +0x0,// limit 15:0
> +0x0,// base 15:0
> +0x0,// base 23:16
> +0x0,// type
> +0x0,// limit 19:16, flags
> +0x0,// base 31:24
>},
>//
> -  // SPARE4_SEL
> +  // LINEAR_DATA64_SEL
>//
>{
> -0x0,// limit 0
> -0x0,// base 0
> -0x0,
> -0x0,// present, ring 0, data, expand-up, writable
> -0x0,// page-granular, 32-bit
> +0x0,// limit 15:0
> +0x0,// base 15:0
> +0x0,// base 23:16
> +0x092,  // present, ring 0, data, read/write
> +0x0CF,  // page-granular, 32-bit
>  0x0,
>},
>//
> +  // LINEAR_CODE64_SEL
> +  //
> +  {
> +0x0,// limit 15:0
> +0x0,// base 15:0
> +0x0,// base 23:16
> +0x09A,  // present, ring 0, code, execute/read
> +0x0AF,  // page-granular, 64-bit code
> +0x0,// base (high)
> +  },
> +  //
>// SPARE5_SEL
>//
>{
> -0x0,// limit 0
> -0x0,// base 0
> -0x0,
> -0x0,// present, ring 0, data, expand-up, writable
> -0x0,// page-granular, 32-bit
> -0x0,
> +0x0,// limit 15:0
> +0x0,// base 15:0
> +0x0,// base 23:16
> +0x0,// type
> 

Re: [edk2] [Patch] UefiCpuPkg/PiSmmCpuDxeSmm: Shouldn't use gSmst->CurrentlyExecutingCpu

[Fan, Jeff]

Laszlo,

Thanks your suggestion. I will add the code in commit log.

Jeff

-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com] 
Sent: Thursday, October 29, 2015 10:43 PM
To: Fan, Jeff; edk2-de...@ml01.01.org
Cc: Kinney, Michael D
Subject: Re: [edk2] [Patch] UefiCpuPkg/PiSmmCpuDxeSmm: Shouldn't use 
gSmst->CurrentlyExecutingCpu

On 10/29/15 14:17, Fan, Jeff wrote:
> Laszlo,
> 
> SmmCoreEntry() is from the following code, and it is just PiSmmCore's 
> SmmEntryPoint(). 
>   //
>   // Invoke SMM Foundation EntryPoint with the processor information context.
>   //
>   gSmmCpuPrivate->SmmCoreEntry (>SmmCoreEntryContext);
> 
> Yes. gSmst->CurrentlyExecutingCpu is not set in PiSmmCpuDxeSmm driver. It is 
> set in PiSmmCore driver by the following code.
>   //
>   // Update SMST using the context
>   //
>   CopyMem (, SmmEntryContext, sizeof 
> (EFI_SMM_ENTRY_CONTEXT));

I don't understand how that CopyMem()  call is supposed to set 
gSmst->CurrentlyExecutingCpu.

- gSmst points to an EFI_SMM_SYSTEM_TABLE2 object.
- gSmmCoreSmst is an object of the same type.

In the EFI_SMM_SYSTEM_TABLE2 structure, "SmmStartupThisAp" and 
"CurrentlyExecutingCpu" are *sibling* fields. Neither of both is contained in 
the other.

Therefore I don't understand how the above CopyMem() can set the 
CurrentlyExecutingCpu field.

... OMG... The CopyMem() actually *overflows* the SmmStartupThisAp field. That 
field is only used as a start offset into gSmmCoreSmst. EFI_SMM_ENTRY_CONTEXT 
contains a *subset* of the EFI_SMM_SYSTEM_TABLE2 fields, in the same order, and 
with the same sizes:
- SmmStartupThisAp
- CurrentlyExecutingCpu
- NumberOfCpus
- CpuSaveStateSize
- ...

Which is why the CopyMem() works.

I'm sorry to say, but  I think this is one of the ugliest pieces of code I've 
seen in my life. :(

Anyway, what we have now is the following order:

  BSPHandler()
gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu = CpuIndex;

gSmmCpuPrivate->SmmCoreEntry (>SmmCoreEntryContext);

  CopyMem (, SmmEntryContext, sizeof 
(EFI_SMM_ENTRY_CONTEXT));

PerformRemainingTasks()  [UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c]
  ConfigSmmCodeAccessCheck()
... gSmst->CurrentlyExecutingCpu ...

So, I don't understand how the bug is possible. In the commit message you say, 
"ConfigSmmCodeAccessCheck() maybe invoked before executing SmmCoreEntry()". How?

Ah! Now I understand, there is another ConfigSmmCodeAccessCheck() call in 
BSPHandler(), *before* SmmCoreEntry. It depends on 
"mRestoreSmmConfigurationInS3".

Can you please add the following to the commit message:

---
  BSPHandler()
gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu =
  CpuIndex;

//
// when mRestoreSmmConfigurationInS3 is set:
//
ConfigSmmCodeAccessCheck()
  //
  // reads gSmst->CurrentlyExecutingCpu to early
  //

gSmmCpuPrivate->SmmCoreEntry (
  >SmmCoreEntryContext)
  //
  // sets gSmst->CurrentlyExecutingCpu with CopyMem() too late
  //
  CopyMem (,
SmmEntryContext, sizeof (EFI_SMM_ENTRY_CONTEXT));
---

With that change:

Reviewed-by: Laszlo Ersek 

Thanks
Laszlo


> 
> Thanks!
> Jeff
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Thursday, October 29, 2015 8:46 PM
> To: Fan, Jeff; edk2-de...@ml01.01.org
> Cc: Kinney, Michael D
> Subject: Re: [edk2] [Patch] UefiCpuPkg/PiSmmCpuDxeSmm: Shouldn't use 
> gSmst->CurrentlyExecutingCpu
> 
> On 10/29/15 09:11, Jeff Fan wrote:
>> In ConfigSmmCodeAccessCheck(), we used gSmst->CurrentlyExecutingCpu 
>> to get the current SMM BSP. But ConfigSmmCodeAccessCheck() maybe 
>> invoked before executing
>> SmmCoreEntry() and gSmst->CurrentlyExecutingCpu hasn't been updated 
>> to the latest value. Instead, we should use
>> gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu directly.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Jeff Fan 
>> Cc: Michael Kinney 
>> Cc: Laszlo Ersek 
>> ---
>>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> index e210c8d..c351875 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> @@ -1386,7 +1386,7 @@ ConfigSmmCodeAccessCheck (
>>//
>>// Check to see if the Feature Control MSR is supported on this CPU
>>//
>> -  Index = gSmst->CurrentlyExecutingCpu;
>> +  Index = gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu;
>>if (!SmmCpuFeaturesIsSmmRegisterSupported (Index, SmmRegFeatureControl)) {
>>  mSmmCodeAccessCheckEnable = FALSE;
>>  return;
>> @@ -1428,7 +1428,7 @@ ConfigSmmCodeAccessCheck (
>>// Enable SMM Code Access Check feature for the APs.
>> 

[edk2] [Patch] UefiCpuPkg/PiSmmCpuDxeSmm: Shouldn't use gSmst->CurrentlyExecutingCpu

[Jeff Fan]

In ConfigSmmCodeAccessCheck(), we used gSmst->CurrentlyExecutingCpu to get the
current SMM BSP. But ConfigSmmCodeAccessCheck() maybe invoked before executing
SmmCoreEntry() and gSmst->CurrentlyExecutingCpu hasn't been updated to the
latest value. Instead, we should use
gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu directly.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Fan 
Cc: Michael Kinney 
Cc: Laszlo Ersek 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
index e210c8d..c351875 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -1386,7 +1386,7 @@ ConfigSmmCodeAccessCheck (
   //
   // Check to see if the Feature Control MSR is supported on this CPU
   //
-  Index = gSmst->CurrentlyExecutingCpu;
+  Index = gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu;
   if (!SmmCpuFeaturesIsSmmRegisterSupported (Index, SmmRegFeatureControl)) {
 mSmmCodeAccessCheckEnable = FALSE;
 return;
@@ -1428,7 +1428,7 @@ ConfigSmmCodeAccessCheck (
   // Enable SMM Code Access Check feature for the APs.
   //
   for (Index = 0; Index < gSmst->NumberOfCpus; Index++) {
-if (Index != gSmst->CurrentlyExecutingCpu) {
+if (Index != gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu) {
 
   //
   // Acquire Config SMM Code Access Check spin lock.  The AP will release 
the
-- 
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 1/6] UefiCpuPkg/CpuDxe: Don't use gBS->Stall

[Fan, Jeff]

It's good!

Reviewed-by: Jeff Fan 

-Original Message-
From: Justen, Jordan L 
Sent: Thursday, October 29, 2015 9:33 AM
To: edk2-devel@lists.01.org
Cc: Justen, Jordan L; Fan, Jeff; Laszlo Ersek
Subject: [PATCH 1/6] UefiCpuPkg/CpuDxe: Don't use gBS->Stall

The CpuDxe driver may run before the gEfiMetronomeArchProtocolGuid protocol is 
installed. gBS->Stall does not work until this arch protocol is installed.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jordan Justen 
Cc: Jeff Fan 
Cc: Laszlo Ersek 
---
 UefiCpuPkg/CpuDxe/CpuMp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c index 
04c2f1f..98fdfdf 100644
--- a/UefiCpuPkg/CpuDxe/CpuMp.c
+++ b/UefiCpuPkg/CpuDxe/CpuMp.c
@@ -801,7 +801,7 @@ StartupAllAPs (
   goto Done;
 }
 
-gBS->Stall (gPollInterval);
+MicroSecondDelay (gPollInterval);
 mMpSystemData.Timeout -= gPollInterval;
   }
 
@@ -987,7 +987,7 @@ StartupThisAP (
   return EFI_TIMEOUT;
 }
 
-gBS->Stall (gPollInterval);
+MicroSecondDelay (gPollInterval);
 CpuData->Timeout -= gPollInterval;
   }
 
@@ -1755,7 +1755,7 @@ InitializeMpSupport (
 if (CheckAllAPsSleeping ()) {
   break;
 }
-gBS->Stall (gPollInterval);
+MicroSecondDelay (gPollInterval);
 Timeout += gPollInterval;
   } while (Timeout <= PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds));
   ASSERT (Timeout <= PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds));
--
2.5.1

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

Re: [edk2] [PATCH 2/6] UefiCpuPkg/CpuDxe: Ignore extra APs in the system

[Fan, Jeff]

Jordan,

I think we could ASSERT() if the actually processors number is larger than 
maximum number supported in system. 

Otherwise, send broadcast SMI or IPI may break the system.

Jeff
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jordan 
Justen
Sent: Thursday, October 29, 2015 9:33 AM
To: edk2-devel@lists.01.org
Cc: Justen, Jordan L; Laszlo Ersek; Fan, Jeff
Subject: [edk2] [PATCH 2/6] UefiCpuPkg/CpuDxe: Ignore extra APs in the system

The PcdCpuMaxLogicalProcessorNumber specifies the maximum number of logical 
processors which are expected to be seen by the system. If more APs actually 
are available in the system, we should prevent them from being used.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jordan Justen 
Cc: Jeff Fan 
Cc: Laszlo Ersek 
---
 UefiCpuPkg/CpuDxe/CpuMp.c | 22 +-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c index 
98fdfdf..e80835f 100644
--- a/UefiCpuPkg/CpuDxe/CpuMp.c
+++ b/UefiCpuPkg/CpuDxe/CpuMp.c
@@ -1196,6 +1196,17 @@ WhoAmI (
 }
   }
 
+  if (Index >= mMpSystemData.NumberOfProcessors) {
+//
+// This is not a valid error for the WhoAmI function, but it should never
+// happen from outside the driver. It could only happen if more APs
+// started than the PcdCpuMaxLogicalProcessorNumber was set to. This call
+// would come from ApEntryPointInC, and we use this error to prevent the
+// AP from being used by MP services.
+//
+return EFI_DEVICE_ERROR;
+  }
+
   *ProcessorNumber = Index;
   return EFI_SUCCESS;
 }
@@ -1446,10 +1457,15 @@ ApEntryPointInC (
   VOID
   )
 {
+  EFI_STATUS  Status;
   VOID*   TopOfApStack;
   UINTN   ProcessorNumber;
 
   if (!mAPsAlreadyInitFinished) {
+if (mMpSystemData.NumberOfProcessors >= gMaxLogicalProcessorNumber) {
+  return;
+}
+
 FillInProcessorInformation (FALSE, mMpSystemData.NumberOfProcessors);
 TopOfApStack  = (UINT8*)mApStackStart + gApStackSize;
 mApStackStart = TopOfApStack;
@@ -1461,7 +1477,11 @@ ApEntryPointInC (
 mMpSystemData.NumberOfProcessors++;
 mMpSystemData.NumberOfEnabledProcessors++;
   } else {
-WhoAmI (, );
+Status = WhoAmI (, );
+if (EFI_ERROR (Status)) {
+  return;
+}
+
 //
 // Get the original stack address.
 //
--
2.5.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] BaseTools/GenFw: add option to retain image version info

[Gao, Liming]

Carl:
  Could you let me how you uses the version information in application .EFI 
file?

Thanks
Liming
From: Miller, Carl H [mailto:carl.mil...@pnnl.gov]
Sent: Wednesday, October 28, 2015 10:32 PM
To: edk2-devel@lists.01.org
Cc: Zhu, Yonghong; Gao, Liming
Subject: [PATCH] BaseTools/GenFw: add option to retain image version info


add a command line option("-leaveversion") to GenFw to retain the version 
information in application .EFI files.



Cc: M: Yonghong Zhu >

   M: Liming Gao >

Contributed-under: TianoCore Contribution Agreement 1.0

Signed-off-by: Thompson, James J 
>

---
diff --git a/BaseTools/Source/C/GenFw/GenFw.c b/BaseTools/Source/C/GenFw/GenFw.c
index 4756c52..eea131f 100644
--- a/BaseTools/Source/C/GenFw/GenFw.c
+++ b/BaseTools/Source/C/GenFw/GenFw.c
@@ -1078,6 +1078,7 @@ Returns:
   UNWIND_INFO  *UnwindInfo;
   STATUS   Status;
   BOOLEAN  ReplaceFlag;
+  BOOLEAN  LeaveVersionFlag;
   BOOLEAN  KeepExceptionTableFlag;
   BOOLEAN  KeepZeroPendingFlag;
   UINT64   LogLevel;
@@ -1140,6 +1141,7 @@ Returns:
   InputFileLength   = 0;
   Optional32= NULL;
   Optional64= NULL;
+  LeaveVersionFlag  = FALSE;
   KeepExceptionTableFlag = FALSE;
   KeepZeroPendingFlag= FALSE;
   NumberOfFormPacakge= 0;
@@ -1262,7 +1264,14 @@ Returns:
   continue;
 }

-if (stricmp (argv[0], "--keepexceptiontable") == 0) {
+ if (stricmp(argv[0], "--leaveversion") == 0) {
+ LeaveVersionFlag = TRUE;
+ argc--;
+ argv++;
+ continue;
+ }
+
+ if (stricmp(argv[0], "--keepexceptiontable") == 0) {
   KeepExceptionTableFlag = TRUE;
   argc --;
   argv ++;
@@ -2397,8 +2406,10 @@ Returns:
 Optional64 = (EFI_IMAGE_OPTIONAL_HEADER64 *)>Pe32.OptionalHeader;
 Optional64->MajorOperatingSystemVersion = 0;
 Optional64->MinorOperatingSystemVersion = 0;
-Optional64->MajorImageVersion   = 0;
-Optional64->MinorImageVersion   = 0;
+ if (!LeaveVersionFlag) {
+ Optional64->MajorImageVersion = 0;
+ Optional64->MinorImageVersion = 0;
+ }
 Optional64->MajorSubsystemVersion   = 0;
 Optional64->MinorSubsystemVersion   = 0;
 Optional64->Win32VersionValue   = 0;
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] PATCH BaseTools/toolsetup.bat : enquote PATH reassignment

[Zhu, Yonghong]

Hi Carl,

May I know the detail error we would meet if we don't add the quotes ?

Best Regards,

Zhu Yonghong

From: Miller, Carl H [mailto:carl.mil...@pnnl.gov]
Sent: Wednesday, October 28, 2015 11:03 PM
To: edk2-devel@lists.01.org
Cc: Zhu, Yonghong; Gao, Liming
Subject: PATCH BaseTools/toolsetup.bat : enquote PATH reassignment

add quotes around updating of the PATH env. var. to avoid errors when existing 
path contains spaces.

Cc: M: Yonghong Zhu >
   M: Liming Gao >
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Thompson, James J 
>
diff --git a/BaseTools/toolsetup.bat b/BaseTools/toolsetup.bat
index 59874c5..e90e28e 100755
--- a/BaseTools/toolsetup.bat
+++ b/BaseTools/toolsetup.bat
@@ -322,10 +322,10 @@ goto end
   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
+ 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
 )
   )
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] about the SMM_S3_RESUME_SMM_64 branch in S3Resume2Pei

[Laszlo Ersek]

On 10/29/15 03:28, Yao, Jiewen wrote:
> Thanks for the info. I think we might have to dig out why LONG_JUMP fail here.
> Most our IA BIOS support Ia32X64 mode, so I am sure it should work.

Right. I'll ask Paolo for help when he's back from his trip.

> 
> For "RSM from 64-bit mode ", I have already confirmed with IA32 SDM owner 
> that it is just typo.
> RSM should work in 64bit mode, so this patch is unnecessary, I believe.

Indeed.

> For debug purpose, you can apply it at first in your branch, just in case it 
> is emulator error.

That's the case; it should become unnecessary on v4.4+ Linux hosts.

> 
> Have a good night!

Thanks!
Laszlo

> 
> Thank you
> Yao Jiewen
> 
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
> Ersek
> Sent: Thursday, October 29, 2015 9:50 AM
> To: Yao, Jiewen; Kinney, Michael D; Fan, Jeff
> Cc: edk2-devel-01
> Subject: Re: [edk2] about the SMM_S3_RESUME_SMM_64 branch in S3Resume2Pei
> 
> On 10/29/15 02:32, Laszlo Ersek wrote:
>> On 10/29/15 01:31, Yao, Jiewen wrote:
>>> Hi Ersek
>>> I think S3ResumePei supports Ia32 and Ia32X64. It does not support X64. So 
>>> I believe Ia32X64 crash is a bug somewhere.
>>>
>>> Since you already run into SmmRestoreCpu(), would you please help to check 
>>> where is last instruction causing crash?
>>
>> Sure. The crash occurs on the following call path (starting with 
>> SmmRestoreCpu()):
>>
>> SmmRestoreCpu()
>> [UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c]
>>   EarlyInitializeCpu() [UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c]
>> SendInitSipiSipiAllExcludingSelf() 
>> [UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c]
>>
>> I have a debug print right after the
>> SendInitSipiSipiAllExcludingSelf(), and the BSP starts printing it, 
>> but before the message is completely printed, one of the APs (just
>> started) seems to crash the VM.
>>
>> It is perfectly possible that the ACPI_CPU_DATA structure that 
>> OvmfPkg/QuarkPort/CpuS3DataDxe collects earlier causes this. Because, 
>> before EarlyInitializeCpu() calls SendInitSipiSipiAllExcludingSelf(),
>> the startup vector is prepared from ACPI_CPU_DATA, in the
>> PrepareApStartupVector() function. No clue what goes wrong there.
>>
>> Let me check the KVM trace though... Okay, I massaged it a little bit 
>> for easier readability. Here's when VCPU#0 runs
>> SendInitSipiSipiAllExcludingSelf():
>>
>>> kvm_exit: reason APIC_ACCESS rip 0x7ffc9d7f info 1300 0
>>> kvm_emulate_insn: 0:7ffc9d7f: 89 10
>>> kvm_mmio: mmio write len 4 gpa 0xfee00300 val 0xc4697
>>> kvm_apic: apic_write APIC_ICR = 0xc4697
>>> kvm_apic_ipi: dst 0 vec 151 (SIPI|physical|assert|edge|all-but-self)
>>> kvm_apic_accept_irq:  apicid 1 vec 151 (SIPI|edge)
>>> kvm_apic_accept_irq:  apicid 2 vec 151 (SIPI|edge)
>>> kvm_apic_accept_irq:  apicid 3 vec 151 (SIPI|edge)
>>> kvm_entry:vcpu 0
>>
>>> kvm_exit: reason APIC_ACCESS rip 0x7ffc9d06 info 300 0
>>> kvm_emulate_insn: 0:7ffc9d06: 8b 00
>>> kvm_apic: apic_read APIC_ICR = 0xc4697
>>> kvm_mmio: mmio read len 4 gpa 0xfee00300 val 0xc4697
>>> kvm_entry:vcpu 0
>>
>>> kvm_exit: reason APIC_ACCESS rip 0x7ffc9d7f info 1310 0
>>> kvm_emulate_insn: 0:7ffc9d7f: 89 10
>>> kvm_mmio: mmio write len 4 gpa 0xfee00310 val 0x0
>>> kvm_apic: apic_write APIC_ICR2 = 0x0
>>> kvm_entry:vcpu 0
>>
>> And in response, this is how VCPU#1 behaves (I guess this matches 
>> "UefiCpuPkg/PiSmmCpuDxeSmm/X64/MpFuncs.asm"?):
>>
>>> kvm_exit: reason CR_ACCESS rip 0x35 info 0 0
>>> kvm_cr:   cr_write 0 = 0x6011
>>> kvm_entry:vcpu 1
>>
>>> kvm_exit: reason CR_ACCESS rip 0x9705b info 4 0
>>> kvm_cr:   cr_write 4 = 0x20
>>> kvm_entry:vcpu 1
>>
>>> kvm_exit: reason CR_ACCESS rip 0x9705e info 103 0
>>> kvm_cr:   cr_write 3 = 0x7ff83000
>>> kvm_entry:vcpu 1
>>
>>> kvm_exit: reason MSR_READ rip 0x97068 info 0 0
>>> kvm_msr:  msr_read c080 = 0x0
>>> kvm_entry:vcpu 1
>>
>>> kvm_exit: reason MSR_WRITE rip 0x9706e info 0 0
>>> kvm_msr:  msr_write c080 = 0x100
>>> kvm_entry:vcpu 1
>>
>>> kvm_exit: reason CR_ACCESS rip 0x97077 info 0 0
>>> kvm_cr:   cr_write 0 = 0xe011
>>> kvm_entry:vcpu 1
>>
>>> kvm_exit: reason TRIPLE_FAULT rip 0x9707a info 0 0
>>> kvm_userspace_exit:   reason KVM_EXIT_SHUTDOWN (8)
>>
>> From counting the bytes in "MpFuncs.asm", and correlating them with 
>> the RIP values in the trace, I *think* that the triple fault happens 
>> right here:
>>
>> LONG_JUMP::
>>
>> db 67h,  0EAh ; far jump
>> dd 0h ; 32-bit offset
>> dw 38h; 

Re: [edk2] [PATCH] OvmfPkg: increase MP services startup timeout

[Laszlo Ersek]

On 10/20/15 21:27, Laszlo Ersek wrote:
> Due to Linux kernel commit b18d5431acc7 ("KVM: x86: fix CR0.CD
> virtualization"), vCPUs need more time to start up, because:
> 
> - KVM now zaps all the mappings for the guest memory in EPT or shadow page
>   table, hence more VM exits are required to rebuild the mappings for all
>   memory accesses.
> 
> - If a physical device has been assigned to the guest, and the IOMMU lacks
>   the snoop control feature, guest memory will become uncacheable after
>   CR0.CD is set to 1.
> 
> UefiCpuPkg/UefiCpuPkg.dec sets the timeout to 50ms; startup failures with
> 100ms have been reported. Xiao Guangrong suggested 1s, and helped word the
> commit message.
> 
> Cc: Xiao Guangrong 
> Cc: Jordan Justen 
> Cc: Janusz Mocek 
> Cc: Alex Williamson 
> Reported-by: Janusz Mocek 
> Suggested-by: Xiao Guangrong 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek 
> ---
>  OvmfPkg/OvmfPkgIa32.dsc| 6 ++
>  OvmfPkg/OvmfPkgIa32X64.dsc | 8 +++-
>  OvmfPkg/OvmfPkgX64.dsc | 6 ++
>  3 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 0d044c2..670a80f 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -358,6 +358,12 @@ [PcdsFixedAtBuild]
>gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>  !endif
>  
> +  # Initial AP detection may take a long time on KVM; dependent on device
> +  # assignment, IOMMU features, VCPU count, and more. Allow a generous 
> interval
> +  # for the MP services initialization in UefiCpuPkg/CpuDxe to count the APs,
> +  # and to wait until they become idle again.
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|100
> +
>  !ifndef $(USE_OLD_SHELL)
>gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdShellFile|{ 0x83, 0xA5, 0x04, 
> 0x7C, 0x3E, 0x9E, 0x1C, 0x4F, 0xAD, 0x65, 0xE0, 0x52, 0x68, 0xD0, 0xB4, 0xD1 }
>  !endif
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 19d2221..8c00096 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -51,7 +51,7 @@ [BuildOptions]
>  
>  [BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]
>GCC:*_*_*_DLINK_FLAGS = -z common-page-size=0x1000
> -  
> +
>  
> 
>  #
>  # SKU Identification section - list of all SKU IDs supported by this 
> Platform.
> @@ -363,6 +363,12 @@ [PcdsFixedAtBuild]
>gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>  !endif
>  
> +  # Initial AP detection may take a long time on KVM; dependent on device
> +  # assignment, IOMMU features, VCPU count, and more. Allow a generous 
> interval
> +  # for the MP services initialization in UefiCpuPkg/CpuDxe to count the APs,
> +  # and to wait until they become idle again.
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|100
> +
>  !ifndef $(USE_OLD_SHELL)
>gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdShellFile|{ 0x83, 0xA5, 0x04, 
> 0x7C, 0x3E, 0x9E, 0x1C, 0x4F, 0xAD, 0x65, 0xE0, 0x52, 0x68, 0xD0, 0xB4, 0xD1 }
>  !endif
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index b8df1dc..eb4da4f 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -363,6 +363,12 @@ [PcdsFixedAtBuild]
>gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>  !endif
>  
> +  # Initial AP detection may take a long time on KVM; dependent on device
> +  # assignment, IOMMU features, VCPU count, and more. Allow a generous 
> interval
> +  # for the MP services initialization in UefiCpuPkg/CpuDxe to count the APs,
> +  # and to wait until they become idle again.
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|100
> +
>  !ifndef $(USE_OLD_SHELL)
>gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdShellFile|{ 0x83, 0xA5, 0x04, 
> 0x7C, 0x3E, 0x9E, 0x1C, 0x4F, 0xAD, 0x65, 0xE0, 0x52, 0x68, 0xD0, 0xB4, 0xD1 }
>  !endif
> 

Withdrawing this; it's superseded by Jordan's

[edk2] [PATCH 0/6] UefiCpuPkg/CpuDxe MP Startup Fixes / Optimizations
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug

[Ni, Ruiyu]

Leif, Feng,
Ok I will revert the two patches I committed (big patch + small patch). But I 
want to clarify one thing that not every big patch can be easily understood by 
just splitting to small patches.

Thanks,
Ray

-Original Message-
From: Tian, Feng 
Sent: Friday, October 30, 2015 9:57 AM
To: Leif Lindholm 
Cc: Ni, Ruiyu ; Kinney, Michael D 
; edk2-devel@lists.01.org; Fan, Jeff 
; Tian, Feng 
Subject: RE: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug

Leif,

Thanks for raising this issue.

I agree with you that the patch should be split to small ones and make it more 
readable. I also agree we should give community more time to review those big 
patch before getting committed in.

Ray, who is the module owner, will follow up your suggestions.

Thanks
Feng

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Leif 
Lindholm
Sent: Thursday, October 29, 2015 19:55
To: Tian, Feng
Cc: Ni, Ruiyu; Kinney, Michael D; edk2-devel@lists.01.org; Fan, Jeff
Subject: Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug

Hi Feng,

On Thu, Oct 29, 2015 at 12:23:04AM +, Tian, Feng wrote:
> Ray has sent the patch for review, you may ignore it. 
> (http://article.gmane.org/gmane.comp.bios.edk2.devel/3484)

Yes, I see it has now been committed - which solves the known bug.

But this still leaves us with a clearly not sufficiently reviewed/tested 
invasive patch in core PCI code, and a jumbled commit history.

Since I have not received any response when asking previously, I will ask 
again: Can we please revert SVN r18658 and introduce this changeset in a more 
reviewable form, with proper review/testing?

Regards,

Leif

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> Leif Lindholm
> Sent: Wednesday, October 28, 2015 22:51
> To: Tian, Feng
> Cc: Ni, Ruiyu; edk2-devel@lists.01.org; Fan, Jeff
> Subject: Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug
> 
> Feng,
> 
> Any update on the below?
> The hard crash bug is still in SVN r18690.
> 
> Regards,
> 
> Leif
> 
> On Mon, Oct 26, 2015 at 03:35:15PM +, Leif Lindholm wrote:
> > Hi Ray,
> > 
> > On Mon, Oct 26, 2015 at 03:17:22PM +, Ni, Ruiyu wrote:
> > > Can you add a null pointer check to
> > > PciIoDevice->ResourcePaddingDescriptors before calling
> > > DumpPpbPaddingResource? Does it help?
> > 
> > It removes the delay and the crash - thanks!
> > 
> > But it does nothing for the commit history ;)
> > 
> > Regards,
> > 
> > Leif
> > 
> > > > 在 2015年10月26日，21:13，Leif Lindholm  写道：
> > > > 
> > > > Hi Ruiyu, Feng,
> > > > 
> > > > I am currently tracking down an issue on (at least) one of my 
> > > > platforms - that happens with this (now committed) patch, but 
> > > > not without it.
> > > > 
> > > > Symptoms are a _long_ delay, followed by an unaligned access in 
> > > > (I
> > > > think) DumpPpbPaddingResource.
> > > > 
> > > > Anyway, there could be other things playing in here - I'm 
> > > > testing a new card in a platform I haven't previously tested cards.
> > > > 
> > > > But this is one large patch, which could have been split up into 
> > > > multiple ones to make the introduced changes more reviewable:
> > > > - There are both functional changes and whitespace fixups.
> > > > - There are (text-only) changes to existing messages.
> > > > - There is refactoring of internal APIs.
> > > > 
> > > > It is certainly too invasive a change to be committed ~32 
> > > > minutes after having first been posted to the list.
> > > > 
> > > > Any chance we can revert this one and introduce it in smaller 
> > > > portions, making the individual change sets more reviewable?
> > > > 
> > > > Regards,
> > > > 
> > > > Leif
> > > > 
> > > >> On Fri, Oct 23, 2015 at 03:57:40PM +0800, Ruiyu Ni wrote:
> > > >> For a hot plug bridge with device attached, PciBusDxe driver 
> > > >> reserves the resources which equal to the total amount of 
> > > >> padding resource returned from HotPlug->GetResourcePadding() 
> > > >> and the actual occupied resource by the attached device. The behavior 
> > > >> is incorrect.
> > > >> Correct behavior is to reserve the bigger one between the 
> > > >> padding resource and the actual occupied resource.
> > > >> 
> > > >> Contributed-under: TianoCore Contribution Agreement 1.0
> > > >> Signed-off-by: Ruiyu Ni 
> > > >> Cc: Jeff Fan 
> > > >> Cc: Feng Tian 
> > > >> ---
> > > >> .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.c   |  76 +-
> > > >> .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.h   |  13 ++
> > > >> MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c| 159 
> > > >> ++---
> > > >> .../Bus/Pci/PciBusDxe/PciResourceSupport.c |  58 +---
> > > >> 4 files 

Re: [edk2] [PATCH] MdeModulePkg: Add a BDS platform hook function PlatformBootManagerDefaultBootFail to PlatformBootManagerLib

[Wang, Sunny (HPS SW)]

Hi Ray,
Thanks for the information.
However, the Status codes you mentioned in EfiBootManagerBoot() is for 
indicating booting a specific boot option failure rather than booting ALL boot 
options failure, so I can't use these Status codes for my purpose. The 
following two are detailed reasons:
-  I can't figure out how to know that all the boot options are booted in the 
Status code handler which is triggered by the REPORT_STATUS_CODE in 
EfiBootManagerBoot()
-  If there is no boot option in the boot order, this status code will not work 
either because BDS code doesn't call EfiBootManagerBoot().

Regards,
Sunny Wang

-Original Message-
From: Ni, Ruiyu [mailto:ruiyu...@intel.com] 
Sent: Thursday, October 29, 2015 1:07 PM
To: El-Haj-Mahmoud, Samer
Cc: Wang, Sunny (HPS SW); edk2-devel@lists.01.org
Subject: RE: [edk2] [PATCH] MdeModulePkg: Add a BDS platform hook function 
PlatformBootManagerDefaultBootFail to PlatformBootManagerLib

Samer,
EfiBootManagerBoot() function reports the failure to load boot option and the 
failure to start boot option through status code.

//
// Report Status Code to indicate that the failure to load boot option // 
REPORT_STATUS_CODE (
  EFI_ERROR_CODE | EFI_ERROR_MINOR,
  (EFI_SOFTWARE_DXE_BS_DRIVER | EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR)
  );
...

//
// Report Status Code to indicate that boot failure // REPORT_STATUS_CODE (
  EFI_ERROR_CODE | EFI_ERROR_MINOR,
  (EFI_SOFTWARE_DXE_BS_DRIVER | EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED)
  );

Platform can hook the above two status code so that the 
PlatformBootManagerDefaultBootFail() can be avoided.

We are working on the Platform Recovery feature right now. But not sure whether 
it can be done by the end of this year.
Some spec clarification/discussion are on-going.

Thanks,
Ray

-Original Message-
From: El-Haj-Mahmoud, Samer [mailto:samer.el-haj-mahm...@hpe.com]
Sent: Wednesday, October 28, 2015 10:49 PM
To: Ni, Ruiyu 
Cc: Wang, Sunny (HPS SW) ; edk2-devel@lists.01.org; 
El-Haj-Mahmoud, Samer 
Subject: RE: [edk2] [PATCH] MdeModulePkg: Add a BDS platform hook function 
PlatformBootManagerDefaultBootFail to PlatformBootManagerLib

Ray,

The use case is in taking some platform specific action when all boot options 
have failed. I guess this is more in line with UEFI 2.5 Platform Recovery 
(section 3.4.3). Since EDK2 has not implemented that yet, and there is no 
processing of PlatformRecovery variables, a hook function seems to solve 
the issue for now.

I guess a Status Code could solve this as well, if you are OK with introducting 
a new StatusCode in BootAllBootOptions() or DefaultBootBehavior(). If so, Sunny 
can update the patch with this proposal.

On a separate note, what is the plan for implementing UEFI 2.5 Platform 
Recovery in EDK2?

Thanks,
--Samer






-Original Message-
From: Ni, Ruiyu [mailto:ruiyu...@intel.com]
Sent: Wednesday, October 28, 2015 8:18 AM
To: El-Haj-Mahmoud, Samer 
Cc: Wang, Sunny (HPS SW) ; edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH] MdeModulePkg: Add a BDS platform hook function 
PlatformBootManagerDefaultBootFail to PlatformBootManagerLib

Do you have real requirement about the boot failure notify?
Have you considered to use report status handler?

Thanks,
Ray

> 在 2015年10月27日，23:18，El-Haj-Mahmoud, Samer 
>  写道：
> 
> Reviewed-by: Samer El-Haj-Mahmoud 
> 
> -Original Message-
> From: Wang, Sunny (HPS SW)
> Sent: Tuesday, October 27, 2015 4:21 AM
> To: edk2-devel@lists.01.org
> Cc: El-Haj-Mahmoud, Samer ; Wang, Sunny 
> (HPS SW) 
> Subject: [PATCH] MdeModulePkg: Add a BDS platform hook function 
> PlatformBootManagerDefaultBootFail to PlatformBootManagerLib
> 
> Add a BDS platform hook function PlatformBootManagerDefaultBootFail to 
> PlatformBootManagerLib
> 
> Why we need this hook function :
> 1) According to UEFI 2.5 Section 3.4.3, the 3rd paragraph, It seems we need 
> to have a hook function (PlatformBootManagerDefaultBootFail) for platform 
> firmware to do platform action like recovering or setting to a known set of 
> boot options.
> 2) This hook function is similar to IntelFrameworkModulePkg's 
> PlatformBdsLib's PlatformBdsBootFail function which was used in several 
> platforms in EDK2, so it would be helpful for other platforms to change their 
> BDS platform library from IntelFrameworkModulePkg's PlatformBdsLib to 
> MdeModulePkg's PlatformBootManagerLib in the future.
> 
> Where we call/run this hook function:
> 1) According to UEFI 2.5 Section 3.1.1, the 5th paragraph, we need to present 
> a boot manager menu to the user for the successful boot attempt, so we only 
> call PlatformBootManagerDefaultBootFail when booting enumerated boot options 
> fails.
> 
> Reference:
> 1) UEFI 2.5 Section 3.4.3, the 3rd paragraph:
>- It is 

[edk2] [Patch 1/3] Revert "MdeModulePkg: Fix a PciBusDxe hot plug bug"

[Ruiyu Ni]

This reverts commit 73b7f115c653c807b9d0be97bf516871d8aff7ba.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni 
---
 .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.c   |  80 +--
 .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.h   |  13 --
 MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c| 159 +++--
 .../Bus/Pci/PciBusDxe/PciResourceSupport.c |  58 +++-
 4 files changed, 102 insertions(+), 208 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
index a6ade26..f7aea4f 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
@@ -325,81 +325,6 @@ PciSearchDevice (
 }
 
 /**
-  Dump the PPB padding resource information.
-
-  @param PciIoDevice PCI IO instance.
-  @param ResourceTypeThe desired resource type to dump.
- PciBarTypeUnknown means to dump all types of 
resources.
-**/
-VOID
-DumpPpbPaddingResource (
-  IN PCI_IO_DEVICE*PciIoDevice,
-  IN PCI_BAR_TYPE ResourceType
-  )
-{
-  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptor;
-  PCI_BAR_TYPE  Type;
-
-  if (PciIoDevice->ResourcePaddingDescriptors == NULL) {
-return;
-  }
-
-  if (ResourceType == PciBarTypeIo16 || ResourceType == PciBarTypeIo32) {
-ResourceType = PciBarTypeIo;
-  }
-
-  for (Descriptor = PciIoDevice->ResourcePaddingDescriptors; Descriptor->Desc 
!= ACPI_END_TAG_DESCRIPTOR; Descriptor++) {
-
-Type = PciBarTypeUnknown;
-if (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR && 
Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_IO) {
-  Type = PciBarTypeIo;
-} else if (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR && 
Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) {
-
-  if (Descriptor->AddrSpaceGranularity == 32) {
-//
-// prefechable
-//
-if (Descriptor->SpecificFlag == 
EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) {
-  Type = PciBarTypePMem32;
-}
-
-//
-// Non-prefechable
-//
-if (Descriptor->SpecificFlag == 0) {
-  Type = PciBarTypeMem32;
-}
-  }
-
-  if (Descriptor->AddrSpaceGranularity == 64) {
-//
-// prefechable
-//
-if (Descriptor->SpecificFlag == 
EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) {
-  Type = PciBarTypePMem64;
-}
-
-//
-// Non-prefechable
-//
-if (Descriptor->SpecificFlag == 0) {
-  Type = PciBarTypeMem64;
-}
-  }
-}
-
-if ((Type != PciBarTypeUnknown) && ((ResourceType == PciBarTypeUnknown) || 
(ResourceType == Type))) {
-  DEBUG ((
-EFI_D_INFO,
-"   Padding: Type = %s; Alignment = 0x%lx;\tLength = 0x%lx\n",
-mBarTypeStr[Type], Descriptor->AddrRangeMax, Descriptor->AddrLen
-));
-}
-  }
-
-}
-
-/**
   Dump the PCI BAR information.
 
   @param PciIoDevice PCI IO instance.
@@ -661,10 +586,7 @@ GatherPpbInfo (
 
   GetResourcePaddingPpb (PciIoDevice);
 
-  DEBUG_CODE (
-DumpPpbPaddingResource (PciIoDevice, PciBarTypeUnknown);
-DumpPciBars (PciIoDevice);
-  );
+  DEBUG_CODE (DumpPciBars (PciIoDevice););
 
   return PciIoDevice;
 }
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h
index 4d7b3b7..a4489b8 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h
@@ -460,17 +460,4 @@ ResetAllPpbBusNumber (
   IN UINT8  StartBusNumber
   );
 
-/**
-  Dump the PPB padding resource information.
-
-  @param PciIoDevice PCI IO instance.
-  @param ResourceTypeThe desired resource type to dump.
- PciBarTypeUnknown means to dump all types of 
resources.
-**/
-VOID
-DumpPpbPaddingResource (
-  IN PCI_IO_DEVICE*PciIoDevice,
-  IN PCI_BAR_TYPE ResourceType
-  );
-
 #endif
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
index f4b6ebf..3e275e3 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
@@ -188,21 +188,19 @@ DumpBridgeResource (
   BridgeResource->PciDev->PciBar[BridgeResource->Bar].BaseAddress,
   BridgeResource->Length, BridgeResource->Alignment
   ));
-for ( Link = GetFirstNode (>ChildList)
-; !IsNull (>ChildList, Link)
-; Link = GetNextNode (>ChildList, Link)
+for ( Link = BridgeResource->ChildList.ForwardLink
+; Link != >ChildList
+; Link = Link->ForwardLink
 ) {
   Resource = RESOURCE_NODE_FROM_LINK (Link);
   if (Resource->ResourceUsage == PciResUsageTypical) {
 

[edk2] [Patch 0/3] MdeModulePkg: Fix a PciBusDxe hot plug bug

[Ruiyu Ni]

For a hot plug bridge with device attached, PciBusDxe driver reserves
the resources which equal to the total amount of padding resource
returned from HotPlug->GetResourcePadding() and the actual occupied
resource by the attached device. The behavior is incorrect.
Correct behavior is to reserve the bigger one between the padding
resource and the actual occupied resource.

The first patch is to revert the previous two commits which combine
the debug log fix and resource reservation fix together.

Ruiyu Ni (3):
  Revert "MdeModulePkg: Fix a PciBusDxe hot plug bug"
  MdeModulePkg: Fix a PCI resource dumping bug in PciBusDxe driver
  MdeModulePkg: Fix a PciBusDxe hot plug bug

 MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h | 2 +-
 MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c   | 2 +-
 MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c   | 2 +-
 3 files changed, 3 insertions(+), 3 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] MdeModulePkg/RegularExpressionDxe: Correct copyright

[Cinnamon Shia]

Correct copyrights in RegularExpressionDxe

Signed-off-by: Cinnamon Shia 
---
 .../Universal/RegularExpressionDxe/Oniguruma/OnigurumaIntrinsics.c  | 2 +-
 .../Universal/RegularExpressionDxe/Oniguruma/OnigurumaUefiPort.c| 2 +-
 .../Universal/RegularExpressionDxe/Oniguruma/OnigurumaUefiPort.h| 2 +-
 MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regcomp.c | 2 +-
 MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regenc.c  | 2 +-
 MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regerror.c| 2 +-
 MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regexec.c | 2 +-
 MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/reggnu.c  | 2 +-
 MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c| 2 +-
 MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.h| 2 +-
 MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regposerr.c   | 2 +-
 MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regposix.c| 2 +-
 MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.c  | 2 +-
 MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.h  | 2 +-
 MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.inf| 2 +-
 15 files changed, 15 insertions(+), 15 deletions(-)

diff --git 
a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/OnigurumaIntrinsics.c 
b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/OnigurumaIntrinsics.c
index a60b647..c48522e 100644
--- 
a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/OnigurumaIntrinsics.c
+++ 
b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/OnigurumaIntrinsics.c
@@ -2,7 +2,7 @@
 
   Provide intrinsics within Oniguruma
 
-  Copyright (c) 2015, Hewlett-Packard Development Company, L.P.
+  (C) Copyright 2015 Hewlett Packard Enterprise Development LP
 
   This program and the accompanying materials are licensed and made available
   under the terms and conditions of the BSD License that accompanies this
diff --git 
a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/OnigurumaUefiPort.c 
b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/OnigurumaUefiPort.c
index 98822f3..081fcb3 100644
--- a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/OnigurumaUefiPort.c
+++ b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/OnigurumaUefiPort.c
@@ -2,7 +2,7 @@
   
   Module to rewrite stdlib references within Oniguruma
 
-  Copyright (c) 2014-2015, Hewlett-Packard Development Company, L.P.
+  (C) Copyright 2014-2015 Hewlett Packard Enterprise Development LP
 
   This program and the accompanying materials are licensed and made available
   under the terms and conditions of the BSD License that accompanies this
diff --git 
a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/OnigurumaUefiPort.h 
b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/OnigurumaUefiPort.h
index 4ca7367..18f2851 100644
--- a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/OnigurumaUefiPort.h
+++ b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/OnigurumaUefiPort.h
@@ -2,7 +2,7 @@
   
   Module to rewrite stdlib references within Oniguruma
 
-  Copyright (c) 2014-2015, Hewlett-Packard Development Company, L.P.
+  (C) Copyright 2014-2015 Hewlett Packard Enterprise Development LP
 
   This program and the accompanying materials are licensed and made available
   under the terms and conditions of the BSD License that accompanies this
diff --git a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regcomp.c 
b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regcomp.c
index 6dc6c28..25b768b 100644
--- a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regcomp.c
+++ b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regcomp.c
@@ -5,7 +5,7 @@
  * Copyright (c) 2002-2013  K.Kosako  
  * All rights reserved.
  *
- * Copyright (c) 2015, Hewlett Packard Enterprise Development, L.P.
+ * (C) Copyright 2015 Hewlett Packard Enterprise Development LP
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
diff --git a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regenc.c 
b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regenc.c
index b7aa225..ec05262 100644
--- a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regenc.c
+++ b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regenc.c
@@ -5,7 +5,7 @@
  * Copyright (c) 2002-2007  K.Kosako  
  * All rights reserved.
  * 
- * Copyright (c) 2015, Hewlett Packard Enterprise Development, L.P.
+ * (C) Copyright 2015 Hewlett Packard Enterprise Development LP
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
diff --git a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regerror.c 

[edk2] [PATCH 2/2] MdeModulePkg/RegularExpressionDxe: Add missing PrintLib

[Cinnamon Shia]

AsciiVSPrint is used in RegularExpressionDxe/Oniguruma/OnigurumaUefiPort.c.
But PrintLib is missing in RegularExpressionDxe.inf.

Signed-off-by: Cinnamon Shia 
---
 MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.inf | 1 +
 1 file changed, 1 insertion(+)

diff --git 
a/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.inf 
b/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.inf
index 1f94aef..cfe42a6 100644
--- a/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.inf
+++ b/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.inf
@@ -63,6 +63,7 @@
   MemoryAllocationLib
   BaseMemoryLib
   DebugLib
+  PrintLib
 
 [Guids]
   gEfiRegexSyntaxTypePosixExtendedGuid## CONSUMES  ## GUID
-- 
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] MdeModulePkg: Fix a PciBusDxe hot plug bug

[Tian, Feng]

Leif,

Thanks for raising this issue.

I agree with you that the patch should be split to small ones and make it more 
readable. I also agree we should give community more time to review those big 
patch before getting committed in.

Ray, who is the module owner, will follow up your suggestions.

Thanks
Feng

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Leif 
Lindholm
Sent: Thursday, October 29, 2015 19:55
To: Tian, Feng
Cc: Ni, Ruiyu; Kinney, Michael D; edk2-devel@lists.01.org; Fan, Jeff
Subject: Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug

Hi Feng,

On Thu, Oct 29, 2015 at 12:23:04AM +, Tian, Feng wrote:
> Ray has sent the patch for review, you may ignore it. 
> (http://article.gmane.org/gmane.comp.bios.edk2.devel/3484)

Yes, I see it has now been committed - which solves the known bug.

But this still leaves us with a clearly not sufficiently reviewed/tested 
invasive patch in core PCI code, and a jumbled commit history.

Since I have not received any response when asking previously, I will ask 
again: Can we please revert SVN r18658 and introduce this changeset in a more 
reviewable form, with proper review/testing?

Regards,

Leif

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> Leif Lindholm
> Sent: Wednesday, October 28, 2015 22:51
> To: Tian, Feng
> Cc: Ni, Ruiyu; edk2-devel@lists.01.org; Fan, Jeff
> Subject: Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug
> 
> Feng,
> 
> Any update on the below?
> The hard crash bug is still in SVN r18690.
> 
> Regards,
> 
> Leif
> 
> On Mon, Oct 26, 2015 at 03:35:15PM +, Leif Lindholm wrote:
> > Hi Ray,
> > 
> > On Mon, Oct 26, 2015 at 03:17:22PM +, Ni, Ruiyu wrote:
> > > Can you add a null pointer check to
> > > PciIoDevice->ResourcePaddingDescriptors before calling
> > > DumpPpbPaddingResource? Does it help?
> > 
> > It removes the delay and the crash - thanks!
> > 
> > But it does nothing for the commit history ;)
> > 
> > Regards,
> > 
> > Leif
> > 
> > > > 在 2015年10月26日，21:13，Leif Lindholm  写道：
> > > > 
> > > > Hi Ruiyu, Feng,
> > > > 
> > > > I am currently tracking down an issue on (at least) one of my 
> > > > platforms - that happens with this (now committed) patch, but 
> > > > not without it.
> > > > 
> > > > Symptoms are a _long_ delay, followed by an unaligned access in 
> > > > (I
> > > > think) DumpPpbPaddingResource.
> > > > 
> > > > Anyway, there could be other things playing in here - I'm 
> > > > testing a new card in a platform I haven't previously tested cards.
> > > > 
> > > > But this is one large patch, which could have been split up into 
> > > > multiple ones to make the introduced changes more reviewable:
> > > > - There are both functional changes and whitespace fixups.
> > > > - There are (text-only) changes to existing messages.
> > > > - There is refactoring of internal APIs.
> > > > 
> > > > It is certainly too invasive a change to be committed ~32 
> > > > minutes after having first been posted to the list.
> > > > 
> > > > Any chance we can revert this one and introduce it in smaller 
> > > > portions, making the individual change sets more reviewable?
> > > > 
> > > > Regards,
> > > > 
> > > > Leif
> > > > 
> > > >> On Fri, Oct 23, 2015 at 03:57:40PM +0800, Ruiyu Ni wrote:
> > > >> For a hot plug bridge with device attached, PciBusDxe driver 
> > > >> reserves the resources which equal to the total amount of 
> > > >> padding resource returned from HotPlug->GetResourcePadding() 
> > > >> and the actual occupied resource by the attached device. The behavior 
> > > >> is incorrect.
> > > >> Correct behavior is to reserve the bigger one between the 
> > > >> padding resource and the actual occupied resource.
> > > >> 
> > > >> Contributed-under: TianoCore Contribution Agreement 1.0
> > > >> Signed-off-by: Ruiyu Ni 
> > > >> Cc: Jeff Fan 
> > > >> Cc: Feng Tian 
> > > >> ---
> > > >> .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.c   |  76 +-
> > > >> .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.h   |  13 ++
> > > >> MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c| 159 
> > > >> ++---
> > > >> .../Bus/Pci/PciBusDxe/PciResourceSupport.c |  58 +---
> > > >> 4 files changed, 204 insertions(+), 102 deletions(-)
> > > >> 
> > > >> diff --git
> > > >> a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > > >> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > > >> index f7aea4f..030ef42 100644
> > > >> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > > >> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > > >> @@ -325,6 +325,77 @@ PciSearchDevice ( }
> > > >> 
> > > >> /**
> > > >> +  Dump the PPB padding resource information.
> > > >> +
> > > >> +  @param PciIoDevice PCI IO instance.
> > > >> +  @param 

[edk2] [PATCH] MdeModulePkg SmbiosMeasurementDxe: NULL check to String from GetSmbiosStringById

[Star Zeng]

When StringId is not 0, String returned from GetSmbiosStringById is expected to 
non-NULL.
Add ASSERT (String != NULL); to ensure this.

Cc: Jiewen Yao 
Cc: Shumin Qiu 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Star Zeng 
---
 MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c 
b/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c
index 5aafabf..f9e0196 100644
--- a/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c
+++ b/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c
@@ -286,6 +286,7 @@ FilterSmbiosEntry (
   if (StringId != 0) {
 // set ' ' for string field
 String = GetSmbiosStringById (TableEntry, StringId, );
+ASSERT (String != NULL);
 //DEBUG ((EFI_D_INFO,"StrId(0x%x)-%a(%d)\n", StringId, String, 
StringLen));
 SetMem (String, StringLen, ' ');
   }
-- 
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] MdeModulePkg SmbiosMeasurementDxe: NULL check to String from GetSmbiosStringById

[Qiu, Shumin]

Reviewed-by: Qiu Shumin 

-Original Message-
From: Zeng, Star 
Sent: Friday, October 30, 2015 10:12 AM
To: edk2-devel@lists.01.org
Cc: Yao, Jiewen; Qiu, Shumin
Subject: [PATCH] MdeModulePkg SmbiosMeasurementDxe: NULL check to String from 
GetSmbiosStringById

When StringId is not 0, String returned from GetSmbiosStringById is expected to 
non-NULL.
Add ASSERT (String != NULL); to ensure this.

Cc: Jiewen Yao 
Cc: Shumin Qiu 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Star Zeng 
---
 MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c 
b/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c
index 5aafabf..f9e0196 100644
--- a/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c
+++ b/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c
@@ -286,6 +286,7 @@ FilterSmbiosEntry (
   if (StringId != 0) {
 // set ' ' for string field
 String = GetSmbiosStringById (TableEntry, StringId, );
+ASSERT (String != NULL);
 //DEBUG ((EFI_D_INFO,"StrId(0x%x)-%a(%d)\n", StringId, String, 
StringLen));
 SetMem (String, StringLen, ' ');
   }
-- 
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] MdeModulePkg SmbiosMeasurementDxe: NULL check to String from GetSmbiosStringById

[Yao, Jiewen]

Reviewed-by: jiewen@intel.com


-Original Message-
From: Zeng, Star 
Sent: Friday, October 30, 2015 10:12 AM
To: edk2-devel@lists.01.org
Cc: Yao, Jiewen; Qiu, Shumin
Subject: [PATCH] MdeModulePkg SmbiosMeasurementDxe: NULL check to String from 
GetSmbiosStringById

When StringId is not 0, String returned from GetSmbiosStringById is expected to 
non-NULL.
Add ASSERT (String != NULL); to ensure this.

Cc: Jiewen Yao 
Cc: Shumin Qiu 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Star Zeng 
---
 MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c 
b/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c
index 5aafabf..f9e0196 100644
--- a/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c
+++ b/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c
@@ -286,6 +286,7 @@ FilterSmbiosEntry (
   if (StringId != 0) {
 // set ' ' for string field
 String = GetSmbiosStringById (TableEntry, StringId, );
+ASSERT (String != NULL);
 //DEBUG ((EFI_D_INFO,"StrId(0x%x)-%a(%d)\n", StringId, String, 
StringLen));
 SetMem (String, StringLen, ' ');
   }
-- 
1.9.5.msysgit.0

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