Ok, I will submit the V4 today. Thanks, Chao On 2023/12/11 16:13, Ni, Ray wrote: > > Thanks. I think we are aligned. Looking forward to your v4 patch. > > Thanks, > > Ray > > *From:*devel@edk2.groups.io *On Behalf Of *Chao Li > *Sent:* Friday, December 8, 2023 10:10 AM > *To:* Ni, Ray ; devel@edk2.groups.io > *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, > > 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 (#112282): https://edk2.groups.io/g/devel/message/112282 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] -=-=-=-=-=-=-=-=-=-=-=-