Re: [edk2-devel] [PATCH] ArmVirtPkg: Allow EFI memory attributes protocol to be disabled

[Ard Biesheuvel]

On Wed, Dec 6, 2023 at 7:37 PM Oliver Smith-Denny
 wrote:
>
> My first caveat here is I am writing this email from the depths of a
> parental leave, so my brain is only half here and I haven't been up to
> date for the past two months :). I'll return in Jan and hopefully be
> a bit more sensical.
>

No worries - and congrats!

> On 12/6/2023 5:23 AM, Ard Biesheuvel wrote:
> > But what we might do is invent a way to avoid setting the XP attribute
> > on the entire region based on some heuristic. Given that the main
> > purpose of the EFI memory attribute protocol is to provide the ability
> > to remove XP (and set RO instead), perhaps we can avoid the set
> > entirely? Just brainstorming here.
> >
> > (cc'ing Taylor and Oliver given that this is related to the memory
> > policy work as well) Perhaps we can use the fact that the active image
> > is non-NX compat to make some tweaks?
> >
> > What I really want to avoid is derail our effort to tighten things
> > down and comply with the NX compat related policies, by adding some
> > build time control that the distros will enable now and never disable
> > again, citing backward compat concerns.
> > And the deafening silence from the shim developers is not an
> > encouragement either.
> >
>
> I completely agree here. My thoughts on this specific issue tend to be:
> - edk2 is reference FW and the mainline branch in particular. It should
> "do the right thing" not the hacky thing.
>
> - The bug here, as we all agree, is in shim. This is not a case where
> FW made a change for the better that broke something and so needs to fix
> the change it made, this is a case where FW offered a new option, which
> shim took advantage of and completely borked. edk2 can't guarantee that
> every OS will do the right thing and we should have guardrails that
> give us the best chance of booting, which is after all our top goal.
> However, we can't prevent an OS (or bootloader) from blowing its toes
> off. There's a part of me that thinks well, if your OS/bootloader sucks,
> get a better one...I understand this is complicated by the lack of
> release of a new shim (as this bug has been fixed upstream in shim for
> almost a year [1]).
>
> These two points being said, I tend to prefer Ard's approach, if I am
> reading it correctly in a quick skim, where this change is confined to
> QEMU. The platform FW (ArmVirtPkg in this instance) is the right place
> for the hacky stuff. The platform in the end is what has the requirement
> to boot certain versions of an OS/bootloader. edk2's requirement is to
> have sane and usable interfaces.
>
> So, I personally think that if we think edk2 has a sane memory attribute
> protocol (or at least as sane as we think it can be), then we should not
> change it. Now, it is totally valid to say edk2 does not have a sane
> memory attribute protocol and it should have better guardrails. However,
> I agree with Ard that if we add any generic edk2 level ability to
> disable the memory attribute protocol, it will never be used.
>

OK

> I think the problem with the pattern Ard is describing (don't set XP
> based on some heuristic) is that in case it won't work. Because the XP
> comes from shim first and then they ask us to unset XP for an
> unaligned region. Aligning this call to a larger region seems fraught
> with peril. To Gerd's point, we could attempt to catch this in the
> fault handler, set some flag, not set XP in the next boot (different
> memory policy) and then everything would work (hmm, although in this
> case, the memory policy doesn't come into play right, because shim
> explicitly asks us to set XP, so either you'd have to disable the
> protocol or in the protocol check the memory policy, which feels
> wrong). If something is worked out here it feels...better than
> a boot time flag and I suppose the more reasonable way to go here. But,
> I think the platform is still the place to implement this. This feels
> much more like a specific workaround than a generic fault flow we are
> going to catch. If this version of shim had been tested on all the
> platforms it was going to support, this would have been caught
> immediately. So I go back to, the platform can work around this by
> enabling a custom compat memory policy (which I think is the benefit
> of that framework), but that edk2 shouldn't back bend here and
> compromise safety for a bad shim.
>

Yeah, and if we do decide to do this, it requires a more comprehensive
approach, along the lines of what Taylor has implemented.

> Requiring a platform to fix this is more of a burden, but this is a
> burden the shim community created by not releasing a new shim and
> not testing the old one. We can't mitigate that. I think memory
> protections is an area where having the distros do this the right
> way is important.
>
> Sorry for the rambling and any context I'm missing, I'm typing this
> with a baby in arm :)
>

Thanks for the insights - much appreciated.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links

Re: [edk2-devel] [PATCH v2] MdeModulePkg/UefiBootManagerLib: Signal ReadyToBoot on platform recovery

[Ard Biesheuvel]

(cc Liming)

On Thu, 7 Dec 2023 at 05:48, Neal Gompa  wrote:
>
> On Fri, Nov 24, 2023 at 6:36 PM Neal Gompa  wrote:
> >
> > On Thu, Nov 2, 2023 at 6:35 AM Laszlo Ersek  wrote:
> > >
> > > On 10/31/23 23:27, Jeremy Linton wrote:
> > > > On 10/31/23 12:37, Neal Gompa via groups.io wrote:
> > > >> From: Neal Gompa 
> > > >>
> > > >> Currently, the ReadyToBoot event is only signaled when a formal Boot
> > > >> Manager option is executed (in BmBoot.c -> EfiBootManagerBoot ()).
> > > >>
> > > >> However, the introduction of Platform Recovery in UEFI 2.5 makes it
> > > >> necessary to signal ReadyToBoot when a Platform Recovery boot loader
> > > >> runs because otherwise it may lead to the execution of a boot loader
> > > >> that has similar requirements to a regular one that is not launched
> > > >> as a Boot Manager option.
> > > >>
> > > >> This is especially critical to ensuring that the graphical console
> > > >> is actually usable during platform recovery, as some platforms do
> > > >> rely on the ConsolePrefDxe driver, which only performs console
> > > >> initialization after ReadyToBoot is triggered.
> > > >>
> > > >> This patch fixes that behavior by calling EfiSignalEventReadyToBoot ()
> > > >> in EfiBootManagerProcessLoadOption () when invoking platform recovery,
> > > >> which is the function that sets up the platform recovery boot process.
> > > >>
> > > >> The expected behavior has been clarified in the UEFI 2.10 specification
> > > >> to explicitly indicate this behavior is required for correct operation.
> > > >>
> > > >> This is a rebased version of the patch originally written by Pete 
> > > >> Batard.
> > > >
> > > > Took me a bit to swap in that whole conversation again, and recheck the
> > > > spec's and code paths, but this all looks fine to me and should allow
> > > > the PFTF build to drop the similar patch from Pete that has been carried
> > > > downstream for the past couple years.
> > > >
> > > > As for testing the previous patch has been in the field for a couple
> > > > years now and i'm not aware of it causing any issues. The additional
> > > > restriction of limiting it to platform recovery logically makes sense,
> > > > and as far as I can see shouldn't cause any problems.
> > > >
> > > > So,
> > > >
> > > > Reviewed-by: Jeremy Linton 
> > > >
> > > >
> > > > As a PS: I also went to check the ready to boot behavior for OsRecovery
> > > > and realized that apparently none of that support was ever merged?
> > >
> > > What else is there, outside of this patch, in need of upstreaming?
> > >
> > > > That seems a bit of an oversight since its been in the spec for a few 
> > > > years now.
> > >
> > > "ready-to-boot for OsRecovery" is new in UEFI 2.10 (according to the
> > > commit message), which is quite recent ("Aug 29, 2022").
> > >
> > > I couldn't find the Mantis ticket in the Revision History (for 2.10) 
> > > though.
> > >
> >
> > Is there anything else holding up committing this patch? Jeremy and
> > you reviewed it earlier in the month...
> >
>
> Friendly ping again? It's been a month now...
>

Apologies for the delay - Liming, can we progress with this?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112165): https://edk2.groups.io/g/devel/message/112165
Mute This Topic: https://groups.io/mt/102302654/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [Patch V2 3/6] UefiCpuPkg: Consume MpInfo2Hob in PiSmmCpuDxe

[duntan]

Consume MpInfo2Hob in PiSmmCpuDxe driver to get
NumberOfProcessors, MaxNumberOfCpus and
EFI_PROCESSOR_INFORMATION for all CPU from the
MpInformation2 HOB.
This can avoid calling MP service.

Signed-off-by: Dun Tan 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Rahul Kumar 
Cc: Gerd Hoffmann 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c   | 210 
--
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   |   2 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf |   8 
 3 files changed, 169 insertions(+), 51 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
index 1d022a7051..b729f8ee63 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -99,8 +99,8 @@ UINTN  mSmmStackSize;
 UINTNmSmmShadowStackSize;
 BOOLEAN  mCetSupported = TRUE;
 
-UINTN  mMaxNumberOfCpus = 1;
-UINTN  mNumberOfCpus= 1;
+UINTN  mMaxNumberOfCpus = 0;
+UINTN  mNumberOfCpus= 0;
 
 //
 // SMM ready to lock flag
@@ -586,6 +586,147 @@ SmmReadyToLockEventNotify (
   return EFI_SUCCESS;
 }
 
