public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] OvmfPkg/X86QemuLoadImageLib: fix "unused variable" error in X64 DXE builds
@ 2020-03-06 23:04 Laszlo Ersek
  2020-03-07  0:01 ` Philippe Mathieu-Daudé
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Laszlo Ersek @ 2020-03-06 23:04 UTC (permalink / raw)
  To: edk2-devel-groups-io
  Cc: Ard Biesheuvel, Jordan Justen, Philippe Mathieu-Daudé

When the MDE_CPU_IA32 macro is not defined, there is no access to the
"KernelImageHandle" local variable in QemuStartKernelImage(). This breaks
the OvmfPkgIa32X64 and OvmfPkgX64 platform builds, at least with gcc-8.

Move the local variable to the inner scope, where declaration and usage
are inseparable.

(Note that such inner-scope declarations are frowned upon in the wider
edk2 codebase, but we use them liberally in ArmVirtPkg and OvmfPkg anyway,
because they help us reason about variable lifetime and visibility.)

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Fixes: 7c47d89003a6f8f7f6f0ce8ca7d3e87c630d14cc
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2572
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    Ard, if you get to it first, feel free to push this in my stead. Thanks!
    
    Repo:   https://pagure.io/lersek/edk2.git
    Branch: x86qlil_build_fix

 OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
index c5bd6862b265..1868c9fcafdf 100644
--- a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
+++ b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
@@ -457,67 +457,68 @@ EFIAPI
 QemuStartKernelImage (
   IN  OUT EFI_HANDLE            *ImageHandle
   )
 {
   EFI_STATUS                    Status;
   OVMF_LOADED_X86_LINUX_KERNEL  *LoadedImage;
-  EFI_HANDLE                    KernelImageHandle;
 
   Status = gBS->OpenProtocol (
                   *ImageHandle,
                   &gOvmfLoadedX86LinuxKernelProtocolGuid,
                   (VOID **)&LoadedImage,
                   gImageHandle,                  // AgentHandle
                   NULL,                          // ControllerHandle
                   EFI_OPEN_PROTOCOL_GET_PROTOCOL
                   );
   if (!EFI_ERROR (Status)) {
     return QemuStartLegacyImage (*ImageHandle);
   }
 
   Status = gBS->StartImage (
                   *ImageHandle,
                   NULL,              // ExitDataSize
                   NULL               // ExitData
                   );
 #ifdef MDE_CPU_IA32
   if (Status == EFI_UNSUPPORTED) {
+    EFI_HANDLE KernelImageHandle;
+
     //
     // On IA32, EFI_UNSUPPORTED means that the image's machine type is X64 while
     // we are expecting a IA32 one, and the StartImage () boot service is unable
     // to handle it, either because the image does not have the special .compat
     // PE/COFF section that Linux specifies for mixed mode capable images, or
     // because we are running without the support code for that. So load the
     // image again, using the legacy loader, and unload the normally loaded
     // image before starting the legacy one.
     //
     Status = QemuLoadLegacyImage (&KernelImageHandle);
     if (EFI_ERROR (Status)) {
       //
       // Note: no change to (*ImageHandle), the caller will release it.
       //
       return Status;
     }
     //
     // Swap in the legacy-loaded image.
     //
     QemuUnloadKernelImage (*ImageHandle);
     *ImageHandle = KernelImageHandle;
     return QemuStartLegacyImage (KernelImageHandle);
   }
 #endif
   return Status;
 }
 
 /**
   Unloads an image loaded with QemuLoadKernelImage ().
 
   @param  ImageHandle             Handle that identifies the image to be
                                   unloaded.
 
   @retval EFI_SUCCESS             The image has been unloaded.
   @retval EFI_UNSUPPORTED         The image has been started, and does not
                                   support unload.
   @retval EFI_INVALID_PARAMETER   ImageHandle is not a valid image handle.
 
   @return                         Exit code from the image's unload function.
 **/
-- 
2.19.1.3.g30247aa5d201


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] OvmfPkg/X86QemuLoadImageLib: fix "unused variable" error in X64 DXE builds
  2020-03-06 23:04 [PATCH] OvmfPkg/X86QemuLoadImageLib: fix "unused variable" error in X64 DXE builds Laszlo Ersek
@ 2020-03-07  0:01 ` Philippe Mathieu-Daudé
  2020-03-07  1:00   ` [edk2-devel] " Laszlo Ersek
  2020-03-07 14:34 ` Ard Biesheuvel
  2020-03-09 23:19 ` Laszlo Ersek
  2 siblings, 1 reply; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-03-07  0:01 UTC (permalink / raw)
  To: Laszlo Ersek, edk2-devel-groups-io; +Cc: Ard Biesheuvel, Jordan Justen

On 3/7/20 12:04 AM, Laszlo Ersek wrote:
> When the MDE_CPU_IA32 macro is not defined, there is no access to the
> "KernelImageHandle" local variable in QemuStartKernelImage(). This breaks
> the OvmfPkgIa32X64 and OvmfPkgX64 platform builds, at least with gcc-8.
> 
> Move the local variable to the inner scope, where declaration and usage
> are inseparable.
> 
> (Note that such inner-scope declarations are frowned upon in the wider
> edk2 codebase, but we use them liberally in ArmVirtPkg and OvmfPkg anyway,
> because they help us reason about variable lifetime and visibility.)
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Fixes: 7c47d89003a6f8f7f6f0ce8ca7d3e87c630d14cc
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2572
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daude <philmd@redhat.com>

> ---
> 
> Notes:
>      Ard, if you get to it first, feel free to push this in my stead. Thanks!
>      
>      Repo:   https://pagure.io/lersek/edk2.git
>      Branch: x86qlil_build_fix
> 
>   OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
> index c5bd6862b265..1868c9fcafdf 100644
> --- a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
> +++ b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
> @@ -457,67 +457,68 @@ EFIAPI
>   QemuStartKernelImage (
>     IN  OUT EFI_HANDLE            *ImageHandle
>     )
>   {
>     EFI_STATUS                    Status;
>     OVMF_LOADED_X86_LINUX_KERNEL  *LoadedImage;
> -  EFI_HANDLE                    KernelImageHandle;
>   
>     Status = gBS->OpenProtocol (
>                     *ImageHandle,
>                     &gOvmfLoadedX86LinuxKernelProtocolGuid,
>                     (VOID **)&LoadedImage,
>                     gImageHandle,                  // AgentHandle
>                     NULL,                          // ControllerHandle
>                     EFI_OPEN_PROTOCOL_GET_PROTOCOL
>                     );
>     if (!EFI_ERROR (Status)) {
>       return QemuStartLegacyImage (*ImageHandle);
>     }
>   
>     Status = gBS->StartImage (
>                     *ImageHandle,
>                     NULL,              // ExitDataSize
>                     NULL               // ExitData
>                     );
>   #ifdef MDE_CPU_IA32
>     if (Status == EFI_UNSUPPORTED) {
> +    EFI_HANDLE KernelImageHandle;
> +
>       //
>       // On IA32, EFI_UNSUPPORTED means that the image's machine type is X64 while
>       // we are expecting a IA32 one, and the StartImage () boot service is unable
>       // to handle it, either because the image does not have the special .compat
>       // PE/COFF section that Linux specifies for mixed mode capable images, or
>       // because we are running without the support code for that. So load the
>       // image again, using the legacy loader, and unload the normally loaded
>       // image before starting the legacy one.
>       //
>       Status = QemuLoadLegacyImage (&KernelImageHandle);
>       if (EFI_ERROR (Status)) {
>         //
>         // Note: no change to (*ImageHandle), the caller will release it.
>         //
>         return Status;
>       }
>       //
>       // Swap in the legacy-loaded image.
>       //
>       QemuUnloadKernelImage (*ImageHandle);
>       *ImageHandle = KernelImageHandle;
>       return QemuStartLegacyImage (KernelImageHandle);
>     }
>   #endif
>     return Status;
>   }
>   
>   /**
>     Unloads an image loaded with QemuLoadKernelImage ().
>   
>     @param  ImageHandle             Handle that identifies the image to be
>                                     unloaded.
>   
>     @retval EFI_SUCCESS             The image has been unloaded.
>     @retval EFI_UNSUPPORTED         The image has been started, and does not
>                                     support unload.
>     @retval EFI_INVALID_PARAMETER   ImageHandle is not a valid image handle.
>   
>     @return                         Exit code from the image's unload function.
>   **/
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [edk2-devel] [PATCH] OvmfPkg/X86QemuLoadImageLib: fix "unused variable" error in X64 DXE builds
  2020-03-07  0:01 ` Philippe Mathieu-Daudé
