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

[Masami Hiramatsu]

Hi Sughosh,

2022年2月2日(水) 14:35 Sughosh Ganu <sughosh.ganu at linaro.org>:
>
> 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.

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 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



-- 
Masami Hiramatsu