Re: [edk2-devel] Moving edk2-platforms reviews to GitHub Pull Requests

2024-07-03 Thread Chang, Abner via groups.io 
[AMD Official Use Only - AMD Internal Distribution Only]

Hi Rebecca,
Having GitHub CODEOWNERS to assign reviewers for the PR is fine to me, as well 
as only maintainers have the privilege to set "Push" label. I would suggest we 
use only one CODEOWNERS file on the repo so we can easily find the code owner 
from that file.
I think community should be able to create a PR for updating the CODEOWNERS 
under any folder on the repo, right?  But how can we determine the role of 
maintainers and reviewers in CODEOWNERS? As only maintainers can set "push" 
label. Also, how can we identify the newly added code owner has the "push" 
privilege in the PR against the CODEOWNERS file.

Thanks
Abner

> -Original Message-
> From: Rebecca Cran 
> Sent: Wednesday, July 3, 2024 10:20 PM
> To: devel@edk2.groups.io; rebe...@bsdio.com
> Cc: Leif Lindholm ; Michael D Kinney
> ; Ard Biesheuvel ;
> Chang, Abner ; Attar, AbdulLateef (Abdul Lateef)
> ; Grimes, Paul ; Xing,
> Eric ; Yao, Ken ; Zhai, MingXin
> (Duke) ; Fu, Igniculus ; Nhi
> Pham ; Chuong Tran
> ; Thomas Abraham
> ; Sami Mujawar ;
> Ray Ni ; Ilias Apalodimas ;
> Andy Hayes ; Wenyi Xie
> ; Pedro Falcato ; Marvin
> Häuser ; Sai Chaganty
> ; Nate DeSimone
> ; Liming Gao ;
> Eric Dong ; Dandan Bi ; Nickle
> Wang ; Kelly Steele ; Zailiang
> Sun ; Yi Qian ; Chasel Chiu
> ; Benjamin Doron ;
> Jeremy Soller ; Daniel Schaefer
> ; Sunil V L ; Yuwei Chen
> ; Jeremy Linton ; Marcin
> Juszkiewicz ; Graeme Gregory
> 
> Subject: Re: [edk2-devel] Moving edk2-platforms reviews to GitHub Pull
> Requests
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On 7/1/24 13:16, Rebecca Cran via groups.io wrote:
> > Now that edk2 has been using PRs for a few weeks, I'd like to propose
> > enabling the same workflow for edk2-platfoms.
> >
> > As maintainers or reviewers of platforms in the edk2-platforms repo, I'd
> > like to get any feedback on moving from email-based reviews to GitHub
> > Pull Requests and any concerns or issues people might have with it.
> As Leif pointed out, the other question is around assigning reviewers.
>
> Would people be okay with moving to CODEOWNERS as a method of
> assigning
> reviewers, moving away from Maintainers.txt?
> It's less flexible because it doesn't have the concept of maintainers vs
> reviewers, so the idea would be to add both to CODEOWNERS but only allow
> maintainers to set the 'push' label or otherwise write to the repository.
>
> --
> Rebecca Cran


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

Re: [edk2-devel] Moving edk2-platforms reviews to GitHub Pull Requests

2024-07-03 Thread Rebecca Cran 

On 7/1/24 13:16, Rebecca Cran via groups.io wrote:
Now that edk2 has been using PRs for a few weeks, I'd like to propose 
enabling the same workflow for edk2-platfoms.


As maintainers or reviewers of platforms in the edk2-platforms repo, I'd 
like to get any feedback on moving from email-based reviews to GitHub 
Pull Requests and any concerns or issues people might have with it.

As Leif pointed out, the other question is around assigning reviewers.

Would people be okay with moving to CODEOWNERS as a method of assigning 
reviewers, moving away from Maintainers.txt?
It's less flexible because it doesn't have the concept of maintainers vs 
reviewers, so the idea would be to add both to CODEOWNERS but only allow 
maintainers to set the 'push' label or otherwise write to the repository.


--
Rebecca Cran


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

[edk2-devel] Event: TianoCore edk2-test Bug Triage Meeting - Thursday, July 4, 2024 #cal-reminder

2024-07-03 Thread Group Notification 
*Reminder: TianoCore edk2-test Bug Triage Meeting*

*When:*
Thursday, July 4, 2024
10:00pm to 11:00pm
(UTC+08:00) Asia/Shanghai

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

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

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

*Description:*


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

[edk2-devel] [PATCH edk2-platforms v2] SbsaQemu: use FEAT_RNG for EFI_RNG_PROTOCOL

2024-07-03 Thread Marcin Juszkiewicz 
By default we have Neoverse-N2 cpu which supports FEAT_RNG feature. This
allows us to add RngDxe to have EFI_RNG_PROTOCOL available on
Neoverse-N2 and 'max' cpu cores.

Commit 5de5e230a80bed083360da95ba16a2c4a001620d (in EDK2) enabled that for
ArmVirt platform.

RNDR is implemented by both Neoverse-N2 and 'max' cpu implemented by QEMU.
Other cpu models lack it which prevents the RngDxe driver from running,
resulting in the same situation as before.

TRNG is not implemented in TCG mode but is required by RngDxe to run.

On older cpu cores nothing changes.

Signed-off-by: Marcin Juszkiewicz 
---
By default we have Neoverse-N2 cpu which supports FEAT_RNG feature. This
allows us to add RngDxe to have EFI_RNG_PROTOCOL available on
Neoverse-N2 and 'max' cpu cores.

When I boot with Neoverse-N2 or 'max' cpu then EFI_RNG_PROTOCOL gets
reported by 'EFI stub' on Linux boot and KASLR gets enabled.

Commit 5de5e230a80bed083360da95ba16a2c4a001620d (in EDK2) enabled that for
ArmVirt platform.

RNDR is implemented by both Neoverse-N2 and 'max' cpu implemented by QEMU.
Other cpu models lack it which prevents the RngDxe driver from running,
resulting in the same situation as before.

TRNG is not implemented in TCG mode but is required by RngDxe to run.

On older cpu cores nothing changes.
---
 Platform/Qemu/SbsaQemu/SbsaQemu.dsc | 7 +++
 Platform/Qemu/SbsaQemu/SbsaQemu.fdf | 1 +
 2 files changed, 8 insertions(+)

diff --git a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc 
b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
index 9306986bf7c0..72b6a6d9a8b8 100644
--- a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
+++ b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
@@ -660,6 +660,13 @@ [Components.common]
   OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf
   MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
   Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuHighMemDxe/SbsaQemuHighMemDxe.inf
+  SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf {
+
+  RngLib|MdePkg/Library/BaseRngLib/BaseRngLib.inf
+  ArmTrngLib|ArmPkg/Library/ArmTrngLib/ArmTrngLib.inf
+  ArmMonitorLib|ArmPkg/Library/ArmMonitorLib/ArmMonitorLib.inf
+  }
+
 
   #
   # FAT filesystem + GPT/MBR partitioning
diff --git a/Platform/Qemu/SbsaQemu/SbsaQemu.fdf 
b/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
index b35f42e11aa4..51a1ef8519f9 100644
--- a/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
+++ b/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
@@ -192,6 +192,7 @@ [FV.FvMain]
   INF ArmPkg/Drivers/TimerDxe/TimerDxe.inf
   INF OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf
   INF MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
+  INF SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
 
   #
   # FAT filesystem + GPT/MBR partitioning + UDF filesystem

---
base-commit: c7ed8deaa8c1d7ee83af994b2c90d4490ef27bdc
change-id: 20240703-efi-rng-protocol-be991536709a

Best regards,
-- 
Marcin Juszkiewicz 



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

[edk2-devel] How to optimize boot time?

2024-07-03 Thread Hamit Can Karaca 
Hi,

So I've a SBL+EDK2 customized firmware. This firmware enters the BIOS menu in 
approximately 17 seconds. I haven't seen this as a problem for a long time, but 
I've seen some CFL motherboards boot up the OS in 17 seconds. The motherboard I 
use is CFL-H. Is there a solution that will allow me to shorten this period 
even more?

