On 10/30/23 12:18, Sunil V L wrote:
> On Sun, Oct 29, 2023 at 08:16:12PM +0530, Dhaval Sharma 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.kin...@intel.com>
>> Cc: Liming Gao <gaolim...@byosoft.com.cn>
>> Cc: Zhiguang Liu <zhiguang....@intel.com>
>> Cc: Laszlo Ersek <ler...@redhat.com>
>>
>> Signed-off-by: Dhaval Sharma <dha...@rivosinc.com>
>> Acked-by: Laszlo Ersek <ler...@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.
> NIT: I am wondering whether PcdRiscVCpuFeatureDisable is better so that
> it is explicit.
>
>> + #
>> +
>> gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFFFF|UINT64|0x69
>> +
> Instead of this, can default value match only those features which are
> enabled by default for qemu virt machine? That way, I think we can avoid
> having this PCD defined again in RiscVVirt.
I proposed the all-bits-one value.
First, PcdRiscVFeatureOverride is in MdePkg, which is much more general
than QEMU.
Second, the all-bits-one pattern means that the MdePkg.dec default does
not try to disable any features enabled by earlier components in the
boot process, *even such features* that it does not know about
specifically (i.e., those for which edk2 does not define a specific
feature bit).
IMO, for any "feature negotiation" bitmask, there are two choices:
- clear all bits except those that we know about (and know how to use),
- use all-bits-one.
The first option is good if unknown (future) features are expected to
*break* us.
The second option is good if we are unaffected by extra (future)
features, and we want to keep everything enabled for the runtime OS that
comes after us.
I understood that we had case #2 here.
If we have case#1 instead, then we should indeed limit the bitmask to
those features that *core edk2* knows about. But even in that case, I
think MdePkg.dec should be as permissive as possible, and any
QEMU-specific limitation should go into the OVMF DSC file.
>
>> [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
>> +
> Can we define these bits in the header file so that the definitions can
> be used by multiple modules?
I noticed this too, but didn't call it out, because the DEC file
sufficiently explained the bit meaning(s).
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110397): https://edk2.groups.io/g/devel/message/110397
Mute This Topic: https://groups.io/mt/102256468/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
