Re: [RFC PATCH 1/1] efi_loader: get rid of ad-hoc EFI subsystem init

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

On Mon, Jan 23, 2023 at 10:15:40AM +0200, Ilias Apalodimas wrote:
> Hi Simon, Heinrich
> 
> On Fri, Jan 20, 2023 at 03:11:13PM -0700, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Fri, 20 Jan 2023 at 13:36, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >
> > > On 1/20/23 20:19, Simon Glass wrote:
> > > > Hi,
> > > >
> > > > On Fri, 20 Jan 2023 at 06:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > >>
> > > >> Am 20. Januar 2023 13:31:19 MEZ schrieb Ilias Apalodimas <ilias.apalodimas@linaro.org>:
> > > >>> Up to now the EFI subsystem was left out of the main U-Boot init
> > > >>> process.  This has led to various hacks over the years, with the most
> > > >>> notable one being sprinkling around the efi init call to various places
> > > >>> such as U-Boot commands, the early boot code etc.
> > > >>>
> > > >>> Since EFI has it's own Kconfig option and people can remove it, let's
> > > >>> wire up the EFI init call on an event for EVT_MAIN_LOOP.
> > > >>>
> > > >>> This will also get rid of ad-hoc code in the main event loop, which was
> > > >>> trying to initialize the subsystem early and perform capsule updates.
> > > >>>
> > > >>> TODO:
> > > >>> - The efi_tcg protocol implicitly initializes the TPM, as a result
> > > >>>   some of the tpm selftests will fail with the RFC.  If everyone
> > > >>>   agrees that this is a good idea, I'll clean up the TPM hacks as well
> > > >>> - We  still need to run capsule updates on the main_loop() code since
> > > >>>   in some cases (e.g sandbox) we need preboot commands.
> > > >>> - wider tests, I've only run QEMU for now
> > > >>>
> > > >>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > >>
> > > >>
> > > >> In case of efi_init_obj_list() failing we should still reach the U-Boot console but each of the EFI commands should abort early.
> > > >>
> > > >> Please, put the Kconfig related capsule change into a separate patch.
> > > >>
> > > >> Otherwise looks good to me.
> > > >>
> > > >
> > > > I am OK with this change too.
> > > >
> > > > Two points from my side:
> > > >
> > > > 1. The main loop capsule update is (still) a mistake, unfortunately.
> > > > It should be a command which is run on boot. For sandbox testing, that
> > > > command should be run *without* rebooting. I am sure I asked for that
> > >
> > > Capsule updates must run outside of any command as this is required by
> > > the UEFI specification.
> 
> Yes it's still a mistake but we can't get rid of it easily.  What I was

I don't think it's a mistake. When I implemented the feature, 
update_tftp() was already placed in the main loop. I followed
this tradition.

> going to try is add another event notifier which would run post boot.  That
> would work and allow us to define events, after the preboot commands have
> executed.

It's an idea, but it's a matter of machanism, but not a matter of timing.

-Takahiro Akashi

> Simon there *is* a command do that. It's documented in [0]. The tl;dr is
> run:
> => efidebug boot add 0 Boot0000 virtio 0:1 <capsule_file_name>
> => efidebug boot next 0
> => setenv -e -nv -bs -rt -v OsIndications =0x0000000000000004
> => efidebug capsule disk-update
> 
> The command exists for testing,  but as Heinrich explains,  we also need to
> test  the automatic upgrade after a reboot since that's what the EFI
> specification expects.
> 
> >
> > What do you mean? Does the specification mention U-Boot commands?
> >
> > >
> > > Capsule updates require a reboot and the sandbox must behave as a
> > > regular system so that we can run the same tests on all systems.
> >
> > How can I get you past this thinking? We need to 'design for test'.
> > The current test is a mess, sorry. Perhaps we could have a call to go
> > through it?
> >
> > >
> > > > at the time but for various reasons it didn't happen. Please can you
> > > > make that change also?
> > > >
> > > > 2. EFI should not be maintaining its own separate data structures, but
> > > > should keep them attached to driver model. They should be created as
> > > > needed, dynamically, not all at the start. Is anyone looking at this?
> > > > I am happy to help suggest initial target for this refactoring.
> 
> This patch doesn't change anything wrt to structures or how it interacts
> with DM.  That's a different topic, but since there has been no progress
> apart from block devices for a while, I'll start looking into it myself.
> FWIW I agree we should refactor the protocol registration and match what
> U-Boot does with the rest of the DM.
> 
> > >
> > > We have started with such a link for block devices. Once those block
> > > devices are really UEFI compliant we should refactor the other devices
> > > currently supported by EFI sub-system:
> > >
> > > UART, network, RNG, video, TPM.
> > >
> > > Some initialization like setting up UEFI variables will not have any
> > > equivalence in the driver model.
> >
> > One way to handle that is to:
> >
> > - set up 'global' EFI structures attached to an EFI uclass
> > - set up 'per device' EFI structures when the device is bound or probed
> >
> > Regards,
> > Simon
> 
> Can we do this is small steps since I prefer testing everything
> thoroughly and making sure the EFI init doesn't break?
> So my plan is to resend this, fixing any TPM selftest regressions and then
> start cleaning up the protocol registration.
> 
> [0] https://u-boot.readthedocs.io/en/v2021.04/board/emulation/qemu_capsule_update.html
> 
> Regards
> /Ilias