Re: [Minios-devel] [UNIKRAFT RFC PATCH] plat/common:Add helper to simplify accessing fdt for arm

[Julien Grall <julien.grall@xxxxxxx>]

On 11/15/18 3:36 AM, Jianyong Wu (Arm Technology China) wrote:
> Hi,
Hi Jianyong,


> > -----Original Message-----
> > From: Julien Grall <julien.grall@xxxxxxx>
> > Sent: Monday, November 12, 2018 7:26 PM
> > To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@xxxxxxx>; minios-
> > devel@xxxxxxxxxxxxxxxxxxxx; simon.kuenzer@xxxxxxxxx
> > Cc: Kaly Xin (Arm Technology China) <Kaly.Xin@xxxxxxx>; nd
> > <nd@xxxxxxx>; Wei Chen (Arm Technology China) <Wei.Chen@xxxxxxx>
> > Subject: Re: [Minios-devel] [UNIKRAFT RFC PATCH] plat/common:Add helper
> > to simplify accessing fdt for arm
> >
> > Hi,
> >
> > On 11/9/18 8:54 AM, Jianyong Wu wrote:
> > > Wrap some reading ops on dtb like reading property and address
> > > conversion op to simplify acquiring device address for arm.
> > >
> > > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > > Signed-off-by: Jianyong Wu <jianyong.wu@xxxxxxx>
> > > ---
> > >    plat/common/arm/fdt.c         | 113
> > ++++++++++++++++++++++++++++++++++++++++++
> > >    plat/common/include/arm/fdt.h |  52 +++++++++++++++++++
> > >    plat/kvm/Makefile.uk          |   1 +
> > >    3 files changed, 166 insertions(+)
> > >    create mode 100644 plat/common/arm/fdt.c
> > >    create mode 100644 plat/common/include/arm/fdt.h
> > >
> > > diff --git a/plat/common/arm/fdt.c b/plat/common/arm/fdt.c new file
> > > mode 100644 index 0000000..1087063
> > > --- /dev/null
> > > +++ b/plat/common/arm/fdt.c
> > > @@ -0,0 +1,113 @@
> > > +/* SPDX-License-Identifier: ISC */
> > > +/*
> > > + * Authors: Wei Chen <Wei.Chen@xxxxxxx>
> > > + * Authors: Jianyong Wu <Jianyong.Wu@xxxxxxx>
> > > + *
> > > + * Copyright (c) 2018 Arm Ltd.
> > > + *
> > > + * Permission to use, copy, modify, and/or distribute this software
> > > + * for any purpose with or without fee is hereby granted, provided
> > > + * that the above copyright notice and this permission notice appear
> > > + * in all copies.
> > > + *
> > > + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL
> > > + * WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL
> > IMPLIED
> > > + * WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL
> > THE
> > > + * AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, INDIRECT, OR
> > > + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER
> > RESULTING FROM
> > > +LOSS
> > > + * OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT,
> > > + * NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
> > > + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> > > + */
> > > +#include <libfdt.h>
> > > +#include <kvm/console.h>
> > > +#include <uk/assert.h>
> > > +#include <kvm-arm/mm.h>
> > > +#include <arm/cpu.h>
> > > +#include <arm/fdt.h>
> > > +#include <uk/arch/limits.h>
> > > +
> > > +void *_libkvmplat_dtb;
> > > +void *_libkvmplat_pagetable;
> > > +void *_libkvmplat_heap_start;
> > > +void *_libkvmplat_stack_top;
> > > +void *_libkvmplat_mem_end;
> > Can you explain why all the variables but _libkvmplat_dtb belongs to that file?
> Ok, I will move those variables to another file.
> > > +
> > > +/*
> > > + * uk_dtb_find_device find device offset in dtb by
> > > + * searching device list
> > > + */
> > > +int uk_dtb_find_device(void *dtb, const char *device_list[],
> > AFAICT, the dtb is always to be _libkvmplat_dtb. So how about remove the
> > parameter dtb and directly use _libkvmplat_dtb in the code?
> Dtb is short and is spread everywhere, so just keep it unchanged.
What do you mean by spread everywhere? This is a new interface. With the 
current solution you would always have to write myfoo(_libkmvplat_dtb,...).
I would make more concise to do myfoo(....).

> > > +       uint32_t size)
> > > +{
> > > +        uint32_t idx;
> > > +        int device = -1;
> > > +
> > > +        for (idx = 0;
> > > +                idx < size / sizeof(device_list[0]);
> > > +                idx++) {
> > > +                device = fdt_node_offset_by_compatible(dtb, -1,
> > > +                                        device_list[idx]);
> > > +                if (device >= 0)
> > > +                        break;
> > > +        }
> > > +
> > > +        return device;
> > > +}
> > > +
> > > +/*
> > > + * uk_dtb_read_region read cell size of device address
> > > + * and its length in dtb by input device offset.
> > > + */
> > > +uint32_t uk_dtb_read_region(void *dtb, int device,
> > Same question here.
> >
> > > +       uint32_t *nsize, fdt32_t **regs)
> > > +{
> > > +        int  prop_len, prop_min_len;
> > > +       uint32_t naddr;
> > The indentation of the code looks strange. You seem to mix hard tab and soft
> > tab. Please use what Unikraft usually use.
> Ok, I will fix it.
>
> > > +
> > > +       UK_ASSERT(device != -1);
> > > +        naddr = fdt_address_cells(dtb, device);
> > > +        UK_ASSERT(naddr < FDT_MAX_NCELLS);
> > > +
> > > +        *nsize = fdt_size_cells(dtb, device);
> > > +        UK_ASSERT(*nsize < FDT_MAX_NCELLS);
> > > +
> > > +        *regs = fdt_getprop(dtb, device, "reg", &prop_len);
> > > +        prop_min_len = (int)sizeof(fdt32_t) * (naddr + *nsize);
> > > +        UK_ASSERT(*regs != NULL && prop_len >= prop_min_len);
> > This assert is not very useful for "regs" property describing more than
> > 1 regions. I think it would make sense to move the check in the
> > uk_dtb_read_term to check if the region requested by the caller is correct.
> Ok, I will check reg in uk_dtb_read_term.
>
> > > +       return naddr;
> > > +}
> > > +
> > > +/*
> > > + * this read reg part in device property and output
> > > + * address and size of term specified by index.
> > > + */
> > > +uint64_t uk_dtb_read_term(fdt32_t *regs, uint32_t index,
> > > +       uint32_t naddr, uint32_t nsize, uint64_t *size)
> > > +{
> > > +       uint64_t addr, term_size;
> > > +
> > > +       term_size = nsize + naddr;
> > > +       index = index * term_size;
> > > +       addr = uk_dtb_read_number(regs + index, naddr);
> > > +
> > > +       *size = uk_dtb_read_number(regs + index + naddr, nsize);
> > Sorry, I realized I forgot to mention about bus earlier on. A device may be
> > under a bus and therefore the "regs" would need to be translated to the
> > parent bus. You can have a look at 2.3.8 in [1].
> >
> > At the moment, the code is probably OK with the current usage, so this could
> > be implemented when it is necessary. Although, this may require some
> > changes in the interface.
> >
> > At the moment, the code is probably OK with the current usage. Although, I
> > would keep that in mind as the interface is likely to be changed if the
> > interface is part of a stable ABI.
> It's really large effort to do this check, I will keep in mind and add it if 
> necessary.
How about at least making the interface correct from now? Is this going 
to be a concern to modify the function API afterwards?
For instance, you would likely need to rework all the callers.

Cheers,

--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel