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 BD4687803CE for ; Wed, 22 Nov 2023 01:47:23 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=Jqsqg33Sl6sjyvjQX7o46o+JE7fxn8a0JVFFpVMU07E=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:User-Agent:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1700617642; v=1; b=Os9eKVuCjLhfKGVHVAibHc8UbC+88hw0+e1STZAnhxmkezxgAkYhaRwr1SvqJAgsEr2+BLRm PLQUakfxdj0lUt9P8Df9NqJGSHIZosWpv5Sphit+Xk315PXHTYqjmzisQbTwu7WUrL7g0XDSeAm sdUAZlLMq2+LttymD7oamAVo= X-Received: by 127.0.0.2 with SMTP id rwxWYY7687511xKLrGxbGvXC; Tue, 21 Nov 2023 17:47:22 -0800 X-Received: from mail.loongson.cn (mail.loongson.cn [114.242.206.163]) by mx.groups.io with SMTP id smtpd.web10.9960.1700617640731794339 for ; Tue, 21 Nov 2023 17:47:21 -0800 X-Received: from loongson.cn (unknown [10.40.24.149]) by gateway (Coremail) with SMTP id _____8BxJvGjXV1lIss7AA--.53022S3; Wed, 22 Nov 2023 09:47:15 +0800 (CST) X-Received: from [10.40.24.149] (unknown [10.40.24.149]) by localhost.localdomain (Coremail) with SMTP id AQAAf8AxG9ygXV1lBwxJAA--.29738S3; Wed, 22 Nov 2023 09:47:13 +0800 (CST) Message-ID: <3975af54-465a-4549-9273-8dd1228f8b44@loongson.cn> Date: Wed, 22 Nov 2023 09:47:12 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [PATCH v3 09/39] MdePkg: Add a new library named PeiServicesTablePointerLibReg To: Laszlo Ersek , devel@edk2.groups.io Cc: Michael D Kinney , Liming Gao , 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> From: "Chao Li" In-Reply-To: <03178ec2-b5e1-0ca7-3bd5-afe4e6d50554@redhat.com> X-CM-TRANSID: AQAAf8AxG9ygXV1lBwxJAA--.29738S3 X-CM-SenderInfo: xolfxt3r6o00pqjv00gofq/1tbiAQADCGVcE6oKNAACs2 X-Coremail-Antispam: 1Uk129KBj93XoW3Jw4rWr1kJF4DGrW3Jr18JFc_yoW7Xr4kpF 4UKr45tr1DJryftryava1UWrW5uFs7CFy3CFn7GF1DCwn8ZrykXr4IgrWFkF1rZFn5Cw1I vrW2yw1Uua4DZFXCm3ZEXasCq-sJn29KB7ZKAUJUUUUU529EdanIXcx71UUUUU7KY7ZEXa sCq-sGcSsGvfJ3UbIjqfuFe4nvWSU5nxnvy29KBjDU0xBIdaVrnRJUUUymb4IE77IF4wAF F20E14v26r1j6r4UM7CY07I20VC2zVCF04k26cxKx2IYs7xG6rWj6s0DM7CIcVAFz4kK6r 106r15M28lY4IEw2IIxxk0rwA2F7IY1VAKz4vEj48ve4kI8wA2z4x0Y4vE2Ix0cI8IcVAF wI0_Jr0_JF4l84ACjcxK6xIIjxv20xvEc7CjxVAFwI0_Jr0_Gr1l84ACjcxK6I8E87Iv67 AKxVW8JVWxJwA2z4x0Y4vEx4A2jsIEc7CjxVAFwI0_Gr0_Gr1UM2AIxVAIcxkEcVAq07x2 0xvEncxIr21l57IF6xkI12xvs2x26I8E6xACxx1lYx0E2Ix0cI8IcVAFwI0_Jr0_Jr4lYx 0Ex4A2jsIE14v26r1j6r4UMcvjeVCFs4IE7xkEbVWUJVW8JwACjcxG0xvEwIxGrwCjr7xv wVCIw2I0I7xG6c02F41l42xK82IYc2Ij64vIr41l4I8I3I0E4IkC6x0Yz7v_Jr0_Gr1lx2 IqxVAqx4xG67AKxVWUGVWUWwC20s026x8GjcxK67AKxVWUGVWUWwC2zVAF1VAY17CE14v2 6r1q6r43MIIYrxkI7VAKI48JMIIF0xvE2Ix0cI8IcVAFwI0_Jr0_JF4lIxAIcVC0I7IYx2 IY6xkF7I0E14v26r1j6r4UMIIF0xvE42xK8VAvwI8IcIk0rVWUJVWUCwCI42IY6I8E87Iv 67AKxVWUJVW8JwCI42IY6I8E87Iv6xkF7I0E14v26r1j6r4UYxBIdaVFxhVjvjDU0xZFpf 9x07UEFAJUUUUU= 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,lichao@loongson.cn List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: vIEzQvfR8Jk5tzdTjhxqcNHwx7686176AA= Content-Type: multipart/alternative; boundary="------------UgVNwgLBA5Mzp8JnBDO3fOCL" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=Os9eKVuC; 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 --------------UgVNwgLBA5Mzp8JnBDO3fOCL Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable 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=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/PeiSer= vicesTablePointer.c >> create mode 100644 MdePkg/Library/PeiServicesTablePointerLibReg/PeiSer= vicesTablePointerLib.uni >> create mode 100644 MdePkg/Library/PeiServicesTablePointerLibReg/PeiSer= vicesTablePointerLibReg.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=20 the PI SPEC, all ARCH except IA32 and X64 using the register mechanism,=20 if this library can be approved, all of them can moved into this=20 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=20 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,=20 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=20 this way. I believe this library should be a public library for register=20 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 -=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 (#111592): https://edk2.groups.io/g/devel/message/111592 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- --------------UgVNwgLBA5Mzp8JnBDO3fOCL Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

Hi Laszlo,


=
Thanks,
Chao
On 2023/11/21 22:37, Laszlo Ersek wrote:
On 11/17/23 10:59, Chao Li wro=
te:

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=
=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 <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/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.

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:

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

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

_._,_._,_
--------------UgVNwgLBA5Mzp8JnBDO3fOCL--