@ 2020-03-07  1:00   ` Laszlo Ersek
  2020-03-07  6:10     ` Ard Biesheuvel
  0 siblings, 1 reply; 9+ messages in thread
From: Laszlo Ersek @ 2020-03-07  1:00 UTC (permalink / raw)
  To: devel, philmd; +Cc: Ard Biesheuvel, Jordan Justen

On 03/07/20 01:01, Philippe Mathieu-Daudé wrote:
> On 3/7/20 12:04 AM, Laszlo Ersek wrote:
>> When the MDE_CPU_IA32 macro is not defined, there is no access to the
>> "KernelImageHandle" local variable in QemuStartKernelImage(). This breaks
>> the OvmfPkgIa32X64 and OvmfPkgX64 platform builds, at least with gcc-8.
>>
>> Move the local variable to the inner scope, where declaration and usage
>> are inseparable.
>>
>> (Note that such inner-scope declarations are frowned upon in the wider
>> edk2 codebase, but we use them liberally in ArmVirtPkg and OvmfPkg
>> anyway,
>> because they help us reason about variable lifetime and visibility.)
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Fixes: 7c47d89003a6f8f7f6f0ce8ca7d3e87c630d14cc
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2572
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> 
> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
> Tested-by: Philippe Mathieu-Daude <philmd@redhat.com>

