[PATCH 0/3] xen/arm: fix build errors with -Og

[PATCH 0/3] xen/arm: fix build errors with -Og

[Stewart Hildebrand]

This is a collection of fixes needed to build the hypervisor with -Og for arm64.

I build-tested this with the following command:

make -j $(nproc) \
    EXTRA_CFLAGS_XEN_CORE="-Og" \
    XEN_TARGET_ARCH=arm64 \
    CROSS_COMPILE=aarch64-none-linux-gnu- \
    dist-xen


Stewart Hildebrand (3):
  xen/arm: mark __guest_cmpxchg always_inline
  xen/efi: fix unitialized use warning
  xen/arm: fix unitialized use warning

 xen/arch/arm/domain_build.c              |  2 +-
 xen/arch/arm/include/asm/guest_atomics.h | 10 +++++-----
 xen/common/efi/boot.c                    |  9 +++++++++
 3 files changed, 15 insertions(+), 6 deletions(-)

-- 
2.40.0

[PATCH 1/3] xen/arm: mark __guest_cmpxchg always_inline

[Stewart Hildebrand]

When building the hypervisor with -Og, we run into a __bad_cmpxchg link error:

aarch64-none-linux-gnu-ld: prelink.o: in function `__int_cmpxchg':
.../xen/./arch/arm/include/asm/arm64/cmpxchg.h:117: undefined reference to `__bad_cmpxchg'
aarch64-none-linux-gnu-ld: .../xen/./arch/arm/include/asm/arm64/cmpxchg.h:117: undefined reference to `__bad_cmpxchg'
aarch64-none-linux-gnu-ld: ./.xen-syms.0: hidden symbol `__bad_cmpxchg' isn't defined
aarch64-none-linux-gnu-ld: final link failed: bad value

This is due to the function __guest_cmpxchg not being inlined in the -Og build
with gcc 12. Fix this by marking __guest_cmpxchg always_inline.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com
---
I considered also changing "guest_cmpxchg64" just below in the same file to
always_inline, but I decided not to because this function does not take a size
parameter.
---
 xen/arch/arm/include/asm/guest_atomics.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/include/asm/guest_atomics.h b/xen/arch/arm/include/asm/guest_atomics.h
index 9e2e96d4ff72..a1745f8613f6 100644
--- a/xen/arch/arm/include/asm/guest_atomics.h
+++ b/xen/arch/arm/include/asm/guest_atomics.h
@@ -86,11 +86,11 @@ static inline void guest_clear_mask16(struct domain *d, uint16_t mask,
     domain_unpause(d);
 }
 
-static inline unsigned long __guest_cmpxchg(struct domain *d,
-                                            volatile void *ptr,
-                                            unsigned long old,
-                                            unsigned long new,
-                                            unsigned int size)
+static always_inline unsigned long __guest_cmpxchg(struct domain *d,
+                                                   volatile void *ptr,
+                                                   unsigned long old,
+                                                   unsigned long new,
+                                                   unsigned int size)
 {
     unsigned long oldval = old;
 
-- 
2.40.0

[PATCH 2/3] xen/efi: fix unitialized use warning

[Stewart Hildebrand]

When building the hypervisor for arm64 with -Og, we encounter a (false)
uninitialized use warning:

arch/arm/efi/boot.c: In function ‘efi_start’:
arch/arm/efi/boot.c:1468:9: error: ‘argc’ may be used uninitialized [-Werror=maybe-uninitialized]
 1468 |         efi_arch_handle_cmdline(argc ? *argv : NULL, options, name.s);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/arm/efi/boot.c:1263:21: note: ‘argc’ was declared here
 1263 |     unsigned int i, argc;
      |                     ^~~~
cc1: all warnings being treated as errors

Fix this by initializing argc. As a precaution, also initialize argv.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
See previous discussion here
https://lists.xenproject.org/archives/html/xen-devel/2022-10/msg00805.html
---
 xen/common/efi/boot.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index b69c83e354ee..c5850c26af9f 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1344,6 +1344,15 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
         if ( !base_video )
             efi_console_set_mode();
     }
+    else
+    {
+        /*
+         * Some compilers may emit a false "uninitialized use" warning for argc,
+         * so initialize argc/argv here to avoid the warning.
+         */
+        argc = 0;
+        argv = NULL;
+    }
 
     PrintStr(L"Xen " XEN_VERSION_STRING XEN_EXTRAVERSION
 	     " (c/s " XEN_CHANGESET ") EFI loader\r\n");
-- 
2.40.0

[PATCH 3/3] xen/arm: fix unitialized use warning

[Stewart Hildebrand]

When building the hypervisor with -Og, we encounter the following error:

arch/arm/domain_build.c: In function ‘make_cpus_node’:
arch/arm/domain_build.c:2040:12: error: ‘clock_valid’ may be used uninitialized [-Werror=maybe-uninitialized]
 2040 |         if ( clock_valid )
      |            ^
arch/arm/domain_build.c:1947:10: note: ‘clock_valid’ was declared here
 1947 |     bool clock_valid;
      |          ^~~~~~~~~~~
cc1: all warnings being treated as errors

Fix it by initializing the variable.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
See previous discussion here
https://lists.xenproject.org/archives/html/xen-devel/2022-10/msg00741.html
---
 xen/arch/arm/domain_build.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 4f9d4f9d8867..18b350734a8e 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1944,7 +1944,7 @@ static int __init make_cpus_node(const struct domain *d, void *fdt)
     /* Placeholder for cpu@ + a 32-bit hexadecimal number + \0 */
     char buf[13];
     u32 clock_frequency;
-    bool clock_valid;
+    bool clock_valid = false;
     uint64_t mpidr_aff;
 
     dt_dprintk("Create cpus node\n");
-- 
2.40.0

RE: [PATCH 1/3] xen/arm: mark __guest_cmpxchg always_inline

[Henry Wang]

Hi Stewart,

> -----Original Message-----
> Subject: [PATCH 1/3] xen/arm: mark __guest_cmpxchg always_inline
> 
> When building the hypervisor with -Og, we run into a __bad_cmpxchg link
> error:
> 
> aarch64-none-linux-gnu-ld: prelink.o: in function `__int_cmpxchg':
> .../xen/./arch/arm/include/asm/arm64/cmpxchg.h:117: undefined reference
> to `__bad_cmpxchg'
> aarch64-none-linux-gnu-
> ld: .../xen/./arch/arm/include/asm/arm64/cmpxchg.h:117: undefined
> reference to `__bad_cmpxchg'
> aarch64-none-linux-gnu-ld: ./.xen-syms.0: hidden symbol `__bad_cmpxchg'
> isn't defined
> aarch64-none-linux-gnu-ld: final link failed: bad value
> 
> This is due to the function __guest_cmpxchg not being inlined in the -Og build
> with gcc 12. Fix this by marking __guest_cmpxchg always_inline.
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com

Hmmm I think you missed the ">" in the end of your signoff...But anyway the
patch looks good to me so:

Reviewed-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

RE: [PATCH 3/3] xen/arm: fix unitialized use warning

[Henry Wang]

Hi Stewart,

> -----Original Message-----
> Subject: [PATCH 3/3] xen/arm: fix unitialized use warning
> 
> When building the hypervisor with -Og, we encounter the following error:
> 
> arch/arm/domain_build.c: In function ‘make_cpus_node’:
> arch/arm/domain_build.c:2040:12: error: ‘clock_valid’ may be used
> uninitialized [-Werror=maybe-uninitialized]
>  2040 |         if ( clock_valid )
>       |            ^
> arch/arm/domain_build.c:1947:10: note: ‘clock_valid’ was declared here
>  1947 |     bool clock_valid;
>       |          ^~~~~~~~~~~
> cc1: all warnings being treated as errors
> 
> Fix it by initializing the variable.
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>

Reviewed-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

Re: [PATCH 1/3] xen/arm: mark __guest_cmpxchg always_inline

[Julien Grall]

Hi Stewart,

