Re: 回复: [edk2-devel] [PATCH v3 09/39] MdePkg: Add a new library named PeiServicesTablePointerLibReg
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 <mailto:michael.d.kin...@intel.com> Cc: Liming Gao <mailto:gaolim...@byosoft.com.cn> Cc: Zhiguang Liu <mailto:zhiguang@intel.com> Cc: Leif Lindholm <mailto:quic_llind...@quicinc.com> Cc: Ard Biesheuvel <mailto:ardb+tianoc...@kernel.org> Cc: Sami Mujawar <mailto:sami.muja...@arm.com> Cc: Laszlo Ersek <mailto:ler...@redhat.com> Cc: Sunil V L <mailto:suni...@ventanamicro.com> Signed-off-by: Chao Li <mailto:lic...@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 ma
回复: [edk2-devel] [PATCH v3 09/39] MdePkg: Add a new library named PeiServicesTablePointerLibReg
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 <mailto:michael.d.kin...@intel.com> Cc: Liming Gao <mailto:gaolim...@byosoft.com.cn> Cc: Zhiguang Liu <mailto:zhiguang@intel.com> Cc: Leif Lindholm <mailto:quic_llind...@quicinc.com> Cc: Ard Biesheuvel <mailto:ardb+tianoc...@kernel.org> Cc: Sami Mujawar <mailto:sami.muja...@arm.com> Cc: Laszlo Ersek <mailto:ler...@redhat.com> Cc: Sunil V L <mailto:suni...@ventanamicro.com> Signed-off-by: Chao Li <mailto:lic...@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
Re: [edk2-devel] [PATCH v3 09/39] MdePkg: Add a new library named PeiServicesTablePointerLibReg
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 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
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 /
Re: [edk2-devel] [PATCH v3 09/39] MdePkg: Add a new library named PeiServicesTablePointerLibReg
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 /
Re: [edk2-devel] [PATCH v3 09/39] MdePkg: Add a new library named PeiServicesTablePointerLibReg
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. 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: 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/21656 Group
Re: [edk2-devel] [PATCH v3 09/39] MdePkg: Add a new library named PeiServicesTablePointerLibReg
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. 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. 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. (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 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]. (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 (#111550): https://edk2.groups.io/g/devel/message/111550 Mute This Topic: https://groups.io/mt/102644754/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v3 09/39] MdePkg: Add a new library named PeiServicesTablePointerLibReg
Hi Leif, There are indeed typos and I will fix them in V4. Thank you! Thanks, Chao On 2023/11/17 19:35, Leif Lindholm wrote: Not my package, just spotted a typo below: On Fri, Nov 17, 2023 at 17:59:49 +0800, 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 diff --git a/MdePkg/Include/Library/PeiServicesTablePointerLib.h b/MdePkg/Include/Library/PeiServicesTablePointerLib.h index 61635eff00..f5c764cb13 100644 --- a/MdePkg/Include/Library/PeiServicesTablePointerLib.h +++ b/MdePkg/Include/Library/PeiServicesTablePointerLib.h @@ -52,10 +52,11 @@ SetPeiServicesTablePointer ( immediately preceding the Interrupt Descriptor Table (IDT) in memory. For X64 CPUs, the PEI Services Table pointer is stored in the 8 bytes immediately preceding the Interrupt Descriptor Table (IDT) in memory. - For Itanium and ARM CPUs, a the PEI Services Table Pointer is stored in - a dedicated CPU register. This means that there is no memory storage - associated with storing the PEI Services Table pointer, so no additional - migration actions are required for Itanium or ARM CPUs. + For Itanium, ARM and LoongArch CPUs, a the PEI Services Table Pointer + is stored in a dedicated CPU register. This means that there is no + memory storage associated with storing the PEI Services Table pointer, + so no additional migration actions are required for Itanium, ARM and + LoongArch CPUs. **/ VOID @@ -64,4 +65,32 @@ MigratePeiServicesTablePointer ( VOID ); +/** + Retrieves the cached value of the PEI Services Table pointer from a CPU register. + + Returns the cached value of the PEI Services Table pointer in a CPU specific manner + as specified in the CPU binding section of the Platform Initialization Pre-EFI + Initialization Core Interface Specification. + + @return The pointer to PeiServices. +**/ +CONST EFI_PEI_SERVICES ** +EFIAPI +GetPeiServicesTablePointerFromRegister ( + VOID + ); + +/** + Set the pointer PEI Service Table to a CPU register. + + Caches the pointer to the PEI Services Table specified by PeiServicesTablePointer + in a platform specific manner. + + @paramPeiServicesTablePointer The address of PeiServices. +**/ +VOID +EFIAPI +SetPeiServicesTablePointerToRegester ( SetPeiServicesTablePointerToRegester -> SetPeiServicesTablePointerToRegister Regester -> Register. / Leif + IN UINTN PeiServicesTablePointer + ); #endif diff --git a/MdePkg/Library/PeiServicesTablePointerLibReg/PeiServicesTablePointer.c b/MdePkg/Library/PeiServicesTablePointerLibReg/PeiServicesTablePointer.c new file mode 100644 index 00..0227f98871 --- /dev/null +++ b/MdePkg/Library/PeiServicesTablePointerLibReg/PeiServicesTablePointer.c @@ -0,0 +1,86 @@ +/** @file + PEI Services Table Pointer Library For Reigseter Mechanism. + + This library is used for PEIM which does executed from flash device directly but + executed in memory. + + Copyright (c) 2006 - 2010, Intel Corporation. All rights reserved. + Copyright (c) 2011 Hewlett-Packard Corporation. All rights reserved. + Copyright (c) 2023 Loongson Technology Corporation Limited. All rights reserved. + + SPDX-License-Identifier: BSD-2-Clause-Patent + +**/ + +#include +#include +#include + +/** + Caches a pointer PEI Services Table. + + Caches the pointer to the PEI Services Table specified by PeiServicesTablePointer + in a platform specific manner. + + If PeiServicesTablePointer is NULL, then ASSERT(). + + @param
Re: [edk2-devel] [PATCH v3 09/39] MdePkg: Add a new library named PeiServicesTablePointerLibReg
Not my package, just spotted a typo below: On Fri, Nov 17, 2023 at 17:59:49 +0800, 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 > > diff --git a/MdePkg/Include/Library/PeiServicesTablePointerLib.h > b/MdePkg/Include/Library/PeiServicesTablePointerLib.h > index 61635eff00..f5c764cb13 100644 > --- a/MdePkg/Include/Library/PeiServicesTablePointerLib.h > +++ b/MdePkg/Include/Library/PeiServicesTablePointerLib.h > @@ -52,10 +52,11 @@ SetPeiServicesTablePointer ( >immediately preceding the Interrupt Descriptor Table (IDT) in memory. >For X64 CPUs, the PEI Services Table pointer is stored in the 8 bytes >immediately preceding the Interrupt Descriptor Table (IDT) in memory. > - For Itanium and ARM CPUs, a the PEI Services Table Pointer is stored in > - a dedicated CPU register. This means that there is no memory storage > - associated with storing the PEI Services Table pointer, so no additional > - migration actions are required for Itanium or ARM CPUs. > + For Itanium, ARM and LoongArch CPUs, a the PEI Services Table Pointer > + is stored in a dedicated CPU register. This means that there is no > + memory storage associated with storing the PEI Services Table pointer, > + so no additional migration actions are required for Itanium, ARM and > + LoongArch CPUs. > > **/ > VOID > @@ -64,4 +65,32 @@ MigratePeiServicesTablePointer ( >VOID >); > > +/** > + Retrieves the cached value of the PEI Services Table pointer from a CPU > register. > + > + Returns the cached value of the PEI Services Table pointer in a CPU > specific manner > + as specified in the CPU binding section of the Platform Initialization > Pre-EFI > + Initialization Core Interface Specification. > + > + @return The pointer to PeiServices. > +**/ > +CONST EFI_PEI_SERVICES ** > +EFIAPI > +GetPeiServicesTablePointerFromRegister ( > + VOID > + ); > + > +/** > + Set the pointer PEI Service Table to a CPU register. > + > + Caches the pointer to the PEI Services Table specified by > PeiServicesTablePointer > + in a platform specific manner. > + > + @paramPeiServicesTablePointer The address of PeiServices. > +**/ > +VOID > +EFIAPI > +SetPeiServicesTablePointerToRegester ( SetPeiServicesTablePointerToRegester -> SetPeiServicesTablePointerToRegister Regester -> Register. / Leif > + IN UINTN PeiServicesTablePointer > + ); > #endif > diff --git > a/MdePkg/Library/PeiServicesTablePointerLibReg/PeiServicesTablePointer.c > b/MdePkg/Library/PeiServicesTablePointerLibReg/PeiServicesTablePointer.c > new file mode 100644 > index 00..0227f98871 > --- /dev/null > +++ b/MdePkg/Library/PeiServicesTablePointerLibReg/PeiServicesTablePointer.c > @@ -0,0 +1,86 @@ > +/** @file > + PEI Services Table Pointer Library For Reigseter Mechanism. > + > + This library is used for PEIM which does executed from flash device > directly but > + executed in memory. > + > + Copyright (c) 2006 - 2010, Intel Corporation. All rights reserved. > + Copyright (c) 2011 Hewlett-Packard Corporation. All rights reserved. > + Copyright (c) 2023 Loongson Technology Corporation Limited. All rights > reserved. > + > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#include > +#include > +#include > + > +/** > + Caches a pointer PEI Services Table. > + > + Caches the pointer to the PEI Services Table specified by > PeiServicesTablePointer >
[edk2-devel] [PATCH v3 09/39] MdePkg: Add a new library named PeiServicesTablePointerLibReg
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
diff --git a/MdePkg/Include/Library/PeiServicesTablePointerLib.h
b/MdePkg/Include/Library/PeiServicesTablePointerLib.h
index 61635eff00..f5c764cb13 100644
--- a/MdePkg/Include/Library/PeiServicesTablePointerLib.h
+++ b/MdePkg/Include/Library/PeiServicesTablePointerLib.h
@@ -52,10 +52,11 @@ SetPeiServicesTablePointer (
immediately preceding the Interrupt Descriptor Table (IDT) in memory.
For X64 CPUs, the PEI Services Table pointer is stored in the 8 bytes
immediately preceding the Interrupt Descriptor Table (IDT) in memory.
- For Itanium and ARM CPUs, a the PEI Services Table Pointer is stored in
- a dedicated CPU register. This means that there is no memory storage
- associated with storing the PEI Services Table pointer, so no additional
- migration actions are required for Itanium or ARM CPUs.
+ For Itanium, ARM and LoongArch CPUs, a the PEI Services Table Pointer
+ is stored in a dedicated CPU register. This means that there is no
+ memory storage associated with storing the PEI Services Table pointer,
+ so no additional migration actions are required for Itanium, ARM and
+ LoongArch CPUs.
**/
VOID
@@ -64,4 +65,32 @@ MigratePeiServicesTablePointer (
VOID
);
+/**
+ Retrieves the cached value of the PEI Services Table pointer from a CPU
register.
+
+ Returns the cached value of the PEI Services Table pointer in a CPU specific
manner
+ as specified in the CPU binding section of the Platform Initialization
Pre-EFI
+ Initialization Core Interface Specification.
+
+ @return The pointer to PeiServices.
+**/
+CONST EFI_PEI_SERVICES **
+EFIAPI
+GetPeiServicesTablePointerFromRegister (
+ VOID
+ );
+
+/**
+ Set the pointer PEI Service Table to a CPU register.
+
+ Caches the pointer to the PEI Services Table specified by
PeiServicesTablePointer
+ in a platform specific manner.
+
+ @paramPeiServicesTablePointer The address of PeiServices.
+**/
+VOID
+EFIAPI
+SetPeiServicesTablePointerToRegester (
+ IN UINTN PeiServicesTablePointer
+ );
#endif
diff --git
a/MdePkg/Library/PeiServicesTablePointerLibReg/PeiServicesTablePointer.c
b/MdePkg/Library/PeiServicesTablePointerLibReg/PeiServicesTablePointer.c
new file mode 100644
index 00..0227f98871
--- /dev/null
+++ b/MdePkg/Library/PeiServicesTablePointerLibReg/PeiServicesTablePointer.c
@@ -0,0 +1,86 @@
+/** @file
+ PEI Services Table Pointer Library For Reigseter Mechanism.
+
+ This library is used for PEIM which does executed from flash device directly
but
+ executed in memory.
+
+ Copyright (c) 2006 - 2010, Intel Corporation. All rights reserved.
+ Copyright (c) 2011 Hewlett-Packard Corporation. All rights reserved.
+ Copyright (c) 2023 Loongson Technology Corporation Limited. All rights
reserved.
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include
+#include
+#include
+
+/**
+ Caches a pointer PEI Services Table.
+
+ Caches the pointer to the PEI Services Table specified by
PeiServicesTablePointer
+ in a platform specific manner.
+
+ If PeiServicesTablePointer is NULL, then ASSERT().
+
+ @paramPeiServicesTablePointer The address of PeiServices pointer.
+**/
+VOID
+EFIAPI
+SetPeiServicesTablePointer (
+ IN CONST EFI_PEI_SERVICES **PeiServicesTablePointer
+ )
+{
+ ASSERT (PeiServicesTablePointer != NULL);
+ SetPeiServicesTablePointerToRegester ((UINTN)PeiServicesTablePointer);
+}
+
+/**
+ Retrieves the cached value of the PEI Services Table pointer.
+