Thank you, Phil, this really helps -- I want to push the patch as
quickly as possible, due to it averting a build break.

So I've submitted <https://github.com/tianocore/edk2/pull/428> with your
tags applied... but mergify is apparently not picking up the "push"
label again... I have no idea what's going on.

Laszlo


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [edk2-devel] [PATCH] OvmfPkg/X86QemuLoadImageLib: fix "unused variable" error in X64 DXE builds
  2020-03-07  1:00   ` [edk2-devel] " Laszlo Ersek
@ 2020-03-07  6:10     ` Ard Biesheuvel
  2020-03-07  7:28       ` Laszlo Ersek
  0 siblings, 1 reply; 9+ messages in thread
From: Ard Biesheuvel @ 2020-03-07  6:10 UTC (permalink / raw)
  To: Laszlo Ersek
  Cc: edk2-devel-groups-io, Philippe Mathieu-Daudé, Jordan Justen

On Sat, 7 Mar 2020 at 02:00, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 03/07/20 01:01, Philippe Mathieu-Daudé wrote:
> > On 3/7/20 12:04 AM, Laszlo Ersek wrote:
> >> When the MDE_CPU_IA32 macro is not defined, there is no access to the
> >> "KernelImageHandle" local variable in QemuStartKernelImage(). This breaks
> >> the OvmfPkgIa32X64 and OvmfPkgX64 platform builds, at least with gcc-8.
> >>
> >> Move the local variable to the inner scope, where declaration and usage
> >> are inseparable.
> >>
> >> (Note that such inner-scope declarations are frowned upon in the wider
> >> edk2 codebase, but we use them liberally in ArmVirtPkg and OvmfPkg
> >> anyway,
> >> because they help us reason about variable lifetime and visibility.)
> >>
> >> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> Cc: Jordan Justen <jordan.l.justen@intel.com>
> >> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> Fixes: 7c47d89003a6f8f7f6f0ce8ca7d3e87c630d14cc
> >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2572
> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> >
> > Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
> > Tested-by: Philippe Mathieu-Daude <philmd@redhat.com>
>
> Thank you, Phil, this really helps -- I want to push the patch as
> quickly as possible, due to it averting a build break.
>
> So I've submitted <https://github.com/tianocore/edk2/pull/428> with your
> tags applied... but mergify is apparently not picking up the "push"
> label again... I have no idea what's going on.
>

I queued it here as well:
https://github.com/tianocore/edk2/pull/427

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [edk2-devel] [PATCH] OvmfPkg/X86QemuLoadImageLib: fix "unused variable" error in X64 DXE builds
  2020-03-07  6:10     ` Ard Biesheuvel
@ 2020-03-07  7:28       ` Laszlo Ersek
  2020-03-07  7:41         ` Laszlo Ersek
  0 siblings, 1 reply; 9+ messages in thread
From: Laszlo Ersek @ 2020-03-07  7:28 UTC (permalink / raw)
  To: devel, ard.biesheuvel; +Cc: Philippe Mathieu-Daudé, Jordan Justen

On 03/07/20 07:10, Ard Biesheuvel wrote:
> On Sat, 7 Mar 2020 at 02:00, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 03/07/20 01:01, Philippe Mathieu-Daudé wrote:
>>> On 3/7/20 12:04 AM, Laszlo Ersek wrote:
>>>> When the MDE_CPU_IA32 macro is not defined, there is no access to the
>>>> "KernelImageHandle" local variable in QemuStartKernelImage(). This breaks
>>>> the OvmfPkgIa32X64 and OvmfPkgX64 platform builds, at least with gcc-8.
>>>>
>>>> Move the local variable to the inner scope, where declaration and usage
>>>> are inseparable.
>>>>
>>>> (Note that such inner-scope declarations are frowned upon in the wider
>>>> edk2 codebase, but we use them liberally in ArmVirtPkg and OvmfPkg
>>>> anyway,
>>>> because they help us reason about variable lifetime and visibility.)
>>>>
>>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> Fixes: 7c47d89003a6f8f7f6f0ce8ca7d3e87c630d14cc
>>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2572
>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>
>>> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
>>> Tested-by: Philippe Mathieu-Daude <philmd@redhat.com>
>>
>> Thank you, Phil, this really helps -- I want to push the patch as
>> quickly as possible, due to it averting a build break.
>>
>> So I've submitted <https://github.com/tianocore/edk2/pull/428> with your
>> tags applied... but mergify is apparently not picking up the "push"
>> label again... I have no idea what's going on.
>>
> 
> I queued it here as well:
> https://github.com/tianocore/edk2/pull/427


Nope, still stuck, both #427 and #428.

"We are currently investigating a delay in notification delivery."

"We are monitoring service recovery. We are processing a backlog of
notifications."

https://www.githubstatus.com/incidents/lcgmxy9pmgm4

The last update says "resolved" (Mar 06, 2020 - 00:22 UTC), but I don't
really believe that...

I've contacted github about this.

Laszlo


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [edk2-devel] [PATCH] OvmfPkg/X86QemuLoadImageLib: fix "unused variable" error in X64 DXE builds
  2020-03-07  7:28       ` Laszlo Ersek
@ 2020-03-07  7:41         ` Laszlo Ersek
  0 siblings, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2020-03-07  7:41 UTC (permalink / raw)
  To: devel, ard.biesheuvel; +Cc: Philippe Mathieu-Daudé, Jordan Justen

On 03/07/20 08:28, Laszlo Ersek wrote:
> On 03/07/20 07:10, Ard Biesheuvel wrote:

>> I queued it here as well:
>> https://github.com/tianocore/edk2/pull/427
> 
> 
> Nope, still stuck, both #427 and #428.
> 
> "We are currently investigating a delay in notification delivery."
> 
> "We are monitoring service recovery. We are processing a backlog of
> notifications."
> 
> https://www.githubstatus.com/incidents/lcgmxy9pmgm4
> 
> The last update says "resolved" (Mar 06, 2020 - 00:22 UTC), but I don't
> really believe that...
> 
> I've contacted github about this.

