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 *On Behalf Of >> *Chao Li >> *Sent:* Monday, December 4, 2023 3:32 PM >> *To:* devel@edk2.groups.io; Ni, Ray >> *Cc:* Dong, Eric ; Kumar, Rahul R >> ; Gerd Hoffmann ; Leif >> Lindholm ; Ard Biesheuvel >> ; Sami Mujawar ; >> Sunil V L ; Warkentin, Andrei >> >> *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 >> >> Sent: Friday, November 17, 2023 6:00 PM >> >> To:devel@edk2.groups.io >> >> Cc: Dong, Eric ; Ni, Ray ; Kumar, >> >> Rahul R ; Gerd Hoffmann ; >> >> Leif Lindholm ; Ard Biesheuvel >> >> ; Sami Mujawar ; >> >> Sunil V L ; Warkentin, Andrei >> >> >> >> 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 >> >> Cc: Ray Ni >> >> Cc: Rahul Kumar >> >> Cc: Gerd Hoffmann >> >> Cc: Leif Lindholm >> >> Cc: Ard Biesheuvel >> >> Cc: Sami Mujawar >> >> Cc: Sunil V L >> >> Cc: Andrei Warkentin >> >> Signed-off-by: Chao Li >> >> --- >> >> 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.
>> >> + >> >> +  SPDX-License-Identifier: BSD-2-Clause-Patent >> >> + >> >> +**/ >> >> + >> >> +#ifndef CPU_MMU_LIB_H_ >> >> +#define CPU_MMU_LIB_H_ >> >> + >> >> +#include >> >> + >> >> +#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 (#112206): https://edk2.groups.io/g/devel/message/112206 Mute This Topic: https://groups.io/mt/102644768/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-