| Message ID | 164370437972.365087.4761473791973948840.stgit@localhost |
|---|---|
| State | Superseded |
| Headers | show |
| Series | EFI: Reset system after capsule-on-disk | expand |
hi Masami, On Tue, 1 Feb 2022 at 14:03, Masami Hiramatsu <masami.hiramatsu@linaro.org> wrote: > > The efi_update_capsule() may have to handle the capsule flags as an UEFI > runtime and boottime service, but the capsule-on-disk process doesn't. > Thus, the capsule-on-disk should use the efi_capsule_update_firmware() > directly instead of efi_update_capsule(). > > Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org> > --- > Changes in v2: > - Fix to pass correct pointer to efi_capsule_update_firmware > - Remove ESRT generation, because this part anyway will be removed > next patch. > --- > lib/efi_loader/efi_capsule.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > index 4463ae00fd..1ec7ea29ff 100644 > --- a/lib/efi_loader/efi_capsule.c > +++ b/lib/efi_loader/efi_capsule.c > @@ -1118,7 +1118,7 @@ efi_status_t efi_launch_capsules(void) > index = 0; > ret = efi_capsule_read_file(files[i], &capsule); > if (ret == EFI_SUCCESS) { > - ret = EFI_CALL(efi_update_capsule(&capsule, 1, 0)); > + ret = efi_capsule_update_firmware(capsule); I believe this is not fixing any issue as such. If so, I would vote for keeping the call to efi_update_capsule. With the FWU Multi Bank feature enabled, the checks for capsule acceptance and revert are being done in this function. The reason I have put this code in the function is that it caters to both scenarios of capsule-on-disk and the runtime functionality. In addition, the FWU bootup checks are also done in this function through a call to fwu_update_checks_pass. So if this is not a fix, which I don't think it is, I would prefer this call to remain. -sughosh > if (ret != EFI_SUCCESS) > log_err("Applying capsule %ls failed\n", > files[i]); >
Am 1. Februar 2022 16:42:43 MEZ schrieb Sughosh Ganu <sughosh.ganu@linaro.org>: >hi Masami, > >On Tue, 1 Feb 2022 at 14:03, Masami Hiramatsu ><masami.hiramatsu@linaro.org> wrote: >> >> The efi_update_capsule() may have to handle the capsule flags as an UEFI >> runtime and boottime service, but the capsule-on-disk process doesn't. >> Thus, the capsule-on-disk should use the efi_capsule_update_firmware() >> directly instead of efi_update_capsule(). >> >> Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org> >> --- >> Changes in v2: >> - Fix to pass correct pointer to efi_capsule_update_firmware >> - Remove ESRT generation, because this part anyway will be removed >> next patch. >> --- >> lib/efi_loader/efi_capsule.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c >> index 4463ae00fd..1ec7ea29ff 100644 >> --- a/lib/efi_loader/efi_capsule.c >> +++ b/lib/efi_loader/efi_capsule.c >> @@ -1118,7 +1118,7 @@ efi_status_t efi_launch_capsules(void) >> index = 0; >> ret = efi_capsule_read_file(files[i], &capsule); >> if (ret == EFI_SUCCESS) { >> - ret = EFI_CALL(efi_update_capsule(&capsule, 1, 0)); >> + ret = efi_capsule_update_firmware(capsule); > >I believe this is not fixing any issue as such. If so, I would vote >for keeping the call to efi_update_capsule. No, this is just about reducing code size by avoiding the EFI_CALL(). It should not change behaviour. Best regards Heinrich With the FWU Multi Bank >feature enabled, the checks for capsule acceptance and revert are >being done in this function. The reason I have put this code in the >function is that it caters to both scenarios of capsule-on-disk and >the runtime functionality. In addition, the FWU bootup checks are also >done in this function through a call to fwu_update_checks_pass. So if >this is not a fix, which I don't think it is, I would prefer this call >to remain. > >-sughosh > >> if (ret != EFI_SUCCESS) >> log_err("Applying capsule %ls failed\n", >> files[i]); >>
On Tue, 1 Feb 2022 at 22:14, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > Am 1. Februar 2022 16:42:43 MEZ schrieb Sughosh Ganu <sughosh.ganu@linaro.org>: > >hi Masami, > > > >On Tue, 1 Feb 2022 at 14:03, Masami Hiramatsu > ><masami.hiramatsu@linaro.org> wrote: > >> > >> The efi_update_capsule() may have to handle the capsule flags as an UEFI > >> runtime and boottime service, but the capsule-on-disk process doesn't. > >> Thus, the capsule-on-disk should use the efi_capsule_update_firmware() > >> directly instead of efi_update_capsule(). > >> > >> Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org> > >> --- > >> Changes in v2: > >> - Fix to pass correct pointer to efi_capsule_update_firmware > >> - Remove ESRT generation, because this part anyway will be removed > >> next patch. > >> --- > >> lib/efi_loader/efi_capsule.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > >> index 4463ae00fd..1ec7ea29ff 100644 > >> --- a/lib/efi_loader/efi_capsule.c > >> +++ b/lib/efi_loader/efi_capsule.c > >> @@ -1118,7 +1118,7 @@ efi_status_t efi_launch_capsules(void) > >> index = 0; > >> ret = efi_capsule_read_file(files[i], &capsule); > >> if (ret == EFI_SUCCESS) { > >> - ret = EFI_CALL(efi_update_capsule(&capsule, 1, 0)); > >> + ret = efi_capsule_update_firmware(capsule); > > > >I believe this is not fixing any issue as such. If so, I would vote > >for keeping the call to efi_update_capsule. > > No, this is just about reducing code size by avoiding the EFI_CALL(). It should not change behaviour. Okay, in that case, I will put a check for the FWU Multi Banks feature being enabled -- with the feature enabled, the call will be to efi_update_capsule, and with the feature disabled, the call will be made to efi_capsule_update_firmware. The compiler should compile out the code whenever the FWU feature is disabled and that will not impact the code size. -sughosh > > Best regards > > Heinrich > > With the FWU Multi Bank > >feature enabled, the checks for capsule acceptance and revert are > >being done in this function. The reason I have put this code in the > >function is that it caters to both scenarios of capsule-on-disk and > >the runtime functionality. In addition, the FWU bootup checks are also > >done in this function through a call to fwu_update_checks_pass. So if > >this is not a fix, which I don't think it is, I would prefer this call > >to remain. > > > >-sughosh > > > >> if (ret != EFI_SUCCESS) > >> log_err("Applying capsule %ls failed\n", > >> files[i]); > >>
On Tue, Feb 01, 2022 at 10:33:20PM +0530, Sughosh Ganu wrote: > On Tue, 1 Feb 2022 at 22:14, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > > > Am 1. Februar 2022 16:42:43 MEZ schrieb Sughosh Ganu <sughosh.ganu@linaro.org>: > > >hi Masami, > > > > > >On Tue, 1 Feb 2022 at 14:03, Masami Hiramatsu > > ><masami.hiramatsu@linaro.org> wrote: > > >> > > >> The efi_update_capsule() may have to handle the capsule flags as an UEFI > > >> runtime and boottime service, but the capsule-on-disk process doesn't. > > >> Thus, the capsule-on-disk should use the efi_capsule_update_firmware() > > >> directly instead of efi_update_capsule(). > > >> > > >> Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org> > > >> --- > > >> Changes in v2: > > >> - Fix to pass correct pointer to efi_capsule_update_firmware > > >> - Remove ESRT generation, because this part anyway will be removed > > >> next patch. > > >> --- > > >> lib/efi_loader/efi_capsule.c | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > > >> index 4463ae00fd..1ec7ea29ff 100644 > > >> --- a/lib/efi_loader/efi_capsule.c > > >> +++ b/lib/efi_loader/efi_capsule.c > > >> @@ -1118,7 +1118,7 @@ efi_status_t efi_launch_capsules(void) > > >> index = 0; > > >> ret = efi_capsule_read_file(files[i], &capsule); > > >> if (ret == EFI_SUCCESS) { > > >> - ret = EFI_CALL(efi_update_capsule(&capsule, 1, 0)); > > >> + ret = efi_capsule_update_firmware(capsule); > > > > > >I believe this is not fixing any issue as such. If so, I would vote > > >for keeping the call to efi_update_capsule. > > > > No, this is just about reducing code size by avoiding the EFI_CALL(). It should not change behaviour. > > Okay, in that case, I will put a check for the FWU Multi Banks feature > being enabled -- with the feature enabled, the call will be to > efi_update_capsule, and with the feature disabled, the call will be > made to efi_capsule_update_firmware. Please don't do that. Instead, you should carve out a *common* function for UpdateCapsule api and capsule-on-disk. Please note, as I repeatedly said, that I didn't intend to implement the API with my initial commits. I think I should not have added efi_update_capsule() function to avoid any confusion. -Takahiro Akashi > The compiler should compile out > the code whenever the FWU feature is disabled and that will not impact > the code size. > > -sughosh > > > > > Best regards > > > > Heinrich > > > > With the FWU Multi Bank > > >feature enabled, the checks for capsule acceptance and revert are > > >being done in this function. The reason I have put this code in the > > >function is that it caters to both scenarios of capsule-on-disk and > > >the runtime functionality. In addition, the FWU bootup checks are also > > >done in this function through a call to fwu_update_checks_pass. So if > > >this is not a fix, which I don't think it is, I would prefer this call > > >to remain. > > > > > >-sughosh > > > > > >> if (ret != EFI_SUCCESS) > > >> log_err("Applying capsule %ls failed\n", > > >> files[i]); > > >>
Hi Sughosh, Could you tell me why do you need to do the FWU code in the efi_update_capsule? If you need to add some logic to both of the efi_update_capsule API and capsule-on-disk, it is better to be implemented in the efi_capsule_update_firmware() as a common part. Or, make an independent additional function and call it from both path. This is for decoupling the EFI boottime API wrapper (efi_capsule_update) from the capsule update logic itself. Thank you, 2022年2月2日(水) 2:03 Sughosh Ganu <sughosh.ganu@linaro.org>: > > On Tue, 1 Feb 2022 at 22:14, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > > > Am 1. Februar 2022 16:42:43 MEZ schrieb Sughosh Ganu <sughosh.ganu@linaro.org>: > > >hi Masami, > > > > > >On Tue, 1 Feb 2022 at 14:03, Masami Hiramatsu > > ><masami.hiramatsu@linaro.org> wrote: > > >> > > >> The efi_update_capsule() may have to handle the capsule flags as an UEFI > > >> runtime and boottime service, but the capsule-on-disk process doesn't. > > >> Thus, the capsule-on-disk should use the efi_capsule_update_firmware() > > >> directly instead of efi_update_capsule(). > > >> > > >> Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org> > > >> --- > > >> Changes in v2: > > >> - Fix to pass correct pointer to efi_capsule_update_firmware > > >> - Remove ESRT generation, because this part anyway will be removed > > >> next patch. > > >> --- > > >> lib/efi_loader/efi_capsule.c | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > > >> index 4463ae00fd..1ec7ea29ff 100644 > > >> --- a/lib/efi_loader/efi_capsule.c > > >> +++ b/lib/efi_loader/efi_capsule.c > > >> @@ -1118,7 +1118,7 @@ efi_status_t efi_launch_capsules(void) > > >> index = 0; > > >> ret = efi_capsule_read_file(files[i], &capsule); > > >> if (ret == EFI_SUCCESS) { > > >> - ret = EFI_CALL(efi_update_capsule(&capsule, 1, 0)); > > >> + ret = efi_capsule_update_firmware(capsule); > > > > > >I believe this is not fixing any issue as such. If so, I would vote > > >for keeping the call to efi_update_capsule. > > > > No, this is just about reducing code size by avoiding the EFI_CALL(). It should not change behaviour. > > Okay, in that case, I will put a check for the FWU Multi Banks feature > being enabled -- with the feature enabled, the call will be to > efi_update_capsule, and with the feature disabled, the call will be > made to efi_capsule_update_firmware. The compiler should compile out > the code whenever the FWU feature is disabled and that will not impact > the code size. > > -sughosh > > > > > Best regards > > > > Heinrich > > > > With the FWU Multi Bank > > >feature enabled, the checks for capsule acceptance and revert are > > >being done in this function. The reason I have put this code in the > > >function is that it caters to both scenarios of capsule-on-disk and > > >the runtime functionality. In addition, the FWU bootup checks are also > > >done in this function through a call to fwu_update_checks_pass. So if > > >this is not a fix, which I don't think it is, I would prefer this call > > >to remain. > > > > > >-sughosh > > > > > >> if (ret != EFI_SUCCESS) > > >> log_err("Applying capsule %ls failed\n", > > >> files[i]); > > >>
On Wed, 2 Feb 2022 at 05:17, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > On Tue, Feb 01, 2022 at 10:33:20PM +0530, Sughosh Ganu wrote: > > On Tue, 1 Feb 2022 at 22:14, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > > > > > > > Am 1. Februar 2022 16:42:43 MEZ schrieb Sughosh Ganu <sughosh.ganu@linaro.org>: > > > >hi Masami, > > > > > > > >On Tue, 1 Feb 2022 at 14:03, Masami Hiramatsu > > > ><masami.hiramatsu@linaro.org> wrote: > > > >> > > > >> The efi_update_capsule() may have to handle the capsule flags as an UEFI > > > >> runtime and boottime service, but the capsule-on-disk process doesn't. > > > >> Thus, the capsule-on-disk should use the efi_capsule_update_firmware() > > > >> directly instead of efi_update_capsule(). > > > >> > > > >> Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org> > > > >> --- > > > >> Changes in v2: > > > >> - Fix to pass correct pointer to efi_capsule_update_firmware > > > >> - Remove ESRT generation, because this part anyway will be removed > > > >> next patch. > > > >> --- > > > >> lib/efi_loader/efi_capsule.c | 2 +- > > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > > >> > > > >> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > > > >> index 4463ae00fd..1ec7ea29ff 100644 > > > >> --- a/lib/efi_loader/efi_capsule.c > > > >> +++ b/lib/efi_loader/efi_capsule.c > > > >> @@ -1118,7 +1118,7 @@ efi_status_t efi_launch_capsules(void) > > > >> index = 0; > > > >> ret = efi_capsule_read_file(files[i], &capsule); > > > >> if (ret == EFI_SUCCESS) { > > > >> - ret = EFI_CALL(efi_update_capsule(&capsule, 1, 0)); > > > >> + ret = efi_capsule_update_firmware(capsule); > > > > > > > >I believe this is not fixing any issue as such. If so, I would vote > > > >for keeping the call to efi_update_capsule. > > > > > > No, this is just about reducing code size by avoiding the EFI_CALL(). It should not change behaviour. > > > > Okay, in that case, I will put a check for the FWU Multi Banks feature > > being enabled -- with the feature enabled, the call will be to > > efi_update_capsule, and with the feature disabled, the call will be > > made to efi_capsule_update_firmware. > > Please don't do that. > Instead, you should carve out a *common* function for UpdateCapsule api > and capsule-on-disk. Can you also point out the issue you see with having the FWU checks in the efi_update_capsule. As I have said, having the checks here caters to both the scenarios -- capsule-on-disk update as well as secure world update. I think with the FWU feature enabled for secure world, the efi_update_capsule function will get called, before branching off to a different FMP. > Please note, as I repeatedly said, that I didn't intend to implement > the API with my initial commits. I think I should not have added > efi_update_capsule() function to avoid any confusion. Maybe I missed this, but I don't know why you think the efi_update_capsule is superfluous. Also, if it really is superfluous, this commit from Masami should also be removing the function definition rather than just not calling the function. -sughosh > > -Takahiro Akashi > > > The compiler should compile out > > the code whenever the FWU feature is disabled and that will not impact > > the code size. > > > > -sughosh > > > > > > > > Best regards > > > > > > Heinrich > > > > > > With the FWU Multi Bank > > > >feature enabled, the checks for capsule acceptance and revert are > > > >being done in this function. The reason I have put this code in the > > > >function is that it caters to both scenarios of capsule-on-disk and > > > >the runtime functionality. In addition, the FWU bootup checks are also > > > >done in this function through a call to fwu_update_checks_pass. So if > > > >this is not a fix, which I don't think it is, I would prefer this call > > > >to remain. > > > > > > > >-sughosh > > > > > > > >> if (ret != EFI_SUCCESS) > > > >> log_err("Applying capsule %ls failed\n", > > > >> files[i]); > > > >>
hi Masami, On Wed, 2 Feb 2022 at 05:39, Masami Hiramatsu <masami.hiramatsu@linaro.org> wrote: > > Hi Sughosh, > > Could you tell me why do you need to do the FWU code in the efi_update_capsule? I thought I explained this in my previous email. Putting the FWU checks in efi_update_capsule caters to the scenario where FWU updates are being done in secure world. Even for such scenario, the efi_update_capsule function will get called. So having the checks in one single place is better. > If you need to add some logic to both of the efi_update_capsule API > and capsule-on-disk, > it is better to be implemented in the efi_capsule_update_firmware() as > a common part. > Or, make an independent additional function and call it from both path. > This is for decoupling the EFI boottime API wrapper (efi_capsule_update) from > the capsule update logic itself. Like I asked Takahiro, I don't understand why you find the efi_update_capsule function superfluous. I do see it being called for secure world FWU updates. Also, if the function is indeed superfluous, you should also be removing the function definition as well as part of this patch. -sughosh > > Thank you, > > > 2022年2月2日(水) 2:03 Sughosh Ganu <sughosh.ganu@linaro.org>: > > > > On Tue, 1 Feb 2022 at 22:14, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > > > > > > > Am 1. Februar 2022 16:42:43 MEZ schrieb Sughosh Ganu <sughosh.ganu@linaro.org>: > > > >hi Masami, > > > > > > > >On Tue, 1 Feb 2022 at 14:03, Masami Hiramatsu > > > ><masami.hiramatsu@linaro.org> wrote: > > > >> > > > >> The efi_update_capsule() may have to handle the capsule flags as an UEFI > > > >> runtime and boottime service, but the capsule-on-disk process doesn't. > > > >> Thus, the capsule-on-disk should use the efi_capsule_update_firmware() > > > >> directly instead of efi_update_capsule(). > > > >> > > > >> Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org> > > > >> --- > > > >> Changes in v2: > > > >> - Fix to pass correct pointer to efi_capsule_update_firmware > > > >> - Remove ESRT generation, because this part anyway will be removed > > > >> next patch. > > > >> --- > > > >> lib/efi_loader/efi_capsule.c | 2 +- > > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > > >> > > > >> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > > > >> index 4463ae00fd..1ec7ea29ff 100644 > > > >> --- a/lib/efi_loader/efi_capsule.c > > > >> +++ b/lib/efi_loader/efi_capsule.c > > > >> @@ -1118,7 +1118,7 @@ efi_status_t efi_launch_capsules(void) > > > >> index = 0; > > > >> ret = efi_capsule_read_file(files[i], &capsule); > > > >> if (ret == EFI_SUCCESS) { > > > >> - ret = EFI_CALL(efi_update_capsule(&capsule, 1, 0)); > > > >> + ret = efi_capsule_update_firmware(capsule); > > > > > > > >I believe this is not fixing any issue as such. If so, I would vote > > > >for keeping the call to efi_update_capsule. > > > > > > No, this is just about reducing code size by avoiding the EFI_CALL(). It should not change behaviour. > > > > Okay, in that case, I will put a check for the FWU Multi Banks feature > > being enabled -- with the feature enabled, the call will be to > > efi_update_capsule, and with the feature disabled, the call will be > > made to efi_capsule_update_firmware. The compiler should compile out > > the code whenever the FWU feature is disabled and that will not impact > > the code size. > > > > -sughosh > > > > > > > > Best regards > > > > > > Heinrich > > > > > > With the FWU Multi Bank > > > >feature enabled, the checks for capsule acceptance and revert are > > > >being done in this function. The reason I have put this code in the > > > >function is that it caters to both scenarios of capsule-on-disk and > > > >the runtime functionality. In addition, the FWU bootup checks are also > > > >done in this function through a call to fwu_update_checks_pass. So if > > > >this is not a fix, which I don't think it is, I would prefer this call > > > >to remain. > > > > > > > >-sughosh > > > > > > > >> if (ret != EFI_SUCCESS) > > > >> log_err("Applying capsule %ls failed\n", > > > >> files[i]); > > > >> > > > > -- > Masami Hiramatsu
Hi Sughosh, 2022年2月2日(水) 14:35 Sughosh Ganu <sughosh.ganu@linaro.org>: > > hi Masami, > > On Wed, 2 Feb 2022 at 05:39, Masami Hiramatsu > <masami.hiramatsu@linaro.org> wrote: > > > > Hi Sughosh, > > > > Could you tell me why do you need to do the FWU code in the efi_update_capsule? > > I thought I explained this in my previous email. Putting the FWU > checks in efi_update_capsule caters to the scenario where FWU updates > are being done in secure world. Even for such scenario, the > efi_update_capsule function will get called. So having the checks in > one single place is better. Hmm, I'm not so sure the process flow of when the FWU update are being done in secure world. What will happen? [OS] -> [UEFI UpdateCapsule()] -(SMC)> [secure FWU] -> [update firmware] ? Or, [OS] -(SMC)> [secure FWU] -> [UEFI UpdateCapsule()] -> [update firmware] ? And anyway, if the FWU is done in secure world, will the FWU metadata be processed in the secure world too? (in this case, U-boot may not do anything about firmware update but just an interface, right?) > > > If you need to add some logic to both of the efi_update_capsule API > > and capsule-on-disk, > > it is better to be implemented in the efi_capsule_update_firmware() as > > a common part. > > Or, make an independent additional function and call it from both path. > > This is for decoupling the EFI boottime API wrapper (efi_capsule_update) from > > the capsule update logic itself. > > Like I asked Takahiro, I don't understand why you find the > efi_update_capsule function superfluous. I do see it being called for > secure world FWU updates. Also, if the function is indeed superfluous, > you should also be removing the function definition as well as part of > this patch. We don't said that the efi_update_capsule() is superfluous, but it has a different role (e.g. processing multiple capsules and handle the capsule flags) as UpdateCapsule() UEFI service API, which is defined in UEFI spec. This means we will allow user to run CapsuleApp.efi on U-Boot. If it has to call secure world for FWU, I think that should be done in the efi_update_capsule_firmware(), so that that is called from *both* of UpdateCapsule() API and Capsule-on-disk. Thank you, > > -sughosh > > > > > Thank you, > > > > > > 2022年2月2日(水) 2:03 Sughosh Ganu <sughosh.ganu@linaro.org>: > > > > > > On Tue, 1 Feb 2022 at 22:14, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > > > > > > > > > > > Am 1. Februar 2022 16:42:43 MEZ schrieb Sughosh Ganu <sughosh.ganu@linaro.org>: > > > > >hi Masami, > > > > > > > > > >On Tue, 1 Feb 2022 at 14:03, Masami Hiramatsu > > > > ><masami.hiramatsu@linaro.org> wrote: > > > > >> > > > > >> The efi_update_capsule() may have to handle the capsule flags as an UEFI > > > > >> runtime and boottime service, but the capsule-on-disk process doesn't. > > > > >> Thus, the capsule-on-disk should use the efi_capsule_update_firmware() > > > > >> directly instead of efi_update_capsule(). > > > > >> > > > > >> Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org> > > > > >> --- > > > > >> Changes in v2: > > > > >> - Fix to pass correct pointer to efi_capsule_update_firmware > > > > >> - Remove ESRT generation, because this part anyway will be removed > > > > >> next patch. > > > > >> --- > > > > >> lib/efi_loader/efi_capsule.c | 2 +- > > > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > > > >> > > > > >> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > > > > >> index 4463ae00fd..1ec7ea29ff 100644 > > > > >> --- a/lib/efi_loader/efi_capsule.c > > > > >> +++ b/lib/efi_loader/efi_capsule.c > > > > >> @@ -1118,7 +1118,7 @@ efi_status_t efi_launch_capsules(void) > > > > >> index = 0; > > > > >> ret = efi_capsule_read_file(files[i], &capsule); > > > > >> if (ret == EFI_SUCCESS) { > > > > >> - ret = EFI_CALL(efi_update_capsule(&capsule, 1, 0)); > > > > >> + ret = efi_capsule_update_firmware(capsule); > > > > > > > > > >I believe this is not fixing any issue as such. If so, I would vote > > > > >for keeping the call to efi_update_capsule. > > > > > > > > No, this is just about reducing code size by avoiding the EFI_CALL(). It should not change behaviour. > > > > > > Okay, in that case, I will put a check for the FWU Multi Banks feature > > > being enabled -- with the feature enabled, the call will be to > > > efi_update_capsule, and with the feature disabled, the call will be > > > made to efi_capsule_update_firmware. The compiler should compile out > > > the code whenever the FWU feature is disabled and that will not impact > > > the code size. > > > > > > -sughosh > > > > > > > > > > > Best regards > > > > > > > > Heinrich > > > > > > > > With the FWU Multi Bank > > > > >feature enabled, the checks for capsule acceptance and revert are > > > > >being done in this function. The reason I have put this code in the > > > > >function is that it caters to both scenarios of capsule-on-disk and > > > > >the runtime functionality. In addition, the FWU bootup checks are also > > > > >done in this function through a call to fwu_update_checks_pass. So if > > > > >this is not a fix, which I don't think it is, I would prefer this call > > > > >to remain. > > > > > > > > > >-sughosh > > > > > > > > > >> if (ret != EFI_SUCCESS) > > > > >> log_err("Applying capsule %ls failed\n", > > > > >> files[i]); > > > > >> > > > > > > > > -- > > Masami Hiramatsu
hi Masami, On Wed, 2 Feb 2022 at 12:33, Masami Hiramatsu <masami.hiramatsu@linaro.org> wrote: > > Hi Sughosh, > > 2022年2月2日(水) 14:35 Sughosh Ganu <sughosh.ganu@linaro.org>: > > > > hi Masami, > > > > On Wed, 2 Feb 2022 at 05:39, Masami Hiramatsu > > <masami.hiramatsu@linaro.org> wrote: > > > > > > Hi Sughosh, > > > > > > Could you tell me why do you need to do the FWU code in the efi_update_capsule? > > > > I thought I explained this in my previous email. Putting the FWU > > checks in efi_update_capsule caters to the scenario where FWU updates > > are being done in secure world. Even for such scenario, the > > efi_update_capsule function will get called. So having the checks in > > one single place is better. > > Hmm, I'm not so sure the process flow of when the FWU update are > being done in secure world. What will happen? > > [OS] -> [UEFI UpdateCapsule()] -(SMC)> [secure FWU] -> [update firmware] ? Yes, this would be the flow. > > Or, > > [OS] -(SMC)> [secure FWU] -> [UEFI UpdateCapsule()] -> [update firmware] ? > > And anyway, if the FWU is done in secure world, will the FWU metadata > be processed in the secure world too? (in this case, U-boot may not do > anything about firmware update but just an interface, right?) I think certain api's can be re-used, but I will re-check Jose's secure FWU implementation. > > > > > > If you need to add some logic to both of the efi_update_capsule API > > > and capsule-on-disk, > > > it is better to be implemented in the efi_capsule_update_firmware() as > > > a common part. > > > Or, make an independent additional function and call it from both path. > > > This is for decoupling the EFI boottime API wrapper (efi_capsule_update) from > > > the capsule update logic itself. > > > > Like I asked Takahiro, I don't understand why you find the > > efi_update_capsule function superfluous. I do see it being called for > > secure world FWU updates. Also, if the function is indeed superfluous, > > you should also be removing the function definition as well as part of > > this patch. > > We don't said that the efi_update_capsule() is superfluous, but it has > a different role (e.g. processing multiple capsules and handle the > capsule flags) as UpdateCapsule() UEFI service API, which is defined > in UEFI spec. This means we will allow user to run CapsuleApp.efi on > U-Boot. > > If it has to call secure world for FWU, I think that should be done in the > efi_update_capsule_firmware(), so that that is called from *both* of > UpdateCapsule() API and Capsule-on-disk. Okay. In that case, I will put the checks in efi_update_capsule_firmware. Can you please expand your commit message a bit to explain why is the call to efi_update_capsule being bypassed. I believe there was a discussion between you and Takahiro where there is a more detailed explanation. It would help to have that in the commit message. Thanks. -sughosh > > Thank you, > > > > > -sughosh > > > > > > > > Thank you, > > > > > > > > > 2022年2月2日(水) 2:03 Sughosh Ganu <sughosh.ganu@linaro.org>: > > > > > > > > On Tue, 1 Feb 2022 at 22:14, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > > > > > > > > > > > > > > > Am 1. Februar 2022 16:42:43 MEZ schrieb Sughosh Ganu <sughosh.ganu@linaro.org>: > > > > > >hi Masami, > > > > > > > > > > > >On Tue, 1 Feb 2022 at 14:03, Masami Hiramatsu > > > > > ><masami.hiramatsu@linaro.org> wrote: > > > > > >> > > > > > >> The efi_update_capsule() may have to handle the capsule flags as an UEFI > > > > > >> runtime and boottime service, but the capsule-on-disk process doesn't. > > > > > >> Thus, the capsule-on-disk should use the efi_capsule_update_firmware() > > > > > >> directly instead of efi_update_capsule(). > > > > > >> > > > > > >> Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org> > > > > > >> --- > > > > > >> Changes in v2: > > > > > >> - Fix to pass correct pointer to efi_capsule_update_firmware > > > > > >> - Remove ESRT generation, because this part anyway will be removed > > > > > >> next patch. > > > > > >> --- > > > > > >> lib/efi_loader/efi_capsule.c | 2 +- > > > > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > >> > > > > > >> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > > > > > >> index 4463ae00fd..1ec7ea29ff 100644 > > > > > >> --- a/lib/efi_loader/efi_capsule.c > > > > > >> +++ b/lib/efi_loader/efi_capsule.c > > > > > >> @@ -1118,7 +1118,7 @@ efi_status_t efi_launch_capsules(void) > > > > > >> index = 0; > > > > > >> ret = efi_capsule_read_file(files[i], &capsule); > > > > > >> if (ret == EFI_SUCCESS) { > > > > > >> - ret = EFI_CALL(efi_update_capsule(&capsule, 1, 0)); > > > > > >> + ret = efi_capsule_update_firmware(capsule); > > > > > > > > > > > >I believe this is not fixing any issue as such. If so, I would vote > > > > > >for keeping the call to efi_update_capsule. > > > > > > > > > > No, this is just about reducing code size by avoiding the EFI_CALL(). It should not change behaviour. > > > > > > > > Okay, in that case, I will put a check for the FWU Multi Banks feature > > > > being enabled -- with the feature enabled, the call will be to > > > > efi_update_capsule, and with the feature disabled, the call will be > > > > made to efi_capsule_update_firmware. The compiler should compile out > > > > the code whenever the FWU feature is disabled and that will not impact > > > > the code size. > > > > > > > > -sughosh > > > > > > > > > > > > > > Best regards > > > > > > > > > > Heinrich > > > > > > > > > > With the FWU Multi Bank > > > > > >feature enabled, the checks for capsule acceptance and revert are > > > > > >being done in this function. The reason I have put this code in the > > > > > >function is that it caters to both scenarios of capsule-on-disk and > > > > > >the runtime functionality. In addition, the FWU bootup checks are also > > > > > >done in this function through a call to fwu_update_checks_pass. So if > > > > > >this is not a fix, which I don't think it is, I would prefer this call > > > > > >to remain. > > > > > > > > > > > >-sughosh > > > > > > > > > > > >> if (ret != EFI_SUCCESS) > > > > > >> log_err("Applying capsule %ls failed\n", > > > > > >> files[i]); > > > > > >> > > > > > > > > > > > > -- > > > Masami Hiramatsu > > > > -- > Masami Hiramatsu
diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c index 4463ae00fd..1ec7ea29ff 100644 --- a/lib/efi_loader/efi_capsule.c +++ b/lib/efi_loader/efi_capsule.c @@ -1118,7 +1118,7 @@ efi_status_t efi_launch_capsules(void) index = 0; ret = efi_capsule_read_file(files[i], &capsule); if (ret == EFI_SUCCESS) { - ret = EFI_CALL(efi_update_capsule(&capsule, 1, 0)); + ret = efi_capsule_update_firmware(capsule); if (ret != EFI_SUCCESS) log_err("Applying capsule %ls failed\n", files[i]);
The efi_update_capsule() may have to handle the capsule flags as an UEFI runtime and boottime service, but the capsule-on-disk process doesn't. Thus, the capsule-on-disk should use the efi_capsule_update_firmware() directly instead of efi_update_capsule(). Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org> --- Changes in v2: - Fix to pass correct pointer to efi_capsule_update_firmware - Remove ESRT generation, because this part anyway will be removed next patch. --- lib/efi_loader/efi_capsule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)