| Message ID | 20231218023842.281336-4-takahiro.akashi@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cmd: bootefi: refactor the code for bootmgr | expand |
On Mon, Dec 18, 2023 at 5:17 AM 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> > --- > net/tftp.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/net/tftp.c b/net/tftp.c > index 88e71e67de35..2e335413492b 100644 > --- a/net/tftp.c > +++ b/net/tftp.c > @@ -302,12 +302,10 @@ static void tftp_complete(void) > time_start * 1000, "/s"); > } > puts("\ndone\n"); > - if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) { > - if (!tftp_put_active) > - efi_set_bootdev("Net", "", tftp_filename, > - map_sysmem(tftp_load_addr, 0), > - net_boot_file_size); > - } > + if (!tftp_put_active) > + efi_set_bootdev("Net", "", tftp_filename, > + map_sysmem(tftp_load_addr, 0), > + net_boot_file_size); > net_set_state(NETLOOP_SUCCESS); > } > > -- > 2.34.1 > Reviewed-by: Ramon Fried <rfried.dev@gmail.com>
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> > --- > net/tftp.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > I have the same comment here as the 'fs' patch. > diff --git a/net/tftp.c b/net/tftp.c > index 88e71e67de35..2e335413492b 100644 > --- a/net/tftp.c > +++ b/net/tftp.c > @@ -302,12 +302,10 @@ static void tftp_complete(void) > time_start * 1000, "/s"); > } > puts("\ndone\n"); > - if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) { Shouldn't this depend on your new CONFIG? What happens if EFI_LOADER is not enabled? > - if (!tftp_put_active) > - efi_set_bootdev("Net", "", tftp_filename, > - map_sysmem(tftp_load_addr, 0), > - net_boot_file_size); > - } > + if (!tftp_put_active) > + efi_set_bootdev("Net", "", tftp_filename, > + map_sysmem(tftp_load_addr, 0), > + net_boot_file_size); > net_set_state(NETLOOP_SUCCESS); > } > > -- > 2.34.1 > Regards, Simon
Hi Simon, On Mon, Dec 18, 2023 at 08:01:46AM -0700, Simon Glass wrote: > 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> > > --- > > net/tftp.c | 10 ++++------ > > 1 file changed, 4 insertions(+), 6 deletions(-) > > > > I have the same comment here as the 'fs' patch. > > > diff --git a/net/tftp.c b/net/tftp.c > > index 88e71e67de35..2e335413492b 100644 > > --- a/net/tftp.c > > +++ b/net/tftp.c > > @@ -302,12 +302,10 @@ static void tftp_complete(void) > > time_start * 1000, "/s"); > > } > > puts("\ndone\n"); > > - if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) { > > Shouldn't this depend on your new CONFIG? What happens if EFI_LOADER > is not enabled? The trick is in efi_loader.h. If EFI_LOADER (more specifically CONFIG_EFI_BINARY_EXEC) is not defined, this function gets voided. See patch#1 in this version. I took this approach in order not to make users much worried about what config be used as they are not familiar with UEFI implementation. -Takahiro Akashi > > - if (!tftp_put_active) > > - efi_set_bootdev("Net", "", tftp_filename, > > - map_sysmem(tftp_load_addr, 0), > > - net_boot_file_size); > > - } > > + if (!tftp_put_active) > > + efi_set_bootdev("Net", "", tftp_filename, > > + map_sysmem(tftp_load_addr, 0), > > + net_boot_file_size); > > net_set_state(NETLOOP_SUCCESS); > > } > > > > -- > > 2.34.1 > > > > Regards, > Simon
Hi, On Mon, 18 Dec 2023 at 17:17, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > Hi Simon, > > On Mon, Dec 18, 2023 at 08:01:46AM -0700, Simon Glass wrote: > > 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> > > > --- > > > net/tftp.c | 10 ++++------ > > > 1 file changed, 4 insertions(+), 6 deletions(-) > > > > > > > I have the same comment here as the 'fs' patch. > > > > > diff --git a/net/tftp.c b/net/tftp.c > > > index 88e71e67de35..2e335413492b 100644 > > > --- a/net/tftp.c > > > +++ b/net/tftp.c > > > @@ -302,12 +302,10 @@ static void tftp_complete(void) > > > time_start * 1000, "/s"); > > > } > > > puts("\ndone\n"); > > > - if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) { > > > > Shouldn't this depend on your new CONFIG? What happens if EFI_LOADER > > is not enabled? > > The trick is in efi_loader.h. > If EFI_LOADER (more specifically CONFIG_EFI_BINARY_EXEC) is not defined, > this function gets voided. See patch#1 in this version. > > I took this approach in order not to make users much worried about > what config be used as they are not familiar with UEFI implementation. OK, but we really need to delete this function, so what is the plan for that? The info that EFI needs should be passed to the bootefi() function, not set in a global. > > -Takahiro Akashi > > > > - if (!tftp_put_active) > > > - efi_set_bootdev("Net", "", tftp_filename, > > > - map_sysmem(tftp_load_addr, 0), > > > - net_boot_file_size); > > > - } > > > + if (!tftp_put_active) > > > + efi_set_bootdev("Net", "", tftp_filename, > > > + map_sysmem(tftp_load_addr, 0), > > > + net_boot_file_size); > > > net_set_state(NETLOOP_SUCCESS); > > > } > > > > > > -- > > > 2.34.1 Regards, Simon
Am 20. Dezember 2023 05:46:16 MEZ schrieb Simon Glass <sjg@chromium.org>: >Hi, > >On Mon, 18 Dec 2023 at 17:17, AKASHI Takahiro ><takahiro.akashi@linaro.org> wrote: >> >> Hi Simon, >> >> On Mon, Dec 18, 2023 at 08:01:46AM -0700, Simon Glass wrote: >> > 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> >> > > --- >> > > net/tftp.c | 10 ++++------ >> > > 1 file changed, 4 insertions(+), 6 deletions(-) >> > > >> > >> > I have the same comment here as the 'fs' patch. >> > >> > > diff --git a/net/tftp.c b/net/tftp.c >> > > index 88e71e67de35..2e335413492b 100644 >> > > --- a/net/tftp.c >> > > +++ b/net/tftp.c >> > > @@ -302,12 +302,10 @@ static void tftp_complete(void) >> > > time_start * 1000, "/s"); >> > > } >> > > puts("\ndone\n"); >> > > - if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) { >> > >> > Shouldn't this depend on your new CONFIG? What happens if EFI_LOADER >> > is not enabled? >> >> The trick is in efi_loader.h. >> If EFI_LOADER (more specifically CONFIG_EFI_BINARY_EXEC) is not defined, >> this function gets voided. See patch#1 in this version. >> >> I took this approach in order not to make users much worried about >> what config be used as they are not familiar with UEFI implementation. > >OK, but we really need to delete this function, so what is the plan >for that? The info that EFI needs should be passed to the bootefi() >function, not set in a global. Hello Simon, Bootstd is not the only way to boot. Please, do not forget the shell. The user loads a file with tftpboot. At some later moment the user executes bootefi. We need a place where we store the device from which the image was loaded. In future we might have a register of loaded files. But that is beyond the scope of this patch series. Best regards Heinrich > > >> >> -Takahiro Akashi >> >> > > - if (!tftp_put_active) >> > > - efi_set_bootdev("Net", "", tftp_filename, >> > > - map_sysmem(tftp_load_addr, 0), >> > > - net_boot_file_size); >> > > - } >> > > + if (!tftp_put_active) >> > > + efi_set_bootdev("Net", "", tftp_filename, >> > > + map_sysmem(tftp_load_addr, 0), >> > > + net_boot_file_size); >> > > net_set_state(NETLOOP_SUCCESS); >> > > } >> > > >> > > -- >> > > 2.34.1 > >Regards, >Simon
On 12/18/23 03:38, AKASHI Takahiro 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> > --- > net/tftp.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/net/tftp.c b/net/tftp.c > index 88e71e67de35..2e335413492b 100644 > --- a/net/tftp.c > +++ b/net/tftp.c > @@ -302,12 +302,10 @@ static void tftp_complete(void) > time_start * 1000, "/s"); > } > puts("\ndone\n"); > - if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) { > - if (!tftp_put_active) > - efi_set_bootdev("Net", "", tftp_filename, > - map_sysmem(tftp_load_addr, 0), > - net_boot_file_size); > - } > + if (!tftp_put_active) > + efi_set_bootdev("Net", "", tftp_filename, Function efi_set_bootdev() will not be compiled for CONFIG_EFI_LOADER=n. So this change may lead to build failures. Best regards Heinrich > + map_sysmem(tftp_load_addr, 0), > + net_boot_file_size); > net_set_state(NETLOOP_SUCCESS); > } >
On Mon, Dec 25, 2023 at 10:23:35AM +0100, Heinrich Schuchardt wrote: > On 12/18/23 03:38, AKASHI Takahiro 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> > > --- > > net/tftp.c | 10 ++++------ > > 1 file changed, 4 insertions(+), 6 deletions(-) > > > > diff --git a/net/tftp.c b/net/tftp.c > > index 88e71e67de35..2e335413492b 100644 > > --- a/net/tftp.c > > +++ b/net/tftp.c > > @@ -302,12 +302,10 @@ static void tftp_complete(void) > > time_start * 1000, "/s"); > > } > > puts("\ndone\n"); > > - if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) { > > - if (!tftp_put_active) > > - efi_set_bootdev("Net", "", tftp_filename, > > - map_sysmem(tftp_load_addr, 0), > > - net_boot_file_size); > > - } > > + if (!tftp_put_active) > > + efi_set_bootdev("Net", "", tftp_filename, > > Function efi_set_bootdev() will not be compiled for CONFIG_EFI_LOADER=n. > So this change may lead to build failures. In the EFI_LOADER=n case <efi_loader.h> has an empty inline instead so this should be fine. Reviewed-by: Tom Rini <trini@konsulko.com>
Hi Heinrich, On Wed, Dec 20, 2023 at 9:17 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > Am 20. Dezember 2023 05:46:16 MEZ schrieb Simon Glass <sjg@chromium.org>: > >Hi, > > > >On Mon, 18 Dec 2023 at 17:17, AKASHI Takahiro > ><takahiro.akashi@linaro.org> wrote: > >> > >> Hi Simon, > >> > >> On Mon, Dec 18, 2023 at 08:01:46AM -0700, Simon Glass wrote: > >> > 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> > >> > > --- > >> > > net/tftp.c | 10 ++++------ > >> > > 1 file changed, 4 insertions(+), 6 deletions(-) > >> > > > >> > > >> > I have the same comment here as the 'fs' patch. > >> > > >> > > diff --git a/net/tftp.c b/net/tftp.c > >> > > index 88e71e67de35..2e335413492b 100644 > >> > > --- a/net/tftp.c > >> > > +++ b/net/tftp.c > >> > > @@ -302,12 +302,10 @@ static void tftp_complete(void) > >> > > time_start * 1000, "/s"); > >> > > } > >> > > puts("\ndone\n"); > >> > > - if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) { > >> > > >> > Shouldn't this depend on your new CONFIG? What happens if EFI_LOADER > >> > is not enabled? > >> > >> The trick is in efi_loader.h. > >> If EFI_LOADER (more specifically CONFIG_EFI_BINARY_EXEC) is not defined, > >> this function gets voided. See patch#1 in this version. > >> > >> I took this approach in order not to make users much worried about > >> what config be used as they are not familiar with UEFI implementation. > > > >OK, but we really need to delete this function, so what is the plan > >for that? The info that EFI needs should be passed to the bootefi() > >function, not set in a global. > > Hello Simon, > > Bootstd is not the only way to boot. Please, do not forget the shell. > > The user loads a file with tftpboot. At some later moment the user executes bootefi. > > We need a place where we store the device from which the image was loaded. Yes, agreed. See my other reply on that. > > In future we might have a register of loaded files. But that is beyond the scope of this patch series. I believe we could just record the device and partition number (which is itself a device these days, I suppose). Then for EFI can do the translation at the start of the bootm cmd if not using bootstd. Reviewed-by: Simon Glass <sjg@chromium.org> > > Best regards > > Heinrich > > > > > > >> > >> -Takahiro Akashi > >> > >> > > - if (!tftp_put_active) > >> > > - efi_set_bootdev("Net", "", tftp_filename, > >> > > - map_sysmem(tftp_load_addr, 0), > >> > > - net_boot_file_size); > >> > > - } > >> > > + if (!tftp_put_active) > >> > > + efi_set_bootdev("Net", "", tftp_filename, > >> > > + map_sysmem(tftp_load_addr, 0), > >> > > + net_boot_file_size); > >> > > net_set_state(NETLOOP_SUCCESS); > >> > > } > >> > > > >> > > -- > >> > > 2.34.1 > > > >Regards, Regards, Simon
Hi Simon, On Tue, Dec 26, 2023 at 09:47:03AM +0000, Simon Glass wrote: > Hi Heinrich, > > On Wed, Dec 20, 2023 at 9:17???AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > > > Am 20. Dezember 2023 05:46:16 MEZ schrieb Simon Glass <sjg@chromium.org>: > > >Hi, > > > > > >On Mon, 18 Dec 2023 at 17:17, AKASHI Takahiro > > ><takahiro.akashi@linaro.org> wrote: > > >> > > >> Hi Simon, > > >> > > >> On Mon, Dec 18, 2023 at 08:01:46AM -0700, Simon Glass wrote: > > >> > 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> > > >> > > --- > > >> > > net/tftp.c | 10 ++++------ > > >> > > 1 file changed, 4 insertions(+), 6 deletions(-) > > >> > > > > >> > > > >> > I have the same comment here as the 'fs' patch. > > >> > > > >> > > diff --git a/net/tftp.c b/net/tftp.c > > >> > > index 88e71e67de35..2e335413492b 100644 > > >> > > --- a/net/tftp.c > > >> > > +++ b/net/tftp.c > > >> > > @@ -302,12 +302,10 @@ static void tftp_complete(void) > > >> > > time_start * 1000, "/s"); > > >> > > } > > >> > > puts("\ndone\n"); > > >> > > - if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) { > > >> > > > >> > Shouldn't this depend on your new CONFIG? What happens if EFI_LOADER > > >> > is not enabled? > > >> > > >> The trick is in efi_loader.h. > > >> If EFI_LOADER (more specifically CONFIG_EFI_BINARY_EXEC) is not defined, > > >> this function gets voided. See patch#1 in this version. > > >> > > >> I took this approach in order not to make users much worried about > > >> what config be used as they are not familiar with UEFI implementation. > > > > > >OK, but we really need to delete this function, so what is the plan > > >for that? The info that EFI needs should be passed to the bootefi() > > >function, not set in a global. > > > > Hello Simon, > > > > Bootstd is not the only way to boot. Please, do not forget the shell. > > > > The user loads a file with tftpboot. At some later moment the user executes bootefi. > > > > We need a place where we store the device from which the image was loaded. > > Yes, agreed. See my other reply on that. > > > > > In future we might have a register of loaded files. But that is beyond the scope of this patch series. > > I believe we could just record the device and partition number (which > is itself a device these days, I suppose). Then for EFI can do the > translation at the start of the bootm cmd if not using bootstd. Then, what is the difference between "record the device and partition number (strictly it's not accurate because we also support tftp and others.) and "call efi_set_bootdev()"? The latter does the translation on the fly and saves the info in bootefi_image_path and bootefi_device_path local variables. I believe that both are essentially the same. > Reviewed-by: Simon Glass <sjg@chromium.org> Always thank you for your review. -Takahiro Akashi > > > > > Best regards > > > > Heinrich > > > > > > > > > > >> > > >> -Takahiro Akashi > > >> > > >> > > - if (!tftp_put_active) > > >> > > - efi_set_bootdev("Net", "", tftp_filename, > > >> > > - map_sysmem(tftp_load_addr, 0), > > >> > > - net_boot_file_size); > > >> > > - } > > >> > > + if (!tftp_put_active) > > >> > > + efi_set_bootdev("Net", "", tftp_filename, > > >> > > + map_sysmem(tftp_load_addr, 0), > > >> > > + net_boot_file_size); > > >> > > net_set_state(NETLOOP_SUCCESS); > > >> > > } > > >> > > > > >> > > -- > > >> > > 2.34.1 > > > > > >Regards, > > Regards, > Simon
diff --git a/net/tftp.c b/net/tftp.c index 88e71e67de35..2e335413492b 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -302,12 +302,10 @@ static void tftp_complete(void) time_start * 1000, "/s"); } puts("\ndone\n"); - if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) { - if (!tftp_put_active) - efi_set_bootdev("Net", "", tftp_filename, - map_sysmem(tftp_load_addr, 0), - net_boot_file_size); - } + if (!tftp_put_active) + efi_set_bootdev("Net", "", tftp_filename, + map_sysmem(tftp_load_addr, 0), + net_boot_file_size); net_set_state(NETLOOP_SUCCESS); }
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> --- net/tftp.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)