Re: [PATCH] efi_loader: Allow also empty capsule to be process

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Thu, Jul 20, 2023 at 10:42:10PM +0200, Heinrich Schuchardt wrote:
> On 7/20/23 11:48, Sughosh Ganu wrote:
> > On Thu, 20 Jul 2023 at 14:56, Michal Simek <michal.simek@amd.com> wrote:
> > > 
> > > 
> > > 
> > > On 7/20/23 10:45, Sughosh Ganu wrote:
> > > > On Thu, 20 Jul 2023 at 13:26, Michal Simek <michal.simek@amd.com> wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > On 7/20/23 08:36, Sughosh Ganu wrote:
> > > > > > On Thu, 20 Jul 2023 at 11:37, Michal Simek <michal.simek@amd.com> wrote:
> > > > > > > 
> > > > > > > Hi,
> > > > > > > 
> > > > > > > On 7/20/23 07:49, AKASHI Takahiro wrote:
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > On Wed, Jul 19, 2023 at 08:28:41AM +0200, Michal Simek wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On 7/18/23 17:41, Heinrich Schuchardt wrote:
> > > > > > > > > > On 13.07.23 16:35, Michal Simek wrote:
> > > > > > > > > > > Empty capsule are also allowed to be process. Without it updated images
> > > > > > > > > > > can't change their Image Acceptance state from no to yes.
> > > > > > > > > > 
> > > > > > > > > > Is there any documentation describing the usage of empty capsule to set
> > > > > > > > > > the image acceptance state?
> > > > > > > > > 
> > > > > > > > > I actually don't know about documentation. I was talking to Ilias to make
> > > > > > > > > sure that documentation is up2date because there are missing couple of
> > > > > > > > > things there.
> > > > > > > > 
> > > > > > > > Sughosh should have more to say here about A/B update.
> > > > > > > > 
> > > > > > > > > I am testing A/B update and if you setup oemflags to 0x8000 then capsules
> > > > > > > > > are not automatically accepted and waiting for acceptance capsule to be
> > > > > > > > > passed.
> > > > > > > > > When I tested it I found out that they are not process that's why I created
> > > > > > > > > this patch.
> > > > > > > > 
> > > > > > > > The path you tried to modify is only executed by "efidebug capsule update"
> > > > > > > > or more specifically via the runtime service, UPDATE_CAPSULE.
> > > > > > > > 
> > > > > > > > But this API is NOT officially supported in the current capsule implementation
> > > > > > > > (at least, in my initial intention).
> > > > > > > > The only way to invoke capsule updates is to reboot the system.
> > > > > > > > If you want to test A/B update, please do the reboot.
> > > > > > > 
> > > > > > > I realized that to get full flow you need to use capsule update on disk to get
> > > > > > > all functionalities. But it is very impractical. Actually I would expect via
> > > > > > > efidebug you should be able to perform all steps as capsule update performs when
> > > > > > > you do reboot.
> > > > > > > I would also understand that via efidebug you are not able to apply any capsule
> > > > > > > but I don't think it is right that you can apply just update capsules but not
> > > > > > > empty capsules. I would understand none or all but not something in the middle.
> > > > > > 
> > > > > > The A/B update functionality requires using the capsule-on-disk
> > > > > > functionality for performing the updates. This is also mentioned in
> > > > > > the fwu_updates.rst document. You should be able to apply empty
> > > > > > capsules even with the 'efidebug disk-update' command.
> > > > > 
> > > > > Yes this is working fine.
> > > > > 
> > > > > ZynqMP> efidebug capsule disk-update
> > > > > #####
> > > > > Applying capsule capsule1.bin succeeded.
> > > > > #########################
> > > > > Applying capsule capsule2.bin succeeded.
> > > > > Reboot after firmware update.
> > > > > 
> > > > > I tested it also with empty capsules which are also process properly.
> > > > > 
> > > > > > I have never
> > > > > > used the 'efidebug capsule update' command, so I'm not sure if that is
> > > > > > supported. Like Takahiro mentioned, if you place the capsules(genuine
> > > > > > or empty) under the /EFI/UpdateCapsule/ directory, the update should
> > > > > > happen automatically, since the fwu update feature also enables the
> > > > > > EFI_CAPSULE_ON_DISK_EARLY config.
> > > > > 
> > > > > Yes that's work fine on production systems.
> > > > > But from my point of view there shouldn't be really a problem to also apply
> > > > > empty capsule via efidebug capsule update to be able to see that steps and
> > > > > changes in mdata structure without performing reset.
> > > > 
> > > > The 'efidebug capsule update' command calls the efi_update_capsule
> > > > function, which implements the UpdateCapsule runtime service call. The
> > > > initial versions of my fwu patches were indeed adding support for this
> > > > path, but one of the review comments was to restrict support only for
> > > > the capsule-on-disk path when performing the update in u-boot, since
> > > > we are not using the runtime call in u-boot.
> > > 
> > > I don't think this is a valid argument. As I said I would understand if there is
> > > no interface for any capsule. It means having support for both or none is IMHO
> > > the way we should support.
> > > Can you please point me to that discussion?
> > 
> > There is mention of the point in this discussion [1]. Even this thread
> > has Takahiro mention the point he is making above, that maybe there
> > shouldn't be the efi_update_capsule function.
> > 
> > -sughosh
> 
> Hello Sugosh,
> 
> fwu_empty_capsule() detects an empty capsule as one with a GUID
> fwu_guid_os_request_fw_revert or fwu_guid_os_request_fw_accept.
> 
> I am not aware of a requirement in the UEFI specification to treat
> capsules read from file in a different way than capsules passed via
> UpdateCapsule(). Is there any reason why UpdateCapsule() should not
> process an empty capsule when called from a boot-time EFI application?

Here is a story behind efi_update_capsule():
===
commit a6aafce494ab
Author: Masami Hiramatsu <masami.hiramatsu@linaro.org>
Date:   Wed Feb 16 15:15:42 2022 +0900

    efi_loader: use efi_update_capsule_firmware() for capsule on disk
===

I still believe that this is a valid change, but we should have
moved 'capsule->capsule_guid' check into efi_update_capsule_firmware()
at the same time.

-Takahiro Akashi



> Best regards
> 
> Heinrich
> 
> > 
> > [1] - https://lists.denx.de/pipermail/u-boot/2022-February/473891.html
>