Re: [edk2-devel] [PATCH v1 1/1] RISCV: Fix InternalLongJump to return correct value

["Andrei Warkentin" <andrei.warkentin@intel.com>]

Thanks Leif, this unfortunate inclusion has already been remedied in the v2. What happens is that random files in the tree get changed by virtue of just doing a build, so it is very easy to accidentally pull it in. And it's a single line that I missed manually looking over the patch.

Wrt license inclusion, I'm just aping what everyone else appears to do. My personal two cents is that the whole thing is rather silly - the code ought to be entirely owned by Tiano, but if other vendors are going to add Copyrights - all of them might as well. I'd rather this discussion be taken separately from fixing this rather critical issue, and ideally bubbled up to Tiano leadership.

A

> -----Original Message-----
> From: Leif Lindholm <quic_llindhol@quicinc.com>
> Sent: Tuesday, September 19, 2023 9:41 AM
> To: Ard Biesheuvel <ardb@kernel.org>; devel@edk2.groups.io; Warkentin,
> Andrei <andrei.warkentin@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Andrew Fish <afish@apple.com>
> Cc: Li, Yong <yong.li@intel.com>; Sunil V L <sunilvl@ventanamicro.com>; Tuan
> Phan <tphan@ventanamicro.com>; Daniel Schaefer <git@danielschaefer.me>
> Subject: Re: [edk2-devel] [PATCH v1 1/1] RISCV: Fix InternalLongJump to return
> correct value
> 
> On 2023-09-19 15:31, Ard Biesheuvel wrote:
> > Hello Andrei,
> >
> > On Tue, 19 Sept 2023 at 04:43, Andrei Warkentin
> > <andrei.warkentin@intel.com> wrote:
> >>
> >> InternalLongJump was not returning the 2nd parameter passed to
> >> LongJmp (Value) as the return value from SetJmp.
> >>
> >> Seen with code compiled with -Os, where an LongJmp (Buffer, -1)
> >> somehow translated to SetJmp returning 0...
> >>
> >> Cc: Yong Li <yong.li@intel.com>
> >> Cc: Sunil V L <sunilvl@ventanamicro.com>
> >> Cc: Tuan Phan <tphan@ventanamicro.com>
> >> Cc: Daniel Schaefer <git@danielschaefer.me>
> >> Signed-off-by: Andrei Warkentin <andrei.warkentin@intel.com>
> >> ---
> >>   CryptoPkg/Library/OpensslLib/openssl                  | 2 +-
> >>   MdePkg/Library/BaseLib/RiscV64/RiscVSetJumpLongJump.S | 7 ++-----
> >>   2 files changed, 3 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/CryptoPkg/Library/OpensslLib/openssl
> >> b/CryptoPkg/Library/OpensslLib/openssl
> >> index de90e54bbe82..830bf8e1e474 160000
> >> --- a/CryptoPkg/Library/OpensslLib/openssl
> >> +++ b/CryptoPkg/Library/OpensslLib/openssl
> >> @@ -1 +1 @@
> >> -Subproject commit de90e54bbe82e5be4fb9608b6f5c308bb837d355
> >> +Subproject commit 830bf8e1e4749ad65c51b6a1d0d769ae689404ba
> >
> > This does not belong here
> >
> >> diff --git a/MdePkg/Library/BaseLib/RiscV64/RiscVSetJumpLongJump.S
> >> b/MdePkg/Library/BaseLib/RiscV64/RiscVSetJumpLongJump.S
> >> index 34486eabba4c..e97a7d0727b8 100644
> >> --- a/MdePkg/Library/BaseLib/RiscV64/RiscVSetJumpLongJump.S
> >> +++ b/MdePkg/Library/BaseLib/RiscV64/RiscVSetJumpLongJump.S
> >> @@ -3,6 +3,7 @@
> >>   // Set/Long jump for RISC-V
> >>   //
> >>   // Copyright (c) 2020, Hewlett Packard Enterprise Development LP.
> >> All rights reserved.<BR>
> >> +// Copyright (c) 2023, Intel Corporation. All rights reserved.<BR>
> >
> > I suppose there is some internal policy at Intel that tells you to
> > claim copyright, but do you really think fixing existing HP code by
> > removing 4 instructions and adding one back is sufficient for claiming
> > copyright on the entire file?
> >
> > Note that I am not objecting to this in principle: I am just curious
> > (and I have objected in the past to patches that only removed lines
> > from existing code and then added a copyright line)
> >
> > Should we have some project/community wide guidance on this?
> 
> I reacted early on when joining this project that copyright was frequently
> added/bumped for trivial changes, including things like fixing typos in
> comments. So I think the custom for this project is that the bar is a lot lower
> than for projects like Linux, grub, etc.
> 
> So I see this as on par for tianocore.
> 
> > (The problem is that claiming copyright gives the right to distribute
> > the code without being bound by the terms of the license)
> 
> Wouldn't all the other copyright holders also need to agree?
> 
> /
>      Leif
> 
> >>   //
> >>   // SPDX-License-Identifier: BSD-2-Clause-Patent
> >>   //
> >> @@ -47,9 +48,5 @@ InternalLongJump:
> >>       REG_L s10, 11*SZREG(a0)
> >>       REG_L s11, 12*SZREG(a0)
> >>       REG_L sp,  13*SZREG(a0)
> >> -
> >> -    add   a0, s0, 0
> >> -    add   a1, s1, 0
> >> -    add   a2, s2, 0
> >> -    add   a3, s3, 0
> >> +    mv    a0, a1
> >>       ret
> >> --
> >> 2.34.1
> >>
> >>
> >>
> >> 
> >>
> >>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108850): https://edk2.groups.io/g/devel/message/108850
Mute This Topic: https://groups.io/mt/101450445/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-