The principles of EDK2 module reconstruction for archs


Chang, Abner
 

[AMD Official Use Only - General]


All,

Today in edk2 open design meeting, we went through the draft of principles of the EDK2 module reconstruction for accommodating different archs (IA32, X64, Arm, AArch64, RiscV64, Loongson64 and etc.) or different vendors of the same arch (AMD/Intel to IA32 and X64).

 

@Ray Ni and @michael.d.kinney@...

  1. We may need somewhere on edk2 repo or another place that can have PR for the easy review, please let me know where I can create the PR instead of reviewing this through dev mailing list.
  2. I didn’t mention using CPUID, Family ID or PCD to have the different code path for AMD and Intel. The reason is,
    1. This decreases the readability of code.
    2. That makes a confusing to the review process

                                                    i.     Says the maintainer/reviewer owns the package, however the patch is specific to AMD implementation but the change is in the file that mixes up Intel and AMD code. Then who is supposed the right person to give the reviewed-by? Perhaps the AMD edk2 module maintainers or reviewers is the proper person to give the reviewed-by for this case. Of course, other maintainers still can join the review process and give the comments. So to separate the arch-specific code in a arch-specific source file simplifies the review, even that is just a small delta between two implementations.

                                                   ii.     We can have the maintainers or reviewers for the entire module or *Amd* files only. So the maintainers/reviewers do not have to review the changes that only made for other archs. But they have to help reviewing the common code if that gets impact.

  1. I didn’t mention to have <phase><arch><basename> for the new module. I prefer we just inherit the original module name or file name so we can know the module or the source file has the different implementation for archs in the file browser (when the files are sorted in alphabet).

 

Lets discuss this using PR if possible.

Thanks

Abner

 

 

Below is the draft of principles:

 

Preface:

The principle is mainly for UefiCpuPKg, but the same principle maybe applied to the EDK2 module that has the processor architecture dependence (such as the BaseLib under MdePkg/Library). Most of the EDK2 modules under UefiCpuPkg were developed specifically to IA32/X64 architecture, that is necessary to reconstruct the folder or revise the source files to accommodate other processor architectures. The EDK2 module reconstruction is also required for accommodating the same-arch-but-different-vendor’s implementation (e.g., Intel and AMD for the X86 based processors). The EDK2 module may be strictly developed based on the specific processor architecture. The new introduced implementation for other processor architectures may consider to have a separate EDK2 module instance. Not all of the EDK2 modules revising can exactly meet the principles listed below, that depends on the delta between the original EDK2 module and the implementation for the new introduced processor architecture. It may require the further discussions with EDK2 module maintainers.

 

The [Arch] refers to the Processor Architecture.

The [Module] refer to the EDK2 module.

The [X86] refers to both IA32 and X64.

 

The principles to create the X86 folder in the module:

  1. When X86-vendor’s implementation is introduced to the existing module:
  1. The folder reconstruction:

A-1. If the module is obviously used by IA32/X64 only

  • No need to create X86 folder
  • Create X86-vendor’s stuff under the root directory of module

A-2. If the existing module is expected to accommodate the different archs or the module already has multiple archs:

  • Create X86 folder
  • Create X86-vendor’s stuff under X86 folder

 

  1. The files reconstruction:

B-1. The module INF metafile

  • The existing INF metafile should be kept without relocation. Should not have the impacts to the existing DSC/FDF file.
  • The new introduced INF metafile should be located under the root directory of module with the file naming format as below. This keeps the consistent module file structure.
    • <OriginalFileName><arch>.inf

 

                     B-2. Source files

                              The new arch implementation is introduced to the module in order to leverage the source code and the module design architecture, so

  • That is contributor’s responsibility to review the source code and strip the arch-dependent code away and put it into the arch-specific file. Leave the common code in the original file if there is no arch-specific and arch-specific-feature wordings in the file name. Create a common file for the common implementation otherwise.
    • The file naming for the arch-specific file

<OriginalFileName ><arch>.*

    • The file naming for the common implementation

< OriginalFileNaming >Common.*

  • That is contributor’s responsibility to relocate the arch-specific source files to the arch-specific folder.
  • That is contributor’s responsibility to make sure the original INF metafile can properly pull-in the source file from arch-specific folder after the source file reconstruction.
  • The common source files should be located under the root directory of module

 

  1. When a new arch’s implementation is introduced to the existing module which was developed for the specific arch:
  1. The folder reconstruction:
  • Create arch folder for the existing arch implementation
  • Create the arch folder for the new introduced arch

 

  1. The files reconstruction:

B-1. The module INF metafile

  • The existing INF file should be kept without the relocation. Should not have the impacts to the existing DSC/FDF file.
  • The new introduced INF metafile should be located under the root directory of module with the file naming format as below. This keeps the consistent module file structure.
    • < OriginalFileNaming><arch>.inf

 

B-2. Source files

                             The new arch implementation is introduced to this module in order to leverage the source code and the module design architecture, so

  • That is contributor’s responsibility to review the source code and strip the arch-dependent code away and put it into the arch-specific file. Leave the common code in the original file if there is no arch-specific wording in the file name. Create a common file for the common implementation otherwise.
    • The file naming for the arch-specific source file

< OriginalFileNaming ><arch>.*

    • The file naming for the common implementation

<OriginalFileNaming>Common.*

  • That is contributor’s responsibility to relocate the arch-specific source files to the arch-specific folder.
  • That is contributor’s responsibility to make sure the original INF metafile can properly pull-in the source file from arch-specific folder after the source file reconstruction.
  • The common source files should be located under the root directory of module

 

  1. When a new arch implementation has a huge delta with the original implementation

Create a separate module instance. The naming of the module should follow below format,

  • Add the arch prefix with the original module name:

< OriginalModuleNaming><arch>

 


Michael D Kinney
 

Hi Abner,

 

I think it would be good to clarify when a difference in implementation is due to a CPU Arch difference or a Vendor implementation difference.

 

I would also be good to provide guidelines for directory names and file names for all EDK II meta data file types.  Here are some examples to get started:

 

Package Directory Name:                             <PackageName>Pkg

Package DEC File Name:                                 <PackageName>Pkg.dec

 

                <PackageName>              REQUIRED           *

 

Module Directory Name:                              <ModuleName><Phase>

                                                                                < Feature >/<Phase>

Module INF File Name:                                  <ModuleName><Phase>.inf

                                                                                < Feature>/<Phase>/<ModuleName>.inf

 

                <Feature>                           OPTIONAL           Only used if implementation does not have any shared code between phases (e.g. MdeModulePkg/Universal/PCD)

                <Phase>                              REQUIRED           Base, Sec, Pei, Dxe, DxeRuntime, Mm, StandaloneMm, Smm, Uefi

                <ModuleName>               REQUIRED           *

 

Library Instance Directory Name:              <Phase><CpuArch><Vendor><LibraryClassName><Dependency>

Library Instance INF File Name:                  <Phase><CpuArch><Vendor><LibraryClassName><Dependency>.inf

 

                <Phase>                              REQUIRED           Base, Sec, Pei, Dxe, DxeRuntime, Mm, StandaloneMm, Smm, Uefi

                <CpuArch>                         OPTIONAL           Ia32, X64, Arm, AArch64, RiscV64, LoongArch64, Ebc                                        If not provided, then component supports >=2 or all CPU archs

                <Vendor>                           OPTIONAL           *

                <LibraryClassName>       REQUIRED           *

                <Dependency>                 OPTIONAL           *             Typically name of PPI, Protocol, LibraryClass that the implementation is layered on top of.

 

Source File Paths within a Library/Module instance

                <FileName>.c

                <FileName>.h

                <CpuArch>/<FileName>.c

                <CpuArch>/<FileName>.h

                <CpuArch>/<FileName>.nasm

                <CpuArch>/<FileName>.S

               

                <CpuArch>                         OPTIONAL           Ia32, X64, Arm, AArch64, RiscV64, LoongArch64, Ebc

 

I think the point you are raising in the discussion is that sometimes there may be shared content between a small subset of CPU archs (e.g. IA32/X64 or Arm/AArch64 or RiscV32/RiscV64/RiscV128) and that you are proposing a new standard directory name for these combinations.  Your proposal is X86 for a directory that contains content for both IA32 and X64.  You are also wanting to support vendor specific content in the naming convention.  An example where it is already being done is in MdePkg/Include/Registers/<VendorName>.   So an enhancement to the above Source File Paths would be:

 

Source File Paths within a Library/Module instance

                <FileName>.c

                <FileName>.h

                <CpuArch>/<FileName>.c

                <CpuArch>/<FileName>.h

                <CpuArch>/<FileName>.nasm

                <CpuArch>/<FileName>.S

                <CpuArch>/<Vendor>/<FileName>.c

                <CpuArch>/<Vendor>/<FileName>.h

                <CpuArch>/<Vendor>/<FileName>.nasm

                <CpuArch>/<Vendor>/<FileName>.S

               

                <CpuArch>                         OPTIONAL           Ia32, X64, Arm, AArch64, RiscV64, LoongArch64, Ebc, X86

                <Vendor>                           OPTIONAL           *

 

I am not sure if we should use “Common” in the naming conventions.  I think by default, any content that is not CpuArch or Vendor specific could be considered common content.

 

Thanks,

 

Mike

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Chang, Abner via groups.io
Sent: Friday, September 23, 2022 8:39 AM
To: devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@...>; Kinney, Michael D <michael.d.kinney@...>; Sunil V L <sunilvl@...>; lichao <lichao@...>; Kirkendall, Garrett <Garrett.Kirkendall@...>; Grimes, Paul <Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm <quic_llindhol@...>; Andrew Fish <afish@...>
Subject: [edk2-devel] The principles of EDK2 module reconstruction for archs

 

[AMD Official Use Only - General]

 

All,

Today in edk2 open design meeting, we went through the draft of principles of the EDK2 module reconstruction for accommodating different archs (IA32, X64, Arm, AArch64, RiscV64, Loongson64 and etc.) or different vendors of the same arch (AMD/Intel to IA32 and X64).

 

@Ray Ni and @michael.d.kinney@...

  1. We may need somewhere on edk2 repo or another place that can have PR for the easy review, please let me know where I can create the PR instead of reviewing this through dev mailing list.
  2. I didn’t mention using CPUID, Family ID or PCD to have the different code path for AMD and Intel. The reason is,
    1. This decreases the readability of code.
    2. That makes a confusing to the review process

                                                               i.      Says the maintainer/reviewer owns the package, however the patch is specific to AMD implementation but the change is in the file that mixes up Intel and AMD code. Then who is supposed the right person to give the reviewed-by? Perhaps the AMD edk2 module maintainers or reviewers is the proper person to give the reviewed-by for this case. Of course, other maintainers still can join the review process and give the comments. So to separate the arch-specific code in a arch-specific source file simplifies the review, even that is just a small delta between two implementations.

                                                             ii.      We can have the maintainers or reviewers for the entire module or *Amd* files only. So the maintainers/reviewers do not have to review the changes that only made for other archs. But they have to help reviewing the common code if that gets impact.

  1. I didn’t mention to have <phase><arch><basename> for the new module. I prefer we just inherit the original module name or file name so we can know the module or the source file has the different implementation for archs in the file browser (when the files are sorted in alphabet).

 

Lets discuss this using PR if possible.

Thanks

Abner

 

 

Below is the draft of principles:

 

Preface:

The principle is mainly for UefiCpuPKg, but the same principle maybe applied to the EDK2 module that has the processor architecture dependence (such as the BaseLib under MdePkg/Library). Most of the EDK2 modules under UefiCpuPkg were developed specifically to IA32/X64 architecture, that is necessary to reconstruct the folder or revise the source files to accommodate other processor architectures. The EDK2 module reconstruction is also required for accommodating the same-arch-but-different-vendor’s implementation (e.g., Intel and AMD for the X86 based processors). The EDK2 module may be strictly developed based on the specific processor architecture. The new introduced implementation for other processor architectures may consider to have a separate EDK2 module instance. Not all of the EDK2 modules revising can exactly meet the principles listed below, that depends on the delta between the original EDK2 module and the implementation for the new introduced processor architecture. It may require the further discussions with EDK2 module maintainers.

 

The [Arch] refers to the Processor Architecture.

The [Module] refer to the EDK2 module.

The [X86] refers to both IA32 and X64.

 

The principles to create the X86 folder in the module:

  1. When X86-vendor’s implementation is introduced to the existing module:
  1. The folder reconstruction:

A-1. If the module is obviously used by IA32/X64 only

  • No need to create X86 folder
  • Create X86-vendor’s stuff under the root directory of module

A-2. If the existing module is expected to accommodate the different archs or the module already has multiple archs:

  • Create X86 folder
  • Create X86-vendor’s stuff under X86 folder

 

  1. The files reconstruction:

B-1. The module INF metafile

  • The existing INF metafile should be kept without relocation. Should not have the impacts to the existing DSC/FDF file.
  • The new introduced INF metafile should be located under the root directory of module with the file naming format as below. This keeps the consistent module file structure.
    • <OriginalFileName><arch>.inf

 

                     B-2. Source files

                              The new arch implementation is introduced to the module in order to leverage the source code and the module design architecture, so

  • That is contributor’s responsibility to review the source code and strip the arch-dependent code away and put it into the arch-specific file. Leave the common code in the original file if there is no arch-specific and arch-specific-feature wordings in the file name. Create a common file for the common implementation otherwise.
    • The file naming for the arch-specific file

<OriginalFileName ><arch>.*

    • The file naming for the common implementation

< OriginalFileNaming >Common.*

  • That is contributor’s responsibility to relocate the arch-specific source files to the arch-specific folder.
  • That is contributor’s responsibility to make sure the original INF metafile can properly pull-in the source file from arch-specific folder after the source file reconstruction.
  • The common source files should be located under the root directory of module

 

  1. When a new arch’s implementation is introduced to the existing module which was developed for the specific arch:
  1. The folder reconstruction:
  • Create arch folder for the existing arch implementation
  • Create the arch folder for the new introduced arch

 

  1. The files reconstruction:

B-1. The module INF metafile

  • The existing INF file should be kept without the relocation. Should not have the impacts to the existing DSC/FDF file.
  • The new introduced INF metafile should be located under the root directory of module with the file naming format as below. This keeps the consistent module file structure.
    • < OriginalFileNaming><arch>.inf

 

B-2. Source files

                             The new arch implementation is introduced to this module in order to leverage the source code and the module design architecture, so

  • That is contributor’s responsibility to review the source code and strip the arch-dependent code away and put it into the arch-specific file. Leave the common code in the original file if there is no arch-specific wording in the file name. Create a common file for the common implementation otherwise.
    • The file naming for the arch-specific source file

< OriginalFileNaming ><arch>.*

    • The file naming for the common implementation

<OriginalFileNaming>Common.*

  • That is contributor’s responsibility to relocate the arch-specific source files to the arch-specific folder.
  • That is contributor’s responsibility to make sure the original INF metafile can properly pull-in the source file from arch-specific folder after the source file reconstruction.
  • The common source files should be located under the root directory of module

 

  1. When a new arch implementation has a huge delta with the original implementation

Create a separate module instance. The naming of the module should follow below format,

  • Add the arch prefix with the original module name:

< OriginalModuleNaming><arch>

 


Chang, Abner
 

[AMD Official Use Only - General]

 

Thanks for the reply Mike,

>>> I think it would be good to clarify when a difference in implementation is due to a CPU Arch difference or a Vendor implementation difference.

Right, we can have a paragraph to clarify the difference of CPU Arch or a vendor implementation of the same CPU Arch.

 

If the difference of CPU Arch or a vendor implementation triggers the module reconstruction; and it is a new module or the delta is huge to share the same module, then the file/module name should follow the naming rule you listed above.

It the difference could be added to the existing module, then I think we just keep the existing naming of the file/module to prevent from introducing the impacts on the existing platform or projects meta files.

 

>>>I am not sure if we should use “Common” in the naming conventions.  I think by default, any content that is not CpuArch or Vendor specific could be considered common content.

Yes agree. The existing file could be a common file if there is no CpuArch or Vendor tag in the file/module name. However, there would be four scenarios,

  1. CpuArch or vendor specific tag in the existing module/file name and some of the code could be leverage by other arch/vendor:

Strip away the share code and put it into new file and name it without arch/vendor tag. We don’t need “common” in the file name.

  1. No CpuArch or vendor specific tag in the existing module/file name and some of the code could be leverage by other arch/vendor:

Strip away the arch/vendor specific code and put it into new file named with arch/vendor tag.

  1. No CpuArch or vendor specific tag in the existing module/file name and the code can be fully leveraged.

Keep it without any change on file/module name.

  1. If the existing file has the “Common” tag, then just keep it as it.

 

How is it?

 

I will revise the doc. I don’t see the good place to create this doc and PR for the review online. I would just create a markdown file under tianocore.github.io/docs just for the temporary. Any other suggestions?

 

Thanks

Abner

 

 

 

From: Kinney, Michael D <michael.d.kinney@...>
Sent: Saturday, September 24, 2022 2:01 AM
To: devel@edk2.groups.io; Chang, Abner <Abner.Chang@...>; Kinney, Michael D <michael.d.kinney@...>
Cc: Ni, Ray <ray.ni@...>; Sunil V L <sunilvl@...>; lichao <lichao@...>; Kirkendall, Garrett <Garrett.Kirkendall@...>; Grimes, Paul <Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm <quic_llindhol@...>; Andrew Fish <afish@...>
Subject: RE: The principles of EDK2 module reconstruction for archs

 

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

 

Hi Abner,

 

I think it would be good to clarify when a difference in implementation is due to a CPU Arch difference or a Vendor implementation difference.

 

I would also be good to provide guidelines for directory names and file names for all EDK II meta data file types.  Here are some examples to get started:

 

Package Directory Name:                          <PackageName>Pkg

Package DEC File Name:                                            <PackageName>Pkg.dec

 

             <PackageName>              REQUIRED           *

 

Module Directory Name:                           <ModuleName><Phase>

                                                                         < Feature >/<Phase>

Module INF File Name:                              <ModuleName><Phase>.inf

                                                                         < Feature>/<Phase>/<ModuleName>.inf

 

             <Feature>                           OPTIONAL           Only used if implementation does not have any shared code between phases (e.g. MdeModulePkg/Universal/PCD)

             <Phase>                              REQUIRED           Base, Sec, Pei, Dxe, DxeRuntime, Mm, StandaloneMm, Smm, Uefi

             <ModuleName>                REQUIRED           *

 

Library Instance Directory Name:             <Phase><CpuArch><Vendor><LibraryClassName><Dependency>

Library Instance INF File Name:              <Phase><CpuArch><Vendor><LibraryClassName><Dependency>.inf

 

             <Phase>                              REQUIRED           Base, Sec, Pei, Dxe, DxeRuntime, Mm, StandaloneMm, Smm, Uefi

             <CpuArch>                         OPTIONAL           Ia32, X64, Arm, AArch64, RiscV64, LoongArch64, Ebc                                        If not provided, then component supports >=2 or all CPU archs

             <Vendor>                           OPTIONAL           *

             <LibraryClassName>        REQUIRED           *

             <Dependency>                  OPTIONAL           *             Typically name of PPI, Protocol, LibraryClass that the implementation is layered on top of.

 

Source File Paths within a Library/Module instance

             <FileName>.c

             <FileName>.h

             <CpuArch>/<FileName>.c

             <CpuArch>/<FileName>.h

             <CpuArch>/<FileName>.nasm

             <CpuArch>/<FileName>.S

            

             <CpuArch>                         OPTIONAL           Ia32, X64, Arm, AArch64, RiscV64, LoongArch64, Ebc

 

I think the point you are raising in the discussion is that sometimes there may be shared content between a small subset of CPU archs (e.g. IA32/X64 or Arm/AArch64 or RiscV32/RiscV64/RiscV128) and that you are proposing a new standard directory name for these combinations.  Your proposal is X86 for a directory that contains content for both IA32 and X64.  You are also wanting to support vendor specific content in the naming convention.  An example where it is already being done is in MdePkg/Include/Registers/<VendorName>.   So an enhancement to the above Source File Paths would be:

 

Source File Paths within a Library/Module instance

             <FileName>.c

             <FileName>.h

             <CpuArch>/<FileName>.c

             <CpuArch>/<FileName>.h

             <CpuArch>/<FileName>.nasm

             <CpuArch>/<FileName>.S

             <CpuArch>/<Vendor>/<FileName>.c

             <CpuArch>/<Vendor>/<FileName>.h

             <CpuArch>/<Vendor>/<FileName>.nasm

             <CpuArch>/<Vendor>/<FileName>.S

            

             <CpuArch>                         OPTIONAL           Ia32, X64, Arm, AArch64, RiscV64, LoongArch64, Ebc, X86

             <Vendor>                           OPTIONAL           *

 

I am not sure if we should use “Common” in the naming conventions.  I think by default, any content that is not CpuArch or Vendor specific could be considered common content.

 

Thanks,

 

Mike

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Chang, Abner via groups.io
Sent: Friday, September 23, 2022 8:39 AM
To: devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@...>; Kinney, Michael D <michael.d.kinney@...>; Sunil V L <sunilvl@...>; lichao <lichao@...>; Kirkendall, Garrett <Garrett.Kirkendall@...>; Grimes, Paul <Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm <quic_llindhol@...>; Andrew Fish <afish@...>
Subject: [edk2-devel] The principles of EDK2 module reconstruction for archs

 

[AMD Official Use Only - General]

 

All,

Today in edk2 open design meeting, we went through the draft of principles of the EDK2 module reconstruction for accommodating different archs (IA32, X64, Arm, AArch64, RiscV64, Loongson64 and etc.) or different vendors of the same arch (AMD/Intel to IA32 and X64).

 

@Ray Ni and @michael.d.kinney@...

  1. We may need somewhere on edk2 repo or another place that can have PR for the easy review, please let me know where I can create the PR instead of reviewing this through dev mailing list.
  2. I didn’t mention using CPUID, Family ID or PCD to have the different code path for AMD and Intel. The reason is,
    1. This decreases the readability of code.
    2. That makes a confusing to the review process

                                                       i.     Says the maintainer/reviewer owns the package, however the patch is specific to AMD implementation but the change is in the file that mixes up Intel and AMD code. Then who is supposed the right person to give the reviewed-by? Perhaps the AMD edk2 module maintainers or reviewers is the proper person to give the reviewed-by for this case. Of course, other maintainers still can join the review process and give the comments. So to separate the arch-specific code in a arch-specific source file simplifies the review, even that is just a small delta between two implementations.

                                                      ii.     We can have the maintainers or reviewers for the entire module or *Amd* files only. So the maintainers/reviewers do not have to review the changes that only made for other archs. But they have to help reviewing the common code if that gets impact.

  1. I didn’t mention to have <phase><arch><basename> for the new module. I prefer we just inherit the original module name or file name so we can know the module or the source file has the different implementation for archs in the file browser (when the files are sorted in alphabet).

 

Lets discuss this using PR if possible.

Thanks

Abner

 

 

Below is the draft of principles:

 

Preface:

The principle is mainly for UefiCpuPKg, but the same principle maybe applied to the EDK2 module that has the processor architecture dependence (such as the BaseLib under MdePkg/Library). Most of the EDK2 modules under UefiCpuPkg were developed specifically to IA32/X64 architecture, that is necessary to reconstruct the folder or revise the source files to accommodate other processor architectures. The EDK2 module reconstruction is also required for accommodating the same-arch-but-different-vendor’s implementation (e.g., Intel and AMD for the X86 based processors). The EDK2 module may be strictly developed based on the specific processor architecture. The new introduced implementation for other processor architectures may consider to have a separate EDK2 module instance. Not all of the EDK2 modules revising can exactly meet the principles listed below, that depends on the delta between the original EDK2 module and the implementation for the new introduced processor architecture. It may require the further discussions with EDK2 module maintainers.

 

The [Arch] refers to the Processor Architecture.

The [Module] refer to the EDK2 module.

The [X86] refers to both IA32 and X64.

 

The principles to create the X86 folder in the module:

  1. When X86-vendor’s implementation is introduced to the existing module:
  1. The folder reconstruction:

A-1. If the module is obviously used by IA32/X64 only

  • No need to create X86 folder
  • Create X86-vendor’s stuff under the root directory of module

A-2. If the existing module is expected to accommodate the different archs or the module already has multiple archs:

  • Create X86 folder
  • Create X86-vendor’s stuff under X86 folder

 

  1. The files reconstruction:

B-1. The module INF metafile

  • The existing INF metafile should be kept without relocation. Should not have the impacts to the existing DSC/FDF file.
  • The new introduced INF metafile should be located under the root directory of module with the file naming format as below. This keeps the consistent module file structure.
    • <OriginalFileName><arch>.inf

 

                     B-2. Source files

                              The new arch implementation is introduced to the module in order to leverage the source code and the module design architecture, so

  • That is contributor’s responsibility to review the source code and strip the arch-dependent code away and put it into the arch-specific file. Leave the common code in the original file if there is no arch-specific and arch-specific-feature wordings in the file name. Create a common file for the common implementation otherwise.
    • The file naming for the arch-specific file

<OriginalFileName ><arch>.*

    • The file naming for the common implementation

< OriginalFileNaming >Common.*

  • That is contributor’s responsibility to relocate the arch-specific source files to the arch-specific folder.
  • That is contributor’s responsibility to make sure the original INF metafile can properly pull-in the source file from arch-specific folder after the source file reconstruction.
  • The common source files should be located under the root directory of module

 

  1. When a new arch’s implementation is introduced to the existing module which was developed for the specific arch:
  1. The folder reconstruction:
  • Create arch folder for the existing arch implementation
  • Create the arch folder for the new introduced arch

 

  1. The files reconstruction:

B-1. The module INF metafile

  • The existing INF file should be kept without the relocation. Should not have the impacts to the existing DSC/FDF file.
  • The new introduced INF metafile should be located under the root directory of module with the file naming format as below. This keeps the consistent module file structure.
    • < OriginalFileNaming><arch>.inf

 

B-2. Source files

                             The new arch implementation is introduced to this module in order to leverage the source code and the module design architecture, so

  • That is contributor’s responsibility to review the source code and strip the arch-dependent code away and put it into the arch-specific file. Leave the common code in the original file if there is no arch-specific wording in the file name. Create a common file for the common implementation otherwise.
    • The file naming for the arch-specific source file

< OriginalFileNaming ><arch>.*

    • The file naming for the common implementation

<OriginalFileNaming>Common.*

  • That is contributor’s responsibility to relocate the arch-specific source files to the arch-specific folder.
  • That is contributor’s responsibility to make sure the original INF metafile can properly pull-in the source file from arch-specific folder after the source file reconstruction.
  • The common source files should be located under the root directory of module

 

  1. When a new arch implementation has a huge delta with the original implementation

Create a separate module instance. The naming of the module should follow below format,

  • Add the arch prefix with the original module name:

< OriginalModuleNaming><arch>

 


Michael D Kinney
 

As far as where this type documentation can go there are a couple options.

 

  1. The EDK II C Coding Standard Specification does provide some rules for directory names and file names.
  2. We could add a EDKII Wiki page that covers this topic
  3. If we want a new published document, we have the tianocore-docs org with support for GitBook syntax documents.

 

Mike

 

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Chang, Abner via groups.io
Sent: Monday, September 26, 2022 12:34 AM
To: Kinney, Michael D <michael.d.kinney@...>; devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@...>; Sunil V L <sunilvl@...>; lichao <lichao@...>; Kirkendall, Garrett <Garrett.Kirkendall@...>; Grimes, Paul <Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm <quic_llindhol@...>; Andrew Fish <afish@...>
Subject: Re: [edk2-devel] The principles of EDK2 module reconstruction for archs

 

[AMD Official Use Only - General]

 

Thanks for the reply Mike,

>>> I think it would be good to clarify when a difference in implementation is due to a CPU Arch difference or a Vendor implementation difference.

Right, we can have a paragraph to clarify the difference of CPU Arch or a vendor implementation of the same CPU Arch.

 

If the difference of CPU Arch or a vendor implementation triggers the module reconstruction; and it is a new module or the delta is huge to share the same module, then the file/module name should follow the naming rule you listed above.

It the difference could be added to the existing module, then I think we just keep the existing naming of the file/module to prevent from introducing the impacts on the existing platform or projects meta files.

 

>>>I am not sure if we should use “Common” in the naming conventions.  I think by default, any content that is not CpuArch or Vendor specific could be considered common content.

Yes agree. The existing file could be a common file if there is no CpuArch or Vendor tag in the file/module name. However, there would be four scenarios,

  1. CpuArch or vendor specific tag in the existing module/file name and some of the code could be leverage by other arch/vendor:

Strip away the share code and put it into new file and name it without arch/vendor tag. We don’t need “common” in the file name.

  1. No CpuArch or vendor specific tag in the existing module/file name and some of the code could be leverage by other arch/vendor:

Strip away the arch/vendor specific code and put it into new file named with arch/vendor tag.

  1. No CpuArch or vendor specific tag in the existing module/file name and the code can be fully leveraged.

Keep it without any change on file/module name.

  1. If the existing file has the “Common” tag, then just keep it as it.

 

How is it?

 

I will revise the doc. I don’t see the good place to create this doc and PR for the review online. I would just create a markdown file under tianocore.github.io/docs just for the temporary. Any other suggestions?

 

Thanks

Abner

 

 

 

From: Kinney, Michael D <michael.d.kinney@...>
Sent: Saturday, September 24, 2022 2:01 AM
To: devel@edk2.groups.io; Chang, Abner <Abner.Chang@...>; Kinney, Michael D <michael.d.kinney@...>
Cc: Ni, Ray <ray.ni@...>; Sunil V L <sunilvl@...>; lichao <lichao@...>; Kirkendall, Garrett <Garrett.Kirkendall@...>; Grimes, Paul <Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm <quic_llindhol@...>; Andrew Fish <afish@...>
Subject: RE: The principles of EDK2 module reconstruction for archs

 

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

 

Hi Abner,

 

I think it would be good to clarify when a difference in implementation is due to a CPU Arch difference or a Vendor implementation difference.

 

I would also be good to provide guidelines for directory names and file names for all EDK II meta data file types.  Here are some examples to get started:

 

Package Directory Name:                          <PackageName>Pkg

Package DEC File Name:                                            <PackageName>Pkg.dec

 

             <PackageName>              REQUIRED           *

 

Module Directory Name:                           <ModuleName><Phase>

                                                                         < Feature >/<Phase>

Module INF File Name:                              <ModuleName><Phase>.inf

                                                                         < Feature>/<Phase>/<ModuleName>.inf

 

             <Feature>                           OPTIONAL           Only used if implementation does not have any shared code between phases (e.g. MdeModulePkg/Universal/PCD)

             <Phase>                              REQUIRED           Base, Sec, Pei, Dxe, DxeRuntime, Mm, StandaloneMm, Smm, Uefi

             <ModuleName>                REQUIRED           *

 

Library Instance Directory Name:             <Phase><CpuArch><Vendor><LibraryClassName><Dependency>

Library Instance INF File Name:              <Phase><CpuArch><Vendor><LibraryClassName><Dependency>.inf

 

             <Phase>                              REQUIRED           Base, Sec, Pei, Dxe, DxeRuntime, Mm, StandaloneMm, Smm, Uefi

             <CpuArch>                         OPTIONAL           Ia32, X64, Arm, AArch64, RiscV64, LoongArch64, Ebc                                        If not provided, then component supports >=2 or all CPU archs

             <Vendor>                           OPTIONAL           *

             <LibraryClassName>        REQUIRED           *

             <Dependency>                  OPTIONAL           *             Typically name of PPI, Protocol, LibraryClass that the implementation is layered on top of.

 

Source File Paths within a Library/Module instance

             <FileName>.c

             <FileName>.h

             <CpuArch>/<FileName>.c

             <CpuArch>/<FileName>.h

             <CpuArch>/<FileName>.nasm

             <CpuArch>/<FileName>.S

            

             <CpuArch>                         OPTIONAL           Ia32, X64, Arm, AArch64, RiscV64, LoongArch64, Ebc

 

I think the point you are raising in the discussion is that sometimes there may be shared content between a small subset of CPU archs (e.g. IA32/X64 or Arm/AArch64 or RiscV32/RiscV64/RiscV128) and that you are proposing a new standard directory name for these combinations.  Your proposal is X86 for a directory that contains content for both IA32 and X64.  You are also wanting to support vendor specific content in the naming convention.  An example where it is already being done is in MdePkg/Include/Registers/<VendorName>.   So an enhancement to the above Source File Paths would be:

 

Source File Paths within a Library/Module instance

             <FileName>.c

             <FileName>.h

             <CpuArch>/<FileName>.c

             <CpuArch>/<FileName>.h

             <CpuArch>/<FileName>.nasm

             <CpuArch>/<FileName>.S

             <CpuArch>/<Vendor>/<FileName>.c

             <CpuArch>/<Vendor>/<FileName>.h

             <CpuArch>/<Vendor>/<FileName>.nasm

             <CpuArch>/<Vendor>/<FileName>.S

            

             <CpuArch>                         OPTIONAL           Ia32, X64, Arm, AArch64, RiscV64, LoongArch64, Ebc, X86

             <Vendor>                           OPTIONAL           *

 

I am not sure if we should use “Common” in the naming conventions.  I think by default, any content that is not CpuArch or Vendor specific could be considered common content.

 

Thanks,

 

Mike

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Chang, Abner via groups.io
Sent: Friday, September 23, 2022 8:39 AM
To: devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@...>; Kinney, Michael D <michael.d.kinney@...>; Sunil V L <sunilvl@...>; lichao <lichao@...>; Kirkendall, Garrett <Garrett.Kirkendall@...>; Grimes, Paul <Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm <quic_llindhol@...>; Andrew Fish <afish@...>
Subject: [edk2-devel] The principles of EDK2 module reconstruction for archs

 

[AMD Official Use Only - General]

 

All,

Today in edk2 open design meeting, we went through the draft of principles of the EDK2 module reconstruction for accommodating different archs (IA32, X64, Arm, AArch64, RiscV64, Loongson64 and etc.) or different vendors of the same arch (AMD/Intel to IA32 and X64).

 

@Ray Ni and @michael.d.kinney@...

  1. We may need somewhere on edk2 repo or another place that can have PR for the easy review, please let me know where I can create the PR instead of reviewing this through dev mailing list.
  2. I didn’t mention using CPUID, Family ID or PCD to have the different code path for AMD and Intel. The reason is,
    1. This decreases the readability of code.
    2. That makes a confusing to the review process

                                                                  i.      Says the maintainer/reviewer owns the package, however the patch is specific to AMD implementation but the change is in the file that mixes up Intel and AMD code. Then who is supposed the right person to give the reviewed-by? Perhaps the AMD edk2 module maintainers or reviewers is the proper person to give the reviewed-by for this case. Of course, other maintainers still can join the review process and give the comments. So to separate the arch-specific code in a arch-specific source file simplifies the review, even that is just a small delta between two implementations.

                                                                ii.      We can have the maintainers or reviewers for the entire module or *Amd* files only. So the maintainers/reviewers do not have to review the changes that only made for other archs. But they have to help reviewing the common code if that gets impact.

  1. I didn’t mention to have <phase><arch><basename> for the new module. I prefer we just inherit the original module name or file name so we can know the module or the source file has the different implementation for archs in the file browser (when the files are sorted in alphabet).

 

Lets discuss this using PR if possible.

Thanks

Abner

 

 

Below is the draft of principles:

 

Preface:

The principle is mainly for UefiCpuPKg, but the same principle maybe applied to the EDK2 module that has the processor architecture dependence (such as the BaseLib under MdePkg/Library). Most of the EDK2 modules under UefiCpuPkg were developed specifically to IA32/X64 architecture, that is necessary to reconstruct the folder or revise the source files to accommodate other processor architectures. The EDK2 module reconstruction is also required for accommodating the same-arch-but-different-vendor’s implementation (e.g., Intel and AMD for the X86 based processors). The EDK2 module may be strictly developed based on the specific processor architecture. The new introduced implementation for other processor architectures may consider to have a separate EDK2 module instance. Not all of the EDK2 modules revising can exactly meet the principles listed below, that depends on the delta between the original EDK2 module and the implementation for the new introduced processor architecture. It may require the further discussions with EDK2 module maintainers.

 

The [Arch] refers to the Processor Architecture.

The [Module] refer to the EDK2 module.

The [X86] refers to both IA32 and X64.

 

The principles to create the X86 folder in the module:

  1. When X86-vendor’s implementation is introduced to the existing module:
  1. The folder reconstruction:

A-1. If the module is obviously used by IA32/X64 only

  • No need to create X86 folder
  • Create X86-vendor’s stuff under the root directory of module

A-2. If the existing module is expected to accommodate the different archs or the module already has multiple archs:

  • Create X86 folder
  • Create X86-vendor’s stuff under X86 folder

 

  1. The files reconstruction:

B-1. The module INF metafile

  • The existing INF metafile should be kept without relocation. Should not have the impacts to the existing DSC/FDF file.
  • The new introduced INF metafile should be located under the root directory of module with the file naming format as below. This keeps the consistent module file structure.
    • <OriginalFileName><arch>.inf

 

                     B-2. Source files

                              The new arch implementation is introduced to the module in order to leverage the source code and the module design architecture, so

  • That is contributor’s responsibility to review the source code and strip the arch-dependent code away and put it into the arch-specific file. Leave the common code in the original file if there is no arch-specific and arch-specific-feature wordings in the file name. Create a common file for the common implementation otherwise.
    • The file naming for the arch-specific file

<OriginalFileName ><arch>.*

    • The file naming for the common implementation

< OriginalFileNaming >Common.*

  • That is contributor’s responsibility to relocate the arch-specific source files to the arch-specific folder.
  • That is contributor’s responsibility to make sure the original INF metafile can properly pull-in the source file from arch-specific folder after the source file reconstruction.
  • The common source files should be located under the root directory of module

 

  1. When a new arch’s implementation is introduced to the existing module which was developed for the specific arch:
  1. The folder reconstruction:
  • Create arch folder for the existing arch implementation
  • Create the arch folder for the new introduced arch

 

  1. The files reconstruction:

B-1. The module INF metafile

  • The existing INF file should be kept without the relocation. Should not have the impacts to the existing DSC/FDF file.
  • The new introduced INF metafile should be located under the root directory of module with the file naming format as below. This keeps the consistent module file structure.
    • < OriginalFileNaming><arch>.inf

 

B-2. Source files

                             The new arch implementation is introduced to this module in order to leverage the source code and the module design architecture, so

  • That is contributor’s responsibility to review the source code and strip the arch-dependent code away and put it into the arch-specific file. Leave the common code in the original file if there is no arch-specific wording in the file name. Create a common file for the common implementation otherwise.
    • The file naming for the arch-specific source file

< OriginalFileNaming ><arch>.*

    • The file naming for the common implementation

<OriginalFileNaming>Common.*

  • That is contributor’s responsibility to relocate the arch-specific source files to the arch-specific folder.
  • That is contributor’s responsibility to make sure the original INF metafile can properly pull-in the source file from arch-specific folder after the source file reconstruction.
  • The common source files should be located under the root directory of module

 

  1. When a new arch implementation has a huge delta with the original implementation

Create a separate module instance. The naming of the module should follow below format,

  • Add the arch prefix with the original module name:

< OriginalModuleNaming><arch>

 


Chang, Abner
 

[AMD Official Use Only - General]

 

Mike how about we take this way,

  • Add a section in EDK II C Coding standard spec for the module naming rule (you listed above). The naming rule covers the modules under edk2 and edk2-platforms.
  • Add a EDKII Wiki page for “The Principles of EDK2 Module Reconstruction for the Processor Architecture”

 

Refer to the Module Naming Rule section in “EDK II C Coding standard spec” for the module reconstruction mentioned in “The Principles of EDK2 Module Reconstruction for the Processor Architecture” doc.

Abner

 

From: Kinney, Michael D <michael.d.kinney@...>
Sent: Monday, September 26, 2022 11:45 PM
To: devel@edk2.groups.io; Chang, Abner <Abner.Chang@...>; Kinney, Michael D <michael.d.kinney@...>
Cc: Ni, Ray <ray.ni@...>; Sunil V L <sunilvl@...>; lichao <lichao@...>; Kirkendall, Garrett <Garrett.Kirkendall@...>; Grimes, Paul <Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm <quic_llindhol@...>; Andrew Fish <afish@...>; Kinney, Michael D <michael.d.kinney@...>
Subject: RE: [edk2-devel] The principles of EDK2 module reconstruction for archs

 

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

 

As far as where this type documentation can go there are a couple options.

 

  1. The EDK II C Coding Standard Specification does provide some rules for directory names and file names.
  2. We could add a EDKII Wiki page that covers this topic
  3. If we want a new published document, we have the tianocore-docs org with support for GitBook syntax documents.

 

Mike

 

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Chang, Abner via groups.io
Sent: Monday, September 26, 2022 12:34 AM
To: Kinney, Michael D <michael.d.kinney@...>; devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@...>; Sunil V L <sunilvl@...>; lichao <lichao@...>; Kirkendall, Garrett <Garrett.Kirkendall@...>; Grimes, Paul <Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm <quic_llindhol@...>; Andrew Fish <afish@...>
Subject: Re: [edk2-devel] The principles of EDK2 module reconstruction for archs

 

[AMD Official Use Only - General]

 

Thanks for the reply Mike,

>>> I think it would be good to clarify when a difference in implementation is due to a CPU Arch difference or a Vendor implementation difference.

Right, we can have a paragraph to clarify the difference of CPU Arch or a vendor implementation of the same CPU Arch.

 

If the difference of CPU Arch or a vendor implementation triggers the module reconstruction; and it is a new module or the delta is huge to share the same module, then the file/module name should follow the naming rule you listed above.

It the difference could be added to the existing module, then I think we just keep the existing naming of the file/module to prevent from introducing the impacts on the existing platform or projects meta files.

 

>>>I am not sure if we should use “Common” in the naming conventions.  I think by default, any content that is not CpuArch or Vendor specific could be considered common content.

Yes agree. The existing file could be a common file if there is no CpuArch or Vendor tag in the file/module name. However, there would be four scenarios,

  1. CpuArch or vendor specific tag in the existing module/file name and some of the code could be leverage by other arch/vendor:

Strip away the share code and put it into new file and name it without arch/vendor tag. We don’t need “common” in the file name.

  1. No CpuArch or vendor specific tag in the existing module/file name and some of the code could be leverage by other arch/vendor:

Strip away the arch/vendor specific code and put it into new file named with arch/vendor tag.

  1. No CpuArch or vendor specific tag in the existing module/file name and the code can be fully leveraged.

Keep it without any change on file/module name.

  1. If the existing file has the “Common” tag, then just keep it as it.

 

How is it?

 

I will revise the doc. I don’t see the good place to create this doc and PR for the review online. I would just create a markdown file under tianocore.github.io/docs just for the temporary. Any other suggestions?

 

Thanks

Abner

 

 

 

From: Kinney, Michael D <michael.d.kinney@...>
Sent: Saturday, September 24, 2022 2:01 AM
To: devel@edk2.groups.io; Chang, Abner <Abner.Chang@...>; Kinney, Michael D <michael.d.kinney@...>
Cc: Ni, Ray <ray.ni@...>; Sunil V L <sunilvl@...>; lichao <lichao@...>; Kirkendall, Garrett <Garrett.Kirkendall@...>; Grimes, Paul <Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm <quic_llindhol@...>; Andrew Fish <afish@...>
Subject: RE: The principles of EDK2 module reconstruction for archs

 

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

 

Hi Abner,

 

I think it would be good to clarify when a difference in implementation is due to a CPU Arch difference or a Vendor implementation difference.

 

I would also be good to provide guidelines for directory names and file names for all EDK II meta data file types.  Here are some examples to get started:

 

Package Directory Name:                          <PackageName>Pkg

Package DEC File Name:                                            <PackageName>Pkg.dec

 

             <PackageName>              REQUIRED           *

 

Module Directory Name:                           <ModuleName><Phase>

                                                                         < Feature >/<Phase>

Module INF File Name:                              <ModuleName><Phase>.inf

                                                                         < Feature>/<Phase>/<ModuleName>.inf

 

             <Feature>                           OPTIONAL           Only used if implementation does not have any shared code between phases (e.g. MdeModulePkg/Universal/PCD)

             <Phase>                              REQUIRED           Base, Sec, Pei, Dxe, DxeRuntime, Mm, StandaloneMm, Smm, Uefi

             <ModuleName>                REQUIRED           *

 

Library Instance Directory Name:             <Phase><CpuArch><Vendor><LibraryClassName><Dependency>

Library Instance INF File Name:              <Phase><CpuArch><Vendor><LibraryClassName><Dependency>.inf

 

             <Phase>                              REQUIRED           Base, Sec, Pei, Dxe, DxeRuntime, Mm, StandaloneMm, Smm, Uefi

             <CpuArch>                         OPTIONAL           Ia32, X64, Arm, AArch64, RiscV64, LoongArch64, Ebc                                        If not provided, then component supports >=2 or all CPU archs

             <Vendor>                           OPTIONAL           *

             <LibraryClassName>        REQUIRED           *

             <Dependency>                  OPTIONAL           *             Typically name of PPI, Protocol, LibraryClass that the implementation is layered on top of.

 

Source File Paths within a Library/Module instance

             <FileName>.c

             <FileName>.h

             <CpuArch>/<FileName>.c

             <CpuArch>/<FileName>.h

             <CpuArch>/<FileName>.nasm

             <CpuArch>/<FileName>.S

            

             <CpuArch>                         OPTIONAL           Ia32, X64, Arm, AArch64, RiscV64, LoongArch64, Ebc

 

I think the point you are raising in the discussion is that sometimes there may be shared content between a small subset of CPU archs (e.g. IA32/X64 or Arm/AArch64 or RiscV32/RiscV64/RiscV128) and that you are proposing a new standard directory name for these combinations.  Your proposal is X86 for a directory that contains content for both IA32 and X64.  You are also wanting to support vendor specific content in the naming convention.  An example where it is already being done is in MdePkg/Include/Registers/<VendorName>.   So an enhancement to the above Source File Paths would be:

 

Source File Paths within a Library/Module instance

             <FileName>.c

             <FileName>.h

             <CpuArch>/<FileName>.c

             <CpuArch>/<FileName>.h

             <CpuArch>/<FileName>.nasm

             <CpuArch>/<FileName>.S

             <CpuArch>/<Vendor>/<FileName>.c

             <CpuArch>/<Vendor>/<FileName>.h

             <CpuArch>/<Vendor>/<FileName>.nasm

             <CpuArch>/<Vendor>/<FileName>.S

            

             <CpuArch>                         OPTIONAL           Ia32, X64, Arm, AArch64, RiscV64, LoongArch64, Ebc, X86

             <Vendor>                           OPTIONAL           *

 

I am not sure if we should use “Common” in the naming conventions.  I think by default, any content that is not CpuArch or Vendor specific could be considered common content.

 

Thanks,

 

Mike

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Chang, Abner via groups.io
Sent: Friday, September 23, 2022 8:39 AM
To: devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@...>; Kinney, Michael D <michael.d.kinney@...>; Sunil V L <sunilvl@...>; lichao <lichao@...>; Kirkendall, Garrett <Garrett.Kirkendall@...>; Grimes, Paul <Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm <quic_llindhol@...>; Andrew Fish <afish@...>
Subject: [edk2-devel] The principles of EDK2 module reconstruction for archs

 

[AMD Official Use Only - General]

 

All,

Today in edk2 open design meeting, we went through the draft of principles of the EDK2 module reconstruction for accommodating different archs (IA32, X64, Arm, AArch64, RiscV64, Loongson64 and etc.) or different vendors of the same arch (AMD/Intel to IA32 and X64).

 

@Ray Ni and @michael.d.kinney@...

  1. We may need somewhere on edk2 repo or another place that can have PR for the easy review, please let me know where I can create the PR instead of reviewing this through dev mailing list.
  2. I didn’t mention using CPUID, Family ID or PCD to have the different code path for AMD and Intel. The reason is,
    1. This decreases the readability of code.
    2. That makes a confusing to the review process

                                                          i.     Says the maintainer/reviewer owns the package, however the patch is specific to AMD implementation but the change is in the file that mixes up Intel and AMD code. Then who is supposed the right person to give the reviewed-by? Perhaps the AMD edk2 module maintainers or reviewers is the proper person to give the reviewed-by for this case. Of course, other maintainers still can join the review process and give the comments. So to separate the arch-specific code in a arch-specific source file simplifies the review, even that is just a small delta between two implementations.

                                                        ii.     We can have the maintainers or reviewers for the entire module or *Amd* files only. So the maintainers/reviewers do not have to review the changes that only made for other archs. But they have to help reviewing the common code if that gets impact.

  1. I didn’t mention to have <phase><arch><basename> for the new module. I prefer we just inherit the original module name or file name so we can know the module or the source file has the different implementation for archs in the file browser (when the files are sorted in alphabet).

 

Lets discuss this using PR if possible.

Thanks

Abner

 

 

Below is the draft of principles:

 

Preface:

The principle is mainly for UefiCpuPKg, but the same principle maybe applied to the EDK2 module that has the processor architecture dependence (such as the BaseLib under MdePkg/Library). Most of the EDK2 modules under UefiCpuPkg were developed specifically to IA32/X64 architecture, that is necessary to reconstruct the folder or revise the source files to accommodate other processor architectures. The EDK2 module reconstruction is also required for accommodating the same-arch-but-different-vendor’s implementation (e.g., Intel and AMD for the X86 based processors). The EDK2 module may be strictly developed based on the specific processor architecture. The new introduced implementation for other processor architectures may consider to have a separate EDK2 module instance. Not all of the EDK2 modules revising can exactly meet the principles listed below, that depends on the delta between the original EDK2 module and the implementation for the new introduced processor architecture. It may require the further discussions with EDK2 module maintainers.

 

The [Arch] refers to the Processor Architecture.

The [Module] refer to the EDK2 module.

The [X86] refers to both IA32 and X64.

 

The principles to create the X86 folder in the module:

  1. When X86-vendor’s implementation is introduced to the existing module:
  1. The folder reconstruction:

A-1. If the module is obviously used by IA32/X64 only

  • No need to create X86 folder
  • Create X86-vendor’s stuff under the root directory of module

A-2. If the existing module is expected to accommodate the different archs or the module already has multiple archs:

  • Create X86 folder
  • Create X86-vendor’s stuff under X86 folder

 

  1. The files reconstruction:

