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 4930ED811A0 for ; Fri, 2 Feb 2024 03:30:50 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=VtLZvnwreolFamRfXN6anjHU+1vCQ38IpHGKGh3f5qs=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:User-Agent: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-Type; s=20140610; t=1706844648; v=1; b=c70HYAWR1RmfBheLZJINog+dGH/WsM/iEPFFacJ+bvcidmVenGz3qSBa23Qo6vtvPRm4j4mH ttjAhuEkioobjJS2x1H1zH7fPNwtrEIk7Lwjmb/OO5RmqiYKTBgYBzo/GqRC4XbN99/um9FCueF m08lnyuxiCq1OK0+hmaOJjdA= X-Received: by 127.0.0.2 with SMTP id WbUwYY7687511xnDGgtKdJXc; Thu, 01 Feb 2024 19:30:48 -0800 X-Received: from mail.loongson.cn (mail.loongson.cn [114.242.206.163]) by mx.groups.io with SMTP id smtpd.web10.15346.1706844647435335410 for ; Thu, 01 Feb 2024 19:30:48 -0800 X-Received: from loongson.cn (unknown [10.40.24.149]) by gateway (Coremail) with SMTP id _____8AxOOjkYbxl5fsJAA--.9299S3; Fri, 02 Feb 2024 11:30:44 +0800 (CST) X-Received: from [10.40.24.149] (unknown [10.40.24.149]) by localhost.localdomain (Coremail) with SMTP id AQAAf8CxrhPiYbxlbdQsAA--.50259S3; Fri, 02 Feb 2024 11:30:42 +0800 (CST) Message-ID: <24c71c6a-6f3d-4cfc-bcc7-65e1683cb3a9@loongson.cn> Date: Fri, 2 Feb 2024 11:30:42 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [PATCH v8 14/37] UefiCpuPkg: Add CpuMmuLib to UefiCpuPkg To: Laszlo Ersek , devel@edk2.groups.io Cc: Eric Dong , Ray Ni , Rahul Kumar , Gerd Hoffmann , Baoqi Zhang , Dongyan Qian , Xianglai Li , Bibo Mao References: <20240126062715.3099433-1-lichao@loongson.cn> <20240126062919.3101691-1-lichao@loongson.cn> <856ebd67-d345-6e41-2e34-74b9bdd7ed7e@redhat.com> <2a91f2f0-df4c-e106-65cd-79be167224f2@redhat.com> From: "Chao Li" In-Reply-To: <2a91f2f0-df4c-e106-65cd-79be167224f2@redhat.com> X-CM-TRANSID: AQAAf8CxrhPiYbxlbdQsAA--.50259S3 X-CM-SenderInfo: xolfxt3r6o00pqjv00gofq/1tbiAQAPCGW7VmEOHgALs0 X-Coremail-Antispam: 1Uk129KBj93XoWxWFWfAF1DWr17ZFyUXFyUtwc_yoW5AF4UpF W3K3yjkw1kA34fXrs3Zw48Zr1S9ws5CFW3CrW5C34DCwn8GrnF9r40y3yYg3W7Cr4kJ3WY vrWjg3WUZas0g3gCm3ZEXasCq-sJn29KB7ZKAUJUUUUU529EdanIXcx71UUUUU7KY7ZEXa sCq-sGcSsGvfJ3UbIjqfuFe4nvWSU5nxnvy29KBjDU0xBIdaVrnRJUUUyCb4IE77IF4wAF F20E14v26r1j6r4UM7CY07I20VC2zVCF04k26cxKx2IYs7xG6rWj6s0DM7CIcVAFz4kK6r 106r15M28lY4IEw2IIxxk0rwA2F7IY1VAKz4vEj48ve4kI8wA2z4x0Y4vE2Ix0cI8IcVAF wI0_Jr0_JF4l84ACjcxK6xIIjxv20xvEc7CjxVAFwI0_Jr0_Gr1l84ACjcxK6I8E87Iv67 AKxVWxJVW8Jr1l84ACjcxK6I8E87Iv6xkF7I0E14v26r4UJVWxJr1le2I262IYc4CY6c8I j28IcVAaY2xG8wAqjxCEc2xF0cIa020Ex4CE44I27wAv7VC0I7IYx2IY67AKxVWUJVWUGw Av7VC2z280aVAFwI0_Gr0_Cr1lOx8S6xCaFVCjc4AY6r1j6r4UM4x0Y48IcVAKI48JMx8G jcxK6IxK0xIIj40E5I8CrwCF04k20xvY0x0EwIxGrwCFx2IqxVCFs4IE7xkEbVWUJVW8Jw C20s026c02F40E14v26r106r1rMI8I3I0E7480Y4vE14v26r106r1rMI8E67AF67kF1VAF wI0_Jw0_GFylIxkGc2Ij64vIr41lIxAIcVC0I7IYx2IY67AKxVWUJVWUCwCI42IY6xIIjx v20xvEc7CjxVAFwI0_Jr0_Gr1lIxAIcVCF04k26cxKx2IYs7xG6r1j6r1xMIIF0xvEx4A2 jsIE14v26r4j6F4UMIIF0xvEx4A2jsIEc7CjxVAFwI0_Gr0_Gr1UYxBIdaVFxhVjvjDU0x ZFpf9x07j873kUUUUU= 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,lichao@loongson.cn List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: pjzftU1O7Ofdbka0QQ3kyAN3x7686176AA= Content-Type: multipart/alternative; boundary="------------cuYD00A5QeTnskjpEmR8Qmid" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=c70HYAWR; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=none --------------cuYD00A5QeTnskjpEmR8Qmid Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Hi Laszlo, Thanks, Chao On 2024/2/2 06:46, Laszlo Ersek wrote: > On 2/1/24 08:57, Chao Li wrote: > >> Let's me tell you this library how to work: >> >> In PEI stage, in addition to ensuring that the MMU is not used, the user >> must call the ConfigureMemoryManagementUnit toinitialization the MMU, >> such as filling the static page tables, set the TLB refill exception >> entry point, set the page size etc. the ConfigureMemoryManagementUnit is >> a private API but related to ARCH. >> >> During DXE stage, this library will provide services for CpuDxe and >> other drivers, but almost changes to memory page attributes use the >> protocols provided by CpuDxe. That is why the CpuMmuLib.h only contains >> two APIs, it only can get/set the attribute. >> >> In short, the PEI stage needs to initialize and set up TLB refill entry >> point and method(dynamically populate the TLB, keep the PA =3D=3D VA), a= nd >> DXE stage is provides services for changing the memory attributes. > My understanding is the following then: > > (1) You should provide a very low level library, implementing and also > publicly exposing primitives for manipulating page tables. This library > could / should expose ConfigureMemoryManagementUnit(). This library > should not use global variables at all, I think -- the user of the > library would be responsible for tracking state. API names in this > library should have a good, distinctive prefix. > > (2) Introduce a higher level lib class that contains very few APIs -- > APIs that are possible to implement by multiple arches. > > (3) Provide a library instance of class (2) by way of consuming (1). > > (4) In your platform PEIM, for setting up paging, use (1) directly, or > (if you can do it) only (2)+(3). > > (5) In DXE phase code, use (2)+(3). > > Basically I both agree and disagree with Ray's review for your > > [PATCH v3 13/39] UefiCpuPkg: Add CpuMmuLib.h to UefiCpuPkg > > I think that Ray is right in that we want good high-level paging APIs > that can be implemented by multiple arches. On the other hand, > architectural differences do exist, they are idiosyncratic, they need to > exist *somewhere*, and that idiosyncratic logic may still be necessary > to use from *multiple* places -- so that indicates that we need a > library (low-level) that is not "well designed" but actually a "grab bag > of utilities" that just does the work. > > To satisfy both requirements, insert a new layer of indirection :) > > I'm not a big fan of top-down library design. I rather like to look at > existent usage patterns, and then factoring out common stuff to a > library. At that point, unification / cleanup possibilities may emerge. > But getting the API design right immediately -- that requires seeing the > future, or *extreme* amounts of experience. So an "ugly" interface > library may be unavoidable. But, we can always wrap it into nicer > interfaces, for those clients that don't really need the low-level > primitives. When I redesign this module, I will try to add a new layer, what you=20 call a low-level library. > > 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 (#115008): https://edk2.groups.io/g/devel/message/115008 Mute This Topic: https://groups.io/mt/103971653/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- --------------cuYD00A5QeTnskjpEmR8Qmid Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

Hi Laszlo,


=
Thanks,
Chao
On 2024/2/2 06:46, Laszlo Ersek wrote: 
On 2/1/24 08:57, Chao Li wrote=
:


Let's me tell you this libra=
ry how to work:

In PEI stage, in addition to ensuring that the MMU is not used, the user
must call the ConfigureMemoryManagementUnit toinitialization the MMU,
such as filling the static page tables, set the TLB refill exception
entry point, set the page size etc. the ConfigureMemoryManagementUnit is
a private API but related to ARCH.

During DXE stage, this library will provide services for CpuDxe and
other drivers, but almost changes to memory page attributes use the
protocols provided by CpuDxe. That is why the CpuMmuLib.h only contains
two APIs, it only can get/set the attribute.

In short, the PEI stage needs to initialize and set up TLB refill entry
point and method(dynamically populate the TLB, keep the PA =3D=3D VA), and
DXE stage is provides services for changing the memory attributes.

My understanding is the following then:

(1) You should provide a very low level library, implementing and also
publicly exposing primitives for manipulating page tables. This library
could / should expose ConfigureMemoryManagementUnit(). This library
should not use global variables at all, I think -- the user of the
library would be responsible for tracking state. API names in this
library should have a good, distinctive prefix.

(2) Introduce a higher level lib class that contains very few APIs --
APIs that are possible to implement by multiple arches.

(3) Provide a library instance of class (2) by way of consuming (1).

(4) In your platform PEIM, for setting up paging, use (1) directly, or
(if you can do it) only (2)+(3).

(5) In DXE phase code, use (2)+(3).

Basically I both agree and disagree with Ray's review for your

  [PATCH v3 13/39] UefiCpuPkg: Add CpuMmuLib.h to UefiCpuPkg

I think that Ray is right in that we want good high-level paging APIs
that can be implemented by multiple arches. On the other hand,
architectural differences do exist, they are idiosyncratic, they need to
exist *somewhere*, and that idiosyncratic logic may still be necessary
to use from *multiple* places -- so that indicates that we need a
library (low-level) that is not "well designed" but actually a "grab bag
of utilities" that just does the work.

To satisfy both requirements, insert a new layer of indirection :)

I'm not a big fan of top-down library design. I rather like to look at
existent usage patterns, and then factoring out common stuff to a
library. At that point, unification / cleanup possibilities may emerge.
But getting the API design right immediately -- that requires seeing the
future, or *extreme* amounts of experience. So an "ugly" interface
library may be unavoidable. But, we can always wrap it into nicer
interfaces, for those clients that don't really need the low-level
primitives.
When I redesign this module, I will try to add a new layer, what you call a low-level library.

Laszlo
_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#115008) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--------------cuYD00A5QeTnskjpEmR8Qmid--