From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1gmLmA-0004H4-P7 for mharc-grub-devel@gnu.org; Wed, 23 Jan 2019 11:53:18 -0500 Received: from eggs.gnu.org ([209.51.188.92]:59328) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gmLm8-0004Gx-LE for grub-devel@gnu.org; Wed, 23 Jan 2019 11:53:17 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gmLm7-0000UV-Fb for grub-devel@gnu.org; Wed, 23 Jan 2019 11:53:16 -0500 Received: from dibed.net-space.pl ([84.10.22.86]:41274) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_3DES_EDE_CBC_SHA1:24) (Exim 4.71) (envelope-from ) id 1gmLm7-0000TH-3z for grub-devel@gnu.org; Wed, 23 Jan 2019 11:53:15 -0500 Received: from router-fw.i.net-space.pl ([192.168.52.1]:46476 "EHLO tomti.i.net-space.pl") by router-fw-old.i.net-space.pl with ESMTP id S1271478AbfAWQxE (ORCPT ); Wed, 23 Jan 2019 17:53:04 +0100 X-Comment: RFC 2476 MSA function at dibed.net-space.pl logged sender identity as: dkiper Date: Wed, 23 Jan 2019 17:53:00 +0100 From: Daniel Kiper To: Leif Lindholm Cc: Alexander Graf , The development of GNU GRUB , David Abdurachmanov , Andreas Schwab , greentime , atish.patra@wdc.com, Michael Chang , Alistair Francis , Lukas Auer , Paul Walmsley , rickchen36@gmail.com, Bin Meng Subject: Re: [PATCH v4 06/10] RISC-V: Add Linux load logic Message-ID: <20190123165300.ngvxvpq7fbhjf5r4@tomti.i.net-space.pl> References: <20181125233815.56392-1-agraf@suse.de> <20181125233815.56392-7-agraf@suse.de> <20190117122429.ri2k6x7ydd6o3kp4@tomti.i.net-space.pl> <977f5e45-c3eb-087c-6bc3-c7f6bb84c0b2@suse.de> <20190123114725.oiyi6kjp5gsrgk3p@bivouac.eciton.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190123114725.oiyi6kjp5gsrgk3p@bivouac.eciton.net> User-Agent: NeoMutt/20170113 (1.7.2) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 84.10.22.86 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 23 Jan 2019 16:53:17 -0000 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: [...] > > >> + return GRUB_ERR_NONE; > > >> + > > >> + failure: > > > > > > s/failure/fail/? > > > > Why? I mean, sure, I can change it. But why? > > $ git grep "fail:" | wc -l > 180 > $ git grep "failure:" | wc -l > 5 > > Err, yeah, fair enough. And the only perpetrators in C code (that > aren't part of an imported project) were added by me. > > Daniel: would you take a single patch for > loader/arm/linux.c and loader/arm64/linux.c? If you change label name only then I am OK with that. > > >> + grub_fdt_unload(); > > > > > > s/grub_fdt_unload()/grub_fdt_unload ()/ May I ask you to fix similar > > > mistakes in the other patches too? > > > > Can you please write a checkpatch.pl or similar to catch them? At this > > point, all those coding style issues just add to frustration on both > > sides I think. To me, the GNU style comes very close in uglyness to the > > TianoCore one - and my fingers just simply refuse to naturally adhere to it. > > > > That said, same as above. This is a copy from the arm64 linux.c. > > > > > > > >> + return grub_error(GRUB_ERR_BAD_OS, "failed to install/update FDT"); > > >> +} > > >> + > > >> +grub_err_t > > >> +grub_arch_efi_linux_boot_image (grub_addr_t addr, grub_size_t size, char *args) > > >> +{ > > >> + grub_efi_memory_mapped_device_path_t *mempath; > > >> + grub_efi_handle_t image_handle; > > >> + grub_efi_boot_services_t *b; > > >> + grub_efi_status_t status; > > >> + grub_efi_loaded_image_t *loaded_image; > > >> + int len; > > >> + > > >> + mempath = grub_malloc (2 * sizeof (grub_efi_memory_mapped_device_path_t)); > > >> + if (!mempath) > > >> + return grub_errno; > > >> + > > >> + mempath[0].header.type = GRUB_EFI_HARDWARE_DEVICE_PATH_TYPE; > > >> + mempath[0].header.subtype = GRUB_EFI_MEMORY_MAPPED_DEVICE_PATH_SUBTYPE; > > >> + mempath[0].header.length = grub_cpu_to_le16_compile_time (sizeof (*mempath)); > > >> + mempath[0].memory_type = GRUB_EFI_LOADER_DATA; > > >> + mempath[0].start_address = addr; > > >> + mempath[0].end_address = addr + size; > > >> + > > >> + mempath[1].header.type = GRUB_EFI_END_DEVICE_PATH_TYPE; > > >> + mempath[1].header.subtype = GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE; > > >> + mempath[1].header.length = sizeof (grub_efi_device_path_t); > > >> + > > >> + b = grub_efi_system_table->boot_services; > > >> + status = b->load_image (0, grub_efi_image_handle, > > >> + (grub_efi_device_path_t *) mempath, > > >> + (void *) addr, size, &image_handle); > > >> + if (status != GRUB_EFI_SUCCESS) > > >> + return grub_error (GRUB_ERR_BAD_OS, "cannot load image"); > > >> + > > >> + grub_dprintf ("linux", "linux command line: '%s'\n", args); > > >> + > > >> + /* 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. [...] > > >> + 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? Daniel