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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 9 Sep 2022, 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?
> 
> 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?

The suggestion makes sense. I raw a few tests myself and also a pipeline
through gitlab-ci to be sure. Everything passed, so (also mindful of the
deadlines) I committed the series with this change.