From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1YqM2d-0000xc-3P for mharc-grub-devel@gnu.org; Thu, 07 May 2015 09:40:43 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55371) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YqM2a-0000tj-3G for grub-devel@gnu.org; Thu, 07 May 2015 09:40:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YqM2W-0005Ms-TW for grub-devel@gnu.org; Thu, 07 May 2015 09:40:40 -0400 Received: from mail-wg0-x22d.google.com ([2a00:1450:400c:c00::22d]:36466) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YqM2W-0005Mh-IM for grub-devel@gnu.org; Thu, 07 May 2015 09:40:36 -0400 Received: by wgiu9 with SMTP id u9so43897446wgi.3 for ; Thu, 07 May 2015 06:40:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=message-id:date:from:user-agent:mime-version:to:subject:references :in-reply-to:content-type; bh=UnRx874nj2ZYTqY1m3gJRx7mkTyuKHObczpK0uL+xOw=; b=Ln09fmi7oCttrlyEKoku5bpBC9TZ0m1yk7STo6XnxQwwPOSqc7v6vk0APIrqUHmIcn HHUWFwApKyYtEFZvsrn977MjpjWBfzzjK8lsaRDnA1yTWN3O44EFPNDyERK5SF8vRiBM yfJI8yNocwcPDAmxeaLtcIal6P/Vzln3WeV4AbgpwVxtaQ+1xS50N37gAQkpwSbcD99z 79nsOgvnS4oCyycgftRLYOUaWVihFnbPkZIQETEkV3PuS/11MT3se97xx+3nSR4di/aM bCT1JcDZnbzybdALpHqyG/IB0V7tiwKyVmGXISDsqDyXWM4JZmGrrEj4rwRf7NSLGtst L/Nw== X-Received: by 10.194.236.66 with SMTP id us2mr7923762wjc.54.1431006035779; Thu, 07 May 2015 06:40:35 -0700 (PDT) Received: from ?IPv6:2620:0:105f:fd00:863a:4bff:fe50:abc4? ([2620:0:105f:fd00:863a:4bff:fe50:abc4]) by mx.google.com with ESMTPSA id f8sm4074003wiy.7.2015.05.07.06.40.34 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 07 May 2015 06:40:34 -0700 (PDT) Message-ID: <554B6B52.6040602@gmail.com> Date: Thu, 07 May 2015 15:40:34 +0200 From: =?UTF-8?B?VmxhZGltaXIgJ8+GLWNvZGVyL3BoY29kZXInIFNlcmJpbmVua28=?= User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.6.0 MIME-Version: 1.0 To: The development of GNU GRUB Subject: Re: [PATCH 4/5] arm64: Add multiboot support file References: <54931912.1050804@linaro.org> In-Reply-To: <54931912.1050804@linaro.org> Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="naAu7ABLoTXeXNcCP9cjH3knjcA096hS2" X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a00:1450:400c:c00::22d X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 07 May 2015 13:40:41 -0000 This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --naAu7ABLoTXeXNcCP9cjH3knjcA096hS2 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 18.12.2014 19:12, Fu Wei wrote: > grub-core/loader/arm64/multiboot.c > include/grub/arm64/multiboot.h >=20 > Add commands register code and hearder file into=20 > grub-core/loader/arm64/linux.c >=20 > - This multiboot support is built into linux module for aarch64. >=20 > - The implementation for Xen is following : > http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Mul= tiboot > and xen/docs/misc/arm/device-tree/booting.txt in Xen source code. >=20 > - This adds support for the Xen Multiboot on ARM specification for ar= m64, > enabling config file portability across the architectures. >=20 > - The multiboot command is currently x86-only, so reusing these comma= nd names > should not conflict with any future additions of ARM support to mul= tiboot2. >=20 > - The reason of adding this functionality to the existing "linux" mod= ule > rather than "multiboot(2)" > (1)multiboot is x86 only multiboot2 isn't. But what you implement here is not multiboot. It's xen-specific protocol. So it should be called xen_hypervisor and xen_module. It's ok to add xen_hypervisor as an alias to multiboot on x86= > (2)Multiboot is added to "linux" module because it reuses existing = code. >=20 Not a reason. You can move the code you reuse to separate module. > diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def > index 42443bc..b963a62 100644 > --- a/grub-core/Makefile.core.def > +++ b/grub-core/Makefile.core.def > @@ -1673,6 +1673,7 @@ module =3D { > ia64_efi =3D loader/ia64/efi/linux.c; > arm =3D loader/arm/linux.c; > arm64 =3D loader/arm64/linux.c; > + arm64 =3D loader/arm64/multiboot.c; Ditto > + > + grub_arm64_linux_register_multiboot_command (mod); > + Ditto > my_mod =3D mod; > } > =20 > @@ -478,4 +482,6 @@ GRUB_MOD_FINI (linux) > grub_unregister_command (cmd_linux); > grub_unregister_command (cmd_initrd); > grub_unregister_command (cmd_devicetree); > + > + grub_arm64_linux_unregister_multiboot_command (); Ditto > +static grub_dl_t linux_mod; > +static int loaded; > + > +static struct multiboot_binary *multiboot_kernel =3D NULL; > +static struct multiboot_binary *module_head =3D NULL; > +static const grub_size_t module_default_align[] =3D { > + MODULE_IMAGE_MIN_ALIGN, > + MODULE_INITRD_MIN_ALIGN, > + MODULE_OTHER_MIN_ALIGN, > + MODULE_CUSTOM_MIN_ALIGN > +}; > + > +static void *multiboot_fdt =3D NULL; > +static const compat_string_struct_t default_compat_string[] =3D { > + FDT_COMPATIBLE (MODULE_IMAGE_COMPATIBLE), > + FDT_COMPATIBLE (MODULE_INITRD_COMPATIBLE), > + FDT_COMPATIBLE (MODULE_OTHER_COMPATIBLE) > +}; > + > +static grub_err_t > +set_module_type (struct multiboot_binary *module, > + int argc, char *argv[], int *file_name_index) > +{ > + static module_type_t default_type =3D MODULE_IMAGE; > + char *temp =3D NULL; > + grub_size_t total_size =3D 0; > + int num_types =3D 0, i; > + char **compat_string_temp_array =3D > + (char **) grub_zalloc (sizeof (char *) * argc); > + > + *file_name_index =3D 0; > + > + /* if there are some options we need to process. */ > + if (argc >=3D 3) > + { > + while (argc > 1) > + { > + if (grub_strcmp (argv[0], "--type") =3D=3D 0) > + { Can we have separate xen_* commands for different types? It's ok for them to call similar code paths under the hood > + module->node_info.type =3D MODULE_CUSTOM; > + ARG_SHIFT(argc, argv); > + total_size +=3D grub_strlen (argv[0]) + 1; > + compat_string_temp_array[num_types++] =3D argv[0]; > + ARG_SHIFT(argc, argv); > + (*file_name_index) +=3D 2; > + } > + else > + /* we can add more options process code here. */ > + break; > + } > + } > + > + /* For the default module type : > + * The implementation is following := Could this thing be renamed to "xen ARM boot protocol"? > + if (module->node_info.type !=3D MODULE_CUSTOM) > + { > + /* the module type is set by the load order */ > + module->node_info.type =3D default_type; > + switch (default_type) > + { > + case MODULE_IMAGE: > + default_type =3D MODULE_INITRD; > + break; > + > + case MODULE_INITRD: > + default_type =3D MODULE_OTHER; > + break; > + > + case MODULE_OTHER: > + break; > + > + default: > + default_type =3D MODULE_IMAGE; /* error, reset the type */ > + return (grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid argum= ent"))); > + } > + } > + else > + { > + /* the module type is set by "--type"(MODULE_CUSTOM) */ > + module->node_info.compat_string =3D temp =3D > + (char *) grub_zalloc (total_size); > + module->node_info.compat_string_size =3D total_size; > + for (i =3D 0; num_types > 0; num_types--, i++, temp++) > + { > + grub_strcpy (temp, compat_string_temp_array[i]); > + temp +=3D grub_strlen (compat_string_temp_array[i]); > + } > + } > + Unnecessarry complexity just to have less commands. We've been down this path with grub1, it's not good. > + grub_free (compat_string_temp_array); > + return (GRUB_ERR_NONE); > +} > + > +static grub_err_t > +prepare_kernel_params (void) > +{ > + int retval; > + int chosen_node =3D 0; > + > + multiboot_fdt =3D grub_linux_get_fdt (); > + if (!multiboot_fdt) > + return (grub_error (GRUB_ERR_BAD_OS, "failed to install/update FDT= ")); > + > + chosen_node =3D grub_fdt_find_subnode (multiboot_fdt, 0, "chosen"); > + if (chosen_node < 0) > + chosen_node =3D grub_fdt_add_subnode (multiboot_fdt, 0, "chosen");= > + if (chosen_node < 1) > + return (grub_error (GRUB_ERR_BAD_OS, "failed to install/update FDT= ")); > + > + grub_dprintf ("multiboot_loader", > + "Multiboot Kernel cmdline : %s @ %p size:%d\n", > + multiboot_kernel->cmdline, multiboot_kernel->cmdline, > + multiboot_kernel->cmdline_size); > + > + retval =3D grub_fdt_set_prop (multiboot_fdt, chosen_node, "bootargs"= , > + multiboot_kernel->cmdline, > + multiboot_kernel->cmdline_size); > + if (retval) > + return grub_error (GRUB_ERR_BAD_OS, "failed to install/update FDT"= ); > + > + return (GRUB_ERR_NONE); > +} > + > +static grub_err_t > +prepare_module_params (struct multiboot_binary *module) > +{ > + int retval, chosen_node =3D 0, module_node =3D 0; > + char module_name[FDT_NODE_NAME_MAX_SIZE]; > + > + retval =3D grub_snprintf (module_name, FDT_NODE_NAME_MAX_SIZE, > + "module@%lx", > + multiboot_address_align (module->start, > + module->align)); > + grub_dprintf ("multiboot_loader", "Module node name %s \n", module_n= ame); > + > + if (retval < (int) sizeof ("module@")) > + return (grub_error (GRUB_ERR_BAD_OS, > + N_("failed to install/update FDT"))); > + What kind of errors do you expect here? I don't see grub_snprintf erroring here at all under any circumstances. Please make errors more descriptive. You have same error message for at least 5 different problems. Is this error likely to happen to a normal user? If not, let's not load translators with obscure technical strings. If it is likely it must way more clearer and have TRANSLATORS comment on context. > + chosen_node =3D grub_fdt_find_subnode (multiboot_fdt, 0, "chosen"); > + if (chosen_node < 0) > + chosen_node =3D grub_fdt_add_subnode (multiboot_fdt, 0, "chosen");= > + if (chosen_node < 1) > + return (grub_error (GRUB_ERR_BAD_OS, "failed to install/update FDT= ")); > + > + module_node =3D > + grub_fdt_find_subnode (multiboot_fdt, chosen_node, module_name); > + if (module_node < 0) > + module_node =3D > + grub_fdt_add_subnode (multiboot_fdt, chosen_node, module_name); > + Could you please encapsulate find/add/setprop to a separate function? > + retval =3D grub_fdt_set_prop (multiboot_fdt, module_node, "compatibl= e", > + module->node_info.compat_string, > + (grub_uint32_t) module->node_info.compat_string_size); > + if (retval) > + return (grub_error (GRUB_ERR_BAD_OS, > + N_("failed to install/update FDT"))); > + Style here and in many other places: no parens for return. > +static grub_err_t > +multiboot_binary_load (struct multiboot_binary *binary, grub_file_t fi= le, > + int argc, char *argv[]) > +{ > + binary->size =3D grub_file_size (file); > + grub_dprintf ("multiboot_loader", "Multiboot %s file size: 0x%lx\n",= > + binary->name, binary->size); > + > + binary->start =3D (grub_addr_t) grub_efi_allocate_pages (0, > + (BYTES_TO_PAGES > + (binary->size + > + binary->align))); > + if (!binary->start) > + return (grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory")));= > + > + grub_dprintf ("multiboot_loader", "Multiboot %s numpages: 0x%lx\n", > + binary->name, BYTES_TO_PAGES (binary->size + binary->align)); > + > + if (grub_file_read (file, > + (void *) multiboot_address_align (binary->start, > + binary->align), > + binary->size) < (grub_ssize_t) binary->size) > + { > + single_binary_unload (binary); > + return (grub_error (GRUB_ERR_BAD_OS, > + N_("premature end of file %s"), argv[0])); Please don't override grub_errno if it's already set. See other loader for example. > --- /dev/null > +++ b/include/grub/arm64/multiboot.h Again, please don't call it multiboot --naAu7ABLoTXeXNcCP9cjH3knjcA096hS2 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iF4EAREKAAYFAlVLa1IACgkQmBXlbbo5nOvSYAD/Z+pf3UQEK0ImI9VGxYJ8D2sc BaQ2KZ0ELH+YoOiv1EkA/0J/GBzKn3isqWvCIqdqR7DLoBef5cPAWNCbWoTYspwG =efo1 -----END PGP SIGNATURE----- --naAu7ABLoTXeXNcCP9cjH3knjcA096hS2--