Re: [Minios-devel] [UNIKRAFT RFC PATCH] Implement PL031 RTC library for Arm

["Jianyong Wu (Arm Technology China)" <Jianyong.Wu@xxxxxxx>]

Hi,

> -----Original Message-----
> From: Jianyong Wu (Arm Technology China)
> Sent: Thursday, November 15, 2018 1:37 PM
> To: Julien Grall <julien.grall@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] Implement PL031 RTC
> library for Arm
> 
> Hi,
> 
> > -----Original Message-----
> > From: Julien Grall <julien.grall@xxxxxxx>
> > Sent: Monday, November 12, 2018 6:37 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] Implement PL031 RTC
> > library for Arm
> >
> >
> >
> > On 11/11/18 1:59 AM, Jianyong Wu (Arm Technology China) wrote:
> > > Hi,
> >
> > Hi,
> >
> > >>> +void rtc_clear_intr(void)
> > >>> +{
> > >>> +       ioreg_write32(RTC_REG(RTC_REG_ICR), 1); }
> > >>> +
> > >>> +int _dtb_init_rtc(void *dtb)
> > >>> +{
> > >>> +       uint32_t idx;
> > >>> +       int fdt_rtc, naddr, nsize, size, prop_len, prop_min_len;
> > >>> +       const uint64_t *regs;
> > >>> +
> > >>> +       uk_printd(DLVL_INFO, "Probing RTC...\n");
> > >>> +       fdt_rtc = uk_dtb_find_device(dtb, rtc_device_list,
> > >>> +               sizeof(rtc_device_list));
> > >>
> > >> I would not assume the RTC is always present.
> > >
> > > The check is inside uk_dtb_read_region.
> >
> > Your check is an ASSERT(...). This means that if an RTC is not present
> > then you will get Unikraft crashed in debug build.
> >
> > On non-debug build, it will just continue silently.
> >
> > In both case, I don't think this is the correct behavior. If the RTC
> > is a mandatory device, then it should be an error/crash in appropriate
> > place (from uk_dtb_read_region it is not very obvious what failed).
> >
> > If the RTC is not mandatory, then you should return here and not
> > configured the RTC.
> >
> 
> Ok, I will check fdt_rtc and give a crash if there is no that device.
> 
> > >>
> > >>> +       naddr = uk_dtb_read_region(dtb, fdt_rtc, &nsize, &regs);
> > >>> +       rtc_base_addr = uk_dtb_read_term(regs, 0, naddr, nsize, &size);
> > >>
> > >> Similarly here, I would not assume uk_dtb_read_term() will always
> > succeed.
> > >> It would make the code more safe if the DT passed is wrong.
> > >
> > >
> > > Check has been done in this function.
> >
> > I am afraid this is not the case. If you look at your function there
> > are no check whether the region N you are trying to access is in the device-
> tree.
> >
> > There are an ASSERT(...) in uk_dtb_read_region but that's only make
> > sure "regs" describes at least one region.
> >
> Ok, I will check rtc_base_addr.
> 
Sorry rtc_base_addr is difficult to check. I will check regs in 
uk_dtb_read_term() as I say in fdt part of mail list.
> > >>
> > >>> +
> > >>> +       /* Record the boot seconds */
> > >>> +       rtc_boot_seconds = rtc_read();
> > >>> +
> > >>> +       uk_printd(DLVL_INFO, "Found RTC on: %p\n", rtc_base_addr);
> > >>
> > >> I am slightly surprised this compile in Unikraft. This function is
> > >> meant to return an integer but I don't see any return here.
> > >
> > > Which function? Rtc_read()? Check it again.
> >
> > Please tone down your writing if you want people to continue reviewing
> > your code.
> >
> > In that particular case, my comment was at the end of _dtb_init_rtc.
> > The prototype of the function is:
> >
> > int _dtb_init_rtc(void *dtb)
> >
> > That function return an int but I can't see a 'return X' in the code.
> > It seems Unikraft does not use -Werror flags, which means this kind of
> > error would be only a warning. Yet the return value will be unknown.
> >
> > Regarding -Werror, I think Unikraft community should consider use it
> > as a lot of warning can be harmful in long run.
> 
> Oh, I'm sorry. It's not my intention to offend you. Words does not convey the
> exact emotion of the talker.
> Anyway I will watch out of my words. I am really happy to have my patch
> reviewed by someone like you. Thanks.
> 
>  Yeah, there is really my fault to miss that return statement. I will add it.
> 
> Bests
> Jianyong wu
> >
> > Cheers,
> >
> > --
> > Julien Grall
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel