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

[Julien Grall <julien.grall@xxxxxxx>]

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?
> +
> +/*
> + * 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?
> +       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.
> +
> +       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.
> +       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.
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