| Message ID | 20240306085622.87248-1-cuiyunhui@bytedance.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] Revert "riscv/efistub: Ensure GP-relative addressing is not used" | expand |
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 >
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.
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
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
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
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.
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
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 --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
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(-)