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 A146B9418BA for ; Tue, 21 Nov 2023 14:37:41 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=T4ssDv+uZFbBess8MghS4x1xlOES3Ya2/W9nfYiB2QU=; 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=1700577460; v=1; b=eBGqS/y0aPIKGq5q+5HkaRpTUH2kuhwcwiVZ5NvLSesm6C7YJ113Pv+YaThKLlA4cP1lQFf5 njcRyXLsFfjeoaaP0DGFwk+MTMIsmGtgLlljmjYlFYaf+2mYZHiu5GeiXxWDyfx8LNY0acKPKcn 8O2f0gkS3bPHMZgvEbWLaJHs= X-Received: by 127.0.0.2 with SMTP id 8TcxYY7687511xatsDUSXO5G; Tue, 21 Nov 2023 06:37:40 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web10.42152.1700577459716697665 for ; Tue, 21 Nov 2023 06:37:39 -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-453-Gncw2miCM6ilme0aj5QztQ-1; Tue, 21 Nov 2023 09:37:33 -0500 X-MC-Unique: Gncw2miCM6ilme0aj5QztQ-1 X-Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (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 D1C4D8314FF; Tue, 21 Nov 2023 14:37:32 +0000 (UTC) X-Received: from [10.39.194.228] (unknown [10.39.194.228]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 3F1B62166B26; Tue, 21 Nov 2023 14:37:30 +0000 (UTC) Message-ID: <03178ec2-b5e1-0ca7-3bd5-afe4e6d50554@redhat.com> Date: Tue, 21 Nov 2023 15:37:28 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v3 09/39] MdePkg: Add a new library named PeiServicesTablePointerLibReg To: devel@edk2.groups.io, lichao@loongson.cn 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> From: "Laszlo Ersek" In-Reply-To: <20231117095949.3608941-1-lichao@loongson.cn> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.6 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: JkVL1qDPZrJBZWI3AwA0tpYSx7686176AA= 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="eBGqS/y0"; 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/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. >=20 > 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. >=20 > 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. >=20 > The idea of this library is derived from > ArmPkg/Library/PeiServicesTablePointerLib/ >=20 > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4584 >=20 > 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/PeiServi= cesTablePointer.c > create mode 100644 MdePkg/Library/PeiServicesTablePointerLibReg/PeiServi= cesTablePointerLib.uni > create mode 100644 MdePkg/Library/PeiServicesTablePointerLibReg/PeiServi= cesTablePointerLibReg.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 -=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 (#111550): https://edk2.groups.io/g/devel/message/111550 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-