| Message ID | 20180809065020.8895-1-takahiro.akashi@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | efi_loader: execute image's unload function | expand |
On 08/09/2018 08:50 AM, AKASHI Takahiro wrote: > Currently, unload function in EFI_LOADED_IMAGE_PROTOCOL is never called > at UnloadImage Boot Service. This is not compliant to UEFI specification. > See chapter "9.1 EFI Loaded Image Protocol." With all the patches we got into U-Boot (+HII protocols, +HOB and DXE tables) we now can start the EFI shell. But I do not succeed to run the SCT. Could you, please, provide an overview what is needed. I use the sct-prepare target in https://github.com/xypron/u-boot-build/blob/qemu-arm64/Makefile to set up my disk image with the SCT. Are we missing further patches or is there something wrong with the image I generate? > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > include/efi_api.h | 4 +++- > lib/efi_loader/efi_boottime.c | 9 +++++++++ > 2 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/include/efi_api.h b/include/efi_api.h > index a4343ae98e2..b2806fba3b8 100644 > --- a/include/efi_api.h > +++ b/include/efi_api.h > @@ -326,6 +326,8 @@ struct efi_system_table { > > #define EFI_LOADED_IMAGE_PROTOCOL_REVISION 0x1000 > > +typedef efi_status_t (EFIAPI *efi_image_unload_t)(efi_handle_t *image_handle); In the rest of efi_api.h we never used typedefs for function prototypes. I would prefer to use the explicit type. In scripts/checkpatch.pl this will create a warning. > + > struct efi_loaded_image { > u32 revision; > void *parent_handle; > @@ -339,7 +341,7 @@ struct efi_loaded_image { > aligned_u64 image_size; > unsigned int image_code_type; > unsigned int image_data_type; > - unsigned long unload; > + efi_image_unload_t unload; > > /* Below are efi loader private fields */ > #ifdef CONFIG_EFI_LOADER > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c > index 80061e10ebc..d6fcf7e0049 100644 > --- a/lib/efi_loader/efi_boottime.c > +++ b/lib/efi_loader/efi_boottime.c > @@ -1843,9 +1843,18 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle, > */ > static efi_status_t EFIAPI efi_unload_image(efi_handle_t image_handle) > { > + struct efi_loaded_image *info = image_handle; Please, do not assume the the image_handle points to the EFI_LOADED_IMAGE_PROTOCOL. Anyway this is not valid anymore after http://git.denx.de/?p=u-boot.git;a=commitdiff;h=c982874e930d5d673501cd94df07bcbd215d5883 Best regards Heinrich > struct efi_object *efiobj; > + efi_status_t ret; > > EFI_ENTRY("%p", image_handle); > + > + if (info->unload) { > + ret = info->unload(image_handle); > + if (ret != EFI_SUCCESS) > + return EFI_EXIT(ret); > + } > + > efiobj = efi_search_obj(image_handle); > if (efiobj) > list_del(&efiobj->link); >
Heinrich, On Fri, Sep 28, 2018 at 04:35:57AM +0200, Heinrich Schuchardt wrote: > On 08/09/2018 08:50 AM, AKASHI Takahiro wrote: > > Currently, unload function in EFI_LOADED_IMAGE_PROTOCOL is never called > > at UnloadImage Boot Service. This is not compliant to UEFI specification. > > See chapter "9.1 EFI Loaded Image Protocol." > > With all the patches we got into U-Boot (+HII protocols, +HOB and DXE > tables) we now can start the EFI shell. But I do not succeed to run the > SCT. Could you, please, provide an overview what is needed. > > I use the sct-prepare target in > https://github.com/xypron/u-boot-build/blob/qemu-arm64/Makefile to set > up my disk image with the SCT. > > Are we missing further patches or is there something wrong with the > image I generate? I have the same problem since I rebased my own branch to Alex's efi-next this week. I'm trying to understand what's happening now. I will address your comments below later. Thanks, -Takahiro Akashi > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > include/efi_api.h | 4 +++- > > lib/efi_loader/efi_boottime.c | 9 +++++++++ > > 2 files changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/include/efi_api.h b/include/efi_api.h > > index a4343ae98e2..b2806fba3b8 100644 > > --- a/include/efi_api.h > > +++ b/include/efi_api.h > > @@ -326,6 +326,8 @@ struct efi_system_table { > > > > #define EFI_LOADED_IMAGE_PROTOCOL_REVISION 0x1000 > > > > +typedef efi_status_t (EFIAPI *efi_image_unload_t)(efi_handle_t *image_handle); > > In the rest of efi_api.h we never used typedefs for function prototypes. > I would prefer to use the explicit type. > > In scripts/checkpatch.pl this will create a warning. > > > + > > struct efi_loaded_image { > > u32 revision; > > void *parent_handle; > > @@ -339,7 +341,7 @@ struct efi_loaded_image { > > aligned_u64 image_size; > > unsigned int image_code_type; > > unsigned int image_data_type; > > - unsigned long unload; > > + efi_image_unload_t unload; > > > > /* Below are efi loader private fields */ > > #ifdef CONFIG_EFI_LOADER > > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c > > index 80061e10ebc..d6fcf7e0049 100644 > > --- a/lib/efi_loader/efi_boottime.c > > +++ b/lib/efi_loader/efi_boottime.c > > @@ -1843,9 +1843,18 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle, > > */ > > static efi_status_t EFIAPI efi_unload_image(efi_handle_t image_handle) > > { > > + struct efi_loaded_image *info = image_handle; > > Please, do not assume the the image_handle points to the > EFI_LOADED_IMAGE_PROTOCOL. Anyway this is not valid anymore after > http://git.denx.de/?p=u-boot.git;a=commitdiff;h=c982874e930d5d673501cd94df07bcbd215d5883 > > Best regards > > Heinrich > > > struct efi_object *efiobj; > > + efi_status_t ret; > > > > EFI_ENTRY("%p", image_handle); > > + > > + if (info->unload) { > > + ret = info->unload(image_handle); > > + if (ret != EFI_SUCCESS) > > + return EFI_EXIT(ret); > > + } > > + > > efiobj = efi_search_obj(image_handle); > > if (efiobj) > > list_del(&efiobj->link); > > >
On 09/28/2018 06:24 AM, AKASHI Takahiro wrote: > Heinrich, > > On Fri, Sep 28, 2018 at 04:35:57AM +0200, Heinrich Schuchardt wrote: >> On 08/09/2018 08:50 AM, AKASHI Takahiro wrote: >>> Currently, unload function in EFI_LOADED_IMAGE_PROTOCOL is never called >>> at UnloadImage Boot Service. This is not compliant to UEFI specification. >>> See chapter "9.1 EFI Loaded Image Protocol." >> >> With all the patches we got into U-Boot (+HII protocols, +HOB and DXE >> tables) we now can start the EFI shell. But I do not succeed to run the >> SCT. Could you, please, provide an overview what is needed. >> >> I use the sct-prepare target in >> https://github.com/xypron/u-boot-build/blob/qemu-arm64/Makefile to set >> up my disk image with the SCT. >> >> Are we missing further patches or is there something wrong with the >> image I generate? > > I have the same problem since I rebased my own branch to Alex's > efi-next this week. I'm trying to understand what's happening now. > > I will address your comments below later. > > Thanks, > -Takahiro Akashi > > >>> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>> --- >>> include/efi_api.h | 4 +++- >>> lib/efi_loader/efi_boottime.c | 9 +++++++++ >>> 2 files changed, 12 insertions(+), 1 deletion(-) >>> >>> diff --git a/include/efi_api.h b/include/efi_api.h >>> index a4343ae98e2..b2806fba3b8 100644 >>> --- a/include/efi_api.h >>> +++ b/include/efi_api.h >>> @@ -326,6 +326,8 @@ struct efi_system_table { >>> >>> #define EFI_LOADED_IMAGE_PROTOCOL_REVISION 0x1000 >>> >>> +typedef efi_status_t (EFIAPI *efi_image_unload_t)(efi_handle_t *image_handle); >> >> In the rest of efi_api.h we never used typedefs for function prototypes. >> I would prefer to use the explicit type. >> >> In scripts/checkpatch.pl this will create a warning. >> >>> + >>> struct efi_loaded_image { >>> u32 revision; >>> void *parent_handle; >>> @@ -339,7 +341,7 @@ struct efi_loaded_image { >>> aligned_u64 image_size; >>> unsigned int image_code_type; >>> unsigned int image_data_type; >>> - unsigned long unload; >>> + efi_image_unload_t unload; >>> >>> /* Below are efi loader private fields */ >>> #ifdef CONFIG_EFI_LOADER >>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c >>> index 80061e10ebc..d6fcf7e0049 100644 >>> --- a/lib/efi_loader/efi_boottime.c >>> +++ b/lib/efi_loader/efi_boottime.c >>> @@ -1843,9 +1843,18 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle, >>> */ >>> static efi_status_t EFIAPI efi_unload_image(efi_handle_t image_handle) >>> { >>> + struct efi_loaded_image *info = image_handle; >> >> Please, do not assume the the image_handle points to the >> EFI_LOADED_IMAGE_PROTOCOL. Anyway this is not valid anymore after >> http://git.denx.de/?p=u-boot.git;a=commitdiff;h=c982874e930d5d673501cd94df07bcbd215d5883 >> >> Best regards >> >> Heinrich >> >>> struct efi_object *efiobj; >>> + efi_status_t ret; >>> >>> EFI_ENTRY("%p", image_handle); >>> + >>> + if (info->unload) { >>> + ret = info->unload(image_handle); >>> + if (ret != EFI_SUCCESS) >>> + return EFI_EXIT(ret); >>> + } We have to check that image_handle is valid and refers to a loaded image before trying to call the unload function. The unload function shall only be called for a loaded image. When unloading we have to close all protocol interfaces opened by image_handle as agent. Best regards Heinrich >>> + >>> efiobj = efi_search_obj(image_handle); >>> if (efiobj) >>> list_del(&efiobj->link); >>> >> >
Heinrich, On Fri, Sep 28, 2018 at 01:24:42PM +0900, AKASHI Takahiro wrote: > Heinrich, > > On Fri, Sep 28, 2018 at 04:35:57AM +0200, Heinrich Schuchardt wrote: > > On 08/09/2018 08:50 AM, AKASHI Takahiro wrote: > > > Currently, unload function in EFI_LOADED_IMAGE_PROTOCOL is never called > > > at UnloadImage Boot Service. This is not compliant to UEFI specification. > > > See chapter "9.1 EFI Loaded Image Protocol." > > > > With all the patches we got into U-Boot (+HII protocols, +HOB and DXE > > tables) we now can start the EFI shell. But I do not succeed to run the > > SCT. Could you, please, provide an overview what is needed. > > > > I use the sct-prepare target in > > https://github.com/xypron/u-boot-build/blob/qemu-arm64/Makefile to set > > up my disk image with the SCT. > > > > Are we missing further patches or is there something wrong with the > > image I generate? > > I have the same problem since I rebased my own branch to Alex's > efi-next this week. I'm trying to understand what's happening now. Does my patch[1] fix your case? [1] https://lists.denx.de/pipermail/u-boot/2018-October/342501.html Or do you see another issue here? -Takahiro Akashi > I will address your comments below later. > > Thanks, > -Takahiro Akashi > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > --- > > > include/efi_api.h | 4 +++- > > > lib/efi_loader/efi_boottime.c | 9 +++++++++ > > > 2 files changed, 12 insertions(+), 1 deletion(-) > > > > > > diff --git a/include/efi_api.h b/include/efi_api.h > > > index a4343ae98e2..b2806fba3b8 100644 > > > --- a/include/efi_api.h > > > +++ b/include/efi_api.h > > > @@ -326,6 +326,8 @@ struct efi_system_table { > > > > > > #define EFI_LOADED_IMAGE_PROTOCOL_REVISION 0x1000 > > > > > > +typedef efi_status_t (EFIAPI *efi_image_unload_t)(efi_handle_t *image_handle); > > > > In the rest of efi_api.h we never used typedefs for function prototypes. > > I would prefer to use the explicit type. > > > > In scripts/checkpatch.pl this will create a warning. > > > > > + > > > struct efi_loaded_image { > > > u32 revision; > > > void *parent_handle; > > > @@ -339,7 +341,7 @@ struct efi_loaded_image { > > > aligned_u64 image_size; > > > unsigned int image_code_type; > > > unsigned int image_data_type; > > > - unsigned long unload; > > > + efi_image_unload_t unload; > > > > > > /* Below are efi loader private fields */ > > > #ifdef CONFIG_EFI_LOADER > > > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c > > > index 80061e10ebc..d6fcf7e0049 100644 > > > --- a/lib/efi_loader/efi_boottime.c > > > +++ b/lib/efi_loader/efi_boottime.c > > > @@ -1843,9 +1843,18 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle, > > > */ > > > static efi_status_t EFIAPI efi_unload_image(efi_handle_t image_handle) > > > { > > > + struct efi_loaded_image *info = image_handle; > > > > Please, do not assume the the image_handle points to the > > EFI_LOADED_IMAGE_PROTOCOL. Anyway this is not valid anymore after > > http://git.denx.de/?p=u-boot.git;a=commitdiff;h=c982874e930d5d673501cd94df07bcbd215d5883 > > > > Best regards > > > > Heinrich > > > > > struct efi_object *efiobj; > > > + efi_status_t ret; > > > > > > EFI_ENTRY("%p", image_handle); > > > + > > > + if (info->unload) { > > > + ret = info->unload(image_handle); > > > + if (ret != EFI_SUCCESS) > > > + return EFI_EXIT(ret); > > > + } > > > + > > > efiobj = efi_search_obj(image_handle); > > > if (efiobj) > > > list_del(&efiobj->link); > > > > >
diff --git a/include/efi_api.h b/include/efi_api.h index a4343ae98e2..b2806fba3b8 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -326,6 +326,8 @@ struct efi_system_table { #define EFI_LOADED_IMAGE_PROTOCOL_REVISION 0x1000 +typedef efi_status_t (EFIAPI *efi_image_unload_t)(efi_handle_t *image_handle); + struct efi_loaded_image { u32 revision; void *parent_handle; @@ -339,7 +341,7 @@ struct efi_loaded_image { aligned_u64 image_size; unsigned int image_code_type; unsigned int image_data_type; - unsigned long unload; + efi_image_unload_t unload; /* Below are efi loader private fields */ #ifdef CONFIG_EFI_LOADER diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 80061e10ebc..d6fcf7e0049 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1843,9 +1843,18 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle, */ static efi_status_t EFIAPI efi_unload_image(efi_handle_t image_handle) { + struct efi_loaded_image *info = image_handle; struct efi_object *efiobj; + efi_status_t ret; EFI_ENTRY("%p", image_handle); + + if (info->unload) { + ret = info->unload(image_handle); + if (ret != EFI_SUCCESS) + return EFI_EXIT(ret); + } + efiobj = efi_search_obj(image_handle); if (efiobj) list_del(&efiobj->link);
Currently, unload function in EFI_LOADED_IMAGE_PROTOCOL is never called at UnloadImage Boot Service. This is not compliant to UEFI specification. See chapter "9.1 EFI Loaded Image Protocol." Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- include/efi_api.h | 4 +++- lib/efi_loader/efi_boottime.c | 9 +++++++++ 2 files changed, 12 insertions(+), 1 deletion(-)