| Message ID | 20231218023842.281336-5-takahiro.akashi@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cmd: bootefi: refactor the code for bootmgr | expand |
Hi AKASHI, On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > 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 f33b85f92b61..82ee03b160e9 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); As I understand it, this is setting the boot device so that (if it happens to be an efi application) it will know which device it came from. But this is a hack. For bootstd, the device is known as it loads the kernel. Also it does not deal with memory allocation (nor can it). Where are we using the 'load' command to load a kernel? The distro scripts are deprecated. At some point this code should be removed. Is it too early for that? > > printf("%llu bytes read in %lu ms", len_read, time); > if (time > 0) { > -- > 2.34.1 > Regards, Simon
Am 18. Dezember 2023 16:01:40 MEZ schrieb Simon Glass <sjg@chromium.org>: >Hi AKASHI, > >On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro ><takahiro.akashi@linaro.org> wrote: >> >> 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 f33b85f92b61..82ee03b160e9 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); > >As I understand it, this is setting the boot device so that (if it >happens to be an efi application) it will know which device it came >from. But this is a hack. For bootstd, the device is known as it loads >the kernel. Please, consider what happens when the user interactively executes the load and bootefi command from the console. > >Also it does not deal with memory allocation (nor can it). > >Where are we using the 'load' command to load a kernel? The distro >scripts are deprecated. Some users use boot.scr scripts > >At some point this code should be removed. Is it too early for that? Yes, as long as we allow users to invoke the bootefi command with an address pointer. Best regards Heinrich > >> >> printf("%llu bytes read in %lu ms", len_read, time); >> if (time > 0) { >> -- >> 2.34.1 >> > >Regards, >Simon
Hi Heinrich, On Mon, 18 Dec 2023 at 15:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > Am 18. Dezember 2023 16:01:40 MEZ schrieb Simon Glass <sjg@chromium.org>: > >Hi AKASHI, > > > >On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro > ><takahiro.akashi@linaro.org> wrote: > >> > >> 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 f33b85f92b61..82ee03b160e9 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); > > > >As I understand it, this is setting the boot device so that (if it > >happens to be an efi application) it will know which device it came > >from. But this is a hack. For bootstd, the device is known as it loads > >the kernel. > > Please, consider what happens when the user interactively executes the load and bootefi command from the console. > > > > >Also it does not deal with memory allocation (nor can it). > > > >Where are we using the 'load' command to load a kernel? The distro > >scripts are deprecated. > > Some users use boot.scr scripts > > > > > >At some point this code should be removed. Is it too early for that? > > Yes, as long as we allow users to invoke the bootefi command with an address pointer. So how about we create the new path, with the info passed in as part of the bootefi call? Then we can remove the call from bootstd, at least. Regards, Simon
Am 20. Dezember 2023 05:46:27 MEZ schrieb Simon Glass <sjg@chromium.org>: >Hi Heinrich, > >On Mon, 18 Dec 2023 at 15:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> >> >> Am 18. Dezember 2023 16:01:40 MEZ schrieb Simon Glass <sjg@chromium.org>: >> >Hi AKASHI, >> > >> >On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro >> ><takahiro.akashi@linaro.org> wrote: >> >> >> >> 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 f33b85f92b61..82ee03b160e9 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); >> > >> >As I understand it, this is setting the boot device so that (if it >> >happens to be an efi application) it will know which device it came >> >from. But this is a hack. For bootstd, the device is known as it loads >> >the kernel. >> >> Please, consider what happens when the user interactively executes the load and bootefi command from the console. >> >> > >> >Also it does not deal with memory allocation (nor can it). >> > >> >Where are we using the 'load' command to load a kernel? The distro >> >scripts are deprecated. >> >> Some users use boot.scr scripts >> >> >> > >> >At some point this code should be removed. Is it too early for that? >> >> Yes, as long as we allow users to invoke the bootefi command with an address pointer. > >So how about we create the new path, with the info passed in as part >of the bootefi call? Then we can remove the call from bootstd, at >least. Hello Simon, Do you see a problem merging the current patch? Or are you talking about what we might do after this patch? Best regards Heinrich
On Tue, Dec 19, 2023 at 09:46:27PM -0700, Simon Glass wrote: > Hi Heinrich, > > On Mon, 18 Dec 2023 at 15:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > > > Am 18. Dezember 2023 16:01:40 MEZ schrieb Simon Glass <sjg@chromium.org>: > > >Hi AKASHI, > > > > > >On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro > > ><takahiro.akashi@linaro.org> wrote: > > >> > > >> 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 f33b85f92b61..82ee03b160e9 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); > > > > > >As I understand it, this is setting the boot device so that (if it > > >happens to be an efi application) it will know which device it came > > >from. But this is a hack. For bootstd, the device is known as it loads > > >the kernel. > > > > Please, consider what happens when the user interactively executes the load and bootefi command from the console. > > > > > > > >Also it does not deal with memory allocation (nor can it). > > > > > >Where are we using the 'load' command to load a kernel? The distro > > >scripts are deprecated. > > > > Some users use boot.scr scripts > > > > > > > > > >At some point this code should be removed. Is it too early for that? > > > > Yes, as long as we allow users to invoke the bootefi command with an address pointer. > > So how about we create the new path, with the info passed in as part > of the bootefi call? Then we can remove the call from bootstd, at > least. Why? It's not clear to me we'd have this information available at that point unless we stored things to pass along at that point too.
Hi, On Wed, Dec 20, 2023 at 1:25 PM Tom Rini <trini@konsulko.com> wrote: > > On Tue, Dec 19, 2023 at 09:46:27PM -0700, Simon Glass wrote: > > Hi Heinrich, > > > > On Mon, 18 Dec 2023 at 15:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > > > > > > > Am 18. Dezember 2023 16:01:40 MEZ schrieb Simon Glass <sjg@chromium.org>: > > > >Hi AKASHI, > > > > > > > >On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro > > > ><takahiro.akashi@linaro.org> wrote: > > > >> > > > >> 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 f33b85f92b61..82ee03b160e9 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); > > > > > > > >As I understand it, this is setting the boot device so that (if it > > > >happens to be an efi application) it will know which device it came > > > >from. But this is a hack. For bootstd, the device is known as it loads > > > >the kernel. > > > > > > Please, consider what happens when the user interactively executes the load and bootefi command from the console. Yes, that case needs to be handled. > > > > > > > > > > >Also it does not deal with memory allocation (nor can it). > > > > > > > >Where are we using the 'load' command to load a kernel? The distro > > > >scripts are deprecated. > > > > > > Some users use boot.scr scripts We will need to figure out that. But we should make bootstd do the right thing. > > > > > > > > > > > > > >At some point this code should be removed. Is it too early for that? > > > > > > Yes, as long as we allow users to invoke the bootefi command with an address pointer. > > > > So how about we create the new path, with the info passed in as part > > of the bootefi call? Then we can remove the call from bootstd, at > > least. > > Why? It's not clear to me we'd have this information available at that > point unless we stored things to pass along at that point too. For the bootstd case, we know the device where the kernel came from. See distro_efi_boot() for this code. It calls efiload_read_file() which calls set_efi_bootdev(). We should instead pass the device information to efi_binary_run(). With that done, we can perhaps make the 'load' and 'tftp' commands record the device information somewhere, so that bootm can pick it up when it starts and put it in 'images'. Then at some point we can clean up the 'devnr' parameter, which should really be a partition number, since the 'dev' parameter tells you the device. Anyway, this series is fine, so long as we can agree to the above. Reviewed-by: Simon Glass <sjg@chromium.org> Regards, Simon
On Tue, Dec 26, 2023 at 09:46:52AM +0000, Simon Glass wrote: > Hi, > > On Wed, Dec 20, 2023 at 1:25 PM Tom Rini <trini@konsulko.com> wrote: > > > > On Tue, Dec 19, 2023 at 09:46:27PM -0700, Simon Glass wrote: > > > Hi Heinrich, > > > > > > On Mon, 18 Dec 2023 at 15:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > > > > > > > > > > > Am 18. Dezember 2023 16:01:40 MEZ schrieb Simon Glass <sjg@chromium.org>: > > > > >Hi AKASHI, > > > > > > > > > >On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro > > > > ><takahiro.akashi@linaro.org> wrote: > > > > >> > > > > >> 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 f33b85f92b61..82ee03b160e9 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); > > > > > > > > > >As I understand it, this is setting the boot device so that (if it > > > > >happens to be an efi application) it will know which device it came > > > > >from. But this is a hack. For bootstd, the device is known as it loads > > > > >the kernel. > > > > > > > > Please, consider what happens when the user interactively executes the load and bootefi command from the console. > > Yes, that case needs to be handled. > > > > > > > > > > > > > > >Also it does not deal with memory allocation (nor can it). > > > > > > > > > >Where are we using the 'load' command to load a kernel? The distro > > > > >scripts are deprecated. > > > > > > > > Some users use boot.scr scripts > > We will need to figure out that. But we should make bootstd do the right thing. > > > > > > > > > > > > > > > > > > >At some point this code should be removed. Is it too early for that? > > > > > > > > Yes, as long as we allow users to invoke the bootefi command with an address pointer. > > > > > > So how about we create the new path, with the info passed in as part > > > of the bootefi call? Then we can remove the call from bootstd, at > > > least. > > > > Why? It's not clear to me we'd have this information available at that > > point unless we stored things to pass along at that point too. > > For the bootstd case, we know the device where the kernel came from. > See distro_efi_boot() for this code. It calls efiload_read_file() > which calls set_efi_bootdev(). > > We should instead pass the device information to efi_binary_run(). > > With that done, we can perhaps make the 'load' and 'tftp' commands > record the device information somewhere, so that bootm can pick it up > when it starts and put it in 'images'. Then at some point we can clean > up the 'devnr' parameter, which should really be a partition number, > since the 'dev' parameter tells you the device. > > Anyway, this series is fine, so long as we can agree to the above. I'm getting very worried that we're spending a lot of time deciding how users are going to use the system, without getting input from them on what their use cases are, and are not, and why they've chosen what they've chosen. Talking with some of the Debian people about why they're using script + extlinux, and Armbian people about what they're doing (which is just scripts?) should be the first priority before we decide how booting things is going to work, outside of the EFI cases (which to be clear, are having their interests / needs represented already I believe).
diff --git a/fs/fs.c b/fs/fs.c index f33b85f92b61..82ee03b160e9 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(-)