Thanks. I think we are aligned. Looking forward to your v4 patch.
Thanks,
Ray
*From:*devel@edk2.groups.io <devel@edk2.groups.io> *On Behalf Of *Chao Li
*Sent:* Friday, December 8, 2023 10:10 AM
*To:* Ni, Ray <ray...@intel.com>; devel@edk2.groups.io
*Cc:* Dong, Eric <eric.d...@intel.com>; Kumar, Rahul R
<rahul.r.ku...@intel.com>; Gerd Hoffmann <kra...@redhat.com>; Leif
Lindholm <quic_llind...@quicinc.com>; Ard Biesheuvel
<ardb+tianoc...@kernel.org>; Sami Mujawar <sami.muja...@arm.com>;
Sunil V L <suni...@ventanamicro.com>; Warkentin, Andrei
<andrei.warken...@intel.com>
*Subject:* Re: [edk2-devel] [PATCH v3 13/39] UefiCpuPkg: Add
CpuMmuLib.h to UefiCpuPkg
Hi Ray,
Do you think this plan is OK? If possible, I will submit the V4 today.
Thanks,
Chao
On 2023/12/5 20:27, Chao Li wrote:
Hi Ray,
On 2023/12/5 16:27, Ni, Ray wrote:
Thanks,
Ray
*From:*devel@edk2.groups.io <devel@edk2.groups.io>
<mailto:devel@edk2.groups.io> *On Behalf Of *Chao Li
*Sent:* Monday, December 4, 2023 3:32 PM
*To:* devel@edk2.groups.io; Ni, Ray <ray...@intel.com>
<mailto:ray...@intel.com>
*Cc:* Dong, Eric <eric.d...@intel.com>
<mailto:eric.d...@intel.com>; Kumar, Rahul R
<rahul.r.ku...@intel.com> <mailto:rahul.r.ku...@intel.com>;
Gerd Hoffmann <kra...@redhat.com> <mailto:kra...@redhat.com>;
Leif Lindholm <quic_llind...@quicinc.com>
<mailto:quic_llind...@quicinc.com>; Ard Biesheuvel
<ardb+tianoc...@kernel.org>
<mailto:ardb+tianoc...@kernel.org>; Sami Mujawar
<sami.muja...@arm.com> <mailto:sami.muja...@arm.com>; Sunil V
L <suni...@ventanamicro.com>
<mailto:suni...@ventanamicro.com>; Warkentin, Andrei
<andrei.warken...@intel.com> <mailto:andrei.warken...@intel.com>
*Subject:* Re: [edk2-devel] [PATCH v3 13/39] UefiCpuPkg: Add
CpuMmuLib.h to UefiCpuPkg
Hi Ray,
For this patch, I checked again and here are my opinions:
1. (Set|Get)MemoryRegionAttribute is difficult to merge
together, because the parameters between the tow APIs are
not similar. So I suggest they be independent.
[Ray] What I mean is to merge SetMemoryRegion(NonExec|ReadOly)
to SetMemoryRegionAttribute(). Similarly, GetXXX can be merged
as well. What’s your opinion?
Ok, I already said it in point 2, other APIs will be removed.
2. The EfiAttributeConverse, GetMemoryRegionAttribute,
SetMemoryRegionAttributes and
ConfigureMemoryManagementUnit will be retained and other
APIs will be removed. Because the functions expressed by
other APIs can be completed though the retained API.
[Ray] I didn’t notice EfiAttributeConverse(). I guess callers
may not need to know the architectural specific attributes. So
EfiAttributeConverse() might be not needed as a public API.
I agree, the EfiAttributeCoverse() complete by caller or as a
private API.
3. You pointed out MEMORY_REGION_DESCRIPTOR have no one to
construct it, do I need add a new API to construct it?
Could it be named GetMemoryMapPolicy and accept a
parameter with MEMORY_REGION_DESCRIPTOR** ?
[Ray] So the GetMemoryRegionAttribute() and
SetMemoryRegionAttributes() are performed on the active
translation table. ConfigureMemoryManagementUint() is to
create a translation table with a list of memory attributes.
How about the following idea?
[Ray] (Set|Get)MemoryRegionAttribute() are performed on a
translation table buffer. And caller calls
SetMemoryRegionAttribute() to modify the translation table
buffer supplied as the parameter. With this,
ConfigureMemoryManagementUnit() is not needed.
Ah, I think you may have some misunderstanding, the
ConfigureMemoryManagementUint is a function that to initialize the
MMU. The MEMORY_REGION_DESCRIPTOR will created by the private API,
and then the caller will call the ConfigureMemoryManagementUnit to
initialize the MMU first(may be fill the static page tables and so
on).
But I thought about it again and it seems you are right, the
ConfigureMemoryManagementUnit and discrptor creater as the public
APIs are not appropriate. They are more suitable as some private APIs.
So, (Get|Set)MemoryRegionAttribute() will be the public APIs and
the parameters will be the same with this change?
Hope to hear from you! :)
Thanks,
Chao
On 2023/11/30 10:25, Chao Li wrote:
Hi Ray,
Thanks for review, here are some of my thoughts:
On 2023/11/30 08:59, Ni, Ray wrote:
Chao,
Since the lib class is so general, I'd like to
understand more details to make sure it can properly
fit into any CPU arch.
In X86, cache setting is through MSRs and Page tables,
and memory access control (read-only, not-present,
non-executable) is through page tables.
Let me understand, 'cache setting' means does it access a
certain address(probably a memory address) via cache? If
so, I'd say the 'cache setting' should be a part of
attributes.
This CpuMmuLib is to provide both services. How does
LoongArch64 manage the cache settings and memory
access control?
Is it proper to combine both services into one lib?
In LoongArch64, cache settings and memory access control
are performed via page tables. Please check the patch 14
of this series.
If the backend silicon IP is the same one that
supports the "one" lib design, can we refine the lib
API a bit?
Yes, I think Attribute's instance family can be bear the
memory access and cache setting. So what are you
suggestions if we improve the lib API?
We have (Set|Get)MemoryRegionAttribute() and
(Set|Clear)MemoryRegion(NoExec|ReadOnly). Can we merge
them together?
Do you means the (Set|Get) merge together(differentiate
Get or Set operations by parameters)? If so, I think it's
OK, but maybe some existing instances will be modified
together.
And the API ConfigureMemoryManagementUint() accepts
MEMORY_REGION_DESCRIPTOR but none of other APIs helps
to construct the descriptor.
Yes, currently, no one helps construct
MEMORY_REGION_DESCRIPTOR. I think the construction of
descriptors is not part of the API, it should be the
localized or private when I design them. Do I need to add
an API to construct descripters?
It seems to me the MmuLib is simply a combination of
different random APIs.
It's not a well-designed library class.
We need more discussion to make it be able to be
accommodated by other archs in future, at least by
figuring out the path of X86, ARM.
Yes, the APIs looks like so fragmented and we should
improve them. So we should talk more about this API, thanks.
Thanks,
Ray
-----Original Message-----
From: Chao Li <lic...@loongson.cn>
<mailto:lic...@loongson.cn>
Sent: Friday, November 17, 2023 6:00 PM
To: devel@edk2.groups.io
Cc: Dong, Eric <eric.d...@intel.com>
<mailto:eric.d...@intel.com>; Ni, Ray
<ray...@intel.com> <mailto:ray...@intel.com>; Kumar,
Rahul R <rahul.r.ku...@intel.com>
<mailto:rahul.r.ku...@intel.com>; Gerd Hoffmann
<kra...@redhat.com> <mailto:kra...@redhat.com>;
Leif Lindholm <quic_llind...@quicinc.com>
<mailto:quic_llind...@quicinc.com>; Ard Biesheuvel
<ardb+tianoc...@kernel.org>
<mailto:ardb+tianoc...@kernel.org>; Sami Mujawar
<sami.muja...@arm.com> <mailto:sami.muja...@arm.com>;
Sunil V L <suni...@ventanamicro.com>
<mailto:suni...@ventanamicro.com>; Warkentin, Andrei
<andrei.warken...@intel.com>
<mailto:andrei.warken...@intel.com>
Subject: [PATCH v3 13/39] UefiCpuPkg: Add
CpuMmuLib.h to UefiCpuPkg
Add a new header file CpuMmuLib.h, whitch is
referenced from
ArmPkg/Include/Library/ArmMmuLib.h. Currently,
only support for
LoongArch64 is added, and more architectures can
be accommodated in the
future.
BZ:
https://bugzilla.tianocore.org/show_bug.cgi?id=4584
Cc: Eric Dong <eric.d...@intel.com>
<mailto:eric.d...@intel.com>
Cc: Ray Ni <ray...@intel.com>
<mailto:ray...@intel.com>
Cc: Rahul Kumar <rahul1.ku...@intel.com>
<mailto:rahul1.ku...@intel.com>
Cc: Gerd Hoffmann <kra...@redhat.com>
<mailto:kra...@redhat.com>
Cc: Leif Lindholm <quic_llind...@quicinc.com>
<mailto:quic_llind...@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>
<mailto:ardb+tianoc...@kernel.org>
Cc: Sami Mujawar <sami.muja...@arm.com>
<mailto:sami.muja...@arm.com>
Cc: Sunil V L <suni...@ventanamicro.com>
<mailto:suni...@ventanamicro.com>
Cc: Andrei Warkentin <andrei.warken...@intel.com>
<mailto:andrei.warken...@intel.com>
Signed-off-by: Chao Li <lic...@loongson.cn>
<mailto:lic...@loongson.cn>
---
UefiCpuPkg/Include/Library/CpuMmuLib.h | 155
+++++++++++++++++++++++++
UefiCpuPkg/UefiCpuPkg.dec | 4 +
2 files changed, 159 insertions(+)
create mode 100644
UefiCpuPkg/Include/Library/CpuMmuLib.h
diff --git a/UefiCpuPkg/Include/Library/CpuMmuLib.h
b/UefiCpuPkg/Include/Library/CpuMmuLib.h
new file mode 100644
index 0000000000..23b2fe34ac
--- /dev/null
+++ b/UefiCpuPkg/Include/Library/CpuMmuLib.h
@@ -0,0 +1,155 @@
+/** @file
+
+ Copyright (c) 2023 Loongson Technology
Corporation Limited. All rights
reserved.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef CPU_MMU_LIB_H_
+#define CPU_MMU_LIB_H_
+
+#include <Uefi/UefiBaseType.h>
+
+#define EFI_MEMORY_CACHETYPE_MASK
(EFI_MEMORY_UC | \
+
EFI_MEMORY_WC | \
+
EFI_MEMORY_WT | \
+
EFI_MEMORY_WB | \
+
EFI_MEMORY_UCE \
+ )
+
+typedef struct {
+ EFI_PHYSICAL_ADDRESS PhysicalBase;
+ EFI_VIRTUAL_ADDRESS VirtualBase;
+ UINTN Length;
+ UINTN Attributes;
+} MEMORY_REGION_DESCRIPTOR;
+
+/**
+ Converts EFI Attributes to corresponding
architecture Attributes.
+
+ @param[in] EfiAttributes Efi Attributes.
+
+ @retval Corresponding architecture attributes.
+**/
+UINTN
+EfiAttributeConverse (
+ IN UINTN EfiAttributes
+ );
+
+/**
+ Finds the length and memory properties of the
memory region
corresponding to the specified base address.
+
+ @param[in] BaseAddress To find the base
address of the memory
region.
+ @param[in] EndAddress To find the end
address of the memory
region.
+ @param[out] RegionLength The length of the
memory region
found.
+ @param[out] RegionAttributes Properties of
the memory region
found.
+
+ @retval EFI_SUCCESS The corresponding
memory area was
successfully found
+ EFI_NOT_FOUND No memory area found
+**/
+EFI_STATUS
+GetMemoryRegionAttribute (
+ IN UINTN BaseAddress,
+ IN UINTN EndAddress,
+ OUT UINTN *RegionLength,
+ OUT UINTN *RegionAttributes
+ );
+
+/**
+ Sets the Attributes of the specified memory region
+
+ @param[in] BaseAddress The base address of
the memory region
to set the Attributes.
+ @param[in] Length The length of the
memory region to set
the Attributes.
+ @param[in] Attributes The Attributes to be
set.
+ @param[in] AttributeMask Mask of memory
attributes to take into
account.
+
+ @retval EFI_SUCCESS The Attributes was set
successfully
+**/
+EFI_STATUS
+SetMemoryRegionAttributes (
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINTN Length,
+ IN UINTN Attributes,
+ IN UINT64 AttributeMask
+ );
+
+/**
+ Sets the non-executable Attributes for the
specified memory region
+
+ @param[in] BaseAddress The base address of
the memory region to
set the Attributes.
+ @param[in] Length The length of the
memory region to set the
Attributes.
+
+ @retval EFI_SUCCESS The Attributes was set
successfully
+**/
+EFI_STATUS
+SetMemoryRegionNoExec (
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINTN Length
+ );
+
+/**
+ Clears the non-executable Attributes for the
specified memory region
+
+ @param[in] BaseAddress The base address of
the memory region to
clear the Attributes.
+ @param[in] Length The length of the
memory region to clear
the Attributes.
+
+ @retval EFI_SUCCESS The Attributes was
clear successfully
+**/
+EFI_STATUS
+EFIAPI
+ClearMemoryRegionNoExec (
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length
+ );
+
+/**
+ Sets the read-only Attributes for the specified
memory region
+
+ @param[in] BaseAddress The base address of
the memory region to
set the Attributes.
+ @param[in] Length The length of the
memory region to set the
Attributes.
+
+ @retval EFI_SUCCESS The Attributes was set
successfully
+**/
+EFI_STATUS
+EFIAPI
+SetMemoryRegionReadOnly (
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length
+ );
+
+/**
+ Clears the read-only Attributes for the
specified memory region
+
+ @param[in] BaseAddress The base address of
the memory region to
clear the Attributes.
+ @param[in] Length The length of the
memory region to clear
the Attributes.
+
+ @retval EFI_SUCCESS The Attributes was
clear successfully
+**/
+EFI_STATUS
+EFIAPI
+ClearMemoryRegionReadOnly (
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length
+ );
+
+/**
+ Create a page table and initialize the memory
management unit(MMU).
+
+ @param[in] MemoryTable A pointer to
a memory ragion
table.
+ @param[out] TranslationTableBase A pointer to
a translation table base
address.
+ @param[out] TranslationTableSize A pointer to
a translation table base
size.
+
+ @retval EFI_SUCCESS Configure
MMU successfully.
+ EFI_INVALID_PARAMETER MemoryTable
is NULL.
+ EFI_UNSUPPORTED Out of
memory space or
size not aligned.
+**/
+EFI_STATUS
+EFIAPI
+ConfigureMemoryManagementUint (
+ IN MEMORY_REGION_DESCRIPTOR *MemoryTable,
+ OUT VOID
**TranslationTableBase OPTIONAL,
+ OUT UINTN
*TranslationTableSize OPTIONAL
+ );
+
+#endif // CPU_MMU_LIB_H_
diff --git a/UefiCpuPkg/UefiCpuPkg.dec
b/UefiCpuPkg/UefiCpuPkg.dec
index 154b1d06fe..150beae981 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -62,6 +62,10 @@
## @libraryclass Provides function for
manipulating x86 paging
structures.
CpuPageTableLib|Include/Library/CpuPageTableLib.h
+[LibraryClasses.LoongArch64]
+ ## @libraryclass Provides macros and
functions for the memory
management unit.
+ CpuMmuLib|Include/Library/CpuMmuLib.h
+
## @libraryclass Provides functions for
manipulating smram savestate
registers.
MmSaveStateLib|Include/Library/MmSaveStateLib.h
--
2.27.0
