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 <lic...@loongson.cn>
*发送时间:*2023年11月27日11:28
*收件人:*Laszlo Ersek <ler...@redhat.com>; devel@edk2.groups.io
*抄送:*Michael D Kinney <michael.d.kin...@intel.com>; Liming Gao
<gaolim...@byosoft.com.cn>; Zhiguang Liu <zhiguang....@intel.com>;
Leif Lindholm <quic_llind...@quicinc.com>; Ard Biesheuvel
<ardb+tianoc...@kernel.org>; Sami Mujawar <sami.muja...@arm.com>;
Sunil V L <suni...@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.kin...@intel.com>
<mailto:michael.d.kin...@intel.com>
Cc: Liming Gao <gaolim...@byosoft.com.cn>
<mailto:gaolim...@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang....@intel.com>
<mailto:zhiguang....@intel.com>
Cc: Leif Lindholm <quic_llind...@quicinc.com>
<mailto:quic_llind...@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>
<mailto:ardb+tianoc...@kernel.org>
Cc: Sami Mujawar <sami.muja...@arm.com>
<mailto:sami.muja...@arm.com>
Cc: Laszlo Ersek <ler...@redhat.com>
<mailto:ler...@redhat.com>
Cc: Sunil V L <suni...@ventanamicro.com>
<mailto:suni...@ventanamicro.com>
Signed-off-by: Chao Li <lic...@loongson.cn>
<mailto:lic...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
