Re: [RFC PATCH V2 06/19] MdePkg/BaseLib: RISC-V: Add generic CPU related functions

["Sunil V L" <sunilvl@ventanamicro.com>]

On Thu, Dec 08, 2022 at 02:43:56AM +0100, Heinrich Schuchardt wrote:
> 
> 
> On 9/7/22 13:36, Sunil V L wrote:
> > EDK2 in S-mode needs to use SSCRATCH register. Implement functions
> > to set/get the SSCRATCH register.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >   MdePkg/Library/BaseLib/BaseLib.inf      |  1 +
> >   MdePkg/Include/Library/BaseLib.h        | 10 ++++++
> >   MdePkg/Library/BaseLib/RiscV64/CpuGen.S | 33 ++++++++++++++++++++
> >   3 files changed, 44 insertions(+)
> > 
> > diff --git a/MdePkg/Library/BaseLib/BaseLib.inf b/MdePkg/Library/BaseLib/BaseLib.inf
> > index 6be5be9428f2..5429329e39b0 100644
> > --- a/MdePkg/Library/BaseLib/BaseLib.inf
> > +++ b/MdePkg/Library/BaseLib/BaseLib.inf
> > @@ -401,6 +401,7 @@ [Sources.RISCV64]
> >     RiscV64/RiscVCpuPause.S           | GCC
> >     RiscV64/RiscVInterrupt.S          | GCC
> >     RiscV64/FlushCache.S              | GCC
> > +  RiscV64/CpuGen.S                  | GCC
> >   [Packages]
> >     MdePkg/MdePkg.dec
> > diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
> > index a6f9a194ef1c..a742de61a442 100644
> > --- a/MdePkg/Include/Library/BaseLib.h
> > +++ b/MdePkg/Include/Library/BaseLib.h
> > @@ -150,6 +150,16 @@ typedef struct {
> >   #define BASE_LIBRARY_JUMP_BUFFER_ALIGNMENT  8
> > +VOID
> > +RiscVSetSupervisorScratch (
> > +  UINT64
> > +  );
> > +
> > +UINT64
> > +RiscVGetSupervisorScratch (
> > +  VOID
> > +  );
> > +
> >   #endif // defined (MDE_CPU_RISCV64)
> >   //
> > diff --git a/MdePkg/Library/BaseLib/RiscV64/CpuGen.S b/MdePkg/Library/BaseLib/RiscV64/CpuGen.S
> > new file mode 100644
> > index 000000000000..d11929cf3233
> > --- /dev/null
> > +++ b/MdePkg/Library/BaseLib/RiscV64/CpuGen.S
> > @@ -0,0 +1,33 @@
> > +//------------------------------------------------------------------------------
> > +//
> > +// Generic CPU related functions for RISC-V
> > +//
> > +// Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> > +// Copyright (c) 2022, Ventana Micro Systems Inc. All rights reserved.<BR>
> > +//
> > +// SPDX-License-Identifier: BSD-2-Clause-Patent
> > +//
> > +//------------------------------------------------------------------------------
> > +
> > +#include <Register/RiscV64/RiscVAsm.h>
> > +#include <Register/RiscV64/RiscVImpl.h>
> > +
> > +.data
> > +.align 3
> > +.section .text
> > +
> > +//
> > +// Set Supervisor mode scratch.
> > +// @param a0 : Value set to Supervisor mode scratch
> > +//
> > +ASM_FUNC (RiscVSetSupervisorScratch)
> > +    csrrw a1, CSR_SSCRATCH, a0
> > +    ret
> 
> I am wondering why you are reading the old value into a1 while the function
> has a VOID return value.
> 
> If you are not interested in reading the old value, the "RISC-V Unprivileged
> ISA Specification" suggests to use the x0 (zero) register as read register
> or just use the CSRW pseudo code.
> 

Hi Heinrich,

Many thanks!. I agree. Will fix this and your inputs for other patches.

BTW, https://edk2.groups.io/g/devel/message/95726 was the latest series
after couple of design discussions. But your comments are still valid.
Will update this series and send the next version.

Thanks!
Sunil