Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v1 02/22]: RiscVPkg/Include: Add header files of RISC-V CPU package

["Abner Chang" <abner.chang@hpe.com>]

> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Tuesday, September 17, 2019 9:54 PM
> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>
> Cc: devel@edk2.groups.io
> Subject: Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v1 02/22]:
> RiscVPkg/Include: Add header files of RISC-V CPU package
> 
> On Mon, Sep 16, 2019 at 04:02:10AM +0000, Chang, Abner (HPS SW/FW
> Technologist) wrote:
> > > -----Original Message-----
> > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf
> > > Of Leif Lindholm
> > > Sent: Thursday, September 5, 2019 2:56 AM
> > > To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
> > > <abner.chang@hpe.com>
> > > Subject: Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v1 02/22]:
> > > RiscVPkg/Include: Add header files of RISC-V CPU package
> > >
> > > On Wed, Sep 04, 2019 at 06:42:57PM +0800, Abner Chang wrote:
> > > > RISC-V package library definitions.
> > > >
> > > > RiscV.h
> > > > -Add RiscV.h which conform with RISC-V Privilege Spec v1.10.
> > > >
> > > > sbi.h
> > > > sbi_bits.h
> > > > sbi_types.h
> > > > - Add definitions for RISC-V OpenSBI EDK2 port.
> > >
> > > A web search suggests this refers to the RISC-V Open Source
> > > Supervisor Binary Interface. It would be helpful to expand it on first use.
> > > https://github.com/riscv/opensbi/?
> > > Is this expected to fluctuate much?
> >
> > Yes it does change often, the community keeps adding new features to
> openSBI.
> 
> OK. I got some more intro to this at Linux Plumbers Conference last week.
> 
> > > I ask for two reasons:
> > > 1) Because if it is not, I would much prefer to see the
> > >    files/directories renamed to conform the the coding style.
> > >    If it is, I would like for us to consider implementing this as a
> > >    git submodule instead.
> >
> > Yes. Please use submodule. Don't touch the open source from openSBI to
> avoid maintenance effort to edk2.
> 
> Sounds good.
> 
> ...
> 
> > > > diff --git a/RiscVPkg/Include/sbi/sbi_bits.h
> > > b/RiscVPkg/Include/sbi/sbi_bits.h
> > > > new file mode 100644
> > > > index 0000000..4116ee6
> > > > --- /dev/null
> > > > +++ b/RiscVPkg/Include/sbi/sbi_bits.h
> > > > @@ -0,0 +1,23 @@
> > > > +/** @file
> > > > +  RISC-V OpesbSBI header file reference.
> > > > +
> > > > +  Copyright (c) 2019, Hewlett Packard Enterprise Development LP.
> > > > + All rights
> > > reserved.<BR>
> > > > +
> > > > +  This program and the accompanying materials  are licensed and
> > > > + made available under the terms and conditions of the
> > > BSD License
> > > > +  which accompanies this distribution.  The full text of the
> > > > + license may be
> > > found at
> > > > +  INVALID URI REMOVED
> > > 3A__opensource.org_licenses_bsd-
> > >
> 2Dlicense.php&d=DwIBAg&c=C5b8zRQO1miGmBeVZ2LFWg&r=_SN6FZBN4V
> > >
> gi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=iwfkW8MQjzEkixp0gv3xsvh20ei
> > >
> odo7hGcTLXEL_I0o&s=mLKjYgrdQ6MuAN9UVYQeCDB0pNA44m9yBOylxW-
> > > Koiw&e=
> > > > +
> > > > +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS
> IS"
> > > BASIS,
> > > > +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> > > EXPRESS OR IMPLIED.
> > > > +
> > > > +**/
> > > > +#ifndef __EDK2_SBI_BITS_H__
> > > > +#define __EDK2_SBI_BITS_H__
> > > > +
> > > > +#undef MAX
> > > > +#undef MIN
> > >
> > > Why?
> > OpebSBI sbi_bits.h has its own MAX/MIN definitions which are
> > duplicated with edk2 ones. OpenSBI is the implementation of  RISC-V
> > sbi spec which is similar to edk2 for UEFI, the duplicate macros are
> > expected. This is the wrapper file to OpenSBI because of we don't want
> > to touch OpenSBI code.
> 
> I think we should look at refactoring this in OpenSBI instead.
> Especially with us using this as effectively a library, we would need to be
> actively monitoring (well, on every update, but you suggested they may be
> frequent) whether any new clashes developed.
> 
> The guys who attended Plumbers suggested thy would be quite flexible to
> restructure code in ways that makes the project more consumable.
> 
> I am OK with this being here while it is on the edk2-staging branch.

I am happy to see OpenSbi could be more flexible to adopt different firmware code bases.
But it takes time to revise OpebSbi and I don't want to keep RISC-V edk2 port in edk2-staing for a long time just for waiting the refactoring. That's painful to me when merge/syncup RISC-V edk2 port with edk2 repo while edk2 is changed often as well.
I would like to get RISC-V edk2 works in edk2 repo first and revise it once OpenSbi has refactored.

RISC-V edk2 has been staying in edk2-staing since 2016...

> 
> > >
> > > > +
> > > > +#include "../opensbi/include/sbi/sbi_bits.h"
> > >
> > > No relative includes. Let's figure out a way to expose the interface
> properly.
> >
> > Can be fixed by RiscVPkg.dec
> 
> Sounds good.
> 
> > > > +
> > > > +#endif
> > > > \ No newline at end of file
> > > > diff --git a/RiscVPkg/Include/sbi/sbi_types.h
> > > b/RiscVPkg/Include/sbi/sbi_types.h
> > > > new file mode 100644
> > > > index 0000000..fe877f2
> > > > --- /dev/null
> > > > +++ b/RiscVPkg/Include/sbi/sbi_types.h
> > > > @@ -0,0 +1,24 @@
> > > > +/** @file
> > > > +  RISC-V OpesbSBI header file reference.
> > > > +
> > > > +  Copyright (c) 2019, Hewlett Packard Enterprise Development LP.
> > > > + All rights
> > > reserved.<BR>
> > > > +
> > > > +  This program and the accompanying materials  are licensed and
> > > > + made available under the terms and conditions of the
> > > BSD License
> > > > +  which accompanies this distribution.  The full text of the
> > > > + license may be
> > > found at
> > > > +  INVALID URI REMOVED
> > > 3A__opensource.org_licenses_bsd-
> > >
> 2Dlicense.php&d=DwIBAg&c=C5b8zRQO1miGmBeVZ2LFWg&r=_SN6FZBN4V
> > >
> gi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=iwfkW8MQjzEkixp0gv3xsvh20ei
> > >
> odo7hGcTLXEL_I0o&s=mLKjYgrdQ6MuAN9UVYQeCDB0pNA44m9yBOylxW-
> > > Koiw&e=
> > > > +
> > > > +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS
> IS"
> > > BASIS,
> > > > +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> > > EXPRESS OR IMPLIED.
> > > > +
> > > > +**/
> > > > +#ifndef __EDK2_SBI_TYPES_H__
> > > > +#define __EDK2_SBI_TYPES_H__
> > > > +
> > > > +#undef TRUE
> > > > +#undef FALSE
> > > > +#undef NULL
> > >
> > > Why?
> > Same reason as above.
> 
> OK, same response as above.
> 
> > > > +
> > > > +#include "../opensbi/include/sbi/sbi_types.h"
> > >
> > > No relative includes.
> > Can be fixed by RiscVPkg.dec
> 
> OK.
> 
> /
>     Leif