[PATCH v2 1/2] efi_loader: Avoid using efi_update_capsule() from update capsule on disk

[Sughosh Ganu]

hi Masami,

On Wed, 2 Feb 2022 at 05:39, Masami Hiramatsu
<masami.hiramatsu at 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 at linaro.org>:
> >
> > On Tue, 1 Feb 2022 at 22:14, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> > >
> > >
> > >
> > > Am 1. Februar 2022 16:42:43 MEZ schrieb Sughosh Ganu <sughosh.ganu at linaro.org>:
> > > >hi Masami,
> > > >
> > > >On Tue, 1 Feb 2022 at 14:03, Masami Hiramatsu
> > > ><masami.hiramatsu at 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 at linaro.org>
> > > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu at 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