Re: [edk2-devel] [PATCH v10 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V
Based on the above discussion, and some more thoughts I am thinking it is
okay to at least replace ASSERT from CMO code and let other platform code
place its own guards to avoid calling this code when it is known that
platform does not support such operations. If there are no objections to
this we can go ahead.
On Tue, Jan 9, 2024 at 9:50 PM Warkentin, Andrei
wrote:
> For now, this is really something that ought to be hidden by DmaLib
> abstraction (Map/Unmap). This would allow the driver to be minimally aware
> of how the IP is integrated into the SoC.
>
> A
>
> > -Original Message-
> > From: Sunil V L
> > Sent: Monday, January 8, 2024 11:32 PM
> > To: Pedro Falcato
> > Cc: devel@edk2.groups.io; dha...@rivosinc.com; yorange
> > ; Warkentin, Andrei
> > ; Ard Biesheuvel >;
> > Leif Lindholm
> > Subject: Re: [edk2-devel] [PATCH v10 4/5] MdePkg: Utilize Cache
> > Management Operations Implementation For RISC-V
> >
> > On Mon, Jan 08, 2024 at 09:53:46PM +, Pedro Falcato wrote:
> > > On Mon, Jan 8, 2024 at 4:23 PM Dhaval Sharma
> > wrote:
> > > >
> > > > Hi yangcheng/Pedro,
> > >
> > > +CC a bunch of relevant people
> > >
> > > Hi, (FYI you did not CC me)
> > >
> > > Looking at yangcheng's example:
> > >
> > > Status = PrepareDmaData (gpIdmacDesc, Length, Buffer); <-- We write
> > > to the IDMAC desc
> > > if (EFI_ERROR (Status)) {
> > > goto out;
> > > }
> > >
> > > WriteBackDataCacheRange (gpIdmacDesc, DescPages * EFI_PAGE_SIZE);
> > > <-- Make sure it's DMA-coherent
> > > StartDma (Length); <-- We've flushed the cache, everything is now in
> > > DRAM and DMA-coherent, start DMA
> > >
> > > which screams of "bad abstractions" because you don't actually need to
> > > write data back, if the device and platform are DMA coherent.
> > >
> > > So what we want here really depends. My local "Volume I: RISC-V
> > > Unprivileged ISA V20191213" says, section A.5:
> > >
> > > "Table A.5 provides a mapping of Linux memory ordering macros onto
> > > RISC-V memory instructions.
> > > The Linux fences dma rmb() and dma wmb() map onto FENCE R,R and FENCE
> > > W,W, respectively, since the RISC-V Unix Platform requires coherent
> > > DMA, but would be mapped onto FENCE RI,RI and FENCE WO,WO,
> > > respectively, on a platform with non-coherent DMA.
> > > Platforms with non-
> > > coherent DMA may also require a mechanism by which cache lines can be
> > > flushed and/or invalidated.
> > > Such mechanisms will be device-specific and/or standardized in a
> > > future extension to the ISA."
> > >
> > > The (current date) RISCV Platform Spec also says: "Memory accesses by
> > > I/O masters can be coherent or non-coherent with respect to all
> > > hart-related caches."
> > > which is brilliantly useless.
> > >
> > > so I think the best solution here is to:
> > >
> > > 1) Add a new PCD for platform DMA coherency, and test that on
> > > WriteBackDataCacheRange's ASSERT (if (!Coherent) ASSERT() else
> > > return;)
> > > 2) Add a more abstracting API that doesn't necessarily map to
> > > WriteBackDataCache when all we wanted was to assert DMA coherency
> > >
> > > but, alas, I've seen a lot less funky platforms than many of you, and
> > > DMA/cache-coherency is not really my thing, so I'll defer to others..
> > >
> > My preference is just remove the assertion and add the debug verbose
> > message instead of changing drivers/introduce new interfaces. It is a
> nop in
> > linux as well if CMO is not present.
> >
> > Thanks,
> > Sunil
>
--
Thanks!
=D
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113652): https://edk2.groups.io/g/devel/message/113652
Mute This Topic: https://groups.io/mt/103150435/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v10 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V
For now, this is really something that ought to be hidden by DmaLib abstraction
(Map/Unmap). This would allow the driver to be minimally aware of how the IP is
integrated into the SoC.
A
> -Original Message-
> From: Sunil V L
> Sent: Monday, January 8, 2024 11:32 PM
> To: Pedro Falcato
> Cc: devel@edk2.groups.io; dha...@rivosinc.com; yorange
> ; Warkentin, Andrei
> ; Ard Biesheuvel ;
> Leif Lindholm
> Subject: Re: [edk2-devel] [PATCH v10 4/5] MdePkg: Utilize Cache
> Management Operations Implementation For RISC-V
>
> On Mon, Jan 08, 2024 at 09:53:46PM +, Pedro Falcato wrote:
> > On Mon, Jan 8, 2024 at 4:23 PM Dhaval Sharma
> wrote:
> > >
> > > Hi yangcheng/Pedro,
> >
> > +CC a bunch of relevant people
> >
> > Hi, (FYI you did not CC me)
> >
> > Looking at yangcheng's example:
> >
> > Status = PrepareDmaData (gpIdmacDesc, Length, Buffer); <-- We write
> > to the IDMAC desc
> > if (EFI_ERROR (Status)) {
> > goto out;
> > }
> >
> > WriteBackDataCacheRange (gpIdmacDesc, DescPages * EFI_PAGE_SIZE);
> > <-- Make sure it's DMA-coherent
> > StartDma (Length); <-- We've flushed the cache, everything is now in
> > DRAM and DMA-coherent, start DMA
> >
> > which screams of "bad abstractions" because you don't actually need to
> > write data back, if the device and platform are DMA coherent.
> >
> > So what we want here really depends. My local "Volume I: RISC-V
> > Unprivileged ISA V20191213" says, section A.5:
> >
> > "Table A.5 provides a mapping of Linux memory ordering macros onto
> > RISC-V memory instructions.
> > The Linux fences dma rmb() and dma wmb() map onto FENCE R,R and FENCE
> > W,W, respectively, since the RISC-V Unix Platform requires coherent
> > DMA, but would be mapped onto FENCE RI,RI and FENCE WO,WO,
> > respectively, on a platform with non-coherent DMA.
> > Platforms with non-
> > coherent DMA may also require a mechanism by which cache lines can be
> > flushed and/or invalidated.
> > Such mechanisms will be device-specific and/or standardized in a
> > future extension to the ISA."
> >
> > The (current date) RISCV Platform Spec also says: "Memory accesses by
> > I/O masters can be coherent or non-coherent with respect to all
> > hart-related caches."
> > which is brilliantly useless.
> >
> > so I think the best solution here is to:
> >
> > 1) Add a new PCD for platform DMA coherency, and test that on
> > WriteBackDataCacheRange's ASSERT (if (!Coherent) ASSERT() else
> > return;)
> > 2) Add a more abstracting API that doesn't necessarily map to
> > WriteBackDataCache when all we wanted was to assert DMA coherency
> >
> > but, alas, I've seen a lot less funky platforms than many of you, and
> > DMA/cache-coherency is not really my thing, so I'll defer to others..
> >
> My preference is just remove the assertion and add the debug verbose
> message instead of changing drivers/introduce new interfaces. It is a nop in
> linux as well if CMO is not present.
>
> Thanks,
> Sunil
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113457): https://edk2.groups.io/g/devel/message/113457
Mute This Topic: https://groups.io/mt/103150435/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v10 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V
On Mon, Jan 08, 2024 at 09:53:46PM +, Pedro Falcato wrote:
> On Mon, Jan 8, 2024 at 4:23 PM Dhaval Sharma wrote:
> >
> > Hi yangcheng/Pedro,
>
> +CC a bunch of relevant people
>
> Hi, (FYI you did not CC me)
>
> Looking at yangcheng's example:
>
> Status = PrepareDmaData (gpIdmacDesc, Length, Buffer); <-- We write
> to the IDMAC desc
> if (EFI_ERROR (Status)) {
> goto out;
> }
>
> WriteBackDataCacheRange (gpIdmacDesc, DescPages * EFI_PAGE_SIZE);
> <-- Make sure it's DMA-coherent
> StartDma (Length); <-- We've flushed the cache, everything is now in
> DRAM and DMA-coherent, start DMA
>
> which screams of "bad abstractions" because you don't actually need to
> write data back, if the device and platform are DMA coherent.
>
> So what we want here really depends. My local "Volume I: RISC-V
> Unprivileged ISA V20191213" says, section A.5:
>
> "Table A.5 provides a mapping of Linux memory ordering macros onto
> RISC-V memory instructions.
> The Linux fences dma rmb() and dma wmb() map onto FENCE R,R and FENCE
> W,W, respectively,
> since the RISC-V Unix Platform requires coherent DMA, but would be
> mapped onto FENCE RI,RI
> and FENCE WO,WO, respectively, on a platform with non-coherent DMA.
> Platforms with non-
> coherent DMA may also require a mechanism by which cache lines can be
> flushed and/or invalidated.
> Such mechanisms will be device-specific and/or standardized in a
> future extension to the ISA."
>
> The (current date) RISCV Platform Spec also says: "Memory accesses by
> I/O masters can be coherent or non-coherent with respect to all
> hart-related caches."
> which is brilliantly useless.
>
> so I think the best solution here is to:
>
> 1) Add a new PCD for platform DMA coherency, and test that on
> WriteBackDataCacheRange's ASSERT (if (!Coherent) ASSERT() else
> return;)
> 2) Add a more abstracting API that doesn't necessarily map to
> WriteBackDataCache when all we wanted was to assert DMA coherency
>
> but, alas, I've seen a lot less funky platforms than many of you, and
> DMA/cache-coherency is not really my thing, so I'll defer to others..
>
My preference is just remove the assertion and add the debug verbose
message instead of changing drivers/introduce new interfaces. It is a
nop in linux as well if CMO is not present.
Thanks,
Sunil
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113422): https://edk2.groups.io/g/devel/message/113422
Mute This Topic: https://groups.io/mt/103150435/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v10 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V
Hi Dhaval, I can understand a little bit why ASSERT is used，If we can determine that Riscv's BaseCacheMaintenanceLib needs to depend on the CMO instruction set, then RISCV processor platforms that do not have the CMO instruction set should not use this library. They should use BaseCacheMaintenanceLibNull or other libraries instead. In fact, like the DwEmmcDxe driver I use on VisionFive2, doing nothing when it comes to Cache management will have no effect. . . However, we may be able to add some DEBUG information before ASSERT to prompt developers to use the correct Cache management library. After all, many RISCV platforms use BaseCacheMaintenanceLib that previously only printed information that RISC-V unsupported function. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113417): https://edk2.groups.io/g/devel/message/113417 Mute This Topic: https://groups.io/mt/103150435/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v10 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V
On Mon, Jan 8, 2024 at 4:23 PM Dhaval Sharma wrote:
>
> Hi yangcheng/Pedro,
+CC a bunch of relevant people
Hi, (FYI you did not CC me)
Looking at yangcheng's example:
Status = PrepareDmaData (gpIdmacDesc, Length, Buffer); <-- We write
to the IDMAC desc
if (EFI_ERROR (Status)) {
goto out;
}
WriteBackDataCacheRange (gpIdmacDesc, DescPages * EFI_PAGE_SIZE);
<-- Make sure it's DMA-coherent
StartDma (Length); <-- We've flushed the cache, everything is now in
DRAM and DMA-coherent, start DMA
which screams of "bad abstractions" because you don't actually need to
write data back, if the device and platform are DMA coherent.
So what we want here really depends. My local "Volume I: RISC-V
Unprivileged ISA V20191213" says, section A.5:
"Table A.5 provides a mapping of Linux memory ordering macros onto
RISC-V memory instructions.
The Linux fences dma rmb() and dma wmb() map onto FENCE R,R and FENCE
W,W, respectively,
since the RISC-V Unix Platform requires coherent DMA, but would be
mapped onto FENCE RI,RI
and FENCE WO,WO, respectively, on a platform with non-coherent DMA.
Platforms with non-
coherent DMA may also require a mechanism by which cache lines can be
flushed and/or invalidated.
Such mechanisms will be device-specific and/or standardized in a
future extension to the ISA."
The (current date) RISCV Platform Spec also says: "Memory accesses by
I/O masters can be coherent or non-coherent with respect to all
hart-related caches."
which is brilliantly useless.
so I think the best solution here is to:
1) Add a new PCD for platform DMA coherency, and test that on
WriteBackDataCacheRange's ASSERT (if (!Coherent) ASSERT() else
return;)
2) Add a more abstracting API that doesn't necessarily map to
WriteBackDataCache when all we wanted was to assert DMA coherency
but, alas, I've seen a lot less funky platforms than many of you, and
DMA/cache-coherency is not really my thing, so I'll defer to others..
--
Pedro
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113412): https://edk2.groups.io/g/devel/message/113412
Mute This Topic: https://groups.io/mt/103150435/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v10 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V
Hi yangcheng/Pedro, Thanks for bringing this up. I understand the issue and probably we could just keep it simple with a warning instead of an assert. But wanted to mention a couple of points: 1. I think initially even in my patchset it was DEBUG message but there was a comment to turn it into Assert and I kind of agreed to it thinking that Assert is also typically ignored in release mode and comes into effect during debug build. 2. It might be okay to keep it that way because at least in debug mode it brings it to a developer's notice that the functionality he/she intends to call is not implemented by underlying layer. Whatever we decide, will be applicable to other places in this file as well. Pedro IMO, it would be better to have simple warnings instead of injecting NOPS as it at least notifies the user about actual underlying behavior. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113406): https://edk2.groups.io/g/devel/message/113406 Mute This Topic: https://groups.io/mt/103150435/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v10 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V
Hi, all
I`m yangcheng, and I found this bug when using the
*edk2-platforms/Silicon/Synopsys/DesignWare/Drivers/DwEmmcDxe* driver on my
VisionFive2 development board, which does not support the *CMO* instruction
set. There are many functions in *DwEmmcDxe.c* that call Cache operations. For
example, the *DwEmmcReadBlockData* function below calls
*WriteBackDataCacheRange* ().
EFI_STATUS
*DwEmmcReadBlockData* (
IN EFI_MMC_HOST_PROTOCOL *This,
IN EFI_LBA Lba,
IN UINTN Length,
IN UINT32* Buffer
)
{
...
*WriteBackDataCacheRange* (gpIdmacDesc, DescPages * EFI_PAGE_SIZE);
StartDma (Length);
Status = SendCommand (mDwEmmcCommand, mDwEmmcArgument);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "Failed to read data, mDwEmmcCommand:%x,
mDwEmmcArgument:%x, Status:%r\n", mDwEmmcCommand, mDwEmmcArgument, Status));
goto out;
}
out:
// Restore Tpl
gBS->RestoreTPL (Tpl);
return Status;
}
Initially, I didn't set *PcdRiscVFeatureOverride* , so I got an illegal
instruction exception. Then I set my *PcdRiscVFeatureOverride* to
*0xFFFE* to avoid using the *CMO* instruction set, and I got an
*ASSERT*.
Most cross-platform driver codes may call Cache management operations.
Modifying these driver codes may cost a lot, and I think we may need some
better way than ASSERT.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113404): https://edk2.groups.io/g/devel/message/113404
Mute This Topic: https://groups.io/mt/103150435/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v10 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V
Hi Dhaval,
On 12/13/23 15:59, 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
> Cc: Liming Gao
> Cc: Zhiguang Liu
> Cc: Laszlo Ersek
> Cc: Pedro Falcato
>
> Signed-off-by: Dhaval Sharma
> Acked-by: Laszlo Ersek
> Reviewed-by: Pedro Falcato
> ---
>
> Notes:
> V10:
> - Fix formatting to keep comments within 80
> - Replace RV with RISC-V
> - Fix an issue with multi line comments
> - Added assert to an unsupported function
> - Minor case modification in str in .uni
>
> V9:
> - Fixed an issue with Instruction cache invalidation. Use fence.i
> instruction as CMO does not support i-cache operations.
> V8:
> - Added note to convert PCD into RISC-V feature bitmap pointer
> - Modified function names to be more explicit about cache ops
> - Added RB tag
> 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| 177
>
> MdePkg/MdePkg.uni | 4 +
> 4 files changed, 166 insertions(+), 28 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*|0x0037
>
> +[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|0x|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 ac2a3c23a249..7c53a17abbb5 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.
> + Copyright (c) 2023, Rivos Inc. All rights reserved.
>
>SPDX-License-Identifier: BSD-2-Clause-Patent
> **/
> @@ -9,10 +10,116 @@
> #include
> #include
> #include
> +#include
> +
> +//
> +// TODO: Grab cache block size and make Cache Management Operation
> +// enabling decision based on RISC-V CPU HOB in
> +// future when it is available and convert PcdRiscVFeatureOverride
> +// PCD to a pointer that contains pointer to bitmap structure
> +// which can be operated more elegantly.
> +//
> +#define RISCV_CACHE_BLOCK_SIZE 64
> +#define RISCV_CPU_FEATURE_CMO_BITMASK 0x1
> +
> +typedef enum {
> + CacheOpClean,
> + CacheOpFlush,
> + CacheOpInvld,
> +} CACHE_OP;
> +
> +/**
> +Verify CBOs are supported by this HW
> +TODO: Use RISC-V CPU HOB once available.
> +
> +**/
> +STATIC
> +BOOLEAN
> +RiscVIsCMOEnabled (
> + VOID
> + )
> +{
> + // If CMO is disabled in HW, skip Override check
> + // Otherwise this PCD can override settings
> + return ((PcdGet64 (PcdRiscVFeatureOverride) &
> RISCV_CPU_FEATURE_CMO_BITMASK) != 0);
> +}
> +
> +/**
> + Performs required opeartion on cache lines in the cache coherency domain
> + of the calling CPU. If Address is not aligned on a cache line boundary,
> + then entire cache
Re: [edk2-devel] [PATCH v10 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V
Thanks. Just to clarify, In the earlier formatting
"InvalidateDataCacheRange:\
> + Zicbom not supported.\n" \
A missing " after *Range:\ was causing slightly skewed prints. After adding
this " it looks okay. So that is one change I had addressed.
But if keeping it in a single line works better please feel free to update.
And Thanks!
On Tue, Dec 19, 2023 at 12:59 PM Sunil V L wrote:
> On Wed, Dec 13, 2023 at 08:29:30PM +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
> > Cc: Liming Gao
> > Cc: Zhiguang Liu
> > Cc: Laszlo Ersek
> > Cc: Pedro Falcato
> >
> > Signed-off-by: Dhaval Sharma
> > Acked-by: Laszlo Ersek
> > Reviewed-by: Pedro Falcato
> > ---
> >
> > Notes:
> > V10:
> > - Fix formatting to keep comments within 80
> > - Replace RV with RISC-V
> > - Fix an issue with multi line comments
> > - Added assert to an unsupported function
> > - Minor case modification in str in .uni
> >
> > V9:
> > - Fixed an issue with Instruction cache invalidation. Use fence.i
> > instruction as CMO does not support i-cache operations.
> > V8:
> > - Added note to convert PCD into RISC-V feature bitmap pointer
> > - Modified function names to be more explicit about cache ops
> > - Added RB tag
> > 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|
> 177
> > MdePkg/MdePkg.uni |
> 4 +
> > 4 files changed, 166 insertions(+), 28 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*|0x0037
> >
> > +[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|0x|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 ac2a3c23a249..7c53a17abbb5 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.
> > + Copyright (c) 2023, Rivos Inc. All rights reserved.
> >
> >SPDX-License-Identifier: BSD-2-Clause-Patent
> > **/
> > @@ -9,10 +10,116 @@
> > #include
> > #include
> > #include
> > +#include
> > +
> > +//
> > +// TODO: Grab cache block size and make Cache Management Operation
> > +// enabling decision based on RISC-V CPU HOB in
> > +// future when it is available and convert PcdRiscVFeatureOverride
> > +// PCD to a pointer that contains pointer to bitmap structure
> > +// which can be operated more elegantly.
> > +//
> > +#define RISCV_CACHE_BLOCK_SIZE 64
> > +#define RISCV_CPU_FEATURE_CMO_BITMASK 0x1
> > +
> > +typedef enum {
> > + CacheOpClean,
> > +
Re: [edk2-devel] [PATCH v10 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V
On Wed, Dec 13, 2023 at 08:29:30PM +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
> Cc: Liming Gao
> Cc: Zhiguang Liu
> Cc: Laszlo Ersek
> Cc: Pedro Falcato
>
> Signed-off-by: Dhaval Sharma
> Acked-by: Laszlo Ersek
> Reviewed-by: Pedro Falcato
> ---
>
> Notes:
> V10:
> - Fix formatting to keep comments within 80
> - Replace RV with RISC-V
> - Fix an issue with multi line comments
> - Added assert to an unsupported function
> - Minor case modification in str in .uni
>
> V9:
> - Fixed an issue with Instruction cache invalidation. Use fence.i
> instruction as CMO does not support i-cache operations.
> V8:
> - Added note to convert PCD into RISC-V feature bitmap pointer
> - Modified function names to be more explicit about cache ops
> - Added RB tag
> 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| 177
>
> MdePkg/MdePkg.uni | 4 +
> 4 files changed, 166 insertions(+), 28 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*|0x0037
>
> +[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|0x|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 ac2a3c23a249..7c53a17abbb5 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.
> + Copyright (c) 2023, Rivos Inc. All rights reserved.
>
>SPDX-License-Identifier: BSD-2-Clause-Patent
> **/
> @@ -9,10 +10,116 @@
> #include
> #include
> #include
> +#include
> +
> +//
> +// TODO: Grab cache block size and make Cache Management Operation
> +// enabling decision based on RISC-V CPU HOB in
> +// future when it is available and convert PcdRiscVFeatureOverride
> +// PCD to a pointer that contains pointer to bitmap structure
> +// which can be operated more elegantly.
> +//
> +#define RISCV_CACHE_BLOCK_SIZE 64
> +#define RISCV_CPU_FEATURE_CMO_BITMASK 0x1
> +
> +typedef enum {
> + CacheOpClean,
> + CacheOpFlush,
> + CacheOpInvld,
> +} CACHE_OP;
> +
> +/**
> +Verify CBOs are supported by this HW
> +TODO: Use RISC-V CPU HOB once available.
> +
> +**/
> +STATIC
> +BOOLEAN
> +RiscVIsCMOEnabled (
> + VOID
> + )
> +{
> + // If CMO is disabled in HW, skip Override check
> + // Otherwise this PCD can override settings
> + return ((PcdGet64 (PcdRiscVFeatureOverride) &
> RISCV_CPU_FEATURE_CMO_BITMASK) != 0);
> +}
> +
> +/**
> + Performs required opeartion on cache lines in the cache coherency domain
> + of the calling CPU. If Address is not aligned on a cache line boundary,
> + then
[edk2-devel] [PATCH v10 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V
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
Cc: Liming Gao
Cc: Zhiguang Liu
Cc: Laszlo Ersek
Cc: Pedro Falcato
Signed-off-by: Dhaval Sharma
Acked-by: Laszlo Ersek
Reviewed-by: Pedro Falcato
---
Notes:
V10:
- Fix formatting to keep comments within 80
- Replace RV with RISC-V
- Fix an issue with multi line comments
- Added assert to an unsupported function
- Minor case modification in str in .uni
V9:
- Fixed an issue with Instruction cache invalidation. Use fence.i
instruction as CMO does not support i-cache operations.
V8:
- Added note to convert PCD into RISC-V feature bitmap pointer
- Modified function names to be more explicit about cache ops
- Added RB tag
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| 177
MdePkg/MdePkg.uni | 4 +
4 files changed, 166 insertions(+), 28 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*|0x0037
+[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|0x|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 ac2a3c23a249..7c53a17abbb5 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.
+ Copyright (c) 2023, Rivos Inc. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -9,10 +10,116 @@
#include
#include
#include
+#include
+
+//
+// TODO: Grab cache block size and make Cache Management Operation
+// enabling decision based on RISC-V CPU HOB in
+// future when it is available and convert PcdRiscVFeatureOverride
+// PCD to a pointer that contains pointer to bitmap structure
+// which can be operated more elegantly.
+//
+#define RISCV_CACHE_BLOCK_SIZE 64
+#define RISCV_CPU_FEATURE_CMO_BITMASK 0x1
+
+typedef enum {
+ CacheOpClean,
+ CacheOpFlush,
+ CacheOpInvld,
+} CACHE_OP;
+
+/**
+Verify CBOs are supported by this HW
+TODO: Use RISC-V CPU HOB once available.
+
+**/
+STATIC
+BOOLEAN
+RiscVIsCMOEnabled (
+ VOID
+ )
+{
+ // If CMO is disabled in HW, skip Override check
+ // Otherwise this PCD can override settings
+ return ((PcdGet64 (PcdRiscVFeatureOverride) & RISCV_CPU_FEATURE_CMO_BITMASK)
!= 0);
+}
+
+/**
+ Performs required opeartion on cache lines in the cache coherency domain
+ of the calling CPU. If Address is not aligned on a cache line boundary,
+ then entire cache line containing Address is operated. If Address + Length
+ is not aligned on a cache line boundary, then the entire cache line
+ containing Address + Length -1 is operated.
+ If Length is greater than (MAX_ADDRESS - Address + 1), then ASSERT().
+ @param Address The base address of the cache lines to
+ invalidate.
+
