Re: [edk2-devel] [PATCH v2 5/5] UefiCpuPkg/MpInitLib: Add support for multiple HOBs to SaveCpuMpData()

[Laszlo Ersek]

On 2/22/24 02:49, Laszlo Ersek wrote:
> (commenting once more, to explain the actual reason:)
> 
> On 2/21/24 11:24, Gerd Hoffmann wrote:
>> On Wed, Feb 21, 2024 at 03:48:25AM +, Ni, Ray wrote:

 +  MaxCpusPerHob = (MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE) - sizeof
 (MP_HAND_OFF)) / sizeof (PROCESSOR_HAND_OFF);
>>>
>>> Above formula assumes the maximum HOB length could be 0x.
>>
>> Which is IMHO correct.
> 
> It is not correct:
> 
>>
>>> But actually the maximum HOB length could be only 0xFFF8 because
>>> PeiCore::PeiCreateHob() contains following logic:
>>>
>>>   if (0x1 - Length <= 0x7) {
>>> return EFI_INVALID_PARAMETER;
>>>   }
>>
>> That Length is the *data* size, the HOB header is not included.

sorry, I meant to insert here: no; "Length" at this location does
include everything, except the padding.

The BuildGuidHob() library function takes a data length, and calculates
the total HOB length, by adding 24 bytes, for the EFI_HOB_GUID_TYPE header.

InternalPeiCreateHob() (also in "MdePkg/Library/PeiHobLib/HobLib.c")
already takes the total HOB length, and passes it on to the CreateHob()
PEI service -- which Ray quotes above.

The total HOB length cannot exceed 0xFFF8 (due to the rounding up, for
Itanium's sake), so the GUID HOB data length cannot exceed
(0xFFF8-sizeof(EFI_HOB_GUID_TYPE)).

Laszlo

>>
>> The "- sizeof (EFI_HOB_GUID_TYPE)" in the formula above accounts the
>> space needed for HOB header and GUID.
> 
> Yes, but the problem is not that we need the extra space for
> EFI_HOB_GUID_TYPE (24 bytes) -- we need the extra space for padding /
> alignment.
> 
> This is what the PI spec says (v1.8, section "III-4.5.2 HOB Construction
> Rules"):
> 
>   HOB construction must obey the following rules:
> 
>   [...]
> 
>   4. All HOBs must be multiples of 8 bytes in length. This requirement
>  meets the alignment restrictions of the Itanium® processor family.
> 
>   [...]
> 
> And if your total *desired* HOB length exceeds 0xFFF8 (i.e., it falls in
> [0xFFF9..0x]), then the required upwards rounding produces 0x1_.
> But that rounded-up value cannot be expressed in
> "EFI_HOB_GENERIC_HEADER.HobLength" -- a UINT16 field.
> 
> In short, you've got Itanium to thank for this. :)
> 
> Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115772): https://edk2.groups.io/g/devel/message/115772
Mute This Topic: https://groups.io/mt/104472313/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 v2 2/4] RedfishClientPkg: refine RedfishExternalResourceResourceFeatureCallback

[Nickle Wang via groups.io]

Thanks for addressing my comment.


Reviewed-by: Nickle Wang 

Regards,
Nickle

> -Original Message-
> From: Mike Maslenkin 
> Sent: Thursday, February 22, 2024 4:06 AM
> To: devel@edk2.groups.io
> Cc: Mike Maslenkin ; Nickle Wang
> ; Abner Chang ; Igor Kulchytskyy
> 
> Subject: [edk2-redfish-client][PATCH v2 2/4] RedfishClientPkg: refine
> RedfishExternalResourceResourceFeatureCallback
> 
> External email: Use caution opening links or attachments
> 
> 
> Use local variable for BiosUri passed to HandleResource() to avoid problems in
> case of Private->Uri is overriden down the call stack.
> 
> Suggested-by: Nickle Wang 
> Cc: Abner Chang 
> Cc: Nickle Wang 
> Cc: Igor Kulchytskyy 
> Signed-off-by: Mike Maslenkin 
> ---
>  RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c | 11 ++-
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c
> b/RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c
> index f40f2d85af80..db77ed3dfccb 100644
> --- a/RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c
> +++ b/RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c
> @@ -670,6 +670,7 @@ RedfishExternalResourceResourceFeatureCallback (
>REDFISH_SERVICE  RedfishService;
> 
>REDFISH_RESOURCE_COMMON_PRIVATE  *Private;
> 
>EFI_STRING   ResourceUri;
> 
> +  EFI_STRING   BiosUri;
> 
> 
> 
>if (FeatureAction != CallbackActionStartOperation) {
> 
>  return EFI_UNSUPPORTED;
> 
> @@ -707,19 +708,19 @@ RedfishExternalResourceResourceFeatureCallback (
>//
> 
>// Initialize collection path
> 
>//
> 
> -  Private->Uri = RedfishGetUri (ResourceUri);
> 
> -  if (Private->Uri == NULL) {
> 
> +  BiosUri = RedfishGetUri (ResourceUri);
> 
> +  if (BiosUri == NULL) {
> 
>  ASSERT (FALSE);
> 
>  FreePool (ResourceUri);
> 
>  return EFI_OUT_OF_RESOURCES;
> 
>}
> 
> 
> 
> -  Status = HandleResource (Private, Private->Uri);
> 
> +  Status = HandleResource (Private, BiosUri);
> 
>if (EFI_ERROR (Status)) {
> 
> -DEBUG ((DEBUG_ERROR, "%a, process external resource: %a failed: %r\n",
> __func__, Private->Uri, Status));
> 
> +DEBUG ((DEBUG_ERROR, "%a, process external resource: %s failed:
> + %r\n", __func__, BiosUri, Status));
> 
>}
> 
> 
> 
> -  FreePool (Private->Uri);
> 
> +  FreePool (BiosUri);
> 
>FreePool (ResourceUri);
> 
>return Status;
> 
>  }
> 
> --
> 2.32.0 (Apple Git-132)



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115771): https://edk2.groups.io/g/devel/message/115771
Mute This Topic: https://groups.io/mt/104495956/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 5/5] UefiCpuPkg/MpInitLib: Add support for multiple HOBs to SaveCpuMpData()

[Laszlo Ersek]

On 2/20/24 18:49, Gerd Hoffmann wrote:
> Add support for splitting Hand-Off data into multiple HOBs.  This is
> required for VMs with thousands of CPUs.  The actual CPU count per HOB
> is much smaller (128) for better test coverage.

(1) The mention of the count 128 now seems stale for the code.

> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 44 +++--
>  1 file changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c 
> b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> index f80e00edcff3..8a916a218016 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> @@ -126,35 +126,45 @@ SaveCpuMpData (
>IN CPU_MP_DATA  *CpuMpData
>)
>  {
> +  UINT32   MaxCpusPerHob, CpusInHob;
>UINT64   Data64;
> -  UINTNIndex;
> +  UINT32   Index, HobBase;
>CPU_INFO_IN_HOB  *CpuInfoInHob;
>MP_HAND_OFF  *MpHandOff;
>UINTNMpHandOffSize;
>  
> +  MaxCpusPerHob = (MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE) - sizeof 
> (MP_HAND_OFF)) / sizeof (PROCESSOR_HAND_OFF);

(2) MAX_UINT16 should be 0xFFF8 instead; see subthread.

> +
>//
>// When APs are in a state that can be waken up by a store operation to a 
> memory address,
>// report the MP_HAND_OFF data for DXE to use.
>//
> -  CpuInfoInHob  = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob;
> -  MpHandOffSize = sizeof (MP_HAND_OFF) + sizeof (PROCESSOR_HAND_OFF) * 
> CpuMpData->CpuCount;
> -  MpHandOff = (MP_HAND_OFF *)BuildGuidHob (, 
> MpHandOffSize);
> -  ASSERT (MpHandOff != NULL);
> -  ZeroMem (MpHandOff, MpHandOffSize);
> -  MpHandOff->ProcessorIndex = 0;
> +  CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob;
>  
> -  MpHandOff->CpuCount = CpuMpData->CpuCount;
> -  if (CpuMpData->ApLoopMode != ApInHltLoop) {
> -MpHandOff->StartupSignalValue= MP_HAND_OFF_SIGNAL;
> -MpHandOff->WaitLoopExecutionMode = sizeof (VOID *);
> -  }
> +  for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
> +if (Index % MaxCpusPerHob == 0) {
> +  HobBase   = Index;
> +  CpusInHob = MIN (CpuMpData->CpuCount - HobBase, MaxCpusPerHob);
>  
> -  for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
> -MpHandOff->Info[Index].ApicId = CpuInfoInHob[Index].ApicId;
> -MpHandOff->Info[Index].Health = CpuInfoInHob[Index].Health;
> +  MpHandOffSize = sizeof (MP_HAND_OFF) + sizeof (PROCESSOR_HAND_OFF) * 
> CpusInHob;
> +  MpHandOff = (MP_HAND_OFF *)BuildGuidHob (, 
> MpHandOffSize);
> +  ASSERT (MpHandOff != NULL);
> +  ZeroMem (MpHandOff, MpHandOffSize);
> +
> +  MpHandOff->ProcessorIndex = HobBase;
> +  MpHandOff->CpuCount   = CpusInHob;
> +
> +  if (CpuMpData->ApLoopMode != ApInHltLoop) {
> +MpHandOff->StartupSignalValue= MP_HAND_OFF_SIGNAL;
> +MpHandOff->WaitLoopExecutionMode = sizeof (VOID *);
> +  }
> +}
> +
> +MpHandOff->Info[Index-HobBase].ApicId = CpuInfoInHob[Index].ApicId;
> +MpHandOff->Info[Index-HobBase].Health = CpuInfoInHob[Index].Health;
>  if (CpuMpData->ApLoopMode != ApInHltLoop) {
> -  MpHandOff->Info[Index].StartupSignalAddress= 
> (UINT64)(UINTN)CpuMpData->CpuData[Index].StartupApSignal;
> -  MpHandOff->Info[Index].StartupProcedureAddress = 
> (UINT64)(UINTN)>CpuData[Index].ApFunction;
> +  MpHandOff->Info[Index-HobBase].StartupSignalAddress= 
> (UINT64)(UINTN)CpuMpData->CpuData[Index].StartupApSignal;
> +  MpHandOff->Info[Index-HobBase].StartupProcedureAddress = 
> (UINT64)(UINTN)>CpuData[Index].ApFunction;
>  }
>}
>  

(3) The conversion looks good otherwise, but I dislike that
StartupSignalValue and WaitLoopExecutionMode get uselessly replicated
over all HOBs. Again, it *increases* technical debt. It's fine to ignore
existent debt (if you can), but adding to it is not right.

Can you file a new TianoCore BZ, for moving these fields to separate
dynamic PCDs, or to a new singleton GUID HOB (containing both fields)?
If you assign that BZ to yourself, and reference it in patches #4 and
#5, I'll be happy to R-b version 3 of the series. (Which is something
I'd like to do.)

Version 3 may be mergeable regardless, of course, if Ray accepts it.

Thanks,
Laszlo



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

Re: [edk2-devel] [PATCH] NetworkPkg:Resolved Consecutive Pxe-Http Boot Issue

[Sivaraman Nainar via groups.io]

Laszlo:

Thanks for the detailed feedback on the changes for this issue. Since we are 
not sure if this change are valid / violate some purpose of SNP driver, it 
mentioned as Workaround.

@Saloni Kasbekar and @Clark-williams, Zachary can add more on these changes.

As you recommended, we can have PCD which controls these changes till the 
changes are addressed in grub.

@Santhosh Kumar V is this issue can be seen only in SLES 15 or it can be found 
in any OS having Grub 2.x?

Thanks
Siva
-Original Message-
From: Laszlo Ersek 
Sent: Thursday, February 22, 2024 5:15 AM
To: devel@edk2.groups.io; Santhosh Kumar V 
Cc: Sivaraman Nainar ; Raj V Akilan ; 
Soundharia R ; Saloni Kasbekar 
; Zachary Clark-williams 

Subject: [EXTERNAL] Re: [edk2-devel] [PATCH] NetworkPkg:Resolved Consecutive 
Pxe-Http Boot Issue


**CAUTION: The e-mail below is from an external source. Please exercise caution 
before opening attachments, clicking links, or following guidance.**

On 2/21/24 18:15, Santhosh Kumar V via groups.io wrote:
> The customer has a server environment where PXE and HTTP service run in same 
> Linux Server. In this environment a SUT trying to boot to SLES 15 OS via PXE 
> from the Boot Menu. After PXE Boot file downloaded and grub Loaded without 
> continuing for installation Exit is pressed and control back to Setup.
> Now the HTTP boot to SLES 15 OS tried in the same environment and failed to 
> download the file. If there is a reconnect -r performed before this HTTP Boot 
> then boot file download and installation is getting success.
> Root cause of the issue is, when Exit from grub performed, boot Loader Stops 
> the SNP Driver and starts the same.

This sentence feels like the key one.

Are you saying that grub calls Snp->Start() just before it exits?

If so, am I right to suspect that that's a grub bug? It sounds like a resource 
leak, after all.

Can you perhaps include a grub source code location / pointer in the commit 
message?

> During this process SNP is in Initialized State. When HTTP boot is performed 
> immediately after PXE Failure, the MNP configure method initiates the SNP 
> Start again. Since the SNP already started by grub it returns 
> EFI_ALREADY_STARTED and none of the upper Layer drivers are getting started.
> As a work around in MNPConfigure(), if the SNP Start failed with Already 
> Started and in Initialized state we can return success so that rest of the 
> drivers can be loaded and HTTP boot can work.
>
>
> Cc: Saloni Kasbekar 
> Cc: Zachary Clark-williams 
>
> Signed-off-by: SanthoshKumar 
> ---
>  NetworkPkg/MnpDxe/MnpConfig.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/NetworkPkg/MnpDxe/MnpConfig.c
> b/NetworkPkg/MnpDxe/MnpConfig.c index 93587d53aa..0f2df28d73 100644
> --- a/NetworkPkg/MnpDxe/MnpConfig.c
> +++ b/NetworkPkg/MnpDxe/MnpConfig.c
> @@ -1120,7 +1120,9 @@ MnpStartSnp (
>// Start the simple network.
>
>//
>
>Status = Snp->Start (Snp);
>
> -
>
> +  if ((Status == EFI_ALREADY_STARTED ) && (Snp->Mode->State ==
> + EfiSimpleNetworkInitialized)) {
>
> +  return EFI_SUCCESS;
>
> +  }
>
>if (!EFI_ERROR (Status)) {
>
>  //
>
>  // Initialize the simple network.
>

The commit message does say this is a workaround, and I don't immediately any 
see why this workaround (in the code) would be problematic in practice, but it 
still leaves a bad taste in my mouth.

Consider: the call path is the following:

MnpConfigure()   [NetworkPkg/MnpDxe/MnpConfig.c] -- public .Configure() 
protocol member function
  MnpConfigureInstance() [NetworkPkg/MnpDxe/MnpConfig.c]
MnpStart()   [NetworkPkg/MnpDxe/MnpConfig.c]
  // see notes!
  MnpStartSnp()  [NetworkPkg/MnpDxe/MnpConfig.c]

Notes: the MnpStartSnp() call in MnpStart() is conditional on two circumstances 
(at the same time):
- "If it's not a configuration update, increase the configured children number."
- "It's the first configured child, start the simple network."

In other words, the MNP driver has just bound SNP "BY_DRIVER" (i.e., 
exclusively), installed the MNP service binding protocol for each vlan (IIUC), 
and one of those SB instances is now being used to create the first MNP 
instance. I think that under these circumstances, it is reasonable for the MNP 
driver to expect that the underlying SNP be in stopped state. :/

How long would NetworkPkg have to carry this workaround? (I.e., how long before 
the grub issue is fixed, and the buggy version deprecated?)

I'd prefer at least a comment in the code that the return path is a workaround 
for (I feel) an earlier SNP usage violation.

A FeaturePCD to disable the workaround could be reasonable too (but the 
NetworkPkg maintainers could disagree about that).


BTW, the commit message should be wrapped at 75 characters. These long lines 
(in the body) will pass PatchCheck, but generate warnings. Those warnings are 
tolerable for log quotes, URLs, etc, but for normal English text, wrapping 

Re: [edk2-devel] [PATCH v2 5/5] UefiCpuPkg/MpInitLib: Add support for multiple HOBs to SaveCpuMpData()

[Laszlo Ersek]

(commenting once more, to explain the actual reason:)

On 2/21/24 11:24, Gerd Hoffmann wrote:
> On Wed, Feb 21, 2024 at 03:48:25AM +, Ni, Ray wrote:
>>>
>>> +  MaxCpusPerHob = (MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE) - sizeof
>>> (MP_HAND_OFF)) / sizeof (PROCESSOR_HAND_OFF);
>>
>> Above formula assumes the maximum HOB length could be 0x.
> 
> Which is IMHO correct.

It is not correct:

> 
>> But actually the maximum HOB length could be only 0xFFF8 because
>> PeiCore::PeiCreateHob() contains following logic:
>>
>>   if (0x1 - Length <= 0x7) {
>> return EFI_INVALID_PARAMETER;
>>   }
> 
> That Length is the *data* size, the HOB header is not included.
> 
> The "- sizeof (EFI_HOB_GUID_TYPE)" in the formula above accounts the
> space needed for HOB header and GUID.

Yes, but the problem is not that we need the extra space for
EFI_HOB_GUID_TYPE (24 bytes) -- we need the extra space for padding /
alignment.

This is what the PI spec says (v1.8, section "III-4.5.2 HOB Construction
Rules"):

  HOB construction must obey the following rules:

  [...]

  4. All HOBs must be multiples of 8 bytes in length. This requirement
 meets the alignment restrictions of the Itanium® processor family.

  [...]

And if your total *desired* HOB length exceeds 0xFFF8 (i.e., it falls in
[0xFFF9..0x]), then the required upwards rounding produces 0x1_.
But that rounded-up value cannot be expressed in
"EFI_HOB_GENERIC_HEADER.HobLength" -- a UINT16 field.

In short, you've got Itanium to thank for this. :)

Laszlo



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

Re: [edk2-devel] [edk2-platforms 1/1] Platform/Loongson: loongarch supports parsing multi-chip flash

[Chao Li]

Reviewed-by: Chao Li 


Thanks,
Chao
On 2024/2/19 10:03, xianglai wrote:

The current implementation of loongarch NorFlashQemuLib is to
parse fdt and think that the first flash address resolved to
the NVRAM space, with the evolution of qemu code, loongarch
uses the first flash to store UEFI code and the second flash
as NVRAM space, so NorFlashQemuLib needs to be able to parse
multiple flash base addresses. By default, the first piece of
flash other than UEFI code is considered as NVRAM space.

Cc: Andrea Bolognani
Cc: Bibo Mao
Cc: Chao Li
Signed-off-by: Xianglai Li
---
  .../Library/NorFlashQemuLib/NorFlashQemuLib.c | 75 ---
  .../NorFlashQemuLib/NorFlashQemuLib.inf   |  2 +
  .../Loongson/LoongArchQemuPkg/Loongson.fdf|  4 +-
  3 files changed, 51 insertions(+), 30 deletions(-)

diff --git 
a/Platform/Loongson/LoongArchQemuPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c 
b/Platform/Loongson/LoongArchQemuPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
index 2e0bf3cef0..1781c1c321 100644
--- 
a/Platform/Loongson/LoongArchQemuPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
+++ 
b/Platform/Loongson/LoongArchQemuPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
@@ -84,34 +84,53 @@ VirtNorFlashPlatformGetDevices (
  return EFI_NOT_FOUND;
}
  
-  Base = SwapBytes64 (ReadUnaligned64 ((VOID *)[0]));

-  Size = SwapBytes64 (ReadUnaligned64 ((VOID *)[2]));
-
-  mNorFlashDevices.DeviceBaseAddress = (UINTN)Base;
-  mNorFlashDevices.RegionBaseAddress = (UINTN)Base;
-  mNorFlashDevices.Size  = (UINTN)Size;
-  mNorFlashDevices.BlockSize = QEMU_NOR_BLOCK_SIZE;
-
-  Status = PcdSet32S (PcdFlashNvStorageVariableBase, Base);
-  ASSERT_EFI_ERROR (Status);
-
-  /*
-   * Base is the value of PcdFlashNvStorageVariableBase,
-   * PcdFlashNvStorageFtwWorkingBase can be got by
-   *   PcdFlashNvStorageVariableBase + PcdFlashNvStorageVariableSize
-   */
-  Base += PcdGet32 (PcdFlashNvStorageVariableSize);
-  Status = PcdSet32S (PcdFlashNvStorageFtwWorkingBase, Base);
-  ASSERT_EFI_ERROR (Status);
-
-  /*
-   * Now,Base is the value of PcdFlashNvStorageFtwWorkingBase,
-   * PcdFlashNvStorageFtwSpareBase can be got by
-   *   PcdFlashNvStorageFtwWorkingBase + PcdFlashNvStorageFtwWorkingSize.
-   */
-  Base += PcdGet32 (PcdFlashNvStorageFtwWorkingSize);
-  Status = PcdSet32S (PcdFlashNvStorageFtwSpareBase, Base);
-  ASSERT_EFI_ERROR (Status);
+  while (PropSize >= (4 * sizeof (UINT32))) {
+Base = SwapBytes64 (ReadUnaligned64 ((VOID *)[0]));
+Size = SwapBytes64 (ReadUnaligned64 ((VOID *)[2]));
+Reg += 4;
+
+PropSize -= 4 * sizeof (UINT32);
+
+//
+// Disregard any flash devices that overlap with the primary FV.
+// The firmware is not updatable from inside the guest anyway.
+//
+if ((PcdGet32 (PcdOvmfFdBaseAddress) + PcdGet32 (PcdOvmfFirmwareFdSize) > Base) 
&&
+((Base + Size) > PcdGet32 (PcdOvmfFdBaseAddress)))
+{
+  continue;
+}
+
+//
+//By default, the second available flash is stored as a non-volatile 
variable.
+//
+mNorFlashDevices.DeviceBaseAddress = (UINTN)Base;
+mNorFlashDevices.RegionBaseAddress = (UINTN)Base;
+mNorFlashDevices.Size  = (UINTN)Size;
+mNorFlashDevices.BlockSize = QEMU_NOR_BLOCK_SIZE;
+
+Status = PcdSet32S (PcdFlashNvStorageVariableBase, Base);
+ASSERT_EFI_ERROR (Status);
+
+/*
+ * Base is the value of PcdFlashNvStorageVariableBase,
+ * PcdFlashNvStorageFtwWorkingBase can be got by
+ *   PcdFlashNvStorageVariableBase + PcdFlashNvStorageVariableSize
+ */
+Base += PcdGet32 (PcdFlashNvStorageVariableSize);
+Status = PcdSet32S (PcdFlashNvStorageFtwWorkingBase, Base);
+ASSERT_EFI_ERROR (Status);
+
+/*
+ * Now,Base is the value of PcdFlashNvStorageFtwWorkingBase,
+ * PcdFlashNvStorageFtwSpareBase can be got by
+ *   PcdFlashNvStorageFtwWorkingBase + PcdFlashNvStorageFtwWorkingSize.
+ */
+Base += PcdGet32 (PcdFlashNvStorageFtwWorkingSize);
+Status = PcdSet32S (PcdFlashNvStorageFtwSpareBase, Base);
+ASSERT_EFI_ERROR (Status);
+break;
+  }
  
//

// UEFI takes ownership of the NOR flash, and exposes its functionality
diff --git 
a/Platform/Loongson/LoongArchQemuPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
 
b/Platform/Loongson/LoongArchQemuPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
index da05ca0898..a9b6c38783 100644
--- 
a/Platform/Loongson/LoongArchQemuPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
+++ 
b/Platform/Loongson/LoongArchQemuPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
@@ -35,6 +35,8 @@
gFdtClientProtocolGuid
  
  [Pcd]

+gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress
+gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize
  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
diff --git a/Platform/Loongson/LoongArchQemuPkg/Loongson.fdf 

Re: [edk2-devel] [PATCH v2 5/5] UefiCpuPkg/MpInitLib: Add support for multiple HOBs to SaveCpuMpData()

[Laszlo Ersek]

On 2/21/24 11:24, Gerd Hoffmann wrote:
> On Wed, Feb 21, 2024 at 03:48:25AM +, Ni, Ray wrote:
>>>
>>> +  MaxCpusPerHob = (MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE) - sizeof
>>> (MP_HAND_OFF)) / sizeof (PROCESSOR_HAND_OFF);
>>
>> Above formula assumes the maximum HOB length could be 0x.
> 
> Which is IMHO correct.
> 
>> But actually the maximum HOB length could be only 0xFFF8 because
>> PeiCore::PeiCreateHob() contains following logic:
>>
>>   if (0x1 - Length <= 0x7) {
>> return EFI_INVALID_PARAMETER;
>>   }
> 
> That Length is the *data* size, the HOB header is not included.
> 
> The "- sizeof (EFI_HOB_GUID_TYPE)" in the formula above accounts the
> space needed for HOB header and GUID.


>From the patch:

  MaxCpusPerHob = (MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE) - sizeof 
(MP_HAND_OFF)) / sizeof (PROCESSOR_HAND_OFF);
  ...
  CpusInHob = MIN (CpuMpData->CpuCount - HobBase, MaxCpusPerHob);
  MpHandOffSize = sizeof (MP_HAND_OFF) + sizeof (PROCESSOR_HAND_OFF) * 
CpusInHob;
  MpHandOff = (MP_HAND_OFF *)BuildGuidHob (, MpHandOffSize);

Assuming that the division is exact, and that the MIN selects MaxCpusPerHob for 
CpusInHob, we get:

  MpHandOffSize = sizeof (MP_HAND_OFF) + sizeof (PROCESSOR_HAND_OFF) * 
(MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE) - sizeof (MP_HAND_OFF)) / sizeof 
(PROCESSOR_HAND_OFF);

the multiplication and the division cancel out (again, assuming exact division):

  MpHandOffSize = sizeof (MP_HAND_OFF) + (MAX_UINT16 - sizeof 
(EFI_HOB_GUID_TYPE) - sizeof (MP_HAND_OFF));

the sizeof (MP_HAND_OFF) addition and subtraction cancel out:

  MpHandOffSize = MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE);

then we get:

  MpHandOff = (MP_HAND_OFF *)BuildGuidHob (, MAX_UINT16 - 
sizeof (EFI_HOB_GUID_TYPE));

Inside BuildGuidHob() [MdePkg/Library/PeiHobLib/HobLib.c], we get

  DataLength = MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE);

and further

  ASSERT (DataLength <= (0xFFF8 - sizeof (EFI_HOB_GUID_TYPE)));

Substituting for DataLength,

  ASSERT (MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE) <= (0xFFF8 - sizeof 
(EFI_HOB_GUID_TYPE)));

