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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Luca,

On 09/09/2022 14:35, Luca Fancellu wrote:
> 
>> 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?

This is not a bug. The documentation states what is the purpose of it even in 
case of functions returning type.
https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html

To sump up __builtin_unreachable generates code itself to return.

> 
> 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
> 

~Michal