Re: [PATCH v8 7/9] xen/arm: create shared memory nodes in guest device tree

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 9 Sep 2022, at 10:40, Bertrand Marquis <Bertrand.Marquis@xxxxxxx> wrote:
> 
> Hi Julien,
> 
>> On 9 Sep 2022, at 10:27, Julien Grall <julien@xxxxxxx> wrote:
>> 
>> Hi,
>> 
>> On 09/09/2022 08:45, Bertrand Marquis wrote:
>>>> 
>>>> It should be:
>>>> 
>>>> /*
>>>> * TODO:
>>>> *
>>>> 
>>>> I think this is good to go. The two minor style issues could be fixed on
>>>> commit. I haven't committed to give Julien & Bertrand another chance to
>>>> have a look.
>>> I think that it is ok to fix those on commit and I am ok with the rest so:
>>> Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>> 
>> This series doesn't build without !CONFIG_STATIC_SHM:
>> 
>> UPD     include/xen/compile.h
>> Xen 4.17-unstable
>> make[1]: Nothing to be done for `include'.
>> make[1]: `arch/arm/include/asm/asm-offsets.h' is up to date.
>> CC      common/version.o
>> LD      common/built_in.o
>> CC      arch/arm/domain_build.o
>> arch/arm/domain_build.c: In function ‘make_shm_memory_node’:
>> arch/arm/domain_build.c:1445:1: error: no return statement in function 
>> returning non-void [-Werror=return-type]
>> }
>> ^
>> cc1: all warnings being treated as errors
>> make[2]: *** [arch/arm/domain_build.o] Error 1
>> make[1]: *** [arch/arm] Error 2
>> make: *** [xen] Error 2
>> 
>> This is because...
>> 
>>>>> +         * - xen,offset: (borrower VMs only)
>>>>> +         *   64 bit integer offset within the owner virtual machine's 
>>>>> shared
>>>>> +         *   memory region used for the mapping in the borrower VM
>>>>> +         */
>>>>> +        res = fdt_property_u64(fdt, "xen,offset", 0);
>>>>> +        if ( res )
>>>>> +            return res;
>>>>> +
>>>>> +        res = fdt_end_node(fdt);
>>>>> +        if ( res )
>>>>> +            return res;
>>>>> +    }
>>>>> +
>>>>> +    return res;
>>>>> +}
>>>>> +#else
>>>>> +static int __init make_shm_memory_node(const struct domain *d,
>>>>> +                                       void *fdt,
>>>>> +                                       int addrcells, int sizecells,
>>>>> +                                       const struct meminfo *mem)
>>>>> +{
>>>>> +    ASSERT_UNREACHABLE();
>> 
>> ... there is a missing 'return -ENOTSUPP' here. While this is simple enough 
>> to fix, this indicates to me that this version was not tested with 
>> !CONFIG_STATIC_SHM.
>> 
>> As this is the default option, I will not commit until I get confirmation 
>> that some smoke was done.
> 
> This is a case our internal CI should have gone through.
> Let me check and come back to you.
> 

Hi Julien,

Thanks for catching it, in this case I can confirm that the problem was that we 
are building with CONFIG_DEBUG enabled, I don’t know why GCC doesn’t complain 
when
you have __builtin_unreachable() in that function without any return value, it 
doesn’t even throw a warning. Could it be considered a bug in GCC?

Building Xen without CONFIG_DEBUG instead shows up the error you found.

In this case this change will fix the problem, do you agree on it?

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 8c77c764bcf2..c5d66f18bd49 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1439,6 +1439,8 @@ static int __init make_shm_memory_node(const struct 
domain *d,
                                        const struct meminfo *mem)
 {
     ASSERT_UNREACHABLE();
+
+    return -EOPNOTSUPP;
 }
 #endif

Is it something that can be addressed on commit?

Cheers,
Luca


> Regards
> Bertrand
> 
>> 
>> Cheers,
>> 
>> -- 
>> Julien Grall