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 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/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-