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 3E6BDAC0294 for ; Tue, 19 Dec 2023 01:59:22 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=m1Cg7biIZsu4YJgejcCm6YXARztGlbOT3GmA5JiL5YA=; 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=1702951160; v=1; b=SgdaXuCw7ybJvuMbrr1ZNyH76HMkJ154ncuqRjNM5BQjHHVXk7Ypv7RfHLIGk2mJ5ZYu+b25 5aH3cw3UsDLyvlzK8Gwc19b3GtjKz1UexUI9jmVBhlvEhW1ehNX2KNmk1/CSiztluKpgFMED7rB 7rmPWhZ8EXmjsj5QO4JUkZxA= X-Received: by 127.0.0.2 with SMTP id 1YpGYY7687511x7gfhGlEPRj; Mon, 18 Dec 2023 17:59:20 -0800 X-Received: from mail.loongson.cn (mail.loongson.cn [114.242.206.163]) by mx.groups.io with SMTP id smtpd.web11.2132.1702951159348157598 for ; Mon, 18 Dec 2023 17:59:20 -0800 X-Received: from loongson.cn (unknown [10.40.24.149]) by gateway (Coremail) with SMTP id _____8AxXOnx+IBlxnUCAA--.12591S3; Tue, 19 Dec 2023 09:59:13 +0800 (CST) X-Received: from [10.40.24.149] (unknown [10.40.24.149]) by localhost.localdomain (Coremail) with SMTP id AQAAf8DxSXPv+IBl8ZgLAA--.39617S3; Tue, 19 Dec 2023 09:59:11 +0800 (CST) Message-ID: <0e0d16f2-cf43-45cc-a79c-b27d556aa0ea@loongson.cn> Date: Tue, 19 Dec 2023 09:59:11 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [PATCH v4 12/37] UefiCpuPkg: Add CpuMmuLib.h to UefiCpuPkg From: "Chao Li" To: devel@edk2.groups.io, ray.ni@intel.com 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: <20231212130932.2467028-1-lichao@loongson.cn> <20231212131216.2470799-1-lichao@loongson.cn> <17A0932406FD861E.11381@groups.io> In-Reply-To: <17A0932406FD861E.11381@groups.io> X-CM-TRANSID: AQAAf8DxSXPv+IBl8ZgLAA--.39617S3 X-CM-SenderInfo: xolfxt3r6o00pqjv00gofq/1tbiAQAKCGV-rC0UbwABsi X-Coremail-Antispam: 1Uk129KBj93XoWxGFWrCry3Zw4rWF1kJF1kWFX_yoW5tFWrpr 1kCrWUJ34Utr1xXr18Jr4UXry0yry8Ja4UJr1DAF1UAr1UJw1IqF1UZw1j9r17GF48A34U Xr1UXw1UZFW5GwbCm3ZEXasCq-sJn29KB7ZKAUJUUUU8529EdanIXcx71UUUUU7KY7ZEXa sCq-sGcSsGvfJ3UbIjqfuFe4nvWSU5nxnvy29KBjDU0xBIdaVrnRJUUUymb4IE77IF4wAF F20E14v26r1j6r4UM7CY07I20VC2zVCF04k26cxKx2IYs7xG6rWj6s0DM7CIcVAFz4kK6r 1Y6r17M28lY4IEw2IIxxk0rwA2F7IY1VAKz4vEj48ve4kI8wA2z4x0Y4vE2Ix0cI8IcVAF wI0_Jr0_JF4l84ACjcxK6xIIjxv20xvEc7CjxVAFwI0_Jr0_Gr1l84ACjcxK6I8E87Iv67 AKxVW8JVWxJwA2z4x0Y4vEx4A2jsIEc7CjxVAFwI0_Gr0_Gr1UM2AIxVAIcxkEcVAq07x2 0xvEncxIr21l57IF6xkI12xvs2x26I8E6xACxx1lYx0E2Ix0cI8IcVAFwI0_JrI_JrylYx 0Ex4A2jsIE14v26r1j6r4UMcvjeVCFs4IE7xkEbVWUJVW8JwACjcxG0xvEwIxGrwCjr7xv wVCIw2I0I7xG6c02F41l42xK82IYc2Ij64vIr41l4I8I3I0E4IkC6x0Yz7v_Jr0_Gr1lx2 IqxVAqx4xG67AKxVWUGVWUWwC20s026x8GjcxK67AKxVWUGVWUWwC2zVAF1VAY17CE14v2 6r1q6r43MIIYrxkI7VAKI48JMIIF0xvE2Ix0cI8IcVAFwI0_Jr0_JF4lIxAIcVC0I7IYx2 IY6xkF7I0E14v26r1j6r4UMIIF0xvE42xK8VAvwI8IcIk0rVWUJVWUCwCI42IY6I8E87Iv 67AKxVWUJVW8JwCI42IY6I8E87Iv6xkF7I0E14v26r1j6r4UYxBIdaVFxhVjvjDU0xZFpf 9x07UAWrXUUUUU= 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: 9KZzMG5bmhsfID0cGYhyd2zPx7686176AA= Content-Type: multipart/alternative; boundary="------------RDJ1uzuyy6IRSGq41RyzqzaO" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=SgdaXuCw; 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 --------------RDJ1uzuyy6IRSGq41RyzqzaO Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi Ray, For the GetMemoryRegionAttributes, here are some of my plans: Thanks, Chao On 2023/12/14 10:53, Chao Li wrote: > Hi Ray, > > For you two comments: > > BTY, can you review the other UefiCpuPkg patches in this series? > > On 2023/12/13 下午1:17, Ni, Ray wrote: >> Chao, >> Thanks for providing such a cleaner lib API. >> Only 2 comments in below: >> >>> + >>> +#define EFI_MEMORY_CACHETYPE_MASK  (EFI_MEMORY_UC  | \ >>> +                                    EFI_MEMORY_WC  | \ >>> +                                    EFI_MEMORY_WT  | \ >>> +                                    EFI_MEMORY_WB  | \ >>> +                                    EFI_MEMORY_UCE   \ >>> +                                    ) >> 1. Why do you need to define the EFI_MEMORY_CACHETYPE_MASK here? >> Can you just mention that the following APIs only return 5 bits: >> UC/WC/WT/WB/UCE >> without defining the EFI_MEMORY_CACHETYPE_MASK? > Agree, I will remove this definition in V5 and if someone uses this > macro, it will be private. >> >>> + >>> +typedef struct { >>> +  EFI_PHYSICAL_ADDRESS    PhysicalBase; >>> +  EFI_VIRTUAL_ADDRESS     VirtualBase; >>> +  UINTN                   Length; >>> +  UINTN                   Attributes; >>> +} MEMORY_REGION_DESCRIPTOR; >>> + >>> +/** >>> +  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 >>> +EFIAPI >>> +GetMemoryRegionAttributes ( >>> +  IN     UINTN  BaseAddress, >>> +  IN     UINTN  EndAddress, >>> +  OUT    UINTN  *RegionLength, >>> +  OUT    UINTN  *RegionAttributes >>> +  ); >> 2. If the actual memory ranges are as follows: >> [0 - 1M, UC] >> [1M - 1G, WB] >> >> What's the result of following call: >> a. GetMemoryRegionAttributes (512KB, 1MB) >> b. GetMemoryRegionAttributes (512KB, 2MB) > Yes, if the given memory region has two or more types of attributes, > this API may return the first attribute, I should design this API > again, thanks. For this API, I think there may be two options: *Plan A:* It will return all of the attributes and lengths located in the region. We need to return a pointer to a structure that records the attributes and length of each part. *Plan B:* It only returns the attribute and length of the first part, leaving it up to the caller to decide to look for the next part. I'm leaning toward plan B, what do you think? >> >> >> >> >> > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112662): https://edk2.groups.io/g/devel/message/112662 Mute This Topic: https://groups.io/mt/103129095/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- --------------RDJ1uzuyy6IRSGq41RyzqzaO Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

