Or else the EFI service calls will use the wrong calling convention.

The __ms_abi__ attribute is available on all supported versions of
clang.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <iwj@xenproject.org>

Without this a Xen built with clang won't be able to correctly use the
EFI services, leading to weird messages from the firmware and crashes.
The impact of this fix for GCC users is exactly 0, and will fix the
build on clang.

The biggest fallout from this could be using the attribute on a
compiler that doesn't support it, which would translate into a build
failure, but the gitlab tests have shown no issues.
---
 xen/include/asm-x86/x86_64/efibind.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/asm-x86/x86_64/efibind.h b/xen/include/asm-x86/x86_64/efibind.h
index b013db175d..ddcfae07ec 100644
--- a/xen/include/asm-x86/x86_64/efibind.h
+++ b/xen/include/asm-x86/x86_64/efibind.h
@@ -172,7 +172,7 @@ typedef uint64_t   UINTN;
 #ifndef EFIAPI                  // Forces EFI calling conventions reguardless of compiler options
     #ifdef _MSC_EXTENSIONS
         #define EFIAPI __cdecl  // Force C calling convention for Microsoft C compiler
-    #elif __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
+    #elif __clang__ || __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
         #define EFIAPI __attribute__((__ms_abi__))  // Force Microsoft ABI
     #else
         #define EFIAPI          // Substitute expresion to force C calling convention
-- 
2.29.2

On 03.02.2021 18:58, Roger Pau Monne wrote:
> --- a/xen/include/asm-x86/x86_64/efibind.h
> +++ b/xen/include/asm-x86/x86_64/efibind.h
> @@ -172,7 +172,7 @@ typedef uint64_t   UINTN;
>  #ifndef EFIAPI                  // Forces EFI calling conventions reguardless of compiler options
>      #ifdef _MSC_EXTENSIONS
>          #define EFIAPI __cdecl  // Force C calling convention for Microsoft C compiler
> -    #elif __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
> +    #elif __clang__ || __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
>          #define EFIAPI __attribute__((__ms_abi__))  // Force Microsoft ABI
>      #else
>          #define EFIAPI          // Substitute expresion to force C calling convention
> 

So the problem is that some capable Clang versions set too low
a __GNUC_MINOR__ (I'm observing 2 alongside __GNUC__ being 4
on Clang5). The way the description and change are written
made me rather imply __GNUC__ to not be available at all,
which I then thought can't be the case because iirc we use it
elsewhere.

Jan

On Thu, Feb 04, 2021 at 11:27:17AM +0100, Jan Beulich wrote:
> On 03.02.2021 18:58, Roger Pau Monne wrote:
> > --- a/xen/include/asm-x86/x86_64/efibind.h
> > +++ b/xen/include/asm-x86/x86_64/efibind.h
> > @@ -172,7 +172,7 @@ typedef uint64_t   UINTN;
> >  #ifndef EFIAPI                  // Forces EFI calling conventions reguardless of compiler options
> >      #ifdef _MSC_EXTENSIONS
> >          #define EFIAPI __cdecl  // Force C calling convention for Microsoft C compiler
> > -    #elif __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
> > +    #elif __clang__ || __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
> >          #define EFIAPI __attribute__((__ms_abi__))  // Force Microsoft ABI
> >      #else
> >          #define EFIAPI          // Substitute expresion to force C calling convention
> > 
> 
> So the problem is that some capable Clang versions set too low
> a __GNUC_MINOR__ (I'm observing 2 alongside __GNUC__ being 4
> on Clang5). The way the description and change are written
> made me rather imply __GNUC__ to not be available at all,
> which I then thought can't be the case because iirc we use it
> elsewhere.

Yes, I also see 4.2 on Clang 11.

Do you want me to expand the description by adding:

"Add a specific Clang check because the GCC version reported by Clang
is below the required 4.4 to use the __ms_abi__ attribute."

Thanks, Roger.

On 04.02.2021 11:51, Roger Pau Monné wrote:
> On Thu, Feb 04, 2021 at 11:27:17AM +0100, Jan Beulich wrote:
>> On 03.02.2021 18:58, Roger Pau Monne wrote:
>>> --- a/xen/include/asm-x86/x86_64/efibind.h
>>> +++ b/xen/include/asm-x86/x86_64/efibind.h
>>> @@ -172,7 +172,7 @@ typedef uint64_t   UINTN;
>>>  #ifndef EFIAPI                  // Forces EFI calling conventions reguardless of compiler options
>>>      #ifdef _MSC_EXTENSIONS
>>>          #define EFIAPI __cdecl  // Force C calling convention for Microsoft C compiler
>>> -    #elif __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
>>> +    #elif __clang__ || __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
>>>          #define EFIAPI __attribute__((__ms_abi__))  // Force Microsoft ABI
>>>      #else
>>>          #define EFIAPI          // Substitute expresion to force C calling convention
>>>
>>
>> So the problem is that some capable Clang versions set too low
>> a __GNUC_MINOR__ (I'm observing 2 alongside __GNUC__ being 4
>> on Clang5). The way the description and change are written
>> made me rather imply __GNUC__ to not be available at all,
>> which I then thought can't be the case because iirc we use it
>> elsewhere.
> 
> Yes, I also see 4.2 on Clang 11.
> 
> Do you want me to expand the description by adding:
> 
> "Add a specific Clang check because the GCC version reported by Clang
> is below the required 4.4 to use the __ms_abi__ attribute."

I guess adding this is easy enough when done while committing.
Thanks for the addition.

Jan

Roger Pau Monne writes ("[PATCH for-4.15] x86/efi: enable MS ABI attribute on clang"):
> Or else the EFI service calls will use the wrong calling convention.
> 
> The __ms_abi__ attribute is available on all supported versions of
> clang.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Ian Jackson <iwj@xenproject.org>
> 
> Without this a Xen built with clang won't be able to correctly use the
> EFI services, leading to weird messages from the firmware and crashes.
> The impact of this fix for GCC users is exactly 0, and will fix the
> build on clang.

Reviewed-by: Ian Jackson <iwj@xenproject.org>

> The biggest fallout from this could be using the attribute on a
> compiler that doesn't support it, which would translate into a build
> failure, but the gitlab tests have shown no issues.

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

Thanks for the thorough attention to the release question in your
mails.  You're making my work very easy :-).

Ian.

On 03/02/2021 17:58, Roger Pau Monne wrote:
> Or else the EFI service calls will use the wrong calling convention.
>
> The __ms_abi__ attribute is available on all supported versions of
> clang.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
> Cc: Ian Jackson <iwj@xenproject.org>
>
> Without this a Xen built with clang won't be able to correctly use the
> EFI services, leading to weird messages from the firmware and crashes.
> The impact of this fix for GCC users is exactly 0, and will fix the
> build on clang.

DYM "fix the compiled binary"?

The build on clang isn't broken atm, but it provably has the wrong ABI.

~Andrew