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

["Chao Li" <lichao@loongson.cn>]

[-- Attachment #1: Type: text/plain, Size: 8061 bytes --]

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<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/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 (#111712): https://edk2.groups.io/g/devel/message/111712
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]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 11323 bytes --]