Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
 xen/arch/arm/domain_build.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index b975d0b309..9fa283d694 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1051,8 +1051,8 @@ static void __init assign_static_memory_11(struct domain *d,
  * The current heuristic assumes that a device is a host bridge
  * if the type is "pci" and then parent type is not "pci".
  */
-static int handle_linux_pci_domain(struct kernel_info *kinfo,
-                                   const struct dt_device_node *node)
+static int __init handle_linux_pci_domain(struct kernel_info *kinfo,
+                                          const struct dt_device_node *node)
 {
     uint16_t segment;
     int res;
-- 
2.38.0

On 14.10.2022 04:53, Stewart Hildebrand wrote:
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>

I guess a non-empty description and a Fixes: tag would be nice.

Jan

Hi,

On 14/10/2022 08:16, Jan Beulich wrote:
> On 14.10.2022 04:53, Stewart Hildebrand wrote:
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> 
> I guess a non-empty description and a Fixes: tag would be nice.

+1. I am actually quite interested to understand how this was spotted.

The build system should check that any function/data in domain_build.c 
are part of the __init section. So I guess the compiler you are using 
doesn't inline the function?

If so, I am actually surprised you are the first one spotted this... We 
are building on various distribution without any issues (?). I would be 
interested to know the compiler version and maybe we could add it in the 
CI.

Cheers,

-- 
Julien Grall

On 10/14/22 04:22, Julien Grall wrote:
> Hi,
>
> On 14/10/2022 08:16, Jan Beulich wrote:
>> On 14.10.2022 04:53, Stewart Hildebrand wrote:
>>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>>
>> I guess a non-empty description and a Fixes: tag would be nice.

Okay, I will send a v2 with the following description:
All functions in domain_build.c should be marked __init. This was 
spotted when building the hypervisor with -Og.

Fixes: 1050a7b91c xen/arm: add pci-domain for disabled devices
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>

> +1. I am actually quite interested to understand how this was spotted.
>
> The build system should check that any function/data in domain_build.c
> are part of the __init section. So I guess the compiler you are using
> doesn't inline the function?
>
> If so, I am actually surprised you are the first one spotted this... We
> are building on various distribution without any issues (?). I would be
> interested to know the compiler version and maybe we could add it in the
> CI.

I added -Og to the make command line so it takes precedence over the 
default -O1/-O2:

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

Indeed, I did observe the build error:
Error: size of arch/arm/domain_build.o:.text is 0x00000008

I used this rune to reveal the culprit:

$ aarch64-none-linux-gnu-objdump -d xen/arch/arm/domain_build.o | head

xen/arch/arm/domain_build.o:     file format elf64-littleaarch64


Disassembly of section .text:

0000000000000000 <handle_linux_pci_domain>:
     0:   52800000        mov     w0, #0x0                        // #0
     4:   d65f03c0        ret

I am using this toolchain: 
https://developer.arm.com/-/media/Files/downloads/gnu/11.3.rel1/binrel/arm-gnu-toolchain-11.3.rel1-x86_64-aarch64-none-linux-gnu.tar.xz

Further, there were two more build errors observed when building with -Og:
arch/arm/domain_build.c: In function ‘make_cpus_node’:
arch/arm/domain_build.c:2013:12: error: ‘clock_valid’ may be used 
uninitialized in this function [-Werror=maybe-uninitialized]
  2013 |         if ( clock_valid )
       |            ^

arch/arm/efi/boot.c: In function ‘efi_start’:
arch/arm/efi/boot.c:1464:9: error: ‘argc’ may be used uninitialized in 
this function [-Werror=maybe-uninitialized]
  1464 |         efi_arch_handle_cmdline(argc ? *argv : NULL, options, 
name.s);
       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I assume these uninitialized use errors can simply be fixed by 
initializing the respective variables to false/0, but a second opinion 
would certainly be helpful.

Hi Stewart,

On 14/10/2022 20:23, Stewart Hildebrand wrote:
> On 10/14/22 04:22, Julien Grall wrote:
>> Hi,
>>
>> On 14/10/2022 08:16, Jan Beulich wrote:
>>> On 14.10.2022 04:53, Stewart Hildebrand wrote:
>>>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>>>
>>> I guess a non-empty description and a Fixes: tag would be nice.
> 
> Okay, I will send a v2 with the following description:
> All functions in domain_build.c should be marked __init. This was 
> spotted when building the hypervisor with -Og.
> 
> Fixes: 1050a7b91c xen/arm: add pci-domain for disabled devices
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> 
>> +1. I am actually quite interested to understand how this was spotted.
>>
>> The build system should check that any function/data in domain_build.c
>> are part of the __init section. So I guess the compiler you are using
>> doesn't inline the function?
>>
>> If so, I am actually surprised you are the first one spotted this... We
>> are building on various distribution without any issues (?). I would be
>> interested to know the compiler version and maybe we could add it in the
>> CI.
> 
> I added -Og to the make command line so it takes precedence over the 
> default -O1/-O2:
> 
> $ make EXTRA_CFLAGS_XEN_CORE="-Og" XEN_TARGET_ARCH=arm64 
> CROSS_COMPILE=aarch64-none-linux-gnu- dist-xen -j $(nproc)
> 
> Indeed, I did observe the build error:
> Error: size of arch/arm/domain_build.o:.text is 0x00000008
> 
> I used this rune to reveal the culprit:
> 
> $ aarch64-none-linux-gnu-objdump -d xen/arch/arm/domain_build.o | head
> 
> xen/arch/arm/domain_build.o:     file format elf64-littleaarch64
> 
> 
> Disassembly of section .text:
> 
> 0000000000000000 <handle_linux_pci_domain>:
>      0:   52800000        mov     w0, #0x0                        // #0
>      4:   d65f03c0        ret
> 
> I am using this toolchain: 
> https://developer.arm.com/-/media/Files/downloads/gnu/11.3.rel1/binrel/arm-gnu-toolchain-11.3.rel1-x86_64-aarch64-none-linux-gnu.tar.xz

Thanks for the details. I guess the '-Og' is the culprint.

> 
> Further, there were two more build errors observed when building with -Og:
> arch/arm/domain_build.c: In function ‘make_cpus_node’:
> arch/arm/domain_build.c:2013:12: error: ‘clock_valid’ may be used 
> uninitialized in this function [-Werror=maybe-uninitialized]
>   2013 |         if ( clock_valid )
>        |            ^

I think this is a false positive because 'clock_valid' is set at the 
same time as 'compatible'. The latter is check that is not NULL just 
after it is set.

In general, I tend to prefer if variable are not initialized (unless 
strictly necessary) because we can take advantage of the compiler to 
spot any issue.

In this case, it should not be a big problem because the default value 
(false) would be sensible here.

> 
> arch/arm/efi/boot.c: In function ‘efi_start’:
> arch/arm/efi/boot.c:1464:9: error: ‘argc’ may be used uninitialized in 
> this function [-Werror=maybe-uninitialized]
>   1464 |         efi_arch_handle_cmdline(argc ? *argv : NULL, options, 
> name.s);
>        | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I am a bit puzzled why it warn on this line but not few lines above 
where it is already used.

This function is a bit difficult to read. AFAIU, the code look like:

if ( use_cfg_file )
{
    argc = ...
}

/* do something common */

if ( use_cfg_file )
   efi_arch_handle_cmd(argc, ...);

The GCC with -Og is probably not capable to detect that argc will always 
be used when 'use_cfg_file'.

The "do something common" is two lines. So I am tempted to suggest to 
just duplicate those two lines. This could also allow us to move all the 
code in the ifs (nearly 100 lines over the two ifs!) in a separate function.

But I think Jan (the maintainer of the code) may not be happy with 
that). So short of a second better suggestion, initializing 'argc' to 0 
(?) and a comment explaining this is to silence the compiler may be the 
way to go.

Cheers,

-- 
Julien Grall

On 14.10.2022 22:15, Julien Grall wrote:
> On 14/10/2022 20:23, Stewart Hildebrand wrote:
>> arch/arm/efi/boot.c: In function ‘efi_start’:
>> arch/arm/efi/boot.c:1464:9: error: ‘argc’ may be used uninitialized in 
>> this function [-Werror=maybe-uninitialized]
>>   1464 |         efi_arch_handle_cmdline(argc ? *argv : NULL, options, 
>> name.s);
>>        | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> I am a bit puzzled why it warn on this line but not few lines above 
> where it is already used.

Same here. Plus it ought to warn for argv then too, I think.

> This function is a bit difficult to read. AFAIU, the code look like:
> 
> if ( use_cfg_file )
> {
>     argc = ...
> }
> 
> /* do something common */
> 
> if ( use_cfg_file )
>    efi_arch_handle_cmd(argc, ...);
> 
> The GCC with -Og is probably not capable to detect that argc will always 
> be used when 'use_cfg_file'.
> 
> The "do something common" is two lines. So I am tempted to suggest to 
> just duplicate those two lines. This could also allow us to move all the 
> code in the ifs (nearly 100 lines over the two ifs!) in a separate function.
> 
> But I think Jan (the maintainer of the code) may not be happy with 
> that).

Indeed. Even if it's only two statements now, my view is that we ought to
avoid such code duplication.

Further I wonder whether the call to efi_check_dt_boot() shouldn't
actually live in an "else" to the 2nd if(). It would be at least
questionable if parts of the modules were described by the .cfg file and
other parts by DT (which in turn makes assumptions about the relative
placement of those modules wrt xen.efi on the EFI partition, when I'd
expect it to be self-contained).

> So short of a second better suggestion, initializing 'argc' to 0 
> (?) and a comment explaining this is to silence the compiler may be the 
> way to go.

I'd prefer if we avoided that, not the least because this could then trip
(good) static checkers to report written-but-never-used instances. What I
might accept (albeit that doesn't address said concern) is an "else" to
the first if() setting argc and argv (accompanied by a suitable comment,
down the road perhaps including a SAF-<nnn>-false-positive marker).

Jan

All functions in domain_build.c should be marked __init. This was
spotted when building the hypervisor with -Og.

Fixes: 1050a7b91c xen/arm: add pci-domain for disabled devices
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>

---
v1 -> v2:
  Add Fixes: tag
  Add patch description
---
 xen/arch/arm/domain_build.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 61cda8e843..fc2961895b 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1051,8 +1051,8 @@ static void __init assign_static_memory_11(struct domain *d,
  * The current heuristic assumes that a device is a host bridge
  * if the type is "pci" and then parent type is not "pci".
  */
-static int handle_linux_pci_domain(struct kernel_info *kinfo,
-                                   const struct dt_device_node *node)
+static int __init handle_linux_pci_domain(struct kernel_info *kinfo,
+                                          const struct dt_device_node *node)
 {
     uint16_t segment;
     int res;
-- 
2.38.0

Hi Stewart,

I nearly missed this one because it was threaded under v1. In the 
future, would you be able to send new version in a separate thread? This 
makes easier to track it.

On 14/10/2022 21:09, Stewart Hildebrand wrote:
> All functions in domain_build.c should be marked __init. This was
> spotted when building the hypervisor with -Og.
> 
> Fixes: 1050a7b91c xen/arm: add pci-domain for disabled devices
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>

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

Henry, this patch is fixing a potential build failure on some compiler 
(at the moment we are relying on the compiler to inline 
handle_linux_pci_domain). AFAIU, the problem was introduced in Xen 4.17. 
Would you be happy if we include it in the release?

Cheers,

> 
> ---
> v1 -> v2:
>    Add Fixes: tag
>    Add patch description
> ---
>   xen/arch/arm/domain_build.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 61cda8e843..fc2961895b 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1051,8 +1051,8 @@ static void __init assign_static_memory_11(struct domain *d,
>    * The current heuristic assumes that a device is a host bridge
>    * if the type is "pci" and then parent type is not "pci".
>    */
> -static int handle_linux_pci_domain(struct kernel_info *kinfo,
> -                                   const struct dt_device_node *node)
> +static int __init handle_linux_pci_domain(struct kernel_info *kinfo,
> +                                          const struct dt_device_node *node)
>   {
>       uint16_t segment;
>       int res;

-- 
Julien Grall

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Subject: Re: [PATCH v2] xen/arm: mark handle_linux_pci_domain() __init
> 
> Hi Stewart,
> 
> I nearly missed this one because it was threaded under v1. In the
> future, would you be able to send new version in a separate thread? This
> makes easier to track it.
> 
> On 14/10/2022 21:09, Stewart Hildebrand wrote:
> > All functions in domain_build.c should be marked __init. This was
> > spotted when building the hypervisor with -Og.
> >
> > Fixes: 1050a7b91c xen/arm: add pci-domain for disabled devices
> > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> 
> Acked-by: Julien Grall <jgrall@amazon.com>
> 
> Henry, this patch is fixing a potential build failure on some compiler
> (at the moment we are relying on the compiler to inline
> handle_linux_pci_domain). AFAIU, the problem was introduced in Xen 4.17.
> Would you be happy if we include it in the release?

Of course. Thanks for the ping :)

Release-acked-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

> 
> Cheers,
> 
> >
> > ---
> > v1 -> v2:
> >    Add Fixes: tag
> >    Add patch description
> > ---
> >   xen/arch/arm/domain_build.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 61cda8e843..fc2961895b 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1051,8 +1051,8 @@ static void __init assign_static_memory_11(struct
> domain *d,
> >    * The current heuristic assumes that a device is a host bridge
> >    * if the type is "pci" and then parent type is not "pci".
> >    */
> > -static int handle_linux_pci_domain(struct kernel_info *kinfo,
> > -                                   const struct dt_device_node *node)
> > +static int __init handle_linux_pci_domain(struct kernel_info *kinfo,
> > +                                          const struct dt_device_node *node)
> >   {
> >       uint16_t segment;
> >       int res;
> 
> --
> Julien Grall

On 10/20/22 14:19, Julien Grall wrote:
> Hi Stewart,

Hi Julien,

> I nearly missed this one because it was threaded under v1. In the
> future, would you be able to send new version in a separate thread? This
> makes easier to track it.

I will keep this in mind for next time.

> On 14/10/2022 21:09, Stewart Hildebrand wrote:
>> All functions in domain_build.c should be marked __init. This was
>> spotted when building the hypervisor with -Og.
>>
>> Fixes: 1050a7b91c xen/arm: add pci-domain for disabled devices

I missed parenthesis and quotes around the referenced commit. To keep it in the same format as other Fixes: tags, can you please add during commit (pending release ack)?

>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> 
> Acked-by: Julien Grall <jgrall@amazon.com>
> 
> Henry, this patch is fixing a potential build failure on some compiler
> (at the moment we are relying on the compiler to inline
> handle_linux_pci_domain). AFAIU, the problem was introduced in Xen 4.17.
> Would you be happy if we include it in the release?
> 
> Cheers,
> 
>>
>> ---
>> v1 -> v2:
>>    Add Fixes: tag
>>    Add patch description
>> ---
>>   xen/arch/arm/domain_build.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 61cda8e843..fc2961895b 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -1051,8 +1051,8 @@ static void __init assign_static_memory_11(struct domain *d,
>>    * The current heuristic assumes that a device is a host bridge
>>    * if the type is "pci" and then parent type is not "pci".
>>    */
>> -static int handle_linux_pci_domain(struct kernel_info *kinfo,
>> -                                   const struct dt_device_node *node)
>> +static int __init handle_linux_pci_domain(struct kernel_info *kinfo,
>> +                                          const struct dt_device_node *node)
>>   {
>>       uint16_t segment;
>>       int res;
> 
> -- 
> Julien Grall

On 20/10/2022 19:53, Stewart Hildebrand wrote:
> On 10/20/22 14:19, Julien Grall wrote:
>> Hi Stewart,
> 
> Hi Julien,
> 
>> I nearly missed this one because it was threaded under v1. In the
>> future, would you be able to send new version in a separate thread? This
>> makes easier to track it.
> 
> I will keep this in mind for next time.
> 
>> On 14/10/2022 21:09, Stewart Hildebrand wrote:
>>> All functions in domain_build.c should be marked __init. This was
>>> spotted when building the hypervisor with -Og.
>>>
>>> Fixes: 1050a7b91c xen/arm: add pci-domain for disabled devices
> 
> I missed parenthesis and quotes around the referenced commit. To keep it in the same format as other Fixes: tags, can you please add during commit (pending release ack)?

Will do.

Cheers,

-- 
Julien Grall

On 20/10/2022 19:56, Julien Grall wrote:
> On 20/10/2022 19:53, Stewart Hildebrand wrote:
>> On 10/20/22 14:19, Julien Grall wrote:
>>> Hi Stewart,
>>
>> Hi Julien,
>>
>>> I nearly missed this one because it was threaded under v1. In the
>>> future, would you be able to send new version in a separate thread? This
>>> makes easier to track it.
>>
>> I will keep this in mind for next time.
>>
>>> On 14/10/2022 21:09, Stewart Hildebrand wrote:
>>>> All functions in domain_build.c should be marked __init. This was
>>>> spotted when building the hypervisor with -Og.
>>>>
>>>> Fixes: 1050a7b91c xen/arm: add pci-domain for disabled devices
>>
>> I missed parenthesis and quotes around the referenced commit. To keep 
>> it in the same format as other Fixes: tags, can you please add during 
>> commit (pending release ack)?
> 
> Will do.

The commit ID was also too short. Xen (and Linux) moved to 12 characters 
because 10 is not enough anymore to uniquely distinguish a commit.

You can ask git to change its default value by adding the following 
lines in either the global config or per-repo one:

[core]
         abbrev = 12

It is now committed.

Cheers,

-- 
Julien Grall