| Message ID | 1376090777-20090-13-git-send-email-roy.franz@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2013-08-09 at 16:26 -0700, Roy Franz wrote: > EFI calls can made directly on ARM, so the function pointers > are directly invoked. This allows types to be checked at > compile time, so here we ensure that the parameters match > the function signature. > > Signed-off-by: Roy Franz <roy.franz@linaro.org> > --- Tested on arm64. Acked-by: Mark Salter <msalter@redhat.com>
On Fri, 9 Aug 2013 16:26:13 -0700, Roy Franz <roy.franz@linaro.org> wrote: > EFI calls can made directly on ARM, so the function pointers > are directly invoked. This allows types to be checked at > compile time, so here we ensure that the parameters match > the function signature. > > Signed-off-by: Roy Franz <roy.franz@linaro.org> > --- > drivers/firmware/efi/efi-stub-helper.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/firmware/efi/efi-stub-helper.c b/drivers/firmware/efi/efi-stub-helper.c > index b707a9f..4bb542f 100644 > --- a/drivers/firmware/efi/efi-stub-helper.c > +++ b/drivers/firmware/efi/efi-stub-helper.c > @@ -321,7 +321,7 @@ static efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg, > status = efi_call_phys3(sys_table_arg->boottime->allocate_pool, > EFI_LOADER_DATA, > nr_files * sizeof(*files), > - &files); > + (void **)&files); > if (status != EFI_SUCCESS) { > efi_printk(sys_table_arg, "Failed to alloc mem for file handle list\n"); > goto fail; > @@ -372,7 +372,8 @@ static efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg, > boottime = sys_table_arg->boottime; > > status = efi_call_phys3(boottime->handle_protocol, > - image->device_handle, &fs_proto, &io); > + image->device_handle, &fs_proto, > + (void **)&io); > if (status != EFI_SUCCESS) { > efi_printk(sys_table_arg, "Failed to handle fs_proto\n"); > goto free_files; > @@ -406,7 +407,8 @@ static efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg, > > grow: > status = efi_call_phys3(sys_table_arg->boottime->allocate_pool, > - EFI_LOADER_DATA, info_sz, &info); > + EFI_LOADER_DATA, info_sz, > + (void **)&info); > if (status != EFI_SUCCESS) { > efi_printk(sys_table_arg, "Failed to alloc mem for file info\n"); > goto close_handles; > @@ -456,18 +458,19 @@ grow: > > addr = file_addr; > for (j = 0; j < nr_files; j++) { > - u64 size; > + unsigned long size; > > size = files[j].size; > while (size) { > - u64 chunksize; > + unsigned long chunksize; > if (size > EFI_READ_CHUNK_SIZE) > chunksize = EFI_READ_CHUNK_SIZE; > else > chunksize = size; > status = efi_call_phys3(fh->read, > files[j].handle, > - &chunksize, addr); > + &chunksize, > + (void *)addr); I think you need some description in the commit text about why the type of chunksize is changing. It's not clear why you're making this change. It is a bug fix? g.
On Fri, Aug 30, 2013 at 6:39 AM, Grant Likely <grant.likely@secretlab.ca> wrote: > On Fri, 9 Aug 2013 16:26:13 -0700, Roy Franz <roy.franz@linaro.org> wrote: >> EFI calls can made directly on ARM, so the function pointers >> are directly invoked. This allows types to be checked at >> compile time, so here we ensure that the parameters match >> the function signature. >> >> Signed-off-by: Roy Franz <roy.franz@linaro.org> >> --- >> drivers/firmware/efi/efi-stub-helper.c | 15 +++++++++------ >> 1 file changed, 9 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/firmware/efi/efi-stub-helper.c b/drivers/firmware/efi/efi-stub-helper.c >> index b707a9f..4bb542f 100644 >> --- a/drivers/firmware/efi/efi-stub-helper.c >> +++ b/drivers/firmware/efi/efi-stub-helper.c >> @@ -321,7 +321,7 @@ static efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg, >> status = efi_call_phys3(sys_table_arg->boottime->allocate_pool, >> EFI_LOADER_DATA, >> nr_files * sizeof(*files), >> - &files); >> + (void **)&files); >> if (status != EFI_SUCCESS) { >> efi_printk(sys_table_arg, "Failed to alloc mem for file handle list\n"); >> goto fail; >> @@ -372,7 +372,8 @@ static efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg, >> boottime = sys_table_arg->boottime; >> >> status = efi_call_phys3(boottime->handle_protocol, >> - image->device_handle, &fs_proto, &io); >> + image->device_handle, &fs_proto, >> + (void **)&io); >> if (status != EFI_SUCCESS) { >> efi_printk(sys_table_arg, "Failed to handle fs_proto\n"); >> goto free_files; >> @@ -406,7 +407,8 @@ static efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg, >> >> grow: >> status = efi_call_phys3(sys_table_arg->boottime->allocate_pool, >> - EFI_LOADER_DATA, info_sz, &info); >> + EFI_LOADER_DATA, info_sz, >> + (void **)&info); >> if (status != EFI_SUCCESS) { >> efi_printk(sys_table_arg, "Failed to alloc mem for file info\n"); >> goto close_handles; >> @@ -456,18 +458,19 @@ grow: >> >> addr = file_addr; >> for (j = 0; j < nr_files; j++) { >> - u64 size; >> + unsigned long size; >> >> size = files[j].size; >> while (size) { >> - u64 chunksize; >> + unsigned long chunksize; >> if (size > EFI_READ_CHUNK_SIZE) >> chunksize = EFI_READ_CHUNK_SIZE; >> else >> chunksize = size; >> status = efi_call_phys3(fh->read, >> files[j].handle, >> - &chunksize, addr); >> + &chunksize, >> + (void *)addr); > > I think you need some description in the commit text about why the type > of chunksize is changing. It's not clear why you're making this change. > It is a bug fix? The type was changed to match the EFI_FILE_PROTOCOL specification - chunk size is defined as a native width unsigned integer. So yes, this is a bug in that this is wrong for ia32, since that really should be a u32 in that case. Since the x86 wrappers break type checking by the compiler these types were never checked at compile time before. chunksize is the the only change that isn't a cast to a type of void pointer. I can break this change out as a separate patch if desired. Matt - would you like me to split the chunksize type change into a separate patch from the arm EFI stub series? Roy > > g.
On Sat, Aug 31, 2013 at 12:12 AM, Roy Franz <roy.franz@linaro.org> wrote: > On Fri, Aug 30, 2013 at 6:39 AM, Grant Likely <grant.likely@secretlab.ca> wrote: >> I think you need some description in the commit text about why the type >> of chunksize is changing. It's not clear why you're making this change. >> It is a bug fix? > > The type was changed to match the EFI_FILE_PROTOCOL specification - > chunk size is > defined as a native width unsigned integer. So yes, this is a bug in > that this is wrong for > ia32, since that really should be a u32 in that case. > Since the x86 wrappers break type checking by the compiler these types > were never checked at compile time > before. chunksize is the the only change that isn't a cast to a type > of void pointer. > > I can break this change out as a separate patch if desired. I don't think it needs to be in a separate patch, but it does need to be described in the patch description. g.
On Fri, Aug 30, 2013 at 4:14 PM, Grant Likely <grant.likely@secretlab.ca> wrote: > On Sat, Aug 31, 2013 at 12:12 AM, Roy Franz <roy.franz@linaro.org> wrote: >> On Fri, Aug 30, 2013 at 6:39 AM, Grant Likely <grant.likely@secretlab.ca> wrote: >>> I think you need some description in the commit text about why the type >>> of chunksize is changing. It's not clear why you're making this change. >>> It is a bug fix? >> >> The type was changed to match the EFI_FILE_PROTOCOL specification - >> chunk size is >> defined as a native width unsigned integer. So yes, this is a bug in >> that this is wrong for >> ia32, since that really should be a u32 in that case. >> Since the x86 wrappers break type checking by the compiler these types >> were never checked at compile time >> before. chunksize is the the only change that isn't a cast to a type >> of void pointer. >> >> I can break this change out as a separate patch if desired. > > I don't think it needs to be in a separate patch, but it does need to > be described in the patch description. > > g. Did that - how about: Fix types in EFI calls to match EFI function definitions. EFI calls can made directly on ARM, so the function pointers are directly invoked. This allows types to be checked at compile time, so here we ensure that the parameters match the function signature. The wrappers used by x86 prevent any type checking. Correct the type of chunksize to be based on native width as specified by the EFI_FILE_PROTOCOL read() function. Signed-off-by: Roy Franz <roy.franz@linaro.org>
diff --git a/drivers/firmware/efi/efi-stub-helper.c b/drivers/firmware/efi/efi-stub-helper.c index b707a9f..4bb542f 100644 --- a/drivers/firmware/efi/efi-stub-helper.c +++ b/drivers/firmware/efi/efi-stub-helper.c @@ -321,7 +321,7 @@ static efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg, status = efi_call_phys3(sys_table_arg->boottime->allocate_pool, EFI_LOADER_DATA, nr_files * sizeof(*files), - &files); + (void **)&files); if (status != EFI_SUCCESS) { efi_printk(sys_table_arg, "Failed to alloc mem for file handle list\n"); goto fail; @@ -372,7 +372,8 @@ static efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg, boottime = sys_table_arg->boottime; status = efi_call_phys3(boottime->handle_protocol, - image->device_handle, &fs_proto, &io); + image->device_handle, &fs_proto, + (void **)&io); if (status != EFI_SUCCESS) { efi_printk(sys_table_arg, "Failed to handle fs_proto\n"); goto free_files; @@ -406,7 +407,8 @@ static efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg, grow: status = efi_call_phys3(sys_table_arg->boottime->allocate_pool, - EFI_LOADER_DATA, info_sz, &info); + EFI_LOADER_DATA, info_sz, + (void **)&info); if (status != EFI_SUCCESS) { efi_printk(sys_table_arg, "Failed to alloc mem for file info\n"); goto close_handles; @@ -456,18 +458,19 @@ grow: addr = file_addr; for (j = 0; j < nr_files; j++) { - u64 size; + unsigned long size; size = files[j].size; while (size) { - u64 chunksize; + unsigned long chunksize; if (size > EFI_READ_CHUNK_SIZE) chunksize = EFI_READ_CHUNK_SIZE; else chunksize = size; status = efi_call_phys3(fh->read, files[j].handle, - &chunksize, addr); + &chunksize, + (void *)addr); if (status != EFI_SUCCESS) { efi_printk(sys_table_arg, "Failed to read file\n"); goto free_file_total;
EFI calls can made directly on ARM, so the function pointers are directly invoked. This allows types to be checked at compile time, so here we ensure that the parameters match the function signature. Signed-off-by: Roy Franz <roy.franz@linaro.org> --- drivers/firmware/efi/efi-stub-helper.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)