[PATCH] efi: Fix booting failure with UEFI on ARM

[PATCH] efi: Fix booting failure with UEFI on ARM

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Commit 9fd08b4 (efi: split out efi_get_gop()) splits out the
codes getting the pointer to GOP as efi_get_gop(), but it doesn't
initialize the variable handles and gop to NULL like what the original
codes do. This will cause booting failure on ARM while printing below
logs:
	Xen 4.7-unstable (c/s Tue Oct 13 14:40:28 2015 +0100 git:7a92036) EFI loader
	Synchronous Exception at 0x00000000FECB021C

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 xen/common/efi/boot.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 29ce66a..d233cef 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -636,8 +636,8 @@ static void __init efi_console_set_mode(void)
 static EFI_GRAPHICS_OUTPUT_PROTOCOL __init *efi_get_gop(void)
 {
     EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info;
-    EFI_GRAPHICS_OUTPUT_PROTOCOL *gop;
-    EFI_HANDLE *handles;
+    EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
+    EFI_HANDLE *handles = NULL;
     EFI_STATUS status;
     UINTN info_size, size = 0;
     static EFI_GUID __initdata gop_guid = EFI_GRAPHICS_OUTPUT_PROTOCOL_GUID;
-- 
2.0.4

Re: [PATCH] efi: Fix booting failure with UEFI on ARM

[Jan Beulich]

>>> On 10.11.15 at 08:55, <zhaoshenglong@huawei.com> wrote:
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -636,8 +636,8 @@ static void __init efi_console_set_mode(void)
>  static EFI_GRAPHICS_OUTPUT_PROTOCOL __init *efi_get_gop(void)
>  {
>      EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info;
> -    EFI_GRAPHICS_OUTPUT_PROTOCOL *gop;
> -    EFI_HANDLE *handles;
> +    EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
> +    EFI_HANDLE *handles = NULL;

I can see the issue with handles being left uninitialized, but if you
think gop also needs initializing, then I would have expected an
explanation. The change is simple enough to take as is, but in the
future please explain changes that aren't obviously needed.

Also please Cc maintainers for the touched code on your patches.

Jan

Re: [PATCH] efi: Fix booting failure with UEFI on ARM

[Daniel Kiper]

On Tue, Nov 10, 2015 at 03:55:50PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> Commit 9fd08b4 (efi: split out efi_get_gop()) splits out the
> codes getting the pointer to GOP as efi_get_gop(), but it doesn't
> initialize the variable handles and gop to NULL like what the original

As I can see original code did not initialized gop to NULL but it does
not hurt. So, ...

> codes do. This will cause booting failure on ARM while printing below
> logs:
> 	Xen 4.7-unstable (c/s Tue Oct 13 14:40:28 2015 +0100 git:7a92036) EFI loader
> 	Synchronous Exception at 0x00000000FECB021C
>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH] efi: Fix booting failure with UEFI on ARM

[Shannon Zhao]

On 2015/11/10 18:45, Jan Beulich wrote:
>>>> On 10.11.15 at 08:55, <zhaoshenglong@huawei.com> wrote:
>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -636,8 +636,8 @@ static void __init efi_console_set_mode(void)
>>  static EFI_GRAPHICS_OUTPUT_PROTOCOL __init *efi_get_gop(void)
>>  {
>>      EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info;
>> -    EFI_GRAPHICS_OUTPUT_PROTOCOL *gop;
>> -    EFI_HANDLE *handles;
>> +    EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
>> +    EFI_HANDLE *handles = NULL;
> 
> I can see the issue with handles being left uninitialized, but if you
> think gop also needs initializing, then I would have expected an
> explanation. The change is simple enough to take as is, but in the
> future please explain changes that aren't obviously needed.
> 
Yes, the root cause is the uninitialized handles not the uninitialized
gop. It just does what the original codes do before applying commit
9fd08b4.

@@ -738,14 +773,12 @@ void EFIAPI __init noreturn
 efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
 {
     static EFI_GUID __initdata loaded_image_guid = LOADED_IMAGE_PROTOCOL;
-    static EFI_GUID __initdata gop_guid =
EFI_GRAPHICS_OUTPUT_PROTOCOL_GUID;
     static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID;
     EFI_LOADED_IMAGE *loaded_image;
     EFI_STATUS status;
     unsigned int i, argc;
     CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL;
     UINTN map_key, info_size, gop_mode = ~0;
-    EFI_HANDLE *handles = NULL;
     EFI_SHIM_LOCK_PROTOCOL *shim_lock;
     EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;

If you think it's not proper, I'll fix this.

Thanks,
-- 
Shannon