+/**
+  Function to compare 2 MP_INFORMATION2_HOB_DATA pointer based on 
ProcessorIndex.
+
+  @param[in] Buffer1pointer to MP_INFORMATION2_HOB_DATA poiner to 
compare
+  @param[in] Buffer2pointer to second MP_INFORMATION2_HOB_DATA 
pointer to compare
+
+  @retval 0 Buffer1 equal to Buffer2
+  @retval <0Buffer1 is less than Buffer2
+  @retval >0Buffer1 is greater than Buffer2
+**/
+INTN
+EFIAPI
+MpInformation2HobCompare (
+  IN  CONST VOID  *Buffer1,
+  IN  CONST VOID  *Buffer2
+  )
+{
+  if ((*(MP_INFORMATION2_HOB_DATA **)Buffer1)->ProcessorIndex > 
(*(MP_INFORMATION2_HOB_DATA **)Buffer2)->ProcessorIndex) {
+return 1;
+  } else if ((*(MP_INFORMATION2_HOB_DATA **)Buffer1)->ProcessorIndex < 
(*(MP_INFORMATION2_HOB_DATA **)Buffer2)->ProcessorIndex) {
+return -1;
+  }
+
+  return 0;
+}
+
+/**
+  Extract NumberOfCpus, MaxNumberOfCpus and EFI_PROCESSOR_INFORMATION for all 
CPU from MpInformation2 HOB.
+
+  @param[out] NumberOfCpus   Pointer to NumberOfCpus.
+  @param[out] MaxNumberOfCpusPointer to MaxNumberOfCpus.
+
+  @retval ProcessorInfo  Pointer to EFI_PROCESSOR_INFORMATION 
buffer.
+**/
+EFI_PROCESSOR_INFORMATION *
+GetMpInformation (
+  OUT UINTN  *NumberOfCpus,
+  OUT UINTN  *MaxNumberOfCpus
+  )
+{
+  EFI_HOB_GUID_TYPE  *GuidHob;
+  EFI_HOB_GUID_TYPE  *FirstMpInfo2Hob;
+  MP_INFORMATION2_HOB_DATA   *MpInformation2HobData;
+  UINTN  HobCount;
+  UINTN  HobIndex;
+  MP_INFORMATION2_HOB_DATA   **MpInfo2Hobs;
+  UINTN  SortBuffer;
+  UINTN  ProcessorIndex;
+  UINT64 PrevProcessorIndex;
+  MP_INFORMATION2_ENTRY  *MpInformation2Entry;
+  EFI_PROCESSOR_INFORMATION  *ProcessorInfo;
+
+  GuidHob   = NULL;
+  MpInformation2HobData = NULL;
+  FirstMpInfo2Hob   = NULL;
+  MpInfo2Hobs   = NULL;
+  HobIndex  = 0;
+  HobCount  = 0;
+
+  FirstMpInfo2Hob = GetFirstGuidHob (&gMpInformationHobGuid2);
+  ASSERT (FirstMpInfo2Hob != NULL);
+  GuidHob = FirstMpInfo2Hob;
+  while (GuidHob != NULL) {
+MpInformation2HobData = GET_GUID_HOB_DATA (GuidHob);
+
+//
+// This is the last MpInformationHob in the HOB list.
+//
+if (MpInformation2HobData->NumberOfProcessors == 0) {
+  ASSERT (HobCount != 0);
+  break;
+}
+
+HobCount++;
+*NumberOfCpus += MpInformation2HobData->NumberOfProcessors;
+GuidHob= GetNextGuidHob (&gMpInformationHobGuid2, GET_NEXT_HOB 
(GuidHob));
+  }
+
+  ASSERT (*NumberOfCpus <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
+
+  //
+  // If support CPU hot plug, we need to allocate resources for possibly 
hot-added processors
+  //
+  if (FeaturePcdGet (PcdCpuHotPlugSupport)) {
+*MaxNumberOfCpus = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
+  } else {
+*MaxNumberOfCpus = *NumberOfCpus;
+  }
+
+  MpInfo2Hobs = AllocatePool (sizeof (MP_INFORMATION2_HOB_DATA *) * HobCount);
+  ASSERT (MpInfo2Hobs != NULL);
+  if (MpInfo2Hobs == NULL) {
+return NULL;
+  }
+
+  //
+  // Record each MpInformation2Hob pointer in the MpInfo2Hobs.
+  // The FirstMpInfo2Hob is to speed up this while-loop without
+  // needing to look for MpInfo2Hob from beginning.
+  //
+  GuidHob = FirstMpInfo2Hob;
+  ASSERT (GuidHob != NULL);
+  while (HobIndex < HobCount) {
+MpInfo2Hobs[HobIndex++] = GET_GUID_HOB_DATA (GuidHob);
+GuidHob = GetNextGuidHob (&gMpInformationHobGuid2, 
GET_NEXT_HOB (GuidHob));
+  }
+
+  ProcessorInfo = (EFI_PROCESSOR_INFORMATION *)AllocatePool (sizeof 
(EFI_PROCESSOR_INFORMATION) * (*MaxNumber

[edk2-devel] [Patch V2 6/6] UefiCpuPkg: Avoid assuming only one smmbasehob

[duntan]

Modify the gSmmBaseHobGuid consumption code to
remove the asuumption that there is only one
gSmmBaseHobGuid. If the CPU number is big enough,
there will be more than one SmmBaseHob in the
HOB list.

Signed-off-by: Dun Tan 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Rahul Kumar 
Cc: Gerd Hoffmann 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 179 
+++
 1 file changed, 147 insertions(+), 32 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
index b729f8ee63..fab2fed286 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -586,6 +586,132 @@ SmmReadyToLockEventNotify (
   return EFI_SUCCESS;
 }
 
+/**
+  Function to compare 2 SMM_BASE_HOB_DATA pointer based on ProcessorIndex.
+
+  @param[in] Buffer1pointer to SMM_BASE_HOB_DATA poiner to compare
+  @param[in] Buffer2pointer to second SMM_BASE_HOB_DATA pointer to 
compare
+
+  @retval 0 Buffer1 equal to Buffer2
+  @retval <0Buffer1 is less than Buffer2
+  @retval >0Buffer1 is greater than Buffer2
+**/
+INTN
+EFIAPI
+SmBaseHobCompare (
+  IN  CONST VOID  *Buffer1,
+  IN  CONST VOID  *Buffer2
+  )
+{
+  if ((*(SMM_BASE_HOB_DATA **)Buffer1)->ProcessorIndex > (*(SMM_BASE_HOB_DATA 
**)Buffer2)->ProcessorIndex) {
+return 1;
+  } else if ((*(SMM_BASE_HOB_DATA **)Buffer1)->ProcessorIndex < 
(*(SMM_BASE_HOB_DATA **)Buffer2)->ProcessorIndex) {
+return -1;
+  }
+
+  return 0;
+}
+
+/**
+  Extract SmBase for all CPU from SmmBase HOB.
+
+  @param[in]  MaxNumberOfCpus   Max NumberOfCpus.
+
+  @retval SmBaseBuffer  Pointer to SmBase Buffer.
+  @retval NULL  gSmmBaseHobGuid was not been created.
+**/
+UINTN *
+GetSmBase (
+  IN  UINTN  MaxNumberOfCpus
+  )
+{
+  UINTN  HobCount;
+  EFI_HOB_GUID_TYPE  *GuidHob;
+  SMM_BASE_HOB_DATA  *SmmBaseHobData;
+  UINTN  NumberOfProcessors;
+  SMM_BASE_HOB_DATA  **SmBaseHobs;
+  UINTN  *SmBaseBuffer;
+  UINTN  HobIndex;
+  UINTN  SortBuffer;
+  UINTN  ProcessorIndex;
+  UINT64 PrevProcessorIndex;
+  EFI_HOB_GUID_TYPE  *FirstSmmBaseGuidHob;
+
+  SmmBaseHobData = NULL;
+  HobIndex   = 0;
+  ProcessorIndex = 0;
+  HobCount   = 0;
+  NumberOfProcessors = 0;
+
+  FirstSmmBaseGuidHob = GetFirstGuidHob (&gSmmBaseHobGuid);
+  if (FirstSmmBaseGuidHob == NULL) {
+return NULL;
+  }
+
+  GuidHob = FirstSmmBaseGuidHob;
+  while (GuidHob != NULL) {
+HobCount++;
+SmmBaseHobData = GET_GUID_HOB_DATA (GuidHob);
+
+if (NumberOfProcessors >= MaxNumberOfCpus) {
+  break;
+}
+
+NumberOfProcessors += SmmBaseHobData->NumberOfProcessors;
+GuidHob = GetNextGuidHob (&gSmmBaseHobGuid, GET_NEXT_HOB 
(GuidHob));
+  }
+
+  ASSERT (NumberOfProcessors == MaxNumberOfCpus);
+  if (NumberOfProcessors != MaxNumberOfCpus) {
+CpuDeadLoop ();
+  }
+
+  SmBaseHobs = AllocatePool (sizeof (SMM_BASE_HOB_DATA *) * HobCount);
+  ASSERT (SmBaseHobs != NULL);
+  if (SmBaseHobs == NULL) {
+return NULL;
+  }
+
+  //
+  // Record each SmmBaseHob pointer in the SmBaseHobs.
+  // The FirstSmmBaseGuidHob is to speed up this while-loop
+  // without needing to look for SmBaseHob from beginning.
+  //
+  GuidHob = FirstSmmBaseGuidHob;
+  while (HobIndex < HobCount) {
+SmBaseHobs[HobIndex++] = GET_GUID_HOB_DATA (GuidHob);
+GuidHob= GetNextGuidHob (&gSmmBaseHobGuid, GET_NEXT_HOB 
(GuidHob));
+  }
+
+  SmBaseBuffer = (UINTN *)AllocatePool (sizeof (UINTN) * (MaxNumberOfCpus));
+  ASSERT (SmBaseBuffer != NULL);
+  if (SmBaseBuffer == NULL) {
+FreePool (SmBaseHobs);
+return NULL;
+  }
+
+  QuickSort (SmBaseHobs, HobCount, sizeof (SMM_BASE_HOB_DATA *), 
(BASE_SORT_COMPARE)SmBaseHobCompare, &SortBuffer);
+  PrevProcessorIndex = 0;
+  for (HobIndex = 0; HobIndex < HobCount; HobIndex++) {
+//
+// Make sure no overlap and no gap in the CPU range covered by each HOB
+//
+ASSERT (SmBaseHobs[HobIndex]->ProcessorIndex == PrevProcessorIndex);
+
+//
+// Cache each SmBase in order.
+//
+for (ProcessorIndex = 0; ProcessorIndex < 
SmBaseHobs[HobIndex]->NumberOfProcessors; ProcessorIndex++) {
+  SmBaseBuffer[PrevProcessorIndex + ProcessorIndex] = 
(UINTN)SmBaseHobs[HobIndex]->SmBase[ProcessorIndex];
+}
+
+PrevProcessorIndex += SmBaseHobs[HobIndex]->NumberOfProcessors;
+  }
+
+  FreePool (SmBaseHobs);
+  return SmBaseBuffer;
+}
+
 /**
   Function to compare 2 MP_INFORMATION2_HOB_DATA pointer based on 
ProcessorIndex.
 
@@ -744,27 +870,22 @@ PiCpuSmmEntry (
   IN EFI_SYSTEM_TABLE  *SystemTable
   )
 {
-  EFI_STATUS Status;
-  UINTN  Index;
-  VOID   *Buffer;

[edk2-devel] [Patch V2 5/6] UefiCpuPkg: Cache core type in MpInfo2 HOB

[duntan]

Cache core type in MpInfo2 HOB by CpuMpPei module.

Signed-off-by: Dun Tan 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Rahul Kumar 
Cc: Gerd Hoffmann 
---
 UefiCpuPkg/CpuMpPei/CpuMpPei.c | 59 
+--
 UefiCpuPkg/CpuMpPei/CpuMpPei.h |  2 ++
 2 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
index 8cacf4ddf5..7e2d7efb6b 100644
--- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
+++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
@@ -541,6 +541,30 @@ InitializeMpExceptionStackSwitchHandlers (
   FreePages (SwitchStackData, EFI_SIZE_TO_PAGES (NumberOfProcessors * sizeof 
(EXCEPTION_STACK_SWITCH_CONTEXT)));
 }
 
+/**
+  Get CPU core type.
+
+  @param[in, out] Buffer  Argument of the procedure.
+**/
+VOID
+EFIAPI
+GetProcessorCoreType (
+  IN OUT VOID  *Buffer
+  )
+{
+  EFI_STATUS   Status;
+  UINT8*CoreTypes;
+  CPUID_NATIVE_MODEL_ID_AND_CORE_TYPE_EAX  NativeModelIdAndCoreTypeEax;
+  UINTNProcessorIndex;
+
+  Status = MpInitLibWhoAmI (&ProcessorIndex);
+  ASSERT_EFI_ERROR (Status);
+
+  CoreTypes = (UINT8 *)Buffer;
+  AsmCpuidEx (CPUID_HYBRID_INFORMATION, CPUID_HYBRID_INFORMATION_MAIN_LEAF, 
&NativeModelIdAndCoreTypeEax.Uint32, NULL, NULL, NULL);
+  CoreTypes[ProcessorIndex] = (UINT8)NativeModelIdAndCoreTypeEax.Bits.CoreType;
+}
+
 /**
   Create gMpInformationHobGuid2.
 **/
@@ -558,13 +582,36 @@ BuildMpInformationHob (
   MP_INFORMATION2_HOB_DATA  *MpInformation2HobData;
   MP_INFORMATION2_ENTRY *MpInformation2Entry;
   UINTN Index;
+  UINT8 *CoreTypes;
+  UINT32CpuidMaxInput;
+  UINTN CoreTypePages;
 
   ProcessorIndex= 0;
   MpInformation2HobData = NULL;
   MpInformation2Entry   = NULL;
+  CoreTypes = NULL;
+  CoreTypePages = 0;
 
   Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, 
&NumberOfEnabledProcessors);
   ASSERT_EFI_ERROR (Status);
+
+  //
+  // Get Processors CoreType
+  //
+  AsmCpuid (CPUID_SIGNATURE, &CpuidMaxInput, NULL, NULL, NULL);
+  if (CpuidMaxInput >= CPUID_HYBRID_INFORMATION) {
+CoreTypePages = EFI_SIZE_TO_PAGES (sizeof (UINT8) * NumberOfProcessors);
+CoreTypes = AllocatePages (CoreTypePages);
+ASSERT (CoreTypes != NULL);
+
+Status = MpInitLibStartupAllCPUs (
+   GetProcessorCoreType,
+   0,
+   (VOID *)CoreTypes
+   );
+ASSERT_EFI_ERROR (Status);
+  }
+
   MaxProcessorsPerHob = ((MAX_UINT16 & ~7) - sizeof (EFI_HOB_GUID_TYPE) - 
sizeof (MP_INFORMATION2_HOB_DATA)) / sizeof (MP_INFORMATION2_ENTRY);
   NumberOfProcessorsInHob = MaxProcessorsPerHob;
 
@@ -597,12 +644,16 @@ BuildMpInformationHob (
   NULL
   );
   ASSERT_EFI_ERROR (Status);
+
+  MpInformation2Entry->CoreType = (CoreTypes != NULL) ? CoreTypes[Index + 
ProcessorIndex] : 0;
+
   DEBUG ((
 DEBUG_INFO,
-"  Processor[%04d]: ProcessorId = 0x%lx, StatusFlag = 0x%x\n",
+"  Processor[%04d]: ProcessorId = 0x%lx, StatusFlag = 0x%x, CoreType = 
0x%x\n",
 Index + ProcessorIndex,
 MpInformation2Entry->ProcessorInfo.ProcessorId,
-MpInformation2Entry->ProcessorInfo.StatusFlag
+MpInformation2Entry->ProcessorInfo.StatusFlag,
+MpInformation2Entry->CoreType
 ));
   DEBUG ((
 DEBUG_INFO,
@@ -625,6 +676,10 @@ BuildMpInformationHob (
 
 ProcessorIndex += NumberOfProcessorsInHob;
   }
+
+  if (CoreTypes != NULL) {
+FreePages (CoreTypes, CoreTypePages);
+  }
 }
 
 /**
diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.h b/UefiCpuPkg/CpuMpPei/CpuMpPei.h
index a40fd2c077..e7d07ffd64 100644
--- a/UefiCpuPkg/CpuMpPei/CpuMpPei.h
+++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.h
@@ -32,6 +32,8 @@
 
 #include 
 
+#include 
+
 extern EFI_PEI_PPI_DESCRIPTOR  mPeiCpuMpPpiDesc;
 
 /**
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112163): https://edk2.groups.io/g/devel/message/112163
Mute This Topic: https://groups.io/mt/103030295/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [Patch V2 1/6] UefiCpuPkg: Create gMpInformationHobGuid2 in UefiCpuPkg

[duntan]

Create gMpInformationHobGuid2 in UefiCpuPkg.

Currently, there is a gMpInformationHobGuid defined,
created and consumed only in StandaloneMmPkg. The HOB
contains the EFI_PROCESSOR_INFORMATION structure for
each CPU and the number of processors. This is the same
as the information that PiSmmCpuDxeSmm uses MpService
Protocol to get.

This new gMpInformationHobGuid2 also contains the
NumberOfProcessors and the EFI_PROCESSOR_INFORMATION
for each CPU. Also the HOB is extended to support the
case that the maximum HOB length is not enough for all
CPU. So there might be more than one HOB instance in the
HOB list. Each HOB describes the corresponding CPU index
range.

The plan is to create gMpInformationHob2Guid in CpuMpPei
module(implemented in next commit). Then PiSmmCpuDxeSmm
and other MM_STANDALONE modules can consume the hob. This
can avoid calling MpService Protocol in PiSmmCpuDxeSmm.
Also the issue that one gMpInformationHobGuid might be not
enough when CPU number is 1~2000 or bigger can be solved.

Signed-off-by: Dun Tan 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Rahul Kumar 
Cc: Gerd Hoffmann 
---
 UefiCpuPkg/Include/Guid/MpInformation2.h | 56 

 UefiCpuPkg/UefiCpuPkg.dec|  3 +++
 2 files changed, 59 insertions(+)

diff --git a/UefiCpuPkg/Include/Guid/MpInformation2.h 
b/UefiCpuPkg/Include/Guid/MpInformation2.h
new file mode 100644
index 00..7a25fcc2f3
--- /dev/null
+++ b/UefiCpuPkg/Include/Guid/MpInformation2.h
@@ -0,0 +1,56 @@
+/** @file
+  EFI MP information protocol provides a lightweight MP_SERVICES_PROTOCOL.
+
+  MP information protocol only provides static information of MP processor.
+
+  If SwitchBSP or Enable/DisableAP in MP service is called between the HOB
+  production and HOB consumption, EFI_PROCESSOR_INFORMATION.StatusFlag field
+  in this HOB may be invalidated.
+
+  Copyright (c) 2023, Intel Corporation. All rights reserved.
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef MP_INFORMATION2_H_
+#define MP_INFORMATION2_H_
+
+#include 
+#include 
+#include 
+
+#define MP_INFORMATION2_HOB_REVISION  1
+
+#define MP_INFORMATION2_GUID \
+  { \
+0x417a7f64, 0xf4e9, 0x4b32, {0x84, 0x6a, 0x5c, 0xc4, 0xd8, 0x62, 0x18, 
0x79}  \
+  }
+
+typedef struct {
+  EFI_PROCESSOR_INFORMATIONProcessorInfo;
+  //
+  // Add more fields in future
+  //
+} MP_INFORMATION2_ENTRY;
+
+typedef struct {
+  UINT16   NumberOfProcessors;
+  UINT16   EntrySize;
+  UINT8Version;
+  UINT8Reserved[3];
+  UINT64   ProcessorIndex;
+  MP_INFORMATION2_ENTRYEntry[0];
+} MP_INFORMATION2_HOB_DATA;
+
+//
+// Producer of MP_INFORMATION2_HOB_DATA should assign sizeof 
(MP_INFORMATION2_ENTRY) to MP_INFORMATION2_HOB_DATA.EntrySize
+// Consumer of MP_INFORMATION2_HOB_DATA should use below macro or similar 
logic to get the individual entry
+// as the entry structure might be updated to include more information.
+//
+#define GET_MP_INFORMATION_ENTRY(MpInfoHobData, Index) \
+(MP_INFORMATION2_ENTRY *)((UINTN)&((MP_INFORMATION2_HOB_DATA 
*)(MpInfoHobData))->Entry + (MpInfoHobData)->EntrySize * Index)
+
+extern EFI_GUID  gMpInformationHobGuid2;
+
+#endif
diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index 0b5431dbf7..61bd34ef17 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -85,6 +85,9 @@
   ## Include/Guid/SmmBaseHob.h
   gSmmBaseHobGuid  = { 0xc2217ba7, 0x03bb, 0x4f63, {0xa6, 0x47, 0x7c, 
0x25, 0xc5, 0xfc, 0x9d, 0x73 }}
 
+  ## Include/Guid/MpInformation2.h
+  gMpInformationHobGuid2 = { 0x417a7f64, 0xf4e9, 0x4b32, {0x84, 0x6a, 
0x5c, 0xc4, 0xd8, 0x62, 0x18, 0x79 }}
+
 [Protocols]
   ## Include/Protocol/SmmCpuService.h
   gEfiSmmCpuServiceProtocolGuid   = { 0x1d202cab, 0xc8ab, 0x4d5c, { 0x94, 
0xf7, 0x3c, 0xfc, 0xc0, 0xd3, 0xd3, 0x35 }}
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112160): https://edk2.groups.io/g/devel/message/112160
Mute This Topic: https://groups.io/mt/103030292/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [Patch V2 2/6] UefiCpuPkg: Build MpInfo2HOB in CpuMpPei

[duntan]

Build MpInfo2HOB in CpuMpPei module so that later
PiSmmCpuDxe or other StandaloneMm module can consume
the HOB.
Since there might be more one gMpInformationHobGuid2
in HOB list, CpuMpPei create a gMpInformationHobGuid2
with 0 value NumberOfProcessors field in the end of the
process to indicate it's the last MP_INFORMATION2_HOB.

Signed-off-by: Dun Tan 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Rahul Kumar 
Cc: Gerd Hoffmann 
---
 UefiCpuPkg/CpuMpPei/CpuMpPei.c   | 91 
+++
 UefiCpuPkg/CpuMpPei/CpuMpPei.h   |  4 +++-
 UefiCpuPkg/CpuMpPei/CpuMpPei.inf |  3 ++-
 3 files changed, 96 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
index b504bea3cf..8cacf4ddf5 100644
--- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
+++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
@@ -541,6 +541,92 @@ InitializeMpExceptionStackSwitchHandlers (
   FreePages (SwitchStackData, EFI_SIZE_TO_PAGES (NumberOfProcessors * sizeof 
(EXCEPTION_STACK_SWITCH_CONTEXT)));
 }
 
+/**
+  Create gMpInformationHobGuid2.
+**/
+VOID
+BuildMpInformationHob (
+  VOID
+  )
+{
+  EFI_STATUSStatus;
+  UINTN ProcessorIndex;
+  UINTN NumberOfProcessors;
+  UINTN NumberOfEnabledProcessors;
+  UINTN NumberOfProcessorsInHob;
+  UINTN MaxProcessorsPerHob;
+  MP_INFORMATION2_HOB_DATA  *MpInformation2HobData;
+  MP_INFORMATION2_ENTRY *MpInformation2Entry;
+  UINTN Index;
+
+  ProcessorIndex= 0;
+  MpInformation2HobData = NULL;
+  MpInformation2Entry   = NULL;
+
+  Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, 
&NumberOfEnabledProcessors);
+  ASSERT_EFI_ERROR (Status);
+  MaxProcessorsPerHob = ((MAX_UINT16 & ~7) - sizeof (EFI_HOB_GUID_TYPE) - 
sizeof (MP_INFORMATION2_HOB_DATA)) / sizeof (MP_INFORMATION2_ENTRY);
+  NumberOfProcessorsInHob = MaxProcessorsPerHob;
+
+  //
+  // Create MP_INFORMATION2_HOB. when the max HobLength 0xFFF8 is not enough, 
there
+  // will be a MP_INFORMATION2_HOB series in the HOB list.
+  // In the HOB list, there is a gMpInformationHobGuid2 with 0 value 
NumberOfProcessors
+  // fields to indicate it's the last MP_INFORMATION2_HOB.
+  //
+  while (NumberOfProcessorsInHob != 0) {
+NumberOfProcessorsInHob = MIN (NumberOfProcessors - ProcessorIndex, 
MaxProcessorsPerHob);
+MpInformation2HobData   = BuildGuidHob (
+&gMpInformationHobGuid2,
+sizeof (MP_INFORMATION2_HOB_DATA) + sizeof 
(MP_INFORMATION2_ENTRY) * NumberOfProcessorsInHob
+);
+ASSERT (MpInformation2HobData != NULL);
+
+MpInformation2HobData->Version= MP_INFORMATION2_HOB_REVISION;
+MpInformation2HobData->ProcessorIndex = ProcessorIndex;
+MpInformation2HobData->NumberOfProcessors = 
(UINT16)NumberOfProcessorsInHob;
+MpInformation2HobData->EntrySize  = sizeof (MP_INFORMATION2_ENTRY);
+
+DEBUG ((DEBUG_INFO, "Creating MpInformation2 HOB...\n"));
+
+for (Index = 0; Index < NumberOfProcessorsInHob; Index++) {
+  MpInformation2Entry = &MpInformation2HobData->Entry[Index];
+  Status  = MpInitLibGetProcessorInfo (
+  (Index + ProcessorIndex) | 
CPU_V2_EXTENDED_TOPOLOGY,
+  &MpInformation2Entry->ProcessorInfo,
+  NULL
+  );
+  ASSERT_EFI_ERROR (Status);
+  DEBUG ((
+DEBUG_INFO,
+"  Processor[%04d]: ProcessorId = 0x%lx, StatusFlag = 0x%x\n",
+Index + ProcessorIndex,
+MpInformation2Entry->ProcessorInfo.ProcessorId,
+MpInformation2Entry->ProcessorInfo.StatusFlag
+));
+  DEBUG ((
+DEBUG_INFO,
+"Location (Package/Core/Thread) = %d/%d/%d\n",
+MpInformation2Entry->ProcessorInfo.Location.Package,
+MpInformation2Entry->ProcessorInfo.Location.Core,
+MpInformation2Entry->ProcessorInfo.Location.Thread
+));
+  DEBUG ((
+DEBUG_INFO,
+"Location2 (Package/Die/Tile/Module/Core/Thread) = 
%d/%d/%d/%d/%d/%d\n",
+
MpInformation2Entry->ProcessorInfo.ExtendedInformation.Location2.Package,
+MpInformation2Entry->ProcessorInfo.ExtendedInformation.Location2.Die,
+MpInformation2Entry->ProcessorInfo.ExtendedInformation.Location2.Tile,
+
MpInformation2Entry->ProcessorInfo.ExtendedInformation.Location2.Module,
+MpInformation2Entry->ProcessorInfo.ExtendedInformation.Location2.Core,
+MpInformation2Entry->ProcessorInfo.ExtendedInformation.Location2.Thread
+));
+}
+
+ProcessorIndex += NumberOfProcessorsInHob;
+  }
+}
+
 /**
   Initializes MP and exceptions handlers.
 
@@ -602,6 +688,11 @@ InitializeCpuMpWorker (
   Status = PeiServicesInstallPpi (mPeiCpuMpPpiList);
   ASSER

[edk2-devel] [Patch V2 0/6] Create and consume a new gMpInformationHobGuid2 in UefiCpuPkg.

[duntan]

In the V2 patch set, modify some function protype and some variables naming.

Original message:
Create and consume gMpInformationHobGuid2 in UefiCpuPkg in this patch series.

Currently, there is a gMpInformationHobGuid defined, created and consumed only 
in StandaloneMmPkg.
The HOB contains the EFI_PROCESSOR_INFORMATION structure for each CPU and the 
number of processors.
This is the same as the information that PiSmmCpuDxeSmm uses MpService Protocol 
to get.

This new gMpInformationHobGuid2 also contains the NumberOfProcessors and the 
EFI_PROCESSOR_INFORMATION for each CPU. 
lso the HOB is extended to support the case that the maximum HOB length is not 
enough for all CPU.
So there might be more than one HOB instance in the HOB list. Each HOB 
describes the corresponding CPU index range.

The plan is to create gMpInformationHob2Guid in CpuMpPei module. Then 
PiSmmCpuDxeSmm and other MM_STANDALONE modules can consume the hob.
This can avoid calling MpService Protocol in PiSmmCpuDxeSmm. Also the issue 
that one gMpInformationHobGuid might be not enough when CPU number is 1~2000 or 
bigger can be solved.

Also the code to extracting information from gSmmBaseHobGuid is modified to 
remove the assumption that there is only one HOB instance of gSmmBaseHobGuid.

Dun Tan (6):
  UefiCpuPkg: Create gMpInformationHobGuid2 in UefiCpuPkg
  UefiCpuPkg: Build MpInfo2HOB in CpuMpPei
  UefiCpuPkg: Consume MpInfo2Hob in PiSmmCpuDxe
  UefiCpuPkg: Add a new field in MpInfo2 HOB
  UefiCpuPkg: Cache core type in MpInfo2 HOB
  UefiCpuPkg: Avoid assuming only one smmbasehob

 UefiCpuPkg/CpuMpPei/CpuMpPei.c   | 146 
++
 UefiCpuPkg/CpuMpPei/CpuMpPei.h   |   6 +-
 UefiCpuPkg/CpuMpPei/CpuMpPei.inf |   3 ++-
 UefiCpuPkg/Include/Guid/MpInformation2.h |  58 
++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c   | 353 
+
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   |   2 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf |   8 
 UefiCpuPkg/UefiCpuPkg.dec|   3 +++
 8 files changed, 512 insertions(+), 67 deletions(-)
 create mode 100644 UefiCpuPkg/Include/Guid/MpInformation2.h

-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112159): https://edk2.groups.io/g/devel/message/112159
Mute This Topic: https://groups.io/mt/103030291/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v9 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V

[Dhaval Sharma]

Comments inline:


On Wed, Dec 6, 2023 at 7:50 PM Sunil V L  wrote:

> Hi Dhaval,
>
> Thank you very much for fixing the issue with instruction cache
> invalidation and confirming with the spec owner. Few minor comments
> below.
>
> On Mon, Dec 04, 2023 at 01:59:49PM +0530, Dhaval Sharma wrote:
> > Use newly defined cache management operations for RISC-V where possible
> > It builds up on the support added for RISC-V cache management
> > instructions in BaseLib.
> > Cc: Michael D Kinney 
> > Cc: Liming Gao 
> > Cc: Zhiguang Liu 
> > Cc: Laszlo Ersek 
> >
> > Signed-off-by: Dhaval Sharma 
> > Acked-by: Laszlo Ersek 
> > ---
> >
> > Notes:
> > V9:
> > - Fixed an issue with Instruction cache invalidation. Use fence.i
> >   instruction as CMO does not support i-cache operations.
> > V8:
> > - Added note to convert PCD into RISC-V feature bitmap pointer
> > - Modified function names to be more explicit about cache ops
> > - Added RB tag
> > V7:
> > - Added PcdLib
> > - Restructure DEBUG message based on feedback on V6
> > - Make naming consistent to CMO, remove all CBO references
> > - Add ASSERT for not supported functions instead of plain debug
> message
> > - Added RB tag
> > V6:
> > - Utilize cache management instructions if HW supports it
> >   This patch is part of restructuring on top of v5
> >
> IMO, it is better to keep the change log in the cover letter. Since not
> all patches may be CC'd to every one apart from the cover letter, it is
> difficult to understand from the cover letter what has changed in the new
> series.
>
[Dhaval] AFAIU notes are tied to specific commits. But it makes sense, I
can add an update to the cover letter.

>
> >  MdePkg/MdePkg.dec  |
>  8 +
> >  MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf |
>  5 +
> >  MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c|
> 173 
> >  MdePkg/MdePkg.uni  |
>  4 +
> >  4 files changed, 160 insertions(+), 30 deletions(-)
> >
> > diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> > index ac54338089e8..fa92673ff633 100644
> > --- a/MdePkg/MdePkg.dec
> > +++ b/MdePkg/MdePkg.dec
> > @@ -2399,6 +2399,14 @@ [PcdsFixedAtBuild.AARCH64,
> PcdsPatchableInModule.AARCH64]
> ># @Prompt CPU Rng algorithm's GUID.
> >
> gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm|{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}|VOID*|0x0037
> >
> > +[PcdsFixedAtBuild.RISCV64, PcdsPatchableInModule.RISCV64]
> > +  #
> > +  # Configurability to override RISC-V CPU Features
> > +  # BIT 0 = Cache Management Operations. This bit is relevant only if
> > +  # previous stage has feature enabled and user wants to disable it.
> > +  #
> > +
> gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0x|UINT64|0x69
> > +
> >  [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
> >## This value is used to set the base address of PCI express
> hierarchy.
> ># @Prompt PCI Express Base Address.
> > diff --git
> a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> > index 6fd9cbe5f6c9..601a38d6c109 100644
> > --- a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> > +++ b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> > @@ -56,3 +56,8 @@ [LibraryClasses]
> >BaseLib
> >DebugLib
> >
> > +[LibraryClasses.RISCV64]
> > +  PcdLib
> > +
> > +[Pcd.RISCV64]
> > +  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride  ## CONSUMES
> > diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> > index ac2a3c23a249..cacc38eff4f4 100644
> > --- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> > +++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> > @@ -2,6 +2,7 @@
> >RISC-V specific functionality for cache.
> >
> >Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All
> rights reserved.
> > +  Copyright (c) 2023, Rivos Inc. All rights reserved.
> >
> >SPDX-License-Identifier: BSD-2-Clause-Patent
> >  **/
> > @@ -9,10 +10,117 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> > +
> > +//
> > +// TODO: Grab cache block size and make Cache Management Operation
> > +// enabling decision based on RISC-V CPU HOB in
> > +// future when it is available and convert PcdRiscVFeatureOverride
> > +// PCD to a pointer that contains pointer to bitmap structure
> > +// which can be operated more elegantly.
> > +//
> > +#define RISCV_CACHE_BLOCK_SIZE 64
> Can we make this also as a PCD?
> [Dhaval] Actually this define should go away once we have CPU HOB. So
> thought to keep it simple for now. Anyways there is no plan to change this
> size
>
anytime in the near future

Re: [edk2-devel] [PATCH v2] MdeModulePkg/UefiBootManagerLib: Signal ReadyToBoot on platform recovery

[Neal Gompa]

On Fri, Nov 24, 2023 at 6:36 PM Neal Gompa  wrote:
>
> On Thu, Nov 2, 2023 at 6:35 AM Laszlo Ersek  wrote:
> >
> > On 10/31/23 23:27, Jeremy Linton wrote:
> > > On 10/31/23 12:37, Neal Gompa via groups.io wrote:
> > >> From: Neal Gompa 
> > >>
> > >> Currently, the ReadyToBoot event is only signaled when a formal Boot
> > >> Manager option is executed (in BmBoot.c -> EfiBootManagerBoot ()).
> > >>
> > >> However, the introduction of Platform Recovery in UEFI 2.5 makes it
> > >> necessary to signal ReadyToBoot when a Platform Recovery boot loader
> > >> runs because otherwise it may lead to the execution of a boot loader
> > >> that has similar requirements to a regular one that is not launched
> > >> as a Boot Manager option.
> > >>
> > >> This is especially critical to ensuring that the graphical console
> > >> is actually usable during platform recovery, as some platforms do
> > >> rely on the ConsolePrefDxe driver, which only performs console
> > >> initialization after ReadyToBoot is triggered.
> > >>
> > >> This patch fixes that behavior by calling EfiSignalEventReadyToBoot ()
> > >> in EfiBootManagerProcessLoadOption () when invoking platform recovery,
> > >> which is the function that sets up the platform recovery boot process.
> > >>
> > >> The expected behavior has been clarified in the UEFI 2.10 specification
> > >> to explicitly indicate this behavior is required for correct operation.
> > >>
> > >> This is a rebased version of the patch originally written by Pete Batard.
> > >
> > > Took me a bit to swap in that whole conversation again, and recheck the
> > > spec's and code paths, but this all looks fine to me and should allow
> > > the PFTF build to drop the similar patch from Pete that has been carried
> > > downstream for the past couple years.
> > >
> > > As for testing the previous patch has been in the field for a couple
> > > years now and i'm not aware of it causing any issues. The additional
> > > restriction of limiting it to platform recovery logically makes sense,
> > > and as far as I can see shouldn't cause any problems.
> > >
> > > So,
> > >
> > > Reviewed-by: Jeremy Linton 
> > >
> > >
> > > As a PS: I also went to check the ready to boot behavior for OsRecovery
> > > and realized that apparently none of that support was ever merged?
> >
> > What else is there, outside of this patch, in need of upstreaming?
> >
> > > That seems a bit of an oversight since its been in the spec for a few 
> > > years now.
> >
> > "ready-to-boot for OsRecovery" is new in UEFI 2.10 (according to the
> > commit message), which is quite recent ("Aug 29, 2022").
> >
> > I couldn't find the Mantis ticket in the Revision History (for 2.10) though.
> >
>
> Is there anything else holding up committing this patch? Jeremy and
> you reviewed it earlier in the month...
>

Friendly ping again? It's been a month now...



-- 
真実はいつも一つ！/ Always, there's only one truth!


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112157): https://edk2.groups.io/g/devel/message/112157
Mute This Topic: https://groups.io/mt/102302654/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] OutOfBandManagement/SpcrFeaturePkg: PCD based IRQ/GSI

[Abdul Lateef Attar via groups.io]

[Public]

Hi Maintainers,
Could you please review and merge this patch.
Thanks
AbduL


-Original Message-
From: Attar, AbdulLateef (Abdul Lateef)
Sent: Thursday, August 17, 2023 12:14 PM
To: Chang, Abner ; devel@edk2.groups.io
Cc: Sai Chaganty ; Isaac Oram 
; Nate DeSimone ; 
Liming Gao 
Subject: RE: [PATCH 1/1] OutOfBandManagement/SpcrFeaturePkg: PCD based IRQ/GSI

Gentle reminder, review please.

-Original Message-
From: Chang, Abner 
Sent: Thursday, August 3, 2023 12:12 PM
To: Attar, AbdulLateef (Abdul Lateef) ; 
devel@edk2.groups.io
Cc: Attar, AbdulLateef (Abdul Lateef) ; Sai Chaganty 
; Isaac Oram ; Nate 
DeSimone ; Liming Gao 
Subject: RE: [PATCH 1/1] OutOfBandManagement/SpcrFeaturePkg: PCD based IRQ/GSI

[AMD Official Use Only - General]

Acked-by: Abner Chang 

> -Original Message-
> From: Abdul Lateef Attar 
> Sent: Thursday, August 3, 2023 2:35 PM
> To: devel@edk2.groups.io
> Cc: Attar, AbdulLateef (Abdul Lateef) ; Sai
> Chaganty ; Isaac Oram
> ; Nate DeSimone
> ; Liming Gao
> ; Chang, Abner 
> Subject: [PATCH 1/1] OutOfBandManagement/SpcrFeaturePkg: PCD based
> IRQ/GSI
>
> From: Abdul Lateef Attar 
>
> Create a new PCD to hold the IRQ or GSI number for SPCR, with default
> values of 4.
> Update the ACPI SPCR table's IRQ value based on PCD.
>
> Cc: Sai Chaganty 
> Cc: Isaac Oram 
> Cc: Nate DeSimone 
> Cc: Liming Gao 
> Cc: Abner Chang 
> Signed-off-by: Abdul Lateef Attar 
>
> Change-Id: I7218903fa5572f8139ad45db598ab085f079713b
> ---
>  .../OutOfBandManagement/SpcrFeaturePkg/SpcrFeaturePkg.dec  | 5 +
>  .../SpcrFeaturePkg/SpcrAcpiDxe/SpcrAcpiDxe.inf | 4 
>  .../SpcrFeaturePkg/SpcrAcpiDxe/SpcrAcpi.h  | 3 +++
>  .../SpcrFeaturePkg/SpcrAcpiDxe/SpcrAcpi.c  | 7 ---
>  4 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git
> a/Features/Intel/OutOfBandManagement/SpcrFeaturePkg/SpcrFeaturePkg.d
> ec
> b/Features/Intel/OutOfBandManagement/SpcrFeaturePkg/SpcrFeaturePkg.d
> ec
> index b084fad89220..d69d650f3f20 100644
> ---
> a/Features/Intel/OutOfBandManagement/SpcrFeaturePkg/SpcrFeaturePkg.d
> ec
> +++
> b/Features/Intel/OutOfBandManagement/SpcrFeaturePkg/SpcrFeaturePkg.d
> ec
> @@ -7,6 +7,7 @@
>  # for the build infrastructure.
>  #
>  # Copyright (c) 2020, Intel Corporation. All rights reserved.
> +# Copyright (C) 2023 Advanced Micro Devices, Inc. All rights reserved
>  #
>  # SPDX-License-Identifier: BSD-2-Clause-Patent  # @@ -24,6 +25,10 @@
> [Includes]  [Guids]
>gSpcrFeaturePkgTokenSpaceGuid = { 0xe978c988, 0xeeba, 0x4671, {
> 0xb8, 0x0d, 0xcc, 0x8b, 0x89, 0xb5, 0xd1, 0xef }}
>
> +[PcdsFixedAtBuild]
> +  # SPCR default IRQ set to 4
> +  gSpcrFeaturePkgTokenSpaceGuid.PcdSpcrInterrupt|4|UINT8|0x0010
> +
>  [PcdsFeatureFlag]
>
> gSpcrFeaturePkgTokenSpaceGuid.PcdSpcrFeatureEnable|FALSE|BOOLEAN|0x
> 0001
>
> diff --git
> a/Features/Intel/OutOfBandManagement/SpcrFeaturePkg/SpcrAcpiDxe/Spcr
> AcpiDxe.inf
> b/Features/Intel/OutOfBandManagement/SpcrFeaturePkg/SpcrAcpiDxe/Spcr
> AcpiDxe.inf
> index 9a4f95e86bbf..cd43afea5242 100644
> ---
> a/Features/Intel/OutOfBandManagement/SpcrFeaturePkg/SpcrAcpiDxe/Spcr
> AcpiDxe.inf
> +++
> b/Features/Intel/OutOfBandManagement/SpcrFeaturePkg/SpcrAcpiDxe/Spcr
> AcpiDxe.inf
> @@ -24,6 +24,7 @@ [LibraryClasses]
>UefiDriverEntryPoint
>UefiLib
>SpcrDeviceLib
> +  PcdLib
>
>  [Packages]
>MdePkg/MdePkg.dec
> @@ -51,5 +52,8 @@ [Pcd]
>gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride
>gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio
>
> +[FixedPcd]
> +  gSpcrFeaturePkgTokenSpaceGuid.PcdSpcrInterrupt
> +
>  [Depex]
>TRUE
> diff --git
> a/Features/Intel/OutOfBandManagement/SpcrFeaturePkg/SpcrAcpiDxe/Spcr
> Acpi.h
> b/Features/Intel/OutOfBandManagement/SpcrFeaturePkg/SpcrAcpiDxe/Spcr
> Acpi.h
> index c11da439fcb8..245a847762c5 100644
> ---
> a/Features/Intel/OutOfBandManagement/SpcrFeaturePkg/SpcrAcpiDxe/Spcr
> Acpi.h
> +++
> b/Features/Intel/OutOfBandManagement/SpcrFeaturePkg/SpcrAcpiDxe/Spcr
> Acpi.h
> @@ -3,6 +3,8 @@
>SPCR is abbreviation of Serial Port Console Redirection Table (SPCR).
>
>Copyright (c) 2004 - 2020, Intel Corporation. All rights
> reserved.
> +  Copyright (C) 2023 Advanced Micro Devices, Inc. All rights reserved.
> +
>SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  **/
> @@ -23,6 +25,7 @@
>  #include 
>  #include   #include
> 
> +#include 
>
>  #include 
>  #include 
> diff --git
> a/Features/Intel/OutOfBandManagement/SpcrFeaturePkg/SpcrAcpiDxe/Spcr
> Acpi.c
> b/Features/Intel/OutOfBandManagement/SpcrFeaturePkg/SpcrAcpiDxe/Spcr
> Acpi.c
> index 51449d0fad9e..e92db96caaa9 100644
> ---
> a/Features/Intel/OutOfBandManagement/SpcrFeaturePkg/SpcrAcpiDxe/Spcr
> Acpi.c
> +++
> b/Features/Intel/OutOfBandManagement/SpcrFeaturePkg/SpcrAcpiDxe/Spcr
> Acpi.c
> @@ -3,7 +3,7 @@
>SPCR is abbreviation of Serial Port Console Redirection Table (SPCR).
>
>Copyright (c) 2004 - 2020, Intel Corporation. All rights
> reserve

Re: [edk2-devel] [PATCH 2/2] UefiPayloadPkg/UefiPayloadEntry: Add 5 level paging support

[Guo, Gua]

Reviewed-by: Gua Guo 

-Original Message-
From: Liu, Zhiguang  
Sent: Thursday, December 7, 2023 10:40 AM
To: devel@edk2.groups.io
Cc: Liu, Zhiguang ; Gao, Liming 
; Wu, Jiaxin ; Ni, Ray 
; Dong, Guo ; Rhodes, Sean 
; Lu, James ; Guo, Gua 

Subject: [PATCH 2/2] UefiPayloadPkg/UefiPayloadEntry: Add 5 level paging support

Add 5 level paging support when set the page table memory range as RO to 
protect page table.

Cc: Liming Gao 
Cc: Jiaxin Wu 
Cc: Ray Ni 
Cc: Guo Dong 
Cc: Sean Rhodes 
Cc: James Lu 
Cc: Gua Guo 
Signed-off-by: Zhiguang Liu 
---
 .../UefiPayloadEntry/Ia32/DxeLoadFunc.c   |  2 +-
 .../UefiPayloadEntry/X64/VirtualMemory.c  | 23 ---
 .../UefiPayloadEntry/X64/VirtualMemory.h  |  5 +++-
 3 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/UefiPayloadPkg/UefiPayloadEntry/Ia32/DxeLoadFunc.c 
b/UefiPayloadPkg/UefiPayloadEntry/Ia32/DxeLoadFunc.c
index 61a9f01ec9..4912298109 100644
--- a/UefiPayloadPkg/UefiPayloadEntry/Ia32/DxeLoadFunc.c
+++ b/UefiPayloadPkg/UefiPayloadEntry/Ia32/DxeLoadFunc.c
@@ -174,7 +174,7 @@ Create4GPageTablesIa32Pae (
   // Protect the page table by marking the memory used for page table to be
   // read-only.
   //
-  EnablePageTableProtection ((UINTN)PageMap, FALSE);
+  EnablePageTableProtection ((UINTN)PageMap, FALSE, FALSE);
 
   return (UINTN)PageMap;
 }
diff --git a/UefiPayloadPkg/UefiPayloadEntry/X64/VirtualMemory.c 
b/UefiPayloadPkg/UefiPayloadEntry/X64/VirtualMemory.c
index 1899404b24..8401eba83d 100644
--- a/UefiPayloadPkg/UefiPayloadEntry/X64/VirtualMemory.c
+++ b/UefiPayloadPkg/UefiPayloadEntry/X64/VirtualMemory.c
@@ -482,13 +482,15 @@ Split1GPageTo2M (
   @param[in] PageTableBaseBase address of page table (CR3).
   @param[in] Address  Start address of a page to be set as read-only.
   @param[in] Level4Paging Level 4 paging flag.
+  @param[in] Level5Paging Level 5 paging flag.
 
 **/
 VOID
 SetPageTablePoolReadOnly (
   IN  UINTN PageTableBase,
   IN  EFI_PHYSICAL_ADDRESS  Address,
-  IN  BOOLEAN   Level4Paging
+  IN  BOOLEAN   Level4Paging,
+  IN  BOOLEAN   Level5Paging
   )
 {
   UINTN Index;
@@ -498,9 +500,9 @@ SetPageTablePoolReadOnly (
   UINT64*PageTable;
   UINT64*NewPageTable;
   UINT64PageAttr;
-  UINT64LevelSize[5];
-  UINT64LevelMask[5];
-  UINTN LevelShift[5];
+  UINT64LevelSize[6];
+  UINT64LevelMask[6];
+  UINTN LevelShift[6];
   UINTN Level;
   UINT64PoolUnitSize;
 
@@ -517,23 +519,26 @@ SetPageTablePoolReadOnly (
   LevelShift[2] = PAGING_L2_ADDRESS_SHIFT;
   LevelShift[3] = PAGING_L3_ADDRESS_SHIFT;
   LevelShift[4] = PAGING_L4_ADDRESS_SHIFT;
+  LevelShift[5] = PAGING_L5_ADDRESS_SHIFT;
 
   LevelMask[1] = PAGING_4K_ADDRESS_MASK_64;
   LevelMask[2] = PAGING_2M_ADDRESS_MASK_64;
   LevelMask[3] = PAGING_1G_ADDRESS_MASK_64;
   LevelMask[4] = PAGING_1G_ADDRESS_MASK_64;
+  LevelMask[5] = 0;
 
   LevelSize[1] = SIZE_4KB;
   LevelSize[2] = SIZE_2MB;
   LevelSize[3] = SIZE_1GB;
   LevelSize[4] = SIZE_512GB;
+  LevelSize[5] = SIZE_256TB;
 
   AddressEncMask = PcdGet64 (PcdPteMemoryEncryptionAddressOrMask) &
PAGING_1G_ADDRESS_MASK_64;
   PageTable= (UINT64 *)(UINTN)PageTableBase;
   PoolUnitSize = PAGE_TABLE_POOL_UNIT_SIZE;
 
-  for (Level = (Level4Paging) ? 4 : 3; Level > 0; --Level) {
+  for (Level = Level5Paging ? 5 : (Level4Paging ? 4 : 3); Level > 0; 
+ --Level) {
 Index  = ((UINTN)RShiftU64 (Address, LevelShift[Level]));
 Index &= PAGING_PAE_INDEX_MASK;
 
@@ -604,12 +609,14 @@ SetPageTablePoolReadOnly (
 
   @param[in] PageTableBaseBase address of page table (CR3).
   @param[in] Level4Paging Level 4 paging flag.
+  @param[in] Level5Paging Level 5 paging flag.
 
 **/
 VOID
 EnablePageTableProtection (
   IN  UINTNPageTableBase,
-  IN  BOOLEAN  Level4Paging
+  IN  BOOLEAN  Level4Paging,
+  IN  BOOLEAN  Level5Paging
   )
 {
   PAGE_TABLE_POOL   *HeadPool;
@@ -638,7 +645,7 @@ EnablePageTableProtection (
 // protection to them one by one.
 //
 while (PoolSize > 0) {
-  SetPageTablePoolReadOnly (PageTableBase, Address, Level4Paging);
+  SetPageTablePoolReadOnly (PageTableBase, Address, Level4Paging, 
+ Level5Paging);
   Address  += PAGE_TABLE_POOL_UNIT_SIZE;
   PoolSize -= PAGE_TABLE_POOL_UNIT_SIZE;
 }
@@ -933,7 +940,7 @@ CreateIdentityMappingPageTables (
   // Protect the page table by marking the memory used for page table to be
   // read-only.
   //
-  EnablePageTableProtection ((UINTN)PageMap, TRUE);
+  EnablePageTableProtection ((UINTN)PageMap, !Enable5LevelPaging, 
+ Enable5LevelPaging);
 
   //
   // Set IA32_EFER.NXE if necessary.
diff --git a/UefiPayloadPkg/UefiPayloadEntry/X64/VirtualMemory.h 
b/UefiPayloadPkg/UefiPayloadEntry/X64/VirtualMemory.h
index 616ebe4

Re: [edk2-devel] TianoCore Community Meeting call for topics

[Michael D Kinney]

No topics.  Meeting canceled.

Mike

From: Kinney, Michael D 
Sent: Tuesday, December 5, 2023 3:11 PM
To: devel@edk2.groups.io
Cc: Kinney, Michael D 
Subject: TianoCore Community Meeting call for topics

Are there any topics for the TianoCore Community Meeting this week?

Thanks,

Mike


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112154): https://edk2.groups.io/g/devel/message/112154
Mute This Topic: https://groups.io/mt/103002996/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 2/2] UefiPayloadPkg/UefiPayloadEntry: Add 5 level paging support

[Zhiguang Liu]

Add 5 level paging support when set the page table
memory range as RO to protect page table.

Cc: Liming Gao 
Cc: Jiaxin Wu 
Cc: Ray Ni 
Cc: Guo Dong 
Cc: Sean Rhodes 
Cc: James Lu 
Cc: Gua Guo 
Signed-off-by: Zhiguang Liu 
---
 .../UefiPayloadEntry/Ia32/DxeLoadFunc.c   |  2 +-
 .../UefiPayloadEntry/X64/VirtualMemory.c  | 23 ---
 .../UefiPayloadEntry/X64/VirtualMemory.h  |  5 +++-
 3 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/UefiPayloadPkg/UefiPayloadEntry/Ia32/DxeLoadFunc.c 
b/UefiPayloadPkg/UefiPayloadEntry/Ia32/DxeLoadFunc.c
index 61a9f01ec9..4912298109 100644
--- a/UefiPayloadPkg/UefiPayloadEntry/Ia32/DxeLoadFunc.c
+++ b/UefiPayloadPkg/UefiPayloadEntry/Ia32/DxeLoadFunc.c
@@ -174,7 +174,7 @@ Create4GPageTablesIa32Pae (
   // Protect the page table by marking the memory used for page table to be
   // read-only.
   //
-  EnablePageTableProtection ((UINTN)PageMap, FALSE);
+  EnablePageTableProtection ((UINTN)PageMap, FALSE, FALSE);
 
   return (UINTN)PageMap;
 }
diff --git a/UefiPayloadPkg/UefiPayloadEntry/X64/VirtualMemory.c 
b/UefiPayloadPkg/UefiPayloadEntry/X64/VirtualMemory.c
index 1899404b24..8401eba83d 100644
--- a/UefiPayloadPkg/UefiPayloadEntry/X64/VirtualMemory.c
+++ b/UefiPayloadPkg/UefiPayloadEntry/X64/VirtualMemory.c
@@ -482,13 +482,15 @@ Split1GPageTo2M (
   @param[in] PageTableBaseBase address of page table (CR3).
   @param[in] Address  Start address of a page to be set as read-only.
   @param[in] Level4Paging Level 4 paging flag.
+  @param[in] Level5Paging Level 5 paging flag.
 
 **/
 VOID
 SetPageTablePoolReadOnly (
   IN  UINTN PageTableBase,
   IN  EFI_PHYSICAL_ADDRESS  Address,
-  IN  BOOLEAN   Level4Paging
+  IN  BOOLEAN   Level4Paging,
+  IN  BOOLEAN   Level5Paging
   )
 {
   UINTN Index;
@@ -498,9 +500,9 @@ SetPageTablePoolReadOnly (
   UINT64*PageTable;
   UINT64*NewPageTable;
   UINT64PageAttr;
-  UINT64LevelSize[5];
-  UINT64LevelMask[5];
-  UINTN LevelShift[5];
+  UINT64LevelSize[6];
+  UINT64LevelMask[6];
+  UINTN LevelShift[6];
   UINTN Level;
   UINT64PoolUnitSize;
 
@@ -517,23 +519,26 @@ SetPageTablePoolReadOnly (
   LevelShift[2] = PAGING_L2_ADDRESS_SHIFT;
   LevelShift[3] = PAGING_L3_ADDRESS_SHIFT;
   LevelShift[4] = PAGING_L4_ADDRESS_SHIFT;
+  LevelShift[5] = PAGING_L5_ADDRESS_SHIFT;
 
   LevelMask[1] = PAGING_4K_ADDRESS_MASK_64;
   LevelMask[2] = PAGING_2M_ADDRESS_MASK_64;
   LevelMask[3] = PAGING_1G_ADDRESS_MASK_64;
   LevelMask[4] = PAGING_1G_ADDRESS_MASK_64;
+  LevelMask[5] = 0;
 
   LevelSize[1] = SIZE_4KB;
   LevelSize[2] = SIZE_2MB;
   LevelSize[3] = SIZE_1GB;
   LevelSize[4] = SIZE_512GB;
+  LevelSize[5] = SIZE_256TB;
 
   AddressEncMask = PcdGet64 (PcdPteMemoryEncryptionAddressOrMask) &
PAGING_1G_ADDRESS_MASK_64;
   PageTable= (UINT64 *)(UINTN)PageTableBase;
   PoolUnitSize = PAGE_TABLE_POOL_UNIT_SIZE;
 
-  for (Level = (Level4Paging) ? 4 : 3; Level > 0; --Level) {
+  for (Level = Level5Paging ? 5 : (Level4Paging ? 4 : 3); Level > 0; --Level) {
 Index  = ((UINTN)RShiftU64 (Address, LevelShift[Level]));
 Index &= PAGING_PAE_INDEX_MASK;
 
@@ -604,12 +609,14 @@ SetPageTablePoolReadOnly (
 
   @param[in] PageTableBaseBase address of page table (CR3).
   @param[in] Level4Paging Level 4 paging flag.
+  @param[in] Level5Paging Level 5 paging flag.
 
 **/
 VOID
 EnablePageTableProtection (
   IN  UINTNPageTableBase,
-  IN  BOOLEAN  Level4Paging
+  IN  BOOLEAN  Level4Paging,
+  IN  BOOLEAN  Level5Paging
   )
 {
   PAGE_TABLE_POOL   *HeadPool;
@@ -638,7 +645,7 @@ EnablePageTableProtection (
 // protection to them one by one.
 //
 while (PoolSize > 0) {
-  SetPageTablePoolReadOnly (PageTableBase, Address, Level4Paging);
+  SetPageTablePoolReadOnly (PageTableBase, Address, Level4Paging, 
Level5Paging);
   Address  += PAGE_TABLE_POOL_UNIT_SIZE;
   PoolSize -= PAGE_TABLE_POOL_UNIT_SIZE;
 }
@@ -933,7 +940,7 @@ CreateIdentityMappingPageTables (
   // Protect the page table by marking the memory used for page table to be
   // read-only.
   //
-  EnablePageTableProtection ((UINTN)PageMap, TRUE);
+  EnablePageTableProtection ((UINTN)PageMap, !Enable5LevelPaging, 
Enable5LevelPaging);
 
   //
   // Set IA32_EFER.NXE if necessary.
diff --git a/UefiPayloadPkg/UefiPayloadEntry/X64/VirtualMemory.h 
b/UefiPayloadPkg/UefiPayloadEntry/X64/VirtualMemory.h
index 616ebe42b0..f2a5cbec33 100644
--- a/UefiPayloadPkg/UefiPayloadEntry/X64/VirtualMemory.h
+++ b/UefiPayloadPkg/UefiPayloadEntry/X64/VirtualMemory.h
@@ -157,6 +157,7 @@ typedef union {
 #define PAGING_L2_ADDRESS_SHIFT  21
 #define PAGING_L3_ADDRESS_SHIFT  30
 #define PAGING_L4_ADDRESS_SHIFT  39
+#define PAGING_L5_ADDRESS_SHIFT  48
 
 #define PAGIN

[edk2-devel] [PATCH 1/2] MdeModulePkg/DxeIpl: Add 5 level paging support

[Zhiguang Liu]

Add 5 level paging support when set the page table
memory range as RO to protect page table.

Cc: Liming Gao 
Cc: Jiaxin Wu 
Cc: Ray Ni 
Cc: Guo Dong 
Cc: Sean Rhodes 
Cc: James Lu 
Cc: Gua Guo 
Signed-off-by: Zhiguang Liu 
---
 .../Core/DxeIplPeim/Ia32/DxeLoadFunc.c|  2 +-
 .../Core/DxeIplPeim/X64/VirtualMemory.c   | 23 ---
 .../Core/DxeIplPeim/X64/VirtualMemory.h   |  5 +++-
 3 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c 
b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
index 65e9bdc99e..ba871dafc7 100644
--- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
+++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
@@ -166,7 +166,7 @@ Create4GPageTablesIa32Pae (
   // Protect the page table by marking the memory used for page table to be
   // read-only.
   //
-  EnablePageTableProtection ((UINTN)PageMap, FALSE);
+  EnablePageTableProtection ((UINTN)PageMap, FALSE, FALSE);
 
   return (UINTN)PageMap;
 }
diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c 
b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
index 980c2002d4..1c2e29b132 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
@@ -486,13 +486,15 @@ Split1GPageTo2M (
   @param[in] PageTableBaseBase address of page table (CR3).
   @param[in] Address  Start address of a page to be set as read-only.
   @param[in] Level4Paging Level 4 paging flag.
+  @param[in] Level5Paging Level 5 paging flag.
 
 **/
 VOID
 SetPageTablePoolReadOnly (
   IN  UINTN PageTableBase,
   IN  EFI_PHYSICAL_ADDRESS  Address,
-  IN  BOOLEAN   Level4Paging
+  IN  BOOLEAN   Level4Paging,
+  IN  BOOLEAN   Level5Paging
   )
 {
   UINTN Index;
@@ -502,9 +504,9 @@ SetPageTablePoolReadOnly (
   UINT64*PageTable;
   UINT64*NewPageTable;
   UINT64PageAttr;
-  UINT64LevelSize[5];
-  UINT64LevelMask[5];
-  UINTN LevelShift[5];
+  UINT64LevelSize[6];
+  UINT64LevelMask[6];
+  UINTN LevelShift[6];
   UINTN Level;
   UINT64PoolUnitSize;
 
@@ -521,23 +523,26 @@ SetPageTablePoolReadOnly (
   LevelShift[2] = PAGING_L2_ADDRESS_SHIFT;
   LevelShift[3] = PAGING_L3_ADDRESS_SHIFT;
   LevelShift[4] = PAGING_L4_ADDRESS_SHIFT;
+  LevelShift[5] = PAGING_L5_ADDRESS_SHIFT;
 
   LevelMask[1] = PAGING_4K_ADDRESS_MASK_64;
   LevelMask[2] = PAGING_2M_ADDRESS_MASK_64;
   LevelMask[3] = PAGING_1G_ADDRESS_MASK_64;
   LevelMask[4] = PAGING_1G_ADDRESS_MASK_64;
+  LevelMask[5] = 0;
 
   LevelSize[1] = SIZE_4KB;
   LevelSize[2] = SIZE_2MB;
   LevelSize[3] = SIZE_1GB;
   LevelSize[4] = SIZE_512GB;
+  LevelSize[5] = SIZE_256TB;
 
   AddressEncMask = PcdGet64 (PcdPteMemoryEncryptionAddressOrMask) &
PAGING_1G_ADDRESS_MASK_64;
   PageTable= (UINT64 *)(UINTN)PageTableBase;
   PoolUnitSize = PAGE_TABLE_POOL_UNIT_SIZE;
 
-  for (Level = (Level4Paging) ? 4 : 3; Level > 0; --Level) {
+  for (Level = Level5Paging ? 5 : (Level4Paging ? 4 : 3); Level > 0; --Level) {
 Index  = ((UINTN)RShiftU64 (Address, LevelShift[Level]));
 Index &= PAGING_PAE_INDEX_MASK;
 
@@ -608,12 +613,14 @@ SetPageTablePoolReadOnly (
 
   @param[in] PageTableBaseBase address of page table (CR3).
   @param[in] Level4Paging Level 4 paging flag.
+  @param[in] Level5Paging Level 5 paging flag.
 
 **/
 VOID
 EnablePageTableProtection (
   IN  UINTNPageTableBase,
-  IN  BOOLEAN  Level4Paging
+  IN  BOOLEAN  Level4Paging,
+  IN  BOOLEAN  Level5Paging
   )
 {
   PAGE_TABLE_POOL   *HeadPool;
@@ -642,7 +649,7 @@ EnablePageTableProtection (
 // protection to them one by one.
 //
 while (PoolSize > 0) {
-  SetPageTablePoolReadOnly (PageTableBase, Address, Level4Paging);
+  SetPageTablePoolReadOnly (PageTableBase, Address, Level4Paging, 
Level5Paging);
   Address  += PAGE_TABLE_POOL_UNIT_SIZE;
   PoolSize -= PAGE_TABLE_POOL_UNIT_SIZE;
 }
@@ -959,7 +966,7 @@ CreateIdentityMappingPageTables (
   // Protect the page table by marking the memory used for page table to be
   // read-only.
   //
-  EnablePageTableProtection ((UINTN)PageMap, TRUE);
+  EnablePageTableProtection ((UINTN)PageMap, !Page5LevelSupport, 
Page5LevelSupport);
 
   //
   // Set IA32_EFER.NXE if necessary.
diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h 
b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h
index 616ebe42b0..f2a5cbec33 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h
@@ -157,6 +157,7 @@ typedef union {
 #define PAGING_L2_ADDRESS_SHIFT  21
 #define PAGING_L3_ADDRESS_SHIFT  30
 #define PAGING_L4_ADDRESS_SHIFT  39
+#define PAGING_L5_ADDRESS_SHIFT  48
 
 #define PAGING_PML4E_NUMBER  4
 
@@ -294,12 +295,14

Re: [edk2-devel] [edk2-redfish-client][PATCH] RedfishClientPkg/RedfishFeatureCoreDxe: fix Redfish event issue.

[Chang, Abner via groups.io]

[AMD Official Use Only - General]

Ah sorry, I missed that one.

Reviewed-by: Abner Chang 

> -Original Message-
> From: Nickle Wang 
> Sent: Thursday, December 7, 2023 7:46 AM
> To: Chang, Abner ; devel@edk2.groups.io
> Cc: Igor Kulchytskyy ; Nick Ramirez 
> Subject: RE: [edk2-redfish-client][PATCH]
> RedfishClientPkg/RedfishFeatureCoreDxe: fix Redfish event issue.
>
> [AMD Official Use Only - General]
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Hi Abner,
>
> > If we use PCD here, then we have to also update CreateEventEx in the
> RedfishFeatureCoreEntryPoint. Create the event using
> REDFISH_FEATURE_CORE_TPL.
>
> Yes, I also modify RedfishFeatureCoreEntryPoint in below together.
>
> >  }
> >
> >  /**
> > @@ -670,7 +682,7 @@ RedfishFeatureCoreEntryPoint (
> >
> >Status = gBS->CreateEventEx (
> >EVT_NOTIFY_SIGNAL,
> > -  TPL_CALLBACK,
> > +  REDFISH_FEATURE_CORE_TPL,
> >RedfishFeatureDriverStartup,
> >(CONST VOID *)&mFeatureDriverStartupContext,
> >EventGuid,
>
> Regards,
> Nickle
>
> > -Original Message-
> > From: Chang, Abner 
> > Sent: Wednesday, December 6, 2023 8:08 PM
> > To: Nickle Wang ; devel@edk2.groups.io
> > Cc: Igor Kulchytskyy ; Nick Ramirez
> 
> > Subject: RE: [edk2-redfish-client][PATCH]
> > RedfishClientPkg/RedfishFeatureCoreDxe: fix Redfish event issue.
> >
> > External email: Use caution opening links or attachments
> >
> >
> > [AMD Official Use Only - General]
> >
> > Hi Nickle, one comment below.
> >
> > > -Original Message-
> > > From: Nickle Wang 
> > > Sent: Wednesday, December 6, 2023 4:57 PM
> > > To: devel@edk2.groups.io
> > > Cc: Chang, Abner ; Igor Kulchytskyy
> > > ; Nick Ramirez 
> > > Subject: [edk2-redfish-client][PATCH]
> > > RedfishClientPkg/RedfishFeatureCoreDxe: fix Redfish event issue.
> > >
> > > Caution: This message originated from an External Source. Use proper
> > > caution when opening attachments, clicking links, or responding.
> > >
> > >
> > > RedfishFeatureDriverStartup is callback function at TPL_CALLBACK
> > > level. In this function, Redfish events are signaled. However, Redfish
> > > events are created in TPL_CALLBACK level too. As the result, Redfish
> > > events cannot be invoked in desired sequence. Decrease the TPL to
> > > TPL_APPLICATION level inside RedfishFeatureDriverStartup and restore
> > > it to TPL_CALLBACK level before leaving this function. Now, Redfish
> > > events are called in correct sequence.
> > >
> > > Signed-off-by: Nickle Wang 
> > > Cc: Abner Chang 
> > > Cc: Igor Kulchytskyy 
> > > Cc: Nick Ramirez 
> > > ---
> > >  .../RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.h  |  1 +
> > > .../RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.c  | 14
> +-
> > >  2 files changed, 14 insertions(+), 1 deletion(-)
> > >
> > > diff --git
> > > a/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.h
> > > b/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.h
> > > index acefa41b..de08d79d 100644
> > > --- a/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.h
> > > +++ b/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.h
> > > @@ -33,6 +33,7 @@
> > >  #define NodeIsCollectionLeftBracket   L'{'
> > >  #define NodeIsCollectionRightBracket  L'}'
> > >  #define NodeIsCollectionSymbolL"/{}"
> > > +#define REDFISH_FEATURE_CORE_TPL  TPL_CALLBACK
> > >
> > >  typedef struct _REDFISH_FEATURE_INTERNAL_DATA
> > > REDFISH_FEATURE_INTERNAL_DATA;  struct
> > _REDFISH_FEATURE_INTERNAL_DATA
> > > { diff --git
> > > a/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.c
> > > b/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.c
> > > index f3188ddf..c0c3ec47 100644
> > > --- a/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.c
> > > +++ b/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.c
> > > @@ -272,6 +272,13 @@ RedfishFeatureDriverStartup (
> > >  return;
> > >}
> > >
> > > +  //
> > > +  // Lower the TPL to TPL_APPLICATION so that  // Redfish event and
> > > + report status code can be  // triggered  //  gBS->RestoreTPL
> > > + (TPL_APPLICATION);
> > > +
> > >//
> > >// Reset PcdRedfishSystemRebootRequired flag
> > >//
> > > @@ -321,6 +328,11 @@ RedfishFeatureDriverStartup (
> > >  gRT->ResetSystem (EfiResetCold, EFI_SUCCESS, 0, NULL);
> > >  CpuDeadLoop ();
> > >}
> > > +
> > > +  //
> > > +  // Restore to the TPL where this callback handler is called.
> > > +  //
> > > +  gBS->RaiseTPL (REDFISH_FEATURE_CORE_TPL);
> > If we use PCD here, then we have to also update CreateEventEx in the
> > RedfishFeatureCoreEntryPoint. Create the event using
> > REDFISH_FEATURE_CORE_TPL.
> >
> > Abner
> >
> >
> > >  }
> > >
> > >  /**
> > > @@ -670,7 +682,7 @@ RedfishFeatureCoreEntryPoint (
> > >
> > 

Re: [edk2-devel] [PATCH 3/6] UefiCpuPkg: Consume MpInfo2Hob in PiSmmCpuDxe

[Ni, Ray]

> -Original Message-
> From: Tan, Dun 
> Sent: Thursday, December 7, 2023 8:23 AM
> To: Ni, Ray ; devel@edk2.groups.io
> Cc: Dong, Eric ; Kumar, Rahul R
> ; Gerd Hoffmann 
> Subject: RE: [PATCH 3/6] UefiCpuPkg: Consume MpInfo2Hob in PiSmmCpuDxe
> 
> Will change the code based on comments 1-9.
> 
> About comments 10, "10. The depex change means that CpuSmm driver could
> run before CpuMp driver runs. Have you verified if CpuSmm can start well
> even removing CpuMp DXE driver?"
> 
> Yes, I verified it in OvmfIa32X64 boot. CpuSmm can start well even removing
> CpuMp DXE driver.
Great!

> (also removed some checking for gEfiCpuArchProtocolGuid)
I assume the checking for CpuArch protocol is from other modules.


> 
> Thanks,
> Dun
> 
> -Original Message-
> From: Ni, Ray 
> Sent: Wednesday, December 6, 2023 5:55 PM
> To: Tan, Dun ; devel@edk2.groups.io
> Cc: Dong, Eric ; Kumar, Rahul R
> ; Gerd Hoffmann 
> Subject: RE: [PATCH 3/6] UefiCpuPkg: Consume MpInfo2Hob in PiSmmCpuDxe
> 
> 1. The function name can be "GetMpInformation()" without mentioning
> "FromMpInfo2Hob".
> 
> > +  EFI_HOB_GUID_TYPE  *GuidHob;
> > +  EFI_HOB_GUID_TYPE  *FirstMpInfor2Hob;
> 
> 2. "FirstMpInfo2Hob". Please remove "r".
> 
> >+  FirstMpInfor2Hob = GetFirstGuidHob (&gMpInformationHobGuid2);
> 
> 3. Please update comments to explain "FirstMpInfo2Hob" is to speed up the
> 2nd while-loop without needing to look for MpInfo2Hob from beginning.
> 
> > +
> > +  ASSERT (CpuCount <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
> > +  *NumberOfCpus = CpuCount;
> 
> 4. There is no "return" before "*NumberOfCpus" assignment. So, why not
> remove "CpuCount" and directly udpates "*NumberOfCpus" in the
> while-loop?
> 
> > +
> > +  MpInfomation2Buffer = AllocatePool (sizeof
> > (MP_INFORMATION2_HOB_DATA *) * HobCount);
> 
> 5. MpInfomation2Buffer -> MpInfo2Hobs
> 
> 
> 6. Can you move "PrevProcessorIndex" assignment just above the "for" loop?
> 
> > +  for (Index = 0; Index < HobCount; Index++) {
> 7. Index -> HobIndex
> 
> > +  CopyMem (
> > +ProcessorInfo + PrevProcessorIndex + ProcessorIndex,
> 
> 8. &ProcessorInfo[PrevProcessorIndex + ProcessorIndex]
> 
> > +
> > +  *ProcessorInfoPointer = ProcessorInfo;
> 
> 9. If you let the function just return ProcessorInfo and NULL when failure
> happens, will it simplify the code?
> 
> >
> > +[Depex]
> > +  TRUE
> > -[Depex]
> > -  gEfiMpServiceProtocolGuid
> 
> 10. The depex change means that CpuSmm driver could run before CpuMp
> driver runs. Have you verified if CpuSmm can start well even removing CpuMp
> DXE driver?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112150): https://edk2.groups.io/g/devel/message/112150
Mute This Topic: https://groups.io/mt/102987139/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 6/6] UefiCpuPkg: Avoid assuming only one smmbasehob

[Ni, Ray]

> 
> 2. We could break the while-loop when NumberOfProcessors equals to the
> value we retrieved from MpInfo2Hob. Right?
> This can speed up the code when there are lots of HOBs after the last
> SmmBaseHob instance.
> 
> Dun: If the code flow break before finding all potential SmmBaseHob instance,
> there may be more SmmBaseHob instance covering NumberOfProcessors
> more than the expected value. The code is to catch this case. Do you think we
> should also catch this?

When there are more instances than expected, we treat it as a failure.
When number of instances is expected, we treat gap or overlap as a failure.
We do not need to distinguish between the two kinds of failures.
So, we could assume the number of instances is expected, then check if all the
found instances can cover all processors.
If not, we treat it as a failure.
> 
> > +  }
> > +
> > +  ASSERT (NumberOfProcessors == MaxNumberOfCpus);
> 
> 3. ASSERT may fail when HotPlug is TRUE?
> 
> Dun: If HotPlug, I think the SmBase count should be
> PcdCpuMaxLogicalProcessorNumber instead of the NumberOfProcessors
> extracted from MpInfo2Hob?

You are right!


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112149): https://edk2.groups.io/g/devel/message/112149
Mute This Topic: https://groups.io/mt/102987142/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 6/6] UefiCpuPkg: Avoid assuming only one smmbasehob

[duntan]

Updated in original message.

Thanks,
Dun

-Original Message-
From: Ni, Ray  
Sent: Wednesday, December 6, 2023 6:15 PM
To: Tan, Dun ; devel@edk2.groups.io
Cc: Dong, Eric ; Kumar, Rahul R ; 
Gerd Hoffmann 
Subject: RE: [PATCH 6/6] UefiCpuPkg: Avoid assuming only one smmbasehob

> +EFI_STATUS
> +GetSmBaseFromSmmBaseHob (
> +  IN  EFI_HOB_GUID_TYPE  *FirstSmmBaseGuidHob,
> +  IN  UINTN  MaxNumberOfCpus,
> +  OUT UINTN  **SmBaseBufferPointer
> +  )

1. It's a bit strange that caller should locate the first GuidHob.
Can you update the existing code as follows:
mCpuHotPlugData.SmBase = GetSmBase(mMaxNumberOfCpus); if 
(mCpuHotPlugData.SmBase != NULL) {
  mSmmRelocated = TRUE;
}

Dun: Ok, will change code to this.

> +{
> +  UINTN  HobCount;
> +  EFI_HOB_GUID_TYPE  *GuidHob;
> +  SMM_BASE_HOB_DATA  *SmmBaseHobData;
> +  UINTN  NumberOfProcessors;
> +  SMM_BASE_HOB_DATA  **SmBaseHobPointerBuffer;
> +  UINTN  *SmBaseBuffer;
> +  UINTN  Index;
> +  UINTN  SortBuffer;
> +  UINTN  ProcessorIndex;
> +  UINT64 PrevProcessorIndex;
> +
> +  SmmBaseHobData = NULL;
> +  Index  = 0;
> +  ProcessorIndex = 0;
> +  PrevProcessorIndex = 0;
> +  HobCount   = 0;
> +  NumberOfProcessors = 0;
> +  GuidHob= FirstSmmBaseGuidHob;
> +
> +  while (GuidHob != NULL) {
> +HobCount++;
> +SmmBaseHobData  = GET_GUID_HOB_DATA (GuidHob);
> +NumberOfProcessors += SmmBaseHobData->NumberOfProcessors;
> +GuidHob = GetNextGuidHob (&gSmmBaseHobGuid,
> GET_NEXT_HOB (GuidHob));

2. We could break the while-loop when NumberOfProcessors equals to the value we 
retrieved from MpInfo2Hob. Right?
This can speed up the code when there are lots of HOBs after the last 
SmmBaseHob instance.

Dun: If the code flow break before finding all potential SmmBaseHob instance, 
there may be more SmmBaseHob instance covering NumberOfProcessors more than the 
expected value. The code is to catch this case. Do you think we should also 
catch this?

> +  }
> +
> +  ASSERT (NumberOfProcessors == MaxNumberOfCpus);

3. ASSERT may fail when HotPlug is TRUE?

Dun: If HotPlug, I think the SmBase count should be 
PcdCpuMaxLogicalProcessorNumber instead of the NumberOfProcessors extracted 
from MpInfo2Hob?


> +
> +  SmBaseHobPointerBuffer = AllocatePool (sizeof (SMM_BASE_HOB_DATA *)
> * HobCount);

4. SmBaseHobPointerBuffer -> SmBaseHobs

Dun: will change the naming.

> +  for (Index = 0; Index < HobCount; Index++) {
> +//
> +// Make sure no overlap and no gap in the CPU range covered by 
> + each
> HOB
> +//
> +ASSERT (SmBaseHobPointerBuffer[Index]->ProcessorIndex ==
> PrevProcessorIndex);

5. similarly, can you move "PrevProcessorIndex" assignment to just above "for"?

Dun: Will change the code

> +
> +//
> +// Cache each SmBase in order.
> +//
> +if (sizeof (UINTN) == sizeof (UINT64)) {
> +  CopyMem (
> +SmBaseBuffer + PrevProcessorIndex,
> +&SmBaseHobPointerBuffer[Index]->SmBase,
> +sizeof (UINT64) *
> SmBaseHobPointerBuffer[Index]->NumberOfProcessors
> +);
> +} else {
> +  for (ProcessorIndex = 0; ProcessorIndex <
> SmBaseHobPointerBuffer[Index]->NumberOfProcessors; ProcessorIndex++) {
> +SmBaseBuffer[PrevProcessorIndex + ProcessorIndex] =
> (UINTN)SmBaseHobPointerBuffer[Index]->SmBase[ProcessorIndex];
> +  }
> +}


6. I don't like the "if-else" above. Can you just change SmBaseBuffer to UINT64 
*?
Or, you always use for-loop to copy SmBase value for each cpu.

Dun: Ok, will always use for-loop to copy SmBase value for each cpu.

> +
> +PrevProcessorIndex +=
> SmBaseHobPointerBuffer[Index]->NumberOfProcessors;
> +  }
> +
> +  FreePool (SmBaseHobPointerBuffer);
> +
> +  *SmBaseBufferPointer = SmBaseBuffer;

7. Similarly, how about return SmBaseBuffer?

Dun: Ok, will change the code


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112148): https://edk2.groups.io/g/devel/message/112148
Mute This Topic: https://groups.io/mt/102987142/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 5/6] UefiCpuPkg: Cache core type in MpInfo2 HOB

[duntan]

Will change the commit based on the comments

Thanks,
Dun

-Original Message-
From: Ni, Ray  
Sent: Wednesday, December 6, 2023 6:02 PM
To: Tan, Dun ; devel@edk2.groups.io
Cc: Dong, Eric ; Kumar, Rahul R ; 
Gerd Hoffmann 
Subject: RE: [PATCH 5/6] UefiCpuPkg: Cache core type in MpInfo2 HOB

>  /**
>Create gMpInformationHobGuid2.
>  **/
> @@ -558,13 +582,36 @@ BuildMpInformationHob (
>MP_INFORMATION2_HOB_DATA  *MpInformation2HobData;
>MP_INFORMATION2_ENTRY *MpInformation2Entry;
>UINTN Index;
> +  UINT8 *CoreType;
> +  UINT32CpuidMaxInput;
> +  UINTN Pages;

1. CoreTypes, CoreTypePages


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112147): https://edk2.groups.io/g/devel/message/112147
Mute This Topic: https://groups.io/mt/102987141/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 3/6] UefiCpuPkg: Consume MpInfo2Hob in PiSmmCpuDxe

[duntan]

Will change the code based on comments 1-9.

About comments 10, "10. The depex change means that CpuSmm driver could run 
before CpuMp driver runs. Have you verified if CpuSmm can start well even 
removing CpuMp DXE driver?"

Yes, I verified it in OvmfIa32X64 boot. CpuSmm can start well even removing 
CpuMp DXE driver.(also removed some checking for gEfiCpuArchProtocolGuid)

Thanks,
Dun

-Original Message-
From: Ni, Ray  
Sent: Wednesday, December 6, 2023 5:55 PM
To: Tan, Dun ; devel@edk2.groups.io
Cc: Dong, Eric ; Kumar, Rahul R ; 
Gerd Hoffmann 
Subject: RE: [PATCH 3/6] UefiCpuPkg: Consume MpInfo2Hob in PiSmmCpuDxe

1. The function name can be "GetMpInformation()" without mentioning 
"FromMpInfo2Hob".

> +  EFI_HOB_GUID_TYPE  *GuidHob;
> +  EFI_HOB_GUID_TYPE  *FirstMpInfor2Hob;

2. "FirstMpInfo2Hob". Please remove "r".

>+  FirstMpInfor2Hob = GetFirstGuidHob (&gMpInformationHobGuid2);

3. Please update comments to explain "FirstMpInfo2Hob" is to speed up the 2nd 
while-loop without needing to look for MpInfo2Hob from beginning.

> +
> +  ASSERT (CpuCount <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
> +  *NumberOfCpus = CpuCount;

4. There is no "return" before "*NumberOfCpus" assignment. So, why not remove 
"CpuCount" and directly udpates "*NumberOfCpus" in the while-loop?

> +
> +  MpInfomation2Buffer = AllocatePool (sizeof
> (MP_INFORMATION2_HOB_DATA *) * HobCount);

5. MpInfomation2Buffer -> MpInfo2Hobs


6. Can you move "PrevProcessorIndex" assignment just above the "for" loop?

> +  for (Index = 0; Index < HobCount; Index++) {
7. Index -> HobIndex

> +  CopyMem (
> +ProcessorInfo + PrevProcessorIndex + ProcessorIndex,

8. &ProcessorInfo[PrevProcessorIndex + ProcessorIndex]

> +
> +  *ProcessorInfoPointer = ProcessorInfo;

9. If you let the function just return ProcessorInfo and NULL when failure 
happens, will it simplify the code?

> 
> +[Depex]
> +  TRUE
> -[Depex]
> -  gEfiMpServiceProtocolGuid

10. The depex change means that CpuSmm driver could run before CpuMp driver 
runs. Have you verified if CpuSmm can start well even removing CpuMp DXE driver?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112146): https://edk2.groups.io/g/devel/message/112146
Mute This Topic: https://groups.io/mt/102987139/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH] MdeModulePkg/ResetSystemRuntimeDxe: Print Reset Data

[Ashish Singhal via groups.io]

ResetSystem runtime call allows for sending reset data that
starts with a NULL terminated string. Add support to print
that string on console.

Signed-off-by: Ashish Singhal 
---
 .../Universal/ResetSystemRuntimeDxe/ResetSystem.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystem.c 
b/MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystem.c
index 42f1b1d015..72bb1d2be6 100644
--- a/MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystem.c
+++ b/MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystem.c
@@ -252,6 +252,14 @@ RuntimeServiceResetSystem (
 mResetNotifyDepth
 ));
 
+  if ((ResetData != NULL) && (DataSize != 0)) {
+DEBUG ((
+  DEBUG_INFO,
+  "DXE ResetSystem2: ResetData: %s\n",
+  ResetData
+  ));
+  }
+
   if (mResetNotifyDepth <= MAX_RESET_NOTIFY_DEPTH) {
 if (!EfiAtRuntime ()) {
   //
-- 
2.17.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112145): https://edk2.groups.io/g/devel/message/112145
Mute This Topic: https://groups.io/mt/103025596/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/6] UefiCpuPkg: Build MpInfo2HOB in CpuMpPei

[duntan]

Will change the commit based on the comments.

Thanks,
Dun

-Original Message-
From: Ni, Ray  
Sent: Wednesday, December 6, 2023 5:24 PM
To: Tan, Dun ; devel@edk2.groups.io
Cc: Dong, Eric ; Kumar, Rahul R ; 
Gerd Hoffmann 
Subject: RE: [PATCH 2/6] UefiCpuPkg: Build MpInfo2HOB in CpuMpPei

4 minor comments:

> +DEBUG ((DEBUG_INFO, "BuildMpInformationHob\n"));

1. DEBUG ("Creating MpInformation2 HOB...\n")

> +
> +for (Index = 0; Index < NumberOfProcessorsInHob; Index++) {
> +  MpInformation2Entry = GET_MP_INFORMATION_ENTRY
> (MpInformation2HobData, Index);

2. Since EntrySize equals to sizeof (MP_INFORMATION2_ENTRY), is it ok to just 
use MpInformation2HobData->Entry[Index]?

3. Do you think "Entry[0]" is more proper than "MpInformation[0]"?

> +  DEBUG ((
> +DEBUG_INFO,
> +"ProcessorIndex = %x, ProcessorId = %lx, StatusFlag = %x\n",
> +Index + ProcessorIndex,
> +MpInformation2Entry->ProcessorInfo.ProcessorId,
> +MpInformation2Entry->ProcessorInfo.StatusFlag
> +));

4. How about the debug messages are as follows:
Processor[]: ProcessorId = 0x, StatusFlag = 
0x0001\n Location = Package:0 Core:0 Thread:0\n
Location2 = Package:0 Die:0 Tile:0 Module:0 Core:0 
Thread:0\n If a number has "0x" prefix, it uses hex format, otherwise it uses 
dec format. The debug message should clearly tell what format the number 
follows.
Extra 2 spaces in Location/Location2 are to tell that these are extra info for 
the processor #0.



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112144): https://edk2.groups.io/g/devel/message/112144
Mute This Topic: https://groups.io/mt/102987138/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg: Create gMpInformationHobGuid2 in UefiCpuPkg

[duntan]

Thanks for the comments.
Will remove this field.

Thanks,
Dun

-Original Message-
From: Ni, Ray  
Sent: Wednesday, December 6, 2023 5:09 PM
To: Tan, Dun ; devel@edk2.groups.io
Cc: Dong, Eric ; Kumar, Rahul R ; 
Gerd Hoffmann 
Subject: RE: [PATCH 1/6] UefiCpuPkg: Create gMpInformationHobGuid2 in UefiCpuPkg

> +  MP information protocol only provides static information of MP processor.
> +
> +  If SwitchBSP or Enable/DisableAP in MP service is called between 
> + the HOB  production and HOB consumption,
> EFI_PROCESSOR_INFORMATION.StatusFlag and
> +  NumberOfEnabledProcessors fields in this HOB may be invalidated.

1. There is no NumberOfEnabledProcessors field.




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112143): https://edk2.groups.io/g/devel/message/112143
Mute This Topic: https://groups.io/mt/102987137/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-redfish-client][PATCH] RedfishClientPkg/RedfishFeatureCoreDxe: fix Redfish event issue.

[Nickle Wang via groups.io]

Hi Abner,

> If we use PCD here, then we have to also update CreateEventEx in the 
> RedfishFeatureCoreEntryPoint. Create the event using   
> REDFISH_FEATURE_CORE_TPL.

Yes, I also modify RedfishFeatureCoreEntryPoint in below together.

>  }
>
>  /**
> @@ -670,7 +682,7 @@ RedfishFeatureCoreEntryPoint (
>
>Status = gBS->CreateEventEx (
>EVT_NOTIFY_SIGNAL,
> -  TPL_CALLBACK,
> +  REDFISH_FEATURE_CORE_TPL,
>RedfishFeatureDriverStartup,
>(CONST VOID *)&mFeatureDriverStartupContext,
>EventGuid,

Regards,
Nickle

> -Original Message-
> From: Chang, Abner 
> Sent: Wednesday, December 6, 2023 8:08 PM
> To: Nickle Wang ; devel@edk2.groups.io
> Cc: Igor Kulchytskyy ; Nick Ramirez 
> Subject: RE: [edk2-redfish-client][PATCH]
> RedfishClientPkg/RedfishFeatureCoreDxe: fix Redfish event issue.
> 
> External email: Use caution opening links or attachments
> 
> 
> [AMD Official Use Only - General]
> 
> Hi Nickle, one comment below.
> 
> > -Original Message-
> > From: Nickle Wang 
> > Sent: Wednesday, December 6, 2023 4:57 PM
> > To: devel@edk2.groups.io
> > Cc: Chang, Abner ; Igor Kulchytskyy
> > ; Nick Ramirez 
> > Subject: [edk2-redfish-client][PATCH]
> > RedfishClientPkg/RedfishFeatureCoreDxe: fix Redfish event issue.
> >
> > Caution: This message originated from an External Source. Use proper
> > caution when opening attachments, clicking links, or responding.
> >
> >
> > RedfishFeatureDriverStartup is callback function at TPL_CALLBACK
> > level. In this function, Redfish events are signaled. However, Redfish
> > events are created in TPL_CALLBACK level too. As the result, Redfish
> > events cannot be invoked in desired sequence. Decrease the TPL to
> > TPL_APPLICATION level inside RedfishFeatureDriverStartup and restore
> > it to TPL_CALLBACK level before leaving this function. Now, Redfish
> > events are called in correct sequence.
> >
> > Signed-off-by: Nickle Wang 
> > Cc: Abner Chang 
> > Cc: Igor Kulchytskyy 
> > Cc: Nick Ramirez 
> > ---
> >  .../RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.h  |  1 +
> > .../RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.c  | 14 +-
> >  2 files changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git
> > a/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.h
> > b/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.h
> > index acefa41b..de08d79d 100644
> > --- a/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.h
> > +++ b/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.h
> > @@ -33,6 +33,7 @@
> >  #define NodeIsCollectionLeftBracket   L'{'
> >  #define NodeIsCollectionRightBracket  L'}'
> >  #define NodeIsCollectionSymbolL"/{}"
> > +#define REDFISH_FEATURE_CORE_TPL  TPL_CALLBACK
> >
> >  typedef struct _REDFISH_FEATURE_INTERNAL_DATA
> > REDFISH_FEATURE_INTERNAL_DATA;  struct
> _REDFISH_FEATURE_INTERNAL_DATA
> > { diff --git
> > a/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.c
> > b/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.c
> > index f3188ddf..c0c3ec47 100644
> > --- a/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.c
> > +++ b/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.c
> > @@ -272,6 +272,13 @@ RedfishFeatureDriverStartup (
> >  return;
> >}
> >
> > +  //
> > +  // Lower the TPL to TPL_APPLICATION so that  // Redfish event and
> > + report status code can be  // triggered  //  gBS->RestoreTPL
> > + (TPL_APPLICATION);
> > +
> >//
> >// Reset PcdRedfishSystemRebootRequired flag
> >//
> > @@ -321,6 +328,11 @@ RedfishFeatureDriverStartup (
> >  gRT->ResetSystem (EfiResetCold, EFI_SUCCESS, 0, NULL);
> >  CpuDeadLoop ();
> >}
> > +
> > +  //
> > +  // Restore to the TPL where this callback handler is called.
> > +  //
> > +  gBS->RaiseTPL (REDFISH_FEATURE_CORE_TPL);
> If we use PCD here, then we have to also update CreateEventEx in the
> RedfishFeatureCoreEntryPoint. Create the event using
> REDFISH_FEATURE_CORE_TPL.
> 
> Abner
> 
> 
> >  }
> >
> >  /**
> > @@ -670,7 +682,7 @@ RedfishFeatureCoreEntryPoint (
> >
> >Status = gBS->CreateEventEx (
> >EVT_NOTIFY_SIGNAL,
> > -  TPL_CALLBACK,
> > +  REDFISH_FEATURE_CORE_TPL,
> >RedfishFeatureDriverStartup,
> >(CONST VOID *)&mFeatureDriverStartupContext,
> >EventGuid,
> > --
> > 2.17.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112142): https://edk2.groups.io/g/devel/message/112142
Mute This Topic: https://groups.io/mt/103009658/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2] PcAtChipsetPkg: Fix AcpiTimerLib incompatibility with XhciDxe

[Nate DeSimone]

Pushed as df2ec2a

-Original Message-
From: devel@edk2.groups.io  On Behalf Of Nate DeSimone
Sent: Monday, December 4, 2023 10:48 AM
To: devel@edk2.groups.io
Cc: Ni, Ray ; Kinney, Michael D 
Subject: [edk2-devel] [PATCH v2] PcAtChipsetPkg: Fix AcpiTimerLib 
incompatibility with XhciDxe

The DXE & MM standalone variant of AcpiTimerLib defines a global named 
mPerformanceCounterFrequency. A global with an identical name is also present 
in MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c

Since XhciDxe has a dependency on TimerLib, this can cause link errors due to 
the same symbol being defined twice if the platform DSC chooses to use 
AcpiTimerLib as the TimerLib implementation for any given platform.

To resolve this, I have changed made the definition of 
mPerformanceCounterFrequency to static and renamed it to 
mAcpiTimerLibTscFrequency. Since this variable is not used outside of the 
DxeStandaloneMmAcpiTimerLib.c compilation unit, there is no reason to have it 
exported as a global.

Cc: Ray Ni 
Cc: Michael D Kinney 
Signed-off-by: Nate DeSimone 
---
 .../AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.c 
b/PcAtChipsetPkg/Library/AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.c
index 16ac48938f..ccceb8a649 100644
--- a/PcAtChipsetPkg/Library/AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.c
+++ b/PcAtChipsetPkg/Library/AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.c
@@ -1,7 +1,7 @@
 /** @file
   ACPI Timer implements one instance of Timer Library.
 
-  Copyright (c) 2013 - 2018, Intel Corporation. All rights reserved.
+  Copyright (c) 2013 - 2023, Intel Corporation. All rights 
+ reserved.
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -11,6 +11,11 @@
 #include 
 #include 
 
+//
+// Cached performance counter frequency // static UINT64 
+mAcpiTimerLibTscFrequency = 0;
+
 extern GUID  mFrequencyHobGuid;
 
 /**
@@ -48,11 +53,6 @@ InternalCalculateTscFrequency (
   VOID
   );
 
-//
-// Cached performance counter frequency -//
-UINT64  mPerformanceCounterFrequency = 0;
-
 /**
   Internal function to retrieves the 64-bit frequency in Hz.
 
@@ -66,7 +66,7 @@ InternalGetPerformanceCounterFrequency (
   VOID
   )
 {
-  return mPerformanceCounterFrequency;
+  return mAcpiTimerLibTscFrequency;
 }
 
 /**
@@ -92,9 +92,9 @@ CommonAcpiTimerLibConstructor (
   //
   GuidHob = GetFirstGuidHob (&mFrequencyHobGuid);
   if (GuidHob != NULL) {
-mPerformanceCounterFrequency = *(UINT64 *)GET_GUID_HOB_DATA (GuidHob);
+mAcpiTimerLibTscFrequency = *(UINT64 *)GET_GUID_HOB_DATA (GuidHob);
   } else {
-mPerformanceCounterFrequency = InternalCalculateTscFrequency ();
+mAcpiTimerLibTscFrequency = InternalCalculateTscFrequency ();
   }
 
   return EFI_SUCCESS;
--
2.39.2.windows.1








-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112141): https://edk2.groups.io/g/devel/message/112141
Mute This Topic: https://groups.io/mt/103024047/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 0/1] Resolve regex syntax warnings

[Joey Vagedes via groups.io]

Python 3.12 now produces syntax warnings when using an invalid escape
character (\ followed by an unexpected character). This happens
throughout BaseTools due the usage of regular expressions. the re module
in python suggests that when creating regex patterns, to use raw text.
This patch series adds the r prefix to any regex pattern that uses an
invalid escape sequence.

Cc: Rebecca Cran 
Cc: Liming Gao 
Cc: Bob Feng 
Cc: Yuwei Chen 
Signed-off-by: Joey Vagedes 

Joey Vagedes (1):
  BaseTools: Resolve regex syntax warnings

 BaseTools/Source/Python/AmlToC/AmlToC.py |  2 +-
 BaseTools/Source/Python/AutoGen/BuildEngine.py   |  2 +-
 BaseTools/Source/Python/AutoGen/GenDepex.py  |  2 +-
 BaseTools/Source/Python/AutoGen/GenMake.py   |  2 +-
 BaseTools/Source/Python/AutoGen/IdfClassObject.py|  2 +-
 BaseTools/Source/Python/AutoGen/ModuleAutoGen.py |  4 ++--
 BaseTools/Source/Python/AutoGen/StrGather.py |  2 +-
 BaseTools/Source/Python/AutoGen/WorkspaceAutoGen.py  |  2 +-
 BaseTools/Source/Python/Common/Expression.py | 16 ++---
 BaseTools/Source/Python/Common/GlobalData.py |  4 ++--
 BaseTools/Source/Python/Common/Misc.py   | 24 
++--
 BaseTools/Source/Python/Common/ToolDefClassObject.py |  6 ++---
 BaseTools/Source/Python/GenFds/FdfParser.py  | 10 
 BaseTools/Source/Python/GenFds/GenFds.py |  2 +-
 BaseTools/Source/Python/GenPatchPcdTable/GenPatchPcdTable.py | 12 +-
 BaseTools/Source/Python/Trim/Trim.py | 18 
+++
 BaseTools/Source/Python/Workspace/DscBuildData.py|  8 +++
 BaseTools/Source/Python/Workspace/MetaFileParser.py  |  2 +-
 18 files changed, 60 insertions(+), 60 deletions(-)

-- 
2.43.0.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112139): https://edk2.groups.io/g/devel/message/112139
Mute This Topic: https://groups.io/mt/103021364/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 1/1] BaseTools: Resolve regex syntax warnings

[Joey Vagedes via groups.io]

Switches regex patterns to raw text to resolve python 3.12 syntax
warnings in regards to invalid escape sequences, as is suggested by the
re (regex) module in python.

Cc: Rebecca Cran 
Cc: Liming Gao 
Cc: Bob Feng 
Cc: Yuwei Chen 
Signed-off-by: Joey Vagedes 
---
 BaseTools/Source/Python/AmlToC/AmlToC.py |  2 +-
 BaseTools/Source/Python/AutoGen/BuildEngine.py   |  2 +-
 BaseTools/Source/Python/AutoGen/GenDepex.py  |  2 +-
 BaseTools/Source/Python/AutoGen/GenMake.py   |  2 +-
 BaseTools/Source/Python/AutoGen/IdfClassObject.py|  2 +-
 BaseTools/Source/Python/AutoGen/ModuleAutoGen.py |  4 ++--
 BaseTools/Source/Python/AutoGen/StrGather.py |  2 +-
 BaseTools/Source/Python/AutoGen/WorkspaceAutoGen.py  |  2 +-
 BaseTools/Source/Python/Common/Expression.py | 16 ++---
 BaseTools/Source/Python/Common/GlobalData.py |  4 ++--
 BaseTools/Source/Python/Common/Misc.py   | 24 
++--
 BaseTools/Source/Python/Common/ToolDefClassObject.py |  6 ++---
 BaseTools/Source/Python/GenFds/FdfParser.py  | 10 
 BaseTools/Source/Python/GenFds/GenFds.py |  2 +-
 BaseTools/Source/Python/GenPatchPcdTable/GenPatchPcdTable.py | 12 +-
 BaseTools/Source/Python/Trim/Trim.py | 18 
+++
 BaseTools/Source/Python/Workspace/DscBuildData.py|  8 +++
 BaseTools/Source/Python/Workspace/MetaFileParser.py  |  2 +-
 18 files changed, 60 insertions(+), 60 deletions(-)

diff --git a/BaseTools/Source/Python/AmlToC/AmlToC.py 
b/BaseTools/Source/Python/AmlToC/AmlToC.py
index 346de7159de7..63931c9720c9 100644
--- a/BaseTools/Source/Python/AmlToC/AmlToC.py
+++ b/BaseTools/Source/Python/AmlToC/AmlToC.py
@@ -17,7 +17,7 @@ from Common.BuildToolError import *
 import sys
 import os
 
-__description__ = """
+__description__ = r"""
 Convert an AML file to a .c file containing the AML bytecode stored in a C
 array. By default, Tables\Dsdt.aml will generate Tables\Dsdt.c.
 Tables\Dsdt.c will contain a C array named "dsdt_aml_code" that contains
diff --git a/BaseTools/Source/Python/AutoGen/BuildEngine.py 
b/BaseTools/Source/Python/AutoGen/BuildEngine.py
index 752a1a1f6a86..45b39d7878d5 100644
--- a/BaseTools/Source/Python/AutoGen/BuildEngine.py
+++ b/BaseTools/Source/Python/AutoGen/BuildEngine.py
@@ -306,7 +306,7 @@ class BuildRule:
 _SubSectionList = [_InputFile, _OutputFile, _Command]
 
 _PATH_SEP = "(+)"
-_FileTypePattern = re.compile("^[_a-zA-Z][_\-0-9a-zA-Z]*$")
+_FileTypePattern = re.compile(r"^[_a-zA-Z][_\-0-9a-zA-Z]*$")
 _BinaryFileRule = FileBuildRule(TAB_DEFAULT_BINARY_FILE, [], 
[os.path.join("$(OUTPUT_DIR)", "${s_name}")],
 ["$(CP) ${src} ${dst}"], [])
 
diff --git a/BaseTools/Source/Python/AutoGen/GenDepex.py 
b/BaseTools/Source/Python/AutoGen/GenDepex.py
index f2f2e9d65b5f..b6db6645a4fb 100644
--- a/BaseTools/Source/Python/AutoGen/GenDepex.py
+++ b/BaseTools/Source/Python/AutoGen/GenDepex.py
@@ -126,7 +126,7 @@ class DependencyExpression:
 #
 # open and close brace must be taken as individual tokens
 #
-TokenPattern = re.compile("(\(|\)|\{[^{}]+\{?[^{}]+\}?[ ]*\}|\w+)")
+TokenPattern = re.compile(r"(\(|\)|\{[^{}]+\{?[^{}]+\}?[ ]*\}|\w+)")
 
 ## Constructor
 #
diff --git a/BaseTools/Source/Python/AutoGen/GenMake.py 
b/BaseTools/Source/Python/AutoGen/GenMake.py
index daec9c6d54b2..c416fe172fe5 100755
--- a/BaseTools/Source/Python/AutoGen/GenMake.py
+++ b/BaseTools/Source/Python/AutoGen/GenMake.py
@@ -28,7 +28,7 @@ from Common.DataType import TAB_COMPILER_MSFT
 gIncludePattern = re.compile(r"^[ \t]*[#%]?[ \t]*include(?:[ 
\t]*(?:\\(?:\r\n|\r|\n))*[ \t]*)*(?:\(?[\"<]?[ \t]*)([-\w.\\/() \t]+)(?:[ 
\t]*[\">]?\)?)", re.MULTILINE | re.UNICODE | re.IGNORECASE)
 
 ## Regular expression for matching macro used in header file inclusion
-gMacroPattern = re.compile("([_A-Z][_A-Z0-9]*)[ \t]*\((.+)\)", re.UNICODE)
+gMacroPattern = re.compile(r"([_A-Z][_A-Z0-9]*)[ \t]*\((.+)\)", re.UNICODE)
 
 gIsFileMap = {}
 
diff --git a/BaseTools/Source/Python/AutoGen/IdfClassObject.py 
b/BaseTools/Source/Python/AutoGen/IdfClassObject.py
index a6b8123c2539..bb413c6a26e3 100644
--- a/BaseTools/Source/Python/AutoGen/IdfClassObject.py
+++ b/BaseTools/Source/Python/AutoGen/IdfClassObject.py
@@ -18,7 +18,7 @@ import os
 from Common.GlobalData import gIdentifierPattern
 from .UniClassObject import StripComments
 
-IMAGE_TOKEN = re.compile('IMAGE_TOKEN *\(([A-Z0-9_]+) *\)', re.MULTILINE | 
re.UNICODE)
+IMAGE_TOKEN = re.compile(r'IMAGE_TOKEN *\(([A-Z0-9_]+) *\)', re.MULTILINE | 
re.UNICODE)
 
 #
 # Value of different image information block types
diff --git a/BaseTools/Source/Python/AutoGen/ModuleAutoGen.py 
b/BaseTools/Source/Python/AutoGen/ModuleAutoGen.py
index d05410b32966..65a2176ca982 100755
--- a/BaseTo

Re: [edk2-devel] [PATCH] ArmVirtPkg: Allow EFI memory attributes protocol to be disabled

[Taylor Beebe]

But what we might do is invent a way to avoid setting the XP attribute
on the entire region based on some heuristic. Given that the main
purpose of the EFI memory attribute protocol is to provide the ability
to remove XP (and set RO instead), perhaps we can avoid the set
entirely? Just brainstorming here.

Can the fault handler deal with this?  Set a flag somewhere, print a
big'n'fat error message, wait 5 secs, reset machine?  After reset the
firmware will see the flag and come up in 'compat' instead of 'strict'
mode.

Not sure what a good place for such a flag would be.  Do we have other
options than a non-volatile efi variable?  When using a efi variable we
probably should add an 'expires' timestamp, so the machine doesn't stay
in 'compat' mode forever.


This is what we do in Project Mu currently and is what we would like to 
push into


EDK2. For x86 platforms, we use CMOS to communicate to the next boot 
that the


system needs to enter compatibility mode. Of course this doesn't work on ARM

platforms, so we'll have to come up with a more permanent mechanism to 
support


this functionality.


(cc'ing Taylor and Oliver given that this is related to the memory
policy work as well) Perhaps we can use the fact that the active image
is non-NX compat to make some tweaks?

Memory policies would be another candidate which could possibly use a
less strict profile in 'compat' mode.  I'd love to see memory policies
land for the February stable tag.


I don't think the policy can change how the SHIM sets attributes using the

protocol, but you can hook the installation of the Memory Attribute

Protocol into the policy system so it's not installed in compat mode.

I have not revisited the memory protection policy interface update

since Lazlo's feedback in October, but I'd be happy to return to it if 
there's


motivation to get it in over the finish line. Note that there are more 
changes


that will need to be made to add testing, compat mode

switching, support for the nx_compat flag, etc. The patch series that's

currently in flight is just meant to be a lateral move to a runtime 
configurable


interface.


What I really want to avoid is derail our effort to tighten things
down and comply with the NX compat related policies, by adding some
build time control that the distros will enable now and never disable
again, citing backward compat concerns.

Sure, I want that too.  Having an runtime switch is already an
improvement over having a compile time switch.  Having this working
fully automatic would be even better of course.


If a fix for this issue is needed immediately, I'm fine with Ard's 
solution as a stop-gap.


Assuming we can make progress on committing the memory protection updates,

I can update the CpuDxe drivers to check the memory protection policy

before installing the Memory Attribute Protocol. When adding this policy 
config,


I would revert the change made here to uninstall the protocol.


Thanks :)

-Taylor


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112138): https://edk2.groups.io/g/devel/message/112138
Mute This Topic: https://groups.io/mt/102967690/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] ArmVirtPkg: Allow EFI memory attributes protocol to be disabled

[Oliver Smith-Denny]

My first caveat here is I am writing this email from the depths of a
parental leave, so my brain is only half here and I haven't been up to
date for the past two months :). I'll return in Jan and hopefully be
a bit more sensical.

On 12/6/2023 5:23 AM, Ard Biesheuvel wrote:

But what we might do is invent a way to avoid setting the XP attribute
on the entire region based on some heuristic. Given that the main
purpose of the EFI memory attribute protocol is to provide the ability
to remove XP (and set RO instead), perhaps we can avoid the set
entirely? Just brainstorming here.

(cc'ing Taylor and Oliver given that this is related to the memory
policy work as well) Perhaps we can use the fact that the active image
is non-NX compat to make some tweaks?

What I really want to avoid is derail our effort to tighten things
down and comply with the NX compat related policies, by adding some
build time control that the distros will enable now and never disable
again, citing backward compat concerns.
And the deafening silence from the shim developers is not an
encouragement either.



I completely agree here. My thoughts on this specific issue tend to be:
- edk2 is reference FW and the mainline branch in particular. It should
"do the right thing" not the hacky thing.

- The bug here, as we all agree, is in shim. This is not a case where
FW made a change for the better that broke something and so needs to fix
the change it made, this is a case where FW offered a new option, which
shim took advantage of and completely borked. edk2 can't guarantee that
every OS will do the right thing and we should have guardrails that
give us the best chance of booting, which is after all our top goal.
However, we can't prevent an OS (or bootloader) from blowing its toes
off. There's a part of me that thinks well, if your OS/bootloader sucks,
get a better one...I understand this is complicated by the lack of
release of a new shim (as this bug has been fixed upstream in shim for
almost a year [1]).

These two points being said, I tend to prefer Ard's approach, if I am
reading it correctly in a quick skim, where this change is confined to
QEMU. The platform FW (ArmVirtPkg in this instance) is the right place
for the hacky stuff. The platform in the end is what has the requirement
to boot certain versions of an OS/bootloader. edk2's requirement is to
have sane and usable interfaces.

So, I personally think that if we think edk2 has a sane memory attribute
protocol (or at least as sane as we think it can be), then we should not
change it. Now, it is totally valid to say edk2 does not have a sane
memory attribute protocol and it should have better guardrails. However,
I agree with Ard that if we add any generic edk2 level ability to
disable the memory attribute protocol, it will never be used.

I think the problem with the pattern Ard is describing (don't set XP
based on some heuristic) is that in case it won't work. Because the XP
comes from shim first and then they ask us to unset XP for an
unaligned region. Aligning this call to a larger region seems fraught
with peril. To Gerd's point, we could attempt to catch this in the
fault handler, set some flag, not set XP in the next boot (different
memory policy) and then everything would work (hmm, although in this
case, the memory policy doesn't come into play right, because shim
explicitly asks us to set XP, so either you'd have to disable the
protocol or in the protocol check the memory policy, which feels
wrong). If something is worked out here it feels...better than
a boot time flag and I suppose the more reasonable way to go here. But,
I think the platform is still the place to implement this. This feels
much more like a specific workaround than a generic fault flow we are
going to catch. If this version of shim had been tested on all the
platforms it was going to support, this would have been caught
immediately. So I go back to, the platform can work around this by
enabling a custom compat memory policy (which I think is the benefit
of that framework), but that edk2 shouldn't back bend here and
compromise safety for a bad shim.

Requiring a platform to fix this is more of a burden, but this is a
burden the shim community created by not releasing a new shim and
not testing the old one. We can't mitigate that. I think memory
protections is an area where having the distros do this the right
way is important.

Sorry for the rambling and any context I'm missing, I'm typing this
with a baby in arm :)

Thanks,
Oliver



1: 
https://github.com/rhboot/shim/commit/c7b305152802c8db688605654f75e1195def9fd6



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112137): https://edk2.groups.io/g/devel/message/112137
Mute This Topic: https://groups.io/mt/102967690/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 1/1] StandaloneMmPkg/StandaloneMmHobLib: Remove HOB creation

[Nhi Pham]

According to the discussion in "StandaloneMmPkg: Fix HOB space and
heap space conflicted issue" [1], Standalone MM modules should be HOB
consumers where HOB is read-only. Therefore, this patch removes the
supported functions for HOB creation in the StandaloneMmHobLib.

[1] https://edk2.groups.io/g/devel/message/108333

Cc: Ard Biesheuvel 
Cc: Ray Ni 
Cc: Sami Mujawar 
Cc: Oliver Smith-Denny 
Signed-off-by: Nhi Pham 
---
 StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c | 171 
++--
 1 file changed, 51 insertions(+), 120 deletions(-)

diff --git a/StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c 
b/StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c
index ee61bdd227d0..bef66d167494 100644
--- a/StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c
+++ b/StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c
@@ -1,5 +1,5 @@
 /** @file
-  HOB Library implementation for Standalone MM Core.
+  HOB Library implementation for Standalone MM modules.
 
 Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.
 Copyright (c) 2017 - 2018, ARM Limited. All rights reserved.
@@ -250,48 +250,13 @@ GetBootModeHob (
   return HandOffHob->BootMode;
 }
 
-VOID *
-CreateHob (
-  IN  UINT16  HobType,
-  IN  UINT16  HobLength
-  )
-{
-  EFI_HOB_HANDOFF_INFO_TABLE  *HandOffHob;
-  EFI_HOB_GENERIC_HEADER  *HobEnd;
-  EFI_PHYSICAL_ADDRESSFreeMemory;
-  VOID*Hob;
-
-  HandOffHob = GetHobList ();
-
-  HobLength = (UINT16)((HobLength + 0x7) & (~0x7));
-
-  FreeMemory = HandOffHob->EfiFreeMemoryTop - HandOffHob->EfiFreeMemoryBottom;
-
-  if (FreeMemory < HobLength) {
-return NULL;
-  }
-
-  Hob= (VOID 
*)(UINTN)HandOffHob->EfiEndOfHobList;
-  ((EFI_HOB_GENERIC_HEADER *)Hob)->HobType   = HobType;
-  ((EFI_HOB_GENERIC_HEADER *)Hob)->HobLength = HobLength;
-  ((EFI_HOB_GENERIC_HEADER *)Hob)->Reserved  = 0;
-
-  HobEnd  = (EFI_HOB_GENERIC_HEADER *)((UINTN)Hob + 
HobLength);
-  HandOffHob->EfiEndOfHobList = (EFI_PHYSICAL_ADDRESS)(UINTN)HobEnd;
-
-  HobEnd->HobType   = EFI_HOB_TYPE_END_OF_HOB_LIST;
-  HobEnd->HobLength = sizeof (EFI_HOB_GENERIC_HEADER);
-  HobEnd->Reserved  = 0;
-  HobEnd++;
-  HandOffHob->EfiFreeMemoryBottom = (EFI_PHYSICAL_ADDRESS)(UINTN)HobEnd;
-
-  return Hob;
-}
-
 /**
   Builds a HOB for a loaded PE32 module.
 
   This function builds a HOB for a loaded PE32 module.
+  It can only be invoked by Standalone MM Core.
+  For Standalone MM drivers, it will ASSERT() since HOB is read only for 
Standalone MM drivers.
+
   If ModuleName is NULL, then ASSERT().
   If there is no additional space for HOB creation, then ASSERT().
 
@@ -310,33 +275,19 @@ BuildModuleHob (
   IN EFI_PHYSICAL_ADDRESS  EntryPoint
   )
 {
-  EFI_HOB_MEMORY_ALLOCATION_MODULE  *Hob;
-
-  ASSERT (
-((MemoryAllocationModule & (EFI_PAGE_SIZE - 1)) == 0) &&
-((ModuleLength & (EFI_PAGE_SIZE - 1)) == 0)
-);
-
-  Hob = CreateHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, sizeof 
(EFI_HOB_MEMORY_ALLOCATION_MODULE));
-
-  CopyGuid (&(Hob->MemoryAllocationHeader.Name), 
&gEfiHobMemoryAllocModuleGuid);
-  Hob->MemoryAllocationHeader.MemoryBaseAddress = MemoryAllocationModule;
-  Hob->MemoryAllocationHeader.MemoryLength  = ModuleLength;
-  Hob->MemoryAllocationHeader.MemoryType= EfiBootServicesCode;
-
   //
-  // Zero the reserved space to match HOB spec
+  // HOB is read only for Standalone MM drivers
   //
-  ZeroMem (Hob->MemoryAllocationHeader.Reserved, sizeof 
(Hob->MemoryAllocationHeader.Reserved));
-
-  CopyGuid (&Hob->ModuleName, ModuleName);
-  Hob->EntryPoint = EntryPoint;
+  ASSERT (FALSE);
 }
 
 /**
   Builds a HOB that describes a chunk of system memory.
 
   This function builds a HOB that describes a chunk of system memory.
+  It can only be invoked by Standalone MM Core.
+  For Standalone MM drivers, it will ASSERT() since HOB is read only for 
Standalone MM drivers.
+
   If there is no additional space for HOB creation, then ASSERT().
 
   @param  ResourceTypeThe type of resource described by this HOB.
@@ -354,15 +305,10 @@ BuildResourceDescriptorHob (
   IN UINT64   NumberOfBytes
   )
 {
-  EFI_HOB_RESOURCE_DESCRIPTOR  *Hob;
-
-  Hob = CreateHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR, sizeof 
(EFI_HOB_RESOURCE_DESCRIPTOR));
-  ASSERT (Hob != NULL);
-
-  Hob->ResourceType  = ResourceType;
-  Hob->ResourceAttribute = ResourceAttribute;
-  Hob->PhysicalStart = PhysicalStart;
-  Hob->ResourceLength= NumberOfBytes;
+  //
+  // HOB is read only for Standalone MM drivers
+  //
+  ASSERT (FALSE);
 }
 
 /**
@@ -371,6 +317,9 @@ BuildResourceDescriptorHob (
   This function builds a customized HOB tagged with a GUID for identification
   and returns the start address of GUID HOB data so that caller can fill the 
customized data.
   The HOB Header and Name field is already stripped.
+  It can only be invoked by Standalone MM Cor

Re: [edk2-devel] [PATCH 1/1] ShellPkg/SmbiosView: Add Type45 entry to query table

[Thomas Abraham]

Reviewed-by: Thomas Abraham 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112135): https://edk2.groups.io/g/devel/message/112135
Mute This Topic: https://groups.io/mt/101736631/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2] BaseStackCheckLib: Fix STACK FAULT message

[Michael D Kinney]

Merged

Mike

From: Kinney, Michael D 
Sent: Wednesday, December 6, 2023 8:47 AM
To: Jake Garver ; Gao, Liming ; 
devel@edk2.groups.io
Cc: Liu, Zhiguang ; Kinney, Michael D 

Subject: RE: [PATCH v2] BaseStackCheckLib: Fix STACK FAULT message

Hi Jake,

PR opened with Rb tag added: https://github.com/tianocore/edk2/pull/5113

Mike

From: Jake Garver mailto:j...@nvidia.com>>
Sent: Wednesday, December 6, 2023 8:37 AM
To: Gao, Liming mailto:gaolim...@byosoft.com.cn>>; 
devel@edk2.groups.io
Cc: Kinney, Michael D 
mailto:michael.d.kin...@intel.com>>; Liu, Zhiguang 
mailto:zhiguang@intel.com>>
Subject: Re: [PATCH v2] BaseStackCheckLib: Fix STACK FAULT message

Any further comments on this change?

I'd like to get it merged.

Thanks,
Jake

From: Jake Garver mailto:j...@nvidia.com>>
Sent: Wednesday, October 18, 2023 10:45 AM
To: gaoliming mailto:gaolim...@byosoft.com.cn>>; 
devel@edk2.groups.io 
mailto:devel@edk2.groups.io>>
Cc: michael.d.kin...@intel.com 
mailto:michael.d.kin...@intel.com>>; 
zhiguang@intel.com 
mailto:zhiguang@intel.com>>
Subject: Re: [PATCH v2] BaseStackCheckLib: Fix STACK FAULT message

Thanks for the review, Liming Gao.

Any further comments on this change?

Thanks,
Jake

From: gaoliming mailto:gaolim...@byosoft.com.cn>>
Sent: Saturday, October 7, 2023 1:05 AM
To: Jake Garver mailto:j...@nvidia.com>>; 
devel@edk2.groups.io 
mailto:devel@edk2.groups.io>>
Cc: michael.d.kin...@intel.com 
mailto:michael.d.kin...@intel.com>>; 
zhiguang@intel.com 
mailto:zhiguang@intel.com>>
Subject: 回复: [PATCH v2] BaseStackCheckLib: Fix STACK FAULT message

External email: Use caution opening links or attachments


Reviewed-by: Liming Gao 
mailto:gaolim...@byosoft.com.cn>>

> -邮件原件-
> 发件人: Jake Garver mailto:j...@nvidia.com>>
> 发送时间: 2023年10月6日 0:19
> 收件人: devel@edk2.groups.io
> 抄送: michael.d.kin...@intel.com; 
> gaolim...@byosoft.com.cn;
> zhiguang@intel.com; Jake Garver 
> mailto:j...@nvidia.com>>
> 主题: [PATCH v2] BaseStackCheckLib: Fix STACK FAULT message
>
> __builtin_return_address returns a pointer, not a string.  Fix the STACK
> FAULT message in BaseStackCheckLib appropriately.
>
> Signed-off-by: Jake Garver mailto:j...@nvidia.com>>
> ---
>  MdePkg/Library/BaseStackCheckLib/BaseStackCheckGcc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/MdePkg/Library/BaseStackCheckLib/BaseStackCheckGcc.c
> b/MdePkg/Library/BaseStackCheckLib/BaseStackCheckGcc.c
> index 0d2918668e..ea168841b6 100644
> --- a/MdePkg/Library/BaseStackCheckLib/BaseStackCheckGcc.c
> +++ b/MdePkg/Library/BaseStackCheckLib/BaseStackCheckGcc.c
> @@ -6,6 +6,7 @@
>   to exiting the function. If the "canary" is overwritten
__stack_chk_fail()
>   is called. This is GCC specific code.
>
> + Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights
reserved.
>   Copyright (c) 2012, Apple Inc. All rights reserved.
>   SPDX-License-Identifier: BSD-2-Clause-Patent
>
> @@ -34,7 +35,7 @@ __stack_chk_fail (
>  {
>UINT8  DebugPropertyMask;
>
> -  DEBUG ((DEBUG_ERROR, "STACK FAULT: Buffer Overflow in
> function %a.\n", __builtin_return_address (0)));
> +  DEBUG ((DEBUG_ERROR, "STACK FAULT: Buffer Overflow at 0x%p.\n",
> RETURN_ADDRESS (0)));
>
>//
>// Generate a Breakpoint, DeadLoop, or NOP based on PCD settings even
> if
> --
> 2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112134): https://edk2.groups.io/g/devel/message/112134
Mute This Topic: https://groups.io/mt/102040342/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] BaseTools/GenFw: Change opcode when converting ADR to ADRP

[Jake Garver via groups.io]

Thanks, Pedro and Ard,

An update on this issue:

  *   It seems to be very specific to Ubuntu20's 10.5 build of GCC.
  *   I could not reproduce it using a crosstool-ng build of 10.5, even after 
trying to configure it identically to Ubuntu20's.  It might be something in 
Ubuntu's patchset, but nothing stuck out to me there.
  *   We have migrated to Ubuntu22 as a preferred platform.  Their build of GCC 
12.1 and 12.3 do not have this issue.  As a result, this issue is no longer a 
high runner for us.

Next step: Try reproducing it by rebuilding Ubuntu's GCC debs.  I still want to 
get to the bottom of this, just to make sure it doesn't pop-up in later builds 
on GCC on Ubuntu.

Pedro: I haven't attempted to build without LTO yet.  That looked painful to 
setup, so I didn't make it a priority.

Thanks again,
Jake



From: Pedro Falcato 
Sent: Thursday, November 2, 2023 8:47 AM
To: Jake Garver 
Cc: Ard Biesheuvel ; devel@edk2.groups.io 
; rebe...@bsdio.com ; 
gaolim...@byosoft.com.cn ; bob.c.f...@intel.com 
; yuwei.c...@intel.com 
Subject: Re: [edk2-devel] [PATCH] BaseTools/GenFw: Change opcode when 
converting ADR to ADRP

External email: Use caution opening links or attachments


On Thu, Nov 2, 2023 at 11:47 AM Jake Garver  wrote:
>
> Ard, Pedro,
>
> How would you like to proceed here?
>
> Do nothing.  Treat it as a toolchain bug and not attempt a work-around.  
> Practically speaking, this means the Ubuntu20 container at 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Fcontainers&data=05%7C01%7Cjake%40nvidia.com%7C0977cd2f66b749f07d7508dbdba1e777%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638345260662824037%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Ef71H%2FaGAFezT%2Fs6YBanbDnUQge%2B%2F6YZ3OLRI6MuAb8%3D&reserved=0
>  cannot be updated to the latest GCC 10.x (10.5).  We can pin it to 10.3 or 
> EOL it, migrating to Ubuntu22.
> Rework the patch as a work-around.
> ??? Something I missed.
>
> Note in option 1, I'm assuming that other versions of GCC, e.g. 12.x used in 
> Ubuntu22, don't eventually inherit this regression. But we can always revisit 
> the work-around if we determine that to be the case.  I'm starting Ubuntu22 
> testing now, so I may have an answer soon.

I think the correct way to proceed here is to find out what exactly is
causing this. Once we find out, we can edit the patch's commit message
and push (or drop the patch, depending on what the problem is).
The patch itself looks fine to me (And I already Rb'd it I think), and
it should be harmless to apply it, even for toolchains that do not
share this mysterious problem.

Also, FWIW: GCC 10.5 is a stable release, so I find it hard to believe
that something actually broke between 10.4 and 10.5.


One more thing: What happens if you build without LTO?

--
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112133): https://edk2.groups.io/g/devel/message/112133
Mute This Topic: https://groups.io/mt/102202314/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2] BaseStackCheckLib: Fix STACK FAULT message

[Michael D Kinney]

Hi Jake,

PR opened with Rb tag added: https://github.com/tianocore/edk2/pull/5113

Mike

From: Jake Garver 
Sent: Wednesday, December 6, 2023 8:37 AM
To: Gao, Liming ; devel@edk2.groups.io
Cc: Kinney, Michael D ; Liu, Zhiguang 

Subject: Re: [PATCH v2] BaseStackCheckLib: Fix STACK FAULT message

Any further comments on this change?

I'd like to get it merged.

Thanks,
Jake

From: Jake Garver mailto:j...@nvidia.com>>
Sent: Wednesday, October 18, 2023 10:45 AM
To: gaoliming mailto:gaolim...@byosoft.com.cn>>; 
devel@edk2.groups.io 
mailto:devel@edk2.groups.io>>
Cc: michael.d.kin...@intel.com 
mailto:michael.d.kin...@intel.com>>; 
zhiguang@intel.com 
mailto:zhiguang@intel.com>>
Subject: Re: [PATCH v2] BaseStackCheckLib: Fix STACK FAULT message

Thanks for the review, Liming Gao.

Any further comments on this change?

Thanks,
Jake

From: gaoliming mailto:gaolim...@byosoft.com.cn>>
Sent: Saturday, October 7, 2023 1:05 AM
To: Jake Garver mailto:j...@nvidia.com>>; 
devel@edk2.groups.io 
mailto:devel@edk2.groups.io>>
Cc: michael.d.kin...@intel.com 
mailto:michael.d.kin...@intel.com>>; 
zhiguang@intel.com 
mailto:zhiguang@intel.com>>
Subject: 回复: [PATCH v2] BaseStackCheckLib: Fix STACK FAULT message

External email: Use caution opening links or attachments


Reviewed-by: Liming Gao 
mailto:gaolim...@byosoft.com.cn>>

> -邮件原件-
> 发件人: Jake Garver mailto:j...@nvidia.com>>
> 发送时间: 2023年10月6日 0:19
> 收件人: devel@edk2.groups.io
> 抄送: michael.d.kin...@intel.com; 
> gaolim...@byosoft.com.cn;
> zhiguang@intel.com; Jake Garver 
> mailto:j...@nvidia.com>>
> 主题: [PATCH v2] BaseStackCheckLib: Fix STACK FAULT message
>
> __builtin_return_address returns a pointer, not a string.  Fix the STACK
> FAULT message in BaseStackCheckLib appropriately.
>
> Signed-off-by: Jake Garver mailto:j...@nvidia.com>>
> ---
>  MdePkg/Library/BaseStackCheckLib/BaseStackCheckGcc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/MdePkg/Library/BaseStackCheckLib/BaseStackCheckGcc.c
> b/MdePkg/Library/BaseStackCheckLib/BaseStackCheckGcc.c
> index 0d2918668e..ea168841b6 100644
> --- a/MdePkg/Library/BaseStackCheckLib/BaseStackCheckGcc.c
> +++ b/MdePkg/Library/BaseStackCheckLib/BaseStackCheckGcc.c
> @@ -6,6 +6,7 @@
>   to exiting the function. If the "canary" is overwritten
__stack_chk_fail()
>   is called. This is GCC specific code.
>
> + Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights
reserved.
>   Copyright (c) 2012, Apple Inc. All rights reserved.
>   SPDX-License-Identifier: BSD-2-Clause-Patent
>
> @@ -34,7 +35,7 @@ __stack_chk_fail (
>  {
>UINT8  DebugPropertyMask;
>
> -  DEBUG ((DEBUG_ERROR, "STACK FAULT: Buffer Overflow in
> function %a.\n", __builtin_return_address (0)));
> +  DEBUG ((DEBUG_ERROR, "STACK FAULT: Buffer Overflow at 0x%p.\n",
> RETURN_ADDRESS (0)));
>
>//
>// Generate a Breakpoint, DeadLoop, or NOP based on PCD settings even
> if
> --
> 2.34.1




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112132): https://edk2.groups.io/g/devel/message/112132
Mute This Topic: https://groups.io/mt/102040342/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2] BaseStackCheckLib: Fix STACK FAULT message

[Jake Garver via groups.io]

Any further comments on this change?

I'd like to get it merged.

Thanks,
Jake

From: Jake Garver 
Sent: Wednesday, October 18, 2023 10:45 AM
To: gaoliming ; devel@edk2.groups.io 

Cc: michael.d.kin...@intel.com ; 
zhiguang@intel.com 
Subject: Re: [PATCH v2] BaseStackCheckLib: Fix STACK FAULT message

Thanks for the review, Liming Gao.

Any further comments on this change?

Thanks,
Jake

From: gaoliming 
Sent: Saturday, October 7, 2023 1:05 AM
To: Jake Garver ; devel@edk2.groups.io 
Cc: michael.d.kin...@intel.com ; 
zhiguang@intel.com 
Subject: 回复: [PATCH v2] BaseStackCheckLib: Fix STACK FAULT message

External email: Use caution opening links or attachments


Reviewed-by: Liming Gao 

> -邮件原件-
> 发件人: Jake Garver 
> 发送时间: 2023年10月6日 0:19
> 收件人: devel@edk2.groups.io
> 抄送: michael.d.kin...@intel.com; gaolim...@byosoft.com.cn;
> zhiguang@intel.com; Jake Garver 
> 主题: [PATCH v2] BaseStackCheckLib: Fix STACK FAULT message
>
> __builtin_return_address returns a pointer, not a string.  Fix the STACK
> FAULT message in BaseStackCheckLib appropriately.
>
> Signed-off-by: Jake Garver 
> ---
>  MdePkg/Library/BaseStackCheckLib/BaseStackCheckGcc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/MdePkg/Library/BaseStackCheckLib/BaseStackCheckGcc.c
> b/MdePkg/Library/BaseStackCheckLib/BaseStackCheckGcc.c
> index 0d2918668e..ea168841b6 100644
> --- a/MdePkg/Library/BaseStackCheckLib/BaseStackCheckGcc.c
> +++ b/MdePkg/Library/BaseStackCheckLib/BaseStackCheckGcc.c
> @@ -6,6 +6,7 @@
>   to exiting the function. If the "canary" is overwritten
__stack_chk_fail()
>   is called. This is GCC specific code.
>
> + Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights
reserved.
>   Copyright (c) 2012, Apple Inc. All rights reserved.
>   SPDX-License-Identifier: BSD-2-Clause-Patent
>
> @@ -34,7 +35,7 @@ __stack_chk_fail (
>  {
>UINT8  DebugPropertyMask;
>
> -  DEBUG ((DEBUG_ERROR, "STACK FAULT: Buffer Overflow in
> function %a.\n", __builtin_return_address (0)));
> +  DEBUG ((DEBUG_ERROR, "STACK FAULT: Buffer Overflow at 0x%p.\n",
> RETURN_ADDRESS (0)));
>
>//
>// Generate a Breakpoint, DeadLoop, or NOP based on PCD settings even
> if
> --
> 2.34.1





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112131): https://edk2.groups.io/g/devel/message/112131
Mute This Topic: https://groups.io/mt/102040342/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] Event: TianoCore Community Meeting EMEA/NAMO - Thursday, December 7, 2023 #cal-reminder

[Group Notification]

*Reminder: TianoCore Community Meeting EMEA/NAMO*

*When:*
Thursday, December 7, 2023
8:00am to 9:00am
(UTC-08:00) America/Los Angeles

*Where:*
Microsoft Teams meeting Join on your computer or mobile app Click here to join 
the meeting Meeting ID: 226 323 011 029 Passcode: hMRCj6 Download Teams | Join 
on the web Join with a video conferencing device te...@conf.intel.com Video 
Conference ID: 112 716 814 3 Alternate VTC instructions Learn More | Meeting 
options

*Organizer:* Miki Demeter

View Event ( https://edk2.groups.io/g/devel/viewevent?eventid=2108600 )

*Description:*



Microsoft Teams meeting

*Join on your computer or mobile app*

Click here to join the meeting ( 
https://teams.microsoft.com/l/meetup-join/19%3ameeting_MTAyZGJhNjMtYzQ4Mi00MTUxLWFlMWMtOGU0MWNlZDk4NjY5%40thread.v2/0?context=%7b%22Tid%22%3a%2246c98d88-e344-4ed4-8496-4ed7712e255d%22%2c%22Oid%22%3a%226e4ce4c4-1242-431b-9a51-92cd01a5df3c%22%7d
 )

Meeting ID: 226 323 011 029
Passcode: hMRCj6

Download Teams ( https://www.microsoft.com/en-us/microsoft-teams/download-app ) 
| Join on the web ( https://www.microsoft.com/microsoft-teams/join-a-meeting )

*Join with a video conferencing device*

te...@conf.intel.com

Video Conference ID: 112 716 814 3

Alternate VTC instructions ( 
https://conf.intel.com/teams/?conf=1127168143&ivr=teams&d=conf.intel.com&test=test_call
 )

Learn More ( https://aka.ms/JoinTeamsMeeting ) | Meeting options ( 
https://teams.microsoft.com/meetingOptions/?organizerId=6e4ce4c4-1242-431b-9a51-92cd01a5df3c&tenantId=46c98d88-e344-4ed4-8496-4ed7712e255d&threadId=19_meeting_MTAyZGJhNjMtYzQ4Mi00MTUxLWFlMWMtOGU0MWNlZDk4NjY5@thread.v2&messageId=0&language=en-US
 )




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112130): https://edk2.groups.io/g/devel/message/112130
Mute This Topic: https://groups.io/mt/103015488/21656
Mute #cal-reminder:https://edk2.groups.io/g/devel/mutehashtag/cal-reminder
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] ArmVirtPkg: Allow EFI memory attributes protocol to be disabled

[Gerd Hoffmann]

  Hi,

> But what we might do is invent a way to avoid setting the XP attribute
> on the entire region based on some heuristic. Given that the main
> purpose of the EFI memory attribute protocol is to provide the ability
> to remove XP (and set RO instead), perhaps we can avoid the set
> entirely? Just brainstorming here.

Can the fault handler deal with this?  Set a flag somewhere, print a
big'n'fat error message, wait 5 secs, reset machine?  After reset the
firmware will see the flag and come up in 'compat' instead of 'strict'
mode.

Not sure what a good place for such a flag would be.  Do we have other
options than a non-volatile efi variable?  When using a efi variable we
probably should add an 'expires' timestamp, so the machine doesn't stay
in 'compat' mode forever.

> (cc'ing Taylor and Oliver given that this is related to the memory
> policy work as well) Perhaps we can use the fact that the active image
> is non-NX compat to make some tweaks?

Memory policies would be another candidate which could possibly use a
less strict profile in 'compat' mode.  I'd love to see memory policies
land for the February stable tag.

> What I really want to avoid is derail our effort to tighten things
> down and comply with the NX compat related policies, by adding some
> build time control that the distros will enable now and never disable
> again, citing backward compat concerns.

Sure, I want that too.  Having an runtime switch is already an
improvement over having a compile time switch.  Having this working
fully automatic would be even better of course.

> And the deafening silence from the shim developers is not an
> encouragement either.

Indeed.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112129): https://edk2.groups.io/g/devel/message/112129
Mute This Topic: https://groups.io/mt/102967690/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH] RedfishPkg/RedfishDicovery: Remedy Redfish service discovery flow

[Chang, Abner via groups.io]

From: Abner Chang 

Remedy Redfish service discovery flow changes made
in commit 8736b8fd.

The above fix creates the dependency with SMBIOS 42h record,
which has a problem as SMBIOS 42h may not be created when
RedfishDiscovery.Supported() is invoked even all of the
required protocols are ready on the ControllerHandle. We can’t
guarantee SMBIOS 42 structure will be always created before
ConnectController(). USB NIC maybe detected late and it means
PlatformHostInterfaceBmcUsbNicLib can populate SMBIOS 42h
information late as well. Calling to
RedfishServiceGetNetworkInterface with the previous fix may
result in no network interface for BMC-exposed NIC as SMBIOS
42h is not ready yet.This is the first issue.

Second, to skip the network interface when
NetworkInterfaceGetSubnetInfo() returns a failure also has
problem, as the NIC may be configured via RestEx->Configure().
This happens after the Host interface is discovered, as at this
moment we have the sufficient network information to configure
BMC-exposed NIC.

Base on Redfish spec in 31.1.5.2, “EFI Redfish Client may provide
selection UI of network interfaces for Redfish service discovery.",
This means edk2 Redfish client gets all network interfaces
through RedfishServiceGetNetworkInterface and choose the desired
network interface at its discretion for Redfish service.

So the fix here is:
1. In BuildNetworkInterface(), we don’t skip any network
   interface. In RedfishServiceGetNetworkInterface, we don’t
   skip any network interface even the subnet information is not
   retrieved. We will still return all of network interfaces to
   client.
2. In RedfishServiceAcquireService for
   EFI_REDFISH_RISCOVER_HOST_INTERFACE case, we don’t skip any
   network interface even the subnet information is not
   retrieved.

3. Added some more debug information.

Note: The subnet information is used for the scenario the system
is managed by a centralized Redfish service (not on BMC), says
the multiple Redfish computer system instances. As it mentions
in 31.1.5.2, Redfish client they may have to know the subnet
information so they can know the network domain the NIC is
connected. There may have multiple subnets in the corporation
network environment. So the subnet information provides client
an idea when they choose the network interface, so does VLAN ID.

Signed-off-by: Abner Chang 
Cc: Nickle Wang 
Cc: Igor Kulchytskyy 
Cc: Mike Maslenkin 
---
 .../RedfishDiscoverDxe/RedfishDiscoverDxe.c   | 105 ++
 1 file changed, 37 insertions(+), 68 deletions(-)

diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c 
b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
index 833ae2b969f..1cfcbcdc794 100644
--- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
+++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
@@ -487,43 +487,6 @@ CheckIsIpVersion6 (
   return FALSE;
 }
 
-/**
-  This function returns the  IP type supported by the Host Interface.
-
-  @retval 00h is Unknown
-  01h is Ipv4
-  02h is Ipv6
-
-**/
-STATIC
-UINT8
-GetHiIpProtocolType (
-  VOID
-  )
-{
-  EFI_STATUS Status;
-  REDFISH_OVER_IP_PROTOCOL_DATA  *Data;
-  REDFISH_INTERFACE_DATA *DeviceDescriptor;
-
-  Data = NULL;
-  DeviceDescriptor = NULL;
-  if (mSmbios == NULL) {
-Status = gBS->LocateProtocol (&gEfiSmbiosProtocolGuid, NULL, (VOID 
**)&mSmbios);
-if (EFI_ERROR (Status)) {
-  return REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN;
-}
-  }
-
-  Status = RedfishGetHostInterfaceProtocolData (mSmbios, &DeviceDescriptor, 
&Data); // Search for SMBIOS type 42h
-  if (!EFI_ERROR (Status) && (Data != NULL) &&
-  (Data->HostIpAssignmentType == RedfishHostIpAssignmentStatic))
-  {
-return Data->HostIpAddressFormat;
-  }
-
-  return REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN;
-}
-
 /**
   Check if Network Protocol Type matches with SMBIOS Type 42 IP Address Type.
 
@@ -583,7 +546,10 @@ DiscoverRedfishHostInterface (
   }
 
   Status = RedfishGetHostInterfaceProtocolData (mSmbios, &DeviceDescriptor, 
&Data); // Search for SMBIOS type 42h
-  if (!EFI_ERROR (Status) && (Data != NULL) && (DeviceDescriptor != NULL)) {
+  if (EFI_ERROR (Status) || (Data == NULL) || (DeviceDescriptor == NULL)) {
+DEBUG ((DEBUG_ERROR, "%a: RedfishGetHostInterfaceProtocolData is 
failed.\n", __func__));
+return Status;
+  } else {
 // Check IP Type and skip an unnecessary network protocol if does not match
 if (FilterProtocol (Instance->NetworkInterface->NetworkProtocolType, 
Data->HostIpAddressFormat)) {
   return EFI_UNSUPPORTED;
@@ -737,10 +703,7 @@ DiscoverRedfishHostInterface (
  IsHttps
  );
 }
-  } else {
-DEBUG ((DEBUG_ERROR, "%a: RedfishGetHostInterfaceProtocolData is 
failed.\n", __func__));
   }
-
   return Status;
 }
 
@@ -891,6 +854,12 @@ AddAndSignalNewRedfishService (
   DEBUG ((DEBUG_MANAGEABILITY, "Redfish service location: %s.\n", 
Dis

Re: [edk2-devel] [PATCH v9 5/5] OvmfPkg/RiscVVirt: Override for RV CPU Features

[Sunil V L]

Hi Dhaval,

Few minor comments.

1) Please use RISC-V instead of RV every where.

On Mon, Dec 04, 2023 at 01:59:50PM +0530, Dhaval wrote:
> This PCD provides a way for platform to override any
> HW features that are default enabled by previous stages
> of FW (like OpenSBI). For the case where previous/prev
> stage has disabled the feature, this override is not
> useful and its usage should be avoided.
> 
> Cc: Ard Biesheuvel 
> Cc: Jiewen Yao 
> Cc: Jordan Justen 
> Cc: Gerd Hoffmann 
> Cc: Sunil V L 
> Cc: Andrei Warkentin 
> Cc: Laszlo Ersek 
> 
> Signed-off-by: Dhaval Sharma 
> Acked-by: Laszlo Ersek 
> Reviewed-by: Andrei Warkentin 
> ---
> 
> Notes:
> V8:
> - Added RV tag
> V7:
> - Added RB tag
> v6:
> - Modify PCD name according to changes made in Baselib implementation
> V5:
> - Introduce PCD for platform
> 
>  OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc 
> b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> index fe320525153f..5d66f7fe6ae6 100644
> --- a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> +++ b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> @@ -203,6 +203,7 @@ [PcdsFeatureFlag]
>gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE
>  
>  [PcdsFixedAtBuild.common]
> +  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0
Could you set this to 0xFFFE? Just disable only CMO?

Thanks,
Snil


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112127): https://edk2.groups.io/g/devel/message/112127
Mute This Topic: https://groups.io/mt/102967059/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v9 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V

[Sunil V L]

Hi Dhaval,

Thank you very much for fixing the issue with instruction cache
invalidation and confirming with the spec owner. Few minor comments
below.

On Mon, Dec 04, 2023 at 01:59:49PM +0530, Dhaval Sharma wrote:
> Use newly defined cache management operations for RISC-V where possible
> It builds up on the support added for RISC-V cache management
> instructions in BaseLib.
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Cc: Zhiguang Liu 
> Cc: Laszlo Ersek 
> 
> Signed-off-by: Dhaval Sharma 
> Acked-by: Laszlo Ersek 
> ---
> 
> Notes:
> V9:
> - Fixed an issue with Instruction cache invalidation. Use fence.i
>   instruction as CMO does not support i-cache operations.
> V8:
> - Added note to convert PCD into RISC-V feature bitmap pointer
> - Modified function names to be more explicit about cache ops
> - Added RB tag
> V7:
> - Added PcdLib
> - Restructure DEBUG message based on feedback on V6
> - Make naming consistent to CMO, remove all CBO references
> - Add ASSERT for not supported functions instead of plain debug message
> - Added RB tag
> V6:
> - Utilize cache management instructions if HW supports it
>   This patch is part of restructuring on top of v5
> 
IMO, it is better to keep the change log in the cover letter. Since not
all patches may be CC'd to every one apart from the cover letter, it is
difficult to understand from the cover letter what has changed in the new
series.

>  MdePkg/MdePkg.dec  |   8 +
>  MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf |   5 +
>  MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c| 173 
> 
>  MdePkg/MdePkg.uni  |   4 +
>  4 files changed, 160 insertions(+), 30 deletions(-)
> 
> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> index ac54338089e8..fa92673ff633 100644
> --- a/MdePkg/MdePkg.dec
> +++ b/MdePkg/MdePkg.dec
> @@ -2399,6 +2399,14 @@ [PcdsFixedAtBuild.AARCH64, 
> PcdsPatchableInModule.AARCH64]
># @Prompt CPU Rng algorithm's GUID.
>
> gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm|{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}|VOID*|0x0037
>  
> +[PcdsFixedAtBuild.RISCV64, PcdsPatchableInModule.RISCV64]
> +  #
> +  # Configurability to override RISC-V CPU Features
> +  # BIT 0 = Cache Management Operations. This bit is relevant only if
> +  # previous stage has feature enabled and user wants to disable it.
> +  #
> +  
> gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0x|UINT64|0x69
> +
>  [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>## This value is used to set the base address of PCI express hierarchy.
># @Prompt PCI Express Base Address.
> diff --git 
> a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf 
> b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> index 6fd9cbe5f6c9..601a38d6c109 100644
> --- a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> +++ b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> @@ -56,3 +56,8 @@ [LibraryClasses]
>BaseLib
>DebugLib
>  
> +[LibraryClasses.RISCV64]
> +  PcdLib
> +
> +[Pcd.RISCV64]
> +  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride  ## CONSUMES
> diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c 
> b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> index ac2a3c23a249..cacc38eff4f4 100644
> --- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> +++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> @@ -2,6 +2,7 @@
>RISC-V specific functionality for cache.
>  
>Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights 
> reserved.
> +  Copyright (c) 2023, Rivos Inc. All rights reserved.
>  
>SPDX-License-Identifier: BSD-2-Clause-Patent
>  **/
> @@ -9,10 +10,117 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +
> +//
> +// TODO: Grab cache block size and make Cache Management Operation
> +// enabling decision based on RISC-V CPU HOB in
> +// future when it is available and convert PcdRiscVFeatureOverride
> +// PCD to a pointer that contains pointer to bitmap structure
> +// which can be operated more elegantly.
> +//
> +#define RISCV_CACHE_BLOCK_SIZE 64
Can we make this also as a PCD?

> +#define RISCV_CPU_FEATURE_CMO_BITMASK  0x1
> +
> +typedef enum {
> +  CacheOpClean,
> +  CacheOpFlush,
> +  CacheOpInvld,
> +} CACHE_OP;
> +
> +/**
> +Verify CBOs are supported by this HW
> +TODO: Use RISC-V CPU HOB once available.
> +
> +**/
> +STATIC
> +BOOLEAN
> +RiscVIsCMOEnabled (
> +  VOID
> +  )
> +{
> +  // If CMO is disabled in HW, skip Override check
> +  // Otherwise this PCD can override settings
> +  return ((PcdGet64 (PcdRiscVFeatureOverride) & 
> RISCV_CPU_FEATURE_CMO_BITMASK) != 0);
> +}
> +
> +/**
> +  Performs required opeartion on cache 

[edk2-devel] Event: TianoCore edk2-test Bug Triage Meeting - Thursday, December 7, 2023 #cal-reminder

[Group Notification]

*Reminder: TianoCore edk2-test Bug Triage Meeting*

*When:*
Thursday, December 7, 2023
10:00pm to 11:00pm
(UTC+08:00) Asia/Shanghai

*Where:*
https://armltd.zoom.us/j/94348061758?pwd=Q3RDeFA5K2JFaU5jdWUxc1FnaGdyUT09&from=addon

*Organizer:* Edhaya Chandran edhaya.chand...@arm.com ( 
edhaya.chand...@arm.com?subject=Re:%20Event:%20TianoCore%20edk2-test%20Bug%20Triage%20Meeting
 )

View Event ( https://edk2.groups.io/g/devel/viewevent?eventid=2108599 )

*Description:*


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112125): https://edk2.groups.io/g/devel/message/112125
Mute This Topic: https://groups.io/mt/103012830/21656
Mute #cal-reminder:https://edk2.groups.io/g/devel/mutehashtag/cal-reminder
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] ArmVirtPkg: Allow EFI memory attributes protocol to be disabled

[Ard Biesheuvel]

For context:

https://openfw.io/edk2-devel/g2ulyam7plpgrqlganhb5u2wtswq26civqlt4gpnxmjgq65yt7@umm3dta22cdz/T/#t

On Wed, 6 Dec 2023 at 13:51, Gerd Hoffmann  wrote:
>
> > > We can disable the protocol via this method but how would you set it
> > > to =n by default?
> >
> > if (Status != EFI_SUCCESS)
> > // opt/org.tiabocode/MemAttrProtocol not present on the qemu cmdline
> > MemAttrProtocol = ThisBuildsDefault
> > }
>
> FYI: Below is what I'll add to the fedora builds.
>
> Rough plan:  Keep this until we have a fixed shim.efi and release media
> with that (hopefully Fedora 40 next spring).  At that point flip default
> to TRUE and keep it that way for a year or two.  Then drop the patch.
>

Yeah. I am not /quite/ ready to admit defeat, especially because other
systems (such as sbsa-ref) are suffering from the same problem, and so
fixing this in a QEMU specific way is probably not sufficient.

So what happens is:
- shim intends to load fbaa64.efi
- it allocates the region as EfiLoaderCode
- it sets XP and clears RP/RO on the entire region
- it copies and relocates the individual sections, and remaps them but
only if the alignment is >= 4k
- it calls the entrypoint which resides in a section that is still
mapped XP and boom

(this is based on the ubuntu cloud image).

Note that loading grub works fine, so once we've gone through
fbaa64.efi once, the issue goes away.

Shim itself does not have the NX compat attribute, nor does the
fbaa64.efi image that it is loading (afaict)

The EfiLoaderCode region has RWX permissions in this case. Future,
tightened firmware will not create EfiLoaderCode with RWX permissions,
but require the use of the EFI memory attributes protocol to create
executable regions.

The difficulty here is that shim never bothers to call the protocol at
all to remap the individual sections, as it notices that the alignment
is insufficient. So overriding the behavior at this point is
impossible.

But what we might do is invent a way to avoid setting the XP attribute
on the entire region based on some heuristic. Given that the main
purpose of the EFI memory attribute protocol is to provide the ability
to remove XP (and set RO instead), perhaps we can avoid the set
entirely? Just brainstorming here.

(cc'ing Taylor and Oliver given that this is related to the memory
policy work as well) Perhaps we can use the fact that the active image
is non-NX compat to make some tweaks?

What I really want to avoid is derail our effort to tighten things
down and comply with the NX compat related policies, by adding some
build time control that the distros will enable now and never disable
again, citing backward compat concerns.
And the deafening silence from the shim developers is not an
encouragement either.




> --- cut here 
> From c174197c65d2346f519418ded2e645d57423be41 Mon Sep 17 00:00:00 2001
> From: Gerd Hoffmann 
> Date: Wed, 6 Dec 2023 13:00:53 +0100
> Subject: [PATCH 1/1] ArmVirtPkg: add runtime option to enable/disable
>  MemoryAttributesProtocol
>
> Based on a patch by Ard Biesheuvel 
>
> Usage:
> qemu-system-aarch64 $args \
> -fw_cfg name=opt/org.tianocore/MemAttrProtocol,string=y
>
> Default to 'n' (disabled) for now.
>
> Signed-off-by: Gerd Hoffmann 
> ---
>  .../PlatformBootManagerLib.inf|  2 +
>  .../PlatformBootManagerLib/PlatformBm.c   | 69 +++
>  2 files changed, 71 insertions(+)
>
> diff --git 
> a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf 
> b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> index 997eb1a4429f..facd81a5d036 100644
> --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> @@ -46,6 +46,7 @@ [LibraryClasses]
>PcdLib
>PlatformBmPrintScLib
>QemuBootOrderLib
> +  QemuFwCfgSimpleParserLib
>QemuLoadImageLib
>ReportStatusCodeLib
>TpmPlatformHierarchyLib
> @@ -73,5 +74,6 @@ [Guids]
>  [Protocols]
>gEfiFirmwareVolume2ProtocolGuid
>gEfiGraphicsOutputProtocolGuid
> +  gEfiMemoryAttributeProtocolGuid
>gEfiPciRootBridgeIoProtocolGuid
>gVirtioDeviceProtocolGuid
> diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c 
> b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index 85c01351b09d..a50b9aec0f2c 100644
> --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -16,6 +16,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -,6 +1112,49 @@ PlatformBootManagerBeforeConsole (
>FilterAndProcess (&gEfiPciIoProtocolGuid, IsVirtioPciSerial, 
> SetupVirtioSerial);
>  }
>
> +/**
> +  Uninstall the EFI memory attribute protocol if it exists.
> +**/
> +STATIC
> +VOID
> +UninstallEfiMemoryAttributesProtocol (
> +  VOID
> +  )
> +{
> +  EFI_STATUS  Status;
> 

Re: [edk2-devel] [PATCH 1/1] OvmfPkg/VirtNorFlashDxe: sanity-check variables

[Mike Maslenkin]

Hi Gerd,

On Tue, Dec 5, 2023 at 4:51 PM Gerd Hoffmann  wrote:
>
> Extend the ValidateFvHeader function, additionally to the header checks
> walk over the list of variables and sanity check them.
>
> In case we find inconsistencies indicating variable store corruption
> return EFI_NOT_FOUND so the variable store will be re-initialized.
>
> Signed-off-by: Gerd Hoffmann 
> ---
>  OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c | 82 +--
>  1 file changed, 77 insertions(+), 5 deletions(-)
>
> diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c 
> b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
> index 5ee98e9b595a..72a197e5aa20 100644
> --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
> +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
> @@ -185,11 +185,16 @@ ValidateFvHeader (
>IN  NOR_FLASH_INSTANCE  *Instance
>)
>  {
> -  UINT16  Checksum;
> -  EFI_FIRMWARE_VOLUME_HEADER  *FwVolHeader;
> -  VARIABLE_STORE_HEADER   *VariableStoreHeader;
> -  UINTN   VariableStoreLength;
> -  UINTN   FvLength;
> +  UINT16 Checksum;
> +  EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader;
> +  VARIABLE_STORE_HEADER  *VariableStoreHeader;
> +  UINTN  VarOffset;
> +  AUTHENTICATED_VARIABLE_HEADER  *VarHeader;
> +  UINTN  VarSize;
> +  CHAR16 *VarName;
> +  CHAR8  *VarState;
> +  UINTN  VariableStoreLength;
> +  UINTN  FvLength;
>
>FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER *)Instance->RegionBaseAddress;
>
> @@ -260,6 +265,73 @@ ValidateFvHeader (
>  return EFI_NOT_FOUND;
>}
>
> +  // check variables
> +  DEBUG ((DEBUG_INFO, "%a: checking variables\n", __func__));
> +  VarOffset = sizeof (*VariableStoreHeader);
> +  while (VarOffset + sizeof (*VarHeader) < VariableStoreHeader->Size) {
> +VarHeader = (VOID *)((UINTN)VariableStoreHeader + VarOffset);
> +if (VarHeader->StartId != 0x55aa) {
> +  DEBUG ((DEBUG_INFO, "%a: end of var list\n", __func__));
> +  break;
> +}
> +
> +VarSize = sizeof (*VarHeader) + VarHeader->NameSize + 
> VarHeader->DataSize;
> +if (VarOffset + VarSize > VariableStoreHeader->Size) {
> +  DEBUG ((
> +DEBUG_ERROR,
> +"%a: invalid variable size: 0x%x + 0x%x + 0x%x + 0x%x > 0x%x\n",
> +__func__,
> +VarOffset,
> +sizeof (*VarHeader),
> +VarHeader->NameSize,
> +VarHeader->DataSize,
> +VariableStoreHeader->Size
> +));
> +  return EFI_NOT_FOUND;
> +}
> +
> +VarName = (VOID *)((UINTN)VariableStoreHeader + VarOffset
> +   + sizeof (*VarHeader));
> +switch (VarHeader->State) {
> +  case VAR_HEADER_VALID_ONLY:
> +VarState = "header-ok";
> +VarName  = L"";
> +break;
> +  case VAR_ADDED:
> +VarState = "ok";
> +break;
> +  case VAR_ADDED &VAR_IN_DELETED_TRANSITION:
> +VarState = "del-in-transition";
> +break;
> +  case VAR_ADDED &VAR_DELETED:
> +  case VAR_ADDED &VAR_DELETED &VAR_IN_DELETED_TRANSITION:
> +VarState = "deleted";
> +break;
> +  default:
> +DEBUG ((
> +  DEBUG_ERROR,
> +  "%a: invalid variable state: 0x%x\n",
> +  __func__,
> +  VarState
> +  ));

Did you want to print VarHeader->State?

Regards,
Mike.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112123): https://edk2.groups.io/g/devel/message/112123
Mute This Topic: https://groups.io/mt/102991507/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] ArmVirtPkg: Allow EFI memory attributes protocol to be disabled

[Gerd Hoffmann]

> > We can disable the protocol via this method but how would you set it
> > to =n by default?
> 
> if (Status != EFI_SUCCESS) 
> // opt/org.tiabocode/MemAttrProtocol not present on the qemu cmdline
> MemAttrProtocol = ThisBuildsDefault
> }

FYI: Below is what I'll add to the fedora builds.

Rough plan:  Keep this until we have a fixed shim.efi and release media
with that (hopefully Fedora 40 next spring).  At that point flip default
to TRUE and keep it that way for a year or two.  Then drop the patch.

take care,
  Gerd

--- cut here 
>From c174197c65d2346f519418ded2e645d57423be41 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann 
Date: Wed, 6 Dec 2023 13:00:53 +0100
Subject: [PATCH 1/1] ArmVirtPkg: add runtime option to enable/disable
 MemoryAttributesProtocol

Based on a patch by Ard Biesheuvel 

Usage:
qemu-system-aarch64 $args \
-fw_cfg name=opt/org.tianocore/MemAttrProtocol,string=y

Default to 'n' (disabled) for now.

Signed-off-by: Gerd Hoffmann 
---
 .../PlatformBootManagerLib.inf|  2 +
 .../PlatformBootManagerLib/PlatformBm.c   | 69 +++
 2 files changed, 71 insertions(+)

diff --git 
a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf 
b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
index 997eb1a4429f..facd81a5d036 100644
--- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
+++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
@@ -46,6 +46,7 @@ [LibraryClasses]
   PcdLib
   PlatformBmPrintScLib
   QemuBootOrderLib
+  QemuFwCfgSimpleParserLib
   QemuLoadImageLib
   ReportStatusCodeLib
   TpmPlatformHierarchyLib
@@ -73,5 +74,6 @@ [Guids]
 [Protocols]
   gEfiFirmwareVolume2ProtocolGuid
   gEfiGraphicsOutputProtocolGuid
+  gEfiMemoryAttributeProtocolGuid
   gEfiPciRootBridgeIoProtocolGuid
   gVirtioDeviceProtocolGuid
diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c 
b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
index 85c01351b09d..a50b9aec0f2c 100644
--- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -16,6 +16,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -,6 +1112,49 @@ PlatformBootManagerBeforeConsole (
   FilterAndProcess (&gEfiPciIoProtocolGuid, IsVirtioPciSerial, 
SetupVirtioSerial);
 }
 
+/**
+  Uninstall the EFI memory attribute protocol if it exists.
+**/
+STATIC
+VOID
+UninstallEfiMemoryAttributesProtocol (
+  VOID
+  )
+{
+  EFI_STATUS  Status;
+  EFI_HANDLE  Handle;
+  UINTN   Size;
+  VOID*MemoryAttributeProtocol;
+
+  Size   = sizeof (Handle);
+  Status = gBS->LocateHandle (
+  ByProtocol,
+  &gEfiMemoryAttributeProtocolGuid,
+  NULL,
+  &Size,
+  &Handle
+  );
+
+  if (EFI_ERROR (Status)) {
+ASSERT (Status == EFI_NOT_FOUND);
+return;
+  }
+
+  Status = gBS->HandleProtocol (
+  Handle,
+  &gEfiMemoryAttributeProtocolGuid,
+  &MemoryAttributeProtocol
+  );
+  ASSERT_EFI_ERROR (Status);
+
+  Status = gBS->UninstallProtocolInterface (
+  Handle,
+  &gEfiMemoryAttributeProtocolGuid,
+  MemoryAttributeProtocol
+  );
+  ASSERT_EFI_ERROR (Status);
+}
+
 /**
   Do the platform specific action after the console is ready
   Possible things that can be done in PlatformBootManagerAfterConsole:
@@ -1129,12 +1173,37 @@ PlatformBootManagerAfterConsole (
   )
 {
   RETURN_STATUS  Status;
+  BOOLEANMemAttrProtocol;
 
   //
   // Show the splash screen.
   //
   BootLogoEnableLogo ();
 
+  //
+  // Work around shim's terminally broken use of the EFI memory attributes
+  // protocol, by just uninstalling it when requested on the QEMU command line.
+  //
+  Status = QemuFwCfgParseBool (
+ "opt/org.tianocore/MemAttrProtocol",
+ &MemAttrProtocol
+ );
+  if (RETURN_ERROR (Status)) {
+// default
+MemAttrProtocol = FALSE;
+  }
+
+  DEBUG ((
+DEBUG_ERROR,
+"%a: MemAttrProtocol = %a\n",
+__func__,
+MemAttrProtocol ? "yes" : "no"
+));
+
+  if (!MemAttrProtocol) {
+UninstallEfiMemoryAttributesProtocol ();
+  }
+
   //
   // Process QEMU's -kernel command line option. The kernel booted this way
   // will receive ACPI tables: in PlatformBootManagerBeforeConsole(), we
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112122): https://edk2.groups.io/g/devel/message/112122
Mute This Topic: https://groups.io/mt/102967690/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-redfish-client][PATCH] RedfishClientPkg/RedfishFeatureCoreDxe: fix Redfish event issue.

[Chang, Abner via groups.io]

[AMD Official Use Only - General]

Hi Nickle, one comment below.

> -Original Message-
> From: Nickle Wang 
> Sent: Wednesday, December 6, 2023 4:57 PM
> To: devel@edk2.groups.io
> Cc: Chang, Abner ; Igor Kulchytskyy
> ; Nick Ramirez 
> Subject: [edk2-redfish-client][PATCH]
> RedfishClientPkg/RedfishFeatureCoreDxe: fix Redfish event issue.
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> RedfishFeatureDriverStartup is callback function at TPL_CALLBACK
> level. In this function, Redfish events are signaled. However,
> Redfish events are created in TPL_CALLBACK level too. As the result,
> Redfish events cannot be invoked in desired sequence. Decrease the
> TPL to TPL_APPLICATION level inside RedfishFeatureDriverStartup and
> restore it to TPL_CALLBACK level before leaving this function. Now,
> Redfish events are called in correct sequence.
>
> Signed-off-by: Nickle Wang 
> Cc: Abner Chang 
> Cc: Igor Kulchytskyy 
> Cc: Nick Ramirez 
> ---
>  .../RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.h  |  1 +
>  .../RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.c  | 14 +-
>  2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git
> a/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.h
> b/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.h
> index acefa41b..de08d79d 100644
> --- a/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.h
> +++ b/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.h
> @@ -33,6 +33,7 @@
>  #define NodeIsCollectionLeftBracket   L'{'
>  #define NodeIsCollectionRightBracket  L'}'
>  #define NodeIsCollectionSymbolL"/{}"
> +#define REDFISH_FEATURE_CORE_TPL  TPL_CALLBACK
>
>  typedef struct _REDFISH_FEATURE_INTERNAL_DATA
> REDFISH_FEATURE_INTERNAL_DATA;
>  struct _REDFISH_FEATURE_INTERNAL_DATA {
> diff --git a/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.c
> b/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.c
> index f3188ddf..c0c3ec47 100644
> --- a/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.c
> +++ b/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.c
> @@ -272,6 +272,13 @@ RedfishFeatureDriverStartup (
>  return;
>}
>
> +  //
> +  // Lower the TPL to TPL_APPLICATION so that
> +  // Redfish event and report status code can be
> +  // triggered
> +  //
> +  gBS->RestoreTPL (TPL_APPLICATION);
> +
>//
>// Reset PcdRedfishSystemRebootRequired flag
>//
> @@ -321,6 +328,11 @@ RedfishFeatureDriverStartup (
>  gRT->ResetSystem (EfiResetCold, EFI_SUCCESS, 0, NULL);
>  CpuDeadLoop ();
>}
> +
> +  //
> +  // Restore to the TPL where this callback handler is called.
> +  //
> +  gBS->RaiseTPL (REDFISH_FEATURE_CORE_TPL);
If we use PCD here, then we have to also update CreateEventEx in the 
RedfishFeatureCoreEntryPoint. Create the event using   REDFISH_FEATURE_CORE_TPL.

Abner


>  }
>
>  /**
> @@ -670,7 +682,7 @@ RedfishFeatureCoreEntryPoint (
>
>Status = gBS->CreateEventEx (
>EVT_NOTIFY_SIGNAL,
> -  TPL_CALLBACK,
> +  REDFISH_FEATURE_CORE_TPL,
>RedfishFeatureDriverStartup,
>(CONST VOID *)&mFeatureDriverStartupContext,
>EventGuid,
> --
> 2.17.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112121): https://edk2.groups.io/g/devel/message/112121
Mute This Topic: https://groups.io/mt/103009658/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2][PATCH V1 2/2] DynamicTablesPkg/SsdtSerialPortFixupLib: Add Interrupt node for SPIs only

[Himanshu Sharma]

Tested on N1SDP and Morello.

Tested-by: Himanshu Sharma 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112120): https://edk2.groups.io/g/devel/message/112120
Mute This Topic: https://groups.io/mt/103010241/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 6/6] UefiCpuPkg: Avoid assuming only one smmbasehob

[Ni, Ray]

> +EFI_STATUS
> +GetSmBaseFromSmmBaseHob (
> +  IN  EFI_HOB_GUID_TYPE  *FirstSmmBaseGuidHob,
> +  IN  UINTN  MaxNumberOfCpus,
> +  OUT UINTN  **SmBaseBufferPointer
> +  )

1. It's a bit strange that caller should locate the first GuidHob.
Can you update the existing code as follows:
mCpuHotPlugData.SmBase = GetSmBase(mMaxNumberOfCpus);
if (mCpuHotPlugData.SmBase != NULL) {
  mSmmRelocated = TRUE;
}


> +{
> +  UINTN  HobCount;
> +  EFI_HOB_GUID_TYPE  *GuidHob;
> +  SMM_BASE_HOB_DATA  *SmmBaseHobData;
> +  UINTN  NumberOfProcessors;
> +  SMM_BASE_HOB_DATA  **SmBaseHobPointerBuffer;
> +  UINTN  *SmBaseBuffer;
> +  UINTN  Index;
> +  UINTN  SortBuffer;
> +  UINTN  ProcessorIndex;
> +  UINT64 PrevProcessorIndex;
> +
> +  SmmBaseHobData = NULL;
> +  Index  = 0;
> +  ProcessorIndex = 0;
> +  PrevProcessorIndex = 0;
> +  HobCount   = 0;
> +  NumberOfProcessors = 0;
> +  GuidHob= FirstSmmBaseGuidHob;
> +
> +  while (GuidHob != NULL) {
> +HobCount++;
> +SmmBaseHobData  = GET_GUID_HOB_DATA (GuidHob);
> +NumberOfProcessors += SmmBaseHobData->NumberOfProcessors;
> +GuidHob = GetNextGuidHob (&gSmmBaseHobGuid,
> GET_NEXT_HOB (GuidHob));

2. We could break the while-loop when NumberOfProcessors equals to the value we 
retrieved from MpInfo2Hob. Right?
This can speed up the code when there are lots of HOBs after the last 
SmmBaseHob instance.

> +  }
> +
> +  ASSERT (NumberOfProcessors == MaxNumberOfCpus);

3. ASSERT may fail when HotPlug is TRUE?


> +
> +  SmBaseHobPointerBuffer = AllocatePool (sizeof (SMM_BASE_HOB_DATA *)
> * HobCount);

4. SmBaseHobPointerBuffer -> SmBaseHobs

> +  for (Index = 0; Index < HobCount; Index++) {
> +//
> +// Make sure no overlap and no gap in the CPU range covered by each
> HOB
> +//
> +ASSERT (SmBaseHobPointerBuffer[Index]->ProcessorIndex ==
> PrevProcessorIndex);

5. similarly, can you move "PrevProcessorIndex" assignment to just above "for"?

> +
> +//
> +// Cache each SmBase in order.
> +//
> +if (sizeof (UINTN) == sizeof (UINT64)) {
> +  CopyMem (
> +SmBaseBuffer + PrevProcessorIndex,
> +&SmBaseHobPointerBuffer[Index]->SmBase,
> +sizeof (UINT64) *
> SmBaseHobPointerBuffer[Index]->NumberOfProcessors
> +);
> +} else {
> +  for (ProcessorIndex = 0; ProcessorIndex <
> SmBaseHobPointerBuffer[Index]->NumberOfProcessors; ProcessorIndex++) {
> +SmBaseBuffer[PrevProcessorIndex + ProcessorIndex] =
> (UINTN)SmBaseHobPointerBuffer[Index]->SmBase[ProcessorIndex];
> +  }
> +}


6. I don't like the "if-else" above. Can you just change SmBaseBuffer to UINT64 
*?
Or, you always use for-loop to copy SmBase value for each cpu.

> +
> +PrevProcessorIndex +=
> SmBaseHobPointerBuffer[Index]->NumberOfProcessors;
> +  }
> +
> +  FreePool (SmBaseHobPointerBuffer);
> +
> +  *SmBaseBufferPointer = SmBaseBuffer;

7. Similarly, how about return SmBaseBuffer?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112119): https://edk2.groups.io/g/devel/message/112119
Mute This Topic: https://groups.io/mt/102987142/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [edk2][PATCH V1 2/2] DynamicTablesPkg/SsdtSerialPortFixupLib: Add Interrupt node for SPIs only

[Himanshu Sharma]

Add interrupt node to the AML description of the serial-port only if the
IRQ ID from the Configuration Manager is a valid SPI (shared processor
interrupt) or an extended SPI. So, for DBG2 UART ports where interrupt
is not mandatory, adding of an interrupt node in the AML description
using Serial Port Fixup Library can be ignored if the UART is not
defined with a valid SPI, like in N1SDP.

This update generates the interrupt node for the valid SPI range using
the AML Codegen API instead of updating it using the AML Fixup API.

Signed-off-by: Himanshu Sharma 
---
 
DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.inf
 |  3 +-
 
DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.c 
  | 38 
 
DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortTemplate.asl
 | 29 ---
 3 files changed, 42 insertions(+), 28 deletions(-)

diff --git 
a/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.inf
 
b/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.inf
index 965167bdc4e1..2d16a22aeb41 100644
--- 
a/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.inf
+++ 
b/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.inf
@@ -1,7 +1,7 @@
 ## @file
 #  SSDT Serial Port fixup Library
 #
-#  Copyright (c) 2020 - 2021, Arm Limited. All rights reserved.
+#  Copyright (c) 2020 - 2021, 2023, Arm Limited. All rights reserved.
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 ##
@@ -23,6 +23,7 @@
   MdeModulePkg/MdeModulePkg.dec
   EmbeddedPkg/EmbeddedPkg.dec
   DynamicTablesPkg/DynamicTablesPkg.dec
+  ArmPkg/ArmPkg.dec
 
 [LibraryClasses]
   AcpiHelperLib
diff --git 
a/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.c
 
b/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.c
index a65c1fe7e30d..fb77136aa844 100644
--- 
a/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.c
+++ 
b/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.c
@@ -1,7 +1,7 @@
 /** @file
   SSDT Serial Port Fixup Library.
 
-  Copyright (c) 2019 - 2021, Arm Limited. All rights reserved.
+  Copyright (c) 2019 - 2021, 2023, Arm Limited. All rights reserved.
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -9,10 +9,12 @@
   - Arm Server Base Boot Requirements (SBBR), s4.2.1.8 "SPCR".
   - Microsoft Debug Port Table 2 (DBG2) Specification - December 10, 2015.
   - ACPI for Arm Components 1.0 - 2020
+  - Arm Generic Interrupt Controller Architecture Specification, Issue H, 
January 2022.
 **/
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -270,7 +272,6 @@ FixupCrs (
   EFI_STATUS  Status;
   AML_OBJECT_NODE_HANDLE  NameOpCrsNode;
   AML_DATA_NODE_HANDLEQWordRdNode;
-  AML_DATA_NODE_HANDLEInterruptRdNode;
 
   // Get the "_CRS" object defined by the "Name ()" statement.
   Status = AmlFindNode (
@@ -303,20 +304,27 @@ FixupCrs (
 return Status;
   }
 
-  // Get the Interrupt node.
-  // It is the second Resource Data element in the NameOpCrsNode's
-  // variable list of arguments.
-  Status = AmlNameOpGetNextRdNode (QWordRdNode, &InterruptRdNode);
-  if (EFI_ERROR (Status)) {
-return Status;
+  // Generate an interrupt node if the interrupt for the serial-port is a 
valid SPI.
+  // SPI ranges from Table 2-1 in Arm Generic Interrupt Controller 
Architecture Specification.
+  if (((SerialPortInfo->Interrupt >= ARM_GIC_ARCH_SPI_MIN) &&
+   (SerialPortInfo->Interrupt <= ARM_GIC_ARCH_SPI_MAX)) ||
+  ((SerialPortInfo->Interrupt >= ARM_GIC_ARCH_EXT_SPI_MIN) &&
+   (SerialPortInfo->Interrupt <= ARM_GIC_ARCH_EXT_SPI_MAX))) {
+Status = AmlCodeGenRdInterrupt (
+  1,   // Resource Consumer
+  0,   // Level Triggered
+  0,   // Active High
+  0,   // Exclusive
+  (UINT32 *)&SerialPortInfo->Interrupt,
+  1,
+  NameOpCrsNode,
+  NULL
+);
+if (EFI_ERROR (Status)) {
+  return Status;
+}
   }
-
-  if (InterruptRdNode == NULL) {
-return EFI_INVALID_PARAMETER;
-  }
-
-  // Update the interrupt number.
-  return AmlUpdateRdInterrupt (InterruptRdNode, SerialPortInfo->Interrupt);
+  return EFI_SUCCESS;
 }
 
 /** Fixup the Serial Port device name.
diff --git 
a/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortTemplate.asl
 
b/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortTemplate.asl
index fcae2160ac3d..46f800b0cdad 100644
--- 
a/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortTemplate.asl
+++ 
b/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortTemplate.asl
@@ -1,7 +1,7 @@
 /** @file
   SSDT Serial Template
 
-  Copyright (c) 2019 - 2020, Arm 

[edk2-devel] [edk2][PATCH V1 1/2] ArmPkg/ArmGicArchLib: Add macros for SPI and extended SPI ranges

[Himanshu Sharma]

Taking reference from Table 2-1 of the Arm Generic Interrupt Controller
Architecture Specification, Issue H, January 2022, add macros for the
SPI and extended SPI ranges with the purpose of reusability on including
the ArmPkg.

Signed-off-by: Himanshu Sharma 
---
 ArmPkg/Include/Library/ArmGicArchLib.h | 9 +
 1 file changed, 9 insertions(+)

diff --git a/ArmPkg/Include/Library/ArmGicArchLib.h 
b/ArmPkg/Include/Library/ArmGicArchLib.h
index 72ac17e13b5a..1b90b354f785 100644
--- a/ArmPkg/Include/Library/ArmGicArchLib.h
+++ b/ArmPkg/Include/Library/ArmGicArchLib.h
@@ -1,6 +1,7 @@
 /** @file
 *
 *  Copyright (c) 2015, Linaro Ltd. All rights reserved.
+*  Copyright (c) 2023, Arm Limited. All rights reserved.
 *
 *  SPDX-License-Identifier: BSD-2-Clause-Patent
 *
@@ -23,4 +24,12 @@ ArmGicGetSupportedArchRevision (
   VOID
   );
 
+//
+// GIC SPI and extended SPI ranges
+//
+#define ARM_GIC_ARCH_SPI_MIN  32
+#define ARM_GIC_ARCH_SPI_MAX  1019
+#define ARM_GIC_ARCH_EXT_SPI_MIN  4096
+#define ARM_GIC_ARCH_EXT_SPI_MAX  5119
+
 #endif // ARM_GIC_ARCH_LIB_H_
-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112117): https://edk2.groups.io/g/devel/message/112117
Mute This Topic: https://groups.io/mt/103010240/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [edk2][PATCH V1 0/2] Update handling of interrupt node for SSDT Serial Port Fixup Library

[Himanshu Sharma]

Currently in the Dynamic Tables Framework, the interrupt node for the
AML description of the serial-ports is populated using the template
and so is mandatorily added even if the serial-port is enumerated as
a DBG2 port in the platform's configuration manager where the
interrupt is not mandatory. The proposed implementation adds the
interrupt node only if the interrupt defined for the serial-port is a
valid SPI or a valid extended SPI. So, in case of DBG2 ports, 
he platforms with interrupt defined as SPI (like Morello) can have the
interrupt node added to the description and the platforms where it is
not defined (like N1SDP) can ignore the addition of the interrupt node.

The changes include adding the SPI range macros in ArmGicArchLib
(ArmPkg) which can be used by the SSDTSerialPortFixupLib
(DynamicTablesPkg) to put a check for generating the interrupt node
using AML Codegen API.

Link to branch with the patches in this series -
https://github.com/himsha01/edk2/tree/ssdt_serial_port_interrupt

Himanshu Sharma (2):
  ArmPkg/ArmGicArchLib: Add macros for SPI and extended SPI ranges
  DynamicTablesPkg/SsdtSerialPortFixupLib: Add Interrupt node for SPIs only

 
DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.inf
 |  3 +-
 ArmPkg/Include/Library/ArmGicArchLib.h 
   |  9 +
 
DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.c 
  | 38 
 
DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortTemplate.asl
 | 29 ---
 4 files changed, 51 insertions(+), 28 deletions(-)

-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112116): https://edk2.groups.io/g/devel/message/112116
Mute This Topic: https://groups.io/mt/103010239/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 6/6] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SmmCpuSyncLib

[Wu, Jiaxin]

There is the SmmCpuSyncLib Library class define the SMM CPU sync
flow, which is aligned with existing SMM CPU driver sync behavior.
This patch is to consume SmmCpuSyncLib instance directly.

With this change, SMM CPU Sync flow/logic can be customized
with different implementation no matter for any purpose, e.g.
performance tuning, handle specific register, etc.

Cc: Laszlo Ersek 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Zeng Star 
Cc: Gerd Hoffmann 
Cc: Rahul Kumar 
Signed-off-by: Jiaxin Wu 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c| 317 +--
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   |   6 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf |   1 +
 3 files changed, 110 insertions(+), 214 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index 54542262a2..e37c03d0e5 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -27,122 +27,10 @@ MM_COMPLETIONmSmmStartupThisApToken;
 //
 // Processor specified by mPackageFirstThreadIndex[PackageIndex] will do the 
package-scope register check.
 //
 UINT32  *mPackageFirstThreadIndex = NULL;
 
-/**
-  Performs an atomic compare exchange operation to get semaphore.
-  The compare exchange operation must be performed using
-  MP safe mechanisms.
-
-  @param  SemIN:  32-bit unsigned integer
- OUT: original integer - 1
-  @return Original integer - 1
-
-**/
-UINT32
-WaitForSemaphore (
-  IN OUT  volatile UINT32  *Sem
-  )
-{
-  UINT32  Value;
-
-  for ( ; ;) {
-Value = *Sem;
-if ((Value != 0) &&
-(InterlockedCompareExchange32 (
-   (UINT32 *)Sem,
-   Value,
-   Value - 1
-   ) == Value))
-{
-  break;
-}
-
-CpuPause ();
-  }
-
-  return Value - 1;
-}
-
-/**
-  Performs an atomic compare exchange operation to release semaphore.
-  The compare exchange operation must be performed using
-  MP safe mechanisms.
-
-  @param  SemIN:  32-bit unsigned integer
- OUT: original integer + 1
-  @return Original integer + 1
-
-**/
-UINT32
-ReleaseSemaphore (
-  IN OUT  volatile UINT32  *Sem
-  )
-{
-  UINT32  Value;
-
-  do {
-Value = *Sem;
-  } while (Value + 1 != 0 &&
-   InterlockedCompareExchange32 (
- (UINT32 *)Sem,
- Value,
- Value + 1
- ) != Value);
-
-  return Value + 1;
-}
-
-/**
-  Performs an atomic compare exchange operation to lock semaphore.
-  The compare exchange operation must be performed using
-  MP safe mechanisms.
-
-  @param  SemIN:  32-bit unsigned integer
- OUT: -1
-  @return Original integer
-
-**/
-UINT32
-LockdownSemaphore (
-  IN OUT  volatile UINT32  *Sem
-  )
-{
-  UINT32  Value;
-
-  do {
-Value = *Sem;
-  } while (InterlockedCompareExchange32 (
- (UINT32 *)Sem,
- Value,
- (UINT32)-1
- ) != Value);
-
-  return Value;
-}
-
-/**
-  Used for BSP to wait all APs.
-  Wait all APs to performs an atomic compare exchange operation to release 
semaphore.
-
-  @param   NumberOfAPs  AP number
-
-**/
-VOID
-WaitForAllAPs (
-  IN  UINTN  NumberOfAPs
-  )
-{
-  UINTN  BspIndex;
-
-  BspIndex = mSmmMpSyncData->BspIndex;
-  while (NumberOfAPs-- > 0) {
-WaitForSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run);
-  }
-}
-
 /**
   Used for BSP to release all APs.
   Performs an atomic compare exchange operation to release semaphore
   for each AP.
 
@@ -154,57 +42,15 @@ ReleaseAllAPs (
 {
   UINTN  Index;
 
   for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
 if (IsPresentAp (Index)) {
-  ReleaseSemaphore (mSmmMpSyncData->CpuData[Index].Run);
+  SmmCpuSyncReleaseOneAp (mSmmMpSyncData->SmmCpuSyncCtx, Index, 
gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu);
 }
   }
 }
 
-/**
-  Used for BSP to release one AP.
-
-  @param  ApSem IN:  32-bit unsigned integer
-OUT: original integer + 1
-**/
-VOID
-ReleaseOneAp   (
-  IN OUT  volatile UINT32  *ApSem
-  )
-{
-  ReleaseSemaphore (ApSem);
-}
-
-/**
-  Used for AP to wait BSP.
-
-  @param  ApSem  IN:  32-bit unsigned integer
- OUT: original integer - 1
-**/
-VOID
-WaitForBsp  (
-  IN OUT  volatile UINT32  *ApSem
-  )
-{
-  WaitForSemaphore (ApSem);
-}
-
-/**
-  Used for AP to release BSP.
-
-  @param  BspSem IN:  32-bit unsigned integer
- OUT: original integer + 1
-**/
-VOID
-ReleaseBsp   (
-  IN OUT  volatile UINT32  *BspSem
-  )
-{
-  ReleaseSemaphore (BspSem);
-}
-
 /**
   Check whether the index of CPU perform the package level register
   programming during System Management Mode initialization.
 
   The index of Processor specified by mPackageFirstThreadIndex[PackageIndex]
@@ -285,42 +131,53 @@ GetSmmDelayedBlockedDisabledCount (
 BOOLEAN
 AllCpusInSmmExceptBlockedDis

Re: [edk2-devel] [PATCH 5/6] UefiCpuPkg: Cache core type in MpInfo2 HOB

[Ni, Ray]

>  /**
>Create gMpInformationHobGuid2.
>  **/
> @@ -558,13 +582,36 @@ BuildMpInformationHob (
>MP_INFORMATION2_HOB_DATA  *MpInformation2HobData;
>MP_INFORMATION2_ENTRY *MpInformation2Entry;
>UINTN Index;
> +  UINT8 *CoreType;
> +  UINT32CpuidMaxInput;
> +  UINTN Pages;

1. CoreTypes, CoreTypePages


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112113): https://edk2.groups.io/g/devel/message/112113
Mute This Topic: https://groups.io/mt/102987141/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 5/6] UefiPayloadPkg: Specifies SmmCpuSyncLib instance

[Wu, Jiaxin]

This patch is to specify SmmCpuSyncLib instance for UefiPayloadPkg.

Cc: Laszlo Ersek 
Cc: Guo Dong 
Cc: Sean Rhodes 
Cc: James Lu 
Cc: Gua Guo 
Cc: Ray Ni 
Cc: Zeng Star 
Signed-off-by: Jiaxin Wu 
Reviewed-by: Gua Guo 
---
 UefiPayloadPkg/UefiPayloadPkg.dsc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/UefiPayloadPkg/UefiPayloadPkg.dsc 
b/UefiPayloadPkg/UefiPayloadPkg.dsc
index a65f9d5b83..b8b13ad201 100644
--- a/UefiPayloadPkg/UefiPayloadPkg.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkg.dsc
@@ -253,10 +253,11 @@
   #
   MtrrLib|UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
   LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
   MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
   CpuPageTableLib|UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableLib.inf
+  SmmCpuSyncLib|UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
 
   #
   # Platform
   #
 !if $(CPU_TIMER_LIB_ENABLE) == TRUE && $(UNIVERSAL_PAYLOAD) == TRUE
-- 
2.16.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112114): https://edk2.groups.io/g/devel/message/112114
Mute This Topic: https://groups.io/mt/103010167/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 4/6] OvmfPkg: Specifies SmmCpuSyncLib instance

[Wu, Jiaxin]

This patch is to specify SmmCpuSyncLib instance for OvmfPkg.

Cc: Laszlo Ersek 
Cc: Ard Biesheuvel 
Cc: Jiewen Yao 
Cc: Jordan Justen 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Zeng Star 
Cc: Rahul Kumar 
Cc: Gerd Hoffmann 
Signed-off-by: Jiaxin Wu 
---
 OvmfPkg/CloudHv/CloudHvX64.dsc | 2 ++
 OvmfPkg/OvmfPkgIa32.dsc| 2 ++
 OvmfPkg/OvmfPkgIa32X64.dsc | 2 ++
 OvmfPkg/OvmfPkgX64.dsc | 1 +
 4 files changed, 7 insertions(+)

diff --git a/OvmfPkg/CloudHv/CloudHvX64.dsc b/OvmfPkg/CloudHv/CloudHvX64.dsc
index 821ad1b9fa..f735b69a37 100644
--- a/OvmfPkg/CloudHv/CloudHvX64.dsc
+++ b/OvmfPkg/CloudHv/CloudHvX64.dsc
@@ -183,10 +183,12 @@
   PeiHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/PeiHardwareInfoLib.inf
   DxeHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/DxeHardwareInfoLib.inf
   
ImagePropertiesRecordLib|MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.inf
 !if $(SMM_REQUIRE) == FALSE
   LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
+!else
+  SmmCpuSyncLib|UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
 !endif
   
CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
   
FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf
   
MemEncryptTdxLib|OvmfPkg/Library/BaseMemEncryptTdxLib/BaseMemEncryptTdxLib.inf
 
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index bce2aedcd7..b05b13b18c 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -188,10 +188,12 @@
   PeiHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/PeiHardwareInfoLib.inf
   DxeHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/DxeHardwareInfoLib.inf
   
ImagePropertiesRecordLib|MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.inf
 !if $(SMM_REQUIRE) == FALSE
   LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
+!else
+  SmmCpuSyncLib|UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
 !endif
   
CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
   
FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf
 
 !if $(SOURCE_DEBUG_ENABLE) == TRUE
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 631e909a54..5a16eb7abe 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -193,10 +193,12 @@
   PeiHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/PeiHardwareInfoLib.inf
   DxeHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/DxeHardwareInfoLib.inf
   
ImagePropertiesRecordLib|MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.inf
 !if $(SMM_REQUIRE) == FALSE
   LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
+!else
+  SmmCpuSyncLib|UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
 !endif
   
CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
   
FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf
 
 !if $(SOURCE_DEBUG_ENABLE) == TRUE
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 4ea3008cc6..6bb4c777b9 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -209,10 +209,11 @@
 !if $(SMM_REQUIRE) == FALSE
   LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
   CcProbeLib|OvmfPkg/Library/CcProbeLib/DxeCcProbeLib.inf
 !else
   CcProbeLib|MdePkg/Library/CcProbeLibNull/CcProbeLibNull.inf
+  SmmCpuSyncLib|UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
 !endif
   
CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
   
FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf
 
 !if $(SOURCE_DEBUG_ENABLE) == TRUE
-- 
2.16.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112112): https://edk2.groups.io/g/devel/message/112112
Mute This Topic: https://groups.io/mt/103010166/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 3/6] UefiCpuPkg: Implements SmmCpuSyncLib library instance

[Wu, Jiaxin]

Implements SmmCpuSyncLib Library instance. The instance refers the
existing SMM CPU driver (PiSmmCpuDxeSmm) sync implementation
and behavior:
1.Abstract Counter and Run semaphores into SmmCpuSyncCtx.
2.Abstract CPU arrival count operation to
SmmCpuSyncGetArrivedCpuCount(), SmmCpuSyncCheckInCpu(),
SmmCpuSyncCheckOutCpu(), SmmCpuSyncLockDoor().
Implementation is aligned with existing SMM CPU driver.
3. Abstract SMM CPU Sync flow to:
BSP: SmmCpuSyncReleaseOneAp  -->  AP: SmmCpuSyncWaitForBsp
BSP: SmmCpuSyncWaitForAPs<--  AP: SmmCpuSyncReleaseBsp
Semaphores release & wait during sync flow is same as existing SMM
CPU driver.
4.Same operation to Counter and Run semaphores by leverage the atomic
compare exchange.

Cc: Laszlo Ersek 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Zeng Star 
Cc: Gerd Hoffmann 
Cc: Rahul Kumar 
Signed-off-by: Jiaxin Wu 
---
 UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c   | 647 +
 UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf |  39 ++
 UefiCpuPkg/UefiCpuPkg.dsc  |   3 +
 3 files changed, 689 insertions(+)
 create mode 100644 UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c
 create mode 100644 UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf

diff --git a/UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c 
b/UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c
new file mode 100644
index 00..3c2835f8de
--- /dev/null
+++ b/UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c
@@ -0,0 +1,647 @@
+/** @file
+  SMM CPU Sync lib implementation.
+  The lib provides 3 sets of APIs:
+  1. ContextInit/ContextDeinit/ContextReset:
+  ContextInit() is called in driver's entrypoint to allocate and initialize 
the SMM CPU Sync context.
+  ContextDeinit() is called in driver's unload function to deinitialize the 
SMM CPU Sync context.
+  ContextReset() is called before CPU exist SMI, which allows CPU to check 
into the next SMI from this point.
+
+  2. GetArrivedCpuCount/CheckInCpu/CheckOutCpu/LockDoor:
+  When SMI happens, all processors including BSP enter to SMM mode by calling 
CheckInCpu().
+  The elected BSP calls LockDoor() so that CheckInCpu() will return the error 
code after that.
+  CheckOutCpu() can be called in error handling flow for the CPU who calls 
CheckInCpu() earlier.
+  GetArrivedCpuCount() returns the number of checked-in CPUs.
+
+  3. WaitForAPs/ReleaseOneAp/WaitForBsp/ReleaseBsp
+  WaitForAPs() & ReleaseOneAp() are called from BSP to wait the number of APs 
and release one specific AP.
+  WaitForBsp() & ReleaseBsp() are called from APs to wait and release BSP.
+  The 4 APIs are used to synchronize the running flow among BSP and APs. BSP 
and AP Sync flow can be
+  easy understand as below:
+  BSP: ReleaseOneAp  -->  AP: WaitForBsp
+  BSP: WaitForAPs<--  AP: ReleaseBsp
+
+  Copyright (c) 2023, Intel Corporation. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+typedef struct {
+  ///
+  /// Indicate how many CPU entered SMM.
+  ///
+  volatile UINT32*Counter;
+} SMM_CPU_SYNC_SEMAPHORE_GLOBAL;
+
+typedef struct {
+  ///
+  /// Used for control each CPU continue run or wait for signal
+  ///
+  volatile UINT32*Run;
+} SMM_CPU_SYNC_SEMAPHORE_CPU;
+
+struct SMM_CPU_SYNC_CONTEXT  {
+  ///
+  ///  All global semaphores' pointer in SMM CPU Sync
+  ///
+  SMM_CPU_SYNC_SEMAPHORE_GLOBAL*GlobalSem;
+  ///
+  ///  All semaphores for each processor in SMM CPU Sync
+  ///
+  SMM_CPU_SYNC_SEMAPHORE_CPU   *CpuSem;
+  ///
+  /// The number of processors in the system.
+  /// This does not indicate the number of processors that entered SMM.
+  ///
+  UINTNNumberOfCpus;
+  ///
+  /// Address of global and each CPU semaphores
+  ///
+  UINTN*SemBuffer;
+  ///
+  /// Size in bytes of global and each CPU semaphores
+  ///
+  UINTNSemBufferSize;
+};
+
+/**
+  Performs an atomic compare exchange operation to get semaphore.
+  The compare exchange operation must be performed using MP safe
+  mechanisms.
+
+  @param[in,out]  SemIN:  32-bit unsigned integer
+ OUT: original integer - 1
+
+  @retval Original integer - 1
+
+**/
+UINT32
+InternalWaitForSemaphore (
+  IN OUT  volatile UINT32  *Sem
+  )
+{
+  UINT32  Value;
+
+  for ( ; ;) {
+Value = *Sem;
+if ((Value != 0) &&
+(InterlockedCompareExchange32 (
+   (UINT32 *)Sem,
+   Value,
+   Value - 1
+   ) == Value))
+{
+  break;
+}
+
+CpuPause ();
+  }
+
+  return Value - 1;
+}
+
+/**
+  Performs an atomic compare exchange operation to release semaphore.
+  The compare exchange operation must be performed using MP safe
+  mechanisms.
+
+  @param[in,out]  SemIN:  32-bit unsigned integer
+ OUT: original integer + 1
+
+  @retvalOrigin

[edk2-devel] [PATCH v3 1/6] UefiCpuPkg/PiSmmCpuDxeSmm: Optimize Semaphore Sync between BSP and AP

[Wu, Jiaxin]

This patch is to define 3 new functions (WaitForBsp & ReleaseBsp &
ReleaseOneAp) used for the semaphore sync between BSP & AP. With the
change, BSP and AP Sync flow will be easy understand as below:
BSP: ReleaseAllAPs or ReleaseOneAp --> AP: WaitForBsp
BSP: WaitForAllAPs <-- AP: ReleaseBsp

Cc: Laszlo Ersek 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Zeng Star 
Cc: Rahul Kumar 
Cc: Gerd Hoffmann 
Signed-off-by: Jiaxin Wu 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 72 ---
 1 file changed, 58 insertions(+), 14 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index b279f5dfcc..54542262a2 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -120,10 +120,11 @@ LockdownSemaphore (
 
   return Value;
 }
 
 /**
+  Used for BSP to wait all APs.
   Wait all APs to performs an atomic compare exchange operation to release 
semaphore.
 
   @param   NumberOfAPs  AP number
 
 **/
@@ -139,10 +140,11 @@ WaitForAllAPs (
 WaitForSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run);
   }
 }
 
 /**
+  Used for BSP to release all APs.
   Performs an atomic compare exchange operation to release semaphore
   for each AP.
 
 **/
 VOID
@@ -157,10 +159,52 @@ ReleaseAllAPs (
   ReleaseSemaphore (mSmmMpSyncData->CpuData[Index].Run);
 }
   }
 }
 
+/**
+  Used for BSP to release one AP.
+
+  @param  ApSem IN:  32-bit unsigned integer
+OUT: original integer + 1
+**/
+VOID
+ReleaseOneAp   (
+  IN OUT  volatile UINT32  *ApSem
+  )
+{
+  ReleaseSemaphore (ApSem);
+}
+
+/**
+  Used for AP to wait BSP.
+
+  @param  ApSem  IN:  32-bit unsigned integer
+ OUT: original integer - 1
+**/
+VOID
+WaitForBsp  (
+  IN OUT  volatile UINT32  *ApSem
+  )
+{
+  WaitForSemaphore (ApSem);
+}
+
+/**
+  Used for AP to release BSP.
+
+  @param  BspSem IN:  32-bit unsigned integer
+ OUT: original integer + 1
+**/
+VOID
+ReleaseBsp   (
+  IN OUT  volatile UINT32  *BspSem
+  )
+{
+  ReleaseSemaphore (BspSem);
+}
+
 /**
   Check whether the index of CPU perform the package level register
   programming during System Management Mode initialization.
 
   The index of Processor specified by mPackageFirstThreadIndex[PackageIndex]
@@ -632,11 +676,11 @@ BSPHandler (
   // Signal all APs it's time for backup MTRRs
   //
   ReleaseAllAPs ();
 
   //
-  // WaitForSemaphore() may wait for ever if an AP happens to enter SMM at
+  // WaitForAllAPs() may wait for ever if an AP happens to enter SMM at
   // exactly this point. Please make sure PcdCpuSmmMaxSyncLoops has been 
set
   // to a large enough value to avoid this situation.
   // Note: For HT capable CPUs, threads within a core share the same set 
of MTRRs.
   // We do the backup first and then set MTRR to avoid race condition for 
threads
   // in the same core.
@@ -652,11 +696,11 @@ BSPHandler (
   // Let all processors program SMM MTRRs together
   //
   ReleaseAllAPs ();
 
   //
-  // WaitForSemaphore() may wait for ever if an AP happens to enter SMM at
+  // WaitForAllAPs() may wait for ever if an AP happens to enter SMM at
   // exactly this point. Please make sure PcdCpuSmmMaxSyncLoops has been 
set
   // to a large enough value to avoid this situation.
   //
   ReplaceOSMtrrs (CpuIndex);
 
@@ -898,50 +942,50 @@ APHandler (
 
   if ((SyncMode == SmmCpuSyncModeTradition) || 
SmmCpuFeaturesNeedConfigureMtrrs ()) {
 //
 // Notify BSP of arrival at this point
 //
-ReleaseSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run);
+ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
   }
 
   if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
 //
 // Wait for the signal from BSP to backup MTRRs
 //
-WaitForSemaphore (mSmmMpSyncData->CpuData[CpuIndex].Run);
+WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
 
 //
 // Backup OS MTRRs
 //
 MtrrGetAllMtrrs (&Mtrrs);
 
 //
 // Signal BSP the completion of this AP
 //
-ReleaseSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run);
+ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
 
 //
 // Wait for BSP's signal to program MTRRs
 //
-WaitForSemaphore (mSmmMpSyncData->CpuData[CpuIndex].Run);
+WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
 
 //
 // Replace OS MTRRs with SMI MTRRs
 //
 ReplaceOSMtrrs (CpuIndex);
 
 //
 // Signal BSP the completion of this AP
 //
-ReleaseSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run);
+ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
   }
 
   while (TRUE) {
 //
 // Wait for something to happen
 //
-WaitForSemaphore (mSmmMpSyncData->CpuData[CpuIndex].Run);
+WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
 
 //
 // Check if BSP wants to exit SMM
 //
 if (

[edk2-devel] [PATCH v3 2/6] UefiCpuPkg: Adds SmmCpuSyncLib library class

[Wu, Jiaxin]

Intel is planning to provide different SMM CPU Sync implementation
along with some specific registers to improve the SMI performance,
hence need SmmCpuSyncLib Library for Intel.

This patch is to:
1.Adds SmmCpuSyncLib Library class in UefiCpuPkg.dec.
2.Adds SmmCpuSyncLib.h function declaration header file.

For the new SmmCpuSyncLib, it provides 3 sets of APIs:

1. ContextInit/ContextDeinit/ContextReset:
ContextInit() is called in driver's entrypoint to allocate and
initialize the SMM CPU Sync context. ContextDeinit() is called in
driver's unload function to deinitialize SMM CPU Sync context.
ContextReset() is called before CPU exist SMI, which allows CPU to
check into the next SMI from this point.

2. GetArrivedCpuCount/CheckInCpu/CheckOutCpu/LockDoor:
When SMI happens, all processors including BSP enter to SMM mode by
calling CheckInCpu(). The elected BSP calls LockDoor() so that
CheckInCpu() will return the error code after that. CheckOutCpu() can
be called in error handling flow for the CPU who calls CheckInCpu()
earlier. GetArrivedCpuCount() returns the number of checked-in CPUs.

3. WaitForAPs/ReleaseOneAp/WaitForBsp/ReleaseBsp
WaitForAPs() & ReleaseOneAp() are called from BSP to wait the number
of APs and release one specific AP. WaitForBsp() & ReleaseBsp() are
called from APs to wait and release BSP. The 4 APIs are used to
synchronize the running flow among BSP and APs. BSP and AP Sync flow
can be easy understand as below:
BSP: ReleaseOneAp  -->  AP: WaitForBsp
BSP: WaitForAPs<--  AP: ReleaseBsp

Cc: Laszlo Ersek 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Zeng Star 
Cc: Gerd Hoffmann 
Cc: Rahul Kumar 
Signed-off-by: Jiaxin Wu 
---
 UefiCpuPkg/Include/Library/SmmCpuSyncLib.h | 275 +
 UefiCpuPkg/UefiCpuPkg.dec  |   3 +
 2 files changed, 278 insertions(+)
 create mode 100644 UefiCpuPkg/Include/Library/SmmCpuSyncLib.h

diff --git a/UefiCpuPkg/Include/Library/SmmCpuSyncLib.h 
b/UefiCpuPkg/Include/Library/SmmCpuSyncLib.h
new file mode 100644
index 00..0f9eb3414a
--- /dev/null
+++ b/UefiCpuPkg/Include/Library/SmmCpuSyncLib.h
@@ -0,0 +1,275 @@
+/** @file
+  Library that provides SMM CPU Sync related operations.
+  The lib provides 3 sets of APIs:
+  1. ContextInit/ContextDeinit/ContextReset:
+  ContextInit() is called in driver's entrypoint to allocate and initialize 
the SMM CPU Sync context.
+  ContextDeinit() is called in driver's unload function to deinitialize the 
SMM CPU Sync context.
+  ContextReset() is called before CPU exist SMI, which allows CPU to check 
into the next SMI from this point.
+
+  2. GetArrivedCpuCount/CheckInCpu/CheckOutCpu/LockDoor:
+  When SMI happens, all processors including BSP enter to SMM mode by calling 
CheckInCpu().
+  The elected BSP calls LockDoor() so that CheckInCpu() will return the error 
code after that.
+  CheckOutCpu() can be called in error handling flow for the CPU who calls 
CheckInCpu() earlier.
+  GetArrivedCpuCount() returns the number of checked-in CPUs.
+
+  3. WaitForAPs/ReleaseOneAp/WaitForBsp/ReleaseBsp
+  WaitForAPs() & ReleaseOneAp() are called from BSP to wait the number of APs 
and release one specific AP.
+  WaitForBsp() & ReleaseBsp() are called from APs to wait and release BSP.
+  The 4 APIs are used to synchronize the running flow among BSP and APs. BSP 
and AP Sync flow can be
+  easy understand as below:
+  BSP: ReleaseOneAp  -->  AP: WaitForBsp
+  BSP: WaitForAPs<--  AP: ReleaseBsp
+
+  Copyright (c) 2023, Intel Corporation. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef SMM_CPU_SYNC_LIB_H_
+#define SMM_CPU_SYNC_LIB_H_
+
+#include 
+
+//
+// Opaque structure for SMM CPU Sync context.
+//
+typedef struct SMM_CPU_SYNC_CONTEXT SMM_CPU_SYNC_CONTEXT;
+
+/**
+  Create and initialize the SMM CPU Sync context.
+
+  SmmCpuSyncContextInit() function is to allocate and initialize the SMM CPU 
Sync context.
+
+  @param[in]  NumberOfCpus  The number of Logical Processors in the 
system.
+  @param[out] SmmCpuSyncCtx Pointer to the new created and initialized 
SMM CPU Sync context object.
+NULL will be returned if any error happen 
during init.
+
+  @retval RETURN_SUCCESSThe SMM CPU Sync context was successful 
created and initialized.
+  @retval RETURN_INVALID_PARAMETER  SmmCpuSyncCtx is NULL.
+  @retval RETURN_BUFFER_TOO_SMALL   Overflow happen
+  @retval RETURN_OUT_OF_RESOURCES   There are not enough resources available 
to create and initialize SMM CPU Sync context.
+
+**/
+RETURN_STATUS
+EFIAPI
+SmmCpuSyncContextInit (
+  IN   UINTN NumberOfCpus,
+  OUT  SMM_CPU_SYNC_CONTEXT  **SmmCpuSyncCtx
+  );
+
+/**
+  Deinit an allocated SMM CPU Sync context.
+
+  SmmCpuSyncContextDeinit() function is to deinitialize SMM CPU Sync context, 
the resources allocated in
+  SmmCpuSyncContextInit() will be freed.
+
+  Note: This function only can be called after SmmCpuSyncContextInit() return 
succes

[edk2-devel] [PATCH v3 0/6] Refine SMM CPU Sync flow and abstract SmmCpuSyncLib

[Wu, Jiaxin]

The series patches are to refine SMM CPU Sync flow.
After the refinement, SmmCpuSyncLib is abstracted for
any user to provide different SMM CPU Sync implementation.

Compared to V2, has following refinement & changes:
1. rename SMM_CPU_SYNC_CXT to SMM_CPU_SYNC_CONTEXT
2. rename SemBlock to SemBuffer, SemBlockPages to SemBufferSize
3. remove empty lines among the local variable declarations
4. add assert before if condition check
5. update the comments for SmmCpuSyncCheckOutCpu,
SmmCpuSyncGetArrivedCpuCount and SmmCpuSyncLockDoor.
6. remove unnecessary cxt local variable declarations.

Jiaxin Wu (6):
  UefiCpuPkg/PiSmmCpuDxeSmm: Optimize Semaphore Sync between BSP and AP
  UefiCpuPkg: Adds SmmCpuSyncLib library class
  UefiCpuPkg: Implements SmmCpuSyncLib library instance
  OvmfPkg: Specifies SmmCpuSyncLib instance
  UefiPayloadPkg: Specifies SmmCpuSyncLib instance
  UefiCpuPkg/PiSmmCpuDxeSmm: Consume SmmCpuSyncLib

 OvmfPkg/CloudHv/CloudHvX64.dsc |   2 +
 OvmfPkg/OvmfPkgIa32.dsc|   2 +
 OvmfPkg/OvmfPkgIa32X64.dsc |   2 +
 OvmfPkg/OvmfPkgX64.dsc |   1 +
 UefiCpuPkg/Include/Library/SmmCpuSyncLib.h | 275 +
 UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c   | 647 +
 UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf |  39 ++
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c  | 275 -
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h |   6 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf   |   1 +
 UefiCpuPkg/UefiCpuPkg.dec  |   3 +
 UefiCpuPkg/UefiCpuPkg.dsc  |   3 +
 UefiPayloadPkg/UefiPayloadPkg.dsc  |   1 +
 13 files changed, 1086 insertions(+), 171 deletions(-)
 create mode 100644 UefiCpuPkg/Include/Library/SmmCpuSyncLib.h
 create mode 100644 UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c
 create mode 100644 UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf

-- 
2.16.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112108): https://edk2.groups.io/g/devel/message/112108
Mute This Topic: https://groups.io/mt/103010162/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 4/6] UefiCpuPkg: Add a new field in MpInfo2 HOB

[Ni, Ray]

Reviewed-by: Ray Ni 

Thanks,
Ray
> -Original Message-
> From: Tan, Dun 
> Sent: Tuesday, December 5, 2023 1:49 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric ; Ni, Ray ; Kumar,
> Rahul R ; Gerd Hoffmann 
> Subject: [PATCH 4/6] UefiCpuPkg: Add a new field in MpInfo2 HOB
> 
> Add new field CoreType in gMpInformationHobGuid2
> 
> Signed-off-by: Dun Tan 
> Cc: Eric Dong 
> Cc: Ray Ni 
> Cc: Rahul Kumar 
> Cc: Gerd Hoffmann 
> ---
>  UefiCpuPkg/Include/Guid/MpInformation2.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/UefiCpuPkg/Include/Guid/MpInformation2.h
> b/UefiCpuPkg/Include/Guid/MpInformation2.h
> index 4164ce1c30..cb654d6f05 100644
> --- a/UefiCpuPkg/Include/Guid/MpInformation2.h
> +++ b/UefiCpuPkg/Include/Guid/MpInformation2.h
> @@ -29,6 +29,8 @@
> 
>  typedef struct {
>EFI_PROCESSOR_INFORMATIONProcessorInfo;
> +  UINT8CoreType;
> +  UINT8Reserved[7];
>//
>// Add more fields in future
>//
> --
> 2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112107): https://edk2.groups.io/g/devel/message/112107
Mute This Topic: https://groups.io/mt/102987140/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 3/6] UefiCpuPkg: Consume MpInfo2Hob in PiSmmCpuDxe

[Ni, Ray]

1. The function name can be "GetMpInformation()" without mentioning 
"FromMpInfo2Hob".

> +  EFI_HOB_GUID_TYPE  *GuidHob;
> +  EFI_HOB_GUID_TYPE  *FirstMpInfor2Hob;

2. "FirstMpInfo2Hob". Please remove "r".

>+  FirstMpInfor2Hob = GetFirstGuidHob (&gMpInformationHobGuid2);

3. Please update comments to explain "FirstMpInfo2Hob" is to speed up the 2nd 
while-loop without needing to look for MpInfo2Hob from beginning.

> +
> +  ASSERT (CpuCount <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
> +  *NumberOfCpus = CpuCount;

4. There is no "return" before "*NumberOfCpus" assignment. So, why not remove 
"CpuCount" and directly udpates
"*NumberOfCpus" in the while-loop?

> +
> +  MpInfomation2Buffer = AllocatePool (sizeof
> (MP_INFORMATION2_HOB_DATA *) * HobCount);

5. MpInfomation2Buffer -> MpInfo2Hobs


6. Can you move "PrevProcessorIndex" assignment just above the "for" loop?

> +  for (Index = 0; Index < HobCount; Index++) {
7. Index -> HobIndex

> +  CopyMem (
> +ProcessorInfo + PrevProcessorIndex + ProcessorIndex,

8. &ProcessorInfo[PrevProcessorIndex + ProcessorIndex]

> +
> +  *ProcessorInfoPointer = ProcessorInfo;

9. If you let the function just return ProcessorInfo and NULL when failure 
happens, will it simplify the code?

> 
> +[Depex]
> +  TRUE
> -[Depex]
> -  gEfiMpServiceProtocolGuid

10. The depex change means that CpuSmm driver could run before CpuMp driver 
runs. Have you verified if CpuSmm can start well even removing CpuMp DXE driver?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112106): https://edk2.groups.io/g/devel/message/112106
Mute This Topic: https://groups.io/mt/102987139/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] MdePkg/IndustryStandard: Add _PSD/_CPC/Coord types definitions

[PierreGondois]

Hello Ray and the MdePkg maintainers,

Does this patch looks fine ? When/if this patch is accepted,
I will send the other patches relying  on this present patch,
cf. https://edk2.groups.io/g/devel/message/111900

Regards,
Pierre

On 12/1/23 13:26, Pierre Gondois wrote:

Hi Ray,
I followed the way revisions are defined for ACPI tables revisions,
like for the MADT:
EFI_ACPI_x_x_MULTIPLE_APIC_DESCRIPTION_TABLE_REVISION definition

For each ACPI spec. revision, there is a matching MADT revision number.
Sometimes the table has changed and the revision is upgraded, sometimes
not and the revision stays the same.
This also means having a macro definition for each ACPI spec. header
file. I think it should be ok do to the same thing for _PSD/_CPC
revisions,

Regards,
Pierre

On 12/1/23 11:22, Ni, Ray wrote:

--- a/MdePkg/Include/IndustryStandard/Acpi50.h
+++ b/MdePkg/Include/IndustryStandard/Acpi50.h



+#define EFI_ACPI_5_0_AML_PSD_REVISION  0
+#define EFI_ACPI_5_0_AML_CPC_REVISION  1



Do you think it's better to define EFI_ACPI_AML_PSD_REVISION and
EFI_ACPI_AML_CPC_REVISION in Acpi50.h?

So that Acpi51.h doesn't have to redefine a different macro to
the same value?



diff --git a/MdePkg/Include/IndustryStandard/Acpi51.h
b/MdePkg/Include/IndustryStandard/Acpi51.h
index 01ef544c3a29..19dd7b4f864c 100644
--- a/MdePkg/Include/IndustryStandard/Acpi51.h
+++ b/MdePkg/Include/IndustryStandard/Acpi51.h



+#define EFI_ACPI_5_1_AML_PSD_REVISION  0
+#define EFI_ACPI_5_1_AML_CPC_REVISION  1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112105): https://edk2.groups.io/g/devel/message/112105
Mute This Topic: https://groups.io/mt/102891569/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] MdePkg:simplify Fifo API in BaseIoLibIntrinsic

[duntan]

Hi Mike and Liming, 

Could you please help to review this patch?

Thanks,
Dun

-Original Message-
From: devel@edk2.groups.io  On Behalf Of duntan
Sent: Thursday, November 9, 2023 10:50 AM
To: devel@edk2.groups.io
Cc: Kinney, Michael D ; Gao, Liming 
; Liu, Zhiguang ; Ni, Ray 

Subject: [edk2-devel] [PATCH 2/2] MdePkg:simplify Fifo API in BaseIoLibIntrinsic

Simplify IoRead/WriteFifo implement by repeatedly calling IoRead/Write in the C 
code.
This can avoid calling assembly code to use string I/O instructions. With this 
change Ia32/IoFifo.nasm and X64/IoFifo.nasm can be removed. Then the source 
files for IA32 and X64 are the same.

Signed-off-by: Dun Tan 
Cc: Michael D Kinney 
Cc: Liming Gao 
Cc: Zhiguang Liu 
Cc: Ray Ni 
---
 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf |  10 ++
 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm   | 131 
---
 MdePkg/Library/BaseIoLibIntrinsic/IoLibFifo.c| 220 

 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm| 120 

 4 files changed, 222 insertions(+), 259 deletions(-)

diff --git a/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf 
b/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
index aeb072ee95..b587e2cded 100644
--- a/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
+++ b/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
@@ -38,17 +38,11 @@
   IoLibInternalTdxNull.c
   IoLibTdx.h
 
-[Sources.IA32]
+[Sources.IA32, Sources.X64]
   IoLibGcc.c| GCC
   IoLibMsc.c| MSFT
   IoLib.c
-  Ia32/IoFifo.nasm
-
-[Sources.X64]
-  IoLibGcc.c| GCC
-  IoLibMsc.c| MSFT
-  IoLib.c
-  X64/IoFifo.nasm
+  IoLibFifo.c
 
 [Sources.EBC]
   IoLibEbc.c
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm 
b/MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
deleted file mode 100644
index a4ae1a0053..00
--- a/MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
+++ /dev/null
@@ -1,131 +0,0 @@
-;--
-;
-; Copyright (c) 2006 - 2012, Intel Corporation. All rights reserved. -; 
Copyright (c) 2017, AMD Incorporated. All rights reserved. -; -; 
SPDX-License-Identifier: BSD-2-Clause-Patent -;
-;--
-
-SECTION .text
-
-;--
-;  VOID
-;  EFIAPI
-;  IoReadFifo8 (
-;IN  UINTN Port,
-;IN  UINTN Size,
-;OUT VOID  *Buffer
-;);
-;--
-global ASM_PFX(IoReadFifo8)
-ASM_PFX(IoReadFifo8):
-pushedi
-cld
-mov dx, [esp + 8]
-mov ecx, [esp + 12]
-mov edi, [esp + 16]
-rep insb
-pop edi
-ret
-
-;--
-;  VOID
-;  EFIAPI
-;  IoReadFifo16 (
-;IN  UINTN Port,
-;IN  UINTN Size,
-;OUT VOID  *Buffer
-;);
-;--
-global ASM_PFX(IoReadFifo16)
-ASM_PFX(IoReadFifo16):
-pushedi
-cld
-mov dx, [esp + 8]
-mov ecx, [esp + 12]
-mov edi, [esp + 16]
-rep insw
-pop edi
-ret
-
-;--
-;  VOID
-;  EFIAPI
-;  IoReadFifo32 (
-;IN  UINTN Port,
-;IN  UINTN Size,
-;OUT VOID  *Buffer
-;);
-;--
-global ASM_PFX(IoReadFifo32)
-ASM_PFX(IoReadFifo32):
-pushedi
-cld
-mov dx, [esp + 8]
-mov ecx, [esp + 12]
-mov edi, [esp + 16]
-rep insd
-pop edi
-ret
-
-;--
-;  VOID
-;  EFIAPI
-;  IoWriteFifo8 (
-;IN UINTN  Port,
-;IN UINTN  Size,
-;IN VOID   *Buffer
-;);
-;--
-global ASM_PFX(IoWriteFifo8)
-ASM_PFX(IoWriteFifo8):
-pushesi
-cld
-mov dx, [esp + 8]
-mov ecx, [esp + 12]
-mov esi, [esp + 16]
-rep outsb
-pop esi
-ret
-
-;--
-;  

Re: [edk2-devel] [PATCH 1/2] MdePkg: Change IoLibFifo.c to IoLibFifoCc.c

[duntan]

Hi Mike and Liming,

Could you please help to review this patch? 

Thanks.
Dun

-Original Message-
From: devel@edk2.groups.io  On Behalf Of duntan
Sent: Thursday, November 9, 2023 10:50 AM
To: devel@edk2.groups.io
Cc: Kinney, Michael D ; Gao, Liming 
; Liu, Zhiguang ; Ni, Ray 

Subject: [edk2-devel] [PATCH 1/2] MdePkg: Change IoLibFifo.c to IoLibFifoCc.c

Change IoLibFifo.c to IoLibFifoCc.c since the file is for Tdx and SEV in 
BaseIoLibIntrinsicSev.
It's also to distinguish with a new incoming IoLibFifo.c for BaseIoLibIntrinsic.

Signed-off-by: Dun Tan 
Cc: Michael D Kinney 
Cc: Liming Gao 
Cc: Zhiguang Liu 
Cc: Ray Ni 
---
 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf  | 2 +-
 MdePkg/Library/BaseIoLibIntrinsic/{IoLibFifo.c => IoLibFifoCc.c} | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf 
b/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf
index e1b8298ac4..8b62bd4e2a 100644
--- a/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf
+++ b/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf
@@ -45,7 +45,7 @@
   IoLibMsc.c| MSFT
   IoLib.c
   IoLibInternalTdx.c
-  IoLibFifo.c
+  IoLibFifoCc.c
   X64/IoFifoSev.nasm
 
 [Packages]
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibFifo.c 
b/MdePkg/Library/BaseIoLibIntrinsic/IoLibFifoCc.c
similarity index 94%
rename from MdePkg/Library/BaseIoLibIntrinsic/IoLibFifo.c
rename to MdePkg/Library/BaseIoLibIntrinsic/IoLibFifoCc.c
index 9a94bc6a05..0c85cf9426 100644
--- a/MdePkg/Library/BaseIoLibIntrinsic/IoLibFifo.c
+++ b/MdePkg/Library/BaseIoLibIntrinsic/IoLibFifoCc.c
@@ -1,7 +1,7 @@
 /** @file
   IoFifo read/write routines.
 
-  Copyright (c) 2021, Intel Corporation. All rights reserved.
+  Copyright (c) 2021 - 2023, Intel Corporation. All rights 
+ reserved.
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
--
2.31.1.windows.1








-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112103): https://edk2.groups.io/g/devel/message/112103
Mute This Topic: https://groups.io/mt/103009909/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/6] UefiCpuPkg: Build MpInfo2HOB in CpuMpPei

[Ni, Ray]

4 minor comments:

> +DEBUG ((DEBUG_INFO, "BuildMpInformationHob\n"));

1. DEBUG ("Creating MpInformation2 HOB...\n")

> +
> +for (Index = 0; Index < NumberOfProcessorsInHob; Index++) {
> +  MpInformation2Entry = GET_MP_INFORMATION_ENTRY
> (MpInformation2HobData, Index);

2. Since EntrySize equals to sizeof (MP_INFORMATION2_ENTRY), is it ok to just 
use MpInformation2HobData->Entry[Index]?

3. Do you think "Entry[0]" is more proper than "MpInformation[0]"?

> +  DEBUG ((
> +DEBUG_INFO,
> +"ProcessorIndex = %x, ProcessorId = %lx, StatusFlag = %x\n",
> +Index + ProcessorIndex,
> +MpInformation2Entry->ProcessorInfo.ProcessorId,
> +MpInformation2Entry->ProcessorInfo.StatusFlag
> +));

4. How about the debug messages are as follows:
Processor[]: ProcessorId = 0x, StatusFlag = 
0x0001\n
Location = Package:0 Core:0 Thread:0\n
Location2 = Package:0 Die:0 Tile:0 Module:0 Core:0 
Thread:0\n
If a number has "0x" prefix, it uses hex format, otherwise it uses dec format. 
The debug message should
clearly tell what format the number follows.
Extra 2 spaces in Location/Location2 are to tell that these are extra info for 
the processor #0.



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112102): https://edk2.groups.io/g/devel/message/112102
Mute This Topic: https://groups.io/mt/102987138/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg: Create gMpInformationHobGuid2 in UefiCpuPkg

[Ni, Ray]

> +  MP information protocol only provides static information of MP processor.
> +
> +  If SwitchBSP or Enable/DisableAP in MP service is called between the HOB
> +  production and HOB consumption,
> EFI_PROCESSOR_INFORMATION.StatusFlag and
> +  NumberOfEnabledProcessors fields in this HOB may be invalidated.

1. There is no NumberOfEnabledProcessors field.




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112101): https://edk2.groups.io/g/devel/message/112101
Mute This Topic: https://groups.io/mt/102987137/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [edk2-redfish-client][PATCH] RedfishClientPkg/RedfishFeatureCoreDxe: fix Redfish event issue.

[Nickle Wang via groups.io]

RedfishFeatureDriverStartup is callback function at TPL_CALLBACK
level. In this function, Redfish events are signaled. However,
Redfish events are created in TPL_CALLBACK level too. As the result,
Redfish events cannot be invoked in desired sequence. Decrease the
TPL to TPL_APPLICATION level inside RedfishFeatureDriverStartup and
restore it to TPL_CALLBACK level before leaving this function. Now,
Redfish events are called in correct sequence.

Signed-off-by: Nickle Wang 
Cc: Abner Chang 
Cc: Igor Kulchytskyy 
Cc: Nick Ramirez 
---
 .../RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.h  |  1 +
 .../RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.c  | 14 +-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.h 
b/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.h
index acefa41b..de08d79d 100644
--- a/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.h
+++ b/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.h
@@ -33,6 +33,7 @@
 #define NodeIsCollectionLeftBracket   L'{'
 #define NodeIsCollectionRightBracket  L'}'
 #define NodeIsCollectionSymbolL"/{}"
+#define REDFISH_FEATURE_CORE_TPL  TPL_CALLBACK
 
 typedef struct _REDFISH_FEATURE_INTERNAL_DATA REDFISH_FEATURE_INTERNAL_DATA;
 struct _REDFISH_FEATURE_INTERNAL_DATA {
diff --git a/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.c 
b/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.c
index f3188ddf..c0c3ec47 100644
--- a/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.c
+++ b/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.c
@@ -272,6 +272,13 @@ RedfishFeatureDriverStartup (
 return;
   }
 
+  //
+  // Lower the TPL to TPL_APPLICATION so that
+  // Redfish event and report status code can be
+  // triggered
+  //
+  gBS->RestoreTPL (TPL_APPLICATION);
+
   //
   // Reset PcdRedfishSystemRebootRequired flag
   //
@@ -321,6 +328,11 @@ RedfishFeatureDriverStartup (
 gRT->ResetSystem (EfiResetCold, EFI_SUCCESS, 0, NULL);
 CpuDeadLoop ();
   }
+
+  //
+  // Restore to the TPL where this callback handler is called.
+  //
+  gBS->RaiseTPL (REDFISH_FEATURE_CORE_TPL);
 }
 
 /**
@@ -670,7 +682,7 @@ RedfishFeatureCoreEntryPoint (
 
   Status = gBS->CreateEventEx (
   EVT_NOTIFY_SIGNAL,
-  TPL_CALLBACK,
+  REDFISH_FEATURE_CORE_TPL,
   RedfishFeatureDriverStartup,
   (CONST VOID *)&mFeatureDriverStartupContext,
   EventGuid,
-- 
2.17.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112100): https://edk2.groups.io/g/devel/message/112100
Mute This Topic: https://groups.io/mt/103009658/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v7 5/5] UefiCpuPkg: Backup and Restore MSR IA32_U_CET in SMI handler.

[Sheng Wei]

OS may enable CET-IBT feature by set MSR IA32_U_CET.bit2.
If IA32_U_CET.bit2 is set, CPU is in WAIT_FOR_ENDBRANCH state and
 the next assemble code is not ENDBR, it will trigger #CP exception
 when set CR4.CET bit.
SMI handler needs to backup MSR IA32_U_CET and clear MSR IA32_U_CET
 before set CR4.CET bit,
And SMI handler needs to restore MSR IA32_U_CET when exit SMI handler.

Signed-off-by: Sheng Wei 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Laszlo Ersek 
Cc: Wu Jiaxin 
Cc: Tan Dun 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm | 15 +++
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm  | 15 +++
 2 files changed, 30 insertions(+)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm 
b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
index 1da9afab97..9e1155dee6 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
@@ -202,11 +202,21 @@ ASM_PFX(mPatchCetSupported):
 pushedx
 pusheax
 
+mov ecx, MSR_IA32_U_CET
+rdmsr
+pushedx
+pusheax
+
 mov ecx, MSR_IA32_PL0_SSP
 rdmsr
 pushedx
 pusheax
 
+mov ecx, MSR_IA32_U_CET
+xor eax, eax
+xor edx, edx
+wrmsr
+
 mov ecx, MSR_IA32_S_CET
 mov eax, MSR_IA32_CET_SH_STK_EN
 xor edx, edx
@@ -276,6 +286,11 @@ CetDone:
 pop edx
 wrmsr
 
+mov ecx, MSR_IA32_U_CET
+pop eax
+pop edx
+wrmsr
+
 mov ecx, MSR_IA32_S_CET
 pop eax
 pop edx
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm 
b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
index abf9f1a90a..881d3177f7 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
@@ -217,6 +217,11 @@ ASM_PFX(mPatchCetSupported):
 pushrdx
 pushrax
 
+mov ecx, MSR_IA32_U_CET
+rdmsr
+pushrdx
+pushrax
+
 mov ecx, MSR_IA32_PL0_SSP
 rdmsr
 pushrdx
@@ -227,6 +232,11 @@ ASM_PFX(mPatchCetSupported):
 pushrdx
 pushrax
 
+mov ecx, MSR_IA32_U_CET
+xor eax, eax
+xor edx, edx
+wrmsr
+
 mov ecx, MSR_IA32_S_CET
 mov eax, MSR_IA32_CET_SH_STK_EN
 xor edx, edx
@@ -325,6 +335,11 @@ mCetSupportedAbsAddr:
 pop rdx
 wrmsr
 
+mov ecx, MSR_IA32_U_CET
+pop rax
+pop rdx
+wrmsr
+
 mov ecx, MSR_IA32_S_CET
 pop rax
 pop rdx
-- 
2.26.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112099): https://edk2.groups.io/g/devel/message/112099
Mute This Topic: https://groups.io/mt/103009381/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v7 4/5] UefiCpuPkg: Only change CR4.CET bit for enable and disable CET.

[Sheng Wei]

Signed-off-by: Sheng Wei 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Laszlo Ersek 
Cc: Wu Jiaxin 
Cc: Tan Dun 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm | 10 +++---
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm  | 10 +++---
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm 
b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
index 6368982433..1da9afab97 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
@@ -237,7 +237,9 @@ CetInterruptDone:
 bts ecx, 16 ; set WP
 mov cr0, ecx
 
-mov eax, 0x668 | CR4_CET
+; set CR4.CET bit for enable CET
+mov eax, cr4
+bts eax, CR4_CET_BIT
 mov cr4, eax
 
 setssbsy
@@ -264,8 +266,10 @@ CetDone:
 cmp al, 0
 jz  CetDone2
 
-mov eax, 0x668
-mov cr4, eax   ; disable CET
+; clear CR4.CET bit for disable CET
+mov eax, cr4
+btr eax, CR4_CET_BIT
+mov cr4, eax
 
 mov ecx, MSR_IA32_PL0_SSP
 pop eax
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm 
b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
index 9a225bc3be..abf9f1a90a 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
@@ -263,7 +263,9 @@ CetInterruptDone:
 bts ecx, 16 ; set WP
 mov cr0, rcx
 
-mov eax, 0x668 | CR4_CET
+; set CR4.CET bit for enable CET
+mov rax, cr4
+bts rax, CR4_CET_BIT
 mov cr4, rax
 
 setssbsy
@@ -308,8 +310,10 @@ mCetSupportedAbsAddr:
 cmp al, 0
 jz  CetDone2
 
-mov eax, 0x668
-mov cr4, rax   ; disable CET
+; clear CR4.CET bit for disable CET
+mov rax, cr4
+btr rax, CR4_CET_BIT
+mov cr4, rax
 
 mov ecx, MSR_IA32_INTERRUPT_SSP_TABLE_ADDR
 pop rax
-- 
2.26.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112098): https://edk2.groups.io/g/devel/message/112098
Mute This Topic: https://groups.io/mt/103009380/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v7 1/5] UefiCpuPkg: Add macro definitions for CET feature for NASM files.

[Sheng Wei]

Signed-off-by: Sheng Wei 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Laszlo Ersek 
Cc: Wu Jiaxin 
Cc: Tan Dun 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Cet.inc | 26 ++
 1 file changed, 26 insertions(+)
 create mode 100644 UefiCpuPkg/PiSmmCpuDxeSmm/Cet.inc

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Cet.inc 
b/UefiCpuPkg/PiSmmCpuDxeSmm/Cet.inc
new file mode 100644
index 00..41c99988c9
--- /dev/null
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Cet.inc
@@ -0,0 +1,26 @@
+;--
+;
+; Copyright (c) 2023, Intel Corporation. All rights reserved.
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+; Abstract:
+;
+;   This file provides macro definitions for CET feature for NASM files.
+;
+;--
+
+%define MSR_IA32_U_CET 0x6A0
+%define MSR_IA32_S_CET 0x6A2
+%define MSR_IA32_CET_SH_STK_EN (1<<0)
+%define MSR_IA32_CET_WR_SHSTK_EN   (1<<1)
+%define MSR_IA32_CET_ENDBR_EN  (1<<2)
+%define MSR_IA32_CET_LEG_IW_EN (1<<3)
+%define MSR_IA32_CET_NO_TRACK_EN   (1<<4)
+%define MSR_IA32_CET_SUPPRESS_DIS  (1<<5)
+%define MSR_IA32_CET_SUPPRESS  (1<<10)
+%define MSR_IA32_CET_TRACKER   (1<<11)
+%define MSR_IA32_PL0_SSP   0x6A4
+%define MSR_IA32_INTERRUPT_SSP_TABLE_ADDR  0x6A8
+
+%define CR4_CET_BIT23
+%define CR4_CET(1

[edk2-devel] [PATCH v7 3/5] UefiCpuPkg: Use CET macro definitions in Cet.inc for SmiEntry.nasm files.

[Sheng Wei]

Signed-off-by: Sheng Wei 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Laszlo Ersek 
Cc: Wu Jiaxin 
Cc: Tan Dun 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm | 14 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm  | 15 +--
 2 files changed, 2 insertions(+), 27 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm 
b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
index 19de5f614e..6368982433 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
@@ -15,19 +15,7 @@
 
 %include "StuffRsbNasm.inc"
 %include "Nasm.inc"
-
-%define MSR_IA32_S_CET 0x6A2
-%define   MSR_IA32_CET_SH_STK_EN 0x1
-%define   MSR_IA32_CET_WR_SHSTK_EN   0x2
-%define   MSR_IA32_CET_ENDBR_EN  0x4
-%define   MSR_IA32_CET_LEG_IW_EN 0x8
-%define   MSR_IA32_CET_NO_TRACK_EN   0x10
-%define   MSR_IA32_CET_SUPPRESS_DIS  0x20
-%define   MSR_IA32_CET_SUPPRESS  0x400
-%define   MSR_IA32_CET_TRACKER   0x800
-%define MSR_IA32_PL0_SSP   0x6A4
-
-%define CR4_CET0x80
+%include "Cet.inc"
 
 %define MSR_IA32_MISC_ENABLE 0x1A0
 %define MSR_EFER  0xc080
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm 
b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
index d302ca8d01..9a225bc3be 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
@@ -15,25 +15,12 @@
 
 %include "StuffRsbNasm.inc"
 %include "Nasm.inc"
+%include "Cet.inc"
 
 ;
 ; Variables referenced by C code
 ;
 
-%define MSR_IA32_S_CET 0x6A2
-%define   MSR_IA32_CET_SH_STK_EN 0x1
-%define   MSR_IA32_CET_WR_SHSTK_EN   0x2
-%define   MSR_IA32_CET_ENDBR_EN  0x4
-%define   MSR_IA32_CET_LEG_IW_EN 0x8
-%define   MSR_IA32_CET_NO_TRACK_EN   0x10
-%define   MSR_IA32_CET_SUPPRESS_DIS  0x20
-%define   MSR_IA32_CET_SUPPRESS  0x400
-%define   MSR_IA32_CET_TRACKER   0x800
-%define MSR_IA32_PL0_SSP   0x6A4
-%define MSR_IA32_INTERRUPT_SSP_TABLE_ADDR  0x6A8
-
-%define CR4_CET0x80
-
 %define MSR_IA32_MISC_ENABLE 0x1A0
 %define MSR_EFER  0xc080
 %define MSR_EFER_XD   0x800
-- 
2.26.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112097): https://edk2.groups.io/g/devel/message/112097
Mute This Topic: https://groups.io/mt/103009379/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v7 2/5] UefiCpuPkg: Use macro CR4_CET_BIT to replace hard code value in Cet.nasm.

[Sheng Wei]

Signed-off-by: Sheng Wei 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Laszlo Ersek 
Cc: Wu Jiaxin 
Cc: Tan Dun 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/Cet.nasm | 5 +++--
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/Cet.nasm  | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/Cet.nasm 
b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/Cet.nasm
index 9d66b9c5da..3d07da1cd4 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/Cet.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/Cet.nasm
@@ -5,6 +5,7 @@
 
;---
 
 %include "Nasm.inc"
+%include "Cet.inc"
 
 SECTION .text
 
@@ -16,7 +17,7 @@ ASM_PFX(DisableCet):
 incsspd eax
 
 mov eax, cr4
-btr eax, 23  ; clear CET
+btr eax, CR4_CET_BIT ; clear CET
 mov cr4, eax
 ret
 
@@ -24,7 +25,7 @@ global ASM_PFX(EnableCet)
 ASM_PFX(EnableCet):
 
 mov eax, cr4
-bts eax, 23  ; set CET
+bts eax, CR4_CET_BIT ; set CET
 mov cr4, eax
 
 ; use jmp to skip the check for ret
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/Cet.nasm 
b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/Cet.nasm
index 8bbdbb31cc..700aef4703 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/Cet.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/Cet.nasm
@@ -5,6 +5,7 @@
 
;---
 
 %include "Nasm.inc"
+%include "Cet.inc"
 
 DEFAULT REL
 SECTION .text
@@ -17,7 +18,7 @@ ASM_PFX(DisableCet):
 incsspq rax
 
 mov rax, cr4
-btr eax, 23  ; clear CET
+btr eax, CR4_CET_BIT ; clear CET
 mov cr4, rax
 ret
 
@@ -25,7 +26,7 @@ global ASM_PFX(EnableCet)
 ASM_PFX(EnableCet):
 
 mov rax, cr4
-bts eax, 23  ; set CET
+bts eax, CR4_CET_BIT ; set CET
 mov cr4, rax
 
 ; use jmp to skip the check for ret
-- 
2.26.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112096): https://edk2.groups.io/g/devel/message/112096
Mute This Topic: https://groups.io/mt/103009378/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v7 0/5] MdePkg: Add macro definitions for CET feature for NASM files.

[Sheng Wei]

Patch V7:
  Remove all the change in MdePkg.
  Move cet.inc to UefiCpuPkg\PiSmmCpuDxeSmm,
  beacuse CET feature is only used in SMM.

Patch V6:
  Cet.inc only contains definitions for x86 CPU.
  Move the file to \Ia32 and \X64 folder.
  Refine code for cet.inc.

Patch V5:
  File cet.inc will be used in both MdePkg UefiCpuPkg.
  Move cet.inc file from UefiCpuPkg to MdePkg.
  Use macro CR4_CET_BIT to replace hard code value for
   both LongJump.nasm and SetJump.nasm.

Patch V4:
  Separate the changes to 5 patches.
1) Add macro definitions for CET feature for NASM files.
2) Use macro CR4_CET_BIT to replace hard code value in Cet.nasm.
3) Use CET macro definitions in Cet.inc for SmiEntry.nasm files.
4) Only change CR4.CET bit for enable/disable CET.
5) Backup and Restore MSR IA32_U_CET in SMI handler.
  Remove some unused code.
It is no need to clear MSR IA32_S_CET,
 because clear CR4.CET bit will disable all CET functions.
Since CET is disabled between clear CR4.CET and run 'rsm',
 it is no need to delay MSR IA32_S_CET restoration.

Patch V3:
  Remove the 3rd patch. mSmmInterruptSspTables is a global variable.
  It is unnecessary to initializ it to zero manually.

Patch V2:
  No function change with Patch V1.
  Split the patch to into 3 separate patches.


Sheng Wei (5):
  UefiCpuPkg: Add macro definitions for CET feature for NASM files.
  UefiCpuPkg: Use macro CR4_CET_BIT to replace hard code value in
Cet.nasm.
  UefiCpuPkg: Use CET macro definitions in Cet.inc for SmiEntry.nasm
files.
  UefiCpuPkg: Only change CR4.CET bit for enable and disable CET.
  UefiCpuPkg: Backup and Restore MSR IA32_U_CET in SMI handler.

 UefiCpuPkg/PiSmmCpuDxeSmm/Cet.inc| 26 +
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/Cet.nasm  |  5 ++-
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm | 39 +++
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/Cet.nasm   |  5 ++-
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm  | 40 +++-
 5 files changed, 78 insertions(+), 37 deletions(-)
 create mode 100644 UefiCpuPkg/PiSmmCpuDxeSmm/Cet.inc

-- 
2.26.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112094): https://edk2.groups.io/g/devel/message/112094
Mute This Topic: https://groups.io/mt/103009376/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-