Re: [Minios-devel] [UNIKRAFT PATCHv9 1/7] lib/fdt: Introduce fdt_getprop_u32_by_offset helper

["Justin He (Arm Technology China)" <Justin.He@xxxxxxx>]

Hi Sharan

> -----Original Message-----
> From: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
> Sent: 2019年7月11日 0:34
> To: Justin He (Arm Technology China) <Justin.He@xxxxxxx>; minios-
> devel@xxxxxxxxxxxxxxxxxxxx; Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> Cc: Florian Schmidt <florian.schmidt@xxxxxxxxx>; Felipe Huici
> <felipe.huici@xxxxxxxxx>; Julien Grall <Julien.Grall@xxxxxxx>;
> yuri.volchkov@xxxxxxxxx; Kaly Xin (Arm Technology China)
> <Kaly.Xin@xxxxxxx>; Wei Chen (Arm Technology China)
> <Wei.Chen@xxxxxxx>; Jianyong Wu (Arm Technology China)
> <Jianyong.Wu@xxxxxxx>
> Subject: Re: [UNIKRAFT PATCHv9 1/7] lib/fdt: Introduce
> fdt_getprop_u32_by_offset helper
>
> Hello,
>
> Please find the comment inline.
>
>
> Thanks & Regards
> Sharan
>
> On 7/10/19 6:20 AM, Jia He wrote:
> > This helper will be used very frequently to u32 from properties.
> > So we provide this helper here to avoid using fdt_get_property_namelen
> > and fdt32_to_cpu everywhere.
> >
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > Signed-off-by: Jianyong Wu <jianyong.wu@xxxxxxx>
> > Signed-off-by: Jia He <justin.he@xxxxxxx>
> > ---
> >   lib/fdt/Makefile.uk      |  1 +
> >   lib/fdt/exportsyms.uk    |  1 +
> >   lib/fdt/include/libfdt.h | 21 ++++++++++++++++
> >   lib/fdt/uk_fdt.c         | 54 ++++++++++++++++++++++++++++++++++++++++
> >   4 files changed, 77 insertions(+)
> >   create mode 100644 lib/fdt/uk_fdt.c
> >
> > diff --git a/lib/fdt/Makefile.uk b/lib/fdt/Makefile.uk
> > index 03251f6..d4d85a0 100644
> > --- a/lib/fdt/Makefile.uk
> > +++ b/lib/fdt/Makefile.uk
> > @@ -14,3 +14,4 @@ LIBFDT_SRCS-y += $(LIBFDT_BASE)/fdt_rw.c
> >   LIBFDT_SRCS-y += $(LIBFDT_BASE)/fdt_strerror.c
> >   LIBFDT_SRCS-y += $(LIBFDT_BASE)/fdt_sw.c
> >   LIBFDT_SRCS-y += $(LIBFDT_BASE)/fdt_wip.c
> > +LIBFDT_SRCS-y += $(LIBFDT_BASE)/uk_fdt.c
> > diff --git a/lib/fdt/exportsyms.uk b/lib/fdt/exportsyms.uk
> > index 2fe4c32..d64d9dc 100644
> > --- a/lib/fdt/exportsyms.uk
> > +++ b/lib/fdt/exportsyms.uk
> > @@ -60,3 +60,4 @@ fdt_size_cells
> >   fdt_stringlist_contains
> >   fdt_resize
> >   fdt_overlay_apply
> > +fdt_getprop_u32_by_offset
> > diff --git a/lib/fdt/include/libfdt.h b/lib/fdt/include/libfdt.h
> > index 05dedbd..e75f0bb 100644
> > --- a/lib/fdt/include/libfdt.h
> > +++ b/lib/fdt/include/libfdt.h
> > @@ -1863,6 +1863,27 @@ int fdt_del_node(void *fdt, int nodeoffset);
> >    */
> >   int fdt_overlay_apply(void *fdt, void *fdto);
> >
> > +/**
> > + * fdt_getprop_u32_by_offset - retrieve u32 of a given property
> > + * @fdt: pointer to the device tree blob
> > + * @nodeoffset: offset of the node whose property to find
> > + * @name: name of the property to find
> > + * @out: pointer to u32 variable (will be overwritten) or NULL
> > + *
> > + * fdt_getprop_u32_by_offset() retrieves u32 to the value of the property
> > + * named 'name' of the node at offset nodeoffset (this will be a
> > + * pointer to within the device blob itself, not a copy of the value).
> > + * If out is non-NULL, the u32 of the property value is returned.
> > + *
> > + * returns:
> > + *0, on success
> > + *out contains the u32 of a given property at nodeoffset.
> > + *-FDT_ERR_NOTFOUND, node does not have named property
> > + *-FDT_ERR_BADNCELLS,
> > + */
> > +int fdt_getprop_u32_by_offset(const void *fdt, int nodeoffset,
> > +const char *name, uint32_t *out);
> > +
> >
> /***************************************************************
> *******/
> >   /* Debugging / informational functions                                */
> >
> /***************************************************************
> *******/
> > diff --git a/lib/fdt/uk_fdt.c b/lib/fdt/uk_fdt.c
> > new file mode 100644
> > index 0000000..b93d208
> > --- /dev/null
> > +++ b/lib/fdt/uk_fdt.c
> > @@ -0,0 +1,54 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause */
> > +/*
> > + * Authors: Wei Chen <Wei.Chen@xxxxxxx>
> > + *          Jianyong Wu <Jianyong.Wu@xxxxxxx>
> > + *
> > + * Copyright (c) 2018, Arm Ltd. All rights reserved.
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions
> > + * are met:
> > + *
> > + * 1. Redistributions of source code must retain the above copyright
> > + *    notice, this list of conditions and the following disclaimer.
> > + * 2. Redistributions in binary form must reproduce the above copyright
> > + *    notice, this list of conditions and the following disclaimer in the
> > + *    documentation and/or other materials provided with the distribution.
> > + * 3. Neither the name of the copyright holder nor the names of its
> > + *    contributors may be used to endorse or promote products derived
> from
> > + *    this software without specific prior written permission.
> > + *
> > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> CONTRIBUTORS "AS IS"
> > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> LIMITED TO, THE
> > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
> PARTICULAR PURPOSE
> > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR
> CONTRIBUTORS BE
> > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY,
> OR
> > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> PROCUREMENT OF
> > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> BUSINESS
> > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
> WHETHER IN
> > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
> OTHERWISE)
> > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
> ADVISED OF THE
> > + * POSSIBILITY OF SUCH DAMAGE.
> > + *
> > + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> > + */
> > +#include <libfdt_env.h>
> > +#include <fdt.h>
> > +#include <libfdt.h>
> > +
> > +int fdt_getprop_u32_by_offset(const void *fdt, int offset,
> > +const char *name, uint32_t *out)
> > +{
> > +const struct fdt_property *prop;
> > +int prop_len;
> > +
> > +prop = fdt_get_property_namelen(fdt, offset, name, strlen(name),
> > +&prop_len);
> > +if (!prop)
> > +return prop_len;
> > +
> > +if (out)
> > +*out = fdt32_to_cpu(*(fdt32_t *)prop->data);
> uk_fdt.c:51:2: warning: dereferencing type-punned pointer will break
> strict-aliasing rules [-Wstrict-aliasing]
>
> We get a compiler warning with aarch64-linux-gnu-gcc (Linaro GCC
> 6.3-2017.05) 6.3.1 20170404.

Thanks for the pointing, in my arm server + Ubuntu 18.04 gcc Linaro 7.3.0-27,
there is no such warnings.

Anyway, I will update the patch as you suggested although I thought the strict
aliasing check is not necessary here 😉


--
Cheers,
Justin (Jia He)


>
> Since the prop->data is a character array, we cannot reference it
> directly as fdt32_t pointer. One possible solution might be perform a
> memcpy on the data like the below snippet:
>
>           fdt32_t result;
>
>
>
>           prop = fdt_get_property_namelen(fdt, offset, name,
> strlen(name), &prop_len);
>
>
>           if (!prop)
>
>
>
>                    return prop_len;
>           memcpy(&result, prop->data, sizeof(result));
>           if (out)
>                  *out = fdt32_to_cpu(result);
>
> > +
> > +return 0;
> > +}
> >
IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel