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

["Wei Chen (Arm Technology China)" <Wei.Chen@xxxxxxx>]

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

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.
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel