[Minios-devel] Fwd: Re: Fw: [UNIKRAFT PATCH 5/8] lib/fdt: Introduce a fdt_get_address helper

[Jia He <jiakernel2@xxxxxxxxx>]

Hi Mark
For some reason, I will follow up Wei's patch series.

And I can't use outlook to discuss the thread(Due to the base64 encoding 
by Outlook).
So I forwarded previous mail to my gmail mailbox firstly.
Please see my comments below:

On 2019/1/24 9:35, Justin He (Arm Technology China) wrote:
> From: Wei Chen (Arm Technology China)
> Sent: Friday, December 14, 2018 3:37 PM
> To: Mark Rutland
> Cc: minios-devel@xxxxxxxxxxxxxxxxxxxx; simon.kuenzer@xxxxxxxxx; 
> florian.schmidt@xxxxxxxxx; yuri.volchkov@xxxxxxxxx; Sharan.Santhanam@xxxxxxxxx; 
> Felipe.Huici@xxxxxxxxx; Kaly Xin (Arm Technology China); nd; Jianyong Wu (Arm 
> Technology China); Justin He  (Arm Technology China)
> Subject: RE: [Minios-devel] [UNIKRAFT PATCH 5/8] lib/fdt: Introduce a 
> fdt_get_address helper
>
>
> Hi Mark,
>
> > -----Original Message-----
> > From: Mark Rutland <mark.rutland@xxxxxxx>
> > Sent: 2018年12月13日 18:00
> > To: Wei Chen (Arm Technology China) <Wei.Chen@xxxxxxx>
> > Cc: minios-devel@xxxxxxxxxxxxxxxxxxxx; simon.kuenzer@xxxxxxxxx;
> > florian.schmidt@xxxxxxxxx; yuri.volchkov@xxxxxxxxx; Sharan.Santhanam@xxxxxxxxx;
> > Felipe.Huici@xxxxxxxxx; Kaly Xin (Arm Technology China) <Kaly.Xin@xxxxxxx>; nd
> > <nd@xxxxxxx>; Jianyong Wu (Arm Technology China) <Jianyong.Wu@xxxxxxx>; Justin
> > He (Arm Technology China) <Justin.He@xxxxxxx>
> > Subject: Re: [Minios-devel] [UNIKRAFT PATCH 5/8] lib/fdt: Introduce a
> > fdt_get_address helper
> >
> > On Thu, Dec 13, 2018 at 09:33:40AM +0000, Wei Chen (Arm Technology China)
> > wrote:
> > > Hi Mark,
> > >
> > > > -----Original Message-----
> > > > From: Mark Rutland <mark.rutland@xxxxxxx>
> > > > Sent: 2018年12月13日 17:21
> > > > To: Wei Chen (Arm Technology China) <Wei.Chen@xxxxxxx>
> > > > Cc: minios-devel@xxxxxxxxxxxxxxxxxxxx; simon.kuenzer@xxxxxxxxx;
> > > > florian.schmidt@xxxxxxxxx; yuri.volchkov@xxxxxxxxx;
> > Sharan.Santhanam@xxxxxxxxx;
> > > > Felipe.Huici@xxxxxxxxx; Kaly Xin (Arm Technology China) <Kaly.Xin@xxxxxxx>;
> > nd
> > > > <nd@xxxxxxx>; Jianyong Wu (Arm Technology China) <Jianyong.Wu@xxxxxxx>;
> > Justin
> > > > He (Arm Technology China) <Justin.He@xxxxxxx>
> > > > Subject: Re: [Minios-devel] [UNIKRAFT PATCH 5/8] lib/fdt: Introduce a
> > > > fdt_get_address helper
> > > >
> > > > On Thu, Dec 13, 2018 at 09:18:17AM +0000, Wei Chen wrote:
> > > > > This helper will be used very frequently for device libraries
> > > > > to parse their addresses. Introduce this helper to avoid using
> > > > > fdt_address_cells and fdt_size_cells everywhere.
> > > > >
> > > > > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > > > > Signed-off-by: Jianyong Wu <jianyong.wu@xxxxxxx>
> > > > > ---
> > > > >    lib/fdt/exportsyms.uk    |  1 +
> > > > >    lib/fdt/fdt_addresses.c  | 50 ++++++++++++++++++++++++++++++++++++++++
> > > > >    lib/fdt/include/libfdt.h | 18 +++++++++++++++
> > > > >    3 files changed, 69 insertions(+)
> > > > >
> > > > > diff --git a/lib/fdt/exportsyms.uk b/lib/fdt/exportsyms.uk
> > > > > index b11df90..c893caf 100644
> > > > > --- a/lib/fdt/exportsyms.uk
> > > > > +++ b/lib/fdt/exportsyms.uk
> > > > > @@ -62,3 +62,4 @@ fdt_resize
> > > > >    fdt_overlay_apply
> > > > >    fdt_getprop_u32_by_offset
> > > > >    fdt_interrupt_cells
> > > > > +fdt_get_address
> > > > > diff --git a/lib/fdt/fdt_addresses.c b/lib/fdt/fdt_addresses.c
> > > > > index bccb11c..b186fc0 100644
> > > > > --- a/lib/fdt/fdt_addresses.c
> > > > > +++ b/lib/fdt/fdt_addresses.c
> > > > > @@ -64,3 +64,53 @@ int fdt_size_cells(const void *fdt, int nodeoffset)
> > > > >    {
> > > > >            return fdt_get_cells(fdt, "#size-cells", nodeoffset);
> > > > >    }
> > > > > +
> > > > > +static uint64_t fdt_reg_read_number(const fdt32_t *regs, uint32_t size)
> > > > > +{
> > > > > +       uint64_t number = 0;
> > > > > +
> > > > > +       if (size >= 3 || size <= 0)
> > > > > +               return -FDT_ERR_BADNCELLS;
> > > > > +
> > > > > +       for(uint32_t i = 0; i < size; i++) {
> > > > > +               number <<= 32;
> > > > > +               number |= fdt32_to_cpu(*regs);
> > > > > +               regs++;
> > > > > +       }
> > > > > +
> > > > > +       return number;
> > > > > +}
> > > > > +
> > > > > +int fdt_get_address(const void *fdt, int nodeoffset, int index,
> > > > > +                       uint64_t *addr, uint64_t *size)
> > > > > +{
> > > > > +       int len, prop_addr, prop_size;
> > > > > +       int naddr, nsize, term_size;
> > > > > +       const void *regs;
> > > > > +
> > > > > +       naddr = fdt_address_cells(fdt, nodeoffset);
> > > > > +       if (naddr < 0 || naddr >= FDT_MAX_NCELLS)
> > > > > +               return -FDT_ERR_BADNCELLS;
> > > > > +
> > > > > +       nsize = fdt_size_cells(fdt, nodeoffset);
> > > > > +       if (nsize < 0 || nsize >= FDT_MAX_NCELLS)
> > > > > +               return -FDT_ERR_BADNCELLS;
> > > > > +
> > > > > +       /* Get reg content */
> > > > > +       regs = fdt_getprop(fdt, nodeoffset, "reg", &len);
> > > > > +       if (regs == NULL)
> > > > > +               return -FDT_ERR_NOTFOUND;
> > > > > +
> > > > > +       term_size = (int)sizeof(fdt32_t) * (nsize + naddr);
> > > > > +       prop_addr = term_size * index;
> > > > > +       prop_size = prop_addr + (int)sizeof(fdt32_t) * naddr;
> > > > > +
> > > > > +       /* The reg content must cover the reg term[index] at least */
> > > > > +       if (len < (prop_addr + term_size))
> > > > > +               return -FDT_ERR_NOSPACE;
> > > > > +
> > > > > +       *addr = fdt_reg_read_number(regs + prop_addr, naddr);
> > > > > +       *size = fdt_reg_read_number(regs + prop_size, nsize);
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > If this is intended to extract addresses, shouldn't it take ranges
> > > > properties into account?
> > > Ahh, my intention is to fetch the register range, I think I'd better
> > > to rename this helper to fdt_get_reg_range.
> > Any register range is potentially subject to a ranges perty, and ranges
> > should be taken into account.
> >
> > I see subsequent patches using fdt_get_address() to get the GIC MMIO
> > register ranges, and those do not take any ranges properties into
> > account, even though they should per the devicetree spec.
IIUC, current unikraft on arm64 can only run as a guest in qemu with
-machine virt,gic-version=2 . All the mmu design is based on that 
assumption.
The default dtb created by qemu about interrupt-controller is as follows:
    intc@8000000 {
        phandle = < 0x8001 >;
        reg = < 0x00 0x8000000 0x00 0x10000 0x00 0x8010000 0x00 0x10000 >;
        compatible = "arm,cortex-a15-gic";
        ranges;
        #size-cells = < 0x02 >;
        #address-cells = < 0x02 >;
        interrupt-controller;
        #interrupt-cells = < 0x03 >;

        v2m@8020000 {
            phandle = < 0x8002 >;
            reg = < 0x00 0x8020000 0x00 0x1000 >;
            msi-controller;
            compatible = "arm,gic-v2m-frame";
        };
    };
So ranges is NULL in this case, so we don't need to concern the ranges 
property?
Thanks for more comments

Cheers,
Justin (Jia He), Arm

> Did you mean, I should consider to translate this address with the "range"
> property? Just like fdt_translate_address(address of fdt_get_address) to
> check whether the address is in the range or does it need to remap to
> parent bus address space?
>
> > Thanks,
> > Mark.
> 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