On Thu, Dec 5, 2013 at 11:24 AM, Grant Likely <grant.likely@xxxxxxxxxx> wrote:
> On Wed, 27 Nov 2013 15:31:51 -0800, Roy Franz <roy.franz@xxxxxxxxxx> wrote:
>> Both ARM and ARM64 stubs will update the device tree
>> that they pass to the kernel. In both cases they
>> primarily need to add the same UEFI related information,
>> so the function can be shared.
>> Create a new FDT related file for this to avoid use
>> of architecture #ifdefs in efi-stub-helper.c
>> Change EFI allocation type for memory map to
>> EFI_RUNTIME_SERVICES_DATA, since we are passing
>> this buffer to the kernel, or immediately freeing it.
>>
>> Signed-off-by: Roy Franz <roy.franz@xxxxxxxxxx>
>
> Hi Roy,
>
> Comments below...
>
>> +static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
>> + void *fdt, int new_fdt_size, char *cmdline_ptr,
>> + u64 initrd_addr, u64 initrd_size,
>> + efi_memory_desc_t *memory_map,
>> + unsigned long map_size, unsigned long desc_size,
>> + u32 desc_ver)
>> +{
>> + int node;
>> + int status;
>> + u32 fdt_val32;
>> + u64 fdt_val64;
>> + /* Copy definition of linux_banner here. Since this code is
>> + * built as part of the decompressor for ARM v7, pulling
>> + * in version.c where linux_banner is defined for the
>> + * kernel brings other kernel dependencies with it.
>> + */
>> + const char linux_banner[] =
>> + "Linux version " UTS_RELEASE " (" LINUX_COMPILE_BY "@"
>> + LINUX_COMPILE_HOST ") (" LINUX_COMPILER ") " UTS_VERSION "\n";
>
> This is inherently fragile because the string is now defined in two
> places. This issue shouldn't hold off merging the patch, but a follow up
> patch should be crafted to make it possible to pull in version.c without
> the other dependencies....
>
> Then again, the version.c strings have a very big "DON'T TOUCH" warning
> on them so I'm probably worrying about nothing. :-)
I wasn't really happy about copying the definition here, but as this
will be verified by UEFI
code in the kernel if this every is changed it should not be much of a
problem in practice.
I can take a look at what changes would be required for using
version.c here. I haven't worked
out what would need to be done, but was hesitant to make changes there
to support a one-off use
of the kernel version string outside of the kernel (ie in the
decompressor/stub), and was trying to keep
this patchset limited to changing arm/EFI code.
>
>> +
>> + status = fdt_open_into(orig_fdt, fdt, new_fdt_size);
>> + if (status != 0)
>> + goto fdt_set_fail;
>> +
>> + node = fdt_subnode_offset(fdt, 0, "chosen");
>> + if (node < 0) {
>> + node = fdt_add_subnode(fdt, 0, "chosen");
>> + if (node < 0) {
>> + status = node; /* node is error code when negative */
>> + goto fdt_set_fail;
>> + }
>> + }
>> +
>> + if ((cmdline_ptr != NULL) && (strlen(cmdline_ptr) > 0)) {
>> + status = fdt_setprop(fdt, node, "bootargs", cmdline_ptr,
>> + strlen(cmdline_ptr) + 1);
>> + if (status)
>> + goto fdt_set_fail;
>> + }
>> +
>> + /* Set intird address/end in device tree, if present */
>
> sp. initrd
>
> None of the above comments are blockers though.
>
> Acked-by: Grant Likely <grant.likely@xxxxxxxxxx>
>
Thanks,
Roy
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
