Re: [edk2-devel] [PATCH v8 14/37] UefiCpuPkg: Add CpuMmuLib to UefiCpuPkg

["Laszlo Ersek" <lersek@redhat.com>]

On 3/5/24 12:50, Chao Li wrote:
> Hi Laszlo,
> 
> OK, I see, let's me try. And I have another question:
> 
> Where should the low-level library be placed? Under the
> UefiCpuPkg/Library and as the same folder as CpuMmuLib?

In my opinion:

Under UefiCpuPkg/Library -- yes.

In the same folder as CpuMmuLib -- no. Instances of different library
classes are never placed in the same folder.

Laszlo

> 
> 
> Thanks,
> Chao
> On 2024/3/5 17:26, Laszlo Ersek wrote:
>> Hello Chao,
>>
>> On 3/4/24 04:39, Chao Li wrote:
>>> Hi Laszlo,
>>>
>>> OK.
>>>
>>> When I discussed the CpuMmuLib API as a public API with Ray in the early
>>> days, the API recommended by Ray should be the patch 13 in this series,
>>> which only contains set/get memory region attribute, but in the first
>>> version in this series, it contains more API, one is ConfigureMmu API,
>>> which is included in ARM and RISCV versions. This API provides a
>>> interface called in PEI or DXE stage, it will configures the MMU and
>>> enables it, such as creating the page tables, filling the static page
>>> tables, cofniguring the MMU registers, enbale MMU, etc.
>>>
>>> The paths for ARM and RISCV version APIs:
>>>
>>> ARM: ArmPkg/Include/Library/ArmMmuLib.h
>>>
>>> RISCV: UefiCpuPkg/Include/Library/BaseRiscVMmuLib.h
>> I have now re-read this subthread in its entirey, and I think I
>> understand what's up.
>>
>> What confused me recently was your expression "so should we open
>> configure API?"
>>
>> I think what you meant by "opening" was "publicly declaring".
>>
>> Anyway, here's my recommendation (consistently with what I said earlier
>> in this thread):
>>
>> - I think you need a new (lower-level) library class and instance for
>> exposing ConfigureMemoryManagementUnit().
>>
>> Please see: <https://edk2.groups.io/g/devel/message/114972> (msgid
>> <2a91f2f0-df4c-e106-65cd-79be167224f2@redhat.com>).
>>
>> - I do not recommend trying to unify the new LoongArch library classes
>> (low level and high level) with "ArmMmuLib.h" or "BaseRiscVMmuLib.h".
>>
>> Each of "ArmMmuLib.h" and "BaseRiscVMmuLib.h" seems to declare both
>> low-level and high-level interfaces. For LoongArch, we apparently don't
>> want that; hence the idea to introduce two library classes for LoongArch
>> -- that may be considered an improved design.
>>
>> At the same time, I don't believe that this requires us to unify
>> "ArmMmuLib.h" and "BaseRiscVMmuLib.h" with the new, LoongArch-motivated
>> library class design.
>>
>> Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116383): https://edk2.groups.io/g/devel/message/116383
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]
-=-=-=-=-=-=-=-=-=-=-=-