[edk2-devel] [PATCH] OvmfPkg/Sec: Use correct prototype of ProcessLibraryConstructorList()

[edk2-devel] [PATCH] OvmfPkg/Sec: Use correct prototype of ProcessLibraryConstructorList()

[Ard Biesheuvel via groups.io]

From: Ard Biesheuvel <ardb@kernel.org>

The prototype of ProcessLibraryConstructorList(), which is generated by
the build tools, depends on the boot phase, and SEC and PEI differ in
this regard. However, OVMF's SEC implemention includes PeimEntryPoint.h
and calls the PEI version of the protoype (passing NULL arguments),
whereas the actual implementation observed by the compiler follows the
SEC version, which takes VOID.

The compiler usually doesn't spot the difference, but the LTO linker
does, and throws an error:

MdePkg/Include/Library/PeimEntryPoint.h:74:1: error: type of 'ProcessLibraryConstructorList' does not match original declaration [-Werror=lto-type-mismatch]
   74 | ProcessLibraryConstructorList (
      | ^
Build/OvmfX64/RELEASE_GCC5/X64/OvmfPkg/Sec/SecMain/DEBUG/AutoGen.c:310:1: note: type mismatch in parameter 1
  310 | ProcessLibraryConstructorList (
      | ^
Build/OvmfX64/RELEASE_GCC5/X64/OvmfPkg/Sec/SecMain/DEBUG/AutoGen.c:310:1: note: type 'void' should match type 'void *'
Build/OvmfX64/RELEASE_GCC5/X64/OvmfPkg/Sec/SecMain/DEBUG/AutoGen.c:310:1: note: 'ProcessLibraryConstructorList' was previously declared here

Fix this by dropping the #include, and providing a declaration of
ProcessLibraryConstructorList() in SecMain.c

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=4643
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 OvmfPkg/Sec/SecMain.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
index 31da5d0ace51..7b7d4793984c 100644
--- a/OvmfPkg/Sec/SecMain.c
+++ b/OvmfPkg/Sec/SecMain.c
@@ -11,7 +11,6 @@
 
 #include <PiPei.h>
 
-#include <Library/PeimEntryPoint.h>
 #include <Library/BaseLib.h>
 #include <Library/DebugLib.h>
 #include <Library/BaseMemoryLib.h>
@@ -39,6 +38,12 @@ typedef struct _SEC_IDT_TABLE {
   IA32_IDT_GATE_DESCRIPTOR    IdtTable[SEC_IDT_ENTRY_COUNT];
 } SEC_IDT_TABLE;
 
+VOID
+EFIAPI
+ProcessLibraryConstructorList (
+  VOID
+  );
+
 VOID
 EFIAPI
 SecStartupPhase2 (
@@ -844,7 +849,7 @@ SecCoreStartupWithStack (
     InitializeCpuExceptionHandlers (NULL);
   }
 
-  ProcessLibraryConstructorList (NULL, NULL);
+  ProcessLibraryConstructorList ();
 
   if (!SevEsIsEnabled ()) {
     //
-- 
2.43.0.594.gd9cf4e227d-goog



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115091): https://edk2.groups.io/g/devel/message/115091
Mute This Topic: https://groups.io/mt/104167522/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg/Sec: Use correct prototype of ProcessLibraryConstructorList()

[Laszlo Ersek]

Hello Ard,

On 2/2/24 19:24, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> The prototype of ProcessLibraryConstructorList(), which is generated by
> the build tools, depends on the boot phase, and SEC and PEI differ in
> this regard. However, OVMF's SEC implemention includes PeimEntryPoint.h
> and calls the PEI version of the protoype (passing NULL arguments),
> whereas the actual implementation observed by the compiler follows the
> SEC version, which takes VOID.
> 
> The compiler usually doesn't spot the difference, but the LTO linker
> does, and throws an error:
> 
> MdePkg/Include/Library/PeimEntryPoint.h:74:1: error: type of 'ProcessLibraryConstructorList' does not match original declaration [-Werror=lto-type-mismatch]
>    74 | ProcessLibraryConstructorList (
>       | ^
> Build/OvmfX64/RELEASE_GCC5/X64/OvmfPkg/Sec/SecMain/DEBUG/AutoGen.c:310:1: note: type mismatch in parameter 1
>   310 | ProcessLibraryConstructorList (
>       | ^
> Build/OvmfX64/RELEASE_GCC5/X64/OvmfPkg/Sec/SecMain/DEBUG/AutoGen.c:310:1: note: type 'void' should match type 'void *'
> Build/OvmfX64/RELEASE_GCC5/X64/OvmfPkg/Sec/SecMain/DEBUG/AutoGen.c:310:1: note: 'ProcessLibraryConstructorList' was previously declared here
> 
> Fix this by dropping the #include, and providing a declaration of
> ProcessLibraryConstructorList() in SecMain.c
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=4643
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  OvmfPkg/Sec/SecMain.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

This issue was reported 5+ years ago:

  https://bugzilla.tianocore.org/show_bug.cgi?id=990

I asked for a general solution in BaseTools:

  https://bugzilla.tianocore.org/show_bug.cgi?id=991

Liming recommended that BaseTools should generate a proper declaration
in "AutoGen.h":

  https://bugzilla.tianocore.org/show_bug.cgi?id=991#c2

But nothing happened.

> 
> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
> index 31da5d0ace51..7b7d4793984c 100644
> --- a/OvmfPkg/Sec/SecMain.c
> +++ b/OvmfPkg/Sec/SecMain.c
> @@ -11,7 +11,6 @@
>  
>  #include <PiPei.h>
>  
> -#include <Library/PeimEntryPoint.h>
>  #include <Library/BaseLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/BaseMemoryLib.h>
> @@ -39,6 +38,12 @@ typedef struct _SEC_IDT_TABLE {
>    IA32_IDT_GATE_DESCRIPTOR    IdtTable[SEC_IDT_ENTRY_COUNT];
>  } SEC_IDT_TABLE;
>  
> +VOID
> +EFIAPI
> +ProcessLibraryConstructorList (
> +  VOID
> +  );
> +

This is my problem right here.

Why do 10+ files have to do this individually? See the above BZs for
details.

Well, whatever.

What a terrible project.

*shrug*

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

>  VOID
>  EFIAPI
>  SecStartupPhase2 (
> @@ -844,7 +849,7 @@ SecCoreStartupWithStack (
>      InitializeCpuExceptionHandlers (NULL);
>    }
>  
> -  ProcessLibraryConstructorList (NULL, NULL);
> +  ProcessLibraryConstructorList ();
>  
>    if (!SevEsIsEnabled ()) {
>      //



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115089): https://edk2.groups.io/g/devel/message/115089
Mute This Topic: https://groups.io/mt/104167522/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg/Sec: Use correct prototype of ProcessLibraryConstructorList()

[Laszlo Ersek]

On 2/5/24 01:44, Laszlo Ersek wrote:
> Hello Ard,
> 
> On 2/2/24 19:24, Ard Biesheuvel wrote:
>> From: Ard Biesheuvel <ardb@kernel.org>
>>
>> The prototype of ProcessLibraryConstructorList(), which is generated by
>> the build tools, depends on the boot phase, and SEC and PEI differ in
>> this regard. However, OVMF's SEC implemention includes PeimEntryPoint.h
>> and calls the PEI version of the protoype (passing NULL arguments),
>> whereas the actual implementation observed by the compiler follows the
>> SEC version, which takes VOID.
>>
>> The compiler usually doesn't spot the difference, but the LTO linker
>> does, and throws an error:
>>
>> MdePkg/Include/Library/PeimEntryPoint.h:74:1: error: type of 'ProcessLibraryConstructorList' does not match original declaration [-Werror=lto-type-mismatch]
>>    74 | ProcessLibraryConstructorList (
>>       | ^
>> Build/OvmfX64/RELEASE_GCC5/X64/OvmfPkg/Sec/SecMain/DEBUG/AutoGen.c:310:1: note: type mismatch in parameter 1
>>   310 | ProcessLibraryConstructorList (
>>       | ^
>> Build/OvmfX64/RELEASE_GCC5/X64/OvmfPkg/Sec/SecMain/DEBUG/AutoGen.c:310:1: note: type 'void' should match type 'void *'
>> Build/OvmfX64/RELEASE_GCC5/X64/OvmfPkg/Sec/SecMain/DEBUG/AutoGen.c:310:1: note: 'ProcessLibraryConstructorList' was previously declared here
>>
>> Fix this by dropping the #include, and providing a declaration of
>> ProcessLibraryConstructorList() in SecMain.c
>>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=4643
>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>> ---
>>  OvmfPkg/Sec/SecMain.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> This issue was reported 5+ years ago:
> 
>   https://bugzilla.tianocore.org/show_bug.cgi?id=990
> 
> I asked for a general solution in BaseTools:
> 
>   https://bugzilla.tianocore.org/show_bug.cgi?id=991
> 
> Liming recommended that BaseTools should generate a proper declaration
> in "AutoGen.h":
> 
>   https://bugzilla.tianocore.org/show_bug.cgi?id=991#c2
> 
> But nothing happened.
> 
>>
>> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
>> index 31da5d0ace51..7b7d4793984c 100644
>> --- a/OvmfPkg/Sec/SecMain.c
>> +++ b/OvmfPkg/Sec/SecMain.c
>> @@ -11,7 +11,6 @@
>>  
>>  #include <PiPei.h>
>>  
>> -#include <Library/PeimEntryPoint.h>
>>  #include <Library/BaseLib.h>
>>  #include <Library/DebugLib.h>
>>  #include <Library/BaseMemoryLib.h>
>> @@ -39,6 +38,12 @@ typedef struct _SEC_IDT_TABLE {
>>    IA32_IDT_GATE_DESCRIPTOR    IdtTable[SEC_IDT_ENTRY_COUNT];
>>  } SEC_IDT_TABLE;
>>  
>> +VOID
>> +EFIAPI
>> +ProcessLibraryConstructorList (
>> +  VOID
>> +  );
>> +
> 
> This is my problem right here.
> 
> Why do 10+ files have to do this individually? See the above BZs for
> details.

I'm working on patches that will solve this problem centrally; I'm
including this patch at the front of that series.

Thanks
Laszlo

> 
> Well, whatever.
> 
> What a terrible project.
> 
> *shrug*
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
>>  VOID
>>  EFIAPI
>>  SecStartupPhase2 (
>> @@ -844,7 +849,7 @@ SecCoreStartupWithStack (
>>      InitializeCpuExceptionHandlers (NULL);
>>    }
>>  
>> -  ProcessLibraryConstructorList (NULL, NULL);
>> +  ProcessLibraryConstructorList ();
>>  
>>    if (!SevEsIsEnabled ()) {
>>      //
> 
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115191): https://edk2.groups.io/g/devel/message/115191
Mute This Topic: https://groups.io/mt/104167522/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-