public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [edk2-devel] [PATCH v1 0/1] RISCV: Fix InternalLongJump
@ 2023-09-19  4:43 Andrei Warkentin
  2023-09-19  4:43 ` [edk2-devel] [PATCH v1 1/1] RISCV: Fix InternalLongJump to return correct value Andrei Warkentin
  0 siblings, 1 reply; 7+ messages in thread
From: Andrei Warkentin @ 2023-09-19  4:43 UTC (permalink / raw)
  To: devel; +Cc: Andrei Warkentin

I was playing around with building code with -Os and ran into
weird crashes, that I ended up chasing down to an obviously
incorrect InternalLongJump implementation, which has never
been correctly passing its 2nd parameter down as the return
value of SetJump.

You can find the pull at https://github.com/tianocore/edk2/pull/4836,
with the failing tests seemingly unrelated (at least one of these
was related to CryptoPkg not being found).

Andrei Warkentin (1):
  RISCV: Fix InternalLongJump to return correct value

 CryptoPkg/Library/OpensslLib/openssl                  | 2 +-
 MdePkg/Library/BaseLib/RiscV64/RiscVSetJumpLongJump.S | 7 ++-----
 2 files changed, 3 insertions(+), 6 deletions(-)

-- 
2.34.1



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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* [edk2-devel] [PATCH v1 1/1] RISCV: Fix InternalLongJump to return correct value
  2023-09-19  4:43 [edk2-devel] [PATCH v1 0/1] RISCV: Fix InternalLongJump Andrei Warkentin
@ 2023-09-19  4:43 ` Andrei Warkentin
  2023-09-19  8:01   ` Yao, Jiewen
  2023-09-19 14:31   ` Ard Biesheuvel
  0 siblings, 2 replies; 7+ messages in thread
From: Andrei Warkentin @ 2023-09-19  4:43 UTC (permalink / raw)
  To: devel; +Cc: Andrei Warkentin, Yong Li, Sunil V L, Tuan Phan, Daniel Schaefer

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
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>
 //
 // 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 (#108816): https://edk2.groups.io/g/devel/message/108816
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]
-=-=-=-=-=-=-=-=-=-=-=-



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [edk2-devel] [PATCH v1 1/1] RISCV: Fix InternalLongJump to return correct value
  2023-09-19  4:43 ` [edk2-devel] [PATCH v1 1/1] RISCV: Fix InternalLongJump to return correct value Andrei Warkentin
@ 2023-09-19  8:01   ` Yao, Jiewen
  2023-09-19  8:04     ` Andrei Warkentin
  2023-09-19 14:31   ` Ard Biesheuvel
  1 sibling, 1 reply; 7+ messages in thread
From: Yao, Jiewen @ 2023-09-19  8:01 UTC (permalink / raw)
  To: devel@edk2.groups.io, Warkentin, Andrei
  Cc: Li, Yong, Sunil V L, Tuan Phan, Daniel Schaefer

I am OK for the RISC-V change.

Would you please let me know why we need openssl submodule ?


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Andrei
> Warkentin
> Sent: Tuesday, September 19, 2023 12:43 PM
> To: devel@edk2.groups.io
> Cc: Warkentin, Andrei <andrei.warkentin@intel.com>; Li, Yong
> <yong.li@intel.com>; Sunil V L <sunilvl@ventanamicro.com>; Tuan Phan
> <tphan@ventanamicro.com>; Daniel Schaefer <git@danielschaefer.me>
> Subject: [edk2-devel] [PATCH v1 1/1] RISCV: Fix InternalLongJump to return
> correct value
> 
> 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
> 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>
>  //
>  // 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 (#108818): https://edk2.groups.io/g/devel/message/108818
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]
-=-=-=-=-=-=-=-=-=-=-=-



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [edk2-devel] [PATCH v1 1/1] RISCV: Fix InternalLongJump to return correct value
  2023-09-19  8:01   ` Yao, Jiewen
@ 2023-09-19  8:04     ` Andrei Warkentin
  0 siblings, 0 replies; 7+ messages in thread
From: Andrei Warkentin @ 2023-09-19  8:04 UTC (permalink / raw)
  To: Yao, Jiewen, devel@edk2.groups.io
  Cc: Li, Yong, Sunil V L, Tuan Phan, Daniel Schaefer

[-- Attachment #1: Type: text/plain, Size: 3417 bytes --]

My mistake. Didn't see that pulled in... Will rework.
________________________________
От: Yao, Jiewen <jiewen.yao@intel.com>
Отправлено: Tuesday, September 19, 2023 3:01:34 AM
Кому: devel@edk2.groups.io <devel@edk2.groups.io>; Warkentin, Andrei <andrei.warkentin@intel.com>
Копия: Li, Yong <yong.li@intel.com>; Sunil V L <sunilvl@ventanamicro.com>; Tuan Phan <tphan@ventanamicro.com>; Daniel Schaefer <git@danielschaefer.me>
Тема: RE: [edk2-devel] [PATCH v1 1/1] RISCV: Fix InternalLongJump to return correct value

I am OK for the RISC-V change.

Would you please let me know why we need openssl submodule ?


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Andrei
> Warkentin
> Sent: Tuesday, September 19, 2023 12:43 PM
> To: devel@edk2.groups.io
> Cc: Warkentin, Andrei <andrei.warkentin@intel.com>; Li, Yong
> <yong.li@intel.com>; Sunil V L <sunilvl@ventanamicro.com>; Tuan Phan
> <tphan@ventanamicro.com>; Daniel Schaefer <git@danielschaefer.me>
> Subject: [edk2-devel] [PATCH v1 1/1] RISCV: Fix InternalLongJump to return
> correct value
>
> 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
> 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>
>  //
>  // 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 (#108819): https://edk2.groups.io/g/devel/message/108819
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]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 5411 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [edk2-devel] [PATCH v1 1/1] RISCV: Fix InternalLongJump to return correct value
  2023-09-19  4:43 ` [edk2-devel] [PATCH v1 1/1] RISCV: Fix InternalLongJump to return correct value Andrei Warkentin
  2023-09-19  8:01   ` Yao, Jiewen
@ 2023-09-19 14:31   ` Ard Biesheuvel
  2023-09-19 14:41     ` Leif Lindholm
  1 sibling, 1 reply; 7+ messages in thread
From: Ard Biesheuvel @ 2023-09-19 14:31 UTC (permalink / raw)
  To: devel, andrei.warkentin, Michael Kinney, Andrew Fish,
	Leif Lindholm
  Cc: Yong Li, Sunil V L, Tuan Phan, Daniel Schaefer

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?

(The problem is that claiming copyright gives the right to distribute
the code without being bound by the terms of the license)



>  //
>  // 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 (#108839): https://edk2.groups.io/g/devel/message/108839
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]
-=-=-=-=-=-=-=-=-=-=-=-



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [edk2-devel] [PATCH v1 1/1] RISCV: Fix InternalLongJump to return correct value
  2023-09-19 14:31   ` Ard Biesheuvel
@ 2023-09-19 14:41     ` Leif Lindholm
  2023-09-19 16:14       ` Andrei Warkentin
  0 siblings, 1 reply; 7+ messages in thread
From: Leif Lindholm @ 2023-09-19 14:41 UTC (permalink / raw)
  To: Ard Biesheuvel, devel, andrei.warkentin, Michael Kinney,
	Andrew Fish
  Cc: Yong Li, Sunil V L, Tuan Phan, Daniel Schaefer

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 (#108844): https://edk2.groups.io/g/devel/message/108844
Mute This Topic: https://groups.io/mt/101450445/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [edk2-devel] [PATCH v1 1/1] RISCV: Fix InternalLongJump to return correct value
  2023-09-19 14:41     ` Leif Lindholm
@ 2023-09-19 16:14       ` Andrei Warkentin
  0 siblings, 0 replies; 7+ messages in thread
From: Andrei Warkentin @ 2023-09-19 16:14 UTC (permalink / raw)
  To: Leif Lindholm, Ard Biesheuvel, devel@edk2.groups.io,
	Kinney, Michael D, Andrew Fish
  Cc: Li, Yong, Sunil V L, Tuan Phan, Daniel Schaefer

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



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-09-19 16:14 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-19  4:43 [edk2-devel] [PATCH v1 0/1] RISCV: Fix InternalLongJump Andrei Warkentin
2023-09-19  4:43 ` [edk2-devel] [PATCH v1 1/1] RISCV: Fix InternalLongJump to return correct value Andrei Warkentin
2023-09-19  8:01   ` Yao, Jiewen
2023-09-19  8:04     ` Andrei Warkentin
2023-09-19 14:31   ` Ard Biesheuvel
2023-09-19 14:41     ` Leif Lindholm
2023-09-19 16:14       ` Andrei Warkentin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox