Re: 回复: [edk2-devel] [PATCH v3 09/39] MdePkg: Add a new library named PeiServicesTablePointerLibReg

["Chao Li" <lichao@loongson.cn>]

[-- Attachment #1: Type: text/plain, Size: 11459 bytes --]

Hi Liming,

Ok, I see. In V4 I will add the PeiServicesTablePointerLib only for 
LoongArch, and it probably be named PeiServicesTablePointerLibKs0.


Thanks,
Chao
On 2023/12/1 08:32, gaoliming wrote:
>
> Chao:
>
>  I agree with Laszlo. I want to highlight that current design has meet 
> with your requirement. So, I don’t think we need to add new APIs in 
> PeiServicesTablePointerLib header file. Loong Arch can implement its 
> PeiServicesTablePointerLib library instance base on its specific 
> register.
>
> Thanks
>
> Liming
>
> *发件人:*Chao Li <lichao@loongson.cn>
> *发送时间:*2023年11月27日11:28
> *收件人:*Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io
> *抄送:*Michael D Kinney <michael.d.kinney@intel.com>; Liming Gao 
> <gaoliming@byosoft.com.cn>; Zhiguang Liu <zhiguang.liu@intel.com>; 
> Leif Lindholm <quic_llindhol@quicinc.com>; Ard Biesheuvel 
> <ardb+tianocore@kernel.org>; Sami Mujawar <sami.mujawar@arm.com>; 
> Sunil V L <sunilvl@ventanamicro.com>
> *主题:*Re: [edk2-devel] [PATCH v3 09/39] MdePkg: Add a new library named 
> PeiServicesTablePointerLibReg
>
> Hi Mike and Liming,
>
> You opinion is very important, it will decide the direction. I will 
> send the V4 this week, so can you please review the new patch of 
> MdePkg for this series?
>
> Thanks,
> Chao
>
> On 2023/11/24 19:35, Laszlo Ersek wrote:
>
>     On 11/22/23 02:47, Chao Li wrote:
>
>         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>
>                 <mailto:michael.d.kinney@intel.com>
>
>                 Cc: Liming Gao <gaoliming@byosoft.com.cn>
>                 <mailto:gaoliming@byosoft.com.cn>
>
>                 Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>                 <mailto:zhiguang.liu@intel.com>
>
>                 Cc: Leif Lindholm <quic_llindhol@quicinc.com>
>                 <mailto:quic_llindhol@quicinc.com>
>
>                 Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>                 <mailto:ardb+tianocore@kernel.org>
>
>                 Cc: Sami Mujawar <sami.mujawar@arm.com>
>                 <mailto:sami.mujawar@arm.com>
>
>                 Cc: Laszlo Ersek <lersek@redhat.com>
>                 <mailto:lersek@redhat.com>
>
>                 Cc: Sunil V L <sunilvl@ventanamicro.com>
>                 <mailto:sunilvl@ventanamicro.com>
>
>                 Signed-off-by: Chao Li <lichao@loongson.cn>
>                 <mailto: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.
>
>     The library may be fine from a design point of view, but without
>
>     actually putting the extra generality to use, it's a waste. It's a
>
>     maintenance burden. There's a name for this anti-pattern: it is called
>
>     "speculative generality". "It might be useful down the road."
>
>     The new generality is only useful if it carries its own weight;
>     namely,
>
>     if other platform code (aarch64, x64) is converted to it
>     immediately, in
>
>     the same series. (I'm not asking for this series to be longer. You
>     could
>
>     even split it up into multiple "waves" of series.) Just saying that
>
>     "could prove useful later" is a prime way to generate technical debt.
>
>             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.
>
>     Yes, I'm aware. That's not the point.
>
>     When you extend a library *class* with a new API, that means all
>
>     *clients* of the library class can stat calling that API. Which in
>     turn
>
>     means that *all* existent instances of the library class must
>     implement
>
>     the API as well.
>
>     Your series extends the lib class with a new API, but (IIUC) only
>
>     implements the new API in one (new) lib instance, and not in the other
>
>     (existent) instances. This has the potential to cause linkage errors,
>
>     dependent on the actual library instance that a platform DSC chooses.
>
>             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.
>
>     Of course that name is wrong for a generic library instance, but my
>
>     whole point is that this library instance should be
>     loongarch-specific.
>
>     (Unless you port the existent (x64 IDT / aarch64 register) libraries
>
>     over to it.)
>
>             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.
>
>     You misunderstood.
>
>     I didn't suggest to list the *library instance* under
>     [Components.RISCV64].
>
>     I suggested to introduce the [Components.LOONGARCH64] *section* under
>
>     [Components.RISCV64].
>
>         And I do not recommend going
>
>         this way. I believe this library should be a public library
>         for register
>
>         mechanism.
>
>     That's entirely fine, as long as you do the work of porting the
>     existent
>
>     ARM and X64 IDT code over to it. In my opinion anyway; MdePkg
>
>     maintainers are the authoritative sources here.
>
>     Laszlo
>
>             (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.
View/Reply Online (#111975): https://edk2.groups.io/g/devel/message/111975
Mute This Topic: https://groups.io/mt/102906469/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 27511 bytes --]