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

2023-12-01 Thread Chao Li 

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 
*发送时间:*2023年11月27日11:28
*收件人:*Laszlo Ersek ; devel@edk2.groups.io
*抄送:*Michael D Kinney ; Liming Gao 
; Zhiguang Liu ; 
Leif Lindholm ; Ard Biesheuvel 
; Sami Mujawar ; 
Sunil V L 
*主题:*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 


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

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

2023-11-30 Thread gaoliming via groups.io 
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  
发送时间: 2023年11月27日 11:28
收件人: Laszlo Ersek ; devel@edk2.groups.io
抄送: Michael D Kinney ; Liming Gao 
; Zhiguang Liu ; Leif 
Lindholm ; Ard Biesheuvel 
; Sami Mujawar ; Sunil V L 

主题: 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   

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