| Message ID | 1380385403-31904-3-git-send-email-roy.franz@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 28 Sep, at 09:23:19AM, Roy Franz 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. > > Signed-off-by: Roy Franz <roy.franz@linaro.org> > --- > drivers/firmware/efi/efi-stub-helper.c | 86 ++++++++++++++++++++++++++++++++ > 1 file changed, 86 insertions(+) > > diff --git a/drivers/firmware/efi/efi-stub-helper.c b/drivers/firmware/efi/efi-stub-helper.c > index cc0581d..d3448a9 100644 > --- a/drivers/firmware/efi/efi-stub-helper.c > +++ b/drivers/firmware/efi/efi-stub-helper.c > @@ -4,6 +4,7 @@ > * implementation files. > * > * Copyright 2011 Intel Corporation; author Matt Fleming > + * Copyright 2013 Linaro Limited; author Roy Franz > * > * This file is part of the Linux kernel, and is made available > * under the terms of the GNU General Public License version 2. > @@ -636,3 +637,88 @@ static char *efi_convert_cmdline_to_ascii(efi_system_table_t *sys_table_arg, > *cmd_line_len = options_size; > return (char *)cmdline_addr; > } > + > +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64) > +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) Hmm... does this function really belong in efi-stub-helper.c? That file should be for architecture independent functionality only. While it currently does include this little snippet, #ifdef CONFIG_ARM /* * For ARM, allocate at a high address to avoid reserved * regions at low addresses that we don't know the specfics of * at the time we are processing the command line. */ status = efi_high_alloc(sys_table_arg, options_size, 0, &cmdline_addr, 0xfffff000); #else status = efi_low_alloc(sys_table_arg, options_size, 0, &cmdline_addr); #endif I think that single #ifdef is justifiable. But including entire functions that are only used by ARM/ARM64 or x86 isn't.
On Thu, 2013-10-03 at 10:52 +0100, Matt Fleming wrote: > > +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64) > > +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) > > Hmm... does this function really belong in efi-stub-helper.c? That file > should be for architecture independent functionality only. > It isn't really arm-specific although arm is the only user right now. We're using the FDT to pass EFI boot info that is passed in the boot params block on x86. So potentially other architectures could use the same code. How about making it someting like: #ifdef ARCH_NEEDS_EFI_FDT static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt, ... #endif so the architecture can decide whether to include it or not. --Mark
On Thu, 03 Oct, at 09:43:24AM, Mark Salter wrote: > On Thu, 2013-10-03 at 10:52 +0100, Matt Fleming wrote: > > > +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64) > > > +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) > > > > Hmm... does this function really belong in efi-stub-helper.c? That file > > should be for architecture independent functionality only. > > > > It isn't really arm-specific although arm is the only user right now. > We're using the FDT to pass EFI boot info that is passed in the boot > params block on x86. So potentially other architectures could use the > same code. It's not EFI-specific either, apart from the fdt property names. > How about making it someting like: > > #ifdef ARCH_NEEDS_EFI_FDT > static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt, > ... > #endif > > so the architecture can decide whether to include it or not. Is one implementation of this function going to fit all architectures? Can we be sure of that ahead of time? I'd prefer this to be kept in arch/ for now, and possibly moved into efi-stub-helper.c at a later date if indeed it turns out to be generally useful.
On Thu, 2013-10-03 at 15:27 +0100, Matt Fleming wrote: > On Thu, 03 Oct, at 09:43:24AM, Mark Salter wrote: > > On Thu, 2013-10-03 at 10:52 +0100, Matt Fleming wrote: > > > > +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64) > > > > +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) > > > > > > Hmm... does this function really belong in efi-stub-helper.c? That file > > > should be for architecture independent functionality only. > > > > > > > It isn't really arm-specific although arm is the only user right now. > > We're using the FDT to pass EFI boot info that is passed in the boot > > params block on x86. So potentially other architectures could use the > > same code. > > It's not EFI-specific either, apart from the fdt property names. Not EFI-specific in the sense that it uses the EFI services, but it is EFI-specific in its function. > > > How about making it someting like: > > > > #ifdef ARCH_NEEDS_EFI_FDT > > static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt, > > ... > > #endif > > > > so the architecture can decide whether to include it or not. > > Is one implementation of this function going to fit all architectures? > Can we be sure of that ahead of time? > > I'd prefer this to be kept in arch/ for now, and possibly moved into > efi-stub-helper.c at a later date if indeed it turns out to be generally > useful. Fair enough. I don't have a strong opinion about it either way. Just a natural instinct to share the code if possible, but it is easy enough to change later if others want to use it. --Mark
On Thu, Oct 3, 2013 at 7:27 AM, Matt Fleming <matt@console-pimps.org> wrote: > On Thu, 03 Oct, at 09:43:24AM, Mark Salter wrote: >> On Thu, 2013-10-03 at 10:52 +0100, Matt Fleming wrote: >> > > +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64) >> > > +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) >> > >> > Hmm... does this function really belong in efi-stub-helper.c? That file >> > should be for architecture independent functionality only. >> > >> >> It isn't really arm-specific although arm is the only user right now. >> We're using the FDT to pass EFI boot info that is passed in the boot >> params block on x86. So potentially other architectures could use the >> same code. > > It's not EFI-specific either, apart from the fdt property names. > >> How about making it someting like: >> >> #ifdef ARCH_NEEDS_EFI_FDT >> static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt, >> ... >> #endif >> >> so the architecture can decide whether to include it or not. > > Is one implementation of this function going to fit all architectures? > Can we be sure of that ahead of time? > > I'd prefer this to be kept in arch/ for now, and possibly moved into > efi-stub-helper.c at a later date if indeed it turns out to be generally > useful. > > -- > Matt Fleming, Intel Open Source Technology Center Hi Matt, This function is shared between ARM and ARM64, which are separate architectures so to keep this in arch code would require it to be duplicated for ARM/ARM64. Would moving this function to a new "efi-stub-fdt.c" file be satisfactory? This file would just be used the ARM and ARM64, so this would remove the need for the top-level #ifdefs. Thanks, Roy
On Thu, 03 Oct, at 12:28:03PM, Roy Franz wrote: > Hi Matt, > > This function is shared between ARM and ARM64, which are separate > architectures so to keep this in arch code would require it to be > duplicated for ARM/ARM64. Would moving this function to a new > "efi-stub-fdt.c" file be satisfactory? This file would just be used > the ARM and ARM64, so this would remove > the need for the top-level #ifdefs. Fair enough, move it to drivers/firmware/efi/fdt.c - there's no need to prefix it with "efi-" (efi-pstore.c has historic baggage).
diff --git a/drivers/firmware/efi/efi-stub-helper.c b/drivers/firmware/efi/efi-stub-helper.c index cc0581d..d3448a9 100644 --- a/drivers/firmware/efi/efi-stub-helper.c +++ b/drivers/firmware/efi/efi-stub-helper.c @@ -4,6 +4,7 @@ * implementation files. * * Copyright 2011 Intel Corporation; author Matt Fleming + * Copyright 2013 Linaro Limited; author Roy Franz * * This file is part of the Linux kernel, and is made available * under the terms of the GNU General Public License version 2. @@ -636,3 +637,88 @@ static char *efi_convert_cmdline_to_ascii(efi_system_table_t *sys_table_arg, *cmd_line_len = options_size; return (char *)cmdline_addr; } + +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64) +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; + + 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 */ + if (initrd_size != 0) { + u64 initrd_image_end; + u64 initrd_image_start = cpu_to_fdt64(initrd_addr); + status = fdt_setprop(fdt, node, "linux,initrd-start", + &initrd_image_start, sizeof(u64)); + if (status) + goto fdt_set_fail; + initrd_image_end = cpu_to_fdt64(initrd_addr + initrd_size); + status = fdt_setprop(fdt, node, "linux,initrd-end", + &initrd_image_end, sizeof(u64)); + if (status) + goto fdt_set_fail; + } + + /* Add FDT entries for EFI runtime services in chosen node. */ + node = fdt_subnode_offset(fdt, 0, "chosen"); + fdt_val64 = cpu_to_fdt64((u64)(unsigned long)sys_table); + status = fdt_setprop(fdt, node, "linux,efi-system-table", + &fdt_val64, sizeof(fdt_val64)); + if (status) + goto fdt_set_fail; + + fdt_val32 = cpu_to_fdt32(desc_size); + status = fdt_setprop(fdt, node, "linux,efi-mmap-desc-size", + &fdt_val32, sizeof(fdt_val32)); + if (status) + goto fdt_set_fail; + + fdt_val32 = cpu_to_fdt32(desc_ver); + status = fdt_setprop(fdt, node, "linux,efi-mmap-desc-version", + &fdt_val32, sizeof(fdt_val32)); + if (status) + goto fdt_set_fail; + + + /* Stuff the whole memory map into FDT */ + status = fdt_setprop(fdt, node, "linux,efi-mmap", + memory_map, map_size); + if (status) + goto fdt_set_fail; + + return EFI_SUCCESS; + +fdt_set_fail: + if (status == -FDT_ERR_NOSPACE) + return EFI_BUFFER_TOO_SMALL; + + return EFI_LOAD_ERROR; +} +#endif
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. Signed-off-by: Roy Franz <roy.franz@linaro.org> --- drivers/firmware/efi/efi-stub-helper.c | 86 ++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+)