Hi Ray,
Thanks,
Chao
On 2023/12/5 16:27, Ni, Ray wrote:
Thanks,
Ray
*From:* devel@edk2.groups.io <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>
*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,
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112079): https://edk2.groups.io/g/devel/message/112079
Mute This Topic: https://groups.io/mt/102644768/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
