Re: [PATCH v4 06/10] RISC-V: Add Linux load logic

[Daniel Kiper]

On Wed, Jan 23, 2019 at 05:15:58PM +0000, Leif Lindholm wrote:
> On Wed, Jan 23, 2019 at 05:53:00PM +0100, Daniel Kiper wrote:
> > On Wed, Jan 23, 2019 at 11:47:25AM +0000, Leif Lindholm wrote:
> > > On Tue, Jan 22, 2019 at 05:09:18PM +0100, Alexander Graf wrote:
> > > > On 17.01.19 13:24, Daniel Kiper wrote:
> > > > > On Mon, Nov 26, 2018 at 12:38:11AM +0100, Alexander Graf wrote:

[...]

> > > > >> +  /* Convert command line to UCS-2 */
> > > > >> +  loaded_image = grub_efi_get_loaded_image (image_handle);
> > > > >> +  loaded_image->load_options_size = len =
> > > > >> +    (grub_strlen (args) + 1) * sizeof (grub_efi_char16_t);
> > > > >> +  loaded_image->load_options =
> > > > >> +    grub_efi_allocate_any_pages (GRUB_EFI_BYTES_TO_PAGES 
> > > > >> (loaded_image->load_options_size));
> > > > >> +  if (!loaded_image->load_options)
> > > > >> +    return grub_errno;
> > > > >
> > > > > I am afraid that grub_errno may not be set by
> > > > > grub_efi_allocate_any_pages() to correct value.
> > > >
> > > > True. What is the intended fix? Have the efi helpers set errno or set
> > > > errno here? Leif?
> > >
> > > I mean, that would superficially seem like the right thing to do, but
> > > I'd really be happy with either. I haven't really managed to find a
> > > natural pattern to where grub_errno is supposed to be used/set and not.
> >
> > Could you check what is the rule among several/all EFI GRUB functions?
> > If they do not set grub_errno then you should set it here. If more EFI
> > GRUB functions set grub_errno then these ones called here should be
> > updated accordingly.
>
> I'm afraid it doesn't look terribly consistent - some very core

Well, this does not help...

> functions, like grub_efi_set_virtual_address_map...
>
> "£$%$^!
>
> Why?
> Why???
> Why do we have a grub_efi_set_virtual_address_map()?
>
> Requesting permission to nuke.

I think that it can be safely dropped. So, go ahead.

> Anyway, back on topic.
> grub_efi_set_variable() sets it.
> grub_efi_finish_boot_services() sets it (not that we make use of that
> for arm*). To be honest, most calls to grub_error() are in this
> function.

Or let's go different way. If any called function sets grub_errno then
use it. If no then set it properly in your own function. And please if
possible take into account Colin's comment too.

> > [...]
> >
> > > > >> +  return (grub_arch_efi_linux_boot_image((grub_addr_t)kernel_addr,
> > > > >> +                                         kernel_size, linux_args));
> > > > >> +}
> > > > >> +
> > > > >> +static grub_err_t
> > > > >> +grub_linux_unload (void)
> > > > >> +{
> > > > >> +  grub_dl_unref (my_mod);
> > > > >
> > > > > I think that would be safer if you call grub_dl_unref() just before 
> > > > > return
> > > > > at the end of this function.
> > > >
> > > > Same.
> > >
> > > Now this bit I know _I_ cargo-culted :)
> >
> > Well, there is a chance that I do not get it because I am not native 
> > speaker.
> > Could you enlighten me what does it mean?
>
> I copied existing code without particularly paying any attention to
> what it was doing.
> Based on https://en.wikipedia.org/wiki/Cargo_cult.
>
> I probably copied i386/pc/linux.c.

Ahhh... OK. I should be a bit smarter and look for it myself...
Anyway, thanks for the explanation.

Daniel