Re: [PATCH V5 2/6] Add shared update_fdt() function for ARM/ARM64

[Roy Franz]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/