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: Thursday, November 15, 2018 6:20 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/15/18 10:11 AM, Jianyong Wu (Arm Technology China) wrote:
> >> -----Original Message-----
> >> From: Julien Grall <julien.grall@xxxxxxx>
> >> Sent: Thursday, November 15, 2018 5:52 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
> >>
> >>
> >>
> >> On 11/15/18 6:09 AM, Jianyong Wu (Arm Technology China) wrote:
> >>> Hi,
> >>
> >> Hi,
> >>
> >>
> >>>>>> +
> >>>>>> +      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.
> >>>
> >>> But, how to check reg in uk_dtb_read_term? I have an idea that check
> >>> *(reg + index*(naddr+nsize) *4) Does that make sence?
> >>
> >> You want to make sure you are not going to read past the size of the
> >> property. So ((index + 1) * (naddr + nsize) * 4) < size should be fine.
> >
> > The "size" in this function is not the size of all reg, it is just the 
> > length of one
> term like distributor in gic.
> > We not get the all regs cell size in this function.
> 
> Oh sorry, I misread the code sorry.
> 
> We definitely want some safety here, so it is probably a call to rework the
> interface.
> 
> I remembered you dismissed in a early revision, but I think it is worthwhile 
> to
> reconsider the following interface:
> 
> uk_dtb_read_reg(int node, unsigned int index, &size);
> 
> You can then add all the safety and also avoid to have to add more
> parameters to this function.
> 
> What do you think?
>
I'm very appreciate it. if I have enough time I can do all of those. But we 
pending on those for too long a time,
Gicv2 based on fdt, wei's works pend for gicv2. The process on arm part in 
unikraft is largely behind of x86. We only have 2 labors
in unikraft at arm side. If we write every line  of code the best as linux 
kernel have done, we can not move on.
We will do our bests to keep our work safe, but first we must merge our code to 
upstream.
We need your help, the support and understanding. Thank you.

Bests
Jianyong wu
 
> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel