Re: [Minios-devel] [UNIKRAFT PATCHv7 1/8] plat/common: Introduce fdt_getprop_u32_by_offset helper

["Jianyong Wu (Arm Technology China)" <Jianyong.Wu@xxxxxxx>]

Hi sharan,

> -----Original Message-----
> From: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
> Sent: Monday, July 1, 2019 5:41 PM
> 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>; Jianyong Wu (Arm Technology China)
> <Jianyong.Wu@xxxxxxx>; Wei Chen (Arm Technology China)
> <Wei.Chen@xxxxxxx>
> Subject: Re: [UNIKRAFT PATCHv7 1/8] plat/common: Introduce
> fdt_getprop_u32_by_offset helper
>
> Hello Jianyong Wu,
>
> Please find the review comment inline
>
> Thanks & Regards
> Sharan Santhanam
>
>
> On 6/27/19 10:55 AM, Jia He wrote:
> > From: Jianyong Wu <jianyong.wu@xxxxxxx>
> >
> > This helper will be used very frequently to u32 from properties.
> > So we provide this helper here to avoid using fdt_getprop 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>
> > ---
> >   plat/drivers/include/ofw/fdt.h | 59
> +++++++++++++++++++++++++++++++++
> >   plat/drivers/ofw/fdt.c         | 60
> ++++++++++++++++++++++++++++++++++
> >   2 files changed, 119 insertions(+)
> >   create mode 100644 plat/drivers/include/ofw/fdt.h
> >   create mode 100644 plat/drivers/ofw/fdt.c
> >
> > diff --git a/plat/drivers/include/ofw/fdt.h
> > b/plat/drivers/include/ofw/fdt.h new file mode 100644 index
> > 0000000..c202671
> > --- /dev/null
> > +++ b/plat/drivers/include/ofw/fdt.h
> > @@ -0,0 +1,59 @@
> > +/* 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.
> > + */
> > +#ifndef _PLAT_DRIVER_OFW_FDT_H
> > +#define _PLAT_DRIVER_OFW_FDT_H
> > +
>
> Does this helper function belong plat/driver/ofw or lib/fdt? Was there a
> reason why we added it to plat/driver/ofw because in the last patch series
> discussion v4 [1] we had decided on adding to lib/fdt with an entry into
> exportsyms.uk.

Ok, I will move this API under lib/fdt.

> > +/**
> > + * 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);
> > +
> > +#endif
> > diff --git a/plat/drivers/ofw/fdt.c b/plat/drivers/ofw/fdt.c new file
> > mode 100644 index 0000000..e23b7a3
> > --- /dev/null
> > +++ b/plat/drivers/ofw/fdt.c
> > @@ -0,0 +1,60 @@
> > +/* 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>
> > +
>
> need uk/assert.h?
Yeah, should be added.

> > +#include <uk/print.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;
> > +
> > +UK_ASSERT(out);
> > +
> > +prop = fdt_get_property_namelen(fdt, offset, name, strlen(name),
> > +&prop_len);
> > +if (!prop)
> > +return prop_len;
> > +
>
> Why should this be >= sizeof(fdt32_t) instead of == sizeof(fdt_32t)?
Maybe this check should be removed as the prop_len could not be less than 32 
when it is nonnegative.
So I will remove this.

> > +if (prop_len >= (int)sizeof(fdt32_t)) {
> > +*out = fdt32_to_cpu(*(fdt32_t *)prop->data);
> > +return 0;
> > +}
> > +
> > +return -FDT_ERR_NOTFOUND;
> > +}
> >
>
Thanks
Jianyong wu
> [1]
> https://lists.xenproject.org/archives/html/minios-devel/2019-
> 04/msg00128.html
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