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 ADE6AAC0930 for ; Wed, 29 Nov 2023 01:35:46 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=06MCMcy19tvWvA25tM5N3BNsedaOCB3Vsx7qVRHcI2U=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:User-Agent:Subject:From:To:Cc:Reply-To:References:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1701221745; v=1; b=ZwEhXhLVPDuKcMfNNzlJffLtdYvNMYZQ4ouIp/7cNsDz3YVEnB4dn28xr8wk8RZKUgc7ghC1 H56t3+aojDOFho/nI6ECbhc9M3cv0wiBKougXW9Q8wTCG+DFtTo+F3qHhymom7Dc/cpClbK+AO1 pI+rQ9d3BYN3ccrCk94CS3XE= X-Received: by 127.0.0.2 with SMTP id UrczYY7687511xEiOzY8JYkl; Tue, 28 Nov 2023 17:35:45 -0800 X-Received: from mail.loongson.cn (mail.loongson.cn [114.242.206.163]) by mx.groups.io with SMTP id smtpd.web10.17683.1701221743245861078 for ; Tue, 28 Nov 2023 17:35:43 -0800 X-Received: from loongson.cn (unknown [10.40.24.149]) by gateway (Coremail) with SMTP id _____8CxLOtqlWZl9Y09AA--.51322S3; Wed, 29 Nov 2023 09:35:38 +0800 (CST) X-Received: from [10.40.24.149] (unknown [10.40.24.149]) by localhost.localdomain (Coremail) with SMTP id AQAAf8CxO9xklWZlzlVPAA--.43734S3; Wed, 29 Nov 2023 09:35:32 +0800 (CST) Message-ID: <99ad91ed-8d9f-468e-8fc5-1f9cd698cf8f@loongson.cn> Date: Wed, 29 Nov 2023 09:35:32 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [PATCH v3 09/39] MdePkg: Add a new library named PeiServicesTablePointerLibReg From: "Chao Li" To: devel@edk2.groups.io, Michael D Kinney , Liming Gao Cc: Zhiguang Liu , Leif Lindholm , Ard Biesheuvel , Sami Mujawar , Sunil V L , Laszlo Ersek Reply-To: devel@edk2.groups.io,lichao@loongson.cn 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> <179B5D231F190982.32091@groups.io> In-Reply-To: <179B5D231F190982.32091@groups.io> X-CM-TRANSID: AQAAf8CxO9xklWZlzlVPAA--.43734S3 X-CM-SenderInfo: xolfxt3r6o00pqjv00gofq/1tbiAQAKCGVlTisGrgACsM X-Coremail-Antispam: 1Uk129KBj93XoWxtFy7Zw4UJw43WFW3Zw45XFc_yoW3XFy7pF 43Kr45tr4DJrWSyry2va1UuryY9Fs7CFy3CrnrGF1UCwn0vryvqr42grWFk3Wrurs5uw1I vrW7tw1UuayDZFXCm3ZEXasCq-sJn29KB7ZKAUJUUUUk529EdanIXcx71UUUUU7KY7ZEXa sCq-sGcSsGvfJ34c02F40En4AKxVAvwIkv4cxYr27v73VFW2AGmfu7bjvjm3AaLaJ3UjIY CTnIWjp_UUUYR7kC6x804xWl14x267AKxVWUJVW8JwAFc2x0x2IEx4CE42xK8VAvwI8IcI k0rVWrJVCq3wAFIxvE14AKwVWUGVWUXwA2ocxC64kIII0Yj41l84x0c7CEw4AK67xGY2AK 021l84ACjcxK6xIIjxv20xvE14v26r4j6ryUM28EF7xvwVC0I7IYx2IY6xkF7I0E14v26r 4j6F4UM28EF7xvwVC2z280aVAFwI0_Gr0_Cr1l84ACjcxK6I8E87Iv6xkF7I0E14v26r4j 6r4UJwAaw2AFwI0_Jw0_GFyle2I262IYc4CY6c8Ij28IcVAaY2xG8wAqjxCEc2xF0cIa02 0Ex4CE44I27wAqx4xG62kEwI0EY4vaYxAvb48xMcIj6xIIjxv20xvE14v26r126r1DMcIj 6I8E87Iv67AKxVWUJVW8JwAm72CE4IkC6x0Yz7v_Jr0_Gr1lF7xvr2IY64vIr41l7480Y4 vEI4kI2Ix0rVAqx4xJMxkF7I0En4kS14v26r1q6r43MxAIw28IcxkI7VAKI48JMxC20s02 6xCaFVCjc4AY6r1j6r4UMxCIbckI1I0E14v26r1q6r43MI8I3I0E5I8CrVAFwI0_JrI_Jr Wlx2IqxVCjr7xvwVAFwI0_JrI_JrWlx4CE17CEb7AF67AKxVWUtVW8ZwCIc40Y0x0EwIxG rwCI42IY6xIIjxv20xvE14v26r1I6r4UMIIF0xvE2Ix0cI8IcVCY1x0267AKxVWUJVW8Jw CI42IY6xAIw20EY4v20xvaj40_Jr0_JF4lIxAIcVC2z280aVAFwI0_Jr0_Gr1lIxAIcVC2 z280aVCY1x0267AKxVWUJVW8JbIYCTnIWIevJa73UjIFyTuYvjxUI0eHDUUUU 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 List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: 9cGTsa5LQFllsPzNZxWjWfMYx7686176AA= Content-Type: multipart/alternative; boundary="------------9MpuT2N4sxBOhqpdTuSh3lGw" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=ZwEhXhLV; dmarc=none; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io --------------9MpuT2N4sxBOhqpdTuSh3lGw Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Hi Mike and Liming, Ping for review, I'd love to know your opinion, as the last email saied,=20 your opinion can decide the direction, so please review the new patches=20 of MdePkg for this series, please... Thanks, Chao On 2023/11/27 11:27, Chao Li wrote: > > Hi Mike and Liming, > > You opinion is very important, it will decide the direction. I will=20 > send the V4 this week, so can you please review the new patch of=20 > MdePkg for this series? > > 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 serv= ice >>>>> 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=3D4584 >>>>> >>>>> 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/Pei= ServicesTablePointer.c >>>>> create mode 100644 MdePkg/Library/PeiServicesTablePointerLibReg/Pei= ServicesTablePointerLib.uni >>>>> create mode 100644 MdePkg/Library/PeiServicesTablePointerLibReg/Pei= ServicesTablePointerLibReg.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 rework= ed >>>> 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.RISCV6= 4]. >> >> 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 registe= r >>> 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 >=20 -=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 (#111834): https://edk2.groups.io/g/devel/message/111834 Mute This Topic: https://groups.io/mt/102644754/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- --------------9MpuT2N4sxBOhqpdTuSh3lGw Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

Hi Mike and Liming,

Ping for review, I'd love to know your opinion, as the last email saied, your opinion can decide the direction, so please review the new patches of MdePkg for this series, please...


=
Thanks,
Chao
On 2023/11/27 11:27, Chao Li wrote:

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?

On 2023/11/24 19:35, Laszlo Ersek wrote:
On 11/22/23 02:47, Chao Li w=
rote:

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 pla=
tform 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=3D4584

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 &l=
t;lersek@redhat.com>
Cc: Sunil V L <sunilvl@ventanamicro.com>
Signed-off-by: Chao Li &=
lt;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/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 PeiSe=
rvicesTablePointerLib 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 lib=
rary 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 serie=
s has added LoongArch instance of this
library, please check.

Yes, I'm aware. That's not t=
he 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 p=
atch 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 pub=
lic 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 goi=
ng
this way. I believe this library should be a public library for register
mechanism.

That's entirely fine, as lon=
g 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 / sh=
ould 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
=20
_._,_._,_
Groups.io Links:

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

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

_._,_._,_
--------------9MpuT2N4sxBOhqpdTuSh3lGw--