Thanks,
Hamit Can KARACA


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

Re: [edk2-devel] [staging/dynamictables-reorg PATCH 1/2] DynamicTablesPkg: Fix conversion compiler warnings

2024-07-03 Thread PierreGondois 

Hi Sami,

On 7/3/24 12:35, Sami Mujawar wrote:

Hi Pierre,

Thank you for these patches.


On 03/07/2024, 10:54, "Pierre Gondois" mailto:pierre.gond...@arm.com>> wrote:


Some CM objects fields are wider than the targeted field in ACPI
tables. Some assignments are also subject to data loss and
trigger the following warnings:
- '<': signed/unsigned mismatch
- '=': conversion from 'UINTxx' to 'UINTyy', possible loss of data
with xx > yy.


Add checks/cast to remove the warnings.


Signed-off-by: Pierre Gondois mailto:pierre.gond...@arm.com>>
---
.../Acpi/Common/AcpiPcctLib/PcctGenerator.c | 15 +++
.../SsdtCpuTopologyGenerator.c | 6 --
.../Common/AcpiSsdtPcieLib/SsdtPcieGenerator.c | 15 ++-
3 files changed, 25 insertions(+), 11 deletions(-)


diff --git a/DynamicTablesPkg/Library/Acpi/Common/AcpiPcctLib/PcctGenerator.c 
b/DynamicTablesPkg/Library/Acpi/Common/AcpiPcctLib/PcctGenerator.c
index 205c44405785..061e12bf1b3d 100644
--- a/DynamicTablesPkg/Library/Acpi/Common/AcpiPcctLib/PcctGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Common/AcpiPcctLib/PcctGenerator.c
@@ -379,10 +379,12 @@ AddSubspaceStructType1 (
Doorbell = &GenericPccCmObj->DoorbellReg;


ChannelTiming = &GenericPccCmObj->ChannelTiming;






+ ASSERT ((PccCmObj->PlatIrq.Flags >> 8) == 0);

[SAMI] I think we can change this to ASSERT ((PccCmObj->PlatIrq.Flags & 
~MAX_UINT8) == 0);
That way we can also avoid magic numbers.
I think similar changes are required elsewhere in this patch.
If you agree, I will make the changes before merging.


Yes sure, thanks!

Regards,
Pierre



Otherwise, this series looks good to me.

Reviewed-by: Sami Mujawar 


Regards,

Sami Mujawar




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

Re: [edk2-devel] [staging/dynamictables-reorg PATCH 2/2] DynamicTablesPkg: Add EFIAPI to generators hooks

2024-07-03 Thread Sami Mujawar 
Hi Pierre,

Thank you for this patch.

These changes look good to me.

Reviewed-by: Sami Mujawar 

Regards,

Sami Mujawar


On 03/07/2024, 10:54, "Pierre Gondois" mailto:pierre.gond...@arm.com>> wrote:


For X64 builds, the EFIAPI is replaced by '(__attribute__((ms_abi))'.
This might lead to build error for some ACPI tablte generators
due to function prototype mismatch.


Add the EFIAPI to ACPI table generator hooks:
- ACPI_TABLE_GENERATOR_BUILD_TABLEEX
- ACPI_TABLE_GENERATOR_FREE_TABLEEX


Signed-off-by: Pierre Gondois mailto:pierre.gond...@arm.com>>
---
DynamicTablesPkg/Include/AcpiTableGenerator.h | 8 
.../Library/Acpi/Common/AcpiMcfgLib/McfgGenerator.c | 1 +
.../Library/Acpi/Common/AcpiPcctLib/PcctGenerator.c | 1 +
.../Library/Acpi/Common/AcpiPpttLib/PpttGenerator.c | 1 +
.../Library/Acpi/Common/AcpiSratLib/SratGenerator.c | 1 +
.../AcpiSsdtCpuTopologyLib/SsdtCpuTopologyGenerator.c | 1 +
6 files changed, 9 insertions(+), 4 deletions(-)


diff --git a/DynamicTablesPkg/Include/AcpiTableGenerator.h 
b/DynamicTablesPkg/Include/AcpiTableGenerator.h
index d0eda011c301..f5c6179be082 100644
--- a/DynamicTablesPkg/Include/AcpiTableGenerator.h
+++ b/DynamicTablesPkg/Include/AcpiTableGenerator.h
@@ -214,7 +214,7 @@ typedef struct AcpiTableGenerator ACPI_TABLE_GENERATOR;
@return EFI_SUCCESS If the table is generated successfully or other


failure codes as returned by the generator.


**/


-typedef EFI_STATUS (*ACPI_TABLE_GENERATOR_BUILD_TABLE) (


+typedef EFI_STATUS (EFIAPI *ACPI_TABLE_GENERATOR_BUILD_TABLE)(


IN CONST ACPI_TABLE_GENERATOR *This,


IN CONST CM_STD_OBJ_ACPI_TABLE_INFO *CONST AcpiTableInfo,


IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol,


@@ -234,7 +234,7 @@ typedef EFI_STATUS (*ACPI_TABLE_GENERATOR_BUILD_TABLE) (
@return EFI_SUCCESS If freed successfully or other failure codes


as returned by the generator.


**/


-typedef EFI_STATUS (*ACPI_TABLE_GENERATOR_FREE_TABLE) (


+typedef EFI_STATUS (EFIAPI *ACPI_TABLE_GENERATOR_FREE_TABLE)(


IN CONST ACPI_TABLE_GENERATOR *CONST This,


IN CONST CM_STD_OBJ_ACPI_TABLE_INFO *CONST AcpiTableInfo,


IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol,


@@ -257,7 +257,7 @@ typedef EFI_STATUS (*ACPI_TABLE_GENERATOR_FREE_TABLE) (
@return EFI_SUCCESS If the table is generated successfully or other


failure codes as returned by the generator.


**/


-typedef EFI_STATUS (*ACPI_TABLE_GENERATOR_BUILD_TABLEEX) (


+typedef EFI_STATUS (EFIAPI *ACPI_TABLE_GENERATOR_BUILD_TABLEEX)(


IN CONST ACPI_TABLE_GENERATOR *This,


IN CONST CM_STD_OBJ_ACPI_TABLE_INFO *CONST AcpiTableInfo,


IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol,


@@ -280,7 +280,7 @@ typedef EFI_STATUS (*ACPI_TABLE_GENERATOR_BUILD_TABLEEX) (
@return EFI_SUCCESS If freed successfully or other failure codes


as returned by the generator.


**/


-typedef EFI_STATUS (*ACPI_TABLE_GENERATOR_FREE_TABLEEX) (


+typedef EFI_STATUS (EFIAPI *ACPI_TABLE_GENERATOR_FREE_TABLEEX)(


IN CONST ACPI_TABLE_GENERATOR *CONST This,


IN CONST CM_STD_OBJ_ACPI_TABLE_INFO *CONST AcpiTableInfo,


IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol,


diff --git a/DynamicTablesPkg/Library/Acpi/Common/AcpiMcfgLib/McfgGenerator.c 
b/DynamicTablesPkg/Library/Acpi/Common/AcpiMcfgLib/McfgGenerator.c
index 722f9c17d541..40dea304e301 100644
--- a/DynamicTablesPkg/Library/Acpi/Common/AcpiMcfgLib/McfgGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Common/AcpiMcfgLib/McfgGenerator.c
@@ -261,6 +261,7 @@ error_handler:
**/


STATIC


EFI_STATUS


+EFIAPI


FreeMcfgTableResources (


IN CONST ACPI_TABLE_GENERATOR *CONST This,


IN CONST CM_STD_OBJ_ACPI_TABLE_INFO *CONST AcpiTableInfo,


diff --git a/DynamicTablesPkg/Library/Acpi/Common/AcpiPcctLib/PcctGenerator.c 
b/DynamicTablesPkg/Library/Acpi/Common/AcpiPcctLib/PcctGenerator.c
index 061e12bf1b3d..12e34f3e442c 100644
--- a/DynamicTablesPkg/Library/Acpi/Common/AcpiPcctLib/PcctGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Common/AcpiPcctLib/PcctGenerator.c
@@ -1075,6 +1075,7 @@ error_handler:
**/


STATIC


EFI_STATUS


+EFIAPI


FreePcctTableResources (


IN CONST ACPI_TABLE_GENERATOR *CONST This,


IN CONST CM_STD_OBJ_ACPI_TABLE_INFO *CONST AcpiTableInfo,


diff --git a/DynamicTablesPkg/Library/Acpi/Common/AcpiPpttLib/PpttGenerator.c 
b/DynamicTablesPkg/Library/Acpi/Common/AcpiPpttLib/PpttGenerator.c
index 2b8088a07f44..fd465cbab0e9 100644
--- a/DynamicTablesPkg/Library/Acpi/Common/AcpiPpttLib/PpttGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Common/AcpiPpttLib/PpttGenerator.c
@@ -1342,6 +1342,7 @@ error_handler:
**/


STATIC


EFI_STATUS


+EFIAPI


FreePpttTableResources (


IN CONST ACPI_TABLE_GENERATOR *CONST This,


IN CONST CM_STD_OBJ_ACPI_TABLE_INFO *CONST AcpiTableInfo,


diff --git a/DynamicTablesPkg/Library/Acpi/Common/AcpiSratLib/SratGenerator.c 
b/DynamicTablesPkg/Library/Acpi/Common/AcpiSratLib/SratGenerator.c
index dcdacc4e966e..1a9434e6bd08 100644
--- a/DynamicTablesPkg/Library/

Re: [edk2-devel] [staging/dynamictables-reorg PATCH 1/2] DynamicTablesPkg: Fix conversion compiler warnings

2024-07-03 Thread Sami Mujawar 
Hi Pierre,

Thank you for these patches.


On 03/07/2024, 10:54, "Pierre Gondois" mailto:pierre.gond...@arm.com>> wrote:


Some CM objects fields are wider than the targeted field in ACPI
tables. Some assignments are also subject to data loss and
trigger the following warnings:
- '<': signed/unsigned mismatch
- '=': conversion from 'UINTxx' to 'UINTyy', possible loss of data
with xx > yy.


Add checks/cast to remove the warnings.


Signed-off-by: Pierre Gondois mailto:pierre.gond...@arm.com>>
---
.../Acpi/Common/AcpiPcctLib/PcctGenerator.c | 15 +++
.../SsdtCpuTopologyGenerator.c | 6 --
.../Common/AcpiSsdtPcieLib/SsdtPcieGenerator.c | 15 ++-
3 files changed, 25 insertions(+), 11 deletions(-)


diff --git a/DynamicTablesPkg/Library/Acpi/Common/AcpiPcctLib/PcctGenerator.c 
b/DynamicTablesPkg/Library/Acpi/Common/AcpiPcctLib/PcctGenerator.c
index 205c44405785..061e12bf1b3d 100644
--- a/DynamicTablesPkg/Library/Acpi/Common/AcpiPcctLib/PcctGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Common/AcpiPcctLib/PcctGenerator.c
@@ -379,10 +379,12 @@ AddSubspaceStructType1 (
Doorbell = &GenericPccCmObj->DoorbellReg;


ChannelTiming = &GenericPccCmObj->ChannelTiming;






+ ASSERT ((PccCmObj->PlatIrq.Flags >> 8) == 0);

[SAMI] I think we can change this to ASSERT ((PccCmObj->PlatIrq.Flags & 
~MAX_UINT8) == 0);
That way we can also avoid magic numbers. 
I think similar changes are required elsewhere in this patch. 
If you agree, I will make the changes before merging.

Otherwise, this series looks good to me.

Reviewed-by: Sami Mujawar 


Regards,

Sami Mujawar



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

Re: [edk2-devel] [staging/dynamictables-reorg PATCH 00/15] Prepare libraries to support other archs

2024-07-03 Thread PierreGondois 

Hello Sami,

On 7/3/24 11:08, Sami Mujawar wrote:

Hi Pierre,

Overall, this patch series looks good to me.

I have some minor comments regarding the return value for the Arch hook 
functions their placement in Common folder.
e.g. in Patch "DynamicTablesPkg: AcpiFadtLib: Prepare to support other archs"
The file 
DynamicTablesPkg/Library/Acpi/Common/AcpiFadtLib/Common/CommonFadtGenerator.c
provides an empty stub for the arch specific implementation for the 
FadtArchUpdate () and returns
success. I think this function should return EFI_UNSUPPORTED to indicate that 
this function is not
implemented and that the architecture needs to provide an implementation.

Also, the file name AcpiFadtLib/Common/CommonFadtGenerator.c should be changed 
to
AcpiFadtLib/FadtGeneratorNull.c to clarify that the implementation does not 
exist.

Similar changes are required for other patches as well.

Apart from the above, in patch " DynamicTablesPkg: FdtHwInfoParserLib: Move IRQ map 
to arch folder"
I think the file 
DynamicTablesPkg/Library/FdtHwInfoParserLib/Arm/ArmFdtUtility.c should be 
renamed to
DynamicTablesPkg/Library/FdtHwInfoParserLib/Arm/ArmFdtInterrupt.c

If you agree with the above, I will make the necessary changes before merging.


Yes sure, thanks a lot.
You might also need the following 2 patches to pass the CI tests:
  https://edk2.groups.io/g/devel/message/119777

Regards,
Pierre



With that,

Reviewed-by: Sami Mujawar 

Regards,

Sami Mujawar


On 19/06/2024, 23:06, "Pierre Gondois" mailto:pierre.gond...@arm.com>> wrote:


The DynamicTables framework has mainly been developed/tested against Arm
architecture. While still trying to have re-usable libraries, opening the
framework to other architectures implies some re-organization.


The libraries that are generic enough to be directly re-used are moved
to a Common/ directory. For some libraries, additional arch-specific hooks
have been added to allow architectures specific modifications.


---


Changes can be seen at:
https://github.com/pierregondois/edk2/tree/pg/dyntables_libraries_reorg 



---


References:
1. Staging branch creation:
URL: https://edk2.groups.io/g/devel/message/114790 



2. edk2-staging Repo
URL: https://github.com/tianocore/edk2-staging.git 

Branch Name: dynamictables-reorg


3. edk2-platforms Repo
URL: https://github.com/tianocore/edk2-platforms.git 

Branch Name: devel-dynamictables-reorg


---


Cc: AbdulLateef Attar mailto:abdullateef.at...@amd.com>>
Cc: Girish Mahadevan mailto:gmahade...@nvidia.com>>
Cc: Jeff Brasen mailto:jbra...@nvidia.com>>
Cc: Jeshua Smith mailto:jesh...@nvidia.com>>
Cc: Leif Lindholm mailto:quic_llind...@quicinc.com>>
Cc: Meenakshi Aggarwal mailto:meenakshi.aggar...@nxp.com>>
Cc: Pierre Gondois mailto:pierre.gond...@arm.com>>
Cc: Sami Mujawar mailto:sami.muja...@arm.com>>
Cc: Sunil V L mailto:suni...@ventanamicro.com>>
Cc: Yeo Reum Yun mailto:yeoreum@arm.com>>


Pierre Gondois (15):
DynamicTablesPkg: Acpi: Move generic libraries to common folder
DynamicTablesPkg: Acpi: Prepare common libraries to support other
archs
DynamicTablesPkg: AcpiFadtLib: Prepare to support other archs
DynamicTablesPkg: AcpiDbg2Lib: Prepare to support other archs
DynamicTablesPkg: AcpiSpcrLib: Prepare to support other archs
DynamicTablesPkg: AcpiSratLib: Prepare to support other archs
DynamicTablesPkg: AcpiSsdtCpuTopologyLib: Avoid dependency on GICC
DynamicTablesPkg: DynamicTableManagerDxe: Refactor PresenceArray
DynamicTablesPkg: FdtHwInfoParserLib: Move ARM parsers to Arm
directory
DynamicTablesPkg: FdtHwInfoParserLib: Refactor to prepare for other
archs
DynamicTablesPkg: FdtHwInfoParserLib: Make Pci parser arch neutral
DynamicTablesPkg: FdtHwInfoParserLib: Make Serial Port parser arch
neutral
DynamicTablesPkg: FdtHwInfoParserLib: Move ArmLib.h to ArmGicCParser.c
DynamicTablesPkg: FdtHwInfoParserLib: Move IRQ map to arch folder
DynamicTablesPkg: FdtHwInfoParserLib: Create wrapper to get INTC addr
cells


.../Arm/ArmDynamicTableManagerDxe.c | 63 +++
.../Common/CommonDynamicTableManagerDxe.c | 58 +++
.../DynamicTableManagerDxe.c | 70 +--
.../DynamicTableManagerDxe.h | 63 +++
.../DynamicTableManagerDxe.inf | 7 +
DynamicTablesPkg/DynamicTables.dsc.inc | 64 +--
.../SsdtCpuTopologyGenerator.h | 147 ---
.../AcpiDbg2Lib/AcpiDbg2Lib.inf} | 22 +-
.../Common/AcpiDbg2Lib/Arm/ArmDbg2Generator.c | 67 +++
.../AcpiDbg2Lib/Common/CommonDbg2Generator.c | 59 +++
.../AcpiDbg2Lib}/Dbg2Generator.c | 24 +-
.../Acpi/Common/AcpiDbg2Lib/Dbg2Generator.h | 56 +++
.../AcpiFadtLib/AcpiFadtLib.inf} | 16 +-
.../Common/AcpiFadtLib/Arm/ArmFadtGenerator.c | 126 ++
.../AcpiFadtLib/Common/CommonFadtGenerator.c | 46 ++
.../AcpiFadtLib}/FadtGenerator.c | 86 +---
.../Acpi/Common/AcpiFadtLib/FadtGenerator.h | 35 ++
.../AcpiMcfgLib/AcpiMcfgLib.inf}

[edk2-devel] [staging/dynamictables-reorg PATCH 2/2] DynamicTablesPkg: Add EFIAPI to generators hooks

2024-07-03 Thread PierreGondois 
For X64 builds, the EFIAPI is replaced by '(__attribute__((ms_abi))'.
This might lead to build error for some ACPI tablte generators
due to function prototype mismatch.

Add the EFIAPI to ACPI table generator hooks:
- ACPI_TABLE_GENERATOR_BUILD_TABLEEX
- ACPI_TABLE_GENERATOR_FREE_TABLEEX

Signed-off-by: Pierre Gondois 
---
 DynamicTablesPkg/Include/AcpiTableGenerator.h | 8 
 .../Library/Acpi/Common/AcpiMcfgLib/McfgGenerator.c   | 1 +
 .../Library/Acpi/Common/AcpiPcctLib/PcctGenerator.c   | 1 +
 .../Library/Acpi/Common/AcpiPpttLib/PpttGenerator.c   | 1 +
 .../Library/Acpi/Common/AcpiSratLib/SratGenerator.c   | 1 +
 .../AcpiSsdtCpuTopologyLib/SsdtCpuTopologyGenerator.c | 1 +
 6 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/DynamicTablesPkg/Include/AcpiTableGenerator.h 
b/DynamicTablesPkg/Include/AcpiTableGenerator.h
index d0eda011c301..f5c6179be082 100644
--- a/DynamicTablesPkg/Include/AcpiTableGenerator.h
+++ b/DynamicTablesPkg/Include/AcpiTableGenerator.h
@@ -214,7 +214,7 @@ typedef struct AcpiTableGenerator   
ACPI_TABLE_GENERATOR;
   @return  EFI_SUCCESS If the table is generated successfully or other
 failure codes as returned by the generator.
 **/
-typedef EFI_STATUS (*ACPI_TABLE_GENERATOR_BUILD_TABLE) (
+typedef EFI_STATUS (EFIAPI *ACPI_TABLE_GENERATOR_BUILD_TABLE)(
   IN  CONST ACPI_TABLE_GENERATOR   *This,
   IN  CONST CM_STD_OBJ_ACPI_TABLE_INFO *CONST  AcpiTableInfo,
   IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL   *CONST  CfgMgrProtocol,
@@ -234,7 +234,7 @@ typedef EFI_STATUS (*ACPI_TABLE_GENERATOR_BUILD_TABLE) (
   @return EFI_SUCCESS  If freed successfully or other failure codes
 as returned by the generator.
 **/
-typedef EFI_STATUS (*ACPI_TABLE_GENERATOR_FREE_TABLE) (
+typedef EFI_STATUS (EFIAPI *ACPI_TABLE_GENERATOR_FREE_TABLE)(
   IN  CONST ACPI_TABLE_GENERATOR   *CONST  This,
   IN  CONST CM_STD_OBJ_ACPI_TABLE_INFO *CONST  AcpiTableInfo,
   IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL   *CONST  CfgMgrProtocol,
@@ -257,7 +257,7 @@ typedef EFI_STATUS (*ACPI_TABLE_GENERATOR_FREE_TABLE) (
   @return  EFI_SUCCESS If the table is generated successfully or other
 failure codes as returned by the generator.
 **/
-typedef EFI_STATUS (*ACPI_TABLE_GENERATOR_BUILD_TABLEEX) (
+typedef EFI_STATUS (EFIAPI *ACPI_TABLE_GENERATOR_BUILD_TABLEEX)(
   IN  CONST ACPI_TABLE_GENERATOR   *This,
   IN  CONST CM_STD_OBJ_ACPI_TABLE_INFO *CONST  AcpiTableInfo,
   IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL   *CONST  CfgMgrProtocol,
@@ -280,7 +280,7 @@ typedef EFI_STATUS (*ACPI_TABLE_GENERATOR_BUILD_TABLEEX) (
   @return EFI_SUCCESS  If freed successfully or other failure codes
 as returned by the generator.
 **/
-typedef EFI_STATUS (*ACPI_TABLE_GENERATOR_FREE_TABLEEX) (
+typedef EFI_STATUS (EFIAPI *ACPI_TABLE_GENERATOR_FREE_TABLEEX)(
   IN  CONST ACPI_TABLE_GENERATOR   *CONST  This,
   IN  CONST CM_STD_OBJ_ACPI_TABLE_INFO *CONST  AcpiTableInfo,
   IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL   *CONST  CfgMgrProtocol,
diff --git a/DynamicTablesPkg/Library/Acpi/Common/AcpiMcfgLib/McfgGenerator.c 
b/DynamicTablesPkg/Library/Acpi/Common/AcpiMcfgLib/McfgGenerator.c
index 722f9c17d541..40dea304e301 100644
--- a/DynamicTablesPkg/Library/Acpi/Common/AcpiMcfgLib/McfgGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Common/AcpiMcfgLib/McfgGenerator.c
@@ -261,6 +261,7 @@ error_handler:
 **/
 STATIC
 EFI_STATUS
+EFIAPI
 FreeMcfgTableResources (
   IN  CONST ACPI_TABLE_GENERATOR  *CONST  This,
   IN  CONST CM_STD_OBJ_ACPI_TABLE_INFO*CONST  AcpiTableInfo,
diff --git a/DynamicTablesPkg/Library/Acpi/Common/AcpiPcctLib/PcctGenerator.c 
b/DynamicTablesPkg/Library/Acpi/Common/AcpiPcctLib/PcctGenerator.c
index 061e12bf1b3d..12e34f3e442c 100644
--- a/DynamicTablesPkg/Library/Acpi/Common/AcpiPcctLib/PcctGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Common/AcpiPcctLib/PcctGenerator.c
@@ -1075,6 +1075,7 @@ error_handler:
 **/
 STATIC
 EFI_STATUS
+EFIAPI
 FreePcctTableResources (
   IN  CONST ACPI_TABLE_GENERATOR  *CONST  This,
   IN  CONST CM_STD_OBJ_ACPI_TABLE_INFO*CONST  AcpiTableInfo,
diff --git a/DynamicTablesPkg/Library/Acpi/Common/AcpiPpttLib/PpttGenerator.c 
b/DynamicTablesPkg/Library/Acpi/Common/AcpiPpttLib/PpttGenerator.c
index 2b8088a07f44..fd465cbab0e9 100644
--- a/DynamicTablesPkg/Library/Acpi/Common/AcpiPpttLib/PpttGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Common/AcpiPpttLib/PpttGenerator.c
@@ -1342,6 +1342,7 @@ error_handler:
 **/
 STATIC
 EFI_STATUS
+EFIAPI
 FreePpttTableResources (
   IN  CONST ACPI_TABLE_GENERATOR  *CONST  This,
   IN  CONST CM_STD_OBJ_ACPI_TABLE_INFO*CONST  AcpiTableInfo,
diff --g

[edk2-devel] [staging/dynamictables-reorg PATCH 1/2] DynamicTablesPkg: Fix conversion compiler warnings

2024-07-03 Thread PierreGondois 
Some CM objects fields are wider than the targeted field in ACPI
tables. Some assignments are also subject to data loss and
trigger the following warnings:
- '<': signed/unsigned mismatch
- '=': conversion from 'UINTxx' to 'UINTyy', possible loss of data
with xx > yy.

Add checks/cast to remove the warnings.

Signed-off-by: Pierre Gondois 
---
 .../Acpi/Common/AcpiPcctLib/PcctGenerator.c   | 15 +++
 .../SsdtCpuTopologyGenerator.c|  6 --
 .../Common/AcpiSsdtPcieLib/SsdtPcieGenerator.c| 15 ++-
 3 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/DynamicTablesPkg/Library/Acpi/Common/AcpiPcctLib/PcctGenerator.c 
b/DynamicTablesPkg/Library/Acpi/Common/AcpiPcctLib/PcctGenerator.c
index 205c44405785..061e12bf1b3d 100644
--- a/DynamicTablesPkg/Library/Acpi/Common/AcpiPcctLib/PcctGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Common/AcpiPcctLib/PcctGenerator.c
@@ -379,10 +379,12 @@ AddSubspaceStructType1 (
   Doorbell  = &GenericPccCmObj->DoorbellReg;
   ChannelTiming = &GenericPccCmObj->ChannelTiming;
 
+  ASSERT ((PccCmObj->PlatIrq.Flags >> 8) == 0);
+
   PccAcpi->Type   = GenericPccCmObj->Type;
   PccAcpi->Length = sizeof 
(EFI_ACPI_6_4_PCCT_SUBSPACE_1_HW_REDUCED_COMMUNICATIONS);
   PccAcpi->PlatformInterrupt  = PccCmObj->PlatIrq.Interrupt;
-  PccAcpi->PlatformInterruptFlags = PccCmObj->PlatIrq.Flags;
+  PccAcpi->PlatformInterruptFlags = (UINT8)PccCmObj->PlatIrq.Flags;
   PccAcpi->Reserved   = EFI_ACPI_RESERVED_BYTE;
   PccAcpi->BaseAddress= GenericPccCmObj->BaseAddress;
   PccAcpi->AddressLength  = GenericPccCmObj->AddressLength;
@@ -441,10 +443,12 @@ AddSubspaceStructType2 (
   PlatIrqAck= &PccCmObj->PlatIrqAckReg;
   ChannelTiming = &GenericPccCmObj->ChannelTiming;
 
+  ASSERT ((PccCmObj->PlatIrq.Flags >> 8) == 0);
+
   PccAcpi->Type   = GenericPccCmObj->Type;
   PccAcpi->Length = sizeof 
(EFI_ACPI_6_4_PCCT_SUBSPACE_2_HW_REDUCED_COMMUNICATIONS);
   PccAcpi->PlatformInterrupt  = PccCmObj->PlatIrq.Interrupt;
-  PccAcpi->PlatformInterruptFlags = PccCmObj->PlatIrq.Flags;
+  PccAcpi->PlatformInterruptFlags = (UINT8)PccCmObj->PlatIrq.Flags;
   PccAcpi->BaseAddress= GenericPccCmObj->BaseAddress;
   PccAcpi->Reserved   = EFI_ACPI_RESERVED_BYTE;
   PccAcpi->BaseAddress= GenericPccCmObj->BaseAddress;
@@ -519,13 +523,16 @@ AddSubspaceStructType34 (
   ErrorStatus   = &PccCmObj->ErrorStatusReg;
   ChannelTiming = &GenericPccCmObj->ChannelTiming;
 
+  ASSERT ((PccCmObj->PlatIrq.Flags >> 8) == 0);
+  ASSERT ((GenericPccCmObj->AddressLength >> 32) == 0);
+
   PccAcpi->Type   = GenericPccCmObj->Type;
   PccAcpi->Length = sizeof 
(EFI_ACPI_6_4_PCCT_SUBSPACE_3_EXTENDED_PCC);
   PccAcpi->PlatformInterrupt  = PccCmObj->PlatIrq.Interrupt;
-  PccAcpi->PlatformInterruptFlags = PccCmObj->PlatIrq.Flags;
+  PccAcpi->PlatformInterruptFlags = (UINT8)PccCmObj->PlatIrq.Flags;
   PccAcpi->Reserved   = EFI_ACPI_RESERVED_BYTE;
   PccAcpi->BaseAddress= GenericPccCmObj->BaseAddress;
-  PccAcpi->AddressLength  = GenericPccCmObj->AddressLength;
+  PccAcpi->AddressLength  = (UINT32)GenericPccCmObj->AddressLength;
 
   CopyMem (
 &PccAcpi->DoorbellRegister,
diff --git 
a/DynamicTablesPkg/Library/Acpi/Common/AcpiSsdtCpuTopologyLib/SsdtCpuTopologyGenerator.c
 
b/DynamicTablesPkg/Library/Acpi/Common/AcpiSsdtCpuTopologyLib/SsdtCpuTopologyGenerator.c
index 74595131935c..f82b7449713c 100644
--- 
a/DynamicTablesPkg/Library/Acpi/Common/AcpiSsdtCpuTopologyLib/SsdtCpuTopologyGenerator.c
+++ 
b/DynamicTablesPkg/Library/Acpi/Common/AcpiSsdtCpuTopologyLib/SsdtCpuTopologyGenerator.c
@@ -1026,7 +1026,8 @@ CreateAmlCpuTopologyTree (
 if (Generator->ProcNodeList[Index].OverrideNameUidEnabled) {
   Name = Generator->ProcNodeList[Index].OverrideName;
 } else {
-  Name = CpuIndex;
+  ASSERT ((CpuIndex >> 16) == 0);
+  Name = (UINT16)CpuIndex;
 }
 
 Status = CreateAmlCpuFromProcHierarchy (
@@ -1061,7 +1062,8 @@ CreateAmlCpuTopologyTree (
   Name = Generator->ProcNodeList[Index].OverrideName;
   Uid  = Generator->ProcNodeList[Index].OverrideUid;
 } else {
-  Name = ProcContainerName;
+  ASSERT ((ProcContainerName >> 16) == 0);
+  Name = (UINT16)ProcContainerName;
   Uid  = *ProcContainerIndex;
 }
 
diff --git 
a/DynamicTablesPkg/Library/Acpi/Common/AcpiSsdtPcieLib/SsdtPcieGenerator.c 
b/DynamicTablesPkg/Library/Acpi/Common/AcpiSsdtPcieLib/SsdtPcieGenerator.c
index 5b6d5515622b..3dcca2b33987 100644
--- a/DynamicTablesPkg/Library/Acpi/Common/AcpiSsdtPcieLib/SsdtPcieGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Common/AcpiSsdtPcieLib/SsdtPcieGenerator.c
@@ -311,7 +311,7 @@ GeneratePrt (
   )
 {
   EFI_STATUS Status;

[edk2-devel] [staging/dynamictables-reorg PATCH 0/2] DynamicTablesPkg: CI related fixes

2024-07-03 Thread PierreGondois 
Small fixes reported while running the CI.
These patches should be applied on top of:
- [staging/dynamictables-reorg PATCH 00/15] Prepare libraries to support other 
archs 
  https://edk2.groups.io/g/devel/message/119632

Cc: AbdulLateef Attar 
Cc: Girish Mahadevan 
Cc: Jeff Brasen 
Cc: Jeshua Smith 
Cc: Leif Lindholm 
Cc: Meenakshi Aggarwal 
Cc: Pierre Gondois 
Cc: Sami Mujawar 
Cc: Sunil V L 
Cc: Yeo Reum Yun 

Pierre Gondois (2):
  DynamicTablesPkg: Fix conversion compiler warnings
  DynamicTablesPkg: Add EFIAPI to generators hooks

 DynamicTablesPkg/Include/AcpiTableGenerator.h|  8 
 .../Acpi/Common/AcpiMcfgLib/McfgGenerator.c  |  1 +
 .../Acpi/Common/AcpiPcctLib/PcctGenerator.c  | 16 
 .../Acpi/Common/AcpiPpttLib/PpttGenerator.c  |  1 +
 .../Acpi/Common/AcpiSratLib/SratGenerator.c  |  1 +
 .../SsdtCpuTopologyGenerator.c   |  7 +--
 .../Common/AcpiSsdtPcieLib/SsdtPcieGenerator.c   | 15 ++-
 7 files changed, 34 insertions(+), 15 deletions(-)

-- 
2.25.1



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

Re: [edk2-devel] [staging/dynamictables-reorg PATCH 00/15] Prepare libraries to support other archs

2024-07-03 Thread Sunil V L 
On Wed, Jul 03, 2024 at 09:39:22AM +, Sami Mujawar wrote:
> Hi Sunil,
> 
> I think we can look into that. My initial thoughts are that this can be 
> solved using a Pcd. However, we need a bit of investigation.
> Is it ok if we address that in a separate patch?
> 
Sure!.

Thanks,
Sunil
> Regards,
> 
> Sami Mujawar
> 
> On 03/07/2024, 10:37, "Sunil V L"  > wrote:
> 
> 
> Hi Pierre, Sami,
> 
> 
> Thanks a lot again for this work!.
> 
> 
> The series looks good to me as well. I agree with Sami's suggestions.
> 
> 
> However, I have a request for an additional change. The common ACPI
> tables still use ARMH/ARMLTD as the CREATOR_ID/OEM_ID. Can they be made
> architecture specific?
> 
> 
> Reviewed-by: Sunil V L  >
> 
> 
> Thanks!
> Sunil
> On Wed, Jul 03, 2024 at 09:08:08AM +, Sami Mujawar wrote:
> > Hi Pierre,
> > 
> > Overall, this patch series looks good to me.
> > 
> > I have some minor comments regarding the return value for the Arch hook 
> > functions their placement in Common folder.
> > e.g. in Patch "DynamicTablesPkg: AcpiFadtLib: Prepare to support other 
> > archs"
> > The file 
> > DynamicTablesPkg/Library/Acpi/Common/AcpiFadtLib/Common/CommonFadtGenerator.c
> > provides an empty stub for the arch specific implementation for the 
> > FadtArchUpdate () and returns
> > success. I think this function should return EFI_UNSUPPORTED to indicate 
> > that this function is not
> > implemented and that the architecture needs to provide an implementation.
> > 
> > Also, the file name AcpiFadtLib/Common/CommonFadtGenerator.c should be 
> > changed to
> > AcpiFadtLib/FadtGeneratorNull.c to clarify that the implementation does not 
> > exist.
> > 
> > Similar changes are required for other patches as well.
> > 
> > Apart from the above, in patch " DynamicTablesPkg: FdtHwInfoParserLib: Move 
> > IRQ map to arch folder"
> > I think the file 
> > DynamicTablesPkg/Library/FdtHwInfoParserLib/Arm/ArmFdtUtility.c should be 
> > renamed to 
> > DynamicTablesPkg/Library/FdtHwInfoParserLib/Arm/ArmFdtInterrupt.c
> > 
> > If you agree with the above, I will make the necessary changes before 
> > merging.
> > 
> > With that,
> > 
> > Reviewed-by: Sami Mujawar  > >
> > 
> > Regards,
> > 
> > Sami Mujawar
> > 
> > 
> > On 19/06/2024, 23:06, "Pierre Gondois"  >   > >> wrote:
> > 
> > 
> > The DynamicTables framework has mainly been developed/tested against Arm
> > architecture. While still trying to have re-usable libraries, opening the
> > framework to other architectures implies some re-organization.
> > 
> > 
> > The libraries that are generic enough to be directly re-used are moved
> > to a Common/ directory. For some libraries, additional arch-specific hooks
> > have been added to allow architectures specific modifications.
> > 
> > 
> > ---
> > 
> > 
> > Changes can be seen at:
> > https://github.com/pierregondois/edk2/tree/pg/dyntables_libraries_reorg 
> >  
> >  
> > ;>
> > 
> > 
> > ---
> > 
> > 
> > References:
> > 1. Staging branch creation:
> > URL: https://edk2.groups.io/g/devel/message/114790 
> >  
> >  
> > ;>
> > 
> > 
> > 2. edk2-staging Repo
> > URL: https://github.com/tianocore/edk2-staging.git 
> >  
> >  
> > ;>
> > Branch Name: dynamictables-reorg
> > 
> > 
> > 3. edk2-platforms Repo
> > URL: https://github.com/tianocore/edk2-platforms.git 
> >  
> >  
> > ;>
> > Branch Name: devel-dynamictables-reorg
> > 
> > 
> > ---
> > 
> > 
> > Cc: AbdulLateef Attar  >   > >>
> > Cc: Girish Mahadevan mailto:gmahade...@nvidia.com> 
> > >>
> > Cc: Jeff Brasen mailto:jbra...@nvidia.com> 
> > >>
> > Cc: Jeshua Smith mailto:jesh...@nvidia.com> 
> > >>
> > Cc: Leif Lindholm  >   > >>
> > Cc: Meenakshi Aggarwal  >   >

Re: [edk2-devel] [staging/dynamictables-reorg PATCH 00/15] Prepare libraries to support other archs

2024-07-03 Thread Sami Mujawar 
Hi Sunil,

I think we can look into that. My initial thoughts are that this can be solved 
using a Pcd. However, we need a bit of investigation.
Is it ok if we address that in a separate patch?

Regards,

Sami Mujawar

On 03/07/2024, 10:37, "Sunil V L" mailto:suni...@ventanamicro.com>> wrote:


Hi Pierre, Sami,


Thanks a lot again for this work!.


The series looks good to me as well. I agree with Sami's suggestions.


However, I have a request for an additional change. The common ACPI
tables still use ARMH/ARMLTD as the CREATOR_ID/OEM_ID. Can they be made
architecture specific?


Reviewed-by: Sunil V L mailto:suni...@ventanamicro.com>>


Thanks!
Sunil
On Wed, Jul 03, 2024 at 09:08:08AM +, Sami Mujawar wrote:
> Hi Pierre,
> 
> Overall, this patch series looks good to me.
> 
> I have some minor comments regarding the return value for the Arch hook 
> functions their placement in Common folder.
> e.g. in Patch "DynamicTablesPkg: AcpiFadtLib: Prepare to support other archs"
> The file 
> DynamicTablesPkg/Library/Acpi/Common/AcpiFadtLib/Common/CommonFadtGenerator.c
> provides an empty stub for the arch specific implementation for the 
> FadtArchUpdate () and returns
> success. I think this function should return EFI_UNSUPPORTED to indicate that 
> this function is not
> implemented and that the architecture needs to provide an implementation.
> 
> Also, the file name AcpiFadtLib/Common/CommonFadtGenerator.c should be 
> changed to
> AcpiFadtLib/FadtGeneratorNull.c to clarify that the implementation does not 
> exist.
> 
> Similar changes are required for other patches as well.
> 
> Apart from the above, in patch " DynamicTablesPkg: FdtHwInfoParserLib: Move 
> IRQ map to arch folder"
> I think the file 
> DynamicTablesPkg/Library/FdtHwInfoParserLib/Arm/ArmFdtUtility.c should be 
> renamed to 
> DynamicTablesPkg/Library/FdtHwInfoParserLib/Arm/ArmFdtInterrupt.c
> 
> If you agree with the above, I will make the necessary changes before merging.
> 
> With that,
> 
> Reviewed-by: Sami Mujawar mailto:sami.muja...@arm.com>>
> 
> Regards,
> 
> Sami Mujawar
> 
> 
> On 19/06/2024, 23:06, "Pierre Gondois"    >> wrote:
> 
> 
> The DynamicTables framework has mainly been developed/tested against Arm
> architecture. While still trying to have re-usable libraries, opening the
> framework to other architectures implies some re-organization.
> 
> 
> The libraries that are generic enough to be directly re-used are moved
> to a Common/ directory. For some libraries, additional arch-specific hooks
> have been added to allow architectures specific modifications.
> 
> 
> ---
> 
> 
> Changes can be seen at:
> https://github.com/pierregondois/edk2/tree/pg/dyntables_libraries_reorg 
>  
>  
> ;>
> 
> 
> ---
> 
> 
> References:
> 1. Staging branch creation:
> URL: https://edk2.groups.io/g/devel/message/114790 
>  
>  
> ;>
> 
> 
> 2. edk2-staging Repo
> URL: https://github.com/tianocore/edk2-staging.git 
>  
>  
> ;>
> Branch Name: dynamictables-reorg
> 
> 
> 3. edk2-platforms Repo
> URL: https://github.com/tianocore/edk2-platforms.git 
>  
>  
> ;>
> Branch Name: devel-dynamictables-reorg
> 
> 
> ---
> 
> 
> Cc: AbdulLateef Attar    >>
> Cc: Girish Mahadevan mailto:gmahade...@nvidia.com> 
> >>
> Cc: Jeff Brasen mailto:jbra...@nvidia.com> 
> >>
> Cc: Jeshua Smith mailto:jesh...@nvidia.com> 
> >>
> Cc: Leif Lindholm    >>
> Cc: Meenakshi Aggarwal    >>
> Cc: Pierre Gondois mailto:pierre.gond...@arm.com> 
> >>
> Cc: Sami Mujawar mailto:sami.muja...@arm.com> 
> >>
> Cc: Sunil V L mailto:suni...@ventanamicro.com> 
> >>
> Cc: Yeo Reum Yun mailto:yeore

Re: [edk2-devel] [staging/dynamictables-reorg PATCH 00/15] Prepare libraries to support other archs

2024-07-03 Thread Sunil V L 
Hi Pierre, Sami,

Thanks a lot again for this work!.

The series looks good to me as well. I agree with Sami's suggestions.

However, I have a request for an additional change. The common ACPI
tables still use ARMH/ARMLTD as the CREATOR_ID/OEM_ID. Can they be made
architecture specific?

Reviewed-by: Sunil V L 

Thanks!
Sunil
On Wed, Jul 03, 2024 at 09:08:08AM +, Sami Mujawar wrote:
> Hi Pierre,
> 
> Overall, this patch series looks good to me.
> 
> I have some minor comments regarding the return value for the Arch hook 
> functions their placement in Common folder.
> e.g. in Patch "DynamicTablesPkg: AcpiFadtLib: Prepare to support other archs"
> The file 
> DynamicTablesPkg/Library/Acpi/Common/AcpiFadtLib/Common/CommonFadtGenerator.c
> provides an empty stub for the arch specific implementation for the 
> FadtArchUpdate () and returns
> success. I think this function should return EFI_UNSUPPORTED to indicate that 
> this function is not
> implemented and that the architecture needs to provide an implementation.
> 
> Also, the file name AcpiFadtLib/Common/CommonFadtGenerator.c should be 
> changed to
> AcpiFadtLib/FadtGeneratorNull.c to clarify that the implementation does not 
> exist.
> 
> Similar changes are required for other patches as well.
> 
> Apart from the above, in patch " DynamicTablesPkg: FdtHwInfoParserLib: Move 
> IRQ map to arch folder"
> I think the file 
> DynamicTablesPkg/Library/FdtHwInfoParserLib/Arm/ArmFdtUtility.c should be 
> renamed to 
> DynamicTablesPkg/Library/FdtHwInfoParserLib/Arm/ArmFdtInterrupt.c
> 
> If you agree with the above, I will make the necessary changes before merging.
> 
> With that,
> 
> Reviewed-by: Sami Mujawar 
> 
> Regards,
> 
> Sami Mujawar
> 
> 
> On 19/06/2024, 23:06, "Pierre Gondois"  > wrote:
> 
> 
> The DynamicTables framework has mainly been developed/tested against Arm
> architecture. While still trying to have re-usable libraries, opening the
> framework to other architectures implies some re-organization.
> 
> 
> The libraries that are generic enough to be directly re-used are moved
> to a Common/ directory. For some libraries, additional arch-specific hooks
> have been added to allow architectures specific modifications.
> 
> 
> ---
> 
> 
> Changes can be seen at:
> https://github.com/pierregondois/edk2/tree/pg/dyntables_libraries_reorg 
> 
> 
> 
> ---
> 
> 
> References:
> 1. Staging branch creation:
> URL: https://edk2.groups.io/g/devel/message/114790 
> 
> 
> 
> 2. edk2-staging Repo
> URL: https://github.com/tianocore/edk2-staging.git 
> 
> Branch Name: dynamictables-reorg
> 
> 
> 3. edk2-platforms Repo
> URL: https://github.com/tianocore/edk2-platforms.git 
> 
> Branch Name: devel-dynamictables-reorg
> 
> 
> ---
> 
> 
> Cc: AbdulLateef Attar  >
> Cc: Girish Mahadevan mailto:gmahade...@nvidia.com>>
> Cc: Jeff Brasen mailto:jbra...@nvidia.com>>
> Cc: Jeshua Smith mailto:jesh...@nvidia.com>>
> Cc: Leif Lindholm  >
> Cc: Meenakshi Aggarwal  >
> Cc: Pierre Gondois mailto:pierre.gond...@arm.com>>
> Cc: Sami Mujawar mailto:sami.muja...@arm.com>>
> Cc: Sunil V L mailto:suni...@ventanamicro.com>>
> Cc: Yeo Reum Yun mailto:yeoreum@arm.com>>
> 
> 
> Pierre Gondois (15):
> DynamicTablesPkg: Acpi: Move generic libraries to common folder
> DynamicTablesPkg: Acpi: Prepare common libraries to support other
> archs
> DynamicTablesPkg: AcpiFadtLib: Prepare to support other archs
> DynamicTablesPkg: AcpiDbg2Lib: Prepare to support other archs
> DynamicTablesPkg: AcpiSpcrLib: Prepare to support other archs
> DynamicTablesPkg: AcpiSratLib: Prepare to support other archs
> DynamicTablesPkg: AcpiSsdtCpuTopologyLib: Avoid dependency on GICC
> DynamicTablesPkg: DynamicTableManagerDxe: Refactor PresenceArray
> DynamicTablesPkg: FdtHwInfoParserLib: Move ARM parsers to Arm
> directory
> DynamicTablesPkg: FdtHwInfoParserLib: Refactor to prepare for other
> archs
> DynamicTablesPkg: FdtHwInfoParserLib: Make Pci parser arch neutral
> DynamicTablesPkg: FdtHwInfoParserLib: Make Serial Port parser arch
> neutral
> DynamicTablesPkg: FdtHwInfoParserLib: Move ArmLib.h to ArmGicCParser.c
> DynamicTablesPkg: FdtHwInfoParserLib: Move IRQ map to arch folder
> DynamicTablesPkg: FdtHwInfoParserLib: Create wrapper to get INTC addr
> cells
> 
> 
> .../Arm/ArmDynamicTableManagerDxe.c | 63 +++
> .../Common/CommonDynamicTableManagerDxe.c | 58 +++
> .../DynamicTableManagerDxe.c | 70 +--
> .../DynamicTableManagerDxe.h | 63 +++
> .../DynamicTableManagerDxe.inf | 7 +
> DynamicTablesPkg/DynamicTables.dsc.inc | 64 +--
> .../SsdtCpuTopologyGenerator.h | 147 ---
> .../AcpiDbg2Lib/AcpiDbg2Lib.inf} | 22 +-
> .../Common/AcpiDbg2Lib/Arm/ArmDbg2Gen

Re: [edk2-devel] [staging/dynamictables-reorg PATCH 00/15] Prepare libraries to support other archs

2024-07-03 Thread Sami Mujawar 
Hi Pierre,

Overall, this patch series looks good to me.

I have some minor comments regarding the return value for the Arch hook 
functions their placement in Common folder.
e.g. in Patch "DynamicTablesPkg: AcpiFadtLib: Prepare to support other archs"
The file 
DynamicTablesPkg/Library/Acpi/Common/AcpiFadtLib/Common/CommonFadtGenerator.c
provides an empty stub for the arch specific implementation for the 
FadtArchUpdate () and returns
success. I think this function should return EFI_UNSUPPORTED to indicate that 
this function is not
implemented and that the architecture needs to provide an implementation.

Also, the file name AcpiFadtLib/Common/CommonFadtGenerator.c should be changed 
to
AcpiFadtLib/FadtGeneratorNull.c to clarify that the implementation does not 
exist.

Similar changes are required for other patches as well.

Apart from the above, in patch " DynamicTablesPkg: FdtHwInfoParserLib: Move IRQ 
map to arch folder"
I think the file 
DynamicTablesPkg/Library/FdtHwInfoParserLib/Arm/ArmFdtUtility.c should be 
renamed to 
DynamicTablesPkg/Library/FdtHwInfoParserLib/Arm/ArmFdtInterrupt.c

If you agree with the above, I will make the necessary changes before merging.

With that,

Reviewed-by: Sami Mujawar 

Regards,

Sami Mujawar


On 19/06/2024, 23:06, "Pierre Gondois" mailto:pierre.gond...@arm.com>> wrote:


The DynamicTables framework has mainly been developed/tested against Arm
architecture. While still trying to have re-usable libraries, opening the
framework to other architectures implies some re-organization.


The libraries that are generic enough to be directly re-used are moved
to a Common/ directory. For some libraries, additional arch-specific hooks
have been added to allow architectures specific modifications.


---


Changes can be seen at:
https://github.com/pierregondois/edk2/tree/pg/dyntables_libraries_reorg 



---


References:
1. Staging branch creation:
URL: https://edk2.groups.io/g/devel/message/114790 



2. edk2-staging Repo
URL: https://github.com/tianocore/edk2-staging.git 

Branch Name: dynamictables-reorg


3. edk2-platforms Repo
URL: https://github.com/tianocore/edk2-platforms.git 

Branch Name: devel-dynamictables-reorg


---


Cc: AbdulLateef Attar mailto:abdullateef.at...@amd.com>>
Cc: Girish Mahadevan mailto:gmahade...@nvidia.com>>
Cc: Jeff Brasen mailto:jbra...@nvidia.com>>
Cc: Jeshua Smith mailto:jesh...@nvidia.com>>
Cc: Leif Lindholm mailto:quic_llind...@quicinc.com>>
Cc: Meenakshi Aggarwal mailto:meenakshi.aggar...@nxp.com>>
Cc: Pierre Gondois mailto:pierre.gond...@arm.com>>
Cc: Sami Mujawar mailto:sami.muja...@arm.com>>
Cc: Sunil V L mailto:suni...@ventanamicro.com>>
Cc: Yeo Reum Yun mailto:yeoreum@arm.com>>


Pierre Gondois (15):
DynamicTablesPkg: Acpi: Move generic libraries to common folder
DynamicTablesPkg: Acpi: Prepare common libraries to support other
archs
DynamicTablesPkg: AcpiFadtLib: Prepare to support other archs
DynamicTablesPkg: AcpiDbg2Lib: Prepare to support other archs
DynamicTablesPkg: AcpiSpcrLib: Prepare to support other archs
DynamicTablesPkg: AcpiSratLib: Prepare to support other archs
DynamicTablesPkg: AcpiSsdtCpuTopologyLib: Avoid dependency on GICC
DynamicTablesPkg: DynamicTableManagerDxe: Refactor PresenceArray
DynamicTablesPkg: FdtHwInfoParserLib: Move ARM parsers to Arm
directory
DynamicTablesPkg: FdtHwInfoParserLib: Refactor to prepare for other
archs
DynamicTablesPkg: FdtHwInfoParserLib: Make Pci parser arch neutral
DynamicTablesPkg: FdtHwInfoParserLib: Make Serial Port parser arch
neutral
DynamicTablesPkg: FdtHwInfoParserLib: Move ArmLib.h to ArmGicCParser.c
DynamicTablesPkg: FdtHwInfoParserLib: Move IRQ map to arch folder
DynamicTablesPkg: FdtHwInfoParserLib: Create wrapper to get INTC addr
cells


.../Arm/ArmDynamicTableManagerDxe.c | 63 +++
.../Common/CommonDynamicTableManagerDxe.c | 58 +++
.../DynamicTableManagerDxe.c | 70 +--
.../DynamicTableManagerDxe.h | 63 +++
.../DynamicTableManagerDxe.inf | 7 +
DynamicTablesPkg/DynamicTables.dsc.inc | 64 +--
.../SsdtCpuTopologyGenerator.h | 147 ---
.../AcpiDbg2Lib/AcpiDbg2Lib.inf} | 22 +-
.../Common/AcpiDbg2Lib/Arm/ArmDbg2Generator.c | 67 +++
.../AcpiDbg2Lib/Common/CommonDbg2Generator.c | 59 +++
.../AcpiDbg2Lib}/Dbg2Generator.c | 24 +-
.../Acpi/Common/AcpiDbg2Lib/Dbg2Generator.h | 56 +++
.../AcpiFadtLib/AcpiFadtLib.inf} | 16 +-
.../Common/AcpiFadtLib/Arm/ArmFadtGenerator.c | 126 ++
.../AcpiFadtLib/Common/CommonFadtGenerator.c | 46 ++
.../AcpiFadtLib}/FadtGenerator.c | 86 +---
.../Acpi/Common/AcpiFadtLib/FadtGenerator.h | 35 ++
.../AcpiMcfgLib/AcpiMcfgLib.inf} | 9 +-
.../AcpiMcfgLib}/McfgGenerator.c | 0
.../AcpiPcctLib/AcpiPcctLib.inf} | 2 +-
.../AcpiPcctLib}/PcctGenerator.c | 0
.../AcpiPcctLib}/PcctGenerator.h | 0
.../AcpiPpttLib/AcpiPpttLib.inf} | 2 +-
.../AcpiPpt

Re: [edk2-devel] [staging/dynamictables-reorg PATCH 15/15] DynamicTablesPkg: FdtHwInfoParserLib: Create wrapper to get INTC addr cells

2024-07-03 Thread Sami Mujawar 
Hi Sunil,

Thank you for the review.

> +EFI_STATUS
> +EFIAPI
> +FdtGetIntcAddressCells (
> + IN CONST VOID *Fdt,
> + IN INT32 Node,
> + OUT INT32 *AddressCells, OPTIONAL
> + OUT INT32 *SizeCells OPTIONAL
NIT: I might be wrong but alignment doesn't look correct.
[SAMI] I wonder if this is an artifact of the email client. The alignment looks 
ok to me in visual studio code. 
That being said, I may have run uncrustify on Pierre's patch series as I am 
planning to get it merged soon.
In either case I will get it addressed, thank you for the feedback.
[/SAMI]

Otherwise, LGTM.


Reviewed-by: Sunil V L mailto:suni...@ventanamicro.com>>


Thanks!
Sunil














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