diff mbox series

[1/3] Revert "riscv/efistub: Ensure GP-relative addressing is not used"

Message ID 20240306085622.87248-1-cuiyunhui@bytedance.com (mailing list archive)
State Changes Requested
Headers show
Series [1/3] Revert "riscv/efistub: Ensure GP-relative addressing is not used" | expand

Checks

Context Check Description
conchuod/vmtest-fixes-PR fail merge-conflict

Commit Message

Yunhui Cui March 6, 2024, 8:56 a.m. UTC
This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.

Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
---
 drivers/firmware/efi/libstub/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Conor Dooley March 6, 2024, 9:02 a.m. UTC | #1
On Wed, Mar 06, 2024 at 04:56:20PM +0800, Yunhui Cui wrote:
> This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.
> 
> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>

Reverts are commits too. You still need to write commit messages
justifying the revert.

Thanks,
Conor.

> ---
>  drivers/firmware/efi/libstub/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> index 31eb1e287ce1..475f37796779 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -28,7 +28,7 @@ cflags-$(CONFIG_ARM)		+= -DEFI_HAVE_STRLEN -DEFI_HAVE_STRNLEN \
>  				   -DEFI_HAVE_MEMCHR -DEFI_HAVE_STRRCHR \
>  				   -DEFI_HAVE_STRCMP -fno-builtin -fpic \
>  				   $(call cc-option,-mno-single-pic-base)
> -cflags-$(CONFIG_RISCV)		+= -fpic -DNO_ALTERNATIVE -mno-relax
> +cflags-$(CONFIG_RISCV)		+= -fpic -DNO_ALTERNATIVE
>  cflags-$(CONFIG_LOONGARCH)	+= -fpie
>  
>  cflags-$(CONFIG_EFI_PARAMS_FROM_FDT)	+= -I$(srctree)/scripts/dtc/libfdt
> -- 
> 2.20.1
>
Ard Biesheuvel March 6, 2024, 9:38 a.m. UTC | #2
On Wed, 6 Mar 2024 at 10:03, Conor Dooley <conor.dooley@microchip.com> wrote:
>
> On Wed, Mar 06, 2024 at 04:56:20PM +0800, Yunhui Cui wrote:
> > This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.
> >
> > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
>
> Reverts are commits too. You still need to write commit messages
> justifying the revert.
>

Indeed. Also, please revert them in the right [reverse] order so we
don't inadvertently break bisect.
Yunhui Cui March 6, 2024, 12:37 p.m. UTC | #3
Hi Ard, Conor,

On Wed, Mar 6, 2024 at 5:38 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 6 Mar 2024 at 10:03, Conor Dooley <conor.dooley@microchip.com> wrote:
> >
> > On Wed, Mar 06, 2024 at 04:56:20PM +0800, Yunhui Cui wrote:
> > > This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.
> > >
> > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> >
> > Reverts are commits too. You still need to write commit messages
> > justifying the revert.
> >
>
> Indeed. Also, please revert them in the right [reverse] order so we
> don't inadvertently break bisect.

Okay, I'll update it in v2.

Thanks,
Yunhui
Jan Kiszka March 6, 2024, 12:52 p.m. UTC | #4
On 06.03.24 09:56, Yunhui Cui wrote:
> This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.
> 

This comes without a reason - which is likely something around "will fix
this properly later". But then you regress first and only fix
afterwards. Can't that be done the other way around?

Jan

> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> ---
>  drivers/firmware/efi/libstub/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> index 31eb1e287ce1..475f37796779 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -28,7 +28,7 @@ cflags-$(CONFIG_ARM)		+= -DEFI_HAVE_STRLEN -DEFI_HAVE_STRNLEN \
>  				   -DEFI_HAVE_MEMCHR -DEFI_HAVE_STRRCHR \
>  				   -DEFI_HAVE_STRCMP -fno-builtin -fpic \
>  				   $(call cc-option,-mno-single-pic-base)
> -cflags-$(CONFIG_RISCV)		+= -fpic -DNO_ALTERNATIVE -mno-relax
> +cflags-$(CONFIG_RISCV)		+= -fpic -DNO_ALTERNATIVE
>  cflags-$(CONFIG_LOONGARCH)	+= -fpie
>  
>  cflags-$(CONFIG_EFI_PARAMS_FROM_FDT)	+= -I$(srctree)/scripts/dtc/libfdt
Yunhui Cui March 6, 2024, 1:07 p.m. UTC | #5
Hi Jan,

On Wed, Mar 6, 2024 at 8:52 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> On 06.03.24 09:56, Yunhui Cui wrote:
> > This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.
> >
>
> This comes without a reason - which is likely something around "will fix
> this properly later". But then you regress first and only fix
> afterwards. Can't that be done the other way around?

Sorry, I don't quite understand what you mean. Can you help explain it
more clearly? Do you mean "delete mno-relax instead of revert
directly?"


Thanks,
Yunhui
Ard Biesheuvel March 6, 2024, 1:11 p.m. UTC | #6
On Wed, 6 Mar 2024 at 14:08, yunhui cui <cuiyunhui@bytedance.com> wrote:
>
> Hi Jan,
>
> On Wed, Mar 6, 2024 at 8:52 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >
> > On 06.03.24 09:56, Yunhui Cui wrote:
> > > This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.
> > >
> >
> > This comes without a reason - which is likely something around "will fix
> > this properly later". But then you regress first and only fix
> > afterwards. Can't that be done the other way around?
>
> Sorry, I don't quite understand what you mean. Can you help explain it
> more clearly? Do you mean "delete mno-relax instead of revert
> directly?"
>

You should order your patches in a way that does not create
intermediate states (between 1-2 or between 2-3) where the original
problem is being recreated.

So in this case, you should
a) fix the issue
b) revert the existing patches in *opposite* order

However, I don't think the EFI stub can use GP - please refer to my other reply.
Yunhui Cui March 6, 2024, 1:26 p.m. UTC | #7
Hi Ard,

On Wed, Mar 6, 2024 at 9:11 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 6 Mar 2024 at 14:08, yunhui cui <cuiyunhui@bytedance.com> wrote:
> >
> > Hi Jan,
> >
> > On Wed, Mar 6, 2024 at 8:52 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
> > >
> > > On 06.03.24 09:56, Yunhui Cui wrote:
> > > > This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.
> > > >
> > >
> > > This comes without a reason - which is likely something around "will fix
> > > this properly later". But then you regress first and only fix
> > > afterwards. Can't that be done the other way around?
> >
> > Sorry, I don't quite understand what you mean. Can you help explain it
> > more clearly? Do you mean "delete mno-relax instead of revert
> > directly?"
> >
>
> You should order your patches in a way that does not create
> intermediate states (between 1-2 or between 2-3) where the original
> problem is being recreated.
>
> So in this case, you should
> a) fix the issue
> b) revert the existing patches in *opposite* order
Simply, I plan to remove "-mno-relax" and
"\|R_RISCV_$(BITS)\|R_RISCV_RELAX" in the third patch (fix patch).


> However, I don't think the EFI stub can use GP - please refer to my other reply.
The problem we encountered is that gcc 13 will optimize efistub using gp.

Thanks,
Yunhui
Ard Biesheuvel March 6, 2024, 1:46 p.m. UTC | #8
On Wed, 6 Mar 2024 at 14:27, yunhui cui <cuiyunhui@bytedance.com> wrote:
>
> Hi Ard,
>
> On Wed, Mar 6, 2024 at 9:11 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 6 Mar 2024 at 14:08, yunhui cui <cuiyunhui@bytedance.com> wrote:
> > >
> > > Hi Jan,
> > >
> > > On Wed, Mar 6, 2024 at 8:52 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
> > > >
> > > > On 06.03.24 09:56, Yunhui Cui wrote:
> > > > > This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.
> > > > >
> > > >
> > > > This comes without a reason - which is likely something around "will fix
> > > > this properly later". But then you regress first and only fix
> > > > afterwards. Can't that be done the other way around?
> > >
> > > Sorry, I don't quite understand what you mean. Can you help explain it
> > > more clearly? Do you mean "delete mno-relax instead of revert
> > > directly?"
> > >
> >
> > You should order your patches in a way that does not create
> > intermediate states (between 1-2 or between 2-3) where the original
> > problem is being recreated.
> >
> > So in this case, you should
> > a) fix the issue
> > b) revert the existing patches in *opposite* order
> Simply, I plan to remove "-mno-relax" and
> "\|R_RISCV_$(BITS)\|R_RISCV_RELAX" in the third patch (fix patch).
>

Why is that better than the current approach?
diff mbox series

Patch

diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index 31eb1e287ce1..475f37796779 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -28,7 +28,7 @@  cflags-$(CONFIG_ARM)		+= -DEFI_HAVE_STRLEN -DEFI_HAVE_STRNLEN \
 				   -DEFI_HAVE_MEMCHR -DEFI_HAVE_STRRCHR \
 				   -DEFI_HAVE_STRCMP -fno-builtin -fpic \
 				   $(call cc-option,-mno-single-pic-base)
-cflags-$(CONFIG_RISCV)		+= -fpic -DNO_ALTERNATIVE -mno-relax
+cflags-$(CONFIG_RISCV)		+= -fpic -DNO_ALTERNATIVE
 cflags-$(CONFIG_LOONGARCH)	+= -fpie
 
 cflags-$(CONFIG_EFI_PARAMS_FROM_FDT)	+= -I$(srctree)/scripts/dtc/libfdt