B-1. The module INF metafile

  • The existing INF metafile should be kept without relocation. Should not have the impacts to the existing DSC/FDF file.
  • The new introduced INF metafile should be located under the root directory of module with the file naming format as below. This keeps the consistent module file structure.
    • <OriginalFileName><arch>.inf

 

                     B-2. Source files

                              The new arch implementation is introduced to the module in order to leverage the source code and the module design architecture, so

  • That is contributor’s responsibility to review the source code and strip the arch-dependent code away and put it into the arch-specific file. Leave the common code in the original file if there is no arch-specific and arch-specific-feature wordings in the file name. Create a common file for the common implementation otherwise.
    • The file naming for the arch-specific file

<OriginalFileName ><arch>.*

    • The file naming for the common implementation

< OriginalFileNaming >Common.*

  • That is contributor’s responsibility to relocate the arch-specific source files to the arch-specific folder.
  • That is contributor’s responsibility to make sure the original INF metafile can properly pull-in the source file from arch-specific folder after the source file reconstruction.
  • The common source files should be located under the root directory of module

 

  1. When a new arch’s implementation is introduced to the existing module which was developed for the specific arch:
  1. The folder reconstruction:
  • Create arch folder for the existing arch implementation
  • Create the arch folder for the new introduced arch

 

  1. The files reconstruction:

B-1. The module INF metafile

  • The existing INF file should be kept without the relocation. Should not have the impacts to the existing DSC/FDF file.
  • The new introduced INF metafile should be located under the root directory of module with the file naming format as below. This keeps the consistent module file structure.
    • < OriginalFileNaming><arch>.inf

 

B-2. Source files

                             The new arch implementation is introduced to this module in order to leverage the source code and the module design architecture, so

  • That is contributor’s responsibility to review the source code and strip the arch-dependent code away and put it into the arch-specific file. Leave the common code in the original file if there is no arch-specific wording in the file name. Create a common file for the common implementation otherwise.
    • The file naming for the arch-specific source file

< OriginalFileNaming ><arch>.*

    • The file naming for the common implementation

<OriginalFileNaming>Common.*

  • That is contributor’s responsibility to relocate the arch-specific source files to the arch-specific folder.
  • That is contributor’s responsibility to make sure the original INF metafile can properly pull-in the source file from arch-specific folder after the source file reconstruction.
  • The common source files should be located under the root directory of module

 

  1. When a new arch implementation has a huge delta with the original implementation

Create a separate module instance. The naming of the module should follow below format,

  • Add the arch prefix with the original module name:

< OriginalModuleNaming><arch>

 


Ni, Ray
 

The [Arch] refers to the Processor Architecture.

The [Module] refer to the EDK2 module.

The [X86] refers to both IA32 and X64.

The principles to create the X86 folder in the module:

  1. When X86-vendor’s implementation is introduced to the existing module:
  1. The folder reconstruction:

A-1. If the module is obviously used by IA32/X64 only

  • No need to create X86 folder
  • Create X86-vendor’s stuff under the root directory of module

A-2. If the existing module is expected to accommodate the different archs or the module already has multiple archs:

  • Create X86 folder
  • Create X86-vendor’s stuff under X86 folder

[Ray] Looks good.

 

  1. The files reconstruction:

B-1. The module INF metafile

  • The existing INF metafile should be kept without relocation. Should not have the impacts to the existing DSC/FDF file.
  • The new introduced INF metafile should be located under the root directory of module with the file naming format as below. This keeps the consistent module file structure.
    • <OriginalFileName><arch>.inf

[Ray]  “CpuDxe.inf” and “CpuDxeArm.inf”? is that your intention? New developers may be confused that CpuDxe.inf supports only X86 arch.

Do you have an example that one module supporting multiple archs using different INF files? MdeModulePkg/DxeIpl is a module supporting different archs with the same INF file.

Or shall we leave it to be decided between the patch contributor and package/module maintainer?

 

                     B-2. Source files

                              The new arch implementation is introduced to the module in order to leverage the source code and the module design architecture, so

  • That is contributor’s responsibility to review the source code and strip the arch-dependent code away and put it into the arch-specific file. Leave the common code in the original file if there is no arch-specific and arch-specific-feature wordings in the file name. Create a common file for the common implementation otherwise.
    • The file naming for the arch-specific file

<OriginalFileName ><arch>.*

    • The file naming for the common implementation

< OriginalFileNaming >Common.*

  • That is contributor’s responsibility to relocate the arch-specific source files to the arch-specific folder.
  • That is contributor’s responsibility to make sure the original INF metafile can properly pull-in the source file from arch-specific folder after the source file reconstruction.
  • The common source files should be located under the root directory of module

[Ray] If you check the MpInitLib, most of SEV logic are moved to AmdSev.c. But MpLib.c still contains logic to call functions from AmdSev.c based on some AMD specific check. In my opinion, that’s already good enough.

However, if you check MdeModulePkg/DxeIpl, implementations for different archs are in different *folders*.

Can we leave this to the module owner to decide how to place the implementations?

 

 

  1. When a new arch’s implementation is introduced to the existing module which was developed for the specific arch:
  1. The folder reconstruction:
  • Create arch folder for the existing arch implementation

[Ray] Do you move existing arch implementation to that arch folder? It will break existing platforms a lot.

  • Create the arch folder for the new introduced arch

[Ray] I agree. But if we don’t create arch folder for existing arch implementation, the pkg layout will be a mess.

 

[Ray] Hard for me to understand all the principles here. Maybe we review existing code including to-be-upstreamed code and decide how to go.

 

  1. The files reconstruction:

B-1. The module INF metafile

  • The existing INF file should be kept without the relocation. Should not have the impacts to the existing DSC/FDF file.
  • The new introduced INF metafile should be located under the root directory of module with the file naming format as below. This keeps the consistent module file structure.
    • < OriginalFileNaming><arch>.inf

 

B-2. Source files

                             The new arch implementation is introduced to this module in order to leverage the source code and the module design architecture, so

  • That is contributor’s responsibility to review the source code and strip the arch-dependent code away and put it into the arch-specific file. Leave the common code in the original file if there is no arch-specific wording in the file name. Create a common file for the common implementation otherwise.
    • The file naming for the arch-specific source file

< OriginalFileNaming ><arch>.*

    • The file naming for the common implementation

<OriginalFileNaming>Common.*

  • That is contributor’s responsibility to relocate the arch-specific source files to the arch-specific folder.
  • That is contributor’s responsibility to make sure the original INF metafile can properly pull-in the source file from arch-specific folder after the source file reconstruction.
  • The common source files should be located under the root directory of module

 

  1. When a new arch implementation has a huge delta with the original implementation

Create a separate module instance. The naming of the module should follow below format,

  • Add the arch prefix with the original module name:

< OriginalModuleNaming><arch>

 


Ni, Ray
 

Mike,

Has following content already been documented somewhere?

It looks good to me. Very good abstraction of existing cases. Maybe there are some lib/modules that don’t follow this rule. But the number should be very small.

 

But I didn’t check how ARM constructs the pkg. So it’s very welcomed to see non-X86 people for review.

 

Thanks,

Ray

 

From: Kinney, Michael D <michael.d.kinney@...>
Sent: Saturday, September 24, 2022 2:01 AM
To: devel@edk2.groups.io; abner.chang@...; Kinney, Michael D <michael.d.kinney@...>
Cc: Ni, Ray <ray.ni@...>; Sunil V L <sunilvl@...>; lichao <lichao@...>; Kirkendall, Garrett <Garrett.Kirkendall@...>; Grimes, Paul <Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm <quic_llindhol@...>; Andrew Fish <afish@...>
Subject: RE: The principles of EDK2 module reconstruction for archs

 

Hi Abner,

 

I think it would be good to clarify when a difference in implementation is due to a CPU Arch difference or a Vendor implementation difference.

 

I would also be good to provide guidelines for directory names and file names for all EDK II meta data file types.  Here are some examples to get started:

 

Package Directory Name:                           <PackageName>Pkg

Package DEC File Name:                                             <PackageName>Pkg.dec

 

              <PackageName>              REQUIRED           *

 

Module Directory Name:                            <ModuleName><Phase>

                                                                              < Feature >/<Phase>

Module INF File Name:                                <ModuleName><Phase>.inf

                                                                              < Feature>/<Phase>/<ModuleName>.inf

 

              <Feature>                           OPTIONAL           Only used if implementation does not have any shared code between phases (e.g. MdeModulePkg/Universal/PCD)

              <Phase>                              REQUIRED           Base, Sec, Pei, Dxe, DxeRuntime, Mm, StandaloneMm, Smm, Uefi

              <ModuleName>               REQUIRED           *

 

[Ray] Looks good to me. Good abstraction of existing cases.

 

Library Instance Directory Name:            <Phase><CpuArch><Vendor><LibraryClassName><Dependency>

Library Instance INF File Name:                               <Phase><CpuArch><Vendor><LibraryClassName><Dependency>.inf

 

              <Phase>                              REQUIRED           Base, Sec, Pei, Dxe, DxeRuntime, Mm, StandaloneMm, Smm, Uefi

              <CpuArch>                         OPTIONAL           Ia32, X64, Arm, AArch64, RiscV64, LoongArch64, Ebc                                      If not provided, then component supports >=2 or all CPU archs

              <Vendor>                           OPTIONAL           *

              <LibraryClassName>       REQUIRED           *

              <Dependency>                 OPTIONAL           *             Typically name of PPI, Protocol, LibraryClass that the implementation is layered on top of.

 

Source File Paths within a Library/Module instance

              <FileName>.c

              <FileName>.h

              <CpuArch>/<FileName>.c

              <CpuArch>/<FileName>.h

              <CpuArch>/<FileName>.nasm

              <CpuArch>/<FileName>.S

             

              <CpuArch>                         OPTIONAL           Ia32, X64, Arm, AArch64, RiscV64, LoongArch64, Ebc

[Ray] Looks good to me. Good abstraction of existing cases.

 

 

I think the point you are raising in the discussion is that sometimes there may be shared content between a small subset of CPU archs (e.g. IA32/X64 or Arm/AArch64 or RiscV32/RiscV64/RiscV128) and that you are proposing a new standard directory name for these combinations.  Your proposal is X86 for a directory that contains content for both IA32 and X64.  You are also wanting to support vendor specific content in the naming convention.  An example where it is already being done is in MdePkg/Include/Registers/<VendorName>.   So an enhancement to the above Source File Paths would be:

 

Source File Paths within a Library/Module instance

              <FileName>.c

              <FileName>.h

              <CpuArch>/<FileName>.c

              <CpuArch>/<FileName>.h

              <CpuArch>/<FileName>.nasm

              <CpuArch>/<FileName>.S

              <CpuArch>/<Vendor>/<FileName>.c

              <CpuArch>/<Vendor>/<FileName>.h

              <CpuArch>/<Vendor>/<FileName>.nasm

              <CpuArch>/<Vendor>/<FileName>.S

             

              <CpuArch>                         OPTIONAL           Ia32, X64, Arm, AArch64, RiscV64, LoongArch64, Ebc, X86

              <Vendor>                           OPTIONAL           *

 

I am not sure if we should use “Common” in the naming conventions.  I think by default, any content that is not CpuArch or Vendor specific could be considered common content.

 

[Ray] Good to me.

 

Thanks,

 

Mike

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Chang, Abner via groups.io
Sent: Friday, September 23, 2022 8:39 AM
To: devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@...>; Kinney, Michael D <michael.d.kinney@...>; Sunil V L <sunilvl@...>; lichao <lichao@...>; Kirkendall, Garrett <Garrett.Kirkendall@...>; Grimes, Paul <Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm <quic_llindhol@...>; Andrew Fish <afish@...>
Subject: [edk2-devel] The principles of EDK2 module reconstruction for archs

 

[AMD Official Use Only - General]

 

All,

Today in edk2 open design meeting, we went through the draft of principles of the EDK2 module reconstruction for accommodating different archs (IA32, X64, Arm, AArch64, RiscV64, Loongson64 and etc.) or different vendors of the same arch (AMD/Intel to IA32 and X64).

 

@Ray Ni and @michael.d.kinney@...

  1. We may need somewhere on edk2 repo or another place that can have PR for the easy review, please let me know where I can create the PR instead of reviewing this through dev mailing list.
  2. I didn’t mention using CPUID, Family ID or PCD to have the different code path for AMD and Intel. The reason is,
    1. This decreases the readability of code.
    2. That makes a confusing to the review process

                                                                  i.      Says the maintainer/reviewer owns the package, however the patch is specific to AMD implementation but the change is in the file that mixes up Intel and AMD code. Then who is supposed the right person to give the reviewed-by? Perhaps the AMD edk2 module maintainers or reviewers is the proper person to give the reviewed-by for this case. Of course, other maintainers still can join the review process and give the comments. So to separate the arch-specific code in a arch-specific source file simplifies the review, even that is just a small delta between two implementations.

                                                                ii.      We can have the maintainers or reviewers for the entire module or *Amd* files only. So the maintainers/reviewers do not have to review the changes that only made for other archs. But they have to help reviewing the common code if that gets impact.

  1. I didn’t mention to have <phase><arch><basename> for the new module. I prefer we just inherit the original module name or file name so we can know the module or the source file has the different implementation for archs in the file browser (when the files are sorted in alphabet).

 

Lets discuss this using PR if possible.

Thanks

Abner

 

 

Below is the draft of principles:

 

Preface:

The principle is mainly for UefiCpuPKg, but the same principle maybe applied to the EDK2 module that has the processor architecture dependence (such as the BaseLib under MdePkg/Library). Most of the EDK2 modules under UefiCpuPkg were developed specifically to IA32/X64 architecture, that is necessary to reconstruct the folder or revise the source files to accommodate other processor architectures. The EDK2 module reconstruction is also required for accommodating the same-arch-but-different-vendor’s implementation (e.g., Intel and AMD for the X86 based processors). The EDK2 module may be strictly developed based on the specific processor architecture. The new introduced implementation for other processor architectures may consider to have a separate EDK2 module instance. Not all of the EDK2 modules revising can exactly meet the principles listed below, that depends on the delta between the original EDK2 module and the implementation for the new introduced processor architecture. It may require the further discussions with EDK2 module maintainers.

 

The [Arch] refers to the Processor Architecture.

The [Module] refer to the EDK2 module.

The [X86] refers to both IA32 and X64.

 

The principles to create the X86 folder in the module:

  1. When X86-vendor’s implementation is introduced to the existing module:
  1. The folder reconstruction:

A-1. If the module is obviously used by IA32/X64 only

  • No need to create X86 folder
  • Create X86-vendor’s stuff under the root directory of module

A-2. If the existing module is expected to accommodate the different archs or the module already has multiple archs:

  • Create X86 folder
  • Create X86-vendor’s stuff under X86 folder

 

  1. The files reconstruction:

B-1. The module INF metafile

  • The existing INF metafile should be kept without relocation. Should not have the impacts to the existing DSC/FDF file.
  • The new introduced INF metafile should be located under the root directory of module with the file naming format as below. This keeps the consistent module file structure.
    • <OriginalFileName><arch>.inf

 

                     B-2. Source files

                              The new arch implementation is introduced to the module in order to leverage the source code and the module design architecture, so

  • That is contributor’s responsibility to review the source code and strip the arch-dependent code away and put it into the arch-specific file. Leave the common code in the original file if there is no arch-specific and arch-specific-feature wordings in the file name. Create a common file for the common implementation otherwise.
    • The file naming for the arch-specific file

<OriginalFileName ><arch>.*

    • The file naming for the common implementation

< OriginalFileNaming >Common.*

  • That is contributor’s responsibility to relocate the arch-specific source files to the arch-specific folder.
  • That is contributor’s responsibility to make sure the original INF metafile can properly pull-in the source file from arch-specific folder after the source file reconstruction.
  • The common source files should be located under the root directory of module

 

  1. When a new arch’s implementation is introduced to the existing module which was developed for the specific arch:
  1. The folder reconstruction:
  • Create arch folder for the existing arch implementation
  • Create the arch folder for the new introduced arch

 

  1. The files reconstruction:

B-1. The module INF metafile

  • The existing INF file should be kept without the relocation. Should not have the impacts to the existing DSC/FDF file.
  • The new introduced INF metafile should be located under the root directory of module with the file naming format as below. This keeps the consistent module file structure.
    • < OriginalFileNaming><arch>.inf

 

B-2. Source files

                             The new arch implementation is introduced to this module in order to leverage the source code and the module design architecture, so

  • That is contributor’s responsibility to review the source code and strip the arch-dependent code away and put it into the arch-specific file. Leave the common code in the original file if there is no arch-specific wording in the file name. Create a common file for the common implementation otherwise.
    • The file naming for the arch-specific source file

< OriginalFileNaming ><arch>.*

    • The file naming for the common implementation

<OriginalFileNaming>Common.*

  • That is contributor’s responsibility to relocate the arch-specific source files to the arch-specific folder.
  • That is contributor’s responsibility to make sure the original INF metafile can properly pull-in the source file from arch-specific folder after the source file reconstruction.
  • The common source files should be located under the root directory of module

 

  1. When a new arch implementation has a huge delta with the original implementation

