Re: [Minios-devel] [UNIKRAFT RFC PATCH] plat/common:Add helper to simplify accessing fdt for arm

[Julien Grall <julien.grall@xxxxxxx>]

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

--
Julien Grall

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