Ticket ID: 592107

Thanks
Laszlo


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [edk2-devel] [PATCH] OvmfPkg/X86QemuLoadImageLib: fix "unused variable" error in X64 DXE builds
  2020-03-06 23:04 [PATCH] OvmfPkg/X86QemuLoadImageLib: fix "unused variable" error in X64 DXE builds Laszlo Ersek
  2020-03-07  0:01 ` Philippe Mathieu-Daudé
@ 2020-03-07 14:34 ` Ard Biesheuvel
  2020-03-08  9:40   ` Laszlo Ersek
  2020-03-09 23:19 ` Laszlo Ersek
  2 siblings, 1 reply; 9+ messages in thread
From: Ard Biesheuvel @ 2020-03-07 14:34 UTC (permalink / raw)
  To: edk2-devel-groups-io, Laszlo Ersek
  Cc: Jordan Justen, Philippe Mathieu-Daudé

On Sat, 7 Mar 2020 at 00:04, Laszlo Ersek <lersek@redhat.com> wrote:
>
> When the MDE_CPU_IA32 macro is not defined, there is no access to the
> "KernelImageHandle" local variable in QemuStartKernelImage(). This breaks
> the OvmfPkgIa32X64 and OvmfPkgX64 platform builds, at least with gcc-8.
>
> Move the local variable to the inner scope, where declaration and usage
> are inseparable.
>
> (Note that such inner-scope declarations are frowned upon in the wider
> edk2 codebase, but we use them liberally in ArmVirtPkg and OvmfPkg anyway,
> because they help us reason about variable lifetime and visibility.)
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Fixes: 7c47d89003a6f8f7f6f0ce8ca7d3e87c630d14cc
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2572
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

(in case you still need it)

> ---
>
> Notes:
>     Ard, if you get to it first, feel free to push this in my stead. Thanks!
>
>     Repo:   https://pagure.io/lersek/edk2.git
>     Branch: x86qlil_build_fix
>
>  OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
> index c5bd6862b265..1868c9fcafdf 100644
> --- a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
> +++ b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
> @@ -457,67 +457,68 @@ EFIAPI
>  QemuStartKernelImage (
>    IN  OUT EFI_HANDLE            *ImageHandle
>    )
>  {
>    EFI_STATUS                    Status;
>    OVMF_LOADED_X86_LINUX_KERNEL  *LoadedImage;
> -  EFI_HANDLE                    KernelImageHandle;
>
>    Status = gBS->OpenProtocol (
>                    *ImageHandle,
>                    &gOvmfLoadedX86LinuxKernelProtocolGuid,
>                    (VOID **)&LoadedImage,
>                    gImageHandle,                  // AgentHandle
>                    NULL,                          // ControllerHandle
>                    EFI_OPEN_PROTOCOL_GET_PROTOCOL
>                    );
>    if (!EFI_ERROR (Status)) {
>      return QemuStartLegacyImage (*ImageHandle);
>    }
>
>    Status = gBS->StartImage (
>                    *ImageHandle,
>                    NULL,              // ExitDataSize
>                    NULL               // ExitData
>                    );
>  #ifdef MDE_CPU_IA32
>    if (Status == EFI_UNSUPPORTED) {
> +    EFI_HANDLE KernelImageHandle;
> +
>      //
>      // On IA32, EFI_UNSUPPORTED means that the image's machine type is X64 while
>      // we are expecting a IA32 one, and the StartImage () boot service is unable
>      // to handle it, either because the image does not have the special .compat
>      // PE/COFF section that Linux specifies for mixed mode capable images, or
>      // because we are running without the support code for that. So load the
>      // image again, using the legacy loader, and unload the normally loaded
>      // image before starting the legacy one.
>      //
>      Status = QemuLoadLegacyImage (&KernelImageHandle);
>      if (EFI_ERROR (Status)) {
>        //
>        // Note: no change to (*ImageHandle), the caller will release it.
>        //
>        return Status;
>      }
>      //
>      // Swap in the legacy-loaded image.
>      //
>      QemuUnloadKernelImage (*ImageHandle);
>      *ImageHandle = KernelImageHandle;
>      return QemuStartLegacyImage (KernelImageHandle);
>    }
>  #endif
>    return Status;
>  }
>
>  /**
>    Unloads an image loaded with QemuLoadKernelImage ().
>
>    @param  ImageHandle             Handle that identifies the image to be
>                                    unloaded.
>
>    @retval EFI_SUCCESS             The image has been unloaded.
>    @retval EFI_UNSUPPORTED         The image has been started, and does not
>                                    support unload.
>    @retval EFI_INVALID_PARAMETER   ImageHandle is not a valid image handle.
>
>    @return                         Exit code from the image's unload function.
>  **/
> --
> 2.19.1.3.g30247aa5d201
>
>
> 
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [edk2-devel] [PATCH] OvmfPkg/X86QemuLoadImageLib: fix "unused variable" error in X64 DXE builds
  2020-03-07 14:34 ` Ard Biesheuvel
@ 2020-03-08  9:40   ` Laszlo Ersek
  0 siblings, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2020-03-08  9:40 UTC (permalink / raw)
  To: Ard Biesheuvel, edk2-devel-groups-io
  Cc: Jordan Justen, Philippe Mathieu-Daudé

