Hi Laszlo,

Thanks,
Chao

On 2023/11/21 22:37, Laszlo Ersek wrote:

On 11/17/23 10:59, Chao Li wrote:

Since some ARCH or platform not require execute code on memory during
PEI phase, some values may transferred via CPU registers.

Adding PeiServcieTablePointerLibReg to allow set and get the PEI service
table pointer depend by a CPU register, this library can accommodate lot
of platforms who not require execte code on memory during PEI phase.

Adding PeiServiceTablePointerLibReg to allows setting and getting the
PEI service table pointer via CPU registers, and the library can
accommodate many platforms that do not need to execute code on memory
during the PEI phase.

The idea of this library is derived from
ArmPkg/Library/PeiServicesTablePointerLib/

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4584

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Sunil V L <sunilvl@ventanamicro.com>
Signed-off-by: Chao Li <lichao@loongson.cn>
---
 .../Library/PeiServicesTablePointerLib.h      | 37 +++++++-
 .../PeiServicesTablePointer.c                 | 86 +++++++++++++++++++
 .../PeiServicesTablePointerLib.uni            | 20 +++++
 .../PeiServicesTablePointerLibReg.inf         | 40 +++++++++
 MdePkg/MdePkg.dsc                             |  1 +
 5 files changed, 180 insertions(+), 4 deletions(-)
 create mode 100644 MdePkg/Library/PeiServicesTablePointerLibReg/PeiServicesTablePointer.c
 create mode 100644 MdePkg/Library/PeiServicesTablePointerLibReg/PeiServicesTablePointerLib.uni
 create mode 100644 MdePkg/Library/PeiServicesTablePointerLibReg/PeiServicesTablePointerLibReg.inf

In my opinion, the PeiServicesTablePointerLib class header should not be
extended with new interfaces. I understand that the generality is
attractive, but it is not put to use; only the loongarch architecture
applies the new interfaces (in the subsequent patch), and for example
the ARM code (ArmPkg/Library/PeiServicesTablePointerLib) is not reworked
in terms of these new interfaces.

This libarary have ability of accommodate more ARCH why not? I checked the PI SPEC, all ARCH except IA32 and X64 using the register mechanism, if this library can be approved, all of them can moved into this libraryso that code con be reused more, I think this library is fine.


What's more, the new library interfaces, even though they are exposed in
the lib class header, are not implemented for other architectures, so
they aren't even callable on those arches.

The patch 10 in this series has added LoongArch instance of this library, please check.


I'm commenting on this patch and the subsequent patch in the series
together, as seen squashed together. NB I'm not an MdePkg maintainer, so
this is just my opinion.

So, Mike and Liming, what do your think?


(1) As noted above, the library class should not be modified.

(2) Modifying the *comments* in
"MdePkg/Include/Library/PeiServicesTablePointerLib.h" is welcome, I
think, but then we might want to add a (separate!) patch for removing
the Itanium language, as edk2 no longer supports Itanium.

(3) The PeiServicesTablePointerLibReg instance should be called
PeiServicesTablePointerLibCsrKs0 or just PeiServicesTablePointerLibKs0.

This library will be a public libray which using the reigster mechanism, so the name like PeiServiceTablePointerLibCsrKs0 would not appropriate.


This follows the example of the lib instance name
"PeiServicesTablePointerLibIdt". The whole library instance should be
loongaarch-specific IMO; there isn't much code that's being duplicated,
so the extra interfaces (internal or external) do not help with code
unification.

(4) "PeiServicesTablePointerLib.uni" should be named similarly (suffix
missing).

(5) BASE_NAME in the library instance INF file should be defined
similarly (suffix missing).

(6) The contents of the UNI file should be loongarch-specific, i.e. be
explicit about CSR KS0, in both comments and string constants.

(7) The comments in the library instance INF file should be similarly
loongarch-specific.

(8) I suggest dropping VALID_ARCHITECTURES altogether. If we want to
keep it, it should exclusively say LOONGARCH64.

(9) The new library instance should be listed in
[Components.LOONGARCH64] in MdePkg.dec.

This section does not exist yet; I suggest introducing it under
[Components.RISCV64].

No, it is RISC-V area, not LOONGARCH64. And I do not recommend going this way. I believe this library should be a public library for register mechanism.


(10) There need not / should not be two separate C source files; just
access the KS0 CSR in SetPeiServicesTablePointer() and
GetPeiServicesTablePointer() directly.

(11) The new library instance should probably not introduce new
references to Itanium.

Thanks,
Laszlo

_._,_._,_

Groups.io Links:

You receive all messages sent to this group.

_._,_._,_