which is

  ASSERT (MAX_UINT16  <= 0xFFF8);

which fails.

So, as Ray says, and as I wrote under v1, you need to use 0xFFF8 here, not 
MAX_UINT16.

... Under v1, I wrote

  CpusPerHob = (0xFFE0 - sizeof (MP_HAND_OFF)) / sizeof (PROCESSOR_HAND_OFF);

If you want to make "sizeof (EFI_HOB_GUID_TYPE)" -- 24 bytes -- explicit in 
that subtraction, that is fine, but then we just get back to 0xFFF8:

  CpusPerHob = (0xFFF8 - sizeof (EFI_HOB_GUID_TYPE) - sizeof (MP_HAND_OFF)) / 
sizeof (PROCESSOR_HAND_OFF);

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115766): https://edk2.groups.io/g/devel/message/115766
Mute This Topic: https://groups.io/mt/104472313/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 4/5] UefiCpuPkg/MpInitLib: Add support for multiple HOBs to MpInitLibInitialize

[Laszlo Ersek]

On 2/20/24 18:49, Gerd Hoffmann wrote:
> Loop over all MP_HAND_OFF HOBs instead of expecting a single HOB
> covering all CPUs in the system.
> 
> Add a new HaveMpHandOff variable to track whenever MP_HAND_OFF HOBs are
> present, using the MpHandOff pointer for that does not work because the
> variable will be NULL after looping over all HOBs.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 68 +++-
>  1 file changed, 47 insertions(+), 21 deletions(-)

as Ray says, the commit message is a bit stale

> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c 
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index c13de34e5769..80585f676b1d 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -2025,6 +2025,7 @@ MpInitLibInitialize (
>VOID
>)
>  {
> +  MP_HAND_OFF  *FirstMpHandOff;
>MP_HAND_OFF  *MpHandOff;
>CPU_INFO_IN_HOB  *CpuInfoInHob;
>UINT32   MaxLogicalProcessorNumber;
> @@ -2038,17 +2039,31 @@ MpInitLibInitialize (
>CPU_MP_DATA  *CpuMpData;
>UINT8ApLoopMode;
>UINT8*MonitorBuffer;
> -  UINTNIndex;
> +  UINT32   Index, HobIndex;
>UINTNApResetVectorSizeBelow1Mb;
>UINTNApResetVectorSizeAbove1Mb;
>UINTNBackupBufferAddr;
>UINTNApIdtBase;
>  
> -  MpHandOff = GetNextMpHandOffHob (NULL);
> -  if (MpHandOff == NULL) {
> +  FirstMpHandOff = GetNextMpHandOffHob (NULL);
> +  if (FirstMpHandOff != NULL) {
> +MaxLogicalProcessorNumber = 0;
> +for (MpHandOff = FirstMpHandOff;
> + MpHandOff != NULL;
> + MpHandOff = GetNextMpHandOffHob (MpHandOff))
> +{
> +  DEBUG ((
> +DEBUG_INFO,
> +"%a: ProcessorIndex=%u CpuCount=%u\n",
> +__func__,
> +MpHandOff->ProcessorIndex,
> +MpHandOff->CpuCount
> +));
> +  ASSERT (MaxLogicalProcessorNumber == MpHandOff->ProcessorIndex);
> +  MaxLogicalProcessorNumber += MpHandOff->CpuCount;
> +}
> +  } else {
>  MaxLogicalProcessorNumber = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
> -  } else {
> -MaxLogicalProcessorNumber = MpHandOff->CpuCount;
>}
>  
>ASSERT (MaxLogicalProcessorNumber != 0);
> @@ -2192,7 +2207,7 @@ MpInitLibInitialize (
>//
>ProgramVirtualWireMode ();
>  
> -  if (MpHandOff == NULL) {
> +  if (FirstMpHandOff == NULL) {
>  if (MaxLogicalProcessorNumber > 1) {
>//
>// Wakeup all APs and calculate the processor count in system
> @@ -2208,21 +2223,32 @@ MpInitLibInitialize (
>AmdSevUpdateCpuMpData (CpuMpData);
>  }
>  
> -CpuMpData->CpuCount  = MpHandOff->CpuCount;
> -CpuMpData->BspNumber = GetBspNumber (MpHandOff);
> +CpuMpData->CpuCount  = MaxLogicalProcessorNumber;
> +CpuMpData->BspNumber = GetBspNumber (FirstMpHandOff);
>  CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob;
> -for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
> -  InitializeSpinLock (>CpuData[Index].ApLock);
> -  CpuMpData->CpuData[Index].CpuHealthy = (MpHandOff->Info[Index].Health 
> == 0) ? TRUE : FALSE;
> -  CpuMpData->CpuData[Index].ApFunction = 0;
> -  CpuInfoInHob[Index].InitialApicId= MpHandOff->Info[Index].ApicId;
> -  CpuInfoInHob[Index].ApTopOfStack = CpuMpData->Buffer + (Index + 1) 
> * CpuMpData->CpuApStackSize;
> -  CpuInfoInHob[Index].ApicId   = MpHandOff->Info[Index].ApicId;
> -  CpuInfoInHob[Index].Health   = MpHandOff->Info[Index].Health;
> +for (MpHandOff = FirstMpHandOff;
> + MpHandOff != NULL;
> + MpHandOff = GetNextMpHandOffHob (MpHandOff))
> +{
> +  for (HobIndex = 0; HobIndex < MpHandOff->CpuCount; HobIndex++) {
> +Index = MpHandOff->ProcessorIndex + HobIndex;
> +InitializeSpinLock (>CpuData[Index].ApLock);
> +CpuMpData->CpuData[Index].CpuHealthy = 
> (MpHandOff->Info[HobIndex].Health == 0) ? TRUE : FALSE;
> +CpuMpData->CpuData[Index].ApFunction = 0;
> +CpuInfoInHob[Index].InitialApicId= 
> MpHandOff->Info[HobIndex].ApicId;
> +CpuInfoInHob[Index].ApTopOfStack = CpuMpData->Buffer + (Index + 
> 1) * CpuMpData->CpuApStackSize;
> +CpuInfoInHob[Index].ApicId   = 
> MpHandOff->Info[HobIndex].ApicId;
> +CpuInfoInHob[Index].Health   = 
> MpHandOff->Info[HobIndex].Health;
> +  }
>  }
>  
> -DEBUG ((DEBUG_INFO, "MpHandOff->WaitLoopExecutionMode: %04d, sizeof 
> (VOID *): %04d\n", MpHandOff->WaitLoopExecutionMode, sizeof (VOID *)));
> -if (MpHandOff->WaitLoopExecutionMode == sizeof (VOID *)) {
> +DEBUG ((
> +  DEBUG_INFO,
> +  "FirstMpHandOff->WaitLoopExecutionMode: %04d, sizeof (VOID *): %04d\n",
> +  FirstMpHandOff->WaitLoopExecutionMode,
> +   

Re: [edk2-devel] [PATCH v2 3/5] UefiCpuPkg/MpInitLib: Add support for multiple HOBs to SwitchApContext()

[Laszlo Ersek]

On 2/20/24 18:49, Gerd Hoffmann wrote:
> Rename the MpHandOff parameter to FirstMpHandOff.  Add loops so the
> function inspects all HOBs present in the system.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.h |  2 +-
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 35 ++--
>  2 files changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h 
> b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index bc2a0232291d..b5214b904b41 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -482,7 +482,7 @@ GetWakeupBuffer (
>  **/
>  VOID
>  SwitchApContext (
> -  IN MP_HAND_OFF  *MpHandOff
> +  IN CONST MP_HAND_OFF  *FirstMpHandOff
>);
>  
>  /**
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c 
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 8f198ff6d817..c13de34e5769 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1938,31 +1938,42 @@ GetBspNumber (
>This procedure allows the AP to switch to another section of
>memory and continue its loop there.
>  
> -  @param[in] MpHandOff  Pointer to MP hand-off data structure.
> +  @param[in] FirstMpHandOff  Pointer to first MP hand-off HOB.

comment should say sth like "Pointer to first MP hand-off HOB body".

Reviewed-by: Laszlo Ersek 


>  **/
>  VOID
>  SwitchApContext (
> -  IN MP_HAND_OFF  *MpHandOff
> +  IN CONST MP_HAND_OFF  *FirstMpHandOff
>)
>  {
> -  UINTN   Index;
> -  UINT32  BspNumber;
> +  UINTN  Index;
> +  UINT32 BspNumber;
> +  CONST MP_HAND_OFF  *MpHandOff;
>  
> -  BspNumber = GetBspNumber (MpHandOff);
> +  BspNumber = GetBspNumber (FirstMpHandOff);
>  
> -  for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
> -if (Index != BspNumber) {
> -  *(UINTN *)(UINTN)MpHandOff->Info[Index].StartupProcedureAddress = 
> (UINTN)SwitchContextPerAp;
> -  *(UINT32 *)(UINTN)MpHandOff->Info[Index].StartupSignalAddress   = 
> MpHandOff->StartupSignalValue;
> +  for (MpHandOff = FirstMpHandOff;
> +   MpHandOff != NULL;
> +   MpHandOff = GetNextMpHandOffHob (MpHandOff))
> +  {
> +for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
> +  if (MpHandOff->ProcessorIndex + Index != BspNumber) {
> +*(UINTN *)(UINTN)MpHandOff->Info[Index].StartupProcedureAddress = 
> (UINTN)SwitchContextPerAp;
> +*(UINT32 *)(UINTN)MpHandOff->Info[Index].StartupSignalAddress   = 
> MpHandOff->StartupSignalValue;
> +  }
>  }
>}
>  
>//
>// Wait all APs waken up if this is not the 1st broadcast of SIPI
>//
> -  for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
> -if (Index != BspNumber) {
> -  WaitApWakeup ((UINT32 
> *)(UINTN)(MpHandOff->Info[Index].StartupSignalAddress));
> +  for (MpHandOff = FirstMpHandOff;
> +   MpHandOff != NULL;
> +   MpHandOff = GetNextMpHandOffHob (MpHandOff))
> +  {
> +for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
> +  if (MpHandOff->ProcessorIndex + Index != BspNumber) {
> +WaitApWakeup ((UINT32 
> *)(UINTN)(MpHandOff->Info[Index].StartupSignalAddress));
> +  }
>  }
>}
>  }



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115764): https://edk2.groups.io/g/devel/message/115764
Mute This Topic: https://groups.io/mt/104472306/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 2/5] UefiCpuPkg/MpInitLib: Add support for multiple HOBs to GetBspNumber()

[Laszlo Ersek]

On 2/20/24 18:49, Gerd Hoffmann wrote:
> Rename the MpHandOff parameter to FirstMpHandOff.  Add a loop so the
> function inspects all HOBs present in the system.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 23 +++
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c 
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index e764bc9e4228..8f198ff6d817 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1894,26 +1894,33 @@ CheckAllAPs (
>  /**
>This function Get BspNumber.
>  
> -  @param[in] MpHandOffPointer to MpHandOff
> +  @param[in] FirstMpHandOff   Pointer to first MpHandOff HOB.

... also, it would be more precise to say "first MpHandOff HOB body".

Laszlo

>@return BspNumber
>  **/
>  UINT32
>  GetBspNumber (
> -  IN CONST MP_HAND_OFF  *MpHandOff
> +  IN CONST MP_HAND_OFF  *FirstMpHandOff
>)
>  {
> -  UINT32  ApicId;
> -  UINT32  BspNumber;
> -  UINT32  Index;
> +  UINT32 ApicId;
> +  UINT32 BspNumber;
> +  UINT32 Index;
> +  CONST MP_HAND_OFF  *MpHandOff;
>  
>//
>// Get the processor number for the BSP
>//
>BspNumber = MAX_UINT32;
>ApicId= GetInitialApicId ();
> -  for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
> -if (MpHandOff->Info[Index].ApicId == ApicId) {
> -  BspNumber = Index;
> +
> +  for (MpHandOff = FirstMpHandOff;
> +   MpHandOff != NULL;
> +   MpHandOff = GetNextMpHandOffHob (MpHandOff))
> +  {
> +for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
> +  if (MpHandOff->Info[Index].ApicId == ApicId) {
> +BspNumber = MpHandOff->ProcessorIndex + Index;
> +  }
>  }
>}
>  



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115763): https://edk2.groups.io/g/devel/message/115763
Mute This Topic: https://groups.io/mt/104472310/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 2/5] UefiCpuPkg/MpInitLib: Add support for multiple HOBs to GetBspNumber()

[Laszlo Ersek]

On 2/20/24 18:49, Gerd Hoffmann wrote:
> Rename the MpHandOff parameter to FirstMpHandOff.  Add a loop so the
> function inspects all HOBs present in the system.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 23 +++
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c 
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index e764bc9e4228..8f198ff6d817 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1894,26 +1894,33 @@ CheckAllAPs (
>  /**
>This function Get BspNumber.
>  
> -  @param[in] MpHandOffPointer to MpHandOff
> +  @param[in] FirstMpHandOff   Pointer to first MpHandOff HOB.
>@return BspNumber
>  **/
>  UINT32
>  GetBspNumber (
> -  IN CONST MP_HAND_OFF  *MpHandOff
> +  IN CONST MP_HAND_OFF  *FirstMpHandOff
>)
>  {
> -  UINT32  ApicId;
> -  UINT32  BspNumber;
> -  UINT32  Index;
> +  UINT32 ApicId;
> +  UINT32 BspNumber;
> +  UINT32 Index;
> +  CONST MP_HAND_OFF  *MpHandOff;
>  
>//
>// Get the processor number for the BSP
>//
>BspNumber = MAX_UINT32;
>ApicId= GetInitialApicId ();
> -  for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
> -if (MpHandOff->Info[Index].ApicId == ApicId) {
> -  BspNumber = Index;
> +
> +  for (MpHandOff = FirstMpHandOff;
> +   MpHandOff != NULL;
> +   MpHandOff = GetNextMpHandOffHob (MpHandOff))
> +  {
> +for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
> +  if (MpHandOff->Info[Index].ApicId == ApicId) {
> +BspNumber = MpHandOff->ProcessorIndex + Index;
> +  }
>  }
>}
>  

It's strange that the pre-patch code keeps looking even after finding
the BSP by APIC-ID; now it's stranger yet, because we even go to further
HOBs.

In theory, the inner loop body could grow a "break", and the outer loop
condition could be restricted with

  && (BspNumber == MAX_UINT32)

but that would be a separate patch.

Reviewed-by: Laszlo Ersek 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115762): https://edk2.groups.io/g/devel/message/115762
Mute This Topic: https://groups.io/mt/104472310/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 1/5] UefiCpuPkg/MpInitLib: Add support for multiple HOBs to GetMpHandOffHob

[Laszlo Ersek]

On 2/21/24 04:35, Ni, Ray wrote:
>> -Original Message-
>> From: Gerd Hoffmann 
>> Sent: Wednesday, February 21, 2024 1:50 AM
>> To: devel@edk2.groups.io
>> Cc: Oliver Steffen ; Laszlo Ersek
>> ; Kumar, Rahul R ; Ni, Ray
>> ; Gerd Hoffmann 
>> Subject: [PATCH v2 1/5] UefiCpuPkg/MpInitLib: Add support for multiple
>> HOBs to GetMpHandOffHob
>>
>> Rename the function to GetNextMpHandOffHob(), add MP_HAND_OFF
>> parameter.
>> When called with NULL pointer return the first HOB, otherwise return the
>> next in the chain.
>>
>> Also add the function prototype to the MpLib.h header file.
>>
>> Signed-off-by: Gerd Hoffmann 
>> ---
>>  UefiCpuPkg/Library/MpInitLib/MpLib.h | 12 
>>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 26 --
>>  2 files changed, 28 insertions(+), 10 deletions(-)
>>
>> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
>> b/UefiCpuPkg/Library/MpInitLib/MpLib.h
>> index a96a6389c17d..bc2a0232291d 100644
>> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
>> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
>> @@ -485,6 +485,18 @@ SwitchApContext (
>>IN MP_HAND_OFF  *MpHandOff
>>);
>>
>> +/**
>> +  Get pointer to next MP_HAND_OFF GUIDed HOB.
>> +
>> +  @param[in] MpHandOff  Previous HOB.  Pass NULL to get the first
>> HOB.
> 
> Can you please emphasize in above comments that MpHandOff points to the 
> GUIDed HOB data?
> The function implementation assumes that.

+1; had the same observation.

Laszlo



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

Re: [edk2-devel] Peims are not gettting Dispatched in EagleStream Platform

[Nate DeSimone]

> -Original Message-
> From: devel@edk2.groups.io  On Behalf Of Laszlo
> Ersek
> Sent: Wednesday, February 21, 2024 3:59 PM
> To: devel@edk2.groups.io; memrist...@proton.me
> Subject: Re: [edk2-devel] Peims are not gettting Dispatched in EagleStream
> Platform
> 
> On 2/21/24 07:59, memristor2 via groups.io wrote:
> > Hi,
> > I am trying to build edk2-platforms for EagleStream Platform. The
> > problem I am facing now is that the Peims are not getting dispatched
> > when The PeiMain routine calls PeiDispatcher().
> > After digging deeper into it it seems that the DepexSatisfied()
> > routine is always returning false. So I also checked this and realized
> > that the place that is returning false is inside the
> > PeimDispatchReadiness
> > function:
> > ||​```
> > case (EFI_DEP_END):        DEBUG ((DEBUG_DISPATCH, "  END\n"));
> >         StackPtr--;
> >         //
> >         // Check to make sure EvalStack is balanced.  If not, then
> > there is
> >         // an error in the dependency grammar, so return
> > EFI_INVALID_PARAMETER.
> >         //
> >         if (StackPtr != [0]) {
> >
> >           DEBUG ((DEBUG_DISPATCH, "  RESULT = FALSE (Underflow
> > Error)\n"));
> >           return FALSE;
> >         }
> >
> >         DEBUG ((DEBUG_DISPATCH, "  RESULT = %a\n", IsPpiInstalled
> > (PeiServices, StackPtr) ? "TRUE" : "FALSE"));
> >
> >         return IsPpiInstalled (PeiServices, StackPtr); ``` It seems
> > that when entering IsPpiInstalled StackPtr in always NULL.
> > Any thoughts on this?
> 
> StackPtr being NULL seems extremely unlikely; it is supposed to point to
> elements of the EvalStack local array (or I guess one past the last element).
> 
> Now, I can see two potential problems here:
> 
> - your depex is malformed (for whatever reason), and the eval stack is not 
> torn
> down entirely before reachig EFI_DEP_END. The code seems to handle that
> correctly, by returning FALSE.
> 
> - your depex is malformed such that it immediately starts with an
> EFI_DEP_END. The code is actually buggy for that case, because it decrements
> StackPtr *first*, before comparing it against [0].
> That decrement invokes undefined behavior. However, I assume in practice the
> behavior will be the same as in the previous paragraph.
> 
> A NULL StackPtr value I cannot explain at all.

Agreed with Laszlo here... the only thing that I could think of is memory 
corruption. Several people at Intel (myself included) as working on getting an 
EaglestreamOpenBoardPkg posted to edk2-platforms right now. I would recommend 
you wait for us to release our code as opposed to trying to develop support for 
Eagle Steam independently.

> 
> Laszlo
> 
> 
> 
> 
> 



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

Re: [edk2-devel] Peims are not gettting Dispatched in EagleStream Platform

[Laszlo Ersek]

On 2/21/24 07:59, memristor2 via groups.io wrote:
> Hi,
> I am trying to build edk2-platforms for EagleStream Platform. The
> problem I am facing now is that the Peims are not getting dispatched
> when The PeiMain routine calls PeiDispatcher().
> After digging deeper into it it seems that the DepexSatisfied() routine
> is always returning false. So I also checked this and realized that the
> place that is returning false is inside the PeimDispatchReadiness
> function:
> ||​```
> case (EFI_DEP_END):        DEBUG ((DEBUG_DISPATCH, "  END\n"));
>         StackPtr--;
>         //
>         // Check to make sure EvalStack is balanced.  If not, then there is
>         // an error in the dependency grammar, so return
> EFI_INVALID_PARAMETER.
>         //
>         if (StackPtr != [0]) {
>  
>           DEBUG ((DEBUG_DISPATCH, "  RESULT = FALSE (Underflow Error)\n"));
>           return FALSE;
>         }
> 
>         DEBUG ((DEBUG_DISPATCH, "  RESULT = %a\n",
> IsPpiInstalled (PeiServices, StackPtr) ? "TRUE" : "FALSE"));
> 
>         return IsPpiInstalled (PeiServices, StackPtr);
> ```
> It seems that when entering IsPpiInstalled StackPtr in always NULL.
> Any thoughts on this?

StackPtr being NULL seems extremely unlikely; it is supposed to point to
elements of the EvalStack local array (or I guess one past the last
element).

Now, I can see two potential problems here:

- your depex is malformed (for whatever reason), and the eval stack is
not torn down entirely before reachig EFI_DEP_END. The code seems to
handle that correctly, by returning FALSE.

- your depex is malformed such that it immediately starts with an
EFI_DEP_END. The code is actually buggy for that case, because it
decrements StackPtr *first*, before comparing it against [0].
That decrement invokes undefined behavior. However, I assume in practice
the behavior will be the same as in the previous paragraph.

A NULL StackPtr value I cannot explain at all.

Laszlo



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

Re: [edk2-devel] [PATCH] NetworkPkg:Resolved Consecutive Pxe-Http Boot Issue

[Laszlo Ersek]

On 2/21/24 18:15, Santhosh Kumar V via groups.io wrote:
> The customer has a server environment where PXE and HTTP service run in same 
> Linux Server. In this environment a SUT trying to boot to SLES 15 OS via PXE 
> from the Boot Menu. After PXE Boot file downloaded and grub Loaded without 
> continuing for installation Exit is pressed and control back to Setup.
> Now the HTTP boot to SLES 15 OS tried in the same environment and failed to 
> download the file. If there is a reconnect -r performed before this HTTP Boot 
> then boot file download and installation is getting success.
> Root cause of the issue is, when Exit from grub performed, boot Loader Stops 
> the SNP Driver and starts the same.

This sentence feels like the key one.

Are you saying that grub calls Snp->Start() just before it exits?

If so, am I right to suspect that that's a grub bug? It sounds like a
resource leak, after all.

Can you perhaps include a grub source code location / pointer in the
commit message?

> During this process SNP is in Initialized State. When HTTP boot is performed 
> immediately after PXE Failure, the MNP configure method initiates the SNP 
> Start again. Since the SNP already started by grub it returns 
> EFI_ALREADY_STARTED and none of the upper Layer drivers are getting started.
> As a work around in MNPConfigure(), if the SNP Start failed with Already 
> Started and in Initialized state we can return success so that rest of the 
> drivers can be loaded and HTTP boot can work.
>
>
> Cc: Saloni Kasbekar 
> Cc: Zachary Clark-williams 
>
> Signed-off-by: SanthoshKumar 
> ---
>  NetworkPkg/MnpDxe/MnpConfig.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/NetworkPkg/MnpDxe/MnpConfig.c b/NetworkPkg/MnpDxe/MnpConfig.c
> index 93587d53aa..0f2df28d73 100644
> --- a/NetworkPkg/MnpDxe/MnpConfig.c
> +++ b/NetworkPkg/MnpDxe/MnpConfig.c
> @@ -1120,7 +1120,9 @@ MnpStartSnp (
>// Start the simple network.
>
>//
>
>Status = Snp->Start (Snp);
>
> -
>
> +  if ((Status == EFI_ALREADY_STARTED ) && (Snp->Mode->State == 
> EfiSimpleNetworkInitialized)) {
>
> +  return EFI_SUCCESS;
>
> +  }
>
>if (!EFI_ERROR (Status)) {
>
>  //
>
>  // Initialize the simple network.
>

The commit message does say this is a workaround, and I don't
immediately any see why this workaround (in the code) would be
problematic in practice, but it still leaves a bad taste in my mouth.

Consider: the call path is the following:

MnpConfigure()   [NetworkPkg/MnpDxe/MnpConfig.c] -- public .Configure() 
protocol member function
  MnpConfigureInstance() [NetworkPkg/MnpDxe/MnpConfig.c]
MnpStart()   [NetworkPkg/MnpDxe/MnpConfig.c]
  // see notes!
  MnpStartSnp()  [NetworkPkg/MnpDxe/MnpConfig.c]

Notes: the MnpStartSnp() call in MnpStart() is conditional on two
circumstances (at the same time):
- "If it's not a configuration update, increase the configured children number."
- "It's the first configured child, start the simple network."

In other words, the MNP driver has just bound SNP "BY_DRIVER" (i.e.,
exclusively), installed the MNP service binding protocol for each vlan
(IIUC), and one of those SB instances is now being used to create the
first MNP instance. I think that under these circumstances, it is
reasonable for the MNP driver to expect that the underlying SNP be in
stopped state. :/

How long would NetworkPkg have to carry this workaround? (I.e., how long
before the grub issue is fixed, and the buggy version deprecated?)

I'd prefer at least a comment in the code that the return path is a
workaround for (I feel) an earlier SNP usage violation.

A FeaturePCD to disable the workaround could be reasonable too (but the
NetworkPkg maintainers could disagree about that).


BTW, the commit message should be wrapped at 75 characters. These long
lines (in the body) will pass PatchCheck, but generate warnings. Those
warnings are tolerable for log quotes, URLs, etc, but for normal English
text, wrapping is much preferred.


Another comment on the commit message: the subject line should state
something like

  NetworkPkg/MnpDxe: work aroung SNP state leak in grub

Laszlo



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

Re: [edk2-devel] [PATCH 0/2] Intel maintainers updates

[Nate DeSimone]

The series has been pushed as 73ebcac~..b7b8dd4.

> -Original Message-
> From: devel@edk2.groups.io  On Behalf Of Nathaniel
> Haller
> Sent: Friday, February 16, 2024 5:47 PM
> To: devel@edk2.groups.io
> Subject: [edk2-devel] [PATCH 0/2] Intel maintainers updates
> 
> Adding maintainers for new Intel packages, EaglestreamSiliconBinPkg and
> EaglestreamOpenBoardBinPkg, then replacing Isaac W Oram from the Intel
> package maintainers lists.
> 
> Nathaniel Haller (2):
>   Add maintainers for EaglestreamSiliconBinPkg and
> EaglestreamOpenBoardBinPkg
>   Update Maintainers for Intel packages
> 
>  Maintainers.txt | 17 ++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> --
> 2.43.0.windows.1
> 
> 
> 
> 
> 



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

Re: [edk2-devel] Merge commit in edk2-non-osi

[Nate DeSimone]

> -Original Message-
> From: Ard Biesheuvel 
> Sent: Wednesday, February 21, 2024 3:29 PM
> To: devel@edk2.groups.io; Kinney, Michael D 
> Cc: Desimone, Nathaniel L 
> Subject: Re: [edk2-devel] Merge commit in edk2-non-osi
> 
> On Wed, 21 Feb 2024 at 16:05, Michael D Kinney
>  wrote:
> >
> > Hi Ard,
> >
> > I disagree.  We have never allowed a force push to the main branch of
> > TianoCore repos.  This has happened before and was discussed and the
> > policy is to not fix.  Even the edk2 repo has some merge commits in
> > its history that were discussed and not fixed.
> >
> > We can never know how many downstream consumers are syncing with
> main
> > branches.
> >
> 
> Fair enough. Better to be cautious, and my argument works both ways:
> the contents would not change by 'fixing' the history, and this change does 
> not
> look problematic for bisect.

Fair enough, I will continue with my reviews/patch merges.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115753): https://edk2.groups.io/g/devel/message/115753
Mute This Topic: https://groups.io/mt/104481195/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/4] BaseTools/Scripts/PatchCheck: Error if no Cc tags are present

[Ard Biesheuvel]

On Wed, 21 Feb 2024 at 23:16, Laszlo Ersek  wrote:
>
> On 2/20/24 15:48, Ard Biesheuvel wrote:
> > Hello Mike,
> >
> > I understand the desire to be pedantic about cc'ing the right
> > maintainers, but I'm not convinced this is the way.
> >
> > - the presence of a cc: tag does not guarantee that the person was
> > cc'ed - only git send-email will take CC:s in the message body into
> > account by default (but this can also be disabled), but generally, the
> > sender has to ensure the cc list is copied into the cc: field
> > - the absence of a cc: tag does not imply that the person was not cc'ed,
> >
> > - in Linux, the cc: tag has slightly different semantics from the ones
> > we appear to be assuming here: a cc tag in patch going into the
> > repository is a statement by the maintainer that the person in
> > question has been cc'ed, may have some 'jurisdiction' over the area,
> > but hasn't bothered to respond. IOW, it is to record the fact that
> > this person has been given the opportunity to respond.
> >
> > Then there is the matter of a maintainer that has reviewed the patch
> > themselves. I usually remove the cc lines listing people that have
> > reviewed/acked/tested the patch, as those tags already convey that the
> > person is aware of it cc'ed or not.
>
> I've noticed this (on patches you merged), and -- not having similar
> maintainer experience in Linux -- I was surprised. I more or less
> deduced the intent, but it felt a bit foreign (or at least novel!) to edk2.
>
> To me, the greatest benefits of Cc's in commit messages are (as opposed
> to command line specified Cc's):
>
> - fine-grained: each patch can target a specific set of reviewers /
> maintainers,
>
> - long-lived: the CC list survives rebases / v2, v3 etc iterations! (Of
> course, if a patch undergoes serious scope changes when revised, then
> the Cc list will have to be updated manually. But that's quite rare.)
>
> >
> > So perhaps it would be better to make this check part of the
> > contributor workflow but not the GitHub PR/CI workflow?
>
> I agree that adding Cc's to the commit message body is not fool-proof
> (like you explain), but like Mike, I have no better idea for preventing
> contributors from posting patches without properly CC'ing
> reviewers/maintainers (be it with whatever particular CC'ing method they
> prefer).
>
> I tend to run PatchCheck locally (not solely relying on CI for that --
> PatchCheck is quick and has no intrusive dependencies, plus seeing CI
> fail just because of PatchCheck is super irritating), so in my workflow,
> this patch would fit well. Of course, with the same effort of
> remembering to run PatchCheck locally, I also remember to add Cc's in
> the first place...
>
> I admit that reviewer assignment is a significant shortcoming of the
> email-based workflow. What we'd really need is a groups.io-level action
> :) -- if the subject contains PATCH or Patch in brackets, but the body
> lacks ^Cc or ^CC, *reject* the email. (Rejection gives the sender an
> explanation.) Alas, rejection is currently only a manual action that's
> available to moderators (and only on messages for senders that have not
> been unmoderated yet).
>
> So, my take: not perfect, but much better than nothing.
>

Yeah, I can't argue with that. I agree that it is very annoying that
patches don't get cc'ed to the right people. (It is even more annoying
that many maintainers [including myself at times - mea culpa] don't
bother to respond, but that is a different matter)

So let's try this, and in case it does more harm than good, we can
always back it out again (or make modifications to the logic)


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

Re: [edk2-devel] Merge commit in edk2-non-osi

[Ard Biesheuvel]

On Wed, 21 Feb 2024 at 16:05, Michael D Kinney
 wrote:
>
> Hi Ard,
>
> I disagree.  We have never allowed a force push to the main
> branch of TianoCore repos.  This has happened before and
> was discussed and the policy is to not fix.  Even the edk2
> repo has some merge commits in its history that were discussed
> and not fixed.
>
> We can never know how many downstream consumers are syncing
> with main branches.
>

Fair enough. Better to be cautious, and my argument works both ways:
the contents would not change by 'fixing' the history, and this change
does not look problematic for bisect.


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

[edk2-devel] [PATCH] UefiCpuPkg: Fix IN OUT parameters marked as IN

[Zhou Jianfeng]

Some IN OUT parameters in CpuPageTableMap.c were mistakenly marked as IN.
"IN" replaced with "IN OUT" in the following interfaces:

PageTableLibSetPte4K(): Pte4K
PageTableLibSetPleB():  PleB
PageTableLibSetPle():   Ple
PageTableLibSetPnle():  Pnle

Signed-off-by: Zhou Jianfeng 
Cc: Ray Ni 
Cc: Laszlo Ersek 
Cc: Rahul Kumar 
Cc: Gerd Hoffmann 
---
 .../Library/CpuPageTableLib/CpuPageTableMap.c | 32 +--
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c 
b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index ae4caf8dfe..2ea40666cc 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -20,10 +20,10 @@
 **/
 VOID
 PageTableLibSetPte4K (
-  IN IA32_PTE_4K *Pte4K,
-  IN UINT64  Offset,
-  IN IA32_MAP_ATTRIBUTE  *Attribute,
-  IN IA32_MAP_ATTRIBUTE  *Mask
+  IN OUT IA32_PTE_4K *Pte4K,
+  IN UINT64  Offset,
+  IN IA32_MAP_ATTRIBUTE  *Attribute,
+  IN IA32_MAP_ATTRIBUTE  *Mask
   )
 {
   IA32_PTE_4K  LocalPte4K;
@@ -94,10 +94,10 @@ PageTableLibSetPte4K (
 **/
 VOID
 PageTableLibSetPleB (
-  IN IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  *PleB,
-  IN UINT64 Offset,
-  IN IA32_MAP_ATTRIBUTE *Attribute,
-  IN IA32_MAP_ATTRIBUTE *Mask
+  IN OUT IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  *PleB,
+  IN UINT64 Offset,
+  IN IA32_MAP_ATTRIBUTE *Attribute,
+  IN IA32_MAP_ATTRIBUTE *Mask
   )
 {
   IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  LocalPleB;
@@ -171,11 +171,11 @@ PageTableLibSetPleB (
 **/
 VOID
 PageTableLibSetPle (
-  IN UINTN   Level,
-  IN IA32_PAGING_ENTRY   *Ple,
-  IN UINT64  Offset,
-  IN IA32_MAP_ATTRIBUTE  *Attribute,
-  IN IA32_MAP_ATTRIBUTE  *Mask
+  IN UINTN   Level,
+  IN OUT IA32_PAGING_ENTRY   *Ple,
+  IN UINT64  Offset,
+  IN IA32_MAP_ATTRIBUTE  *Attribute,
+  IN IA32_MAP_ATTRIBUTE  *Mask
   )
 {
   if (Level == 1) {
@@ -195,9 +195,9 @@ PageTableLibSetPle (
 **/
 VOID
 PageTableLibSetPnle (
-  IN IA32_PAGE_NON_LEAF_ENTRY  *Pnle,
-  IN IA32_MAP_ATTRIBUTE*Attribute,
-  IN IA32_MAP_ATTRIBUTE*Mask
+  IN OUT IA32_PAGE_NON_LEAF_ENTRY  *Pnle,
+  IN IA32_MAP_ATTRIBUTE*Attribute,
+  IN IA32_MAP_ATTRIBUTE*Mask
   )
 {
   IA32_PAGE_NON_LEAF_ENTRY  LocalPnle;
--
2.31.1.windows.1



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

[edk2-devel] Peims are not gettting Dispatched in EagleStream Platform

[memristor2 via groups.io]

Hi,
I am trying to build edk2-platforms for EagleStream Platform. The problem I am 
facing now is that the Peims are not getting dispatched when The PeiMain 
routine calls PeiDispatcher().
After digging deeper into it it seems that the DepexSatisfied() routine is 
always returning false. So I also checked this and realized that the place that 
is returning false is inside the PeimDispatchReadiness
function:
​```
case (EFI_DEP_END): DEBUG ((DEBUG_DISPATCH, " END\n"));
StackPtr--;
//
// Check to make sure EvalStack is balanced. If not, then there is
// an error in the dependency grammar, so return EFI_INVALID_PARAMETER.
//
if (StackPtr != [0]) {

DEBUG ((DEBUG_DISPATCH, " RESULT = FALSE (Underflow Error)\n"));
return FALSE;
}

DEBUG ((DEBUG_DISPATCH, " RESULT = %a\n", IsPpiInstalled (PeiServices, 
StackPtr) ? "TRUE" : "FALSE"));

return IsPpiInstalled (PeiServices, StackPtr);
```
It seems that when entering IsPpiInstalled StackPtr in always NULL.Any thoughts 
on this?

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

[edk2-devel] [PATCH] NetworkPkg:Resolved Consecutive Pxe-Http Boot Issue

[Santhosh Kumar V via groups.io]

The customer has a server environment where PXE and HTTP service run in same 
Linux Server. In this environment a SUT trying to boot to SLES 15 OS via PXE 
from the Boot Menu. After PXE Boot file downloaded and grub Loaded without 
continuing for installation Exit is pressed and control back to Setup.
Now the HTTP boot to SLES 15 OS tried in the same environment and failed to 
download the file. If there is a reconnect -r performed before this HTTP Boot 
then boot file download and installation is getting success.
Root cause of the issue is, when Exit from grub performed, boot Loader Stops 
the SNP Driver and starts the same. During this process SNP is in Initialized 
State. When HTTP boot is performed immediately after PXE Failure, the MNP 
configure method initiates the SNP Start again. Since the SNP already started 
by grub it returns EFI_ALREADY_STARTED and none of the upper Layer drivers are 
getting started.
As a work around in MNPConfigure(), if the SNP Start failed with Already 
Started and in Initialized state we can return success so that rest of the 
drivers can be loaded and HTTP boot can work.


Cc: Saloni Kasbekar 
Cc: Zachary Clark-williams 

Signed-off-by: SanthoshKumar 
---
 NetworkPkg/MnpDxe/MnpConfig.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/NetworkPkg/MnpDxe/MnpConfig.c b/NetworkPkg/MnpDxe/MnpConfig.c
index 93587d53aa..0f2df28d73 100644
--- a/NetworkPkg/MnpDxe/MnpConfig.c
+++ b/NetworkPkg/MnpDxe/MnpConfig.c
@@ -1120,7 +1120,9 @@ MnpStartSnp (
   // Start the simple network.

   //

   Status = Snp->Start (Snp);

-

+  if ((Status == EFI_ALREADY_STARTED ) && (Snp->Mode->State == 
EfiSimpleNetworkInitialized)) {

+  return EFI_SUCCESS;

+  }

   if (!EFI_ERROR (Status)) {

 //

 // Initialize the simple network.

--
2.42.0.windows.2
-The information contained in this message may be confidential and proprietary 
to American Megatrends (AMI). This communication is intended to be read only by 
the individual or entity to whom it is addressed or by their designee. If the 
reader of this message is not the intended recipient, you are on notice that 
any distribution of this message, in any form, is strictly prohibited. Please 
promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and 
then delete or destroy all copies of the transmission.


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

[edk2-devel] [PATCH] UefiCpuPkg: add volatile qualifier to page table related variable

[Zhou Jianfeng]

Add volatile qualifier to page table related variable to prevent
compiler from optimizing away the variables which may lead to
unexpected result.

Signed-off-by: Zhou Jianfeng 
Cc: Ray Ni 
Cc: Laszlo Ersek 
Cc: Rahul Kumar 
Cc: Gerd Hoffmann 
---
 UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c 
b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 2ea40666cc..5cf6e8fea0 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -26,7 +26,7 @@ PageTableLibSetPte4K (
   IN IA32_MAP_ATTRIBUTE  *Mask
   )
 {
-  IA32_PTE_4K  LocalPte4K;
+  volatile IA32_PTE_4K  LocalPte4K;

   LocalPte4K.Uint64 = Pte4K->Uint64;
   if (Mask->Bits.PageTableBaseAddressLow || 
Mask->Bits.PageTableBaseAddressHigh) {
@@ -78,7 +78,7 @@ PageTableLibSetPte4K (
   }

   if (Pte4K->Uint64 != LocalPte4K.Uint64) {
-Pte4K->Uint64 = LocalPte4K.Uint64;
+*(volatile UINT64 *)&(Pte4K->Uint64) = LocalPte4K.Uint64;
   }
 }

@@ -100,7 +100,7 @@ PageTableLibSetPleB (
   IN IA32_MAP_ATTRIBUTE *Mask
   )
 {
-  IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  LocalPleB;
+  volatile IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  LocalPleB;

   LocalPleB.Uint64 = PleB->Uint64;
   if (Mask->Bits.PageTableBaseAddressLow || 
Mask->Bits.PageTableBaseAddressHigh) {
@@ -154,7 +154,7 @@ PageTableLibSetPleB (
   }

   if (PleB->Uint64 != LocalPleB.Uint64) {
-PleB->Uint64 = LocalPleB.Uint64;
+*(volatile UINT64 *)&(PleB->Uint64) = LocalPleB.Uint64;
   }
 }

@@ -200,7 +200,7 @@ PageTableLibSetPnle (
   IN IA32_MAP_ATTRIBUTE*Mask
   )
 {
-  IA32_PAGE_NON_LEAF_ENTRY  LocalPnle;
+  volatile IA32_PAGE_NON_LEAF_ENTRY  LocalPnle;

   LocalPnle.Uint64 = Pnle->Uint64;
   if (Mask->Bits.Present) {
@@ -231,7 +231,7 @@ PageTableLibSetPnle (
   LocalPnle.Bits.WriteThrough  = 0;
   LocalPnle.Bits.CacheDisabled = 0;
   if (Pnle->Uint64 != LocalPnle.Uint64) {
-Pnle->Uint64 = LocalPnle.Uint64;
+*(volatile UINT64 *)&(Pnle->Uint64) = LocalPnle.Uint64;
   }
 }

--
2.31.1.windows.1



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

[edk2-devel] Peims are not gettting Dispatched in EagleStream Platform

[memristor2 via groups.io]

Hi,
I am trying to build edk2-platforms for EagleStream Platform. The problem I am 
facing now is that the Peims are not getting dispatched when The PeiMain 
routine calls PeiDispatcher().
After digging deeper into it it seems that the DepexSatisfied() routine is 
always returning false. So I also checked this and realized that the place that 
is returning false is inside the PeimDispatchReadiness
function:
​```
case (EFI_DEP_END): DEBUG ((DEBUG_DISPATCH, " END\n"));
StackPtr--;
//
// Check to make sure EvalStack is balanced. If not, then there is
// an error in the dependency grammar, so return EFI_INVALID_PARAMETER.
//
if (StackPtr != [0]) {

DEBUG ((DEBUG_DISPATCH, " RESULT = FALSE (Underflow Error)\n"));
return FALSE;
}

DEBUG ((DEBUG_DISPATCH, " RESULT = %a\n", IsPpiInstalled (PeiServices, 
StackPtr) ? "TRUE" : "FALSE"));

return IsPpiInstalled (PeiServices, StackPtr);
```
It seems that when entering IsPpiInstalled StackPtr in always NULL.
Any thoughts on this?

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115748): https://edk2.groups.io/g/devel/message/115748
Mute This Topic: https://groups.io/mt/104498510/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/4] BaseTools/Scripts/PatchCheck: Error if no Cc tags are present

[Laszlo Ersek]

On 2/20/24 15:48, Ard Biesheuvel wrote:
> Hello Mike,
> 
> I understand the desire to be pedantic about cc'ing the right
> maintainers, but I'm not convinced this is the way.
> 
> - the presence of a cc: tag does not guarantee that the person was
> cc'ed - only git send-email will take CC:s in the message body into
> account by default (but this can also be disabled), but generally, the
> sender has to ensure the cc list is copied into the cc: field
> - the absence of a cc: tag does not imply that the person was not cc'ed,
> 
> - in Linux, the cc: tag has slightly different semantics from the ones
> we appear to be assuming here: a cc tag in patch going into the
> repository is a statement by the maintainer that the person in
> question has been cc'ed, may have some 'jurisdiction' over the area,
> but hasn't bothered to respond. IOW, it is to record the fact that
> this person has been given the opportunity to respond.
> 
> Then there is the matter of a maintainer that has reviewed the patch
> themselves. I usually remove the cc lines listing people that have
> reviewed/acked/tested the patch, as those tags already convey that the
> person is aware of it cc'ed or not.

I've noticed this (on patches you merged), and -- not having similar
maintainer experience in Linux -- I was surprised. I more or less
deduced the intent, but it felt a bit foreign (or at least novel!) to edk2.

To me, the greatest benefits of Cc's in commit messages are (as opposed
to command line specified Cc's):

- fine-grained: each patch can target a specific set of reviewers /
maintainers,

- long-lived: the CC list survives rebases / v2, v3 etc iterations! (Of
course, if a patch undergoes serious scope changes when revised, then
the Cc list will have to be updated manually. But that's quite rare.)

> 
> So perhaps it would be better to make this check part of the
> contributor workflow but not the GitHub PR/CI workflow?

I agree that adding Cc's to the commit message body is not fool-proof
(like you explain), but like Mike, I have no better idea for preventing
contributors from posting patches without properly CC'ing
reviewers/maintainers (be it with whatever particular CC'ing method they
prefer).

I tend to run PatchCheck locally (not solely relying on CI for that --
PatchCheck is quick and has no intrusive dependencies, plus seeing CI
fail just because of PatchCheck is super irritating), so in my workflow,
this patch would fit well. Of course, with the same effort of
remembering to run PatchCheck locally, I also remember to add Cc's in
the first place...

I admit that reviewer assignment is a significant shortcoming of the
email-based workflow. What we'd really need is a groups.io-level action
:) -- if the subject contains PATCH or Patch in brackets, but the body
lacks ^Cc or ^CC, *reject* the email. (Rejection gives the sender an
explanation.) Alas, rejection is currently only a manual action that's
available to moderators (and only on messages for senders that have not
been unmoderated yet).

So, my take: not perfect, but much better than nothing.

Laszlo

> 
> 
> On Sun, 18 Feb 2024 at 22:00, Michael D Kinney
>  wrote:
>>
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4694
>>
>> If no Cc tags are detected in a commit message, then generate an
>> error. All patches sent for review are required to provide the set
>> of maintainers and reviewers responsible for the directories/files
>> modified. The set of maintainers and reviewers are documented in
>> Maintainers.txt and can be retrieved using the script
>> BaseTools/Scripts/GetMaintainer.py.
>>
>> Cc: Rebecca Cran 
>> Cc: Liming Gao 
>> Cc: Bob Feng 
>> Cc: Yuwei Chen 
>> Cc: Michael Kubacki 
>> Signed-off-by: Michael D Kinney 
>> ---
>>  BaseTools/Scripts/PatchCheck.py | 6 --
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/BaseTools/Scripts/PatchCheck.py 
>> b/BaseTools/Scripts/PatchCheck.py
>> index 158a2b30a5ce..415198e3824e 100755
>> --- a/BaseTools/Scripts/PatchCheck.py
>> +++ b/BaseTools/Scripts/PatchCheck.py
>> @@ -229,8 +229,10 @@ class CommitMessageCheck:
>>  )
>>
>>  def check_misc_signatures(self):
>> -for sig in self.sig_types:
>> -self.find_signatures(sig)
>> +for sigtype in self.sig_types:
>> +sigs = self.find_signatures(sigtype)
>> +if sigtype == 'Cc' and len(sigs) == 0:
>> +self.error('No Cc: tags for maintainers/reviewers found!')
>>
>>  cve_re = re.compile('CVE-[0-9]{4}-[0-9]{5}[^0-9]')
>>
>> --
>> 2.40.1.windows.1
>>
>>
>>
>>
>>
>>
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115745): https://edk2.groups.io/g/devel/message/115745
Mute This Topic: https://groups.io/mt/104434584/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] MdeModulePkg: Load Serial driver earlier in DXE

[Laszlo Ersek]

On 2/21/24 18:15, Borzeszkowski, Alan wrote:
>> It does not make sense to have a UEFI Driver active in early DXE because it 
>> will not be connected yet and has dependencies on other UEFI drivers that 
>> will not be connected yet.
> 
> With suggested change, we connect to this driver successfully in early DXE 
> using ConnectController(). We did not observe any issues when Serial driver 
> came online, debug messages are printed and console redirection works just 
> fine.

Yes, and that ConnectController() call in early DXE -- wherever you are
performing it -- is a driver model violation: one that works around the
*other* driver model violation (the one Mike points out). :)

> 
>> Did you consider the use of the SerialPortLib for early DXE that can use PCI 
>> serial devices with PcdSerialPciDeviceInfo that can be used for DEBUG() 
>> messages.
> 
> That's the opposite of what we are trying to accomplish, this way additional 
> maintenance cost is required and on top of that, management of PCI device 
> from library level is complicated (e.g., checking device state).

Your approach here could be salvageable, but it must change the external
interface of the driver. More precisely, you'd need a new INF file, and
some extra C sources, for building the driver as a platform DXE driver
-- one that installs the SerialIo protocol and the Device Path protocol
on a brand new handle (or maybe on the image handle itself).

At the same time, your platform DSC would have to:

- remove the UEFI driver build of the driver,

- dispatch the platform DXE driver build of the driver really early on,
probably using the APRIORI DXE file.

This is generally the model that the UEFI driver model was supposed to
*supersede*. But, if you do consider the serial port a platform device,
it is doable.

Now, here's at least one more complication. The original driver accesses
PCI config space via the PciIo protocol, from a quick grep -- that's
correct, that's what a UEFI driver following the UEFI driver model
should do.

However, once you turn the SerialIo driver into a platform DXE driver,
you cannot depend on PciIo anymore.

(

And this actually goes on to show just how much of a layering violation
your explicit ConnectController() call is:

(1) Normally, platform firmware includes the PCI host bridge driver,
which *is* a platform DXE driver, producing
EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL instances in its entry point function.

In edk2, this is
"MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf", with
platform-specific bits delegated to various library instances; such as
PCI Config Space access delegated to PciSegmentLib, and root bridge
enumeration delegated to PciHostBridgeLib.

(2) PciBusDxe is a UEFI_DRIVER, binding EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL
instances, and producing PciIo protocol instances.

Importantly, the component that kicks off PciBusDxe -- i.e., the one
that causes PciBusDxe to perform PCI enumeration and resource assignment
-- is Platform BDS. Platform BDS *is* supposed to manually connect
PciBusDxe (or, well, all possible drivers) to
EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL instances.

Therefore, when you call ConnectController() in early DXE -- so that the
serial port driver start to operate on top of PciIo instances --, you
actually recursively set off that whole shebang *prematurely* -- PCI
enumeration included. Full PCI enumeration should not really occur
before BDS.

(3) Note that UEFI drivers, unlike platform DXE drivers, have an
implicit depex on *all* the architectural UEFI protocols. Such depexes
are generally expected to be satisfiable by the time BDS is entered.
Thus your workaround -- which causes UEFI drivers to bind devices way
before BDS -- in fact depends on the relevant DXE drivers -- providing
the arch protocols -- being dispatched "early enough", so that those
UEFI drivers can even be dispatched.

This is quite a mangling of how drivers are supposed to be dispatched by
the DXE core.

Anyway, digression ends; here's my larger point about PciIo:

)

Rather than via PciIo protocol instances, you have to access PCI config
space with one of the platform-matching PCI library classes / instances;
for example, PciSegmentLib, PciLib, PciExpressLib, PciCf8Lib.

Thus, you cannot depend on PCI enumeration in the first place, and you'd
have to abstract away PCI config space access internally to the serial
driver. In the UEFI driver build, those abstraction would boil down to
PciIo member calls, while in the platform DXE driver build, to Pci*Lib
calls.

(You'll also have to manually compose a minimal [depex] section for the
INF file, for those protocols -- provided by other platform DXE drivers,
not UEFI drivers! -- that your serial port driver consumes.)

Finally, the PCI config space aspect brings me to a broader platform
design question. Using a *PCI* serial port for *debugging* is a terrible
choice -- not a software choice, mind you, but a hardware one. That's
precisely because PCI is so complex and high-level. 

Re: [edk2-devel] [PATCH] UefiCpuPkg: add volatile qualifier to page table related variable

[Pedro Falcato]

On Wed, Feb 21, 2024 at 8:36 PM Laszlo Ersek  wrote:
>
> On 2/21/24 02:25, Zhou Jianfeng wrote:
> > Add volatile qualifier to page table related variable to prevent
> > compiler from optimizing away the variables which may lead to
> > unexpected result.
> >
> > Signed-off-by: Zhou Jianfeng 
> > Cc: Ray Ni 
> > Cc: Laszlo Ersek 
> > Cc: Rahul Kumar 
> > Cc: Gerd Hoffmann 

I'd appreciate getting CC'd on my own suggestion

> > ---
> >  UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 12 ++--
> >  1 file changed, 6 insertions(+), 6 deletions(-)
>
> (1) subject should be something like:
>
>   UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile
>
> >
> > diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c 
> > b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> > index 2ea40666cc..5cf6e8fea0 100644
> > --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> > +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> > @@ -26,7 +26,7 @@ PageTableLibSetPte4K (
> >IN IA32_MAP_ATTRIBUTE  *Mask
> >)
> >  {
> > -  IA32_PTE_4K  LocalPte4K;
> > +  volatile IA32_PTE_4K  LocalPte4K;
> >
> >LocalPte4K.Uint64 = Pte4K->Uint64;
> >if (Mask->Bits.PageTableBaseAddressLow || 
> > Mask->Bits.PageTableBaseAddressHigh) {
> > @@ -78,7 +78,7 @@ PageTableLibSetPte4K (
> >}
> >
> >if (Pte4K->Uint64 != LocalPte4K.Uint64) {
> > -Pte4K->Uint64 = LocalPte4K.Uint64;
> > +*(volatile UINT64 *)&(Pte4K->Uint64) = LocalPte4K.Uint64;
> >}
> >  }
> >
> > @@ -100,7 +100,7 @@ PageTableLibSetPleB (
> >IN IA32_MAP_ATTRIBUTE *Mask
> >)
> >  {
> > -  IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  LocalPleB;
> > +  volatile IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  LocalPleB;
> >
> >LocalPleB.Uint64 = PleB->Uint64;
> >if (Mask->Bits.PageTableBaseAddressLow || 
> > Mask->Bits.PageTableBaseAddressHigh) {
> > @@ -154,7 +154,7 @@ PageTableLibSetPleB (
> >}
> >
> >if (PleB->Uint64 != LocalPleB.Uint64) {
> > -PleB->Uint64 = LocalPleB.Uint64;
> > +*(volatile UINT64 *)&(PleB->Uint64) = LocalPleB.Uint64;
> >}
> >  }
> >
> > @@ -200,7 +200,7 @@ PageTableLibSetPnle (
> >IN IA32_MAP_ATTRIBUTE*Mask
> >)
> >  {
> > -  IA32_PAGE_NON_LEAF_ENTRY  LocalPnle;
> > +  volatile IA32_PAGE_NON_LEAF_ENTRY  LocalPnle;
> >
> >LocalPnle.Uint64 = Pnle->Uint64;
> >if (Mask->Bits.Present) {
> > @@ -231,7 +231,7 @@ PageTableLibSetPnle (
> >LocalPnle.Bits.WriteThrough  = 0;
> >LocalPnle.Bits.CacheDisabled = 0;
> >if (Pnle->Uint64 != LocalPnle.Uint64) {
> > -Pnle->Uint64 = LocalPnle.Uint64;
> > +*(volatile UINT64 *)&(Pnle->Uint64) = LocalPnle.Uint64;
> >}
> >  }
>
> I agree with the idea (I think it's a necessary change, or put
> differently, an improvement, even though I may not be convinced that it
> is a *sufficient* improvement; but let's not rehash all that here
> again); however, I think the implementation is not the greatest.
>
> Volatile-qualifying the local variables does not seem useful for
> anything. It's fine -- actually: it's beneficial -- if the compiler
> optimizes accesses to those locals -- being on the stack -- as heavily
> as it can. In other words, those parts of the patch look like a small
> performance regression.
>
> (2) What we want to qualify as volatile here are the *targets* of the
> Pte4K, PleB and Pnle pointers. Your other patch ("UefiCpuPkg: Fix IN OUT
> parameters marked as IN") correctly marks those as "IN OUT", so in this
> patch, we should update them to:
>
>   IN OUT volatile IA32_PAGE_NON_LEAF_ENTRY  *Pnle
>
> and similar. Then the existent assignment expressions
>
>   Pnle->Uint64 = LocalPnle.Uint64;
>
> don't have to be changed.

I echo these comments :)

>
> Note that call sites will not have to be updated either; see C99 6.3.2.3
> Pointers, paragraph 2:
>
> For any qualifier q, a pointer to a non-q-qualified type may be
> converted to a pointer to the q-qualified version of the type; the
> values stored in the original and converted pointers shall compare
> equal.

Ugh, honestly converting to volatile implicitly is kind-of yucky, but
I guess it works; personally I'd rather have explicit conversion, but
it's just a matter of taste.
What I *really* prefer in these cases (when we're not dealing with
MMIO) is something like READ_ONCE and WRITE_ONCE, where the
"volatility points" are very well annotated, but oh well :)

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115743): https://edk2.groups.io/g/devel/message/115743
Mute This Topic: https://groups.io/mt/104483610/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] MdeModulePkg: Load Serial driver earlier in DXE

[Michael D Kinney]

DXE env is not UEFI conformant.  UEFI Drivers can not be executed
until the UEFI env is fully established which is at end of DXE
after all DXE Arch Protocols are produced and DXE Core supports the
full set of requires UEFI services. Running a UEFI Driver or UEFI
Application before all DXE Arch Protocols are produced has many 
risks. 

What do you mean by "UEFI Debug Prints" in the patch?  What service 
is being used to print and what components in DXE Phase before BDS
are using a UEFI print service?

I may have incorrectly assumed that "UEFI Debug Print" was the 
use of DEBUG() macro.

Mike

> -Original Message-
> From: Borzeszkowski, Alan 
> Sent: Wednesday, February 21, 2024 9:16 AM
> To: Kinney, Michael D ;
> devel@edk2.groups.io
> Cc: Albecki, Mateusz ; Gao, Zhichao
> ; Ni, Ray 
> Subject: RE: [edk2-devel] [PATCH 1/1] MdeModulePkg: Load Serial driver
> earlier in DXE
> 
> > It does not make sense to have a UEFI Driver active in early DXE
> because it will not be connected yet and has dependencies on other UEFI
> drivers that will not be connected yet.
> 
> With suggested change, we connect to this driver successfully in early
> DXE using ConnectController(). We did not observe any issues when
> Serial driver came online, debug messages are printed and console
> redirection works just fine.
> 
> > Did you consider the use of the SerialPortLib for early DXE that can
> use PCI serial devices with PcdSerialPciDeviceInfo that can be used for
> DEBUG() messages.
> 
> That's the opposite of what we are trying to accomplish, this way
> additional maintenance cost is required and on top of that, management
> of PCI device from library level is complicated (e.g., checking device
> state).
> 
> > The other option is to map the PCI UART into Report Status Code.
> 
> Could you elaborate on that?
> 
> Also, could you please explain why DXE drivers cannot use Driver
> Binding?
> 
> Regards,
> Alan
> 
> 
> 
> -Original Message-
> From: Kinney, Michael D 
> Sent: Tuesday, February 20, 2024 6:12 PM
> To: devel@edk2.groups.io; Borzeszkowski, Alan
> 
> Cc: Albecki, Mateusz ; Gao, Zhichao
> ; Ni, Ray ; Kinney, Michael D
> 
> Subject: RE: [edk2-devel] [PATCH 1/1] MdeModulePkg: Load Serial driver
> earlier in DXE
> 
> This is a UEFI Driver that depends on the Driver Binding Protocol and
> use of ConnectController(). These drivers cannot be used until the BDS
> phase when the active consoles and boot devices are evaluated and the
> smallest set of drivers required to boot are connected.
> 
> It does not make sense to have a UEFI Driver active in early DXE
> because it will not be connected yet and has dependencies on other UEFI
> drivers that will not be connected yet.
> 
> Did you consider the use of the SerialPortLib for early DXE that can
> use PCI serial devices with PcdSerialPciDeviceInfo that can be used for
> DEBUG() messages.
> 
> The other option is to map the PCI UART into Report Status Code.
> 
> Best regards,
> 
> Mike
> 
> > -Original Message-
> > From: devel@edk2.groups.io  On Behalf Of
> > Borzeszkowski, Alan
> > Sent: Tuesday, February 20, 2024 4:11 AM
> > To: devel@edk2.groups.io
> > Cc: Albecki, Mateusz ; Gao, Zhichao
> > ; Ni, Ray ; Borzeszkowski,
> > Alan 
> > Subject: [edk2-devel] [PATCH 1/1] MdeModulePkg: Load Serial driver
> > earlier in DXE
> >
> > For the purpose of UEFI debug prints enablement in DXE phase, Serial
> > driver should load earlier. Separate .inf file is created in order to
> > make minimal changes to current implementation.
> >
> > Signed-off-by: Alan Borzeszkowski 
> > ---
> >  .../PciSioSerialDxe/PciSioSerialDxeEarly.inf  | 80
> > +++
> >  1 file changed, 80 insertions(+)
> >  create mode 100644
> > MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxeEarly.inf
> >
> > diff --git
> > a/MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxeEarly.inf
> > b/MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxeEarly.inf
> > new file mode 100644
> > index 00..2ead654898
> > --- /dev/null
> > +++ b/MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxeEarly.inf
> > @@ -0,0 +1,80 @@
> > +## @file
> > +# Serial driver for standard UARTS on a SIO chip or PCI/PCIE card.
> > +#
> > +# Produces the Serial I/O protocol for standard UARTS using Super
> I/O
> > or PCI I/O.
> > +# This version is used shortly after DXE Core is invoked # #
> > +Copyright (c) 2007 - 2018, Intel Corporation. All rights
> > reserved.
> > +#
> > +# SPDX-License-Identifier: BSD-2-Clause-Patent # ##
> > +
> > +[Defines]
> > +  INF_VERSION= 0x00010005
> > +  BASE_NAME  = PciSioSerialDxeEarly
> > +  MODULE_UNI_FILE= PciSioSerialDxe.uni
> > +  FILE_GUID  = 8BCC425E-585F-4E66-ADA5-
> > FEA9A635F911
> > +  MODULE_TYPE= DXE_DRIVER
> > +  VERSION_STRING = 1.0
> > +  ENTRY_POINT= InitializePciSioSerial
> > +
> > +#
> > +# The following information is for 

Re: [edk2-devel] [PATCH] UefiCpuPkg: add volatile qualifier to page table related variable

[Laszlo Ersek]

On 2/21/24 02:25, Zhou Jianfeng wrote:
> Add volatile qualifier to page table related variable to prevent
> compiler from optimizing away the variables which may lead to
> unexpected result.
> 
> Signed-off-by: Zhou Jianfeng 
> Cc: Ray Ni 
> Cc: Laszlo Ersek 
> Cc: Rahul Kumar 
> Cc: Gerd Hoffmann 
> ---
>  UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)

(1) subject should be something like:

  UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile

> 
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c 
> b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> index 2ea40666cc..5cf6e8fea0 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> @@ -26,7 +26,7 @@ PageTableLibSetPte4K (
>IN IA32_MAP_ATTRIBUTE  *Mask
>)
>  {
> -  IA32_PTE_4K  LocalPte4K;
> +  volatile IA32_PTE_4K  LocalPte4K;
> 
>LocalPte4K.Uint64 = Pte4K->Uint64;
>if (Mask->Bits.PageTableBaseAddressLow || 
> Mask->Bits.PageTableBaseAddressHigh) {
> @@ -78,7 +78,7 @@ PageTableLibSetPte4K (
>}
> 
>if (Pte4K->Uint64 != LocalPte4K.Uint64) {
> -Pte4K->Uint64 = LocalPte4K.Uint64;
> +*(volatile UINT64 *)&(Pte4K->Uint64) = LocalPte4K.Uint64;
>}
>  }
> 
> @@ -100,7 +100,7 @@ PageTableLibSetPleB (
>IN IA32_MAP_ATTRIBUTE *Mask
>)
>  {
> -  IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  LocalPleB;
> +  volatile IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  LocalPleB;
> 
>LocalPleB.Uint64 = PleB->Uint64;
>if (Mask->Bits.PageTableBaseAddressLow || 
> Mask->Bits.PageTableBaseAddressHigh) {
> @@ -154,7 +154,7 @@ PageTableLibSetPleB (
>}
> 
>if (PleB->Uint64 != LocalPleB.Uint64) {
> -PleB->Uint64 = LocalPleB.Uint64;
> +*(volatile UINT64 *)&(PleB->Uint64) = LocalPleB.Uint64;
>}
>  }
> 
> @@ -200,7 +200,7 @@ PageTableLibSetPnle (
>IN IA32_MAP_ATTRIBUTE*Mask
>)
>  {
> -  IA32_PAGE_NON_LEAF_ENTRY  LocalPnle;
> +  volatile IA32_PAGE_NON_LEAF_ENTRY  LocalPnle;
> 
>LocalPnle.Uint64 = Pnle->Uint64;
>if (Mask->Bits.Present) {
> @@ -231,7 +231,7 @@ PageTableLibSetPnle (
>LocalPnle.Bits.WriteThrough  = 0;
>LocalPnle.Bits.CacheDisabled = 0;
>if (Pnle->Uint64 != LocalPnle.Uint64) {
> -Pnle->Uint64 = LocalPnle.Uint64;
> +*(volatile UINT64 *)&(Pnle->Uint64) = LocalPnle.Uint64;
>}
>  }

I agree with the idea (I think it's a necessary change, or put
differently, an improvement, even though I may not be convinced that it
is a *sufficient* improvement; but let's not rehash all that here
again); however, I think the implementation is not the greatest.

Volatile-qualifying the local variables does not seem useful for
anything. It's fine -- actually: it's beneficial -- if the compiler
optimizes accesses to those locals -- being on the stack -- as heavily
as it can. In other words, those parts of the patch look like a small
performance regression.

(2) What we want to qualify as volatile here are the *targets* of the
Pte4K, PleB and Pnle pointers. Your other patch ("UefiCpuPkg: Fix IN OUT
parameters marked as IN") correctly marks those as "IN OUT", so in this
patch, we should update them to:

  IN OUT volatile IA32_PAGE_NON_LEAF_ENTRY  *Pnle

and similar. Then the existent assignment expressions

  Pnle->Uint64 = LocalPnle.Uint64;

don't have to be changed.

Note that call sites will not have to be updated either; see C99 6.3.2.3
Pointers, paragraph 2:

For any qualifier q, a pointer to a non-q-qualified type may be
converted to a pointer to the q-qualified version of the type; the
values stored in the original and converted pointers shall compare
equal.

and 6.7.3 Type qualifiers, p5-6:

If an attempt is made to modify an object defined with a
const-qualified type through use of an lvalue with
non-const-qualified type, the behavior is undefined. If an attempt
is made to refer to an object defined with a volatile-qualified type
through use of an lvalue with non-volatile-qualified type, the
behavior is undefined. 115)

An object that has volatile-qualified type may be modified in ways
unknown to the implementation or have other unknown side effects.
Therefore any expression referring to such an object shall be
evaluated strictly according to the rules of the abstract machine,
as described in 5.1.2.3. Furthermore, at every sequence point the
value last stored in the object shall agree with that prescribed by
the abstract machine, except as modified by the unknown factors
mentioned previously. 116) What constitutes an access to an object
that has volatile-qualified type is implementation-defined.

Footnotes:

115) This applies to those objects that behave as if they were defined
 with qualified types, even if they are never actually defined as
 objects in the program (such as an object at a 

Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix IN OUT parameters marked as IN

[Laszlo Ersek]

On 2/21/24 21:06, Laszlo Ersek wrote:
> On 2/21/24 06:46, Ni, Ray wrote:
>> Reviewed-by: Ray Ni 
> 
> Thank you Ray for reviewing this; I'm happy if this goes in with your
> review.

small suggestion: I think we could improve the subject line as follows:

  UefiCpuPkg/CpuPageTableLib: Fix IN OUT parameters marked as IN

Thanks
Laszlo

>> Thanks,
>> Ray
>>> -Original Message-
>>> From: Zhou, Jianfeng 
>>> Sent: Wednesday, February 21, 2024 9:25 AM
>>> To: devel@edk2.groups.io
>>> Cc: Zhou, Jianfeng ; Ni, Ray ;
>>> Laszlo Ersek ; Kumar, Rahul R
>>> ; Gerd Hoffmann 
>>> Subject: [PATCH] UefiCpuPkg: Fix IN OUT parameters marked as IN
>>>
>>> Some IN OUT parameters in CpuPageTableMap.c were mistakenly marked as
>>> IN.
>>> "IN" replaced with "IN OUT" in the following interfaces:
>>>
>>> PageTableLibSetPte4K(): Pte4K
>>> PageTableLibSetPleB():  PleB
>>> PageTableLibSetPle():   Ple
>>> PageTableLibSetPnle():  Pnle
>>>
>>> Signed-off-by: Zhou Jianfeng 
>>> Cc: Ray Ni 
>>> Cc: Laszlo Ersek 
>>> Cc: Rahul Kumar 
>>> Cc: Gerd Hoffmann 
>>> ---
>>>  .../Library/CpuPageTableLib/CpuPageTableMap.c | 32 +--
>>>  1 file changed, 16 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
>>> b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
>>> index ae4caf8dfe..2ea40666cc 100644
>>> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
>>> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
>>> @@ -20,10 +20,10 @@
>>>  **/
>>>
>>>  VOID
>>>
>>>  PageTableLibSetPte4K (
>>>
>>> -  IN IA32_PTE_4K *Pte4K,
>>>
>>> -  IN UINT64  Offset,
>>>
>>> -  IN IA32_MAP_ATTRIBUTE  *Attribute,
>>>
>>> -  IN IA32_MAP_ATTRIBUTE  *Mask
>>>
>>> +  IN OUT IA32_PTE_4K *Pte4K,
>>>
>>> +  IN UINT64  Offset,
>>>
>>> +  IN IA32_MAP_ATTRIBUTE  *Attribute,
>>>
>>> +  IN IA32_MAP_ATTRIBUTE  *Mask
>>>
>>>)
>>>
>>>  {
>>>
>>>IA32_PTE_4K  LocalPte4K;
>>>
>>> @@ -94,10 +94,10 @@ PageTableLibSetPte4K (
>>>  **/
>>>
>>>  VOID
>>>
>>>  PageTableLibSetPleB (
>>>
>>> -  IN IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  *PleB,
>>>
>>> -  IN UINT64 Offset,
>>>
>>> -  IN IA32_MAP_ATTRIBUTE *Attribute,
>>>
>>> -  IN IA32_MAP_ATTRIBUTE *Mask
>>>
>>> +  IN OUT IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  *PleB,
>>>
>>> +  IN UINT64 Offset,
>>>
>>> +  IN IA32_MAP_ATTRIBUTE *Attribute,
>>>
>>> +  IN IA32_MAP_ATTRIBUTE *Mask
>>>
>>>)
>>>
>>>  {
>>>
>>>IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  LocalPleB;
>>>
>>> @@ -171,11 +171,11 @@ PageTableLibSetPleB (
>>>  **/
>>>
>>>  VOID
>>>
>>>  PageTableLibSetPle (
>>>
>>> -  IN UINTN   Level,
>>>
>>> -  IN IA32_PAGING_ENTRY   *Ple,
>>>
>>> -  IN UINT64  Offset,
>>>
>>> -  IN IA32_MAP_ATTRIBUTE  *Attribute,
>>>
>>> -  IN IA32_MAP_ATTRIBUTE  *Mask
>>>
>>> +  IN UINTN   Level,
>>>
>>> +  IN OUT IA32_PAGING_ENTRY   *Ple,
>>>
>>> +  IN UINT64  Offset,
>>>
>>> +  IN IA32_MAP_ATTRIBUTE  *Attribute,
>>>
>>> +  IN IA32_MAP_ATTRIBUTE  *Mask
>>>
>>>)
>>>
>>>  {
>>>
>>>if (Level == 1) {
>>>
>>> @@ -195,9 +195,9 @@ PageTableLibSetPle (
>>>  **/
>>>
>>>  VOID
>>>
>>>  PageTableLibSetPnle (
>>>
>>> -  IN IA32_PAGE_NON_LEAF_ENTRY  *Pnle,
>>>
>>> -  IN IA32_MAP_ATTRIBUTE*Attribute,
>>>
>>> -  IN IA32_MAP_ATTRIBUTE*Mask
>>>
>>> +  IN OUT IA32_PAGE_NON_LEAF_ENTRY  *Pnle,
>>>
>>> +  IN IA32_MAP_ATTRIBUTE*Attribute,
>>>
>>> +  IN IA32_MAP_ATTRIBUTE*Mask
>>>
>>>)
>>>
>>>  {
>>>
>>>IA32_PAGE_NON_LEAF_ENTRY  LocalPnle;
>>>
>>> --
>>> 2.31.1.windows.1
>>
> 



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

Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix IN OUT parameters marked as IN

[Laszlo Ersek]

On 2/21/24 06:46, Ni, Ray wrote:
> Reviewed-by: Ray Ni 

Thank you Ray for reviewing this; I'm happy if this goes in with your
review.

Laszlo

> 
> Thanks,
> Ray
>> -Original Message-
>> From: Zhou, Jianfeng 
>> Sent: Wednesday, February 21, 2024 9:25 AM
>> To: devel@edk2.groups.io
>> Cc: Zhou, Jianfeng ; Ni, Ray ;
>> Laszlo Ersek ; Kumar, Rahul R
>> ; Gerd Hoffmann 
>> Subject: [PATCH] UefiCpuPkg: Fix IN OUT parameters marked as IN
>>
>> Some IN OUT parameters in CpuPageTableMap.c were mistakenly marked as
>> IN.
>> "IN" replaced with "IN OUT" in the following interfaces:
>>
>> PageTableLibSetPte4K(): Pte4K
>> PageTableLibSetPleB():  PleB
>> PageTableLibSetPle():   Ple
>> PageTableLibSetPnle():  Pnle
>>
>> Signed-off-by: Zhou Jianfeng 
>> Cc: Ray Ni 
>> Cc: Laszlo Ersek 
>> Cc: Rahul Kumar 
>> Cc: Gerd Hoffmann 
>> ---
>>  .../Library/CpuPageTableLib/CpuPageTableMap.c | 32 +--
>>  1 file changed, 16 insertions(+), 16 deletions(-)
>>
>> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
>> b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
>> index ae4caf8dfe..2ea40666cc 100644
>> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
>> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
>> @@ -20,10 +20,10 @@
>>  **/
>>
>>  VOID
>>
>>  PageTableLibSetPte4K (
>>
>> -  IN IA32_PTE_4K *Pte4K,
>>
>> -  IN UINT64  Offset,
>>
>> -  IN IA32_MAP_ATTRIBUTE  *Attribute,
>>
>> -  IN IA32_MAP_ATTRIBUTE  *Mask
>>
>> +  IN OUT IA32_PTE_4K *Pte4K,
>>
>> +  IN UINT64  Offset,
>>
>> +  IN IA32_MAP_ATTRIBUTE  *Attribute,
>>
>> +  IN IA32_MAP_ATTRIBUTE  *Mask
>>
>>)
>>
>>  {
>>
>>IA32_PTE_4K  LocalPte4K;
>>
>> @@ -94,10 +94,10 @@ PageTableLibSetPte4K (
>>  **/
>>
>>  VOID
>>
>>  PageTableLibSetPleB (
>>
>> -  IN IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  *PleB,
>>
>> -  IN UINT64 Offset,
>>
>> -  IN IA32_MAP_ATTRIBUTE *Attribute,
>>
>> -  IN IA32_MAP_ATTRIBUTE *Mask
>>
>> +  IN OUT IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  *PleB,
>>
>> +  IN UINT64 Offset,
>>
>> +  IN IA32_MAP_ATTRIBUTE *Attribute,
>>
>> +  IN IA32_MAP_ATTRIBUTE *Mask
>>
>>)
>>
>>  {
>>
>>IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  LocalPleB;
>>
>> @@ -171,11 +171,11 @@ PageTableLibSetPleB (
>>  **/
>>
>>  VOID
>>
>>  PageTableLibSetPle (
>>
>> -  IN UINTN   Level,
>>
>> -  IN IA32_PAGING_ENTRY   *Ple,
>>
>> -  IN UINT64  Offset,
>>
>> -  IN IA32_MAP_ATTRIBUTE  *Attribute,
>>
>> -  IN IA32_MAP_ATTRIBUTE  *Mask
>>
>> +  IN UINTN   Level,
>>
>> +  IN OUT IA32_PAGING_ENTRY   *Ple,
>>
>> +  IN UINT64  Offset,
>>
>> +  IN IA32_MAP_ATTRIBUTE  *Attribute,
>>
>> +  IN IA32_MAP_ATTRIBUTE  *Mask
>>
>>)
>>
>>  {
>>
>>if (Level == 1) {
>>
>> @@ -195,9 +195,9 @@ PageTableLibSetPle (
>>  **/
>>
>>  VOID
>>
>>  PageTableLibSetPnle (
>>
>> -  IN IA32_PAGE_NON_LEAF_ENTRY  *Pnle,
>>
>> -  IN IA32_MAP_ATTRIBUTE*Attribute,
>>
>> -  IN IA32_MAP_ATTRIBUTE*Mask
>>
>> +  IN OUT IA32_PAGE_NON_LEAF_ENTRY  *Pnle,
>>
>> +  IN IA32_MAP_ATTRIBUTE*Attribute,
>>
>> +  IN IA32_MAP_ATTRIBUTE*Mask
>>
>>)
>>
>>  {
>>
>>IA32_PAGE_NON_LEAF_ENTRY  LocalPnle;
>>
>> --
>> 2.31.1.windows.1
> 



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

[edk2-devel] [edk2-redfish-client][PATCH v2 4/4] RedfishClientPkg: use Json value from a function argument

[Mike Maslenkin]

This patch replaces value Private->Json with Json used as second argument
for RedfishIdentifyResource(). Currently Json argument is not used at all
and the pattern for caller side is:
  Status = RedfishIdentifyResourceCommon (Private, Private->Json);

So in scope of RedfishIdentifyResourceCommon Json actually is the same
value as Private->Json. Let's make code a bit cleaner.

Cc: Abner Chang 
Cc: Nickle Wang 
Cc: Igor Kulchytskyy 
Signed-off-by: Mike Maslenkin 
Reviewed-by: Abner Chang 
Reviewed-by: Nickle Wang 
---
 RedfishClientPkg/Features/Bios/v1_0_9/Common/BiosCommon.c   | 2 +-
 .../Features/BootOption/v1_0_4/Common/BootOptionCommon.c| 2 +-
 .../ComputerSystem/v1_13_0/Common/ComputerSystemCommon.c| 2 +-
 .../ComputerSystem/v1_5_0/Common/ComputerSystemCommon.c | 2 +-
 RedfishClientPkg/Features/Memory/V1_7_1/Common/MemoryCommon.c   | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/RedfishClientPkg/Features/Bios/v1_0_9/Common/BiosCommon.c 
b/RedfishClientPkg/Features/Bios/v1_0_9/Common/BiosCommon.c
index 0ae841499692..f3f993c8782e 100644
--- a/RedfishClientPkg/Features/Bios/v1_0_9/Common/BiosCommon.c
+++ b/RedfishClientPkg/Features/Bios/v1_0_9/Common/BiosCommon.c
@@ -729,7 +729,7 @@ RedfishIdentifyResourceCommon (
   EFI_STRING   EndOfChar;
   REDFISH_FEATURE_ARRAY_TYPE_CONFIG_LANG_LIST  ConfigLangList;
 
-  Supported = RedfishIdentifyResource (Private->Uri, Private->Json);
+  Supported = RedfishIdentifyResource (Private->Uri, Json);
   if (Supported) {
 Status = RedfishFeatureGetUnifiedArrayTypeConfigureLang (RESOURCE_SCHEMA, 
RESOURCE_SCHEMA_VERSION, REDPATH_ARRAY_PATTERN, );
 if (EFI_ERROR (Status)) {
diff --git 
a/RedfishClientPkg/Features/BootOption/v1_0_4/Common/BootOptionCommon.c 
b/RedfishClientPkg/Features/BootOption/v1_0_4/Common/BootOptionCommon.c
index 0b9f2bf28434..f471c01c3790 100644
--- a/RedfishClientPkg/Features/BootOption/v1_0_4/Common/BootOptionCommon.c
+++ b/RedfishClientPkg/Features/BootOption/v1_0_4/Common/BootOptionCommon.c
@@ -791,7 +791,7 @@ RedfishIdentifyResourceCommon (
 {
   BOOLEAN  Supported;
 
-  Supported = RedfishIdentifyResource (Private->Uri, Private->Json);
+  Supported = RedfishIdentifyResource (Private->Uri, Json);
   if (Supported) {
 return EFI_SUCCESS;
   }
diff --git 
a/RedfishClientPkg/Features/ComputerSystem/v1_13_0/Common/ComputerSystemCommon.c
 
b/RedfishClientPkg/Features/ComputerSystem/v1_13_0/Common/ComputerSystemCommon.c
index cee6c8bf9ba1..d69fc176ad94 100644
--- 
a/RedfishClientPkg/Features/ComputerSystem/v1_13_0/Common/ComputerSystemCommon.c
+++ 
b/RedfishClientPkg/Features/ComputerSystem/v1_13_0/Common/ComputerSystemCommon.c
@@ -848,7 +848,7 @@ RedfishIdentifyResourceCommon (
   EFI_STRING   EndOfChar;
   REDFISH_FEATURE_ARRAY_TYPE_CONFIG_LANG_LIST  ConfigLangList;
 
-  Supported = RedfishIdentifyResource (Private->Uri, Private->Json);
+  Supported = RedfishIdentifyResource (Private->Uri, Json);
   if (Supported) {
 Status = RedfishFeatureGetUnifiedArrayTypeConfigureLang (RESOURCE_SCHEMA, 
RESOURCE_SCHEMA_VERSION, REDPATH_ARRAY_PATTERN, );
 if (EFI_ERROR (Status)) {
diff --git 
a/RedfishClientPkg/Features/ComputerSystem/v1_5_0/Common/ComputerSystemCommon.c 
b/RedfishClientPkg/Features/ComputerSystem/v1_5_0/Common/ComputerSystemCommon.c
index a67ef3dac283..11bcb5f76cab 100644
--- 
a/RedfishClientPkg/Features/ComputerSystem/v1_5_0/Common/ComputerSystemCommon.c
+++ 
b/RedfishClientPkg/Features/ComputerSystem/v1_5_0/Common/ComputerSystemCommon.c
@@ -1718,7 +1718,7 @@ RedfishIdentifyResourceCommon (
   EFI_STRING   EndOfChar;
   REDFISH_FEATURE_ARRAY_TYPE_CONFIG_LANG_LIST  ConfigLangList;
 
-  Supported = RedfishIdentifyResource (Private->Uri, Private->Json);
+  Supported = RedfishIdentifyResource (Private->Uri, Json);
   if (Supported) {
 Status = RedfishFeatureGetUnifiedArrayTypeConfigureLang (RESOURCE_SCHEMA, 
RESOURCE_SCHEMA_VERSION, REDPATH_ARRAY_PATTERN, );
 if (EFI_ERROR (Status)) {
diff --git a/RedfishClientPkg/Features/Memory/V1_7_1/Common/MemoryCommon.c 
b/RedfishClientPkg/Features/Memory/V1_7_1/Common/MemoryCommon.c
index eb52c68c5dcb..00a69f748c3c 100644
--- a/RedfishClientPkg/Features/Memory/V1_7_1/Common/MemoryCommon.c
+++ b/RedfishClientPkg/Features/Memory/V1_7_1/Common/MemoryCommon.c
@@ -2516,7 +2516,7 @@ RedfishIdentifyResourceCommon (
   EFI_STRING   EndOfChar;
   REDFISH_FEATURE_ARRAY_TYPE_CONFIG_LANG_LIST  ConfigLangList;
 
-  Supported = RedfishIdentifyResource (Private->Uri, Private->Json);
+  Supported = RedfishIdentifyResource (Private->Uri, Json);
   if (Supported) {
 Status = RedfishFeatureGetUnifiedArrayTypeConfigureLang (RESOURCE_SCHEMA, 
RESOURCE_SCHEMA_VERSION, REDPATH_ARRAY_PATTERN, );
 if (EFI_ERROR (Status)) {
-- 
2.32.0 (Apple Git-132)



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to 

[edk2-devel] [edk2-redfish-client][PATCH v2 3/4] RedfishClientPkg/Bios: fix leak of GetPendingSettings URI.

[Mike Maslenkin]

Cc: Abner Chang 
Cc: Nickle Wang 
Cc: Igor Kulchytskyy 
Signed-off-by: Mike Maslenkin 
Reviewed-by: Abner Chang 
Reviewed-by: Nickle Wang 
---
 .../Features/Bios/v1_0_9/Dxe/BiosDxe.c  | 17 +++--
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c 
b/RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c
index db77ed3dfccb..a442d446bc35 100644
--- a/RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c
+++ b/RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c
@@ -132,12 +132,13 @@ RedfishResourceConsumeResource (
   // Check and see if "@Redfish.Settings" exist or not.
   //
   ZeroMem (, sizeof (REDFISH_RESPONSE));
-  Status = GetPendingSettings (
- Private->RedfishService,
- Response.Payload,
- ,
- 
- );
+  PendingSettingUri = NULL;
+  Status= GetPendingSettings (
+Private->RedfishService,
+Response.Payload,
+,
+
+);
   if (!EFI_ERROR (Status)) {
 DEBUG ((REDFISH_DEBUG_TRACE, "%a: @Redfish.Settings found: %s\n", 
__func__, PendingSettingUri));
 Private->Uri = PendingSettingUri;
@@ -206,6 +207,10 @@ RedfishResourceConsumeResource (
 FreePool (Etag);
   }
 
+  if (PendingSettingUri != NULL) {
+FreePool (PendingSettingUri);
+  }
+
   return Status;
 }
 
-- 
2.32.0 (Apple Git-132)



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

[edk2-devel] [edk2-redfish-client][PATCH v2 1/4] RedfishClientPkg/RedfishFeatureUtilityLib: fix memory leak on error path

[Mike Maslenkin]

Cc: Abner Chang 
Cc: Igor Kulchytskyy 
Cc: Nickle Wang 
Signed-off-by: Mike Maslenkin 
Reviewed-by: Abner Chang 
Reviewed-by: Nickle Wang 
---
 .../Library/RedfishFeatureUtilityLib/RedfishFeatureUtilityLib.c  | 1 +
 1 file changed, 1 insertion(+)

diff --git 
a/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtilityLib.c 
b/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtilityLib.c
index e1494471038c..21ce8ddad9d5 100644
--- 
a/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtilityLib.c
+++ 
b/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtilityLib.c
@@ -4010,6 +4010,7 @@ RedfishRemoveUnchangeableProperties (
  (RedfishCS_uint32)AsciiStrSize (*JsonString)
  );
   if (Status != RedfishCS_status_success) {
+FreePool (UpdatedJsonString);
 return EFI_DEVICE_ERROR;
   }
 
-- 
2.32.0 (Apple Git-132)



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

[edk2-devel] [edk2-redfish-client][PATCH v2 2/4] RedfishClientPkg: refine RedfishExternalResourceResourceFeatureCallback

[Mike Maslenkin]

Use local variable for BiosUri passed to HandleResource() to avoid
problems in case of Private->Uri is overriden down the call stack.

Suggested-by: Nickle Wang 
Cc: Abner Chang 
Cc: Nickle Wang 
Cc: Igor Kulchytskyy 
Signed-off-by: Mike Maslenkin 
---
 RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c 
b/RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c
index f40f2d85af80..db77ed3dfccb 100644
--- a/RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c
+++ b/RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c
@@ -670,6 +670,7 @@ RedfishExternalResourceResourceFeatureCallback (
   REDFISH_SERVICE  RedfishService;
   REDFISH_RESOURCE_COMMON_PRIVATE  *Private;
   EFI_STRING   ResourceUri;
+  EFI_STRING   BiosUri;
 
   if (FeatureAction != CallbackActionStartOperation) {
 return EFI_UNSUPPORTED;
@@ -707,19 +708,19 @@ RedfishExternalResourceResourceFeatureCallback (
   //
   // Initialize collection path
   //
-  Private->Uri = RedfishGetUri (ResourceUri);
-  if (Private->Uri == NULL) {
+  BiosUri = RedfishGetUri (ResourceUri);
+  if (BiosUri == NULL) {
 ASSERT (FALSE);
 FreePool (ResourceUri);
 return EFI_OUT_OF_RESOURCES;
   }
 
-  Status = HandleResource (Private, Private->Uri);
+  Status = HandleResource (Private, BiosUri);
   if (EFI_ERROR (Status)) {
-DEBUG ((DEBUG_ERROR, "%a, process external resource: %a failed: %r\n", 
__func__, Private->Uri, Status));
+DEBUG ((DEBUG_ERROR, "%a, process external resource: %s failed: %r\n", 
__func__, BiosUri, Status));
   }
 
-  FreePool (Private->Uri);
+  FreePool (BiosUri);
   FreePool (ResourceUri);
   return Status;
 }
-- 
2.32.0 (Apple Git-132)



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

[edk2-devel] [edk2-redfish-client][PATCH v2 0/4] RedfishClientPkg: fix memory leaks and refine code

[Mike Maslenkin]

This set contains a trivial fix for a leak reviewed on Feb 1 [1] and a fix for 
а leak discussed in [2]

PR: https://github.com/tianocore/edk2-redfish-client/pull/76

[1] https://edk2.groups.io/g/devel/message/114925
[2] https://edk2.groups.io/g/devel/message/114765


v2:
in commit message put Cc back for the ones had given R-b
collected R-b
renamed CollectionUri to BiosUri

Signed-off-by: Mike Maslenkin 
Cc: Abner Chang 
Cc: Nickle Wang 
Cc: Igor Kulchytskyy 




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

Re: [edk2-devel] [PATCH] MdeModulePkg/PciBusDxe: plug device hierarchy leak upon bridge hot-unplug

[Laszlo Ersek]

On 2/20/24 20:48, Hsueh, Hong-Chih (Neo) wrote:
> [AMD Official Use Only - General]
> 
> 
> Hi Feng & Laszlo,
> 
> Thank you for the feedback, I have changed the title of this email and
> the title of the commit message of this patch.
> The new patch as attached. If this patch looks good to you, could you
> please help to add reviewed-by?

Please post the patch (with "v2" in the subject prefix) to the mailing
list, as a standalone thread-starter.

Thanks,
Laszlo


> 
> Thanks!
> 
> Regards,
> Neo
> 
> 
> *From:* Ding, Feng (Sunnyvale) 
> *Sent:* Thursday, February 8, 2024 5:09 PM
> *To:* Laszlo Ersek ; devel@edk2.groups.io
> ; Hsueh, Hong-Chih (Neo) 
> *Cc:* He, Jiangang ; Chang, Abner
> ; ray...@intel.com ;
> gaolim...@byosoft.com.cn ; Gopal, Pradeep
> 
> *Subject:* RE: [edk2-devel] [PATCH] MdeModulePkg/PciBusDxe: Fix hotplug
> functionality for USB4 bridge
>  
> [AMD Official Use Only - General]
> 
> Hi Laszlo,
> 
> " MdeModulePkg/PciBusDxe: plug device hierarchy leak upon bridge
> hot-unplug " is perfect description for the issue.
> "a root bridge" is "a (PCIe Hotplug) bridge", locating anywhere.
> 
> Thanks
> feng
> 
> -Original Message-
> From: Laszlo Ersek 
> Sent: Wednesday, February 7, 2024 12:51 PM
> To: devel@edk2.groups.io; Hsueh, Hong-Chih (Neo) 
> Cc: Ding, Feng (Sunnyvale) ; He, Jiangang
> ; Chang, Abner ;
> ray...@intel.com; gaolim...@byosoft.com.cn
> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PciBusDxe: Fix hotplug
> functionality for USB4 bridge
> 
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
> 
> 
> On 2/6/24 23:34, Hsueh, Hong-Chih (Neo) via groups.io wrote:
>> A USB4 or TBT bridge can be plugged or unplugged on USB4 port. The actions 
>> require PciHotPlugRequestNotify to add a root bridge or remove a root bridge 
>> completely.
>> In the plug-unplug-plug scenerio, PciHotPlugRequestNotify will return with 
>> no-action on second plug because bridge tree shows configured.
>> Destroy Pci Device Tree in function PciHotPlugRequestNotify for unplug event 
>> to fix this issue.
>>
>> Cc: Feng Ding 
>> Cc: Jiangang He 
>> Signed-off-by: Neo Hsueh 
>> ---
>>  MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c 
>> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
>> index 3f8c6e6da7..2b7af60e0a 100644
>> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
>> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
>> @@ -2103,6 +2103,8 @@ PciHotPlugRequestNotify (
>>    }
>>  }
>>
>> +    DestroyPciDeviceTree (Bridge);
>> +
>>  //
>>  // End for
>>  //
> 
> I think the subject line is too specific. This patch appears to fix a
> general resource leak in the PCI hot-unplug functionality. Writing up
> the USB4 angle in the commit message is welcome in my opinion, but the
> subject should state something like:
> 
> MdeModulePkg/PciBusDxe: plug device hierarchy leak upon bridge hot-unplug
> 
> (And I think the bridge doesn't even have to be a *root* bridge for the
> leak to occur; is that right?)
> 
> Laszlo
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115733): https://edk2.groups.io/g/devel/message/115733
Mute This Topic: https://groups.io/mt/104474693/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/4] RedfishClientPkg: refine RedfishExternalResourceResourceFeatureCallback

[Mike Maslenkin]

On Wed, Feb 21, 2024 at 5:34 AM Chang, Abner  wrote:
>
> [AMD Official Use Only - General]
>
> > -Original Message-
> > From: Mike Maslenkin 
> > Sent: Wednesday, February 21, 2024 8:13 AM
> > To: devel@edk2.groups.io
> > Cc: Mike Maslenkin ; Nickle Wang
> > ; Chang, Abner ; Igor
> > Kulchytskyy 
> > Subject: [PATCH 2/4] RedfishClientPkg: refine
> > RedfishExternalResourceResourceFeatureCallback
> >
> > Caution: This message originated from an External Source. Use proper caution
> > when opening attachments, clicking links, or responding.
> >
> >
> > Use local variable for CollectionUri passed to HandleResource() to avoid
> > problems in case of Private->Uri is overriden down the call stack.
> >
> > Suggested-by: Nickle Wang 
> > Cc: Abner Chang 
> > Cc: Nickle Wang 
> > Cc: Igor Kulchytskyy 
> > Signed-off-by: Mike Maslenkin 
> > ---
> >  RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c | 11 ++-
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c
> > b/RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c
> > index f40f2d85af80..396ec22969b5 100644
> > --- a/RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c
> > +++ b/RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c
> > @@ -670,6 +670,7 @@ RedfishExternalResourceResourceFeatureCallback (
> >REDFISH_SERVICE  RedfishService;
> >
> >REDFISH_RESOURCE_COMMON_PRIVATE  *Private;
> >
> >EFI_STRING   ResourceUri;
> >
> > +  EFI_STRING   CollectionUri;
> >
> >
> >
> >if (FeatureAction != CallbackActionStartOperation) {
> >
> >  return EFI_UNSUPPORTED;
> >
> > @@ -707,19 +708,19 @@ RedfishExternalResourceResourceFeatureCallback
> > (
> >//
> >
> >// Initialize collection path
> >
> >//
> >
> > -  Private->Uri = RedfishGetUri (ResourceUri);
> >
> > -  if (Private->Uri == NULL) {
> >
> > +  CollectionUri = RedfishGetUri (ResourceUri);
>
> I would like to leave this to Nickle to review if there is any impacts of not 
> initializing Private->Uri. Is Private->Uri referred in the later process?
> Apart from above, the naming of CollectionUri is not proper as BiosDxe is not 
> a collection driver. I think the comment of " // Initialize collection path" 
> is a copy & paste error.
>
> Thanks
> Abner
>

Good question about Private->Uri.
It looks like it's safe not to initialize Private->Uri here, because
it is being initialized in every function of
EDKII_REDFISH_RESOURCE_CONFIG_PROTOCOL.
That was exactly my previous concern.  After this patch only callee
from EDKII_REDFISH_RESOURCE_CONFIG_PROTOCOL are dealing with
Private->Uri.
So it could be dropped in favor of additional parameter.

I mean currently EDKII_REDFISH_RESOURCE_CONFIG_PROTOCOL function
initializes Private->Uri explicitly, then it is used in function
implementing actual implementation of
EDKII_REDFISH_RESOURCE_CONFIG_PROTOCOL. For example:
RedfishResourceConsumeResource->RedfishConsumeResourceCommon,
RedfishResourceConsumeResource->RedfishConsumeResourceCommon,
RedfishResourceProvisioningResource->RedfishProvisioningResourceCommon->{ProvisioningBiosExistResource,ProvisioningBiosResources},
RedfishResourceCheck->RedfishCheckResourceCommon,
RedfishResourceUpdate->RedfishUpdateResourceCommon,
RedfishResourceIdentify->RedfishIdentifyResourceCommon.
So, in all this cases Private->Uri can be passed as a function
argument down to call stack.


And HandleResource() mentioned above calls functions from
EdkIIRedfishResourceConfigLib that in turn calls
EDKII_REDFISH_RESOURCE_CONFIG_PROTOCOL implementation, where
Private->Uri is being initialized.  See previous paragraph.

I don't think I missed anything.

Regards,
Mike.


> >
> > +  if (CollectionUri == NULL) {
> >
> >  ASSERT (FALSE);
> >
> >  FreePool (ResourceUri);
> >
> >  return EFI_OUT_OF_RESOURCES;
> >
> >}
> >
> >
> >
> > -  Status = HandleResource (Private, Private->Uri);
> >
> > +  Status = HandleResource (Private, CollectionUri);
> >
> >if (EFI_ERROR (Status)) {
> >
> > -DEBUG ((DEBUG_ERROR, "%a, process external resource: %a failed: %r\n",
> > __func__, Private->Uri, Status));
> >
> > +DEBUG ((DEBUG_ERROR, "%a, process external resource: %s failed: %r\n",
> > __func__, CollectionUri, Status));
> >
> >}
> >
> >
> >
> > -  FreePool (Private->Uri);
> >
> > +  FreePool (CollectionUri);
> >
> >FreePool (ResourceUri);
> >
> >return Status;
> >
> >  }
> >
> > --
> > 2.32.0 (Apple Git-132)
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115732): https://edk2.groups.io/g/devel/message/115732
Mute This Topic: https://groups.io/mt/104479572/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] MdeModulePkg: Load Serial driver earlier in DXE

[Borzeszkowski, Alan]

> It does not make sense to have a UEFI Driver active in early DXE because it 
> will not be connected yet and has dependencies on other UEFI drivers that 
> will not be connected yet.

With suggested change, we connect to this driver successfully in early DXE 
using ConnectController(). We did not observe any issues when Serial driver 
came online, debug messages are printed and console redirection works just fine.

> Did you consider the use of the SerialPortLib for early DXE that can use PCI 
> serial devices with PcdSerialPciDeviceInfo that can be used for DEBUG() 
> messages.

That's the opposite of what we are trying to accomplish, this way additional 
maintenance cost is required and on top of that, management of PCI device from 
library level is complicated (e.g., checking device state).

> The other option is to map the PCI UART into Report Status Code.

Could you elaborate on that?

Also, could you please explain why DXE drivers cannot use Driver Binding?

Regards,
Alan



-Original Message-
From: Kinney, Michael D  
Sent: Tuesday, February 20, 2024 6:12 PM
To: devel@edk2.groups.io; Borzeszkowski, Alan 
Cc: Albecki, Mateusz ; Gao, Zhichao 
; Ni, Ray ; Kinney, Michael D 

Subject: RE: [edk2-devel] [PATCH 1/1] MdeModulePkg: Load Serial driver earlier 
in DXE

This is a UEFI Driver that depends on the Driver Binding Protocol and use of 
ConnectController(). These drivers cannot be used until the BDS phase when the 
active consoles and boot devices are evaluated and the smallest set of drivers 
required to boot are connected.

It does not make sense to have a UEFI Driver active in early DXE because it 
will not be connected yet and has dependencies on other UEFI drivers that will 
not be connected yet.

Did you consider the use of the SerialPortLib for early DXE that can use PCI 
serial devices with PcdSerialPciDeviceInfo that can be used for DEBUG() 
messages.

The other option is to map the PCI UART into Report Status Code.

Best regards,

Mike

> -Original Message-
> From: devel@edk2.groups.io  On Behalf Of 
> Borzeszkowski, Alan
> Sent: Tuesday, February 20, 2024 4:11 AM
> To: devel@edk2.groups.io
> Cc: Albecki, Mateusz ; Gao, Zhichao 
> ; Ni, Ray ; Borzeszkowski, 
> Alan 
> Subject: [edk2-devel] [PATCH 1/1] MdeModulePkg: Load Serial driver 
> earlier in DXE
> 
> For the purpose of UEFI debug prints enablement in DXE phase, Serial 
> driver should load earlier. Separate .inf file is created in order to 
> make minimal changes to current implementation.
> 
> Signed-off-by: Alan Borzeszkowski 
> ---
>  .../PciSioSerialDxe/PciSioSerialDxeEarly.inf  | 80 
> +++
>  1 file changed, 80 insertions(+)
>  create mode 100644
> MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxeEarly.inf
> 
> diff --git
> a/MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxeEarly.inf
> b/MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxeEarly.inf
> new file mode 100644
> index 00..2ead654898
> --- /dev/null
> +++ b/MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxeEarly.inf
> @@ -0,0 +1,80 @@
> +## @file
> +# Serial driver for standard UARTS on a SIO chip or PCI/PCIE card.
> +#
> +# Produces the Serial I/O protocol for standard UARTS using Super I/O
> or PCI I/O.
> +# This version is used shortly after DXE Core is invoked # # 
> +Copyright (c) 2007 - 2018, Intel Corporation. All rights
> reserved.
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent # ##
> +
> +[Defines]
> +  INF_VERSION= 0x00010005
> +  BASE_NAME  = PciSioSerialDxeEarly
> +  MODULE_UNI_FILE= PciSioSerialDxe.uni
> +  FILE_GUID  = 8BCC425E-585F-4E66-ADA5-
> FEA9A635F911
> +  MODULE_TYPE= DXE_DRIVER
> +  VERSION_STRING = 1.0
> +  ENTRY_POINT= InitializePciSioSerial
> +
> +#
> +# The following information is for reference only and not required by
> the build tools.
> +#
> +#  VALID_ARCHITECTURES   = IA32 X64 EBC
> +#
> +#  DRIVER_BINDING=  gSerialControllerDriver
> +#  COMPONENT_NAME=  gPciSioSerialComponentName
> +#  COMPONENT_NAME2   =  gPciSioSerialComponentName2
> +#
> +
> +[Sources]
> +  ComponentName.c
> +  SerialIo.c
> +  SerialIoCommon.c
> +  Serial.h
> +  Serial.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +
> +[LibraryClasses]
> +  PcdLib
> +  ReportStatusCodeLib
> +  UefiBootServicesTableLib
> +  MemoryAllocationLib
> +  BaseMemoryLib
> +  DevicePathLib
> +  UefiLib
> +  UefiDriverEntryPoint
> +  DebugLib
> +  IoLib
> +
> +[Guids]
> +  gEfiUartDevicePathGuid## SOMETIMES_CONSUMES
> ## GUID
> +
> +[Protocols]
> +  gEfiSioProtocolGuid   ## TO_START
> +  gEfiDevicePathProtocolGuid## TO_START
> +  gEfiPciIoProtocolGuid ## TO_START
> +  gEfiSerialIoProtocolGuid  ## BY_START
> +  

Re: [edk2-devel] Merge commit in edk2-non-osi

[Michael D Kinney]

Thanks for the reminder.  "Require Linear History" was not set
in edk2-non-osi.  It is now.

Mike

> -Original Message-
> From: Marcin Juszkiewicz 
> Sent: Wednesday, February 21, 2024 12:58 AM
> To: devel@edk2.groups.io; Desimone, Nathaniel L
> 
> Cc: Kinney, Michael D 
> Subject: Re: [edk2-devel] Merge commit in edk2-non-osi
> 
> W dniu 21.02.2024 o 2:49 AM, Nate DeSimone pisze:
> 
> > I would like to remind everyone that we generally don't accept
> > submissions via PRs yet. At the very least please click "Rebase and
> > merge" when closing the PR instead of "Merge pull request". Since
> that
> > merge commit is currently at the top of the tree, can we delete it?
> 
> You can configure repository on github to not have merge commits on PR
> merge. Settings allow to choose and you can leave 'allow rebase
> merging'
> as the only option.
> 
> Pull requests functionality cannot be disabled but there are pages
> describing how to reply and close/reject automatically.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115730): https://edk2.groups.io/g/devel/message/115730
Mute This Topic: https://groups.io/mt/104481195/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/4] RedfishClientPkg: refine RedfishExternalResourceResourceFeatureCallback

[Chang, Abner via groups.io]

[AMD Official Use Only - General]

Hi Nickle,
I have no problem with either one, maybe BiosUri is a better choice.

Thanks
Abner

From: Nickle Wang 
Sent: Wednesday, February 21, 2024 3:30 PM
To: Mike Maslenkin ; devel@edk2.groups.io; Chang, 
Abner 
Cc: Igor Kulchytskyy 
Subject: RE: [PATCH 2/4] RedfishClientPkg: refine 
RedfishExternalResourceResourceFeatureCallback

Caution: This message originated from an External Source. Use proper caution 
when opening attachments, clicking links, or responding.


Hi Mike,



Thanks for incorporating my suggestion to address memory issue. For the name 
"CollectionUri", I know this is from my suggestion, but I think Abner is right. 
Could you please change it to "BiosUri" or "ResourceUri" since BIOS resource is 
not a Redfish collection?



@Abner Chang please comment here if you have other 
naming preference.



Thanks,

Nickle



> -Original Message-

> From: Mike Maslenkin 
> mailto:mike.maslen...@gmail.com>>

> Sent: Wednesday, February 21, 2024 8:13 AM

> To: devel@edk2.groups.io

> Cc: Mike Maslenkin 
> mailto:mike.maslen...@gmail.com>>; Nickle Wang

> mailto:nick...@nvidia.com>>; Abner Chang 
> mailto:abner.ch...@amd.com>>; Igor Kulchytskyy

> mailto:ig...@ami.com>>

> Subject: [PATCH 2/4] RedfishClientPkg: refine

> RedfishExternalResourceResourceFeatureCallback

>

> External email: Use caution opening links or attachments

>

>

> Use local variable for CollectionUri passed to HandleResource() to avoid 
> problems

> in case of Private->Uri is overriden down the call stack.

>

> Suggested-by: Nickle Wang mailto:nick...@nvidia.com>>

> Cc: Abner Chang mailto:abner.ch...@amd.com>>

> Cc: Nickle Wang mailto:nick...@nvidia.com>>

> Cc: Igor Kulchytskyy mailto:ig...@ami.com>>

> Signed-off-by: Mike Maslenkin 
> mailto:mike.maslen...@gmail.com>>

> ---

>  RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c | 11 ++-

>  1 file changed, 6 insertions(+), 5 deletions(-)

>

> diff --git a/RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c

> b/RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c

> index f40f2d85af80..396ec22969b5 100644

> --- a/RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c

> +++ b/RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c

> @@ -670,6 +670,7 @@ RedfishExternalResourceResourceFeatureCallback (

>REDFISH_SERVICE  RedfishService;

>

>REDFISH_RESOURCE_COMMON_PRIVATE  *Private;

>

>EFI_STRING   ResourceUri;

>

> +  EFI_STRING   CollectionUri;

>

>

>

>if (FeatureAction != CallbackActionStartOperation) {

>

>  return EFI_UNSUPPORTED;

>

> @@ -707,19 +708,19 @@ RedfishExternalResourceResourceFeatureCallback (

>//

>

>// Initialize collection path

>

>//

>

> -  Private->Uri = RedfishGetUri (ResourceUri);

>

> -  if (Private->Uri == NULL) {

>

> +  CollectionUri = RedfishGetUri (ResourceUri);

>

> +  if (CollectionUri == NULL) {

>

>  ASSERT (FALSE);

>

>  FreePool (ResourceUri);

>

>  return EFI_OUT_OF_RESOURCES;

>

>}

>

>

>

> -  Status = HandleResource (Private, Private->Uri);

>

> +  Status = HandleResource (Private, CollectionUri);

>

>if (EFI_ERROR (Status)) {

>

> -DEBUG ((DEBUG_ERROR, "%a, process external resource: %a failed: %r\n",

> __func__, Private->Uri, Status));

>

> +DEBUG ((DEBUG_ERROR, "%a, process external resource: %s failed:

> + %r\n", __func__, CollectionUri, Status));

>

>}

>

>

>

> -  FreePool (Private->Uri);

>

> +  FreePool (CollectionUri);

>

>FreePool (ResourceUri);

>

>return Status;

>

>  }

>

> --

> 2.32.0 (Apple Git-132)




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

Re: [edk2-devel] Merge commit in edk2-non-osi

[Michael D Kinney]

Hi Ard,

I disagree.  We have never allowed a force push to the main
branch of TianoCore repos.  This has happened before and
was discussed and the policy is to not fix.  Even the edk2
repo has some merge commits in its history that were discussed
and not fixed.

We can never know how many downstream consumers are syncing
with main branches.

Mike

> -Original Message-
> From: Ard Biesheuvel 
> Sent: Wednesday, February 21, 2024 12:07 AM
> To: devel@edk2.groups.io; Desimone, Nathaniel L
> 
> Cc: Kinney, Michael D 
> Subject: Re: [edk2-devel] Merge commit in edk2-non-osi
> 
> On Wed, 21 Feb 2024 at 02:49, Nate DeSimone
>  wrote:
> >
> > Hi Everyone,
> >
> > It appears that a merge commit was introduced to edk2-non-osi due to
> a PR merge:
> >
> > https://github.com/tianocore/edk2-non-
> osi/commit/61b65fccfe4c75bc9ecb7b542412a436e3db5de6
> >
> > I would like to remind everyone that we generally don't accept
> submissions via PRs yet. At the very least please click "Rebase and
> merge" when closing the PR instead of "Merge pull request". Since that
> merge commit is currently at the top of the tree, can we delete it?
> >
> 
> 
> I think force rebasing is fine in this particular case - the file
> contents will remain the same, it is just the git history that gets
> linearized, so everyone that pulls from it should get the expected
> results.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115728): https://edk2.groups.io/g/devel/message/115728
Mute This Topic: https://groups.io/mt/104481195/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 0/3] Fix Runtime Granularity Issues

[Ard Biesheuvel]

On Sat, 17 Feb 2024 at 02:27, Oliver Smith-Denny
 wrote:
>
> This patch series is the second version of
> MdeModulePkg: DxeCore: Don't Guard Large Runtime Granularity Allocations.
> The subject line has been updated because this went from a one commit
> patch with no cover letter to a multi-commit patch.
>
> The commit messages cover the vast amount of detail here, but this
> patchset fixes three issues:
> - a UEFI spec violation for which memory types require runtime page
> allocation granularity alignment
> - An incompatibility of the heap guard system to guard these regions
> that require runtime page allocation granularities greater than
> the EFI_PAGE_SIZE.
> - A CodeQL error that fails CI when updating the Page.c code
>
> v2:
> - Add commit to fix UEFI spec violation
> - Add commit to fix newly flagged CodeQL error
> - Update guard commit message, comments, and static assert to use
> the correct types
>
> Cc: Leif Lindholm 
> Cc: Ard Biesheuvel 
> Cc: Sami Mujawar 
> Cc: Liming Gao 
>
> Oliver Smith-Denny (3):
>   MdeModulePkg: DxeCore: Fix CodeQL Error in FreePages
>   MdeModulePkg: DxeCore: Correct Runtime Granularity Memory Type
>   MdeModulePkg: DxeCore: Do Not Apply Guards to Unsupported Types
>

Reviewed-by: Ard Biesheuvel 


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

Re: [edk2-devel] [PATCH v3 0/6] OvmfPkg: Add support for 5-level paging

[Ard Biesheuvel]

On Tue, 20 Feb 2024 at 10:06, Gerd Hoffmann  wrote:
>
> Patch #1 + #2 fix MdeModulePkg/DxeIplPeim to not assert in case a
> 5-level enabled build runs in 4-level paging mode.
>
> Patch #2 - #4 update OvmfPkg ResetVector, adding support for 5-level
> paging (setup 5-level page tables in case both la57 and gigabyte pages
> are supported by the vCPU).
>
> Patch #5 updates PlatformInitLib for 5-level paging support (update
> PhysBits calculation).
>
> Known issues / limitations:
>  - BaseMemEncryptSevLib must be updated to also support 5-level
>paging for full 5-level paging support in SEV mode.
>
> The patch series does *not* enable 5-level paging by default.
> Building with 5-level paging support can be done by compiling
> OVMF with '--pcd PcdUse5LevelPageTable=TRUE'.
>
> v3:
>  - add resetvector fixes for sev and tdx
> v2 changes:
>  - fix sev/tdx handling with 5-level paging.
>  - more comments for 5-level page table setup.
>  - improve PAGE_* naming (new patch #3).
>  - rename Page5LevelSupported to Page5LevelEnabled (new patch #2).
>  - pick up some review tags.
>
> Gerd Hoffmann (6):
>   MdeModulePkg/DxeIplPeim: fix PcdUse5LevelPageTable assert
>   MdeModulePkg/DxeIplPeim: rename variable
>   OvmfPkg/ResetVector: improve page table flag names
>   OvmfPkg/ResetVector: SEV: keep #vc handler installed longer
>   OvmfPkg/ResetVector: add 5-level paging support
>   OvmfPkg/PlatformInitLib: add 5-level paging support
>

I'm fine with all this but I haven't looked in great detail, so

Acked-by: Ard Biesheuvel 


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

Re: [edk2-devel] [PATCH v3 1/1] SbsaQemu: add memory space for the high memory nodes

[Marcin Juszkiewicz]

W dniu 20.02.2024 o 8:33 AM, Xiong Yining pisze:

To support more memory nodes, we refer to the implement of
"OvmfPkg/Fdt/HighMemDxe" to add memory space for the high memory nodes
except the first one.

Signed-off-by: Xiong Yining
Signed-off-by: Chen Baozi


Tested-by: Marcin Juszkiewicz 

EDK2 reported memory from both NUMA nodes, Linux got whole memory too 
(with SRAT table patch).




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115725): https://edk2.groups.io/g/devel/message/115725
Mute This Topic: https://groups.io/mt/104463710/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] SbsaQemu: AcpiDxe: Create SRAT table at runtime

[Marcin Juszkiewicz]

W dniu 20.02.2024 o 9:03 AM, Xiong Yining pisze:
This is beacuse UEFI only allocates the first memory node memory space 
for SbsaQemu platform,  i refer to implemet of "OvmfPkg/Fdt/HighMemDxe" 
and add the support for other memory nodes via GCD services. Maybe you 
can apply patch "support multi memory nodes" together with this patch.


Collected patches:

6ea06a5ae4c9 (tag: multi-node-v3) SbsaQemu: add memory space for the high 
memory nodes
6cad2691e06e (tag: srat-v2) SbsaQemu: AcpiDxe: Create SRAT table at runtime
86c5fc908bd4 Platform/SbsaQemu: add DeviceTree fallbacks to parse memory 
information
a2e8ffb1e046 Platform/SbsaQemu: get the information of memory via SMC calls
6007fcaae876 Platform/SbsaQemu: hang if there is no cpu information
dc9360e2e2c8 Platform/SbsaQemu: move FdtHandlerLib to SbsaQemuHardwareInfoLib
5a0a7fa00139 Platform/SbsaQemu: use PcdCoreCount directly
9bebc3a2a7b9 Platform/SbsaQemu: read amount of cpus during init
e7ec1d2d346b (tag: nodt-v5) Platform/SbsaQemu: add SbsaQemuHardwareInfoLib

And effect is nice.

QEMU args:

-smp 4,sockets=4,maxcpus=4
-m 4G,slots=2,maxmem=5G
-object memory-backend-ram,size=1G,id=m0
-object memory-backend-ram,size=3G,id=m1
-numa node,nodeid=0,cpus=0-1,memdev=m0
-numa node,nodeid=1,cpus=2,memdev=m1
-numa node,nodeid=2,cpus=3

EDK2 reports 4GB ram, Linux gets 4GB ram too.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115724): https://edk2.groups.io/g/devel/message/115724
Mute This Topic: https://groups.io/mt/104074353/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 5/5] UefiCpuPkg/MpInitLib: Add support for multiple HOBs to SaveCpuMpData()

[Gerd Hoffmann]

On Wed, Feb 21, 2024 at 03:48:25AM +, Ni, Ray wrote:
> > 
> > +  MaxCpusPerHob = (MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE) - sizeof
> > (MP_HAND_OFF)) / sizeof (PROCESSOR_HAND_OFF);
> 
> Above formula assumes the maximum HOB length could be 0x.

Which is IMHO correct.

> But actually the maximum HOB length could be only 0xFFF8 because
> PeiCore::PeiCreateHob() contains following logic:
> 
>   if (0x1 - Length <= 0x7) {
> return EFI_INVALID_PARAMETER;
>   }

That Length is the *data* size, the HOB header is not included.

The "- sizeof (EFI_HOB_GUID_TYPE)" in the formula above accounts the
space needed for HOB header and GUID.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115723): https://edk2.groups.io/g/devel/message/115723
Mute This Topic: https://groups.io/mt/104472313/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] UefiPayloadPkg: Make UPL build script arch agnostic

[Dhaval Sharma]

Current implementation makes assumptions about arch it will be built
for. Need to make it more generic to add follow up support for RISCV.
Right now it does not build for RV until relevant dsc file is available.

Cc: Guo Dong 
Cc: Sean Rhodes 
Cc: James Lu 
Cc: Gua Guo 
Signed-off-by: Dhaval Sharma 
---
 UefiPayloadPkg/UefiPayloadPkg.dsc   |  2 +-
 UefiPayloadPkg/Tools/MkFitImage.py  | 18 +++
 UefiPayloadPkg/UniversalPayloadBuild.py | 23 +---
 3 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/UefiPayloadPkg/UefiPayloadPkg.dsc 
b/UefiPayloadPkg/UefiPayloadPkg.dsc
index 0e142bb7c2a2..abe3d3c3d870 100644
--- a/UefiPayloadPkg/UefiPayloadPkg.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkg.dsc
@@ -22,7 +22,7 @@ [Defines]
   SUPPORTED_ARCHITECTURES = IA32|X64
   BUILD_TARGETS   = DEBUG|RELEASE|NOOPT
   SKUID_IDENTIFIER= DEFAULT
-  OUTPUT_DIRECTORY= Build/UefiPayloadPkgX64
+  OUTPUT_DIRECTORY= Build/UefiPayloadPkg
   FLASH_DEFINITION= UefiPayloadPkg/UefiPayloadPkg.fdf
   PCD_DYNAMIC_AS_DYNAMICEX= TRUE
 
diff --git a/UefiPayloadPkg/Tools/MkFitImage.py 
b/UefiPayloadPkg/Tools/MkFitImage.py
index 41a259960b2b..b76c2156dd18 100644
--- a/UefiPayloadPkg/Tools/MkFitImage.py
+++ b/UefiPayloadPkg/Tools/MkFitImage.py
@@ -59,16 +59,16 @@ def BuildConfNode(Fdt, ParentNode, MultiImage):
 libfdt.fdt_setprop(Fdt, ConfNode1, 'require-fit', b'', 0)
 libfdt.fdt_setprop(Fdt, ConfNode1, 'firmware', bytes('tianocore', 
'utf-8'), len('tianocore') + 1)
 
-def BuildFvImageNode(Fdt, InfoHeader, ParentNode, DataOffset, DataSize, 
Description):
+def BuildFvImageNode(Fdt, InfoHeader, ParentNode, DataOffset, DataSize, 
Description, Arch):
 libfdt.fdt_setprop_u32(Fdt, ParentNode, 'data-size', DataSize)
 libfdt.fdt_setprop_u32(Fdt, ParentNode, 'data-offset', DataOffset)
 libfdt.fdt_setprop(Fdt, ParentNode, 'compression', bytes('none',   
 'utf-8'), len('none') + 1)
 libfdt.fdt_setprop(Fdt, ParentNode, 'project ',bytes('tianocore',  
 'utf-8'), len('tianocore') + 1)
-libfdt.fdt_setprop(Fdt, ParentNode, 'arch',bytes('x86_64', 
 'utf-8'), len('x86_64') + 1)
+libfdt.fdt_setprop(Fdt, ParentNode, 'arch',bytes(Arch, 
 'utf-8'), len(Arch) + 1)
 libfdt.fdt_setprop(Fdt, ParentNode, 'type',bytes('flat-binary',
 'utf-8'), len('flat-binary') + 1)
 libfdt.fdt_setprop(Fdt, ParentNode, 'description', bytes(Description,  
 'utf-8'), len(Description) + 1)
 
-def BuildTianoImageNode(Fdt, InfoHeader, ParentNode, DataOffset, DataSize, 
Description):
+def BuildTianoImageNode(Fdt, InfoHeader, ParentNode, DataOffset, DataSize, 
Description, Arch):
 #
 # Set 'load' and 'data-offset' to reserve the memory first.
 # They would be set again when Fdt completes or this function parses 
target binary file.
@@ -100,7 +100,7 @@ def BuildTianoImageNode(Fdt, InfoHeader, ParentNode, 
DataOffset, DataSize, Descr
 #
 # The subnode would be inserted from bottom to top of structure block.
 #
-def BuildFitImage(Fdt, InfoHeader):
+def BuildFitImage(Fdt, InfoHeader, Arch):
 MultiImage = [
 ["tianocore",   InfoHeader.Binary,BuildTianoImageNode , 
InfoHeader.Description, None, 0 ],
 ["uefi-fv", InfoHeader.UefifvPath,BuildFvImageNode, "UEFI 
Firmware Volume", None, 0 ],
@@ -143,7 +143,7 @@ def BuildFitImage(Fdt, InfoHeader):
 if os.path.exists (Item[1]) == False:
 continue
 FvNode = libfdt.fdt_add_subnode(Fdt, ImageNode, Name)
-BuildFvNode (Fdt, InfoHeader, FvNode, DataOffset, len(BinaryData), 
Description)
+BuildFvNode (Fdt, InfoHeader, FvNode, DataOffset, len(BinaryData), 
Description, Arch)
 
 #
 # Create new image file and combine all binary.
@@ -160,7 +160,7 @@ def BuildFitImage(Fdt, InfoHeader):
 
 return True
 
-def MakeFitImage(InfoHeader):
+def MakeFitImage(InfoHeader, Arch):
 #
 # Allocate fdt byte array.
 #
@@ -175,9 +175,9 @@ def MakeFitImage(InfoHeader):
 #
 # Parse args to build fit image.
 #
-return BuildFitImage(Fdt, InfoHeader)
+return BuildFitImage(Fdt, InfoHeader, Arch)
 
-def ReplaceFv (UplBinary, SectionFvFile, SectionName):
+def ReplaceFv (UplBinary, SectionFvFile, SectionName, Arch):
 try:
 #
 # Get Original Multi Fv
@@ -231,7 +231,7 @@ def ReplaceFv (UplBinary, SectionFvFile, SectionName):
 SectionFvFileBinary = File.read ()
 MultiFvList.append ([SectionName, SectionFvFileBinary])
 FvNode = libfdt.fdt_add_subnode(NewFitHeader, ImagesNode, 
SectionName)
-BuildFvImageNode (NewFitHeader, None, FvNode, FitSize, 
len(SectionFvFileBinary), SectionName + " Firmware Volume")
+BuildFvImageNode (NewFitHeader, None, FvNode, FitSize, 

[edk2-devel] [PATCH v1 0/1] Make FIT building more generic

[Dhaval Sharma]

Current implementation makes assumptions about arch it will be built
for. Need to make it more generic to add follow up support for RISCV.
Right now it does not build for RV until relevant dsc file is available.
https://github.com/tianocore/edk2/pull/5395

Cc: Guo Dong 
Cc: Sean Rhodes 
Cc: James Lu 
Cc: Gua Guo 

Dhaval (1):
  UefiPayloadPkg: Make UPL build script arch agnostic

 UefiPayloadPkg/UefiPayloadPkg.dsc   |  2 +-
 UefiPayloadPkg/Tools/MkFitImage.py  | 18 +++
 UefiPayloadPkg/UniversalPayloadBuild.py | 23 +---
 3 files changed, 25 insertions(+), 18 deletions(-)

-- 
2.39.2



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

Re: [edk2-devel] Merge commit in edk2-non-osi

[Marcin Juszkiewicz]

W dniu 21.02.2024 o 2:49 AM, Nate DeSimone pisze:


I would like to remind everyone that we generally don't accept
submissions via PRs yet. At the very least please click "Rebase and
merge" when closing the PR instead of "Merge pull request". Since that
merge commit is currently at the top of the tree, can we delete it?


You can configure repository on github to not have merge commits on PR 
merge. Settings allow to choose and you can leave 'allow rebase merging' 
as the only option.


Pull requests functionality cannot be disabled but there are pages 
describing how to reply and close/reject automatically.



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

Re: [edk2-devel] Merge commit in edk2-non-osi

[Ard Biesheuvel]

On Wed, 21 Feb 2024 at 02:49, Nate DeSimone
 wrote:
>
> Hi Everyone,
>
> It appears that a merge commit was introduced to edk2-non-osi due to a PR 
> merge:
>
> https://github.com/tianocore/edk2-non-osi/commit/61b65fccfe4c75bc9ecb7b542412a436e3db5de6
>
> I would like to remind everyone that we generally don't accept submissions 
> via PRs yet. At the very least please click "Rebase and merge" when closing 
> the PR instead of "Merge pull request". Since that merge commit is currently 
> at the top of the tree, can we delete it?
>


I think force rebasing is fine in this particular case - the file
contents will remain the same, it is just the git history that gets
linearized, so everyone that pulls from it should get the expected
results.


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