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

[Mark Rutland <mark.rutland@xxxxxxx>]

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.

Thanks,
Mark.

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel