> -----Original Message----- > From: Jacobo Pantoja <jacobopantoja@xxxxxxxxx> > Sent: Wednesday, September 16, 2020 11:50 > To: Ard Biesheuvel > Cc: Arvind Sankar; Limonciello, Mario; Peter Jones; linux-efi > Subject: Re: [PATCH 2/2] efi/x86: Add a quirk to support command line > arguments on Dell EFI firmware > > > [EXTERNAL EMAIL] > > Hi, I'd like to update my testing and share my thoughts. > > Regarding the patches: > 1) The patches in email 1/2 (the functions "efi_warn", etc.) are not working > properly. I got some suggestions for testing from Ard in a separate email. > 3a. If, in this 2nd patch, I switch the "efi_warn_once" with an > "efi_printk", the > messages appear. > 1a. I've set CONFIG_CONSOLE_LOGLEVEL_DEFAULT=5, same result > 2a. I've switched from "efi_warn_once" to "efi_warn", same result. > 2) Even if they would be working, since it is not logged anywhere, I > don't really > think these messages make sense. Idk if these can be made available to dmesg. > 3) The function "efi_apply_loadoptions_quirk" is called twice, it seems to me > that calling it from the "file.c" is redundant, but probably I'm > missing something. > > Regarding the quirk itself, in my opinion we should wait for Mario's > news, since, > again in my opinion, this is something that should be fixed in the > firmware itself. > Being Dell a serious company, I think it is feasible that, at least > for their enterprise > products, they might fix it. I'm sorry to say this is going to be a slow response and I have nothing to share yet. My "expectation" however would be that if it's changed it will only be in modern/current systems and there should still be a need to decide whether to carry the quirk to support systems with this firmware aberration. > > On Tue, 15 Sep 2020 at 17:09, Ard Biesheuvel <ardb@xxxxxxxxxx> wrote: > > > > On Tue, 15 Sep 2020 at 00:35, Arvind Sankar <nivedita@xxxxxxxxxxxx> wrote: > > > > > > At least some versions of Dell EFI firmware pass the entire > > > EFI_LOAD_OPTION descriptor, rather than just the OptionalData part, to > > > the loaded image. This was verified with firmware revision 2.15.0 on a > > > Dell Precision T3620 by Jacobo Pontaja. > > Please be so kind to correct my name, if it's being included in the commit > msg. > > > > > > > To handle this, add a quirk to check if the options look like a valid > > > EFI_LOAD_OPTION descriptor, and if so, use the OptionalData part as the > > > command line. > > > > > > Signed-off-by: Arvind Sankar <nivedita@xxxxxxxxxxxx> > > > Reported-by: Jacobo Pantoja <jacobopantoja@xxxxxxxxx> > > > Link: https://lore.kernel.org/linux- > efi/20200907170021.GA2284449@xxxxxxxxxxxxxxxxxx/ > > > > I'll queue these up for v5.10 > > > > Thanks all > > > > > --- > > > .../firmware/efi/libstub/efi-stub-helper.c | 101 +++++++++++++++++- > > > drivers/firmware/efi/libstub/efistub.h | 31 ++++++ > > > drivers/firmware/efi/libstub/file.c | 5 +- > > > 3 files changed, 135 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c > b/drivers/firmware/efi/libstub/efi-stub-helper.c > > > index f735db55adc0..aa8da0a49829 100644 > > > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c > > > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c > > > @@ -238,6 +238,102 @@ efi_status_t efi_parse_options(char const *cmdline) > > > return EFI_SUCCESS; > > > } > > > > > > +/* > > > + * The EFI_LOAD_OPTION descriptor has the following layout: > > > + * u32 Attributes; > > > + * u16 FilePathListLength; > > > + * u16 Description[]; > > > + * efi_device_path_protocol_t FilePathList[]; > > > + * u8 OptionalData[]; > > > + * > > > + * This function validates and unpacks the variable-size data fields. > > > + */ > > > +static > > > +bool efi_load_option_unpack(efi_load_option_unpacked_t *dest, > > > + const efi_load_option_t *src, size_t size) > > > +{ > > > + const void *pos; > > > + u16 c; > > > + efi_device_path_protocol_t header; > > > + const efi_char16_t *description; > > > + const efi_device_path_protocol_t *file_path_list; > > > + > > > + if (size < offsetof(efi_load_option_t, variable_data)) > > > + return false; > > > + pos = src->variable_data; > > > + size -= offsetof(efi_load_option_t, variable_data); > > > + > > > + if ((src->attributes & ~EFI_LOAD_OPTION_MASK) != 0) > > > + return false; > > > + > > > + /* Scan description. */ > > > + description = pos; > > > + do { > > > + if (size < sizeof(c)) > > > + return false; > > > + c = *(const u16 *)pos; > > > + pos += sizeof(c); > > > + size -= sizeof(c); > > > + } while (c != L'\0'); > > > + > > > + /* Scan file_path_list. */ > > > + file_path_list = pos; > > > + do { > > > + if (size < sizeof(header)) > > > + return false; > > > + header = *(const efi_device_path_protocol_t *)pos; > > > + if (header.length < sizeof(header)) > > > + return false; > > > + if (size < header.length) > > > + return false; > > > + pos += header.length; > > > + size -= header.length; > > > + } while ((header.type != EFI_DEV_END_PATH && header.type != > EFI_DEV_END_PATH2) || > > > + (header.sub_type != EFI_DEV_END_ENTIRE)); > > > + if (pos != (const void *)file_path_list + src- > >file_path_list_length) > > > + return false; > > > + > > > + dest->attributes = src->attributes; > > > + dest->file_path_list_length = src->file_path_list_length; > > > + dest->description = description; > > > + dest->file_path_list = file_path_list; > > > + dest->optional_data_size = size; > > > + dest->optional_data = size ? pos : NULL; > > > + > > > + return true; > > > +} > > > + > > > +/* > > > + * At least some versions of Dell firmware pass the entire contents of > the > > > + * Boot#### variable, i.e. the EFI_LOAD_OPTION descriptor, rather than > just the > > > + * OptionalData field. > > > + * > > > + * Detect this case and extract OptionalData. > > > + */ > > > +void efi_apply_loadoptions_quirk(const void **load_options, int > *load_options_size) > > > +{ > > > + const efi_load_option_t *load_option = *load_options; > > > + efi_load_option_unpacked_t load_option_unpacked; > > > + > > > + if (!IS_ENABLED(CONFIG_X86)) > > > + return; > > > + if (!load_option) > > > + return; > > > + if (*load_options_size < sizeof(*load_option)) > > > + return; > > > + if ((load_option->attributes & ~EFI_LOAD_OPTION_BOOT_MASK) != 0) > > > + return; > > > + > > > + if (!efi_load_option_unpack(&load_option_unpacked, load_option, > *load_options_size)) > > > + return; > > > + > > > + efi_warn_once(FW_BUG "LoadOptions is an EFI_LOAD_OPTION > descriptor\n"); > > > + efi_warn_once(FW_BUG "Using OptionalData as a workaround\n"); > > > + > > > + *load_options = load_option_unpacked.optional_data; > > > + *load_options_size = load_option_unpacked.optional_data_size; > > > +} > > > + > > > /* > > > * Convert the unicode UEFI command line to ASCII to pass to kernel. > > > * Size of memory allocated return in *cmd_line_len. > > > @@ -247,12 +343,15 @@ char *efi_convert_cmdline(efi_loaded_image_t *image, > int *cmd_line_len) > > > { > > > const u16 *s2; > > > unsigned long cmdline_addr = 0; > > > - int options_chars = efi_table_attr(image, load_options_size) / 2; > > > + int options_chars = efi_table_attr(image, load_options_size); > > > const u16 *options = efi_table_attr(image, load_options); > > > int options_bytes = 0, safe_options_bytes = 0; /* UTF-8 bytes */ > > > bool in_quote = false; > > > efi_status_t status; > > > > > > + efi_apply_loadoptions_quirk((const void **)&options, > &options_chars); > > > + options_chars /= sizeof(*options); > > > + > > > if (options) { > > > s2 = options; > > > while (options_bytes < COMMAND_LINE_SIZE && options_chars- > -) { > > > diff --git a/drivers/firmware/efi/libstub/efistub.h > b/drivers/firmware/efi/libstub/efistub.h > > > index e33b9395bc23..7997890c756d 100644 > > > --- a/drivers/firmware/efi/libstub/efistub.h > > > +++ b/drivers/firmware/efi/libstub/efistub.h > > > @@ -711,6 +711,35 @@ union efi_load_file_protocol { > > > } mixed_mode; > > > }; > > > > > > +typedef struct { > > > + u32 attributes; > > > + u16 file_path_list_length; > > > + u8 variable_data[]; > > > + // efi_char16_t description[]; > > > + // efi_device_path_protocol_t file_path_list[]; > > > + // u8 optional_data[]; > > > +} __packed efi_load_option_t; > > > + > > > +#define EFI_LOAD_OPTION_ACTIVE 0x0001U > > > +#define EFI_LOAD_OPTION_FORCE_RECONNECT 0x0002U > > > +#define EFI_LOAD_OPTION_HIDDEN 0x0008U > > > +#define EFI_LOAD_OPTION_CATEGORY 0x1f00U > > > +#define EFI_LOAD_OPTION_CATEGORY_BOOT 0x0000U > > > +#define EFI_LOAD_OPTION_CATEGORY_APP 0x0100U > > > + > > > +#define EFI_LOAD_OPTION_BOOT_MASK \ > > > + > (EFI_LOAD_OPTION_ACTIVE|EFI_LOAD_OPTION_HIDDEN|EFI_LOAD_OPTION_CATEGORY) > > > +#define EFI_LOAD_OPTION_MASK > (EFI_LOAD_OPTION_FORCE_RECONNECT|EFI_LOAD_OPTION_BOOT_MASK) > > > + > > > +typedef struct { > > > + u32 attributes; > > > + u16 file_path_list_length; > > > + const efi_char16_t *description; > > > + const efi_device_path_protocol_t *file_path_list; > > > + size_t optional_data_size; > > > + const void *optional_data; > > > +} efi_load_option_unpacked_t; > > > + > > > void efi_pci_disable_bridge_busmaster(void); > > > > > > typedef efi_status_t (*efi_exit_boot_map_processing)( > > > @@ -753,6 +782,8 @@ __printf(1, 2) int efi_printk(char const *fmt, ...); > > > > > > void efi_free(unsigned long size, unsigned long addr); > > > > > > +void efi_apply_loadoptions_quirk(const void **load_options, int > *load_options_size); > > > + > > > char *efi_convert_cmdline(efi_loaded_image_t *image, int *cmd_line_len); > > > > > > efi_status_t efi_get_memory_map(struct efi_boot_memmap *map); > > > diff --git a/drivers/firmware/efi/libstub/file.c > b/drivers/firmware/efi/libstub/file.c > > > index 630caa6b1f4c..4e81c6077188 100644 > > > --- a/drivers/firmware/efi/libstub/file.c > > > +++ b/drivers/firmware/efi/libstub/file.c > > > @@ -136,7 +136,7 @@ efi_status_t handle_cmdline_files(efi_loaded_image_t > *image, > > > unsigned long *load_size) > > > { > > > const efi_char16_t *cmdline = image->load_options; > > > - int cmdline_len = image->load_options_size / 2; > > > + int cmdline_len = image->load_options_size; > > > unsigned long efi_chunk_size = ULONG_MAX; > > > efi_file_protocol_t *volume = NULL; > > > efi_file_protocol_t *file; > > > @@ -148,6 +148,9 @@ efi_status_t handle_cmdline_files(efi_loaded_image_t > *image, > > > if (!load_addr || !load_size) > > > return EFI_INVALID_PARAMETER; > > > > > > + efi_apply_loadoptions_quirk((const void **)&cmdline, > &cmdline_len); > > > + cmdline_len /= sizeof(*cmdline); > > > + > > > if (IS_ENABLED(CONFIG_X86) && !efi_nochunk) > > > efi_chunk_size = EFI_READ_CHUNK_SIZE; > > > > > > -- > > > 2.26.2 > > >
