Re: [edk2-devel] [PATCH v1 5/7] OvmfPkg: Specifies SmmCpuSyncLib instance

["Laszlo Ersek" <lersek@redhat.com>]

On 11/3/23 16:30, Wu, Jiaxin wrote:
> The SmmCpuSyncLib instance is included in UefiCpuLibs.dsc.inc.
> This patch is to specify SmmCpuSyncLib instance in OvmfPkg by
> using "!include UefiCpuPkg/UefiCpuLibs.dsc.inc".
> 
> Change-Id: I2ab1737425e26a7bfc4f564b3b7f15ca5c2268fb
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zeng Star <star.zeng@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
> ---
>  OvmfPkg/CloudHv/CloudHvX64.dsc | 1 +
>  OvmfPkg/OvmfPkgIa32.dsc        | 1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc     | 1 +
>  OvmfPkg/OvmfPkgX64.dsc         | 1 +
>  4 files changed, 4 insertions(+)
> 
> diff --git a/OvmfPkg/CloudHv/CloudHvX64.dsc b/OvmfPkg/CloudHv/CloudHvX64.dsc
> index c23c7eaf6c..e65767fe16 100644
> --- a/OvmfPkg/CloudHv/CloudHvX64.dsc
> +++ b/OvmfPkg/CloudHv/CloudHvX64.dsc
> @@ -122,10 +122,11 @@
>  # Library Class section - list of all Library Classes needed by this Platform.
>  #
>  ################################################################################
>  
>  !include MdePkg/MdeLibs.dsc.inc
> +!include UefiCpuPkg/UefiCpuLibs.dsc.inc
>  
>  [LibraryClasses]
>    PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
>    TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
>    ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index ed3a19feeb..07d16e6935 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -125,10 +125,11 @@
>  # Library Class section - list of all Library Classes needed by this Platform.
>  #
>  ################################################################################
>  
>  !include MdePkg/MdeLibs.dsc.inc
> +!include UefiCpuPkg/UefiCpuLibs.dsc.inc
>  
>  [LibraryClasses]
>    PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
>    TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
>    ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 16ca139b29..8d243b7075 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -130,10 +130,11 @@
>  # Library Class section - list of all Library Classes needed by this Platform.
>  #
>  ################################################################################
>  
>  !include MdePkg/MdeLibs.dsc.inc
> +!include UefiCpuPkg/UefiCpuLibs.dsc.inc
>  
>  [LibraryClasses]
>    PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
>    TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
>    ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index dc1a0942aa..6343789152 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -143,10 +143,11 @@
>  # Library Class section - list of all Library Classes needed by this Platform.
>  #
>  ################################################################################
>  
>  !include MdePkg/MdeLibs.dsc.inc
> +!include UefiCpuPkg/UefiCpuLibs.dsc.inc
>  
>  [LibraryClasses]
>    PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
>    TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
>    ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf

I am undecided about this patch.

More precisely, I'm undecided that this is the right way to introduce
"UefiCpuLibs.dsc.inc".

In my opinion,"UefiCpuLibs.dsc.inc" should be a general-purpose include
file.

But, at this moment, the include file only resolves the "SmmCpuSyncLib"
class -- and that resolution is useless for any IA32/X64 platform that
does not use the SMM machinery. In particular, the resolution is useless
even for OVMF X64 (for example) if it is built without SMM_REQUIRE.

Furthermore, OVMF X64 uses a bunch of other UefiCpuPkg libraries:

BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
CcExitLibNull/CcExitLibNull.inf
CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
CpuPageTableLib/CpuPageTableLib.inf
MicrocodeLib/MicrocodeLib.inf
MmSaveStateLib/AmdMmSaveStateLib.inf
MpInitLib/DxeMpInitLib.inf
MpInitLib/PeiMpInitLib.inf
MpInitLibUp/MpInitLibUp.inf
MtrrLib/MtrrLib.inf
SmmCpuRendezvousLib/SmmCpuRendezvousLib.inf

It's weird to put SmmCpuSyncLib in the new DSC include file, but not
CpuPageTableLib or MtrrLib (for example), which seem much more generic.

For now I'd suggest avoiding "UefiCpuLibs.dsc.inc" altogether, and
resolving just "SmmCpuSyncLib" in the OVMF DSC files -- and only when
SMM_REQUIRE is defined on the build command line (i.e., when
PiSmmCpuDxeSmm is built into the firmware platform).

We can certainly introduce "UefiCpuLibs.dsc.inc" later, but for that:

- we need to collect all UefiCpuPkg library classes and instances that
are commonly used (and then probably use multiple module type
sections!). Example: "OvmfPkg/Include/Dsc/OvmfTpmLibs.dsc.inc"

- some of those resolutions should be conditional, and so we'd have to
properly prefix macro names like SMM_REQUIRE with something like
UEFI_CPU_. Basically introduce a namespace for those build time macros.
Example: "NetworkPkg/NetworkDefines.dsc.inc", which uses the NETWORK_
macro prefix for enabling various features.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110840): https://edk2.groups.io/g/devel/message/110840
Mute This Topic: https://groups.io/mt/102366302/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-