Create a separate module instance. The naming of the module should follow below format,

  • Add the arch prefix with the original module name:

< OriginalModuleNaming><arch>

 


Chang, Abner
 

[AMD Official Use Only - General]

 

 

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray via groups.io
Sent: Wednesday, September 28, 2022 11:34 AM
To: devel@edk2.groups.io; Chang, Abner <Abner.Chang@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; Sunil V L <sunilvl@...>; lichao <lichao@...>; Kirkendall, Garrett <Garrett.Kirkendall@...>; Grimes, Paul <Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm <quic_llindhol@...>; Andrew Fish <afish@...>
Subject: Re: [edk2-devel] The principles of EDK2 module reconstruction for archs

 

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

 

The [Arch] refers to the Processor Architecture.

The [Module] refer to the EDK2 module.

The [X86] refers to both IA32 and X64.

The principles to create the X86 folder in the module:

  1. When X86-vendor’s implementation is introduced to the existing module:
  1. The folder reconstruction:

A-1. If the module is obviously used by IA32/X64 only

  • No need to create X86 folder
  • Create X86-vendor’s stuff under the root directory of module

A-2. If the existing module is expected to accommodate the different archs or the module already has multiple archs:

  • Create X86 folder
  • Create X86-vendor’s stuff under X86 folder

[Ray] Looks good.

 

  1. The files reconstruction:

B-1. The module INF metafile

  • The existing INF metafile should be kept without relocation. Should not have the impacts to the existing DSC/FDF file.
  • The new introduced INF metafile should be located under the root directory of module with the file naming format as below. This keeps the consistent module file structure.
    • <OriginalFileName><arch>.inf

[Ray]  “CpuDxe.inf” and “CpuDxeArm.inf”? is that your intention? New developers may be confused that CpuDxe.inf supports only X86 arch.

Do you have an example that one module supporting multiple archs using different INF files? MdeModulePkg/DxeIpl is a module supporting different archs with the same INF file.

Or shall we leave it to be decided between the patch contributor and package/module maintainer?

[Chang, Abner]  Here I mean the library module, for example SmmCpuFeatureLib.inf and AmdSmmCpuFeatureLib.inf. Will make the statement clear. The file naming above would be changed to <arch><OriginalFileName>.inf as Mike suggested.

 

I am not sure if there is another example having arch-specific INF file. Usually the driver module has the abstract interface and the implementation in the library module. A newly introduced processor architecture driver may create it’s own module such as ArmCpuDxe if the delta between implementations  is huge. However, I would prefer to have arch-specific INF for the module if the module implementation can be leveraged. And yes, this requires the discussion between contributor and module maintainer if the principles can not perfectly identify the case.

 

                     B-2. Source files

                              The new arch implementation is introduced to the module in order to leverage the source code and the module design architecture, so

  • That is contributor’s responsibility to review the source code and strip the arch-dependent code away and put it into the arch-specific file. Leave the common code in the original file if there is no arch-specific and arch-specific-feature wordings in the file name. Create a common file for the common implementation otherwise.
    • The file naming for the arch-specific file

<OriginalFileName ><arch>.*

    • The file naming for the common implementation

< OriginalFileNaming >Common.*

  • That is contributor’s responsibility to relocate the arch-specific source files to the arch-specific folder.
  • That is contributor’s responsibility to make sure the original INF metafile can properly pull-in the source file from arch-specific folder after the source file reconstruction.
  • The common source files should be located under the root directory of module

[Ray] If you check the MpInitLib, most of SEV logic are moved to AmdSev.c. But MpLib.c still contains logic to call functions from AmdSev.c based on some AMD specific check. In my opinion, that’s already good enough.

[Chang, Abner]  I am not quite lean to the If-AMD-else in the source file solution. I prefer to separate AMD stuff or vendor feature to a separated file. So we can have the reviewer or maintainer for *Amd* files/module/packages specifically. This makes the review responsibility clear and efficient.

 

However, if you check MdeModulePkg/DxeIpl, implementations for different archs are in different *folders*.

Can we leave this to the module owner to decide how to place the implementations?

 

 

 

  1. When a new arch’s implementation is introduced to the existing module which was developed for the specific arch:
  1. The folder reconstruction:
  • Create arch folder for the existing arch implementation

[Ray] Do you move existing arch implementation to that arch folder? It will break existing platforms a lot.

[Chang, Abner] We will move the arch implementation to the arch folder without moving INF. This wont impact the platform DSC and FDF, however this has the impact to the existing overwrite.

  • Create the arch folder for the new introduced arch

[Ray] I agree. But if we don’t create arch folder for existing arch implementation, the pkg layout will be a mess.

[Chang, Abner] right, so the first bullet is important.

 

[Ray] Hard for me to understand all the principles here. Maybe we review existing code including to-be-upstreamed code and decide how to go.

 

  1. The files reconstruction:

B-1. The module INF metafile

  • The existing INF file should be kept without the relocation. Should not have the impacts to the existing DSC/FDF file.
  • The new introduced INF metafile should be located under the root directory of module with the file naming format as below. This keeps the consistent module file structure.
    • < OriginalFileNaming><arch>.inf

 

B-2. Source files

                             The new arch implementation is introduced to this module in order to leverage the source code and the module design architecture, so

  • That is contributor’s responsibility to review the source code and strip the arch-dependent code away and put it into the arch-specific file. Leave the common code in the original file if there is no arch-specific wording in the file name. Create a common file for the common implementation otherwise.
    • The file naming for the arch-specific source file

< OriginalFileNaming ><arch>.*

    • The file naming for the common implementation

<OriginalFileNaming>Common.*

  • That is contributor’s responsibility to relocate the arch-specific source files to the arch-specific folder.
  • That is contributor’s responsibility to make sure the original INF metafile can properly pull-in the source file from arch-specific folder after the source file reconstruction.
  • The common source files should be located under the root directory of module

 

  1. When a new arch implementation has a huge delta with the original implementation

Create a separate module instance. The naming of the module should follow below format,

  • Add the arch prefix with the original module name:

< OriginalModuleNaming><arch>

 


Chang, Abner
 

[AMD Official Use Only - General]

 

We had the conversation this morning regarding the proper place for the file/module naming rule.

The proposal is the naming rule content would be documented in “edk2 C coding standard spec”, and the “The principles of EDK2 module reconstruction for archs” would be on the edk2 Wiki page then refers to the section in “edk2 C coding standard spec” for the naming rule.

 

Abner

 

From: Ni, Ray <ray.ni@...>
Sent: Wednesday, September 28, 2022 11:56 AM
To: Kinney, Michael D <michael.d.kinney@...>; devel@edk2.groups.io; Chang, Abner <Abner.Chang@...>
Cc: Sunil V L <sunilvl@...>; lichao <lichao@...>; Kirkendall, Garrett <Garrett.Kirkendall@...>; Grimes, Paul <Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm <quic_llindhol@...>; Andrew Fish <afish@...>
Subject: RE: The principles of EDK2 module reconstruction for archs

 

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

 

Mike,

Has following content already been documented somewhere?

It looks good to me. Very good abstraction of existing cases. Maybe there are some lib/modules that don’t follow this rule. But the number should be very small.

 

But I didn’t check how ARM constructs the pkg. So it’s very welcomed to see non-X86 people for review.

 

Thanks,

Ray

 

From: Kinney, Michael D <michael.d.kinney@...>
Sent: Saturday, September 24, 2022 2:01 AM
To: devel@edk2.groups.io; abner.chang@...; Kinney, Michael D <michael.d.kinney@...>
Cc: Ni, Ray <ray.ni@...>; Sunil V L <sunilvl@...>; lichao <lichao@...>; Kirkendall, Garrett <Garrett.Kirkendall@...>; Grimes, Paul <Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm <quic_llindhol@...>; Andrew Fish <afish@...>
Subject: RE: The principles of EDK2 module reconstruction for archs

 

Hi Abner,

 

I think it would be good to clarify when a difference in implementation is due to a CPU Arch difference or a Vendor implementation difference.

 

I would also be good to provide guidelines for directory names and file names for all EDK II meta data file types.  Here are some examples to get started:

 

Package Directory Name:                           <PackageName>Pkg

Package DEC File Name:                                             <PackageName>Pkg.dec

 

              <PackageName>              REQUIRED           *

 

Module Directory Name:                            <ModuleName><Phase>

                                                                              < Feature >/<Phase>

Module INF File Name:                                <ModuleName><Phase>.inf

                                                                              < Feature>/<Phase>/<ModuleName>.inf

 

              <Feature>                           OPTIONAL           Only used if implementation does not have any shared code between phases (e.g. MdeModulePkg/Universal/PCD)

              <Phase>                              REQUIRED           Base, Sec, Pei, Dxe, DxeRuntime, Mm, StandaloneMm, Smm, Uefi

              <ModuleName>               REQUIRED           *

 

[Ray] Looks good to me. Good abstraction of existing cases.

 

Library Instance Directory Name:            <Phase><CpuArch><Vendor><LibraryClassName><Dependency>

Library Instance INF File Name:                               <Phase><CpuArch><Vendor><LibraryClassName><Dependency>.inf

 

              <Phase>                              REQUIRED           Base, Sec, Pei, Dxe, DxeRuntime, Mm, StandaloneMm, Smm, Uefi

              <CpuArch>                         OPTIONAL           Ia32, X64, Arm, AArch64, RiscV64, LoongArch64, Ebc                                      If not provided, then component supports >=2 or all CPU archs

              <Vendor>                           OPTIONAL           *

              <LibraryClassName>       REQUIRED           *

              <Dependency>                 OPTIONAL           *             Typically name of PPI, Protocol, LibraryClass that the implementation is layered on top of.

 

Source File Paths within a Library/Module instance

              <FileName>.c

              <FileName>.h

              <CpuArch>/<FileName>.c

              <CpuArch>/<FileName>.h

              <CpuArch>/<FileName>.nasm

              <CpuArch>/<FileName>.S

             

              <CpuArch>                         OPTIONAL           Ia32, X64, Arm, AArch64, RiscV64, LoongArch64, Ebc

[Ray] Looks good to me. Good abstraction of existing cases.

 

 

I think the point you are raising in the discussion is that sometimes there may be shared content between a small subset of CPU archs (e.g. IA32/X64 or Arm/AArch64 or RiscV32/RiscV64/RiscV128) and that you are proposing a new standard directory name for these combinations.  Your proposal is X86 for a directory that contains content for both IA32 and X64.  You are also wanting to support vendor specific content in the naming convention.  An example where it is already being done is in MdePkg/Include/Registers/<VendorName>.   So an enhancement to the above Source File Paths would be:

 

Source File Paths within a Library/Module instance

              <FileName>.c

              <FileName>.h

              <CpuArch>/<FileName>.c

              <CpuArch>/<FileName>.h

              <CpuArch>/<FileName>.nasm

              <CpuArch>/<FileName>.S

              <CpuArch>/<Vendor>/<FileName>.c

              <CpuArch>/<Vendor>/<FileName>.h

              <CpuArch>/<Vendor>/<FileName>.nasm

              <CpuArch>/<Vendor>/<FileName>.S

             

              <CpuArch>                         OPTIONAL           Ia32, X64, Arm, AArch64, RiscV64, LoongArch64, Ebc, X86

              <Vendor>                           OPTIONAL           *

 

I am not sure if we should use “Common” in the naming conventions.  I think by default, any content that is not CpuArch or Vendor specific could be considered common content.

 

[Ray] Good to me.

 

Thanks,

 

Mike

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Chang, Abner via groups.io
Sent: Friday, September 23, 2022 8:39 AM
To: devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@...>; Kinney, Michael D <michael.d.kinney@...>; Sunil V L <sunilvl@...>; lichao <lichao@...>; Kirkendall, Garrett <Garrett.Kirkendall@...>; Grimes, Paul <Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm <quic_llindhol@...>; Andrew Fish <afish@...>
Subject: [edk2-devel] The principles of EDK2 module reconstruction for archs

 

[AMD Official Use Only - General]

 

All,

Today in edk2 open design meeting, we went through the draft of principles of the EDK2 module reconstruction for accommodating different archs (IA32, X64, Arm, AArch64, RiscV64, Loongson64 and etc.) or different vendors of the same arch (AMD/Intel to IA32 and X64).

 

@Ray Ni and @michael.d.kinney@...

  1. We may need somewhere on edk2 repo or another place that can have PR for the easy review, please let me know where I can create the PR instead of reviewing this through dev mailing list.
  2. I didn’t mention using CPUID, Family ID or PCD to have the different code path for AMD and Intel. The reason is,
    1. This decreases the readability of code.
    2. That makes a confusing to the review process

                                                          i.     Says the maintainer/reviewer owns the package, however the patch is specific to AMD implementation but the change is in the file that mixes up Intel and AMD code. Then who is supposed the right person to give the reviewed-by? Perhaps the AMD edk2 module maintainers or reviewers is the proper person to give the reviewed-by for this case. Of course, other maintainers still can join the review process and give the comments. So to separate the arch-specific code in a arch-specific source file simplifies the review, even that is just a small delta between two implementations.

                                                        ii.     We can have the maintainers or reviewers for the entire module or *Amd* files only. So the maintainers/reviewers do not have to review the changes that only made for other archs. But they have to help reviewing the common code if that gets impact.

  1. I didn’t mention to have <phase><arch><basename> for the new module. I prefer we just inherit the original module name or file name so we can know the module or the source file has the different implementation for archs in the file browser (when the files are sorted in alphabet).

 

Lets discuss this using PR if possible.

Thanks

Abner

 

 

Below is the draft of principles:

 

Preface:

The principle is mainly for UefiCpuPKg, but the same principle maybe applied to the EDK2 module that has the processor architecture dependence (such as the BaseLib under MdePkg/Library). Most of the EDK2 modules under UefiCpuPkg were developed specifically to IA32/X64 architecture, that is necessary to reconstruct the folder or revise the source files to accommodate other processor architectures. The EDK2 module reconstruction is also required for accommodating the same-arch-but-different-vendor’s implementation (e.g., Intel and AMD for the X86 based processors). The EDK2 module may be strictly developed based on the specific processor architecture. The new introduced implementation for other processor architectures may consider to have a separate EDK2 module instance. Not all of the EDK2 modules revising can exactly meet the principles listed below, that depends on the delta between the original EDK2 module and the implementation for the new introduced processor architecture. It may require the further discussions with EDK2 module maintainers.

 

The [Arch] refers to the Processor Architecture.

The [Module] refer to the EDK2 module.

The [X86] refers to both IA32 and X64.

 

The principles to create the X86 folder in the module:

  1. When X86-vendor’s implementation is introduced to the existing module:
  1. The folder reconstruction:

A-1. If the module is obviously used by IA32/X64 only

  • No need to create X86 folder
  • Create X86-vendor’s stuff under the root directory of module

A-2. If the existing module is expected to accommodate the different archs or the module already has multiple archs:

  • Create X86 folder
  • Create X86-vendor’s stuff under X86 folder

 

  1. The files reconstruction:

B-1. The module INF metafile

  • The existing INF metafile should be kept without relocation. Should not have the impacts to the existing DSC/FDF file.
  • The new introduced INF metafile should be located under the root directory of module with the file naming format as below. This keeps the consistent module file structure.
    • <OriginalFileName><arch>.inf

 

                     B-2. Source files

                              The new arch implementation is introduced to the module in order to leverage the source code and the module design architecture, so

  • That is contributor’s responsibility to review the source code and strip the arch-dependent code away and put it into the arch-specific file. Leave the common code in the original file if there is no arch-specific and arch-specific-feature wordings in the file name. Create a common file for the common implementation otherwise.
    • The file naming for the arch-specific file

<OriginalFileName ><arch>.*

    • The file naming for the common implementation

< OriginalFileNaming >Common.*

  • That is contributor’s responsibility to relocate the arch-specific source files to the arch-specific folder.
  • That is contributor’s responsibility to make sure the original INF metafile can properly pull-in the source file from arch-specific folder after the source file reconstruction.
  • The common source files should be located under the root directory of module

 

  1. When a new arch’s implementation is introduced to the existing module which was developed for the specific arch:
  1. The folder reconstruction:
  • Create arch folder for the existing arch implementation
  • Create the arch folder for the new introduced arch

 

  1. The files reconstruction:

B-1. The module INF metafile

  • The existing INF file should be kept without the relocation. Should not have the impacts to the existing DSC/FDF file.
  • The new introduced INF metafile should be located under the root directory of module with the file naming format as below. This keeps the consistent module file structure.
    • < OriginalFileNaming><arch>.inf

 

B-2. Source files

                             The new arch implementation is introduced to this module in order to leverage the source code and the module design architecture, so

  • That is contributor’s responsibility to review the source code and strip the arch-dependent code away and put it into the arch-specific file. Leave the common code in the original file if there is no arch-specific wording in the file name. Create a common file for the common implementation otherwise.
    • The file naming for the arch-specific source file

