Re: [edk2-devel] [PATCH v7 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V

["Laszlo Ersek" <lersek@redhat.com>]

On 10/29/23 20:07, Pedro Falcato wrote:
> On Sun, Oct 29, 2023 at 2:46 PM Dhaval Sharma <dhaval@rivosinc.com> wrote:
>>
>> Use newly defined cache management operations for RISC-V where possible
>> It builds up on the support added for RISC-V cache management
>> instructions in BaseLib.
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>>
>> Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
>> Acked-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> Notes:
>>     V7:
>>     - Added PcdLib
>>     - Restructure DEBUG message based on feedback on V6
>>     - Make naming consistent to CMO, remove all CBO references
>>     - Add ASSERT for not supported functions instead of plain debug message
>>     - Added RB tag
>>     V6:
>>     - Utilize cache management instructions if HW supports it
>>       This patch is part of restructuring on top of v5
>>
>>  MdePkg/MdePkg.dec                                                  |   8 +
>>  MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf |   5 +
>>  MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c                | 168 +++++++++++++++++---
>>  MdePkg/MdePkg.uni                                                  |   4 +
>>  4 files changed, 165 insertions(+), 20 deletions(-)
>>
>> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
>> index ac54338089e8..fa92673ff633 100644
>> --- a/MdePkg/MdePkg.dec
>> +++ b/MdePkg/MdePkg.dec
>> @@ -2399,6 +2399,14 @@ [PcdsFixedAtBuild.AARCH64, PcdsPatchableInModule.AARCH64]
>>    # @Prompt CPU Rng algorithm's GUID.
>>    gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm|{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}|VOID*|0x00000037
>>
>> +[PcdsFixedAtBuild.RISCV64, PcdsPatchableInModule.RISCV64]
>> +  #
>> +  # Configurability to override RISC-V CPU Features
>> +  # BIT 0 = Cache Management Operations. This bit is relevant only if
>> +  # previous stage has feature enabled and user wants to disable it.
>> +  #
>> +  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFFFF|UINT64|0x69
>> +
>>  [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>>    ## This value is used to set the base address of PCI express hierarchy.
>>    # @Prompt PCI Express Base Address.
>> diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
>> index 6fd9cbe5f6c9..601a38d6c109 100644
>> --- a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
>> +++ b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
>> @@ -56,3 +56,8 @@ [LibraryClasses]
>>    BaseLib
>>    DebugLib
>>
>> +[LibraryClasses.RISCV64]
>> +  PcdLib
>> +
>> +[Pcd.RISCV64]
>> +  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride  ## CONSUMES
>> diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
>> index 4eb18edb9aa7..5b3104afb67e 100644
>> --- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
>> +++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
>> @@ -2,6 +2,7 @@
>>    RISC-V specific functionality for cache.
>>
>>    Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
>> +  Copyright (c) 2023, Rivos Inc. All rights reserved.<BR>
>>
>>    SPDX-License-Identifier: BSD-2-Clause-Patent
>>  **/
>> @@ -9,10 +10,115 @@
>>  #include <Base.h>
>>  #include <Library/BaseLib.h>
>>  #include <Library/DebugLib.h>
>> +#include <Library/PcdLib.h>
>> +
>> +//
>> +// TODO: Grab cache block size and make Cache Management Operation
>> +// enabling decision based on RISC-V CPU HOB in
>> +// future when it is available.
>> +//
>> +#define RISCV_CACHE_BLOCK_SIZE         64
>> +#define RISCV_CPU_FEATURE_CMO_BITMASK  0x1
>> +
>> +typedef enum {
>> +  Clean,
>> +  Flush,
>> +  Invld,
>> +} CACHE_OP;
> 
> small nit: You may want to do something like CACHE_OP_{CLEAN, FLUSH,
> INVID}. but since this is a file-local enum I don't consider this
> merge-blocking.
> 

Agree with you on the prefix, but not on the uppercase spelling; edk2
(and UEFI) uses CamelCase enumeration constants. Compare: at the very
top of "MdePkg/Include/Uefi/UefiSpec.h", we have EFI_ALLOCATE_TYPE with
constants like AllocateAnyPages.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110304): https://edk2.groups.io/g/devel/message/110304
Mute This Topic: https://groups.io/mt/102256468/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-