On 14/04/2023 19:57, Stewart Hildebrand wrote:
> When building the hypervisor with -Og, we run into a __bad_cmpxchg link error:
> 
> aarch64-none-linux-gnu-ld: prelink.o: in function `__int_cmpxchg':
> .../xen/./arch/arm/include/asm/arm64/cmpxchg.h:117: undefined reference to `__bad_cmpxchg'
> aarch64-none-linux-gnu-ld: .../xen/./arch/arm/include/asm/arm64/cmpxchg.h:117: undefined reference to `__bad_cmpxchg'
> aarch64-none-linux-gnu-ld: ./.xen-syms.0: hidden symbol `__bad_cmpxchg' isn't defined
> aarch64-none-linux-gnu-ld: final link failed: bad value
> 
> This is due to the function __guest_cmpxchg not being inlined in the -Og build
> with gcc 12. Fix this by marking __guest_cmpxchg always_inline.
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com
> ---
> I considered also changing "guest_cmpxchg64" just below in the same file to
> always_inline, but I decided not to because this function does not take a size
> parameter.

Make sense. I will fixed the signed-off-by line issue reported by Henry 
while committing:

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH 3/3] xen/arm: fix unitialized use warning

[Julien Grall]

Hi Stewart,

On 14/04/2023 19:57, Stewart Hildebrand wrote:
> When building the hypervisor with -Og, we encounter the following error:

Is this with GCC 12 as well?

> arch/arm/domain_build.c: In function ‘make_cpus_node’:
> arch/arm/domain_build.c:2040:12: error: ‘clock_valid’ may be used uninitialized [-Werror=maybe-uninitialized]
>   2040 |         if ( clock_valid )
>        |            ^
> arch/arm/domain_build.c:1947:10: note: ‘clock_valid’ was declared here
>   1947 |     bool clock_valid;
>        |          ^~~~~~~~~~~
> cc1: all warnings being treated as errors
> 
> Fix it by initializing the variable.
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> ---
> See previous discussion here
> https://lists.xenproject.org/archives/html/xen-devel/2022-10/msg00741.html
> ---
>   xen/arch/arm/domain_build.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 4f9d4f9d8867..18b350734a8e 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1944,7 +1944,7 @@ static int __init make_cpus_node(const struct domain *d, void *fdt)
>       /* Placeholder for cpu@ + a 32-bit hexadecimal number + \0 */
>       char buf[13];
>       u32 clock_frequency;
> -    bool clock_valid;
> +    bool clock_valid = false;

NIT: I would add "Keep the compiler happy with -Og"

I am happy to add it while committing if you agree.

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH 1/3] xen/arm: mark __guest_cmpxchg always_inline

[Stewart Hildebrand]

On 4/16/23 08:50, Julien Grall wrote:
> Hi Stewart,
> 
> On 14/04/2023 19:57, Stewart Hildebrand wrote:
>> When building the hypervisor with -Og, we run into a __bad_cmpxchg link error:
>>
>> aarch64-none-linux-gnu-ld: prelink.o: in function `__int_cmpxchg':
>> .../xen/./arch/arm/include/asm/arm64/cmpxchg.h:117: undefined reference to `__bad_cmpxchg'
>> aarch64-none-linux-gnu-ld: .../xen/./arch/arm/include/asm/arm64/cmpxchg.h:117: undefined reference to `__bad_cmpxchg'
>> aarch64-none-linux-gnu-ld: ./.xen-syms.0: hidden symbol `__bad_cmpxchg' isn't defined
>> aarch64-none-linux-gnu-ld: final link failed: bad value
>>
>> This is due to the function __guest_cmpxchg not being inlined in the -Og build
>> with gcc 12. Fix this by marking __guest_cmpxchg always_inline.
>>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com
>> ---
>> I considered also changing "guest_cmpxchg64" just below in the same file to
>> always_inline, but I decided not to because this function does not take a size
>> parameter.
> 
> Make sense. I will fixed the signed-off-by line issue reported by Henry
> while committing:
> 
> Reviewed-by: Julien Grall <jgrall@amazon.com>

Thank you!

Re: [PATCH 3/3] xen/arm: fix unitialized use warning

[Stewart Hildebrand]

On 4/16/23 08:53, Julien Grall wrote:
> Hi Stewart,