< OriginalFileNaming ><arch>.*

    • The file naming for the common implementation

<OriginalFileNaming>Common.*

  • That is contributor’s responsibility to relocate the arch-specific source files to the arch-specific folder.
  • That is contributor’s responsibility to make sure the original INF metafile can properly pull-in the source file from arch-specific folder after the source file reconstruction.
  • The common source files should be located under the root directory of module

 

  1. When a new arch implementation has a huge delta with the original implementation

Create a separate module instance. The naming of the module should follow below format,

  • Add the arch prefix with the original module name:

< OriginalModuleNaming><arch>

 


Ni, Ray
 

Abner,

I think we Mike’s first email regarding the package structure is a good guideline regarding how to place the modules.

 

What we are discussing is how to organize the module internal contents for different archs. Do you agree?

I want to clarify this so we have a clear boundary between the two topics.

 

However, Mike’s proposal also defines the <arch> folder inside a module directory.

Maybe we just work together to refine Mike’s proposal and document it in EDKII coding standard document.

 

Chao, can you check Mike’s proposal and raise if there is anything not captured?

 

Thanks,

Ray

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Chang, Abner via groups.io
Sent: Wednesday, September 28, 2022 12:31 PM
To: devel@edk2.groups.io; Ni, Ray <ray.ni@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; Sunil V L <sunilvl@...>; lichao <lichao@...>; Kirkendall, Garrett <Garrett.Kirkendall@...>; Grimes, Paul <Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm <quic_llindhol@...>; Andrew Fish <afish@...>
Subject: Re: [edk2-devel] The principles of EDK2 module reconstruction for archs

 

[AMD Official Use Only - General]

 

 

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray via groups.io
Sent: Wednesday, September 28, 2022 11:34 AM
To: devel@edk2.groups.io; Chang, Abner <Abner.Chang@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; Sunil V L <sunilvl@...>; lichao <lichao@...>; Kirkendall, Garrett <Garrett.Kirkendall@...>; Grimes, Paul <Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm <quic_llindhol@...>; Andrew Fish <afish@...>
Subject: Re: [edk2-devel] The principles of EDK2 module reconstruction for archs

 

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

 

The [Arch] refers to the Processor Architecture.

The [Module] refer to the EDK2 module.

The [X86] refers to both IA32 and X64.

The principles to create the X86 folder in the module:

  1. When X86-vendor’s implementation is introduced to the existing module:
  1. The folder reconstruction:

A-1. If the module is obviously used by IA32/X64 only

  • No need to create X86 folder
  • Create X86-vendor’s stuff under the root directory of module

A-2. If the existing module is expected to accommodate the different archs or the module already has multiple archs:

  • Create X86 folder
  • Create X86-vendor’s stuff under X86 folder

[Ray] Looks good.

 

  1. The files reconstruction:

B-1. The module INF metafile

  • The existing INF metafile should be kept without relocation. Should not have the impacts to the existing DSC/FDF file.
  • The new introduced INF metafile should be located under the root directory of module with the file naming format as below. This keeps the consistent module file structure.
    • <OriginalFileName><arch>.inf

[Ray]  “CpuDxe.inf” and “CpuDxeArm.inf”? is that your intention? New developers may be confused that CpuDxe.inf supports only X86 arch.

Do you have an example that one module supporting multiple archs using different INF files? MdeModulePkg/DxeIpl is a module supporting different archs with the same INF file.

Or shall we leave it to be decided between the patch contributor and package/module maintainer?

[Chang, Abner]  Here I mean the library module, for example SmmCpuFeatureLib.inf and AmdSmmCpuFeatureLib.inf. Will make the statement clear. The file naming above would be changed to <arch><OriginalFileName>.inf as Mike suggested.

 

I am not sure if there is another example having arch-specific INF file. Usually the driver module has the abstract interface and the implementation in the library module. A newly introduced processor architecture driver may create it’s own module such as ArmCpuDxe if the delta between implementations  is huge. However, I would prefer to have arch-specific INF for the module if the module implementation can be leveraged. And yes, this requires the discussion between contributor and module maintainer if the principles can not perfectly identify the case.

 

                     B-2. Source files

                              The new arch implementation is introduced to the module in order to leverage the source code and the module design architecture, so

  • That is contributor’s responsibility to review the source code and strip the arch-dependent code away and put it into the arch-specific file. Leave the common code in the original file if there is no arch-specific and arch-specific-feature wordings in the file name. Create a common file for the common implementation otherwise.
    • The file naming for the arch-specific file

<OriginalFileName ><arch>.*

    • The file naming for the common implementation

< OriginalFileNaming >Common.*

  • That is contributor’s responsibility to relocate the arch-specific source files to the arch-specific folder.
  • That is contributor’s responsibility to make sure the original INF metafile can properly pull-in the source file from arch-specific folder after the source file reconstruction.
  • The common source files should be located under the root directory of module

[Ray] If you check the MpInitLib, most of SEV logic are moved to AmdSev.c. But MpLib.c still contains logic to call functions from AmdSev.c based on some AMD specific check. In my opinion, that’s already good enough.

[Chang, Abner]  I am not quite lean to the If-AMD-else in the source file solution. I prefer to separate AMD stuff or vendor feature to a separated file. So we can have the reviewer or maintainer for *Amd* files/module/packages specifically. This makes the review responsibility clear and efficient.

[Ray] You can take a look at the MpInitLib. There are some code paths that are AMD only. I agree to have dedicated reviewers from different vendors if the implementations are different.

 

However, if you check MdeModulePkg/DxeIpl, implementations for different archs are in different *folders*.

Can we leave this to the module owner to decide how to place the implementations?

 

 

 

  1. When a new arch’s implementation is introduced to the existing module which was developed for the specific arch:
  1. The folder reconstruction:
  • Create arch folder for the existing arch implementation

[Ray] Do you move existing arch implementation to that arch folder? It will break existing platforms a lot.

[Chang, Abner] We will move the arch implementation to the arch folder without moving INF. This wont impact the platform DSC and FDF, however this has the impact to the existing overwrite.

[Ray] I might misunderstand your idea as “UefiCpuPkg/CpuDxe -> UefiCpuPkg/X86/CpuDxe”.

Now I guess you tries to say: “UefiCpuPkg/CpuDxe -> UefiCpuPkg/CpuDxe/X86”. That looks good to me.

We need a more clear statement here😊

 

  • Create the arch folder for the new introduced arch

[Ray] I agree. But if we don’t create arch folder for existing arch implementation, the pkg layout will be a mess.

[Chang, Abner] right, so the first bullet is important.

 

[Ray] Hard for me to understand all the principles here. Maybe we review existing code including to-be-upstreamed code and decide how to go.

 

  1. The files reconstruction:

B-1. The module INF metafile

  • The existing INF file should be kept without the relocation. Should not have the impacts to the existing DSC/FDF file.
  • The new introduced INF metafile should be located under the root directory of module with the file naming format as below. This keeps the consistent module file structure.
    • < OriginalFileNaming><arch>.inf

 

B-2. Source files

                             The new arch implementation is introduced to this module in order to leverage the source code and the module design architecture, so

  • That is contributor’s responsibility to review the source code and strip the arch-dependent code away and put it into the arch-specific file. Leave the common code in the original file if there is no arch-specific wording in the file name. Create a common file for the common implementation otherwise.
    • The file naming for the arch-specific source file

< OriginalFileNaming ><arch>.*

    • The file naming for the common implementation

<OriginalFileNaming>Common.*

  • That is contributor’s responsibility to relocate the arch-specific source files to the arch-specific folder.
  • That is contributor’s responsibility to make sure the original INF metafile can properly pull-in the source file from arch-specific folder after the source file reconstruction.
  • The common source files should be located under the root directory of module

 

  1. When a new arch implementation has a huge delta with the original implementation

Create a separate module instance. The naming of the module should follow below format,

  • Add the arch prefix with the original module name:

< OriginalModuleNaming><arch>

 


Chang, Abner
 

[AMD Official Use Only - General]

 

 

 

From: Ni, Ray <ray.ni@...>
Sent: Wednesday, September 28, 2022 1:43 PM
To: devel@edk2.groups.io; Chang, Abner <Abner.Chang@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; Sunil V L <sunilvl@...>; lichao <lichao@...>; Kirkendall, Garrett <Garrett.Kirkendall@...>; Grimes, Paul <Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm <quic_llindhol@...>; Andrew Fish <afish@...>
Subject: RE: [edk2-devel] The principles of EDK2 module reconstruction for archs

 

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

 

Abner,

I think we Mike’s first email regarding the package structure is a good guideline regarding how to place the modules.

 

What we are discussing is how to organize the module internal contents for different archs. Do you agree?

[Chang, Abner] Yes.

I want to clarify this so we have a clear boundary between the two topics.

 

However, Mike’s proposal also defines the <arch> folder inside a module directory.

Maybe we just work together to refine Mike’s proposal and document it in EDKII coding standard document.

[Chang, Abner] we can start from that and see how to accommodate the module reconstruction principles. I am modifying edk2 C coding standard spec to add the section for naming rule Mike proposed. Will send the patch for this later.

Thanks

Abner

 

Chao, can you check Mike’s proposal and raise if there is anything not captured?

 

Thanks,

Ray

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Chang, Abner via groups.io
Sent: Wednesday, September 28, 2022 12:31 PM
To: devel@edk2.groups.io; Ni, Ray <ray.ni@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; Sunil V L <sunilvl@...>; lichao <lichao@...>; Kirkendall, Garrett <Garrett.Kirkendall@...>; Grimes, Paul <Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm <quic_llindhol@...>; Andrew Fish <afish@...>
Subject: Re: [edk2-devel] The principles of EDK2 module reconstruction for archs

 

[AMD Official Use Only - General]

 

 

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray via groups.io
Sent: Wednesday, September 28, 2022 11:34 AM
To: devel@edk2.groups.io; Chang, Abner <Abner.Chang@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; Sunil V L <sunilvl@...>; lichao <lichao@...>; Kirkendall, Garrett <Garrett.Kirkendall@...>; Grimes, Paul <Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm <quic_llindhol@...>; Andrew Fish <afish@...>
Subject: Re: [edk2-devel] The principles of EDK2 module reconstruction for archs

 

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

 

The [Arch] refers to the Processor Architecture.

The [Module] refer to the EDK2 module.

The [X86] refers to both IA32 and X64.

The principles to create the X86 folder in the module:

  1. When X86-vendor’s implementation is introduced to the existing module:
  1. The folder reconstruction:

A-1. If the module is obviously used by IA32/X64 only

  • No need to create X86 folder
  • Create X86-vendor’s stuff under the root directory of module

A-2. If the existing module is expected to accommodate the different archs or the module already has multiple archs:

  • Create X86 folder
  • Create X86-vendor’s stuff under X86 folder

[Ray] Looks good.

 

  1. The files reconstruction:

B-1. The module INF metafile

  • The existing INF metafile should be kept without relocation. Should not have the impacts to the existing DSC/FDF file.
  • The new introduced INF metafile should be located under the root directory of module with the file naming format as below. This keeps the consistent module file structure.
    • <OriginalFileName><arch>.inf

[Ray]  “CpuDxe.inf” and “CpuDxeArm.inf”? is that your intention? New developers may be confused that CpuDxe.inf supports only X86 arch.

Do you have an example that one module supporting multiple archs using different INF files? MdeModulePkg/DxeIpl is a module supporting different archs with the same INF file.

Or shall we leave it to be decided between the patch contributor and package/module maintainer?

[Chang, Abner]  Here I mean the library module, for example SmmCpuFeatureLib.inf and AmdSmmCpuFeatureLib.inf. Will make the statement clear. The file naming above would be changed to <arch><OriginalFileName>.inf as Mike suggested.

 

I am not sure if there is another example having arch-specific INF file. Usually the driver module has the abstract interface and the implementation in the library module. A newly introduced processor architecture driver may create it’s own module such as ArmCpuDxe if the delta between implementations  is huge. However, I would prefer to have arch-specific INF for the module if the module implementation can be leveraged. And yes, this requires the discussion between contributor and module maintainer if the principles can not perfectly identify the case.

 

                     B-2. Source files

                              The new arch implementation is introduced to the module in order to leverage the source code and the module design architecture, so

  • That is contributor’s responsibility to review the source code and strip the arch-dependent code away and put it into the arch-specific file. Leave the common code in the original file if there is no arch-specific and arch-specific-feature wordings in the file name. Create a common file for the common implementation otherwise.
    • The file naming for the arch-specific file

<OriginalFileName ><arch>.*

    • The file naming for the common implementation

< OriginalFileNaming >Common.*

  • That is contributor’s responsibility to relocate the arch-specific source files to the arch-specific folder.
  • That is contributor’s responsibility to make sure the original INF metafile can properly pull-in the source file from arch-specific folder after the source file reconstruction.
  • The common source files should be located under the root directory of module

[Ray] If you check the MpInitLib, most of SEV logic are moved to AmdSev.c. But MpLib.c still contains logic to call functions from AmdSev.c based on some AMD specific check. In my opinion, that’s already good enough.

[Chang, Abner]  I am not quite lean to the If-AMD-else in the source file solution. I prefer to separate AMD stuff or vendor feature to a separated file. So we can have the reviewer or maintainer for *Amd* files/module/packages specifically. This makes the review responsibility clear and efficient.

[Ray] You can take a look at the MpInitLib. There are some code paths that are AMD only. I agree to have dedicated reviewers from different vendors if the implementations are different.

 

However, if you check MdeModulePkg/DxeIpl, implementations for different archs are in different *folders*.

Can we leave this to the module owner to decide how to place the implementations?

 

 

 

  1. When a new arch’s implementation is introduced to the existing module which was developed for the specific arch:
  1. The folder reconstruction:
  • Create arch folder for the existing arch implementation

[Ray] Do you move existing arch implementation to that arch folder? It will break existing platforms a lot.

[Chang, Abner] We will move the arch implementation to the arch folder without moving INF. This wont impact the platform DSC and FDF, however this has the impact to the existing overwrite.

[Ray] I might misunderstand your idea as “UefiCpuPkg/CpuDxe -> UefiCpuPkg/X86/CpuDxe”.

Now I guess you tries to say: “UefiCpuPkg/CpuDxe -> UefiCpuPkg/CpuDxe/X86”. That looks good to me.

We need a more clear statement here😊

 

  • Create the arch folder for the new introduced arch

[Ray] I agree. But if we don’t create arch folder for existing arch implementation, the pkg layout will be a mess.

[Chang, Abner] right, so the first bullet is important.

 

[Ray] Hard for me to understand all the principles here. Maybe we review existing code including to-be-upstreamed code and decide how to go.

 

  1. The files reconstruction:

B-1. The module INF metafile

  • The existing INF file should be kept without the relocation. Should not have the impacts to the existing DSC/FDF file.
  • The new introduced INF metafile should be located under the root directory of module with the file naming format as below. This keeps the consistent module file structure.
    • < OriginalFileNaming><arch>.inf

 

B-2. Source files

                             The new arch implementation is introduced to this module in order to leverage the source code and the module design architecture, so

  • That is contributor’s responsibility to review the source code and strip the arch-dependent code away and put it into the arch-specific file. Leave the common code in the original file if there is no arch-specific wording in the file name. Create a common file for the common implementation otherwise.
    • The file naming for the arch-specific source file

< OriginalFileNaming ><arch>.*

    • The file naming for the common implementation

<OriginalFileNaming>Common.*

  • That is contributor’s responsibility to relocate the arch-specific source files to the arch-specific folder.
  • That is contributor’s responsibility to make sure the original INF metafile can properly pull-in the source file from arch-specific folder after the source file reconstruction.
  • The common source files should be located under the root directory of module

 

  1. When a new arch implementation has a huge delta with the original implementation

Create a separate module instance. The naming of the module should follow below format,

  • Add the arch prefix with the original module name:

< OriginalModuleNaming><arch>

 


Sunil V L
 

On Wed, Sep 28, 2022 at 03:33:45AM +0000, Ni, Ray wrote:
Hi Ray,

1. When a new arch's implementation is introduced to the existing module which was developed for the specific arch:

1. The folder reconstruction:

* Create arch folder for the existing arch implementation
[Ray] Do you move existing arch implementation to that arch folder? It will break existing platforms a lot.

* Create the arch folder for the new introduced arch
[Ray] I agree. But if we don't create arch folder for existing arch implementation, the pkg layout will be a mess.

[Ray] Hard for me to understand all the principles here. Maybe we review existing code including to-be-upstreamed code and decide how to go.
Could you please take a look below changes which is trying to add RISC-V
support for CpuDxe?
https://github.com/tianocore/edk2-staging/commit/bba1a11be47dd091734e185afbed73ea75708749
https://github.com/tianocore/edk2-staging/commit/7fccf92a97a6d0618a20f10622220e78b3687906

What do you suggest with above example?

1) Common INF for all architectures - but modify INF alone, no X86
folder creation.

