[PATCH v2 2/4] bootflow: bootmeth_efi: Don't set bootdev again

[Simon Glass]

Hi Shantur,

On Sun, 19 Nov 2023 at 10:01, Shantur Rathore <i at shantur.com> wrote:
>
> Hi Simon,
>
> On Sun, Nov 19, 2023 at 12:44 AM Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Shantur,
> >
> > On Sat, 18 Nov 2023 at 14:17, Shantur Rathore <i at shantur.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Sat, Nov 18, 2023 at 5:11 PM Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > +Ilias too as this involves a design decision
> > > >
> > > > Hi Shantur,
> > > >
> > > > On Fri, 17 Nov 2023 at 14:22, Shantur Rathore <i at shantur.com> wrote:
> > > > >
> > > > > efi_set_bootdev is already called as part of tftp while doing dhcp_run()
> > > >
> > > > Is that in tftp_complete() ?
> > > >
> > > > > Doing this again crashes U-boot and we don't need to call again.
> > > >
> > > > U-Boot
> > > >
> > > > The problem is that we might scan for 4 bootflows, then later decide
> > > > which one to boot. So we cannot know which one it will be until that
> > > > point.
> > > >
> > > > The hack is needed so that it actually tells EFI about the right one.
> > > >
> > > > The addition of EFI hacks when reading files (i.e. the code in
> > > > tftp_complete()) is a real problem, unfortunately. I understand that
> > > > it was a convenient hack, but with standard boot we really don't want
> > > > it to do that. We end up calling it multiple times for bootflows that
> > > > won't be used.
> > > >
> > > > So I believe the fix is to be able to disable those calls, leaving it
> > > > to bootstd.
> > > >
> > > > I'm not sure of the best way to do that. Perhaps we should have a
> > > > function like bootstd_scanning() which returns a bool indicating that
> > > > bootstd is scanning? If that is true then no efi_set_bootdev() calls
> > > > are made? We could have a flag in 'struct bootstd_priv' which is set
> > > > before bootflow_check() is called and cleared afterwards.
> > > >
> > > > As to how this should be done with standard boot, we should create a
> > > > function like efi_start_app() and pass it the information we need.
> > > > That function should do the equivalent of the 'bootefi' command,
> > > > except programmatically, with no prior state hanging around.
> > > >
> > > > As to when we might be able to do that, we need more refactoring of
> > > > the bootm code. There is a start on this [1] but it likely needs 2-3
> > > > more series before it might be possible.
> > > >
> > > > So for now, I think bootstd_scanning() is the best approach. If you
> > > > are not sure about that, I can send a patch.
> > > >
> > >
> > > My apologies, the fix I submitted is incorrect.
> > > After reading your explanation, I started thinking on why dhcp can
> > > call this multiple times but bootmethod can't.
> > >
> > > There it was clear that the issue bflow->fname is null when we call
> > > this function.
> > > The correct fix should be below
> > >
> > > +       /* bootfile should be setup by dhcp by now*/
> > > +       bootfile_name = env_get("bootfile");
> > > +       if (!bootfile_name)
> > > +               return log_msg_ret("bootfile_name", ret);
> > > +       bflow->fname = strdup(bootfile_name);
> > > +
> > >     /* do the hideous EFI hack */
> > >     efi_set_bootdev("Net", "", bflow->fname, map_sysmem(addr, 0),
> > >                     bflow->size);
> > >
> > > I have a patch ready for this which I will send but I have some
> > > queries regarding that.
> > > As I sent this a series of patches out of which 2 were reviewed,
> > > should I send a new version for the whole series even though 2 are not
> > > going to be changed?
> >
> > Yes you normally send the whole series but include any review/tested
> > tags that you received for each patch ('patman status' can help).
> >
>
> patman... that's the missing piece in the puzzle. I was trying to find
> out how everyone is managing their commits.
>
> Thanks for pointing it out to me.

:-) If you have ideas to improve it, please send patches.


- Simon