From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 7F498D80477 for ; Fri, 1 Dec 2023 00:33:00 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=uYu6cCPFP8Mfpssr1MLOHbZ/D+7p1oKUbkjsnmID/fc=; c=relaxed/simple; d=groups.io; h=From:To:Cc:References:In-Reply-To:Subject:Date:Message-ID:MIME-Version:Thread-Index:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Language; s=20140610; t=1701390779; v=1; b=Pm3gMnAQy1A29qJe6BzKE8db3ycqVPcOMlhCZLN2vEoXvUsI1e2Rj5tIWg+GH4Texjppi5DJ dR8/a27Etq+OdW4CGErdsgi/ZPahS4wfIYDEfMnk1vPAvQW+YSEAvLOJpnzC9Kydr/S2dOclaOu NaDn7Bj+SZYT3kaXWO3oQT7c= X-Received: by 127.0.0.2 with SMTP id Vnn8YY7687511xzspoRo3hei; Thu, 30 Nov 2023 16:32:59 -0800 X-Received: from cxsh.intel-email.com (cxsh.intel-email.com [121.46.250.151]) by mx.groups.io with SMTP id smtpd.web10.11797.1701390777775221153 for ; Thu, 30 Nov 2023 16:32:58 -0800 X-Received: from cxsh.intel-email.com (localhost [127.0.0.1]) by cxsh.intel-email.com (Postfix) with ESMTP id 0F633DDA7E2 for ; Fri, 1 Dec 2023 08:32:55 +0800 (CST) X-Received: from localhost (localhost [127.0.0.1]) by cxsh.intel-email.com (Postfix) with ESMTP id 0A7F1DDA7E6 for ; Fri, 1 Dec 2023 08:32:55 +0800 (CST) X-Received: from mail.byosoft.com.cn (mail.byosoft.com.cn [58.240.74.242]) by cxsh.intel-email.com (Postfix) with SMTP id 24A41DDA7E3 for ; Fri, 1 Dec 2023 08:32:50 +0800 (CST) X-Received: from DESKTOPS6D0PVI ([58.246.60.130]) (envelope-sender ) by 192.168.6.13 with ESMTP(SSL) for ; Fri, 01 Dec 2023 08:32:43 +0800 X-WM-Sender: gaoliming@byosoft.com.cn X-Originating-IP: 58.246.60.130 X-WM-AuthFlag: YES X-WM-AuthUser: gaoliming@byosoft.com.cn From: "gaoliming via groups.io" To: "'Chao Li'" , "'Laszlo Ersek'" , Cc: "'Michael D Kinney'" , "'Zhiguang Liu'" , "'Leif Lindholm'" , "'Ard Biesheuvel'" , "'Sami Mujawar'" , "'Sunil V L'" References: <20231117095742.3605778-1-lichao@loongs> <20231117095949.3608941-1-lichao@loongson.cn> <03178ec2-b5e1-0ca7-3bd5-afe4e6d50554@redhat.com> <3975af54-465a-4549-9273-8dd1228f8b44@loongson.cn> <2ba95277-b93f-4412-8676-b9dd7f3b2565@loongson.cn> In-Reply-To: <2ba95277-b93f-4412-8676-b9dd7f3b2565@loongson.cn> Subject: =?UTF-8?B?5Zue5aSNOiBbZWRrMi1kZXZlbF0gW1BBVENIIHYzIDA5LzM5XSBNZGVQa2c6IEFkZCBhIG5ldyBsaWJyYXJ5IG5hbWVkIFBlaVNlcnZpY2VzVGFibGVQb2ludGVyTGliUmVn?= Date: Fri, 1 Dec 2023 08:32:45 +0800 Message-ID: <060301da23ed$e782eef0$b688ccd0$@byosoft.com.cn> MIME-Version: 1.0 Thread-Index: AQDW4ezWdlkHGoo+FGEH2DBHd4P2BAH1vQ1ZAsBZup8BQk0vAAIxjKkpApGfwvqyRDoF4A== Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,gaoliming@byosoft.com.cn List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: HZfn5veXFZkKD9BXWn3yvmE8x7686176AA= Content-Type: multipart/alternative; boundary="----=_NextPart_000_0604_01DA2430.F5A89FF0" Content-Language: zh-cn X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=Pm3gMnAQ; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=none ------=_NextPart_000_0604_01DA2430.F5A89FF0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Chao: I agree with Laszlo. I want to highlight that current design has meet with= your requirement. So, I don=E2=80=99t think we need to add new APIs in Pei= ServicesTablePointerLib header file. Loong Arch can implement its PeiServic= esTablePointerLib library instance base on its specific register.=20 =20 Thanks Liming =E5=8F=91=E4=BB=B6=E4=BA=BA: Chao Li =20 =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2023=E5=B9=B411=E6=9C=8827=E6=97=A5 1= 1:28 =E6=94=B6=E4=BB=B6=E4=BA=BA: Laszlo Ersek ; devel@edk2.g= roups.io =E6=8A=84=E9=80=81: Michael D Kinney ; Liming G= ao ; Zhiguang Liu ; Leif = Lindholm ; Ard Biesheuvel ; Sami Mujawar ; Sunil V L =E4=B8=BB=E9=A2=98: Re: [edk2-devel] [PATCH v3 09/39] MdePkg: Add a new lib= rary named PeiServicesTablePointerLibReg =20 Hi Mike and Liming, You opinion is very important, it will decide the direction. I will send th= e V4 this week, so can you please review the new patch of MdePkg for this s= eries? =20 Thanks, Chao On 2023/11/24 19:35, Laszlo Ersek wrote: On 11/22/23 02:47, Chao Li wrote: Hi Laszlo, =20 =20 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. =20 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. =20 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. =20 The idea of this library is derived from ArmPkg/Library/PeiServicesTablePointerLib/ =20 BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4584 =20 Cc: Michael D Kinney Cc: Liming Gao Cc: Zhiguang Liu Cc: Leif Lindholm Cc: Ard Biesheuvel Cc: Sami Mujawar Cc: Laszlo Ersek Cc: Sunil V L Signed-off-by: Chao Li --- .../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/PeiService= sTablePointer.c create mode 100644 MdePkg/Library/PeiServicesTablePointerLibReg/PeiService= sTablePointerLib.uni create mode 100644 MdePkg/Library/PeiServicesTablePointerLibReg/PeiService= sTablePointerLibReg.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. =20 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. =20 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." =20 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. =20 =20 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. =20 Yes, I'm aware. That's not the point. =20 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. =20 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. =20 =20 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. =20 (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. =20 (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. =20 Of course that name is wrong for a generic library instance, but my whole point is that this library instance should be loongarch-specific. =20 (Unless you port the existent (x64 IDT / aarch64 register) libraries over to it.) =20 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. =20 (4) "PeiServicesTablePointerLib.uni" should be named similarly (suffix missing). =20 (5) BASE_NAME in the library instance INF file should be defined similarly (suffix missing). =20 (6) The contents of the UNI file should be loongarch-specific, i.e. be explicit about CSR KS0, in both comments and string constants. =20 (7) The comments in the library instance INF file should be similarly loongarch-specific. =20 (8) I suggest dropping VALID_ARCHITECTURES altogether. If we want to keep it, it should exclusively say LOONGARCH64. =20 (9) The new library instance should be listed in [Components.LOONGARCH64] in MdePkg.dec. =20 This section does not exist yet; I suggest introducing it under [Components.RISCV64]. No, it is RISC-V area, not LOONGARCH64. =20 You misunderstood. =20 I didn't suggest to list the *library instance* under [Components.RISCV64]. =20 I suggested to introduce the [Components.LOONGARCH64] *section* under [Components.RISCV64]. =20 And I do not recommend going this way. I believe this library should be a public library for register mechanism. =20 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. =20 Laszlo =20 (10) There need not / should not be two separate C source files; just access the KS0 CSR in SetPeiServicesTablePointer() and GetPeiServicesTablePointer() directly. =20 (11) The new library instance should probably not introduce new references to Itanium. =20 Thanks, Laszlo -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111933): https://edk2.groups.io/g/devel/message/111933 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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- ------=_NextPart_000_0604_01DA2430.F5A89FF0 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable

Chao:

=C2=A0I= agree with Laszlo. I want to highlight that current design has meet with y= our requirement. So, I don=E2=80=99t think we need to add new APIs in PeiSe= rvicesTablePointerLib header file. Loong Arch can implement its PeiServices= TablePointerLib library instance base on its specific register. =

 

Thanks

Liming

=E5=8F=91=E4=BB=B6=E4=BA= =BA: Chao Li <lichao@loongson.cn&= gt;
=E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2023=E5=B9=B411=E6=9C=8827=E6=97=A5 11:28
=E6=94=B6=E4= =BB=B6=E4=BA=BA: Laszlo Er= sek <lersek@redhat.com>; devel@edk2.groups.io
=E6=8A=84= =E9=80=81: Michael D Kinne= y <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@ventanam= icro.com>
=E4=B8=BB=E9=A2=98:= Re: [edk2-devel] [PATCH v3 09/39] MdePkg: Add a new lib= rary named PeiServicesTablePointerLibReg

=

 

<= p>Hi Mike and Liming,<= span lang=3DEN-US>

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 Mde= Pkg for this series?

 

Thanks,
Ch= ao

On 2023/11/24 19:35, Laszlo Ersek wrote:

<= blockquote style=3D'margin-top:5.0pt;margin-bottom:5.0pt'>
On 11/22/23 02:47, Chao Li wrote:
H=
i Laszlo,
 <=
/span>
 
Thanks,
Chao<=
o:p>
On 2023/11/21 22:37, Laszlo =
Ersek wrote:
On 11/17/23 10:59, Chao Li wrot=
e:
Since some ARCH or platform not require e=
xecute 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.
&nbs=
p;
Adding PeiServiceTablePointerL=
ibReg to allows setting and getting the
PEI service table pointer via CPU registers, and the library ca=
n
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://b=
ugzilla.tianocore.org/show_bug.cgi?id=3D4584
 
=
Cc: Michael D Kinney <mich=
ael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming=
@byosoft.com.cn>
Cc: =
Zhiguang Liu <zhiguang.liu@int=
el.com>
Cc: Leif Lind=
holm <quic_llindhol@quicinc=
.com>
Cc: Ard Biesheu=
vel <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/Pe=
iServicesTablePointerLib.h=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | 37 +++++++-=

 .../PeiServicesTablePointer.c=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0 | 86 +++++++++++++++++++
=
 .../PeiServicesTablePointerLib.uni=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | 20 +++++<=
/pre>
 .../PeiServicesTablePointerLibReg.inf=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | 40 +++++++++=

 MdePkg/MdePkg.dsc=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0 =
1 +
 5 files changed, 180 in=
sertions(+), 4 deletions(-)
=
 create mode 100644 MdePkg/Library/PeiServicesTablePointerLibReg/PeiService=
sTablePointer.c
 create mode=
 100644 MdePkg/Library/PeiServicesTablePointerLibReg/PeiServicesTablePointe=
rLib.uni
 create mode 100644=
 MdePkg/Library/PeiServicesTablePointerLibReg/PeiServicesTablePointerLibReg=
.inf
In my opin=
ion, the PeiServicesTablePointerLib class header should not be
extended with new interfaces. I understa=
nd that the generality is
at=
tractive, 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 re=
worked
in terms of these new=
 interfaces.
 
This libarary have abil=
ity of accommodate more ARCH why not? I checked
the PI SPEC, all ARCH except IA32 and X64 using the reg= ister mechanism,
if this lib=
rary can be approved, all of them can moved into this
libraryso that code con be reused more, I think t=
his 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 bu=
rden. There's a name for this anti-pattern: it is called<=
/pre>
"speculative generality". "It m=
ight be useful down the road."
 
The new gene=
rality is only useful if it carries its own weight; namely,
if other platform code (aarch64, x64) is co=
nverted to it immediately, in
the same series. (I'm not asking for this series to be longer. You could<=
o:p>
even split it up into multip=
le "waves" of series.) Just saying that
"could prove useful later" is a prime way t=
o generate technical debt.
<=
o:p> 
 
=
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 add=
ed LoongArch instance of this
library, please check.
 
Yes, I'm aw=
are. That's not the point.
<=
o:p> 
When you extend a libr=
ary *class* with a new API, that means all
*clients* of the library class can stat calling that API. Wh=
ich 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 i=
nstance, 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 se=
ries
together, as seen squas=
hed 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 n=
ot be modified.
 <=
/o:p>
(2) Modifying the *comments* in
"MdePkg/Include/Library/=
PeiServicesTablePointerLib.h" is welcome, I
think, but then we might want to add a (separate!) pat= ch for removing
the Itanium =
language, as edk2 no longer supports Itanium.
<=
span lang=3DEN-US> 
(3)=
 The PeiServicesTablePointerLibReg instance should be called
PeiServicesTablePointerLibCsrKs0 or just P=
eiServicesTablePointerLibKs0.
This library will be a public libray which using the reigste=
r mechanism,
so the name lik=
e PeiServiceTablePointerLibCsrKs0 would not appropriate.<=
/pre>
 
Of course that name is wrong for a generic library ins=
tance, but my
whole point is=
 that this library instance should be loongarch-specific.=

 
(Unless you port the existent (x64 IDT / aarch64 register) librari=
es
over to it.)
 
This follows the e=
xample of the lib instance name
"PeiServicesTablePointerLibIdt". The whole library instance s=
hould 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
unificati=
on.
 =

(4) "PeiServicesTablePointerLib.uni"=
; should be named similarly (suffix
missing).
&nbs=
p;
(5) BASE_NAME in the library i=
nstance INF file should be defined
similarly (suffix missing).
 
(6) The cont=
ents of the UNI file should be loongarch-specific, i.e. be
explicit about CSR KS0, in both comments and=
 string constants.
&nbs=
p;
(7) The comments in the librar=
y instance INF file should be similarly
loongarch-specific.
 
(8) I suggest dro=
pping VALID_ARCHITECTURES altogether. If we want to
=

keep it, it should exclusively say LOONGARCH64.
 
<=
pre>(9) The new library instance should be listed in
[Components.LOONGARCH64] in Mde=
Pkg.dec.
 
This section does not exist yet; I sugge=
st introducing it under
[Com=
ponents.RISCV64].
No, it is RISC-V area, not LOONGARCH64.
 
You misunderstood.
=
 
I didn't suggest to l=
ist the *library instance* under [Components.RISCV64].
 
I suggested to introduce the [Components.LOONGARCH64] *section* under=

[Components.RISCV64].<=
/o:p>
 
And I do not recommend going
this way. I believe this library should be a public library for regis=
ter
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 her=
e.
 <=
/pre>
Laszlo
 
(10) There need not / should not be two sepa=
rate C source files; just
ac=
cess the KS0 CSR in SetPeiServicesTablePointer() and
GetPeiServicesTablePointer() directly.<=
/span>
 
(11) The new library instance should probably not introduce =
new
references to Itanium.
 
Thanks,
Laszlo
<=
/div>
_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#111933) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
------=_NextPart_000_0604_01DA2430.F5A89FF0--