This is what I have done in the commit above. May be of least impact to existing code
since it is only INF change. But like you mentioned this is bit weird that X86 files will
remain in root folder directly along with some common files.

2) Common INF (CpuDxe.inf) + create arch folders X86, X64, IA32, RiscV64 etc

IMO, this is probably the best approach. What would be the challenges
with this?

3) Separate INF for arch like CpuDxe.inf for x86, CpuDxeRiscV64.inf for
RISC-V.

This again probably is not a good idea.

4) If the module/library is specific to one arch (ex: SMM(X86),
SBI(RISC-V)), then create separate INF.

Thanks!
Sunil


Chang, Abner
 

[AMD Official Use Only - General]

I just had created PR to update edkII C coding standard spec for the file and directory naming. We can review and confirm this update first and then go back to the principles of EDK2 module reconstruction for archs.
Here is the PR:
https://github.com/tianocore-docs/edk2-CCodingStandardsSpecification/pull/2

The naming rule is mainly for the new module or new file IMO. Some existing module may not meet the guidelines mentioned in this spec. Thus we need the principles of EDK2 module reconstruction on the existing module to support other processor archs and not impacting the existing platforms (e.g. rename the INF file or directory to meet the guidelines).

Sunil, seems RISC-V CpuDxe meet the guideline. Please check it.
Just feel that having CpuDxe.c to Riscv64 folder is not quite a best solution. I think at least we can abstract the protocol structure and protocol installation under CpuDxe\ and have the arch implementation under arch folder. We can discuss this later after we confirming the guideline and principles.

Thanks
Abner

-----Original Message-----
From: Sunil V L <sunilvl@...>
Sent: Wednesday, September 28, 2022 3:34 PM
To: devel@edk2.groups.io; ray.ni@...
Cc: Chang, Abner <Abner.Chang@...>; Kinney, Michael D
<michael.d.kinney@...>; lichao <lichao@...>; Kirkendall,
Garrett <Garrett.Kirkendall@...>; Grimes, Paul
<Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar,
AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm
<quic_llindhol@...>; Andrew Fish <afish@...>
Subject: Re: [edk2-devel] The principles of EDK2 module reconstruction for
archs

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


On Wed, Sep 28, 2022 at 03:33:45AM +0000, Ni, Ray wrote:
Hi Ray,

1. When a new arch's implementation is introduced to the existing
module which was developed for the specific arch:

1. The folder reconstruction:

* Create arch folder for the existing arch implementation
[Ray] Do you move existing arch implementation to that arch folder? It will
break existing platforms a lot.

* Create the arch folder for the new introduced arch
[Ray] I agree. But if we don't create arch folder for existing arch
implementation, the pkg layout will be a mess.

[Ray] Hard for me to understand all the principles here. Maybe we review
existing code including to-be-upstreamed code and decide how to go.
Could you please take a look below changes which is trying to add RISC-V
support for CpuDxe?
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
ub.com%2Ftianocore%2Fedk2-
staging%2Fcommit%2Fbba1a11be47dd091734e185afbed73ea75708749&amp;
data=05%7C01%7Cabner.chang%40amd.com%7Ca419e6a010d34fde464b08d
aa123e080%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C63799947
2732494527%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIj
oiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sd
ata=Vq6pJLnn8yJrJhFZn7LfLbZzrtpG4n1VLWgAil6J38U%3D&amp;reserved=0
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
ub.com%2Ftianocore%2Fedk2-
staging%2Fcommit%2F7fccf92a97a6d0618a20f10622220e78b3687906&amp;da
ta=05%7C01%7Cabner.chang%40amd.com%7Ca419e6a010d34fde464b08daa1
23e080%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C63799947273
2494527%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV
2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata
=xFmvUv58vh4AUAM17Qy9G5jZWFZlK2Ozt3njpG1e8%2BY%3D&amp;reserv
ed=0

What do you suggest with above example?

1) Common INF for all architectures - but modify INF alone, no X86 folder
creation.

This is what I have done in the commit above. May be of least impact to
existing code since it is only INF change. But like you mentioned this is bit
weird that X86 files will remain in root folder directly along with some
common files.

2) Common INF (CpuDxe.inf) + create arch folders X86, X64, IA32, RiscV64 etc

IMO, this is probably the best approach. What would be the challenges with
this?

3) Separate INF for arch like CpuDxe.inf for x86, CpuDxeRiscV64.inf for RISC-V.

This again probably is not a good idea.

4) If the module/library is specific to one arch (ex: SMM(X86), SBI(RISC-V)),
then create separate INF.

Thanks!
Sunil


Ni, Ray
 

-----Original Message-----
From: Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>
Sent: Thursday, September 29, 2022 3:11 PM
To: Chang, Abner <Abner.Chang@...>; Sunil V L
<sunilvl@...>; devel@edk2.groups.io; Ni, Ray
<ray.ni@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; lichao
<lichao@...>; Kirkendall, Garrett <Garrett.Kirkendall@...>;
Grimes, Paul <Paul.Grimes@...>; He, Jiangang
<Jiangang.He@...>; Leif Lindholm <quic_llindhol@...>;
Andrew Fish <afish@...>
Subject: RE: [edk2-devel] The principles of EDK2 module reconstruction for
archs

Hi Abner,
Looks good to me.
Reviewed-by: Abdul Lateef Attar <abdattar@...>

Thanks
AbduL

-----Original Message-----
From: Chang, Abner <Abner.Chang@...>
Sent: 28 September 2022 20:31
To: Sunil V L <sunilvl@...>; devel@edk2.groups.io;
ray.ni@...
Cc: Kinney, Michael D <michael.d.kinney@...>; lichao
<lichao@...>; Kirkendall, Garrett <Garrett.Kirkendall@...>;
Grimes, Paul <Paul.Grimes@...>; He, Jiangang
<Jiangang.He@...>; Attar, AbdulLateef (Abdul Lateef)
<AbdulLateef.Attar@...>; Leif Lindholm <quic_llindhol@...>;
Andrew Fish <afish@...>
Subject: RE: [edk2-devel] The principles of EDK2 module reconstruction for
archs

[AMD Official Use Only - General]

I just had created PR to update edkII C coding standard spec for the file and
directory naming. We can review and confirm this update first and then go
back to the principles of EDK2 module reconstruction for archs.
Here is the PR:
https://github.com/tianocore-docs/edk2-
CCodingStandardsSpecification/pull/2

The naming rule is mainly for the new module or new file IMO. Some existing
module may not meet the guidelines mentioned in this spec. Thus we need
the principles of EDK2 module reconstruction on the existing module to
support other processor archs and not impacting the existing platforms (e.g.
rename the INF file or directory to meet the guidelines).

Sunil, seems RISC-V CpuDxe meet the guideline. Please check it.
Just feel that having CpuDxe.c to Riscv64 folder is not quite a best solution. I
think at least we can abstract the protocol structure and protocol installation
under CpuDxe\ and have the arch implementation under arch folder. We can
discuss this later after we confirming the guideline and principles.

Thanks
Abner

-----Original Message-----
From: Sunil V L <sunilvl@...>
Sent: Wednesday, September 28, 2022 3:34 PM
To: devel@edk2.groups.io; ray.ni@...
Cc: Chang, Abner <Abner.Chang@...>; Kinney, Michael D
<michael.d.kinney@...>; lichao <lichao@...>; Kirkendall,
Garrett <Garrett.Kirkendall@...>; Grimes, Paul
<Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar,
AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm
<quic_llindhol@...>; Andrew Fish <afish@...>
Subject: Re: [edk2-devel] The principles of EDK2 module reconstruction
for archs

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


On Wed, Sep 28, 2022 at 03:33:45AM +0000, Ni, Ray wrote:
Hi Ray,

1. When a new arch's implementation is introduced to the existing
module which was developed for the specific arch:

1. The folder reconstruction:

* Create arch folder for the existing arch implementation
[Ray] Do you move existing arch implementation to that arch folder?
It will
break existing platforms a lot.

* Create the arch folder for the new introduced arch
[Ray] I agree. But if we don't create arch folder for existing arch
implementation, the pkg layout will be a mess.

[Ray] Hard for me to understand all the principles here. Maybe we
review
existing code including to-be-upstreamed code and decide how to go.
Could you please take a look below changes which is trying to add
RISC-V support for CpuDxe?
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
ub.com%2Ftianocore%2Fedk2-
staging%2Fcommit%2Fbba1a11be47dd091734e185afbed73ea75708749&amp;
data=05%7C01%7Cabner.chang%40amd.com%7Ca419e6a010d34fde464b08d
aa123e080%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C63799947
2732494527%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIj
oiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sd
ata=Vq6pJLnn8yJrJhFZn7LfLbZzrtpG4n1VLWgAil6J38U%3D&amp;reserved=0
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
ub.com%2Ftianocore%2Fedk2-
staging%2Fcommit%2F7fccf92a97a6d0618a20f10622220e78b3687906&amp;da
ta=05%7C01%7Cabner.chang%40amd.com%7Ca419e6a010d34fde464b08daa1
23e080%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C63799947273
2494527%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV
2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata
=xFmvUv58vh4AUAM17Qy9G5jZWFZlK2Ozt3njpG1e8%2BY%3D&amp;reserv
ed=0

What do you suggest with above example?

1) Common INF for all architectures - but modify INF alone, no X86
folder creation.

This is what I have done in the commit above. May be of least impact
to existing code since it is only INF change. But like you mentioned
this is bit weird that X86 files will remain in root folder directly
along with some common files.

2) Common INF (CpuDxe.inf) + create arch folders X86, X64, IA32,
RiscV64 etc

IMO, this is probably the best approach. What would be the challenges
with this?

3) Separate INF for arch like CpuDxe.inf for x86, CpuDxeRiscV64.inf for
RISC-V.

This again probably is not a good idea.

4) If the module/library is specific to one arch (ex: SMM(X86),
SBI(RISC-V)), then create separate INF.

Thanks!
Sunil


Ni, Ray
 

Sunil, I don't have concern with your changes.
Perhaps you can also move all existing source files to X86 folder.

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sunil V L
Sent: Wednesday, September 28, 2022 3:34 PM
To: devel@edk2.groups.io; Ni, Ray <ray.ni@...>
Cc: abner.chang@...; Kinney, Michael D
<michael.d.kinney@...>; lichao <lichao@...>; Kirkendall,
Garrett <Garrett.Kirkendall@...>; Grimes, Paul
<Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar,
AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm
<quic_llindhol@...>; Andrew Fish <afish@...>
Subject: Re: [edk2-devel] The principles of EDK2 module reconstruction for
archs

On Wed, Sep 28, 2022 at 03:33:45AM +0000, Ni, Ray wrote:
Hi Ray,

1. When a new arch's implementation is introduced to the existing
module which was developed for the specific arch:

1. The folder reconstruction:

* Create arch folder for the existing arch implementation
[Ray] Do you move existing arch implementation to that arch folder? It will
break existing platforms a lot.

* Create the arch folder for the new introduced arch
[Ray] I agree. But if we don't create arch folder for existing arch
implementation, the pkg layout will be a mess.

[Ray] Hard for me to understand all the principles here. Maybe we review
existing code including to-be-upstreamed code and decide how to go.
Could you please take a look below changes which is trying to add RISC-V
support for CpuDxe?
https://github.com/tianocore/edk2-
staging/commit/bba1a11be47dd091734e185afbed73ea75708749
https://github.com/tianocore/edk2-
staging/commit/7fccf92a97a6d0618a20f10622220e78b3687906

What do you suggest with above example?

1) Common INF for all architectures - but modify INF alone, no X86
folder creation.

This is what I have done in the commit above. May be of least impact to
existing code
since it is only INF change. But like you mentioned this is bit weird that X86
files will
remain in root folder directly along with some common files.

2) Common INF (CpuDxe.inf) + create arch folders X86, X64, IA32, RiscV64 etc

IMO, this is probably the best approach. What would be the challenges
with this?

3) Separate INF for arch like CpuDxe.inf for x86, CpuDxeRiscV64.inf for
RISC-V.

This again probably is not a good idea.

4) If the module/library is specific to one arch (ex: SMM(X86),
SBI(RISC-V)), then create separate INF.

Thanks!
Sunil





Chang, Abner
 

[AMD Official Use Only - General]

Hi Sunil,
One more thing other than the module reconstruction for archs before you sending patch to edk2:
Not sure how would you do on migrating the RISC-V code from edk2-platforms to edk2. Did you make some other changes to the RISC-V CpuDxe on edk2-platform?
Please keep the files history and send the patch for the migration first. Then have the follow up patches for your changes if any and also add the Ventana license.

Below branches could be the reference for this migration,
https://github.com/changab/edk2/commits/RISC-V-MIGRATION-EDK2-PR
https://github.com/changab/edk2-platforms/commits/RISC-V-MIGRATION-EDK2-PLATFORM

Thanks Sunil.
Abner

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
via groups.io
Sent: Thursday, September 29, 2022 3:48 PM
To: devel@edk2.groups.io; sunilvl@...
Cc: Chang, Abner <Abner.Chang@...>; Kinney, Michael D
<michael.d.kinney@...>; lichao <lichao@...>; Kirkendall,
Garrett <Garrett.Kirkendall@...>; Grimes, Paul
<Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar,
AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm
<quic_llindhol@...>; Andrew Fish <afish@...>
Subject: Re: [edk2-devel] The principles of EDK2 module reconstruction for
archs

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


Sunil, I don't have concern with your changes.
Perhaps you can also move all existing source files to X86 folder.

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sunil V
L
Sent: Wednesday, September 28, 2022 3:34 PM
To: devel@edk2.groups.io; Ni, Ray <ray.ni@...>
Cc: abner.chang@...; Kinney, Michael D
<michael.d.kinney@...>; lichao <lichao@...>; Kirkendall,
Garrett <Garrett.Kirkendall@...>; Grimes, Paul
<Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar,
AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm
<quic_llindhol@...>; Andrew Fish <afish@...>
Subject: Re: [edk2-devel] The principles of EDK2 module reconstruction
for archs

On Wed, Sep 28, 2022 at 03:33:45AM +0000, Ni, Ray wrote:
Hi Ray,

1. When a new arch's implementation is introduced to the existing
module which was developed for the specific arch:

1. The folder reconstruction:

* Create arch folder for the existing arch implementation
[Ray] Do you move existing arch implementation to that arch folder?
It will
break existing platforms a lot.

* Create the arch folder for the new introduced arch
[Ray] I agree. But if we don't create arch folder for existing arch
implementation, the pkg layout will be a mess.

[Ray] Hard for me to understand all the principles here. Maybe we
review
existing code including to-be-upstreamed code and decide how to go.
Could you please take a look below changes which is trying to add
RISC-V support for CpuDxe?
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
ub.com%2Ftianocore%2Fedk2-
&amp;data=05%7C01%7Cabner.chang%40amd.com%7C
84150fde7ae94437a06908daa1eee77a%7C3dd8961fe4884e608e11a82d994e18
3d%7C
0%7C0%7C638000344717043181%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiM
C4wLjAwMDA
iLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&
amp;sd
ata=P0QZ8%2B3IxaTnoEJPYOn3SgDLGhLZohPna53RoX6o2sc%3D&amp;reserv
ed=0
staging/commit/bba1a11be47dd091734e185afbed73ea75708749
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
ub.com%2Ftianocore%2Fedk2-
&amp;data=05%7C01%7Cabner.chang%40amd.com%7C
84150fde7ae94437a06908daa1eee77a%7C3dd8961fe4884e608e11a82d994e18
3d%7C
0%7C0%7C638000344717043181%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiM
C4wLjAwMDA
iLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&
amp;sd
ata=P0QZ8%2B3IxaTnoEJPYOn3SgDLGhLZohPna53RoX6o2sc%3D&amp;reserv
ed=0
staging/commit/7fccf92a97a6d0618a20f10622220e78b3687906

What do you suggest with above example?

1) Common INF for all architectures - but modify INF alone, no X86
folder creation.

This is what I have done in the commit above. May be of least impact
to existing code since it is only INF change. But like you mentioned
this is bit weird that X86 files will remain in root folder directly
along with some common files.

2) Common INF (CpuDxe.inf) + create arch folders X86, X64, IA32,
RiscV64 etc

IMO, this is probably the best approach. What would be the challenges
with this?

3) Separate INF for arch like CpuDxe.inf for x86, CpuDxeRiscV64.inf
for RISC-V.

This again probably is not a good idea.

4) If the module/library is specific to one arch (ex: SMM(X86),
SBI(RISC-V)), then create separate INF.

Thanks!
Sunil








Sunil V L
 

On Thu, Sep 29, 2022 at 02:54:05PM +0000, Chang, Abner wrote:
[AMD Official Use Only - General]

Hi Sunil,
One more thing other than the module reconstruction for archs before you sending patch to edk2:
Not sure how would you do on migrating the RISC-V code from edk2-platforms to edk2. Did you make some other changes to the RISC-V CpuDxe on edk2-platform?
Please keep the files history and send the patch for the migration first. Then have the follow up patches for your changes if any and also add the Ventana license.

