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 12E31941553 for ; Fri, 24 Nov 2023 11:35:23 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=e4q0Cg7W9x6Nb6gFQpL7egQ5aDIrnKZsLyOnJXuu9OI=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version: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-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1700825722; v=1; b=vM0ab1uR17MF+cbj6BpV5dLyf9NkkDRvEal4uZLfJ02Q7xH6G6lNCYZK2e6WnEzh6CqOBVOR ew2dsi55qt1OBOoFkwybBGjYzariQ8aVT7NgT+HROywDN/LgeBk7dE+jieOvhS6LcAocBkSxSGN 7dLIxw238YU96VTUCgtiavZw= X-Received: by 127.0.0.2 with SMTP id JtSRYY7687511xmfA0aE2zln; Fri, 24 Nov 2023 03:35:22 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.133677.1700825721981021096 for ; Fri, 24 Nov 2023 03:35:22 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-158-DQ3EWeIcOL6gZs6lWQ2O4A-1; Fri, 24 Nov 2023 06:35:18 -0500 X-MC-Unique: DQ3EWeIcOL6gZs6lWQ2O4A-1 X-Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 68E32101AA44; Fri, 24 Nov 2023 11:35:17 +0000 (UTC) X-Received: from [10.39.192.172] (unknown [10.39.192.172]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 16BD1C1596F; Fri, 24 Nov 2023 11:35:14 +0000 (UTC) Message-ID: Date: Fri, 24 Nov 2023 12:35:13 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v3 09/39] MdePkg: Add a new library named PeiServicesTablePointerLibReg To: Chao Li , 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> <3975af54-465a-4549-9273-8dd1228f8b44@loongson.cn> From: "Laszlo Ersek" In-Reply-To: <3975af54-465a-4549-9273-8dd1228f8b44@loongson.cn> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.8 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com 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,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: RXpztG2cz3ORH4aMhdjpTo6Zx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=vM0ab1uR; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=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 On 11/22/23 02:47, Chao Li wrote: > Hi Laszlo, >=20 >=20 > 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 servic= e >>> table pointer depend by a CPU register, this library can accommodate lo= t >>> 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. >=20 > 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. >=20 >> 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 -=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 (#111700): https://edk2.groups.io/g/devel/message/111700 Mute This Topic: https://groups.io/mt/102644754/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-