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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

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.

Regards
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall