| Message ID | 20181017073207.13150-4-takahiro.akashi@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | efi: make efi and bootmgr more usable | expand |
On 17.10.18 09:32, AKASHI Takahiro wrote: > In this patch, helper functions for an load option variable (BootXXXX) > are added: > * efi_parse_load_option(): parse a string into load_option data > (renamed from parse_load_option and exported) > * efi_marshel_load_option(): convert load_option data into a string > > Those functions will be used to implement efishell command. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > include/efi_loader.h | 25 +++++++++++++ > lib/efi_loader/efi_bootmgr.c | 68 ++++++++++++++++++++++++------------ > 2 files changed, 70 insertions(+), 23 deletions(-) > > diff --git a/include/efi_loader.h b/include/efi_loader.h > index b73fbb6de23f..1cabb1680d20 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -502,6 +502,31 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, efi_guid_t *vendor, > u32 attributes, efi_uintn_t data_size, > void *data); > > +/* > + * See section 3.1.3 in the v2.7 UEFI spec for more details on > + * the layout of EFI_LOAD_OPTION. In short it is: > + * > + * typedef struct _EFI_LOAD_OPTION { > + * UINT32 Attributes; > + * UINT16 FilePathListLength; > + * // CHAR16 Description[]; <-- variable length, NULL terminated > + * // EFI_DEVICE_PATH_PROTOCOL FilePathList[]; > + * <-- FilePathListLength bytes > + * // UINT8 OptionalData[]; > + * } EFI_LOAD_OPTION; > + */ > +struct load_option { > + u32 attributes; > + u16 file_path_length; > + u16 *label; > + struct efi_device_path *file_path; > + u8 *optional_data; > +}; If this is part of the spec, shouldn't it rather be in efi_api.h? It probably also wants an efi_ prefix then :). > + > +void efi_parse_load_option(struct load_option *lo, void *ptr); > +unsigned long efi_marshal_load_option(u32 attr, u16 *label, > + struct efi_device_path *file_path, > + char *option, void **data); > void *efi_bootmgr_load(struct efi_device_path **device_path, > struct efi_device_path **file_path); > > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c > index 0c5764db127b..c2d29f956065 100644 > --- a/lib/efi_loader/efi_bootmgr.c > +++ b/lib/efi_loader/efi_bootmgr.c > @@ -30,28 +30,8 @@ static const struct efi_runtime_services *rs; > */ > > > -/* > - * See section 3.1.3 in the v2.7 UEFI spec for more details on > - * the layout of EFI_LOAD_OPTION. In short it is: > - * > - * typedef struct _EFI_LOAD_OPTION { > - * UINT32 Attributes; > - * UINT16 FilePathListLength; > - * // CHAR16 Description[]; <-- variable length, NULL terminated > - * // EFI_DEVICE_PATH_PROTOCOL FilePathList[]; <-- FilePathListLength bytes > - * // UINT8 OptionalData[]; > - * } EFI_LOAD_OPTION; > - */ > -struct load_option { > - u32 attributes; > - u16 file_path_length; > - u16 *label; > - struct efi_device_path *file_path; > - u8 *optional_data; > -}; > - > /* parse an EFI_LOAD_OPTION, as described above */ > -static void parse_load_option(struct load_option *lo, void *ptr) > +void efi_parse_load_option(struct load_option *lo, void *ptr) I think you want to rename this to "deserialize" to better align with ... > { > lo->attributes = *(u32 *)ptr; > ptr += sizeof(u32); > @@ -60,7 +40,7 @@ static void parse_load_option(struct load_option *lo, void *ptr) > ptr += sizeof(u16); > > lo->label = ptr; > - ptr += (u16_strlen(lo->label) + 1) * 2; > + ptr += (u16_strlen(lo->label) + 1) * sizeof(u16); > > lo->file_path = ptr; > ptr += lo->file_path_length; > @@ -68,6 +48,48 @@ static void parse_load_option(struct load_option *lo, void *ptr) > lo->optional_data = ptr; > } > > +unsigned long efi_marshal_load_option(u32 attr, u16 *label, ... this function which really should be called "serialize" rather than "marshal". "Marshalling" is what you do to convert from one API to another. Here, we want to write an in-memory object to disk. I also think the API would make more sense if you pass in a struct efi_load_option *. It's more symmetric to the deserialize one then. You also want to add comments to the functions to say what they do and what their parameters are there for. That will make it easier for people to grasp on how to use this (now public) API. And I really dislike void * for anything that isn't "opaque". In this function (and the one above) void * really gets used as byte pointer. Please mark it as such (u8 *). The next problem are the casts. If you cast from u8 * to u32 * and dereference it, the compiler does not know if that pointer is aligned or not. So on platforms that don't enable caches yet in the bootmgr path, we may get unaligned exceptions. So you want to use get_unaligned_le32() and friends or memcpy (as you did in your function) above. Alex > + struct efi_device_path *file_path, > + char *option, void **data) > +{ > + unsigned long size; > + unsigned long label_len, option_len; > + u16 file_path_len; > + void *p; > + > + label_len = (u16_strlen(label) + 1) * sizeof(u16); > + file_path_len = efi_dp_size(file_path) > + + sizeof(struct efi_device_path); /* for END */ > + option_len = strlen(option); > + > + /* total size */ > + size = sizeof(attr); > + size += file_path_len; > + size += label_len; > + size += option_len + 1; > + p = malloc(size); > + if (!p) > + return 0; > + > + /* copy data */ > + *data = p; > + memcpy(p, &attr, sizeof(attr)); > + p += sizeof(attr); > + memcpy(p, &file_path_len, sizeof(file_path_len)); > + p += sizeof(file_path_len); > + memcpy(p, label, label_len); > + p += label_len; > + > + memcpy(p, file_path, file_path_len); > + p += file_path_len; > + > + memcpy(p, option, option_len); > + p += option_len; > + *(char *)p = '\0'; > + > + return size; > +} > + > /* free() the result */ > static void *get_var(u16 *name, const efi_guid_t *vendor, > efi_uintn_t *size) > @@ -115,7 +137,7 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path, > if (!load_option) > return NULL; > > - parse_load_option(&lo, load_option); > + efi_parse_load_option(&lo, load_option); > > if (lo.attributes & LOAD_OPTION_ACTIVE) { > efi_status_t ret; >
On Wed, Oct 17, 2018 at 10:40:26AM +0200, Alexander Graf wrote: > > > On 17.10.18 09:32, AKASHI Takahiro wrote: > > In this patch, helper functions for an load option variable (BootXXXX) > > are added: > > * efi_parse_load_option(): parse a string into load_option data > > (renamed from parse_load_option and exported) > > * efi_marshel_load_option(): convert load_option data into a string > > > > Those functions will be used to implement efishell command. > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > include/efi_loader.h | 25 +++++++++++++ > > lib/efi_loader/efi_bootmgr.c | 68 ++++++++++++++++++++++++------------ > > 2 files changed, 70 insertions(+), 23 deletions(-) > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > index b73fbb6de23f..1cabb1680d20 100644 > > --- a/include/efi_loader.h > > +++ b/include/efi_loader.h > > @@ -502,6 +502,31 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, efi_guid_t *vendor, > > u32 attributes, efi_uintn_t data_size, > > void *data); > > > > +/* > > + * See section 3.1.3 in the v2.7 UEFI spec for more details on > > + * the layout of EFI_LOAD_OPTION. In short it is: > > + * > > + * typedef struct _EFI_LOAD_OPTION { > > + * UINT32 Attributes; > > + * UINT16 FilePathListLength; > > + * // CHAR16 Description[]; <-- variable length, NULL terminated > > + * // EFI_DEVICE_PATH_PROTOCOL FilePathList[]; > > + * <-- FilePathListLength bytes > > + * // UINT8 OptionalData[]; > > + * } EFI_LOAD_OPTION; > > + */ > > +struct load_option { > > + u32 attributes; > > + u16 file_path_length; > > + u16 *label; > > + struct efi_device_path *file_path; > > + u8 *optional_data; > > +}; > > If this is part of the spec, shouldn't it rather be in efi_api.h? While uefi spec mentions this structure, I don't have good confidence that I can say that it is part of spec or API because there is no interface (or function) that handles this structure. > It > probably also wants an efi_ prefix then :). OK. > > + > > +void efi_parse_load_option(struct load_option *lo, void *ptr); > > +unsigned long efi_marshal_load_option(u32 attr, u16 *label, > > + struct efi_device_path *file_path, > > + char *option, void **data); > > void *efi_bootmgr_load(struct efi_device_path **device_path, > > struct efi_device_path **file_path); > > > > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c > > index 0c5764db127b..c2d29f956065 100644 > > --- a/lib/efi_loader/efi_bootmgr.c > > +++ b/lib/efi_loader/efi_bootmgr.c > > @@ -30,28 +30,8 @@ static const struct efi_runtime_services *rs; > > */ > > > > > > -/* > > - * See section 3.1.3 in the v2.7 UEFI spec for more details on > > - * the layout of EFI_LOAD_OPTION. In short it is: > > - * > > - * typedef struct _EFI_LOAD_OPTION { > > - * UINT32 Attributes; > > - * UINT16 FilePathListLength; > > - * // CHAR16 Description[]; <-- variable length, NULL terminated > > - * // EFI_DEVICE_PATH_PROTOCOL FilePathList[]; <-- FilePathListLength bytes > > - * // UINT8 OptionalData[]; > > - * } EFI_LOAD_OPTION; > > - */ > > -struct load_option { > > - u32 attributes; > > - u16 file_path_length; > > - u16 *label; > > - struct efi_device_path *file_path; > > - u8 *optional_data; > > -}; > > - > > /* parse an EFI_LOAD_OPTION, as described above */ > > -static void parse_load_option(struct load_option *lo, void *ptr) > > +void efi_parse_load_option(struct load_option *lo, void *ptr) > > I think you want to rename this to "deserialize" to better align with ... > > > { > > lo->attributes = *(u32 *)ptr; > > ptr += sizeof(u32); > > @@ -60,7 +40,7 @@ static void parse_load_option(struct load_option *lo, void *ptr) > > ptr += sizeof(u16); > > > > lo->label = ptr; > > - ptr += (u16_strlen(lo->label) + 1) * 2; > > + ptr += (u16_strlen(lo->label) + 1) * sizeof(u16); > > > > lo->file_path = ptr; > > ptr += lo->file_path_length; > > @@ -68,6 +48,48 @@ static void parse_load_option(struct load_option *lo, void *ptr) > > lo->optional_data = ptr; > > } > > > > +unsigned long efi_marshal_load_option(u32 attr, u16 *label, > > ... this function which really should be called "serialize" rather than > "marshal". "Marshalling" is what you do to convert from one API to > another. Here, we want to write an in-memory object to disk. Well, as you know, I borrow this word from RPC world. While I believe that those two are interchangeable in most contexts, I don't care either way if you prefer 'serialize'. > I also think the API would make more sense if you pass in a struct > efi_load_option *. It's more symmetric to the deserialize one then. Absolutely agree. > You also want to add comments to the functions to say what they do and > what their parameters are there for. That will make it easier for people > to grasp on how to use this (now public) API. OK > And I really dislike void * for anything that isn't "opaque". In this > function (and the one above) void * really gets used as byte pointer. > Please mark it as such (u8 *). Right, but there are quite a few of places where "void *" is used instead of "u8 *" for a string. For instance, get_var() in bootmgr.c or more in other files. It's quite painful to change all the places for consistency. > The next problem are the casts. If you cast from u8 * to u32 * and > dereference it, the compiler does not know if that pointer is aligned or > not. So on platforms that don't enable caches yet in the bootmgr path, > we may get unaligned exceptions. So you want to use get_unaligned_le32() > and friends or memcpy (as you did in your function) above. Good point. I will fix them. (This reveals other bugs :) -Takahiro Akashi > > Alex > > > + struct efi_device_path *file_path, > > + char *option, void **data) > > +{ > > + unsigned long size; > > + unsigned long label_len, option_len; > > + u16 file_path_len; > > + void *p; > > + > > + label_len = (u16_strlen(label) + 1) * sizeof(u16); > > + file_path_len = efi_dp_size(file_path) > > + + sizeof(struct efi_device_path); /* for END */ > > + option_len = strlen(option); > > + > > + /* total size */ > > + size = sizeof(attr); > > + size += file_path_len; > > + size += label_len; > > + size += option_len + 1; > > + p = malloc(size); > > + if (!p) > > + return 0; > > + > > + /* copy data */ > > + *data = p; > > + memcpy(p, &attr, sizeof(attr)); > > + p += sizeof(attr); > > + memcpy(p, &file_path_len, sizeof(file_path_len)); > > + p += sizeof(file_path_len); > > + memcpy(p, label, label_len); > > + p += label_len; > > + > > + memcpy(p, file_path, file_path_len); > > + p += file_path_len; > > + > > + memcpy(p, option, option_len); > > + p += option_len; > > + *(char *)p = '\0'; > > + > > + return size; > > +} > > + > > /* free() the result */ > > static void *get_var(u16 *name, const efi_guid_t *vendor, > > efi_uintn_t *size) > > @@ -115,7 +137,7 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path, > > if (!load_option) > > return NULL; > > > > - parse_load_option(&lo, load_option); > > + efi_parse_load_option(&lo, load_option); > > > > if (lo.attributes & LOAD_OPTION_ACTIVE) { > > efi_status_t ret; > >
On 18.10.18 09:57, AKASHI Takahiro wrote: > On Wed, Oct 17, 2018 at 10:40:26AM +0200, Alexander Graf wrote: >> >> >> On 17.10.18 09:32, AKASHI Takahiro wrote: >>> In this patch, helper functions for an load option variable (BootXXXX) >>> are added: >>> * efi_parse_load_option(): parse a string into load_option data >>> (renamed from parse_load_option and exported) >>> * efi_marshel_load_option(): convert load_option data into a string >>> >>> Those functions will be used to implement efishell command. >>> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>> --- >>> include/efi_loader.h | 25 +++++++++++++ >>> lib/efi_loader/efi_bootmgr.c | 68 ++++++++++++++++++++++++------------ >>> 2 files changed, 70 insertions(+), 23 deletions(-) >>> >>> diff --git a/include/efi_loader.h b/include/efi_loader.h >>> index b73fbb6de23f..1cabb1680d20 100644 >>> --- a/include/efi_loader.h >>> +++ b/include/efi_loader.h >>> @@ -502,6 +502,31 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, efi_guid_t *vendor, >>> u32 attributes, efi_uintn_t data_size, >>> void *data); >>> >>> +/* >>> + * See section 3.1.3 in the v2.7 UEFI spec for more details on >>> + * the layout of EFI_LOAD_OPTION. In short it is: >>> + * >>> + * typedef struct _EFI_LOAD_OPTION { >>> + * UINT32 Attributes; >>> + * UINT16 FilePathListLength; >>> + * // CHAR16 Description[]; <-- variable length, NULL terminated >>> + * // EFI_DEVICE_PATH_PROTOCOL FilePathList[]; >>> + * <-- FilePathListLength bytes >>> + * // UINT8 OptionalData[]; >>> + * } EFI_LOAD_OPTION; >>> + */ >>> +struct load_option { >>> + u32 attributes; >>> + u16 file_path_length; >>> + u16 *label; >>> + struct efi_device_path *file_path; >>> + u8 *optional_data; >>> +}; >> >> If this is part of the spec, shouldn't it rather be in efi_api.h? > > While uefi spec mentions this structure, I don't have good confidence > that I can say that it is part of spec or API because there is no interface > (or function) that handles this structure. Good point, it's internal only. Then efi_loader.h is probably a good fit. > >> It >> probably also wants an efi_ prefix then :). > > OK. > >>> + >>> +void efi_parse_load_option(struct load_option *lo, void *ptr); >>> +unsigned long efi_marshal_load_option(u32 attr, u16 *label, >>> + struct efi_device_path *file_path, >>> + char *option, void **data); >>> void *efi_bootmgr_load(struct efi_device_path **device_path, >>> struct efi_device_path **file_path); >>> >>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c >>> index 0c5764db127b..c2d29f956065 100644 >>> --- a/lib/efi_loader/efi_bootmgr.c >>> +++ b/lib/efi_loader/efi_bootmgr.c >>> @@ -30,28 +30,8 @@ static const struct efi_runtime_services *rs; >>> */ >>> >>> >>> -/* >>> - * See section 3.1.3 in the v2.7 UEFI spec for more details on >>> - * the layout of EFI_LOAD_OPTION. In short it is: >>> - * >>> - * typedef struct _EFI_LOAD_OPTION { >>> - * UINT32 Attributes; >>> - * UINT16 FilePathListLength; >>> - * // CHAR16 Description[]; <-- variable length, NULL terminated >>> - * // EFI_DEVICE_PATH_PROTOCOL FilePathList[]; <-- FilePathListLength bytes >>> - * // UINT8 OptionalData[]; >>> - * } EFI_LOAD_OPTION; >>> - */ >>> -struct load_option { >>> - u32 attributes; >>> - u16 file_path_length; >>> - u16 *label; >>> - struct efi_device_path *file_path; >>> - u8 *optional_data; >>> -}; >>> - >>> /* parse an EFI_LOAD_OPTION, as described above */ >>> -static void parse_load_option(struct load_option *lo, void *ptr) >>> +void efi_parse_load_option(struct load_option *lo, void *ptr) >> >> I think you want to rename this to "deserialize" to better align with ... >> >>> { >>> lo->attributes = *(u32 *)ptr; >>> ptr += sizeof(u32); >>> @@ -60,7 +40,7 @@ static void parse_load_option(struct load_option *lo, void *ptr) >>> ptr += sizeof(u16); >>> >>> lo->label = ptr; >>> - ptr += (u16_strlen(lo->label) + 1) * 2; >>> + ptr += (u16_strlen(lo->label) + 1) * sizeof(u16); >>> >>> lo->file_path = ptr; >>> ptr += lo->file_path_length; >>> @@ -68,6 +48,48 @@ static void parse_load_option(struct load_option *lo, void *ptr) >>> lo->optional_data = ptr; >>> } >>> >>> +unsigned long efi_marshal_load_option(u32 attr, u16 *label, >> >> ... this function which really should be called "serialize" rather than >> "marshal". "Marshalling" is what you do to convert from one API to >> another. Here, we want to write an in-memory object to disk. > > Well, as you know, I borrow this word from RPC world. > While I believe that those two are interchangeable in most contexts, > I don't care either way if you prefer 'serialize'. Yeah, I read up on marshalling afterwards too because I got curious and I guess it really boils down to which world you come from. The Windows DOM world always calls everything marshalling, while the Java world calls it serialize usually. Either way, I personally find serialize the more obvious name :). > >> I also think the API would make more sense if you pass in a struct >> efi_load_option *. It's more symmetric to the deserialize one then. > > Absolutely agree. > >> You also want to add comments to the functions to say what they do and >> what their parameters are there for. That will make it easier for people >> to grasp on how to use this (now public) API. > > OK > >> And I really dislike void * for anything that isn't "opaque". In this >> function (and the one above) void * really gets used as byte pointer. >> Please mark it as such (u8 *). > > Right, but there are quite a few of places where "void *" is used > instead of "u8 *" for a string. For instance, get_var() in bootmgr.c > or more in other files. > > It's quite painful to change all the places for consistency. Yeah, let's focus on the serialize/deserialize functions for now and just pass u8* there ;). We can tackle the others when we get to them. Sorry to make you go through this - I should've caught that back when Rob submitted the patches. Alex > >> The next problem are the casts. If you cast from u8 * to u32 * and >> dereference it, the compiler does not know if that pointer is aligned or >> not. So on platforms that don't enable caches yet in the bootmgr path, >> we may get unaligned exceptions. So you want to use get_unaligned_le32() >> and friends or memcpy (as you did in your function) above. > > Good point. I will fix them. > (This reveals other bugs :) > > -Takahiro Akashi > >> >> Alex >> >>> + struct efi_device_path *file_path, >>> + char *option, void **data) >>> +{ >>> + unsigned long size; >>> + unsigned long label_len, option_len; >>> + u16 file_path_len; >>> + void *p; >>> + >>> + label_len = (u16_strlen(label) + 1) * sizeof(u16); >>> + file_path_len = efi_dp_size(file_path) >>> + + sizeof(struct efi_device_path); /* for END */ >>> + option_len = strlen(option); >>> + >>> + /* total size */ >>> + size = sizeof(attr); >>> + size += file_path_len; >>> + size += label_len; >>> + size += option_len + 1; >>> + p = malloc(size); >>> + if (!p) >>> + return 0; >>> + >>> + /* copy data */ >>> + *data = p; >>> + memcpy(p, &attr, sizeof(attr)); >>> + p += sizeof(attr); >>> + memcpy(p, &file_path_len, sizeof(file_path_len)); >>> + p += sizeof(file_path_len); >>> + memcpy(p, label, label_len); >>> + p += label_len; >>> + >>> + memcpy(p, file_path, file_path_len); >>> + p += file_path_len; >>> + >>> + memcpy(p, option, option_len); >>> + p += option_len; >>> + *(char *)p = '\0'; >>> + >>> + return size; >>> +} >>> + >>> /* free() the result */ >>> static void *get_var(u16 *name, const efi_guid_t *vendor, >>> efi_uintn_t *size) >>> @@ -115,7 +137,7 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path, >>> if (!load_option) >>> return NULL; >>> >>> - parse_load_option(&lo, load_option); >>> + efi_parse_load_option(&lo, load_option); >>> >>> if (lo.attributes & LOAD_OPTION_ACTIVE) { >>> efi_status_t ret; >>>
On Thu, Oct 18, 2018 at 10:39:30AM +0200, Alexander Graf wrote: > > > On 18.10.18 09:57, AKASHI Takahiro wrote: > > On Wed, Oct 17, 2018 at 10:40:26AM +0200, Alexander Graf wrote: > >> > >> > >> On 17.10.18 09:32, AKASHI Takahiro wrote: > >>> In this patch, helper functions for an load option variable (BootXXXX) > >>> are added: > >>> * efi_parse_load_option(): parse a string into load_option data > >>> (renamed from parse_load_option and exported) > >>> * efi_marshel_load_option(): convert load_option data into a string > >>> > >>> Those functions will be used to implement efishell command. > >>> > >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > >>> --- > >>> include/efi_loader.h | 25 +++++++++++++ > >>> lib/efi_loader/efi_bootmgr.c | 68 ++++++++++++++++++++++++------------ > >>> 2 files changed, 70 insertions(+), 23 deletions(-) > >>> > >>> diff --git a/include/efi_loader.h b/include/efi_loader.h > >>> index b73fbb6de23f..1cabb1680d20 100644 > >>> --- a/include/efi_loader.h > >>> +++ b/include/efi_loader.h > >>> @@ -502,6 +502,31 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, efi_guid_t *vendor, > >>> u32 attributes, efi_uintn_t data_size, > >>> void *data); > >>> > >>> +/* > >>> + * See section 3.1.3 in the v2.7 UEFI spec for more details on > >>> + * the layout of EFI_LOAD_OPTION. In short it is: > >>> + * > >>> + * typedef struct _EFI_LOAD_OPTION { > >>> + * UINT32 Attributes; > >>> + * UINT16 FilePathListLength; > >>> + * // CHAR16 Description[]; <-- variable length, NULL terminated > >>> + * // EFI_DEVICE_PATH_PROTOCOL FilePathList[]; > >>> + * <-- FilePathListLength bytes > >>> + * // UINT8 OptionalData[]; > >>> + * } EFI_LOAD_OPTION; > >>> + */ > >>> +struct load_option { > >>> + u32 attributes; > >>> + u16 file_path_length; > >>> + u16 *label; > >>> + struct efi_device_path *file_path; > >>> + u8 *optional_data; > >>> +}; > >> > >> If this is part of the spec, shouldn't it rather be in efi_api.h? > > > > While uefi spec mentions this structure, I don't have good confidence > > that I can say that it is part of spec or API because there is no interface > > (or function) that handles this structure. > > Good point, it's internal only. Then efi_loader.h is probably a good fit. OK. > > > >> It > >> probably also wants an efi_ prefix then :). > > > > OK. > > > >>> + > >>> +void efi_parse_load_option(struct load_option *lo, void *ptr); > >>> +unsigned long efi_marshal_load_option(u32 attr, u16 *label, > >>> + struct efi_device_path *file_path, > >>> + char *option, void **data); > >>> void *efi_bootmgr_load(struct efi_device_path **device_path, > >>> struct efi_device_path **file_path); > >>> > >>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c > >>> index 0c5764db127b..c2d29f956065 100644 > >>> --- a/lib/efi_loader/efi_bootmgr.c > >>> +++ b/lib/efi_loader/efi_bootmgr.c > >>> @@ -30,28 +30,8 @@ static const struct efi_runtime_services *rs; > >>> */ > >>> > >>> > >>> -/* > >>> - * See section 3.1.3 in the v2.7 UEFI spec for more details on > >>> - * the layout of EFI_LOAD_OPTION. In short it is: > >>> - * > >>> - * typedef struct _EFI_LOAD_OPTION { > >>> - * UINT32 Attributes; > >>> - * UINT16 FilePathListLength; > >>> - * // CHAR16 Description[]; <-- variable length, NULL terminated > >>> - * // EFI_DEVICE_PATH_PROTOCOL FilePathList[]; <-- FilePathListLength bytes > >>> - * // UINT8 OptionalData[]; > >>> - * } EFI_LOAD_OPTION; > >>> - */ > >>> -struct load_option { > >>> - u32 attributes; > >>> - u16 file_path_length; > >>> - u16 *label; > >>> - struct efi_device_path *file_path; > >>> - u8 *optional_data; > >>> -}; > >>> - > >>> /* parse an EFI_LOAD_OPTION, as described above */ > >>> -static void parse_load_option(struct load_option *lo, void *ptr) > >>> +void efi_parse_load_option(struct load_option *lo, void *ptr) > >> > >> I think you want to rename this to "deserialize" to better align with ... > >> > >>> { > >>> lo->attributes = *(u32 *)ptr; > >>> ptr += sizeof(u32); > >>> @@ -60,7 +40,7 @@ static void parse_load_option(struct load_option *lo, void *ptr) > >>> ptr += sizeof(u16); > >>> > >>> lo->label = ptr; > >>> - ptr += (u16_strlen(lo->label) + 1) * 2; > >>> + ptr += (u16_strlen(lo->label) + 1) * sizeof(u16); > >>> > >>> lo->file_path = ptr; > >>> ptr += lo->file_path_length; > >>> @@ -68,6 +48,48 @@ static void parse_load_option(struct load_option *lo, void *ptr) > >>> lo->optional_data = ptr; > >>> } > >>> > >>> +unsigned long efi_marshal_load_option(u32 attr, u16 *label, > >> > >> ... this function which really should be called "serialize" rather than > >> "marshal". "Marshalling" is what you do to convert from one API to > >> another. Here, we want to write an in-memory object to disk. > > > > Well, as you know, I borrow this word from RPC world. > > While I believe that those two are interchangeable in most contexts, > > I don't care either way if you prefer 'serialize'. > > Yeah, I read up on marshalling afterwards too because I got curious and > I guess it really boils down to which world you come from. The Windows > DOM world always calls everything marshalling, while the Java world > calls it serialize usually. > > Either way, I personally find serialize the more obvious name :). OK. > > > >> I also think the API would make more sense if you pass in a struct > >> efi_load_option *. It's more symmetric to the deserialize one then. > > > > Absolutely agree. > > > >> You also want to add comments to the functions to say what they do and > >> what their parameters are there for. That will make it easier for people > >> to grasp on how to use this (now public) API. > > > > OK > > > >> And I really dislike void * for anything that isn't "opaque". In this > >> function (and the one above) void * really gets used as byte pointer. > >> Please mark it as such (u8 *). > > > > Right, but there are quite a few of places where "void *" is used > > instead of "u8 *" for a string. For instance, get_var() in bootmgr.c > > or more in other files. > > > > It's quite painful to change all the places for consistency. > > Yeah, let's focus on the serialize/deserialize functions for now and > just pass u8* there ;). We can tackle the others when we get to them. Sure. -Takahiro Akashi > Sorry to make you go through this - I should've caught that back when > Rob submitted the patches. > > > Alex > > > > >> The next problem are the casts. If you cast from u8 * to u32 * and > >> dereference it, the compiler does not know if that pointer is aligned or > >> not. So on platforms that don't enable caches yet in the bootmgr path, > >> we may get unaligned exceptions. So you want to use get_unaligned_le32() > >> and friends or memcpy (as you did in your function) above. > > > > Good point. I will fix them. > > (This reveals other bugs :) > > > > -Takahiro Akashi > > > >> > >> Alex > >> > >>> + struct efi_device_path *file_path, > >>> + char *option, void **data) > >>> +{ > >>> + unsigned long size; > >>> + unsigned long label_len, option_len; > >>> + u16 file_path_len; > >>> + void *p; > >>> + > >>> + label_len = (u16_strlen(label) + 1) * sizeof(u16); > >>> + file_path_len = efi_dp_size(file_path) > >>> + + sizeof(struct efi_device_path); /* for END */ > >>> + option_len = strlen(option); > >>> + > >>> + /* total size */ > >>> + size = sizeof(attr); > >>> + size += file_path_len; > >>> + size += label_len; > >>> + size += option_len + 1; > >>> + p = malloc(size); > >>> + if (!p) > >>> + return 0; > >>> + > >>> + /* copy data */ > >>> + *data = p; > >>> + memcpy(p, &attr, sizeof(attr)); > >>> + p += sizeof(attr); > >>> + memcpy(p, &file_path_len, sizeof(file_path_len)); > >>> + p += sizeof(file_path_len); > >>> + memcpy(p, label, label_len); > >>> + p += label_len; > >>> + > >>> + memcpy(p, file_path, file_path_len); > >>> + p += file_path_len; > >>> + > >>> + memcpy(p, option, option_len); > >>> + p += option_len; > >>> + *(char *)p = '\0'; > >>> + > >>> + return size; > >>> +} > >>> + > >>> /* free() the result */ > >>> static void *get_var(u16 *name, const efi_guid_t *vendor, > >>> efi_uintn_t *size) > >>> @@ -115,7 +137,7 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path, > >>> if (!load_option) > >>> return NULL; > >>> > >>> - parse_load_option(&lo, load_option); > >>> + efi_parse_load_option(&lo, load_option); > >>> > >>> if (lo.attributes & LOAD_OPTION_ACTIVE) { > >>> efi_status_t ret; > >>>
diff --git a/include/efi_loader.h b/include/efi_loader.h index b73fbb6de23f..1cabb1680d20 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -502,6 +502,31 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, efi_guid_t *vendor, u32 attributes, efi_uintn_t data_size, void *data); +/* + * See section 3.1.3 in the v2.7 UEFI spec for more details on + * the layout of EFI_LOAD_OPTION. In short it is: + * + * typedef struct _EFI_LOAD_OPTION { + * UINT32 Attributes; + * UINT16 FilePathListLength; + * // CHAR16 Description[]; <-- variable length, NULL terminated + * // EFI_DEVICE_PATH_PROTOCOL FilePathList[]; + * <-- FilePathListLength bytes + * // UINT8 OptionalData[]; + * } EFI_LOAD_OPTION; + */ +struct load_option { + u32 attributes; + u16 file_path_length; + u16 *label; + struct efi_device_path *file_path; + u8 *optional_data; +}; + +void efi_parse_load_option(struct load_option *lo, void *ptr); +unsigned long efi_marshal_load_option(u32 attr, u16 *label, + struct efi_device_path *file_path, + char *option, void **data); void *efi_bootmgr_load(struct efi_device_path **device_path, struct efi_device_path **file_path); diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index 0c5764db127b..c2d29f956065 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -30,28 +30,8 @@ static const struct efi_runtime_services *rs; */ -/* - * See section 3.1.3 in the v2.7 UEFI spec for more details on - * the layout of EFI_LOAD_OPTION. In short it is: - * - * typedef struct _EFI_LOAD_OPTION { - * UINT32 Attributes; - * UINT16 FilePathListLength; - * // CHAR16 Description[]; <-- variable length, NULL terminated - * // EFI_DEVICE_PATH_PROTOCOL FilePathList[]; <-- FilePathListLength bytes - * // UINT8 OptionalData[]; - * } EFI_LOAD_OPTION; - */ -struct load_option { - u32 attributes; - u16 file_path_length; - u16 *label; - struct efi_device_path *file_path; - u8 *optional_data; -}; - /* parse an EFI_LOAD_OPTION, as described above */ -static void parse_load_option(struct load_option *lo, void *ptr) +void efi_parse_load_option(struct load_option *lo, void *ptr) { lo->attributes = *(u32 *)ptr; ptr += sizeof(u32); @@ -60,7 +40,7 @@ static void parse_load_option(struct load_option *lo, void *ptr) ptr += sizeof(u16); lo->label = ptr; - ptr += (u16_strlen(lo->label) + 1) * 2; + ptr += (u16_strlen(lo->label) + 1) * sizeof(u16); lo->file_path = ptr; ptr += lo->file_path_length; @@ -68,6 +48,48 @@ static void parse_load_option(struct load_option *lo, void *ptr) lo->optional_data = ptr; } +unsigned long efi_marshal_load_option(u32 attr, u16 *label, + struct efi_device_path *file_path, + char *option, void **data) +{ + unsigned long size; + unsigned long label_len, option_len; + u16 file_path_len; + void *p; + + label_len = (u16_strlen(label) + 1) * sizeof(u16); + file_path_len = efi_dp_size(file_path) + + sizeof(struct efi_device_path); /* for END */ + option_len = strlen(option); + + /* total size */ + size = sizeof(attr); + size += file_path_len; + size += label_len; + size += option_len + 1; + p = malloc(size); + if (!p) + return 0; + + /* copy data */ + *data = p; + memcpy(p, &attr, sizeof(attr)); + p += sizeof(attr); + memcpy(p, &file_path_len, sizeof(file_path_len)); + p += sizeof(file_path_len); + memcpy(p, label, label_len); + p += label_len; + + memcpy(p, file_path, file_path_len); + p += file_path_len; + + memcpy(p, option, option_len); + p += option_len; + *(char *)p = '\0'; + + return size; +} + /* free() the result */ static void *get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size) @@ -115,7 +137,7 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path, if (!load_option) return NULL; - parse_load_option(&lo, load_option); + efi_parse_load_option(&lo, load_option); if (lo.attributes & LOAD_OPTION_ACTIVE) { efi_status_t ret;
In this patch, helper functions for an load option variable (BootXXXX) are added: * efi_parse_load_option(): parse a string into load_option data (renamed from parse_load_option and exported) * efi_marshel_load_option(): convert load_option data into a string Those functions will be used to implement efishell command. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- include/efi_loader.h | 25 +++++++++++++ lib/efi_loader/efi_bootmgr.c | 68 ++++++++++++++++++++++++------------ 2 files changed, 70 insertions(+), 23 deletions(-)