Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v1 04/22]: MdePkg/Include: RISC-V definitions.
On Mon, Sep 16, 2019 at 05:31:40AM +, Chang, Abner (HPS SW/FW Technologist)
wrote:
> > -Original Message-
> > From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> > Sent: Thursday, September 5, 2019 4:40 AM
> > To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
> >
> > Subject: Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v1 04/22]:
> > MdePkg/Include: RISC-V definitions.
> >
> > On Wed, Sep 04, 2019 at 06:42:59PM +0800, Abner Chang wrote:
> > > Add RISC-V processor related definitions.
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.0
> > > Signed-off-by: Abner Chang
> > > ---
> > > MdePkg/Include/IndustryStandard/PeImage.h | 14 +-
> > > MdePkg/Include/Library/BaseLib.h | 67 ++
> > > MdePkg/Include/Protocol/DebugSupport.h| 55 +
> > > MdePkg/Include/Protocol/PxeBaseCode.h | 8 +
> > > MdePkg/Include/RiscV64/ProcessorBind.h| 336
> > ++
> > > MdePkg/Include/Uefi/UefiBaseType.h| 25 +++
> > > MdePkg/Include/Uefi/UefiSpec.h| 11 +
> > > 7 files changed, 513 insertions(+), 3 deletions(-) create mode
> > > 100644 MdePkg/Include/RiscV64/ProcessorBind.h
> > >
> > > diff --git a/MdePkg/Include/IndustryStandard/PeImage.h
> > > b/MdePkg/Include/IndustryStandard/PeImage.h
> > > index 720bb08..47796b2 100644
> > > --- a/MdePkg/Include/IndustryStandard/PeImage.h
> > > +++ b/MdePkg/Include/IndustryStandard/PeImage.h
> > > @@ -9,6 +9,8 @@
> > >
> > > Copyright (c) 2006 - 2018, Intel Corporation. All rights
> > > reserved. Portions copyright (c) 2008 - 2009, Apple Inc. All
> > > rights reserved.
> > > +Portions Copyright (c) 2016, Hewlett Packard Enterprise Development
> > > +LP. All rights reserved.
> > > +
> > > SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > > **/
> > > @@ -34,6 +36,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > > #define IMAGE_FILE_MACHINE_X64 0x8664
> > > #define IMAGE_FILE_MACHINE_ARMTHUMB_MIXED 0x01c2
> > > #define IMAGE_FILE_MACHINE_ARM64 0xAA64
> > > +#define IMAGE_FILE_MACHINE_RISCV32 0x5032
> > > +#define IMAGE_FILE_MACHINE_RISCV64 0x5064
> > > +#define IMAGE_FILE_MACHINE_RISCV1280x5128
> > >
> > > //
> > > // EXE file formats
> > > @@ -478,9 +483,9 @@ typedef struct {
> > > ///
> > > #define EFI_IMAGE_SIZEOF_BASE_RELOCATION 8
> > >
> > > -//
> > > -// Based relocation types.
> > > -//
> > > +///
> > > +/// Based relocation types.
> > > +///
> >
> > I don't know if this change to the comment block is a wonky rebase or
> > whatever, but please drop it.
>
> No, I revised it to three back slash because most of comment block
> use three back slash.
It is always appreciated when people provide style fixes, but they
should be provided as separate patches.
In this case I don't see the value in doing that however, since // is
the comment format specified by the coding standars.
A separate patch fixing the existing incorrect ones would be
preferable (but not important).
Best Regards,
Leif
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#47372): https://edk2.groups.io/g/devel/message/47372
Mute This Topic: https://groups.io/mt/33137122/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v1 04/22]: MdePkg/Include: RISC-V definitions.
> -Original Message-
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> Sent: Thursday, September 5, 2019 4:40 AM
> To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
>
> Subject: Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v1 04/22]:
> MdePkg/Include: RISC-V definitions.
>
> On Wed, Sep 04, 2019 at 06:42:59PM +0800, Abner Chang wrote:
> > Add RISC-V processor related definitions.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Abner Chang
> > ---
> > MdePkg/Include/IndustryStandard/PeImage.h | 14 +-
> > MdePkg/Include/Library/BaseLib.h | 67 ++
> > MdePkg/Include/Protocol/DebugSupport.h| 55 +
> > MdePkg/Include/Protocol/PxeBaseCode.h | 8 +
> > MdePkg/Include/RiscV64/ProcessorBind.h| 336
> ++
> > MdePkg/Include/Uefi/UefiBaseType.h| 25 +++
> > MdePkg/Include/Uefi/UefiSpec.h| 11 +
> > 7 files changed, 513 insertions(+), 3 deletions(-) create mode
> > 100644 MdePkg/Include/RiscV64/ProcessorBind.h
> >
> > diff --git a/MdePkg/Include/IndustryStandard/PeImage.h
> > b/MdePkg/Include/IndustryStandard/PeImage.h
> > index 720bb08..47796b2 100644
> > --- a/MdePkg/Include/IndustryStandard/PeImage.h
> > +++ b/MdePkg/Include/IndustryStandard/PeImage.h
> > @@ -9,6 +9,8 @@
> >
> > Copyright (c) 2006 - 2018, Intel Corporation. All rights
> > reserved. Portions copyright (c) 2008 - 2009, Apple Inc. All
> > rights reserved.
> > +Portions Copyright (c) 2016, Hewlett Packard Enterprise Development
> > +LP. All rights reserved.
> > +
> > SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > **/
> > @@ -34,6 +36,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > #define IMAGE_FILE_MACHINE_X64 0x8664
> > #define IMAGE_FILE_MACHINE_ARMTHUMB_MIXED 0x01c2
> > #define IMAGE_FILE_MACHINE_ARM64 0xAA64
> > +#define IMAGE_FILE_MACHINE_RISCV32 0x5032
> > +#define IMAGE_FILE_MACHINE_RISCV64 0x5064
> > +#define IMAGE_FILE_MACHINE_RISCV1280x5128
> >
> > //
> > // EXE file formats
> > @@ -478,9 +483,9 @@ typedef struct {
> > ///
> > #define EFI_IMAGE_SIZEOF_BASE_RELOCATION 8
> >
> > -//
> > -// Based relocation types.
> > -//
> > +///
> > +/// Based relocation types.
> > +///
>
> I don't know if this change to the comment block is a wonky rebase or
> whatever, but please drop it.
>
> > #define EFI_IMAGE_REL_BASED_ABSOLUTE0
> > #define EFI_IMAGE_REL_BASED_HIGH1
> > #define EFI_IMAGE_REL_BASED_LOW 2
> > @@ -488,7 +493,10 @@ typedef struct {
> > #define EFI_IMAGE_REL_BASED_HIGHADJ 4
> > #define EFI_IMAGE_REL_BASED_MIPS_JMPADDR5
> > #define EFI_IMAGE_REL_BASED_ARM_MOV32A 5
> > +#define EFI_IMAGE_REL_BASED_RISCV_HI20 5
> > #define EFI_IMAGE_REL_BASED_ARM_MOV32T 7
> > +#define EFI_IMAGE_REL_BASED_RISCV_LOW12I7
> > +#define EFI_IMAGE_REL_BASED_RISCV_LOW12S8
>
> I agree this is following the existing pattern, but the existing pattern looks
> bonkers. Sorting relocation types by numeric value rather than grouping the
> architecture-specific ones by architecture...
>
> Could you group the RISC-V ones together and put them after a single blank
> line below the current defines? I'll try to come back and fix the others once
> this set has been merged.
>
> > #define EFI_IMAGE_REL_BASED_IA64_IMM64 9
> > #define EFI_IMAGE_REL_BASED_MIPS_JMPADDR16 9
> > #define EFI_IMAGE_REL_BASED_DIR64 10
> > diff --git a/MdePkg/Include/Library/BaseLib.h
> > b/MdePkg/Include/Library/BaseLib.h
> > index 2a75bc0..5f0ee8d 100644
> > --- a/MdePkg/Include/Library/BaseLib.h
> > +++ b/MdePkg/Include/Library/BaseLib.h
> > @@ -4,6 +4,8 @@
> >
> > Copyright (c) 2006 - 2019, Intel Corporation. All rights
> > reserved. Portions copyright (c) 2008 - 2009, Apple Inc. All
> > rights reserved.
> > +Portions Copyright (c) 2016, Hewlett Packard Enterprise Development
> > +LP. All rights reserved.
> > +
> > SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > **/
> > @@ -124,6 +126,71 @@ typedef struct {
> >
> > #endif // defined (MDE_CPU_AARCH64)
> >
> > +#if defined (MDE_CPU_RISCV64)
> > +///
> > +/// The RISC-V architecture context buffer used by SetJump() and
> LongJump().
> > +///
> > +typedef struct {
> > + UINT64RA;
Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v1 04/22]: MdePkg/Include: RISC-V definitions.
On Wed, Sep 04, 2019 at 06:42:59PM +0800, Abner Chang wrote:
> Add RISC-V processor related definitions.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Abner Chang
> ---
> MdePkg/Include/IndustryStandard/PeImage.h | 14 +-
> MdePkg/Include/Library/BaseLib.h | 67 ++
> MdePkg/Include/Protocol/DebugSupport.h| 55 +
> MdePkg/Include/Protocol/PxeBaseCode.h | 8 +
> MdePkg/Include/RiscV64/ProcessorBind.h| 336
> ++
> MdePkg/Include/Uefi/UefiBaseType.h| 25 +++
> MdePkg/Include/Uefi/UefiSpec.h| 11 +
> 7 files changed, 513 insertions(+), 3 deletions(-)
> create mode 100644 MdePkg/Include/RiscV64/ProcessorBind.h
>
> diff --git a/MdePkg/Include/IndustryStandard/PeImage.h
> b/MdePkg/Include/IndustryStandard/PeImage.h
> index 720bb08..47796b2 100644
> --- a/MdePkg/Include/IndustryStandard/PeImage.h
> +++ b/MdePkg/Include/IndustryStandard/PeImage.h
> @@ -9,6 +9,8 @@
>
> Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
> Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.
> +Portions Copyright (c) 2016, Hewlett Packard Enterprise Development LP. All
> rights reserved.
> +
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> **/
> @@ -34,6 +36,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #define IMAGE_FILE_MACHINE_X64 0x8664
> #define IMAGE_FILE_MACHINE_ARMTHUMB_MIXED 0x01c2
> #define IMAGE_FILE_MACHINE_ARM64 0xAA64
> +#define IMAGE_FILE_MACHINE_RISCV32 0x5032
> +#define IMAGE_FILE_MACHINE_RISCV64 0x5064
> +#define IMAGE_FILE_MACHINE_RISCV1280x5128
>
> //
> // EXE file formats
> @@ -478,9 +483,9 @@ typedef struct {
> ///
> #define EFI_IMAGE_SIZEOF_BASE_RELOCATION 8
>
> -//
> -// Based relocation types.
> -//
> +///
> +/// Based relocation types.
> +///
I don't know if this change to the comment block is a wonky rebase or
whatever, but please drop it.
> #define EFI_IMAGE_REL_BASED_ABSOLUTE0
> #define EFI_IMAGE_REL_BASED_HIGH1
> #define EFI_IMAGE_REL_BASED_LOW 2
> @@ -488,7 +493,10 @@ typedef struct {
> #define EFI_IMAGE_REL_BASED_HIGHADJ 4
> #define EFI_IMAGE_REL_BASED_MIPS_JMPADDR5
> #define EFI_IMAGE_REL_BASED_ARM_MOV32A 5
> +#define EFI_IMAGE_REL_BASED_RISCV_HI20 5
> #define EFI_IMAGE_REL_BASED_ARM_MOV32T 7
> +#define EFI_IMAGE_REL_BASED_RISCV_LOW12I7
> +#define EFI_IMAGE_REL_BASED_RISCV_LOW12S8
I agree this is following the existing pattern, but the existing
pattern looks bonkers. Sorting relocation types by numeric value
rather than grouping the architecture-specific ones by architecture...
Could you group the RISC-V ones together and put them after a single
blank line below the current defines? I'll try to come back and fix
the others once this set has been merged.
> #define EFI_IMAGE_REL_BASED_IA64_IMM64 9
> #define EFI_IMAGE_REL_BASED_MIPS_JMPADDR16 9
> #define EFI_IMAGE_REL_BASED_DIR64 10
> diff --git a/MdePkg/Include/Library/BaseLib.h
> b/MdePkg/Include/Library/BaseLib.h
> index 2a75bc0..5f0ee8d 100644
> --- a/MdePkg/Include/Library/BaseLib.h
> +++ b/MdePkg/Include/Library/BaseLib.h
> @@ -4,6 +4,8 @@
>
> Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.
> Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.
> +Portions Copyright (c) 2016, Hewlett Packard Enterprise Development LP. All
> rights reserved.
> +
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> **/
> @@ -124,6 +126,71 @@ typedef struct {
>
> #endif // defined (MDE_CPU_AARCH64)
>
> +#if defined (MDE_CPU_RISCV64)
> +///
> +/// The RISC-V architecture context buffer used by SetJump() and LongJump().
> +///
> +typedef struct {
> + UINT64RA;
> + UINT64S0;
> + UINT64S1;
> + UINT64S2;
> + UINT64S3;
> + UINT64S4;
> + UINT64S5;
> + UINT64S6;
> + UINT64S7;
> + UINT64S8;
> + UINT64S9;
> + UINT64S10;
> + UINT64S11;
> + UINT64SP;
> +} BASE_LIBRARY_JUMP_BUFFER;
> +
> +#define BASE_LIBRARY_JUMP_BUFFER_ALIGNMENT 8
> +
> +/**
> + RISC-V read CSR register.
> +
> +**/
> +UINT32
> +EFIAPI
> +RiscVReadCsr (
This function does not appear to be implemented by any patch in this set?
> + UINT32 CsrIndex
> + );
> +
> +/**
> + RISC-V write CSR register.
> +
> +**/
> +VOID
> +EFIAPI
> +RiscVwriteCsr (
Neither does this one. (Also, that 'w' should probably be upper case
if it was.)
> + UINT32 CsrIndex,
> + UINT32 Value
> + );
> +
> +/**
> + RISC-V invalidate instruction cache.
> +
> +**/
> +VOID
>