Hi Ray,

For the GetMemoryRegionAttributes, here are some of my plans:


Thanks,
Chao
On 2023/12/14 10:53, Chao Li wrote:
Hi Ray,

For you two comments:

BTY, can you review the other UefiCpuPkg patches in this series?

On 2023/12/13 下午1:17, Ni, Ray wrote:
Chao,
Thanks for providing such a cleaner lib API.
Only 2 comments in below:

+
+#define EFI_MEMORY_CACHETYPE_MASK  (EFI_MEMORY_UC  | \
+                                    EFI_MEMORY_WC  | \
+                                    EFI_MEMORY_WT  | \
+                                    EFI_MEMORY_WB  | \
+                                    EFI_MEMORY_UCE   \
+                                    )
1. Why do you need to define the EFI_MEMORY_CACHETYPE_MASK here?
Can you just mention that the following APIs only return 5 bits: UC/WC/WT/WB/UCE
without defining the EFI_MEMORY_CACHETYPE_MASK?
Agree, I will remove this definition in V5 and if someone uses this macro, it will be private.

+
+typedef struct {
+  EFI_PHYSICAL_ADDRESS    PhysicalBase;
+  EFI_VIRTUAL_ADDRESS     VirtualBase;
+  UINTN                   Length;
+  UINTN                   Attributes;
+} MEMORY_REGION_DESCRIPTOR;
+
+/**
+  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
+EFIAPI
+GetMemoryRegionAttributes (
+  IN     UINTN  BaseAddress,
+  IN     UINTN  EndAddress,
+  OUT    UINTN  *RegionLength,
+  OUT    UINTN  *RegionAttributes
+  );
2. If the actual memory ranges are as follows:
[0 - 1M, UC]
[1M - 1G, WB]

What's the result of following call:
a. GetMemoryRegionAttributes (512KB, 1MB)
b. GetMemoryRegionAttributes (512KB, 2MB)
Yes, if the given memory region has two or more types of attributes, this API may return the first attribute, I should design this API again, thanks.

For this API, I think there may be two options:

Plan A:

It will return all of the attributes and lengths located in the region. We need to return a pointer to a structure that records the attributes and length of each part.

Plan B:

It only returns the attribute and length of the first part, leaving it up to the caller to decide to look for the next part.


I'm leaning toward plan B, what do you think?











_._,_._,_
Groups.io Links:

You receive all messages sent to this group.

View/Reply Online (#112662) | | Mute This Topic | New Topic
Your Subscription | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--------------RDJ1uzuyy6IRSGq41RyzqzaO--