| Message ID | 20231026053052.622453-12-takahiro.akashi@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cmd: bootefi: refactor the code for bootmgr | expand |
Am 26. Oktober 2023 07:30:50 MESZ schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>: >Now it is clear that the feature actually depends on efi interfaces, >not "bootefi" command. efi_set_bootdev() will automatically be nullified >if necessary efi component is disabled. > >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >--- > fs/fs.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > >diff --git a/fs/fs.c b/fs/fs.c >index 4cb4310c9cc2..70cdb594c4c8 100644 >--- a/fs/fs.c >+++ b/fs/fs.c >@@ -791,10 +791,9 @@ int do_load(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[], > return 1; > } > >- if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) >- efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "", >- (argc > 4) ? argv[4] : "", map_sysmem(addr, 0), >- len_read); >+ efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "", This function should not exist for CONFIG_EFI_LOADER=n. There are other places where the function is invoked. Please, review all of them. Best regards Heinrich >+ (argc > 4) ? argv[4] : "", map_sysmem(addr, 0), >+ len_read); > > printf("%llu bytes read in %lu ms", len_read, time); > if (time > 0) {
On Thu, Oct 26, 2023 at 09:58:53AM +0200, Heinrich Schuchardt wrote: > > > Am 26. Oktober 2023 07:30:50 MESZ schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>: > >Now it is clear that the feature actually depends on efi interfaces, > >not "bootefi" command. efi_set_bootdev() will automatically be nullified > >if necessary efi component is disabled. > > > >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > >--- > > fs/fs.c | 7 +++---- > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > >diff --git a/fs/fs.c b/fs/fs.c > >index 4cb4310c9cc2..70cdb594c4c8 100644 > >--- a/fs/fs.c > >+++ b/fs/fs.c > >@@ -791,10 +791,9 @@ int do_load(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[], > > return 1; > > } > > > >- if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) > >- efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "", > >- (argc > 4) ? argv[4] : "", map_sysmem(addr, 0), > >- len_read); > >+ efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "", > > This function should not exist for CONFIG_EFI_LOADER=n. There are other places where the function is invoked. Please, review all of them. Please go through the whole patch set, especially patch#8 "efi_loader: split unrelated code from efi_bootmgr.c". efi_[set|clear]_bootdev() will be nullified if not necessary. -Takahiro Akashi > Best regards > > Heinrich > > > >+ (argc > 4) ? argv[4] : "", map_sysmem(addr, 0), > >+ len_read); > > > > printf("%llu bytes read in %lu ms", len_read, time); > > if (time > 0) {
On Thu, Oct 26, 2023 at 05:48:30PM +0900, AKASHI Takahiro wrote: > On Thu, Oct 26, 2023 at 09:58:53AM +0200, Heinrich Schuchardt wrote: > > > > > > Am 26. Oktober 2023 07:30:50 MESZ schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>: > > >Now it is clear that the feature actually depends on efi interfaces, > > >not "bootefi" command. efi_set_bootdev() will automatically be nullified > > >if necessary efi component is disabled. > > > > > >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > >--- > > > fs/fs.c | 7 +++---- > > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > > >diff --git a/fs/fs.c b/fs/fs.c > > >index 4cb4310c9cc2..70cdb594c4c8 100644 > > >--- a/fs/fs.c > > >+++ b/fs/fs.c > > >@@ -791,10 +791,9 @@ int do_load(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[], > > > return 1; > > > } > > > > > >- if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) > > >- efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "", > > >- (argc > 4) ? argv[4] : "", map_sysmem(addr, 0), > > >- len_read); > > >+ efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "", > > > > This function should not exist for CONFIG_EFI_LOADER=n. There are other places where the function is invoked. Please, review all of them. > > Please go through the whole patch set, especially patch#8 > "efi_loader: split unrelated code from efi_bootmgr.c". > > efi_[set|clear]_bootdev() will be nullified if not necessary. In this case I think what we have here today is more readable / clearer. We don't need empty functions as we can either do like this section of the code does today or the linker will discard things correctly as it's a case of funcB() calls funcA() and nothing calls funcB() so it will be discarded. I don't know without digging through the series more what the correct IS_ENABLED() guard should be here (and then also in patch #10).
On Thu, Oct 26, 2023 at 08:47:52AM -0400, Tom Rini wrote: > On Thu, Oct 26, 2023 at 05:48:30PM +0900, AKASHI Takahiro wrote: > > On Thu, Oct 26, 2023 at 09:58:53AM +0200, Heinrich Schuchardt wrote: > > > > > > > > > Am 26. Oktober 2023 07:30:50 MESZ schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>: > > > >Now it is clear that the feature actually depends on efi interfaces, > > > >not "bootefi" command. efi_set_bootdev() will automatically be nullified > > > >if necessary efi component is disabled. > > > > > > > >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > >--- > > > > fs/fs.c | 7 +++---- > > > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > > > > >diff --git a/fs/fs.c b/fs/fs.c > > > >index 4cb4310c9cc2..70cdb594c4c8 100644 > > > >--- a/fs/fs.c > > > >+++ b/fs/fs.c > > > >@@ -791,10 +791,9 @@ int do_load(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[], > > > > return 1; > > > > } > > > > > > > >- if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) > > > >- efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "", > > > >- (argc > 4) ? argv[4] : "", map_sysmem(addr, 0), > > > >- len_read); > > > >+ efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "", > > > > > > This function should not exist for CONFIG_EFI_LOADER=n. There are other places where the function is invoked. Please, review all of them. > > > > Please go through the whole patch set, especially patch#8 > > "efi_loader: split unrelated code from efi_bootmgr.c". > > > > efi_[set|clear]_bootdev() will be nullified if not necessary. > > In this case I think what we have here today is more readable / clearer. > We don't need empty functions as we can either do like this section of > the code does today or the linker will discard things correctly as it's > a case of funcB() calls funcA() and nothing calls funcB() so it will be > discarded. I don't know without digging through the series more what the > correct IS_ENABLED() guard should be here (and then also in patch #10). If I correctly understand your question, it is my point in this commit. I believe that a caller should not be bothered by thinking of what efi CONFIG be used for the guard. All that should be done is to just put a right hook (efi_set_bootdev() in this case) at a right place as a caller (do_load() in this case) is apparently irrelevant to UEFI subsystem. Hereafter, whatever rework may be done on UEFI side (like my refactoring here), other code won't need to be modified. If you want to rely on an intelligent linker behavior, I would suggest another approach, modifying efi_set_bootdev() as follows; efi_set_bootdev(...) { if (!IS_ENABLED(EFI_BINARY_EXEC)) return; ... } I hope it would also work. -Takahiro Akashi > > -- > Tom
On Fri, Oct 27, 2023 at 09:59:02AM +0900, AKASHI Takahiro wrote: > On Thu, Oct 26, 2023 at 08:47:52AM -0400, Tom Rini wrote: > > On Thu, Oct 26, 2023 at 05:48:30PM +0900, AKASHI Takahiro wrote: > > > On Thu, Oct 26, 2023 at 09:58:53AM +0200, Heinrich Schuchardt wrote: > > > > > > > > > > > > Am 26. Oktober 2023 07:30:50 MESZ schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>: > > > > >Now it is clear that the feature actually depends on efi interfaces, > > > > >not "bootefi" command. efi_set_bootdev() will automatically be nullified > > > > >if necessary efi component is disabled. > > > > > > > > > >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > >--- > > > > > fs/fs.c | 7 +++---- > > > > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > > > > > > >diff --git a/fs/fs.c b/fs/fs.c > > > > >index 4cb4310c9cc2..70cdb594c4c8 100644 > > > > >--- a/fs/fs.c > > > > >+++ b/fs/fs.c > > > > >@@ -791,10 +791,9 @@ int do_load(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[], > > > > > return 1; > > > > > } > > > > > > > > > >- if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) > > > > >- efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "", > > > > >- (argc > 4) ? argv[4] : "", map_sysmem(addr, 0), > > > > >- len_read); > > > > >+ efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "", > > > > > > > > This function should not exist for CONFIG_EFI_LOADER=n. There are other places where the function is invoked. Please, review all of them. > > > > > > Please go through the whole patch set, especially patch#8 > > > "efi_loader: split unrelated code from efi_bootmgr.c". > > > > > > efi_[set|clear]_bootdev() will be nullified if not necessary. > > > > In this case I think what we have here today is more readable / clearer. > > We don't need empty functions as we can either do like this section of > > the code does today or the linker will discard things correctly as it's > > a case of funcB() calls funcA() and nothing calls funcB() so it will be > > discarded. I don't know without digging through the series more what the > > correct IS_ENABLED() guard should be here (and then also in patch #10). > > If I correctly understand your question, it is my point in this commit. > > I believe that a caller should not be bothered by thinking of what efi CONFIG > be used for the guard. All that should be done is to just put a right hook > (efi_set_bootdev() in this case) at a right place as a caller (do_load() > in this case) is apparently irrelevant to UEFI subsystem. > Hereafter, whatever rework may be done on UEFI side (like my refactoring > here), other code won't need to be modified. > > If you want to rely on an intelligent linker behavior, I would suggest > another approach, modifying efi_set_bootdev() as follows; > > efi_set_bootdev(...) > { > if (!IS_ENABLED(EFI_BINARY_EXEC)) > return; > ... > } > > I hope it would also work. Looking at all of the ways to solve this particular case again, OK, I think we can go with #8, #10 and #11 as they are in this series, thanks again.
diff --git a/fs/fs.c b/fs/fs.c index 4cb4310c9cc2..70cdb594c4c8 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -791,10 +791,9 @@ int do_load(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[], return 1; } - if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) - efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "", - (argc > 4) ? argv[4] : "", map_sysmem(addr, 0), - len_read); + efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "", + (argc > 4) ? argv[4] : "", map_sysmem(addr, 0), + len_read); printf("%llu bytes read in %lu ms", len_read, time); if (time > 0) {
Now it is clear that the feature actually depends on efi interfaces, not "bootefi" command. efi_set_bootdev() will automatically be nullified if necessary efi component is disabled. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- fs/fs.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)