Re: [edk2-devel] [PATCH v3 13/39] UefiCpuPkg: Add CpuMmuLib.h to UefiCpuPkg

["Chao Li" <lichao@loongson.cn>]

[-- Attachment #1: Type: text/plain, Size: 24211 bytes --]

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 <devel@edk2.groups.io> *On Behalf Of *Chao Li
> *Sent:* Friday, December 8, 2023 10:10 AM
> *To:* Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> *Cc:* Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R 
> <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Leif 
> Lindholm <quic_llindhol@quicinc.com>; Ard Biesheuvel 
> <ardb+tianocore@kernel.org>; Sami Mujawar <sami.mujawar@arm.com>; 
> Sunil V L <sunilvl@ventanamicro.com>; Warkentin, Andrei 
> <andrei.warkentin@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.ni@intel.com>
>         <mailto:ray.ni@intel.com>
>         *Cc:* Dong, Eric <eric.dong@intel.com>
>         <mailto:eric.dong@intel.com>; Kumar, Rahul R
>         <rahul.r.kumar@intel.com> <mailto:rahul.r.kumar@intel.com>;
>         Gerd Hoffmann <kraxel@redhat.com> <mailto:kraxel@redhat.com>;
>         Leif Lindholm <quic_llindhol@quicinc.com>
>         <mailto:quic_llindhol@quicinc.com>; Ard Biesheuvel
>         <ardb+tianocore@kernel.org>
>         <mailto:ardb+tianocore@kernel.org>; Sami Mujawar
>         <sami.mujawar@arm.com> <mailto:sami.mujawar@arm.com>; Sunil V
>         L <sunilvl@ventanamicro.com>
>         <mailto:sunilvl@ventanamicro.com>; Warkentin, Andrei
>         <andrei.warkentin@intel.com> <mailto:andrei.warkentin@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 <lichao@loongson.cn>
>                     <mailto:lichao@loongson.cn>
>
>                     Sent: Friday, November 17, 2023 6:00 PM
>
>                     To: devel@edk2.groups.io
>
>                     Cc: Dong, Eric <eric.dong@intel.com>
>                     <mailto:eric.dong@intel.com>; Ni, Ray
>                     <ray.ni@intel.com> <mailto:ray.ni@intel.com>; Kumar,
>
>                     Rahul R <rahul.r.kumar@intel.com>
>                     <mailto:rahul.r.kumar@intel.com>; Gerd Hoffmann
>                     <kraxel@redhat.com> <mailto:kraxel@redhat.com>;
>
>                     Leif Lindholm <quic_llindhol@quicinc.com>
>                     <mailto:quic_llindhol@quicinc.com>; Ard Biesheuvel
>
>                     <ardb+tianocore@kernel.org>
>                     <mailto:ardb+tianocore@kernel.org>; Sami Mujawar
>                     <sami.mujawar@arm.com> <mailto:sami.mujawar@arm.com>;
>
>                     Sunil V L <sunilvl@ventanamicro.com>
>                     <mailto:sunilvl@ventanamicro.com>; Warkentin, Andrei
>
>                     <andrei.warkentin@intel.com>
>                     <mailto:andrei.warkentin@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.dong@intel.com>
>                     <mailto:eric.dong@intel.com>
>
>                     Cc: Ray Ni <ray.ni@intel.com>
>                     <mailto:ray.ni@intel.com>
>
>                     Cc: Rahul Kumar <rahul1.kumar@intel.com>
>                     <mailto:rahul1.kumar@intel.com>
>
>                     Cc: Gerd Hoffmann <kraxel@redhat.com>
>                     <mailto:kraxel@redhat.com>
>
>                     Cc: Leif Lindholm <quic_llindhol@quicinc.com>
>                     <mailto:quic_llindhol@quicinc.com>
>
>                     Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>                     <mailto:ardb+tianocore@kernel.org>
>
>                     Cc: Sami Mujawar <sami.mujawar@arm.com>
>                     <mailto:sami.mujawar@arm.com>
>
>                     Cc: Sunil V L <sunilvl@ventanamicro.com>
>                     <mailto:sunilvl@ventanamicro.com>
>
>                     Cc: Andrei Warkentin <andrei.warkentin@intel.com>
>                     <mailto:andrei.warkentin@intel.com>
>
>                     Signed-off-by: Chao Li <lichao@loongson.cn>
>                     <mailto:lichao@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 (#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]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 49097 bytes --]