On 03/07/20 15:34, Ard Biesheuvel wrote:
> On Sat, 7 Mar 2020 at 00:04, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> When the MDE_CPU_IA32 macro is not defined, there is no access to the
>> "KernelImageHandle" local variable in QemuStartKernelImage(). This breaks
>> the OvmfPkgIa32X64 and OvmfPkgX64 platform builds, at least with gcc-8.
>>
>> Move the local variable to the inner scope, where declaration and usage
>> are inseparable.
>>
>> (Note that such inner-scope declarations are frowned upon in the wider
>> edk2 codebase, but we use them liberally in ArmVirtPkg and OvmfPkg anyway,
>> because they help us reason about variable lifetime and visibility.)
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Fixes: 7c47d89003a6f8f7f6f0ce8ca7d3e87c630d14cc
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2572
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> 
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> (in case you still need it)

Thanks. If the workaround for the current github / mergify outage turns
out to be "resubmit pending PRs", then I'll likely pick up your R-b too.

Seeing how your latest <https://github.com/tianocore/edk2/pull/430> is
also stuck, I must wonder what is going on at github. They have not
responded to my ticket (592107) and their status page claims "All
Systems Operational":

https://www.githubstatus.com/

Which is not the case.

Mergify have their own status page:

https://www.notion.so/Mergify-Status-Page-7803a762235d4ee6bed1f9976d17bd83

and that also claims "Dashboard Operational" and "Engine Operational".

I've now sent an email to <support@mergify.io> too.

Laszlo


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [edk2-devel] [PATCH] OvmfPkg/X86QemuLoadImageLib: fix "unused variable" error in X64 DXE builds
  2020-03-06 23:04 [PATCH] OvmfPkg/X86QemuLoadImageLib: fix "unused variable" error in X64 DXE builds Laszlo Ersek
  2020-03-07  0:01 ` Philippe Mathieu-Daudé
  2020-03-07 14:34 ` Ard Biesheuvel