Hi Julien,

> On 14/04/2023 19:57, Stewart Hildebrand wrote:
>> When building the hypervisor with -Og, we encounter the following error:
> 
> Is this with GCC 12 as well?

Yes. If my memory serves me correctly this particular one occurs with both GCC 11 and 12.

>> arch/arm/domain_build.c: In function ‘make_cpus_node’:
>> arch/arm/domain_build.c:2040:12: error: ‘clock_valid’ may be used uninitialized [-Werror=maybe-uninitialized]
>>   2040 |         if ( clock_valid )
>>        |            ^
>> arch/arm/domain_build.c:1947:10: note: ‘clock_valid’ was declared here
>>   1947 |     bool clock_valid;
>>        |          ^~~~~~~~~~~
>> cc1: all warnings being treated as errors
>>
>> Fix it by initializing the variable.
>>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>> ---
>> See previous discussion here
>> https://lists.xenproject.org/archives/html/xen-devel/2022-10/msg00741.html
>> ---
>>   xen/arch/arm/domain_build.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 4f9d4f9d8867..18b350734a8e 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -1944,7 +1944,7 @@ static int __init make_cpus_node(const struct domain *d, void *fdt)
>>       /* Placeholder for cpu@ + a 32-bit hexadecimal number + \0 */
>>       char buf[13];
>>       u32 clock_frequency;
>> -    bool clock_valid;
>> +    bool clock_valid = false;
> 
> NIT: I would add "Keep the compiler happy with -Og"
> 
> I am happy to add it while committing if you agree.

Yes, please do. Thanks.

> Reviewed-by: Julien Grall <jgrall@amazon.com>
> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH 3/3] xen/arm: fix unitialized use warning

[Stewart Hildebrand]

On 4/16/23 22:03, Stewart Hildebrand wrote:
> On 4/16/23 08:53, Julien Grall wrote:
>> Hi Stewart,
> 
> Hi Julien,
> 
>> On 14/04/2023 19:57, Stewart Hildebrand wrote:
>>> When building the hypervisor with -Og, we encounter the following error:
>>
>> Is this with GCC 12 as well?
> 
> Yes. If my memory serves me correctly this particular one occurs with both GCC 11 and 12.
> 
>>> arch/arm/domain_build.c: In function ‘make_cpus_node’:
>>> arch/arm/domain_build.c:2040:12: error: ‘clock_valid’ may be used uninitialized [-Werror=maybe-uninitialized]
>>>   2040 |         if ( clock_valid )
>>>        |            ^
>>> arch/arm/domain_build.c:1947:10: note: ‘clock_valid’ was declared here
>>>   1947 |     bool clock_valid;
>>>        |          ^~~~~~~~~~~
>>> cc1: all warnings being treated as errors
>>>
>>> Fix it by initializing the variable.
>>>
>>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>>> ---
>>> See previous discussion here
>>> https://lists.xenproject.org/archives/html/xen-devel/2022-10/msg00741.html
>>> ---
>>>   xen/arch/arm/domain_build.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index 4f9d4f9d8867..18b350734a8e 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -1944,7 +1944,7 @@ static int __init make_cpus_node(const struct domain *d, void *fdt)
>>>       /* Placeholder for cpu@ + a 32-bit hexadecimal number + \0 */
>>>       char buf[13];
>>>       u32 clock_frequency;
>>> -    bool clock_valid;
>>> +    bool clock_valid = false;
>>
>> NIT: I would add "Keep the compiler happy with -Og"
>>
>> I am happy to add it while committing if you agree.
> 
> Yes, please do. Thanks.

One more thing, there is a typo in the subject, if you are willing to correct it while committing. s/unitialized/uninitialized/

>> Reviewed-by: Julien Grall <jgrall@amazon.com>
>>
>> Cheers,
>>
>> --
>> Julien Grall
> 

Re: [PATCH 2/3] xen/efi: fix unitialized use warning

[Jan Beulich]

On 14.04.2023 20:57, Stewart Hildebrand wrote:
> When building the hypervisor for arm64 with -Og, we encounter a (false)
> uninitialized use warning:
> 
> arch/arm/efi/boot.c: In function ‘efi_start’:
> arch/arm/efi/boot.c:1468:9: error: ‘argc’ may be used uninitialized [-Werror=maybe-uninitialized]
>  1468 |         efi_arch_handle_cmdline(argc ? *argv : NULL, options, name.s);
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> arch/arm/efi/boot.c:1263:21: note: ‘argc’ was declared here
>  1263 |     unsigned int i, argc;
>       |                     ^~~~
> cc1: all warnings being treated as errors
> 
> Fix this by initializing argc. As a precaution, also initialize argv.

I'm not happy about this kind of change, and I also wonder whether we
wouldn't better use initializers for both variables if we already have
to work around compiler shortcomings like this one. Nevertheless I can
see the need, so ...

> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH 3/3] xen/arm: fix unitialized use warning

[Julien Grall]

Hi Stewart,

On 17/04/2023 03:08, Stewart Hildebrand wrote:
> On 4/16/23 22:03, Stewart Hildebrand wrote:
>> On 4/16/23 08:53, Julien Grall wrote:
>>> Hi Stewart,
>>
>> Hi Julien,
>>
>>> On 14/04/2023 19:57, Stewart Hildebrand wrote:
>>>> When building the hypervisor with -Og, we encounter the following error:
>>>
>>> Is this with GCC 12 as well?
>>
>> Yes. If my memory serves me correctly this particular one occurs with both GCC 11 and 12.

Thanks. I will update the commit message to mention it.

>>
>>>> arch/arm/domain_build.c: In function ‘make_cpus_node’:
>>>> arch/arm/domain_build.c:2040:12: error: ‘clock_valid’ may be used uninitialized [-Werror=maybe-uninitialized]
>>>>    2040 |         if ( clock_valid )
>>>>         |            ^
>>>> arch/arm/domain_build.c:1947:10: note: ‘clock_valid’ was declared here
>>>>    1947 |     bool clock_valid;
>>>>         |          ^~~~~~~~~~~
>>>> cc1: all warnings being treated as errors
>>>>
>>>> Fix it by initializing the variable.
>>>>
>>>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>>>> ---
>>>> See previous discussion here
>>>> https://lists.xenproject.org/archives/html/xen-devel/2022-10/msg00741.html
>>>> ---
>>>>    xen/arch/arm/domain_build.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>>> index 4f9d4f9d8867..18b350734a8e 100644
>>>> --- a/xen/arch/arm/domain_build.c
>>>> +++ b/xen/arch/arm/domain_build.c
>>>> @@ -1944,7 +1944,7 @@ static int __init make_cpus_node(const struct domain *d, void *fdt)
>>>>        /* Placeholder for cpu@ + a 32-bit hexadecimal number + \0 */
>>>>        char buf[13];
>>>>        u32 clock_frequency;
>>>> -    bool clock_valid;
>>>> +    bool clock_valid = false;
>>>
>>> NIT: I would add "Keep the compiler happy with -Og"
>>>
>>> I am happy to add it while committing if you agree.
>>
>> Yes, please do. Thanks.
> 
> One more thing, there is a typo in the subject, if you are willing to correct it while committing. s/unitialized/uninitialized/

Sure. I will do that.

Cheers,

-- 
Julien Grall

Re: [PATCH 0/3] xen/arm: fix build errors with -Og

[Julien Grall]

Hi Stewart,

On 14/04/2023 19:57, Stewart Hildebrand wrote:
> This is a collection of fixes needed to build the hypervisor with -Og for arm64.
> 
> I build-tested this with the following command:
> 
> make -j $(nproc) \
>      EXTRA_CFLAGS_XEN_CORE="-Og" \
>      XEN_TARGET_ARCH=arm64 \
>      CROSS_COMPILE=aarch64-none-linux-gnu- \
>      dist-xen
> 
> 
> Stewart Hildebrand (3):
>    xen/arm: mark __guest_cmpxchg always_inline
>    xen/efi: fix unitialized use warning
>    xen/arm: fix unitialized use warning

I have committed patch #1 and #3. Jan already committed patch #2.

Thanks,

-- 
Julien Grall