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


Abner Chang
 

-----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

Join devel@edk2.groups.io to automatically receive all group messages.