Re: [edk2-devel] [PATCH v9 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V
On Mon, Dec 4, 2023 at 8:30 AM 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
>
> Signed-off-by: Dhaval Sharma
> Acked-by: Laszlo Ersek
> ---
>
> Notes:
> 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| 173
>
> MdePkg/MdePkg.uni | 4 +
> 4 files changed, 160 insertions(+), 30 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..cacc38eff4f4 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,117 @@
> #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
Re: [edk2-devel] [PATCH v9 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V
On Mon, Dec 11, 2023 at 3:20 PM Sunil V L wrote: > > On Mon, Dec 11, 2023 at 03:09:19PM +, Pedro Falcato wrote: > > On Mon, Dec 11, 2023 at 1:12 PM Sunil V L wrote: > > > > > > On Sun, Dec 10, 2023 at 07:51:12PM +0530, Dhaval Sharma wrote: > > [...] > > > > nit: Can we pick a log style here? Like : > > > > In this case, "CacheOpCacheRange: Performing ...". It's just prettier > > > > and more greppable. > > > > My interpretation of this was removing __func__ and instead having some > > > > relevant text would make it more searchable. > > > > And it kind of did make sense to me. I know many places __func__ is used > > > > but this is just a perspective. > > > > > > > I think the comment meant to follow a standard logging format since > > > there was no ":" and a space in original change. I prefer __func__ over > > > this so that we don't need to update multiple lines in case function > > > name gets changed. > > > > I definitely meant that __func__ should not be used for this as well. > > You can't really search for an error message if you're doing > > gratuitous printf formatting for no reason. > > Linux even has a policy where user-facing strings (i.e logs) cannot > > get broken up, even if you run out of line width. > > > Thanks Pedro. Do you mean __func__ should not be used at all in any > of logging? Or is there a case where it is allowed vs not allowed? My point is that we should aid people trying to do git grep FWIW, Linux itself uses __func__ a bunch for logs. But I personally dislike it, for the reasons stated above. -- Pedro -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112309): https://edk2.groups.io/g/devel/message/112309 Mute This Topic: https://groups.io/mt/102967058/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v9 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V
On Mon, Dec 11, 2023 at 03:09:19PM +, Pedro Falcato wrote: > On Mon, Dec 11, 2023 at 1:12 PM Sunil V L wrote: > > > > On Sun, Dec 10, 2023 at 07:51:12PM +0530, Dhaval Sharma wrote: > [...] > > > nit: Can we pick a log style here? Like : > > > In this case, "CacheOpCacheRange: Performing ...". It's just prettier > > > and more greppable. > > > My interpretation of this was removing __func__ and instead having some > > > relevant text would make it more searchable. > > > And it kind of did make sense to me. I know many places __func__ is used > > > but this is just a perspective. > > > > > I think the comment meant to follow a standard logging format since > > there was no ":" and a space in original change. I prefer __func__ over > > this so that we don't need to update multiple lines in case function > > name gets changed. > > I definitely meant that __func__ should not be used for this as well. > You can't really search for an error message if you're doing > gratuitous printf formatting for no reason. > Linux even has a policy where user-facing strings (i.e logs) cannot > get broken up, even if you run out of line width. > Thanks Pedro. Do you mean __func__ should not be used at all in any of logging? Or is there a case where it is allowed vs not allowed? Thanks, Sunil -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112307): https://edk2.groups.io/g/devel/message/112307 Mute This Topic: https://groups.io/mt/102967058/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v9 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V
On Mon, Dec 11, 2023 at 1:12 PM Sunil V L wrote: > > On Sun, Dec 10, 2023 at 07:51:12PM +0530, Dhaval Sharma wrote: [...] > > nit: Can we pick a log style here? Like : > > In this case, "CacheOpCacheRange: Performing ...". It's just prettier > > and more greppable. > > My interpretation of this was removing __func__ and instead having some > > relevant text would make it more searchable. > > And it kind of did make sense to me. I know many places __func__ is used > > but this is just a perspective. > > > I think the comment meant to follow a standard logging format since > there was no ":" and a space in original change. I prefer __func__ over > this so that we don't need to update multiple lines in case function > name gets changed. I definitely meant that __func__ should not be used for this as well. You can't really search for an error message if you're doing gratuitous printf formatting for no reason. Linux even has a policy where user-facing strings (i.e logs) cannot get broken up, even if you run out of line width. PS: Dhaval, I gave you a bunch of feedback and you dropped me from CCs. Please don't do that, I completely lost track of this patch set :/ -- Pedro -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112305): https://edk2.groups.io/g/devel/message/112305 Mute This Topic: https://groups.io/mt/102967058/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v9 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V
On Sun, Dec 10, 2023 at 07:51:12PM +0530, Dhaval Sharma wrote:
> Thanks for the review. My comments inline:
>
> On Fri, Dec 8, 2023 at 9:58 AM Sunil V L wrote:
>
> > On Thu, Dec 07, 2023 at 10:31:48AM +0530, Dhaval Sharma wrote:
> > > Comments inline:
> > >
> > >
> > > On Wed, Dec 6, 2023 at 7:50 PM Sunil V L
> > wrote:
> > >
> > > > Hi Dhaval,
> > > >
> > > > Thank you very much for fixing the issue with instruction cache
> > > > invalidation and confirming with the spec owner. Few minor comments
> > > > below.
> > > >
> > > > On Mon, Dec 04, 2023 at 01:59:49PM +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
> > > > >
> > > > > Signed-off-by: Dhaval Sharma
> > > > > Acked-by: Laszlo Ersek
> > > > > ---
> > > > >
> > > > > Notes:
> > > > > 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
> > > > >
> > > > IMO, it is better to keep the change log in the cover letter. Since not
> > > > all patches may be CC'd to every one apart from the cover letter, it is
> > > > difficult to understand from the cover letter what has changed in the
> > new
> > > > series.
> > > >
> > > [Dhaval] AFAIU notes are tied to specific commits. But it makes sense, I
> > > can add an update to the cover letter.
> > >
> > > >
> > > > > MdePkg/MdePkg.dec |
> > > > 8 +
> > > > > MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf |
> > > > 5 +
> > > > > MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c|
> > > > 173
> > > > > MdePkg/MdePkg.uni |
> > > > 4 +
> > > > > 4 files changed, 160 insertions(+), 30 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..cacc38eff4f4 100644
> > > > > --- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> > > > > +++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> > > > > @@ -2,6 +2,7 @@
> > > > >RISC-V specific functionality for cache.
Re: [edk2-devel] [PATCH v9 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V
Thanks for the review. My comments inline:
On Fri, Dec 8, 2023 at 9:58 AM Sunil V L wrote:
> On Thu, Dec 07, 2023 at 10:31:48AM +0530, Dhaval Sharma wrote:
> > Comments inline:
> >
> >
> > On Wed, Dec 6, 2023 at 7:50 PM Sunil V L
> wrote:
> >
> > > Hi Dhaval,
> > >
> > > Thank you very much for fixing the issue with instruction cache
> > > invalidation and confirming with the spec owner. Few minor comments
> > > below.
> > >
> > > On Mon, Dec 04, 2023 at 01:59:49PM +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
> > > >
> > > > Signed-off-by: Dhaval Sharma
> > > > Acked-by: Laszlo Ersek
> > > > ---
> > > >
> > > > Notes:
> > > > 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
> > > >
> > > IMO, it is better to keep the change log in the cover letter. Since not
> > > all patches may be CC'd to every one apart from the cover letter, it is
> > > difficult to understand from the cover letter what has changed in the
> new
> > > series.
> > >
> > [Dhaval] AFAIU notes are tied to specific commits. But it makes sense, I
> > can add an update to the cover letter.
> >
> > >
> > > > MdePkg/MdePkg.dec |
> > > 8 +
> > > > MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf |
> > > 5 +
> > > > MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c|
> > > 173
> > > > MdePkg/MdePkg.uni |
> > > 4 +
> > > > 4 files changed, 160 insertions(+), 30 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..cacc38eff4f4 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,117 @@
> > > > #include
> > > >
Re: [edk2-devel] [PATCH v9 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V
On Thu, Dec 07, 2023 at 10:31:48AM +0530, Dhaval Sharma wrote:
> Comments inline:
>
>
> On Wed, Dec 6, 2023 at 7:50 PM Sunil V L wrote:
>
> > Hi Dhaval,
> >
> > Thank you very much for fixing the issue with instruction cache
> > invalidation and confirming with the spec owner. Few minor comments
> > below.
> >
> > On Mon, Dec 04, 2023 at 01:59:49PM +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
> > >
> > > Signed-off-by: Dhaval Sharma
> > > Acked-by: Laszlo Ersek
> > > ---
> > >
> > > Notes:
> > > 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
> > >
> > IMO, it is better to keep the change log in the cover letter. Since not
> > all patches may be CC'd to every one apart from the cover letter, it is
> > difficult to understand from the cover letter what has changed in the new
> > series.
> >
> [Dhaval] AFAIU notes are tied to specific commits. But it makes sense, I
> can add an update to the cover letter.
>
> >
> > > MdePkg/MdePkg.dec |
> > 8 +
> > > MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf |
> > 5 +
> > > MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c|
> > 173
> > > MdePkg/MdePkg.uni |
> > 4 +
> > > 4 files changed, 160 insertions(+), 30 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..cacc38eff4f4 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,117 @@
> > > #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
> > > +//
Re: [edk2-devel] [PATCH v9 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V
Comments inline:
On Wed, Dec 6, 2023 at 7:50 PM Sunil V L wrote:
> Hi Dhaval,
>
> Thank you very much for fixing the issue with instruction cache
> invalidation and confirming with the spec owner. Few minor comments
> below.
>
> On Mon, Dec 04, 2023 at 01:59:49PM +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
> >
> > Signed-off-by: Dhaval Sharma
> > Acked-by: Laszlo Ersek
> > ---
> >
> > Notes:
> > 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
> >
> IMO, it is better to keep the change log in the cover letter. Since not
> all patches may be CC'd to every one apart from the cover letter, it is
> difficult to understand from the cover letter what has changed in the new
> series.
>
[Dhaval] AFAIU notes are tied to specific commits. But it makes sense, I
can add an update to the cover letter.
>
> > MdePkg/MdePkg.dec |
> 8 +
> > MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf |
> 5 +
> > MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c|
> 173
> > MdePkg/MdePkg.uni |
> 4 +
> > 4 files changed, 160 insertions(+), 30 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..cacc38eff4f4 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,117 @@
> > #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
> Can we make this also as a PCD?
> [Dhaval] Actually this define should go away once we have CPU HOB. So
> thought to keep it simple for now. Anyways there is no plan to change this
> size
>
anytime in the near
Re: [edk2-devel] [PATCH v9 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V
Hi Dhaval,
Thank you very much for fixing the issue with instruction cache
invalidation and confirming with the spec owner. Few minor comments
below.
On Mon, Dec 04, 2023 at 01:59:49PM +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
>
> Signed-off-by: Dhaval Sharma
> Acked-by: Laszlo Ersek
> ---
>
> Notes:
> 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
>
IMO, it is better to keep the change log in the cover letter. Since not
all patches may be CC'd to every one apart from the cover letter, it is
difficult to understand from the cover letter what has changed in the new
series.
> MdePkg/MdePkg.dec | 8 +
> MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf | 5 +
> MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c| 173
>
> MdePkg/MdePkg.uni | 4 +
> 4 files changed, 160 insertions(+), 30 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..cacc38eff4f4 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,117 @@
> #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
Can we make this also as a PCD?
> +#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
[edk2-devel] [PATCH v9 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
Signed-off-by: Dhaval Sharma
Acked-by: Laszlo Ersek
---
Notes:
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| 173
MdePkg/MdePkg.uni | 4 +
4 files changed, 160 insertions(+), 30 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..cacc38eff4f4 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,117 @@
#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.
+ @param Length The number of bytes to invalidate from the instruction
+ cache.
+ @param Op Type of CMO operation to be performed
+ @return Address.
+
+**/
+STATIC
+VOID
+CacheOpCacheRange (
+ IN VOID *Address,
+ IN UINTN Length,
+ IN CACHE_OP
