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: 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.

> 
> > +   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