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 7:16 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 11:01 AM, Jianyong Wu (Arm Technology China) wrote:
> > Hi,
> 
> 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.
> 
> I never asked for Unikraft to be as good as Linux. I requested Unikraft to
> follow the Arm Arm.
> 
> Maybe you will save time today by making your code "just work (TM)"
> , but from my experience you are going to put yourself in a sink hole quickly.
> Fixing Arm Arm issues are not fun at all, some of them may require massive
> rewrite.
> 
> In that particular case, I suggested an API change that would avoid rework in
> the future. As I said earlier on, I don't care whether you implement the
> function correctly now or later on. I don't think this was unreasonable, but 
> it
> is your decision to not address them.
> 

Ok, I will merge the two functions,.

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