Below branches could be the reference for this migration,
https://github.com/changab/edk2/commits/RISC-V-MIGRATION-EDK2-PR
https://github.com/changab/edk2-platforms/commits/RISC-V-MIGRATION-EDK2-PLATFORM

Thanks Sunil.
Abner
Thanks Abner. Let me take a look at your branch.

We have some changes and are not migrating everything from
edk2-platforms. But I am not sure whether we can maintain the commit
history when we migrate from a different repo. I think this should be like a
new review and all old RB tags which were for edk2-platforms need to be removed.

For ex:
https://github.com/changab/edk2/commit/eca5ff6bea66be94fd58421ba98cb54d1f4181a6

IMO, RB tag should be removed and should be reviewed fresh when it is
being added to edk2 repo.

Thanks
Sunil


Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>
 

Hi Abner,
Looks good to me.
Reviewed-by: Abdul Lateef Attar <abdattar@...>

Thanks
AbduL

-----Original Message-----
From: Chang, Abner <Abner.Chang@...>
Sent: 28 September 2022 20:31
To: Sunil V L <sunilvl@...>; devel@edk2.groups.io; ray.ni@...
Cc: Kinney, Michael D <michael.d.kinney@...>; lichao <lichao@...>; Kirkendall, Garrett <Garrett.Kirkendall@...>; Grimes, Paul <Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm <quic_llindhol@...>; Andrew Fish <afish@...>
Subject: RE: [edk2-devel] The principles of EDK2 module reconstruction for archs

[AMD Official Use Only - General]

I just had created PR to update edkII C coding standard spec for the file and directory naming. We can review and confirm this update first and then go back to the principles of EDK2 module reconstruction for archs.
Here is the PR:
https://github.com/tianocore-docs/edk2-CCodingStandardsSpecification/pull/2

The naming rule is mainly for the new module or new file IMO. Some existing module may not meet the guidelines mentioned in this spec. Thus we need the principles of EDK2 module reconstruction on the existing module to support other processor archs and not impacting the existing platforms (e.g. rename the INF file or directory to meet the guidelines).

Sunil, seems RISC-V CpuDxe meet the guideline. Please check it.
Just feel that having CpuDxe.c to Riscv64 folder is not quite a best solution. I think at least we can abstract the protocol structure and protocol installation under CpuDxe\ and have the arch implementation under arch folder. We can discuss this later after we confirming the guideline and principles.

Thanks
Abner

-----Original Message-----
From: Sunil V L <sunilvl@...>
Sent: Wednesday, September 28, 2022 3:34 PM
To: devel@edk2.groups.io; ray.ni@...
Cc: Chang, Abner <Abner.Chang@...>; Kinney, Michael D
<michael.d.kinney@...>; lichao <lichao@...>; Kirkendall,
Garrett <Garrett.Kirkendall@...>; Grimes, Paul
<Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar,
AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm
<quic_llindhol@...>; Andrew Fish <afish@...>
Subject: Re: [edk2-devel] The principles of EDK2 module reconstruction
for archs

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


On Wed, Sep 28, 2022 at 03:33:45AM +0000, Ni, Ray wrote:
Hi Ray,

1. When a new arch's implementation is introduced to the existing
module which was developed for the specific arch:

1. The folder reconstruction:

* Create arch folder for the existing arch implementation
[Ray] Do you move existing arch implementation to that arch folder?
It will
break existing platforms a lot.

* Create the arch folder for the new introduced arch
[Ray] I agree. But if we don't create arch folder for existing arch
implementation, the pkg layout will be a mess.

[Ray] Hard for me to understand all the principles here. Maybe we
review
existing code including to-be-upstreamed code and decide how to go.
Could you please take a look below changes which is trying to add
RISC-V support for CpuDxe?
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
ub.com%2Ftianocore%2Fedk2-
staging%2Fcommit%2Fbba1a11be47dd091734e185afbed73ea75708749&amp;
data=05%7C01%7Cabner.chang%40amd.com%7Ca419e6a010d34fde464b08d
aa123e080%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C63799947
2732494527%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIj
oiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sd
ata=Vq6pJLnn8yJrJhFZn7LfLbZzrtpG4n1VLWgAil6J38U%3D&amp;reserved=0
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
ub.com%2Ftianocore%2Fedk2-
staging%2Fcommit%2F7fccf92a97a6d0618a20f10622220e78b3687906&amp;da
ta=05%7C01%7Cabner.chang%40amd.com%7Ca419e6a010d34fde464b08daa1
23e080%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C63799947273
2494527%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV
2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata
=xFmvUv58vh4AUAM17Qy9G5jZWFZlK2Ozt3njpG1e8%2BY%3D&amp;reserv
ed=0

What do you suggest with above example?

1) Common INF for all architectures - but modify INF alone, no X86
folder creation.

This is what I have done in the commit above. May be of least impact
to existing code since it is only INF change. But like you mentioned
this is bit weird that X86 files will remain in root folder directly
along with some common files.

2) Common INF (CpuDxe.inf) + create arch folders X86, X64, IA32,
RiscV64 etc

IMO, this is probably the best approach. What would be the challenges
with this?

3) Separate INF for arch like CpuDxe.inf for x86, CpuDxeRiscV64.inf for RISC-V.

This again probably is not a good idea.

4) If the module/library is specific to one arch (ex: SMM(X86),
SBI(RISC-V)), then create separate INF.

Thanks!
Sunil


Chang, Abner
 

[AMD Official Use Only - General]

-----Original Message-----
From: Sunil V L <sunilvl@...>
Sent: Thursday, September 29, 2022 11:22 PM
To: Chang, Abner <Abner.Chang@...>
Cc: devel@edk2.groups.io; ray.ni@...; Kinney, Michael D
<michael.d.kinney@...>; lichao <lichao@...>; Kirkendall,
Garrett <Garrett.Kirkendall@...>; Grimes, Paul
<Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar,
AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif Lindholm
<quic_llindhol@...>; Andrew Fish <afish@...>
Subject: Re: [edk2-devel] The principles of EDK2 module reconstruction for
archs

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


On Thu, Sep 29, 2022 at 02:54:05PM +0000, Chang, Abner wrote:
[AMD Official Use Only - General]

Hi Sunil,
One more thing other than the module reconstruction for archs before you
sending patch to edk2:
Not sure how would you do on migrating the RISC-V code from edk2-
platforms to edk2. Did you make some other changes to the RISC-V CpuDxe
on edk2-platform?
Please keep the files history and send the patch for the migration first.
Then have the follow up patches for your changes if any and also add the
Ventana license.

Below branches could be the reference for this migration,
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
ub.com%2Fchangab%2Fedk2%2Fcommits%2FRISC-V-MIGRATION-EDK2-
PR&amp;data=
05%7C01%7CAbner.Chang%40amd.com%7Cb644f4b61bbd4e096c2308daa22e
6703%7C3
dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638000617445410044%7C
Unknown
%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha
WwiLCJ
XVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=wRnI%2Br4Dunydf77gSClb
6ghHuY24Qs
LrZTiPqc7pP1I%3D&amp;reserved=0
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
ub.com%2Fchangab%2Fedk2-platforms%2Fcommits%2FRISC-V-
MIGRATION-EDK2-PL
ATFORM&amp;data=05%7C01%7CAbner.Chang%40amd.com%7Cb644f4b61b
bd4e096c23
08daa22e6703%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C63800
0617445
410044%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2
luMzIiLCJ
BTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=5T2PHJOj
%2Bk6Mmu
zNn0vsELm0raYiIDiI1X%2FekgZt6ls%3D&amp;reserved=0

Thanks Sunil.
Abner
Thanks Abner. Let me take a look at your branch.

We have some changes and are not migrating everything from edk2-
platforms. But I am not sure whether we can maintain the commit history
when we migrate from a different repo. I think this should be like a new
review and all old RB tags which were for edk2-platforms need to be
removed.
I was used tool to cherry pick from different repo to keep the history. Not sure if git command line can do this or not.

To treat it as a new file that includes HPE and Ventana copyrights is confusing because HPE didn't have the collaboration with Ventana on those source files. I think you would have some files that are modified by Ventana regarding the functionality and some files without any change; the copyright should be applied to the contribution of functionality but not the migration or build error fix for the migration. I am fine with having a new review process, however, I would suggest below steps for the files from edk2-platform if to keep history is difficult.

1. Migrate the code from edk2-platform and fix the build error on edk2. In the source file keep HPE copyright only. Mention the origin of the file in the commit message. Please do not add Ventana copyright to the source file at this moment.
2. Afterward, add Ventana copyright for the further updates. This makes the contribution clear.
3. Do not delete the one on edk2-platforms. I think we can mention the implementation is obsoleted in the Readme.md under RISC-V PlatformPkg and ProcessorPkg.

BTW, I can help on CpuDxe X86 migration.

Thanks
Abner

For ex:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
ub.com%2Fchangab%2Fedk2%2Fcommit%2Feca5ff6bea66be94fd58421ba98c
b54d1f4181a6&amp;data=05%7C01%7CAbner.Chang%40amd.com%7Cb644f4
b61bbd4e096c2308daa22e6703%7C3dd8961fe4884e608e11a82d994e183d%7C
0%7C0%7C638000617445566273%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiM
C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000
%7C%7C%7C&amp;sdata=2TRD8pySU%2FIhz7ttFwdCHtPpl6JM0YSp%2BZjvo5
%2FEEZ4%3D&amp;reserved=0

IMO, RB tag should be removed and should be reviewed fresh when it is
being added to edk2 repo.

Thanks
Sunil


Chang, Abner
 

[AMD Official Use Only - General]

Thanks Ray, here are my responses.
https://github.com/tianocore-docs/edk2-CCodingStandardsSpecification/pull/2

@Kinney, Michael D we may also need your clarification on the comments.

-----Original Message-----
From: Ni, Ray <ray.ni@...>
Sent: Thursday, September 29, 2022 3:42 PM
To: Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Chang,
Abner <Abner.Chang@...>; Sunil V L <sunilvl@...>;
devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@...>; lichao
<lichao@...>; Kirkendall, Garrett <Garrett.Kirkendall@...>;
Grimes, Paul <Paul.Grimes@...>; He, Jiangang
<Jiangang.He@...>; Leif Lindholm <quic_llindhol@...>;
Andrew Fish <afish@...>
Subject: RE: [edk2-devel] The principles of EDK2 module reconstruction for
archs

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


Abner,
Comments in
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
ub.com%2Ftianocore-docs%2Fedk2-
CCodingStandardsSpecification%2Fpull%2F2%23pullrequestreview-
1124763311&amp;data=05%7C01%7CAbner.Chang%40amd.com%7Cd825e24
8625541e3f43e08daa1ee2883%7C3dd8961fe4884e608e11a82d994e183d%7C0
%7C0%7C638000341502885565%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC
4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%
7C%7C%7C&amp;sdata=RXxgpbEr6ivbqP1R6%2B3Rxl%2ByJAnaUJuaYYKdfCH
8jo8%3D&amp;reserved=0

We can discuss more in tomorrow's meeting.


-----Original Message-----
From: Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>
Sent: Thursday, September 29, 2022 3:11 PM
To: Chang, Abner <Abner.Chang@...>; Sunil V L
<sunilvl@...>; devel@edk2.groups.io; Ni, Ray
<ray.ni@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; lichao
<lichao@...>; Kirkendall, Garrett
<Garrett.Kirkendall@...>; Grimes, Paul <Paul.Grimes@...>;
He,
Jiangang <Jiangang.He@...>; Leif Lindholm
<quic_llindhol@...>; Andrew Fish <afish@...>
Subject: RE: [edk2-devel] The principles of EDK2 module reconstruction
for archs

Hi Abner,
Looks good to me.
Reviewed-by: Abdul Lateef Attar <abdattar@...>

Thanks
AbduL

-----Original Message-----
From: Chang, Abner <Abner.Chang@...>
Sent: 28 September 2022 20:31
To: Sunil V L <sunilvl@...>; devel@edk2.groups.io;
ray.ni@...
Cc: Kinney, Michael D <michael.d.kinney@...>; lichao
<lichao@...>; Kirkendall, Garrett
<Garrett.Kirkendall@...>; Grimes, Paul <Paul.Grimes@...>;
He,
Jiangang <Jiangang.He@...>; Attar, AbdulLateef (Abdul Lateef)
<AbdulLateef.Attar@...>; Leif Lindholm
<quic_llindhol@...>; Andrew Fish <afish@...>
Subject: RE: [edk2-devel] The principles of EDK2 module reconstruction
for archs

[AMD Official Use Only - General]

I just had created PR to update edkII C coding standard spec for the
file and directory naming. We can review and confirm this update first
and then go back to the principles of EDK2 module reconstruction for archs.
Here is the PR:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
ub.com%2Ftianocore-docs%2Fedk2-
&amp;data=05%7C01%7CAbner.Chang%40amd.c
om%7Cd825e248625541e3f43e08daa1ee2883%7C3dd8961fe4884e608e11a82
d994e18
3d%7C0%7C0%7C638000341502885565%7CUnknown%7CTWFpbGZsb3d8eyJ
WIjoiMC4wLj
AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%
7C%7C&a
mp;sdata=X4z9puj81nIGTqtMxC9igDZyBPOT6OTWSU%2BjoIowo%2BE%3D&a
mp;reserv
ed=0
CCodingStandardsSpecification/pull/2

The naming rule is mainly for the new module or new file IMO. Some
existing module may not meet the guidelines mentioned in this spec.
Thus we need the principles of EDK2 module reconstruction on the
existing module to support other processor archs and not impacting the
existing platforms (e.g.
rename the INF file or directory to meet the guidelines).

Sunil, seems RISC-V CpuDxe meet the guideline. Please check it.
Just feel that having CpuDxe.c to Riscv64 folder is not quite a best
solution. I think at least we can abstract the protocol structure and
protocol installation under CpuDxe\ and have the arch implementation
under arch folder. We can discuss this later after we confirming the
guideline and principles.

Thanks
Abner

-----Original Message-----
From: Sunil V L <sunilvl@...>
Sent: Wednesday, September 28, 2022 3:34 PM
To: devel@edk2.groups.io; ray.ni@...
Cc: Chang, Abner <Abner.Chang@...>; Kinney, Michael D
<michael.d.kinney@...>; lichao <lichao@...>;
Kirkendall, Garrett <Garrett.Kirkendall@...>; Grimes, Paul
<Paul.Grimes@...>; He, Jiangang <Jiangang.He@...>; Attar,
AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>; Leif
Lindholm <quic_llindhol@...>; Andrew Fish <afish@...>
Subject: Re: [edk2-devel] The principles of EDK2 module
reconstruction for archs

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


On Wed, Sep 28, 2022 at 03:33:45AM +0000, Ni, Ray wrote:
Hi Ray,

1. When a new arch's implementation is introduced to the
existing
module which was developed for the specific arch:

1. The folder reconstruction:

* Create arch folder for the existing arch implementation
[Ray] Do you move existing arch implementation to that arch folder?
It will
break existing platforms a lot.

* Create the arch folder for the new introduced arch
[Ray] I agree. But if we don't create arch folder for existing
arch
implementation, the pkg layout will be a mess.

[Ray] Hard for me to understand all the principles here. Maybe we
review
existing code including to-be-upstreamed code and decide how to go.
Could you please take a look below changes which is trying to add
RISC-V support for CpuDxe?
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
ub.com%2Ftianocore%2Fedk2-
staging%2Fcommit%2Fbba1a11be47dd091734e185afbed73ea75708749&amp;
data=05%7C01%7Cabner.chang%40amd.com%7Ca419e6a010d34fde464b08d
aa123e080%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C63799947
2732494527%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIj
oiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sd
ata=Vq6pJLnn8yJrJhFZn7LfLbZzrtpG4n1VLWgAil6J38U%3D&amp;reserved=0
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
ub.com%2Ftianocore%2Fedk2-
staging%2Fcommit%2F7fccf92a97a6d0618a20f10622220e78b3687906&amp;da
ta=05%7C01%7Cabner.chang%40amd.com%7Ca419e6a010d34fde464b08daa1
23e080%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C63799947273
2494527%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV
2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata
=xFmvUv58vh4AUAM17Qy9G5jZWFZlK2Ozt3njpG1e8%2BY%3D&amp;reserv
ed=0

What do you suggest with above example?

1) Common INF for all architectures - but modify INF alone, no X86
folder creation.

This is what I have done in the commit above. May be of least impact
to existing code since it is only INF change. But like you mentioned
this is bit weird that X86 files will remain in root folder directly
along with some common files.

2) Common INF (CpuDxe.inf) + create arch folders X86, X64, IA32,
RiscV64 etc

IMO, this is probably the best approach. What would be the
challenges with this?

3) Separate INF for arch like CpuDxe.inf for x86, CpuDxeRiscV64.inf
for
RISC-V.

This again probably is not a good idea.

4) If the module/library is specific to one arch (ex: SMM(X86),
SBI(RISC-V)), then create separate INF.

Thanks!
Sunil