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]
-=-=-=-=-=-=-=-=-=-=-=-