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 == 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:

You receive all messages sent to this group.

_._,_._,_