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

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

Hi,

> -----Original Message-----
> From: Jianyong Wu (Arm Technology China)
> Sent: Thursday, November 15, 2018 11:37 AM
> To: Julien Grall <julien.grall@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,
> 
> > -----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.
> 
> >
> > > + 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.

But, how to check reg in uk_dtb_read_term? I have an idea that check *(reg + 
index*(naddr+nsize) *4)
Does that make sence?
> 
> >
> > > + 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.
> 
> Bests
> Jianyong wu
> 
> > Cheers,
> >
> > [1]
> > https://github.com/devicetree-org/devicetree-
> > specification/releases/tag/v0.2
> >
> > --
> > Julien Grall
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel