From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 3F62DAC0473 for ; Fri, 8 Dec 2023 02:10:13 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=8ljGOgTApiPj/OuN4JXkUebhvWKX4J2SmBaEwFUqzOg=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:User-Agent:Subject:From:To:Cc:Reply-To:References:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1702001411; v=1; b=Ww5zeguax0Oq3snR6cPTJaAJ8Mt8sguRwbnyeLt4G7ChE8ZLdVoPoBTa7wHkgP2+lU8RY0D9 7EV7XUQ+qsCbrpaAUEtHFnFNXr11bpub5rofmQeoKfclKQ/FYqGZhYOtzdGgcFzECDsYl56bJI4 AOHHO36lQdiMHLT9QzhchPTM= X-Received: by 127.0.0.2 with SMTP id vCbLYY7687511xPf1ajj6D3f; Thu, 07 Dec 2023 18:10:11 -0800 X-Received: from mail.loongson.cn (mail.loongson.cn [114.242.206.163]) by mx.groups.io with SMTP id smtpd.web10.10600.1702001409615910667 for ; Thu, 07 Dec 2023 18:10:10 -0800 X-Received: from loongson.cn (unknown [10.40.24.149]) by gateway (Coremail) with SMTP id _____8Dx_7v9enJlUNo_AA--.3138S3; Fri, 08 Dec 2023 10:10:05 +0800 (CST) X-Received: from [10.40.24.149] (unknown [10.40.24.149]) by localhost.localdomain (Coremail) with SMTP id AQAAf8Bx7tz4enJl_PpXAA--.64188S3; Fri, 08 Dec 2023 10:10:01 +0800 (CST) Message-ID: <980334d8-a705-450d-8ecf-c618d0781f20@loongson.cn> Date: Fri, 8 Dec 2023 10:10:00 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [PATCH v3 13/39] UefiCpuPkg: Add CpuMmuLib.h to UefiCpuPkg From: "Chao Li" 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" Reply-To: devel@edk2.groups.io,lichao@loongson.cn References: <20231117095742.3605778-1-lichao@loongs> <20231117100026.3609206-1-lichao@loongson.cn> <179C457B5B852375.31732@groups.io> <179DEF40376B662A.18076@groups.io> In-Reply-To: <179DEF40376B662A.18076@groups.io> X-CM-TRANSID: AQAAf8Bx7tz4enJl_PpXAA--.64188S3 X-CM-SenderInfo: xolfxt3r6o00pqjv00gofq/1tbiAQATCGVxK6wPRwAAsB X-Coremail-Antispam: 1Uk129KBj9fXoWfJFy8Ar47ur18GF4DXw4UKFX_yoW8GFWkCo W5KF4rJrs8Jr1rurWrCr18Jry5GFs8Grs8Jr15tFy7XF4UJr1DJ3yxXayDJayFyr18Gr4D Jry7J34DJFWSgr15l-sFpf9Il3svdjkaLaAFLSUrUUUUjb8apTn2vfkv8UJUUUU8wcxFpf 9Il3svdxBIdaVrnsAqx4xG67k08I80eVW7JVWxJwAqx4xG62kEwI0EY4vaYxAvb48xMc02 F40E57IF67AEF4xIwI1l5I8CrVAKz4kIr2xC04v26r1j6r4UMc02F40E42I26xC2a48xYx n0WfASr-VFAUDa7-sFnT9fnUUIcSsGvfJTRUUUbvAYFVCjjxCrM7AC8VAFwI0_Jr0_Gr1l 1xkIjI8I6I8E6xAIw20EY4v20xvaj40_Wr0E3s1l1IIY67AEw4v_Jrv_JF1l8cAvFVAK0I I2c7xJM28CjxkF64kEwVA0rcxSw2x7M28EF7xvwVC0I7IYx2IY67AKxVW8JVW5JwA2z4x0 Y4vE2Ix0cI8IcVCY1x0267AKxVW8JVWxJwA2z4x0Y4vEx4A2jsIE14v26F4j6r4UJwA2z4 x0Y4vEx4A2jsIEc7CjxVAFwI0_Gr0_Gr1UM2AIxVAIcxkEcVAq07x20xvEncxIr21l57IF 6xkI12xvs2x26I8E6xACxx1l5I8CrVAYj202j2C_Ar0_Cr1l5I8CrVAaz4v26cxKscIFY7 kG0wAqx4xG6c804VAFz4xC04v7Mc02F40Ew4AK048IF2xKxVWUJVW8JwAqx4xG6xAIxVCF xsxG0wAv7VC0I7IYx2IY67AKxVWUGVWUXwAv7VC2z280aVAFwI0_Jr0_Gr1lOx8S6xCaFV Cjc4AY6r1j6r4UM4x0Y48IcVAKI48JMx8GjcxK6IxK0xIIj40E5I8CrwCF04k20xvY0x0E wIxGrwCFx2IqxVCFs4IE7xkEbVWUJVW8JwC20s026c02F40E14v26r106r1rMI8I3I0E74 80Y4vE14v26r106r1rMI8E67AF67kF1VAFwI0_Jw0_GFylIxkGc2Ij64vIr41lIxAIcVC0 I7IYx2IY67AKxVWUJVWUCwCI42IY6xIIjxv20xvEc7CjxVAFwI0_Jr0_Gr1lIxAIcVCF04 k26cxKx2IYs7xG6r1j6r1xMIIF0xvEx4A2jsIE14v26r1j6r4UMIIF0xvEx4A2jsIEc7Cj xVAFwI0_Jr0_GrUvcSsGvfC2KfnxnUUI43ZEXa7IU06c_DUUUUU== Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: C9erxIFZaubKU1LZFl50nTnwx7686176AA= Content-Type: multipart/alternative; boundary="------------xY4zQa6CidP0z7UsK3jY3Q9R" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=Ww5zegua; dmarc=none; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io --------------xY4zQa6CidP0z7UsK3jY3Q9R Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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] -=-=-=-=-=-=-=-=-=-=-=- --------------xY4zQa6CidP0z7UsK3jY3Q9R Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

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> On Behalf Of Chao Li
Sent: Monday, December 4, 2023 3:32 PM
To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
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,

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.

  1. 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.

  1. 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>
Sent: Friday, November 17, 2023 6:00 PM
To: devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@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: [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>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Sunil V L <sunilvl@ventanamicro.com>
Cc: Andrei Warkentin <andrei.warkentin@intel.com>
Signed-off-by: Chao Li <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 (#112206) | | Mute This Topic | New Topic
Your Subscription | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--------------xY4zQa6CidP0z7UsK3jY3Q9R--