@ 2020-03-09 23:19 ` Laszlo Ersek
  2 siblings, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2020-03-09 23:19 UTC (permalink / raw)
  To: edk2-devel-groups-io
  Cc: Ard Biesheuvel, Jordan Justen, Philippe Mathieu-Daudé

On 03/07/20 00:04, Laszlo Ersek wrote:
> When the MDE_CPU_IA32 macro is not defined, there is no access to the
> "KernelImageHandle" local variable in QemuStartKernelImage(). This breaks
> the OvmfPkgIa32X64 and OvmfPkgX64 platform builds, at least with gcc-8.
> 
> Move the local variable to the inner scope, where declaration and usage
> are inseparable.
> 
> (Note that such inner-scope declarations are frowned upon in the wider
> edk2 codebase, but we use them liberally in ArmVirtPkg and OvmfPkg anyway,
> because they help us reason about variable lifetime and visibility.)
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Fixes: 7c47d89003a6f8f7f6f0ce8ca7d3e87c630d14cc
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2572
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     Ard, if you get to it first, feel free to push this in my stead. Thanks!
>     
>     Repo:   https://pagure.io/lersek/edk2.git
>     Branch: x86qlil_build_fix
> 
>  OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
> index c5bd6862b265..1868c9fcafdf 100644
> --- a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
> +++ b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
> @@ -457,67 +457,68 @@ EFIAPI
>  QemuStartKernelImage (
>    IN  OUT EFI_HANDLE            *ImageHandle
>    )
>  {
>    EFI_STATUS                    Status;
>    OVMF_LOADED_X86_LINUX_KERNEL  *LoadedImage;
> -  EFI_HANDLE                    KernelImageHandle;
>  
>    Status = gBS->OpenProtocol (
>                    *ImageHandle,
>                    &gOvmfLoadedX86LinuxKernelProtocolGuid,
>                    (VOID **)&LoadedImage,
>                    gImageHandle,                  // AgentHandle
>                    NULL,                          // ControllerHandle
>                    EFI_OPEN_PROTOCOL_GET_PROTOCOL
>                    );
>    if (!EFI_ERROR (Status)) {
>      return QemuStartLegacyImage (*ImageHandle);
>    }
>  
>    Status = gBS->StartImage (
>                    *ImageHandle,
>                    NULL,              // ExitDataSize
>                    NULL               // ExitData
>                    );
>  #ifdef MDE_CPU_IA32
>    if (Status == EFI_UNSUPPORTED) {
> +    EFI_HANDLE KernelImageHandle;
> +
>      //
>      // On IA32, EFI_UNSUPPORTED means that the image's machine type is X64 while
>      // we are expecting a IA32 one, and the StartImage () boot service is unable
>      // to handle it, either because the image does not have the special .compat
>      // PE/COFF section that Linux specifies for mixed mode capable images, or
>      // because we are running without the support code for that. So load the
>      // image again, using the legacy loader, and unload the normally loaded
>      // image before starting the legacy one.
>      //
>      Status = QemuLoadLegacyImage (&KernelImageHandle);
>      if (EFI_ERROR (Status)) {
>        //
>        // Note: no change to (*ImageHandle), the caller will release it.
>        //
>        return Status;
>      }
>      //
>      // Swap in the legacy-loaded image.
>      //
>      QemuUnloadKernelImage (*ImageHandle);
>      *ImageHandle = KernelImageHandle;
>      return QemuStartLegacyImage (KernelImageHandle);
>    }
>  #endif
>    return Status;
>  }
>  
>  /**
>    Unloads an image loaded with QemuLoadKernelImage ().
>  
>    @param  ImageHandle             Handle that identifies the image to be
>                                    unloaded.
>  
>    @retval EFI_SUCCESS             The image has been unloaded.
>    @retval EFI_UNSUPPORTED         The image has been started, and does not
>                                    support unload.
>    @retval EFI_INVALID_PARAMETER   ImageHandle is not a valid image handle.
>  
>    @return                         Exit code from the image's unload function.
>  **/
> 

Commit a3e25cc8a1dd.

Thanks
Laszlo


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-03-09 23:19 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-03-06 23:04 [PATCH] OvmfPkg/X86QemuLoadImageLib: fix "unused variable" error in X64 DXE builds Laszlo Ersek
2020-03-07  0:01 ` Philippe Mathieu-Daudé
2020-03-07  1:00   ` [edk2-devel] " Laszlo Ersek
2020-03-07  6:10     ` Ard Biesheuvel
2020-03-07  7:28       ` Laszlo Ersek
2020-03-07  7:41         ` Laszlo Ersek
2020-03-07 14:34 ` Ard Biesheuvel
2020-03-08  9:40   ` Laszlo Ersek
2020-03-09 23:19 ` Laszlo Ersek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox